diff --git a/java/com/google/gerrit/server/change/ChangeInserter.java b/java/com/google/gerrit/server/change/ChangeInserter.java index 8ed1ef0860..5e60906d7a 100644 --- a/java/com/google/gerrit/server/change/ChangeInserter.java +++ b/java/com/google/gerrit/server/change/ChangeInserter.java @@ -569,7 +569,8 @@ public class ChangeInserter implements InsertChangeOp { new Branch.NameKey(ctx.getProject(), refName), ctx.getIdentifiedUser(), new NoSshInfo(), - ctx.getRevWalk()) + ctx.getRevWalk(), + change) .validate(event); } } catch (CommitValidationException e) { diff --git a/java/com/google/gerrit/server/change/PatchSetInserter.java b/java/com/google/gerrit/server/change/PatchSetInserter.java index 3cf0dc5346..3a32f8fb1d 100644 --- a/java/com/google/gerrit/server/change/PatchSetInserter.java +++ b/java/com/google/gerrit/server/change/PatchSetInserter.java @@ -345,7 +345,8 @@ public class PatchSetInserter implements BatchUpdateOp { origNotes.getChange().getDest(), ctx.getIdentifiedUser(), new NoSshInfo(), - ctx.getRevWalk()) + ctx.getRevWalk(), + origNotes.getChange()) .validate(event); } catch (CommitValidationException e) { throw new ResourceConflictException(e.getFullMessage()); diff --git a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java index 6ca2266b61..eb3559a897 100644 --- a/java/com/google/gerrit/server/git/receive/ReceiveCommits.java +++ b/java/com/google/gerrit/server/git/receive/ReceiveCommits.java @@ -1852,7 +1852,8 @@ class ReceiveCommits { logDebug("Creating new change for {} even though it is already tracked", name); } - if (!validCommit(rp.getRevWalk(), magicBranch.perm, magicBranch.dest, magicBranch.cmd, c)) { + if (!validCommit( + rp.getRevWalk(), magicBranch.perm, magicBranch.dest, magicBranch.cmd, c, null)) { // Not a change the user can propose? Abort as early as possible. newChanges = Collections.emptyList(); logDebug("Aborting early due to invalid commit"); @@ -2430,7 +2431,7 @@ class ReceiveCommits { } PermissionBackend.ForRef perm = permissions.ref(change.getDest().get()); - if (!validCommit(rp.getRevWalk(), perm, change.getDest(), inputCommand, newCommit)) { + if (!validCommit(rp.getRevWalk(), perm, change.getDest(), inputCommand, newCommit, change)) { return false; } rp.getRevWalk().parseBody(priorCommit); @@ -2794,7 +2795,7 @@ class ReceiveCommits { } if (existing.keySet().contains(c)) { continue; - } else if (!validCommit(walk, perm, branch, cmd, c)) { + } else if (!validCommit(walk, perm, branch, cmd, c, null)) { break; } @@ -2816,7 +2817,8 @@ class ReceiveCommits { PermissionBackend.ForRef perm, Branch.NameKey branch, ReceiveCommand cmd, - ObjectId id) + ObjectId id, + @Nullable Change change) throws IOException { if (validCommits.contains(id)) { @@ -2837,7 +2839,7 @@ class ReceiveCommits { ? commitValidatorsFactory.forMergedCommits( project.getNameKey(), perm, user.asIdentifiedUser()) : commitValidatorsFactory.forReceiveCommits( - perm, branch, user.asIdentifiedUser(), sshInfo, repo, rw); + perm, branch, user.asIdentifiedUser(), sshInfo, repo, rw, change); messages.addAll(validators.validate(receiveEvent)); } catch (CommitValidationException e) { logDebug("Commit validation failed on {}", c.name()); diff --git a/java/com/google/gerrit/server/git/validators/CommitValidators.java b/java/com/google/gerrit/server/git/validators/CommitValidators.java index a80ff73e2f..05ca98bfb0 100644 --- a/java/com/google/gerrit/server/git/validators/CommitValidators.java +++ b/java/com/google/gerrit/server/git/validators/CommitValidators.java @@ -19,6 +19,7 @@ import static com.google.gerrit.reviewdb.client.RefNames.REFS_CHANGES; import static com.google.gerrit.reviewdb.client.RefNames.REFS_CONFIG; import static java.util.stream.Collectors.toList; +import com.google.common.annotations.VisibleForTesting; import com.google.common.base.CharMatcher; import com.google.common.collect.ImmutableList; import com.google.gerrit.common.FooterConstants; @@ -30,6 +31,8 @@ import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.reviewdb.client.Account; import com.google.gerrit.reviewdb.client.BooleanProjectConfig; import com.google.gerrit.reviewdb.client.Branch; +import com.google.gerrit.reviewdb.client.Branch.NameKey; +import com.google.gerrit.reviewdb.client.Change; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; import com.google.gerrit.reviewdb.client.RevId; @@ -46,6 +49,7 @@ import com.google.gerrit.server.git.GitRepositoryManager; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.git.ValidationError; import com.google.gerrit.server.permissions.PermissionBackend; +import com.google.gerrit.server.permissions.PermissionBackend.ForRef; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.RefPermission; import com.google.gerrit.server.project.ProjectCache; @@ -125,7 +129,8 @@ public class CommitValidators { IdentifiedUser user, SshInfo sshInfo, Repository repo, - RevWalk rw) + RevWalk rw, + @Nullable Change change) throws IOException { NoteMap rejectCommits = BanCommit.loadRejectCommitsMap(repo, rw); ProjectState projectState = projectCache.checkedGet(branch.getParentKey()); @@ -138,7 +143,12 @@ public class CommitValidators { new CommitterUploaderValidator(user, perm, canonicalWebUrl), new SignedOffByValidator(user, perm, projectState), new ChangeIdValidator( - projectState, user, canonicalWebUrl, installCommitMsgHookCommand, sshInfo), + projectState, + user, + canonicalWebUrl, + installCommitMsgHookCommand, + sshInfo, + change), new ConfigValidator(branch, user, rw, allUsers, allProjects), new BannedCommitsValidator(rejectCommits), new PluginCommitValidationListener(pluginValidators), @@ -148,11 +158,12 @@ public class CommitValidators { } public CommitValidators forGerritCommits( - PermissionBackend.ForRef perm, - Branch.NameKey branch, + ForRef perm, + NameKey branch, IdentifiedUser user, SshInfo sshInfo, - RevWalk rw) + RevWalk rw, + @Nullable Change change) throws IOException { ProjectState projectState = projectCache.checkedGet(branch.getParentKey()); return new CommitValidators( @@ -163,7 +174,12 @@ public class CommitValidators { new AuthorUploaderValidator(user, perm, canonicalWebUrl), new SignedOffByValidator(user, perm, projectCache.checkedGet(branch.getParentKey())), new ChangeIdValidator( - projectState, user, canonicalWebUrl, installCommitMsgHookCommand, sshInfo), + projectState, + user, + canonicalWebUrl, + installCommitMsgHookCommand, + sshInfo, + change), new ConfigValidator(branch, user, rw, allUsers, allProjects), new PluginCommitValidationListener(pluginValidators), new ExternalIdUpdateListener(allUsers, externalIdsConsistencyChecker), @@ -231,6 +247,15 @@ public class CommitValidators { "[%s] invalid " + FooterConstants.CHANGE_ID.getName() + " line format in commit message footer"; + + @VisibleForTesting + public static final String CHANGE_ID_MISMATCH_MSG = + "[%s] " + + FooterConstants.CHANGE_ID.getName() + + " in commit message footer does not match" + + FooterConstants.CHANGE_ID.getName() + + " of target change"; + private static final Pattern CHANGE_ID = Pattern.compile(CHANGE_ID_PATTERN); private final ProjectState projectState; @@ -238,18 +263,21 @@ public class CommitValidators { private final String installCommitMsgHookCommand; private final SshInfo sshInfo; private final IdentifiedUser user; + private final Change change; public ChangeIdValidator( ProjectState projectState, IdentifiedUser user, String canonicalWebUrl, String installCommitMsgHookCommand, - SshInfo sshInfo) { + SshInfo sshInfo, + Change change) { this.projectState = projectState; this.canonicalWebUrl = canonicalWebUrl; this.installCommitMsgHookCommand = installCommitMsgHookCommand; this.sshInfo = sshInfo; this.user = user; + this.change = change; } @Override @@ -289,7 +317,12 @@ public class CommitValidators { messages.add(getMissingChangeIdErrorMsg(errMsg, receiveEvent.commit)); throw new CommitValidationException(errMsg, messages); } + if (change != null && !v.equals(change.getKey().get())) { + String errMsg = String.format(CHANGE_ID_MISMATCH_MSG, sha1); + throw new CommitValidationException(errMsg); + } } + return Collections.emptyList(); } diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java index 38a94899fc..f7ca2f2cbf 100644 --- a/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java +++ b/javatests/com/google/gerrit/acceptance/git/AbstractPushForReview.java @@ -75,9 +75,11 @@ import com.google.gerrit.reviewdb.client.ChangeMessage; import com.google.gerrit.reviewdb.client.PatchSet; import com.google.gerrit.reviewdb.client.Project; import com.google.gerrit.reviewdb.client.RefNames; +import com.google.gerrit.reviewdb.client.RevId; import com.google.gerrit.server.ChangeMessagesUtil; import com.google.gerrit.server.git.ProjectConfig; import com.google.gerrit.server.git.receive.ReceiveConstants; +import com.google.gerrit.server.git.validators.CommitValidators.ChangeIdValidator; import com.google.gerrit.server.group.SystemGroupBackend; import com.google.gerrit.server.mail.Address; import com.google.gerrit.server.project.testing.Util; @@ -1320,6 +1322,28 @@ public abstract class AbstractPushForReview extends AbstractDaemonTest { pushForReviewOk(testRepo); } + @Test + public void testPushWithChangedChangeId() throws Exception { + PushOneCommit.Result r = pushTo("refs/for/master"); + r.assertOkStatus(); + PushOneCommit push = + pushFactory.create( + db, + admin.getIdent(), + testRepo, + PushOneCommit.SUBJECT + + "\n\n" + + "Change-Id: I55eab7c7a76e95005fa9cc469aa8f9fc16da9eba\n", + "b.txt", + "anotherContent", + r.getChangeId()); + r = push.to("refs/changes/" + r.getChange().change().getId().get()); + r.assertErrorStatus( + String.format( + ChangeIdValidator.CHANGE_ID_MISMATCH_MSG, + r.getCommit().abbreviate(RevId.ABBREV_LEN).name())); + } + @Test public void pushWithMultipleChangeIds() throws Exception { testPushWithMultipleChangeIds();