From 6cbc67ea750129dc646117dc06cc0341ac5446f5 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 11 Aug 2012 07:02:31 -0700 Subject: [PATCH] Improve PolicyFilter and PolicyQuery Summary: - Allow PolicyQuery to require specific sets of capabilities other than "CAN_VIEW", like edit, etc. The default set is "view". - Add some convenience methods to PolicyFilter to test for capabilities. Test Plan: Viewed pastes, projects, etc. Used other stuff in future diff. Reviewers: vrana, btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T603 Differential Revision: https://secure.phabricator.com/D3212 --- .../policy/filter/PhabricatorPolicyFilter.php | 168 ++++++++++++------ .../query/policy/PhabricatorPolicyQuery.php | 19 +- 2 files changed, 135 insertions(+), 52 deletions(-) diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index b0d261b2b0..30608d741d 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -20,16 +20,52 @@ final class PhabricatorPolicyFilter { private $viewer; private $objects; - private $capability; + private $capabilities; private $raisePolicyExceptions; + public static function mustRetainCapability( + PhabricatorUser $user, + PhabricatorPolicyInterface $object, + $capability) { + + if (!self::hasCapability($user, $object, $capability)) { + throw new Exception( + "You can not make that edit, because it would remove your ability ". + "to '{$capability}' the object."); + } + } + + public static function requireCapability( + PhabricatorUser $user, + PhabricatorPolicyInterface $object, + $capability) { + $filter = new PhabricatorPolicyFilter(); + $filter->setViewer($user); + $filter->requireCapabilities(array($capability)); + $filter->raisePolicyExceptions(true); + $filter->apply(array($object)); + } + + public static function hasCapability( + PhabricatorUser $user, + PhabricatorPolicyInterface $object, + $capability) { + + $filter = new PhabricatorPolicyFilter(); + $filter->setViewer($user); + $filter->requireCapabilities(array($capability)); + $result = $filter->apply(array($object)); + + return (count($result) == 1); + } + public function setViewer(PhabricatorUser $user) { $this->viewer = $user; return $this; } - public function setCapability($capability) { - $this->capability = $capability; + public function requireCapabilities(array $capabilities) { + $this->capabilities = $capabilities; return $this; } @@ -41,91 +77,121 @@ final class PhabricatorPolicyFilter { public function apply(array $objects) { assert_instances_of($objects, 'PhabricatorPolicyInterface'); - $viewer = $this->viewer; - $capability = $this->capability; + $viewer = $this->viewer; + $capabilities = $this->capabilities; - if (!$viewer || !$capability) { + if (!$viewer || !$capabilities) { throw new Exception( - 'Call setViewer() and setCapability() before apply()!'); + 'Call setViewer() and requireCapabilities() before apply()!'); } $filtered = array(); foreach ($objects as $key => $object) { $object_capabilities = $object->getCapabilities(); + foreach ($capabilities as $capability) { + if (!in_array($capability, $object_capabilities)) { + throw new Exception( + "Testing for capability '{$capability}' on an object which does ". + "not have that capability!"); + } - if (!in_array($capability, $object_capabilities)) { - throw new Exception( - "Testing for capability '{$capability}' on an object which does not ". - "have that capability!"); + if (!$this->checkCapability($object, $capability)) { + // If we're missing any capability, move on to the next object. + continue 2; + } } - if ($object->hasAutomaticCapability($capability, $this->viewer)) { - $filtered[$key] = $object; - continue; - } + // If we make it here, we have all of the required capabilities. + $filtered[$key] = $object; + } + + return $filtered; + } + + private function checkCapability( + PhabricatorPolicyInterface $object, + $capability) { + + $policy = $object->getPolicy($capability); - $policy = $object->getPolicy($capability); + if (!$policy) { + // TODO: Formalize this somehow? + $policy = PhabricatorPolicies::POLICY_USER; + } + if ($policy == PhabricatorPolicies::POLICY_PUBLIC) { // If the object is set to "public" but that policy is disabled for this // install, restrict the policy to "user". - if ($policy == PhabricatorPolicies::POLICY_PUBLIC) { - if (!PhabricatorEnv::getEnvConfig('policy.allow-public')) { - $policy = PhabricatorPolicies::POLICY_USER; - } + if (!PhabricatorEnv::getEnvConfig('policy.allow-public')) { + $policy = PhabricatorPolicies::POLICY_USER; } - switch ($policy) { - case PhabricatorPolicies::POLICY_PUBLIC: - $filtered[$key] = $object; - break; - case PhabricatorPolicies::POLICY_USER: - if ($viewer->getPHID()) { - $filtered[$key] = $object; - } else { - $this->rejectObject($object, $policy); - } - break; - case PhabricatorPolicies::POLICY_ADMIN: - if ($viewer->getIsAdmin()) { - $filtered[$key] = $object; - } else { - $this->rejectObject($object, $policy); - } - break; - case PhabricatorPolicies::POLICY_NOONE: - $this->rejectObject($object, $policy); - break; - default: - throw new Exception("Object has unknown policy '{$policy}'!"); + // If the object is set to "public" but the capability is anything other + // than "view", restrict the policy to "user". + if ($capability != PhabricatorPolicyCapability::CAN_VIEW) { + $policy = PhabricatorPolicies::POLICY_USER; } } - return $filtered; + $viewer = $this->viewer; + + if ($object->hasAutomaticCapability($capability, $viewer)) { + return true; + } + + switch ($policy) { + case PhabricatorPolicies::POLICY_PUBLIC: + return true; + case PhabricatorPolicies::POLICY_USER: + if ($viewer->getPHID()) { + return true; + } else { + $this->rejectObject($object, $policy, $capability); + } + break; + case PhabricatorPolicies::POLICY_ADMIN: + if ($viewer->getIsAdmin()) { + return true; + } else { + $this->rejectObject($object, $policy, $capability); + } + break; + case PhabricatorPolicies::POLICY_NOONE: + $this->rejectObject($object, $policy, $capability); + break; + default: + throw new Exception("Object has unknown policy '{$policy}'!"); + } + + return false; } - private function rejectObject($object, $policy) { + private function rejectObject($object, $policy, $capability) { if (!$this->raisePolicyExceptions) { return; } - $message = "You do not have permission to view this object."; + // TODO: clean this up + $verb = $capability; + + $message = "You do not have permission to {$verb} this object."; switch ($policy) { case PhabricatorPolicies::POLICY_PUBLIC: - $who = "This is curious, since anyone can view the object."; + $who = "This is curious, since anyone can {$verb} the object."; break; case PhabricatorPolicies::POLICY_USER: - $who = "To view this object, you must be logged in."; + $who = "To {$verb} this object, you must be logged in."; break; case PhabricatorPolicies::POLICY_ADMIN: - $who = "To view this object, you must be an administrator."; + $who = "To {$verb} this object, you must be an administrator."; break; case PhabricatorPolicies::POLICY_NOONE: - $who = "No one can view this object."; + $who = "No one can {$verb} this object."; break; default: - $who = "It is unclear who can view this object."; + $who = "It is unclear who can {$verb} this object."; break; } diff --git a/src/infrastructure/query/policy/PhabricatorPolicyQuery.php b/src/infrastructure/query/policy/PhabricatorPolicyQuery.php index a1dc23ae26..941005a251 100644 --- a/src/infrastructure/query/policy/PhabricatorPolicyQuery.php +++ b/src/infrastructure/query/policy/PhabricatorPolicyQuery.php @@ -46,6 +46,7 @@ abstract class PhabricatorPolicyQuery extends PhabricatorOffsetPagedQuery { private $viewer; private $raisePolicyExceptions; private $rawResultLimit; + private $capabilities; /* -( Query Configuration )------------------------------------------------ */ @@ -77,6 +78,15 @@ abstract class PhabricatorPolicyQuery extends PhabricatorOffsetPagedQuery { } + /** + * @task config + */ + final public function requireCapabilities(array $capabilities) { + $this->capabilities = $capabilities; + return $this; + } + + /* -( Query Execution )---------------------------------------------------- */ @@ -145,8 +155,15 @@ abstract class PhabricatorPolicyQuery extends PhabricatorOffsetPagedQuery { $filter = new PhabricatorPolicyFilter(); $filter->setViewer($this->viewer); - $filter->setCapability(PhabricatorPolicyCapability::CAN_VIEW); + if (!$this->capabilities) { + $capabilities = array( + PhabricatorPolicyCapability::CAN_VIEW, + ); + } else { + $capabilities = $this->capabilities; + } + $filter->requireCapabilities($capabilities); $filter->raisePolicyExceptions($this->raisePolicyExceptions); $offset = (int)$this->getOffset(); -- 2.51.2