From 690a460c8e8da76153a6bfec1ddcbe524d2ea407 Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 27 Jul 2018 12:42:25 -0700 Subject: [PATCH] Allow mailers to be explicitly marked as inbound or outbound Summary: See PHI785. Ref T13164. In this case, an install wants to receive mail via Mailgun, but not configure it (DKIM + SPF) for outbound mail. Allow individual mailers to be marked as not supporting inbound or outbound mail. Test Plan: - Added and ran unit tests. - Went through some mail pathways locally, but I don't have every inbound/outbound configured so this isn't totally conclusive. - Hit `bin/mail send-test` with a no-outbound mailer. - I'll hold this until after the release cut so it can soak on `secure` for a bit. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13164 Differential Revision: https://secure.phabricator.com/D19546 --- .../PhabricatorMailImplementationAdapter.php | 21 +++++ ...ricatorMetaMTAMailgunReceiveController.php | 7 +- ...icatorMetaMTAPostmarkReceiveController.php | 7 +- ...icatorMetaMTASendGridReceiveController.php | 7 +- ...bricatorMailManagementSendTestWorkflow.php | 10 ++- .../storage/PhabricatorMetaMTAMail.php | 79 +++++++++++++++---- .../PhabricatorMailConfigTestCase.php | 73 ++++++++++++++++- .../configuring_outbound_email.diviner | 4 + .../PhabricatorClusterMailersConfigType.php | 2 + 9 files changed, 185 insertions(+), 25 deletions(-) diff --git a/src/applications/metamta/adapter/PhabricatorMailImplementationAdapter.php b/src/applications/metamta/adapter/PhabricatorMailImplementationAdapter.php index dfbe891651..8a6b2d2275 100644 --- a/src/applications/metamta/adapter/PhabricatorMailImplementationAdapter.php +++ b/src/applications/metamta/adapter/PhabricatorMailImplementationAdapter.php @@ -6,6 +6,9 @@ abstract class PhabricatorMailImplementationAdapter extends Phobject { private $priority; private $options = array(); + private $supportsInbound = true; + private $supportsOutbound = true; + final public function getAdapterType() { return $this->getPhobjectClassConstant('ADAPTERTYPE'); } @@ -67,6 +70,24 @@ abstract class PhabricatorMailImplementationAdapter extends Phobject { return $this->priority; } + final public function setSupportsInbound($supports_inbound) { + $this->supportsInbound = $supports_inbound; + return $this; + } + + final public function getSupportsInbound() { + return $this->supportsInbound; + } + + final public function setSupportsOutbound($supports_outbound) { + $this->supportsOutbound = $supports_outbound; + return $this; + } + + final public function getSupportsOutbound() { + return $this->supportsOutbound; + } + final public function getOption($key) { if (!array_key_exists($key, $this->options)) { throw new Exception( diff --git a/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php b/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php index 3ca2711dcf..91f656cf97 100644 --- a/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php +++ b/src/applications/metamta/controller/PhabricatorMetaMTAMailgunReceiveController.php @@ -17,9 +17,12 @@ final class PhabricatorMetaMTAMailgunReceiveController // inbound mail from any of them. Test the signature to see if it matches // any configured Mailgun mailer. - $mailers = PhabricatorMetaMTAMail::newMailersWithTypes( + $mailers = PhabricatorMetaMTAMail::newMailers( array( - PhabricatorMailImplementationMailgunAdapter::ADAPTERTYPE, + 'inbound' => true, + 'types' => array( + PhabricatorMailImplementationMailgunAdapter::ADAPTERTYPE, + ), )); foreach ($mailers as $mailer) { $api_key = $mailer->getOption('api-key'); diff --git a/src/applications/metamta/controller/PhabricatorMetaMTAPostmarkReceiveController.php b/src/applications/metamta/controller/PhabricatorMetaMTAPostmarkReceiveController.php index 345cd93fe1..550a1366f2 100644 --- a/src/applications/metamta/controller/PhabricatorMetaMTAPostmarkReceiveController.php +++ b/src/applications/metamta/controller/PhabricatorMetaMTAPostmarkReceiveController.php @@ -12,9 +12,12 @@ final class PhabricatorMetaMTAPostmarkReceiveController */ public function handleRequest(AphrontRequest $request) { // Don't process requests if we don't have a configured Postmark adapter. - $mailers = PhabricatorMetaMTAMail::newMailersWithTypes( + $mailers = PhabricatorMetaMTAMail::newMailers( array( - PhabricatorMailImplementationPostmarkAdapter::ADAPTERTYPE, + 'inbound' => true, + 'types' => array( + PhabricatorMailImplementationPostmarkAdapter::ADAPTERTYPE, + ), )); if (!$mailers) { return new Aphront404Response(); diff --git a/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php b/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php index 6651f85d6c..9ec32f60a1 100644 --- a/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php +++ b/src/applications/metamta/controller/PhabricatorMetaMTASendGridReceiveController.php @@ -11,9 +11,12 @@ final class PhabricatorMetaMTASendGridReceiveController // SendGrid doesn't sign payloads so we can't be sure that SendGrid // actually sent this request, but require a configured SendGrid mailer // before we activate this endpoint. - $mailers = PhabricatorMetaMTAMail::newMailersWithTypes( + $mailers = PhabricatorMetaMTAMail::newMailers( array( - PhabricatorMailImplementationSendGridAdapter::ADAPTERTYPE, + 'inbound' => true, + 'types' => array( + PhabricatorMailImplementationSendGridAdapter::ADAPTERTYPE, + ), )); if (!$mailers) { return new Aphront404Response(); diff --git a/src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php b/src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php index 152b62fd3f..8140d64bdd 100644 --- a/src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php +++ b/src/applications/metamta/management/PhabricatorMailManagementSendTestWorkflow.php @@ -168,7 +168,8 @@ final class PhabricatorMailManagementSendTestWorkflow $mailer_key = $args->getArg('mailer'); if ($mailer_key !== null) { - $mailers = PhabricatorMetaMTAMail::newMailers(); + $mailers = PhabricatorMetaMTAMail::newMailers(array()); + $mailers = mpull($mailers, null, 'getKey'); if (!isset($mailers[$mailer_key])) { throw new PhutilArgumentUsageException( @@ -178,6 +179,13 @@ final class PhabricatorMailManagementSendTestWorkflow implode(', ', array_keys($mailers)))); } + if (!$mailers[$mailer_key]->getSupportsOutbound()) { + throw new PhutilArgumentUsageException( + pht( + 'Mailer ("%s") is not configured to support outbound mail.', + $mailer_key)); + } + $mail->setTryMailers(array($mailer_key)); } diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php index cca3dfa335..d757868483 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -494,7 +494,10 @@ final class PhabricatorMetaMTAMail throw new Exception(pht('Trying to send an already-sent mail!')); } - $mailers = self::newMailers(); + $mailers = self::newMailers( + array( + 'outbound' => true, + )); $try_mailers = $this->getParam('mailers.try'); if ($try_mailers) { @@ -505,21 +508,15 @@ final class PhabricatorMetaMTAMail return $this->sendWithMailers($mailers); } - public static function newMailersWithTypes(array $types) { - $mailers = self::newMailers(); - $types = array_fuse($types); + public static function newMailers(array $constraints) { + PhutilTypeSpec::checkMap( + $constraints, + array( + 'types' => 'optional list', + 'inbound' => 'optional bool', + 'outbound' => 'optional bool', + )); - foreach ($mailers as $key => $mailer) { - $mailer_type = $mailer->getAdapterType(); - if (!isset($types[$mailer_type])) { - unset($mailers[$key]); - } - } - - return array_values($mailers); - } - - public static function newMailers() { $mailers = array(); $config = PhabricatorEnv::getEnvConfig('cluster.mailers'); @@ -565,10 +562,45 @@ final class PhabricatorMetaMTAMail $options = idx($spec, 'options', array()) + $defaults; $mailer->setOptions($options); + $mailer->setSupportsInbound(idx($spec, 'inbound', true)); + $mailer->setSupportsOutbound(idx($spec, 'outbound', true)); + $mailers[] = $mailer; } } + // Remove mailers with the wrong types. + if (isset($constraints['types'])) { + $types = $constraints['types']; + $types = array_fuse($types); + foreach ($mailers as $key => $mailer) { + $mailer_type = $mailer->getAdapterType(); + if (!isset($types[$mailer_type])) { + unset($mailers[$key]); + } + } + } + + // If we're only looking for inbound mailers, remove mailers with inbound + // support disabled. + if (!empty($constraints['inbound'])) { + foreach ($mailers as $key => $mailer) { + if (!$mailer->getSupportsInbound()) { + unset($mailers[$key]); + } + } + } + + // If we're only looking for outbound mailers, remove mailers with outbound + // support disabled. + if (!empty($constraints['outbound'])) { + foreach ($mailers as $key => $mailer) { + if (!$mailer->getSupportsOutbound()) { + unset($mailers[$key]); + } + } + } + $sorted = array(); $groups = mgroup($mailers, 'getPriority'); krsort($groups); @@ -589,9 +621,24 @@ final class PhabricatorMetaMTAMail public function sendWithMailers(array $mailers) { if (!$mailers) { + $any_mailers = self::newMailers(); + + // NOTE: We can end up here with some custom list of "$mailers", like + // from a unit test. In that case, this message could be misleading. We + // can't really tell if the caller made up the list, so just assume they + // aren't tricking us. + + if ($any_mailers) { + $void_message = pht( + 'No configured mailers support sending outbound mail.'); + } else { + $void_message = pht( + 'No mailers are configured.'); + } + return $this ->setStatus(PhabricatorMailOutboundStatus::STATUS_VOID) - ->setMessage(pht('No mailers are configured.')) + ->setMessage($void_message) ->save(); } diff --git a/src/applications/metamta/storage/__tests__/PhabricatorMailConfigTestCase.php b/src/applications/metamta/storage/__tests__/PhabricatorMailConfigTestCase.php index 25984a2c1d..fb48925ed4 100644 --- a/src/applications/metamta/storage/__tests__/PhabricatorMailConfigTestCase.php +++ b/src/applications/metamta/storage/__tests__/PhabricatorMailConfigTestCase.php @@ -118,11 +118,80 @@ final class PhabricatorMailConfigTestCase $this->assertTrue($saw_a1 && $saw_a2); } - private function newMailersWithConfig(array $config) { + public function testMailerConstraints() { + $config = array( + array( + 'key' => 'X1', + 'type' => 'test', + ), + array( + 'key' => 'X1-in', + 'type' => 'test', + 'outbound' => false, + ), + array( + 'key' => 'X1-out', + 'type' => 'test', + 'inbound' => false, + ), + array( + 'key' => 'X1-void', + 'type' => 'test', + 'inbound' => false, + 'outbound' => false, + ), + ); + + $mailers = $this->newMailersWithConfig( + $config, + array()); + $this->assertEqual(4, count($mailers)); + + $mailers = $this->newMailersWithConfig( + $config, + array( + 'inbound' => true, + )); + $this->assertEqual(2, count($mailers)); + + $mailers = $this->newMailersWithConfig( + $config, + array( + 'outbound' => true, + )); + $this->assertEqual(2, count($mailers)); + + $mailers = $this->newMailersWithConfig( + $config, + array( + 'inbound' => true, + 'outbound' => true, + )); + $this->assertEqual(1, count($mailers)); + + $mailers = $this->newMailersWithConfig( + $config, + array( + 'types' => array('test'), + )); + $this->assertEqual(4, count($mailers)); + + $mailers = $this->newMailersWithConfig( + $config, + array( + 'types' => array('duck'), + )); + $this->assertEqual(0, count($mailers)); + } + + private function newMailersWithConfig( + array $config, + array $constraints = array()) { + $env = PhabricatorEnv::beginScopedEnv(); $env->overrideEnvConfig('cluster.mailers', $config); - $mailers = PhabricatorMetaMTAMail::newMailers(); + $mailers = PhabricatorMetaMTAMail::newMailers($constraints); unset($env); return $mailers; diff --git a/src/docs/user/configuration/configuring_outbound_email.diviner b/src/docs/user/configuration/configuring_outbound_email.diviner index 5de8429c13..a33134fe4e 100644 --- a/src/docs/user/configuration/configuring_outbound_email.diviner +++ b/src/docs/user/configuration/configuring_outbound_email.diviner @@ -82,6 +82,10 @@ The supported keys for each mailer are: - `priority`: Optional string. Advanced option which controls load balancing and failover behavior. See below for details. - `options`: Optional map. Additional options for the mailer type. + - `inbound`: Optional bool. Use `false` to prevent this mailer from being + used to receive inbound mail. + - `outbound`: Optional bool. Use `false` to prevent this mailer from being + used to send outbound mail. The `type` field can be used to select these third-party mailers: diff --git a/src/infrastructure/cluster/config/PhabricatorClusterMailersConfigType.php b/src/infrastructure/cluster/config/PhabricatorClusterMailersConfigType.php index 03f30506bd..387014289d 100644 --- a/src/infrastructure/cluster/config/PhabricatorClusterMailersConfigType.php +++ b/src/infrastructure/cluster/config/PhabricatorClusterMailersConfigType.php @@ -43,6 +43,8 @@ final class PhabricatorClusterMailersConfigType 'type' => 'string', 'priority' => 'optional int', 'options' => 'optional wild', + 'inbound' => 'optional bool', + 'outbound' => 'optional bool', )); } catch (Exception $ex) { throw $this->newException(