Use uploader for approvals specified on push, not the committer

When the uploader has the Forge Committer Identity access right he can
upload commits where another user is committer. This means uploader
and committer can be different users.

When the committer is used for the approvals that are specified on
push then users with the Forge Committer Identity access right can put
votes on behalf of the committer onto the new change. This must not
be.

This was only possible when there were at least two labels defined.

When on upload the committer is forged he is automatically added as
reviewer to the change. This results in a dummy 0 vote. If there was
only one label this dummy 0 vote collided with any vote for the same
label that was specified in the push specification and hence the
upload failed, which means in this case it was not possible to forge a
vote.

However if there were multiple labels defined forging a vote was
possible.

The push also failed with a NullPointerException when a commit was
pushed that had a committer that didn't exist in the Gerrit database,
or when that account didn't have an email address registered.

Putting votes on behalf of another user is possible via the REST API
but this requires the special on behalf of permission for the label.
One can imagine a new option for putting label votes on behalf of
another user on push too, but this is not implemented yet.

Bug: Issue 3602
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I2e848c32cfad81979f22613d2a631fad6e0cea66

gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java

@@ -17,6 +17,10 @@ package com.google.gerrit.acceptance.git;
 import static com.google.common.truth.Truth.assertThat;
 import static com.google.common.truth.TruthJUnit.assume;
 import static com.google.gerrit.acceptance.GitUtil.cloneProject;
+import static com.google.gerrit.acceptance.GitUtil.createCommit;
+import static com.google.gerrit.acceptance.GitUtil.pushHead;
+import static com.google.gerrit.server.project.Util.category;
+import static com.google.gerrit.server.project.Util.value;
 import static java.util.concurrent.TimeUnit.MILLISECONDS;
 import static java.util.concurrent.TimeUnit.SECONDS;
 
@@ -24,8 +28,10 @@ import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.gerrit.acceptance.AbstractDaemonTest;
 import com.google.gerrit.acceptance.GitUtil;
+import com.google.gerrit.acceptance.GitUtil.Commit;
 import com.google.gerrit.acceptance.PushOneCommit;
 import com.google.gerrit.acceptance.TestAccount;
+import com.google.gerrit.common.data.LabelType;
 import com.google.gerrit.common.data.Permission;
 import com.google.gerrit.extensions.api.projects.BranchInput;
 import com.google.gerrit.extensions.client.InheritableBoolean;
@@ -34,6 +40,8 @@ import com.google.gerrit.extensions.common.EditInfo;
 import com.google.gerrit.extensions.common.LabelInfo;
 import com.google.gerrit.extensions.restapi.RestApiException;
 import com.google.gerrit.reviewdb.client.Change;
+import com.google.gerrit.server.git.MetaDataUpdate;
+import com.google.gerrit.server.git.ProjectConfig;
 import com.google.gerrit.server.notedb.NotesMigration;
 import com.google.gerrit.testutil.ConfigSuite;
 import com.google.gwtorm.server.OrmException;
@@ -247,6 +255,64 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest {
         "Uploaded patch set 3.");
   }
 
