From 1f86c73428819492b62d4c575efb788f13dc8c86 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 10 Sep 2013 09:06:20 -0700 Subject: [PATCH] Simplify policy filtering for projects and ObjectQuery Summary: Ref T603. Moves to detangle and optimize how we apply policies to filtering objects. Notably: - Add a short circuit for omnipotent users. - When performing project filtering, do a stricter check for user membership. We don't actually care if the user can see the project or not according to other policy constraints, and checking if they can may be complicated. - When performing project filtering, do a local check to see if we're filtering the project itself. This is a common case (a project editable by members of itself, for example) and we can skip queries when it is satisfied. - Don't perform policy filtering in ObjectQuery. All the data it aggregates is already filtered correctly. - Clean up a little bit of stuff in Feed. Test Plan: Pages like the Maniphest task list and Project profile pages now issue dramatically fewer queries. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T603 Differential Revision: https://secure.phabricator.com/D6931 --- .../feed/story/PhabricatorFeedStory.php | 10 ++- .../phid/query/PhabricatorObjectQuery.php | 9 +++ .../policy/filter/PhabricatorPolicyFilter.php | 75 +++++++++++-------- .../policy/PhabricatorPolicyAwareQuery.php | 20 ++++- 4 files changed, 76 insertions(+), 38 deletions(-) diff --git a/src/applications/feed/story/PhabricatorFeedStory.php b/src/applications/feed/story/PhabricatorFeedStory.php index 07503e86b4..3d815364ef 100644 --- a/src/applications/feed/story/PhabricatorFeedStory.php +++ b/src/applications/feed/story/PhabricatorFeedStory.php @@ -71,9 +71,10 @@ abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface { $object_phids += $phids; } - $objects = id(new PhabricatorObjectHandleData(array_keys($object_phids))) + $objects = id(new PhabricatorObjectQuery()) ->setViewer($viewer) - ->loadObjects(); + ->withPHIDs(array_keys($object_phids)) + ->execute(); foreach ($key_phids as $key => $phids) { if (!$phids) { @@ -102,9 +103,10 @@ abstract class PhabricatorFeedStory implements PhabricatorPolicyInterface { $handle_phids += $key_phids[$key]; } - $handles = id(new PhabricatorObjectHandleData(array_keys($handle_phids))) + $handles = id(new PhabricatorHandleQuery()) ->setViewer($viewer) - ->loadHandles(); + ->withPHIDs(array_keys($handle_phids)) + ->execute(); foreach ($key_phids as $key => $phids) { if (!$phids) { diff --git a/src/applications/phid/query/PhabricatorObjectQuery.php b/src/applications/phid/query/PhabricatorObjectQuery.php index d3ed12584c..94b670ebaa 100644 --- a/src/applications/phid/query/PhabricatorObjectQuery.php +++ b/src/applications/phid/query/PhabricatorObjectQuery.php @@ -118,4 +118,13 @@ final class PhabricatorObjectQuery } } + /** + * This query disables policy filtering because it is performed in the + * subqueries which actually load objects. We don't need to re-filter + * results, since policies have already been applied. + */ + protected function shouldDisablePolicyFiltering() { + return true; + } + } diff --git a/src/applications/policy/filter/PhabricatorPolicyFilter.php b/src/applications/policy/filter/PhabricatorPolicyFilter.php index 9a26b952eb..ca83847a24 100644 --- a/src/applications/policy/filter/PhabricatorPolicyFilter.php +++ b/src/applications/policy/filter/PhabricatorPolicyFilter.php @@ -70,7 +70,19 @@ final class PhabricatorPolicyFilter { 'Call setViewer() and requireCapabilities() before apply()!'); } + // If the viewer is omnipotent, short circuit all the checks and just + // return the input unmodified. This is an optimization; we know the + // result already. + if ($viewer->isOmnipotent()) { + return $objects; + } + $filtered = array(); + $viewer_phid = $viewer->getPHID(); + + if (empty($this->userProjects[$viewer_phid])) { + $this->userProjects[$viewer_phid] = array(); + } $need_projects = array(); foreach ($objects as $key => $object) { @@ -85,7 +97,23 @@ final class PhabricatorPolicyFilter { $policy = $object->getPolicy($capability); $type = phid_get_type($policy); if ($type == PhabricatorProjectPHIDTypeProject::TYPECONST) { - $need_projects[] = $policy; + $need_projects[$policy] = $policy; + } + } + } + + // If we need projects, check if any of the projects we need are also the + // objects we're filtering. Because of how project rules work, this is a + // common case. + if ($need_projects) { + foreach ($objects as $object) { + if ($object instanceof PhabricatorProject) { + $project_phid = $object->getPHID(); + if (isset($need_projects[$project_phid])) { + $is_member = $object->isUserMember($viewer_phid); + $this->userProjects[$viewer_phid][$project_phid] = $is_member; + unset($need_projects[$project_phid]); + } } } } @@ -93,40 +121,21 @@ final class PhabricatorPolicyFilter { if ($need_projects) { $need_projects = array_unique($need_projects); - // If projects have recursive policies, automatically fail them rather - // than looping. This will fall back to automatic capabilities and - // resolve the policies in a sensible way. - static $querying_projects = array(); - foreach ($need_projects as $key => $project) { - if (empty($querying_projects[$project])) { - $querying_projects[$project] = true; - continue; - } - unset($need_projects[$key]); - } + // NOTE: We're using the omnipotent user here to avoid a recursive + // descent into madness. We don't actually need to know if the user can + // see these projects or not, since: the check is "user is member of + // project", not "user can see project"; and membership implies + // visibility anyway. Without this, we may load other projects and + // re-enter the policy filter and generally create a huge mess. - if ($need_projects) { - $caught = null; - try { - $projects = id(new PhabricatorProjectQuery()) - ->setViewer($viewer) - ->withMemberPHIDs(array($viewer->getPHID())) - ->withPHIDs($need_projects) - ->execute(); - } catch (Exception $ex) { - $caught = $ex; - } + $projects = id(new PhabricatorProjectQuery()) + ->setViewer(PhabricatorUser::getOmnipotentUser()) + ->withMemberPHIDs(array($viewer->getPHID())) + ->withPHIDs($need_projects) + ->execute(); - foreach ($need_projects as $key => $project) { - unset($querying_projects[$project]); - } - - if ($caught) { - throw $caught; - } - - $projects = mpull($projects, null, 'getPHID'); - $this->userProjects[$viewer->getPHID()] = $projects; + foreach ($projects as $project) { + $this->userProjects[$viewer_phid][$project->getPHID()] = true; } } diff --git a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php index 79a76d2311..b16a47db73 100644 --- a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php @@ -182,7 +182,11 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { $maybe_visible = array(); } - $visible = $filter->apply($maybe_visible); + if ($this->shouldDisablePolicyFiltering()) { + $visible = $maybe_visible; + } else { + $visible = $filter->apply($maybe_visible); + } $removed = array(); foreach ($maybe_visible as $key => $object) { @@ -328,4 +332,18 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { return $results; } + + /** + * Allows a subclass to disable policy filtering. This method is dangerous. + * It should be used only if the query loads data which has already been + * filtered (for example, because it wraps some other query which uses + * normal policy filtering). + * + * @return bool True to disable all policy filtering. + * @task policyimpl + */ + protected function shouldDisablePolicyFiltering() { + return false; + } + }