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 #1780

Open
eviljeff opened this issue Jul 5, 2018 · 23 comments
Assignees
Labels
needs:info neverstale Use this to tell stalebot to not touch this issue. Should be used infrequently. repository:addons-server Issue relating to addons-server

Comments

@eviljeff
Copy link
Member

eviljeff 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.

┆Issue is synchronized with this Jira Task

@eviljeff
Copy link
Member Author

eviljeff commented Jul 5, 2018

@eviljeff
Copy link
Member Author

eviljeff 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
Copy link
Member

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
Copy link
Member Author

eviljeff 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
Copy link
Member Author

eviljeff 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.

@wagnerand
Copy link
Member

@jvillalobos
Copy link

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
Copy link
Contributor

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

@eviljeff
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

eviljeff 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
Copy link
Member

wagnerand 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.

@stale

This comment has been minimized.

@stale stale bot added the state:stale Issues marked as stale. These can be re-opened should there be plans to fix them. label Sep 6, 2019
@jvillalobos jvillalobos removed the state:stale Issues marked as stale. These can be re-opened should there be plans to fix them. label Sep 6, 2019
@stale

This comment has been minimized.

@stale stale bot added the state:stale Issues marked as stale. These can be re-opened should there be plans to fix them. label Mar 4, 2020
@wagnerand wagnerand removed the state:stale Issues marked as stale. These can be re-opened should there be plans to fix them. label Mar 5, 2020
@wagnerand wagnerand added the neverstale Use this to tell stalebot to not touch this issue. Should be used infrequently. label Jun 3, 2020
@diox diox removed the triaged label Aug 25, 2021
@wagnerand
Copy link
Member

wagnerand commented Jan 19, 2022

We had another user running into this in #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
Copy link
Member

diox commented Jan 19, 2022

That would only work for #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
Copy link
Member

What are other use cases we need to consider?

@diox
Copy link
Member

diox commented Jan 20, 2022

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

@wagnerand
Copy link
Member

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

@eviljeff
Copy link
Member Author

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
Copy link
Member

diox commented Jan 20, 2022

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
Copy link

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
Copy link
Member

diox commented Aug 29, 2023

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.

@wagnerand wagnerand removed their assignment Jan 9, 2024
@KevinMind KevinMind added repository:addons-server Issue relating to addons-server migration:2024 labels Apr 26, 2024
@KevinMind KevinMind transferred this issue from mozilla/addons-server Apr 26, 2024
@diox diox added the needs:info label Sep 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs:info neverstale Use this to tell stalebot to not touch this issue. Should be used infrequently. repository:addons-server Issue relating to addons-server
Projects
None yet
Development

No branches or pull requests

10 participants