From ca182c7f48ca55508d8e9bd76f64a395c971d4f9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 25 Jan 2017 11:28:42 -0800 Subject: [PATCH] Clean up "Audit Authority" code, at least mostly Summary: Ref T2393. We had three copies of this code ("which packages/projects can a user accept on behalf of?"). I removed one in D17250. This consolidates the other two. This still isn't perfect and it should probably live in a Query or something some day, but there's some weird stuff going on with the viewer in the editor context, and at least the code handles the viewer correctly now and isn't living somewhere weird and totally unrelated to auditing, and the callsites don't need to do a bunch of extra work. This also moves towards fixing the "re-accept if you've already accepted but then a new package you have authority over was added" bug, which we fixed recently in Differential. This should be less common in Audit, but should still be fixed. Test Plan: Viewed and audited commits with a mixture of user, package, and project auditors. Saw actions apply to the expected set of auditors. Reviewers: chad Reviewed By: chad Maniphest Tasks: T2393 Differential Revision: https://secure.phabricator.com/D17251 --- .../editor/PhabricatorAuditCommentEditor.php | 36 -------- .../audit/editor/PhabricatorAuditEditor.php | 4 + .../controller/DiffusionCommitController.php | 22 ++--- .../DiffusionCommitAuditTransaction.php | 11 ++- .../storage/PhabricatorRepositoryCommit.php | 90 +++++++++++++++---- 5 files changed, 89 insertions(+), 74 deletions(-) diff --git a/src/applications/audit/editor/PhabricatorAuditCommentEditor.php b/src/applications/audit/editor/PhabricatorAuditCommentEditor.php index 8a17999ee0..988311d74c 100644 --- 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 index 99190d808a..2e7efd4c87 100644 --- a/src/applications/audit/editor/PhabricatorAuditEditor.php +++ b/src/applications/audit/editor/PhabricatorAuditEditor.php @@ -78,6 +78,10 @@ final class PhabricatorAuditEditor } } + $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 index f308892790..f88c463633 100644 --- a/src/applications/diffusion/controller/DiffusionCommitController.php +++ b/src/applications/diffusion/controller/DiffusionCommitController.php @@ -4,9 +4,6 @@ final class DiffusionCommitController extends DiffusionController { const CHANGES_LIMIT = 100; - private $auditAuthorityPHIDs; - private $highlightedAudits; - private $commitParents; private $commitRefs; private $commitMerges; @@ -67,8 +64,7 @@ final class DiffusionCommitController extends DiffusionController { } $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 @@ final class DiffusionCommitController extends DiffusionController { $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 @@ final class DiffusionCommitController extends DiffusionController { 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 @@ final class DiffusionCommitController extends DiffusionController { 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 @@ final class DiffusionCommitController extends DiffusionController { $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 index 86656d85a6..99d2ebb1d2 100644 --- a/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php +++ b/src/applications/diffusion/xaction/DiffusionCommitAuditTransaction.php @@ -91,6 +91,9 @@ abstract class DiffusionCommitAuditTransaction $value, $status) { + $actor = $this->getActor(); + $acting_phid = $this->getActingAsPHID(); + $audits = $commit->getAudits(); $audits = mpull($audits, null, 'getAuditorPHID'); @@ -98,13 +101,9 @@ abstract class DiffusionCommitAuditTransaction $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 index 7f617069ec..3536cfbd0d 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -41,6 +41,7 @@ final class PhabricatorRepositoryCommit 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 @@ final class PhabricatorRepositoryCommit 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(); + } - // You don't have authority over package and project audits on your - // own commits. + // 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. - $auditor_is_user = ($audit->getAuditorPHID() == $user->getPHID()); - $user_is_author = ($commit_author == $user->getPHID()); + // 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 ($auditor_is_user || !$user_is_author) { - $authority_audits[$audit->getID()] = $audit; + 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); } } - 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() {