Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

User reconciliation logic for importer User export scoped models #181

Closed
Tracked by #153
azaslavsky opened this issue Aug 8, 2023 · 0 comments · Fixed by getsentry/sentry#55876
Closed
Tracked by #153

Comments

@azaslavsky
Copy link

azaslavsky commented Aug 8, 2023

Summary

There are still have some open questions regarding how we’ll handle relocating users from self-hosted to SaaS, particularly in the case that the user already has an email with the SaaS service. This document outlines the preferred solution to this latter problem, as well as a rejected alternative.

Background

A detailed description of the problem we are trying to solve, and the concrete tasks scoped out for both Q3 and the rest of FY2024 can be found in the GitHub meta-bug and associated PRD🔒.

For what it’s worth, I think the tricky edge case being considered here (“what if this email already exists on Sentry SaaS?”) is going to be fairly rare - my (totally unevidenced and unsubstantiated - I’m really just spitballing here) guess is that a low single-digit percentage of user accounts imported via the relocation path will have this problem. Most users being imported will be using their corporate-domained email, which is unlikely to already exist on Sentry. However, a small smattering of users like contractors, consultants, abusive actors (and those pretending to be such, like security researchers) or users from personal projects still make this an edge case worth considering.

Design

Suppose a self-hosted account gets imported with exactly 2 users. One of these users has an email that does not yet exist ([email protected]), while another has one that is already in use ([email protected]).

The first user is on a happy path: they don’t already exist in the system, so we just make a brand new User model for them, in unclaimed mode. An Unclaimed User is just like a regular user, except we’ve replaced the password (and possibly the username, if it already exists) with random strings, and set the new is_unclaimed flag to True. Each newly created Unclaimed User gets an email that they’ve been invited to Sentry, which, when they click through, takes them to a screen where they can change their password and username, thereby “claiming” the user account as their own.

Why do we do this differently from the usual “you’ve been invited to join organization X” email we send when not-yet-existing users are invited to join an organization? Because relocating Organization data from self-hosted requires that the User models associated with entries in that Organization already exist (ex: who are the members of this new Team? Who owns a particular SavedSearch? Etc…).

If a user already exists in the system, we use the same exact flow as we do for new users. This is because we allow multiple unique users per email in Sentry, so we’ll just have them create a new user associated with the same email (for now, see Future Work below).

Future Work

In the future, a good improvement would be to add some sort of “account merge” tool to user settings. This would basically have you log into any accounts that share your email and “merge” the two Users together.

This feature is a future nice-to-have, since it would prevent the proliferation of many emails for one account, which is something that we’d like to avoid all other things being equal. This work would not block the main relocation effort, and is something we could scope out and circle back to later as needed.

Risks

One major risk to user relocation is that we accidentally import a User from a self-hosted instance with super admin and/or staff permissions, and don’t properly scrub them when uploading them to SaaS. We’ll need to take extra care when importing UserPermissions, make sure to carefully sanitize those along with User flags like is_superuser, is_staff, etc.

Another concern is that this could be a potential spam avenue: someone could upload a fake import with thousands of emails of users they want to phish somehow (perhaps to check if the emails exist?). Users are still protected in this situation, as they will have to proactively respond to the email to join the fake organization to be fooled by the potential account sniffing attempt. In any case, this is not any more spamming power than we give users when inviting folks to their organizations in the existing Sentry UI.

Alternatives Considered

There was another alternative considered for relocated users that already have emails in use on SaaS. This method would have involved matching the user to the “best account” with a shared email (using the same algorithm for “best account” as we use for password recovery), swapping in that User instead of the one being relocated, and then doing a 2FA re-authorization for them. The 2FA reauthorization requires users to “re-agree” to enter the organization, thereby giving an existing user a means by which to provide confirmation that they want to be in the organization.

This method was rejected because it suffered from a few issues. The first is a security issue: it would be easy to sniff for existing Sentry accounts and spam existing Sentry users using this method.

It is also more complex than the accepted design, since it involves a relatively involved “user swapping” operation for each existing user. This is a fairly brittle song-and-dance to accommodate what it is expected to be a fairly rare occurrence.

@azaslavsky azaslavsky changed the title Custom user-importing logic User reconciliation logic for importer User export scoped models Aug 9, 2023
azaslavsky added a commit to getsentry/sentry that referenced this issue Aug 18, 2023
Importing User models onto SaaS could be dangerous: the imported user
might have overpowered flags (`is_staff`, `is_superuser`, etc),
excessive `UserPermission`s, or naughty `UserRole`s assigned. These
changes modify the import logic remove sanitize those potentially bad
inputs.

Such sanitization only needs to happen sometimes, though: if you are
using this tool to restore a full self-hosted instance, you actually DO
want all of this potentially dangerous data to be imported unchanged
from your own exports. To resolve this, this change introduces the
concept of `ImportScope`s, which maps very closely to `RelocationScope`.
Using `import_in_global_scope` therefore does not perform sanitization,
while the other, narrower `User` and `Organization` scopes do.

Issue: getsentry/team-ospo#166
Issue: getsentry/team-ospo#181
azaslavsky added a commit to getsentry/sentry that referenced this issue Aug 30, 2023
Importing User models onto SaaS could be dangerous: the imported user
might have overpowered flags (`is_staff`, `is_superuser`, etc),
excessive `UserPermission`s, or naughty `UserRole`s assigned. These
changes modify the import logic remove sanitize those potentially bad
inputs.

