From c8127edfe9a8d3f6ad86a634c7961667886f649d Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 4 Oct 2013 15:15:48 -0700 Subject: [PATCH] Tighten up some policy interactions in Herald Summary: Ref T603. Herald is a bit of a policy minefield right now, although I think pretty much everything has straightforward solutions. This change: - Introduces "create" and "create global" permisions for Herald. - Maybe "create" is sort of redundant since there's no reason to have access to the application if not creating rules, but I think this won't be the case for most applications, so having an explicit "create" permission is more consistent. - Add some application policy helper functions. - Improve rendering a bit -- I think we probably need to build some `PolicyType` class, similar to `PHIDType`, to really get this right. - Don't let users who can't use application X create Herald rules for application X. - Remove Maniphest/Pholio rules when those applications are not installed. Test Plan: - Restricted access to Maniphest and uninstalled Pholio. - Verified Pholio rules no longer appear for anyone. - Verified Maniphest ruls no longer appear for restricted users. - Verified users without CREATE_GLOBAL can not create global ruls. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T603 Differential Revision: https://secure.phabricator.com/D7219 --- .../base/controller/PhabricatorController.php | 19 +++++++++ .../herald/adapter/HeraldAdapter.php | 30 ++++++++------ .../herald/adapter/HeraldCommitAdapter.php | 5 +-- .../HeraldDifferentialRevisionAdapter.php | 5 +-- .../adapter/HeraldManiphestTaskAdapter.php | 4 ++ .../adapter/HeraldPholioMockAdapter.php | 4 ++ .../PhabricatorApplicationHerald.php | 17 ++++++++ .../herald/controller/HeraldController.php | 6 ++- .../herald/controller/HeraldNewController.php | 41 +++++++++++++++---- .../controller/HeraldRuleController.php | 12 +++++- .../controller/HeraldRuleListController.php | 2 +- .../controller/HeraldRuleViewController.php | 4 +- .../herald/query/HeraldRuleQuery.php | 10 +++++ .../herald/query/HeraldRuleSearchEngine.php | 6 ++- .../policy/filter/PhabricatorPolicyFilter.php | 9 +++- 15 files changed, 139 insertions(+), 35 deletions(-) diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php index 62c882d131..39d5881d23 100644 --- a/src/applications/base/controller/PhabricatorController.php +++ b/src/applications/base/controller/PhabricatorController.php @@ -350,4 +350,23 @@ abstract class PhabricatorController extends AphrontController { return $view; } + protected function hasApplicationCapability($capability) { + return PhabricatorPolicyFilter::hasCapability( + $this->getRequest()->getUser(), + $this->getCurrentApplication(), + $capability); + } + + protected function requireApplicationCapability($capability) { + PhabricatorPolicyFilter::requireCapability( + $this->getRequest()->getUser(), + $this->getCurrentApplication(), + $capability); + } + + protected function explainApplicationCapability($capability, $message) { + // TODO: Render a link to get more information. + return $message; + } + } diff --git a/src/applications/herald/adapter/HeraldAdapter.php b/src/applications/herald/adapter/HeraldAdapter.php index 76876ef728..4495691ff7 100644 --- a/src/applications/herald/adapter/HeraldAdapter.php +++ b/src/applications/herald/adapter/HeraldAdapter.php @@ -97,6 +97,17 @@ abstract class HeraldAdapter { return true; } + public function isAvailableToUser(PhabricatorUser $viewer) { + $applications = id(new PhabricatorApplicationQuery()) + ->setViewer($viewer) + ->withInstalled(true) + ->withClasses(array($this->getAdapterApplicationClass())) + ->execute(); + + return !empty($applications); + } + + /** * NOTE: You generally should not override this; it exists to support legacy * adapters which had hard-coded content types. @@ -106,6 +117,7 @@ abstract class HeraldAdapter { } abstract public function getAdapterContentName(); + abstract public function getAdapterApplicationClass(); /* -( Fields )------------------------------------------------------------- */ @@ -694,16 +706,6 @@ abstract class HeraldAdapter { return $adapters; } - public static function getAllEnabledAdapters() { - $adapters = self::getAllAdapters(); - foreach ($adapters as $key => $adapter) { - if (!$adapter->isEnabled()) { - unset($adapters[$key]); - } - } - return $adapters; - } - public static function getAdapterForContentType($content_type) { $adapters = self::getAllAdapters(); @@ -719,11 +721,14 @@ abstract class HeraldAdapter { $content_type)); } - public static function getEnabledAdapterMap() { + public static function getEnabledAdapterMap(PhabricatorUser $viewer) { $map = array(); - $adapters = HeraldAdapter::getAllEnabledAdapters(); + $adapters = HeraldAdapter::getAllAdapters(); foreach ($adapters as $adapter) { + if (!$adapter->isAvailableToUser($viewer)) { + continue; + } $type = $adapter->getAdapterContentType(); $name = $adapter->getAdapterContentName(); $map[$type] = $name; @@ -733,7 +738,6 @@ abstract class HeraldAdapter { return $map; } - public function renderRuleAsText(HeraldRule $rule, array $handles) { assert_instances_of($handles, 'PhabricatorObjectHandle'); diff --git a/src/applications/herald/adapter/HeraldCommitAdapter.php b/src/applications/herald/adapter/HeraldCommitAdapter.php index a3ec223d6c..36e5d87375 100644 --- a/src/applications/herald/adapter/HeraldCommitAdapter.php +++ b/src/applications/herald/adapter/HeraldCommitAdapter.php @@ -27,9 +27,8 @@ final class HeraldCommitAdapter extends HeraldAdapter { protected $affectedPackages; protected $auditNeededPackages; - public function isEnabled() { - $app = 'PhabricatorApplicationDiffusion'; - return PhabricatorApplication::isClassInstalled($app); + public function getAdapterApplicationClass() { + return 'PhabricatorApplicationDiffusion'; } public function getAdapterContentType() { diff --git a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php index 5559d8cdc5..7776330d1a 100644 --- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php +++ b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php @@ -20,9 +20,8 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter { protected $affectedPackages; protected $changesets; - public function isEnabled() { - $app = 'PhabricatorApplicationDifferential'; - return PhabricatorApplication::isClassInstalled($app); + public function getAdapterApplicationClass() { + return 'PhabricatorApplicationDifferential'; } public function getAdapterContentType() { diff --git a/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php b/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php index 5c461f8e89..68329e7818 100644 --- a/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php +++ b/src/applications/herald/adapter/HeraldManiphestTaskAdapter.php @@ -10,6 +10,10 @@ final class HeraldManiphestTaskAdapter extends HeraldAdapter { private $assignPHID; private $projectPHIDs = array(); + public function getAdapterApplicationClass() { + return 'PhabricatorApplicationManiphest'; + } + public function setTask(ManiphestTask $task) { $this->task = $task; return $this; diff --git a/src/applications/herald/adapter/HeraldPholioMockAdapter.php b/src/applications/herald/adapter/HeraldPholioMockAdapter.php index 8c5fb1c1de..bdf6e8a5f5 100644 --- a/src/applications/herald/adapter/HeraldPholioMockAdapter.php +++ b/src/applications/herald/adapter/HeraldPholioMockAdapter.php @@ -8,6 +8,10 @@ final class HeraldPholioMockAdapter extends HeraldAdapter { private $mock; private $ccPHIDs = array(); + public function getAdapterApplicationClass() { + return 'PhabricatorApplicationPholio'; + } + public function setMock(PholioMock $mock) { $this->mock = $mock; return $this; diff --git a/src/applications/herald/application/PhabricatorApplicationHerald.php b/src/applications/herald/application/PhabricatorApplicationHerald.php index ce172d72e3..692e37d8be 100644 --- a/src/applications/herald/application/PhabricatorApplicationHerald.php +++ b/src/applications/herald/application/PhabricatorApplicationHerald.php @@ -2,6 +2,9 @@ final class PhabricatorApplicationHerald extends PhabricatorApplication { + const CAN_CREATE_RULE = 'herald.create'; + const CAN_CREATE_GLOBAL_RULE = 'herald.global'; + public function getBaseURI() { return '/herald/'; } @@ -48,4 +51,18 @@ final class PhabricatorApplicationHerald extends PhabricatorApplication { ); } + protected function getCustomCapabilities() { + return array( + self::CAN_CREATE_RULE => array( + 'label' => pht('Can Create Rules'), + ), + self::CAN_CREATE_GLOBAL_RULE => array( + 'label' => pht('Can Create Global Rules'), + 'caption' => pht('Global rules can bypass access controls.'), + 'default' => PhabricatorPolicies::POLICY_ADMIN, + ), + ); + } + + } diff --git a/src/applications/herald/controller/HeraldController.php b/src/applications/herald/controller/HeraldController.php index a4f4eab1e0..4f1e0df62f 100644 --- a/src/applications/herald/controller/HeraldController.php +++ b/src/applications/herald/controller/HeraldController.php @@ -23,11 +23,15 @@ abstract class HeraldController extends PhabricatorController { public function buildApplicationCrumbs() { $crumbs = parent::buildApplicationCrumbs(); + $can_create = $this->hasApplicationCapability( + PhabricatorApplicationHerald::CAN_CREATE_RULE); + $crumbs->addAction( id(new PHUIListItemView()) ->setName(pht('Create Herald Rule')) ->setHref($this->getApplicationURI('new/')) - ->setIcon('create')); + ->setIcon('create') + ->setDisabled(!$can_create)); return $crumbs; } diff --git a/src/applications/herald/controller/HeraldNewController.php b/src/applications/herald/controller/HeraldNewController.php index 725ea7ae62..ab87985440 100644 --- a/src/applications/herald/controller/HeraldNewController.php +++ b/src/applications/herald/controller/HeraldNewController.php @@ -11,11 +11,16 @@ final class HeraldNewController extends HeraldController { } public function processRequest() { - $request = $this->getRequest(); $user = $request->getUser(); - $content_type_map = HeraldAdapter::getEnabledAdapterMap(); + $this->requireApplicationCapability( + PhabricatorApplicationHerald::CAN_CREATE_RULE); + + $can_global = $this->hasApplicationCapability( + PhabricatorApplicationHerald::CAN_CREATE_GLOBAL_RULE); + + $content_type_map = HeraldAdapter::getEnabledAdapterMap($user); if (empty($content_type_map[$this->contentType])) { $this->contentType = head_key($content_type_map); } @@ -32,14 +37,29 @@ final class HeraldNewController extends HeraldController { HeraldRuleTypeConfig::RULE_TYPE_PERSONAL, )) + $rule_type_map; + if (!$can_global) { + $global_link = $this->explainApplicationCapability( + PhabricatorApplicationHerald::CAN_CREATE_GLOBAL_RULE, + pht('You do not have permission to create or manage global rules.')); + } else { + $global_link = null; + } + $captions = array( HeraldRuleTypeConfig::RULE_TYPE_PERSONAL => - pht('Personal rules notify you about events. You own them, but '. - 'they can only affect you.'), + pht( + 'Personal rules notify you about events. You own them, but they can '. + 'only affect you.'), HeraldRuleTypeConfig::RULE_TYPE_GLOBAL => - pht('Global rules notify anyone about events. No one owns them, and '. - 'anyone can edit them. Usually, Global rules are used to notify '. - 'mailing lists.'), + phutil_implode_html( + phutil_tag('br'), + array_filter( + array( + pht( + 'Global rules notify anyone about events. Global rules can '. + 'bypass access control policies.'), + $global_link, + ))), ); $radio = id(new AphrontFormRadioButtonControl()) @@ -48,10 +68,15 @@ final class HeraldNewController extends HeraldController { ->setValue($this->ruleType); foreach ($rule_type_map as $value => $name) { + $disabled = ($value == HeraldRuleTypeConfig::RULE_TYPE_GLOBAL) && + (!$can_global); + $radio->addButton( $value, $name, - idx($captions, $value)); + idx($captions, $value), + $disabled ? 'disabled' : null, + $disabled); } $form = id(new AphrontFormView()) diff --git a/src/applications/herald/controller/HeraldRuleController.php b/src/applications/herald/controller/HeraldRuleController.php index c3868ebbd5..75458a249d 100644 --- a/src/applications/herald/controller/HeraldRuleController.php +++ b/src/applications/herald/controller/HeraldRuleController.php @@ -14,7 +14,7 @@ final class HeraldRuleController extends HeraldController { $request = $this->getRequest(); $user = $request->getUser(); - $content_type_map = HeraldAdapter::getEnabledAdapterMap(); + $content_type_map = HeraldAdapter::getEnabledAdapterMap($user); $rule_type_map = HeraldRuleTypeConfig::getRuleTypeMap(); if ($this->id) { @@ -42,11 +42,19 @@ final class HeraldRuleController extends HeraldController { $rule_type = $request->getStr('rule_type'); if (!isset($rule_type_map[$rule_type])) { - $rule_type = HeraldRuleTypeConfig::RULE_TYPE_GLOBAL; + $rule_type = HeraldRuleTypeConfig::RULE_TYPE_PERSONAL; } $rule->setRuleType($rule_type); $cancel_uri = $this->getApplicationURI(); + + $this->requireApplicationCapability( + PhabricatorApplicationHerald::CAN_CREATE_RULE); + } + + if ($rule->getRuleType() == HeraldRuleTypeConfig::RULE_TYPE_GLOBAL) { + $this->requireApplicationCapability( + PhabricatorApplicationHerald::CAN_CREATE_GLOBAL_RULE); } $adapter = HeraldAdapter::getAdapterForContentType($rule->getContentType()); diff --git a/src/applications/herald/controller/HeraldRuleListController.php b/src/applications/herald/controller/HeraldRuleListController.php index dacf0ec859..9a937c37e9 100644 --- a/src/applications/herald/controller/HeraldRuleListController.php +++ b/src/applications/herald/controller/HeraldRuleListController.php @@ -33,7 +33,7 @@ final class HeraldRuleListController extends HeraldController $phids = mpull($rules, 'getAuthorPHID'); $this->loadHandles($phids); - $content_type_map = HeraldAdapter::getEnabledAdapterMap(); + $content_type_map = HeraldAdapter::getEnabledAdapterMap($viewer); $list = id(new PHUIObjectItemListView()) ->setUser($viewer); diff --git a/src/applications/herald/controller/HeraldRuleViewController.php b/src/applications/herald/controller/HeraldRuleViewController.php index b7b6f582bc..13dbf9e75e 100644 --- a/src/applications/herald/controller/HeraldRuleViewController.php +++ b/src/applications/herald/controller/HeraldRuleViewController.php @@ -96,7 +96,9 @@ final class HeraldRuleViewController extends HeraldController { if ($adapter) { $view->addProperty( pht('Applies To'), - idx(HeraldAdapter::getEnabledAdapterMap(), $rule->getContentType())); + idx( + HeraldAdapter::getEnabledAdapterMap($viewer), + $rule->getContentType())); $view->invokeWillRenderEvent(); diff --git a/src/applications/herald/query/HeraldRuleQuery.php b/src/applications/herald/query/HeraldRuleQuery.php index 7b23231e6f..4e1ea93545 100644 --- a/src/applications/herald/query/HeraldRuleQuery.php +++ b/src/applications/herald/query/HeraldRuleQuery.php @@ -76,6 +76,16 @@ final class HeraldRuleQuery public function willFilterPage(array $rules) { $rule_ids = mpull($rules, 'getID'); + // Filter out any rules that have invalid adapters, or have adapters the + // viewer isn't permitted to see or use (for example, Differential rules + // if the user can't use Differential or Differential is not installed). + $types = HeraldAdapter::getEnabledAdapterMap($this->getViewer()); + foreach ($rules as $key => $rule) { + if (empty($types[$rule->getContentType()])) { + unset($rules[$key]); + } + } + if ($this->needValidateAuthors) { $this->validateRuleAuthors($rules); } diff --git a/src/applications/herald/query/HeraldRuleSearchEngine.php b/src/applications/herald/query/HeraldRuleSearchEngine.php index a875ca648f..0ad2416181 100644 --- a/src/applications/herald/query/HeraldRuleSearchEngine.php +++ b/src/applications/herald/query/HeraldRuleSearchEngine.php @@ -110,11 +110,13 @@ final class HeraldRuleSearchEngine private function getContentTypeOptions() { return array( '' => pht('(All Content Types)'), - ) + HeraldAdapter::getEnabledAdapterMap(); + ) + HeraldAdapter::getEnabledAdapterMap($this->requireViewer()); } private function getContentTypeValues() { - return array_fuse(array_keys(HeraldAdapter::getEnabledAdapterMap())); + return array_fuse( + array_keys( + HeraldAdapter::getEnabledAdapterMap($this->requireViewer()))); } private function getRuleTypeOptions() { diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index a20080a795..d831c4d462 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -281,6 +281,13 @@ final class PhabricatorPolicyFilter { case PhabricatorPolicyCapability::CAN_JOIN: $message = pht('You do not have permission to join this object.'); break; + default: + // TODO: Farm these out to applications? + $message = pht( + 'You do not have a required capability ("%s") to do whatever you '. + 'are trying to do.', + $capability); + break; } switch ($policy) { @@ -369,7 +376,7 @@ final class PhabricatorPolicyFilter { } $more = array_merge( - array($more), + array_filter(array($more)), array_filter((array)$object->describeAutomaticCapability($capability))); $exception = new PhabricatorPolicyException($message);