From d61c931c7b384bed025418abcbf8f643e7a33a5e Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 26 Sep 2013 12:36:45 -0700 Subject: [PATCH] Use Differential policy columns to drive policies Summary: Ref T603. Read policies out of policy columns. When a revision is associated with a repository (which is currently never), require view access on the repository to see the revision (or, require the viewer to be the owner). This is a blanket "do the right thing" rule which should make Differential's default policies align with user expectations. Future diffs will populate the `repositoryPHID` when a revision is created. Test Plan: Tooled around Differential. None of this stuff does anything yet, so nothing very exciting happened. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T603 Differential Revision: https://secure.phabricator.com/D7134 --- .../query/DifferentialRevisionQuery.php | 53 +++++++++++++++++++ .../storage/DifferentialRevision.php | 27 +++++++++- 2 files changed, 79 insertions(+), 1 deletion(-) diff --git a/src/applications/differential/query/DifferentialRevisionQuery.php b/src/applications/differential/query/DifferentialRevisionQuery.php index 85013e4a65..85f8e28a5c 100644 --- a/src/applications/differential/query/DifferentialRevisionQuery.php +++ b/src/applications/differential/query/DifferentialRevisionQuery.php @@ -361,6 +361,59 @@ final class DifferentialRevisionQuery } public function willFilterPage(array $revisions) { + $viewer = $this->getViewer(); + + $repository_phids = mpull($revisions, 'getRepositoryPHID'); + $repository_phids = array_filter($repository_phids); + + $repositories = array(); + if ($repository_phids) { + $repositories = id(new PhabricatorRepositoryQuery()) + ->setViewer($this->getViewer()) + ->withPHIDs($repository_phids) + ->execute(); + $repositories = mpull($repositories, null, 'getPHID'); + } + + // If a revision is associated with a repository: + // + // - the viewer must be able to see the repository; or + // - the viewer must have an automatic view capability. + // + // In the latter case, we'll load the revision but not load the repository. + + $can_view = PhabricatorPolicyCapability::CAN_VIEW; + foreach ($revisions as $key => $revision) { + $repo_phid = $revision->getRepositoryPHID(); + if (!$repo_phid) { + // The revision has no associated repository. Attach `null` and move on. + $revision->attachRepository(null); + continue; + } + + $repository = idx($repositories, $repo_phid); + if ($repository) { + // The revision has an associated repository, and the viewer can see + // it. Attach it and move on. + $revision->attachRepository($repository); + continue; + } + + if ($revision->hasAutomaticCapability($can_view, $viewer)) { + // The revision has an associated repository which the viewer can not + // see, but the viewer has an automatic capability on this revision. + // Load the revision without attaching a repository. + $revision->attachRepository(null); + continue; + } + + // The revision has an associated repository, and the viewer can't see + // it, and the viewer has no special capabilities. Filter out this + // revision. + unset($revisions[$key]); + } + + $table = new DifferentialRevision(); $conn_r = $table->establishConnection('r'); diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index ee98cc5dc0..ae768b8a6a 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -34,6 +34,7 @@ final class DifferentialRevision extends DifferentialDAO private $activeDiff = self::ATTACHABLE; private $diffIDs = self::ATTACHABLE; private $hashes = self::ATTACHABLE; + private $repository = self::ATTACHABLE; private $reviewerStatus = self::ATTACHABLE; @@ -306,10 +307,25 @@ final class DifferentialRevision extends DifferentialDAO } public function getPolicy($capability) { - return PhabricatorPolicies::POLICY_USER; + switch ($capability) { + case PhabricatorPolicyCapability::CAN_VIEW: + return $this->getViewPolicy(); + case PhabricatorPolicyCapability::CAN_EDIT: + return $this->getEditPolicy(); + } } public function hasAutomaticCapability($capability, PhabricatorUser $user) { + + // A revision's author (which effectively means "owner" after we added + // commandeering) can always view and edit it. + $author_phid = $this->getAuthorPHID(); + if ($author_phid) { + if ($user->getPHID() == $author_phid) { + return true; + } + } + return false; } @@ -329,4 +345,13 @@ final class DifferentialRevision extends DifferentialDAO $this->reviewerStatus = $reviewers; return $this; } + + public function getRepository() { + return $this->assertAttached($this->repository); + } + + public function attachRepository(PhabricatorRepository $repository = null) { + $this->repository = $repository; + return $this; + } }