diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -371,7 +371,6 @@ 'DifferentialDoorkeeperRevisionFeedStoryPublisher' => 'applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php', 'DifferentialDraft' => 'applications/differential/storage/DifferentialDraft.php', 'DifferentialEditPolicyField' => 'applications/differential/customfield/DifferentialEditPolicyField.php', - 'DifferentialExceptionMail' => 'applications/differential/mail/DifferentialExceptionMail.php', 'DifferentialFieldParseException' => 'applications/differential/exception/DifferentialFieldParseException.php', 'DifferentialFieldValidationException' => 'applications/differential/exception/DifferentialFieldValidationException.php', 'DifferentialGetWorkingCopy' => 'applications/differential/DifferentialGetWorkingCopy.php', @@ -398,7 +397,6 @@ 'DifferentialLintStatus' => 'applications/differential/constants/DifferentialLintStatus.php', 'DifferentialLocalCommitsView' => 'applications/differential/view/DifferentialLocalCommitsView.php', 'DifferentialMail' => 'applications/differential/mail/DifferentialMail.php', - 'DifferentialMailPhase' => 'applications/differential/constants/DifferentialMailPhase.php', 'DifferentialManiphestTasksField' => 'applications/differential/customfield/DifferentialManiphestTasksField.php', 'DifferentialPHIDTypeDiff' => 'applications/differential/phid/DifferentialPHIDTypeDiff.php', 'DifferentialPHIDTypeRevision' => 'applications/differential/phid/DifferentialPHIDTypeRevision.php', @@ -1693,6 +1691,7 @@ 'PhabricatorMetaMTADAO' => 'applications/metamta/storage/PhabricatorMetaMTADAO.php', 'PhabricatorMetaMTAEmailBodyParser' => 'applications/metamta/parser/PhabricatorMetaMTAEmailBodyParser.php', 'PhabricatorMetaMTAEmailBodyParserTestCase' => 'applications/metamta/parser/__tests__/PhabricatorMetaMTAEmailBodyParserTestCase.php', + 'PhabricatorMetaMTAErrorMailAction' => 'applications/metamta/action/PhabricatorMetaMTAErrorMailAction.php', 'PhabricatorMetaMTAMail' => 'applications/metamta/storage/PhabricatorMetaMTAMail.php', 'PhabricatorMetaMTAMailBody' => 'applications/metamta/view/PhabricatorMetaMTAMailBody.php', 'PhabricatorMetaMTAMailBodyTestCase' => 'applications/metamta/view/__tests__/PhabricatorMetaMTAMailBodyTestCase.php', @@ -2952,7 +2951,6 @@ 'DifferentialDoorkeeperRevisionFeedStoryPublisher' => 'DoorkeeperFeedStoryPublisher', 'DifferentialDraft' => 'DifferentialDAO', 'DifferentialEditPolicyField' => 'DifferentialCoreCustomField', - 'DifferentialExceptionMail' => 'DifferentialMail', 'DifferentialFieldParseException' => 'Exception', 'DifferentialFieldValidationException' => 'Exception', 'DifferentialGitSVNIDField' => 'DifferentialCustomField', @@ -4487,6 +4485,7 @@ 'PhabricatorMetaMTAController' => 'PhabricatorController', 'PhabricatorMetaMTADAO' => 'PhabricatorLiskDAO', 'PhabricatorMetaMTAEmailBodyParserTestCase' => 'PhabricatorTestCase', + 'PhabricatorMetaMTAErrorMailAction' => 'PhabricatorSystemAction', 'PhabricatorMetaMTAMail' => 'PhabricatorMetaMTADAO', 'PhabricatorMetaMTAMailBodyTestCase' => 'PhabricatorTestCase', 'PhabricatorMetaMTAMailTestCase' => 'PhabricatorTestCase', diff --git a/src/applications/differential/constants/DifferentialMailPhase.php b/src/applications/differential/constants/DifferentialMailPhase.php deleted file mode 100644 --- a/src/applications/differential/constants/DifferentialMailPhase.php +++ /dev/null @@ -1,9 +0,0 @@ -revision = $revision; - $this->exception = $exception; - $this->originalBody = $original_body; - } - - protected function renderBody() { - // Never called since buildBody() is overridden. - } - - protected function renderSubject() { - return "Exception: unable to process your mail request"; - } - - protected function renderVaryPrefix() { - return ''; - } - - protected function buildBody() { - $exception = $this->exception; - $original_body = $this->originalBody; - - $message = $exception->getMessage(); - - return <<setContentSource($content_source); } - try { - $editor->applyTransactions($this->getMailReceiver(), $xactions); - } catch (Exception $ex) { - if ($this->receivedMail) { - $error_body = $this->receivedMail->getRawTextBody(); - } else { - $error_body = $body; - } - $exception_mail = new DifferentialExceptionMail( - $this->getMailReceiver(), - $ex, - $error_body); - - $exception_mail->setActor($this->getActor()); - $exception_mail->setToPHIDs(array($this->getActor()->getPHID())); - $exception_mail->send(); - - throw $ex; - } + $editor->applyTransactions($this->getMailReceiver(), $xactions); } } diff --git a/src/applications/metamta/action/PhabricatorMetaMTAErrorMailAction.php b/src/applications/metamta/action/PhabricatorMetaMTAErrorMailAction.php new file mode 100644 --- /dev/null +++ b/src/applications/metamta/action/PhabricatorMetaMTAErrorMailAction.php @@ -0,0 +1,13 @@ + 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'), + self::STATUS_USER_MISMATCH => pht('User Mismatch'), + self::STATUS_POLICY_PROBLEM => pht('Policy Error'), + self::STATUS_NO_SUCH_OBJECT => pht('No Such Object'), + self::STATUS_HASH_MISMATCH => pht('Bad Address'), + self::STATUS_UNHANDLED_EXCEPTION => pht('Unhandled Exception'), + ); + + return idx($map, $status, pht('Processing Exception')); + } + } 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 @@ -19,12 +19,32 @@ PhabricatorMetaMTAReceivedMail $mail, PhabricatorUser $sender) { - if (!$sender->isUserActivated()) { + $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, - pht( - "Sender '%s' does not have an activated user account.", - $sender->getUsername())); + $failure_reason); } } @@ -46,33 +66,34 @@ return $user; } else { $reasons[] = pht( - "The email was sent from '%s', but this address does not correspond ". - "to any user account.", + '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 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; + $raw_reply_to = $mail->getHeader('Reply-To'); + if (strlen($raw_reply_to)) { + $reply_to_key = 'metamta.insecure-auth-with-reply-to'; + $allow_reply_to = PhabricatorEnv::getEnvConfig($reply_to_key); + if ($allow_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 authenticate users using the '. + '"Reply-To" header, but the reply address ("%s") on this '. + 'message does not correspond to any known user account.', + $raw_reply_to); + } } 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); + '(Phabricator is not configured to authenticate users using the '. + '"Reply-To" header, so it was ignored.)'); } - } 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 @@ -82,7 +103,7 @@ if ($allow_email_users) { $from_obj = new PhutilEmailAddress($from); $xuser = id(new PhabricatorExternalAccountQuery()) - ->setViewer($user) + ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withAccountTypes(array('email')) ->withAccountDomains(array($from_obj->getDomainName(), 'self')) ->withAccountIDs(array($from_obj->getAddress())) @@ -90,15 +111,19 @@ 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."); + '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); } + $reasons = implode("\n\n", $reasons); + throw new PhabricatorMetaMTAReceivedMailProcessingException( MetaMTAReceivedMailStatus::STATUS_UNKNOWN_SENDER, - pht('Unknown sender: %s', implode(' ', $reasons))); + $reasons); } /** 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 @@ -55,6 +55,7 @@ parent::validateSender($mail, $sender); $parts = $this->matchObjectAddressInMail($mail); + $pattern = $parts['pattern']; try { $object = $this->loadObjectFromMail($mail, $sender); @@ -62,8 +63,9 @@ throw new PhabricatorMetaMTAReceivedMailProcessingException( MetaMTAReceivedMailStatus::STATUS_POLICY_PROBLEM, pht( - "This mail is addressed to an object you are not permitted ". - "to see: %s", + 'This mail is addressed to an object ("%s") you do not have '. + 'permission to see: %s', + $pattern, $policy_exception->getMessage())); } @@ -71,9 +73,9 @@ throw new PhabricatorMetaMTAReceivedMailProcessingException( MetaMTAReceivedMailStatus::STATUS_NO_SUCH_OBJECT, pht( - "This mail is addressed to an object ('%s'), but that object ". - "does not exist.", - $parts['pattern'])); + 'This mail is addressed to an object ("%s"), but that object '. + 'does not exist.', + $pattern)); } $sender_identifier = $parts['sender']; @@ -83,8 +85,12 @@ throw new PhabricatorMetaMTAReceivedMailProcessingException( MetaMTAReceivedMailStatus::STATUS_NO_PUBLIC_MAIL, pht( - "This mail is addressed to an object's public address, but ". - "public replies are not enabled (`metamta.public-replies`).")); + 'This mail is addressed to the public email address of an object '. + '("%s"), but public replies are not enabled on this Phabricator '. + 'install. An administrator may have recently disabled this '. + 'setting, or you may have replied to an old message. Try '. + 'replying to a more recent message instead.', + $pattern)); } $check_phid = $object->getPHID(); } else { @@ -92,9 +98,12 @@ throw new PhabricatorMetaMTAReceivedMailProcessingException( MetaMTAReceivedMailStatus::STATUS_USER_MISMATCH, pht( - "This mail is addressed to an object's private address, but ". - "the sending user and the private address owner are not the ". - "same user.")); + 'This mail is addressed to the private email address of an object '. + '("%s"), but you are not the user who is authorized to use the '. + 'address you sent mail to. Each private address is unique to the '. + 'user who received the original mail. Try replying to a message '. + 'which was sent directly to you instead.', + $pattern)); } $check_phid = $sender->getPHID(); } @@ -105,8 +114,10 @@ throw new PhabricatorMetaMTAReceivedMailProcessingException( MetaMTAReceivedMailStatus::STATUS_HASH_MISMATCH, pht( - "The hash in this object's address does not match the expected ". - "value.")); + 'This mail is addressed to an object ("%s"), but the address is '. + 'not correct (the security hash is wrong). Check that the address '. + 'is correct.', + $pattern)); } } diff --git a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php --- a/src/applications/metamta/storage/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/PhabricatorMetaMTAMail.php @@ -222,6 +222,15 @@ return $this; } + public function setIsErrorEmail($is_error) { + $this->setParam('is-error', $is_error); + return $this; + } + + public function getIsErrorEmail() { + return $this->getParam('is-error', false); + } + public function getWorkerTaskID() { return $this->getParam('worker-task'); } @@ -549,6 +558,19 @@ return $this->save(); } + if ($this->getIsErrorEmail()) { + $all_recipients = array_merge($add_to, $add_cc); + if ($this->shouldRateLimitMail($all_recipients)) { + $this->setStatus(self::STATUS_VOID); + $this->setMessage( + pht( + 'This is an error email, but one or more recipients have '. + 'exceeded the error email rate limit. Declining to deliver '. + 'message.')); + return $this->save(); + } + } + $mailer->addHeader('X-Phabricator-Sent-This-Message', 'Yes'); $mailer->addHeader('X-Mail-Transport-Agent', 'MetaMTA'); @@ -839,5 +861,17 @@ return $actors; } + private function shouldRateLimitMail(array $all_recipients) { + try { + PhabricatorSystemActionEngine::willTakeAction( + $all_recipients, + new PhabricatorMetaMTAErrorMailAction(), + 1); + return false; + } catch (PhabricatorSystemActionRateLimitException $ex) { + return true; + } + } + } 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 @@ -102,12 +102,25 @@ $receiver->receiveMail($this, $sender); } catch (PhabricatorMetaMTAReceivedMailProcessingException $ex) { + switch ($ex->getStatusCode()) { + case MetaMTAReceivedMailStatus::STATUS_DUPLICATE: + case MetaMTAReceivedMailStatus::STATUS_FROM_PHABRICATOR: + // Don't send an error email back in these cases, since they're + // very unlikely to be the sender's fault. + break; + default: + $this->sendExceptionMail($ex); + break; + } + $this ->setStatus($ex->getStatusCode()) ->setMessage($ex->getMessage()) ->save(); return $this; } catch (Exception $ex) { + $this->sendExceptionMail($ex); + $this ->setStatus(MetaMTAReceivedMailStatus::STATUS_UNHANDLED_EXCEPTION) ->setMessage(pht('Unhandled Exception: %s', $ex->getMessage())) @@ -169,8 +182,9 @@ throw new PhabricatorMetaMTAReceivedMailProcessingException( MetaMTAReceivedMailStatus::STATUS_FROM_PHABRICATOR, - "Ignoring email with 'X-Phabricator-Sent-This-Message' header to avoid ". - "loops."); + pht( + "Ignoring email with 'X-Phabricator-Sent-This-Message' header to ". + "avoid loops.")); } /** @@ -203,8 +217,8 @@ return; } - $message = sprintf( - 'Ignoring email with message id hash "%s" that has been seen %d '. + $message = pht( + 'Ignoring email with "Message-ID" hash "%s" that has been seen %d '. 'times, including this message.', $message_id_hash, $messages_count); @@ -237,18 +251,70 @@ if (!$accept) { throw new PhabricatorMetaMTAReceivedMailProcessingException( MetaMTAReceivedMailStatus::STATUS_NO_RECEIVERS, - "No concrete, enabled subclasses of `PhabricatorMailReceiver` can ". - "accept this mail."); + 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, - "More than one `PhabricatorMailReceiver` claims to accept this mail."); + 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) { + $from = $this->getHeader('from'); + if (!strlen($from)) { + return; + } + + if ($ex instanceof PhabricatorMetaMTAReceivedMailProcessingException) { + $status_code = $ex->getStatusCode(); + $status_name = MetaMTAReceivedMailStatus::getHumanReadableName( + $status_code); + + $title = pht('Error Processing Mail (%s)', $status_name); + $description = $ex->getMessage(); + } else { + $title = pht('Error Processing Mail (%s)', get_class($ex)); + $description = pht('%s: %s', get_class($ex), $ex->getMessage()); + } + + $body = pht(<<getRawTextBody()); + + $mail = id(new PhabricatorMetaMTAMail()) + ->setIsErrorEmail(true) + ->setSubject($title) + ->addRawTos(array($from)) + ->setBody($body) + ->saveAndSend(); + } + } diff --git a/src/applications/people/storage/PhabricatorUser.php b/src/applications/people/storage/PhabricatorUser.php --- a/src/applications/people/storage/PhabricatorUser.php +++ b/src/applications/people/storage/PhabricatorUser.php @@ -90,6 +90,18 @@ return true; } + /** + * Returns `true` if this is a standard user who is logged in. Returns `false` + * for logged out, anonymous, or external users. + * + * @return bool `true` if the user is a standard user who is logged in with + * a normal session. + */ + public function getIsStandardUser() { + $type_user = PhabricatorPeoplePHIDTypeUser::TYPECONST; + return $this->getPHID() && (phid_get_type($this->getPHID()) == $type_user); + } + public function getConfiguration() { return array( self::CONFIG_AUX_PHID => true, diff --git a/src/applications/system/engine/PhabricatorSystemActionEngine.php b/src/applications/system/engine/PhabricatorSystemActionEngine.php --- a/src/applications/system/engine/PhabricatorSystemActionEngine.php +++ b/src/applications/system/engine/PhabricatorSystemActionEngine.php @@ -20,7 +20,9 @@ } } - self::recordAction($actors, $action, $score); + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + self::recordAction($actors, $action, $score); + unset($unguarded); } public static function loadBlockedActors(