Compare incoming change-id to target change

When pushing to an existing change (for example to refs/changes or
editing a change in the UI) the change-id in the message footers must
not differ from the change-id of the target change.

This change will add the target change (if any, else null) to the
ChangeIdValidator, and check that the change-id footer is the same as
the key of the target change.

Change-Id: I92cdd3af04ac6e6548630483041c342509bb1224

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

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

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

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

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