search

mozilla / fxa-auth-server

DEPRECATED - Migrated to https://github.com/mozilla/fxa
Mozilla Public License 2.0
399 stars 108 forks source link

Re-enable verification reminder emails #2939

Closed shane-tomlinson closed 5 years ago

shane-tomlinson commented 5 years ago

In #2060 we removed verification reminder emails because we were using DB calls that were unsafe for replication. Let's bring the feature back using replication safe DB calls. @jbuck suggests we might be able to do this by sending events to SQS and having individual instances consume the events so that only one entry is insert per reminder.

davismtl commented 5 years ago

@shane-tomlinson do you think this could be moved up by one train? 133 instead of 134? I guess it mostly depends on the size of this work which is unclear to me.

shane-tomlinson commented 5 years ago

@shane-tomlinson do you think this could be moved up by one train? 133 instead of 134? I guess it mostly depends on the size of this work which is unclear to me.

I'm trying to get a sense of the size of this myself, one of the reasons we removed the reminders is because we were using SQL that was not safe for database replication. I'm trying to find out which calls those are and whether we have an easy path forward without leaning on another datastore.

@philbooth has expressed an interest in helping here and may have some more ideas on how to move forward.

jbuck commented 5 years ago

If you want to keep the verification reminders in MySQL, write the code assuming that there's a single reader/writer and we could run it on the fxa-admin box

vladikoff commented 5 years ago

I forgot why we didn't redis this, but I suggest y'all redis this.

philbooth commented 5 years ago

Having spent a bit of time this morning familiarising myself with all the context here, I realise the linked issue in the opening comment doesn't contain all the context. So for reference purposes...

Adding my face and moving it into active / train-133, although I'm not guaranteeing that anything is going to ship this train, just yet.

philbooth commented 5 years ago

I suggest y'all redis this.

Fwiw, this is what I intend to do.

shane-tomlinson commented 5 years ago

Fwiw, this is what I int

👍 I hoped this.

philbooth commented 5 years ago

Vague implementation plan:

shane-tomlinson commented 5 years ago

@philbooth to make explicit for the vague implementation plan: reminders are only sent if the account is still unverified.

davismtl commented 5 years ago 
* Run a simple script on `fxa-admin` that fetches the next batch from the aforementioned sorted set, dispatching any reminder emails and deleting records as necessary. We can schedule this with cron so no need to including looping logic in the script itself.

Two questions that come to mind:

I will provide product recommendations for these. I will investigate first.

philbooth commented 5 years ago
  • how long do we want to wait before reminding someone?

Fwiw, defaults from config for the old implementation were 2 days before the first reminder and 7 days before the second reminder.

davismtl commented 5 years ago

Per conversations this morning:

philbooth commented 5 years ago

While working on this, I noticed that POST /recovery_email/verify_code accepts some of its parameters both in the query string and in the body. That's silly and annoys me, because the only client is the content server, which we're in control of and can force to always use either one or the other.

Looking at the code, the key point is the verifyCode method in fxa-js-client. You can see that it always sends the data in the body, never as query params.

Thus I am hereby advertising that, included in this change, I'm also going to remove the query params from POST /recovery_email/verify_code. If anyone has a reason it's wrong for me to do that, please let me know!

shane-tomlinson commented 5 years ago

Thus I am hereby advertising that, included in this change, I'm also going to remove the query params from POST /recovery_email/verify_code. If anyone has a reason it's wrong for me to do that, please let me know!

I'm not sure why they were added as query parameters, perhaps @vladikoff or @dannycoates remember. This seems like a reasonable change to make as a standalone PR.

vladikoff commented 5 years ago

I'm not sure why they were added as query parameters

I think we wanted to POLL that endpoint, so a GET query param was a way to go

philbooth commented 5 years ago

Polling is for /recovery_email/status though right? I'm specifically talking just about POST /recovery_email/verify_code here, which would only need to be sent once, when the user clicks the link in the email.

philbooth commented 5 years ago

(the link itself is a GET request of course, but that's handled by CompleteSignUpView in the content server, which just redirects it to this POST in the auth server)

jbuck commented 5 years ago

We can use big query to ensure nobody is somehow accidentally using the queue parameters

On Thu, Mar 21, 2019 at 10:14 AM Phil Booth notifications@github.com wrote:

Polling is for /recovery_email/status though right? I'm specifically talking just about POST /recovery_email/verify_code here, which would only need to be sent once, when the user clicks the link in the email.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mozilla/fxa-auth-server/issues/2939#issuecomment-475245550, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjToj-btChKW9fQ3wtEtXCG0EHZX8Fkks5vY5PSgaJpZM4bb6G4 .

vladikoff commented 5 years ago

@philbooth ah POST /recovery_email/verify_code, I cannot remember why at this time, hmm..

shane-tomlinson commented 5 years ago 
SELECT
  *
FROM
  `moz-fx-fxa-prod-0712.fxa_prod_logs.docker_fxa_auth_20190320`
WHERE
  jsonPayload.fields.path LIKE "/v1/recovery_email/verify_code?%"
LIMIT
  1000

...

This query returned no results.

philbooth commented 5 years ago

WIP PR is at #2990.