Notedb: Store patch set push certificates in notes

Unlike other fields, push certificates have a predefined format that
includes newlines, so they are not particularly appropriate to store
in a single "Key: Value" footer. Moreover, we already reserve the body
of the meta commit message for ChangeMessages.

Sidestep any ambiguity by storing push certificates in the note for
the revision. Optionally parse a push certificate as the first thing
in the note file, before any inline comments.

Change-Id: Ia8c0674f3b40f6ec100cc9fac8ffec671833774b

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

@@ -81,7 +81,7 @@ public class PatchSetUtil {
     ps.setPushCertificate(pushCertificate);
     db.patchSets().insert(Collections.singleton(ps));
 
-    update.setCommit(rw, commit);
+    update.setCommit(rw, commit, pushCertificate);
     if (draft) {
       update.setPatchSetState(DRAFT);
     }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeNotesParser.java

@@ -478,6 +478,13 @@ class ChangeNotesParser implements AutoCloseable {
         comments.put(revId, plc);
       }
     }
+
+    for (PatchSet ps : patchSets.values()) {
+      RevisionNote rn = revisionNotes.get(ps.getRevision());
+      if (rn != null && rn.pushCert != null) {
+        ps.setPushCertificate(rn.pushCert);
+      }
+    }
   }
 
   private void parseApproval(PatchSet.Id psId, Account.Id accountId,

gerrit-server/src/main/java/com/google/gerrit/server/notedb/ChangeUpdate.java

@@ -15,6 +15,7 @@
 package com.google.gerrit.server.notedb;
 
 import static com.google.common.base.Preconditions.checkArgument;
+import static com.google.common.base.Preconditions.checkState;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_BRANCH;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_COMMIT;
 import static com.google.gerrit.server.notedb.ChangeNoteUtil.FOOTER_GROUPS;
@@ -113,6 +114,7 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   private ChangeNotes notes;
   private PatchSetState psState;
   private Iterable<String> groups;
+  private String pushCert;
 
   private final ChangeDraftUpdate.Factory draftUpdateFactory;
   private ChangeDraftUpdate draftUpdate;
@@ -368,10 +370,16 @@ public class ChangeUpdate extends AbstractChangeUpdate {
   }
 
   public void setCommit(RevWalk rw, ObjectId id) throws IOException {
+    setCommit(rw, id, null);
+  }
+
+  public void setCommit(RevWalk rw, ObjectId id, String pushCert)
+      throws IOException {
     RevCommit commit = rw.parseCommit(id);
     rw.parseBody(commit);
     this.commit = commit;
     subject = commit.getShortMessage();
+    this.pushCert = pushCert;
   }
 
   public void setHashtags(Set<String> hashtags) {
@@ -402,18 +410,17 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     if (noteMap == null) {
       noteMap = NoteMap.newEmptyMap();
     }
-    if (comments.isEmpty()) {
+    if (comments.isEmpty() && pushCert == null) {
       return null;
     }
 
     Map<RevId, RevisionNoteBuilder> builders = new HashMap<>();
     for (PatchLineComment c : comments) {
-      RevisionNoteBuilder b = builders.get(c.getRevId());
-      if (b == null) {
-        b = new RevisionNoteBuilder(notes.getRevisionNotes().get(c.getRevId()));
-        builders.put(c.getRevId(), b);
+      builder(builders, c.getRevId()).addComment(c);
     }
-      b.addComment(c);
+    if (pushCert != null) {
+      checkState(commit != null);
+      builder(builders, new RevId(commit.name())).setPushCertificate(pushCert);
     }
 
     for (Map.Entry<RevId, RevisionNoteBuilder> e : builders.entrySet()) {
@@ -425,6 +432,17 @@ public class ChangeUpdate extends AbstractChangeUpdate {
     return noteMap.writeTree(inserter);
   }
 
+  private RevisionNoteBuilder builder(Map<RevId, RevisionNoteBuilder> builders,
+      RevId revId) {
+    RevisionNoteBuilder b = builders.get(revId);
+    if (b == null) {
+      b = new RevisionNoteBuilder(
+          getChangeNotes().getRevisionNotes().get(revId));
+      builders.put(revId, b);
+    }
+    return b;
+  }
+
   public RevCommit commit() throws IOException {
     BatchMetaDataUpdate batch = openUpdate();
     try {

gerrit-server/src/main/java/com/google/gerrit/server/notedb/CommentsInNotesUtil.java

@@ -22,6 +22,7 @@ import static com.google.gerrit.server.notedb.RevisionNote.MAX_NOTE_SZ;
 import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Multimap;
 import com.google.common.primitives.Ints;
@@ -61,6 +62,7 @@ import org.eclipse.jgit.util.RawParseUtils;
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.io.OutputStream;
 import java.io.OutputStreamWriter;
 import java.io.PrintWriter;
 import java.sql.Timestamp;
@@ -105,7 +107,8 @@ public class CommentsInNotesUtil {
     for (Note note : noteMap) {
       byte[] bytes =
           reader.open(note.getData(), OBJ_BLOB).getCachedBytes(MAX_NOTE_SZ);
-      List<PatchLineComment> result = parseNote(bytes, changeId, status);
+      List<PatchLineComment> result =
+          parseNote(bytes, new MutableInteger(), changeId, status);
       if (result == null || result.isEmpty()) {
         continue;
       }
@@ -114,26 +117,27 @@ public class CommentsInNotesUtil {
     return noteMap;
   }
 
-  public static List<PatchLineComment> parseNote(byte[] note,
+  public static List<PatchLineComment> parseNote(byte[] note, MutableInteger p,
       Change.Id changeId, Status status) throws ConfigInvalidException {
+    if (p.value >= note.length) {
+      return ImmutableList.of();
+    }
     List<PatchLineComment> result = Lists.newArrayList();
     int sizeOfNote = note.length;
-    MutableInteger curr = new MutableInteger();
-    curr.value = 0;
 
     boolean isForBase =
-        (RawParseUtils.match(note, curr.value, PATCH_SET.getBytes(UTF_8))) < 0;
+        (RawParseUtils.match(note, p.value, PATCH_SET.getBytes(UTF_8))) < 0;
 
-    PatchSet.Id psId = parsePsId(note, curr, changeId, isForBase ? BASE_PATCH_SET : PATCH_SET);
+    PatchSet.Id psId = parsePsId(note, p, changeId, isForBase ? BASE_PATCH_SET : PATCH_SET);
 
     RevId revId =
-        new RevId(parseStringField(note, curr, changeId, REVISION));
+        new RevId(parseStringField(note, p, changeId, REVISION));
 
     PatchLineComment c = null;
-    while (curr.value < sizeOfNote) {
+    while (p.value < sizeOfNote) {
       String previousFileName = c == null ?
           null : c.getKey().getParentKey().getFileName();
-      c = parseComment(note, curr, previousFileName, psId, revId,
+      c = parseComment(note, p, previousFileName, psId, revId,
           isForBase, status);
       result.add(c);
     }
@@ -392,8 +396,10 @@ public class CommentsInNotesUtil {
       String fieldName, Change.Id changeId) throws ConfigInvalidException {
     boolean correct =
         RawParseUtils.match(note, curr.value, fieldName.getBytes(UTF_8)) != -1;
-    correct &= (note[curr.value + fieldName.length()] == ':');
-    correct &= (note[curr.value + fieldName.length() + 1] == ' ');
+    int p = curr.value + fieldName.length();
+    correct &= (p < note.length && note[p] == ':');
+    p++;
+    correct &= (p < note.length && note[p] == ' ');
     if (!correct) {
       throw parseException(changeId, "could not parse %s", fieldName);
     }
@@ -412,22 +418,26 @@ public class CommentsInNotesUtil {
     this.anonymousCowardName = anonymousCowardName;
   }
 
+  public byte[] buildNote(List<PatchLineComment> comments) {
+    ByteArrayOutputStream out = new ByteArrayOutputStream();
+    buildNote(comments, out);
+    return out.toByteArray();
+  }
+
   /**
    * Build a note that contains the metadata for and the contents of all of the
    * comments in the given list of comments.
    *
-   * @param comments
-   *            A list of the comments to be written to the returned note
-   *            byte array.
-   *            All of the comments in this list must have the same side and
-   *            must share the same PatchSet.Id.
-   *            This list must not be empty because we cannot build a note
-   *            for no comments.
-   * @return the note. Null if there are no comments in the list.
+   * @param comments A list of the comments to be written to the
+   *            output stream. All of the comments in this list must have the
+   *            same side and must share the same patch set ID.
+   * @param out output stream to write to.
    */
-  public byte[] buildNote(List<PatchLineComment> comments) {
-    ByteArrayOutputStream buf = new ByteArrayOutputStream();
-    OutputStreamWriter streamWriter = new OutputStreamWriter(buf, UTF_8);
+  public void buildNote(List<PatchLineComment> comments, OutputStream out) {
+    if (comments.isEmpty()) {
+      return;
+    }
+    OutputStreamWriter streamWriter = new OutputStreamWriter(out, UTF_8);
     try (PrintWriter writer = new PrintWriter(streamWriter)) {
       PatchLineComment first = comments.get(0);
 
@@ -504,7 +514,6 @@ public class CommentsInNotesUtil {
         writer.print("\n\n");
       }
     }
-    return buf.toByteArray();
   }
 
   /**
@@ -536,7 +545,10 @@ public class CommentsInNotesUtil {
       // We allow comments for multiple commits to be written in the same
       // update, even though the rest of the metadata update is associated with
       // a single patch set.
-      noteMap.set(commit, inserter.insert(OBJ_BLOB, buildNote(comments)));
+      byte[] note = buildNote(comments);
+      if (note != null && note.length > 0) {
+        noteMap.set(commit, inserter.insert(OBJ_BLOB, note));
+      }
     }
   }
 

gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNote.java

@@ -14,27 +14,63 @@
 
 package com.google.gerrit.server.notedb;
 
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.primitives.Bytes;
 import com.google.gerrit.reviewdb.client.Change;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
 
 import org.eclipse.jgit.errors.ConfigInvalidException;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectReader;
+import org.eclipse.jgit.util.MutableInteger;
+import org.eclipse.jgit.util.RawParseUtils;
 
 import java.io.IOException;
 
 class RevisionNote {
   static final int MAX_NOTE_SZ = 25 << 20;
 
+  private static final byte[] CERT_HEADER =
+      "certificate version ".getBytes(UTF_8);
+  // See org.eclipse.jgit.transport.PushCertificateParser.END_SIGNATURE
+  private static final byte[] END_SIGNATURE =
+      "-----END PGP SIGNATURE-----".getBytes(UTF_8);
+
+  private static void trimLeadingEmptyLines(byte[] bytes, MutableInteger p) {
+    while (p.value < bytes.length && bytes[p.value] == '\n') {
+      p.value++;
+    }
+  }
+
+  private static String parsePushCert(Change.Id changeId, byte[] bytes,
+      MutableInteger p) throws ConfigInvalidException {
+    if (RawParseUtils.match(bytes, p.value, CERT_HEADER) < 0) {
+      return null;
+    }
+    int end = Bytes.indexOf(bytes, END_SIGNATURE);
+    if (end < 0) {
+      throw ChangeNotes.parseException(
+          changeId, "invalid push certificate in note");
+    }
+    int start = p.value;
+    p.value = end + END_SIGNATURE.length;
+    return new String(bytes, start, p.value);
+  }
+
   final ImmutableList<PatchLineComment> comments;
+  final String pushCert;
 
   RevisionNote(Change.Id changeId, ObjectReader reader, ObjectId noteId)
       throws ConfigInvalidException, IOException {
     byte[] bytes = reader.open(noteId, OBJ_BLOB).getCachedBytes(MAX_NOTE_SZ);
+    MutableInteger p = new MutableInteger();
+    trimLeadingEmptyLines(bytes, p);
+    pushCert = parsePushCert(changeId, bytes, p);
+    trimLeadingEmptyLines(bytes, p);
     comments = ImmutableList.copyOf(CommentsInNotesUtil.parseNote(
-        bytes, changeId, PatchLineComment.Status.PUBLISHED));
+        bytes, p, changeId, PatchLineComment.Status.PUBLISHED));
   }
 }

gerrit-server/src/main/java/com/google/gerrit/server/notedb/RevisionNoteBuilder.java

@@ -15,15 +15,18 @@
 package com.google.gerrit.server.notedb;
 
 import static com.google.gerrit.server.PatchLineCommentsUtil.PLC_ORDER;
+import static java.nio.charset.StandardCharsets.UTF_8;
 
 import com.google.common.collect.Maps;
 import com.google.gerrit.reviewdb.client.PatchLineComment;
 
+import java.io.ByteArrayOutputStream;
 import java.util.HashMap;
 import java.util.Map;
 
 class RevisionNoteBuilder {
   private final Map<PatchLineComment.Key, PatchLineComment> comments;
+  private String pushCert;
 
   RevisionNoteBuilder(RevisionNote base) {
     if (base != null) {
@@ -31,8 +34,10 @@ class RevisionNoteBuilder {
       for (PatchLineComment c : base.comments) {
         addComment(c);
       }
+      pushCert = base.pushCert;
     } else {
       comments = new HashMap<>();
+      pushCert = null;
     }
   }
 
@@ -40,7 +45,26 @@ class RevisionNoteBuilder {
     comments.put(comment.getKey(), comment);
   }
 
+  void setPushCertificate(String pushCert) {
+    this.pushCert = pushCert;
+  }
+
   byte[] build(CommentsInNotesUtil commentsUtil) {
-    return commentsUtil.buildNote(PLC_ORDER.sortedCopy(comments.values()));
+    ByteArrayOutputStream out = new ByteArrayOutputStream();
+    if (pushCert != null) {
+      byte[] certBytes = pushCert.getBytes(UTF_8);
+      out.write(certBytes, 0, trimTrailingNewlines(certBytes));
+      out.write('\n');
+    }
+    commentsUtil.buildNote(PLC_ORDER.sortedCopy(comments.values()), out);
+    return out.toByteArray();
+  }
+
+  private static int trimTrailingNewlines(byte[] bytes) {
+    int p = bytes.length;
+    while (p > 1 && bytes[p - 1] == '\n') {
+      p--;
+    }
+    return p;
   }
 }

gerrit-server/src/test/java/com/google/gerrit/server/notedb/ChangeNotesTest.java

@@ -19,7 +19,9 @@ import static com.google.gerrit.server.notedb.ReviewerStateInternal.CC;
 import static com.google.gerrit.server.notedb.ReviewerStateInternal.REVIEWER;
 import static com.google.gerrit.testutil.TestChanges.incrementPatchSet;
 import static java.nio.charset.StandardCharsets.UTF_8;
+import static org.eclipse.jgit.lib.Constants.OBJ_BLOB;
 
+import com.google.common.base.CharMatcher;
 import com.google.common.base.Function;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableListMultimap;
@@ -59,6 +61,7 @@ import java.sql.Timestamp;
 import java.util.ArrayList;
 import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Map;
 
 public class ChangeNotesTest extends AbstractChangeNotesTest {
   @Test
@@ -688,6 +691,66 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
       .containsExactly("a", "b").inOrder();
   }
 
+  @Test
+  public void pushCertificate() throws Exception {
+    String pushCert = "certificate version 0.1\n"
+      + "pusher This is not a real push cert\n"
+      + "-----BEGIN PGP SIGNATURE-----\n"
+      + "Version: GnuPG v1\n"
+      + "\n"
+      + "Nor is this a real signature.\n"
+      + "-----END PGP SIGNATURE-----\n";
+    String trimmedCert = CharMatcher.is('\n').trimTrailingFrom(pushCert);
+
+    // ps2 with push cert
+    Change c = newChange();
+    PatchSet.Id psId1 = c.currentPatchSetId();
+    incrementPatchSet(c);
+    PatchSet.Id psId2 = c.currentPatchSetId();
+    ChangeUpdate update = newUpdate(c, changeOwner);
+    update.setPatchSetId(psId2);
+    RevCommit commit = tr.commit().message("PS2").create();
+    update.setCommit(rw, commit, pushCert);
+    update.commit();
+
+    ChangeNotes notes = newNotes(c);
+    assertThat(readNote(notes, commit)).isEqualTo(pushCert);
+    Map<PatchSet.Id, PatchSet> patchSets = notes.getPatchSets();
+    assertThat(patchSets.get(psId1).getPushCertificate()).isNull();
+    assertThat(patchSets.get(psId2).getPushCertificate())
+        .isEqualTo(trimmedCert);
+    assertThat(notes.getComments()).isEmpty();
+
+    // comment on ps2
+    update = newUpdate(c, changeOwner);
+    update.setPatchSetId(psId2);
+    Timestamp ts = TimeUtil.nowTs();
+    update.insertComment(newPublishedComment(psId2, "a.txt",
+        "uuid1", new CommentRange(1, 2, 3, 4), 1, changeOwner, null, ts,
+        "Comment", (short) 1, commit.name()));
+    update.commit();
+
+    notes = newNotes(c);
+    assertThat(readNote(notes, commit)).isEqualTo(
+        pushCert
+        + "Patch-set: 2\n"
+        + "Revision: " + commit.name() + "\n"
+        + "File: a.txt\n"
+        + "\n"
+        + "1:2-3:4\n"
+        + CommentsInNotesUtil.formatTime(serverIdent, ts) + "\n"
+        + "Author: Change Owner <1@gerrit>\n"
+        + "UUID: uuid1\n"
+        + "Bytes: 7\n"
+        + "Comment\n"
+        + "\n");
+    patchSets = notes.getPatchSets();
+    assertThat(patchSets.get(psId1).getPushCertificate()).isNull();
+    assertThat(patchSets.get(psId2).getPushCertificate())
+        .isEqualTo(trimmedCert);
+    assertThat(notes.getComments()).isNotEmpty();
+  }
+
   @Test
   public void emptyExceptSubject() throws Exception {
     ChangeUpdate update = newUpdate(newChange(), changeOwner);
@@ -1580,4 +1643,10 @@ public class ChangeNotesTest extends AbstractChangeNotesTest {
     assertThat(notes.getDraftComments(otherUserId)).isEmpty();
     assertThat(notes.getComments()).hasSize(2);
   }
+
+  private String readNote(ChangeNotes notes, ObjectId noteId) throws Exception {
+    ObjectId dataId = notes.getNoteMap().getNote(noteId).getData();
+    return new String(
+        rw.getObjectReader().open(dataId, OBJ_BLOB).getCachedBytes(), UTF_8);
+  }
 }