Index: src/applications/files/query/PhabricatorFileQuery.php =================================================================== --- src/applications/files/query/PhabricatorFileQuery.php +++ src/applications/files/query/PhabricatorFileQuery.php @@ -128,12 +128,14 @@ $object_phids[$phid] = true; } } + $object_phids = array_keys($object_phids); // Now, load the objects. $objects = array(); if ($object_phids) { $objects = id(new PhabricatorObjectQuery()) + ->setParentQuery($this) ->setViewer($this->getViewer()) ->withPHIDs($object_phids) ->execute(); Index: src/applications/files/storage/PhabricatorFile.php =================================================================== --- src/applications/files/storage/PhabricatorFile.php +++ src/applications/files/storage/PhabricatorFile.php @@ -764,8 +764,10 @@ ); } + // NOTE: Anyone is allowed to access builtin files. + $files = id(new PhabricatorFileQuery()) - ->setViewer($user) + ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withTransforms($specs) ->execute(); Index: src/applications/macro/query/PhabricatorMacroQuery.php =================================================================== --- src/applications/macro/query/PhabricatorMacroQuery.php +++ src/applications/macro/query/PhabricatorMacroQuery.php @@ -191,10 +191,11 @@ return $this->formatWhereClause($where); } - protected function willFilterPage(array $macros) { + protected function didFilterPage(array $macros) { $file_phids = mpull($macros, 'getFilePHID'); $files = id(new PhabricatorFileQuery()) ->setViewer($this->getViewer()) + ->setParentQuery($this) ->withPHIDs($file_phids) ->execute(); $files = mpull($files, null, 'getPHID'); Index: src/applications/paste/query/PhabricatorPasteQuery.php =================================================================== --- src/applications/paste/query/PhabricatorPasteQuery.php +++ src/applications/paste/query/PhabricatorPasteQuery.php @@ -87,7 +87,7 @@ return $pastes; } - protected function willFilterPage(array $pastes) { + protected function didFilterPage(array $pastes) { if ($this->needRawContent) { $pastes = $this->loadRawContent($pastes); } @@ -163,6 +163,7 @@ private function loadRawContent(array $pastes) { $file_phids = mpull($pastes, 'getFilePHID'); $files = id(new PhabricatorFileQuery()) + ->setParentQuery($this) ->setViewer($this->getViewer()) ->withPHIDs($file_phids) ->execute(); Index: src/applications/people/query/PhabricatorPeopleQuery.php =================================================================== --- src/applications/people/query/PhabricatorPeopleQuery.php +++ src/applications/people/query/PhabricatorPeopleQuery.php @@ -113,8 +113,10 @@ $table->putInSet(new LiskDAOSet()); } - $users = $table->loadAllFromArray($data); + return $table->loadAllFromArray($data); + } + protected function didFilterPage(array $users) { if ($this->needProfile) { $user_list = mpull($users, null, 'getPHID'); $profiles = new PhabricatorUserProfile(); @@ -138,6 +140,7 @@ $user_profile_file_phids = array_filter($user_profile_file_phids); if ($user_profile_file_phids) { $files = id(new PhabricatorFileQuery()) + ->setParentQuery($this) ->setViewer($this->getViewer()) ->withPHIDs($user_profile_file_phids) ->execute(); Index: src/applications/phid/query/PhabricatorObjectQuery.php =================================================================== --- src/applications/phid/query/PhabricatorObjectQuery.php +++ src/applications/phid/query/PhabricatorObjectQuery.php @@ -104,13 +104,27 @@ } private function loadObjectsByPHID(array $types, array $phids) { + $results = array(); + + $workspace = $this->getObjectsFromWorkspace($phids); + + foreach ($phids as $key => $phid) { + if (isset($workspace[$phid])) { + $results[$phid] = $workspace[$phid]; + unset($phids[$key]); + } + } + + if (!$phids) { + return $results; + } + $groups = array(); foreach ($phids as $phid) { $type = phid_get_type($phid); $groups[$type][] = $phid; } - $results = array(); foreach ($groups as $type => $group) { if (isset($types[$type])) { $objects = $types[$type]->loadObjects($this, $group); Index: src/applications/pholio/query/PholioImageQuery.php =================================================================== --- src/applications/pholio/query/PholioImageQuery.php +++ src/applications/pholio/query/PholioImageQuery.php @@ -103,9 +103,37 @@ protected function willFilterPage(array $images) { assert_instances_of($images, 'PholioImage'); + if ($this->getMockCache()) { + $mocks = $this->getMockCache(); + } else { + $mock_ids = mpull($images, 'getMockID'); + // DO NOT set needImages to true; recursion results! + $mocks = id(new PholioMockQuery()) + ->setViewer($this->getViewer()) + ->withIDs($mock_ids) + ->execute(); + $mocks = mpull($mocks, null, 'getID'); + } + foreach ($images as $index => $image) { + $mock = idx($mocks, $image->getMockID()); + if ($mock) { + $image->attachMock($mock); + } else { + // mock is missing or we can't see it + unset($images[$index]); + } + } + + return $images; + } + + protected function didFilterPage(array $images) { + assert_instances_of($images, 'PholioImage'); + $file_phids = mpull($images, 'getFilePHID'); $all_files = id(new PhabricatorFileQuery()) + ->setParentQuery($this) ->setViewer($this->getViewer()) ->withPHIDs($file_phids) ->execute(); @@ -130,27 +158,6 @@ } } - if ($this->getMockCache()) { - $mocks = $this->getMockCache(); - } else { - $mock_ids = mpull($images, 'getMockID'); - // DO NOT set needImages to true; recursion results! - $mocks = id(new PholioMockQuery()) - ->setViewer($this->getViewer()) - ->withIDs($mock_ids) - ->execute(); - $mocks = mpull($mocks, null, 'getID'); - } - foreach ($images as $index => $image) { - $mock = idx($mocks, $image->getMockID()); - if ($mock) { - $image->attachMock($mock); - } else { - // mock is missing or we can't see it - unset($images[$index]); - } - } - return $images; } Index: src/applications/project/controller/PhabricatorProjectProfileController.php =================================================================== --- src/applications/project/controller/PhabricatorProjectProfileController.php +++ src/applications/project/controller/PhabricatorProjectProfileController.php @@ -6,6 +6,10 @@ private $id; private $page; + public function shouldAllowPublic() { + return true; + } + public function willProcessRequest(array $data) { $this->id = idx($data, 'id'); $this->page = idx($data, 'page'); Index: src/applications/project/query/PhabricatorProjectQuery.php =================================================================== --- src/applications/project/query/PhabricatorProjectQuery.php +++ src/applications/project/query/PhabricatorProjectQuery.php @@ -114,50 +114,55 @@ ($row['viewerIsMember'] !== null)); } } + } - if ($this->needProfiles) { - $profiles = id(new PhabricatorProjectProfile())->loadAllWhere( - 'projectPHID IN (%Ls)', - mpull($projects, 'getPHID')); - $profiles = mpull($profiles, null, 'getProjectPHID'); - - $default = null; - - if ($profiles) { - $file_phids = mpull($profiles, 'getProfileImagePHID'); - $files = id(new PhabricatorFileQuery()) - ->setViewer($this->getViewer()) - ->withPHIDs($file_phids) - ->execute(); - $files = mpull($files, null, 'getPHID'); - foreach ($profiles as $profile) { - $file = idx($files, $profile->getProfileImagePHID()); - if (!$file) { - if (!$default) { - $default = PhabricatorFile::loadBuiltin( - $this->getViewer(), - 'profile.png'); - } - $file = $default; - } - $profile->attachProfileImageFile($file); - } - } + return $projects; + } - foreach ($projects as $project) { - $profile = idx($profiles, $project->getPHID()); - if (!$profile) { + protected function didFilterPage(array $projects) { + if ($this->needProfiles) { + $profiles = id(new PhabricatorProjectProfile())->loadAllWhere( + 'projectPHID IN (%Ls)', + mpull($projects, 'getPHID')); + $profiles = mpull($profiles, null, 'getProjectPHID'); + + $default = null; + + if ($profiles) { + $file_phids = mpull($profiles, 'getProfileImagePHID'); + $files = id(new PhabricatorFileQuery()) + ->setParentQuery($this) + ->setViewer($this->getViewer()) + ->withPHIDs($file_phids) + ->execute(); + $files = mpull($files, null, 'getPHID'); + foreach ($profiles as $profile) { + $file = idx($files, $profile->getProfileImagePHID()); + if (!$file) { if (!$default) { $default = PhabricatorFile::loadBuiltin( $this->getViewer(), 'profile.png'); } - $profile = id(new PhabricatorProjectProfile()) - ->setProjectPHID($project->getPHID()) - ->attachProfileImageFile($default); + $file = $default; + } + $profile->attachProfileImageFile($file); + } + } + + foreach ($projects as $project) { + $profile = idx($profiles, $project->getPHID()); + if (!$profile) { + if (!$default) { + $default = PhabricatorFile::loadBuiltin( + $this->getViewer(), + 'profile.png'); } - $project->attachProfile($profile); + $profile = id(new PhabricatorProjectProfile()) + ->setProjectPHID($project->getPHID()) + ->attachProfileImageFile($default); } + $project->attachProfile($profile); } } Index: src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php =================================================================== --- src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php +++ src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php @@ -33,6 +33,7 @@ private $parentQuery; private $rawResultLimit; private $capabilities; + private $workspace = array(); /* -( Query Configuration )------------------------------------------------ */ @@ -229,6 +230,11 @@ $visible = $filter->apply($maybe_visible); } + if ($visible) { + $this->putObjectsInWorkspace($this->getWorkspaceMapForPage($visible)); + $visible = $this->didFilterPage($visible); + } + $removed = array(); foreach ($maybe_visible as $key => $object) { if (empty($visible[$key])) { @@ -300,6 +306,108 @@ } +/* -( Query Workspace )---------------------------------------------------- */ + + + /** + * Put a map of objects into the query workspace. Many queries perform + * subqueries, which can eventually end up loading the same objects more than + * once (often to perform policy checks). + * + * For example, loading a user may load the user's profile image, which might + * load the user object again in order to verify that the viewer has + * permission to see the file. + * + * The "query workspace" allows queries to load objects from elsewhere in a + * query block instead of refetching them. + * + * When using the query workspace, it's important to obey two rules: + * + * **Never put objects into the workspace which the viewer may not be able + * to see**. You need to apply all policy filtering //before// putting + * objects in the workspace. Otherwise, subqueries may read the objects and + * use them to permit access to content the user shouldn't be able to view. + * + * **Fully enrich objects pulled from the workspace.** After pulling objects + * from the workspace, you still need to load and attach any additional + * content the query requests. Otherwise, a query might return objects without + * requested content. + * + * Generally, you do not need to update the workspace yourself: it is + * automatically populated as a side effect of objects surviving policy + * filtering. + * + * @param map Objects to add to the query + * workspace. + * @return this + * @task workspace + */ + public function putObjectsInWorkspace(array $objects) { + assert_instances_of($objects, 'PhabricatorPolicyInterface'); + + $viewer_phid = $this->getViewer()->getPHID(); + + // The workspace is scoped per viewer to prevent accidental contamination. + if (empty($this->workspace[$viewer_phid])) { + $this->workspace[$viewer_phid] = array(); + } + + $this->workspace[$viewer_phid] += $objects; + + return $this; + } + + + /** + * Retrieve objects from the query workspace. For more discussion about the + * workspace mechanism, see @{method:putObjectsInWorkspace}. This method + * searches both the current query's workspace and the workspaces of parent + * queries. + * + * @param list List of PHIDs to retreive. + * @return this + * @task workspace + */ + public function getObjectsFromWorkspace(array $phids) { + $viewer_phid = $this->getViewer()->getPHID(); + + $results = array(); + foreach ($phids as $key => $phid) { + if (isset($this->workspace[$viewer_phid][$phid])) { + $results[$phid] = $this->workspace[$viewer_phid][$phid]; + unset($phids[$key]); + } + } + + if ($phids && $this->getParentQuery()) { + $results += $this->getParentQuery()->getObjectsFromWorkspace($phids); + } + + return $results; + } + + + /** + * Convert a result page to a `` map. + * + * @param list Objects. + * @return map Map of objects which can + * be put into the workspace. + * @task workspace + */ + protected function getWorkspaceMapForPage(array $results) { + $map = array(); + foreach ($results as $result) { + $phid = $result->getPHID(); + if ($phid !== null) { + $map[$phid] = $result; + } + } + + return $map; + } + + /* -( Policy Query Implementation )---------------------------------------- */ @@ -353,7 +461,13 @@ /** * Hook for applying a page filter prior to the privacy filter. This allows * you to drop some items from the result set without creating problems with - * pagination or cursor updates. + * pagination or cursor updates. You can also load and attach data which is + * required to perform policy filtering. + * + * Generally, you should load non-policy data and perform non-policy filtering + * later, in @{method:didFilterPage}. Strictly fewer objects will make it that + * far (so the program will load less data) and subqueries from that context + * can use the query workspace to further reduce query load. * * This method will only be called if data is available. Implementations * do not need to handle the case of no results specially. @@ -366,6 +480,29 @@ return $page; } + /** + * Hook for performing additional non-policy loading or filtering after an + * object has satisfied all policy checks. Generally, this means loading and + * attaching related data. + * + * Subqueries executed during this phase can use the query workspace, which + * may improve performance or make circular policies resolvable. Data which + * is not necessary for policy filtering should generally be loaded here. + * + * This callback can still filter objects (for example, if attachable data + * is discovered to not exist), but should not do so for policy reasons. + * + * This method will only be called if data is available. Implementations do + * not need to handle the case of no results specially. + * + * @param list Results from @{method:willFilterPage()}. + * @return list Objects after additional + * non-policy processing. + */ + protected function didFilterPage(array $page) { + return $page; + } + /** * Hook for removing filtered results from alternate result sets. This