Skip to content

Commit

Permalink
feat(backup): Notify users to claim accounts
Browse files Browse the repository at this point in the history
This is the last relocation task! It actually sends out the "recover
your account" email to all imported users.

Issue: getsentry/team-ospo#203
  • Loading branch information
azaslavsky committed Nov 16, 2023
1 parent ac00924 commit d3da641
Show file tree
Hide file tree
Showing 9 changed files with 304 additions and 51 deletions.
95 changes: 90 additions & 5 deletions fixtures/backup/fresh-install.json
Original file line number Diff line number Diff line change
Expand Up @@ -52,10 +52,18 @@
"model": "sentry.email",
"pk": 1,
"fields": {
"email": "testing@example.com",
"email": "admin@example.com",
"date_added": "2023-06-22T22:59:55.531Z"
}
},
{
"model": "sentry.email",
"pk": 2,
"fields": {
"email": "[email protected]",
"date_added": "2023-06-22T22:59:56.531Z"
}
},
{
"model": "sentry.organization",
"pk": 1,
Expand All @@ -75,9 +83,9 @@
"fields": {
"password": "pbkdf2_sha256$150000$iEvdIknqYjTr$+QsGn0tfIJ1FZLxQI37mVU1gL2KbL/wqjMtG/dFhsMA=",
"last_login": null,
"username": "testing@example.com",
"username": "admin@example.com",
"name": "",
"email": "testing@example.com",
"email": "admin@example.com",
"is_staff": true,
"is_active": true,
"is_superuser": true,
Expand All @@ -94,6 +102,31 @@
"avatar_url": null
}
},
{
"model": "sentry.user",
"pk": 2,
"fields": {
"password": "pbkdf2_sha256$150000$iEvdIknqYjTr$+Atix29ch10FZLxQI37mVU1gL2KbL/qtru7a/eu9A17=",
"last_login": null,
"username": "[email protected]",
"name": "",
"email": "[email protected]",
"is_staff": true,
"is_active": true,
"is_superuser": true,
"is_managed": false,
"is_sentry_app": null,
"is_password_expired": false,
"is_unclaimed": false,
"last_password_change": "2023-06-22T22:59:58.023Z",
"flags": "0",
"session_nonce": null,
"date_joined": "2023-06-22T22:59:56.488Z",
"last_active": "2023-06-22T22:59:56.489Z",
"avatar_type": 0,
"avatar_url": null
}
},
{
"model": "sentry.relayusage",
"pk": 1,
Expand Down Expand Up @@ -127,17 +160,39 @@
"config": "\"\""
}
},
{
"model":"sentry.authenticator",
"pk": 2,
"fields": {
"user": 2,
"created_at": "2023-06-22T22:59:56.602Z",
"last_used_at": null,
"type": 1,
"config": "\"\""
}
},
{
"model": "sentry.useremail",
"pk": 1,
"fields": {
"user": 1,
"email": "testing@example.com",
"email": "admin@example.com",
"validation_hash": "mCnWesSVvYQcq7qXQ36AZHwosAd6cghE",
"date_hash_added": "2023-06-22T22:59:55.521Z",
"is_verified": false
}
},
{
"model": "sentry.useremail",
"pk": 2,
"fields": {
"user": 2,
"email": "[email protected]",
"validation_hash": "sJui890evYQcq7qXQ36AZHwochus8f0g",
"date_hash_added": "2023-06-22T22:59:56.521Z",
"is_verified": false
}
},
{
"model": "sentry.userrole",
"pk": 1,
Expand Down Expand Up @@ -189,7 +244,27 @@
"invite_status": 0,
"type": 50,
"user_is_active": true,
"user_email": "[email protected]"
"user_email": "[email protected]"
}
},
{
"model": "sentry.organizationmember",
"pk": 2,
"fields": {
"organization": 1,
"user_id": 2,
"email": null,
"role": "member",
"flags": "0",
"token": null,
"date_added": "2023-06-22T22:59:56.561Z",
"token_expires_at": null,
"has_global_access": true,
"inviter_id": null,
"invite_status": 0,
"type": 50,
"user_is_active": true,
"user_email": "[email protected]"
}
},
{
Expand Down Expand Up @@ -261,6 +336,16 @@
"role": null
}
},
{
"model": "sentry.organizationmemberteam",
"pk": 2,
"fields": {
"team": 1,
"organizationmember": 2,
"is_active": true,
"role": null
}
},
{
"model": "sentry.projectoption",
"pk": 1,
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/api/endpoints/relocations/index.py
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ def post(self, request: Request) -> Response:
).exists():
return Response({"detail": ERR_DUPLICATE_RELOCATION}, status=409)

# TODO(getsentry/team-ospo#203): check import size, and maybe throttle based on that
# TODO(getsentry/team-ospo#216): check import size, and maybe throttle based on that
# information.

