diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1794,6 +1794,7 @@ 'PhabricatorEventListener' => 'infrastructure/events/PhabricatorEventListener.php', 'PhabricatorEventType' => 'infrastructure/events/constant/PhabricatorEventType.php', 'PhabricatorExampleEventListener' => 'infrastructure/events/PhabricatorExampleEventListener.php', + 'PhabricatorExtendedPolicyInterface' => 'applications/policy/interface/PhabricatorExtendedPolicyInterface.php', 'PhabricatorExtendingPhabricatorConfigOptions' => 'applications/config/option/PhabricatorExtendingPhabricatorConfigOptions.php', 'PhabricatorExtensionsSetupCheck' => 'applications/config/check/PhabricatorExtensionsSetupCheck.php', 'PhabricatorExternalAccount' => 'applications/people/storage/PhabricatorExternalAccount.php', @@ -3668,6 +3669,7 @@ 'DifferentialDAO', 'PhabricatorTokenReceiverInterface', 'PhabricatorPolicyInterface', + 'PhabricatorExtendedPolicyInterface', 'PhabricatorFlaggableInterface', 'PhrequentTrackableInterface', 'HarbormasterBuildableInterface', @@ -5698,7 +5700,10 @@ 'PhabricatorPolicyPHIDTypePolicy' => 'PhabricatorPHIDType', 'PhabricatorPolicyQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorPolicyTestCase' => 'PhabricatorTestCase', - 'PhabricatorPolicyTestObject' => 'PhabricatorPolicyInterface', + 'PhabricatorPolicyTestObject' => array( + 'PhabricatorPolicyInterface', + 'PhabricatorExtendedPolicyInterface', + ), 'PhabricatorPolicyType' => 'PhabricatorPolicyConstants', 'PhabricatorPonderApplication' => 'PhabricatorApplication', 'PhabricatorProject' => array( diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -4,6 +4,7 @@ implements PhabricatorTokenReceiverInterface, PhabricatorPolicyInterface, + PhabricatorExtendedPolicyInterface, PhabricatorFlaggableInterface, PhrequentTrackableInterface, HarbormasterBuildableInterface, @@ -280,6 +281,10 @@ return $this; } + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + public function getCapabilities() { return array( PhabricatorPolicyCapability::CAN_VIEW, @@ -326,6 +331,45 @@ return $description; } + +/* -( PhabricatorExtendedPolicyInterface )--------------------------------- */ + + + public function getExtendedPolicy($capability, PhabricatorUser $viewer) { + $extended = array(); + + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + // NOTE: In Differential, an automatic capability on a revision (being + // an author) is sufficient to view it, even if you can not see the + // repository the revision belongs to. We can bail out early in this + // case. + if ($this->hasAutomaticCapability($capability, $viewer)) { + break; + } + + $repository_phid = $this->getRepositoryPHID(); + $repository = $this->getRepository(); + + // Try to use the object if we have it, since it will save us some + // data fetching later on. In some cases, we might not have it. + $repository_ref = nonempty($repository, $repository_phid); + if ($repository_ref) { + $extended[] = array( + $repository_ref, + PhabricatorPolicyCapability::CAN_VIEW, + ); + } + break; + } + + return $extended; + } + + +/* -( PhabricatorTokenReceiverInterface )---------------------------------- */ + + public function getUsersToNotifyOfTokenGiven() { return array( $this->getAuthorPHID(), diff --git a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php --- a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php +++ b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php @@ -190,6 +190,102 @@ /** + * Test that extended policies work. + */ + public function testExtendedPolicies() { + $object = $this->buildObject(PhabricatorPolicies::POLICY_USER) + ->setPHID('PHID-TEST-1'); + + $this->expectVisibility( + $object, + array( + 'public' => false, + 'user' => true, + 'admin' => true, + ), + pht('No Extended Policy')); + + // Add a restrictive extended policy. + $extended = $this->buildObject(PhabricatorPolicies::POLICY_ADMIN) + ->setPHID('PHID-TEST-2'); + $object->setExtendedPolicies( + array( + PhabricatorPolicyCapability::CAN_VIEW => array( + array($extended, PhabricatorPolicyCapability::CAN_VIEW), + ), + )); + + $this->expectVisibility( + $object, + array( + 'public' => false, + 'user' => false, + 'admin' => true, + ), + pht('With Extended Policy')); + + // Depend on a different capability. + $object->setExtendedPolicies( + array( + PhabricatorPolicyCapability::CAN_VIEW => array( + array($extended, PhabricatorPolicyCapability::CAN_EDIT), + ), + )); + + $extended->setCapabilities(array(PhabricatorPolicyCapability::CAN_EDIT)); + $extended->setPolicies( + array( + PhabricatorPolicyCapability::CAN_EDIT => + PhabricatorPolicies::POLICY_NOONE, + )); + + $this->expectVisibility( + $object, + array( + 'public' => false, + 'user' => false, + 'admin' => false, + ), + pht('With Extended Policy + Edit')); + } + + + /** + * Test that cyclic extended policies are arrested properly. + */ + public function testExtendedPolicyCycles() { + $object = $this->buildObject(PhabricatorPolicies::POLICY_USER) + ->setPHID('PHID-TEST-1'); + + $this->expectVisibility( + $object, + array( + 'public' => false, + 'user' => true, + 'admin' => true, + ), + pht('No Extended Policy')); + + // Set a self-referential extended policy on the object. This should + // make it fail all policy checks. + $object->setExtendedPolicies( + array( + PhabricatorPolicyCapability::CAN_VIEW => array( + array($object, PhabricatorPolicyCapability::CAN_VIEW), + ), + )); + + $this->expectVisibility( + $object, + array( + 'public' => false, + 'user' => false, + 'admin' => false, + ), + pht('Extended Policy with Cycle')); + } + + /** * An omnipotent user should be able to see even objects with invalid * policies. */ @@ -274,6 +370,7 @@ $query->setViewer($viewer); $caught = null; + $result = null; try { $result = $query->executeOne(); } catch (PhabricatorPolicyException $ex) { diff --git a/src/applications/policy/__tests__/PhabricatorPolicyTestObject.php b/src/applications/policy/__tests__/PhabricatorPolicyTestObject.php --- a/src/applications/policy/__tests__/PhabricatorPolicyTestObject.php +++ b/src/applications/policy/__tests__/PhabricatorPolicyTestObject.php @@ -4,14 +4,23 @@ * Configurable test object for implementing Policy unit tests. */ final class PhabricatorPolicyTestObject - implements PhabricatorPolicyInterface { + implements + PhabricatorPolicyInterface, + PhabricatorExtendedPolicyInterface { - private $capabilities = array(); - private $policies = array(); - private $automaticCapabilities = array(); + private $phid; + private $capabilities = array(); + private $policies = array(); + private $automaticCapabilities = array(); + private $extendedPolicies = array(); + + public function setPHID($phid) { + $this->phid = $phid; + return $this; + } public function getPHID() { - return null; + return $this->phid; } public function getCapabilities() { @@ -46,4 +55,13 @@ return null; } + public function setExtendedPolicies(array $extended_policies) { + $this->extendedPolicies = $extended_policies; + return $this; + } + + public function getExtendedPolicy($capability, PhabricatorUser $viewer) { + return idx($this->extendedPolicies, $capability, array()); + } + } diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -196,7 +196,6 @@ } foreach ($objects as $key => $object) { - $object_capabilities = $object->getCapabilities(); foreach ($capabilities as $capability) { if (!$this->checkCapability($object, $capability)) { // If we're missing any capability, move on to the next object. @@ -208,7 +207,218 @@ $filtered[$key] = $object; } - return $filtered; + // If we survied the primary checks, apply extended checks to objects + // with extended policies. + $results = array(); + $extended = array(); + foreach ($filtered as $key => $object) { + if ($object instanceof PhabricatorExtendedPolicyInterface) { + $extended[$key] = $object; + } else { + $results[$key] = $object; + } + } + + if ($extended) { + $results += $this->applyExtendedPolicyChecks($extended); + // Put results back in the original order. + $results = array_select_keys($results, array_keys($filtered)); + } + + return $results; + } + + private function applyExtendedPolicyChecks(array $extended_objects) { + // First, we're going to detect cycles and reject any objects which are + // part of a cycle. We don't want to loop forever if an object has a + // self-referential or nonsense policy. + + static $in_flight = array(); + + $all_phids = array(); + foreach ($extended_objects as $key => $object) { + $phid = $object->getPHID(); + if (isset($in_flight[$phid])) { + // TODO: This could be more user-friendly. + $this->rejectObject($extended_objects[$key], false, ''); + unset($extended_objects[$key]); + continue; + } + + // We might throw from rejectObject(), so we don't want to actually mark + // anything as in-flight until we survive this entire step. + $all_phids[$phid] = $phid; + } + + foreach ($all_phids as $phid) { + $in_flight[$phid] = true; + } + + $caught = null; + try { + $extended_objects = $this->executeExtendedPolicyChecks($extended_objects); + } catch (Exception $ex) { + $caught = $ex; + } + + foreach ($all_phids as $phid) { + unset($in_flight[$phid]); + } + + if ($caught) { + throw $caught; + } + + return $extended_objects; + } + + private function executeExtendedPolicyChecks(array $extended_objects) { + $viewer = $this->viewer; + $filter_capabilities = $this->capabilities; + + // Iterate over the objects we need to filter and pull all the nonempty + // policies into a flat, structured list. + $all_structs = array(); + foreach ($extended_objects as $key => $extended_object) { + foreach ($filter_capabilities as $extended_capability) { + $extended_policies = $extended_object->getExtendedPolicy( + $extended_capability, + $viewer); + if (!$extended_policies) { + continue; + } + + foreach ($extended_policies as $extended_policy) { + list($object, $capabilities) = $extended_policy; + + // Build a description of the capabilities we need to check. This + // will be something like `"view"`, or `"edit view"`, or possibly + // a longer string with custom capabilities. Later, group the objects + // up into groups which need the same capabilities tested. + $capabilities = (array)$capabilities; + $capabilities = array_fuse($capabilities); + ksort($capabilities); + $group = implode(' ', $capabilities); + + $struct = array( + 'key' => $key, + 'for' => $extended_capability, + 'object' => $object, + 'capabilities' => $capabilities, + 'group' => $group, + ); + + $all_structs[] = $struct; + } + } + } + + // Extract any bare PHIDs from the structs; we need to load these objects. + // These are objects which are required in order to perform an extended + // policy check but which the original viewer did not have permission to + // see (they presumably had other permissions which let them load the + // object in the first place). + $all_phids = array(); + foreach ($all_structs as $idx => $struct) { + $object = $struct['object']; + if (is_string($object)) { + $all_phids[$object] = $object; + } + } + + // If we have some bare PHIDs, we need to load the corresponding objects. + if ($all_phids) { + // We can pull these with the omnipotent user because we're immediately + // filtering them. + $ref_objects = id(new PhabricatorObjectQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withPHIDs($all_phids) + ->execute(); + $ref_objects = mpull($ref_objects, null, 'getPHID'); + } else { + $ref_objects = array(); + } + + // Group the list of checks by the capabilities we need to check. + $groups = igroup($all_structs, 'group'); + foreach ($groups as $structs) { + $head = head($structs); + + // All of the items in each group are checking for the same capabilities. + $capabilities = $head['capabilities']; + + $key_map = array(); + $objects_in = array(); + foreach ($structs as $struct) { + $extended_key = $struct['key']; + if (empty($extended_objects[$key])) { + // If this object has already been rejected by an earlier filtering + // pass, we don't need to do any tests on it. + continue; + } + + $object = $struct['object']; + if (is_string($object)) { + // This is really a PHID, so look it up. + $object_phid = $object; + if (empty($ref_objects[$object_phid])) { + // We weren't able to load the corresponding object, so just + // reject this result outright. + + $reject = $extended_objects[$key]; + unset($extended_objects[$key]); + + // TODO: This could be friendlier. + $this->rejectObject($reject, false, ''); + continue; + } + $object = $ref_objects[$object_phid]; + } + + $phid = $object->getPHID(); + + $key_map[$phid][] = $extended_key; + $objects_in[$phid] = $object; + } + + if ($objects_in) { + $objects_out = id(new PhabricatorPolicyFilter()) + ->setViewer($viewer) + ->requireCapabilities($capabilities) + ->apply($objects_in); + $objects_out = mpull($objects_out, null, 'getPHID'); + } else { + $objects_out = array(); + } + + // If any objects were removed by filtering, we're going to reject all + // of the original objects which needed them. + foreach ($objects_in as $phid => $object_in) { + if (isset($objects_out[$phid])) { + // This object survived filtering, so we don't need to throw any + // results away. + continue; + } + + foreach ($key_map[$phid] as $extended_key) { + if (empty($extended_objects[$extended_key])) { + // We've already rejected this object, so we don't need to reject + // it again. + continue; + } + + $reject = $extended_objects[$extended_key]; + unset($extended_objects[$extended_key]); + + // TODO: This isn't as user-friendly as it could be. It's possible + // that we're rejecting this object for multiple capability/policy + // failures, though. + $this->rejectObject($reject, false, ''); + } + } + } + + return $extended_objects; } private function checkCapability( diff --git a/src/applications/policy/interface/PhabricatorExtendedPolicyInterface.php b/src/applications/policy/interface/PhabricatorExtendedPolicyInterface.php new file mode 100644 --- /dev/null +++ b/src/applications/policy/interface/PhabricatorExtendedPolicyInterface.php @@ -0,0 +1,88 @@ +> List of extended policies. + */ + public function getExtendedPolicy($capability, PhabricatorUser $viewer); + +} + +// TEMPLATE IMPLEMENTATION ///////////////////////////////////////////////////// + +/* -( PhabricatorExtendedPolicyInterface )--------------------------------- */ +/* + + public function getExtendedPolicy($capability, PhabricatorUser $viewer) { + $extended = array(); + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + // ... + break; + } + return $extended; + } + +*/