diff --git a/src/applications/audit/mail/PhabricatorAuditMailReceiver.php b/src/applications/audit/mail/PhabricatorAuditMailReceiver.php --- a/src/applications/audit/mail/PhabricatorAuditMailReceiver.php +++ b/src/applications/audit/mail/PhabricatorAuditMailReceiver.php @@ -12,7 +12,7 @@ } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)preg_replace('/^COMMIT/', '', $pattern); + $id = (int)preg_replace('/^COMMIT/i', '', $pattern); return id(new DiffusionCommitQuery()) ->setViewer($viewer) diff --git a/src/applications/calendar/mail/PhabricatorCalendarEventMailReceiver.php b/src/applications/calendar/mail/PhabricatorCalendarEventMailReceiver.php --- a/src/applications/calendar/mail/PhabricatorCalendarEventMailReceiver.php +++ b/src/applications/calendar/mail/PhabricatorCalendarEventMailReceiver.php @@ -13,7 +13,7 @@ } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)trim($pattern, 'E'); + $id = (int)substr($pattern, 1); return id(new PhabricatorCalendarEventQuery()) ->setViewer($viewer) diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -394,6 +394,10 @@ 'metamta.insecure-auth-with-reply-to' => pht( 'Authenticating users based on "Reply-To" is no longer supported.'), + + 'phabricator.allow-email-users' => pht( + 'Public email is now accepted if the associated address has a '. + 'default author, and rejected otherwise.'), ); return $ancient_config; diff --git a/src/applications/config/option/PhabricatorCoreConfigOptions.php b/src/applications/config/option/PhabricatorCoreConfigOptions.php --- a/src/applications/config/option/PhabricatorCoreConfigOptions.php +++ b/src/applications/config/option/PhabricatorCoreConfigOptions.php @@ -234,14 +234,6 @@ $this->newOption('phabricator.cache-namespace', 'string', 'phabricator') ->setLocked(true) ->setDescription(pht('Cache namespace.')), - $this->newOption('phabricator.allow-email-users', 'bool', false) - ->setBoolOptions( - array( - pht('Allow'), - pht('Disallow'), - )) - ->setDescription( - pht('Allow non-members to interact with tasks over email.')), $this->newOption('phabricator.silent', 'bool', false) ->setLocked(true) ->setBoolOptions( diff --git a/src/applications/conpherence/mail/ConpherenceThreadMailReceiver.php b/src/applications/conpherence/mail/ConpherenceThreadMailReceiver.php --- a/src/applications/conpherence/mail/ConpherenceThreadMailReceiver.php +++ b/src/applications/conpherence/mail/ConpherenceThreadMailReceiver.php @@ -13,7 +13,7 @@ } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)trim($pattern, 'Z'); + $id = (int)substr($pattern, 1); return id(new ConpherenceThreadQuery()) ->setViewer($viewer) diff --git a/src/applications/countdown/mail/PhabricatorCountdownMailReceiver.php b/src/applications/countdown/mail/PhabricatorCountdownMailReceiver.php --- a/src/applications/countdown/mail/PhabricatorCountdownMailReceiver.php +++ b/src/applications/countdown/mail/PhabricatorCountdownMailReceiver.php @@ -13,7 +13,7 @@ } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)substr($pattern, 4); + $id = (int)substr($pattern, 1); return id(new PhabricatorCountdownQuery()) ->setViewer($viewer) diff --git a/src/applications/differential/mail/DifferentialCreateMailReceiver.php b/src/applications/differential/mail/DifferentialCreateMailReceiver.php --- a/src/applications/differential/mail/DifferentialCreateMailReceiver.php +++ b/src/applications/differential/mail/DifferentialCreateMailReceiver.php @@ -9,14 +9,16 @@ protected function processReceivedMail( PhabricatorMetaMTAReceivedMail $mail, - PhabricatorUser $sender) { + PhutilEmailAddress $target) { + + $author = $this->getAuthor(); $attachments = $mail->getAttachments(); $files = array(); $errors = array(); if ($attachments) { $files = id(new PhabricatorFileQuery()) - ->setViewer($sender) + ->setViewer($author) ->withPHIDs($attachments) ->execute(); foreach ($files as $index => $file) { @@ -37,7 +39,7 @@ array( 'diff' => $file->loadFileData(), )); - $call->setUser($sender); + $call->setUser($author); try { $result = $call->execute(); $diffs[$file->getName()] = $result['uri']; @@ -56,7 +58,7 @@ array( 'diff' => $body, )); - $call->setUser($sender); + $call->setUser($author); try { $result = $call->execute(); $diffs[pht('Mail Body')] = $result['uri']; @@ -108,10 +110,10 @@ } id(new PhabricatorMetaMTAMail()) - ->addTos(array($sender->getPHID())) + ->addTos(array($author->getPHID())) ->setSubject($subject) ->setSubjectPrefix($subject_prefix) - ->setFrom($sender->getPHID()) + ->setFrom($author->getPHID()) ->setBody($body->render()) ->saveAndSend(); } diff --git a/src/applications/differential/mail/DifferentialRevisionMailReceiver.php b/src/applications/differential/mail/DifferentialRevisionMailReceiver.php --- a/src/applications/differential/mail/DifferentialRevisionMailReceiver.php +++ b/src/applications/differential/mail/DifferentialRevisionMailReceiver.php @@ -13,7 +13,7 @@ } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)trim($pattern, 'D'); + $id = (int)substr($pattern, 1); return id(new DifferentialRevisionQuery()) ->setViewer($viewer) diff --git a/src/applications/files/mail/FileCreateMailReceiver.php b/src/applications/files/mail/FileCreateMailReceiver.php --- a/src/applications/files/mail/FileCreateMailReceiver.php +++ b/src/applications/files/mail/FileCreateMailReceiver.php @@ -9,7 +9,8 @@ protected function processReceivedMail( PhabricatorMetaMTAReceivedMail $mail, - PhabricatorUser $sender) { + PhutilEmailAddress $target) { + $author = $this->getAuthor(); $attachment_phids = $mail->getAttachments(); if (empty($attachment_phids)) { @@ -21,6 +22,11 @@ $first_phid = head($attachment_phids); $mail->setRelatedPHID($first_phid); + $sender = $this->getSender(); + if (!$sender) { + return; + } + $attachment_count = count($attachment_phids); if ($attachment_count > 1) { $subject = pht('You successfully uploaded %d files.', $attachment_count); diff --git a/src/applications/files/mail/FileMailReceiver.php b/src/applications/files/mail/FileMailReceiver.php --- a/src/applications/files/mail/FileMailReceiver.php +++ b/src/applications/files/mail/FileMailReceiver.php @@ -12,7 +12,7 @@ } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)trim($pattern, 'F'); + $id = (int)substr($pattern, 1); return id(new PhabricatorFileQuery()) ->setViewer($viewer) diff --git a/src/applications/legalpad/mail/LegalpadMailReceiver.php b/src/applications/legalpad/mail/LegalpadMailReceiver.php --- a/src/applications/legalpad/mail/LegalpadMailReceiver.php +++ b/src/applications/legalpad/mail/LegalpadMailReceiver.php @@ -12,7 +12,7 @@ } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)trim($pattern, 'L'); + $id = (int)substr($pattern, 1); return id(new LegalpadDocumentQuery()) ->setViewer($viewer) diff --git a/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php b/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php --- a/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php +++ b/src/applications/maniphest/mail/ManiphestCreateMailReceiver.php @@ -9,15 +9,20 @@ protected function processReceivedMail( PhabricatorMetaMTAReceivedMail $mail, - PhabricatorUser $sender) { + PhutilEmailAddress $target) { - $task = ManiphestTask::initializeNewTask($sender); - $task->setOriginalEmailSource($mail->getHeader('From')); + $author = $this->getAuthor(); + $task = ManiphestTask::initializeNewTask($author); + + $from_address = $mail->newFromAddress(); + if ($from_address) { + $task->setOriginalEmailSource((string)$from_address); + } $handler = new ManiphestReplyHandler(); $handler->setMailReceiver($task); - $handler->setActor($sender); + $handler->setActor($author); $handler->setExcludeMailRecipientPHIDs( $mail->loadAllRecipientPHIDs()); if ($this->getApplicationEmail()) { diff --git a/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php b/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php --- a/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php +++ b/src/applications/maniphest/mail/ManiphestTaskMailReceiver.php @@ -12,7 +12,7 @@ } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)trim($pattern, 'T'); + $id = (int)substr($pattern, 1); return id(new ManiphestTaskQuery()) ->setViewer($viewer) diff --git a/src/applications/metamta/applicationpanel/PhabricatorMetaMTAApplicationEmailPanel.php b/src/applications/metamta/applicationpanel/PhabricatorMetaMTAApplicationEmailPanel.php --- a/src/applications/metamta/applicationpanel/PhabricatorMetaMTAApplicationEmailPanel.php +++ b/src/applications/metamta/applicationpanel/PhabricatorMetaMTAApplicationEmailPanel.php @@ -261,8 +261,12 @@ ->setName($config_default) ->setLimit(1) ->setValue($v_default) - ->setCaption(pht( - 'Used if the "From:" address does not map to a known account.'))); + ->setCaption( + pht( + 'Used if the "From:" address does not map to a user account. '. + 'Setting a default author will allow anyone on the public '. + 'internet to create objects in Phabricator by sending email to '. + 'this address.'))); if ($is_new) { $title = pht('New Address'); @@ -369,9 +373,17 @@ $email_space = null; } + $default_author_phid = $email->getDefaultAuthorPHID(); + if (!$default_author_phid) { + $default_author = phutil_tag('em', array(), pht('None')); + } else { + $default_author = $viewer->renderHandle($default_author_phid); + } + $rows[] = array( $email_space, $email->getAddress(), + $default_author, $button_edit, $button_remove, ); @@ -383,11 +395,13 @@ array( pht('Space'), pht('Email'), + pht('Default'), pht('Edit'), pht('Delete'), )); $table->setColumnClasses( array( + '', '', 'wide', 'action', @@ -398,6 +412,7 @@ array( PhabricatorSpacesNamespaceQuery::getViewerSpacesExist($viewer), true, + true, $is_edit, $is_edit, )); diff --git a/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php b/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php --- a/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php +++ b/src/applications/metamta/constants/MetaMTAReceivedMailStatus.php @@ -6,7 +6,6 @@ const STATUS_DUPLICATE = 'err:duplicate'; 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'; const STATUS_DISABLED_SENDER = 'err:disabled-sender'; const STATUS_NO_PUBLIC_MAIL = 'err:no-public-mail'; @@ -23,7 +22,6 @@ self::STATUS_DUPLICATE => pht('Duplicate Message'), self::STATUS_FROM_PHABRICATOR => pht('Phabricator Mail'), self::STATUS_NO_RECEIVERS => pht('No Receivers'), - self::STATUS_ABUNDANT_RECEIVERS => pht('Multiple Receivers'), self::STATUS_UNKNOWN_SENDER => pht('Unknown Sender'), self::STATUS_DISABLED_SENDER => pht('Disabled Sender'), self::STATUS_NO_PUBLIC_MAIL => pht('No Public Mail'), diff --git a/src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php b/src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php --- a/src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php +++ b/src/applications/metamta/management/PhabricatorMailManagementReceiveTestWorkflow.php @@ -33,6 +33,7 @@ } public function execute(PhutilArgumentParser $args) { + $viewer = $this->getViewer(); $console = PhutilConsole::getConsole(); $to = $args->getArg('to'); @@ -95,14 +96,23 @@ if (preg_match('/.+@.+/', $to)) { $header_content['to'] = $to; } else { + // We allow the user to use an object name instead of a real address // as a convenience. To build the mail, we build a similar message and // look for a receiver which will accept it. + + // In the general case, mail may be processed by multiple receivers, + // but mail to objects only ever has one receiver today. + $pseudohash = PhabricatorObjectMailReceiver::computeMailHash('x', 'y'); + + $raw_target = $to.'+1+'.$pseudohash; + $target = new PhutilEmailAddress($raw_target.'@local.cli'); + $pseudomail = id(new PhabricatorMetaMTAReceivedMail()) ->setHeaders( array( - 'to' => $to.'+1+'.$pseudohash, + 'to' => $raw_target, )); $receivers = id(new PhutilClassMapQuery()) @@ -112,7 +122,11 @@ $receiver = null; foreach ($receivers as $possible_receiver) { - if (!$possible_receiver->canAcceptMail($pseudomail)) { + $possible_receiver = id(clone $possible_receiver) + ->setViewer($viewer) + ->setSender($user); + + if (!$possible_receiver->canAcceptMail($pseudomail, $target)) { continue; } $receiver = $possible_receiver; diff --git a/src/applications/metamta/query/PhabricatorMetaMTAActorQuery.php b/src/applications/metamta/query/PhabricatorMetaMTAActorQuery.php --- a/src/applications/metamta/query/PhabricatorMetaMTAActorQuery.php +++ b/src/applications/metamta/query/PhabricatorMetaMTAActorQuery.php @@ -121,12 +121,12 @@ $actor->setEmailAddress($xuser->getAccountID()); - // NOTE: This effectively drops all outbound mail to unrecognized - // addresses unless "phabricator.allow-email-users" is set. See T12237 - // for context. - $allow_key = 'phabricator.allow-email-users'; - $allow_value = PhabricatorEnv::getEnvConfig($allow_key); - $actor->setIsVerified((bool)$allow_value); + // Circa T7477, it appears that we never intentionally send email to + // external users (even when they email "bugs@" to create a task). + // Mark these users as unverified so mail to them is always dropped. + // See also T12237. In the future, we might change this behavior. + + $actor->setIsVerified(false); } } diff --git a/src/applications/metamta/receiver/PhabricatorApplicationMailReceiver.php b/src/applications/metamta/receiver/PhabricatorApplicationMailReceiver.php --- a/src/applications/metamta/receiver/PhabricatorApplicationMailReceiver.php +++ b/src/applications/metamta/receiver/PhabricatorApplicationMailReceiver.php @@ -3,32 +3,105 @@ abstract class PhabricatorApplicationMailReceiver extends PhabricatorMailReceiver { + private $applicationEmail; + private $emailList; + private $author; + abstract protected function newApplication(); + final protected function setApplicationEmail( + PhabricatorMetaMTAApplicationEmail $email) { + $this->applicationEmail = $email; + return $this; + } + + final protected function getApplicationEmail() { + return $this->applicationEmail; + } + + final protected function setAuthor(PhabricatorUser $author) { + $this->author = $author; + return $this; + } + + final protected function getAuthor() { + return $this->author; + } + final public function isEnabled() { return $this->newApplication()->isInstalled(); } - final public function canAcceptMail(PhabricatorMetaMTAReceivedMail $mail) { - $application = $this->newApplication(); + final public function canAcceptMail( + PhabricatorMetaMTAReceivedMail $mail, + PhutilEmailAddress $target) { + $viewer = $this->getViewer(); + $sender = $this->getSender(); + + foreach ($this->loadApplicationEmailList() as $application_email) { + $create_address = $application_email->newAddress(); + + if (!PhabricatorMailUtil::matchAddresses($create_address, $target)) { + continue; + } - $application_emails = id(new PhabricatorMetaMTAApplicationEmailQuery()) - ->setViewer($viewer) - ->withApplicationPHIDs(array($application->getPHID())) - ->execute(); - - foreach ($mail->newTargetAddresses() as $address) { - foreach ($application_emails as $application_email) { - $create_address = $application_email->newAddress(); - if (PhabricatorMailUtil::matchAddresses($create_address, $address)) { - $this->setApplicationEmail($application_email); - return true; + if ($sender) { + $author = $sender; + } else { + $author_phid = $application_email->getDefaultAuthorPHID(); + + // If this mail isn't from a recognized sender and the target address + // does not have a default author, we can't accept it, and it's an + // error because you tried to send it here. + + // You either need to be sending from a real address or be sending to + // an address which accepts mail from the public internet. + + if (!$author_phid) { + throw new PhabricatorMetaMTAReceivedMailProcessingException( + MetaMTAReceivedMailStatus::STATUS_UNKNOWN_SENDER, + pht( + 'You are sending from an unrecognized email address to '. + 'an address which does not support public email ("%s").', + (string)$target)); + } + + $author = id(new PhabricatorPeopleQuery()) + ->setViewer($viewer) + ->withPHIDs(array($author_phid)) + ->executeOne(); + if (!$author) { + throw new Exception( + pht( + 'Application email ("%s") has an invalid default author ("%s").', + (string)$create_address, + $author_phid)); } } + + $this + ->setApplicationEmail($application_email) + ->setAuthor($author); + + return true; } return false; } + private function loadApplicationEmailList() { + if ($this->emailList === null) { + $viewer = $this->getViewer(); + $application = $this->newApplication(); + + $this->emailList = id(new PhabricatorMetaMTAApplicationEmailQuery()) + ->setViewer($viewer) + ->withApplicationPHIDs(array($application->getPHID())) + ->execute(); + } + + return $this->emailList; + } + } diff --git a/src/applications/metamta/receiver/PhabricatorMailReceiver.php b/src/applications/metamta/receiver/PhabricatorMailReceiver.php --- a/src/applications/metamta/receiver/PhabricatorMailReceiver.php +++ b/src/applications/metamta/receiver/PhabricatorMailReceiver.php @@ -2,167 +2,40 @@ abstract class PhabricatorMailReceiver extends Phobject { - private $applicationEmail; + private $viewer; + private $sender; - public function setApplicationEmail( - PhabricatorMetaMTAApplicationEmail $email) { - $this->applicationEmail = $email; + final public function setViewer(PhabricatorUser $viewer) { + $this->viewer = $viewer; return $this; } - public function getApplicationEmail() { - return $this->applicationEmail; + final public function getViewer() { + return $this->viewer; } - abstract public function isEnabled(); - abstract public function canAcceptMail(PhabricatorMetaMTAReceivedMail $mail); - - abstract protected function processReceivedMail( - PhabricatorMetaMTAReceivedMail $mail, - PhabricatorUser $sender); - - final public function receiveMail( - PhabricatorMetaMTAReceivedMail $mail, - PhabricatorUser $sender) { - $this->processReceivedMail($mail, $sender); + final public function setSender(PhabricatorUser $sender) { + $this->sender = $sender; + return $this; } - public function getViewer() { - return PhabricatorUser::getOmnipotentUser(); + final public function getSender() { + return $this->sender; } - public function validateSender( + abstract public function isEnabled(); + abstract public function canAcceptMail( PhabricatorMetaMTAReceivedMail $mail, - PhabricatorUser $sender) { - - $failure_reason = null; - if ($sender->getIsDisabled()) { - $failure_reason = pht( - 'Your account (%s) is disabled, so you can not interact with '. - 'Phabricator over email.', - $sender->getUsername()); - } else if ($sender->getIsStandardUser()) { - if (!$sender->getIsApproved()) { - $failure_reason = pht( - 'Your account (%s) has not been approved yet. You can not interact '. - 'with Phabricator over email until your account is approved.', - $sender->getUsername()); - } else if (PhabricatorUserEmail::isEmailVerificationRequired() && - !$sender->getIsEmailVerified()) { - $failure_reason = pht( - 'You have not verified the email address for your account (%s). '. - 'You must verify your email address before you can interact '. - 'with Phabricator over email.', - $sender->getUsername()); - } - } - - if ($failure_reason) { - throw new PhabricatorMetaMTAReceivedMailProcessingException( - MetaMTAReceivedMailStatus::STATUS_DISABLED_SENDER, - $failure_reason); - } - } - - /** - * 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); + PhutilEmailAddress $target); - $reasons = array(); - - // Try to find a user with this email address. - $user = PhabricatorUser::loadOneWithEmailAddress($from); - if ($user) { - return $user; - } else { - $reasons[] = pht( - 'This email was sent from "%s", but that address is not recognized by '. - 'Phabricator and does not correspond to any known user account.', - $raw_from); - } - - // 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) { - $from_obj = new PhutilEmailAddress($from); - $xuser = id(new PhabricatorExternalAccountQuery()) - ->setViewer($this->getViewer()) - ->withAccountTypes(array('email')) - ->withAccountDomains(array($from_obj->getDomainName(), 'self')) - ->withAccountIDs(array($from_obj->getAddress())) - ->requireCapabilities( - array( - PhabricatorPolicyCapability::CAN_VIEW, - PhabricatorPolicyCapability::CAN_EDIT, - )) - ->loadOneOrCreate(); - return $xuser->getPhabricatorUser(); - } else { - // NOTE: Currently, we'll always drop this mail (since it's headed to - // an unverified recipient). See T12237. These details are still useful - // because they'll appear in the mail logs and Mail web UI. - - $reasons[] = pht( - 'Phabricator is also not configured to allow unknown external users '. - 'to send mail to the system using just an email address.'); - $reasons[] = pht( - 'To interact with Phabricator, add this address ("%s") to your '. - 'account.', - $raw_from); - } - - if ($this->getApplicationEmail()) { - $application_email = $this->getApplicationEmail(); - $default_user_phid = $application_email->getConfigValue( - PhabricatorMetaMTAApplicationEmail::CONFIG_DEFAULT_AUTHOR); - - if ($default_user_phid) { - $user = id(new PhabricatorUser())->loadOneWhere( - 'phid = %s', - $default_user_phid); - if ($user) { - return $user; - } - - $reasons[] = pht( - 'Phabricator is misconfigured: the application email '. - '"%s" is set to user "%s", but that user does not exist.', - $application_email->getAddress(), - $default_user_phid); - } - } - - $reasons = implode("\n\n", $reasons); - - throw new PhabricatorMetaMTAReceivedMailProcessingException( - MetaMTAReceivedMailStatus::STATUS_UNKNOWN_SENDER, - $reasons); - } + abstract protected function processReceivedMail( + PhabricatorMetaMTAReceivedMail $mail, + PhutilEmailAddress $target); - /** - * Reduce an email address to its canonical form. For example, an address - * 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)); + final public function receiveMail( + PhabricatorMetaMTAReceivedMail $mail, + PhutilEmailAddress $target) { + $this->processReceivedMail($mail, $target); } } diff --git a/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php b/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php --- a/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php +++ b/src/applications/metamta/receiver/PhabricatorObjectMailReceiver.php @@ -29,52 +29,23 @@ final protected function processReceivedMail( PhabricatorMetaMTAReceivedMail $mail, - PhabricatorUser $sender) { - - $object = $this->loadObjectFromMail($mail, $sender); - $mail->setRelatedPHID($object->getPHID()); - - $this->processReceivedObjectMail($mail, $object, $sender); - - return $this; - } - - protected function processReceivedObjectMail( - PhabricatorMetaMTAReceivedMail $mail, - PhabricatorLiskDAO $object, - PhabricatorUser $sender) { + PhutilEmailAddress $target) { - $handler = $this->getTransactionReplyHandler(); - if ($handler) { - return $handler - ->setMailReceiver($object) - ->setActor($sender) - ->setExcludeMailRecipientPHIDs($mail->loadAllRecipientPHIDs()) - ->processEmail($mail); + $parts = $this->matchObjectAddress($target); + if (!$parts) { + // We should only make it here if we matched already in "canAcceptMail()", + // so this is a surprise. + throw new Exception( + pht( + 'Failed to parse object address ("%s") during processing.', + (string)$target)); } - throw new PhutilMethodNotImplementedException(); - } - - protected function getTransactionReplyHandler() { - return null; - } - - public function loadMailReceiverObject($pattern, PhabricatorUser $viewer) { - return $this->loadObject($pattern, $viewer); - } - - public function validateSender( - PhabricatorMetaMTAReceivedMail $mail, - PhabricatorUser $sender) { - - parent::validateSender($mail, $sender); - - $parts = $this->matchObjectAddressInMail($mail); $pattern = $parts['pattern']; + $sender = $this->getSender(); try { - $object = $this->loadObjectFromMail($mail, $sender); + $object = $this->loadObject($pattern, $sender); } catch (PhabricatorPolicyException $policy_exception) { throw new PhabricatorMetaMTAReceivedMailProcessingException( MetaMTAReceivedMailStatus::STATUS_POLICY_PROBLEM, @@ -95,7 +66,6 @@ } $sender_identifier = $parts['sender']; - if ($sender_identifier === 'public') { if (!PhabricatorEnv::getEnvConfig('metamta.public-replies')) { throw new PhabricatorMetaMTAReceivedMailProcessingException( @@ -136,28 +106,50 @@ 'is correct.', $pattern)); } + + $mail->setRelatedPHID($object->getPHID()); + $this->processReceivedObjectMail($mail, $object, $sender); + + return $this; } + protected function processReceivedObjectMail( + PhabricatorMetaMTAReceivedMail $mail, + PhabricatorLiskDAO $object, + PhabricatorUser $sender) { - final public function canAcceptMail(PhabricatorMetaMTAReceivedMail $mail) { - if ($this->matchObjectAddressInMail($mail)) { - return true; + $handler = $this->getTransactionReplyHandler(); + if ($handler) { + return $handler + ->setMailReceiver($object) + ->setActor($sender) + ->setExcludeMailRecipientPHIDs($mail->loadAllRecipientPHIDs()) + ->processEmail($mail); } - return false; + throw new PhutilMethodNotImplementedException(); } - private function matchObjectAddressInMail( - PhabricatorMetaMTAReceivedMail $mail) { + protected function getTransactionReplyHandler() { + return null; + } - foreach ($mail->newTargetAddresses() as $address) { - $parts = $this->matchObjectAddress($address); - if ($parts) { - return $parts; - } + public function loadMailReceiverObject($pattern, PhabricatorUser $viewer) { + return $this->loadObject($pattern, $viewer); + } + + final public function canAcceptMail( + PhabricatorMetaMTAReceivedMail $mail, + PhutilEmailAddress $target) { + + // If we don't have a valid sender user account, we can never accept + // mail to any object. + $sender = $this->getSender(); + if (!$sender) { + return false; } - return null; + return (bool)$this->matchObjectAddress($target); } private function matchObjectAddress(PhutilEmailAddress $address) { @@ -188,16 +180,6 @@ return $regexp; } - private function loadObjectFromMail( - PhabricatorMetaMTAReceivedMail $mail, - PhabricatorUser $sender) { - $parts = $this->matchObjectAddressInMail($mail); - - return $this->loadObject( - phutil_utf8_strtoupper($parts['pattern']), - $sender); - } - public static function computeMailHash($mail_key, $phid) { $hash = PhabricatorHash::digestWithNamedKey( $mail_key.$phid, diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAApplicationEmail.php b/src/applications/metamta/storage/PhabricatorMetaMTAApplicationEmail.php --- a/src/applications/metamta/storage/PhabricatorMetaMTAApplicationEmail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAApplicationEmail.php @@ -67,6 +67,9 @@ return idx($this->configData, $key, $default); } + public function getDefaultAuthorPHID() { + return $this->getConfigValue(self::CONFIG_DEFAULT_AUTHOR); + } public function getInUseMessage() { $applications = PhabricatorApplication::getAllApplications(); diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php --- a/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php @@ -125,6 +125,7 @@ } public function processReceivedMail() { + $viewer = $this->getViewer(); $sender = null; try { @@ -132,26 +133,94 @@ $this->dropMailAlreadyReceived(); $this->dropEmptyMail(); - $receiver = $this->loadReceiver(); - $sender = $receiver->loadSender($this); - $receiver->validateSender($this, $sender); - - $this->setAuthorPHID($sender->getPHID()); - - // Now that we've identified the sender, mark them as the author of - // any attached files. - $attachments = $this->getAttachments(); - if ($attachments) { - $files = id(new PhabricatorFileQuery()) - ->setViewer(PhabricatorUser::getOmnipotentUser()) - ->withPHIDs($attachments) - ->execute(); - foreach ($files as $file) { - $file->setAuthorPHID($sender->getPHID())->save(); + $sender = $this->loadSender(); + if ($sender) { + $this->setAuthorPHID($sender->getPHID()); + + // If we've identified the sender, mark them as the author of any + // attached files. We do this before we validate them (below), since + // they still authored these files even if their account is not allowed + // to interact via email. + + $attachments = $this->getAttachments(); + if ($attachments) { + $files = id(new PhabricatorFileQuery()) + ->setViewer($viewer) + ->withPHIDs($attachments) + ->execute(); + foreach ($files as $file) { + $file->setAuthorPHID($sender->getPHID())->save(); + } + } + + $this->validateSender($sender); + } + + $receivers = id(new PhutilClassMapQuery()) + ->setAncestorClass('PhabricatorMailReceiver') + ->setFilterMethod('isEnabled') + ->execute(); + + $any_accepted = false; + $receiver_exception = null; + + $targets = $this->newTargetAddresses(); + foreach ($receivers as $receiver) { + $receiver = id(clone $receiver) + ->setViewer($viewer); + + if ($sender) { + $receiver->setSender($sender); + } + + foreach ($targets as $target) { + try { + if (!$receiver->canAcceptMail($this, $target)) { + continue; + } + + $any_accepted = true; + + $receiver->receiveMail($this, $target); + } catch (Exception $ex) { + // If receivers raise exceptions, we'll keep the first one in hope + // that it points at a root cause. + if (!$receiver_exception) { + $receiver_exception = $ex; + } + } } } - $receiver->receiveMail($this, $sender); + if ($receiver_exception) { + throw $receiver_exception; + } + + if (!$any_accepted) { + if (!$sender) { + // NOTE: Currently, we'll always drop this mail (since it's headed to + // an unverified recipient). See T12237. These details are still + // useful because they'll appear in the mail logs and Mail web UI. + + throw new PhabricatorMetaMTAReceivedMailProcessingException( + MetaMTAReceivedMailStatus::STATUS_UNKNOWN_SENDER, + pht( + 'This email was sent from an email address ("%s") that is not '. + 'associated with a Phabricator account. To interact with '. + 'Phabricator via email, add this address to your account.', + (string)$this->newFromAddress())); + } else { + throw new PhabricatorMetaMTAReceivedMailProcessingException( + MetaMTAReceivedMailStatus::STATUS_NO_RECEIVERS, + pht( + 'Phabricator can not process this mail because no application '. + 'knows how to handle it. Check that the address you sent it to '. + 'is correct.'. + "\n\n". + '(No concrete, enabled subclass of PhabricatorMailReceiver can '. + 'accept this mail.)')); + } + } } catch (PhabricatorMetaMTAReceivedMailProcessingException $ex) { switch ($ex->getStatusCode()) { case MetaMTAReceivedMailStatus::STATUS_DUPLICATE: @@ -311,51 +380,6 @@ 'text, and signatures are discarded and ignored.')); } - /** - * Load a concrete instance of the @{class:PhabricatorMailReceiver} which - * accepts this mail, if one exists. - */ - private function loadReceiver() { - $receivers = id(new PhutilClassMapQuery()) - ->setAncestorClass('PhabricatorMailReceiver') - ->setFilterMethod('isEnabled') - ->execute(); - - $accept = array(); - foreach ($receivers as $key => $receiver) { - if ($receiver->canAcceptMail($this)) { - $accept[$key] = $receiver; - } - } - - if (!$accept) { - throw new PhabricatorMetaMTAReceivedMailProcessingException( - MetaMTAReceivedMailStatus::STATUS_NO_RECEIVERS, - pht( - 'Phabricator can not process this mail because no application '. - 'knows how to handle it. Check that the address you sent it to is '. - 'correct.'. - "\n\n". - '(No concrete, enabled subclass of PhabricatorMailReceiver can '. - 'accept this mail.)')); - } - - if (count($accept) > 1) { - $names = implode(', ', array_keys($accept)); - throw new PhabricatorMetaMTAReceivedMailProcessingException( - MetaMTAReceivedMailStatus::STATUS_ABUNDANT_RECEIVERS, - pht( - 'Phabricator is not able to process this mail because more than '. - 'one application is willing to accept it, creating ambiguity. '. - 'Mail needs to be accepted by exactly one receiving application.'. - "\n\n". - 'Accepting receivers: %s.', - $names)); - } - - return head($accept); - } - private function sendExceptionMail( Exception $ex, PhabricatorUser $viewer = null) { @@ -434,4 +458,74 @@ )); } + public function newFromAddress() { + $raw_from = $this->getHeader('From'); + + if (strlen($raw_from)) { + return new PhutilEmailAddress($raw_from); + } + + return null; + } + + private function getViewer() { + return PhabricatorUser::getOmnipotentUser(); + } + + /** + * Identify 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. + */ + private function loadSender() { + $viewer = $this->getViewer(); + + // Try to identify the user based on their "From" address. + $from_address = $this->newFromAddress(); + if ($from_address) { + $user = id(new PhabricatorPeopleQuery()) + ->setViewer($viewer) + ->withEmails(array($from_address->getAddress())) + ->executeOne(); + if ($user) { + return $user; + } + } + + return null; + } + + private function validateSender(PhabricatorUser $sender) { + $failure_reason = null; + if ($sender->getIsDisabled()) { + $failure_reason = pht( + 'Your account ("%s") is disabled, so you can not interact with '. + 'Phabricator over email.', + $sender->getUsername()); + } else if ($sender->getIsStandardUser()) { + if (!$sender->getIsApproved()) { + $failure_reason = pht( + 'Your account ("%s") has not been approved yet. You can not '. + 'interact with Phabricator over email until your account is '. + 'approved.', + $sender->getUsername()); + } else if (PhabricatorUserEmail::isEmailVerificationRequired() && + !$sender->getIsEmailVerified()) { + $failure_reason = pht( + 'You have not verified the email address for your account ("%s"). '. + 'You must verify your email address before you can interact '. + 'with Phabricator over email.', + $sender->getUsername()); + } + } + + if ($failure_reason) { + throw new PhabricatorMetaMTAReceivedMailProcessingException( + MetaMTAReceivedMailStatus::STATUS_DISABLED_SENDER, + $failure_reason); + } + } + } diff --git a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php --- a/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php +++ b/src/applications/metamta/storage/__tests__/PhabricatorMetaMTAReceivedMailTestCase.php @@ -48,11 +48,15 @@ } public function testDropUnreceivableMail() { + $user = $this->generateNewTestUser() + ->save(); + $mail = new PhabricatorMetaMTAReceivedMail(); $mail->setHeaders( array( 'Message-ID' => 'test@example.com', 'To' => 'does+not+exist@example.com', + 'From' => $user->loadPrimaryEmail()->getAddress(), )); $mail->setBodies( array( @@ -70,10 +74,6 @@ public function testDropUnknownSenderMail() { $this->setManiphestCreateEmail(); - $env = PhabricatorEnv::beginScopedEnv(); - $env->overrideEnvConfig('phabricator.allow-email-users', false); - $env->overrideEnvConfig('metamta.maniphest.default-public-author', null); - $mail = new PhabricatorMetaMTAReceivedMail(); $mail->setHeaders( array( diff --git a/src/applications/paste/mail/PasteCreateMailReceiver.php b/src/applications/paste/mail/PasteCreateMailReceiver.php --- a/src/applications/paste/mail/PasteCreateMailReceiver.php +++ b/src/applications/paste/mail/PasteCreateMailReceiver.php @@ -9,7 +9,8 @@ protected function processReceivedMail( PhabricatorMetaMTAReceivedMail $mail, - PhabricatorUser $sender) { + PhutilEmailAddress $target) { + $author = $this->getAuthor(); $title = $mail->getSubject(); if (!$title) { @@ -26,18 +27,23 @@ ->setTransactionType(PhabricatorPasteTitleTransaction::TRANSACTIONTYPE) ->setNewValue($title); - $paste = PhabricatorPaste::initializeNewPaste($sender); + $paste = PhabricatorPaste::initializeNewPaste($author); $content_source = $mail->newContentSource(); $editor = id(new PhabricatorPasteEditor()) - ->setActor($sender) + ->setActor($author) ->setContentSource($content_source) ->setContinueOnNoEffect(true); $xactions = $editor->applyTransactions($paste, $xactions); $mail->setRelatedPHID($paste->getPHID()); + $sender = $this->getSender(); + if (!$sender) { + return; + } + $subject_prefix = PhabricatorEnv::getEnvConfig('metamta.paste.subject-prefix'); $subject = pht('You successfully created a paste.'); @@ -56,5 +62,4 @@ ->saveAndSend(); } - } diff --git a/src/applications/paste/mail/PasteMailReceiver.php b/src/applications/paste/mail/PasteMailReceiver.php --- a/src/applications/paste/mail/PasteMailReceiver.php +++ b/src/applications/paste/mail/PasteMailReceiver.php @@ -12,7 +12,7 @@ } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)trim($pattern, 'P'); + $id = (int)substr($pattern, 1); return id(new PhabricatorPasteQuery()) ->setViewer($viewer) diff --git a/src/applications/phame/mail/PhamePostMailReceiver.php b/src/applications/phame/mail/PhamePostMailReceiver.php --- a/src/applications/phame/mail/PhamePostMailReceiver.php +++ b/src/applications/phame/mail/PhamePostMailReceiver.php @@ -13,7 +13,7 @@ } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)substr($pattern, 4); + $id = (int)substr($pattern, 1); return id(new PhamePostQuery()) ->setViewer($viewer) diff --git a/src/applications/pholio/mail/PholioMockMailReceiver.php b/src/applications/pholio/mail/PholioMockMailReceiver.php --- a/src/applications/pholio/mail/PholioMockMailReceiver.php +++ b/src/applications/pholio/mail/PholioMockMailReceiver.php @@ -12,7 +12,7 @@ } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)trim($pattern, 'M'); + $id = (int)substr($pattern, 1); return id(new PholioMockQuery()) ->setViewer($viewer) diff --git a/src/applications/ponder/mail/PonderAnswerMailReceiver.php b/src/applications/ponder/mail/PonderAnswerMailReceiver.php --- a/src/applications/ponder/mail/PonderAnswerMailReceiver.php +++ b/src/applications/ponder/mail/PonderAnswerMailReceiver.php @@ -12,7 +12,7 @@ } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)trim($pattern, 'ANSR'); + $id = (int)substr($pattern, 4); return id(new PonderAnswerQuery()) ->setViewer($viewer) diff --git a/src/applications/ponder/mail/PonderQuestionCreateMailReceiver.php b/src/applications/ponder/mail/PonderQuestionCreateMailReceiver.php --- a/src/applications/ponder/mail/PonderQuestionCreateMailReceiver.php +++ b/src/applications/ponder/mail/PonderQuestionCreateMailReceiver.php @@ -9,7 +9,8 @@ protected function processReceivedMail( PhabricatorMetaMTAReceivedMail $mail, - PhabricatorUser $sender) { + PhutilEmailAddress $target) { + $author = $this->getAuthor(); $title = $mail->getSubject(); if (!strlen($title)) { @@ -26,18 +27,17 @@ ->setTransactionType(PonderQuestionTransaction::TYPE_CONTENT) ->setNewValue($mail->getCleanTextBody()); - $question = PonderQuestion::initializeNewQuestion($sender); + $question = PonderQuestion::initializeNewQuestion($author); $content_source = $mail->newContentSource(); $editor = id(new PonderQuestionEditor()) - ->setActor($sender) + ->setActor($author) ->setContentSource($content_source) ->setContinueOnNoEffect(true); $xactions = $editor->applyTransactions($question, $xactions); $mail->setRelatedPHID($question->getPHID()); - } diff --git a/src/applications/ponder/mail/PonderQuestionMailReceiver.php b/src/applications/ponder/mail/PonderQuestionMailReceiver.php --- a/src/applications/ponder/mail/PonderQuestionMailReceiver.php +++ b/src/applications/ponder/mail/PonderQuestionMailReceiver.php @@ -12,7 +12,7 @@ } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)trim($pattern, 'Q'); + $id = (int)substr($pattern, 1); return id(new PonderQuestionQuery()) ->setViewer($viewer) diff --git a/src/applications/slowvote/mail/PhabricatorSlowvoteMailReceiver.php b/src/applications/slowvote/mail/PhabricatorSlowvoteMailReceiver.php --- a/src/applications/slowvote/mail/PhabricatorSlowvoteMailReceiver.php +++ b/src/applications/slowvote/mail/PhabricatorSlowvoteMailReceiver.php @@ -13,7 +13,7 @@ } protected function loadObject($pattern, PhabricatorUser $viewer) { - $id = (int)substr($pattern, 4); + $id = (int)substr($pattern, 1); return id(new PhabricatorSlowvoteQuery()) ->setViewer($viewer)