From 4da3918fb99d1177eb7e99afea66227540f83b4a Mon Sep 17 00:00:00 2001 From: Alex Zaslavsky Date: Tue, 3 Oct 2023 10:58:44 -0700 Subject: [PATCH] fix(backup): Don't assign `is_unclaimed` in global import Importing a user in `ImportScope.Global` should never result in them being marked `is_unclaimed`. Issue: getsentry/team-ospo@203 --- src/sentry/models/user.py | 28 +++++++++++++++------------ tests/sentry/backup/test_imports.py | 3 ++- tests/sentry/backup/test_snapshots.py | 4 +++- 3 files changed, 21 insertions(+), 14 deletions(-) diff --git a/src/sentry/models/user.py b/src/sentry/models/user.py index 5ab1d3fe4637b6..bd00be65016421 100644 --- a/src/sentry/models/user.py +++ b/src/sentry/models/user.py @@ -403,23 +403,27 @@ def normalize_before_relocation_import( if old_pk is None: return None - # New users are always unclaimed. - self.is_unclaimed = True - - # Give the user a cryptographically secure random password. The purpose here is to have a - # password that NO ONE knows - the only way to log into this account is to use the "claim - # your account" flow to create a new password (or to click "lost password" and end up there - # anyway), at which point we'll detect the user's `is_unclaimed`` status and prompt them to - # change their `username` as well. - self.set_password( - "".join(secrets.choice(RANDOM_PASSWORD_ALPHABET) for _ in range(RANDOM_PASSWORD_LENGTH)) - ) - if scope not in {ImportScope.Config, ImportScope.Global}: self.is_staff = False self.is_superuser = False self.is_managed = False + # No need to mark users newly "unclaimed" when doing a global backup/restore. + if scope != ImportScope.Global or self.is_unclaimed: + # New users are marked unclaimed. + self.is_unclaimed = True + + # Give the user a cryptographically secure random password. The purpose here is to have + # a password that NO ONE knows - the only way to log into this account is to use the + # "claim your account" flow to create a new password (or to click "lost password" and + # end up there anyway), at which point we'll detect the user's `is_unclaimed` status and + # prompt them to change their `username` as well. + self.set_password( + "".join( + secrets.choice(RANDOM_PASSWORD_ALPHABET) for _ in range(RANDOM_PASSWORD_LENGTH) + ) + ) + return old_pk def write_relocation_import( diff --git a/tests/sentry/backup/test_imports.py b/tests/sentry/backup/test_imports.py index 60f92b6ef9b9d0..ad82ed7cce1c1a 100644 --- a/tests/sentry/backup/test_imports.py +++ b/tests/sentry/backup/test_imports.py @@ -226,7 +226,8 @@ def test_users_unsanitized_in_global_scope(self): import_in_global_scope(tmp_file, printer=NOOP_PRINTER) assert User.objects.count() == 4 - assert User.objects.filter(is_unclaimed=True).count() == 4 + # We don't mark `Global`ly imported `User`s unclaimed. + assert User.objects.filter(is_unclaimed=True).count() == 0 assert User.objects.filter(is_managed=True).count() == 1 assert User.objects.filter(is_staff=True).count() == 2 assert User.objects.filter(is_superuser=True).count() == 2 diff --git a/tests/sentry/backup/test_snapshots.py b/tests/sentry/backup/test_snapshots.py index 910a558ed27d33..6eec9681d79ccd 100644 --- a/tests/sentry/backup/test_snapshots.py +++ b/tests/sentry/backup/test_snapshots.py @@ -87,7 +87,9 @@ def test_auto_suffix_username_and_organization_slug(tmp_path): ) -# Apps are often owned by a fake "proxy_user" with an autogenerated name and no email. This empty email is represented as such in the database, with an empty string as the entry in the `Email` table. This test ensures that this relationship is preserved through an import/export cycle. +# Apps are often owned by a fake "proxy_user" with an autogenerated name and no email. This empty +# email is represented as such in the database, with an empty string as the entry in the `Email` +# table. This test ensures that this relationship is preserved through an import/export cycle. @django_db_all(transaction=True) def test_app_user_with_empty_email(tmp_path): import_export_from_fixture_then_validate(