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

Disconnected OAuth AuthenticationOption bug #24459

Merged
merged 7 commits into from Aug 28, 2018

Conversation

maddiedierker
Copy link
Contributor

@maddiedierker maddiedierker commented Aug 24, 2018

Bug description

I had an account linked to my Microsoft and Google Account. I also had a password attached to it. The primary email address matched my Microsoft Account. I disconnected both the linked accounts. Now my account should not be associated to my gmail account at all. But then if I tried to sign in with my Google Account, it would sign me back to that account.

Longer description

Let's use Google OAuth as an example: You create an account through Google, change your primary authentication option in some way so that you should no longer be attached to the original Google email address you used to create an account. However, because the before-save hooks normalize_email and hash_email were using read_attribute(:email) instead of the email method, that original Google email address was always attached to your account via the email column on the User table.

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Cool.

Copy link
Contributor

@ewjordan ewjordan left a comment

Choose a reason for hiding this comment

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

LGTM code-wise, though there's a failing Circle test that seems like it might be related that's worth looking into pre-merge.

@maddiedierker
Copy link
Contributor Author

@islemaster and @ewjordan just a heads up -- the unit test failure in UserMultiAuthHelperTest was related and informed a slight refactor on my part.

these tests were checking that the email and hashed_email fields are emptied on the User model upon migrating, which my refactor broke. so, in order to fix the bug but keep that functionality (which i think is important for when we want to delete those fields on the User table), i am manually emptying email/hashed_email for migrated users before save.

@maddiedierker maddiedierker merged commit 24b770e into staging Aug 28, 2018
@maddiedierker maddiedierker deleted the disconnected-oauth-bug branch August 28, 2018 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants