Browse Source

fix(backup): Handle multiple emails per user (#60404)

Issue: getsentry/self-hosted#2571
Alex Zaslavsky 1 year ago
parent
commit
573069dcf2

+ 8 - 0
fixtures/backup/user-with-maximum-privileges.json

@@ -24,6 +24,14 @@
             "avatar_url": null
         }
     },
+    {
+        "model": "sentry.email",
+        "pk": 1,
+        "fields": {
+            "email": "maximum@example.com",
+            "date_added": "2023-06-22T22:59:54.521Z"
+        }
+    },
     {
         "model": "sentry.authenticator",
         "pk": 1,

+ 8 - 0
fixtures/backup/user-with-minimum-privileges.json

@@ -24,6 +24,14 @@
             "avatar_url": null
         }
     },
+    {
+        "model": "sentry.email",
+        "pk": 2,
+        "fields": {
+            "email": "testing@example.com",
+            "date_added": "2023-06-22T22:59:54.521Z"
+        }
+    },
     {
         "model": "sentry.authenticator",
         "pk": 2,

+ 8 - 0
fixtures/backup/user-with-roles-no-superadmin.json

@@ -24,6 +24,14 @@
             "avatar_url": null
         }
     },
+    {
+        "model": "sentry.email",
+        "pk": 3,
+        "fields": {
+            "email": "testing@example.com",
+            "date_added": "2023-06-22T22:59:54.521Z"
+        }
+    },
     {
         "model": "sentry.authenticator",
         "pk": 3,

+ 8 - 0
fixtures/backup/user-with-superadmin-no-roles.json

@@ -24,6 +24,14 @@
             "avatar_url": null
         }
     },
