From 506f93b4a39007b50b2daa5ded3223b19c85f490 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 12 Sep 2019 07:23:43 -0700 Subject: [PATCH] Give policy name rendering explicit "text name", "capability link", and "transaction link" pathways Summary: Ref T13411. This cleans up policy name rendering. We ultimately render into three contexts: - Plain text contexts, like `bin/policy show`. - Transaction contexts, where we're showing a policy change. In these cases, we link some policies (like project policies and custom policies) but the links go directly to the relevant object or a minimal explanation of the change. We don't link policies like "All Users". - Capability contexts, where we're describing a capability, like "Can Push" or cases in Applicaitons. In these cases, we link all policies to the full policy explanation flow. Test Plan: - Used `bin/policy show` to examine the policy of an object with a project policy, no longer saw HTML. - Viewed the transaction logs of Applications (ModularTransactions) and Tasks (not ModularTransactions) with policy edits, including project and custom policies. - Clicked "Custom Policy" in both logs, got consistent dialogs. - Viewed application detail pages, saw all capabities linked to explanatory capability dialogs. The value of having this dialog is that the user can get a full explanation of special rules even if the policy is something mundane like "All Users". Maniphest Tasks: T13411 Differential Revision: https://secure.phabricator.com/D20804 --- src/__phutil_library_map__.php | 2 + ...habricatorPolicyManagementShowWorkflow.php | 4 +- .../policy/query/PhabricatorPolicyQuery.php | 3 +- .../policy/storage/PhabricatorPolicy.php | 40 +++----- .../policy/view/PhabricatorPolicyRef.php | 99 +++++++++++++++++++ .../PhabricatorApplicationTransaction.php | 16 ++- .../PhabricatorModularTransactionType.php | 15 ++- src/docs/user/userguide/unlocking.diviner | 17 +++- 8 files changed, 146 insertions(+), 50 deletions(-) create mode 100644 src/applications/policy/view/PhabricatorPolicyRef.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index fad919f4dd..d99427dcab 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4195,6 +4195,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyManagementWorkflow' => 'applications/policy/management/PhabricatorPolicyManagementWorkflow.php', 'PhabricatorPolicyPHIDTypePolicy' => 'applications/policy/phid/PhabricatorPolicyPHIDTypePolicy.php', 'PhabricatorPolicyQuery' => 'applications/policy/query/PhabricatorPolicyQuery.php', + 'PhabricatorPolicyRef' => 'applications/policy/view/PhabricatorPolicyRef.php', 'PhabricatorPolicyRequestExceptionHandler' => 'aphront/handler/PhabricatorPolicyRequestExceptionHandler.php', 'PhabricatorPolicyRule' => 'applications/policy/rule/PhabricatorPolicyRule.php', 'PhabricatorPolicySearchEngineExtension' => 'applications/policy/engineextension/PhabricatorPolicySearchEngineExtension.php', @@ -10675,6 +10676,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyManagementWorkflow' => 'PhabricatorManagementWorkflow', 'PhabricatorPolicyPHIDTypePolicy' => 'PhabricatorPHIDType', 'PhabricatorPolicyQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', + 'PhabricatorPolicyRef' => 'Phobject', 'PhabricatorPolicyRequestExceptionHandler' => 'PhabricatorRequestExceptionHandler', 'PhabricatorPolicyRule' => 'Phobject', 'PhabricatorPolicySearchEngineExtension' => 'PhabricatorSearchEngineExtension', diff --git a/src/applications/policy/management/PhabricatorPolicyManagementShowWorkflow.php b/src/applications/policy/management/PhabricatorPolicyManagementShowWorkflow.php index 208f1ae964..a378ecc076 100644 --- a/src/applications/policy/management/PhabricatorPolicyManagementShowWorkflow.php +++ b/src/applications/policy/management/PhabricatorPolicyManagementShowWorkflow.php @@ -60,8 +60,10 @@ final class PhabricatorPolicyManagementShowWorkflow $console->writeOut("__%s__\n\n", pht('CAPABILITIES')); foreach ($policies as $capability => $policy) { + $ref = $policy->newRef($viewer); + $console->writeOut(" **%s**\n", $capability); - $console->writeOut(" %s\n", $policy->renderDescription()); + $console->writeOut(" %s\n", $ref->getPolicyDisplayName()); $console->writeOut(" %s\n", PhabricatorPolicy::getPolicyExplanation($viewer, $policy->getPHID())); $console->writeOut("\n"); diff --git a/src/applications/policy/query/PhabricatorPolicyQuery.php b/src/applications/policy/query/PhabricatorPolicyQuery.php index 3337a912b0..018007db28 100644 --- a/src/applications/policy/query/PhabricatorPolicyQuery.php +++ b/src/applications/policy/query/PhabricatorPolicyQuery.php @@ -48,7 +48,8 @@ final class PhabricatorPolicyQuery $policies = self::loadPolicies($viewer, $object); foreach ($policies as $capability => $policy) { - $policies[$capability] = $policy->renderDescription(); + $policies[$capability] = $policy->newRef($viewer) + ->newCapabilityLink($object, $capability); } return $policies; diff --git a/src/applications/policy/storage/PhabricatorPolicy.php b/src/applications/policy/storage/PhabricatorPolicy.php index 834ca7a798..82d4f355bc 100644 --- a/src/applications/policy/storage/PhabricatorPolicy.php +++ b/src/applications/policy/storage/PhabricatorPolicy.php @@ -276,32 +276,22 @@ final class PhabricatorPolicy } } - public function renderDescription() { - if ($this->getHref()) { - $desc = javelin_tag( - 'a', - array( - 'href' => $this->getHref(), - 'class' => 'policy-link', - 'sigil' => $this->getWorkflow() ? 'workflow' : null, - ), - $this->getName()); - } else { - $desc = $this->getName(); - } + public function newRef(PhabricatorUser $viewer) { + return id(new PhabricatorPolicyRef()) + ->setViewer($viewer) + ->setPolicy($this); + } - switch ($this->getType()) { - case PhabricatorPolicyType::TYPE_PROJECT: - return pht('%s (Project)', $desc); - case PhabricatorPolicyType::TYPE_CUSTOM: - return $desc; - case PhabricatorPolicyType::TYPE_MASKED: - return pht( - '%s (You do not have permission to view policy details.)', - $desc); - default: - return $desc; - } + public function isProjectPolicy() { + return ($this->getType() === PhabricatorPolicyType::TYPE_PROJECT); + } + + public function isCustomPolicy() { + return ($this->getType() === PhabricatorPolicyType::TYPE_CUSTOM); + } + + public function isMaskedPolicy() { + return ($this->getType() === PhabricatorPolicyType::TYPE_MASKED); } /** diff --git a/src/applications/policy/view/PhabricatorPolicyRef.php b/src/applications/policy/view/PhabricatorPolicyRef.php new file mode 100644 index 0000000000..605d59ee45 --- /dev/null +++ b/src/applications/policy/view/PhabricatorPolicyRef.php @@ -0,0 +1,99 @@ +viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + + public function setPolicy(PhabricatorPolicy $policy) { + $this->policy = $policy; + return $this; + } + + public function getPolicy() { + return $this->policy; + } + + public function getPolicyDisplayName() { + $policy = $this->getPolicy(); + return $policy->getFullName(); + } + + public function newTransactionLink( + $mode, + PhabricatorApplicationTransaction $xaction) { + + $policy = $this->getPolicy(); + + if ($policy->isCustomPolicy()) { + $uri = urisprintf( + '/transactions/%s/%s/', + $mode, + $xaction->getPHID()); + $workflow = true; + } else { + $uri = $policy->getHref(); + $workflow = false; + } + + return $this->newLink($uri, $workflow); + } + + public function newCapabilityLink($object, $capability) { + $policy = $this->getPolicy(); + + $uri = urisprintf( + '/policy/explain/%s/%s/', + $object->getPHID(), + $capability); + + return $this->newLink($uri, true); + } + + private function newLink($uri, $workflow) { + $policy = $this->getPolicy(); + $name = $policy->getName(); + + if ($uri !== null) { + $name = javelin_tag( + 'a', + array( + 'href' => $uri, + 'sigil' => ($workflow ? 'workflow' : null), + ), + $name); + } + + $hint = $this->getPolicyTypeHint(); + if ($hint !== null) { + $name = pht('%s (%s)', $name, $hint); + } + + return $name; + } + + private function getPolicyTypeHint() { + $policy = $this->getPolicy(); + + if ($policy->isProjectPolicy()) { + return pht('Project'); + } + + if ($policy->isMaskedPolicy()) { + return pht('You do not have permission to view policy details.'); + } + + return null; + } + +} diff --git a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php index 5fa4562164..7e65ac0a09 100644 --- a/src/applications/transactions/storage/PhabricatorApplicationTransaction.php +++ b/src/applications/transactions/storage/PhabricatorApplicationTransaction.php @@ -445,19 +445,15 @@ abstract class PhabricatorApplicationTransaction $policy = PhabricatorPolicy::newFromPolicyAndHandle( $phid, $this->getHandleIfExists($phid)); + + $ref = $policy->newRef($this->getViewer()); + if ($this->renderingTarget == self::TARGET_HTML) { - switch ($policy->getType()) { - case PhabricatorPolicyType::TYPE_CUSTOM: - $policy->setHref('/transactions/'.$state.'/'.$this->getPHID().'/'); - $policy->setWorkflow(true); - break; - default: - break; - } - $output = $policy->renderDescription(); + $output = $ref->newTransactionLink($state, $this); } else { - $output = hsprintf('%s', $policy->getFullName()); + $output = $ref->getPolicyDisplayName(); } + return $output; } diff --git a/src/applications/transactions/storage/PhabricatorModularTransactionType.php b/src/applications/transactions/storage/PhabricatorModularTransactionType.php index 2d0cb8e7c1..7d5e3c533e 100644 --- a/src/applications/transactions/storage/PhabricatorModularTransactionType.php +++ b/src/applications/transactions/storage/PhabricatorModularTransactionType.php @@ -215,17 +215,16 @@ abstract class PhabricatorModularTransactionType $phid, $handles[$phid]); + $ref = $policy->newRef($viewer); + if ($this->isTextMode()) { - return $this->renderValue($policy->getFullName()); + $name = $ref->getPolicyDisplayName(); + } else { + $storage = $this->getStorage(); + $name = $ref->newTransactionLink($mode, $storage); } - $storage = $this->getStorage(); - if ($policy->getType() == PhabricatorPolicyType::TYPE_CUSTOM) { - $policy->setHref('/transactions/'.$mode.'/'.$storage->getPHID().'/'); - $policy->setWorkflow(true); - } - - return $this->renderValue($policy->renderDescription()); + return $this->renderValue($name); } final protected function renderHandleList(array $phids) { diff --git a/src/docs/user/userguide/unlocking.diviner b/src/docs/user/userguide/unlocking.diviner index 7dc29f69bd..456655a393 100644 --- a/src/docs/user/userguide/unlocking.diviner +++ b/src/docs/user/userguide/unlocking.diviner @@ -63,16 +63,23 @@ For detailed help on managing and stripping MFA, see the instructions in Unlocking Objects ================= -If you aren't sure who owns an object, or no user account has access to an -object, you can directly change object policies from the CLI: +If you aren't sure who owns an object, you can inspect the policies from the +CLI: + +``` +$ ./bin/policy show +``` + +To identify the object you want to examine, you can specify an object +name (like `T123`) or a PHID as the `` parameter. + +If examining the policy isn't helpful, or no user account has access to an +object, you can then directly change object policies from the CLI: ``` $ ./bin/policy unlock [--view ...] [--edit ...] [--owner ...] ``` -To identify the object you want to unlock, you can specify an object name (like -`T123`) or a PHID as the `` parameter. - Use the `--view` and `--edit` flags (and, for some objects, the `--owner` flag) to specify new policies for the object.