From a6fd8f04792da6653f987677e7d855e32c964e1c Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 13 Mar 2019 08:47:31 -0700 Subject: [PATCH] When performing complex edits, pause sub-editors before they publish to propagate "Must Encrypt" and other state Summary: See PHI1134. Previously, see T13082 and D19969 for some sort-of-related stuff. Currently, edits work roughly like this: - Suppose we're editing object X, and we're also going to edit some other object, Y, because X mentioned Y or the edit is making X a child or parent of Y, or unblocking Y. - Do the actual edit to X, including inverse edits ("alice mentioned Y on X.", "alice added a child revision: X", etc) which apply to Y. - Run Herald rules on X. - Publish the edit to X. The "inverse edits" currently do this whole process inline, in a sub-editor. So the flow expands like this: - Begin editing X. - Update properties on X. - Begin inverse-edge editing Y. - Update properties on Y. - Run (actually, skip) Herald rules on Y. - Publish edits to Y. - Run Herald rules on X. - Publish edits to X. Notably, the "Y" stuff publishes before the "X" Herald rules run. This creates potential problems: - Herald rules may change the name or visibility policy of "X", but we'll publish mail about it via the edits to Y before those edits apply. This is a problem only in theory, we don't ship any upstream rules like this today. - Herald rules may "Require Secure Mail", but we won't know that at the time we're building mail about the indirect change to "Y". This is a problem in practice. Instead, switch to this new flow, where we stop the sub-editors before they publish, then publish everything at the very end once all the edits are complete: - Begin editing X. - Update properties on X. - Begin inverse-edge editing Y. - Update properties on Y. - Skip Herald on Y. - Run Herald rules on X. - Publish X. - Publish all child-editors of X. - Publish Y. Test Plan: - Created "Must Encrypt" Herald rules for Tasks and Revisions. - Edited object "A", an object which the rules applied to directly, and set object "B" (a different object which the rules did not hit) as its parent/child and/or unblocked it. - In `bin/mail list-outbound`, saw: - Mail about object "A" all flagged as "Must Encrypt". - Normal mail from object B not flagged "Must Encrypt". - Mail from object B about changing relationships to object A flagged as "Must Encrypt". Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D20283 --- .../editor/ManiphestTransactionEditor.php | 5 +- ...habricatorApplicationTransactionEditor.php | 86 +++++++++++++++---- 2 files changed, 72 insertions(+), 19 deletions(-) diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 9a95bbdbb8..5cb7cf91ba 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -134,10 +134,7 @@ final class ManiphestTransactionEditor $parent_xaction->setMetadataValue('blocker.new', true); } - id(new ManiphestTransactionEditor()) - ->setActor($this->getActor()) - ->setActingAsPHID($this->getActingAsPHID()) - ->setContentSource($this->getContentSource()) + $this->newSubEditor() ->setContinueOnNoEffect(true) ->setContinueOnMissingFields(true) ->applyTransactions($blocked_task, array($parent_xaction)); diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 24bff2bc57..3e5e9c23c9 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -72,7 +72,7 @@ abstract class PhabricatorApplicationTransactionEditor private $mailShouldSend = false; private $modularTypes; private $silent; - private $mustEncrypt; + private $mustEncrypt = array(); private $stampTemplates = array(); private $mailStamps = array(); private $oldTo = array(); @@ -90,6 +90,11 @@ abstract class PhabricatorApplicationTransactionEditor private $cancelURI; private $extensions; + private $parentEditor; + private $subEditors = array(); + private $publishableObject; + private $publishableTransactions; + const STORAGE_ENCODING_BINARY = 'binary'; /** @@ -1272,10 +1277,9 @@ abstract class PhabricatorApplicationTransactionEditor $herald_source = PhabricatorContentSource::newForSource( PhabricatorHeraldContentSource::SOURCECONST); - $herald_editor = newv(get_class($this), array()) + $herald_editor = $this->newEditorCopy() ->setContinueOnNoEffect(true) ->setContinueOnMissingFields(true) - ->setParentMessageID($this->getParentMessageID()) ->setIsHeraldEditor(true) ->setActor($herald_actor) ->setActingAsPHID($herald_phid) @@ -1330,6 +1334,38 @@ abstract class PhabricatorApplicationTransactionEditor } $this->heraldHeader = $herald_header; + // See PHI1134. If we're a subeditor, we don't publish information about + // the edit yet. Our parent editor still needs to finish applying + // transactions and execute Herald, which may change the information we + // publish. + + // For example, Herald actions may change the parent object's title or + // visibility, or Herald may apply rules like "Must Encrypt" that affect + // email. + + // Once the parent finishes work, it will queue its own publish step and + // then queue publish steps for its children. + + $this->publishableObject = $object; + $this->publishableTransactions = $xactions; + if (!$this->parentEditor) { + $this->queuePublishing(); + } + + return $xactions; + } + + final private function queuePublishing() { + $object = $this->publishableObject; + $xactions = $this->publishableTransactions; + + if (!$object) { + throw new Exception( + pht( + 'Editor method "queuePublishing()" was called, but no publishable '. + 'object is present. This Editor is not ready to publish.')); + } + // We're going to compute some of the data we'll use to publish these // transactions here, before queueing a worker. // @@ -1392,9 +1428,11 @@ abstract class PhabricatorApplicationTransactionEditor 'priority' => PhabricatorWorker::PRIORITY_ALERTS, )); - $this->flushTransactionQueue($object); + foreach ($this->subEditors as $sub_editor) { + $sub_editor->queuePublishing(); + } - return $xactions; + $this->flushTransactionQueue($object); } protected function didCatchDuplicateKeyException( @@ -3818,6 +3856,11 @@ abstract class PhabricatorApplicationTransactionEditor $this->mustEncrypt = $adapter->getMustEncryptReasons(); + // See PHI1134. Propagate "Must Encrypt" state to sub-editors. + foreach ($this->subEditors as $sub_editor) { + $sub_editor->mustEncrypt = $this->mustEncrypt; + } + $apply_xactions = $this->didApplyHeraldRules($object, $adapter, $xscript); assert_instances_of($apply_xactions, 'PhabricatorApplicationTransaction'); @@ -4034,15 +4077,10 @@ abstract class PhabricatorApplicationTransactionEditor ->setOldValue($old_phids) ->setNewValue($new_phids); - $editor + $editor = $this->newSubEditor($editor) ->setContinueOnNoEffect(true) ->setContinueOnMissingFields(true) - ->setParentMessageID($this->getParentMessageID()) - ->setIsInverseEdgeEditor(true) - ->setIsSilent($this->getIsSilent()) - ->setActor($this->requireActor()) - ->setActingAsPHID($this->getActingAsPHID()) - ->setContentSource($this->getContentSource()); + ->setIsInverseEdgeEditor(true); $editor->applyTransactions($node, array($template)); } @@ -4551,23 +4589,41 @@ abstract class PhabricatorApplicationTransactionEditor $xactions = $this->transactionQueue; $this->transactionQueue = array(); - $editor = $this->newQueueEditor(); + $editor = $this->newEditorCopy(); return $editor->applyTransactions($object, $xactions); } - private function newQueueEditor() { - $editor = id(newv(get_class($this), array())) + final protected function newSubEditor( + PhabricatorApplicationTransactionEditor $template = null) { + $editor = $this->newEditorCopy($template); + + $editor->parentEditor = $this; + $this->subEditors[] = $editor; + + return $editor; + } + + private function newEditorCopy( + PhabricatorApplicationTransactionEditor $template = null) { + if ($template === null) { + $template = newv(get_class($this), array()); + } + + $editor = id(clone $template) ->setActor($this->getActor()) ->setContentSource($this->getContentSource()) ->setContinueOnNoEffect($this->getContinueOnNoEffect()) ->setContinueOnMissingFields($this->getContinueOnMissingFields()) + ->setParentMessageID($this->getParentMessageID()) ->setIsSilent($this->getIsSilent()); if ($this->actingAsPHID !== null) { $editor->setActingAsPHID($this->actingAsPHID); } + $editor->mustEncrypt = $this->mustEncrypt; + return $editor; }