Send email when a 2fa method is added or removed

[rascalking]

This addresses #5808.

I'm hoping for some feedback on the approach before I go in and add tests. The "stash in session.info on flush, retrieve on commit" dance seemed to be a fairly common pattern, but none of them needed the request, so I'm not sure how folks will feel about stashing the it in there as well. I didn't see any other way to get the email service without the request.

[di]

Thanks for working on this and sorry for the delay!

I appreciate the thoroughness of hooking into the database events here (and I see that it was suggested in #5808) but I wonder if it's necessary? We could just call send_2fa_added_email/send_2fa_removed_email directly in the views instead.

[rascalking]

If folks aren't worried about it timing out the request, yup, that'd certainly simplify things. I'll redo this with that approach.

[rascalking]

Oh, I'd forgotten, part of the reason for doing it this way is so that we're only sending the email if the db commit succeeds. I'm not sure how often a commit failing happens in practice, but we probably shouldn't send the email if it does, right?

[di]

@rascalking The email gets added to a queue, so not really at risk for timing out the request, but I appreciate you considering it.

The commits don't usually fail (and would not likely fail here) but I think that's probably a valid concern. Curious if @ewdurbin has any thoughts here as well.

[ewdurbin]

That's an interesting concern. I suppose we haven't really considered it in other task enqueuing.

I suppose we have to choose one of two cases:

• Task enqueue succeeds, commit fails
• Commit succeeds, task enqueue fails

Both are similarly possible. I think I'd prefer the first case as far as account notifications are concerned. If someone tried to add or remove a 2FA method on my account I'd like to know about it... even if they got an error :)

[di]

I think I'd prefer the first case as far as account notifications are concerned.

I think I agree, at least for the scope of this PR.

[dstufft]

That's an interesting concern. I suppose we haven't really considered it in other task enqueuing.

I suppose we have to choose one of two cases:

• Task enqueue succeeds, commit fails
• Commit succeeds, task enqueue fails

Both are similarly possible. I think I'd prefer the first case as far as account notifications are concerned. If someone tried to add or remove a 2FA method on my account I'd like to know about it... even if they got an error :)

Actually, our Celery integration is already setup to only enqueue in the case that the database commit goes through successfully. It also ensures that the task gets queued after the database has been written to, to avoid conditions where you fire off a task, the celery worker grabs it and starts a database transaction before you've actually submitted your own.

This is all taken care of here in warehouse/tasks.py#L93-L113.

[rascalking]

Wow, nice bit of plumbing.

[ewdurbin]

This looks great, I'd like to see us change from 2fa to two_factor in the code and email template names before shipping, but that's all!

[ewdurbin]

I feel like 2fa should be spelled out as two_factor here and other places in this changeset. This is more consistent with what we in other places in the code base.

[rascalking]

All set.

[ewdurbin]

Thanks @rascalking!

[brainwane]

Thank you very much @rascalking! Really appreciated. If you are interested in continuing to contribute to Warehouse and would like suggestions on what to do next, reply to let us know!

[rascalking]

Yup, I just grabbed a bunch of things that looked like useful low-hanging fruit for hacktoberfest, but if there's other stuff that needs working on, I'm happy to pick some of that up.

[brainwane]

@rascalking could you take a look at #5867 ?