From 7f2c90fbd12b10e10abc3098c080baae96d4040f Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 5 Feb 2018 15:58:07 -0800 Subject: [PATCH] Prepare for multiple mailers of the same type Summary: Depends on D19000. Ref T13053. Ref T12677. Currently, most mailers are configured with a bunch of `.setting-name` global config options. This means that you can't configure two different SMTP servers, which is a reasonable thing to want to do in the brave new world of mail failover. It also means you can't configure two Mailgun accounts or two SES accounts. Although this might seem a little silly, we've had more service disruptions because of policy issues / administrative error (where a particular account was disabled) than actual downtime, so maybe it's not completely ridiculous. Realign mailers so they can take configuration directly in an explicit way. A later change will add new configuration to take advantage of this and let us move away from having ~10 global options for this stuff eventually. (This also makes writing third-party mailers easier.) Test Plan: Processed some mail, ran existing unit tests. But I wasn't especially thorough. I expect later changes to provide some tools to make this more testable, so I'll vet each provider more thoroughly and add coverage for multiple mailers after that stuff is ready. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13053, T12677 Differential Revision: https://secure.phabricator.com/D19002 --- .../PhabricatorMailImplementationAdapter.php | 40 ++++++++++++++ ...atorMailImplementationAmazonSESAdapter.php | 36 ++++++++++-- ...icatorMailImplementationMailgunAdapter.php | 27 ++++++++- ...atorMailImplementationPHPMailerAdapter.php | 55 ++++++++++++++++--- ...MailImplementationPHPMailerLiteAdapter.php | 24 +++++++- ...catorMailImplementationSendGridAdapter.php | 27 ++++++++- ...abricatorMailImplementationTestAdapter.php | 18 +++++- .../storage/PhabricatorMetaMTAMail.php | 29 +++++++--- .../PhabricatorMetaMTAMailTestCase.php | 4 +- 9 files changed, 229 insertions(+), 31 deletions(-) diff --git a/src/applications/metamta/adapter/PhabricatorMailImplementationAdapter.php b/src/applications/metamta/adapter/PhabricatorMailImplementationAdapter.php index 3363301909..514306758d 100644 --- a/src/applications/metamta/adapter/PhabricatorMailImplementationAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailImplementationAdapter.php @@ -2,6 +2,9 @@ abstract class PhabricatorMailImplementationAdapter extends Phobject { + private $key; + private $options = array(); + abstract public function setFrom($email, $name = ''); abstract public function addReplyTo($email, $name = ''); abstract public function addTos(array $emails); @@ -12,6 +15,7 @@ abstract class PhabricatorMailImplementationAdapter extends Phobject { abstract public function setHTMLBody($html_body); abstract public function setSubject($subject); + /** * Some mailers, notably Amazon SES, do not support us setting a specific * Message-ID header. @@ -32,4 +36,40 @@ abstract class PhabricatorMailImplementationAdapter extends Phobject { */ abstract public function send(); + final public function setKey($key) { + $this->key = $key; + return $this; + } + + final public function getKey() { + return $this->key; + } + + final public function getOption($key) { + if (!array_key_exists($key, $this->options)) { + throw new Exception( + pht( + 'Mailer ("%s") is attempting to access unknown option ("%s").', + get_class($this), + $key)); + } + + return $this->options[$key]; + } + + final public function setOptions(array $options) { + $this->validateOptions($options); + $this->options = $options; + return $this; + } + + abstract protected function validateOptions(array $options); + + abstract public function newDefaultOptions(); + abstract public function newLegacyOptions(); + + public function prepareForSend() { + return; + } + } diff --git a/src/applications/metamta/adapter/PhabricatorMailImplementationAmazonSESAdapter.php b/src/applications/metamta/adapter/PhabricatorMailImplementationAmazonSESAdapter.php index 5b03cd86ac..850b83f1dd 100644 --- a/src/applications/metamta/adapter/PhabricatorMailImplementationAmazonSESAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailImplementationAmazonSESAdapter.php @@ -6,8 +6,8 @@ final class PhabricatorMailImplementationAmazonSESAdapter private $message; private $isHTML; - public function __construct() { - parent::__construct(); + public function prepareForSend() { + parent::prepareForSend(); $this->mailer->Mailer = 'amazon-ses'; $this->mailer->customMailer = $this; } @@ -17,13 +17,39 @@ final class PhabricatorMailImplementationAmazonSESAdapter return false; } + protected function validateOptions(array $options) { + PhutilTypeSpec::checkMap( + $options, + array( + 'access-key' => 'string', + 'secret-key' => 'string', + 'endpoint' => 'string', + )); + } + + public function newDefaultOptions() { + return array( + 'access-key' => null, + 'secret-key' => null, + 'endpoint' => null, + ); + } + + public function newLegacyOptions() { + return array( + 'access-key' => PhabricatorEnv::getEnvConfig('amazon-ses.access-key'), + 'secret-key' => PhabricatorEnv::getEnvConfig('amazon-ses.secret-key'), + 'endpoint' => PhabricatorEnv::getEnvConfig('amazon-ses.endpoint'), + ); + } + /** * @phutil-external-symbol class SimpleEmailService */ public function executeSend($body) { - $key = PhabricatorEnv::getEnvConfig('amazon-ses.access-key'); - $secret = PhabricatorEnv::getEnvConfig('amazon-ses.secret-key'); - $endpoint = PhabricatorEnv::getEnvConfig('amazon-ses.endpoint'); + $key = $this->getOption('access-key'); + $secret = $this->getOption('secret-key'); + $endpoint = $this->getOption('endpoint'); $root = phutil_get_library_root('phabricator'); $root = dirname($root); diff --git a/src/applications/metamta/adapter/PhabricatorMailImplementationMailgunAdapter.php b/src/applications/metamta/adapter/PhabricatorMailImplementationMailgunAdapter.php index cfe6491fe0..a7be6731eb 100644 --- a/src/applications/metamta/adapter/PhabricatorMailImplementationMailgunAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailImplementationMailgunAdapter.php @@ -71,9 +71,32 @@ final class PhabricatorMailImplementationMailgunAdapter return true; } + protected function validateOptions(array $options) { + PhutilTypeSpec::checkMap( + $options, + array( + 'api-key' => 'string', + 'domain' => 'string', + )); + } + + public function newDefaultOptions() { + return array( + 'api-key' => null, + 'domain' => null, + ); + } + + public function newLegacyOptions() { + return array( + 'api-key' => PhabricatorEnv::getEnvConfig('mailgun.api-key'), + 'domain' => PhabricatorEnv::getEnvConfig('mailgun.domain'), + ); + } + public function send() { - $key = PhabricatorEnv::getEnvConfig('mailgun.api-key'); - $domain = PhabricatorEnv::getEnvConfig('mailgun.domain'); + $key = $this->getOption('api-key'); + $domain = $this->getOption('domain'); $params = array(); $params['to'] = implode(', ', idx($this->params, 'tos', array())); diff --git a/src/applications/metamta/adapter/PhabricatorMailImplementationPHPMailerAdapter.php b/src/applications/metamta/adapter/PhabricatorMailImplementationPHPMailerAdapter.php index f4d7e8e156..0eb59629a6 100644 --- a/src/applications/metamta/adapter/PhabricatorMailImplementationPHPMailerAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailImplementationPHPMailerAdapter.php @@ -5,17 +5,55 @@ final class PhabricatorMailImplementationPHPMailerAdapter private $mailer; + protected function validateOptions(array $options) { + PhutilTypeSpec::checkMap( + $options, + array( + 'host' => 'string|null', + 'port' => 'int', + 'user' => 'string|null', + 'password' => 'string|null', + 'protocol' => 'string|null', + 'encoding' => 'string', + 'mailer' => 'string', + )); + } + + public function newDefaultOptions() { + return array( + 'host' => null, + 'port' => 25, + 'user' => null, + 'password' => null, + 'protocol' => null, + 'encoding' => 'base64', + 'mailer' => 'smtp', + ); + } + + public function newLegacyOptions() { + return array( + 'host' => PhabricatorEnv::getEnvConfig('phpmailer.smtp-host'), + 'port' => PhabricatorEnv::getEnvConfig('phpmailer.smtp-port'), + 'user' => PhabricatorEnv::getEnvConfig('phpmailer.smtp-user'), + 'password' => PhabricatorEnv::getEnvConfig('phpmailer.smtp-passsword'), + 'protocol' => PhabricatorEnv::getEnvConfig('phpmailer.smtp-protocol'), + 'encoding' => PhabricatorEnv::getEnvConfig('phpmailer.smtp-encoding'), + 'mailer' => PhabricatorEnv::getEnvConfig('phpmailer.mailer'), + ); + } + /** * @phutil-external-symbol class PHPMailer */ - public function __construct() { + public function prepareForSend() { $root = phutil_get_library_root('phabricator'); $root = dirname($root); require_once $root.'/externals/phpmailer/class.phpmailer.php'; $this->mailer = new PHPMailer($use_exceptions = true); $this->mailer->CharSet = 'utf-8'; - $encoding = PhabricatorEnv::getEnvConfig('phpmailer.smtp-encoding'); + $encoding = $this->getOption('encoding'); $this->mailer->Encoding = $encoding; // By default, PHPMailer sends one mail per recipient. We handle @@ -23,20 +61,19 @@ final class PhabricatorMailImplementationPHPMailerAdapter // send mail exactly like we ask. $this->mailer->SingleTo = false; - $mailer = PhabricatorEnv::getEnvConfig('phpmailer.mailer'); + $mailer = $this->getOption('mailer'); if ($mailer == 'smtp') { $this->mailer->IsSMTP(); - $this->mailer->Host = PhabricatorEnv::getEnvConfig('phpmailer.smtp-host'); - $this->mailer->Port = PhabricatorEnv::getEnvConfig('phpmailer.smtp-port'); - $user = PhabricatorEnv::getEnvConfig('phpmailer.smtp-user'); + $this->mailer->Host = $this->getOption('host'); + $this->mailer->Port = $this->getOption('port'); + $user = $this->getOption('user'); if ($user) { $this->mailer->SMTPAuth = true; $this->mailer->Username = $user; - $this->mailer->Password = - PhabricatorEnv::getEnvConfig('phpmailer.smtp-password'); + $this->mailer->Password = $this->getOption('password'); } - $protocol = PhabricatorEnv::getEnvConfig('phpmailer.smtp-protocol'); + $protocol = $this->getOption('protocol'); if ($protocol) { $protocol = phutil_utf8_strtolower($protocol); $this->mailer->SMTPSecure = $protocol; diff --git a/src/applications/metamta/adapter/PhabricatorMailImplementationPHPMailerLiteAdapter.php b/src/applications/metamta/adapter/PhabricatorMailImplementationPHPMailerLiteAdapter.php index 668f9353ec..4fd8387252 100644 --- a/src/applications/metamta/adapter/PhabricatorMailImplementationPHPMailerLiteAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailImplementationPHPMailerLiteAdapter.php @@ -8,17 +8,37 @@ class PhabricatorMailImplementationPHPMailerLiteAdapter protected $mailer; + protected function validateOptions(array $options) { + PhutilTypeSpec::checkMap( + $options, + array( + 'encoding' => 'string', + )); + } + + public function newDefaultOptions() { + return array( + 'encoding' => 'base64', + ); + } + + public function newLegacyOptions() { + return array( + 'encoding' => PhabricatorEnv::getEnvConfig('phpmailer.smtp-encoding'), + ); + } + /** * @phutil-external-symbol class PHPMailerLite */ - public function __construct() { + public function prepareForSend() { $root = phutil_get_library_root('phabricator'); $root = dirname($root); require_once $root.'/externals/phpmailer/class.phpmailer-lite.php'; $this->mailer = new PHPMailerLite($use_exceptions = true); $this->mailer->CharSet = 'utf-8'; - $encoding = PhabricatorEnv::getEnvConfig('phpmailer.smtp-encoding'); + $encoding = $this->getOption('encoding'); $this->mailer->Encoding = $encoding; // By default, PHPMailerLite sends one mail per recipient. We handle diff --git a/src/applications/metamta/adapter/PhabricatorMailImplementationSendGridAdapter.php b/src/applications/metamta/adapter/PhabricatorMailImplementationSendGridAdapter.php index 566d33fd14..9cd8dd19b4 100644 --- a/src/applications/metamta/adapter/PhabricatorMailImplementationSendGridAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailImplementationSendGridAdapter.php @@ -8,6 +8,29 @@ final class PhabricatorMailImplementationSendGridAdapter private $params = array(); + protected function validateOptions(array $options) { + PhutilTypeSpec::checkMap( + $options, + array( + 'api-user' => 'string', + 'api-key' => 'string', + )); + } + + public function newDefaultOptions() { + return array( + 'api-user' => null, + 'api-key' => null, + ); + } + + public function newLegacyOptions() { + return array( + 'api-user' => PhabricatorEnv::getEnvConfig('sendgrid.api-user'), + 'api-key' => PhabricatorEnv::getEnvConfig('sendgrid.api-key'), + ); + } + public function setFrom($email, $name = '') { $this->params['from'] = $email; $this->params['from-name'] = $name; @@ -73,8 +96,8 @@ final class PhabricatorMailImplementationSendGridAdapter public function send() { - $user = PhabricatorEnv::getEnvConfig('sendgrid.api-user'); - $key = PhabricatorEnv::getEnvConfig('sendgrid.api-key'); + $user = $this->getOption('api-user'); + $key = $this->getOption('api-key'); if (!$user || !$key) { throw new Exception( diff --git a/src/applications/metamta/adapter/PhabricatorMailImplementationTestAdapter.php b/src/applications/metamta/adapter/PhabricatorMailImplementationTestAdapter.php index 0ea2af916f..bd64076a59 100644 --- a/src/applications/metamta/adapter/PhabricatorMailImplementationTestAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailImplementationTestAdapter.php @@ -8,9 +8,23 @@ final class PhabricatorMailImplementationTestAdapter extends PhabricatorMailImplementationAdapter { private $guts = array(); - private $config; + private $config = array(); - public function __construct(array $config = array()) { + protected function validateOptions(array $options) { + PhutilTypeSpec::checkMap( + $options, + array()); + } + + public function newDefaultOptions() { + return array(); + } + + public function newLegacyOptions() { + return array(); + } + + public function prepareForSend(array $config = array()) { $this->config = $config; } diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index 317f9be8df..eb1f1fbea2 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -453,11 +453,6 @@ final class PhabricatorMetaMTAMail return $result; } - public function buildDefaultMailer() { - return PhabricatorEnv::newObjectFromConfig('metamta.mail-adapter'); - } - - /** * Attempt to deliver an email immediately, in this process. * @@ -468,13 +463,31 @@ final class PhabricatorMetaMTAMail throw new Exception(pht('Trying to send an already-sent mail!')); } - $mailers = array( - $this->buildDefaultMailer(), - ); + $mailers = $this->newMailers(); return $this->sendWithMailers($mailers); } + private function newMailers() { + $mailers = array(); + + $mailer = PhabricatorEnv::newObjectFromConfig('metamta.mail-adapter'); + + $defaults = $mailer->newDefaultOptions(); + $options = $mailer->newLegacyOptions(); + + $options = $options + $defaults; + + $mailer + ->setKey('default') + ->setOptions($options); + + $mailer->prepareForSend(); + + $mailers[] = $mailer; + + return $mailers; + } public function sendWithMailers(array $mailers) { $exceptions = array(); diff --git a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php index 9f14e0c4e1..6e72b129b1 100644 --- a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php +++ b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAMailTestCase.php @@ -182,7 +182,9 @@ final class PhabricatorMetaMTAMailTestCase extends PhabricatorTestCase { $supports_message_id, $is_first_mail) { - $mailer = new PhabricatorMailImplementationTestAdapter( + $mailer = new PhabricatorMailImplementationTestAdapter(); + + $mailer->prepareForSend( array( 'supportsMessageIDHeader' => $supports_message_id, ));