Skip to content

Commit

Permalink
feat(admin): Impove user account merging (getsentry#72692)
Browse files Browse the repository at this point in the history
We now ensure that user account merges are much more complete. All
exportable (and even some non-exportable!) data associated with the
user-being-merged is now properly pointed at the newly combined `User`
and `OrganizationMember` models, and awkward collisions (what if the
users had invited one another to a certain team?) are handled
gracefully.
  • Loading branch information
azaslavsky authored Jun 13, 2024
1 parent 473b4dd commit 01d69f1
Show file tree
Hide file tree
Showing 13 changed files with 875 additions and 466 deletions.
27 changes: 26 additions & 1 deletion src/sentry/backup/dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from typing import NamedTuple

from django.db import models
from django.db.models import UniqueConstraint
from django.db.models import Q, UniqueConstraint
from django.db.models.fields.related import ForeignKey, OneToOneField

from sentry.backup.helpers import EXCLUDED_APPS
Expand Down Expand Up @@ -701,3 +701,28 @@ def get_exportable_sentry_models() -> set[type[models.base.Model]]:
get_final_derivations_of(BaseModel),
)
)


def merge_users_for_model_in_org(
model: type[models.base.Model], *, organization_id: int, from_user_id: int, to_user_id: int
) -> None:
"""
All instances of this model in a certain organization that reference both the organization and
user in question will be pointed at the new user instead.
"""

from sentry.models.organization import Organization
from sentry.models.user import User

model_relations = dependencies()[get_model_name(model)]
user_refs = {k for k, v in model_relations.foreign_keys.items() if v.model == User}
org_refs = {
k if k.endswith("_id") else f"{k}_id"
for k, v in model_relations.foreign_keys.items()
if v.model == Organization
}
for_this_org = Q(**{field_name: organization_id for field_name in org_refs})
for user_ref in user_refs:
q = for_this_org & Q(**{user_ref: from_user_id})
obj = model.objects.filter(q)
obj.update(**{user_ref: to_user_id})
46 changes: 36 additions & 10 deletions src/sentry/models/user.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
NormalizedModelName,
PrimaryKeyMap,
get_model_name,
merge_users_for_model_in_org,
)
from sentry.backup.helpers import ImportFlags
from sentry.backup.sanitize import SanitizableField, Sanitizer
Expand All @@ -38,13 +39,15 @@
sane_repr,
)
from sentry.db.models.utils import unique_db_instance
from sentry.db.postgres.transactions import enforce_constraints
from sentry.integrations.types import EXTERNAL_PROVIDERS, ExternalProviders
from sentry.locks import locks
from sentry.models.authenticator import Authenticator
from sentry.models.avatars import UserAvatar
from sentry.models.lostpasswordhash import LostPasswordHash
from sentry.models.organizationmapping import OrganizationMapping
from sentry.models.organizationmembermapping import OrganizationMemberMapping
from sentry.models.orgauthtoken import OrgAuthToken
from sentry.models.outbox import ControlOutboxBase, OutboxCategory, outbox_context
from sentry.services.hybrid_cloud.organization import RpcRegionUser, organization_service
from sentry.services.hybrid_cloud.user import RpcUser
Expand Down Expand Up @@ -332,7 +335,7 @@ def outboxes_for_user_update(
shard_identifier=identifier,
)

def merge_to(from_user, to_user):
def merge_to(from_user: User, to_user: User) -> None:
# TODO: we could discover relations automatically and make this useful
from sentry.models.auditlogentry import AuditLogEntry
from sentry.models.authenticator import Authenticator
Expand All @@ -343,33 +346,56 @@ def merge_to(from_user, to_user):
from sentry.models.organizationmembermapping import OrganizationMemberMapping
from sentry.models.useremail import UserEmail

from_user_id = from_user.id
to_user_id = to_user.id

audit_logger.info(
"user.merge", extra={"from_user_id": from_user.id, "to_user_id": to_user.id}
"user.merge", extra={"from_user_id": from_user_id, "to_user_id": to_user_id}
)

organization_ids: list[int]
organization_ids = OrganizationMemberMapping.objects.filter(
user_id=from_user.id
user_id=from_user_id
).values_list("organization_id", flat=True)

for organization_id in organization_ids:
organization_service.merge_users(
organization_id=organization_id, from_user_id=from_user.id, to_user_id=to_user.id
organization_id=organization_id, from_user_id=from_user_id, to_user_id=to_user_id
)

