diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -3,6 +3,8 @@ final class ManiphestTransactionEditor extends PhabricatorApplicationTransactionEditor { + private $oldOwnerPHID; + public function getTransactionTypes() { $types = parent::getTransactionTypes(); @@ -24,6 +26,91 @@ return $types; } + /** + * We probably want to apply initial effects if there is an owner + * transaction. Note we will only really apply any initial effects + * if the transaction changes ownership. + * + * Overall, the goal is to set the Task owner phid correctly ASAP so + * policy checks all work correctly w.r.t. "owner can always do stuff" + * meta policy. + * + * Similarly, we also want to apply initial effects if there is a change + * to policy *AND* the object is new. This may change if / when we decide + * to have an explicit "can create" permission, in which case "can view" + * and "can edit" policies will need to be removed from the create path. + * For now though, since anyone can create tasks, we have to update the + * policy ASAP so e.g. if the default edit policy is no one people can + * still create tasks so long as they set the edit policy to something + * that lets them edit the Task. + */ + protected function shouldApplyInitialEffects( + PhabricatorLiskDAO $object, + array $xactions) { + $should_apply = false; + foreach ($xactions as $xaction) { + switch ($xaction->getTransactionType()) { + case ManiphestTransaction::TYPE_OWNER: + $should_apply = true; + break 2; + case PhabricatorTransactions::TYPE_VIEW_POLICY: + case PhabricatorTransactions::TYPE_EDIT_POLICY: + if ($this->getIsNewObject()) { + $should_apply = true; + break 2; + } + continue; + default: + continue; + } + } + return $should_apply; + } + + protected function applyInitialEffects( + PhabricatorLiskDAO $object, + array $xactions) { + $changing_owner = false; + $changing_edit_policy = false; + $changing_view_policy = false; + foreach ($xactions as $xaction) { + switch ($xaction->getTransactionType()) { + case ManiphestTransaction::TYPE_OWNER: + $this->oldOwnerPHID = nonempty($object->getOwnerPHID(), null); + $new_owner_phid = $xaction->getNewValue(); + if ($new_owner_phid != $this->oldOwnerPHID) { + $object->setOwnerPHID($new_owner_phid); + $changing_owner = true; + } + break; + case PhabricatorTransactions::TYPE_VIEW_POLICY: + $view_policy = $xaction->getNewValue(); + if ($this->getIsNewObject()) { + $object->setViewPolicy($view_policy); + } else if ($object->getViewPolicy() != $view_policy) { + $changing_view_policy = $view_policy; + } + break; + case PhabricatorTransactions::TYPE_EDIT_POLICY: + $edit_policy = $xaction->getNewValue(); + if ($this->getIsNewObject()) { + $object->setEditPolicy($edit_policy); + } else if ($object->getEditPolicy() != $edit_policy) { + $changing_edit_policy = $edit_policy; + } + break; + default: + continue; + } + } + if ($changing_owner && $changing_view_policy) { + $object->setViewPolicy($changing_view_policy); + } + if ($changing_owner && $changing_edit_policy) { + $object->setEditPolicy($changing_edit_policy); + } + } + protected function getCustomTransactionOldValue( PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction) { @@ -50,7 +137,7 @@ } return $object->getDescription(); case ManiphestTransaction::TYPE_OWNER: - return nonempty($object->getOwnerPHID(), null); + return $this->oldOwnerPHID; case ManiphestTransaction::TYPE_CCS: return array_values(array_unique($object->getCCPHIDs())); case ManiphestTransaction::TYPE_PROJECTS: @@ -396,7 +483,6 @@ $transaction_type = $xaction->getTransactionType(); $app_capability = idx($app_capability_map, $transaction_type); - if ($app_capability) { $app = id(new PhabricatorApplicationQuery()) ->setViewer($this->getActor()) diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -178,10 +178,19 @@ case PhabricatorTransactions::TYPE_SUBSCRIBERS: return array_values($this->subscribers); case PhabricatorTransactions::TYPE_VIEW_POLICY: + if ($this->getIsNewObject()) { + return null; + } return $object->getViewPolicy(); case PhabricatorTransactions::TYPE_EDIT_POLICY: + if ($this->getIsNewObject()) { + return null; + } return $object->getEditPolicy(); case PhabricatorTransactions::TYPE_JOIN_POLICY: + if ($this->getIsNewObject()) { + return null; + } return $object->getJoinPolicy(); case PhabricatorTransactions::TYPE_EDGE: $edge_type = $xaction->getMetadataValue('edge:type'); @@ -511,6 +520,14 @@ $read_locking = false; $transaction_open = false; + if ($this->shouldApplyInitialEffects($object, $xactions)) { + if (!$is_preview) { + $object->openTransaction(); + $transaction_open = true; + } + $this->applyInitialEffects($object, $xactions); + } + if (!$is_preview) { $errors = array(); $type_map = mgroup($xactions, 'getTransactionType'); @@ -529,6 +546,10 @@ } if ($errors) { + if ($transaction_open) { + $object->killTransaction(); + $transaction_open = false; + } throw new PhabricatorApplicationTransactionValidationException($errors); } @@ -544,26 +565,17 @@ // from the state when it was originally loaded. if ($this->shouldReadLock($object, $xaction)) { - $object->openTransaction(); + if (!$transaction_open) { + $object->openTransaction(); + $transaction_open = true; + } $object->beginReadLocking(); - $transaction_open = true; $read_locking = true; $object->reload(); break; } } } - - if ($this->shouldApplyInitialEffects($object, $xactions)) { - if (!$transaction_open) { - $object->openTransaction(); - $transaction_open = true; - } - } - } - - if ($this->shouldApplyInitialEffects($object, $xactions)) { - $this->applyInitialEffects($object, $xactions); } foreach ($xactions as $xaction) { @@ -587,7 +599,15 @@ // Now that we've merged, filtered, and combined transactions, check for // required capabilities. foreach ($xactions as $xaction) { - $this->requireCapabilities($object, $xaction); + try { + $this->requireCapabilities($object, $xaction); + } catch (PhabricatorPolicyException $ex) { + if ($transaction_open) { + $object->killTransaction(); + $transaction_open = false; + } + throw $ex; + } } $xactions = $this->sortTransactions($xactions); @@ -897,33 +917,30 @@ get_class($this))); } } - - // The actor must have permission to view and edit the object. - - $actor = $this->requireActor(); - - PhabricatorPolicyFilter::requireCapability( - $actor, - $object, - PhabricatorPolicyCapability::CAN_VIEW); } protected function requireCapabilities( PhabricatorLiskDAO $object, PhabricatorApplicationTransaction $xaction) { + $capabilities = array( + PhabricatorPolicyCapability::CAN_EDIT => 1, + PhabricatorPolicyCapability::CAN_VIEW => 1); + switch ($xaction->getTransactionType()) { case PhabricatorTransactions::TYPE_EDIT_POLICY: // You must have the edit capability to alter the edit policy of an - // object. For other default transaction types, we don't enforce - // anything for the moment. + // object. - PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT); + $capabilities[PhabricatorPolicyCapability::CAN_EDIT] = 1; break; } + + id(new PhabricatorPolicyFilter()) + ->setViewer($this->requireActor()) + ->requireCapabilities(array_keys($capabilities)) + ->raisePolicyExceptions(true) + ->apply(array($object)); } private function buildMentionTransaction(