From 5cd13c3c65d02dc012b79e23f30e8c40f04d12f2 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 17 May 2013 10:00:49 -0700 Subject: [PATCH] Remove the last hardcoding from PhabricatorMetaMTAReceivedMail Summary: Moves all remaining mail handling into ReplyHandlers. Farewell, `getPhabricatorToInformation()`! You were a bad method and no one liked you. Ref T1205. Test Plan: - Used test console to send mail to Revisions, Tasks, Conpherences and Commits (these all actually work). - Used test console to send mail to Requests, Macros, Questions and Mocks (these accept the mail but don't do anything with it, but didn't do anything before either). Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1205 Differential Revision: https://secure.phabricator.com/D5953 --- .../mail/PhabricatorAuditMailReceiver.php | 13 ++ .../ConpherenceCreateThreadMailReceiver.php | 2 +- .../mail/ConpherenceThreadMailReceiver.php | 13 ++ .../mail/DifferentialRevisionMailReceiver.php | 12 ++ .../mail/PhabricatorMacroMailReceiver.php | 10 ++ .../mail/ManiphestCreateMailReceiver.php | 2 +- .../mail/ManiphestTaskMailReceiver.php | 15 ++ .../constants/MetaMTAReceivedMailStatus.php | 1 + .../receiver/PhabricatorMailReceiver.php | 6 +- .../PhabricatorObjectMailReceiver.php | 56 ++++-- .../PhabricatorMetaMTAReceivedMail.php | 164 +----------------- .../pholio/mail/PholioMockMailReceiver.php | 9 + .../mail/PonderQuestionMailReceiver.php | 8 + .../mail/ReleephRequestMailReceiver.php | 8 + 14 files changed, 146 insertions(+), 173 deletions(-) diff --git a/src/applications/audit/mail/PhabricatorAuditMailReceiver.php b/src/applications/audit/mail/PhabricatorAuditMailReceiver.php index 68e9201524..a66490c2dd 100644 --- a/src/applications/audit/mail/PhabricatorAuditMailReceiver.php +++ b/src/applications/audit/mail/PhabricatorAuditMailReceiver.php @@ -20,4 +20,17 @@ final class PhabricatorAuditMailReceiver extends PhabricatorObjectMailReceiver { ->executeOne(); } + protected function processReceivedObjectMail( + PhabricatorMetaMTAReceivedMail $mail, + PhabricatorLiskDAO $object, + PhabricatorUser $sender) { + + $handler = PhabricatorAuditCommentEditor::newReplyHandlerForCommit($object); + + $handler->setActor($sender); + $handler->setExcludeMailRecipientPHIDs( + $mail->loadExcludeMailRecipientPHIDs()); + $handler->processEmail($mail); + } + } diff --git a/src/applications/conpherence/mail/ConpherenceCreateThreadMailReceiver.php b/src/applications/conpherence/mail/ConpherenceCreateThreadMailReceiver.php index b26879cef3..6e2fa8cd71 100644 --- a/src/applications/conpherence/mail/ConpherenceCreateThreadMailReceiver.php +++ b/src/applications/conpherence/mail/ConpherenceCreateThreadMailReceiver.php @@ -45,7 +45,7 @@ final class ConpherenceCreateThreadMailReceiver $usernames); } - public function processReceivedMail( + protected function processReceivedMail( PhabricatorMetaMTAReceivedMail $mail, PhabricatorUser $sender) { diff --git a/src/applications/conpherence/mail/ConpherenceThreadMailReceiver.php b/src/applications/conpherence/mail/ConpherenceThreadMailReceiver.php index 558e8b4f60..e46950f992 100644 --- a/src/applications/conpherence/mail/ConpherenceThreadMailReceiver.php +++ b/src/applications/conpherence/mail/ConpherenceThreadMailReceiver.php @@ -21,5 +21,18 @@ final class ConpherenceThreadMailReceiver ->executeOne(); } + protected function processReceivedObjectMail( + PhabricatorMetaMTAReceivedMail $mail, + PhabricatorLiskDAO $object, + PhabricatorUser $sender) { + + $handler = id(new ConpherenceReplyHandler()) + ->setMailReceiver($object); + + $handler->setActor($sender); + $handler->setExcludeMailRecipientPHIDs( + $mail->loadExcludeMailRecipientPHIDs()); + $handler->processEmail($mail); + } } diff --git a/src/applications/differential/mail/DifferentialRevisionMailReceiver.php b/src/applications/differential/mail/DifferentialRevisionMailReceiver.php index 3086d176a2..87f63a615e 100644 --- a/src/applications/differential/mail/DifferentialRevisionMailReceiver.php +++ b/src/applications/differential/mail/DifferentialRevisionMailReceiver.php @@ -23,5 +23,17 @@ final class DifferentialRevisionMailReceiver return head($results); } + protected function processReceivedObjectMail( + PhabricatorMetaMTAReceivedMail $mail, + PhabricatorLiskDAO $object, + PhabricatorUser $sender) { + + $handler = DifferentialMail::newReplyHandlerForRevision($object); + + $handler->setActor($sender); + $handler->setExcludeMailRecipientPHIDs( + $mail->loadExcludeMailRecipientPHIDs()); + $handler->processEmail($mail); + } } diff --git a/src/applications/macro/mail/PhabricatorMacroMailReceiver.php b/src/applications/macro/mail/PhabricatorMacroMailReceiver.php index da9ede57b1..3901388e3d 100644 --- a/src/applications/macro/mail/PhabricatorMacroMailReceiver.php +++ b/src/applications/macro/mail/PhabricatorMacroMailReceiver.php @@ -21,4 +21,14 @@ final class PhabricatorMacroMailReceiver extends PhabricatorObjectMailReceiver { ->executeOne(); } + protected function processReceivedObjectMail( + PhabricatorMetaMTAReceivedMail $mail, + PhabricatorLiskDAO $object, + PhabricatorUser $sender) { + + // TODO: For now, we just drop this mail on the floor. + + } + + } diff --git a/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php b/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php index 88d4a8874b..1e31ccbf17 100644 --- a/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php +++ b/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php @@ -56,7 +56,7 @@ final class ManiphestCreateMailReceiver extends PhabricatorMailReceiver { } } - public function processReceivedMail( + protected function processReceivedMail( PhabricatorMetaMTAReceivedMail $mail, PhabricatorUser $sender) { diff --git a/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php b/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php index 9a04cd0bbd..0e1c4ab04d 100644 --- a/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php +++ b/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php @@ -22,4 +22,19 @@ final class ManiphestTaskMailReceiver extends PhabricatorObjectMailReceiver { return head($results); } + protected function processReceivedObjectMail( + PhabricatorMetaMTAReceivedMail $mail, + PhabricatorLiskDAO $object, + PhabricatorUser $sender) { + + $editor = new ManiphestTransactionEditor(); + $editor->setActor($sender); + $handler = $editor->buildReplyHandler($object); + + $handler->setActor($sender); + $handler->setExcludeMailRecipientPHIDs( + $mail->loadExcludeMailRecipientPHIDs()); + $handler->processEmail($mail); + } + } diff --git a/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php b/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php index 062fcbdfbe..157523eecb 100644 --- a/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php +++ b/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php @@ -14,5 +14,6 @@ final class MetaMTAReceivedMailStatus const STATUS_POLICY_PROBLEM = 'err:policy'; const STATUS_NO_SUCH_OBJECT = 'err:not-found'; const STATUS_HASH_MISMATCH = 'err:bad-hash'; + const STATUS_UNHANDLED_EXCEPTION = 'err:exception'; } diff --git a/src/applications/metamta/receiver/PhabricatorMailReceiver.php b/src/applications/metamta/receiver/PhabricatorMailReceiver.php index 3949c656a3..96f83d8435 100644 --- a/src/applications/metamta/receiver/PhabricatorMailReceiver.php +++ b/src/applications/metamta/receiver/PhabricatorMailReceiver.php @@ -5,11 +5,9 @@ abstract class PhabricatorMailReceiver { abstract public function isEnabled(); abstract public function canAcceptMail(PhabricatorMetaMTAReceivedMail $mail); - public function processReceivedMail( + abstract protected function processReceivedMail( PhabricatorMetaMTAReceivedMail $mail, - PhabricatorUser $sender) { - return; - } + PhabricatorUser $sender); final public function receiveMail( PhabricatorMetaMTAReceivedMail $mail, diff --git a/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php b/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php index 7dc37661ab..4e09e7b50a 100644 --- a/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php +++ b/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php @@ -26,25 +26,38 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver { */ abstract protected function loadObject($pattern, PhabricatorUser $viewer); + + final protected function processReceivedMail( + PhabricatorMetaMTAReceivedMail $mail, + PhabricatorUser $sender) { + + $object = $this->loadObjectFromMail($mail, $sender); + $mail->setRelatedPHID($object->getPHID()); + + $this->processReceivedObjectMail($mail, $object, $sender); + + return $this; + } + + abstract protected function processReceivedObjectMail( + PhabricatorMetaMTAReceivedMail $mail, + PhabricatorLiskDAO $object, + PhabricatorUser $sender); + public function loadMailReceiverObject($pattern, PhabricatorUser $viewer) { return $this->loadObject($pattern, $viewer); } - public function validateSender( PhabricatorMetaMTAReceivedMail $mail, PhabricatorUser $sender) { + parent::validateSender($mail, $sender); - foreach ($mail->getToAddresses() as $address) { - $parts = $this->matchObjectAddress($address); - if ($parts) { - break; - } - } + $parts = $this->matchObjectAddressInMail($mail); try { - $object = $this->loadObject($parts['pattern'], $sender); + $object = $this->loadObjectFromMail($mail, $sender); } catch (PhabricatorPolicyException $policy_exception) { throw new PhabricatorMetaMTAReceivedMailProcessingException( MetaMTAReceivedMailStatus::STATUS_POLICY_PROBLEM, @@ -99,15 +112,26 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver { final public function canAcceptMail(PhabricatorMetaMTAReceivedMail $mail) { - foreach ($mail->getToAddresses() as $address) { - if ($this->matchObjectAddress($address)) { - return true; - } + if ($this->matchObjectAddressInMail($mail)) { + return true; } return false; } + private function matchObjectAddressInMail( + PhabricatorMetaMTAReceivedmail $mail) { + + foreach ($mail->getToAddresses() as $address) { + $parts = $this->matchObjectAddress($address); + if ($parts) { + return $parts; + } + } + + return null; + } + private function matchObjectAddress($address) { $regexp = $this->getAddressRegexp(); @@ -137,6 +161,14 @@ abstract class PhabricatorObjectMailReceiver extends PhabricatorMailReceiver { return $regexp; } + private function loadObjectFromMail( + PhabricatorMetaMTAReceivedMail $mail, + PhabricatorUser $sender) { + $parts = $this->matchObjectAddressInMail($mail); + + return $this->loadObject($parts['pattern'], $sender); + } + public static function computeMailHash($mail_key, $phid) { $global_mail_key = PhabricatorEnv::getEnvConfig('phabricator.mail-key'); diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php index 8c7eb0f6e2..0b7b7c9040 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php @@ -88,76 +88,6 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { return array_merge($user_phids, $mailing_list_phids); } - /** - * 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. If nothing can be - * found, it then loads user phids for as many to: email addresses as - * it can, theoretically falling back to create a conpherence amongst - * those users. - */ - 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|E)\d+)\+([\w]+)\+([a-f0-9]{16})@/U"; - - $phabricator_address = null; - $receiver_name = null; - $user_id = null; - $hash = null; - $user_names = array(); - 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; - } - - $parts = explode('@', $address); - $maybe_name = trim($parts[0]); - $maybe_domain = trim($parts[1]); - $mail_domain = PhabricatorEnv::getEnvConfig('metamta.domain'); - if ($mail_domain == $maybe_domain && - PhabricatorUser::validateUsername($maybe_name)) { - $user_names[] = $maybe_name; - } - } - - return array( - $phabricator_address, - $receiver_name, - $user_id, - $hash, - ); - } - - public function processReceivedMail() { try { @@ -170,70 +100,23 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { $this->setAuthorPHID($sender->getPHID()); - // TODO: Once everything can receive mail, nuke this. - $can_receive = false; - if ($receiver instanceof ManiphestCreateMailReceiver) { - $can_receive = true; - } - if ($receiver instanceof ConpherenceCreateThreadMailReceiver) { - $can_receive = true; - } - - if ($can_receive) { - $receiver->receiveMail($this, $sender); - return $this->setMessage('OK')->save(); - } - + $receiver->receiveMail($this, $sender); } catch (PhabricatorMetaMTAReceivedMailProcessingException $ex) { $this ->setStatus($ex->getStatusCode()) ->setMessage($ex->getMessage()) ->save(); return $this; + } catch (Exception $ex) { + $this + ->setStatus(MetaMTAReceivedMailStatus::STATUS_UNHANDLED_EXCEPTION) + ->setMessage(pht('Unhandled Exception: %s', $ex->getMessage())) + ->save(); + + throw $ex; } - list($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'); - - $user = $sender; - - $receiver = self::loadReceiverObject($receiver_name); - if (!$receiver) { - return $this->setMessage("Invalid object '{$receiver_name}'")->save(); - } - - $this->setRelatedPHID($receiver->getPHID()); - - if ($receiver instanceof ManiphestTask) { - $editor = new ManiphestTransactionEditor(); - $editor->setActor($user); - $handler = $editor->buildReplyHandler($receiver); - } else if ($receiver instanceof DifferentialRevision) { - $handler = DifferentialMail::newReplyHandlerForRevision($receiver); - } else if ($receiver instanceof PhabricatorRepositoryCommit) { - $handler = PhabricatorAuditCommentEditor::newReplyHandlerForCommit( - $receiver); - } else if ($receiver instanceof ConpherenceThread) { - $handler = id(new ConpherenceReplyHandler()) - ->setMailReceiver($receiver); - } - - $handler->setActor($user); - $handler->setExcludeMailRecipientPHIDs( - $this->loadExcludeMailRecipientPHIDs()); - $handler->processEmail($this); - - $this->setMessage('OK'); - - return $this->save(); + return $this->setMessage('OK')->save(); } public function getCleanTextBody() { @@ -247,35 +130,6 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { return idx($this->bodies, 'text'); } - public static function loadReceiverObject($receiver_name) { - if (!$receiver_name) { - return null; - } - - $receiver_type = $receiver_name[0]; - $receiver_id = substr($receiver_name, 1); - - $class_obj = null; - switch ($receiver_type) { - case 'T': - $class_obj = new ManiphestTask(); - break; - case 'D': - $class_obj = new DifferentialRevision(); - break; - case 'C': - $class_obj = new PhabricatorRepositoryCommit(); - break; - case 'E': - $class_obj = new ConpherenceThread(); - break; - default: - return null; - } - - return $class_obj->load($receiver_id); - } - /** * Strip an email address down to the actual user@domain.tld part if * necessary, since sometimes it will have formatting like diff --git a/src/applications/pholio/mail/PholioMockMailReceiver.php b/src/applications/pholio/mail/PholioMockMailReceiver.php index bc8f12ede1..7ed6f73de1 100644 --- a/src/applications/pholio/mail/PholioMockMailReceiver.php +++ b/src/applications/pholio/mail/PholioMockMailReceiver.php @@ -20,5 +20,14 @@ final class PholioMockMailReceiver extends PhabricatorObjectMailReceiver { ->executeOne(); } + protected function processReceivedObjectMail( + PhabricatorMetaMTAReceivedMail $mail, + PhabricatorLiskDAO $object, + PhabricatorUser $sender) { + + // TODO: For now, we just drop this mail on the floor. + + } + } diff --git a/src/applications/ponder/mail/PonderQuestionMailReceiver.php b/src/applications/ponder/mail/PonderQuestionMailReceiver.php index 484caee260..58511e1f00 100644 --- a/src/applications/ponder/mail/PonderQuestionMailReceiver.php +++ b/src/applications/ponder/mail/PonderQuestionMailReceiver.php @@ -20,5 +20,13 @@ final class PonderQuestionMailReceiver extends PhabricatorObjectMailReceiver { ->executeOne(); } + protected function processReceivedObjectMail( + PhabricatorMetaMTAReceivedMail $mail, + PhabricatorLiskDAO $object, + PhabricatorUser $sender) { + + // TODO: For now, we just drop this mail on the floor. + + } } diff --git a/src/applications/releeph/mail/ReleephRequestMailReceiver.php b/src/applications/releeph/mail/ReleephRequestMailReceiver.php index 67f6cce5b0..4a1ed016fc 100644 --- a/src/applications/releeph/mail/ReleephRequestMailReceiver.php +++ b/src/applications/releeph/mail/ReleephRequestMailReceiver.php @@ -20,5 +20,13 @@ final class ReleephRequestMailReceiver extends PhabricatorObjectMailReceiver { ->executeOne(); } + protected function processReceivedObjectMail( + PhabricatorMetaMTAReceivedMail $mail, + PhabricatorLiskDAO $object, + PhabricatorUser $sender) { + + // TODO: For now, we just drop this mail on the floor. + + } }