file = File.objects.create(name="raw-relocation-data.tar", type=RELOCATION_FILE_TYPE)
Expand Down
15 changes: 13 additions & 2 deletions src/sentry/models/lostpasswordhash.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,18 @@ def is_valid(self) -> bool:
return self.date_added > timezone.now() - timedelta(hours=1)

@classmethod
def send_email(cls, user, hash, request, mode="recover") -> None:
def send_recover_password_email(cls, user, hash, ip_address) -> None:
extra = {
"ip_address": ip_address,
}
cls._send_email("recover_password", user, hash, extra)

@classmethod
def send_relocate_account_email(cls, user, hash) -> None:
cls._send_email("relocate_account", user, hash, {})

@classmethod
def _send_email(cls, mode, user, hash, extra) -> None:
from sentry import options
from sentry.http import get_server_hostname
from sentry.utils.email import MessageBuilder
Expand All @@ -51,7 +62,7 @@ def send_email(cls, user, hash, request, mode="recover") -> None:
"domain": get_server_hostname(),
"url": cls.get_lostpassword_url(user.id, hash, mode),
"datetime": timezone.now(),
"ip_address": request.META["REMOTE_ADDR"],
**extra,
}

subject = "Password Recovery"
Expand Down
2 changes: 1 addition & 1 deletion src/sentry/models/relocation.py
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ class Kind(Enum):
RAW_USER_DATA = 1
# A normalized version of the user data.
#
# TODO(getsentry/team-ospo#203): Add a normalization step to the relocation flow
# TODO(getsentry/team-ospo#216): Add a normalization step to the relocation flow
NORMALIZED_USER_DATA = 2
# The global configuration we're going to validate against - pulled from the live Sentry
# instance, not supplied by the user.
Expand Down
61 changes: 53 additions & 8 deletions src/sentry/tasks/relocation.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@
from sentry.filestore.gcs import GoogleCloudStorage
from sentry.models.files.file import File
from sentry.models.files.utils import get_storage
from sentry.models.importchunk import RegionImportChunk
from sentry.models.importchunk import ControlImportChunkReplica, RegionImportChunk
from sentry.models.lostpasswordhash import LostPasswordHash as LostPasswordHash
from sentry.models.organization import Organization
from sentry.models.relocation import (
Relocation,
Expand All @@ -38,7 +39,9 @@
ValidationStatus,
)
from sentry.models.user import User
from sentry.services.hybrid_cloud.lost_password_hash import lost_password_hash_service
from sentry.services.hybrid_cloud.organization import organization_service
from sentry.services.hybrid_cloud.user.service import user_service
from sentry.silo import SiloMode
from sentry.tasks.base import instrumented_task
from sentry.utils import json
Expand Down Expand Up @@ -117,7 +120,7 @@
ERR_COMPLETED_INTERNAL = "Internal error during relocation wrap-up."


# TODO(getsentry/team-ospo#203): We should split this task in two, one for "small" imports of say
# TODO(getsentry/team-ospo#216): We should split this task in two, one for "small" imports of say
# <=10MB, and one for large imports >10MB. Then we should limit the number of daily executions of
# the latter.
@instrumented_task(
Expand Down Expand Up @@ -359,7 +362,7 @@ def preprocessing_baseline_config(uuid: str) -> None:
attempts_left,
ERR_PREPROCESSING_INTERNAL,
):
# TODO(getsentry/team-ospo#203): A very nice optimization here is to only pull this down
# TODO(getsentry/team-ospo#216): A very nice optimization here is to only pull this down
# once a day - if we've already done a relocation today, we should just copy that file
# instead of doing this (expensive!) global export again.
fp = BytesIO()
Expand Down Expand Up @@ -1015,7 +1018,7 @@ def postprocessing(uuid: str) -> None:
):
imported_org_ids = imported_org_ids.union(set(chunk.inserted_map.values()))

# Do a sanity check on pk-mapping before we go an make anyone the owner of an org they did
# Do a sanity check on pk-mapping before we go and make anyone the owner of an org they did
# not import - are all of these orgs plausibly ones that the user requested, based on slug
# matching?
imported_orgs = Organization.objects.filter(id__in=imported_org_ids)
Expand All @@ -1040,8 +1043,7 @@ def postprocessing(uuid: str) -> None:
role="owner",
)

# TODO(getsentry/team-ospo#203): Call notifying_users here.
notifying_owner.delay(uuid)
notifying_users.delay(uuid)


