Skip to content

Commit

Permalink
fix(backup): Don't assign is_unclaimed in global import
Browse files Browse the repository at this point in the history
Importing a user in `ImportScope.Global` should never result in them
being marked `is_unclaimed`.

Issue: getsentry/team-ospo@203
  • Loading branch information
azaslavsky committed Oct 11, 2023
1 parent 806b10c commit 4da3918
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 14 deletions.
28 changes: 16 additions & 12 deletions src/sentry/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
3 changes: 2 additions & 1 deletion tests/sentry/backup/test_imports.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion tests/sentry/backup/test_snapshots.py
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down

0 comments on commit 4da3918

Please sign in to comment.