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

Add multiple login support, fix Google-related login issues #72

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

tms
Copy link
Contributor

@tms tms commented Aug 31, 2015

When the Google login got converted from OpenID to OAuth it introduced the big issue of tying the user authentication to the unreliable User.Email value, resulting in instances where the user changes their email and loses access to their account. Similarly, if the user was preexisting but had changed their email, their old OpenID auth claim wasn't recognized, and they too ended up with a new, incorrect account.

I moved Google OAuth auth claims to the now-generic UserAuthClaim table to associate the login to the user independent of the User.Email value. I also requested the additional upgrade information in the Google auth flow to tie the new claim to an existing OpenID claim for the same account, if one exists. This is just an extension of the token request, so it comes back as information we were already requesting if we ask for it.

To top things off, I refactored the login to also just allow for multiple login methods, since there wasn't really a good reason we weren't doing this before. This required some extra UI to display the multiple methods, so there's now a "My Logins" page in the user profile:

my logins page preview

Related items on the todo list:

  • Add ability to remove login methods
  • Merge accounts that diverged because of above issue as necessary (manual work)
  • Fix return URL not working with Google login
  • Additional cleanup of the profile HTML and query management, since it's user-hostile right now

As a side note, the AD logins could have been worked in with this model as well, but I saw no point since it presumably works fine as-is and doesn't have the same problems referencing User.Email does.

@kenorb
Copy link

kenorb commented Mar 30, 2016

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 this pull request may close these issues.

3 participants