From d38e5641cdef24bbab739ac09f21e28787a818fa Mon Sep 17 00:00:00 2001 From: Alex Zaslavsky Date: Tue, 9 Apr 2024 12:22:47 -0700 Subject: [PATCH] feat(relocation): Improve claim user emails (#68495) We make two changes here: 1. The "claim this user" email now lists the slugs of the new relocation, rather than the one being imported from self-hosted. If there was no import collision, these are one and the same, but in cases of collision (or multiple imports) it makes the email easier to parse. 2. The page for expired lost password hashes has been given custom copy for the unclaimed user case. Now, it tells you specifically that it will resend the "claim user" email, rather than a cryptic "reset password" button. Further, it resends the email directly, rather than making you type in your password again. --- src/sentry/tasks/relocation.py | 21 +++- .../sentry/account/relocate/claimed.html | 12 ++ .../sentry/account/relocate/error.html | 14 +++ .../sentry/account/relocate/failure.html | 10 +- .../sentry/account/relocate/sent.html | 12 ++ src/sentry/web/frontend/accounts.py | 66 ++++++++++- src/sentry/web/urls.py | 5 + tests/sentry/tasks/test_relocation.py | 46 +++++++- tests/sentry/web/frontend/test_accounts.py | 110 +++++++++++++++++- 9 files changed, 278 insertions(+), 18 deletions(-) create mode 100644 src/sentry/templates/sentry/account/relocate/claimed.html create mode 100644 src/sentry/templates/sentry/account/relocate/error.html create mode 100644 src/sentry/templates/sentry/account/relocate/sent.html diff --git a/src/sentry/tasks/relocation.py b/src/sentry/tasks/relocation.py index 6709cf2a611625..8a6002475a75fe 100644 --- a/src/sentry/tasks/relocation.py +++ b/src/sentry/tasks/relocation.py @@ -1097,6 +1097,7 @@ def postprocessing(uuid: str) -> None: user_id=relocation.owner_id, role="owner", ) + # Last, but certainly not least: trigger signals, so that interested subscribers in eg: # getsentry can do whatever postprocessing they need to. If even a single one fails, we fail # the entire task. @@ -1104,8 +1105,8 @@ def postprocessing(uuid: str) -> None: if isinstance(result, Exception): raise result - # This signal nust come after the relocated signal, to ensure that the subscription and customer models - # have been appropriately set up before attempting to redeem a promo code. + # This signal must come after the relocated signal, to ensure that the subscription and + # customer models have been appropriately set up before attempting to redeem a promo code. relocation_redeem_promo_code.send_robust( sender=postprocessing, user_id=relocation.owner_id, @@ -1166,6 +1167,12 @@ def notifying_users(uuid: str) -> None: for chunk in chunks: imported_user_ids = imported_user_ids.union(set(chunk.inserted_map.values())) + imported_org_slugs: set[int] = set() + for chunk in RegionImportChunk.objects.filter( + import_uuid=str(uuid), model="sentry.organization" + ): + imported_org_slugs = imported_org_slugs.union(set(chunk.inserted_identifiers.values())) + # Do a sanity check on pk-mapping before we go and reset the passwords of random users - are # all of these usernames plausibly ones that were included in the import, based on username # prefix matching? @@ -1184,7 +1191,7 @@ def notifying_users(uuid: str) -> None: # Okay, everything seems fine - go ahead and send those emails. for user in imported_users: hash = lost_password_hash_service.get_or_create(user_id=user.id).hash - LostPasswordHash.send_relocate_account_email(user, hash, relocation.want_org_slugs) + LostPasswordHash.send_relocate_account_email(user, hash, list(imported_org_slugs)) relocation.latest_unclaimed_emails_sent_at = datetime.now(UTC) relocation.save() @@ -1223,12 +1230,18 @@ def notifying_owner(uuid: str) -> None: attempts_left, ERR_NOTIFYING_INTERNAL, ): + imported_org_slugs: set[int] = set() + for chunk in RegionImportChunk.objects.filter( + import_uuid=str(uuid), model="sentry.organization" + ): + imported_org_slugs = imported_org_slugs.union(set(chunk.inserted_identifiers.values())) + send_relocation_update_email( relocation, Relocation.EmailKind.SUCCEEDED, { "uuid": str(relocation.uuid), - "orgs": relocation.want_org_slugs, + "orgs": list(imported_org_slugs), }, ) diff --git a/src/sentry/templates/sentry/account/relocate/claimed.html b/src/sentry/templates/sentry/account/relocate/claimed.html new file mode 100644 index 00000000000000..5f074a9e517157 --- /dev/null +++ b/src/sentry/templates/sentry/account/relocate/claimed.html @@ -0,0 +1,12 @@ +{% extends "sentry/bases/auth.html" %} + +{% load crispy_forms_tags %} +{% load i18n %} + +{% block title %}{% trans "Claim Account" %} | {{ block.super }}{% endblock %} + +{% block auth_main %} +

{% trans "Claim Account" %}

+ +

This account has already been claimed - you should be able to log in as normal. You can always reset your password if you have forgotten it.

+{% endblock %} diff --git a/src/sentry/templates/sentry/account/relocate/error.html b/src/sentry/templates/sentry/account/relocate/error.html new file mode 100644 index 00000000000000..445b707b968374 --- /dev/null +++ b/src/sentry/templates/sentry/account/relocate/error.html @@ -0,0 +1,14 @@ +{% extends "sentry/bases/auth.html" %} + +{% load crispy_forms_tags %} +{% load i18n %} + +{% block title %}{% trans "Claim Account" %} | {{ block.super }}{% endblock %} + +{% block auth_main %} +

{% trans "Claim Account" %}

+ +

{% blocktrans %}We were unable to locate this account.{% endblocktrans %}

+ +

Please contact support for further assistance if necessary.

+{% endblock %} diff --git a/src/sentry/templates/sentry/account/relocate/failure.html b/src/sentry/templates/sentry/account/relocate/failure.html index e2dba15c9c4f2b..eb331dc22f8b17 100644 --- a/src/sentry/templates/sentry/account/relocate/failure.html +++ b/src/sentry/templates/sentry/account/relocate/failure.html @@ -7,7 +7,11 @@ {% block auth_main %}

{% trans "Claim Account" %}

-

{% blocktrans %}This password link has expired. Request a new password recovery code to set - your account password and claim your account.{% endblocktrans %}

-

Request Reset Code

+

{% blocktrans %}This link has expired. Request a new claim account email to set your account + password and claim your account.{% endblocktrans %}

+
+
+ +
+
{% endblock %} diff --git a/src/sentry/templates/sentry/account/relocate/sent.html b/src/sentry/templates/sentry/account/relocate/sent.html new file mode 100644 index 00000000000000..da5c71b7ae2b0b --- /dev/null +++ b/src/sentry/templates/sentry/account/relocate/sent.html @@ -0,0 +1,12 @@ +{% extends "sentry/bases/auth.html" %} + +{% load crispy_forms_tags %} +{% load i18n %} + +{% block title %}{% trans "Claim Account" %} | {{ block.super }}{% endblock %} + +{% block auth_main %} +

{% trans "Claim Account" %}

+ +

{% blocktrans %}We have sent an email to the address registered with this account containing further instructions to set your password and claim this account.{% endblocktrans %}

+{% endblock %} diff --git a/src/sentry/web/frontend/accounts.py b/src/sentry/web/frontend/accounts.py index 7504c756e59819..5067387849cfff 100644 --- a/src/sentry/web/frontend/accounts.py +++ b/src/sentry/web/frontend/accounts.py @@ -110,6 +110,70 @@ def recover(request): return render_to_response(get_template("recover", "index"), context, request) +@set_referrer_policy("strict-origin-when-cross-origin") +@control_silo_function +def relocate_reclaim(request, user_id): + """ + Ask to receive a new "claim this user" email. + """ + from sentry import ratelimits as ratelimiter + + extra = { + "ip_address": request.META["REMOTE_ADDR"], + "user_agent": request.META.get("HTTP_USER_AGENT"), + "user_id": user_id, + } + if request.method != "POST": + logger.warning("reclaim.error", extra=extra) + return render_to_response(get_template("relocate", "error"), {}, request) + + if ratelimiter.backend.is_limited( + "accounts:reclaim:{}".format(extra["ip_address"]), + limit=5, + window=60, # 5 per minute should be enough for anyone + ): + logger.warning("reclaim.rate-limited", extra=extra) + + return HttpResponse( + "You have made too many password recovery attempts. Please try again later.", + content_type="text/plain", + status=429, + ) + + # Verify that the user is unclaimed. If they are already claimed, tell the requester that this + # is the case, since of course claiming this account would be impossible. + user = User.objects.filter(id=user_id).first() + if user is None: + logger.warning("reclaim.user_not_found", extra=extra) + return render_to_response(get_template("relocate", "error"), {}, request) + if not user.is_unclaimed: + logger.warning("reclaim.already_claimed", extra=extra) + return render_to_response(get_template("relocate", "claimed"), {}, request) + + # Get all orgs for user. We'll need this info to properly compose the new relocation email. + org_ids = OrganizationMemberMapping.objects.filter(user_id=user_id).values_list( + "organization_id", flat=True + ) + org_slugs = list( + OrganizationMapping.objects.filter(organization_id__in=org_ids).values_list( + "slug", flat=True + ) + ) + if len(org_slugs) == 0: + logger.warning("reclaim.error", extra=extra) + return render_to_response(get_template("relocate", "error"), {}, request) + + # Make a new `LostPasswordHash`, and send the "this user has been relocated ..." email again. + password_hash = lost_password_hash_service.get_or_create(user_id=user_id) + LostPasswordHash.send_relocate_account_email(user, password_hash.hash, org_slugs) + extra["passwordhash_id"] = password_hash.id + extra["org_slugs"] = org_slugs + + # Let the user know that we've sent them a new email. + logger.info("recover.sent", extra=extra) + return render_to_response(get_template("relocate", "sent"), {}, request) + + @set_referrer_policy("strict-origin-when-cross-origin") @control_silo_function def recover_confirm(request, user_id, hash, mode="recover"): @@ -120,7 +184,7 @@ def recover_confirm(request, user_id, hash, mode="recover"): raise LostPasswordHash.DoesNotExist user = password_hash.user except LostPasswordHash.DoesNotExist: - return render_to_response(get_template(mode, "failure"), {}, request) + return render_to_response(get_template(mode, "failure"), {"user_id": user_id}, request) # TODO(getsentry/team-ospo#190): Clean up ternary logic and only show relocation form if user is unclaimed form_cls = RelocationForm if mode == "relocate" else ChangePasswordRecoverForm diff --git a/src/sentry/web/urls.py b/src/sentry/web/urls.py index 8e0d92f649849e..ce4b1596f35295 100644 --- a/src/sentry/web/urls.py +++ b/src/sentry/web/urls.py @@ -298,6 +298,11 @@ accounts.recover_confirm, name="sentry-account-recover-confirm", ), + re_path( + r"^relocation/reclaim/(?P[\d]+)/$", + accounts.relocate_reclaim, + name="sentry-account-relocate-reclaim", + ), re_path( r"^password/confirm/(?P[\d]+)/(?P[0-9a-zA-Z]+)/$", accounts.set_password_confirm, diff --git a/tests/sentry/tasks/test_relocation.py b/tests/sentry/tasks/test_relocation.py index 6d92864b3e7714..9ec4bdb2636041 100644 --- a/tests/sentry/tasks/test_relocation.py +++ b/tests/sentry/tasks/test_relocation.py @@ -98,6 +98,17 @@ class FakeCloudBuildClient: class RelocationTaskTestCase(TestCase): def setUp(self): super().setUp() + + # Create a collision with the org slug we'll be requesting. + self.requested_org_slug = "testing" + self.existing_org_owner = self.create_user( + email="existing_org_owner@example.com", + is_superuser=False, + is_staff=False, + is_active=True, + ) + self.existing_org = self.create_organization(name=self.requested_org_slug) + self.owner = self.create_user( email="owner@example.com", is_superuser=False, is_staff=False, is_active=True ) @@ -1859,11 +1870,20 @@ def setUp(self): printer=Printer(), ) - self.imported_users = ControlImportChunkReplica.objects.get( - import_uuid=self.uuid, model="sentry.user" + self.imported_orgs = sorted( + RegionImportChunk.objects.get( + import_uuid=self.uuid, model="sentry.organization" + ).inserted_identifiers.values() + ) + assert len(self.imported_orgs) == 1 + assert ( + len( + ControlImportChunkReplica.objects.get( + import_uuid=self.uuid, model="sentry.user" + ).inserted_map + ) + == 2 ) - - assert len(self.imported_users.inserted_map) == 2 def test_success( self, @@ -1881,8 +1901,8 @@ def test_success( mock_relocation_email.call_args_list[0][0][0].username, mock_relocation_email.call_args_list[1][0][0].username, ] - assert mock_relocation_email.call_args_list[0][0][2] == ["testing"] - assert mock_relocation_email.call_args_list[1][0][2] == ["testing"] + assert sorted(mock_relocation_email.call_args_list[0][0][2]) == self.imported_orgs + assert sorted(mock_relocation_email.call_args_list[1][0][2]) == self.imported_orgs assert "admin@example.com" in email_targets assert "member@example.com" in email_targets @@ -1969,6 +1989,18 @@ def setUp(self): self.relocation.latest_task = OrderedTask.NOTIFYING_USERS.name self.relocation.save() + RegionImportChunk.objects.create( + import_uuid=self.relocation.uuid, + model="sentry.organization", + min_ordinal=0, + max_ordinal=0, + min_source_pk=1, + max_source_pk=1, + inserted_map={1: 1234}, + inserted_identifiers={1: "testing-ab"}, + ) + self.imported_orgs = ["testing-ab"] + def test_success_admin_assisted_relocation( self, completed_mock: Mock, @@ -1980,6 +2012,7 @@ def test_success_admin_assisted_relocation( assert fake_message_builder.call_count == 1 assert fake_message_builder.call_args.kwargs["type"] == "relocation.succeeded" + assert fake_message_builder.call_args.kwargs["context"]["orgs"] == self.imported_orgs fake_message_builder.return_value.send_async.assert_called_once_with( to=[self.owner.email, self.superuser.email] ) @@ -2002,6 +2035,7 @@ def test_success_self_serve_relocation( assert fake_message_builder.call_count == 1 assert fake_message_builder.call_args.kwargs["type"] == "relocation.succeeded" + assert fake_message_builder.call_args.kwargs["context"]["orgs"] == self.imported_orgs fake_message_builder.return_value.send_async.assert_called_once_with(to=[self.owner.email]) assert completed_mock.call_count == 1 diff --git a/tests/sentry/web/frontend/test_accounts.py b/tests/sentry/web/frontend/test_accounts.py index 8da315419d49e1..4bf6a4a7aadcdf 100644 --- a/tests/sentry/web/frontend/test_accounts.py +++ b/tests/sentry/web/frontend/test_accounts.py @@ -1,13 +1,17 @@ +from datetime import timedelta from functools import cached_property from unittest.mock import call, patch +from django.core import mail from django.test import override_settings from django.urls import reverse +from django.utils import timezone from sentry.models.lostpasswordhash import LostPasswordHash from sentry.models.useremail import UserEmail from sentry.services.hybrid_cloud.organization import organization_service from sentry.testutils.cases import TestCase +from sentry.testutils.helpers.task_runner import BurstTaskRunner from sentry.testutils.silo import control_silo_test from sentry.web.frontend.accounts import recover_confirm @@ -26,6 +30,9 @@ def relocation_recover_path(self, user_id, hash_): "sentry-account-relocate-confirm", kwargs={"user_id": user_id, "hash": hash_} ) + def relocation_reclaim_path(self, user_id): + return reverse("sentry-account-relocate-reclaim", kwargs={"user_id": user_id}) + def test_get_renders_form(self): resp = self.client.get(self.path) assert resp.status_code == 200 @@ -118,9 +125,27 @@ def test_relocate_recovery_no_inputs(self): header_name = "Referrer-Policy" assert resp.has_header(header_name) - assert resp.templates[0].name == ("sentry/account/relocate/confirm.html") assert resp.status_code == 200 assert resp[header_name] == "strict-origin-when-cross-origin" + self.assertTemplateUsed("sentry/account/relocate/confirm.html") + + def test_relocate_expired_lost_password_hash(self): + user = self.create_user() + lost_password = LostPasswordHash.objects.create( + user=user, date_added=timezone.now() - timedelta(hours=2) + ) + assert not lost_password.is_valid() + + resp = self.client.get( + self.relocation_recover_path(lost_password.user_id, lost_password.hash), + ) + + header_name = "Referrer-Policy" + + assert resp.has_header(header_name) + assert resp.status_code == 200 + assert resp[header_name] == "strict-origin-when-cross-origin" + self.assertTemplateUsed("sentry/account/relocate/failure.html") @patch("sentry.signals.terms_accepted.send_robust") def test_relocate_recovery_post_multiple_orgs(self, terms_accepted_signal_mock): @@ -148,7 +173,7 @@ def test_relocate_recovery_post_multiple_orgs(self, terms_accepted_signal_mock): user.refresh_from_db() assert resp.has_header(header_name) - assert resp.templates[0].name == ("sentry/emails/password-changed.txt") + self.assertTemplateUsed("sentry/emails/password-changed.txt") assert not user.is_unclaimed assert user.username == new_username assert user.password != old_password @@ -200,7 +225,7 @@ def test_relocate_recovery_post(self, terms_accepted_signal_mock): user.refresh_from_db() assert resp.has_header(header_name) - assert resp.templates[0].name == ("sentry/emails/password-changed.txt") + self.assertTemplateUsed("sentry/emails/password-changed.txt") assert not user.is_unclaimed assert user.username == new_username assert user.password != old_password @@ -267,13 +292,90 @@ def test_relocate_recovery_invalid_password(self): user.refresh_from_db() assert resp.has_header(header_name) - assert resp.templates[0].name == ("sentry/account/relocate/failure.html") + self.assertTemplateUsed("sentry/account/relocate/failure.html") assert user.is_unclaimed assert user.username != new_username assert user.password == old_password assert resp.status_code == 200 assert resp[header_name] == "strict-origin-when-cross-origin" + def test_relocate_reclaim_success(self): + user = self.create_user(email="member@example.com", is_unclaimed=True) + lost_password = LostPasswordHash.objects.create(user=user) + org = self.create_organization(name="test-org") + self.create_member(user=user, organization=org, role="member", teams=[]) + + with BurstTaskRunner() as burst: + resp = self.client.post( + self.relocation_reclaim_path(lost_password.user_id), + ) + + # Sends the async email. + burst() + + assert len(mail.outbox) == 1 + assert mail.outbox[0].to == ["member@example.com"] + assert "Set Username and Password" in mail.outbox[0].subject + assert "test-org" in mail.outbox[0].body + assert "Claim Account" in mail.outbox[0].body + assert f"/relocation/confirm/{user.id}/{lost_password.hash}/" in mail.outbox[0].body + + header_name = "Referrer-Policy" + + assert resp.has_header(header_name) + assert resp.status_code == 200 + assert resp[header_name] == "strict-origin-when-cross-origin" + self.assertTemplateUsed("sentry/account/relocate/sent.html") + + def test_relocate_reclaim_user_not_found(self): + user = self.create_user(email="member@example.com", is_unclaimed=True) + lost_password = LostPasswordHash.objects.create(user=user) + org = self.create_organization(name="test-org") + self.create_member(user=user, organization=org, role="member", teams=[]) + + resp = self.client.post( + self.relocation_reclaim_path(lost_password.user_id + 12345), + ) + + header_name = "Referrer-Policy" + + assert resp.has_header(header_name) + assert resp.status_code == 200 + assert resp[header_name] == "strict-origin-when-cross-origin" + self.assertTemplateUsed("sentry/account/relocate/error.html") + + def test_relocate_reclaim_already_claimed(self): + user = self.create_user(email="member@example.com", is_unclaimed=False) + lost_password = LostPasswordHash.objects.create(user=user) + org = self.create_organization(name="test-org") + self.create_member(user=user, organization=org, role="member", teams=[]) + + resp = self.client.post( + self.relocation_reclaim_path(lost_password.user_id), + ) + + header_name = "Referrer-Policy" + + assert resp.has_header(header_name) + assert resp.status_code == 200 + assert resp[header_name] == "strict-origin-when-cross-origin" + self.assertTemplateUsed("sentry/account/relocate/claimed.html") + + def test_relocate_reclaim_user_not_in_any_orgs(self): + user = self.create_user(email="member@example.com", is_unclaimed=True) + lost_password = LostPasswordHash.objects.create(user=user) + + resp = self.client.post( + self.relocation_reclaim_path(lost_password.user_id), + ) + + header_name = "Referrer-Policy" + + assert resp.has_header(header_name) + assert resp.status_code == 200 + assert resp[header_name] == "strict-origin-when-cross-origin" + self.assertTemplateUsed("sentry/account/relocate/error.html") + def test_confirm_email(self): self.login_as(self.user)