Summary: I'm not super happy with the prettiness of the code, but I wasn't able to come up with a good way to clean it up. Happy for suggestions.
Test Plan: sent message to btrahan@phabricator.dev from gmail. Copied raw email and piped it to mail_handler.php -- it created a conpherence! Repeated but sent to btrahan and xerxes and noted that the conpherence was created for both users
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2431
Differential Revision: https://secure.phabricator.com/D4854
Summary: the editor methods are protected so just build a reply handler directly
Test Plan: push it live and try again
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2399
Differential Revision: https://secure.phabricator.com/D4670
Summary: it doesn't work right now. updating this regex should fix it methinks
Test Plan: gotta push it live
Reviewers: chad, epriestley
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2399
Differential Revision: https://secure.phabricator.com/D4663
Summary: Fixes various array_combine() warnings for PHP < 5.4
Test Plan: lint/unit/grep
Reviewers: btrahan, vrana, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4660
Summary: Added a reply handler. A few problems -- first, I can't seem to get this to actually send me email so I haven't been able to reply (which I would have done by generating a reply, then copying the raw email into scripts/mail_handler.php). Second, the subject is often terrible on these emails -- unless the conpherence is named its something gross like "E4:" Third, on create I am noticing an error on array_combine() which I think is related to the need to write array_combine_not_broken or what have you I saw go by... (PhabricatorTransactionEditor does array_combine(xaction->getOldValue(), xaction->getOldValue()) and complains that the arrays are empty)
Test Plan: noted that /mail/ said mails were being sent
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2399
Differential Revision: https://secure.phabricator.com/D4656
Summary: was doing some work in here and noticed this old crap lying around. T547 was last updated in December 2011 so I think its okay to delete these old mail hashes now.
Test Plan: careful code inspection, though I will be testing mail like whoa for the rest of the day probably
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T547
Differential Revision: https://secure.phabricator.com/D4650
Summary:
T2345
getConfig throws an Exception when the key does not exist.
Also removes dead code that throws an Exception.
Test Plan:
Reloaded the Phabricator home page. In the process, found
2 Exceptions thrown due to nonexistent keys. After addressing these problems,
the home page loads without Exceptions.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4541
Summary:
Fixes T2290. Older versions of PHP (prior to PHP 5.4) raised a warning if you tried to combine empty arrays. (Newer versions don't, which is why I missed this in testing, although I may also not have tried sending empty mail.)
If mail has no recipients, we reach this with an empty array. Just skip the function body and return immediately, the result is empty array.
(You can get mail with no recipients in various valid ways, currently by, e.g., commenting on a Macro with no subscribers.)
Test Plan: Sent mail with zero, nonzero recipients. Received the nonzero recipient mail. Verified on php.net that this is a version issue.
Reviewers: codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2290
Differential Revision: https://secure.phabricator.com/D4360
Summary: this can happen if you have Phabricator and email lists co-mingling such that Phabricator receives an email multiple times. we can prevent this from then spamming everyone or otherwise taking the action multiple times by storing a message id hash and dropping the message if we have more than one message that matches.
Test Plan: simulated sending the same email multiple times on the command line. noted only the first one made it through.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1726
Differential Revision: https://secure.phabricator.com/D4328
Summary: array_filter to the rescue
Test Plan: mostly lots of reasoning (as opposed to making a fake user with a blank email address to reproduce)
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2052
Differential Revision: https://secure.phabricator.com/D3927
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Summary:
a few things
- make the parent mailhandler class not send "blank body" error if you have attachments
- make both differential and maniphest append a list of attachments to the body if any exist
- BONUS - made the cc stuff work in Maniphest
Test Plan: I haven't actually tested this yet. :( i need to figure out how to send a mail with an attachment from the command-line and figured I'd serve this up first.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2012
Differential Revision: https://secure.phabricator.com/D3868
Summary:
Currently, when taskmasters complete a task it is immediately deleted. This prevents us from doing some general things, like:
- Supporting the idea of permanent failure (e.g., after N failures just stop trying).
- Showing the user how fast taskmasters are completing tasks.
- Showing the user how long tasks took to complete.
Having better visibility into this is important to Drydock, which builds on the task system. Also, generally buff debug output for task execution.
Test Plan: Ran `bin/phd debug taskmaster`. Ran `bin/phd debug garbage`. Queued some tasks via various systems.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D3852
Summary: I broke this in D3778. We modify `$parameters` and then ignore it in favor of `$params` for the rest of the method. Unit tests work great since they're one level below this.
Test Plan: Verified "Send email about my own actions" behaved correctly.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3796
Summary: Provide a public interface to get all the filtered recipients of an email. The intent is to pass this along to Notifications so it can mark notifications as read if the user is also receiving an email, possibly based on some preference (see T1403). This also simplifies the enormous sendNow() method a little bit.
Test Plan: Added unit tests, and sent a few mails that should cover most/all of these cases. They appeared to produce the correct recipients.
Reviewers: btrahan, vrana, nh
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1403
Differential Revision: https://secure.phabricator.com/D3778
Summary: When we receive an email, figure out if any of the other tos and ccs are users. If they are, pass their phids through the stach as "exclude phids" and exclude them from getting the email.
Test Plan: used the various applications (audit, differential, maniphest) and noted emails were sent as expected.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin, vrana
Maniphest Tasks: T1676
Differential Revision: https://secure.phabricator.com/D3645
Summary: this lets users specify what "name" to use in email addresses
Test Plan: changed the conf setting in my local instance and used phabricator. observed name format changes
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1862
Differential Revision: https://secure.phabricator.com/D3639
Summary: said differently, if the user included another to address or one or more cc's, don't send the error message email.
Test Plan: played around in the metamta test console and verified that blank replies generated the error handler.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1643
Differential Revision: https://secure.phabricator.com/D3345
Summary: Currently, if no placeholder is configured we always move "Cc" up to "To", even if we have a valid "To". Instead, move "Cc" to "To" only if there's no "To" and no placeholder.
Test Plan: Sent email with "to" and "cc", email with "cc" only with a placeholder, and email with "cc" only without a placeholder. Verified recipients ended up in the right location in all cases.
Reviewers: nh, btrahan
Reviewed By: btrahan
CC: klimek, aran
Differential Revision: https://secure.phabricator.com/D3342
Summary:
There's currently no way to get here from the UI since nav tabs don't exist anymore. It's also always been hard to find this feature even when we had the tabs, since it's surprising that it's inside "MetaMTA".
- Move mailing lists to a separate application.
- Add `buildApplicationPage()`, since we don't really need `buildStandardPageResponse()` any more -- we can infer all the information from `PhabricatorApplication`. This will let us get rid of a lot of the `PhabricatorXXXController` classes which just define application information.
- Add `getApplicationURI()` to reduce code duplication, and in case we want to let you move applications around some day.
Test Plan: Looked/edited/saved mailing lists.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T631
Differential Revision: https://secure.phabricator.com/D3248
Summary: See detailed discussion in T1543.
Test Plan:
- Enabled multiplexing.
- Set user A to "enable Re".
- Created a task owned by user B with user A cc'd.
- Verified A got no "Re:" before this patch.
- Applied patch.
- Verified A got "Re:" after this patch.
Reviewers: nh, btrahan, vrana
Reviewed By: nh
CC: aran
Maniphest Tasks: T1543
Differential Revision: https://secure.phabricator.com/D3031
Summary: This keeps people in the correct To or CC field on multiplexed messages.
Test Plan:
with multiplexing on, checked that I received an email with me in the CC
field instead of the To field for a diff I'm CC'd on.
Reviewers: epriestley, jungejason, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2999
Summary:
- Assigning $cc_phids clobbers the correct value assigned on line 80.
- Remove stack trace noise, the trace is always meaningless and well-known.
- Actually show the original body.
Test Plan: Piped mail to the mail receiver and verified the errors didn't CC revision CCs, no longer had traces, and included the original raw text body.
Reviewers: btrahan, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2969
Summary: People want to create filters checking if they are in CC which is almost impossible with multiplexing in Outlook.
Test Plan: Sent e-mail with multiple CCs, verified headers.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2829
Summary:
Works this way:
- Select users' language with multiplexing.
- Select default language otherwise (it can be different from current user's language).
- Build body and subject for each user individually.
- Set the original language after sending the mails.
Test Plan:
- Comment on a diff of user with custom translation.
- Set default to a custom translation. Comment on a diff of user with default translation.
- Set default to a default translation. Comment on a diff of user with default translation.
Repeat with/without multiplexing.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1139
Differential Revision: https://secure.phabricator.com/D2774
Summary:
It seems that Outlook and Mail.app mostly ignores the threading headers and thread primarily by subject.
They are also very picky about the Re: part in the header.
I guess that's because users of these clients often hit Reply when they want to create a new message to the sender of an e-mail.
We need both of these applications to work with the same setting because we don't use multiplexing to prevent sending multiple e-mails to people in lists.
I also believe that the default behavior should just work in most setups.
I've tried several different combinations of putting "Re:" and none of them seems to always work in both clients.
This diff at least adds more abstraction to the code which should prevent copy/paste errors (two fixed by this diff!).
Test Plan: Sent several e-mails with varying subject, verified that they look as before in Outlook and Mail.app.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D2709
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.
NOTE: `arc diff` timed out so I'm pushing it without review.
Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.
Auditors: epriestley
Maniphest Tasks: T1103
Summary:
It's currently possible to configure Phabricator to send mail to some address it recognizes as relating to an object.
When we receive mail from Phabricator, drop it unconditionally.
Test Plan: Wrote two emails, one with the header and one without. Piped them to `mail_handler.php`, one was dropped immediately.
Reviewers: btrahan, nh, mikaaay, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2529
Summary:
Since user emails aren't in the user table, we had to do extra data fetching
for handles, and the emails are only used in MetaMTA, so we move the email
code into MetaMTA and remove it from handles.
Test Plan: send test emails
Reviewers: jungejason, vrana, epriestley
Reviewed By: epriestley
CC: aran, Koolvin
Differential Revision: https://secure.phabricator.com/D2494
Summary:
- Move email to a separate table.
- Migrate existing email to new storage.
- Allow users to add and remove email addresses.
- Allow users to verify email addresses.
- Allow users to change their primary email address.
- Convert all the registration/reset/login code to understand these changes.
- There are a few security considerations here but I think I've addressed them. Principally, it is important to never let a user acquire a verified email address they don't actually own. We ensure this by tightening the scoping of token generation rules to be (user, email) specific.
- This should have essentially zero impact on Facebook, but may require some minor changes in the registration code -- I don't exactly remember how it is set up.
Not included here (next steps):
- Allow configuration to restrict email to certain domains.
- Allow configuration to require validated email.
Test Plan:
This is a fairly extensive, difficult-to-test change.
- From "Email Addresses" interface:
- Added new email (verified email verifications sent).
- Changed primary email (verified old/new notificactions sent).
- Resent verification emails (verified they sent).
- Removed email.
- Tried to add already-owned email.
- Created new users with "accountadmin". Edited existing users with "accountadmin".
- Created new users with "add_user.php".
- Created new users with web interface.
- Clicked welcome email link, verified it verified email.
- Reset password.
- Linked/unlinked oauth accounts.
- Logged in with oauth account.
- Logged in with email.
- Registered with Oauth account.
- Tried to register with OAuth account with duplicate email.
- Verified errors for email verification with bad tokens, etc.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1184
Differential Revision: https://secure.phabricator.com/D2393
Summary: This appears to sometimes be effective (for MS clients), and we've seen it in the wild on inbound mail.
Test Plan: Sent myself some mail, verified it had the right header.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T571
Differential Revision: https://secure.phabricator.com/D2241
Summary:
- Add an explicit multiplexing option, and enable it by default. This is necessary for Mail.app to coexist with other clients ("Re:" breaks outlook at the very least, and generally sucks in the common case), and allows users with flexible clients to enable subject variance.
- Add an option for subject line variance. Default to not varying the subject, so mail no longer says [Committed], [Closed], etc. This is so the defaults thread correctly in Gmail (not entirely sure this actually works).
- Add a preference to enable subject line variance.
- Unless all mail is multiplexed, don't enable or respect the "Re" or "vary subject" preferences. These are currently shown and respected in non-multiplex cases, which creates inconsistent results.
NOTE: @jungejason @nh @vrana This changes the default behavior (from non-multiplexing to multiplexing), and might break Facebook's integration. You should be able to keep the same behavior by setting the options appropriately, although if you can get the new defaults working they're probably better.
Test Plan:
Send mail from Maniphest, Differential and Audit. Updated preferences. Enabled/disabled multiplexing. Things seem OK?
NOTE: I haven't actually been able to repro the Gmail threading issue so I'm not totally sure what's going on there, maybe it started respecting "Re:" (or always has), but @cpiro and @20after4 both reported it independently. This fixes a bunch of bugs in any case and gives us more conservative set of defaults.
I'll see if I can buff out the Gmail story a bit but every client is basically a giant black box of mystery. :/
Reviewers: btrahan, vrana, jungejason, nh
Reviewed By: btrahan
CC: cpiro, 20after4, aran
Maniphest Tasks: T1097, T847
Differential Revision: https://secure.phabricator.com/D2206
Summary: See discussion in T789. Covered the obvious cases, at least. We can refine this as we get a larger sample size.
Test Plan: Unit test coverage.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T789
Differential Revision: https://secure.phabricator.com/D2154
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2091
Summary: Last of the big final patches. Left a few debatable classes (12 out of about 400) that I'll deal with individually eventually.
Test Plan: Ran testEverythingImplemented.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1881
Summary:
This will allow sending mail to be done by task workers. See T750.
Task ID: #
Blame Rev:
Test Plan:
- started taskmaster daemon in test env
- used "send new test message" feature in MetMTA (with send now unchecked)
- confirmed receipt of 1 email
- repeated 2 & 3 with send now checked
Revert Plan:
Tags:
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T388, T750
Differential Revision: https://secure.phabricator.com/D1723
Summary:
See T926. If you want to write a mail rule that, e.g., captures Differential
mail but ignores people replying to it, it's kind of tricky right now. You can
use the 'X-Mail-Transport-Agent' header but that's not obvious and it's not
necessarily stable.
Add a nice, obvious "X-Phabricator-Sent-This-Message" header.
Test Plan: Sent myself some mail, verified the header appeared.
Reviewers: vrana, btrahan, fugalh, jungejason
Reviewed By: jungejason
CC: aran, epriestley
Maniphest Tasks: T926
Differential Revision: https://secure.phabricator.com/D1732
Summary:
When users submit an audit, send email to relevant parties informing them.
Allow email to be replied to. Just basic support so far; no "!raise" stuff and
no threading with the Herald commit notification.
Test Plan: Made comments, got email. Replied to email, got comments.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D1698
Summary:
A few similar requests have come in across several tools and use cases that I
think this does a reasonable job of resolving.
We currently send one email for each update an object receives, but these aren't
always appreciated:
- Asana does post-commit review via Differential, so the "committed" mails are
useless.
- Quora wants to make project category edits to bugs without spamming people
attached to them.
- Some users in general are very sensitive to email volumes, and this gives us
a good way to reduce the volumes without incurring the complexity of
delayed-send-batching.
The technical mechanism is basically:
- Mail may optionally have "mail tags", which indicate content in the mail
(e.g., "maniphest-priority, maniphest-cc, maniphest-comment" for a mail which
contains a priority change, a CC change, and a comment).
- If a mail has tags, remove any recipients who have opted out of all the
tags.
- Some tags can't be opted out of via the UI, so this ensures that important
email is still delivered (e.g., cc + assign + comment is always delivered
because you can't opt out of "assign" or "comment").
Test Plan:
- Disabled all mail tags in the web UI.
- Used test console to send myself mail with an opt-outable tag, it was
immediately dropped.
- Used test console to send myself mail with an opt-outable tag and a custom
tag, it was delivered.
- Made Differential updates affecting CCs with and without comments, got
appropriate delivery.
- Made Maniphest updates affecting project, priority and CCs with and without
comments, got appropriate delivery.
- Verified mail headers in all cases.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, moskov
Maniphest Tasks: T616, T855
Differential Revision: https://secure.phabricator.com/D1635
your own actions
Summary:
- Mail.app on Lion has cumbersome threading rules, see T782. Add an option to
stick "Re: " in front of all threaded mail so it behaves. This is horrible, but
apparently the least-horrible option.
- While I was in there, I added an option for T228.
Test Plan:
- Sent a bunch of threaded and unthreaded mail with varous "Re:" settings,
seemed to get "Re:" in the right places.
- Disabled email about my stuff, created a task with just me, got voided mail,
added a CC, got mail to just the CC.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, mkjones
Maniphest Tasks: T228, T782
Differential Revision: https://secure.phabricator.com/D1448
Summary: There are lots of callsites to $changeset->getFilename() so it seemed
easier to rename getFileName() to getFilename() even if it includes database
change. Plus I think that getFilename() is better.
Test Plan:
Alter database.
Open revision.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D1437
Summary:
- For context, see T547. This is the last (maybe?) in a series of diffs that
moves us off raw sha1() calls in order to make it easier to audit the codebase
for correct use of hash functions.
- This breaks CSRF tokens. Any open forms will generate an error when
submitted, so maybe upgrade off-peak.
- We now generate HMAC mail keys but accept MAC or HMAC. In a few months, we
can remove the MAC version.
- The only remaining callsite is Conduit. We can't use HMAC since Arcanist
would need to know the key. {T550} provides a better solution to this, anyway.
Test Plan:
- Verified CSRF tokens generate properly.
- Manually changed CSRF to an incorrect value and got an error.
- Verified mail generates with a new mail hash.
- Verified Phabricator accepts both old and new mail hashes.
- Verified Phabricator rejects bad mail hashes.
- Checked user log, things look OK.
Reviewers: btrahan, jungejason, benmathews
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T547
Differential Revision: 1237
Summary: See T625. Facebook's REST-based MTA layer had a check for this so I
overlooked it in porting it out. We should not attempt to deliver email to
disabled users.
Test Plan:
Used MetaMTA console to send email to:
- No users: received "no To" exception.
- A disabled user: received "all To disabled" exception.
- A valid user: received email.
- A valid user and a disabled user: received email to valid user only.
(Note that you can't easily send to disabled users directly since they don't
appear in the typeahead, but you can prefill it and then disable the user by
hitting "Send".)
Reviewers: btrahan, jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: skrul, aran, epriestley
Differential Revision: 1120
Summary: Allow tweaking Differential mail before sending.
Test Plan:
Wrote a listener renaming Differential attachments and it worked without
problems.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, mareksapota, davidreuss
Differential Revision: 1091
Summary: This should hopefully kill off the last of these :P
Test Plan: Should be self explanatory
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 1043
Test Plan:
Used the scripts/mail_handler.php with and without patch and saw
the maniphest task being created with patch applied.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, davidreuss
Differential Revision: 1041
transactional mail
Summary: See T571. SES refuses to deliver mail with this header and there are
various reports of other issues on the internet so I'm defaulting it to off.
Test Plan: Set config to true, tried to send mail, SES rejected it because of
"Precedence: bulk" header.
Reviewers: bmaurer, ola, jungejason, nh, aran
Reviewed By: aran
CC: aran, epriestley, bmaurer
Differential Revision: 1032
Summary: Allow configuration of a default author for bugs@ emails which don't
correspond to a known system user.
Test Plan: Configured a default author, sent some mails from nonsense addresses,
tasks were created.
Reviewers: davidreuss, jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, epriestley, ide
Differential Revision: 1013
Test Plan:
Set 'metamta.send-immediately' to true. Start up several MTA daemons, without
the patch you'll probably get multiple emails, with the patch you should get
only one.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, mareksapota, epriestley
Differential Revision: 1021
Summary: Quora wants to handle some moderation tasks with Phabricator, but want
to lower the barrier to entry for the install and let moderators adopt it
gradually. One request is to allow auth rules to be relaxed so we can auth based
on Reply-To to make things easier. This is insecure if configured but not really
a big deal and the patch isn't big or complicated.
Test Plan: Sent a test email with bogus "From" but valid "Reply-To". It was
rejected with this setting off, and allowed with this setting on.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 842
Summary:
This allows you to configure a single mailbox for all mail sent by phabricator,
so you
can keep a mailaddress like bugs@example.com and don't need a catchall on your
domain/subdomain.
Test Plan:
Enabled and disabled suffix. Saw mails generated have to correct prefix. Also
piped raw mails
into the scripts/mail/mail_handler.php and ensured comments went into
phabricator for both maniphest
and differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 815
Summary:
@skrul reports receiving multiple copies of notification emails since
@hunterbridges configured some bizarre dystopian email replication factory on
their outbound route. Two fixes:
- Ensure "To" and "Cc" are unique. Email shouldn't be replicated for "To:
x@y.com, x@y.com" but it's silly that we do this.
- Remove "To" addresses from "Cc". Email shouldn't be replicated here either,
but we don't really lose anything by accommodating this.
Test Plan:
Sent a mail to the same to/cc, verified I was to'd only and not cc'd when the
mail was delivered.
@hunterbridges, can you apply this patch locally and verify it fixes the issue?
You can test by going to MetaMTA -> Send New Message and sending a message to
yourself as both To and CC.
Reviewed By: skrul
Reviewers: skrul, hunterbridges, jungejason, tuomaspelkonen, aran
CC: aran, skrul, epriestley
Differential Revision: 751
Summary: This lets you configure an email address which will create tasks when
emails are sent to it. It's pretty basic but should get us most of the way
there.
Test Plan: Configured an address and created a task via email. Replied to a task
via email to check that I didn't break that.
Reviewed By: tuomaspelkonen
Reviewers: davidreuss, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 590
Summary:
We already support this (and Facebook uses it) but it is difficult to configure
and you have to write a bunch of code. Instead, provide a simple flag.
See the documentation changes for details, but when this flag is enabled we send
one email with a reply-to like "D2+public+23hf91fh19fh@phabricator.example.com".
Anyone can reply to this, and we figure out who they are based on their "From"
address instead of a unique hash. This is less secure, but a reasonable tradeoff
in many cases.
This also has the advantage over a naive implementation of at least doing object
hash validation.
@jungejason: I don't think this affects Facebook's implementation but this is an
area where we've had problems in the past, so watch out for it when you deploy.
Also note that you must set "metamta.public-replies" to true since Maniphest now
looks for that key specifically before going into public reply mode; it no
longer just tests for a public reply address being generateable (since it can
always generate one now).
Test Plan:
Swapped my local install in and out of public reply mode and commented on
objects. Got expected email behavior. Replied to public and private email
addresses.
Attacked public addresses by using them when the install was configured to
disallow them and by altering the hash and the from address. All this stuff was
rejected.
Reviewed By: jungejason
Reviewers: moskov, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, moskov, jungejason
Differential Revision: 563
Summary:
See T251, where gregprice correctly argues that we need both:
None of the other people on the thread will have seen that message, so it
seems
like a lot of clients would put the server's message in a new thread. In
general, I think you want the References: header to mention every ancestor
message in the thread that you know about, because that's how MUAs keep a
thread
together in the face of missing some of its messages.
Test Plan:
Sent a reply email locally, got a response with both Message-IDs in
"references".
Reviewed By: rm
Reviewers: gregprice, rm
Commenters: gregprice
CC: aran, gregprice, epriestley, rm
Differential Revision: 499
Summary:
See T251. In Gmail, conversations split if you reply to them and the next email
does not "In-Reply-To" your message ID. When an action is triggered by an email,
carry its Message-ID through the stack and use it for "In-Reply-To" and
"References" on the subsequent message.
Test Plan:
Live-patched phabricator.com and replied to a Maniphest thread in Gmail without
disrupting the thread. Locally replied to Maniphest and Differential threads and
verified Message-ID was carried across the reply boundary.
Reviewed By: rm
Reviewers: tcook, jungejason, aran, tuomaspelkonen, rm
CC: aran, epriestley, rm
Differential Revision: 498
identifier
Summary:
Move the parser to a separate class so it can be easily unit tested, add some
tests. Properly parse emails with linebreaks in the quote line.
Test Plan:
Ran unit tests, used mail receiver to reply to an object.
Reviewed By: cadamo
Reviewers: aran, jungejason, tuomaspelkonen, cadamo
CC: aran, cadamo, epriestley
Differential Revision: 392
Summary:
Sendmail is seriously difficult to configure; SendGrid is extremely easy. It's
also pretty expensive ($80/mo) but there are a bunch of startups that already
have plans so it's effectively free for them.
Test Plan:
Configured SendGrid and sent reply email through it.
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran, epriestley
Differential Revision: 376
Summary:
While my client and some others send email replies with an address like
##T1+x+y@example.com##, some other clients have sent either
##<T1+x+y@example.com>## or ##"T1+x+y@example.com" <T1+x+y@example.com>##.
Properly parse all the formats we've seen in the wild.
Test Plan:
Ran the regexp against all the formats observed in the wild (see
https://secure.phabricator.com/mail/received/) and verified it parses them
correctly.
Reviewed By: jungejason
Reviewers: tuomaspelkonen, jungejason, aran
CC: anjali, aran, jungejason
Differential Revision: 370
Summary:
There's an undoubtedly-far-more-refined version of this in xmail if someone
wants to crib it for me. Otherwise we can anneal this as counterexamples arise.
This seems to be what mail.app and gmail do.
Test Plan:
Used mail receiver console to "send" some mail and verified it was correctly
truncated.
Reviewed By: jungejason
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, jungejason
Differential Revision: 290
Phabricator
Summary:
Hook up the last pieces. This shouldn't impact the Facebook install, EXCEPT that
I removed "!accept" and added "!rethink" (plan changes). If you want to continue
supporting !accept, you should override the method in your subclass if you don't
already.
Test Plan:
Used the Mail Receiver test console to send mail to tasks and revisions.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 289
Summary:
Provide an "isAdmin" flag for users, to designate administrative users.
Restore the account editing interface and allow it to set role flags and reset
passwords.
Provide an "isDisabled" flag for users and shut down all system access for them.
Test Plan:
Created "admin" and "disabled" users. Did administrative things with the admin
user. Tried to do stuff with the disabled user and was rebuffed. Tried to access
administrative interfaces with a normal non-admin user and was denied.
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: ccheever, aran
Differential Revision: 278
Summary:
When we multiplex email, add information to the body with an explicit list of
recipients. Also add some headers if people want to write mail rules.
Test Plan:
Commented on a task and a revision, got reasonable looking emails about them.
Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, jungejason, epriestley
Differential Revision: 272
Summary:
Provide a base PhabricatorMailReplyHandler class which handles the plumbing for
multiplexing email if necessary and supporting public and private reply handler
addressses. DifferentialReplyHandler now extends it, and a new
ManiphestReplyHandler also does.
The general approach here is that we have three supported cases:
- no reply handler, default config, same as what we're doing now
- public reply handler, requires overriding classes but just sets "reply-to"
to some address the install generates and still sends only one email
- private reply handler, provides a default generation mechanism or you can
override it and splits mail apart so we send one to each recipient
Test Plan:
Sent email from Maniphest and Differential with and without
reply-handler-domains set.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley
Differential Revision: 254
Summary:
When I tested this earlier I was incorrectly interpreting PHPMailer errors as
SES errors. This works fine as long as you get around the peculiarities of
PHPMailer.
Test Plan:
Sent email to myself, received email from a human-readable address in my mail
client.
Reviewed By: rm
Reviewers: rm, tuomaspelkonen, jungejason, aran
CC: aran, rm, epriestley
Differential Revision: 246
Summary:
Provides support for per-user x per-object unique reply-to email addresses, plus
SMTP integration.
This does not actually make Phabricator use these in outbound email.
Test Plan:
Used test console to validate in-Phabricator routing and handling.
Piped emails into the "mail_handler.php" script to validate mail parsing.
Configured sendmail and sent mail to Phabricator.
Technically I haven't conducted all parts of this test on the same machine since
I lost the will to configure more SMTP servers after configuring phabricator.com
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 226
Summary:
I pretty shortsightedly made sending a side effect of save() in the case that a
server is configured for immediate sending. Move this out, make it explicit, and
get rid of all the tangles surrounding it.
The web tool now ignores the server setting and only repsects the checkbox,
which makes far more sense.
Test Plan:
Sent mails from Maniphest, Differential, and the web console. Also ran all the
unit tests. Verified headers from Maniphest.
Reviewed By: rm
Reviewers: aran, rm
CC: tuomaspelkonen, rm, jungejason, aran
Differential Revision: 200
Summary:
Amazon SES does not allow us to set a Message-ID header, which means
that threads are incorrect in Mail.app (and presumably other applications
which respect In-Reply-To and References) because the initial email does not
have anything which attaches it to the rest of the thread. To fix this, never
rely on Message-ID if the mailer doesn't support Message-ID.
(In the Amazon SES case, Amazon generates its own Message-ID which we can't
know ahead of time).
I additionally used all the Lisk isolation from the other tests to make this
testable and wrote tests for it.
I also moved the idea of a thread ID lower in the stack and out of
DifferentialMail, which should not be responsible for implementation details.
NOTE: If you push this, it will cause a one-time break of threading for
everyone using Outlook since I've changed the seed for generating Thread-Index.
I feel like this is okay to avoid introducing more complexity here.
Test Plan:
Created and then updated a revision, messages delivered over Amazon
SES threaded correctly in Mail.app. Verified headers. Unit tests.
Reviewed By: rm
Reviewers: aran, tuomaspelkonen, jungejason, rm
Commenters: aran
CC: aran, rm, epriestley
Differential Revision: 195
Summary:
add a constants module
src/applications/phid/constants/PhabricatorPHIDConstants.
Test Plan:
Execute applications which were using the hard-coded string.
Differential Revision: 44
Reviewed By: epriestley
Reviewers: epriestley
CC: epriestley
Summary: Amazon SES seems to be working well, except that it takes more than a
second to send mail in-process. Kick it out of process. (Between this and the
ImplementationAdapter layer, MetaMTA almost makes sense. :/)
Test Plan: Ran the daemon and got a flood of unsent test email.
Reviewers:
CC:
