From f214abb63f9df8eb17a0438c7256d9f46957f48b Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 7 Feb 2018 05:52:52 -0800 Subject: [PATCH] When a change removes recipients from an object, send them one last email Summary: Depends on D19018. Fixes T4776. Ref T13053. When you remove someone from an object's recipient list (for example, by removing them a reviewer, auditor, subscriber, owner or author) we currently do not send them mail about it because they're no longer connected to the object. In many of these cases (Commandeer, Reassign) the actual action in the UI adds them back to the object somehow (as a reviewer or subscriber, respectively) so this doesn't actually matter. However, there's no recovery mechanism for reviewer or subscriber removal. This is slightly bad from a policy/threat viewpoint since it means an attacker can remove all the recipients of an object "somewhat" silently. This isn't really silent, but it's less un-silent than it should be. It's also just not very good from a human interaction perspective: if Alice removes Bob as a reviewer, possibly "against his will", he should be notified about that. In the good case, Alice wrote a nice goodbye note that he should get to read. In the bad case, he should get a chance to correct the mistake. Also add a `removed(@user)` mail stamp so you can route these locally if you want. Test Plan: - Created and edited some different objects without catching anything broken. - Removed subscribers from tasks, saw the final email include the removed recipients with a `removed()` stamp. I'm not totally sure this doesn't have any surprising behavior or break any weird objects, but I think anything that crops up should be easy to fix. Reviewers: amckinley Subscribers: sophiebits Maniphest Tasks: T13053, T4776 Differential Revision: https://secure.phabricator.com/D19019 --- ...habricatorApplicationTransactionEditor.php | 70 +++++++++++++++++++ .../PhabricatorEditorMailEngineExtension.php | 7 ++ 2 files changed, 77 insertions(+) diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index e4f9607801..6bb7679fdc 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -74,6 +74,9 @@ abstract class PhabricatorApplicationTransactionEditor private $mustEncrypt; private $stampTemplates = array(); private $mailStamps = array(); + private $oldTo = array(); + private $oldCC = array(); + private $mailRemovedPHIDs = array(); private $transactionQueue = array(); @@ -916,6 +919,8 @@ abstract class PhabricatorApplicationTransactionEditor $this->willApplyTransactions($object, $xactions); if ($object->getID()) { + $this->buildOldRecipientLists($object, $xactions); + foreach ($xactions as $xaction) { // If any of the transactions require a read lock, hold one and @@ -1200,6 +1205,10 @@ abstract class PhabricatorApplicationTransactionEditor $this->mailToPHIDs = $this->getMailTo($object); $this->mailCCPHIDs = $this->getMailCC($object); + // Add any recipients who were previously on the notification list + // but were removed by this change. + $this->applyOldRecipientLists(); + $mail_xactions = $this->getTransactionsForMail($object, $xactions); $stamps = $this->newMailStamps($object, $xactions); foreach ($stamps as $stamp) { @@ -4127,4 +4136,65 @@ abstract class PhabricatorApplicationTransactionEditor return $results; } + public function getRemovedRecipientPHIDs() { + return $this->mailRemovedPHIDs; + } + + private function buildOldRecipientLists($object, $xactions) { + // See T4776. Before we start making any changes, build a list of the old + // recipients. If a change removes a user from the recipient list for an + // object we still want to notify the user about that change. This allows + // them to respond if they didn't want to be removed. + + if (!$this->shouldSendMail($object, $xactions)) { + return; + } + + $this->oldTo = $this->getMailTo($object); + $this->oldCC = $this->getMailCC($object); + + return $this; + } + + private function applyOldRecipientLists() { + $actor_phid = $this->getActingAsPHID(); + + // If you took yourself off the recipient list (for example, by + // unsubscribing or resigning) assume that you know what you did and + // don't need to be notified. + + // If you just moved from "To" to "Cc" (or vice versa), you're still a + // recipient so we don't need to add you back in. + + $map = array_fuse($this->mailToPHIDs) + array_fuse($this->mailCCPHIDs); + + foreach ($this->oldTo as $phid) { + if ($phid === $actor_phid) { + continue; + } + + if (isset($map[$phid])) { + continue; + } + + $this->mailToPHIDs[] = $phid; + $this->mailRemovedPHIDs[] = $phid; + } + + foreach ($this->oldCC as $phid) { + if ($phid === $actor_phid) { + continue; + } + + if (isset($map[$phid])) { + continue; + } + + $this->mailCCPHIDs[] = $phid; + $this->mailRemovedPHIDs[] = $phid; + } + + return $this; + } + } diff --git a/src/applications/transactions/engineextension/PhabricatorEditorMailEngineExtension.php b/src/applications/transactions/engineextension/PhabricatorEditorMailEngineExtension.php index 29d10d641f..5365894429 100644 --- a/src/applications/transactions/engineextension/PhabricatorEditorMailEngineExtension.php +++ b/src/applications/transactions/engineextension/PhabricatorEditorMailEngineExtension.php @@ -40,6 +40,10 @@ final class PhabricatorEditorMailEngineExtension ->setKey('herald') ->setLabel(pht('Herald Rule')); + $templates[] = id(new PhabricatorPHIDMailStamp()) + ->setKey('removed') + ->setLabel(pht('Recipient Removed')); + return $templates; } @@ -69,6 +73,9 @@ final class PhabricatorEditorMailEngineExtension $this->getMailStamp('herald') ->setValue($editor->getHeraldRuleMonograms()); + + $this->getMailStamp('removed') + ->setValue($editor->getRemovedRecipientPHIDs()); } }