Move random Change-Id generation out of Change

The Change-Id generation has been broken for years because
two calls for creating two changes with the same content would
have resulted in the same Change-Id.

The master version of the Change-Id generation uses instead
a SecureRandom which gives a lot more entropy when generating
Change-Id, however, the method was implemented in the Change
class that is used in v2.16 by the GWT UI.

Move the random generation to CommitMessageUtil which gives
more guarantees to be outside the GWT UI when cherry-picked to
v2.16.

Bug: Issue 12246
Change-Id: I9de36332ef1d29a97ee459345fb3351b33b7af67

java/com/google/gerrit/entities/Change.java

@@ -16,20 +16,14 @@ package com.google.gerrit.entities;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.gerrit.entities.RefNames.REFS_CHANGES;
-import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.primitives.Ints;
 import com.google.gerrit.common.Nullable;
 import com.google.gerrit.extensions.client.ChangeStatus;
-import java.security.NoSuchAlgorithmException;
-import java.security.SecureRandom;
 import java.sql.Timestamp;
 import java.util.Arrays;
 import java.util.Optional;
-import org.eclipse.jgit.lib.Constants;
-import org.eclipse.jgit.lib.ObjectId;
-import org.eclipse.jgit.lib.ObjectInserter;
 
 /**
  * A change proposed to be merged into a branch.
@@ -101,15 +95,6 @@ import org.eclipse.jgit.lib.ObjectInserter;
  * notice of a replacement patch set is sent, or when notice of the change submission occurs.
  */
 public final class Change {
-  private static final SecureRandom rng;
-
-  static {
-    try {
-      rng = SecureRandom.getInstance("SHA1PRNG");
-    } catch (NoSuchAlgorithmException e) {
-      throw new RuntimeException("Cannot create RNG for Change-Id generator", e);
-    }
-  }
 
   public static Id id(int id) {
     return new AutoValue_Change_Id(id);
@@ -282,20 +267,6 @@ public final class Change {
     }
   }
 
-  public static ObjectId generateChangeId() {
-    byte[] rand = new byte[Constants.OBJECT_ID_STRING_LENGTH];
-    rng.nextBytes(rand);
-    String randomString = new String(rand, UTF_8);
-
-    try (ObjectInserter f = new ObjectInserter.Formatter()) {
-      return f.idFor(Constants.OBJ_COMMIT, Constants.encode(randomString));
-    }
-  }
-
-  public static Key generateKey() {
-    return key("I" + generateChangeId().name());
-  }
-
   public static Key key(String key) {
     return new AutoValue_Change_Key(key);
   }

java/com/google/gerrit/server/change/ChangeInserter.java

@@ -68,6 +68,7 @@ import com.google.gerrit.server.update.ChangeContext;
 import com.google.gerrit.server.update.Context;
 import com.google.gerrit.server.update.InsertChangeOp;
 import com.google.gerrit.server.update.RepoContext;
+import com.google.gerrit.server.util.CommitMessageUtil;
 import com.google.gerrit.server.util.RequestScopePropagator;
 import com.google.inject.Inject;
 import com.google.inject.assistedinject.Assisted;
@@ -207,7 +208,7 @@ public class ChangeInserter implements InsertChangeOp {
     }
     // A Change-Id is generated for the review, but not appended to the commit message.
     // This can happen if requireChangeId is false.
-    return Change.generateKey();
+    return CommitMessageUtil.generateKey();
   }
 
   public PatchSet.Id getPatchSetId() {

java/com/google/gerrit/server/git/CommitUtil.java

@@ -49,6 +49,7 @@ import com.google.gerrit.server.update.BatchUpdateOp;
 import com.google.gerrit.server.update.ChangeContext;
 import com.google.gerrit.server.update.Context;
 import com.google.gerrit.server.update.UpdateException;
+import com.google.gerrit.server.util.CommitMessageUtil;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
 import com.google.inject.Singleton;
@@ -151,7 +152,7 @@ public class CommitUtil {
         ObjectInserter oi = git.newObjectInserter();
         ObjectReader reader = oi.newReader();
         RevWalk revWalk = new RevWalk(reader)) {
-      ObjectId generatedChangeId = Change.generateChangeId();
+      ObjectId generatedChangeId = CommitMessageUtil.generateChangeId();
       ObjectId revCommit =
           createRevertCommit(message, notes, user, timestamp, oi, revWalk, generatedChangeId);
       return createRevertChangeFromCommit(

java/com/google/gerrit/server/git/meta/VersionedMetaData.java

@@ -18,7 +18,6 @@ import static com.google.common.base.Preconditions.checkArgument;
 
 import com.google.common.base.MoreObjects;
 import com.google.gerrit.common.Nullable;
-import com.google.gerrit.entities.Change;
 import com.google.gerrit.entities.Project;
 import com.google.gerrit.git.GitUpdateFailureException;
 import com.google.gerrit.git.LockFailureException;
@@ -26,6 +25,7 @@ import com.google.gerrit.git.ObjectIds;
 import com.google.gerrit.server.logging.Metadata;
 import com.google.gerrit.server.logging.TraceContext;
 import com.google.gerrit.server.logging.TraceContext.TraceTimer;
+import com.google.gerrit.server.util.CommitMessageUtil;
 import java.io.BufferedReader;
 import java.io.File;
 import java.io.IOException;
@@ -330,7 +330,8 @@ public abstract class VersionedMetaData {
         }
 
         if (update.insertChangeId()) {
-          commit.setMessage(ChangeIdUtil.insertId(commit.getMessage(), Change.generateChangeId()));
+          commit.setMessage(
+              ChangeIdUtil.insertId(commit.getMessage(), CommitMessageUtil.generateChangeId()));
         }
 
         src = rw.parseCommit(inserter.insert(commit));

java/com/google/gerrit/server/restapi/change/CherryPickChange.java

@@ -60,6 +60,7 @@ import com.google.gerrit.server.submit.IntegrationException;
 import com.google.gerrit.server.submit.MergeIdenticalTreeException;
 import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.UpdateException;
+import com.google.gerrit.server.util.CommitMessageUtil;
 import com.google.gerrit.server.util.time.TimeUtil;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -304,7 +305,9 @@ public class CherryPickChange {
       PersonIdent committerIdent = identifiedUser.newCommitterIdent(timestamp, serverTimeZone);
 
       final ObjectId generatedChangeId =
-          changeIdForNewChange != null ? changeIdForNewChange : Change.generateChangeId();
+          changeIdForNewChange != null
+              ? changeIdForNewChange
+              : CommitMessageUtil.generateChangeId();
       String commitMessage = ChangeIdUtil.insertId(message, generatedChangeId).trim() + '\n';
 
       CodeReviewCommit cherryPickCommit;

java/com/google/gerrit/server/restapi/change/CreateChange.java

@@ -76,6 +76,7 @@ import com.google.gerrit.server.restapi.project.CommitsCollection;
 import com.google.gerrit.server.restapi.project.ProjectsCollection;
 import com.google.gerrit.server.update.BatchUpdate;
 import com.google.gerrit.server.update.UpdateException;
+import com.google.gerrit.server.util.CommitMessageUtil;
 import com.google.gerrit.server.util.time.TimeUtil;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -461,7 +462,7 @@ public class CreateChange
     // Add a Change-Id line if there isn't already one
     String commitMessage = subject;
     if (ChangeIdUtil.indexOfChangeId(commitMessage, "\n") == -1) {
-      ObjectId id = Change.generateChangeId();
+      ObjectId id = CommitMessageUtil.generateChangeId();
       commitMessage = ChangeIdUtil.insertId(commitMessage, id);
     }
 

java/com/google/gerrit/server/restapi/change/RevertSubmission.java

@@ -68,6 +68,7 @@ import com.google.gerrit.server.update.BatchUpdateOp;
 import com.google.gerrit.server.update.ChangeContext;
 import com.google.gerrit.server.update.Context;
 import com.google.gerrit.server.update.UpdateException;
+import com.google.gerrit.server.util.CommitMessageUtil;
 import com.google.gerrit.server.util.time.TimeUtil;
 import com.google.inject.Inject;
 import com.google.inject.Provider;
@@ -302,7 +303,7 @@ public class RevertSubmission implements RestModifyView<ChangeResource, RevertIn
     // TODO (paiking): As a future change, the revert should just be done directly on the
     // target rather than just creating a commit and then cherry-picking it.
     cherryPickInput.message = revertInput.message;
-    ObjectId generatedChangeId = Change.generateChangeId();
+    ObjectId generatedChangeId = CommitMessageUtil.generateChangeId();
     Change.Id cherryPickRevertChangeId = Change.id(seq.nextChangeId());
     // TODO (paiking): In the the future, the timestamp should be the same for all the revert
     // changes.

java/com/google/gerrit/server/util/CommitMessageUtil.java

@@ -14,12 +14,29 @@
 
 package com.google.gerrit.server.util;
 
+import static java.nio.charset.StandardCharsets.UTF_8;
+
 import com.google.common.base.Strings;
 import com.google.gerrit.common.Nullable;
+import com.google.gerrit.entities.Change;
 import com.google.gerrit.extensions.restapi.BadRequestException;
+import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
+import org.eclipse.jgit.lib.Constants;
+import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
 
 /** Utility functions to manipulate commit messages. */
 public class CommitMessageUtil {
+  private static final SecureRandom rng;
+
+  static {
+    try {
+      rng = SecureRandom.getInstance("SHA1PRNG");
+    } catch (NoSuchAlgorithmException e) {
+      throw new RuntimeException("Cannot create RNG for Change-Id generator", e);
+    }
+  }
 
   private CommitMessageUtil() {}
 
@@ -42,4 +59,18 @@ public class CommitMessageUtil {
     trimmed = trimmed + "\n";
     return trimmed;
   }
+
+  public static ObjectId generateChangeId() {
+    byte[] rand = new byte[Constants.OBJECT_ID_STRING_LENGTH];
+    rng.nextBytes(rand);
+    String randomString = new String(rand, UTF_8);
+
+    try (ObjectInserter f = new ObjectInserter.Formatter()) {
+      return f.idFor(Constants.OBJ_COMMIT, Constants.encode(randomString));
+    }
+  }
+
+  public static Change.Key generateKey() {
+    return Change.key("I" + generateChangeId().name());
+  }
 }