Such sanitization only needs to happen sometimes, though: if you are
using this tool to restore a full self-hosted instance, you actually DO
want all of this potentially dangerous data to be imported unchanged
from your own exports. To resolve this, this change introduces the
concept of `ImportScope`s, which maps very closely to `RelocationScope`.
Using `import_in_global_scope` therefore does not perform sanitization,
while the other, narrower `User` and `Organization` scopes do.

Issue: getsentry/team-ospo#166
Issue: getsentry/team-ospo#181
azaslavsky added a commit to getsentry/sentry that referenced this issue Aug 30, 2023
The previous code assumed UserEmail collisions are possible. It is now
refactored to treat them all as unique, and always reset them from their
imported state for all `ImportScope`s except `Global`.

Issue: getsentry/team-ospo#181
azaslavsky added a commit to getsentry/sentry that referenced this issue Aug 30, 2023
Importing User models onto SaaS could be dangerous: the imported user
might have overpowered flags (`is_staff`, `is_superuser`, etc),
excessive `UserPermission`s, or naughty `UserRole`s assigned. These
changes modify the import logic remove sanitize those potentially bad
inputs.

Such sanitization only needs to happen sometimes, though: if you are
using this tool to restore a full self-hosted instance, you actually DO
want all of this potentially dangerous data to be imported unchanged
from your own exports. To resolve this, this change introduces the
concept of `ImportScope`s, which maps very closely to `RelocationScope`.
Using `import_in_global_scope` therefore does not perform sanitization,
while the other, narrower `User` and `Organization` scopes do.

Issue: getsentry/team-ospo#166
Issue: getsentry/team-ospo#181
azaslavsky added a commit to getsentry/sentry that referenced this issue Aug 30, 2023
The previous code assumed UserEmail collisions are possible. It is now
refactored to treat them all as unique, and always reset them from their
imported state for all `ImportScope`s except `Global`.

Issue: getsentry/team-ospo#181
azaslavsky added a commit to getsentry/sentry that referenced this issue Sep 1, 2023
The previous code assumed UserEmail collisions are possible. It is now
refactored to treat them all as unique, and always reset them from their
imported state for all `ImportScope`s except `Global`.

Issue: getsentry/team-ospo#181
azaslavsky added a commit to getsentry/sentry that referenced this issue Sep 1, 2023
The previous code assumed UserEmail collisions are possible. It is now
refactored to treat them all as unique, and always reset them from their
imported state for all `ImportScope`s except `Global`.

Issue: getsentry/team-ospo#181
azaslavsky added a commit to getsentry/sentry that referenced this issue Sep 6, 2023
EricHasegawa pushed a commit to getsentry/sentry that referenced this issue Sep 6, 2023
The previous code assumed UserEmail collisions are possible. It is now
refactored to treat them all as unique, and always reset them from their
imported state for all `ImportScope`s except `Global`.

Issue: getsentry/team-ospo#181
azaslavsky added a commit to getsentry/sentry that referenced this issue Sep 6, 2023
As of this PR, everything for supporting unclaimed users save the actual addition of the is_unclaimed flag itself and the email/UI copy to support that is ready.

Issue: getsentry/team-ospo#170
Issue: getsentry/team-ospo#181
Issue: getsentry/team-ospo#182
azaslavsky added a commit to getsentry/sentry that referenced this issue Sep 6, 2023
As of this PR, everything for supporting unclaimed users save the actual
addition of the `is_unclaimed` flag itself and the email/UI copy to
support that is ready.

Issue: getsentry/team-ospo#170
Issue: getsentry/team-ospo#181
Issue: getsentry/team-ospo#182
azaslavsky added a commit to getsentry/sentry that referenced this issue Sep 8, 2023
This flag controls what we do when we encounter a `username` collision.
When the flag is false (the default value), we simply create a new user
with a randomized suffix on their username. When the flag is true,
however, we re-use the existing user.

This is useful for region to region relocations, where we want to avoid
creating a new batch of users every time an org moves between region
silos.

Issue: getsentry/team-ospo#181
azaslavsky added a commit to getsentry/sentry that referenced this issue Sep 8, 2023
This flag controls what we do when we encounter a `username` collision.
When the flag is false (the default value), we simply create a new user
with a randomized suffix on their username. When the flag is true,
however, we re-use the existing user.

This is useful for region to region relocations, where we want to avoid
creating a new batch of users every time an org moves between region
silos.

Issue: getsentry/team-ospo#181
azaslavsky added a commit to getsentry/sentry that referenced this issue Sep 8, 2023
This was redundant code that basically duplicated the functionality of
the `__relocation_scope__` we set on each model. What the models that
used it really wanted was to be in the global relocation scope, which is
now the case.

Issue: getsentry/team-ospo#181
azaslavsky added a commit to getsentry/sentry that referenced this issue Sep 8, 2023
This was redundant code that basically duplicated the functionality of
the `__relocation_scope__` we set on each model. What the models that
used it really wanted was to be in the global relocation scope, which is
now the case.

Issue: getsentry/team-ospo#181
azaslavsky added a commit to getsentry/sentry that referenced this issue Sep 11, 2023
This allows us to import users in an "unclaimed" mode: their password
has been reset, and, in the event of a username collision, they have
been given a random suffix to their old username. Such users will then
receive an email when their relocation is complete, informing them that
an account they owned on another Sentry instance has been imported, and
encouraging them to claim this one by setting a new password and
username. By attaching this flag to the imported `User` model instance,
we are able to detect such unclaimed users, and modify the copy/UI shown
to them during this process.

Closes getsentry/team-ospo#181
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant