revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-30 01:10:58 +01:00
Code Issues Releases Wiki Activity

Show why recipients were excluded from mail

Browse source 
Summary:
Ref T3306. This interface has a hard time balancing security/policy issues and I'm not sure what the best way forward is. Some possibilities:

  # We just let you see everything from the web UI.
    - This makes debugging easier.
    - Anyone who can see this stuff can trivially take over any user's account with five seconds of work and no technical expertise (reset their password from the web UI, then go read the email and click the link).
  # We let you see everything, but only for messages you were a recipient of or author of.
    - This makes it much more difficult to debug issues with mailing lists.
      - But maybe we could just say mailing list recipients are "public", or define some other ruleset.
    - Generally this gets privacy and ease of use right.
  # We could move the whole thing to the CLI.
    - Makes the UI/UX way worse.
  # We could strike an awkward balance between concerns, as we do now.
    - We expose //who// sent and received messages, but not the content of the messages. This doesn't feel great.

I'm inclined to probably go with (2) and figure something out for mailing lists?

Anyway, irrespective of that this should generally make things more clear, and improves the code a lot if nothing else.

Test Plan:
{F49546}

  - Looked at a bunch of mail.
  - Sent mail from different apps.
  - Checked that recipients seem correct.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3306

Differential Revision: https://secure.phabricator.com/D6413
This commit is contained in:
epriestley 2013-07-10 15:17:38 -07:00
parent 65ab9d1780
commit 293a475e39
4 changed files with 361 additions and 261 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

3
src/__phutil_library_map__.php
View file

