From b1b1ff83f239084d952700aa450cd22315e686bf Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 7 Oct 2013 13:28:58 -0700 Subject: [PATCH] Allow applications to define new policy capabilities Summary: Ref T603. I want to let applications define new capabilities (like "can manage global rules" in Herald) and get full support for them, including reasonable error strings in the UI. Currently, this is difficult for a couple of reasons. Partly this is just a code organization issue, which is easy to fix. The bigger thing is that we have a bunch of strings which depend on both the policy and capability, like: "You must be an administrator to view this object." "Administrator" is the policy, and "view" is the capability. That means every new capability has to add a string for each policy, and every new policy (should we introduce any) needs to add a string for each capability. And we can't do any piecemeal "You must be a {$role} to {$action} this object" becuase it's impossible to translate. Instead, make all the strings depend on //only// the policy, //only// the capability, or //only// the object type. This makes the dialogs read a little more strangely, but I think it's still pretty easy to understand, and it makes adding new stuff way way easier. Also provide more context, and more useful exception messages. Test Plan: - See screenshots. - Also triggered a policy exception and verified it was dramatically more useful than it used to be. Reviewers: btrahan, chad Reviewed By: btrahan CC: chad, aran Maniphest Tasks: T603 Differential Revision: https://secure.phabricator.com/D7260 --- src/__celerity_resource_map__.php | 92 +++++----- src/__phutil_library_map__.php | 10 +- ...AphrontDefaultApplicationConfiguration.php | 29 ++-- .../storage/DifferentialRevision.php | 8 +- .../PhabricatorPolicyCapability.php | 61 +++++++ .../PhabricatorPolicyCapabilityCanEdit.php | 18 ++ .../PhabricatorPolicyCapabilityCanJoin.php | 18 ++ .../PhabricatorPolicyCapabilityCanView.php | 18 ++ .../constants/PhabricatorPolicyCapability.php | 9 - .../PhabricatorPolicyExplainController.php | 19 +- .../exception/PhabricatorPolicyException.php | 30 ++++ .../policy/filter/PhabricatorPolicy.php | 76 ++++---- .../policy/filter/PhabricatorPolicyFilter.php | 162 ++++++------------ ...habricatorPolicyManagementShowWorkflow.php | 3 +- webroot/rsrc/css/aphront/dialog-view.css | 8 + 15 files changed, 332 insertions(+), 229 deletions(-) create mode 100644 src/applications/policy/capability/PhabricatorPolicyCapability.php create mode 100644 src/applications/policy/capability/PhabricatorPolicyCapabilityCanEdit.php create mode 100644 src/applications/policy/capability/PhabricatorPolicyCapabilityCanJoin.php create mode 100644 src/applications/policy/capability/PhabricatorPolicyCapabilityCanView.php delete mode 100644 src/applications/policy/constants/PhabricatorPolicyCapability.php diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index aef75161c2..c5a1f52168 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -836,7 +836,7 @@ celerity_register_resource_map(array( ), 'aphront-dialog-view-css' => array( - 'uri' => '/res/609ccc78/rsrc/css/aphront/dialog-view.css', + 'uri' => '/res/830fa2de/rsrc/css/aphront/dialog-view.css', 'type' => 'css', 'requires' => array( @@ -4184,7 +4184,7 @@ celerity_register_resource_map(array( ), array( 'packages' => array( - 'cd37aa53' => + 'c98eaabf' => array( 'name' => 'core.pkg.css', 'symbols' => @@ -4233,7 +4233,7 @@ celerity_register_resource_map(array( 41 => 'phabricator-tag-view-css', 42 => 'phui-list-view-css', ), - 'uri' => '/res/pkg/cd37aa53/core.pkg.css', + 'uri' => '/res/pkg/c98eaabf/core.pkg.css', 'type' => 'css', ), '64eeda79' => @@ -4425,15 +4425,15 @@ celerity_register_resource_map(array( ), 'reverse' => array( - 'aphront-dialog-view-css' => 'cd37aa53', - 'aphront-error-view-css' => 'cd37aa53', - 'aphront-list-filter-view-css' => 'cd37aa53', - 'aphront-pager-view-css' => 'cd37aa53', - 'aphront-panel-view-css' => 'cd37aa53', - 'aphront-table-view-css' => 'cd37aa53', - 'aphront-tokenizer-control-css' => 'cd37aa53', - 'aphront-tooltip-css' => 'cd37aa53', - 'aphront-typeahead-control-css' => 'cd37aa53', + 'aphront-dialog-view-css' => 'c98eaabf', + 'aphront-error-view-css' => 'c98eaabf', + 'aphront-list-filter-view-css' => 'c98eaabf', + 'aphront-pager-view-css' => 'c98eaabf', + 'aphront-panel-view-css' => 'c98eaabf', + 'aphront-table-view-css' => 'c98eaabf', + 'aphront-tokenizer-control-css' => 'c98eaabf', + 'aphront-tooltip-css' => 'c98eaabf', + 'aphront-typeahead-control-css' => 'c98eaabf', 'differential-changeset-view-css' => '4dc2311c', 'differential-core-view-css' => '4dc2311c', 'differential-inline-comment-editor' => '5e9e5c4e', @@ -4447,7 +4447,7 @@ celerity_register_resource_map(array( 'differential-table-of-contents-css' => '4dc2311c', 'diffusion-commit-view-css' => 'c8ce2d88', 'diffusion-icons-css' => 'c8ce2d88', - 'global-drag-and-drop-css' => 'cd37aa53', + 'global-drag-and-drop-css' => 'c98eaabf', 'inline-comment-summary-css' => '4dc2311c', 'javelin-aphlict' => '64eeda79', 'javelin-behavior' => '9564fa17', @@ -4522,56 +4522,56 @@ celerity_register_resource_map(array( 'javelin-util' => '9564fa17', 'javelin-vector' => '9564fa17', 'javelin-workflow' => '9564fa17', - 'lightbox-attachment-css' => 'cd37aa53', + 'lightbox-attachment-css' => 'c98eaabf', 'maniphest-task-summary-css' => '49898640', - 'phabricator-action-list-view-css' => 'cd37aa53', - 'phabricator-application-launch-view-css' => 'cd37aa53', + 'phabricator-action-list-view-css' => 'c98eaabf', + 'phabricator-application-launch-view-css' => 'c98eaabf', 'phabricator-busy' => '64eeda79', 'phabricator-content-source-view-css' => '4dc2311c', - 'phabricator-core-css' => 'cd37aa53', - 'phabricator-crumbs-view-css' => 'cd37aa53', + 'phabricator-core-css' => 'c98eaabf', + 'phabricator-crumbs-view-css' => 'c98eaabf', 'phabricator-drag-and-drop-file-upload' => '5e9e5c4e', 'phabricator-dropdown-menu' => '64eeda79', 'phabricator-file-upload' => '64eeda79', - 'phabricator-filetree-view-css' => 'cd37aa53', - 'phabricator-flag-css' => 'cd37aa53', + 'phabricator-filetree-view-css' => 'c98eaabf', + 'phabricator-flag-css' => 'c98eaabf', 'phabricator-hovercard' => '64eeda79', - 'phabricator-jump-nav' => 'cd37aa53', + 'phabricator-jump-nav' => 'c98eaabf', 'phabricator-keyboard-shortcut' => '64eeda79', 'phabricator-keyboard-shortcut-manager' => '64eeda79', - 'phabricator-main-menu-view' => 'cd37aa53', + 'phabricator-main-menu-view' => 'c98eaabf', 'phabricator-menu-item' => '64eeda79', - 'phabricator-nav-view-css' => 'cd37aa53', + 'phabricator-nav-view-css' => 'c98eaabf', 'phabricator-notification' => '64eeda79', - 'phabricator-notification-css' => 'cd37aa53', - 'phabricator-notification-menu-css' => 'cd37aa53', + 'phabricator-notification-css' => 'c98eaabf', + 'phabricator-notification-menu-css' => 'c98eaabf', 'phabricator-object-selector-css' => '4dc2311c', 'phabricator-phtize' => '64eeda79', 'phabricator-prefab' => '64eeda79', 'phabricator-project-tag-css' => '49898640', - 'phabricator-property-list-view-css' => 'cd37aa53', - 'phabricator-remarkup-css' => 'cd37aa53', + 'phabricator-property-list-view-css' => 'c98eaabf', + 'phabricator-remarkup-css' => 'c98eaabf', 'phabricator-shaped-request' => '5e9e5c4e', - 'phabricator-side-menu-view-css' => 'cd37aa53', - 'phabricator-standard-page-view' => 'cd37aa53', - 'phabricator-tag-view-css' => 'cd37aa53', + 'phabricator-side-menu-view-css' => 'c98eaabf', + 'phabricator-standard-page-view' => 'c98eaabf', + 'phabricator-tag-view-css' => 'c98eaabf', 'phabricator-textareautils' => '64eeda79', 'phabricator-tooltip' => '64eeda79', - 'phabricator-transaction-view-css' => 'cd37aa53', - 'phabricator-zindex-css' => 'cd37aa53', - 'phui-button-css' => 'cd37aa53', - 'phui-form-css' => 'cd37aa53', - 'phui-form-view-css' => 'cd37aa53', - 'phui-header-view-css' => 'cd37aa53', - 'phui-icon-view-css' => 'cd37aa53', - 'phui-list-view-css' => 'cd37aa53', - 'phui-object-item-list-view-css' => 'cd37aa53', - 'phui-spacing-css' => 'cd37aa53', - 'sprite-apps-large-css' => 'cd37aa53', - 'sprite-gradient-css' => 'cd37aa53', - 'sprite-icons-css' => 'cd37aa53', - 'sprite-menu-css' => 'cd37aa53', - 'sprite-status-css' => 'cd37aa53', - 'syntax-highlighting-css' => 'cd37aa53', + 'phabricator-transaction-view-css' => 'c98eaabf', + 'phabricator-zindex-css' => 'c98eaabf', + 'phui-button-css' => 'c98eaabf', + 'phui-form-css' => 'c98eaabf', + 'phui-form-view-css' => 'c98eaabf', + 'phui-header-view-css' => 'c98eaabf', + 'phui-icon-view-css' => 'c98eaabf', + 'phui-list-view-css' => 'c98eaabf', + 'phui-object-item-list-view-css' => 'c98eaabf', + 'phui-spacing-css' => 'c98eaabf', + 'sprite-apps-large-css' => 'c98eaabf', + 'sprite-gradient-css' => 'c98eaabf', + 'sprite-icons-css' => 'c98eaabf', + 'sprite-menu-css' => 'c98eaabf', + 'sprite-status-css' => 'c98eaabf', + 'syntax-highlighting-css' => 'c98eaabf', ), )); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f785604e2f..b58a6d570f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1472,7 +1472,10 @@ phutil_register_library_map(array( 'PhabricatorPolicy' => 'applications/policy/filter/PhabricatorPolicy.php', 'PhabricatorPolicyAwareQuery' => 'infrastructure/query/policy/PhabricatorPolicyAwareQuery.php', 'PhabricatorPolicyAwareTestQuery' => 'applications/policy/__tests__/PhabricatorPolicyAwareTestQuery.php', - 'PhabricatorPolicyCapability' => 'applications/policy/constants/PhabricatorPolicyCapability.php', + 'PhabricatorPolicyCapability' => 'applications/policy/capability/PhabricatorPolicyCapability.php', + 'PhabricatorPolicyCapabilityCanEdit' => 'applications/policy/capability/PhabricatorPolicyCapabilityCanEdit.php', + 'PhabricatorPolicyCapabilityCanJoin' => 'applications/policy/capability/PhabricatorPolicyCapabilityCanJoin.php', + 'PhabricatorPolicyCapabilityCanView' => 'applications/policy/capability/PhabricatorPolicyCapabilityCanView.php', 'PhabricatorPolicyConfigOptions' => 'applications/policy/config/PhabricatorPolicyConfigOptions.php', 'PhabricatorPolicyConstants' => 'applications/policy/constants/PhabricatorPolicyConstants.php', 'PhabricatorPolicyController' => 'applications/policy/controller/PhabricatorPolicyController.php', @@ -3656,7 +3659,10 @@ phutil_register_library_map(array( 'PhabricatorPolicies' => 'PhabricatorPolicyConstants', 'PhabricatorPolicyAwareQuery' => 'PhabricatorOffsetPagedQuery', 'PhabricatorPolicyAwareTestQuery' => 'PhabricatorPolicyAwareQuery', - 'PhabricatorPolicyCapability' => 'PhabricatorPolicyConstants', + 'PhabricatorPolicyCapability' => 'Phobject', + 'PhabricatorPolicyCapabilityCanEdit' => 'PhabricatorPolicyCapability', + 'PhabricatorPolicyCapabilityCanJoin' => 'PhabricatorPolicyCapability', + 'PhabricatorPolicyCapabilityCanView' => 'PhabricatorPolicyCapability', 'PhabricatorPolicyConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorPolicyController' => 'PhabricatorController', 'PhabricatorPolicyDataTestCase' => 'PhabricatorTestCase', diff --git a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php index e047e17b3f..56ea38765e 100644 --- a/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php +++ b/src/aphront/configuration/AphrontDefaultApplicationConfiguration.php @@ -172,22 +172,25 @@ class AphrontDefaultApplicationConfiguration $list = phutil_tag('ul', array(), $list); } - $content = phutil_tag( - 'div', - array( - 'class' => 'aphront-policy-exception', - ), - array( - $ex->getMessage(), - $list, - )); + $content = array( + phutil_tag( + 'div', + array( + 'class' => 'aphront-policy-rejection', + ), + $ex->getRejection()), + phutil_tag( + 'div', + array( + 'class' => 'aphront-capability-details', + ), + pht('Users with the "%s" capability:', $ex->getCapabilityName())), + $list, + ); $dialog = new AphrontDialogView(); $dialog - ->setTitle( - $is_serious - ? 'Access Denied' - : "You Shall Not Pass") + ->setTitle($ex->getTitle()) ->setClass('aphront-access-dialog') ->setUser($user) ->appendChild($content); diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index ef070f62bd..cf50fba48f 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -357,11 +357,9 @@ final class DifferentialRevision extends DifferentialDAO case PhabricatorPolicyCapability::CAN_VIEW: $description[] = pht( "A revision's reviewers can always view it."); - if ($this->getRepositoryPHID()) { - $description[] = pht( - 'This revision belongs to a repository. Other users must be able '. - 'to view the repository in order to view this revision.'); - } + $description[] = pht( + 'If a revision belongs to a repository, other users must be able '. + 'to view the repository in order to view the revision.'); break; } diff --git a/src/applications/policy/capability/PhabricatorPolicyCapability.php b/src/applications/policy/capability/PhabricatorPolicyCapability.php new file mode 100644 index 0000000000..7526c5c5b3 --- /dev/null +++ b/src/applications/policy/capability/PhabricatorPolicyCapability.php @@ -0,0 +1,61 @@ +setAncestorClass(__CLASS__) + ->loadObjects(); + + $map = mpull($capabilities, null, 'getCapabilityKey'); + } + + return $map; + } + +} + + diff --git a/src/applications/policy/capability/PhabricatorPolicyCapabilityCanEdit.php b/src/applications/policy/capability/PhabricatorPolicyCapabilityCanEdit.php new file mode 100644 index 0000000000..ddf3a2e89f --- /dev/null +++ b/src/applications/policy/capability/PhabricatorPolicyCapabilityCanEdit.php @@ -0,0 +1,18 @@ +executeOne(); $object_uri = $handle->getURI(); - $explanation = $policy->getExplanation($capability); + $explanation = PhabricatorPolicy::getPolicyExplanation( + $viewer, + $policy->getPHID()); + $auto_info = (array)$object->describeAutomaticCapability($capability); + $auto_info = array_merge( + array($explanation), + $auto_info); + $auto_info = array_filter($auto_info); + foreach ($auto_info as $key => $info) { $auto_info[$key] = phutil_tag('li', array(), $info); } @@ -56,14 +64,19 @@ final class PhabricatorPolicyExplainController } $content = array( - $explanation, + pht('Users with the "%s" capability:', "Can View"), $auto_info, ); + $object_name = pht( + '%s %s', + $handle->getTypeName(), + $handle->getObjectName()); + $dialog = id(new AphrontDialogView()) ->setUser($viewer) ->setClass('aphront-access-dialog') - ->setTitle(pht('Policy Details')) + ->setTitle(pht('Policy Details: %s', $object_name)) ->appendChild($content) ->addCancelButton($object_uri, pht('Done')); diff --git a/src/applications/policy/exception/PhabricatorPolicyException.php b/src/applications/policy/exception/PhabricatorPolicyException.php index 89649a3b8c..782e1350f0 100644 --- a/src/applications/policy/exception/PhabricatorPolicyException.php +++ b/src/applications/policy/exception/PhabricatorPolicyException.php @@ -2,8 +2,38 @@ final class PhabricatorPolicyException extends Exception { + private $title; + private $rejection; + private $capabilityName; private $moreInfo = array(); + public function setTitle($title) { + $this->title = $title; + return $this; + } + + public function getTitle() { + return $this->title; + } + + public function setCapabilityName($capability_name) { + $this->capabilityName = $capability_name; + return $this; + } + + public function getCapabilityName() { + return $this->capabilityName; + } + + public function setRejection($rejection) { + $this->rejection = $rejection; + return $this; + } + + public function getRejection() { + return $this->rejection; + } + public function setMoreInfo(array $more_info) { $this->moreInfo = $more_info; return $this; diff --git a/src/applications/policy/filter/PhabricatorPolicy.php b/src/applications/policy/filter/PhabricatorPolicy.php index a581c151dc..ee519124f3 100644 --- a/src/applications/policy/filter/PhabricatorPolicy.php +++ b/src/applications/policy/filter/PhabricatorPolicy.php @@ -130,52 +130,40 @@ final class PhabricatorPolicy { return $this->getName(); } - public function getExplanation($capability) { - switch ($capability) { - case PhabricatorPolicyCapability::CAN_VIEW: - switch ($this->getPHID()) { - case PhabricatorPolicies::POLICY_PUBLIC: - return pht('Visible to the entire internet.'); - case PhabricatorPolicies::POLICY_USER: - return pht('Visible to all logged in users.'); - case PhabricatorPolicies::POLICY_ADMIN: - return pht('Visible to all administrators.'); - case PhabricatorPolicies::POLICY_NOONE: - return pht('Not visible to anyone by default.'); - } - - switch ($this->getType()) { - case PhabricatorPolicyType::TYPE_PROJECT: - return pht( - 'Visible to members of the project "%s".', - $this->getName()); - case PhabricatorPolicyType::TYPE_MASKED: - return pht('Other: %s', $this->getName()); - } - break; - case PhabricatorPolicyCapability::CAN_EDIT: - switch ($this->getPHID()) { - case PhabricatorPolicies::POLICY_USER: - return pht('Editable by all logged in users.'); - case PhabricatorPolicies::POLICY_ADMIN: - return pht('Editable by all administrators.'); - case PhabricatorPolicies::POLICY_NOONE: - return pht('Not editable by default.'); - } - - switch ($this->getType()) { - case PhabricatorPolicyType::TYPE_PROJECT: - return pht( - 'Editable by members of the project "%s".', - $this->getName()); - case PhabricatorPolicyType::TYPE_MASKED: - return pht('Other: %s', $this->getName()); + public static function getPolicyExplanation( + PhabricatorUser $viewer, + $policy) { + + switch ($policy) { + case PhabricatorPolicies::POLICY_PUBLIC: + return pht('This object is public.'); + case PhabricatorPolicies::POLICY_USER: + return pht('Logged in users can take this action.'); + case PhabricatorPolicies::POLICY_ADMIN: + return pht('Administrators can take this action.'); + case PhabricatorPolicies::POLICY_NOONE: + return pht('By default, no one can take this action.'); + default: + $handle = id(new PhabricatorHandleQuery()) + ->setViewer($viewer) + ->withPHIDs(array($policy)) + ->executeOne(); + + $type = phid_get_type($policy); + if ($type == PhabricatorProjectPHIDTypeProject::TYPECONST) { + return pht( + 'Members of the project "%s" can take this action.', + $handle->getFullName()); + } else if ($type == PhabricatorPeoplePHIDTypeUser::TYPECONST) { + return pht( + '%s can take this action.', + $handle->getFullName()); + } else { + return pht( + 'This object has an unknown or invalid policy setting ("%s").', + $policy); } - break; } - - - return pht('?'); } public function getFullName() { diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index 2b386c265b..2838982249 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -242,118 +242,68 @@ final class PhabricatorPolicyFilter { return; } - $more = array(); - switch ($capability) { - case PhabricatorPolicyCapability::CAN_VIEW: - $message = pht( - 'This object exists, but you do not have permission to view it.'); - break; - case PhabricatorPolicyCapability::CAN_EDIT: - $message = pht('You do not have permission to edit this object.'); - break; - 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; + $capobj = PhabricatorPolicyCapability::getCapabilityByKey($capability); + if ($capobj) { + $rejection = $capobj->describeCapabilityRejection(); + $capability_name = $capobj->getCapabilityName(); + } else { + $rejection = pht( + 'You do not have the required capability ("%s") to do whatever you '. + 'are trying to do.', + $capability); + $capability_name = $capability; } - switch ($policy) { - case PhabricatorPolicies::POLICY_PUBLIC: - // Presumably, this is a bug, so we don't bother specializing the - // strings. - $more = pht('This object is public.'); - break; - case PhabricatorPolicies::POLICY_USER: - // We always raise this as "log in", so we don't need to specialize. - $more = pht('This object is available to logged in users.'); - break; - case PhabricatorPolicies::POLICY_ADMIN: - switch ($capability) { - case PhabricatorPolicyCapability::CAN_VIEW: - $more = pht('Administrators can view this object.'); - break; - case PhabricatorPolicyCapability::CAN_EDIT: - $more = pht('Administrators can edit this object.'); - break; - case PhabricatorPolicyCapability::CAN_JOIN: - $more = pht('Administrators can join this object.'); - break; - } - break; - case PhabricatorPolicies::POLICY_NOONE: - switch ($capability) { - case PhabricatorPolicyCapability::CAN_VIEW: - $more = pht('By default, no one can view this object.'); - break; - case PhabricatorPolicyCapability::CAN_EDIT: - $more = pht('By default, no one can edit this object.'); - break; - case PhabricatorPolicyCapability::CAN_JOIN: - $more = pht('By default, no one can join this object.'); - break; - } - break; - default: - $handle = id(new PhabricatorHandleQuery()) - ->setViewer($this->viewer) - ->withPHIDs(array($policy)) - ->executeOne(); + $more = PhabricatorPolicy::getPolicyExplanation($this->viewer, $policy); + $exceptions = $object->describeAutomaticCapability($capability); - $type = phid_get_type($policy); - if ($type == PhabricatorProjectPHIDTypeProject::TYPECONST) { - switch ($capability) { - case PhabricatorPolicyCapability::CAN_VIEW: - $more = pht( - 'This object is visible to members of the project "%s".', - $handle->getFullName()); - break; - case PhabricatorPolicyCapability::CAN_EDIT: - $more = pht( - 'This object can be edited by members of the project "%s".', - $handle->getFullName()); - break; - case PhabricatorPolicyCapability::CAN_JOIN: - $more = pht( - 'This object can be joined by members of the project "%s".', - $handle->getFullName()); - break; - } - } else if ($type == PhabricatorPeoplePHIDTypeUser::TYPECONST) { - switch ($capability) { - case PhabricatorPolicyCapability::CAN_VIEW: - $more = pht( - '%s can view this object.', - $handle->getFullName()); - break; - case PhabricatorPolicyCapability::CAN_EDIT: - $more = pht( - '%s can edit this object.', - $handle->getFullName()); - break; - case PhabricatorPolicyCapability::CAN_JOIN: - $more = pht( - '%s can join this object.', - $handle->getFullName()); - break; - } - } else { - $more = pht("This object has an unknown or invalid policy setting."); - } - break; + $details = array_filter(array_merge(array($more), (array)$exceptions)); + + // NOTE: Not every policy object has a PHID, just pull an arbitrary + // "unknown object" handle if this fails. We're just using this to provide + // a better error message if we can. + + $phid = '?'; + if ($object instanceof PhabricatorLiskDAO) { + try { + $phid = $object->getPHID(); + } catch (Exception $ignored) { + // Ignore. + } } - $more = array_merge( - array_filter(array($more)), - array_filter((array)$object->describeAutomaticCapability($capability))); + $handle = id(new PhabricatorHandleQuery()) + ->setViewer($this->viewer) + ->withPHIDs(array($phid)) + ->executeOne(); + $object_name = pht( + '%s %s', + $handle->getTypeName(), + $handle->getObjectName()); + + $is_serious = PhabricatorEnv::getEnvConfig('phabricator.serious-business'); + if ($is_serious) { + $title = pht( + 'Access Denied: %s', + $object_name); + } else { + $title = pht( + 'You Shall Not Pass: %s', + $object_name); + } - $exception = new PhabricatorPolicyException($message); - $exception->setMoreInfo($more); + $full_message = pht( + '[%s] (%s) %s // %s', + $title, + $capability_name, + $rejection, + implode(' ', $details)); + + $exception = id(new PhabricatorPolicyException($full_message)) + ->setTitle($title) + ->setRejection($rejection) + ->setCapabilityName($capability_name) + ->setMoreInfo($details); throw $exception; } diff --git a/src/applications/policy/management/PhabricatorPolicyManagementShowWorkflow.php b/src/applications/policy/management/PhabricatorPolicyManagementShowWorkflow.php index ca6c6754a2..b710eed768 100644 --- a/src/applications/policy/management/PhabricatorPolicyManagementShowWorkflow.php +++ b/src/applications/policy/management/PhabricatorPolicyManagementShowWorkflow.php @@ -64,7 +64,8 @@ final class PhabricatorPolicyManagementShowWorkflow foreach ($policies as $capability => $policy) { $console->writeOut(" **%s**\n", $capability); $console->writeOut(" %s\n", $policy->renderDescription()); - $console->writeOut(" %s\n", $policy->getExplanation($capability)); + $console->writeOut(" %s\n", + PhabricatorPolicy::getPolicyExplanation($viewer, $policy->getPHID())); $console->writeOut("\n"); $more = (array)$object->describeAutomaticCapability($capability); diff --git a/webroot/rsrc/css/aphront/dialog-view.css b/webroot/rsrc/css/aphront/dialog-view.css index e42d1682eb..0462c83017 100644 --- a/webroot/rsrc/css/aphront/dialog-view.css +++ b/webroot/rsrc/css/aphront/dialog-view.css @@ -116,3 +116,11 @@ margin: 12px 24px; list-style: circle; } + +.aphront-policy-rejection { + font-weight: bold; +} + +.aphront-capability-details { + margin: 20px 0 4px; +} -- 2.51.2