Support queueing emails without participants

[rohitpaulk]

← "Ready to verify" (#4584)

---

#4547

• Extracted out a few tests from test_email.py into test_participant_emails.py. Now, the tests in test_email.py only test stuff in gratipay/email.py → Factor out email tests #4570
• Moved email from within context to a new field in the DB. This helps in visibility + simplifies DB queries and constraints
• Added support for queuing emails without participants

Todo

• handle unsubscribes

[chadwhitacre]

Rebased, was 3d2430c.

[chadwhitacre]

Brainstorm: can we decouple participant entirely from the email queue? Is that a cleaner architecture?
I'm also put in mind of a couple other related potential improvements:

• changing the email queue so we don't delete messages (better for debugging/visibility)
• not sending more than one verification email at a time (to address Mitigate npm spam/phishing #4557)
• possibly tweaking the copy on the verification message (also for Mitigate npm spam/phishing #4557)

[chadwhitacre]

We need this for #4569 in addition to #4539.

[chadwhitacre]

Participant is related to throttling. I think we might be able to simplify throttling to only be about verified emails, rather than the generic user_initiated strategy we currently employ. Right?

[chadwhitacre]

Yeah, from a review of ack queue\.put the only place we call put without passing in _user_initiated=False (thereby bypassing throttling) is during verification.

[chadwhitacre]

That means we can resolve #4545, too!

[chadwhitacre]

Alright, let's try to keep this PR focused though ...

[chadwhitacre]

Refactoring throttling will make it easier to extricate participant from gratipay.email.

[chadwhitacre]

I've almost got a throttling rewrite done, which I plan to make as a separate PR ahead of this one. But now I think that we actually want both: throttling based on "N per time", and only allowing one open verification at a time. The reason is that we still need to allow resending verification messages, and you can be sure we'll get turkies flooding some poor maintainer with N00 verification resends if we don't throttle that. I guess we could also do that as N resends per address? Anyway, the point is that to implement (some variant) of both protections we'll need to upgrade email_queue to an email_messages table where we store email messages in perpetuity, so that we can query on recent past. I guess that's a third PR ahead of both of these.

[chadwhitacre]

Throttling redo started in #4571. Turning to permanentizing email messages table ...

[chadwhitacre]

Email saving in #4572.

[chadwhitacre]

Throttling redo redone in #4579.

In addition to better protection against verification spam/phishing, the other abuse protection we need before we can start mailing just any ol' email address is a blacklist/unsubscribe.

[chadwhitacre]

We should probably treat unsubscribes as locks against claims via that email address across all packages.

[chadwhitacre]

Moving out of review, we need unsubscribe as part of this. We can't start sending messages to random email addresses without an unsubscribe/blacklist mechanism.

[chadwhitacre]

Rebased on #4584, was 4bc67bc.

[chadwhitacre]

I'm seeing confusion in the email_messages.participant field. It conflates the sender and the recipient of messages. It's the recipient in cases where no email address is specified in the context, and it's also treated as the recipient for purposes of templating (language, username, etc.[?]). We treat participant as the sender for purposes of throttling.

So far in 27d4c8f on this PR we are bumping context.email up to email_messages.email_address, but the conflation of sender and receiver remains: the put API accepts either a participant (to) or an email address (email). This sort of works on the recipient side because we can just fall back to sane-ish defaults for templating context, but when we get to the sender side we fall into a bit of an absurdity, possibly throttling on what can only be the recipient (email).

I think we need to update the put API to clearly distinguish the sender and the recipient of messages:

• Replace the email_messages.user_initiated boolean with a sender field referencing participants. NULL means it's a system-initiated message.
• Add a required to field to email_messages that holds the recipient email address. Make it the caller's responsibility to provide all context variables required by the template rather than assuming participant and attempting defaults.

I'm not sure yet how to think about unsubscribe/blacklisting, but the above should be cleaned up one way or another, maybe in its own PR.

[chadwhitacre]

Interesting! I want to set up fixture using the old API, meaning old code old slug. But no before.py!

[chadwhitacre]

Note to self: account for the case where we want to send payday charge notifications but the participant doesn't have an email address on file (cf. gratipay/inside.gratipay.com#1149 (comment))

[chadwhitacre]

Rebased, was c61f4d7.

[chadwhitacre]

Alright, upon closer inspection, email_messages.participant is actually the recipient, and so by also using that for throttling we are assuming that the recipient is always also the sender. Hmm ... 🤔

[chadwhitacre]

And user_initiated is how we mark when the receiver is also the sender.

[chadwhitacre]

We have two presenting use cases for sending email messages to addresses unassociated with a participant:

1. email sign-up (Add support for queuing emails without a participant linked #4547), and
2. pledging (Pay for package.json in aggregate #4569).

Under (2) these are going to be system-generated messages, so we don't need to worry about throttling (well, we need to worry a lot about it, but it's on us to throttle ourselves! :D).

Under (1) we do want to throttle on the receiver (mailbombing is annoying) but even moreso on the sender (mass-mailbombing is worse, and mass-phishing is worser). That'll probably require logging IPs, out of scope on this PR.

[chadwhitacre]

Rebased yet again to go back to @rohitpaulk's original 4bc67bc w/ updates for #4590 #4579 #4584. Was 83bacdf.

[chadwhitacre]

Half-baked notes on my desktop from when I was offline:

The only place where we include the email address in the context (overriding any participant.email_address) is verification.

Hah! Pass in recipient_address and dereference a participant based on that (select * from email_addresses where address=%s and verified;). If null then double-check that we're sending a verification message, we shouldn't need username, etc. at that point and we can default to eng.

Dang, no. Email lang should come from the participant that initiated the message.

Pass in lang to put.

sender	receiver	e.g.	lang
auth	auth	user messaging	receiver
anon	auth	n/a
sys	auth
sys

• for sender:auth sender-receiver (charged) it should
• for anon sender-receiver (sign-in) it should come from http lang
• for sys sender/anon receiver (pledging) it should be en
• for auth sender-receiver (charged) it should

[chadwhitacre]

We need this for #4598. slack

[chadwhitacre]

Crap. Wrong base. :-/

[chadwhitacre]

So now my options are:

• self-merge Limit pending verifications in emails UI #4579 and "Ready to verify" #4584
• wait for someone else (@rohitpaulk? @dowski?) to merge those
• make a new PR with these commits against master

[chadwhitacre]

I meant @kaguillera, not @dowski. Honest! 🐭

[rohitpaulk]

Yh umm.. I'm confused - what did you intend to merge this into? master?

[chadwhitacre]

Yes.

[chadwhitacre]

slack