Prefer fxa_id over email when logging in, while allowing multiple accounts to have the same email

[diox]

mozilla/addons#1780

Description

This allow users to log in after changing their email in FxA even when we already have an account with that email for whatever reason (old second account that never logged in to FxA, or old second account that was soft-deleted and for which we are keeping the email temporarily)

It also deals with the fallout of that change, when there are multiple users with the same email (see testing scenarios below).

Context

Because we a) soft-delete accounts while keeping the email for a variable duration and b) still have some accounts that never logged in with FxA, we had this long-standing issue where it was possible for a user to get stuck after changing their email in FxA, because the integrity constraint was preventing us from changing the email if they had an account in either of these situations on top of their "main" account.

This PR makes the email non-unique, allowing the accounts to co-exist, and then using the fxa_id to log in, falling back to email only for accounts with no fxa_id at all.

Logging in will continue to update both fxa_id and email.

Testing

Note: reproducing the scenarios where it was previously impossible for affected users to log in is not possible locally without some manual changes. This is because those scenarios depend on the email changing event that FxA sends. You'll need to manually trigger those in order to test locally. You'll also need to have FxA authentication enabled (USE_FAKE_FXA_AUTH=False, actual FXA_CONFIG that works for local auth)

You need 2 different emails for each scenario.

User profile with duplicate email but no fxa_id

• Set up user1 in AMO so that it has fxa_id=None, email=<email_a>. Make sure FxA does not have an account for that email <email_a> already.
• Log in with a new profile, user2 with a different email, <email_b>.
• In FxA, change the primary email for that user2 to <email_a>.
  • (local environment only) trigger primary_email_change_event with user2, <email_a>
• You should now have 2 users sharing the same email
• Log in with that email <email_a>
• You should be logged in as user2.

User profile with duplicate email but deleted

• Log in with a new profile, user3 with <email_c>.
• From AMO, delete that user
• At this point user3 is soft-deleted in AMO, with <email_c> still. On real environments, after 24 hours that email will be erased if they didn't have addons.
• Log in with a new profile, user4 with <email_d>.
• In FxA, change the primary email for that user4 to <email_c>.
  • (local environment only) trigger primary_email_change_event with user4, <email_c>
• You should now have 2 users sharing the same email
• Log in with that email <email_c>
• You should be logged in as user4.

User profile with no fxa_id

(Not new, existing scenario that should still work)

• Set up user1 in AMO so that it has fxa_id=None, email=<email_a>. Make sure FxA does not have an account for that email <email_a> already.
• Log in with <email_a>
• You should be logged in as user1. In AMO db you should see an fxa_id for that user now.

Deleted and recreated user profile

(Not new, existing scenario that should still work)

• Log in with <email_a>
• Delete that user profile in AMO
• Log in with <email_a> again
• You should be logged in with the same user profile as before. It should have the same id.

Additional scenarios not related to logging in

In addition, if you set up a state where duplicate emails exists in AMO:

• Looking up an user by email in the user admin will redirect to the changelist
• Unsubscribing with an email token will use the most recently created non-deleted account - we shouldn't email the older one(s) in the first place
• Inviting an user to be an author on an add-on will also use the most recently created non-deleted account

[eviljeff]

I didn't expect this extra condition - this means we only fall back to an email match for extremely old pre-fxa UserProfiles. I.e. we're not falling back to email where we have a new (unknown to AMO) fxa_id, but an existing UserProfiles with a different fxa_id.

Why wouldn't we want to take over the old UserProfile with the new fxa_id?

[diox]

I could not think of a scenario where we would need this, so I figured I would err on the side of caution. I've looked back at a bunch of MultipleObjectsReturned tracebacks and could only find the 2 scenarios I have mentioned in the description.

The following is not a valid scenario:
user 1, fxa_id 1, email A
user 2, fxa_id 2, email B
user 1 changes their email to email B: can't happen in FxA.

That being said, let's double-check in the database that there isn't anything I've missed here.

[eviljeff]

there are likely a lot of scenarios involving us missing the notification from FxA of an email change or account deletion, but ignoring those:

• user 1 is created with fxa_id 1, email A
• user logs into AMO; does things
• user 1 is deleted on FxA
• AMO receives the deletion notification and marks the UserProfile as deleted (plus deletes their add-ons, etc)
• user 2 is created with email A
• previously we would reuse the existing UserProfile, undeleting it.

I couldn't see it called out in the issue as a change this PR is making - I'm not really convinced about it personally, but the important thing is Product/Operations are aware.

[diox]

I agree we should make everyone aware of that scenario but I think the new behavior is better here. We shouldn't "undelete" for a different FxA account, it should be considered as a different user.

[diox]

Looking more closely at more MultipleObjectsReturned tracebacks, I found a couple where both accounts had a fxa_id. But in those cases, the user was trying to log in with a valid fxa_id (which was the most recent profile too), and that has priority here.

[diox]

I just realized we don't have a unique constraint on fxa_id. That being said, we don't have any actual conflicts right now: SELECT fxa_id, COUNT(*) FROM users GROUP BY fxa_id HAVING COUNT(*) > 1; returns only NULL or empty string values. The ones with an empty string were created in 2018 or before, so we could clean them up with a migration and add a uniqueness constraint to be sure. I don't think that's urgent though.

[eviljeff]

yeah lets do that