fix(auth, settings): Use redis to store unconfirmed secondary email #19588
Conversation
|
|
||
| await expect(settings.alertBar).toHaveText(/successfully deleted/); | ||
|
|
||
| await settings.secondaryEmail.addButton.click(); |
There was a problem hiding this comment.
Unconfirmed secondary emails will no longer be stored in the database, only temporarily in Redis --> if the setup is incomplete, there is no new "unconfirmed" entry in Settings.
| throw error.unverifiedAccount(); | ||
| } | ||
| // Verifies if existing holds on the requested email can be released | ||
| await clearOtherSecondaryEmailClaims(normalizedEmail); |
There was a problem hiding this comment.
Should this be done at the verify phase? I guess I'm concerned about this edge case.
- User A adds [email protected], but doesn't verify it yet.
- User B adds [email protected], but doesn't verify it yet, and doesn't actually control the email address.
- User A goes to verify [email protected], but can't because user B stole it!
There was a problem hiding this comment.
Might have jumped the gun on this use case. I see the following checks in Redis should help... I guess we could still end up deleting another user's unverified email, but that probably isn't a huge deal considering we are switching over to Redis to hold these temporary states anyway.
Just thinking out loud, but Instead of doing the delete here, could we just relying on the clean up script for this?
There was a problem hiding this comment.
Thanks for rethinking this — it’s a good sanity check. There are a few edge cases, but they should be very rare.
- With the new setup, the Redis reservation will prevent another account from attempting to add the same email as secondary during the same window.
- As for unverified secondary emails in db, I felt it was reasonable to delete them here for the sort window between rolling out the redis reservation and running the cleanup script.
- I suppose, technically, a second user could claim the email as primary while the secondary email reservation is in progress but not confirmed. My thought was that an attempt to create an account should take precedence - those will however be cleared if they have been unverified beyond a reasonable amount of time without active subscriptions.
dschom
left a comment
dschom
left a comment
There was a problem hiding this comment.
Just ran through a bunch of manual cases and couldn't break it! 
Because: * We don't want to hold on to unconfirmed secondary emails * Let's use Redis to store the temporary entry and only add to db once verified This commit: * Update emails route handlers to use Redis * Add new error type * Update front end to handle the flow changes * Update tests Closes #FXA-12548
Because
This pull request
Issue that this pull request solves
Closes: FXA-12548
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.