Fix some file policy issues and add a "Query Workspace"

Summary:
Ref T603. Several issues here:

  1. Currently, `FileQuery` does not actually respect object attachment edges when doing policy checks. Everything else works fine, but this was missing an `array_keys()`.
  2. Once that's fixed, we hit a bunch of recursion issues. For example, when loading a User we load the profile picture, and then that loads the User, and that loads the profile picture, etc.
  3. Introduce a "Query Workspace", which holds objects we know we've loaded and know we can see but haven't finished filtering and/or attaching data to. This allows subqueries to look up objects instead of querying for them.
    - We can probably generalize this a bit to make a few other queries more efficient. Pholio currently has a similar (but less general) "mock cache". However, it's keyed by ID instead of PHID so it's not easy to reuse this right now.

This is a bit complex for the problem being solved, but I think it's the cleanest approach and I believe the primitive will be useful in the future.

Test Plan: Looked at pastes, macros, mocks and projects as a logged-in and logged-out user.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T603

Differential Revision: https://secure.phabricator.com/D7309

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();

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();
 

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');

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();

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();

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);

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

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');

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;

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<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 @@ 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<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