Permalink
Browse files

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
  • Loading branch information...
1 parent 62b3e4a commit 6cbc67ea750129dc646117dc06cc0341ac5446f5 @epriestley epriestley committed Aug 11, 2012
View
168 src/applications/policy/filter/PhabricatorPolicyFilter.php
@@ -20,16 +20,52 @@
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 @@ public function raisePolicyExceptions($raise) {
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;
}
View
19 src/infrastructure/query/policy/PhabricatorPolicyQuery.php
@@ -46,6 +46,7 @@
private $viewer;
private $raisePolicyExceptions;
private $rawResultLimit;
+ private $capabilities;
/* -( Query Configuration )------------------------------------------------ */
@@ -77,6 +78,15 @@
}
+ /**
+ * @task config
+ */
+ final public function requireCapabilities(array $capabilities) {
+ $this->capabilities = $capabilities;
+ return $this;
+ }
+
+
/* -( Query Execution )---------------------------------------------------- */
@@ -145,8 +155,15 @@
$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();

0 comments on commit 6cbc67e

Please sign in to comment.