From ff4073c2f4bbe1388a499caa73c2876c4609d337 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 15 May 2013 08:44:54 -0700 Subject: [PATCH] Copy mail sender selection logic into MailReceivers Summary: Copies sender identification logic into MailReceivers and makes it basically sane. The mess we run into after this try/catch is terrifying so I'm avoiding actually getting rid of any of it quite yet. Ref T1205. Test Plan: Added a bit of test coverage. Used Receiver test console to verify some additional behaviors. Reviewers: btrahan Reviewed By: btrahan CC: Afaque_Hussain, aran Maniphest Tasks: T1205 Differential Revision: https://secure.phabricator.com/D5931 --- .../mail/ManiphestCreateMailReceiver.php | 36 +++++++ .../constants/MetaMTAReceivedMailStatus.php | 1 + .../receiver/PhabricatorMailReceiver.php | 102 +++++++++++++++++- .../PhabricatorMetaMTAReceivedMail.php | 1 + ...PhabricatorMetaMTAReceivedMailTestCase.php | 23 ++++ 5 files changed, 158 insertions(+), 5 deletions(-) diff --git a/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php b/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php index ae3549bba5..2c8412c337 100644 --- a/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php +++ b/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php @@ -20,4 +20,40 @@ final class ManiphestCreateMailReceiver extends PhabricatorMailReceiver { return false; } + public function loadSender(PhabricatorMetaMTAReceivedMail $mail) { + try { + // Try to load the sender normally. + return parent::loadSender($mail); + } catch (PhabricatorMetaMTAReceivedMailProcessingException $ex) { + + // If we failed to load the sender normally, use this special legacy + // black magic. + + // TODO: Deprecate and remove this. + + $default_author_key = 'metamta.maniphest.default-public-author'; + $default_author = PhabricatorEnv::getEnvConfig($default_author_key); + + if (!strlen($default_author)) { + throw $ex; + } + + $user = id(new PhabricatorUser())->loadOneWhere( + 'username = %s', + $default_author); + + if ($user) { + return $user; + } + + throw new PhabricatorMetaMTAReceivedMailProcessingException( + MetaMTAReceivedMailStatus::STATUS_UNKNOWN_SENDER, + pht( + "Phabricator is misconfigured, the configuration key ". + "'metamta.maniphest.default-public-author' is set to user ". + "'%s' but that user does not exist.", + $default_author)); + } + } + } diff --git a/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php b/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php index b8d4d0f68a..7f1e477ad1 100644 --- a/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php +++ b/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php @@ -7,5 +7,6 @@ final class MetaMTAReceivedMailStatus const STATUS_FROM_PHABRICATOR = 'err:self'; const STATUS_NO_RECEIVERS = 'err:no-receivers'; const STATUS_ABUNDANT_RECEIVERS = 'err:multiple-receivers'; + const STATUS_UNKNOWN_SENDER = 'err:unknown-sender'; } diff --git a/src/applications/metamta/receiver/PhabricatorMailReceiver.php b/src/applications/metamta/receiver/PhabricatorMailReceiver.php index 8eb906ee38..575aca6ff9 100644 --- a/src/applications/metamta/receiver/PhabricatorMailReceiver.php +++ b/src/applications/metamta/receiver/PhabricatorMailReceiver.php @@ -6,6 +6,83 @@ abstract class PhabricatorMailReceiver { abstract public function isEnabled(); abstract public function canAcceptMail(PhabricatorMetaMTAReceivedMail $mail); + /** + * 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 + * are, just that they've presented some credential which corresponds to a + * recognizable user. + */ + public function loadSender(PhabricatorMetaMTAReceivedMail $mail) { + $raw_from = $mail->getHeader('From'); + $from = self::getRawAddress($raw_from); + + $reasons = array(); + + // Try to find a user with this email address. + $user = PhabricatorUser::loadOneWithEmailAddress($from); + if ($user) { + return $user; + } else { + $reasons[] = pht( + "The email was sent from '%s', but this address does not correspond ". + "to any user account.", + $raw_from); + } + + // If we missed on "From", try "Reply-To" if we're configured for it. + $reply_to_key = 'metamta.insecure-auth-with-reply-to'; + $allow_reply_to = PhabricatorEnv::getEnvConfig($reply_to_key); + if ($allow_reply_to) { + $raw_reply_to = $mail->getHeader('Reply-To'); + $reply_to = self::getRawAddress($raw_reply_to); + + $user = PhabricatorUser::loadOneWithEmailAddress($reply_to); + if ($user) { + return $user; + } else { + $reasons[] = pht( + "Phabricator is configured to try to authenticate users using ". + "'Reply-To', but the reply to address ('%s') does not correspond ". + "to any user account.", + $raw_reply_to); + } + } else { + $reasons[] = pht( + "Phabricator is not configured to authenticate users using ". + "'Reply-To' (`metamta.insecure-auth-with-reply-to`), so the ". + "'Reply-To' header was not examined."); + } + + // If we don't know who this user is, load or create an external user + // account for them if we're configured for it. + $email_key = 'phabricator.allow-email-users'; + $allow_email_users = PhabricatorEnv::getEnvConfig($email_key); + if ($allow_email_users) { + $xuser = id(new PhabricatorExternalAccount())->loadOneWhere( + 'accountType = %s AND accountDomain IS NULL and accountID = %s', + 'email', + $from); + if (!$xuser) { + $xuser = id(new PhabricatorExternalAccount()) + ->setAccountID($from) + ->setAccountType('email') + ->setDisplayName($from) + ->save(); + } + return $xuser->getPhabricatorUser(); + } else { + $reasons[] = pht( + "Phabricator is not configured to allow unknown external users to ". + "send mail to the system using just an email address ". + "(`phabricator.allow-email-users`), so an implicit external acount ". + "could not be created."); + } + + throw new PhabricatorMetaMTAReceivedMailProcessingException( + MetaMTAReceivedMailStatus::STATUS_UNKNOWN_SENDER, + pht('Unknown sender: %s', implode(' ', $reasons))); + } + /** * Determine if two inbound email addresses are effectively identical. This * method strips and normalizes addresses so that equivalent variations are @@ -22,15 +99,12 @@ abstract class PhabricatorMailReceiver { * @return bool True if addresses match. */ public static function matchAddresses($u, $v) { - $u = id(new PhutilEmailAddress($u))->getAddress(); - $v = id(new PhutilEmailAddress($v))->getAddress(); + $u = self::getRawAddress($u); + $v = self::getRawAddress($v); $u = self::stripMailboxPrefix($u); $v = self::stripMailboxPrefix($v); - $u = trim(phutil_utf8_strtolower($u)); - $v = trim(phutil_utf8_strtolower($v)); - return ($u === $v); } @@ -69,4 +143,22 @@ abstract class PhabricatorMailReceiver { } + /** + * Reduce an email address to its canonical form. For example, an adddress + * like: + * + * "Abraham Lincoln" < ALincoln@example.com > + * + * ...will be reduced to: + * + * alincoln@example.com + * + * @param string Email address in noncanonical form. + * @return string Canonical email address. + */ + public static function getRawAddress($address) { + $address = id(new PhutilEmailAddress($address))->getAddress(); + return trim(phutil_utf8_strtolower($address)); + } + } diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php index 28c55073a0..dd55adf4a9 100644 --- a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php @@ -175,6 +175,7 @@ final class PhabricatorMetaMTAReceivedMail extends PhabricatorMetaMTADAO { $this->dropMailAlreadyReceived(); $receiver = $this->loadReceiver(); + $sender = $receiver->loadSender($this); } catch (PhabricatorMetaMTAReceivedMailProcessingException $ex) { $this diff --git a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php index d734513f38..1e1be9c566 100644 --- a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php +++ b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php @@ -63,5 +63,28 @@ final class PhabricatorMetaMTAReceivedMailTestCase extends PhabricatorTestCase { $mail->getStatus()); } + public function testDropUnknownSenderMail() { + $env = PhabricatorEnv::beginScopedEnv(); + $env->overrideEnvConfig( + 'metamta.maniphest.public-create-email', + 'bugs@example.com'); + $env->overrideEnvConfig('phabricator.allow-email-users', false); + $env->overrideEnvConfig('metamta.maniphest.default-public-author', null); + + $mail = new PhabricatorMetaMTAReceivedMail(); + $mail->setHeaders( + array( + 'Message-ID' => 'test@example.com', + 'To' => 'bugs@example.com', + 'From' => 'does+not+exist@example.com', + )); + $mail->save(); + + $mail->processReceivedMail(); + + $this->assertEqual( + MetaMTAReceivedMailStatus::STATUS_UNKNOWN_SENDER, + $mail->getStatus()); + } }