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; + } + }