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

Handle users changing their email address in FxA to an email address we already have #8765

Open
eviljeff opened this issue Jul 5, 2018 · 12 comments

Comments

Projects
None yet
5 participants
@eviljeff
Copy link
Member

commented Jul 5, 2018

Scenario:
user_a.email = 'foo@baa', user_a.fxa_id=1234
user_b.email = 'yo@lo', user_b.fxa_id=None

user_a changes their email address in FxA to 'yo@lo'. FxA allows this because it doesn't know about AMOs users and 'yo@lo' isn't registered to another FxA account.

AMO gets a notification almost straight away via SQS and a task runs to update the user with fxa_id=1234 (user_a) to their new email address. This fails because email is a unique field - it's already used by user_b - but it's only logged in sentry as an error and can't be fed back to the user via FxA.

user_a tries to log into AMO with their auth'd FxA account. This fails because we do UserProfile.objects.get(Q(fxa_id=identity['uid']) | Q(email=identity['email'])) [src] - i.e. we match on either fxa_id or email and uid=1234 while email='yo@lo' - matching both user_a and user_b.

@eviljeff

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2018

@eviljeff

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2018

Note though this is most likely to happen with old UserProfile that don't have an fxa_id set, it's also possible even with newer accounts, if the user changed their email in the months since FxA launched the email changing feature, but before AMO started processing the SQS notifications on prod.

@wagnerand

This comment has been minimized.

Copy link
Member

commented Jul 5, 2018

This issue originated from an inquiry we got on amo-admins. Would a reasonable work-around for that affected user be to clear the fxa_id of their old account?

@eviljeff

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2018

This issue originated from an inquiry we got on amo-admins. Would a reasonable work-around for that affected user be to clear the fxa_id of their old account?

The safe solution is deleting one of the AMO accounts (hard delete).

@eviljeff

This comment has been minimized.

Copy link
Member Author

commented Jul 5, 2018

Our solutions come down to:
A) favour email over fxa_id. We would need to clear the fxa_id of less-favoured AMO account (user_a in the above scenario) so the favoured (user_b) can use it. For logins, user could be confused how they've gotten access to an old account just by changing their email address on FxA. Though they could create another FxA account with the other email address, or change back in FxA. For the automated update via SQS we could now be sending emails to the wrong address.
B) favour fxa_id over email. We would need to clear the email of less-favoured AMO account (user_b in the above scenario) so the favoured (user_a) can use it. The downside here is the other account is now abandoned without any way of linking it to the user, unless there was an fxa_id for both accounts and they still have access to both accounts. In the meantime (and possibly forever) we have no email to communicate with the user.
C) Throw an error. Pretty much what we're doing now, but rather than returning JSON, return plain text (at least!) and include contact details for amo-admin@m.o so they can talk through it and know what's happening. Then they'll need to tinker with the UserProfiles via django admin.

Far off in the future alternatives:
D) implement a UI that explains everything and gives the user the choice which account they want to keep/link to their FxA account.
E) AMO account merging.

@addons-robot addons-robot added the triaged label Jul 6, 2018

@wagnerand

This comment has been minimized.

@jvillalobos

This comment has been minimized.

Copy link
Member

commented Jul 11, 2018

I think C is the better approach. On both A and B there seems to be a possibility of data being lost or hard to track. Users can forget they had old accounts and may want to keep them after finding out.

@EnTeQuAk

This comment has been minimized.

Copy link
Member

commented Jul 12, 2018

Linking this sentry issue - https://sentry.prod.mozaws.net/operations/olympia-prod/issues/4447706/ here, this looks very related.

@eviljeff

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2018

Okay, we'll go with C then for the user login.

As for FxA updates and the tracebacks that occur like the above: Minimally, we could just handle them better and ignore the update (though that leaves one of the user accounts with the wrong email address). I guess we could email the user to let them know about the clash? Not sure how often we'd be doing that (sentry tracebacks don't seem that frequent_) and if the user confusion is worth the benefit.

@wagnerand

This comment has been minimized.

Copy link
Member

commented Jul 19, 2018

Since admins can't know what correct account to use, we clear the fxa_id for both accounts and then send out the following email:

"
Simplified, you now have two AMO accounts, one linked to your Firefox Account ID and one linked to your (we believe old or secondary) email address. When logging in, AMO does not know which one to use.

We have un-linked both AMO accounts from Firefox Account, so next time you try and log in to AMO, you should be logged in with the AMO account that belongs to your current primary email address on Firefox Accounts. If you need to access your other AMO account, you need to remove that secondary email from your Firefox Account and create a new, separate Firefox Account using that email address as a primary email adress. Unfortunately, AMO does not support account merging yet.
"

Why can't AMO clear the fxa_id automatically and then present something like above text to the user?

@eviljeff

This comment has been minimized.

Copy link
Member Author

commented Jul 19, 2018

Since admins can't know what correct account to use, we clear the fxa_id for both accounts

Okay, but that's not really talking them through the options (c) - it's (a) with an explanation. Their user_a account is now not linked the FxA account they were using; the review emails may be going to an unused email address; and they now have access to the other (most likely older) AMO account instead.

If we want to do (a) then that's one thing, and this issue becomes how to message that to the user (I'm pretty certain that would need to an addon-server thing because that's where the code is ... but it should be in addons-fronted for UX consistency).

@wagnerand

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

Admins cannot know which AMO account should be made the one the user logs in with. This requires back and forth with the affected user. And I doubt they can make an informed choice without knowing what information is attached to which account, and I am not comfortable with sharing that over email to an identity I can't verify. Even if they were able to make that decision eventually, this requires significant effort on both sides.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.