Make most repository reads policy-aware

Summary: Ref T603. This swaps almost all queries against the repository table over to be policy aware. Test Plan: - Made an audit comment on a commit. - Ran `save_lint.php`. - Looked up a commit with `diffusion.getcommits`. - Looked up lint messages with `diffusion.getlintmessages`. - Clicked an external/submodule in Diffusion. - Viewed main lint and repository lint in Diffusion. - Completed and validated Owners paths in Owners. - Executed dry runs via Herald. - Queried for package owners with `owners.query`. - Viewed Owners package. - Edited Owners package. - Viewed Owners package list. - Executed `repository.query`. - Viewed "Repository" tool repository list. - Edited Arcanist project. - Hit "Delete" on repository (this just tells you to use the CLI). - Created a repository. - Edited a repository. - Ran `bin/repository list`. - Ran `bin/search index rGTESTff45d13dffcfb3ea85b03aac8cc36251cacdf01c` - Pushed and parsed a commit. - Skipped all the Drydock stuff, as it it's hard to test and isn't normally reachable. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T603 Differential Revision: https://secure.phabricator.com/D7132
2025-01-20 11:41:08 +01:00 · 2013-09-25 16:54:48 -07:00 · 2013-09-25 16:54:48 -07:00 · c467cc464f
commit c467cc464f
parent b8154cb5e9
31 changed files with 123 additions and 87 deletions
--- a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php
+++ b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php
@ -403,8 +403,10 @@ final class PhabricatorAuditCommentEditor extends PhabricatorEditor {

    $prefix = PhabricatorEnv::getEnvConfig('metamta.diffusion.subject-prefix');

-    $repository = id(new PhabricatorRepository())
-      ->load($commit->getRepositoryID());
+    $repository = id(new PhabricatorRepositoryQuery())
+      ->setViewer($this->getActor())
+      ->withIDs(array($commit->getRepositoryID()))
+      ->executeOne();
    $threading = self::getMailThreading($repository, $commit);
    list($thread_id, $thread_topic) = $threading;

--- a/src/applications/differential/conduit/ConduitAPI_differential_query_Method.php
+++ b/src/applications/differential/conduit/ConduitAPI_differential_query_Method.php
@ -112,9 +112,10 @@ final class ConduitAPI_differential_query_Method
      foreach ($path_pairs as $pair) {
        list($callsign, $path) = $pair;
        if (!idx($repos, $callsign)) {
-          $repos[$callsign] = id(new PhabricatorRepository())->loadOneWhere(
-            'callsign = %s',
-            $callsign);
+          $repos[$callsign] = id(new PhabricatorRepositoryQuery())
+            ->setViewer($request->getUser())
+            ->withCallsigns(array($callsign))
+            ->executeOne();

          if (!$repos[$callsign]) {
            throw id(new ConduitException('ERR-INVALID-PARAMETER'))
--- a/src/applications/diffusion/DiffusionLintSaveRunner.php
+++ b/src/applications/diffusion/DiffusionLintSaveRunner.php
@ -228,8 +228,10 @@ final class DiffusionLintSaveRunner {


  private function blameAuthors() {
-    $repository = id(new PhabricatorRepository())->load(
-      $this->branch->getRepositoryID());
+    $repository = id(new PhabricatorRepositoryQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withIDs(array($this->branch->getRepositoryID()))
+      ->executeOne();

    $queries = array();
    $futures = array();
--- a/src/applications/diffusion/conduit/ConduitAPI_diffusion_getcommits_Method.php
+++ b/src/applications/diffusion/conduit/ConduitAPI_diffusion_getcommits_Method.php
@ -52,9 +52,10 @@ final class ConduitAPI_diffusion_getcommits_Method

    $callsigns = ipull($commits, 'callsign');
    $callsigns = array_unique($callsigns);
-    $repos = id(new PhabricatorRepository())->loadAllWhere(
-      'callsign IN (%Ls)',
-      $callsigns);
+    $repos = id(new PhabricatorRepositoryQuery())
+      ->setViewer($request->getUser())
+      ->withCallsigns($callsigns)
+      ->execute();
    $repos = mpull($repos, null, 'getCallsign');

    foreach ($commits as $name => $info) {
--- a/src/applications/diffusion/conduit/ConduitAPI_diffusion_getlintmessages_Method.php
+++ b/src/applications/diffusion/conduit/ConduitAPI_diffusion_getlintmessages_Method.php
@ -42,8 +42,10 @@ final class ConduitAPI_diffusion_getlintmessages_Method

    $branch_name = $request->getValue('branch');
    if ($branch_name == '') {
-      $repository = id(new PhabricatorRepository())
-        ->load($project->getRepositoryID());
+      $repository = id(new PhabricatorRepositoryQuery())
+        ->setViewer($request->getUser())
+        ->withIDs(array($project->getRepositoryID()))
+        ->executeOne();
      $branch_name = $repository->getDefaultArcanistBranch();
    }

--- a/src/applications/diffusion/controller/DiffusionExternalController.php
+++ b/src/applications/diffusion/controller/DiffusionExternalController.php
@ -12,7 +12,9 @@ final class DiffusionExternalController extends DiffusionController {
    $uri = $request->getStr('uri');
    $id  = $request->getStr('id');

-    $repositories = id(new PhabricatorRepository())->loadAll();
+    $repositories = id(new PhabricatorRepositoryQuery())
+      ->setViewer($request->getUser())
+      ->execute();

    if ($uri) {
      $uri_path = id(new PhutilURI($uri))->getPath();
--- a/src/applications/diffusion/controller/DiffusionLintController.php
+++ b/src/applications/diffusion/controller/DiffusionLintController.php
@ -221,9 +221,10 @@ final class DiffusionLintController extends DiffusionController {
      }

      if ($paths) {
-        $repositories = id(new PhabricatorRepository())->loadAllWhere(
-          'phid IN (%Ls)',
-          array_unique(mpull($paths, 'getRepositoryPHID')));
+        $repositories = id(new PhabricatorRepositoryQuery())
+          ->setViewer($this->getRequest()->getUser())
+          ->withPHIDs(mpull($paths, 'getRepositoryPHID'))
+          ->execute();
        $repositories = mpull($repositories, 'getID', 'getPHID');

        $branches = id(new PhabricatorRepositoryBranch())->loadAllWhere(
@ -233,7 +234,11 @@ final class DiffusionLintController extends DiffusionController {
      }

      foreach ($paths as $path) {
-        $branch = idx($branches, $repositories[$path->getRepositoryPHID()]);
+        $branch = idx(
+          $branches,
+          idx(
+            $repositories,
+            $path->getRepositoryPHID()));
        if ($branch) {
          $condition = qsprintf(
            $conn,
--- a/src/applications/diffusion/controller/DiffusionPathCompleteController.php
+++ b/src/applications/diffusion/controller/DiffusionPathCompleteController.php
@ -10,9 +10,10 @@ final class DiffusionPathCompleteController extends DiffusionController {
    $request = $this->getRequest();

    $repository_phid = $request->getStr('repositoryPHID');
-    $repository = id(new PhabricatorRepository())->loadOneWhere(
-      'phid = %s',
-      $repository_phid);
+    $repository = id(new PhabricatorRepositoryQuery())
+      ->setViewer($request->getUser())
+      ->withPHIDs(array($repository_phid))
+      ->executeOne();
    if (!$repository) {
      return new Aphront400Response();
    }
--- a/src/applications/diffusion/controller/DiffusionPathValidateController.php
+++ b/src/applications/diffusion/controller/DiffusionPathValidateController.php
@ -10,9 +10,10 @@ final class DiffusionPathValidateController extends DiffusionController {
    $request = $this->getRequest();

    $repository_phid = $request->getStr('repositoryPHID');
-    $repository = id(new PhabricatorRepository())->loadOneWhere(
-      'phid = %s',
-      $repository_phid);
+    $repository = id(new PhabricatorRepositoryQuery())
+      ->setViewer($request->getUser())
+      ->withPHIDs(array($repository_phid))
+      ->executeOne();
    if (!$repository) {
      return new Aphront400Response();
    }
--- a/src/applications/diffusion/query/DiffusionSymbolQuery.php
+++ b/src/applications/diffusion/query/DiffusionSymbolQuery.php
@ -265,6 +265,7 @@ final class DiffusionSymbolQuery extends PhabricatorOffsetPagedQuery {
    $repo_ids = array_filter($repo_ids);

    if ($repo_ids) {
+      // TODO: (T603) Provide a viewer here.
      $repos = id(new PhabricatorRepository())->loadAllWhere(
        'id IN (%Ld)',
        $repo_ids);
--- a/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprint.php
+++ b/src/applications/drydock/blueprint/DrydockWorkingCopyBlueprint.php
@ -31,6 +31,8 @@ final class DrydockWorkingCopyBlueprint extends DrydockBlueprint {
        "Lease is missing required 'repositoryID' attribute.");
    }

+    // TODO: (T603) Figure out the interaction between policies and
+    // Drydock.
    $repository = id(new PhabricatorRepository())->load($repository_id);

    if (!$repository) {
--- a/src/applications/harbormaster/worker/HarbormasterRunnerWorker.php
+++ b/src/applications/harbormaster/worker/HarbormasterRunnerWorker.php
@ -19,6 +19,7 @@ final class HarbormasterRunnerWorker extends PhabricatorWorker {
        "Commit '{$id}' does not exist!");
    }

+    // TODO: (T603) Policy interaction?
    $repository = id(new PhabricatorRepository())->loadOneWhere(
      'id = %d',
      $commit->getRepositoryID());
--- a/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
+++ b/src/applications/herald/adapter/HeraldDifferentialRevisionAdapter.php
@ -110,6 +110,8 @@ final class HeraldDifferentialRevisionAdapter extends HeraldAdapter {

      $repository = false;

+      // TODO: (T603) Implement policy stuff in Herald.
+
      if ($diff->getRepositoryUUID()) {
        $repository = id(new PhabricatorRepository())->loadOneWhere(
          'uuid = %s',
--- a/src/applications/herald/controller/HeraldTestConsoleController.php
+++ b/src/applications/herald/controller/HeraldTestConsoleController.php
@ -20,35 +20,14 @@ final class HeraldTestConsoleController extends HeraldController {
      }

      if (!$errors) {
-        $matches = null;
-        $object = null;
-        if (preg_match('/^D(\d+)$/', $object_name, $matches)) {
-          $object = id(new DifferentialRevision())->load($matches[1]);
+        $object = id(new PhabricatorObjectQuery())
+          ->setViewer($user)
+          ->withNames(array($object_name))
+          ->executeOne();
+
        if (!$object) {
          $e_name = pht('Invalid');
-            $errors[] = pht('No Differential Revision with that ID exists.');
-          }
-        } else if (preg_match('/^r([A-Z]+)(\w+)$/', $object_name, $matches)) {
-          $repo = id(new PhabricatorRepository())->loadOneWhere(
-            'callsign = %s',
-            $matches[1]);
-          if (!$repo) {
-            $e_name = pht('Invalid');
-            $errors[] = pht('There is no repository with the callsign: %s.',
-              $matches[1]);
-          }
-          $commit = id(new PhabricatorRepositoryCommit())->loadOneWhere(
-            'repositoryID = %d AND commitIdentifier = %s',
-            $repo->getID(),
-            $matches[2]);
-          if (!$commit) {
-            $e_name = pht('Invalid');
-            $errors[] = pht('There is no commit with that identifier.');
-          }
-          $object = $commit;
-        } else {
-          $e_name = pht('Invalid');
-          $errors[] = pht('This object name is not recognized.');
+          $errors[] = pht('No object exists with that name.');
        }

        if (!$errors) {
@ -61,7 +40,7 @@ final class HeraldTestConsoleController extends HeraldController {
              'commitID = %d',
              $object->getID());
            $adapter = HeraldCommitAdapter::newLegacyAdapter(
-              $repo,
+              $object->getRepository(),
              $object,
              $data);
          } else {
--- a/src/applications/owners/conduit/ConduitAPI_owners_query_Method.php
+++ b/src/applications/owners/conduit/ConduitAPI_owners_query_Method.php
@ -65,11 +65,17 @@ final class ConduitAPI_owners_query_Method
    return $packages;
  }

-  private static function queryByPath($repo_callsign, $path) {
-    $repository = id(new PhabricatorRepository())->loadOneWhere('callsign = %s',
-      $repo_callsign);
+  private static function queryByPath(
+    PhabricatorUser $viewer,
+    $repo_callsign,
+    $path) {

-    if (empty($repository)) {
+    $repository = id(new PhabricatorRepositoryQuery())
+      ->setViewer($viewer)
+      ->withCallsigns(array($repo_callsign))
+      ->executeOne();
+
+    if (!$repository) {
      throw id(new ConduitException('ERR_REP_NOT_FOUND'))
        ->setErrorDescription(
          'Repository callsign '.$repo_callsign.' not recognized');
@ -144,7 +150,7 @@ final class ConduitAPI_owners_query_Method
      $packages = self::queryByOwner($owner);

    } else if ($is_path_query) {
-      $packages = self::queryByPath($repo, $path);
+      $packages = self::queryByPath($request->getUser(), $repo, $path);
    }

    return self::buildPackageInformationDictionaries($packages);
--- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php
+++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php
@ -29,9 +29,10 @@ final class PhabricatorOwnersDetailController
    }

    if ($repository_phids) {
-      $repositories = id(new PhabricatorRepository())->loadAllWhere(
-        'phid in (%Ls)',
-        array_keys($repository_phids));
+      $repositories = id(new PhabricatorRepositoryQuery())
+        ->setViewer($user)
+        ->withPHIDs(array_keys($repository_phids))
+        ->execute();
      $repositories = mpull($repositories, null, 'getPHID');
    } else {
      $repositories = array();
--- a/src/applications/owners/controller/PhabricatorOwnersEditController.php
+++ b/src/applications/owners/controller/PhabricatorOwnersEditController.php
@ -142,7 +142,9 @@ final class PhabricatorOwnersEditController
    }
    $this->setSideNavFilter($side_nav_filter);

-    $repos = id(new PhabricatorRepository())->loadAll();
+    $repos = id(new PhabricatorRepositoryQuery())
+      ->setViewer($user)
+      ->execute();

    $default_paths = array();
    foreach ($repos as $repo) {
--- a/src/applications/owners/controller/PhabricatorOwnersListController.php
+++ b/src/applications/owners/controller/PhabricatorOwnersListController.php
@ -21,8 +21,10 @@ final class PhabricatorOwnersListController

    $repository_phid = '';
    if ($request->getStr('repository') != '') {
-      $repository_phid = id(new PhabricatorRepository())
-        ->loadOneWhere('callsign = %s', $request->getStr('repository'))
+      $repository_phid = id(new PhabricatorRepositoryQuery())
+        ->setViewer($user)
+        ->withCallsigns(array($request->getStr('repository')))
+        ->executeOne()
        ->getPHID();
    }

@ -157,8 +159,10 @@ final class PhabricatorOwnersListController
    }

    $callsigns = array('' => pht('(Any Repository)'));
-    $repositories = id(new PhabricatorRepository())
-      ->loadAllWhere('1 = 1 ORDER BY callsign');
+    $repositories = id(new PhabricatorRepositoryQuery())
+      ->setViewer($user)
+      ->setOrder(PhabricatorRepositoryQuery::ORDER_CALLSIGN)
+      ->execute();
    foreach ($repositories as $repository) {
      $callsigns[$repository->getCallsign()] =
        $repository->getCallsign().': '.$repository->getName();
@ -238,9 +242,10 @@ final class PhabricatorOwnersListController
      }

      if ($repository_phids) {
-        $repositories = id(new PhabricatorRepository())->loadAllWhere(
-          'phid in (%Ls)',
-          array_keys($repository_phids));
+        $repositories = id(new PhabricatorRepositoryQuery())
+          ->setViewer($this->getRequest()->getUser())
+          ->withPHIDs(array_keys($repository_phids))
+          ->execute();
      } else {
        $repositories = array();
      }
--- a/src/applications/owners/storage/PhabricatorOwnersPackage.php
+++ b/src/applications/owners/storage/PhabricatorOwnersPackage.php
@ -297,6 +297,8 @@ final class PhabricatorOwnersPackage extends PhabricatorOwnersDAO

      $cur_paths = mgroup($cur_paths, 'getRepositoryPHID', 'getPath');
      foreach ($new_paths as $repository_phid => $paths) {
+        // TODO: (T603) Thread policy stuff in here.
+
        // get repository object for path validation
        $repository = id(new PhabricatorRepository())->loadOneWhere(
          'phid = %s',
--- a/src/applications/releeph/storage/ReleephProject.php
+++ b/src/applications/releeph/storage/ReleephProject.php
@ -113,7 +113,7 @@ final class ReleephProject extends ReleephDAO
    return $this->assertAttached($this->repository);
  }

-  // TODO: Remove once everything uses ProjectQuery.
+  // TODO: Remove once everything uses ProjectQuery. Also, T603.
  public function loadPhabricatorRepository() {
    return $this->loadOneRelative(
      new PhabricatorRepository(),
--- a/src/applications/repository/conduit/ConduitAPI_repository_query_Method.php
+++ b/src/applications/repository/conduit/ConduitAPI_repository_query_Method.php
@ -33,7 +33,9 @@ final class ConduitAPI_repository_query_Method
  }

  protected function execute(ConduitAPIRequest $request) {
-    $repositories = id(new PhabricatorRepository())->loadAll();
+    $repositories = id(new PhabricatorRepositoryQuery())
+      ->setViewer($request->getUser())
+      ->execute();

    $results = array();
    foreach ($repositories as $repository) {
--- a/src/applications/repository/controller/PhabricatorRepositoryArcanistProjectEditController.php
+++ b/src/applications/repository/controller/PhabricatorRepositoryArcanistProjectEditController.php
@ -19,7 +19,9 @@ final class PhabricatorRepositoryArcanistProjectEditController
      return new Aphront404Response();
    }

-    $repositories = id(new PhabricatorRepository())->loadAll();
+    $repositories = id(new PhabricatorRepositoryQuery())
+      ->setViewer($user)
+      ->execute();
    $repos = array(
      0 => 'None',
    );
--- a/src/applications/repository/controller/PhabricatorRepositoryDeleteController.php
+++ b/src/applications/repository/controller/PhabricatorRepositoryDeleteController.php
@ -10,8 +10,12 @@ final class PhabricatorRepositoryDeleteController
  }

  public function processRequest() {
+    $viewer = $this->getRequest()->getUser();

-    $repository = id(new PhabricatorRepository())->load($this->id);
+    $repository = id(new PhabricatorRepositoryQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($this->id))
+      ->executeOne();
    if (!$repository) {
      return new Aphront404Response();
    }
--- a/src/applications/repository/controller/PhabricatorRepositoryEditController.php
+++ b/src/applications/repository/controller/PhabricatorRepositoryEditController.php
@ -16,8 +16,12 @@ final class PhabricatorRepositoryEditController
  public function processRequest() {

    $request = $this->getRequest();
+    $viewer = $request->getUser();

-    $repository = id(new PhabricatorRepository())->load($this->id);
+    $repository = id(new PhabricatorRepositoryQuery())
+      ->setViewer($viewer)
+      ->withIDs(array($this->id))
+      ->executeOne();
    if (!$repository) {
      return new Aphront404Response();
    }
--- a/src/applications/repository/controller/PhabricatorRepositoryListController.php
+++ b/src/applications/repository/controller/PhabricatorRepositoryListController.php
@ -3,17 +3,15 @@
 final class PhabricatorRepositoryListController
  extends PhabricatorRepositoryController {

-  public function shouldRequireAdmin() {
-    return false;
-  }
-
  public function processRequest() {

    $request = $this->getRequest();
    $user = $request->getUser();
    $is_admin = $user->getIsAdmin();

-    $repos = id(new PhabricatorRepository())->loadAll();
+    $repos = id(new PhabricatorRepositoryQuery())
+      ->setViewer($user)
+      ->execute();
    $repos = msort($repos, 'getName');

    $rows = array();
--- a/src/applications/repository/management/PhabricatorRepositoryManagementListWorkflow.php
+++ b/src/applications/repository/management/PhabricatorRepositoryManagementListWorkflow.php
@ -13,7 +13,9 @@ final class PhabricatorRepositoryManagementListWorkflow
  public function execute(PhutilArgumentParser $args) {
    $console = PhutilConsole::getConsole();

-    $repos = id(new PhabricatorRepository())->loadAll();
+    $repos = id(new PhabricatorRepositoryQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->execute();
    if ($repos) {
      foreach ($repos as $repo) {
        $console->writeOut("%s\n", $repo->getCallsign());
--- a/src/applications/repository/search/PhabricatorRepositoryCommitSearchIndexer.php
+++ b/src/applications/repository/search/PhabricatorRepositoryCommitSearchIndexer.php
@ -17,10 +17,10 @@ final class PhabricatorRepositoryCommitSearchIndexer
    $commit_message = $commit_data->getCommitMessage();
    $author_phid = $commit_data->getCommitDetail('authorPHID');

-    $repository = id(new PhabricatorRepository())->loadOneWhere(
-      'id = %d',
-      $commit->getRepositoryID());
-
+    $repository = id(new PhabricatorRepositoryQuery())
+      ->setViewer($this->getViewer())
+      ->withIDs(array($commit->getRepositoryID()))
+      ->executeOne();
    if (!$repository) {
      throw new Exception("No such repository!");
    }
--- a/src/applications/repository/storage/PhabricatorRepository.php
+++ b/src/applications/repository/storage/PhabricatorRepository.php
@ -452,6 +452,8 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO
  }

  public static function loadAllByPHIDOrCallsign(array $names) {
+    // TODO: (T603) Get rid of this.
+
    $repositories = array();
    foreach ($names as $name) {
      $repo = id(new PhabricatorRepository())->loadOneWhere(
--- a/src/applications/repository/storage/PhabricatorRepositoryArcanistProject.php
+++ b/src/applications/repository/storage/PhabricatorRepositoryArcanistProject.php
@ -32,7 +32,7 @@ final class PhabricatorRepositoryArcanistProject
      PhabricatorRepositoryPHIDTypeArcanistProject::TYPECONST);
  }

-  // TODO: Remove.
+  // TODO: Remove. Also, T603.
  public function loadRepository() {
    if (!$this->getRepositoryID()) {
      return null;
--- a/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php
+++ b/src/applications/repository/worker/PhabricatorRepositoryCommitParserWorker.php
@ -31,9 +31,10 @@ abstract class PhabricatorRepositoryCommitParserWorker
      return;
    }

-    $repository = id(new PhabricatorRepository())->load(
-      $this->commit->getRepositoryID());
-
+    $repository = id(new PhabricatorRepositoryQuery())
+      ->setViewer(PhabricatorUser::getOmnipotentUser())
+      ->withIDs(array($this->commit->getRepositoryID()))
+      ->executeOne();
    if (!$repository) {
      return;
    }
@ -92,6 +93,8 @@ abstract class PhabricatorRepositoryCommitParserWorker
      return $suffix;
    }

+    // TODO: (T603) This method should probably take a viewer.
+
    $repository = id(new PhabricatorRepository())
      ->load($commit->getRepositoryID());
    $link = DiffusionView::linkCommit($repository,
--- a/src/applications/repository/worker/commitchangeparser/PhabricatorOwnersPackagePathValidator.php
+++ b/src/applications/repository/worker/commitchangeparser/PhabricatorOwnersPackagePathValidator.php
@ -15,6 +15,7 @@ final class PhabricatorOwnersPackagePathValidator {
      return;
    }

+    // TODO: (T603) This should be policy-aware.
    $repository =
      id(new PhabricatorRepository())->load($commit->getRepositoryID());
    $move_map = array();