From f0dc06529033062a6c96425cb834c67b40773e7c Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 4 Feb 2021 17:12:37 -0800 Subject: [PATCH] Lift bulk tests for "many users against one object" capabilities into "PolicyFilterSet" Summary: Ref T13602. Currently, the policy framework can not execute "test if many users can see one object" particluarly efficiently. This test must be executed more broadly to implement the changes in T13602. To avoid making this any worse than it already is, lift this block into a wrapper class that has a bulk queue + fetch API and could eventually be optimized. Test Plan: Viewed a task with an `@mention` of a user without permission to see it in the summary, saw it rendered in a disabled style. Maniphest Tasks: T13602 Differential Revision: https://secure.phabricator.com/D21546 --- src/__phutil_library_map__.php | 2 + .../markup/PhabricatorMentionRemarkupRule.php | 35 ++++-- .../filter/PhabricatorPolicyFilterSet.php | 105 ++++++++++++++++++ 3 files changed, 131 insertions(+), 11 deletions(-) create mode 100644 src/applications/policy/filter/PhabricatorPolicyFilterSet.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8af718e70b..57e9a22a57 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4270,6 +4270,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyExplainController' => 'applications/policy/controller/PhabricatorPolicyExplainController.php', 'PhabricatorPolicyFavoritesSetting' => 'applications/settings/setting/PhabricatorPolicyFavoritesSetting.php', 'PhabricatorPolicyFilter' => 'applications/policy/filter/PhabricatorPolicyFilter.php', + 'PhabricatorPolicyFilterSet' => 'applications/policy/filter/PhabricatorPolicyFilterSet.php', 'PhabricatorPolicyInterface' => 'applications/policy/interface/PhabricatorPolicyInterface.php', 'PhabricatorPolicyManagementShowWorkflow' => 'applications/policy/management/PhabricatorPolicyManagementShowWorkflow.php', 'PhabricatorPolicyManagementUnlockWorkflow' => 'applications/policy/management/PhabricatorPolicyManagementUnlockWorkflow.php', @@ -10920,6 +10921,7 @@ phutil_register_library_map(array( 'PhabricatorPolicyExplainController' => 'PhabricatorPolicyController', 'PhabricatorPolicyFavoritesSetting' => 'PhabricatorInternalSetting', 'PhabricatorPolicyFilter' => 'Phobject', + 'PhabricatorPolicyFilterSet' => 'Phobject', 'PhabricatorPolicyInterface' => 'PhabricatorPHIDInterface', 'PhabricatorPolicyManagementShowWorkflow' => 'PhabricatorPolicyManagementWorkflow', 'PhabricatorPolicyManagementUnlockWorkflow' => 'PhabricatorPolicyManagementWorkflow', diff --git a/src/applications/people/markup/PhabricatorMentionRemarkupRule.php b/src/applications/people/markup/PhabricatorMentionRemarkupRule.php index 60d4a8168f..6cf3b4b24b 100644 --- a/src/applications/people/markup/PhabricatorMentionRemarkupRule.php +++ b/src/applications/people/markup/PhabricatorMentionRemarkupRule.php @@ -87,24 +87,37 @@ final class PhabricatorMentionRemarkupRule extends PhutilRemarkupRule { $engine->setTextMetadata($mentioned_key, $mentioned); $context_object = $engine->getConfig('contextObject'); + $policy_object = null; + if ($context_object) { + if ($context_object instanceof PhabricatorPolicyInterface) { + $policy_object = $context_object; + } + } + + if ($policy_object) { + $policy_set = new PhabricatorPolicyFilterSet(); + foreach ($actual_users as $user) { + $policy_set->addCapability( + $user, + $policy_object, + PhabricatorPolicyCapability::CAN_VIEW); + } + } + foreach ($metadata as $username => $tokens) { $exists = isset($actual_users[$username]); - $user_has_no_permission = false; + $user_can_not_view = false; if ($exists) { $user = $actual_users[$username]; Javelin::initBehavior('phui-hovercards'); // Check if the user has view access to the object she was mentioned in - if ($context_object - && $context_object instanceof PhabricatorPolicyInterface) { - if (!PhabricatorPolicyFilter::hasCapability( + if ($policy_object) { + $user_can_not_view = !$policy_set->hasCapability( $user, - $context_object, - PhabricatorPolicyCapability::CAN_VIEW)) { - // User mentioned has no permission to this object - $user_has_no_permission = true; - } + $policy_object, + PhabricatorPolicyCapability::CAN_VIEW); } $user_href = '/p/'.$user->getUserName().'/'; @@ -112,7 +125,7 @@ final class PhabricatorMentionRemarkupRule extends PhutilRemarkupRule { if ($engine->isHTMLMailMode()) { $user_href = PhabricatorEnv::getProductionURI($user_href); - if ($user_has_no_permission) { + if ($user_can_not_view) { $colors = ' border-color: #92969D; color: #92969D; @@ -146,7 +159,7 @@ final class PhabricatorMentionRemarkupRule extends PhutilRemarkupRule { ->setName('@'.$user->getUserName()) ->setHref($user_href); - if ($user_has_no_permission) { + if ($user_can_not_view) { $tag->addClass('phabricator-remarkup-mention-nopermission'); } diff --git a/src/applications/policy/filter/PhabricatorPolicyFilterSet.php b/src/applications/policy/filter/PhabricatorPolicyFilterSet.php new file mode 100644 index 0000000000..c1e8156f09 --- /dev/null +++ b/src/applications/policy/filter/PhabricatorPolicyFilterSet.php @@ -0,0 +1,105 @@ +getUserKey($user); + $this->users[$user_key] = $user; + + $object_key = $this->getObjectKey($object); + $this->objects[$object_key] = $object; + + if (!isset($this->capabilities[$capability][$user_key][$object_key])) { + $this->capabilities[$capability][$user_key][$object_key] = true; + $this->queue[$capability][$user_key][$object_key] = true; + } + + return $this; + } + + public function hasCapability( + PhabricatorUser $user, + PhabricatorPolicyInterface $object, + $capability) { + + $user_key = $this->getUserKey($user); + $this->users[$user_key] = $user; + + $object_key = $this->getObjectKey($object); + $this->objects[$object_key] = $object; + + if (!isset($this->capabilities[$capability][$user_key][$object_key])) { + throw new Exception( + pht( + 'Capability "%s" for user "%s" on object "%s" is being resolved, '. + 'but was never queued with "addCapability()".', + $capability, + $user_key, + $object_key)); + } + + if (!isset($this->results[$capability][$user_key][$object_key])) { + $this->resolveCapabilities(); + } + + return $this->results[$capability][$user_key][$object_key]; + } + + private function getUserKey(PhabricatorUser $user) { + return $user->getCacheFragment(); + } + + private function getObjectKey(PhabricatorPolicyInterface $object) { + $object_phid = $object->getPHID(); + + if (!$object_phid) { + throw new Exception( + pht( + 'Unable to perform capability tests on an object (of class "%s") '. + 'with no PHID.', + get_class($object))); + } + + return $object_phid; + } + + private function resolveCapabilities() { + + // This class is primarily used to test if a list of users (like + // subscribers) can see a single object. It is not structured in a way + // that makes this particularly efficient, and performance would probably + // be improved if filtering supported this use case more narrowly. + + foreach ($this->queue as $capability => $user_map) { + foreach ($user_map as $user_key => $object_map) { + $user = $this->users[$user_key]; + $objects = array_select_keys($this->objects, array_keys($object_map)); + + $filter = id(new PhabricatorPolicyFilter()) + ->setViewer($user) + ->requireCapabilities(array($capability)); + $results = $filter->apply($objects); + + foreach ($object_map as $object_key => $object) { + $has_capability = (bool)isset($results[$object_key]); + $this->results[$capability][$user_key][$object_key] = $has_capability; + } + } + } + + $this->queue = array(); + } + +}