From d6247ffca59ccecbdc3bd733e879945143ce2152 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 11 May 2015 14:23:35 -0700 Subject: [PATCH] Add support for "Extended Policies" Summary: Ref T7703. See that task and inline for a bunch of discussion. Briefly, when we run implicit policy rules ("to see a revision, you must also be able to see its repository") at query time, they don't apply to other viewers we might check later. We do this very rarely, but when we do we're often doing it for a bunch of different viewers (for example, in Herald) so I don't want to just reload the object a million times. Test Plan: - Added and executed unit tests. - Wrote a "flag everything" Herald rule, as in the original report in T7703, and no longer got "Unknown Object" flags on revisions. - Rigged up a lot of cases in the web UI and couldn't find any inconsistencies, although this case is normally very hard to hit. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T7703 Differential Revision: https://secure.phabricator.com/D13104 --- src/__phutil_library_map__.php | 7 +- .../storage/DifferentialRevision.php | 44 ++++ .../__tests__/PhabricatorPolicyTestCase.php | 97 ++++++++ .../__tests__/PhabricatorPolicyTestObject.php | 28 ++- .../policy/filter/PhabricatorPolicyFilter.php | 214 +++++++++++++++++- .../PhabricatorExtendedPolicyInterface.php | 88 +++++++ 6 files changed, 470 insertions(+), 8 deletions(-) create mode 100644 src/applications/policy/interface/PhabricatorExtendedPolicyInterface.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 03278a83d9..ca27915604 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1794,6 +1794,7 @@ phutil_register_library_map(array( '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 @@ phutil_register_library_map(array( 'DifferentialDAO', 'PhabricatorTokenReceiverInterface', 'PhabricatorPolicyInterface', + 'PhabricatorExtendedPolicyInterface', 'PhabricatorFlaggableInterface', 'PhrequentTrackableInterface', 'HarbormasterBuildableInterface', @@ -5698,7 +5700,10 @@ phutil_register_library_map(array( '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 index 0a4d459568..fec69ecf61 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -4,6 +4,7 @@ final class DifferentialRevision extends DifferentialDAO implements PhabricatorTokenReceiverInterface, PhabricatorPolicyInterface, + PhabricatorExtendedPolicyInterface, PhabricatorFlaggableInterface, PhrequentTrackableInterface, HarbormasterBuildableInterface, @@ -280,6 +281,10 @@ final class DifferentialRevision extends DifferentialDAO return $this; } + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + public function getCapabilities() { return array( PhabricatorPolicyCapability::CAN_VIEW, @@ -326,6 +331,45 @@ final class DifferentialRevision extends DifferentialDAO 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 index 327c8a2856..b2475cdee5 100644 --- a/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php +++ b/src/applications/policy/__tests__/PhabricatorPolicyTestCase.php @@ -189,6 +189,102 @@ final class PhabricatorPolicyTestCase extends PhabricatorTestCase { } + /** + * 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 @@ final class PhabricatorPolicyTestCase extends PhabricatorTestCase { $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 index 041b612757..60c77aea83 100644 --- 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 @@ final class PhabricatorPolicyTestObject 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 index 8d52ddc044..88c4c078f3 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -196,7 +196,6 @@ final class PhabricatorPolicyFilter { } 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 @@ final class PhabricatorPolicyFilter { $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 index 0000000000..deeaecdbef --- /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; + } + +*/