Skip to content

Commit

Permalink
feat(relocation): Improve claim user emails (#68495)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
azaslavsky authored Apr 9, 2024
1 parent f915116 commit d38e564
Show file tree
Hide file tree
Showing 9 changed files with 278 additions and 18 deletions.
21 changes: 17 additions & 4 deletions src/sentry/tasks/relocation.py
Original file line number Diff line number Diff line change
Expand Up @@ -1097,15 +1097,16 @@ 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.
for _, result in relocated.send_robust(sender=postprocessing, relocation_uuid=uuid):
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,
Expand Down Expand Up @@ -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?
Expand All @@ -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()
Expand Down Expand Up @@ -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),
},
)

Expand Down
12 changes: 12 additions & 0 deletions src/sentry/templates/sentry/account/relocate/claimed.html
Original file line number Diff line number Diff line change
@@ -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 %}
<h3>{% trans "Claim Account" %}</h3>

<p>This account has already been claimed - you should be able to log in as normal. You can always <a href="{% url 'sentry-account-recover' %}">reset your password</a> if you have forgotten it.</p>
{% endblock %}
14 changes: 14 additions & 0 deletions src/sentry/templates/sentry/account/relocate/error.html
Original file line number Diff line number Diff line change
@@ -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 %}
<h3>{% trans "Claim Account" %}</h3>

<p>{% blocktrans %}We were unable to locate this account.{% endblocktrans %}</p>

<p>Please <a href="https://help.sentry.io">contact support</a> for further assistance if necessary.</p>
{% endblock %}
10 changes: 7 additions & 3 deletions src/sentry/templates/sentry/account/relocate/failure.html
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@

{% block auth_main %}
<h3>{% trans "Claim Account" %}</h3>
<p>{% blocktrans %}This password link has expired. Request a new password recovery code to set
your account password and claim your account.{% endblocktrans %}</p>
<p><a href="{% url 'sentry-account-recover' %}" class="btn btn-primary">Request Reset Code</a></p>
<p>{% blocktrans %}This link has expired. Request a new claim account email to set your account
password and claim your account.{% endblocktrans %}</p>
<form method="POST" action="{% url 'sentry-account-relocate-reclaim' user_id %}">
<fieldset class="form-actions">
<button type="submit" class="btn btn-primary">{% trans "Resend Email" %}</button>
</fieldset>
</form>
{% endblock %}
12 changes: 12 additions & 0 deletions src/sentry/templates/sentry/account/relocate/sent.html
Original file line number Diff line number Diff line change
@@ -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 %}
<h3>{% trans "Claim Account" %}</h3>

<p>{% 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 %}</p>
{% endblock %}
66 changes: 65 additions & 1 deletion src/sentry/web/frontend/accounts.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"):
Expand All @@ -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
Expand Down
5 changes: 5 additions & 0 deletions src/sentry/web/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -298,6 +298,11 @@
accounts.recover_confirm,
name="sentry-account-recover-confirm",
),
re_path(
r"^relocation/reclaim/(?P<user_id>[\d]+)/$",
accounts.relocate_reclaim,
name="sentry-account-relocate-reclaim",
),
re_path(
r"^password/confirm/(?P<user_id>[\d]+)/(?P<hash>[0-9a-zA-Z]+)/$",
accounts.set_password_confirm,
Expand Down
46 changes: 40 additions & 6 deletions tests/sentry/tasks/test_relocation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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="[email protected]",
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="[email protected]", is_superuser=False, is_staff=False, is_active=True
)
Expand Down Expand Up @@ -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,
Expand All @@ -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 "[email protected]" in email_targets
assert "[email protected]" in email_targets

Expand Down Expand Up @@ -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,
Expand All @@ -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]
)
Expand All @@ -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
Expand Down
Loading

0 comments on commit d38e564

Please sign in to comment.