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

@@ -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(

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(),

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) {

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());
+  }
+
 }

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, '<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(

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;
+  }
+
+*/