diff --git a/src/applications/files/query/PhabricatorFileQuery.php b/src/applications/files/query/PhabricatorFileQuery.php index fd859a8fbe..28393b4502 100644 --- a/src/applications/files/query/PhabricatorFileQuery.php +++ b/src/applications/files/query/PhabricatorFileQuery.php @@ -128,12 +128,14 @@ final class PhabricatorFileQuery $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(); diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php index 51e2a55fca..37bd60af16 100644 --- a/src/applications/files/storage/PhabricatorFile.php +++ b/src/applications/files/storage/PhabricatorFile.php @@ -764,8 +764,10 @@ final class PhabricatorFile extends PhabricatorFileDAO ); } + // NOTE: Anyone is allowed to access builtin files. + $files = id(new PhabricatorFileQuery()) - ->setViewer($user) + ->setViewer(PhabricatorUser::getOmnipotentUser()) ->withTransforms($specs) ->execute(); diff --git a/src/applications/macro/query/PhabricatorMacroQuery.php b/src/applications/macro/query/PhabricatorMacroQuery.php index aa1a22fcdc..da7dbd7b83 100644 --- a/src/applications/macro/query/PhabricatorMacroQuery.php +++ b/src/applications/macro/query/PhabricatorMacroQuery.php @@ -191,10 +191,11 @@ final class PhabricatorMacroQuery 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'); diff --git a/src/applications/paste/query/PhabricatorPasteQuery.php b/src/applications/paste/query/PhabricatorPasteQuery.php index d05f78b457..101ceb84ed 100644 --- a/src/applications/paste/query/PhabricatorPasteQuery.php +++ b/src/applications/paste/query/PhabricatorPasteQuery.php @@ -87,7 +87,7 @@ final class PhabricatorPasteQuery return $pastes; } - protected function willFilterPage(array $pastes) { + protected function didFilterPage(array $pastes) { if ($this->needRawContent) { $pastes = $this->loadRawContent($pastes); } @@ -163,6 +163,7 @@ final class PhabricatorPasteQuery private function loadRawContent(array $pastes) { $file_phids = mpull($pastes, 'getFilePHID'); $files = id(new PhabricatorFileQuery()) + ->setParentQuery($this) ->setViewer($this->getViewer()) ->withPHIDs($file_phids) ->execute(); diff --git a/src/applications/people/query/PhabricatorPeopleQuery.php b/src/applications/people/query/PhabricatorPeopleQuery.php index 1f0c12bfc0..39613c5011 100644 --- a/src/applications/people/query/PhabricatorPeopleQuery.php +++ b/src/applications/people/query/PhabricatorPeopleQuery.php @@ -113,8 +113,10 @@ final class PhabricatorPeopleQuery $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 @@ final class PhabricatorPeopleQuery $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(); diff --git a/src/applications/phid/query/PhabricatorObjectQuery.php b/src/applications/phid/query/PhabricatorObjectQuery.php index 1e492fe5d9..a6ffa02e7e 100644 --- a/src/applications/phid/query/PhabricatorObjectQuery.php +++ b/src/applications/phid/query/PhabricatorObjectQuery.php @@ -104,13 +104,27 @@ final class PhabricatorObjectQuery } 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); diff --git a/src/applications/pholio/query/PholioImageQuery.php b/src/applications/pholio/query/PholioImageQuery.php index b619e25739..71bdd3ab0e 100644 --- a/src/applications/pholio/query/PholioImageQuery.php +++ b/src/applications/pholio/query/PholioImageQuery.php @@ -103,33 +103,6 @@ final class PholioImageQuery protected function willFilterPage(array $images) { assert_instances_of($images, 'PholioImage'); - $file_phids = mpull($images, 'getFilePHID'); - - $all_files = id(new PhabricatorFileQuery()) - ->setViewer($this->getViewer()) - ->withPHIDs($file_phids) - ->execute(); - $all_files = mpull($all_files, null, 'getPHID'); - - if ($this->needInlineComments) { - $all_inline_comments = id(new PholioTransactionComment()) - ->loadAllWhere('imageid IN (%Ld)', - mpull($images, 'getID')); - $all_inline_comments = mgroup($all_inline_comments, 'getImageID'); - } - - foreach ($images as $image) { - $file = idx($all_files, $image->getFilePHID()); - if (!$file) { - $file = PhabricatorFile::loadBuiltin($this->getViewer(), 'missing.png'); - } - $image->attachFile($file); - if ($this->needInlineComments) { - $inlines = idx($all_inline_comments, $image->getID(), array()); - $image->attachInlineComments($inlines); - } - } - if ($this->getMockCache()) { $mocks = $this->getMockCache(); } else { @@ -154,4 +127,38 @@ final class PholioImageQuery 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(); + $all_files = mpull($all_files, null, 'getPHID'); + + if ($this->needInlineComments) { + $all_inline_comments = id(new PholioTransactionComment()) + ->loadAllWhere('imageid IN (%Ld)', + mpull($images, 'getID')); + $all_inline_comments = mgroup($all_inline_comments, 'getImageID'); + } + + foreach ($images as $image) { + $file = idx($all_files, $image->getFilePHID()); + if (!$file) { + $file = PhabricatorFile::loadBuiltin($this->getViewer(), 'missing.png'); + } + $image->attachFile($file); + if ($this->needInlineComments) { + $inlines = idx($all_inline_comments, $image->getID(), array()); + $image->attachInlineComments($inlines); + } + } + + return $images; + } + } diff --git a/src/applications/project/controller/PhabricatorProjectProfileController.php b/src/applications/project/controller/PhabricatorProjectProfileController.php index d7c9b2e073..6174b6e795 100644 --- a/src/applications/project/controller/PhabricatorProjectProfileController.php +++ b/src/applications/project/controller/PhabricatorProjectProfileController.php @@ -6,6 +6,10 @@ final class PhabricatorProjectProfileController private $id; private $page; + public function shouldAllowPublic() { + return true; + } + public function willProcessRequest(array $data) { $this->id = idx($data, 'id'); $this->page = idx($data, 'page'); diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php index 00f6ead387..e379a48377 100644 --- a/src/applications/project/query/PhabricatorProjectQuery.php +++ b/src/applications/project/query/PhabricatorProjectQuery.php @@ -114,51 +114,56 @@ final class PhabricatorProjectQuery ($row['viewerIsMember'] !== null)); } } + } - if ($this->needProfiles) { - $profiles = id(new PhabricatorProjectProfile())->loadAllWhere( - 'projectPHID IN (%Ls)', - mpull($projects, 'getPHID')); - $profiles = mpull($profiles, null, 'getProjectPHID'); + return $projects; + } - $default = null; + protected function didFilterPage(array $projects) { + if ($this->needProfiles) { + $profiles = id(new PhabricatorProjectProfile())->loadAllWhere( + 'projectPHID IN (%Ls)', + mpull($projects, 'getPHID')); + $profiles = mpull($profiles, null, 'getProjectPHID'); - 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); - } - } + $default = null; - foreach ($projects as $project) { - $profile = idx($profiles, $project->getPHID()); - if (!$profile) { + 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; } - $project->attachProfile($profile); + $profile->attachProfileImageFile($file); } } + + foreach ($projects as $project) { + $profile = idx($profiles, $project->getPHID()); + if (!$profile) { + if (!$default) { + $default = PhabricatorFile::loadBuiltin( + $this->getViewer(), + 'profile.png'); + } + $profile = id(new PhabricatorProjectProfile()) + ->setProjectPHID($project->getPHID()) + ->attachProfileImageFile($default); + } + $project->attachProfile($profile); + } } return $projects; diff --git a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php index f985d43e67..ad0d7b2a33 100644 --- a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php @@ -33,6 +33,7 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { private $parentQuery; private $rawResultLimit; private $capabilities; + private $workspace = array(); /* -( Query Configuration )------------------------------------------------ */ @@ -229,6 +230,11 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { $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 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { } +/* -( 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 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { /** * 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 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery { 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