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
diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 780d7a84..963fe29 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/git/AbstractPushForReview.java
@@ -17,6 +17,10 @@ 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.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.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 @@ "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 {
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java index 7265196..31058bc 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ApprovalsUtil.java
@@ -222,9 +222,8 @@ } public void addApprovals(ReviewDb db, ChangeUpdate update, - LabelTypes labelTypes, PatchSet ps, PatchSetInfo info, - ChangeControl changeCtl, Map<String, Short> approvals) - throws OrmException { + LabelTypes labelTypes, PatchSet ps, ChangeControl changeCtl, + Map<String, Short> approvals) throws OrmException { if (!approvals.isEmpty()) { checkApprovals(approvals, changeCtl); List<PatchSetApproval> cells = new ArrayList<>(approvals.size()); @@ -233,7 +232,7 @@ 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));
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java index df5c254..15a249f6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/ChangeInserter.java
@@ -211,8 +211,8 @@ LabelTypes labelTypes = projectControl.getLabelTypes(); approvalsUtil.addReviewers(db, update, labelTypes, change, patchSet, patchSetInfo, reviewers, Collections.<Account.Id> emptySet()); - approvalsUtil.addApprovals(db, update, labelTypes, patchSet, patchSetInfo, - ctl, approvals); + approvalsUtil.addApprovals(db, update, labelTypes, patchSet, ctl, + approvals); if (messageIsForChange()) { cmUtil.addChangeMessage(db, update, changeMessage); }
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java index a950eab..b4b30ec 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/git/ReceiveCommits.java
@@ -2197,7 +2197,7 @@ 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);