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

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) {

src/applications/audit/editor/PhabricatorAuditEditor.php

@@ -78,6 +78,10 @@ final class PhabricatorAuditEditor
       }
     }
 
+    $object->loadAndAttachAuditAuthority(
+      $this->getActor(),
+      $this->getActingAsPHID());
+
     return parent::expandTransactions($object, $xactions);
   }
 

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

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;
         }
       }
     }

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() {