Page MenuHomePhabricator
Files F14729131

D19845.id47394.diff
No OneTemporary
Actions

D19845.id47394.diff
View Options

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

File Metadata

Mime Type
text/plain
Expires
Sun, Jan 19, 6:17 AM (8 h, 22 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7018633
Default Alt Text
D19845.id47394.diff (9 KB)

Event Timeline

Phabricator · Built by Phacility