diff --git a/src/applications/policy/controller/PhabricatorPolicyEditController.php b/src/applications/policy/controller/PhabricatorPolicyEditController.php index a43d2baa7e..e48057305d 100644 --- a/src/applications/policy/controller/PhabricatorPolicyEditController.php +++ b/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(); - $new_policy->setRules($rule_data); - $new_policy->setDefaultAction($request->getStr('default')); - $new_policy->save(); + if (!$errors) { + $new_policy = new PhabricatorPolicy(); + $new_policy->setRules($valid_rules); + $new_policy->setDefaultAction($request->getStr('default')); + $new_policy->save(); - $data = array( - 'phid' => $new_policy->getPHID(), - 'info' => array( - 'name' => $new_policy->getName(), - 'full' => $new_policy->getName(), - 'icon' => $new_policy->getIcon(), - ), - ); + $data = array( + 'phid' => $new_policy->getPHID(), + 'info' => array( + 'name' => $new_policy->getName(), + 'full' => $new_policy->getName(), + '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', diff --git a/src/applications/policy/rule/PhabricatorPolicyRule.php b/src/applications/policy/rule/PhabricatorPolicyRule.php index 39fc614af2..ea663499c8 100644 --- a/src/applications/policy/rule/PhabricatorPolicyRule.php +++ b/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; + } + } diff --git a/src/applications/policy/rule/PhabricatorPolicyRuleProjects.php b/src/applications/policy/rule/PhabricatorPolicyRuleProjects.php index 11a08cf878..1940d90ccb 100644 --- a/src/applications/policy/rule/PhabricatorPolicyRuleProjects.php +++ b/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; + } + } diff --git a/src/applications/policy/rule/PhabricatorPolicyRuleUsers.php b/src/applications/policy/rule/PhabricatorPolicyRuleUsers.php index 4e43a50f37..d405fb115a 100644 --- a/src/applications/policy/rule/PhabricatorPolicyRuleUsers.php +++ b/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; + } + }