Only send the "this is blank silly" error message email if the email is sent *just* to Phabricator.

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

src/applications/audit/PhabricatorAuditReplyHandler.php

@@ -49,7 +49,7 @@ final class PhabricatorAuditReplyHandler extends PhabricatorMailReplyHandler {
     }
   }
 
-  public function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) {
+  protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) {
     $commit = $this->getMailReceiver();
     $actor = $this->getActor();
 

src/applications/differential/DifferentialReplyHandler.php

@@ -105,7 +105,7 @@ class DifferentialReplyHandler extends PhabricatorMailReplyHandler {
     return $actions;
   }
 
-  public function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) {
+  protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) {
     $this->receivedMail = $mail;
     $this->handleAction($mail->getCleanTextBody());
   }

src/applications/maniphest/ManiphestReplyHandler.php

@@ -50,7 +50,7 @@ final class ManiphestReplyHandler extends PhabricatorMailReplyHandler {
     }
   }
 
-  public function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) {
+  protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) {
 
     // NOTE: We'll drop in here on both the "reply to a task" and "create a
     // new task" workflows! Make sure you test both if you make changes!

src/applications/metamta/replyhandler/PhabricatorMailReplyHandler.php

@@ -45,7 +45,82 @@ abstract class PhabricatorMailReplyHandler {
     PhabricatorObjectHandle $handle);
   abstract public function getReplyHandlerDomain();
   abstract public function getReplyHandlerInstructions();
-  abstract public function receiveEmail(PhabricatorMetaMTAReceivedMail $mail);
+  abstract protected function receiveEmail(
+    PhabricatorMetaMTAReceivedMail $mail);
+
+  public function processEmail(PhabricatorMetaMTAReceivedMail $mail) {
+    $error = $this->sanityCheckEmail($mail);
+
+    if ($error) {
+      if ($this->shouldSendErrorEmail($mail)) {
+        $this->sendErrorEmail($error, $mail);
+      }
+      return null;
+    }
+
+    return $this->receiveEmail($mail);
+  }
+
+  private function sanityCheckEmail(PhabricatorMetaMTAReceivedMail $mail) {
+    $body = $mail->getCleanTextBody();
+    if (empty($body)) {
+      return 'Empty email body. Email should begin with an !action and / or '.
+             'text to comment. Inline replies and signatures are ignored.';
+    }
+
+    return null;
+  }
+
+  /**
+   * Only send an error email if the user is talking to just Phabricator. We
+   * can assume if there is only one To address it is a Phabricator address
+   * since this code is running and everything.
+   */
+  private function shouldSendErrorEmail(PhabricatorMetaMTAReceivedMail $mail) {
+    return count($mail->getToAddresses() == 1) &&
+           count($mail->getCCAddresses() == 0);
+  }
+
+  private function sendErrorEmail($error,
+                                  PhabricatorMetaMTAReceivedMail $mail) {
+    $template = new PhabricatorMetaMTAMail();
+    $template->setSubject('Exception: unable to process your mail request');
+    $template->setBody($this->buildErrorMailBody($error, $mail));
+    $template->setRelatedPHID($mail->getRelatedPHID());
+    $phid = $this->getActor()->getPHID();
+    $tos = array(
+      $phid => PhabricatorObjectHandleData::loadOneHandle($phid)
+    );
+    $mails = $this->multiplexMail($template, $tos, array());
+
+    foreach ($mails as $email) {
+      $email->saveAndSend();
+    }
+
+    return true;
+  }
+
+  private function buildErrorMailBody($error,
+                                      PhabricatorMetaMTAReceivedMail $mail) {
+    $original_body = $mail->getRawTextBody();
+
+    $main_body = <<<EOBODY
+Your request failed because an error was encoutered while processing it:
+
+  ERROR: {$error}
+
+  -- Original Body -------------------------------------------------------------
+
+  {$original_body}
+
+EOBODY;
+
+    $body = new PhabricatorMetaMTAMailBody();
+    $body->addRawSection($main_body);
+    $body->addReplySection($this->getReplyHandlerInstructions());
+
+    return $body->render();
+  }
 
   public function supportsPrivateReplies() {
     return (bool)$this->getReplyHandlerDomain() &&

src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php

@@ -54,6 +54,71 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
     return idx($this->headers, 'subject');
   }
 
+  public function getCCAddresses() {
+    return $this->getRawEmailAddresses(idx($this->headers, 'cc'));
+  }
+
+  public function getToAddresses() {
+    return $this->getRawEmailAddresses(idx($this->headers, 'to'));
+  }
+
+  /**
+   * Parses "to" addresses, looking for a public create email address
+   * first and if not found parsing the "to" address for reply handler
+   * information: receiver name, user id, and hash.
+   */
+  private function getPhabricatorToInformation() {
+    // Only one "public" create address so far
+    $create_task = PhabricatorEnv::getEnvConfig(
+      'metamta.maniphest.public-create-email');
+
+    // For replies, look for an object address with a format like:
+    // D291+291+b0a41ca848d66dcc@example.com
+    $single_handle_prefix = PhabricatorEnv::getEnvConfig(
+      'metamta.single-reply-handler-prefix');
+
+    $prefixPattern = ($single_handle_prefix)
+      ? preg_quote($single_handle_prefix, '/') . '\+'
+      : '';
+    $pattern = "/^{$prefixPattern}((?:D|T|C)\d+)\+([\w]+)\+([a-f0-9]{16})@/U";
+
+    $phabricator_address = null;
+    $receiver_name       = null;
+    $user_id             = null;
+    $hash                = null;
+    foreach ($this->getToAddresses() as $address) {
+      if ($address == $create_task) {
+        $phabricator_address = $address;
+        // it's okay to stop here because we just need to map a create
+        // address to an application and don't need / won't have more
+        // information in these cases.
+        break;
+      }
+
+      $matches = null;
+      $ok = preg_match(
+        $pattern,
+        $address,
+        $matches);
+
+      if ($ok) {
+        $phabricator_address = $address;
+        $receiver_name       = $matches[1];
+        $user_id             = $matches[2];
+        $hash                = $matches[3];
+        break;
+      }
+    }
+
+    return array(
+      $phabricator_address,
+      $receiver_name,
+      $user_id,
+      $hash
+    );
+  }
+
+
   public function processReceivedMail() {
 
     // If Phabricator sent the mail, always drop it immediately. This prevents
@@ -68,11 +133,19 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
       return $this->setMessage($message)->save();
     }
 
-    $to = idx($this->headers, 'to');
+    list($to,
-    $to = $this->getRawEmailAddress($to);
+         $receiver_name,
+         $user_id,
+         $hash) = $this->getPhabricatorToInformation();
+    if (!$to) {
+      $raw_to = idx($this->headers, 'to');
+      return $this->setMessage("Unrecognized 'to' format: {$raw_to}")->save();
+    }
 
     $from = idx($this->headers, 'from');
 
+    // TODO -- make this a switch statement / better if / when we add more
+    // public create email addresses!
     $create_task = PhabricatorEnv::getEnvConfig(
       'metamta.maniphest.public-create-email');
 
@@ -112,7 +185,7 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
       $handler = $editor->buildReplyHandler($receiver);
 
       $handler->setActor($user);
-      $handler->receiveEmail($this);
+      $handler->processEmail($this);
 
       $this->setRelatedPHID($receiver->getPHID());
       $this->setMessage('OK');
@@ -120,30 +193,6 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
       return $this->save();
     }
 
