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

Author: epriestley

src/applications/files/query/PhabricatorFileQuery.php

@@ -128,12 +128,14 @@ protected function loadPage() {
         $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 @@ public static function loadBuiltins(PhabricatorUser $user, array $names) {
       );
     }
 
+    // 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 @@ protected function buildWhereClause(AphrontDatabaseConnection $conn) {
     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 @@ protected function loadPage() {
     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 getContentCacheKey(PhabricatorPaste $paste) {
   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 @@ public function loadPage() {
       $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 @@ public function loadPage() {
       $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 @@ private function loadObjectsByName(array $types, array $names) {
   }
 
   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,9 +103,37 @@ private function buildWhereClause(AphrontDatabaseConnection $conn_r) {
   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 @@ protected function willFilterPage(array $images) {
       }
     }
 
-    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;
   }
 

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,50 +114,55 @@ protected function loadPage() {
             ($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);
       }
     }