diff --git a/src/applications/audit/editor/PhabricatorAuditEditor.php b/src/applications/audit/editor/PhabricatorAuditEditor.php --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -59,10 +59,6 @@ $this->oldAuditStatus = $object->getAuditStatus(); - $object->loadAndAttachAuditAuthority( - $this->getActor(), - $this->getActingAsPHID()); - return parent::expandTransactions($object, $xactions); } diff --git a/src/applications/diffusion/controller/DiffusionCommitController.php b/src/applications/diffusion/controller/DiffusionCommitController.php --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -45,6 +45,7 @@ ->withIdentifiers(array($commit_identifier)) ->needCommitData(true) ->needAuditRequests(true) + ->needAuditAuthority(array($viewer)) ->setLimit(100) ->needIdentities(true) ->execute(); @@ -111,7 +112,6 @@ } $audit_requests = $commit->getAudits(); - $commit->loadAndAttachAuditAuthority($viewer); $commit_data = $commit->getCommitData(); $is_foreign = $commit_data->getCommitDetail('foreign-svn-stub'); diff --git a/src/applications/diffusion/editor/DiffusionCommitEditEngine.php b/src/applications/diffusion/editor/DiffusionCommitEditEngine.php --- a/src/applications/diffusion/editor/DiffusionCommitEditEngine.php +++ b/src/applications/diffusion/editor/DiffusionCommitEditEngine.php @@ -43,9 +43,12 @@ } protected function newObjectQuery() { + $viewer = $this->getViewer(); + return id(new DiffusionCommitQuery()) ->needCommitData(true) - ->needAuditRequests(true); + ->needAuditRequests(true) + ->needAuditAuthority(array($viewer)); } protected function getEditorURI() { diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php --- a/src/applications/diffusion/query/DiffusionCommitQuery.php +++ b/src/applications/diffusion/query/DiffusionCommitQuery.php @@ -17,6 +17,7 @@ private $unreachable; private $needAuditRequests; + private $needAuditAuthority; private $auditIDs; private $auditorPHIDs; private $epochMin; @@ -121,6 +122,12 @@ return $this; } + public function needAuditAuthority(array $users) { + assert_instances_of($users, 'PhabricatorUser'); + $this->needAuditAuthority = $users; + return $this; + } + public function withAuditIDs(array $ids) { $this->auditIDs = $ids; return $this; @@ -231,14 +238,27 @@ } if (count($subqueries) > 1) { - foreach ($subqueries as $key => $subquery) { - $subqueries[$key] = '('.$subquery.')'; + $unions = null; + foreach ($subqueries as $subquery) { + if (!$unions) { + $unions = qsprintf( + $conn, + '(%Q)', + $subquery); + continue; + } + + $unions = qsprintf( + $conn, + '%Q UNION DISTINCT (%Q)', + $unions, + $subquery); } $query = qsprintf( $conn, '%Q %Q %Q', - implode(' UNION DISTINCT ', $subqueries), + $unions, $this->buildOrderClause($conn, true), $this->buildLimitClause($conn)); } else { @@ -423,6 +443,72 @@ $commits); } + if ($this->needAuditAuthority) { + $authority_users = $this->needAuditAuthority; + + // NOTE: This isn't very efficient since we're running two queries per + // user, but there's currently no way to figure out authority for + // multiple users in one query. Today, we only ever request authority for + // a single user and single commit, so this has no practical impact. + + // NOTE: We're querying with the viewership of query viewer, not the + // actual users. If the viewer can't see a project or package, they + // won't be able to see who has authority on it. This is safer than + // showing them true authority, and should never matter today, but it + // also doesn't seem like a significant disclosure and might be + // reasonable to adjust later if it causes something weird or confusing + // to happen. + + $authority_map = array(); + foreach ($authority_users as $authority_user) { + $authority_phid = $authority_user->getPHID(); + if (!$authority_phid) { + continue; + } + + $result_phids = array(); + + // Users have authority over themselves. + $result_phids[] = $authority_phid; + + // Users have authority over packages they own. + $owned_packages = id(new PhabricatorOwnersPackageQuery()) + ->setViewer($viewer) + ->withAuthorityPHIDs(array($authority_phid)) + ->execute(); + foreach ($owned_packages as $package) { + $result_phids[] = $package->getPHID(); + } + + // Users have authority over projects they're members of. + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($viewer) + ->withMemberPHIDs(array($authority_phid)) + ->execute(); + foreach ($projects as $project) { + $result_phids[] = $project->getPHID(); + } + + $result_phids = array_fuse($result_phids); + + foreach ($commits as $commit) { + $attach_phids = $result_phids; + + // NOTE: When modifying your own commits, you act only on behalf of + // yourself, not your packages or projects. The idea here is that you + // can't accept your own commits. In the future, this might change or + // depend on configuration. + $author_phid = $commit->getAuthorPHID(); + if ($author_phid == $authority_phid) { + $attach_phids = array($author_phid); + $attach_phids = array_fuse($attach_phids); + } + + $commit->attachAuditAuthority($authority_user, $attach_phids); + } + } + } + return $commits; } diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -210,84 +210,32 @@ return $this->assertAttached($this->committerIdentity); } - public function loadAndAttachAuditAuthority( - PhabricatorUser $viewer, - $actor_phid = null) { + public function attachAuditAuthority( + PhabricatorUser $user, + array $authority) { - if ($actor_phid === null) { - $actor_phid = $viewer->getPHID(); - } - - // TODO: This method is a little weird and sketchy, but worlds better than - // what came before it. Eventually, this should probably live in a Query - // class. - - // Figure out which requests the actor has authority over: these are user - // requests where they are the auditor, and packages and projects they are - // a member of. - - if (!$actor_phid) { - $attach_key = $viewer->getCacheFragment(); - $phids = array(); - } else { - $attach_key = $actor_phid; - // At least currently, when modifying your own commits, you act only on - // behalf of yourself, not your packages/projects -- the idea being that - // you can't accept your own commits. This may change or depend on - // config. - $actor_is_author = ($actor_phid == $this->getAuthorPHID()); - if ($actor_is_author) { - $phids = array($actor_phid); - } else { - $phids = array(); - $phids[$actor_phid] = true; - - $owned_packages = id(new PhabricatorOwnersPackageQuery()) - ->setViewer($viewer) - ->withAuthorityPHIDs(array($actor_phid)) - ->execute(); - foreach ($owned_packages as $package) { - $phids[$package->getPHID()] = true; - } - - $projects = id(new PhabricatorProjectQuery()) - ->setViewer($viewer) - ->withMemberPHIDs(array($actor_phid)) - ->execute(); - foreach ($projects as $project) { - $phids[$project->getPHID()] = true; - } - - $phids = array_keys($phids); - } + $user_phid = $user->getPHID(); + if (!$user->getPHID()) { + throw new Exception( + pht('You can not attach audit authority for a user with no PHID.')); } - $this->auditAuthorityPHIDs[$attach_key] = array_fuse($phids); + $this->auditAuthorityPHIDs[$user_phid] = $authority; return $this; } public function hasAuditAuthority( - PhabricatorUser $viewer, - PhabricatorRepositoryAuditRequest $audit, - $actor_phid = null) { + PhabricatorUser $user, + PhabricatorRepositoryAuditRequest $audit) { - if ($actor_phid === null) { - $actor_phid = $viewer->getPHID(); - } - - if (!$actor_phid) { - $attach_key = $viewer->getCacheFragment(); - } else { - $attach_key = $actor_phid; - } - - $map = $this->assertAttachedKey($this->auditAuthorityPHIDs, $attach_key); - - if (!$actor_phid) { + $user_phid = $user->getPHID(); + if (!$user_phid) { return false; } + $map = $this->assertAttachedKey($this->auditAuthorityPHIDs, $user_phid); + return isset($map[$audit->getAuditorPHID()]); }