@instrumented_task(
Expand All @@ -1058,8 +1060,51 @@ def notifying_users(uuid: str) -> None:
Send an email to all users that have been imported, telling them to claim their accounts.
"""

# TODO(getsentry/team-ospo#203): Implement this.
pass
relocation: Optional[Relocation]
attempts_left: int
(relocation, attempts_left) = start_relocation_task(
uuid=uuid,
step=Relocation.Step.NOTIFYING,
task=OrderedTask.NOTIFYING_USERS,
allowed_task_attempts=MAX_FAST_TASK_ATTEMPTS,
)
if relocation is None:
return

with retry_task_or_fail_relocation(
relocation,
OrderedTask.NOTIFYING_USERS,
attempts_left,
ERR_NOTIFYING_INTERNAL,
):
imported_user_ids: set[int] = set()
chunks = ControlImportChunkReplica.objects.filter(
import_uuid=str(uuid), model="sentry.user"
)
for chunk in chunks:
imported_user_ids = imported_user_ids.union(set(chunk.inserted_map.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?
imported_users = user_service.get_many(filter={"user_ids": list(imported_user_ids)})
for user in imported_users:
matched_prefix = False
for username_prefix in relocation.want_usernames:
if user.username.startswith(username_prefix):
matched_prefix = True
break

# This should always be treated as an internal logic error, since we just wrote these
# orgs, so probably there is a serious bug with pk mapping.
assert matched_prefix is True

# 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)

notifying_owner.delay(uuid)


@instrumented_task(
Expand Down
11 changes: 6 additions & 5 deletions src/sentry/utils/relocation.py
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,9 @@ class OrderedTask(Enum):
VALIDATING_COMPLETE = 8
IMPORTING = 9
POSTPROCESSING = 10
NOTIFYING_OWNER = 11
COMPLETED = 12
NOTIFYING_USERS = 11
NOTIFYING_OWNER = 12
COMPLETED = 13


# The file type for a relocation export tarball of any kind.
Expand All @@ -61,10 +62,10 @@ class OrderedTask(Enum):
# be imported, a `/workspace/out` directory for exports that will be generated, and
# `/workspace/findings` for findings.
#
# TODO(getsentry/team-ospo#203): Make `get-self-hosted-repo` pull a pinned version, not
# TODO(getsentry/team-ospo#190): Make `get-self-hosted-repo` pull a pinned version, not
# mainline.
#
# TODO(getsentry/team-ospo#203): Use script in self-hosted to completely flush db instead of
# TODO(getsentry/team-ospo#216): Use script in self-hosted to completely flush db instead of
# using truncation tables.
CLOUDBUILD_YAML_TEMPLATE = Template(
"""
Expand Down Expand Up @@ -548,7 +549,7 @@ def create_cloudbuild_yaml(relocation: Relocation) -> bytes:
kind=RelocationFile.Kind.COLLIDING_USERS_VALIDATION_DATA,
args=[],
),
# TODO(getsentry/team-ospo#203): Add compare-raw-relocation-data as well.
# TODO(getsentry/team-ospo#216): Add compare-raw-relocation-data as well.
]

deps = dependencies()
Expand Down
6 changes: 4 additions & 2 deletions src/sentry/web/frontend/accounts.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def login_redirect(request):

def expired(request, user):
hash = lost_password_hash_service.get_or_create(user_id=user.id).hash
LostPasswordHash.send_email(user, hash, request)
LostPasswordHash.send_recover_password_email(user, hash, request.META["REMOTE_ADDR"])

context = {"email": user.email}
return render_to_response(get_template("recover", "expired"), context, request)
Expand Down Expand Up @@ -80,7 +80,9 @@ def recover(request):
email = form.cleaned_data["user"]
if email:
password_hash = lost_password_hash_service.get_or_create(user_id=email.id)
LostPasswordHash.send_email(email, password_hash.hash, request)
LostPasswordHash.send_recover_password_email(
email, password_hash.hash, request.META["REMOTE_ADDR"]
)

extra["passwordhash_id"] = password_hash.id
extra["user_id"] = password_hash.user_id
Expand Down
13 changes: 2 additions & 11 deletions tests/sentry/models/test_lostpasswordhash.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
from django.core import mail
from django.http import HttpRequest
from django.urls import reverse

from sentry.models.lostpasswordhash import LostPasswordHash
Expand All @@ -12,12 +11,8 @@ class LostPasswordTest(TestCase):
def test_send_recover_mail(self):
password_hash = LostPasswordHash.objects.create(user=self.user)

request = HttpRequest()
request.method = "GET"
request.META["REMOTE_ADDR"] = "1.1.1.1"

with self.options({"system.url-prefix": "http://testserver"}), self.tasks():
LostPasswordHash.send_email(self.user, password_hash.hash, request)
LostPasswordHash.send_recover_password_email(self.user, password_hash.hash, "1.1.1.1")

assert len(mail.outbox) == 1
msg = mail.outbox[0]
Expand All @@ -32,12 +27,8 @@ def test_send_recover_mail(self):
def test_send_relocation_mail(self):
password_hash = LostPasswordHash.objects.create(user=self.user)

request = HttpRequest()
request.method = "GET"
request.META["REMOTE_ADDR"] = "1.1.1.1"

with self.options({"system.url-prefix": "http://testserver"}), self.tasks():
LostPasswordHash.send_email(self.user, password_hash.hash, request, "relocate_account")
LostPasswordHash.send_relocate_account_email(self.user, password_hash.hash)

assert len(mail.outbox) == 1
msg = mail.outbox[0]
Expand Down
Loading

0 comments on commit d3da641

Please sign in to comment.