From 7f98a8575d0a587a46d177bcce8a87f8fc2ef401 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 13 Jun 2015 15:44:03 -0700 Subject: [PATCH] Allow different policy rules for different types of objects Summary: Ref T5681. Policy rules can now select objects they can apply to, so a rule like "task author" only shows up where it makes sense (when defining task policies). This will let us define rules like "members of thread" in Conpherence, "subscribers", etc., to make custom policies more flexible. Notes: - Per D13251, we need to do a little work to get the right options for policies like "Maniphest > Default View Policy". This should allow "task" policies. - This implements a "task author" policy as a simple example. - The `willApplyRule()` signature now accepts `$objects` to support bulk-loading things like subscribers. Test Plan: - Defined a task to be "visible to: task author", verified author could see it and other users could not. - `var_dump()`'d willApplyRule() inputs, verified they were correct (exactly the objects which use the rule). - Set `default view policy` to a task-specific policy. - Verified that other policies like "Can Use Bulk Editor" don't have these options. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T5681 Differential Revision: https://secure.phabricator.com/D13252 --- resources/celerity/map.php | 22 ++++---- src/__phutil_library_map__.php | 2 + .../base/PhabricatorApplication.php | 6 +++ .../ManiphestTaskAuthorPolicyRule.php | 31 +++++++++++ .../PhabricatorApplicationEditController.php | 12 +++-- .../PhabricatorPolicyApplication.php | 10 +++- .../PhabricatorPolicyEditController.php | 40 +++++++++++++- .../policy/filter/PhabricatorPolicyFilter.php | 53 ++++++++++++++----- .../PhabricatorAdministratorsPolicyRule.php | 5 +- ...PhabricatorLegalpadSignaturePolicyRule.php | 13 ++++- .../rule/PhabricatorLunarPhasePolicyRule.php | 6 ++- .../policy/rule/PhabricatorPolicyRule.php | 20 ++++++- .../rule/PhabricatorProjectsPolicyRule.php | 13 ++++- .../rule/PhabricatorUsersPolicyRule.php | 7 ++- .../form/control/AphrontFormPolicyControl.php | 19 +++++++ .../policy/behavior-policy-control.js | 2 +- 16 files changed, 220 insertions(+), 41 deletions(-) create mode 100644 src/applications/maniphest/policyrule/ManiphestTaskAuthorPolicyRule.php diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 48478682fe..1d78bcbc69 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -391,7 +391,7 @@ return array( 'rsrc/js/application/phortune/behavior-stripe-payment-form.js' => '3f5d6dbf', 'rsrc/js/application/phortune/behavior-test-payment-form.js' => 'fc91ab6c', 'rsrc/js/application/phortune/phortune-credit-card-form.js' => '2290aeef', - 'rsrc/js/application/policy/behavior-policy-control.js' => '9a340b3d', + 'rsrc/js/application/policy/behavior-policy-control.js' => '7d470398', 'rsrc/js/application/policy/behavior-policy-rule-editor.js' => '5e9f347c', 'rsrc/js/application/ponder/behavior-votebox.js' => '4e9b766b', 'rsrc/js/application/projects/behavior-project-boards.js' => 'ba4fa35c', @@ -624,7 +624,7 @@ return array( 'javelin-behavior-pholio-mock-view' => 'fbe497e7', 'javelin-behavior-phui-dropdown-menu' => '54733475', 'javelin-behavior-phui-object-box-tabs' => '2bfa2836', - 'javelin-behavior-policy-control' => '9a340b3d', + 'javelin-behavior-policy-control' => '7d470398', 'javelin-behavior-policy-rule-editor' => '5e9f347c', 'javelin-behavior-ponder-votebox' => '4e9b766b', 'javelin-behavior-project-boards' => 'ba4fa35c', @@ -1413,6 +1413,15 @@ return array( 'javelin-request', 'javelin-router', ), + '7d470398' => array( + 'javelin-behavior', + 'javelin-dom', + 'javelin-util', + 'phuix-dropdown-menu', + 'phuix-action-list-view', + 'phuix-action-view', + 'javelin-workflow', + ), '7e41274a' => array( 'javelin-install', ), @@ -1575,15 +1584,6 @@ return array( 'javelin-behavior-device', 'phabricator-title', ), - '9a340b3d' => array( - 'javelin-behavior', - 'javelin-dom', - 'javelin-util', - 'phuix-dropdown-menu', - 'phuix-action-list-view', - 'phuix-action-view', - 'javelin-workflow', - ), '9f36c42d' => array( 'javelin-behavior', 'javelin-stratcom', diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 80cf17dcbc..70eb4103fc 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1051,6 +1051,7 @@ phutil_register_library_map(array( 'ManiphestStatusEmailCommand' => 'applications/maniphest/command/ManiphestStatusEmailCommand.php', 'ManiphestSubpriorityController' => 'applications/maniphest/controller/ManiphestSubpriorityController.php', 'ManiphestTask' => 'applications/maniphest/storage/ManiphestTask.php', + 'ManiphestTaskAuthorPolicyRule' => 'applications/maniphest/policyrule/ManiphestTaskAuthorPolicyRule.php', 'ManiphestTaskClosedStatusDatasource' => 'applications/maniphest/typeahead/ManiphestTaskClosedStatusDatasource.php', 'ManiphestTaskDependedOnByTaskEdgeType' => 'applications/maniphest/edge/ManiphestTaskDependedOnByTaskEdgeType.php', 'ManiphestTaskDependsOnTaskEdgeType' => 'applications/maniphest/edge/ManiphestTaskDependsOnTaskEdgeType.php', @@ -4412,6 +4413,7 @@ phutil_register_library_map(array( 'PhabricatorProjectInterface', 'PhabricatorSpacesInterface', ), + 'ManiphestTaskAuthorPolicyRule' => 'PhabricatorPolicyRule', 'ManiphestTaskClosedStatusDatasource' => 'PhabricatorTypeaheadDatasource', 'ManiphestTaskDependedOnByTaskEdgeType' => 'PhabricatorEdgeType', 'ManiphestTaskDependsOnTaskEdgeType' => 'PhabricatorEdgeType', diff --git a/src/applications/base/PhabricatorApplication.php b/src/applications/base/PhabricatorApplication.php index a021b5f1c6..0e49bad23d 100644 --- a/src/applications/base/PhabricatorApplication.php +++ b/src/applications/base/PhabricatorApplication.php @@ -583,6 +583,12 @@ abstract class PhabricatorApplication implements PhabricatorPolicyInterface { } public function getCapabilityTemplatePHIDType($capability) { + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + case PhabricatorPolicyCapability::CAN_EDIT: + return null; + } + $spec = $this->getCustomCapabilitySpecification($capability); return idx($spec, 'template'); } diff --git a/src/applications/maniphest/policyrule/ManiphestTaskAuthorPolicyRule.php b/src/applications/maniphest/policyrule/ManiphestTaskAuthorPolicyRule.php new file mode 100644 index 0000000000..18502545c6 --- /dev/null +++ b/src/applications/maniphest/policyrule/ManiphestTaskAuthorPolicyRule.php @@ -0,0 +1,31 @@ +getPHID(); + if (!$viewer_phid) { + return false; + } + + return ($object->getAuthorPHID() == $viewer_phid); + } + + public function getValueControlType() { + return self::CONTROL_TYPE_NONE; + } + +} diff --git a/src/applications/meta/controller/PhabricatorApplicationEditController.php b/src/applications/meta/controller/PhabricatorApplicationEditController.php index dffb46c6a1..104fdb5b01 100644 --- a/src/applications/meta/controller/PhabricatorApplicationEditController.php +++ b/src/applications/meta/controller/PhabricatorApplicationEditController.php @@ -124,8 +124,7 @@ final class PhabricatorApplicationEditController ->setValue(idx($descriptions, $capability)) ->setCaption($caption)); } else { - $form->appendChild( - id(new AphrontFormPolicyControl()) + $control = id(new AphrontFormPolicyControl()) ->setUser($user) ->setDisabled($locked) ->setCapability($capability) @@ -133,7 +132,14 @@ final class PhabricatorApplicationEditController ->setPolicies($policies) ->setLabel($label) ->setName('policy:'.$capability) - ->setCaption($caption)); + ->setCaption($caption); + + $template = $application->getCapabilityTemplatePHIDType($capability); + if ($template) { + $control->setTemplatePHIDType($template); + } + + $form->appendControl($control); } } diff --git a/src/applications/policy/application/PhabricatorPolicyApplication.php b/src/applications/policy/application/PhabricatorPolicyApplication.php index cefccd2cb5..8aaf8eb1bf 100644 --- a/src/applications/policy/application/PhabricatorPolicyApplication.php +++ b/src/applications/policy/application/PhabricatorPolicyApplication.php @@ -19,7 +19,15 @@ final class PhabricatorPolicyApplication extends PhabricatorApplication { '/policy/' => array( 'explain/(?P[^/]+)/(?P[^/]+)/' => 'PhabricatorPolicyExplainController', - 'edit/(?:(?P[^/]+)/)?' => 'PhabricatorPolicyEditController', + 'edit/'. + '(?:'. + 'object/(?P[^/]+)'. + '|'. + 'type/(?P[^/]+)'. + '|'. + 'template/(?P[^/]+)'. + ')/'. + '(?:(?P[^/]+)/)?' => 'PhabricatorPolicyEditController', ), ); } diff --git a/src/applications/policy/controller/PhabricatorPolicyEditController.php b/src/applications/policy/controller/PhabricatorPolicyEditController.php index 80b04e4ea7..30c9836557 100644 --- a/src/applications/policy/controller/PhabricatorPolicyEditController.php +++ b/src/applications/policy/controller/PhabricatorPolicyEditController.php @@ -4,8 +4,37 @@ final class PhabricatorPolicyEditController extends PhabricatorPolicyController { public function handleRequest(AphrontRequest $request) { - $request = $this->getRequest(); - $viewer = $request->getUser(); + $viewer = $this->getViewer(); + + // TODO: This doesn't do anything yet, but sets up template policies; see + // T6860. + $is_template = false; + + $object_phid = $request->getURIData('objectPHID'); + if ($object_phid) { + $object = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withPHIDs(array($object_phid)) + ->executeOne(); + if (!$object) { + return new Aphront404Response(); + } + } else { + $object_type = $request->getURIData('objectType'); + if (!$object_type) { + $object_type = $request->getURIData('templateType'); + $is_template = true; + } + + $phid_types = PhabricatorPHIDType::getAllInstalledTypes($viewer); + if (empty($phid_types[$object_type])) { + return new Aphront404Response(); + } + $object = $phid_types[$object_type]->newObject(); + if (!$object) { + return new Aphront404Response(); + } + } $action_options = array( PhabricatorPolicy::ACTION_ALLOW => pht('Allow'), @@ -15,6 +44,13 @@ final class PhabricatorPolicyEditController $rules = id(new PhutilSymbolLoader()) ->setAncestorClass('PhabricatorPolicyRule') ->loadObjects(); + + foreach ($rules as $key => $rule) { + if (!$rule->canApplyToObject($object)) { + unset($rules[$key]); + } + } + $rules = msort($rules, 'getRuleOrder'); $default_rule = array( diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index 46cb9352f7..36c73006a0 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -149,13 +149,13 @@ final class PhabricatorPolicyFilter { } if ($type == PhabricatorPolicyPHIDTypePolicy::TYPECONST) { - $need_policies[$policy] = $policy; + $need_policies[$policy][] = $object; } } } if ($need_policies) { - $this->loadCustomPolicies(array_keys($need_policies)); + $this->loadCustomPolicies($need_policies); } // If we need projects, check if any of the projects we need are also the @@ -500,7 +500,7 @@ final class PhabricatorPolicyFilter { $this->rejectObject($object, $policy, $capability); } } else if ($type == PhabricatorPolicyPHIDTypePolicy::TYPECONST) { - if ($this->checkCustomPolicy($policy)) { + if ($this->checkCustomPolicy($policy, $object)) { return true; } else { $this->rejectObject($object, $policy, $capability); @@ -573,30 +573,47 @@ final class PhabricatorPolicyFilter { throw $exception; } - private function loadCustomPolicies(array $phids) { + private function loadCustomPolicies(array $map) { $viewer = $this->viewer; $viewer_phid = $viewer->getPHID(); $custom_policies = id(new PhabricatorPolicyQuery()) ->setViewer($viewer) - ->withPHIDs($phids) + ->withPHIDs(array_keys($map)) ->execute(); $custom_policies = mpull($custom_policies, null, 'getPHID'); - $classes = array(); $values = array(); - foreach ($custom_policies as $policy) { + $objects = array(); + foreach ($custom_policies as $policy_phid => $policy) { foreach ($policy->getCustomRuleClasses() as $class) { $classes[$class] = $class; $values[$class][] = $policy->getCustomRuleValues($class); + + foreach (idx($map, $policy_phid, array()) as $object) { + $objects[$class][] = $object; + } } } foreach ($classes as $class => $ignored) { - $object = newv($class, array()); - $object->willApplyRules($viewer, array_mergev($values[$class])); - $classes[$class] = $object; + $rule_object = newv($class, array()); + + // Filter out any objects which the rule can't apply to. + $target_objects = idx($objects, $class, array()); + foreach ($target_objects as $key => $target_object) { + if (!$rule_object->canApplyToObject($target_object)) { + unset($target_objects[$key]); + } + } + + $rule_object->willApplyRules( + $viewer, + array_mergev($values[$class]), + $target_objects); + + $classes[$class] = $rule_object; } foreach ($custom_policies as $policy) { @@ -610,7 +627,10 @@ final class PhabricatorPolicyFilter { $this->customPolicies[$viewer->getPHID()] += $custom_policies; } - private function checkCustomPolicy($policy_phid) { + private function checkCustomPolicy( + $policy_phid, + PhabricatorPolicyInterface $object) { + $viewer = $this->viewer; $viewer_phid = $viewer->getPHID(); @@ -623,14 +643,19 @@ final class PhabricatorPolicyFilter { $objects = $policy->getRuleObjects(); $action = null; foreach ($policy->getRules() as $rule) { - $object = idx($objects, idx($rule, 'rule')); - if (!$object) { + $rule_object = idx($objects, idx($rule, 'rule')); + if (!$rule_object) { // Reject, this policy has a bogus rule. return false; } + if (!$rule_object->canApplyToObject($object)) { + // Reject, this policy rule can't be applied to the given object. + return false; + } + // If the user matches this rule, use this action. - if ($object->applyRule($viewer, idx($rule, 'value'))) { + if ($rule_object->applyRule($viewer, idx($rule, 'value'), $object)) { $action = idx($rule, 'action'); break; } diff --git a/src/applications/policy/rule/PhabricatorAdministratorsPolicyRule.php b/src/applications/policy/rule/PhabricatorAdministratorsPolicyRule.php index f232cb3008..b6e450adde 100644 --- a/src/applications/policy/rule/PhabricatorAdministratorsPolicyRule.php +++ b/src/applications/policy/rule/PhabricatorAdministratorsPolicyRule.php @@ -6,7 +6,10 @@ final class PhabricatorAdministratorsPolicyRule extends PhabricatorPolicyRule { return pht('administrators'); } - public function applyRule(PhabricatorUser $viewer, $value) { + public function applyRule( + PhabricatorUser $viewer, + $value, + PhabricatorPolicyInterface $object) { return $viewer->getIsAdmin(); } diff --git a/src/applications/policy/rule/PhabricatorLegalpadSignaturePolicyRule.php b/src/applications/policy/rule/PhabricatorLegalpadSignaturePolicyRule.php index 9797d5ef80..3aa7ef8d9c 100644 --- a/src/applications/policy/rule/PhabricatorLegalpadSignaturePolicyRule.php +++ b/src/applications/policy/rule/PhabricatorLegalpadSignaturePolicyRule.php @@ -9,7 +9,11 @@ final class PhabricatorLegalpadSignaturePolicyRule return pht('signers of legalpad documents'); } - public function willApplyRules(PhabricatorUser $viewer, array $values) { + public function willApplyRules( + PhabricatorUser $viewer, + array $values, + array $objects) { + $values = array_unique(array_filter(array_mergev($values))); if (!$values) { return; @@ -26,12 +30,17 @@ final class PhabricatorLegalpadSignaturePolicyRule $this->signatures = mpull($documents, 'getPHID', 'getPHID'); } - public function applyRule(PhabricatorUser $viewer, $value) { + public function applyRule( + PhabricatorUser $viewer, + $value, + PhabricatorPolicyInterface $object) { + foreach ($value as $document_phid) { if (!isset($this->signatures[$document_phid])) { return false; } } + return true; } diff --git a/src/applications/policy/rule/PhabricatorLunarPhasePolicyRule.php b/src/applications/policy/rule/PhabricatorLunarPhasePolicyRule.php index 24f8f26138..a3336a7971 100644 --- a/src/applications/policy/rule/PhabricatorLunarPhasePolicyRule.php +++ b/src/applications/policy/rule/PhabricatorLunarPhasePolicyRule.php @@ -11,7 +11,11 @@ final class PhabricatorLunarPhasePolicyRule extends PhabricatorPolicyRule { return pht('when the moon'); } - public function applyRule(PhabricatorUser $viewer, $value) { + public function applyRule( + PhabricatorUser $viewer, + $value, + PhabricatorPolicyInterface $object) { + $moon = new PhutilLunarPhase(PhabricatorTime::getNow()); switch ($value) { diff --git a/src/applications/policy/rule/PhabricatorPolicyRule.php b/src/applications/policy/rule/PhabricatorPolicyRule.php index eaa04e1e9e..e0ac96ec2b 100644 --- a/src/applications/policy/rule/PhabricatorPolicyRule.php +++ b/src/applications/policy/rule/PhabricatorPolicyRule.php @@ -8,9 +8,15 @@ abstract class PhabricatorPolicyRule { const CONTROL_TYPE_NONE = 'none'; abstract public function getRuleDescription(); - abstract public function applyRule(PhabricatorUser $viewer, $value); + abstract public function applyRule( + PhabricatorUser $viewer, + $value, + PhabricatorPolicyInterface $object); - public function willApplyRules(PhabricatorUser $viewer, array $values) { + public function willApplyRules( + PhabricatorUser $viewer, + array $values, + array $objects) { return; } @@ -22,6 +28,16 @@ abstract class PhabricatorPolicyRule { return null; } + /** + * Return `true` if this rule can be applied to the given object. + * + * Some policy rules may only operation on certain kinds of objects. For + * example, a "task author" rule + */ + public function canApplyToObject(PhabricatorPolicyInterface $object) { + return true; + } + protected function getDatasourceTemplate( PhabricatorTypeaheadDatasource $datasource) { return array( diff --git a/src/applications/policy/rule/PhabricatorProjectsPolicyRule.php b/src/applications/policy/rule/PhabricatorProjectsPolicyRule.php index eca97ce37d..c3fbfd4f7e 100644 --- a/src/applications/policy/rule/PhabricatorProjectsPolicyRule.php +++ b/src/applications/policy/rule/PhabricatorProjectsPolicyRule.php @@ -8,7 +8,11 @@ final class PhabricatorProjectsPolicyRule extends PhabricatorPolicyRule { return pht('members of projects'); } - public function willApplyRules(PhabricatorUser $viewer, array $values) { + public function willApplyRules( + PhabricatorUser $viewer, + array $values, + array $objects) { + $values = array_unique(array_filter(array_mergev($values))); if (!$values) { return; @@ -24,12 +28,17 @@ final class PhabricatorProjectsPolicyRule extends PhabricatorPolicyRule { } } - public function applyRule(PhabricatorUser $viewer, $value) { + public function applyRule( + PhabricatorUser $viewer, + $value, + PhabricatorPolicyInterface $object) { + foreach ($value as $project_phid) { if (isset($this->memberships[$viewer->getPHID()][$project_phid])) { return true; } } + return false; } diff --git a/src/applications/policy/rule/PhabricatorUsersPolicyRule.php b/src/applications/policy/rule/PhabricatorUsersPolicyRule.php index c60e43abb6..aa75027cb1 100644 --- a/src/applications/policy/rule/PhabricatorUsersPolicyRule.php +++ b/src/applications/policy/rule/PhabricatorUsersPolicyRule.php @@ -6,12 +6,17 @@ final class PhabricatorUsersPolicyRule extends PhabricatorPolicyRule { return pht('users'); } - public function applyRule(PhabricatorUser $viewer, $value) { + public function applyRule( + PhabricatorUser $viewer, + $value, + PhabricatorPolicyInterface $object) { + foreach ($value as $phid) { if ($phid == $viewer->getPHID()) { return true; } } + return false; } diff --git a/src/view/form/control/AphrontFormPolicyControl.php b/src/view/form/control/AphrontFormPolicyControl.php index 46dde71826..40caba33d7 100644 --- a/src/view/form/control/AphrontFormPolicyControl.php +++ b/src/view/form/control/AphrontFormPolicyControl.php @@ -6,6 +6,7 @@ final class AphrontFormPolicyControl extends AphrontFormControl { private $capability; private $policies; private $spacePHID; + private $templatePHIDType; public function setPolicyObject(PhabricatorPolicyInterface $object) { $this->object = $object; @@ -27,6 +28,11 @@ final class AphrontFormPolicyControl extends AphrontFormControl { return $this->spacePHID; } + public function setTemplatePHIDType($type) { + $this->templatePHIDType = $type; + return $this; + } + public function setCapability($capability) { $this->capability = $capability; @@ -178,6 +184,18 @@ final class AphrontFormPolicyControl extends AphrontFormControl { } + if ($this->templatePHIDType) { + $context_path = 'template/'.$this->templatePHIDType.'/'; + } else { + $object_phid = $this->object->getPHID(); + if ($object_phid) { + $context_path = 'object/'.$object_phid.'/'; + } else { + $object_type = phid_get_type($this->object->generatePHID()); + $context_path = 'type/'.$object_type.'/'; + } + } + Javelin::initBehavior( 'policy-control', array( @@ -190,6 +208,7 @@ final class AphrontFormPolicyControl extends AphrontFormControl { 'labels' => $labels, 'value' => $this->getValue(), 'capability' => $this->capability, + 'editURI' => '/policy/edit/'.$context_path, 'customPlaceholder' => $this->getCustomPolicyPlaceholder(), )); diff --git a/webroot/rsrc/js/application/policy/behavior-policy-control.js b/webroot/rsrc/js/application/policy/behavior-policy-control.js index ddbe9f17ec..ea66761191 100644 --- a/webroot/rsrc/js/application/policy/behavior-policy-control.js +++ b/webroot/rsrc/js/application/policy/behavior-policy-control.js @@ -101,7 +101,7 @@ JX.behavior('policy-control', function(config) { * Get the workflow URI to create or edit a policy with a given PHID. */ var get_custom_uri = function(phid, capability) { - var uri = '/policy/edit/'; + var uri = config.editURI; if (phid != config.customPlaceholder) { uri += phid + '/'; }