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
FND 1264 - restrict silent takeover scenarios #36716
Conversation
if allows_silent_takeover(user, auth_hash) | ||
user = silent_takeover user, auth_hash | ||
sign_in_user user | ||
if email_already_taken(user) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
won't this new conditional break the one below it?
code-dot-org/dashboard/app/controllers/omniauth_callbacks_controller.rb
Lines 193 to 197 in e921188
elsif (looked_up_user = User.find_by_email_or_hashed_email(user.email)) | |
email_already_taken_redirect \ | |
provider: provider, | |
found_provider: looked_up_user.provider, | |
email: user.email |
lookup_user = User.find_by_email_or_hashed_email(oauth_user.email) | ||
verified_email_credentials = AuthenticationOption::TRUSTED_EMAIL_CREDENTIAL_TYPES - [AuthenticationOption::EMAIL] | ||
|
||
lookup_user = AuthenticationOption.where(credential_type: verified_email_credentials).find_by(hashed_email: User.hash_email(oauth_user.email))&.user || User.where(hashed_email: User.hash_email(oauth_user.email)).where(provider: verified_email_credentials).first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[i know this is a draft, so excuse the nit] we should break this into multiple lines for readability and so that we only need to look up the User
once
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! i also wouldn't mind if we stopped supporting silent takeover altogether 😈
See the JIRA ticket for more info.
This PR limits silent takeover cases to only when the the account being taken over already has a trusted authentication option.
In other cases, when a user tries to sign up, they'll see
Links
Testing story
Reviewer Checklist: