From e6118bcbaf7d227d70e5d1da3e5e7ad933f93fd2 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Thu, 13 Mar 2014 13:50:08 -0700 Subject: [PATCH] Tweak application and maniphest editors to handle policy corner cases better Summary: Fixes T4362. If you have a default edit + view policy of "no one" things get weird when you try to create a task - basically its impossible. Ergo, re-jigger how we do policy checks just a bit. - if its a new object, don't bother with the "can the $actor edit this thing by virtue of having can see / can edit priveleges?" That makes no sense on create. - add a hook so when doing the "will $actor still be able to edit this thing after all the edits" checks the object can be updated to its ultimate state. This matters for Maniphest as being the owner lets you do all sorts of stuff. Test Plan: - made a task with no one policy and assigned to no one - exception - made a task with no one policy and assigned to me - success - made a comment on the task - success - reassigned the task to another user - exception - reassigned the task to another user and updated policies to "users" - success Reviewers: epriestley Reviewed By: epriestley Subscribers: aran, epriestley, Korvin Maniphest Tasks: T4362 Differential Revision: https://secure.phabricator.com/D8508 --- .../editor/ManiphestTransactionEditor.php | 17 +++ ...habricatorApplicationTransactionEditor.php | 137 +++++++++++++----- 2 files changed, 118 insertions(+), 36 deletions(-) diff --git a/src/applications/maniphest/editor/ManiphestTransactionEditor.php b/src/applications/maniphest/editor/ManiphestTransactionEditor.php index 2ef22808e7..90489b7077 100644 --- a/src/applications/maniphest/editor/ManiphestTransactionEditor.php +++ b/src/applications/maniphest/editor/ManiphestTransactionEditor.php @@ -409,6 +409,23 @@ final class ManiphestTransactionEditor } } + protected function adjustObjectForPolicyChecks( + PhabricatorLiskDAO $object, + array $xactions) { + + $copy = parent::adjustObjectForPolicyChecks($object, $xactions); + foreach ($xactions as $xaction) { + switch ($xaction->getTransactionType()) { + case ManiphestTransaction::TYPE_OWNER: + $copy->setOwnerPHID($xaction->getNewValue()); + break; + default: + continue; + } + } + + return $copy; + } private function getNextSubpriority($pri, $sub) { diff --git a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php index 75c50fbc80..63585c5364 100644 --- a/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php +++ b/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php @@ -897,29 +897,39 @@ abstract class PhabricatorApplicationTransactionEditor 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) { - 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. + if ($this->getIsNewObject()) { + return; + } + $actor = $this->requireActor(); + switch ($xaction->getTransactionType()) { + case PhabricatorTransactions::TYPE_COMMENT: PhabricatorPolicyFilter::requireCapability( - $this->requireActor(), + $actor, + $object, + PhabricatorPolicyCapability::CAN_VIEW); + break; + case PhabricatorTransactions::TYPE_VIEW_POLICY: + PhabricatorPolicyFilter::requireCapability( + $actor, + $object, + PhabricatorPolicyCapability::CAN_EDIT); + break; + case PhabricatorTransactions::TYPE_EDIT_POLICY: + PhabricatorPolicyFilter::requireCapability( + $actor, + $object, + PhabricatorPolicyCapability::CAN_EDIT); + break; + case PhabricatorTransactions::TYPE_JOIN_POLICY: + PhabricatorPolicyFilter::requireCapability( + $actor, $object, PhabricatorPolicyCapability::CAN_EDIT); break; @@ -1464,28 +1474,19 @@ abstract class PhabricatorApplicationTransactionEditor $errors = array(); switch ($type) { + case PhabricatorTransactions::TYPE_VIEW_POLICY: + $errors[] = $this->validatePolicyTransaction( + $object, + $xactions, + $type, + PhabricatorPolicyCapability::CAN_VIEW); + break; case PhabricatorTransactions::TYPE_EDIT_POLICY: - // Make sure the user isn't editing away their ability to edit this - // object. - foreach ($xactions as $xaction) { - try { - PhabricatorPolicyFilter::requireCapabilityWithForcedPolicy( - $this->requireActor(), - $object, - PhabricatorPolicyCapability::CAN_EDIT, - $xaction->getNewValue()); - } catch (PhabricatorPolicyException $ex) { - $errors[] = array( - new PhabricatorApplicationTransactionValidationError( - $type, - pht('Invalid'), - pht( - 'You can not select this edit policy, because you would '. - 'no longer be able to edit the object.'), - $xaction), - ); - } - } + $errors[] = $this->validatePolicyTransaction( + $object, + $xactions, + $type, + PhabricatorPolicyCapability::CAN_EDIT); break; case PhabricatorTransactions::TYPE_CUSTOMFIELD: $groups = array(); @@ -1514,6 +1515,70 @@ abstract class PhabricatorApplicationTransactionEditor return array_mergev($errors); } + private function validatePolicyTransaction( + PhabricatorLiskDAO $object, + array $xactions, + $transaction_type, + $capability) { + + $actor = $this->requireActor(); + $errors = array(); + // Note $this->xactions is necessary; $xactions is $this->xactions of + // $transaction_type + $policy_object = $this->adjustObjectForPolicyChecks( + $object, + $this->xactions); + + // Make sure the user isn't editing away their ability to $capability this + // object. + foreach ($xactions as $xaction) { + try { + PhabricatorPolicyFilter::requireCapabilityWithForcedPolicy( + $actor, + $policy_object, + $capability, + $xaction->getNewValue()); + } catch (PhabricatorPolicyException $ex) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $transaction_type, + pht('Invalid'), + pht( + 'You can not select this %s policy, because you would no longer '. + 'be able to %s the object.', + $capability, + $capability), + $xaction); + } + } + + if ($this->getIsNewObject()) { + if (!$xactions) { + $has_capability = PhabricatorPolicyFilter::hasCapability( + $actor, + $policy_object, + $capability); + if (!$has_capability) { + $errors[] = new PhabricatorApplicationTransactionValidationError( + $transaction_type, + pht('Invalid'), + pht('The selected %s policy excludes you. Choose a %s policy '. + 'which allows you to %s the object.', + $capability, + $capability, + $capability)); + } + } + } + + return $errors; + } + + protected function adjustObjectForPolicyChecks( + PhabricatorLiskDAO $object, + array $xactions) { + + return clone $object; + } /** * Check for a missing text field.