From 72e33c9e5a53eccd58bf516e40a816929c8b7d6b Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 30 Apr 2011 11:47:00 -0700 Subject: [PATCH] Fix a threading issue with Amazon SES 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 --- src/__phutil_library_map__.php | 4 + .../mail/base/DifferentialMail.php | 46 ++-------- ...atorMailImplementationAmazonSESAdapter.php | 7 +- .../metamta/adapter/amazonses/__init__.php | 1 + .../PhabricatorMailImplementationAdapter.php | 6 ++ ...MailImplementationPHPMailerLiteAdapter.php | 4 + ...abricatorMailImplementationTestAdapter.php | 90 +++++++++++++++++++ .../metamta/adapter/test/__init__.php | 12 +++ .../storage/mail/PhabricatorMetaMTAMail.php | 79 +++++++++++++++- .../PhabricatorMetaMTAMailTestCase.php | 77 ++++++++++++++++ .../storage/mail/__tests__/__init__.php | 16 ++++ 11 files changed, 298 insertions(+), 44 deletions(-) create mode 100644 src/applications/metamta/adapter/test/PhabricatorMailImplementationTestAdapter.php create mode 100644 src/applications/metamta/adapter/test/__init__.php create mode 100644 src/applications/metamta/storage/mail/__tests__/PhabricatorMetaMTAMailTestCase.php create mode 100644 src/applications/metamta/storage/mail/__tests__/__init__.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 05d4d64f4d..fadafe5e4e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -315,11 +315,13 @@ phutil_register_library_map(array( 'PhabricatorMailImplementationAdapter' => 'applications/metamta/adapter/base', 'PhabricatorMailImplementationAmazonSESAdapter' => 'applications/metamta/adapter/amazonses', 'PhabricatorMailImplementationPHPMailerLiteAdapter' => 'applications/metamta/adapter/phpmailerlite', + 'PhabricatorMailImplementationTestAdapter' => 'applications/metamta/adapter/test', 'PhabricatorMetaMTAController' => 'applications/metamta/controller/base', 'PhabricatorMetaMTADAO' => 'applications/metamta/storage/base', 'PhabricatorMetaMTADaemon' => 'applications/metamta/daemon/mta', 'PhabricatorMetaMTAListController' => 'applications/metamta/controller/list', 'PhabricatorMetaMTAMail' => 'applications/metamta/storage/mail', + 'PhabricatorMetaMTAMailTestCase' => 'applications/metamta/storage/mail/__tests__', 'PhabricatorMetaMTAMailingList' => 'applications/metamta/storage/mailinglist', 'PhabricatorMetaMTAMailingListEditController' => 'applications/metamta/controller/mailinglistedit', 'PhabricatorMetaMTAMailingListsController' => 'applications/metamta/controller/mailinglists', @@ -711,11 +713,13 @@ phutil_register_library_map(array( 'PhabricatorLogoutController' => 'PhabricatorAuthController', 'PhabricatorMailImplementationAmazonSESAdapter' => 'PhabricatorMailImplementationPHPMailerLiteAdapter', 'PhabricatorMailImplementationPHPMailerLiteAdapter' => 'PhabricatorMailImplementationAdapter', + 'PhabricatorMailImplementationTestAdapter' => 'PhabricatorMailImplementationAdapter', 'PhabricatorMetaMTAController' => 'PhabricatorController', 'PhabricatorMetaMTADAO' => 'PhabricatorLiskDAO', 'PhabricatorMetaMTADaemon' => 'PhabricatorDaemon', 'PhabricatorMetaMTAListController' => 'PhabricatorMetaMTAController', 'PhabricatorMetaMTAMail' => 'PhabricatorMetaMTADAO', + 'PhabricatorMetaMTAMailTestCase' => 'PhabricatorTestCase', 'PhabricatorMetaMTAMailingList' => 'PhabricatorMetaMTADAO', 'PhabricatorMetaMTAMailingListEditController' => 'PhabricatorMetaMTAController', 'PhabricatorMetaMTAMailingListsController' => 'PhabricatorMetaMTAController', diff --git a/src/applications/differential/mail/base/DifferentialMail.php b/src/applications/differential/mail/base/DifferentialMail.php index a8a680867f..4f1e98d073 100644 --- a/src/applications/differential/mail/base/DifferentialMail.php +++ b/src/applications/differential/mail/base/DifferentialMail.php @@ -65,8 +65,6 @@ abstract class DifferentialMail { throw new Exception('No "To:" users provided!'); } - $message_id = $this->getMessageID(); - $cc_phids = $this->getCCPHIDs(); $subject = $this->buildSubject(); $body = $this->buildBody(); @@ -91,15 +89,11 @@ abstract class DifferentialMail { ->setSubject($subject) ->setBody($body) ->setIsHTML($this->shouldMarkMailAsHTML()) - ->addHeader('Thread-Topic', $this->getRevision()->getTitle()) - ->addHeader('Thread-Index', $this->generateThreadIndex()); + ->addHeader('Thread-Topic', $this->getRevision()->getTitle()); - if ($this->isFirstMailAboutRevision()) { - $mail->addHeader('Message-ID', $message_id); - } else { - $mail->addHeader('In-Reply-To', $message_id); - $mail->addHeader('References', $message_id); - } + $mail->setThreadID( + $this->getThreadID(), + $this->isFirstMailAboutRevision()); if ($this->heraldRulesHeader) { $mail->addHeader('X-Herald-Rules', $this->heraldRulesHeader); @@ -218,7 +212,7 @@ EOTEXT; return $this->revision; } - protected function getMessageID() { + protected function getThreadID() { $phid = $this->getRevision()->getPHID(); $domain = PhabricatorEnv::getEnvConfig('metamta.domain'); return ""; @@ -278,36 +272,6 @@ EOTEXT; return $this->isFirstMailAboutRevision; } - protected function generateThreadIndex() { - // When threading, Outlook ignores the 'References' and 'In-Reply-To' - // headers that most clients use. Instead, it uses a custom 'Thread-Index' - // header. The format of this header is something like this (from - // camel-exchange-folder.c in Evolution Exchange): - - /* A new post to a folder gets a 27-byte-long thread index. (The value - * is apparently unique but meaningless.) Each reply to a post gets a - * 32-byte-long thread index whose first 27 bytes are the same as the - * parent's thread index. Each reply to any of those gets a - * 37-byte-long thread index, etc. The Thread-Index header contains a - * base64 representation of this value. - */ - - // The specific implementation uses a 27-byte header for the first email - // a recipient receives, and a random 5-byte suffix (32 bytes total) - // thereafter. This means that all the replies are (incorrectly) siblings, - // but it would be very difficult to keep track of the entire tree and this - // gets us reasonable client behavior. - - $base = substr(md5($this->getRevision()->getPHID()), 0, 27); - if (!$this->isFirstMailAboutRevision()) { - // not totally sure, but it seems like outlook orders replies by - // thread-index rather than timestamp, so to get these to show up in the - // right order we use the time as the last 4 bytes. - $base .= ' ' . pack("N", time()); - } - return base64_encode($base); - } - public function setHeraldTranscriptURI($herald_transcript_uri) { $this->heraldTranscriptURI = $herald_transcript_uri; return $this; diff --git a/src/applications/metamta/adapter/amazonses/PhabricatorMailImplementationAmazonSESAdapter.php b/src/applications/metamta/adapter/amazonses/PhabricatorMailImplementationAmazonSESAdapter.php index d7b964175b..6ee8395566 100644 --- a/src/applications/metamta/adapter/amazonses/PhabricatorMailImplementationAmazonSESAdapter.php +++ b/src/applications/metamta/adapter/amazonses/PhabricatorMailImplementationAmazonSESAdapter.php @@ -28,6 +28,11 @@ class PhabricatorMailImplementationAmazonSESAdapter $this->mailer->customMailer = $this; } + public function supportsMessageIDHeader() { + // Amazon SES will ignore any Message-ID we provide. + return false; + } + public function executeSend($body) { $key = PhabricatorEnv::getEnvConfig('amazon-ses.access-key'); $secret = PhabricatorEnv::getEnvConfig('amazon-ses.secret-key'); @@ -36,7 +41,7 @@ class PhabricatorMailImplementationAmazonSESAdapter $root = dirname($root); require_once $root.'/externals/amazon-ses/ses.php'; - $service = new SimpleEmailService($key, $secret); + $service = newv('SimpleEmailService', array($key, $secret)); return $service->sendRawEmail($body); } diff --git a/src/applications/metamta/adapter/amazonses/__init__.php b/src/applications/metamta/adapter/amazonses/__init__.php index 5e546c1a3e..884af1e1c5 100644 --- a/src/applications/metamta/adapter/amazonses/__init__.php +++ b/src/applications/metamta/adapter/amazonses/__init__.php @@ -10,6 +10,7 @@ phutil_require_module('phabricator', 'applications/metamta/adapter/phpmailerlite phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phutil', 'moduleutils'); +phutil_require_module('phutil', 'utils'); phutil_require_source('PhabricatorMailImplementationAmazonSESAdapter.php'); diff --git a/src/applications/metamta/adapter/base/PhabricatorMailImplementationAdapter.php b/src/applications/metamta/adapter/base/PhabricatorMailImplementationAdapter.php index 10746a01c3..e00e99b2e2 100644 --- a/src/applications/metamta/adapter/base/PhabricatorMailImplementationAdapter.php +++ b/src/applications/metamta/adapter/base/PhabricatorMailImplementationAdapter.php @@ -27,6 +27,12 @@ abstract class PhabricatorMailImplementationAdapter { abstract public function setSubject($subject); abstract public function setIsHTML($is_html); + /** + * Some mailers, notably Amazon SES, do not support us setting a specific + * Message-ID header. + */ + abstract public function supportsMessageIDHeader(); + abstract public function send(); } diff --git a/src/applications/metamta/adapter/phpmailerlite/PhabricatorMailImplementationPHPMailerLiteAdapter.php b/src/applications/metamta/adapter/phpmailerlite/PhabricatorMailImplementationPHPMailerLiteAdapter.php index af131b28fb..c17ca045e2 100644 --- a/src/applications/metamta/adapter/phpmailerlite/PhabricatorMailImplementationPHPMailerLiteAdapter.php +++ b/src/applications/metamta/adapter/phpmailerlite/PhabricatorMailImplementationPHPMailerLiteAdapter.php @@ -26,6 +26,10 @@ class PhabricatorMailImplementationPHPMailerLiteAdapter $this->mailer = newv('PHPMailerLite', array($use_exceptions = true)); } + public function supportsMessageIDHeader() { + return true; + } + public function setFrom($email) { $this->mailer->SetFrom($email, '', $crazy_side_effects = false); return $this; diff --git a/src/applications/metamta/adapter/test/PhabricatorMailImplementationTestAdapter.php b/src/applications/metamta/adapter/test/PhabricatorMailImplementationTestAdapter.php new file mode 100644 index 0000000000..86c3518570 --- /dev/null +++ b/src/applications/metamta/adapter/test/PhabricatorMailImplementationTestAdapter.php @@ -0,0 +1,90 @@ +config = $config; + } + + public function setFrom($email) { + $this->guts['from'] = $email; + return $this; + } + + public function addReplyTo($email) { + $this->guts['reply-to'] = $email; + return $this; + } + + public function addTos(array $emails) { + foreach ($emails as $email) { + $this->guts['tos'][] = $email; + } + return $this; + } + + public function addCCs(array $emails) { + foreach ($emails as $email) { + $this->guts['ccs'][] = $email; + } + return $this; + } + + public function addHeader($header_name, $header_value) { + $this->guts['headers'][] = array($header_name, $header_value); + return $this; + } + + public function setBody($body) { + $this->guts['body'] = $body; + return $this; + } + + public function setSubject($subject) { + $this->guts['subject'] = $subject; + return $this; + } + + public function setIsHTML($is_html) { + $this->guts['is-html'] = $is_html; + return $this; + } + + public function supportsMessageIDHeader() { + return $this->config['supportsMessageIDHeader']; + } + + public function send() { + $this->guts['did-send'] = true; + return true; + } + + public function getGuts() { + return $this->guts; + } + +} diff --git a/src/applications/metamta/adapter/test/__init__.php b/src/applications/metamta/adapter/test/__init__.php new file mode 100644 index 0000000000..513e48bb6d --- /dev/null +++ b/src/applications/metamta/adapter/test/__init__.php @@ -0,0 +1,12 @@ +status = self::STATUS_QUEUE; @@ -115,9 +117,26 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { return $this; } + /** + * Use this method to set an ID used for message threading. MetaMTA will + * set appropriate headers (Message-ID, In-Reply-To, References and + * Thread-Index) based on the capabilities of the underlying mailer. + * + * @param string Unique identifier, appropriate for use in a Message-ID, + * In-Reply-To or References headers. + * @param bool If true, indicates this is the first message in the thread. + * @return this + */ + public function setThreadID($thread_id, $is_first_message = false) { + $this->setParam('thread-id', $thread_id); + $this->setParam('is-first-message', $is_first_message); + return $this; + } + public function save() { $try_send = (PhabricatorEnv::getEnvConfig('metamta.send-immediately')) && - (!$this->getID()); + (!$this->getID()) && + (!$this->skipSendOnSave); $ret = parent::save(); @@ -128,12 +147,22 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { return $ret; } - private function buildDefaultMailer() { + public function buildDefaultMailer() { $class_name = PhabricatorEnv::getEnvConfig('metamta.mail-adapter'); PhutilSymbolLoader::loadClass($class_name); return newv($class_name, array()); } + /** + * Attempt to deliver an email immediately, in this process. + * + * @param bool Try to deliver this email even if it has already been + * delivered or is in backoff after a failed delivery attempt. + * @param PhabricatorMailImplementationAdapter Use a specific mail adapter, + * instead of the default. + * + * @return void + */ public function sendNow( $force_send = false, PhabricatorMailImplementationAdapter $mailer = null) { @@ -152,6 +181,8 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { } } + $this->skipSendOnSave = true; + try { $parameters = $this->parameters; $phids = array(); @@ -186,6 +217,9 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { unset($params['from']); } + $is_first = !empty($params['is-first-message']); + unset($params['is-first-message']); + foreach ($params as $key => $value) { switch ($key) { case 'from': @@ -224,6 +258,16 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { $mailer->setIsHTML(true); } break; + case 'thread-id': + if ($is_first && $mailer->supportsMessageIDHeader()) { + $mailer->addHeader('Message-ID', $value); + } else { + $mailer->addHeader('In-Reply-To', $value); + $mailer->addHeader('References', $value); + } + $thread_index = $this->generateThreadIndex($value, $is_first); + $mailer->addHeader('Thread-Index', $thread_index); + break; default: // Just discard. } @@ -277,4 +321,35 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { return idx($readable, $status_code, $status_code); } + private function generateThreadIndex($seed, $is_first_mail) { + // When threading, Outlook ignores the 'References' and 'In-Reply-To' + // headers that most clients use. Instead, it uses a custom 'Thread-Index' + // header. The format of this header is something like this (from + // camel-exchange-folder.c in Evolution Exchange): + + /* A new post to a folder gets a 27-byte-long thread index. (The value + * is apparently unique but meaningless.) Each reply to a post gets a + * 32-byte-long thread index whose first 27 bytes are the same as the + * parent's thread index. Each reply to any of those gets a + * 37-byte-long thread index, etc. The Thread-Index header contains a + * base64 representation of this value. + */ + + // The specific implementation uses a 27-byte header for the first email + // a recipient receives, and a random 5-byte suffix (32 bytes total) + // thereafter. This means that all the replies are (incorrectly) siblings, + // but it would be very difficult to keep track of the entire tree and this + // gets us reasonable client behavior. + + $base = substr(md5($seed), 0, 27); + if (!$is_first_mail) { + // Not totally sure, but it seems like outlook orders replies by + // thread-index rather than timestamp, so to get these to show up in the + // right order we use the time as the last 4 bytes. + $base .= ' '.pack('N', time()); + } + + return base64_encode($base); + } + } diff --git a/src/applications/metamta/storage/mail/__tests__/PhabricatorMetaMTAMailTestCase.php b/src/applications/metamta/storage/mail/__tests__/PhabricatorMetaMTAMailTestCase.php new file mode 100644 index 0000000000..54bcc2c625 --- /dev/null +++ b/src/applications/metamta/storage/mail/__tests__/PhabricatorMetaMTAMailTestCase.php @@ -0,0 +1,77 @@ +runThreadIDHeadersWithConfiguration(true, true); + $this->runThreadIDHeadersWithConfiguration(true, false); + $this->runThreadIDHeadersWithConfiguration(false, true); + $this->runThreadIDHeadersWithConfiguration(false, false); + } + + private function runThreadIDHeadersWithConfiguration( + $supports_message_id, + $is_first_mail) { + + $mailer = new PhabricatorMailImplementationTestAdapter( + array( + 'supportsMessageIDHeader' => $supports_message_id, + )); + + $thread_id = ''; + + $mail = new PhabricatorMetaMTAMail(); + $mail->setThreadID($thread_id, $is_first_mail); + $mail->sendNow($force = true, $mailer); + + $guts = $mailer->getGuts(); + $dict = ipull($guts['headers'], 1, 0); + + if ($is_first_mail && $supports_message_id) { + $expect_message_id = true; + $expect_in_reply_to = false; + $expect_references = false; + } else { + $expect_message_id = false; + $expect_in_reply_to = true; + $expect_references = true; + } + + $case = ""; + + $this->assertEqual( + true, + isset($dict['Thread-Index']), + "Expect Thread-Index header for case {$case}."); + $this->assertEqual( + $expect_message_id, + isset($dict['Message-ID']), + "Expectation about existence of Message-ID header for case {$case}."); + $this->assertEqual( + $expect_in_reply_to, + isset($dict['In-Reply-To']), + "Expectation about existence of In-Reply-To header for case {$case}."); + $this->assertEqual( + $expect_references, + isset($dict['References']), + "Expectation about existence of References header for case {$case}."); + } + +} diff --git a/src/applications/metamta/storage/mail/__tests__/__init__.php b/src/applications/metamta/storage/mail/__tests__/__init__.php new file mode 100644 index 0000000000..b55a708549 --- /dev/null +++ b/src/applications/metamta/storage/mail/__tests__/__init__.php @@ -0,0 +1,16 @@ +