Move sender validation into MailReceiver classes

Summary:
Ref T1205. Finally able to delete a big chunk of this nastiness.

Make MailReceivers responsible for validating senders. For object creation receivers (bugs, conpherences) this just means that users must not be disabled. For other receivers the senders must be able to see the objects, have the right hashes, etc., according to policy.

Test Plan: Added a bunch of test cases (everything except policy). Verified behavior via the Receive test console.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1205

Differential Revision: https://secure.phabricator.com/D5943

src/__phutil_library_map__.php

@@ -1185,6 +1185,7 @@ phutil_register_library_map(array(
     'PhabricatorObjectItemView' => 'view/layout/PhabricatorObjectItemView.php',
     'PhabricatorObjectListView' => 'view/control/PhabricatorObjectListView.php',
     'PhabricatorObjectMailReceiver' => 'applications/metamta/receiver/PhabricatorObjectMailReceiver.php',
+    'PhabricatorObjectMailReceiverTestCase' => 'applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php',
     'PhabricatorObjectSelectorDialog' => 'view/control/PhabricatorObjectSelectorDialog.php',
     'PhabricatorOffsetPagedQuery' => 'infrastructure/query/PhabricatorOffsetPagedQuery.php',
     'PhabricatorOwnerPathQuery' => 'applications/owners/query/PhabricatorOwnerPathQuery.php',
@@ -2920,6 +2921,7 @@ phutil_register_library_map(array(
     'PhabricatorObjectItemView' => 'AphrontTagView',
     'PhabricatorObjectListView' => 'AphrontView',
     'PhabricatorObjectMailReceiver' => 'PhabricatorMailReceiver',
+    'PhabricatorObjectMailReceiverTestCase' => 'PhabricatorTestCase',
     'PhabricatorOffsetPagedQuery' => 'PhabricatorQuery',
     'PhabricatorOwnersConfigOptions' => 'PhabricatorApplicationConfigOptions',
     'PhabricatorOwnersController' => 'PhabricatorController',

src/applications/maniphest/lipsum/PhabricatorManiphestTaskTestDataGenerator.php

@@ -72,7 +72,10 @@ final class PhabricatorManiphestTaskTestDataGenerator
   public function getProjectPHIDs() {
     $projects = array();
     for ($i = 0; $i < rand(1, 4);$i++) {
-      $projects[] = $this->loadOneRandom("PhabricatorProject")->getPHID();
+      $project = $this->loadOneRandom("PhabricatorProject");
+      if ($project) {
+        $projects[] = $project->getPHID();
+      }
     }
     return $projects;
   }

src/applications/metamta/constants/MetaMTAReceivedMailStatus.php

@@ -8,5 +8,11 @@ final class MetaMTAReceivedMailStatus
   const STATUS_NO_RECEIVERS         = 'err:no-receivers';
   const STATUS_ABUNDANT_RECEIVERS   = 'err:multiple-receivers';
   const STATUS_UNKNOWN_SENDER       = 'err:unknown-sender';
+  const STATUS_DISABLED_SENDER      = 'err:disabled-sender';
+  const STATUS_NO_PUBLIC_MAIL       = 'err:no-public-mail';
+  const STATUS_USER_MISMATCH        = 'err:bad-user';
+  const STATUS_POLICY_PROBLEM       = 'err:policy';
+  const STATUS_NO_SUCH_OBJECT       = 'err:not-found';
+  const STATUS_HASH_MISMATCH        = 'err:bad-hash';
 
 }

src/applications/metamta/receiver/PhabricatorMailReceiver.php

@@ -6,6 +6,22 @@ abstract class PhabricatorMailReceiver {
   abstract public function isEnabled();
   abstract public function canAcceptMail(PhabricatorMetaMTAReceivedMail $mail);
 
+  public function validateSender(
+    PhabricatorMetaMTAReceivedMail $mail,
+    PhabricatorUser $sender) {
+
+    if ($sender->getIsDisabled()) {
+      throw new PhabricatorMetaMTAReceivedMailProcessingException(
+        MetaMTAReceivedMailStatus::STATUS_DISABLED_SENDER,
+        pht(
+          "Sender '%s' has a disabled user account.",
+          $sender->getUsername()));
+    }
+
+
+    return;
+  }
+
   /**
    * Identifies the sender's user account for a piece of received mail. Note
    * that this method does not validate that the sender is who they say they

src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php

@@ -2,15 +2,101 @@
 
 abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver {
 
+  /**
+   * Return a regular expression fragment which matches the name of an
+   * object which can receive mail. For example, Differential uses:
+   *
+   *  D[1-9]\d*
+   *
+   * ...to match `D123`, etc., identifying Differential Revisions.
+   *
+   * @return string Regular expression fragment.
+   */
   abstract protected function getObjectPattern();
 
-  final public function canAcceptMail(PhabricatorMetaMTAReceivedMail $mail) {
-    $regexp = $this->getAddressRegexp();
+
+  /**
+   * Load the object receiving mail, based on an identifying pattern. Normally
+   * this pattern is some sort of object ID.
+   *
+   * @param   string          A string matched by @{method:getObjectPattern}
+   *                          fragment.
+   * @param   PhabricatorUser The viewing user.
+   * @return  void
+   */
+  abstract protected function loadObject($pattern, PhabricatorUser $viewer);
+
+
+  public function validateSender(
+    PhabricatorMetaMTAReceivedMail $mail,
+    PhabricatorUser $sender) {
+    parent::validateSender($mail, $sender);
 
     foreach ($mail->getToAddresses() as $address) {
-      $address = self::stripMailboxPrefix($address);
-      $local = id(new PhutilEmailAddress($address))->getLocalPart();
-      if (preg_match($regexp, $local)) {
+      $parts = $this->matchObjectAddress($address);
+      if ($parts) {
+        break;
+      }
+    }
+
+    try {
+      $object = $this->loadObject($parts['pattern'], $sender);
+    } catch (PhabricatorPolicyException $policy_exception) {
+      throw new PhabricatorMetaMTAReceivedMailProcessingException(
+        MetaMTAReceivedMailStatus::STATUS_POLICY_PROBLEM,
+        pht(
+          "This mail is addressed to an object you are not permitted ".
+          "to see: %s",
+          $policy_exception->getMessage()));
+    }
+
+    if (!$object) {
+      throw new PhabricatorMetaMTAReceivedMailProcessingException(
+        MetaMTAReceivedMailStatus::STATUS_NO_SUCH_OBJECT,
+        pht(
+          "This mail is addressed to an object ('%s'), but that object ".
+          "does not exist.",
+          $parts['pattern']));
+    }
+
+    $sender_identifier = $parts['sender'];
+
+    if ($sender_identifier === 'public') {
+      if (!PhabricatorEnv::getEnvConfig('metamta.public-replies')) {
+        throw new PhabricatorMetaMTAReceivedMailProcessingException(
+          MetaMTAReceivedMailStatus::STATUS_NO_PUBLIC_MAIL,
+          pht(
+            "This mail is addressed to an object's public address, but ".
+            "public replies are not enabled (`metamta.public-replies`)."));
+      }
+      $check_phid = $object->getPHID();
+    } else {
+      if ($sender_identifier != $sender->getID()) {
+        throw new PhabricatorMetaMTAReceivedMailProcessingException(
+          MetaMTAReceivedMailStatus::STATUS_USER_MISMATCH,
+          pht(
+            "This mail is addressed to an object's private address, but ".
+            "the sending user and the private address owner are not the ".
+            "same user."));
+      }
+      $check_phid = $sender->getPHID();
+    }
+
+    $expect_hash = self::computeMailHash($object->getMailKey(), $check_phid);
+
+    if ($expect_hash != $parts['hash']) {
+      throw new PhabricatorMetaMTAReceivedMailProcessingException(
+        MetaMTAReceivedMailStatus::STATUS_HASH_MISMATCH,
+        pht(
+          "The hash in this object's address does not match the expected ".
+          "value."));
+    }
+  }
+
+
+  final public function canAcceptMail(PhabricatorMetaMTAReceivedMail $mail) {
+    foreach ($mail->getToAddresses() as $address) {
+      if ($this->matchObjectAddress($address)) {
         return true;
       }
     }
@@ -18,6 +104,20 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver {
     return false;
   }
 
+  private function matchObjectAddress($address) {
+    $regexp = $this->getAddressRegexp();
+
+    $address = self::stripMailboxPrefix($address);
+    $local = id(new PhutilEmailAddress($address))->getLocalPart();
+
+    $matches = null;
+    if (!preg_match($regexp, $local, $matches)) {
+      return false;
+    }
+
+    return $matches;
+  }
+
   private function getAddressRegexp() {
     $pattern = $this->getObjectPattern();
 

src/applications/metamta/receiver/__tests__/PhabricatorObjectMailReceiverTestCase.php

@@ -0,0 +1,128 @@
+<?php
+
+final class PhabricatorObjectMailReceiverTestCase
+  extends PhabricatorTestCase {
+
+  protected function getPhabricatorTestCaseConfiguration() {
+    return array(
+      self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES => true,
+    );
+  }
+
+  public function testDropUnconfiguredPublicMail() {
+    list($task, $user, $mail) = $this->buildMail('public');
+
+    $env = PhabricatorEnv::beginScopedEnv();
+    $env->overrideEnvConfig('metamta.public-replies', false);
+
+    $mail->save();
+    $mail->processReceivedMail();
+
+    $this->assertEqual(
+      MetaMTAReceivedMailStatus::STATUS_NO_PUBLIC_MAIL,
+      $mail->getStatus());
+  }
+
+/*
+
+  TODO: Tasks don't support policies yet. Implement this once they do.
+
+  public function testDropPolicyViolationMail() {
+    list($task, $user, $mail) = $this->buildMail('public');
+
+    // TODO: Set task policy to "no one" here.
+
+    $mail->save();
+    $mail->processReceivedMail();
+
+    $this->assertEqual(
+      MetaMTAReceivedMailStatus::STATUS_POLICY_PROBLEM,
+      $mail->getStatus());
+  }
+
+*/
+
+  public function testDropInvalidObjectMail() {
+    list($task, $user, $mail) = $this->buildMail('404');
+
+    $mail->save();
+    $mail->processReceivedMail();
+
+    $this->assertEqual(
+      MetaMTAReceivedMailStatus::STATUS_NO_SUCH_OBJECT,
+      $mail->getStatus());
+  }
+
+  public function testDropUserMismatchMail() {
+    list($task, $user, $mail) = $this->buildMail('baduser');
+
+    $mail->save();
+    $mail->processReceivedMail();
+
+    $this->assertEqual(
+      MetaMTAReceivedMailStatus::STATUS_USER_MISMATCH,
+      $mail->getStatus());
+  }
+
+  public function testDropHashMismatchMail() {
+    list($task, $user, $mail) = $this->buildMail('badhash');
+
+    $mail->save();
+    $mail->processReceivedMail();
+
+    $this->assertEqual(
+      MetaMTAReceivedMailStatus::STATUS_HASH_MISMATCH,
+      $mail->getStatus());
+  }
+
+  private function buildMail($style) {
+
+    // TODO: Clean up test data generators so that we don't need to guarantee
+    // the existence of a user.
+    $this->generateNewTestUser();
+
+    $task = id(new PhabricatorManiphestTaskTestDataGenerator())->generate();
+    $user = $this->generateNewTestUser();
+
+    $is_public = ($style === 'public');
+    $is_bad_hash = ($style == 'badhash');
+    $is_bad_user = ($style == 'baduser');
+    $is_404_object = ($style == '404');
+
+    if ($is_public) {
+      $user_identifier = 'public';
+    } else if ($is_bad_user) {
+      $user_identifier = $user->getID() + 1;
+    } else {
+      $user_identifier = $user->getID();
+    }
+
+    if ($is_bad_hash) {
+      $hash = PhabricatorObjectMailReceiver::computeMailHash('x', 'y');
+    } else {
+      $hash = PhabricatorObjectMailReceiver::computeMailHash(
+        $task->getMailKey(),
+        $is_public ? $task->getPHID() : $user->getPHID());
+    }
+
+    if ($is_404_object) {
+      $task_identifier = 'T'.($task->getID() + 1);
+    } else {
+      $task_identifier = 'T'.$task->getID();
+    }
+
+    $to = $task_identifier.'+'.$user_identifier.'+'.$hash.'@example.com';
+
+    $mail = new PhabricatorMetaMTAReceivedMail();
+    $mail->setHeaders(
+      array(
+        'Message-ID' => 'test@example.com',
+        'From'       => $user->loadPrimaryEmail()->getAddress(),
+        'To'         => $to,
+      ));
+
+    return array($task, $user, $mail);
+  }
+
+
+}

src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php

@@ -176,6 +176,7 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
 
       $receiver = $this->loadReceiver();
       $sender = $receiver->loadSender($this);
+      $receiver->validateSender($this, $sender);
 
     } catch (PhabricatorMetaMTAReceivedMailProcessingException $ex) {
       $this
@@ -197,62 +198,11 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
 
     $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');
-
-    if ($create_task && $to == $create_task) {
+    // TODO: Move this into `ManiphestCreateMailReceiver`.
+    if ($receiver instanceof ManiphestCreateMailReceiver) {
       $receiver = new ManiphestTask();
 
-      $user = $this->lookupSender();
-      if ($user) {
-        $this->setAuthorPHID($user->getPHID());
-      } else {
-        $allow_email_users = PhabricatorEnv::getEnvConfig(
-          'phabricator.allow-email-users');
-
-        if ($allow_email_users) {
-          $email = new PhutilEmailAddress($from);
-
-          $xuser = id(new PhabricatorExternalAccount())->loadOneWhere(
-            'accountType = %s AND accountDomain IS NULL and accountID = %s',
-            'email',
-            $email->getAddress());
-
-          if (!$xuser) {
-            $xuser = new PhabricatorExternalAccount();
-            $xuser->setAccountID($email->getAddress());
-            $xuser->setAccountType('email');
-            $xuser->setDisplayName($email->getDisplayName());
-            $xuser->save();
-          }
-
-          $user = $xuser->getPhabricatorUser();
-        } else {
-          $default_author = PhabricatorEnv::getEnvConfig(
-            'metamta.maniphest.default-public-author');
-
-          if ($default_author) {
-            $user = id(new PhabricatorUser())->loadOneWhere(
-              'username = %s',
-              $default_author);
-
-            if (!$user) {
-              throw new Exception(
-                "Phabricator is misconfigured, the configuration key ".
-                "'metamta.maniphest.default-public-author' is set to user ".
-                "'{$default_author}' but that user does not exist.");
-            }
-
-          } else {
-            // TODO: We should probably bounce these since from the user's
-            // perspective their email vanishes into a black hole.
-            return $this->setMessage("Invalid public user '{$from}'.")->save();
-          }
-        }
-
-      }
+      $user = $sender;
 
       $receiver->setAuthorPHID($user->getPHID());
       $receiver->setOriginalEmailSource($from);
@@ -275,11 +225,7 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
 
     // means we're creating a conpherence...!
     if ($user_phids) {
-      // we must have a valid user who created this conpherence
-      $user = $this->lookupSender();
-      if (!$user) {
-        return $this->setMessage("Invalid public user '{$from}'.")->save();
-      }
+      $user = $sender;
 
       $conpherence = id(new ConpherenceReplyHandler())
         ->setMailReceiver(new ConpherenceThread())
@@ -293,31 +239,7 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
       return $this->save();
     }
 
-    if ($user_id == 'public') {
-      if (!PhabricatorEnv::getEnvConfig('metamta.public-replies')) {
-        return $this->setMessage("Public replies not enabled.")->save();
-      }
-
-      $user = $this->lookupSender();
-
-      if (!$user) {
-        return $this->setMessage("Invalid public user '{$from}'.")->save();
-      }
-
-      $use_user_hash = false;
-    } else {
-      $user = id(new PhabricatorUser())->load($user_id);
-      if (!$user) {
-        return $this->setMessage("Invalid private user '{$user_id}'.")->save();
-      }
-
-      $use_user_hash = true;
-    }
-
-    if ($user->getIsDisabled()) {
-      return $this->setMessage("User '{$user_id}' is disabled")->save();
-    }
-
+    $user = $sender;
     $this->setAuthorPHID($user->getPHID());
 
     $receiver = self::loadReceiverObject($receiver_name);
@@ -327,24 +249,6 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
 
     $this->setRelatedPHID($receiver->getPHID());
 
-    if ($use_user_hash) {
-      // This is a private reply-to address, check that the user hash is
-      // correct.
-      $check_phid = $user->getPHID();
-    } else {
-      // This is a public reply-to address, check that the object hash is
-      // correct.
-      $check_phid = $receiver->getPHID();
-    }
-
-    $expect_hash = PhabricatorObjectMailReceiver::computeMailHash(
-      $receiver->getMailKey(),
-      $check_phid);
-
-    if ($expect_hash != $hash) {
-      return $this->setMessage("Invalid mail hash!")->save();
-    }
-
     if ($receiver instanceof ManiphestTask) {
       $editor = new ManiphestTransactionEditor();
       $editor->setActor($user);
@@ -431,39 +335,6 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO {
     return array_filter($raw_addresses);
   }
 
-  private function lookupSender() {
-    $from = idx($this->headers, 'from');
-    $from = $this->getRawEmailAddress($from);
-
-    $user = PhabricatorUser::loadOneWithEmailAddress($from);
-
-    // If Phabricator is configured to allow "Reply-To" authentication, try
-    // the "Reply-To" address if we failed to match the "From" address.
-    $config_key = 'metamta.insecure-auth-with-reply-to';
-    $allow_reply_to = PhabricatorEnv::getEnvConfig($config_key);
-
-    if (!$user && $allow_reply_to) {
-      $reply_to = idx($this->headers, 'reply-to');
-      $reply_to = $this->getRawEmailAddress($reply_to);
-      if ($reply_to) {
-        $user = PhabricatorUser::loadOneWithEmailAddress($reply_to);
-      }
-    }
-
-    $allow_email_users = PhabricatorEnv::getEnvConfig(
-          'phabricator.allow-email-users');
-
-    if (!$user && $allow_email_users) {
-      $xusr = id(new PhabricatorExternalAccount())->loadOneWhere(
-            'accountType = %s AND accountDomain IS NULL and accountID = %s',
-            'email', $from);
-
-      $user = $xusr->getPhabricatorUser();
-    }
-
-    return $user;
-  }
-
   /**
    * If Phabricator sent the mail, always drop it immediately. This prevents
    * loops where, e.g., the public bug address is also a user email address

src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php

@@ -87,4 +87,31 @@ final class PhabricatorMetaMTAReceivedMailTestCase extends PhabricatorTestCase {
       $mail->getStatus());
   }
 
+
+  public function testDropDisabledSenderMail() {
+    $env = PhabricatorEnv::beginScopedEnv();
+    $env->overrideEnvConfig(
+      'metamta.maniphest.public-create-email',
+      'bugs@example.com');
+
+    $user = $this->generateNewTestUser()
+      ->setIsDisabled(true)
+      ->save();
+
+    $mail = new PhabricatorMetaMTAReceivedMail();
+    $mail->setHeaders(
+      array(
+        'Message-ID'  => 'test@example.com',
+        'From'        => $user->loadPrimaryEmail()->getAddress(),
+        'To'          => 'bugs@example.com',
+      ));
+    $mail->save();
+
+    $mail->processReceivedMail();
+
+    $this->assertEqual(
+      MetaMTAReceivedMailStatus::STATUS_DISABLED_SENDER,
+      $mail->getStatus());
+  }
+
 }