From f4582dc49d8d0e4f99765a0dcee695fa2bbdaa84 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 9 Oct 2013 15:06:18 -0700 Subject: [PATCH] Allow "Default View" policies to be set to Public Summary: Ref T603. Currently, we hard-code defense against setting policies to "Public" in several places, and special case only the CAN_VIEW policy. In fact, other policies (like Default View) should also be able to be set to public. Instead of hard-coding this, move it to the capability definitions. Test Plan: Set default view policy in Maniphest to "Public", created a task, verified default policy. Reviewers: btrahan, asherkin Reviewed By: asherkin CC: asherkin, aran Maniphest Tasks: T603 Differential Revision: https://secure.phabricator.com/D7276 --- .../capability/DifferentialCapabilityDefaultView.php | 4 ++++ .../capability/ManiphestCapabilityDefaultView.php | 4 ++++ .../PhabricatorApplicationEditController.php | 6 +++--- .../capability/PhabricatorPolicyCapability.php | 9 +++++++++ .../PhabricatorPolicyCapabilityCanView.php | 4 ++++ .../policy/filter/PhabricatorPolicyFilter.php | 7 ++++--- src/view/form/control/AphrontFormPolicyControl.php | 12 ++++++++---- 7 files changed, 36 insertions(+), 10 deletions(-) diff --git a/src/applications/differential/capability/DifferentialCapabilityDefaultView.php b/src/applications/differential/capability/DifferentialCapabilityDefaultView.php index c33fdc7a03..a0dbd99e5f 100644 --- a/src/applications/differential/capability/DifferentialCapabilityDefaultView.php +++ b/src/applications/differential/capability/DifferentialCapabilityDefaultView.php @@ -13,4 +13,8 @@ final class DifferentialCapabilityDefaultView return pht('Default View Policy'); } + public function shouldAllowPublicPolicySetting() { + return true; + } + } diff --git a/src/applications/maniphest/capability/ManiphestCapabilityDefaultView.php b/src/applications/maniphest/capability/ManiphestCapabilityDefaultView.php index 73638007a1..cd181561dd 100644 --- a/src/applications/maniphest/capability/ManiphestCapabilityDefaultView.php +++ b/src/applications/maniphest/capability/ManiphestCapabilityDefaultView.php @@ -13,4 +13,8 @@ final class ManiphestCapabilityDefaultView return pht('Default View Policy'); } + public function shouldAllowPublicPolicySetting() { + return true; + } + } diff --git a/src/applications/meta/controller/PhabricatorApplicationEditController.php b/src/applications/meta/controller/PhabricatorApplicationEditController.php index d401f4f8c5..635d6d299b 100644 --- a/src/applications/meta/controller/PhabricatorApplicationEditController.php +++ b/src/applications/meta/controller/PhabricatorApplicationEditController.php @@ -51,9 +51,9 @@ final class PhabricatorApplicationEditController continue; } - if ($new == PhabricatorPolicies::POLICY_PUBLIC && - $capability != PhabricatorPolicyCapability::CAN_VIEW) { - // Can't set policies other than "view" to public. + $capobj = PhabricatorPolicyCapability::getCapabilityByKey($capability); + if (!$capobj || !$capobj->shouldAllowPublicPolicySetting()) { + // Can't set non-public policies to public. continue; } diff --git a/src/applications/policy/capability/PhabricatorPolicyCapability.php b/src/applications/policy/capability/PhabricatorPolicyCapability.php index ea925f2541..a34e0f76bd 100644 --- a/src/applications/policy/capability/PhabricatorPolicyCapability.php +++ b/src/applications/policy/capability/PhabricatorPolicyCapability.php @@ -40,6 +40,15 @@ abstract class PhabricatorPolicyCapability extends Phobject { return null; } + /** + * Can this capability be set to "public"? Broadly, this is only appropriate + * for view and view-related policies. + * + * @return bool True to allow the "public" policy. Returns false by default. + */ + public function shouldAllowPublicPolicySetting() { + return false; + } final public static function getCapabilityByKey($key) { return idx(self::getCapabilityMap(), $key); diff --git a/src/applications/policy/capability/PhabricatorPolicyCapabilityCanView.php b/src/applications/policy/capability/PhabricatorPolicyCapabilityCanView.php index b9d2df47b9..f56b1d7407 100644 --- a/src/applications/policy/capability/PhabricatorPolicyCapabilityCanView.php +++ b/src/applications/policy/capability/PhabricatorPolicyCapabilityCanView.php @@ -15,4 +15,8 @@ final class PhabricatorPolicyCapabilityCanView return pht('You do not have permission to view this object.'); } + public function shouldAllowPublicPolicySetting() { + return true; + } + } diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index 8bc32e22a7..2ed1dec241 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -173,9 +173,10 @@ final class PhabricatorPolicyFilter { $policy = PhabricatorPolicies::POLICY_USER; } - // If the object is set to "public" but the capability is anything other - // than "view", restrict the policy to "user". - if ($capability != PhabricatorPolicyCapability::CAN_VIEW) { + // If the object is set to "public" but the capability is not a public + // capability, restrict the policy to "user". + $capobj = PhabricatorPolicyCapability::getCapabilityByKey($capability); + if (!$capobj || !$capobj->shouldAllowPublicPolicySetting()) { $policy = PhabricatorPolicies::POLICY_USER; } } diff --git a/src/view/form/control/AphrontFormPolicyControl.php b/src/view/form/control/AphrontFormPolicyControl.php index 3db5be8d18..c145a9707b 100644 --- a/src/view/form/control/AphrontFormPolicyControl.php +++ b/src/view/form/control/AphrontFormPolicyControl.php @@ -36,12 +36,16 @@ final class AphrontFormPolicyControl extends AphrontFormControl { } protected function getOptions() { + $capability = $this->capability; + $options = array(); foreach ($this->policies as $policy) { - if (($policy->getPHID() == PhabricatorPolicies::POLICY_PUBLIC) && - ($this->capability != PhabricatorPolicyCapability::CAN_VIEW)) { - // Never expose "Public" for anything except "Can View". - continue; + if ($policy->getPHID() == PhabricatorPolicies::POLICY_PUBLIC) { + // Never expose "Public" for capabilities which don't support it. + $capobj = PhabricatorPolicyCapability::getCapabilityByKey($capability); + if (!$capobj || !$capobj->shouldAllowPublicPolicySetting()) { + continue; + } } $type_name = PhabricatorPolicyType::getPolicyTypeName($policy->getType());