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

feat(settings): Expose all identities in user settings #29230

Merged
merged 49 commits into from
Nov 5, 2021

Conversation

RyanSkonnord
Copy link
Contributor

@RyanSkonnord RyanSkonnord commented Oct 9, 2021

Show a user's entry from the Identity and AuthIdentity tables in the
user "Identities" settings view, in addition to UserSocialAuth
identities. Add UI elements to explain the difference between the
identity types. Allow the new identities to be manually disconnected at
the user's choice, but not if doing so would lock them out of their
account or an organization.

Show a user's entry from the Identity and AuthIdentity tables in the
user "Identities" settings view, in addition to UserSocialAuth
identities. Add UI elements to explain the difference between the
identity types. Allow the new identities to be manually disconnected at
the user's choice, but not if doing so would lock them out of their
account or an organization.

TODO:
  - Tests
  - Design review on UI changes
@RyanSkonnord RyanSkonnord requested a review from a team October 9, 2021 03:31
@RyanSkonnord
Copy link
Contributor Author

Sample of my placeholder redesign

Screen Shot 2021-10-08 at 8 31 03 PM

"""

identities = list(get_identities(user))
return Response(serialize(identities))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need to consider paginating this endpoint? haha 💀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or i guess, handle if they have too many identities.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😅 I'm hoping we can just punt pagination. It would be awkward because the results span multiple models, are interdependent, and don't have a natural order. It's totally doable, but my impression is that it would be a lot of code without a lot of need.

It helps that, since the platform supports a finite number of providers, there is a natural ceiling to the number of identities you can have on one account (at least without belonging to an unbounded number of organizations and having one AuthIdentity for each).

Copy link
Member

@JoshFerge JoshFerge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

backend stuff looking good to me 👍🏼

(Oops, had accidentally reverted this. The import in
endpoints/user_identity_config.py was just broken.)
@github-actions
Copy link
Contributor

github-actions bot commented Oct 11, 2021

size-limit report

Path Base Size (eb70030) Current Size Change
src/sentry/static/sentry/dist/entrypoints/app.js 52.77 KB 52.76 KB -0.01% 🔽
src/sentry/static/sentry/dist/entrypoints/sentry.css 70.89 KB 70.89 KB 0%



@dataclass(eq=True, frozen=True)
class UserIdentityConfig:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very nice

Co-authored-by: Evan Purkhiser <evanpurkhiser@gmail.com>
Co-authored-by: Evan Purkhiser <evanpurkhiser@gmail.com>
@RyanSkonnord
Copy link
Contributor Author

RyanSkonnord commented Nov 4, 2021

A note about deployment: this needs to go out near-concurrently with https://github.com/getsentry/getsentry/pull/6536. If there is any lag time between the two (because of having to wait on CI) that's not bridged with a deployment freeze, the consequence is that deletion protection for global login identities would be disabled during that window. (That is, users with only one global login identity and no password would not be stopped from disconnecting that identity.)

Considering this is an obscure corner of the settings page and it's unlikely a user would both stumble across it and do something stupid during this window, I think this is okay? They'd still be able to do a password reset even in the worst case. Anyway, if the risk is unacceptable, there are a few alternatives: the simplest would be to temporarily hard-code the affected providers and then do another PR to remove the hard-coding afterward.

I'm happy to do it the safer way if someone can volunteer to stand by and click "approve" on the extra PR. But I believe the risk is acceptably minimal with the simpler way.

@RyanSkonnord RyanSkonnord changed the title feat(settings): Expose all login identities in user settings feat(settings): Expose all identities in user settings Nov 4, 2021
Copy link
Member

@evanpurkhiser evanpurkhiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@RyanSkonnord RyanSkonnord merged commit 61d7305 into master Nov 5, 2021
@RyanSkonnord RyanSkonnord deleted the manual-identity-management branch November 5, 2021 16:50
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants