Make the "you can't edit away your edit capability" policy check generic

Summary:
Ref T4379. Currently, you can edit away your edit capability in Projects. Prevent this in a general way.

Since some objects have complex edit policies (like "the owner can always edit"), we can't just check the value itself. We also can't fairly assume that every object has a `setEditPolicy()` method, even though almost all do right now. Instead, provide a way to pretend we've completed the edit and changed the policy.

Test Plan: Unit tests, tried to edit away my edit capability.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4379

Differential Revision: https://secure.phabricator.com/D8179

src/applications/policy/filter/PhabricatorPolicyFilter.php

@@ -8,6 +8,7 @@ final class PhabricatorPolicyFilter {
   private $raisePolicyExceptions;
   private $userProjects;
   private $customPolicies = array();
+  private $forcedPolicy;
 
   public static function mustRetainCapability(
     PhabricatorUser $user,
@@ -25,11 +26,48 @@ final class PhabricatorPolicyFilter {
     PhabricatorUser $user,
     PhabricatorPolicyInterface $object,
     $capability) {
-    $filter = new PhabricatorPolicyFilter();
+    $filter = id(new PhabricatorPolicyFilter())
-    $filter->setViewer($user);
+      ->setViewer($user)
-    $filter->requireCapabilities(array($capability));
+      ->requireCapabilities(array($capability))
-    $filter->raisePolicyExceptions(true);
+      ->raisePolicyExceptions(true)
-    $filter->apply(array($object));
+      ->apply(array($object));
+  }
+
+  /**
+   * Perform a capability check, acting as though an object had a specific
+   * policy. This is primarily used to check if a policy is valid (for example,
+   * to prevent users from editing away their ability to edit an object).
+   *
+   * Specifically, a check like this:
+   *
+   *   PhabricatorPolicyFilter::requireCapabilityWithForcedPolicy(
+   *     $viewer,
+   *     $object,
+   *     PhabricatorPolicyCapability::CAN_EDIT,
+   *     $potential_new_policy);
+   *
+   * ...will throw a @{class:PhabricatorPolicyException} if the new policy would
+   * remove the user's ability to edit the object.
+   *
+   * @param PhabricatorUser   The viewer to perform a policy check for.
+   * @param PhabricatorPolicyInterface The object to perform a policy check on.
+   * @param string            Capability to test.
+   * @param string            Perform the test as though the object has this
+   *                          policy instead of the policy it actually has.
+   * @return void
+   */
+  public static function requireCapabilityWithForcedPolicy(
+    PhabricatorUser $viewer,
+    PhabricatorPolicyInterface $object,
+    $capability,
+    $forced_policy) {
+
+    id(new PhabricatorPolicyFilter())
+      ->setViewer($viewer)
+      ->requireCapabilities(array($capability))
+      ->raisePolicyExceptions(true)
+      ->forcePolicy($forced_policy)
+      ->apply(array($object));
   }
 
   public static function hasCapability(
@@ -60,6 +98,11 @@ final class PhabricatorPolicyFilter {
     return $this;
   }
 
+  public function forcePolicy($forced_policy) {
+    $this->forcedPolicy = $forced_policy;
+    return $this;
+  }
+
   public function apply(array $objects) {
     assert_instances_of($objects, 'PhabricatorPolicyInterface');
 
@@ -96,7 +139,7 @@ final class PhabricatorPolicyFilter {
             "not have that capability!");
         }
 
-        $policy = $object->getPolicy($capability);
+        $policy = $this->getObjectPolicy($object, $capability);
         $type = phid_get_type($policy);
         if ($type == PhabricatorProjectPHIDTypeProject::TYPECONST) {
           $need_projects[$policy] = $policy;
@@ -169,7 +212,7 @@ final class PhabricatorPolicyFilter {
     PhabricatorPolicyInterface $object,
     $capability) {
 
-    $policy = $object->getPolicy($capability);
+    $policy = $this->getObjectPolicy($object, $capability);
 
     if (!$policy) {
       // TODO: Formalize this somehow?
@@ -402,4 +445,15 @@ final class PhabricatorPolicyFilter {
     return false;
   }
 
+  private function getObjectPolicy(
+    PhabricatorPolicyInterface $object,
+    $capability) {
+
+    if ($this->forcedPolicy) {
+      return $this->forcedPolicy;
+    } else {
+      return $object->getPolicy($capability);
+    }
+  }
+
 }

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -1144,6 +1144,29 @@ abstract class PhabricatorApplicationTransactionEditor
 
     $errors = array();
     switch ($type) {
+      case PhabricatorTransactions::TYPE_EDIT_POLICY:
+        // Make sure the user isn't editing away their ability to edit this
+        // object.
+        foreach ($xactions as $xaction) {
+          try {
+            PhabricatorPolicyFilter::requireCapabilityWithForcedPolicy(
+              $this->requireActor(),
+              $object,
+              PhabricatorPolicyCapability::CAN_EDIT,
+              $xaction->getNewValue());
+          } catch (PhabricatorPolicyException $ex) {
+            $errors[] = array(
+              new PhabricatorApplicationTransactionValidationError(
+                $type,
+                pht('Invalid'),
+                pht(
+                  'You can not select this edit policy, because you would '.
+                  'no longer be able to edit the object.'),
+                $xaction),
+            );
+          }
+        }
+        break;
       case PhabricatorTransactions::TYPE_CUSTOMFIELD:
         $groups = array();
         foreach ($xactions as $xaction) {