Address a transaction issue with some audit actions not applying correctly

Summary:
See <https://discourse.phabricator-community.org/t/cannot-accept-commits-in-audit/2166/>.

In D19842, I changed `PhabricatorEditField->shouldGenerateTransactionsFromComment()`.

  - Previously, it bailed on `getIsConduitOnly()`.
  - After the patch, it bails on a missing `getCommentActionLabel()`.

The old code was actually wrong, and it was previously possible to apply possibly-invalid actions in some cases (or, at least, sneak them through this layer: they would only actually apply if not validated properly).

In practice, it let a different bug through: we sometimes loaded commits without loading their audit authority, so testing whether the viewer could "Accept" the commit or not (or take some other actions like "Raise Concern") would always fail and throw an exception: "Trying to access data not attached to this object..."

Fixing the insufficiently-strict transaction generation code exposed the "authority not attached" bug, which caused some actions to fail to generate transactions.

This appeared in the UI as either an unhelpful error ("You can't post an empty comment") or an action with no effect. The unhelpful error was because we show that error if you aren't taking any //other// actions, and we wouldn't generate an "Accept" action because of the interaction of these bugs, so the code thought you were just posting an empty comment.

Test Plan: Without leaving comments, accepted and rejected commits. No more error messages, and actions took effect.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: stephan.senkbeil, hskiba

Differential Revision: https://secure.phabricator.com/D19845

src/applications/audit/editor/PhabricatorAuditEditor.php

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

src/applications/diffusion/controller/DiffusionCommitController.php

@@ -45,6 +45,7 @@ final class DiffusionCommitController extends DiffusionController {
       ->withIdentifiers(array($commit_identifier))
       ->needCommitData(true)
       ->needAuditRequests(true)
+      ->needAuditAuthority(array($viewer))
       ->setLimit(100)
       ->needIdentities(true)
       ->execute();
@@ -111,7 +112,6 @@ final class DiffusionCommitController extends DiffusionController {
     }
 
     $audit_requests = $commit->getAudits();
-    $commit->loadAndAttachAuditAuthority($viewer);
 
     $commit_data = $commit->getCommitData();
     $is_foreign = $commit_data->getCommitDetail('foreign-svn-stub');

src/applications/diffusion/editor/DiffusionCommitEditEngine.php

@@ -43,9 +43,12 @@ final class DiffusionCommitEditEngine
   }
 
   protected function newObjectQuery() {
+    $viewer = $this->getViewer();
+
     return id(new DiffusionCommitQuery())
       ->needCommitData(true)
-      ->needAuditRequests(true);
+      ->needAuditRequests(true)
+      ->needAuditAuthority(array($viewer));
   }
 
   protected function getEditorURI() {

src/applications/diffusion/query/DiffusionCommitQuery.php

@@ -17,6 +17,7 @@ final class DiffusionCommitQuery
   private $unreachable;
 
   private $needAuditRequests;
+  private $needAuditAuthority;
   private $auditIDs;
   private $auditorPHIDs;
   private $epochMin;
@@ -121,6 +122,12 @@ final class DiffusionCommitQuery
     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 @@ final class DiffusionCommitQuery
     }
 
     if (count($subqueries) > 1) {
-      foreach ($subqueries as $key => $subquery) {
+      $unions = null;
-        $subqueries[$key] = '('.$subquery.')';
+      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 @@ final class DiffusionCommitQuery
         $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;
   }
 

src/applications/repository/storage/PhabricatorRepositoryCommit.php

@@ -210,84 +210,32 @@ final class PhabricatorRepositoryCommit
     return $this->assertAttached($this->committerIdentity);
   }
 
-  public function loadAndAttachAuditAuthority(
+  public function attachAuditAuthority(
-    PhabricatorUser $viewer,
+    PhabricatorUser $user,
-    $actor_phid = null) {
+    array $authority) {
 
-    if ($actor_phid === null) {
+    $user_phid = $user->getPHID();
-      $actor_phid = $viewer->getPHID();
+    if (!$user->getPHID()) {
+      throw new Exception(
+        pht('You can not attach audit authority for a user with no PHID.'));
     }
 
-    // TODO: This method is a little weird and sketchy, but worlds better than
+    $this->auditAuthorityPHIDs[$user_phid] = $authority;
-    // 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);
-      }
-    }
-
-    $this->auditAuthorityPHIDs[$attach_key] = array_fuse($phids);
 
     return $this;
   }
 
   public function hasAuditAuthority(
-    PhabricatorUser $viewer,
+    PhabricatorUser $user,
-    PhabricatorRepositoryAuditRequest $audit,
+    PhabricatorRepositoryAuditRequest $audit) {
-    $actor_phid = null) {
 
-    if ($actor_phid === null) {
+    $user_phid = $user->getPHID();
-      $actor_phid = $viewer->getPHID();
+    if (!$user_phid) {
-    }
-
-    if (!$actor_phid) {
-      $attach_key = $viewer->getCacheFragment();
-    } else {
-      $attach_key = $actor_phid;
-    }
-
-    $map = $this->assertAttachedKey($this->auditAuthorityPHIDs, $attach_key);
-
-    if (!$actor_phid) {
       return false;
     }
 
+    $map = $this->assertAttachedKey($this->auditAuthorityPHIDs, $user_phid);
+
     return isset($map[$audit->getAuditorPHID()]);
   }