@ -1233,6 +1233,8 @@ phutil_register_library_map(array(
'PhabricatorMarkupInterface' => 'infrastructure/markup/PhabricatorMarkupInterface.php',
'PhabricatorMarkupOneOff' => 'infrastructure/markup/PhabricatorMarkupOneOff.php',
'PhabricatorMercurialGraphStream' => 'applications/repository/daemon/PhabricatorMercurialGraphStream.php',
'PhabricatorMetaMTAActor' => 'applications/metamta/query/PhabricatorMetaMTAActor.php',
'PhabricatorMetaMTAActorQuery' => 'applications/metamta/query/PhabricatorMetaMTAActorQuery.php',
'PhabricatorMetaMTAAttachment' => 'applications/metamta/storage/PhabricatorMetaMTAAttachment.php',
'PhabricatorMetaMTAConfigOptions' => 'applications/config/option/PhabricatorMetaMTAConfigOptions.php',
'PhabricatorMetaMTAController' => 'applications/metamta/controller/PhabricatorMetaMTAController.php',
@ -3183,6 +3185,7 @@ phutil_register_library_map(array(
'PhabricatorManiphestTaskTestDataGenerator' => 'PhabricatorTestDataGenerator',
'PhabricatorMarkupCache' => 'PhabricatorCacheDAO',
'PhabricatorMarkupOneOff' => 'PhabricatorMarkupInterface',
'PhabricatorMetaMTAActorQuery' => 'PhabricatorQuery',
'PhabricatorMetaMTAConfigOptions' => 'PhabricatorApplicationConfigOptions',
'PhabricatorMetaMTAController' => 'PhabricatorController',
'PhabricatorMetaMTADAO' => 'PhabricatorLiskDAO',

50
src/applications/metamta/query/PhabricatorMetaMTAActor.php Normal file
View file

@ -0,0 +1,50 @@
<?php
final class PhabricatorMetaMTAActor {
private $phid;
private $emailAddress;
private $name;
private $reasons = array();
public function setName($name) {
$this->name = $name;
return $this;
}
public function getName() {
return $this->name;
}
public function setEmailAddress($email_address) {
$this->emailAddress = $email_address;
return $this;
}
public function getEmailAddress() {
return $this->emailAddress;
}
public function setPHID($phid) {
$this->phid = $phid;
return $this;
}
public function getPHID() {
return $this->phid;
}
public function setUndeliverable($reason) {
$this->reasons[] = $reason;
return $this;
}
public function isDeliverable() {
return empty($this->reasons);
}
public function getUndeliverableReasons() {
return $this->reasons;
}
}

181
src/applications/metamta/query/PhabricatorMetaMTAActorQuery.php Normal file
View file

@ -0,0 +1,181 @@
<?php
final class PhabricatorMetaMTAActorQuery extends PhabricatorQuery {
private $phids = array();
private $viewer;
public function setViewer(PhabricatorUser $viewer) {
$this->viewer = $viewer;
return $this;
}
public function getViewer() {
return $this->viewer;
}
public function withPHIDs(array $phids) {
$this->phids = $phids;
return $this;
}
public function execute() {
$phids = array_fuse($this->phids);
$actors = array();
$type_map = array();
foreach ($phids as $phid) {
$type_map[phid_get_type($phid)][] = $phid;
$actors[$phid] = id(new PhabricatorMetaMTAActor())->setPHID($phid);
}
foreach ($type_map as $type => $phids) {
switch ($type) {
case PhabricatorPHIDConstants::PHID_TYPE_USER:
$this->loadUserActors($actors, $phids);
break;
case PhabricatorPHIDConstants::PHID_TYPE_XUSR:
$this->loadExternalUserActors($actors, $phids);
break;
case PhabricatorPHIDConstants::PHID_TYPE_MLST:
$this->loadMailingListActors($actors, $phids);
break;
default:
$this->loadUnknownActors($actors, $phids);
break;
}
}
return $actors;
}
private function loadUserActors(array $actors, array $phids) {
assert_instances_of($actors, 'PhabricatorMetaMTAActor');
$emails = id(new PhabricatorUserEmail())->loadAllWhere(
'userPHID IN (%Ls) AND isPrimary = 1',
$phids);
$emails = mpull($emails, null, 'getUserPHID');
$users = id(new PhabricatorPeopleQuery())
->setViewer($this->getViewer())
->withPHIDs($phids)
->execute();
$users = mpull($users, null, 'getPHID');
foreach ($phids as $phid) {
$actor = $actors[$phid];
$user = idx($users, $phid);
if (!$user) {
$actor->setUndeliverable(
pht('Unable to load user record for this PHID.'));
} else {
$actor->setName($this->getUserName($user));
if ($user->getIsDisabled()) {
$actor->setUndeliverable(
pht('This user is disabled; disabled users do not receive mail.'));
}
if ($user->getIsSystemAgent()) {
$actor->setUndeliverable(
pht('This user is a bot; bot accounts do not receive mail.'));
}
}
$email = idx($emails, $phid);
if (!$email) {
$actor->setUndeliverable(
pht('Unable to load email record for this PHID.'));
} else {
$actor->setEmailAddress($email->getAddress());
}
}
}
private function loadExternalUserActors(array $actors, array $phids) {
assert_instances_of($actors, 'PhabricatorMetaMTAActor');
$xusers = id(new PhabricatorExternalAccountQuery())
->setViewer($this->getViewer())
->withPHIDs($phids)
->execute();
$xusers = mpull($xusers, null, 'getPHID');
foreach ($phids as $phid) {
$actor = $actors[$phid];
$xuser = idx($xusers, $phid);
if (!$xuser) {
$actor->setUndeliverable(
pht('Unable to load external user record for this PHID.'));
continue;
}
$actor->setName($xuser->getDisplayName());
if ($xuser->getAccountType() != 'email') {
$actor->setUndeliverable(
pht(
'Only external accounts of type "email" are deliverable; this '.
'account has a different type.'));
continue;
}
$actor->setEmailAddress($xuser->getAccountID());
}
}
private function loadMailingListActors(array $actors, array $phids) {
assert_instances_of($actors, 'PhabricatorMetaMTAActor');
$lists = id(new PhabricatorMetaMTAMailingList())->loadAllWhere(
'phid IN (%Ls)',
$phids);
foreach ($phids as $phid) {
$actor = $actors[$phid];
$list = idx($lists, $phid);
if (!$list) {
$actor->setUndeliverable(
pht(
'Unable to load mailing list record for this PHID.'));
continue;
}
$actor->setName($list->getName());
$actor->setEmailAddress($list->getEmail());
}
}
private function loadUnknownActors(array $actors, array $phids) {
foreach ($phids as $phid) {
$actor = $actors[$phid];
$actor->setUndeliverable(pht('This PHID type is not mailable.'));
}
}
/**
* Small helper function to make sure we format the username properly as
* specified by the `metamta.user-address-format` configuration value.
*/
private function getUserName(PhabricatorUser $user) {
$format = PhabricatorEnv::getEnvConfig('metamta.user-address-format');
switch ($format) {
case 'short':
$name = $user->getUserName();
break;
case 'real':
$name = $user->getRealName();
break;
case 'full':
default:
$name = $user->getFullName();
break;
}
return $name;
}
}

388
src/applications/metamta/storage/PhabricatorMetaMTAMail.php
View file

@ -66,6 +66,10 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
return $this;
}
public function getMailTags() {
return $this->getParam('mailtags', array());
}
/**
* In Gmail, conversations will be broken if you reply to a thread and the
* server sends back a response without referencing your Message-ID, even if
@ -242,6 +246,18 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
return $this;
}
public function getToPHIDs() {
return $this->getParam('to', array());
}
public function getRawToAddresses() {
return $this->getParam('raw-to', array());
}
public function getCcPHIDs() {
return $this->getParam('cc', array());
}
/**
* Flag that this is an auto-generated bulk message and should have bulk
* headers added to it if appropriate. Broadly, this means some flavor of
@ -343,52 +359,13 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
try {
$params = $this->parameters;
$phids = array();
foreach ($params as $key => $value) {
switch ($key) {
case 'to':
$params[$key] = $this->buildToList();
break;
case 'cc':
$params[$key] = $this->buildCCList();
break;
}
}
$actors = $this->loadAllActors();
$deliverable_actors = $this->filterDeliverableActors($actors);
foreach ($params as $key => $value) {
switch ($key) {
case 'from':
$value = array($value);
/* fallthrough */
case 'to':
case 'cc':
foreach ($value as $phid) {
$type = phid_get_type($phid);
$phids[$phid] = $type;
}
break;
}
}
$this->loadEmailAndNameDataFromPHIDs($phids);
$default = PhabricatorEnv::getEnvConfig('metamta.default-address');
$default_from = PhabricatorEnv::getEnvConfig('metamta.default-address');
if (empty($params['from'])) {
$mailer->setFrom($default);
} else {
$from = $params['from'];
if (!PhabricatorEnv::getEnvConfig('metamta.can-send-as-user')) {
if (empty($params['reply-to'])) {
$params['reply-to'] = $phids[$from]['email'];
$params['reply-to-name'] = $phids[$from]['name'];
}
$mailer->setFrom(
$default,
$phids[$from]['name']);
unset($params['from']);
}
$mailer->setFrom($default_from);
}
$is_first = idx($params, 'is-first-message');
@ -405,25 +382,45 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
foreach ($params as $key => $value) {
switch ($key) {
case 'from':
$mailer->setFrom($phids[$from]['email']);
$from = $value;
$actor = $actors[$from];
$actor_email = $actor->getEmailAddress();
$actor_name = $actor->getName();
$can_send_as_user = $actor_email &&
PhabricatorEnv::getEnvConfig('metamta.can-send-as-user');
if ($can_send_as_user) {
$mailer->setFrom($actor_email);
} else {
$from_email = coalesce($actor_email, $default_from);
$from_name = coalesce($actor_name, pht('Phabricator'));
if (empty($params['reply-to'])) {
$params['reply-to'] = $from_email;
$params['reply-to-name'] = $from_name;
}
$mailer->setFrom($default_from, $from_name);
}
break;
case 'reply-to':
$mailer->addReplyTo($value, $reply_to_name);
break;
case 'to':
$to_emails = $this->filterSendable($value, $phids);
if ($to_emails) {
$add_to = array_merge($add_to, $to_emails);
}
$to_actors = array_select_keys($deliverable_actors, $value);
$add_to = array_merge(
$add_to,
mpull($to_actors, 'getEmailAddress'));
break;
case 'raw-to':
$add_to = array_merge($add_to, $value);
break;
case 'cc':
$cc_emails = $this->filterSendable($value, $phids);
if ($cc_emails) {
$add_cc = $cc_emails;
}
$cc_actors = array_select_keys($deliverable_actors, $value);
$add_cc = array_merge(
$add_cc,
mpull($cc_actors, 'getEmailAddress'));
break;
case 'headers':
foreach ($value as $pair) {
@ -573,8 +570,8 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
if (!$add_to && !$add_cc) {
$this->setStatus(self::STATUS_VOID);
$this->setMessage(
"Message has no valid recipients: all To/CC are disabled or ".
"configured not to receive this mail.");
"Message has no valid recipients: all To/Cc are disabled, invalid, ".
"or configured not to receive this mail.");
return $this->save();
}
@ -610,6 +607,9 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
}
}
$add_to = array_unique($add_to);
$add_cc = array_unique($add_cc);
$mailer->addTos($add_to);
if ($add_cc) {
$mailer->addCCs($add_cc);
@ -691,126 +691,6 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
return base64_encode($base);
}
private function loadEmailAndNameDataFromPHIDs(array &$phids) {
$users = array();
$xusrs = array();
$mlsts = array();
// first iteration - group by types to do data fetches
foreach ($phids as $phid => $type) {
switch ($type) {
case PhabricatorPHIDConstants::PHID_TYPE_USER:
$users[] = $phid;
break;
case PhabricatorPHIDConstants::PHID_TYPE_XUSR:
$xusrs[] = $phid;
break;
case PhabricatorPHIDConstants::PHID_TYPE_MLST:
$mlsts[] = $phid;
break;
}
}
$user_emails = array();
if ($users) {
$user_emails = id(new PhabricatorUserEmail())->loadAllWhere(
'userPHID IN (%Ls) AND isPrimary = 1', $users);
$users = id(new PhabricatorUser())->loadAllWhere(
'phid IN (%Ls)', $users);
$user_emails = mpull($user_emails, null, 'getUserPHID');
$users = mpull($users, null, 'getPHID');
}
if ($xusrs) {
$xusrs = id(new PhabricatorExternalAccount())->loadAllWhere(
'phid IN (%Ls)', $xusrs);
$xusrs = mpull($xusrs, null, 'getPHID');
}
if ($mlsts) {
$mlsts = id(new PhabricatorMetaMTAMailingList())->loadAllWhere(
'phid IN (%Ls)', $mlsts);
$mlsts = mpull($mlsts, null, 'getPHID');
}
// second iteration - create entries for each phid
$default = PhabricatorEnv::getEnvConfig('metamta.default-address');
foreach ($phids as $phid => &$value) {
$name = '';
$email = $default;
$is_mailable = false;
switch ($value) {
case PhabricatorPHIDConstants::PHID_TYPE_USER:
$user = idx($users, $phid);
if ($user) {
$name = $this->getUserName($user);
$is_mailable = !$user->getIsDisabled()
&& !$user->getIsSystemAgent();
}
$email = isset($user_emails[$phid]) ?
$user_emails[$phid]->getAddress() :
$default;
break;
case PhabricatorPHIDConstants::PHID_TYPE_XUSR:
$xusr = $xusrs[$phid];
if ($xusr) {
$name = $xusr->getDisplayName();
$email = $xusr->getAccountID();
$accountType = $xusr->getAccountType();
if ($accountType == 'email') {
$is_mailable = true;
}
}
break;
case PhabricatorPHIDConstants::PHID_TYPE_MLST:
$mlst = $mlsts[$phid];
if ($mlst) {
$name = $mlst->getName();
$email = $mlst->getEmail();
$is_mailable = true;
}
break;
}
$value = array(
'name' => $name,
'email' => $email,
'mailable' => $is_mailable,
);
}
}
/**
* Small helper function to make sure we format the username properly as
* specified by the `metamta.user-address-format` configuration value.
*/
private function getUserName($user) {
$format = PhabricatorEnv::getEnvConfig('metamta.user-address-format');
switch ($format) {
case 'short':
$name = $user->getUserName();
break;
case 'real':
$name = $user->getRealName();
break;
case 'full':
default:
$name = $user->getFullName();
break;
}
return $name;
}
private function filterSendable($value, $phids) {
$result = array();
foreach ($value as $phid) {
if (isset($phids[$phid]) && $phids[$phid]['mailable']) {
$result[$phid] = $phids[$phid]['email'];
}
}
return $result;
}
public static function shouldMultiplexAllMail() {
return PhabricatorEnv::getEnvConfig('metamta.one-mail-per-recipient');
}
@ -821,60 +701,90 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
/**
* Get all of the recipients for this mail, after preference filters are
* applied. This list has all objects to whom delivery will be attempted, but
* does not exclude recipeints two whom delivery may be impossible.
* applied. This list has all objects to whom delivery will be attempted.
*
* @return list<phid> A list of all recipients to whom delivery will be
* attempted.
* @task recipients
*/
public function buildRecipientList() {
return $this->resolveRecipients(
$actors = $this->loadActors(
array_merge(
$this->getRawToPHIDs(),
$this->getRawCCPHIDs()));
$this->getToPHIDs(),
$this->getCcPHIDs()));
$actors = $this->filterDeliverableActors($actors);
return mpull($actors, 'getPHID');
}
public function loadAllActors() {
$actor_phids = array_merge(
array($this->getParam('from')),
$this->getToPHIDs(),
$this->getCcPHIDs());
/**
* Filter out duplicate, invalid, or excluded recipients from a PHID list.
*
* @param list<phid> Unfiltered recipients.
* @return list<phid> Filtered recipients.
*
* @task recipients
*/
private function resolveRecipients(array $phids) {
if (!$phids) {
return $this->loadActors($actor_phids);
}
private function filterDeliverableActors(array $actors) {
assert_instances_of($actors, 'PhabricatorMetaMTAActor');
$deliverable_actors = array();
foreach ($actors as $phid => $actor) {
if ($actor->isDeliverable()) {
$deliverable_actors[$phid] = $actor;
}
}
return $deliverable_actors;
}
private function loadActors(array $actor_phids) {
$actor_phids = array_filter($actor_phids);
$viewer = PhabricatorUser::getOmnipotentUser();
$actors = id(new PhabricatorMetaMTAActorQuery())
->setViewer($viewer)
->withPHIDs($actor_phids)
->execute();
if (!$actors) {
return array();
}
$phids = array_fuse($phids);
// Exclude explicit recipients.
foreach ($this->getExcludeMailRecipientPHIDs() as $phid) {
$actor = idx($actors, $phid);
if (!$actor) {
continue;
}
$actor->setUndeliverable(
pht(
'This message is a response to another email message, and this '.
'recipient received the original email message, so we are not '.
'sending them this substantially similar message (for example, '.
'the sender used "Reply All" instead of "Reply" in response to '.
'mail from Phabricator).'));
}
// Exclude PHIDs explicitly marked for exclusion. We use this to prevent
// recipients of an accidental "Reply All" from receiving the followup
// mail from Phabricator.
$exclude = $this->getExcludeMailRecipientPHIDs();
$exclude = array_fill_keys($exclude, true);
$phids = array_diff_key($phids, $exclude);
// If the actor is a recipient and has configured their preferences not to
// send them mail about their own actions, drop them from the recipient
// list.
$from = $this->getParam('from');
if (isset($phids[$from])) {
$from_user = id(new PhabricatorUser())->loadOneWhere(
'phid = %s',
$from);
// Exclude the actor if their preferences are set.
$from_phid = $this->getParam('from');
$from_actor = idx($actors, $from_phid);
if ($from_actor) {
$from_user = id(new PhabricatorPeopleQuery())
->setViewer($viewer)
->withPHIDs(array($from_phid))
->execute();
$from_user = head($from_user);
if ($from_user && !$this->getOverrideNoSelfMailPreference()) {
$pref_key = PhabricatorUserPreferences::PREFERENCE_NO_SELF_MAIL;
$exclude_self = $from_user
->loadPreferences()
->getPreference($pref_key);
if ($exclude_self) {
unset($phids[$from]);
$from_actor->setUndeliverable(
pht(
'This recipient is the user whose actions caused delivery of '.
'this message, but they have set preferences so they do not '.
'receive mail about their own actions (Settings > Email '.
'Preferences > Self Actions).'));
}
}
}
@ -884,18 +794,13 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
// of email (for example, a user who says they don't want emails about task
// CC changes).
$tags = $this->getParam('mailtags');
if ($tags && $phids) {
if ($tags) {
$all_prefs = id(new PhabricatorUserPreferences())->loadAllWhere(
'userPHID in (%Ls)',
$phids);
$actor_phids);
$all_prefs = mpull($all_prefs, null, 'getUserPHID');
foreach ($phids as $phid) {
$prefs = idx($all_prefs, $phid);
if (!$prefs) {
continue;
}
foreach ($all_prefs as $phid => $prefs) {
$user_mailtags = $prefs->getPreference(
PhabricatorUserPreferences::PREFERENCE_MAILTAGS,
array());
@ -911,56 +816,17 @@ final class PhabricatorMetaMTAMail extends PhabricatorMetaMTADAO {
}
if (!$send) {
unset($phids[$phid]);
$actors[$phid]->setUndeliverable(
pht(
'This mail has tags which control which users receive it, and '.
'this recipient has not elected to receive mail with any of '.
'the tags on this message (Settings > Email Preferences).'));
}
}
}
return array_keys($phids);
return $actors;
}
/**
* @task recipients
*/
private function buildToList() {
return $this->resolveRecipients($this->getRawToPHIDs());
}
/**
* @task recipients
*/
private function buildCCList() {
return $this->resolveRecipients($this->getRawCCPHIDs());
}
/**
* @task recipients
*/
private function getRawToPHIDs() {
$to = $this->getParam('to', array());
return $this->filterRawPHIDList($to);
}
/**
* @task recipients
*/
private function getRawCCPHIDs() {
$cc = $this->getParam('cc', array());
return $this->filterRawPHIDList($cc);
}
/**
* @task recipients
*/
private function filterRawPHIDList(array $list) {
$list = array_filter($list);
$list = array_unique($list);
return array_values($list);
}
}