Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F15446657
D17251.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
9 KB
Referenced Files
None
Subscribers
None
D17251.diff
View Options
diff --git a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php
--- a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php
+++ b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php
@@ -2,42 +2,6 @@
final class PhabricatorAuditCommentEditor extends PhabricatorEditor {
- /**
- * Load the PHIDs for all objects the user has the authority to act as an
- * audit for. This includes themselves, and any packages they are an owner
- * of.
- */
- public static function loadAuditPHIDsForUser(PhabricatorUser $user) {
- $phids = array();
-
- // TODO: This method doesn't really use the right viewer, but in practice we
- // never issue this query of this type on behalf of another user and are
- // unlikely to do so in the future. This entire method should be refactored
- // into a Query class, however, and then we should use a proper viewer.
-
- // The user can audit on their own behalf.
- $phids[$user->getPHID()] = true;
-
- $owned_packages = id(new PhabricatorOwnersPackageQuery())
- ->setViewer($user)
- ->withAuthorityPHIDs(array($user->getPHID()))
- ->execute();
- foreach ($owned_packages as $package) {
- $phids[$package->getPHID()] = true;
- }
-
- // The user can audit on behalf of all projects they are a member of.
- $projects = id(new PhabricatorProjectQuery())
- ->setViewer($user)
- ->withMemberPHIDs(array($user->getPHID()))
- ->execute();
- foreach ($projects as $project) {
- $phids[$project->getPHID()] = true;
- }
-
- return array_keys($phids);
- }
-
public static function getMailThreading(
PhabricatorRepository $repository,
PhabricatorRepositoryCommit $commit) {
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
@@ -78,6 +78,10 @@
}
}
+ $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
@@ -4,9 +4,6 @@
const CHANGES_LIMIT = 100;
- private $auditAuthorityPHIDs;
- private $highlightedAudits;
-
private $commitParents;
private $commitRefs;
private $commitMerges;
@@ -67,8 +64,7 @@
}
$audit_requests = $commit->getAudits();
- $this->auditAuthorityPHIDs =
- PhabricatorAuditCommentEditor::loadAuditPHIDsForUser($viewer);
+ $commit->loadAndAttachAuditAuthority($viewer);
$commit_data = $commit->getCommitData();
$is_foreign = $commit_data->getCommitDetail('foreign-svn-stub');
@@ -209,10 +205,6 @@
$timeline = $this->buildComments($commit);
$merge_table = $this->buildMergesTable($commit);
- $highlighted_audits = $commit->getAuthorityAudits(
- $viewer,
- $this->auditAuthorityPHIDs);
-
$show_changesets = false;
$info_panel = null;
$change_list = null;
@@ -520,13 +512,13 @@
if ($user_requests) {
$view->addProperty(
pht('Auditors'),
- $this->renderAuditStatusView($user_requests));
+ $this->renderAuditStatusView($commit, $user_requests));
}
if ($other_requests) {
$view->addProperty(
pht('Group Auditors'),
- $this->renderAuditStatusView($other_requests));
+ $this->renderAuditStatusView($commit, $other_requests));
}
}
@@ -848,12 +840,12 @@
return $file->getRedirectResponse();
}
- private function renderAuditStatusView(array $audit_requests) {
+ private function renderAuditStatusView(
+ PhabricatorRepositoryCommit $commit,
+ array $audit_requests) {
assert_instances_of($audit_requests, 'PhabricatorRepositoryAuditRequest');
$viewer = $this->getViewer();
- $authority_map = array_fill_keys($this->auditAuthorityPHIDs, true);
-
$view = new PHUIStatusListView();
foreach ($audit_requests as $request) {
$code = $request->getAuditStatus();
@@ -873,7 +865,7 @@
$target = $viewer->renderHandle($auditor_phid);
$item->setTarget($target);
- if (isset($authority_map[$auditor_phid])) {
+ if ($commit->hasAuditAuthority($viewer, $request)) {
$item->setHighlighted(true);
}
diff --git a/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php
--- a/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php
@@ -91,6 +91,9 @@
$value,
$status) {
+ $actor = $this->getActor();
+ $acting_phid = $this->getActingAsPHID();
+
$audits = $commit->getAudits();
$audits = mpull($audits, null, 'getAuditorPHID');
@@ -98,13 +101,9 @@
$with_authority = ($status != PhabricatorAuditStatusConstants::RESIGNED);
if ($with_authority) {
- $has_authority = PhabricatorAuditCommentEditor::loadAuditPHIDsForUser(
- $viewer);
- $has_authority = array_fuse($has_authority);
foreach ($audits as $audit) {
- $auditor_phid = $audit->getAuditorPHID();
- if (isset($has_authority[$auditor_phid])) {
- $map[$auditor_phid] = $status;
+ if ($commit->hasAuditAuthority($actor, $audit, $acting_phid)) {
+ $map[$audit->getAuditorPHID()] = $status;
}
}
}
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
@@ -41,6 +41,7 @@
private $repository = self::ATTACHABLE;
private $customFields = self::ATTACHABLE;
private $drafts = array();
+ private $auditAuthorityPHIDs = array();
public function attachRepository(PhabricatorRepository $repository) {
$this->repository = $repository;
@@ -180,30 +181,85 @@
return $this->assertAttached($this->audits);
}
- public function getAuthorityAudits(
- PhabricatorUser $user,
- array $authority_phids) {
+ public function loadAndAttachAuditAuthority(
+ PhabricatorUser $viewer,
+ $actor_phid = null) {
- $authority = array_fill_keys($authority_phids, true);
- $audits = $this->getAudits();
- $authority_audits = array();
- foreach ($audits as $audit) {
- $has_authority = !empty($authority[$audit->getAuditorPHID()]);
- if ($has_authority) {
- $commit_author = $this->getAuthorPHID();
+ 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.
- // You don't have authority over package and project audits on your
- // own commits.
+ // 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.
- $auditor_is_user = ($audit->getAuditorPHID() == $user->getPHID());
- $user_is_author = ($commit_author == $user->getPHID());
+ 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;
+ }
- if ($auditor_is_user || !$user_is_author) {
- $authority_audits[$audit->getID()] = $audit;
+ $projects = id(new PhabricatorProjectQuery())
+ ->setViewer($viewer)
+ ->withMemberPHIDs(array($actor_phid))
+ ->execute();
+ foreach ($projects as $project) {
+ $phids[$project->getPHID()] = true;
}
+
+ $phids = array_keys($phids);
}
}
- return $authority_audits;
+
+ $this->auditAuthorityPHIDs[$attach_key] = array_fuse($phids);
+
+ return $this;
+ }
+
+ public function hasAuditAuthority(
+ PhabricatorUser $viewer,
+ PhabricatorRepositoryAuditRequest $audit,
+ $actor_phid = null) {
+
+ 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) {
+ return false;
+ }
+
+ return isset($map[$audit->getAuditorPHID()]);
}
public function getAuditorPHIDsForEdit() {
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Fri, Mar 28, 7:40 PM (4 d, 3 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7717147
Default Alt Text
D17251.diff (9 KB)
Attached To
Mode
D17251: Clean up "Audit Authority" code, at least mostly
Attached
Detach File
Event Timeline
Log In to Comment