Page MenuHomePhabricator
Files F72495

D7309.diff
All Users
Actions

D7309.diff
View Options

diff --git a/src/applications/files/query/PhabricatorFileQuery.php b/src/applications/files/query/PhabricatorFileQuery.php
--- a/src/applications/files/query/PhabricatorFileQuery.php
+++ b/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();
diff --git a/src/applications/files/storage/PhabricatorFile.php b/src/applications/files/storage/PhabricatorFile.php
--- a/src/applications/files/storage/PhabricatorFile.php
+++ b/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();
diff --git a/src/applications/macro/query/PhabricatorMacroQuery.php b/src/applications/macro/query/PhabricatorMacroQuery.php
--- a/src/applications/macro/query/PhabricatorMacroQuery.php
+++ b/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');
diff --git a/src/applications/paste/query/PhabricatorPasteQuery.php b/src/applications/paste/query/PhabricatorPasteQuery.php
--- a/src/applications/paste/query/PhabricatorPasteQuery.php
+++ b/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();
diff --git a/src/applications/people/query/PhabricatorPeopleQuery.php b/src/applications/people/query/PhabricatorPeopleQuery.php
--- a/src/applications/people/query/PhabricatorPeopleQuery.php
+++ b/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();
diff --git a/src/applications/phid/query/PhabricatorObjectQuery.php b/src/applications/phid/query/PhabricatorObjectQuery.php
--- a/src/applications/phid/query/PhabricatorObjectQuery.php
+++ b/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);
diff --git a/src/applications/pholio/query/PholioImageQuery.php b/src/applications/pholio/query/PholioImageQuery.php
--- a/src/applications/pholio/query/PholioImageQuery.php
+++ b/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;
}
diff --git a/src/applications/project/controller/PhabricatorProjectProfileController.php b/src/applications/project/controller/PhabricatorProjectProfileController.php
--- a/src/applications/project/controller/PhabricatorProjectProfileController.php
+++ b/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');
diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php
--- a/src/applications/project/query/PhabricatorProjectQuery.php
+++ b/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);
}
}
diff --git a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
--- a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
+++ b/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<phid, PhabricatorPolicyInterface> 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<phid> 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 `<phid, PhabricatorPolicyInterface>` map.
+ *
+ * @param list<PhabricatorPolicyInterface> Objects.
+ * @return map<phid, PhabricatorPolicyInterface> 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<wild> Results from @{method:willFilterPage()}.
+ * @return list<PhabricatorPolicyInterface> Objects after additional
+ * non-policy processing.
+ */
+ protected function didFilterPage(array $page) {
+ return $page;
+ }
+
/**
* Hook for removing filtered results from alternate result sets. This

File Metadata

Mime Type
text/x-diff
Storage Engine
amazon-s3
Storage Format
Raw Data
Storage Handle
phabricator/bv/jy/qjww5wnyh2grt4gs
Default Alt Text
D7309.diff (16 KB)

Event Timeline

Phabricator · Built by Phacility