model_list: tuple[type[BaseModel], ...] = (
# Update all organization control models to only use the new user id.
#
# TODO: in the future, proactively update `OrganizationMemberTeamReplica` as well.
with enforce_constraints(
transaction.atomic(using=router.db_for_write(OrganizationMemberMapping))
):
control_side_org_models: tuple[type[BaseModel], ...] = (
OrgAuthToken,
OrganizationMemberMapping,
)
for model in control_side_org_models:
merge_users_for_model_in_org(
model,
organization_id=organization_id,
from_user_id=from_user_id,
to_user_id=to_user_id,
)

# While it would be nice to make the following changes in a transaction, there are too many
# unique constraints to make this feasible. Instead, we just do it sequentially and ignore
# the `IntegrityError`s.
user_related_models: tuple[type[BaseModel], ...] = (
Authenticator,
Identity,
UserAvatar,
UserEmail,
UserOption,
)

for model in model_list:
for obj in model.objects.filter(user_id=from_user.id):
for model in user_related_models:
for obj in model.objects.filter(user_id=from_user_id):
try:
with transaction.atomic(using=router.db_for_write(User)):
obj.update(user_id=to_user.id)
obj.update(user_id=to_user_id)
except IntegrityError:
pass

Expand All @@ -385,7 +411,7 @@ def merge_to(from_user, to_user):
for ai in AuthIdentity.objects.filter(
user=from_user,
auth_provider__organization_id__in=AuthIdentity.objects.filter(
user_id=to_user.id
user_id=to_user_id
).values("auth_provider__organization_id"),
):
ai.delete()
Expand Down
107 changes: 81 additions & 26 deletions src/sentry/services/hybrid_cloud/organization/impl.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,26 +3,39 @@
from collections.abc import Mapping
from typing import Any

from django.db import IntegrityError, models, router, transaction
from django.db import models, router, transaction
from django.db.models.expressions import CombinedExpression, F
from django.dispatch import Signal

from sentry import roles
from sentry.api.serializers import serialize
from sentry.backup.dependencies import merge_users_for_model_in_org
from sentry.db.postgres.transactions import enforce_constraints
from sentry.incidents.models.alert_rule import AlertRule, AlertRuleActivity
from sentry.incidents.models.incident import IncidentActivity, IncidentSubscription
from sentry.models.activity import Activity
from sentry.models.dashboard import Dashboard
from sentry.models.dynamicsampling import CustomDynamicSamplingRule
from sentry.models.groupassignee import GroupAssignee
from sentry.models.groupbookmark import GroupBookmark
from sentry.models.groupsearchview import GroupSearchView
from sentry.models.groupseen import GroupSeen
from sentry.models.groupshare import GroupShare
from sentry.models.groupsubscription import GroupSubscription
from sentry.models.organization import Organization, OrganizationStatus
from sentry.models.organizationaccessrequest import OrganizationAccessRequest
from sentry.models.organizationmapping import OrganizationMapping
from sentry.models.organizationmember import InviteStatus, OrganizationMember
from sentry.models.organizationmemberteam import OrganizationMemberTeam
from sentry.models.outbox import ControlOutbox, OutboxCategory, OutboxScope, outbox_context
from sentry.models.projectbookmark import ProjectBookmark
from sentry.models.recentsearch import RecentSearch
from sentry.models.rule import Rule, RuleActivity
from sentry.models.rulesnooze import RuleSnooze
from sentry.models.savedsearch import SavedSearch
from sentry.models.scheduledeletion import RegionScheduledDeletion
from sentry.models.team import Team, TeamStatus
from sentry.monitors.models import Monitor
from sentry.services.hybrid_cloud import OptionValue, logger
from sentry.services.hybrid_cloud.app import app_service
from sentry.services.hybrid_cloud.organization import (
Expand Down Expand Up @@ -515,33 +528,75 @@ def merge_users(self, *, organization_id: int, from_user_id: int, to_user_id: in

assert to_member

for team in from_member.teams.all():
try:
with enforce_constraints(
transaction.atomic(router.db_for_write(OrganizationMemberTeam))
):
with enforce_constraints(transaction.atomic(using=router.db_for_write(OrganizationMember))):
# Delete all org access requests between the two now-merged users.
OrganizationAccessRequest.objects.filter(
member=from_member, requester_id=to_user_id
).delete()
OrganizationAccessRequest.objects.filter(
member=to_member, requester_id=from_user_id
).delete()

# All other org access requests should be pointed from the old member to the new
# one.
reqs = OrganizationAccessRequest.objects.filter(member=from_member)
for req in reqs:
req.member = to_member
req.save()

# Move all old team memberships to the newly merged `OrganizationMember`.
for team in from_member.teams.all():
OrganizationMemberTeam.objects.filter(
organizationmember=from_member, team=team
).delete()
to_member_team = OrganizationMemberTeam.objects.filter(
organizationmember=to_member, team=team
).first()
if to_member_team is None:
OrganizationMemberTeam.objects.create(organizationmember=to_member, team=team)
except IntegrityError:
pass

model_list = (
GroupAssignee,
GroupBookmark,
GroupSeen,
GroupShare,
GroupSubscription,
Activity,
)

for model in model_list:
for obj in model.objects.filter(
user_id=from_user_id, project__organization_id=organization_id
):
try:
with enforce_constraints(transaction.atomic(router.db_for_write(model))):
obj.update(user_id=to_user_id)
except IntegrityError:
pass
# Update all organization region models to only use the new user id.
model_list = [
Activity,
AlertRule,
AlertRuleActivity,
CustomDynamicSamplingRule,
Dashboard,
GroupAssignee,
GroupBookmark,
GroupSeen,
GroupShare,
GroupSearchView,
GroupSubscription,
IncidentActivity,
IncidentSubscription,
OrganizationAccessRequest,
ProjectBookmark,
RecentSearch,
Rule,
RuleActivity,
RuleSnooze,
SavedSearch,
]
for model in model_list:
merge_users_for_model_in_org(
model,
organization_id=organization_id,
from_user_id=from_user_id,
to_user_id=to_user_id,
)

# Finally, delete the old member.
from_member.delete()

# TODO: for some reason, `Monitor` insists on being updated outside of the transaction, even
# though it's also not region siloed?
merge_users_for_model_in_org(
Monitor,
organization_id=organization_id,
from_user_id=from_user_id,
to_user_id=to_user_id,
)

def reset_idp_flags(self, *, organization_id: int) -> None:
with unguarded_write(using=router.db_for_write(OrganizationMember)):
Expand Down
Loading

0 comments on commit 01d69f1

Please sign in to comment.