+  /**
+   * There was a bug that allowed a user with Forge Committer Identity access
+   * right to upload a commit and put *votes on behalf of another user* on it.
+   * This test checks that this is not possible, but that the votes that are
+   * specified on push are applied only in the name of the uploader.
+   *
+   * This particular bug only occurred when there was more than one label
+   * defined. Hence the test defines a custom label.
+   *
+   * When on upload the committer is forged he is automatically added as
+   * reviewer to the change. This results in a dummy 0 vote. If there was only
+   * one label this dummy 0 vote collided with any vote for the same label that
+   * was specified in the push specification and hence the upload failed, which
+   * means in this case it was not possible to forge a vote.
+   */
+  @Test
+  public void testPushForMasterWithApprovalsForgeCommitterButNoForgeVote()
+      throws GitAPIException, IOException, RestApiException {
+    // add custom label because the bug only allowed to forge a vote when there
+    // were at least two labels
+    LabelType Q = category("CustomLabel",
+        value(1, "Positive"),
+        value(0, "No score"),
+        value(-1, "Negative"));
+    ProjectConfig cfg = projectCache.checkedGet(allProjects).getConfig();
+    cfg.getLabelSections().put(Q.getName(), Q);
+    MetaDataUpdate md = metaDataUpdateFactory.create(allProjects);
+    try {
+      cfg.commit(md);
+    } finally {
+      md.close();
+    }
+    projectCache.evict(allProjects);
+
+    // Create a commit with "User" as author and committer
+    Commit c = createCommit(git, user.getIdent(), PushOneCommit.SUBJECT);
+
+    // Push this commit as "Administrator" (requires Forge Committer Identity)
+    pushHead(git, "refs/for/master/%l=Code-Review+1", false);
+
+    // Expected Code-Review votes:
+    // 1. 0 from User (committer):
+    //    When the committer is forged, the committer is automatically added as
+    //    reviewer, hence we expect a dummy 0 vote for the committer.
+    // 2. +1 from Administrator (uploader):
+    //    On push Code-Review+1 was specified, hence we expect a +1 vote from
+    //    the uploader.
+    ChangeInfo ci = get(c.getChangeId());
+    LabelInfo cr = ci.labels.get("Code-Review");
+    assertThat(cr.all).hasSize(2);
+    assertThat(cr.all.get(0).name).isEqualTo("Administrator");
+    assertThat(cr.all.get(0).value.intValue()).is(1);
+    assertThat(cr.all.get(1).name).isEqualTo("User");
+    assertThat(cr.all.get(1).value.intValue()).is(0);
+    assertThat(Iterables.getLast(ci.messages).message).isEqualTo(
+        "Uploaded patch set 1: Code-Review+1.");
+  }
+
   @Test
   public void testPushNewPatchsetToRefsChanges() throws GitAPIException,
     IOException, OrmException {

gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java

@@ -222,9 +222,8 @@ public class ApprovalsUtil {
   }
 
   public void addApprovals(ReviewDb db, ChangeUpdate update,
-      LabelTypes labelTypes, PatchSet ps, PatchSetInfo info,
+      LabelTypes labelTypes, PatchSet ps, ChangeControl changeCtl,
-      ChangeControl changeCtl, Map<String, Short> approvals)
+      Map<String, Short> approvals) throws OrmException {
-      throws OrmException {
     if (!approvals.isEmpty()) {
       checkApprovals(approvals, changeCtl);
       List<PatchSetApproval> cells = new ArrayList<>(approvals.size());
@@ -233,7 +232,7 @@ public class ApprovalsUtil {
         LabelType lt = labelTypes.byLabel(vote.getKey());
         cells.add(new PatchSetApproval(new PatchSetApproval.Key(
             ps.getId(),
-            info.getCommitter().getAccount(),
+            ps.getUploader(),
             lt.getLabelId()),
             vote.getValue(),
             ts));

gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java

@@ -211,8 +211,8 @@ public class ChangeInserter {
       LabelTypes labelTypes = projectControl.getLabelTypes();
       approvalsUtil.addReviewers(db, update, labelTypes, change,
           patchSet, patchSetInfo, reviewers, Collections.<Account.Id> emptySet());
-      approvalsUtil.addApprovals(db, update, labelTypes, patchSet, patchSetInfo,
+      approvalsUtil.addApprovals(db, update, labelTypes, patchSet, ctl,
-          ctl, approvals);
+          approvals);
       if (messageIsForChange()) {
         cmUtil.addChangeMessage(db, update, changeMessage);
       }

gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java

@@ -2197,7 +2197,7 @@ public class ReceiveCommits {
         approvalCopier.copy(db, changeCtl, newPatchSet);
         approvalsUtil.addReviewers(db, update, labelTypes, change, newPatchSet,
             info, recipients.getReviewers(), oldRecipients.getAll());
-        approvalsUtil.addApprovals(db, update, labelTypes, newPatchSet, info,
+        approvalsUtil.addApprovals(db, update, labelTypes, newPatchSet,
             changeCtl, approvals);
         recipients.add(oldRecipients);