Allow multiple mail receivers to react to an individual email
ClosedPublic
Actions

Authored by epriestley on Jan 3 2019, 7:47 PM.

Details

Reviewers

Maniphest Tasks

T7477: Handle inbound email with phabricator address in the CC
T13066: Refine inbound mail error behaviors, distinguishing between actual mail sender and acting-as sender

Commits

rPe3aa043a0272: Allow multiple mail receivers to react to an individual email

Summary

Fixes T7477. Fixes T13066. Currently, inbound mail is processed by the first receiver that matches any "To:" address. "Cc" addresses are ignored.

To, CC, and Multiple Receivers

Some users would like to be able to "Cc" addresses like bugs@ instead of having to "To" the address, which makes perfect sense. That's the driving use case behind T7477.

Since users can To/Cc multiple "create object" or "update object" addresses, I also wanted to make the behavior more general. For example, if you email bugs@ and also paste@, your mail might reasonably make both a Task and a Paste. Is this useful? I'm not sure. But it seems like it's pretty clearly the best match for user intent, and the least-surprising behavior we can have. There's also no good rule for picking which address "wins" when two or more match -- we ended up with "address order", which is pretty arbitrary since "To" and "Cc" are not really ordered fields.

One part of this change is removing phabricator.allow-email-users. In practice, this option only controlled whether users were allowed to send mail to "Application Email" addresses with a configured default author, and it's unlikely that we'll expand it since I think the future of external/grey users is Nuance, not richer interaction with Maniphest/Differential/etc. Since this option only made "Default Author" work and "Default Author" is optional, we can simplify behavior by making the rule work like this:

If an address specifies a default author, it allows public email.
If an address does not, it doesn't.

That's basically how it worked already, except that you could intentionally "break" the behavior by not configuring phabricator.allow-email-users. This is a backwards compatility change with possible security implications (it might allow email in that was previously blocked by configuration) that I'll call out in the changelog, but I suspect that no installs are really impacted and this new behavior is generally more intuitive.

A somewhat related change here is that each receiver is allowed to react to each individual email address, instead of firing once. This allows you to configure bugs-a@ and bugs-b@ and CC them both and get two tasks. Useful? Maybe not, but seems like the best execution of intent.

Sender vs Author

