Simplify custom policies before saving, and reject meaningless policies

Summary:
Ref T603. Do a little more sanity checking on custom policies, so policies like this:

  [ Allow ] [ Users ] [ <no users> ]

...that don't specify anything and thus which aren't meaningful raise errors.

Test Plan: {F69570}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

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

src/applications/policy/controller/PhabricatorPolicyEditController.php

@@ -49,6 +49,7 @@ final class PhabricatorPolicyEditController
     $default_action = $policy->getDefaultAction();
     $rule_data = $policy->getRules();
 
+    $errors = array();
     if ($request->isFormPost()) {
       $data = $request->getStr('rules');
       $data = @json_decode($data, true);
@@ -83,26 +84,42 @@ final class PhabricatorPolicyEditController
         );
       }
 
+      // Filter out nonsense rules, like a "users" rule without any users
+      // actually specified.
+      $valid_rules = array();
+      foreach ($rule_data as $rule) {
+        $rule_class = $rule['rule'];
+        if ($rules[$rule_class]->ruleHasEffect($rule['value'])) {
+          $valid_rules[] = $rule;
+        }
+      }
+
+      if (!$valid_rules) {
+        $errors[] = pht('None of these policy rules have any effect.');
+      }
+
       // NOTE: Policies are immutable once created, and we always create a new
       // policy here. If we didn't, we would need to lock this endpoint down,
       // as users could otherwise just go edit the policies of objects with
       // custom policies.
 
-      $new_policy = new PhabricatorPolicy();
+      if (!$errors) {
-      $new_policy->setRules($rule_data);
+        $new_policy = new PhabricatorPolicy();
-      $new_policy->setDefaultAction($request->getStr('default'));
+        $new_policy->setRules($valid_rules);
-      $new_policy->save();
+        $new_policy->setDefaultAction($request->getStr('default'));
+        $new_policy->save();
 
-      $data = array(
+        $data = array(
-        'phid' => $new_policy->getPHID(),
+          'phid' => $new_policy->getPHID(),
-        'info' => array(
+          'info' => array(
-          'name' => $new_policy->getName(),
+            'name' => $new_policy->getName(),
-          'full' => $new_policy->getName(),
+            'full' => $new_policy->getName(),
-          'icon' => $new_policy->getIcon(),
+            'icon' => $new_policy->getIcon(),
-        ),
+          ),
-      );
+        );
 
-      return id(new AphrontAjaxResponse())->setContent($data);
+        return id(new AphrontAjaxResponse())->setContent($data);
+      }
     }
 
     // Convert rule values to display format (for example, expanding PHIDs
@@ -120,7 +137,13 @@ final class PhabricatorPolicyEditController
         'name' => 'default',
       ));
 
+    if ($errors) {
+      $errors = id(new AphrontErrorView())
+        ->setErrors($errors);
+    }
+
     $form = id(new PHUIFormLayoutView())
+      ->appendChild($errors)
       ->appendChild(
         javelin_tag(
           'input',

src/applications/policy/rule/PhabricatorPolicyRule.php

@@ -34,4 +34,15 @@ abstract class PhabricatorPolicyRule {
     return $value;
   }
 
+  /**
+   * Return true if the given value creates a rule with a meaningful effect.
+   * An example of a rule with no meaningful effect is a "users" rule with no
+   * users specified.
+   *
+   * @return bool True if the value creates a meaningful rule.
+   */
+  public function ruleHasEffect($value) {
+    return true;
+  }
+
 }

src/applications/policy/rule/PhabricatorPolicyRuleProjects.php

@@ -64,4 +64,8 @@ final class PhabricatorPolicyRuleProjects
     return mpull($handles, 'getFullName', 'getPHID');
   }
 
+  public function ruleHasEffect($value) {
+    return (bool)$value;
+  }
+
 }

src/applications/policy/rule/PhabricatorPolicyRuleUsers.php

@@ -50,4 +50,8 @@ final class PhabricatorPolicyRuleUsers
     return mpull($handles, 'getFullName', 'getPHID');
   }
 
+  public function ruleHasEffect($value) {
+    return (bool)$value;
+  }
+
 }