-    // We've already stripped this, so look for an object address which has
-    // a format like: D291+291+b0a41ca848d66dcc@example.com
-    $matches = null;
-    $single_handle_prefix = PhabricatorEnv::getEnvConfig(
-      'metamta.single-reply-handler-prefix');
-
-    $prefixPattern = ($single_handle_prefix)
-      ? preg_quote($single_handle_prefix, '/') . '\+'
-      : '';
-    $pattern = "/^{$prefixPattern}((?:D|T|C)\d+)\+([\w]+)\+([a-f0-9]{16})@/U";
-
-    $ok = preg_match(
-      $pattern,
-      $to,
-      $matches);
-
-    if (!$ok) {
-      return $this->setMessage("Unrecognized 'to' format: {$to}")->save();
-    }
-
-    $receiver_name = $matches[1];
-    $user_id = $matches[2];
-    $hash = $matches[3];
-
     if ($user_id == 'public') {
       if (!PhabricatorEnv::getEnvConfig('metamta.public-replies')) {
         return $this->setMessage("Public replies not enabled.")->save();
@@ -208,7 +257,7 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
     }
 
     $handler->setActor($user);
-    $handler->receiveEmail($this);
+    $handler->processEmail($this);
 
     $this->setMessage('OK');
 
@@ -287,6 +336,14 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
     return $address;
   }
 
+  private function getRawEmailAddresses($addresses) {
+    $raw_addresses = array();
+    foreach (explode(',', $addresses) as $address) {
+      $raw_addresses[] = $this->getRawEmailAddress($address);
+    }
+    return $raw_addresses;
+  }
+
   private function lookupPublicUser() {
     $from = idx($this->headers, 'from');
     $from = $this->getRawEmailAddress($from);

src/applications/owners/OwnersPackageReplyHandler.php

@@ -40,7 +40,7 @@ final class OwnersPackageReplyHandler extends PhabricatorMailReplyHandler {
     return null;
   }
 
-  public function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) {
+  protected function receiveEmail(PhabricatorMetaMTAReceivedMail $mail) {
     return;
   }
 }