Adjacently, T13066 described an improvement to error handling behavior here: we did not distinguish between "sender" (the user matching the email "From" address) and "actor" (the user we're actually acting as in the application). These are different when you're some internet rando and send to bugs@, which has a default author. Then the "sender" is null and the "author" is @bugs-robot or whatever (some user account you've configured).

This refines "Sender" vs "Author". This is mostly a purity/correctness change, but it means that we won't send random email error messages to @bugs-robot.

Since receivers are now allowed to process mail with no "sender" if they have some default "actor" they would rather use instead, it's not an error to send from an invalid address unless nothing processes the mail.

Other

This removes the "abundant receivers" error since this is no longer an error.

This always sets "external user" mail recipients to be unverified. As far as I can tell, there's no pathway by which we send them email anyway (before or after this change), although it's possible I'm missing something somewhere.

Test Plan

I did most of this with bin/mail receive-test. I rigged the workflow slightly for some of it since it doesn't support multiple addresses or explicit "CC" and adding either would be a bit tricky.

These could also be tested with scripts/mail/mail_handler.php, but I don't currently have the MIME parser extension installed locally after a recent upgrade to Mojave and suspect T13232 makes it tricky to install.

Ran unit tests, which provide significant coverage of this flow.
Sent mail to multiple Maniphest application emails, got multiple tasks.
Sent mail to a Maniphest and a Paste application email, got a task and a paste.
Sent mail to a task.
- Saw original email recorded on tasks. This is a behavior particular to tasks.
Sent mail to a paste.
Sent mail to a mock.
Sent mail to a Phame blog post.
Sent mail to a Legalpad document.
Sent mail to a Conpherence thread.
Sent mail to a poll.
This isn't every type of supported object but it's enough of them that I'm pretty confident I didn't break the whole flow.
Sent mail to an object I could not view (got an error).
As a non-user, sent mail to several "create an object..." addresses.
- Addresses with a default user worked (e.g., created a task).
- Addresses without a default user did not work.

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley created this revision.Jan 3 2019, 7:47 PM

Harbormaster completed remote builds in B21478: Diff 47625.Jan 3 2019, 7:48 PM

epriestley requested review of this revision.Jan 3 2019, 7:48 PM

Let me actually test this and annotate it a bit.

epriestley edited the test plan for this revision. (Show Details)Jan 3 2019, 8:58 PM

Fixed some minor call/syntax issues.
Fixed some parsing issues with case sensitivity and object names.
Added a "Default Author" column to the "Application Email" tables.
Fixed an issue where the receive-test workflow mutated receivers and bled into the actual receive workflow. This only affected test workflows.
Made it an error to send to a "bugs@" address which does not have a default author when your "From" address is unrecognized. (Preivously, this continued silently.)
When iterating through the list of receivers, continue to the next receiver if an exception is raised from either canAcceptMail() or receiveMail().

Harbormaster completed remote builds in B21479: Diff 47626.Jan 3 2019, 9:00 PM

epriestley added inline comments.Jan 3 2019, 9:05 PM

src/applications/calendar/mail/PhabricatorCalendarEventMailReceiver.php
16	All of these are because I removed a random `phutil_utf8_strtoupper` that wasn't really very "pure". The code around pattern matching addresses could probably be cleaner than it is, but I'm going to leave that for another day.
src/applications/countdown/mail/PhabricatorCountdownMailReceiver.php
16	Some of these, like this one, were just wrong (`4` vs `1`). This got copy-pasted from a `CMIT123` or `VOTE123` implementation or something, I think.
src/applications/maniphest/mail/ManiphestCreateMailReceiver.php
17–20	This shows up in the details of tasks created via email. It's a one-off particular to Maniphest:
src/applications/metamta/query/PhabricatorMetaMTAActorQuery.php
127	Slightly unclear, I mean "may not always be the right behavior in the future", i.e. this is a product question.
src/applications/metamta/receiver/PhabricatorApplicationMailReceiver.php
75	This is a bare exception, not a "MailProcessingException", since it has nothing to do with the mail. You `bin/remove destroy'd` the user or something.
src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php
75	This was removed about 900 years ago, this callsite was just missed.

epriestley edited the test plan for this revision. (Show Details)Jan 3 2019, 9:05 PM

epriestley added a child revision: D19953: Prevent inbound processing of the "void/placeholder" address and other reserved addresses.Jan 3 2019, 11:59 PM

amckinley accepted this revision.Jan 4 2019, 9:59 PM

amckinley added inline comments.

src/applications/countdown/mail/PhabricatorCountdownMailReceiver.php
16	So this presumably never worked? Not like a lot of users were emailing countdown objects, I suppose.
src/applications/differential/mail/DifferentialCreateMailReceiver.php
54	It's probably not worth changing the existing behavior, but I'm surprised that we always try attachments and the mail body, instead of skipping the body if we found attachments.
src/applications/metamta/applicationpanel/PhabricatorMetaMTAApplicationEmailPanel.php
265–266	Isn't this more like "Setting this will allow anyone who can email your Phabricator instance to create objects", or am I not understanding what this setting does?
src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php
182–184	Shouldn't these happen in the opposite order?
208–210	"This email was sent from an email address not associated with any active Phabricator users"?
src/applications/phame/mail/PhamePostMailReceiver.php
16	I'm only thinking about this because I've read similar changes about 30 times in this diff, but why is it the mail handler's responsibility to turn a callsign into an ID? Shouldn't the application itself have a standardized way of doing this?
src/applications/ponder/mail/PonderAnswerMailReceiver.php
15	Boooooo for not having made this "PNDR".

This revision is now accepted and ready to land.Jan 4 2019, 9:59 PM

epriestley added inline comments.Jan 4 2019, 10:33 PM

src/applications/countdown/mail/PhabricatorCountdownMailReceiver.php
16	Yeah, some of these should never have worked -- Countdown and Slowvote among them. Not terribly surprising that no one has ever reported issues trying to interact with these over email, though.
src/applications/differential/mail/DifferentialCreateMailReceiver.php
54	Agreed that this would be a better behavior.
src/applications/metamta/applicationpanel/PhabricatorMetaMTAApplicationEmailPanel.php
265–266	Your description/understanding is correct, my "send mail to this address" wording isn't very clear. I'll tailor this a bit, although I do want it to be slightly scary for less-technical users setting up `bugs@` and to err on the side of "this is a big access opening". Maybe something like: Setting a default author will allow anyone on the public internet to create objects in Phabricator by sending mail to this address. This isn't exactly true because some intermediate MTA may drop the mail before it hits Phabricator -- and you can configure `alincoln@whitehouse.gov` as an inbound address even if Phabricator does not actually receive mail for that address. Actually, one general issue here is that you maybe can intercept "Reply-All" mail that users are CC'd on... this is a weird mess, I'll file a separate task to fix it.
src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php
182–184	If `receiveMail()` throws, we shouldn't raise a "nothing received this" exception. We don't anyway, but the theory here is "once something has said it's going to accept it, the 'nothing accept this' exception is off the table".
208–210	That's probably more clear.
src/applications/phame/mail/PhamePostMailReceiver.php
16	Yeah, the general structure of this isn't great. Some objects have a mail pattern which isn't a monogram, usually because they don't have a monogram. `ANSR` is one example. `ObjectQuery` can load `M123`, `C123`, etc., but not `ANSR123`. A better approach might be to separate this into two parts: One part identifies which receiver it's for, e.g. `ANSR`. The other part is an object identifier, defaulting to the ID. For every object which exists today, this default would be satisfactory. So instead of `D123+10+abcdef@phabricator.com`, we'd get a reply address like `D+123+10+abcdef@phabricator.com` (one extra `+` after `D` at the beginning). This is like a tiny tiny bit worse for users, maybe? But probably no one cares and these addresses are clearly not human-readable. Then parsing the pieces out would be completely standard. But the whole `ReplyHandler` vs `MailReceiver` tree is kind of not great, and I think they should really be one class. That's just kind of a lot of scope to add. T11934 may be a better opportunity to fix this.
16	We could also use PHIDs, but in theory it's possible for the same object to have multiple addresses or something, where you send to one address to add comments and another address to update the mock or something, I suppose. This will probably never happen.
src/applications/ponder/mail/PonderAnswerMailReceiver.php
15	Ponder is weird and you can reply to questions (`Q123`) and answers separately. Answers don't have a monogram so we get `ANSR`. Ehhh.

epriestley mentioned this in T13234: Application email addresses may shadow user email addresses.Jan 4 2019, 10:39 PM

Actually, one general issue here is that you maybe can intercept "Reply-All" mail that users are CC'd on... this is a weird mess, I'll file a separate task to fix it.

See T13234. This is sort of a refinement/generalization of the reserved list in D19953.