From 71efb46ba77d26a63590bbc49b85e05de9374d63 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 May 2011 16:31:26 -0700 Subject: [PATCH] Support email multiplexing for private Reply-To addresses Summary: Provide a base PhabricatorMailReplyHandler class which handles the plumbing for multiplexing email if necessary and supporting public and private reply handler addressses. DifferentialReplyHandler now extends it, and a new ManiphestReplyHandler also does. The general approach here is that we have three supported cases: - no reply handler, default config, same as what we're doing now - public reply handler, requires overriding classes but just sets "reply-to" to some address the install generates and still sends only one email - private reply handler, provides a default generation mechanism or you can override it and splits mail apart so we send one to each recipient Test Plan: Sent email from Maniphest and Differential with and without reply-handler-domains set. Reviewed By: aran Reviewers: jungejason, tuomaspelkonen, aran CC: aran, epriestley Differential Revision: 254 --- conf/default.conf.php | 35 ++++- src/__phutil_library_map__.php | 4 + .../mail/base/DifferentialMail.php | 94 +++++------- .../differential/mail/base/__init__.php | 3 +- .../replyhandler/DifferentialReplyHandler.php | 64 +++----- .../differential/replyhandler/__init__.php | 1 + .../ManiphestTransactionEditor.php | 42 +++++- .../replyhandler/ManiphestReplyHandler.php | 47 ++++++ .../maniphest/replyhandler/__init__.php | 13 ++ .../base/PhabricatorMailReplyHandler.php | 139 ++++++++++++++++++ .../metamta/replyhandler/base/__init__.php | 15 ++ .../PhabricatorMetaMTAReceivedMail.php | 11 +- .../phid/handle/PhabricatorObjectHandle.php | 10 ++ .../data/PhabricatorObjectHandleData.php | 1 + 14 files changed, 365 insertions(+), 114 deletions(-) create mode 100644 src/applications/maniphest/replyhandler/ManiphestReplyHandler.php create mode 100644 src/applications/maniphest/replyhandler/__init__.php create mode 100644 src/applications/metamta/replyhandler/base/PhabricatorMailReplyHandler.php create mode 100644 src/applications/metamta/replyhandler/base/__init__.php diff --git a/conf/default.conf.php b/conf/default.conf.php index 581b062938..d5160451ae 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -180,6 +180,38 @@ return array( 'amazon-ses.access-key' => null, 'amazon-ses.secret-key' => null, + // You can configure a reply handler domain so that email sent from Maniphest + // will have a special "Reply To" address like "T123+82+af19f@example.com" + // that allows recipients to reply by email and interact with tasks. For + // instructions on configurating reply handlers, see the article + // "Configuring Inbound Email" in the Phabricator documentation. By default, + // this is set to 'null' and Phabricator will use a generic 'noreply@' address + // or the address of the acting user instead of a special reply handler + // address (see 'metamta.default-address'). If you set a domain here, + // Phabricator will begin generating private reply handler addresses. See + // also 'metamta.maniphest.reply-handler' to further configure behavior. + // This key should be set to the domain part after the @, like "example.com". + 'metamta.maniphest.reply-handler-domain' => null, + + // You can follow the instructions in "Configuring Inbound Email" in the + // Phabricator documentation and set 'metamta.maniphest.reply-handler-domain' + // to support updating Maniphest tasks by email. If you want more advanced + // customization than this provides, you can override the reply handler + // class with an implementation of your own. This will allow you to do things + // like have a single public reply handler or change how private reply + // handlers are generated and validated. + // This key should be set to a loadable subclass of + // PhabricatorMailReplyHandler (and possibly of ManiphestReplyHandler). + 'metamta.maniphest.reply-handler' => 'ManiphestReplyHandler', + + // See 'metamta.maniphest.reply-handler-domain'. This does the same thing, + // but allows email replies via Differential. + 'metamta.differential.reply-handler-domain' => null, + + // See 'metamta.maniphest.reply-handler'. This does the same thing, but + // affects Differential. + 'metamta.differential.reply-handler' => 'DifferentialReplyHandler', + // -- Auth ------------------------------------------------------------------ // @@ -317,9 +349,6 @@ return array( 'differential.revision-custom-detail-renderer' => null, - 'phabricator.enable-reply-handling' => false, - 'differential.replyhandler' => 'DifferentialReplyHandler', - // -- Maniphest ------------------------------------------------------------- // diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d5ff859d10..16999cf9fe 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -248,6 +248,7 @@ phutil_register_library_map(array( 'LiskIsolationTestDAOException' => 'storage/lisk/dao/__tests__', 'ManiphestController' => 'applications/maniphest/controller/base', 'ManiphestDAO' => 'applications/maniphest/storage/base', + 'ManiphestReplyHandler' => 'applications/maniphest/replyhandler', 'ManiphestTask' => 'applications/maniphest/storage/task', 'ManiphestTaskDetailController' => 'applications/maniphest/controller/taskdetail', 'ManiphestTaskEditController' => 'applications/maniphest/controller/taskedit', @@ -331,6 +332,7 @@ phutil_register_library_map(array( 'PhabricatorMailImplementationAmazonSESAdapter' => 'applications/metamta/adapter/amazonses', 'PhabricatorMailImplementationPHPMailerLiteAdapter' => 'applications/metamta/adapter/phpmailerlite', 'PhabricatorMailImplementationTestAdapter' => 'applications/metamta/adapter/test', + 'PhabricatorMailReplyHandler' => 'applications/metamta/replyhandler/base', 'PhabricatorMetaMTAController' => 'applications/metamta/controller/base', 'PhabricatorMetaMTADAO' => 'applications/metamta/storage/base', 'PhabricatorMetaMTADaemon' => 'applications/metamta/daemon/mta', @@ -615,6 +617,7 @@ phutil_register_library_map(array( 'DifferentialInlineCommentPreviewController' => 'DifferentialController', 'DifferentialInlineCommentView' => 'AphrontView', 'DifferentialNewDiffMail' => 'DifferentialReviewRequestMail', + 'DifferentialReplyHandler' => 'PhabricatorMailReplyHandler', 'DifferentialReviewRequestMail' => 'DifferentialMail', 'DifferentialRevision' => 'DifferentialDAO', 'DifferentialRevisionCommentListView' => 'AphrontView', @@ -677,6 +680,7 @@ phutil_register_library_map(array( 'LiskIsolationTestDAO' => 'LiskDAO', 'ManiphestController' => 'PhabricatorController', 'ManiphestDAO' => 'PhabricatorLiskDAO', + 'ManiphestReplyHandler' => 'PhabricatorMailReplyHandler', 'ManiphestTask' => 'ManiphestDAO', 'ManiphestTaskDetailController' => 'ManiphestController', 'ManiphestTaskEditController' => 'ManiphestController', diff --git a/src/applications/differential/mail/base/DifferentialMail.php b/src/applications/differential/mail/base/DifferentialMail.php index b091d7585c..a517440392 100644 --- a/src/applications/differential/mail/base/DifferentialMail.php +++ b/src/applications/differential/mail/base/DifferentialMail.php @@ -70,47 +70,49 @@ abstract class DifferentialMail { $subject = $this->buildSubject(); $body = $this->buildBody(); - $mail = new PhabricatorMetaMTAMail(); + $template = new PhabricatorMetaMTAMail(); $actor_handle = $this->getActorHandle(); $reply_handler = $this->getReplyHandler(); if ($actor_handle) { - $mail->setFrom($actor_handle->getPHID()); + $template->setFrom($actor_handle->getPHID()); } - if ($reply_handler) { - if ($actor_handle) { - $actor = id(new PhabricatorUser())->loadOneWhere( - 'phid = %s', - $actor_handle->getPHID()); - $reply_handler->setActor($actor); - } - - $reply_to = $reply_handler->getReplyHandlerEmailAddress(); - if ($reply_to) { - $mail->setReplyTo($reply_to); - } - } - - $mail - ->addTos($to_phids) - ->addCCs($cc_phids) + $template ->setSubject($subject) ->setBody($body) ->setIsHTML($this->shouldMarkMailAsHTML()) ->addHeader('Thread-Topic', $this->getRevision()->getTitle()); - $mail->setThreadID( + $template->setThreadID( $this->getThreadID(), $this->isFirstMailAboutRevision()); if ($this->heraldRulesHeader) { - $mail->addHeader('X-Herald-Rules', $this->heraldRulesHeader); + $template->addHeader('X-Herald-Rules', $this->heraldRulesHeader); } - $mail->setRelatedPHID($this->getRevision()->getPHID()); + $template->setRelatedPHID($this->getRevision()->getPHID()); - $mail->saveAndSend(); + $phids = array(); + foreach ($to_phids as $phid) { + $phids[$phid] = true; + } + foreach ($cc_phids as $phid) { + $phids[$phid] = true; + } + $phids = array_keys($phids); + + $handles = id(new PhabricatorObjectHandleData($phids))->loadHandles(); + + $mails = $reply_handler->multiplexMail( + $template, + array_select_keys($handles, $to_phids), + array_select_keys($handles, $cc_phids)); + + foreach ($mails as $mail) { + $mail->saveAndSend(); + } } protected function buildSubject() { @@ -125,9 +127,12 @@ abstract class DifferentialMail { $body = $this->renderBody(); - $handler_body_text = $this->getReplyHandlerBodyText(); - if ($handler_body_text) { - $body .= $handler_body_text; + $reply_handler = $this->getReplyHandler(); + $reply_instructions = $reply_handler->getReplyHandlerInstructions(); + if ($reply_instructions) { + $body .= + "\nREPLY HANDLER ACTIONS\n". + " {$reply_instructions}\n"; } if ($this->getHeraldTranscriptURI() && $this->isFirstMailToRecipients()) { @@ -151,47 +156,22 @@ EOTEXT; return $body; } - protected function getReplyHandlerBodyText() { - $reply_handler = $this->getReplyHandler(); - - if (!$reply_handler) { - return null; - } - - return $reply_handler->getBodyText(); - } - protected function getReplyHandler() { if ($this->replyHandler) { return $this->replyHandler; } - $reply_handler = self::loadReplyHandler(); - if (!$reply_handler) { - return null; - } + $handler_class = PhabricatorEnv::getEnvConfig( + 'metamta.differential.reply-handler'); + + $reply_handler = newv($handler_class, array()); + $reply_handler->setMailReceiver($this->getRevision()); - $reply_handler->setRevision($this->getRevision()); $this->replyHandler = $reply_handler; + return $this->replyHandler; } - public static function loadReplyHandler() { - if (!PhabricatorEnv::getEnvConfig('phabricator.enable-reply-handling')) { - return null; - } - - $reply_handler = PhabricatorEnv::getEnvConfig('differential.replyhandler'); - - if (!$reply_handler) { - return null; - } - - PhutilSymbolLoader::loadClass($reply_handler); - $reply_handler = newv($reply_handler, array()); - return $reply_handler; - } - protected function formatText($text) { $text = explode("\n", $text); foreach ($text as &$line) { diff --git a/src/applications/differential/mail/base/__init__.php b/src/applications/differential/mail/base/__init__.php index 26c687044d..c69b84db4d 100644 --- a/src/applications/differential/mail/base/__init__.php +++ b/src/applications/differential/mail/base/__init__.php @@ -7,10 +7,9 @@ phutil_require_module('phabricator', 'applications/metamta/storage/mail'); -phutil_require_module('phabricator', 'applications/people/storage/user'); +phutil_require_module('phabricator', 'applications/phid/handle/data'); phutil_require_module('phabricator', 'infrastructure/env'); -phutil_require_module('phutil', 'symbols'); phutil_require_module('phutil', 'utils'); diff --git a/src/applications/differential/replyhandler/DifferentialReplyHandler.php b/src/applications/differential/replyhandler/DifferentialReplyHandler.php index d23f0adab7..c37b72c38c 100644 --- a/src/applications/differential/replyhandler/DifferentialReplyHandler.php +++ b/src/applications/differential/replyhandler/DifferentialReplyHandler.php @@ -16,9 +16,23 @@ * limitations under the License. */ -class DifferentialReplyHandler { - protected $revision; - protected $actor; +class DifferentialReplyHandler extends PhabricatorMailReplyHandler { + + public function validateMailReceiver($mail_receiver) { + if (!($mail_receiver instanceof DifferentialRevision)) { + throw new Exception("Receiver is not a DifferentialRevision!"); + } + } + + public function getPrivateReplyHandlerEmailAddress( + PhabricatorObjectHandle $handle) { + return $this->getDefaultPrivateReplyHandlerEmailAddress($handle, 'D'); + } + + public function getReplyHandlerDomain() { + return PhabricatorEnv::getEnvConfig( + 'metamta.differential.reply-handler-domain'); + } /* * Generate text like the following from the supported commands. @@ -29,7 +43,11 @@ class DifferentialReplyHandler { * * " */ - public function getBodyText() { + public function getReplyHandlerInstructions() { + if (!$this->supportsReplies()) { + return null; + } + $supported_commands = $this->getSupportedCommands(); $text = ''; if (empty($supported_commands)) { @@ -37,7 +55,6 @@ class DifferentialReplyHandler { } $comment_command_printed = false; - $text .= "\nACTIONS\n"; if (in_array(DifferentialAction::ACTION_COMMENT, $supported_commands)) { $text .= 'Reply to comment'; $comment_command_printed = true; @@ -59,7 +76,7 @@ class DifferentialReplyHandler { $text .= implode(', ', $modified_commands); } - $text .= ".\n\n"; + $text .= "."; return $text; } @@ -75,19 +92,6 @@ class DifferentialReplyHandler { ); } - public function getReplyHandlerEmailAddress() { - if (!self::isEnabled()) { - return null; - } - - $revision = $this->getRevision(); - if (!$revision) { - return null; - } - - return '...'; // TODO: build the D1234+92+aldsbn@domain.com as per D226 - } - public function handleAction($body) { // all commands start with a bang and separated from the body by a newline // to make sure that actual feedback text couldn't trigger an action. @@ -131,26 +135,4 @@ class DifferentialReplyHandler { } } - public function setActor(PhabricatorUser $actor) { - $this->actor = $actor; - return $this; - } - - public function getActor() { - return $this->actor; - } - - public function setRevision(DifferentialRevision $revision) { - $this->revision = $revision; - return $this; - } - - public function getRevision() { - return $this->revision; - } - - public static function isEnabled() { - return PhabricatorEnv::getEnvConfig('phabricator.enable-reply-handling'); - } - } diff --git a/src/applications/differential/replyhandler/__init__.php b/src/applications/differential/replyhandler/__init__.php index 16530653d1..ada5f63ba5 100644 --- a/src/applications/differential/replyhandler/__init__.php +++ b/src/applications/differential/replyhandler/__init__.php @@ -9,6 +9,7 @@ phutil_require_module('phabricator', 'applications/differential/constants/action'); phutil_require_module('phabricator', 'applications/differential/editor/comment'); phutil_require_module('phabricator', 'applications/differential/mail/exception'); +phutil_require_module('phabricator', 'applications/metamta/replyhandler/base'); phutil_require_module('phabricator', 'infrastructure/env'); diff --git a/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php index ade1eca2f6..8b34b0880a 100644 --- a/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/transaction/ManiphestTransactionEditor.php @@ -140,6 +140,12 @@ class ManiphestTransactionEditor { $phids[$phid] = true; } } + foreach ($email_to as $phid) { + $phids[$phid] = true; + } + foreach ($email_cc as $phid) { + $phids[$phid] = true; + } $phids = array_keys($phids); $handles = id(new PhabricatorObjectHandleData($phids)) @@ -162,6 +168,8 @@ class ManiphestTransactionEditor { $task_uri = PhabricatorEnv::getURI('/T'.$task->getID()); + $reply_handler = $this->buildReplyHandler($task); + if ($is_create) { $body .= "\n\n". @@ -174,19 +182,43 @@ class ManiphestTransactionEditor { "TASK DETAIL\n". " ".$task_uri."\n"; + $reply_instructions = $reply_handler->getReplyHandlerInstructions(); + if ($reply_instructions) { + $body .= + "\n". + "REPLY HANDLER ACTIONS\n". + " ".$reply_instructions."\n"; + } + $thread_id = 'getPHID().'>'; $task_id = $task->getID(); $title = $task->getTitle(); - id(new PhabricatorMetaMTAMail()) + $template = id(new PhabricatorMetaMTAMail()) ->setSubject(self::SUBJECT_PREFIX." [{$action}] T{$task_id}: {$title}") ->setFrom($transaction->getAuthorPHID()) - ->addTos($email_to) - ->addCCs($email_cc) ->addHeader('Thread-Topic', 'Maniphest Task '.$task->getID()) ->setThreadID($thread_id, $is_create) ->setRelatedPHID($task->getPHID()) - ->setBody($body) - ->saveAndSend(); + ->setBody($body); + + $mails = $reply_handler->multiplexMail( + $template, + array_select_keys($handles, $email_to), + array_select_keys($handles, $email_cc)); + + foreach ($mails as $mail) { + $mail->saveAndSend(); + } + } + + private function buildReplyHandler(ManiphestTask $task) { + $handler_class = PhabricatorEnv::getEnvConfig( + 'metamta.maniphest.reply-handler'); + + $handler_object = newv($handler_class, array()); + $handler_object->setMailReceiver($task); + + return $handler_object; } } diff --git a/src/applications/maniphest/replyhandler/ManiphestReplyHandler.php b/src/applications/maniphest/replyhandler/ManiphestReplyHandler.php new file mode 100644 index 0000000000..5b06ef951d --- /dev/null +++ b/src/applications/maniphest/replyhandler/ManiphestReplyHandler.php @@ -0,0 +1,47 @@ +getDefaultPrivateReplyHandlerEmailAddress($handle, 'T'); + } + + public function getReplyHandlerDomain() { + return PhabricatorEnv::getEnvConfig( + 'metamta.maniphest.reply-handler-domain'); + } + + public function getReplyHandlerInstructions() { + if ($this->supportsReplies()) { + return "Reply to comment or attach files, or !close, !claim, ". + "!unsubscribe, or !assign . ". + "TODO: None of this works yet!"; + } else { + return null; + } + } + +} diff --git a/src/applications/maniphest/replyhandler/__init__.php b/src/applications/maniphest/replyhandler/__init__.php new file mode 100644 index 0000000000..a77e07150e --- /dev/null +++ b/src/applications/maniphest/replyhandler/__init__.php @@ -0,0 +1,13 @@ +validateMailReceiver($mail_receiver); + $this->mailReceiver = $mail_receiver; + return $this; + } + + final public function getMailReceiver() { + return $this->mailReceiver; + } + + final public function setActor(PhabricatorUser $actor) { + $this->actor = $actor; + return $this; + } + + final public function getActor() { + return $this->actor; + } + + abstract public function validateMailReceiver($mail_receiver); + abstract public function getPrivateReplyHandlerEmailAddress( + PhabricatorObjectHandle $handle); + abstract public function getReplyHandlerDomain(); + abstract public function getReplyHandlerInstructions(); + + public function supportsPrivateReplies() { + return (bool)$this->getReplyHandlerDomain(); + } + + final public function supportsReplies() { + return $this->supportsPrivateReplies() || + (bool)$this->getPublicReplyHandlerEmailAddress(); + } + + public function getPublicReplyHandlerEmailAddress() { + return null; + } + + final public function multiplexMail( + PhabricatorMetaMTAMail $mail_template, + array $to_handles, + array $cc_handles) { + + $result = array(); + + // If private replies are not supported, simply send one email to all + // recipients and CCs. This covers cases where we have no reply handler, + // or we have a public reply handler. + if (!$this->supportsPrivateReplies()) { + $mail = clone $mail_template; + $mail->addTos(mpull($to_handles, 'getPHID')); + $mail->addCCs(mpull($cc_handles, 'getPHID')); + + $reply_to = $this->getPublicReplyHandlerEmailAddress(); + if ($reply_to) { + $mail->setReplyTo($reply_to); + } + + $result[] = $mail; + + return $result; + } + + // Merge all the recipients together. TODO: We could keep the CCs as real + // CCs and send to a "noreply@domain.com" type address, but keep it simple + // for now. + $recipients = mpull($to_handles, null, 'getPHID') + + mpull($cc_handles, null, 'getPHID'); + + // This grouping is just so we can use the public reply-to for any + // recipients without a private reply-to, e.g. mailing lists. + $groups = array(); + foreach ($recipients as $recipient) { + $private = $this->getPrivateReplyHandlerEmailAddress($recipient); + $groups[$private][] = $recipient; + } + + foreach ($groups as $reply_to => $group) { + $mail = clone $mail_template; + $mail->addTos(mpull($group, 'getPHID')); + + if (!$reply_to) { + $reply_to = $this->getPublicReplyHandlerEmailAddress(); + } + + if ($reply_to) { + $mail->setReplyTo($reply_to); + } + + $result[] = $mail; + } + + return $result; + } + + protected function getDefaultPrivateReplyHandlerEmailAddress( + PhabricatorObjectHandle $handle, + $prefix) { + + if ($handle->getType() != PhabricatorPHIDConstants::PHID_TYPE_USER) { + // You must be a real user to get a private reply handler address. + return null; + } + + $receiver = $this->getMailReceiver(); + $receiver_id = $receiver->getID(); + $user_id = $handle->getAlternateID(); + $hash = PhabricatorMetaMTAReceivedMail::computeMailHash( + $receiver->getMailKey(), + $handle->getPHID()); + $domain = $this->getReplyHandlerDomain(); + + return "{$prefix}{$receiver_id}+{$user_id}+{$hash}@{$domain}"; + } + +} diff --git a/src/applications/metamta/replyhandler/base/__init__.php b/src/applications/metamta/replyhandler/base/__init__.php new file mode 100644 index 0000000000..5ff3e91560 --- /dev/null +++ b/src/applications/metamta/replyhandler/base/__init__.php @@ -0,0 +1,15 @@ +setRelatedPHID($receiver->getPHID()); - $expect_hash = self::computeMailHash($receiver, $user); + $expect_hash = self::computeMailHash( + $receiver->getMailKey(), + $user->getPHID()); if ($expect_hash != $hash) { return $this->setMessage("Invalid mail hash!")->save(); } @@ -138,13 +140,10 @@ class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { return $class_obj->load($receiver_id); } - public static function computeMailHash( - $mail_receiver, - PhabricatorUser $user) { + public static function computeMailHash($mail_key, $phid) { $global_mail_key = PhabricatorEnv::getEnvConfig('phabricator.mail-key'); - $local_mail_key = $mail_receiver->getMailKey(); - $hash = sha1($local_mail_key.$global_mail_key.$user->getPHID()); + $hash = sha1($mail_key.$global_mail_key.$phid); return substr($hash, 0, 16); } diff --git a/src/applications/phid/handle/PhabricatorObjectHandle.php b/src/applications/phid/handle/PhabricatorObjectHandle.php index 173bdd7d5c..97aa437b99 100644 --- a/src/applications/phid/handle/PhabricatorObjectHandle.php +++ b/src/applications/phid/handle/PhabricatorObjectHandle.php @@ -26,6 +26,7 @@ class PhabricatorObjectHandle { private $fullName; private $imageURI; private $timestamp; + private $alternateID; public function setURI($uri) { $this->uri = $uri; @@ -102,6 +103,15 @@ class PhabricatorObjectHandle { return $this->timestamp; } + public function setAlternateID($alternate_id) { + $this->alternateID = $alternate_id; + return $this; + } + + public function getAlternateID() { + return $this->alternateID; + } + public function renderLink() { switch ($this->getType()) { diff --git a/src/applications/phid/handle/data/PhabricatorObjectHandleData.php b/src/applications/phid/handle/data/PhabricatorObjectHandleData.php index db2a5d505b..44c582c6cd 100644 --- a/src/applications/phid/handle/data/PhabricatorObjectHandleData.php +++ b/src/applications/phid/handle/data/PhabricatorObjectHandleData.php @@ -76,6 +76,7 @@ class PhabricatorObjectHandleData { $handle->setEmail($user->getEmail()); $handle->setFullName( $user->getUsername().' ('.$user->getRealName().')'); + $handle->setAlternateID($user->getID()); $img_phid = $user->getProfileImagePHID(); if ($img_phid) {