+    {
+        "model": "sentry.email",
+        "pk": 4,
+        "fields": {
+            "email": "testing@example.com",
+            "date_added": "2023-06-22T22:59:54.521Z"
+        }
+    },
     {
         "model": "sentry.authenticator",
         "pk": 4,

+ 13 - 7
src/sentry/models/useremail.py

@@ -120,13 +120,19 @@ class UserEmail(ControlOutboxProducingModel):
     def write_relocation_import(
         self, _s: ImportScope, _f: ImportFlags
     ) -> Optional[Tuple[int, ImportKind]]:
-        # The `UserEmail` was automatically generated `post_save()`. We just need to update it with
-        # the data being imported. Note that if we've reached this point, we cannot be merging into
-        # an existing user, and are instead modifying the just-created `UserEmail` for a new one.
-        useremail = self.__class__.objects.get(user=self.user, email=self.email)
-        for f in self._meta.fields:
-            if f.name not in ["id", "pk"]:
-                setattr(useremail, f.name, getattr(self, f.name))
+        # The `UserEmail` was automatically generated `post_save()`, but only if it was the user's
+        # primary email. We just need to update it with the data being imported. Note that if we've
+        # reached this point, we cannot be merging into an existing user, and are instead modifying
+        # the just-created `UserEmail` for a new one.
+        try:
+            useremail = self.__class__.objects.get(user=self.user, email=self.email)
+            for f in self._meta.fields:
+                if f.name not in ["id", "pk"]:
+                    setattr(useremail, f.name, getattr(self, f.name))
+        except self.__class__.DoesNotExist:
+            # This is a non-primary email, so was not auto-created - go ahead and add it in.
+            useremail = self
+
         useremail.save()
 
         # If we've entered this method at all, we can be sure that the `UserEmail` was created as

+ 104 - 0
tests/sentry/backup/test_imports.py

@@ -514,6 +514,110 @@ class SanitizationTests(ImportTestCase):
                 assert err.value.context.get_kind() == RpcImportErrorKind.ValidationError
                 assert err.value.context.on.model == "sentry.userip"
 
+    # Regression test for getsentry/self-hosted#2571.
+    def test_good_multiple_useremails_per_user_in_user_scope(self):
+        with tempfile.TemporaryDirectory() as tmp_dir:
+            tmp_path = Path(tmp_dir).joinpath(f"{self._testMethodName}.json")
+            with open(tmp_path, "w+") as tmp_file:
+                models = self.json_of_exhaustive_user_with_minimum_privileges()
+
+                # Add two copies (1 verified, 1 not) of the same `UserEmail` - so the user now has 3
+                # `UserEmail` models, the latter of which have no corresponding `Email` entry.
+                models.append(
+                    {
+                        "model": "sentry.useremail",
+                        "pk": 100,
+                        "fields": {
+                            "user": 2,
+                            "email": "second@example.com",
+                            "validation_hash": "7jvwev0oc8sFyEyEwfvDAwxidtGzpAov",
+                            "date_hash_added": "2023-06-22T22:59:56.521Z",
+                            "is_verified": True,
+                        },
+                    }
+                )
+                models.append(
+                    {
+                        "model": "sentry.useremail",
+                        "pk": 101,
+                        "fields": {
+                            "user": 2,
+                            "email": "third@example.com",
+                            "validation_hash": "",
+                            "date_hash_added": "2023-06-22T22:59:57.521Z",
+                            "is_verified": False,
+                        },
+                    }
+                )
+
+                json.dump(self.sort_in_memory_json(models), tmp_file)
+
+            with open(tmp_path, "rb") as tmp_file:
+                import_in_user_scope(tmp_file, printer=NOOP_PRINTER)
+
+        with assume_test_silo_mode(SiloMode.CONTROL):
+            assert UserEmail.objects.count() == 3
+            assert UserEmail.objects.values("user").distinct().count() == 1
+            assert UserEmail.objects.filter(email="testing@example.com").exists()
+            assert UserEmail.objects.filter(email="second@example.com").exists()
+            assert UserEmail.objects.filter(email="third@example.com").exists()
+
+            # Validations are scrubbed and regenerated in non-global scopes.
+            assert UserEmail.objects.filter(validation_hash="").count() == 0
+            assert UserEmail.objects.filter(is_verified=True).count() == 0
+
+    # Regression test for getsentry/self-hosted#2571.
+    def test_good_multiple_useremails_per_user_in_global_scope(self):
+        with tempfile.TemporaryDirectory() as tmp_dir:
+            tmp_path = Path(tmp_dir).joinpath(f"{self._testMethodName}.json")
+            with open(tmp_path, "w+") as tmp_file:
+                models = self.json_of_exhaustive_user_with_minimum_privileges()
+
+                # Add two copies (1 verified, 1 not) of the same `UserEmail` - so the user now has 3
+                # `UserEmail` models, the latter of which have no corresponding `Email` entry.
+                models.append(
+                    {
+                        "model": "sentry.useremail",
+                        "pk": 100,
+                        "fields": {
+                            "user": 2,
+                            "email": "second@example.com",
+                            "validation_hash": "7jvwev0oc8sFyEyEwfvDAwxidtGzpAov",
+                            "date_hash_added": "2023-06-22T22:59:56.521Z",
+                            "is_verified": True,
+                        },
+                    }
+                )
+                models.append(
+                    {
+                        "model": "sentry.useremail",
+                        "pk": 101,
+                        "fields": {
+                            "user": 2,
+                            "email": "third@example.com",
+                            "validation_hash": "",
+                            "date_hash_added": "2023-06-22T22:59:57.521Z",
+                            "is_verified": False,
+                        },
+                    }
+                )
+
+                json.dump(self.sort_in_memory_json(models), tmp_file)
+
+            with open(tmp_path, "rb") as tmp_file:
+                import_in_global_scope(tmp_file, printer=NOOP_PRINTER)
+
+        with assume_test_silo_mode(SiloMode.CONTROL):
+            assert UserEmail.objects.count() == 3
+            assert UserEmail.objects.values("user").distinct().count() == 1
+            assert UserEmail.objects.filter(email="testing@example.com").exists()
+            assert UserEmail.objects.filter(email="second@example.com").exists()
+            assert UserEmail.objects.filter(email="third@example.com").exists()
+
+            # Validation hashes are not touched in the global scope.
+            assert UserEmail.objects.filter(validation_hash="").count() == 1
+            assert UserEmail.objects.filter(is_verified=True).count() == 2
+
     def test_bad_invalid_user_option(self):
         with tempfile.TemporaryDirectory() as tmp_dir:
             tmp_path = Path(tmp_dir).joinpath(f"{self._testMethodName}.json")