D13104.id31769.diff
No OneTemporary
Actions

Size

19 KB

Referenced Files

None

Subscribers

None

D13104.id31769.diff
View Options

	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, '<cycle>');
	+ 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, '<bad-ref>');
	+ 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, '<extended>');
	+ }
	+ }
	+ }
	+
	+ 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 @@
	+<?php
	+
	+/**
	+ * Allows an object to define a more complex policy than it can with
	+ * @{interface:PhabricatorPolicyInterface} alone.
	+ *
	+ * Some objects have complex policies which depend on the policies of other
	+ * objects. For example, users can generally only see a Revision in
	+ * Differential if they can also see the Repository it belongs to.
	+ *
	+ * These policies are normally enforced implicitly in the Query layer, by
	+ * discarding objects which have related objects that can not be loaded. In
	+ * most cases this has the same effect as really applying these policy checks
	+ * would.
	+ *
	+ * However, in some cases an object's policies are later checked by a different
	+ * viewer. For example, before we execute Herald rules, we check that the rule
	+ * owners can see the object we are about to evaluate.
	+ *
	+ * In these cases, we need to account for these complex policies. We could do
	+ * this by reloading the object over and over again for each viewer, but this
	+ * implies a large performance cost. Instead, extended policies make it
	+ * efficient to check policies against an object for multiple viewers.
	+ */
	+interface PhabricatorExtendedPolicyInterface {
	+
	+ /**
	+ * Get the extended policy for an object.
	+ *
	+ * Return a list of additional policy checks that the viewer must satisfy
	+ * in order to have the specified capability. This allows you to encode rules
	+ * like "to see a revision, the viewer must also be able to see the repository
	+ * it belongs to".
	+ *
	+ * For example, to specify that the viewer must be able to see some other
	+ * object in order to see this one, you could return:
	+ *
	+ * return array(
	+ * array($other_object, PhabricatorPolicyCapability::CAN_VIEW),
	+ * // ...
	+ * );
	+ *
	+ * If you don't have the actual object you want to check, you can return its
	+ * PHID instead:
	+ *
	+ * return array(
	+ * array($other_phid, PhabricatorPolicyCapability::CAN_VIEW),
	+ * // ...
	+ * );
	+ *
	+ * You can return a list of capabilities instead of a single capability if
	+ * you want to require multiple capabilities on a single object:
	+ *
	+ * return array(
	+ * array(
	+ * $other_object,
	+ * array(
	+ * PhabricatorPolicyCapability::CAN_VIEW,
	+ * PhabricatorPolicyCapability::CAN_EDIT,
	+ * ),
	+ * ),
	+ * // ...
	+ * );
	+ *
	+ * @param const Capability being tested.
	+ * @param PhabricatorUser Viewer whose capabilities are being tested.
	+ * @return list<pair<wild, wild>> 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;
	+ }
	+
	+*/

File Metadata

Mime Type: text/plain
Expires: Wed, May 22, 10:15 AM (3 w, 4 d ago)
Storage Engine: blob
Storage Format: Encrypted (AES-256-CBC)
Storage Handle: 6288494
Default Alt Text: D13104.id31769.diff (19 KB)

D13104.id31769.diffNo OneTemporaryActions

D13104.id31769.diffView Options

File Metadata

Event Timeline

D13104.id31769.diff
No OneTemporary
Actions

D13104.id31769.diff
View Options