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

[eviljeff]

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.

┆Issue is synchronized with this Jira Task

[eviljeff]

This happened for real here (sqs update, then login): https://sentry.prod.mozaws.net/operations/olympia-prod/issues/4424264/ and then https://sentry.prod.mozaws.net/operations/olympia-prod/issues/691108/

[eviljeff]

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 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 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]

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.

[wagnerand]

Happened again: https://sentry.prod.mozaws.net/operations/olympia-prod/issues/4448986/

[jvillalobos]

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]

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

[eviljeff]

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]

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]

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]

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.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. If you think this bug should stay open, please comment on the issue with further details. Thank you for your contributions.

[wagnerand]

We had another user running into this in mozilla/addons#1856. When talking through the options to fix for that user, @diox and I came up with a potential idea to resolve this for all cases: During the signup/login account lookup, we could exclude deleted and not banned accounts in the query.

[diox]

That would only work for mozilla/addons#1856, and only if we also changing the unique constraint so that it covers both the deleted and email field - because for a successful login, we also try to update the user fxa_id and email if what we have in the database differs from what FxA sent.

(In any case we need to be careful about not accidentally allowing a banned user to create a new account with the same email)

[wagnerand]

What are other use cases we need to consider?

[diox]

The original use case in this issue doesn't have a deleted account at all

[wagnerand]

Can this still happen (excluding ancient accounts), given that we listen to email change events?

[eviljeff]

Can this still happen (excluding ancient accounts), given that we listen to email change events?

It shouldn't but it's not 100% guaranteed we receive a notification and don't fail during processing it. Though those are edge cases (along with prehistoric accounts) so we probably don't need to consider them (i.e. just let them fail like now)

[diox]

Since this is all async and we don't have a guarantee we'll receive and process a particular event before another, yes. (plus, as you allude to, there are old accounts, accounts for which a task failed, etc)

[johnmellor]

I'm stuck in this situation. Before Firefox Accounts existed (and probably before Pocket was acquired by Mozilla) I created three accounts:

• a Pocket account using email A with some Pocket saves
• an AMO account using email B which uploaded a couple of extensions
• a BMO account using email B (but I think we can ignore that since BMO has separate accounts)

(I don't think I've signed into Firefox Sync yet on any Firefox browsers.)

My Pocket account got upgraded into a Firefox Account for email A. I accidentally signed in to AMO with that Firefox Account, before remembering that I used to use a different email address with AMO. So then I wanted to merge the Firefox Account with my old AMO account from email B. So I followed these instructions and tried the following:

1. Signed out of AMO on https://accounts.firefox.com/settings#connected-services
2. Deleted the empty AMO account for email A from the AMO profile page (I may have had to sign back in first).
3. Cleared cookies
4. Changed the Firefox Account email address from email A to email B on https://accounts.firefox.com/settings#profile
5. Signed in to AMO using the Firefox account (which now uses email B).

Expected result: I was hoping this would associate my old email B AMO account with my Firefox Account, so I would have both the Pocket and AMO data on a single account. (Then I planned to change my Firefox Account email back from email B to email A, in order to access both Pocket and AMO using email A).

Actual result: Internal Server Error on https://addons.mozilla.org/api/auth/authenticate-callback/

It sounds like in this case excluding deleted and not banned accounts as suggested by wagnerand would have avoided this error?

[diox]

The problem with that approach is that if the second account also gets deleted, we can't keep the email for a little while like we do now, as it would conflict with the first one because of the unique constraint. We'd need to add a workaround for this as well.