From 98c8e150b0747d8c03269eb5683ee07c92d3b31b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 16 Nov 2011 09:49:18 -0800 Subject: [PATCH] Prevent delivery of email to disabled objects Summary: See T625. Facebook's REST-based MTA layer had a check for this so I overlooked it in porting it out. We should not attempt to deliver email to disabled users. Test Plan: Used MetaMTA console to send email to: - No users: received "no To" exception. - A disabled user: received "all To disabled" exception. - A valid user: received email. - A valid user and a disabled user: received email to valid user only. (Note that you can't easily send to disabled users directly since they don't appear in the typeahead, but you can prefill it and then disable the user by hitting "Send".) Reviewers: btrahan, jungejason, nh, tuomaspelkonen, aran Reviewed By: aran CC: skrul, aran, epriestley Differential Revision: 1120 --- .../storage/mail/PhabricatorMetaMTAMail.php | 39 +++++++++++++++---- .../people/storage/user/PhabricatorUser.php | 30 ++++++++------ .../phid/handle/PhabricatorObjectHandle.php | 38 ++++++++++++++++++ .../data/PhabricatorObjectHandleData.php | 1 + 4 files changed, 89 insertions(+), 19 deletions(-) diff --git a/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php b/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php index 386ac84d7f..3f51168f82 100644 --- a/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php +++ b/src/applications/metamta/storage/mail/PhabricatorMetaMTAMail.php @@ -296,18 +296,23 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { $mailer->addReplyTo($value, $reply_to_name); break; case 'to': - $emails = array(); - foreach ($value as $phid) { - $emails[] = $handles[$phid]->getEmail(); + $emails = $this->getDeliverableEmailsFromHandles($value, $handles); + if ($emails) { + $mailer->addTos($emails); + } else { + if ($value) { + throw new Exception( + "All 'To' objects are undeliverable (e.g., disabled users)."); + } else { + throw new Exception("No 'To' specified!"); + } } - $mailer->addTos($emails); break; case 'cc': - $emails = array(); - foreach ($value as $phid) { - $emails[] = $handles[$phid]->getEmail(); + $emails = $this->getDeliverableEmailsFromHandles($value, $handles); + if ($emails) { + $mailer->addCCs($emails); } - $mailer->addCCs($emails); break; case 'headers': foreach ($value as $header_key => $header_value) { @@ -444,4 +449,22 @@ class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO { return base64_encode($base); } + private function getDeliverableEmailsFromHandles( + array $phids, + array $handles) { + + $emails = array(); + foreach ($phids as $phid) { + if ($handles[$phid]->isDisabled()) { + continue; + } + if (!$handles[$phid]->isComplete()) { + continue; + } + $emails[] = $handles[$phid]->getEmail(); + } + + return $emails; + } + } diff --git a/src/applications/people/storage/user/PhabricatorUser.php b/src/applications/people/storage/user/PhabricatorUser.php index 433cfd2c17..ca5eb1b5cd 100644 --- a/src/applications/people/storage/user/PhabricatorUser.php +++ b/src/applications/people/storage/user/PhabricatorUser.php @@ -43,18 +43,26 @@ class PhabricatorUser extends PhabricatorUserDAO { private $preferences = null; protected function readField($field) { - if ($field === 'profileImagePHID') { - return nonempty( - $this->profileImagePHID, - PhabricatorEnv::getEnvConfig('user.default-profile-image-phid')); + switch ($field) { + case 'profileImagePHID': + return nonempty( + $this->profileImagePHID, + PhabricatorEnv::getEnvConfig('user.default-profile-image-phid')); + case 'timezoneIdentifier': + // If the user hasn't set one, guess the server's time. + return nonempty( + $this->timezoneIdentifier, + date_default_timezone_get()); + // Make sure these return booleans. + case 'isAdmin': + return (bool)$this->isAdmin; + case 'isDisabled': + return (bool)$this->isDisabled; + case 'isSystemAgent': + return (bool)$this->isSystemAgent; + default: + return parent::readField($field); } - if ($field === 'timezoneIdentifier') { - // If the user hasn't set one, guess the server's time. - return nonempty( - $this->timezoneIdentifier, - date_default_timezone_get()); - } - return parent::readField($field); } public function getConfiguration() { diff --git a/src/applications/phid/handle/PhabricatorObjectHandle.php b/src/applications/phid/handle/PhabricatorObjectHandle.php index 050c5a2849..cb1ae91f9f 100644 --- a/src/applications/phid/handle/PhabricatorObjectHandle.php +++ b/src/applications/phid/handle/PhabricatorObjectHandle.php @@ -29,6 +29,7 @@ class PhabricatorObjectHandle { private $alternateID; private $status = 'open'; private $complete; + private $disabled; public function setURI($uri) { $this->uri = $uri; @@ -135,11 +136,20 @@ class PhabricatorObjectHandle { return idx($map, $this->getType()); } + + /** + * Set whether or not the underlying object is complete. See + * @{method:getComplete} for an explanation of what it means to be complete. + * + * @param bool True if the handle represents a complete object. + * @return this + */ public function setComplete($complete) { $this->complete = $complete; return $this; } + /** * Determine if the handle represents an object which was completely loaded * (i.e., the underlying object exists) vs an object which could not be @@ -156,6 +166,34 @@ class PhabricatorObjectHandle { return $this->complete; } + + /** + * Set whether or not the underlying object is disabled. See + * @{method:getDisabled} for an explanation of what it means to be disabled. + * + * @param bool True if the handle represents a disabled object. + * @return this + */ + public function setDisabled($disabled) { + $this->disabled = $disabled; + return $this; + } + + + /** + * Determine if the handle represents an object which has been disabled -- + * for example, disabled users, archived projects, etc. These objects are + * complete and exist, but should be excluded from some system interactions + * (for instance, they usually should not appear in typeaheads, and should + * not have mail/notifications delivered to or about them). + * + * @return bool True if the handle represents a disabled object. + */ + public function isDisabled() { + return $this->disabled; + } + + public function renderLink() { switch ($this->getType()) { diff --git a/src/applications/phid/handle/data/PhabricatorObjectHandleData.php b/src/applications/phid/handle/data/PhabricatorObjectHandleData.php index 3322af127b..91bf8096ea 100644 --- a/src/applications/phid/handle/data/PhabricatorObjectHandleData.php +++ b/src/applications/phid/handle/data/PhabricatorObjectHandleData.php @@ -158,6 +158,7 @@ class PhabricatorObjectHandleData { $user->getUsername().' ('.$user->getRealName().')'); $handle->setAlternateID($user->getID()); $handle->setComplete(true); + $handle->setDisabled($user->getIsDisabled()); $img_uri = idx($images, $user->getProfileImagePHID()); if ($img_uri) {