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

epriestley 8 years ago f214abb6 dbe479f0

+77

2 changed files

expand all

src

applications

transactions

editor

PhabricatorApplicationTransactionEditor.php

engineextension

PhabricatorEditorMailEngineExtension.php

+70

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

··· 74 74 private $mustEncrypt; 75 75 private $stampTemplates = array(); 76 76 private $mailStamps = array(); 77 + private $oldTo = array(); 78 + private $oldCC = array(); 79 + private $mailRemovedPHIDs = array(); 77 80 78 81 private $transactionQueue = array(); 79 82 ··· 916 919 $this->willApplyTransactions($object, $xactions); 917 920 918 921 if ($object->getID()) { 922 + $this->buildOldRecipientLists($object, $xactions); 923 + 919 924 foreach ($xactions as $xaction) { 920 925 921 926 // If any of the transactions require a read lock, hold one and ··· 1199 1204 $this->mailShouldSend = true; 1200 1205 $this->mailToPHIDs = $this->getMailTo($object); 1201 1206 $this->mailCCPHIDs = $this->getMailCC($object); 1207 + 1208 + // Add any recipients who were previously on the notification list 1209 + // but were removed by this change. 1210 + $this->applyOldRecipientLists(); 1202 1211 1203 1212 $mail_xactions = $this->getTransactionsForMail($object, $xactions); 1204 1213 $stamps = $this->newMailStamps($object, $xactions); ··· 4125 4134 natcasesort($results); 4126 4135 4127 4136 return $results; 4137 + } 4138 + 4139 + public function getRemovedRecipientPHIDs() { 4140 + return $this->mailRemovedPHIDs; 4141 + } 4142 + 4143 + private function buildOldRecipientLists($object, $xactions) { 4144 + // See T4776. Before we start making any changes, build a list of the old 4145 + // recipients. If a change removes a user from the recipient list for an 4146 + // object we still want to notify the user about that change. This allows 4147 + // them to respond if they didn't want to be removed. 4148 + 4149 + if (!$this->shouldSendMail($object, $xactions)) { 4150 + return; 4151 + } 4152 + 4153 + $this->oldTo = $this->getMailTo($object); 4154 + $this->oldCC = $this->getMailCC($object); 4155 + 4156 + return $this; 4157 + } 4158 + 4159 + private function applyOldRecipientLists() { 4160 + $actor_phid = $this->getActingAsPHID(); 4161 + 4162 + // If you took yourself off the recipient list (for example, by 4163 + // unsubscribing or resigning) assume that you know what you did and 4164 + // don't need to be notified. 4165 + 4166 + // If you just moved from "To" to "Cc" (or vice versa), you're still a 4167 + // recipient so we don't need to add you back in. 4168 + 4169 + $map = array_fuse($this->mailToPHIDs) + array_fuse($this->mailCCPHIDs); 4170 + 4171 + foreach ($this->oldTo as $phid) { 4172 + if ($phid === $actor_phid) { 4173 + continue; 4174 + } 4175 + 4176 + if (isset($map[$phid])) { 4177 + continue; 4178 + } 4179 + 4180 + $this->mailToPHIDs[] = $phid; 4181 + $this->mailRemovedPHIDs[] = $phid; 4182 + } 4183 + 4184 + foreach ($this->oldCC as $phid) { 4185 + if ($phid === $actor_phid) { 4186 + continue; 4187 + } 4188 + 4189 + if (isset($map[$phid])) { 4190 + continue; 4191 + } 4192 + 4193 + $this->mailCCPHIDs[] = $phid; 4194 + $this->mailRemovedPHIDs[] = $phid; 4195 + } 4196 + 4197 + return $this; 4128 4198 } 4129 4199 4130 4200 }

src/applications/transactions/engineextension/PhabricatorEditorMailEngineExtension.php

··· 40 40 ->setKey('herald') 41 41 ->setLabel(pht('Herald Rule')); 42 42 43 + $templates[] = id(new PhabricatorPHIDMailStamp()) 44 + ->setKey('removed') 45 + ->setLabel(pht('Recipient Removed')); 46 + 43 47 return $templates; 44 48 } 45 49 ··· 69 73 70 74 $this->getMailStamp('herald') 71 75 ->setValue($editor->getHeraldRuleMonograms()); 76 + 77 + $this->getMailStamp('removed') 78 + ->setValue($editor->getRemovedRecipientPHIDs()); 72 79 } 73 80 74 81 }

Configure Feed

Configure Feed