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

@@ -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',

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 "<differential-rev-{$phid}-req@{$domain}>";
@@ -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;

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);
   }
 

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');

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();
 
 }

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;

src/applications/metamta/adapter/test/PhabricatorMailImplementationTestAdapter.php

@@ -0,0 +1,90 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+/**
+ * Mail adapter that doesn't actually send any email, for writing unit tests
+ * against.
+ */
+class PhabricatorMailImplementationTestAdapter
+  extends PhabricatorMailImplementationAdapter {
+
+  private $guts = array();
+  private $config;
+
+  public function __construct(array $config) {
+    $this->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;
+  }
+
+}

src/applications/metamta/adapter/test/__init__.php

@@ -0,0 +1,12 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/metamta/adapter/base');
+
+
+phutil_require_source('PhabricatorMailImplementationTestAdapter.php');

src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php

@@ -35,6 +35,8 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
   protected $nextRetry;
   protected $relatedPHID;
 
+  private $skipSendOnSave;
+
   public function __construct() {
 
     $this->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);
+  }
+
 }

src/applications/metamta/storage/mail/__tests__/PhabricatorMetaMTAMailTestCase.php

@@ -0,0 +1,77 @@
+<?php
+
+/*
+ * Copyright 2011 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+class PhabricatorMetaMTAMailTestCase extends PhabricatorTestCase {
+
+  public function testThreadIDHeaders() {
+    $this->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 = '<somethread-12345@somedomain.tld>';
+
+    $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 = "<message-id = ".($supports_message_id ? 'Y' : 'N').", ".
+            "first = ".($is_first_mail ? 'Y' : 'N').">";
+
+    $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}.");
+  }
+
+}

src/applications/metamta/storage/mail/__tests__/__init__.php

@@ -0,0 +1,16 @@
+<?php
+/**
+ * This file is automatically generated. Lint this module to rebuild it.
+ * @generated
+ */
+
+
+
+phutil_require_module('phabricator', 'applications/metamta/adapter/test');
+phutil_require_module('phabricator', 'applications/metamta/storage/mail');
+phutil_require_module('phabricator', 'infrastructure/testing/testcase');
+
+phutil_require_module('phutil', 'utils');
+
+
+phutil_require_source('PhabricatorMetaMTAMailTestCase.php');