PackCertificateParser: return null if nothing was received

Add test for this case in both the enabled and disabled cases.

Change-Id: If9d12192a2dc9f9dd1eac9844b5a7b0edadc0b34

org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/PushCertificateParserTest.java

@@ -43,55 +43,124 @@
 package org.eclipse.jgit.transport;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotEquals;
+import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
 import java.io.ByteArrayInputStream;
 import java.io.EOFException;
 import java.io.IOException;
 
+import org.eclipse.jgit.errors.PackProtocolException;
 import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.lib.Config;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.Repository;
+import org.junit.Before;
 import org.junit.Test;
 
 /** Test for push certificate parsing. */
 public class PushCertificateParserTest {
-	@Test
+	// Example push certificate generated by C git 2.2.0.
-	public void parseCertFromPktLine() throws Exception {
+	private static final String INPUT = "001ccertificate version 0.1\n"
-		// Example push certificate generated by C git 2.2.0.
+			+ "0041pusher Dave Borowitz <dborowitz@google.com> 1433954361 -0700\n"
-		String input = "001ccertificate version 0.1\n"
+			+ "0024pushee git://localhost/repo.git\n"
-		+ "0041pusher Dave Borowitz <dborowitz@google.com> 1433954361 -0700\n"
+			+ "002anonce 1433954361-bde756572d665bba81d8\n"
-		+ "0024pushee git://localhost/repo.git\n"
+			+ "0005\n"
-		+ "002anonce 1433954361-bde756572d665bba81d8\n"
+			+ "00680000000000000000000000000000000000000000"
-		+ "0005\n"
+			+ " 6c2b981a177396fb47345b7df3e4d3f854c6bea7"
-		+ "00680000000000000000000000000000000000000000"
+			+ " refs/heads/master\n"
-		+ " 6c2b981a177396fb47345b7df3e4d3f854c6bea7"
+			+ "0022-----BEGIN PGP SIGNATURE-----\n"
-		+ " refs/heads/master\n"
+			+ "0016Version: GnuPG v1\n"
-		+ "0022-----BEGIN PGP SIGNATURE-----\n"
+			+ "0005\n"
-		+ "0016Version: GnuPG v1\n"
+			+ "0045iQEcBAABAgAGBQJVeGg5AAoJEPfTicJkUdPkUggH/RKAeI9/i/LduuiqrL/SSdIa\n"
-		+ "0005\n"
+			+ "00459tYaSqJKLbXz63M/AW4Sp+4u+dVCQvnAt/a35CVEnpZz6hN4Kn/tiswOWVJf4CO7\n"
-		+ "0045iQEcBAABAgAGBQJVeGg5AAoJEPfTicJkUdPkUggH/RKAeI9/i/LduuiqrL/SSdIa\n"
+			+ "0045htNubGs5ZMwvD6sLYqKAnrM3WxV/2TbbjzjZW6Jkidz3jz/WRT4SmjGYiEO7aA+V\n"
-		+ "00459tYaSqJKLbXz63M/AW4Sp+4u+dVCQvnAt/a35CVEnpZz6hN4Kn/tiswOWVJf4CO7\n"
+			+ "00454ZdIS9f7sW5VsHHYlNThCA7vH8Uu48bUovFXyQlPTX0pToSgrWV3JnTxDNxfn3iG\n"
-		+ "0045htNubGs5ZMwvD6sLYqKAnrM3WxV/2TbbjzjZW6Jkidz3jz/WRT4SmjGYiEO7aA+V\n"
+			+ "0045IL0zTY/qwVCdXgFownLcs6J050xrrBWIKqfcWr3u4D2aCLyR0v+S/KArr7ulZygY\n"
-		+ "00454ZdIS9f7sW5VsHHYlNThCA7vH8Uu48bUovFXyQlPTX0pToSgrWV3JnTxDNxfn3iG\n"
+			+ "0045+SOklImn8TAZiNxhWtA6ens66IiammUkZYFv7SSzoPLFZT4dC84SmGPWgf94NoQ=\n"
-		+ "0045IL0zTY/qwVCdXgFownLcs6J050xrrBWIKqfcWr3u4D2aCLyR0v+S/KArr7ulZygY\n"
+			+ "000a=XFeC\n"
-		+ "0045+SOklImn8TAZiNxhWtA6ens66IiammUkZYFv7SSzoPLFZT4dC84SmGPWgf94NoQ=\n"
+			+ "0020-----END PGP SIGNATURE-----\n"
-		+ "000a=XFeC\n"
+			+ "0012push-cert-end\n";
-		+ "0020-----END PGP SIGNATURE-----\n"
-		+ "0012push-cert-end\n";
 
-		PacketLineIn pckIn = newPacketLineIn(input);
+	private Repository db;
+
+	@Before
+	public void setUp() {
+		db = new InMemoryRepository(new DfsRepositoryDescription("repo"));
+	}
+
+	private static SignedPushConfig newEnabledConfig() {
 		Config cfg = new Config();
 		cfg.setString("receive", null, "certnonceseed", "sekret");
-		Repository db = new InMemoryRepository(
+		return SignedPushConfig.KEY.parse(cfg);
-				new DfsRepositoryDescription("repo"));
+	}
+
+	private static SignedPushConfig newDisabledConfig() {
+		return SignedPushConfig.KEY.parse(new Config());
+	}
+
+	@Test
+	public void noCert() throws Exception {
+		PushCertificateParser parser =
+				new PushCertificateParser(db, newEnabledConfig());
+		assertTrue(parser.enabled());
+		assertNull(parser.build());
+
+		ObjectId oldId = ObjectId.zeroId();
+		ObjectId newId =
+				ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef");
+		String rawLine =
+				oldId.name() + " " + newId.name() + " refs/heads/master";
+		ReceiveCommand cmd = BaseReceivePack.parseCommand(rawLine);
+
+		parser.addCommand(cmd, rawLine);
+		parser.addCommand(rawLine);
+		assertNull(parser.build());
+	}
 
-		PushCertificateParser parser = new PushCertificateParser(
+	@Test
-				db, new SignedPushConfig(cfg));
+	public void disabled() throws Exception {
+		PacketLineIn pckIn = newPacketLineIn(INPUT);
+		PushCertificateParser parser =
+				new PushCertificateParser(db, newDisabledConfig());
+		assertFalse(parser.enabled());
+		assertNull(parser.build());
+
+		parser.receiveHeader(pckIn, false);
+		parser.addCommand(pckIn.readStringRaw());
+		assertEquals(PushCertificateParser.BEGIN_SIGNATURE, pckIn.readStringRaw());
+		parser.receiveSignature(pckIn);
+		assertNull(parser.build());
+	}
+
+	@Test
+	public void disabledParserStillRequiresCorrectSyntax() throws Exception {
+		PacketLineIn pckIn = newPacketLineIn("001ccertificate version XYZ\n");
+		PushCertificateParser parser =
+				new PushCertificateParser(db, newDisabledConfig());
+		assertFalse(parser.enabled());
+		try {
+			parser.receiveHeader(pckIn, false);
+			fail("Expected PackProtocolException");
+		} catch (PackProtocolException e) {
+			assertEquals(
+					"Push certificate has missing or invalid value for certificate"
+						+ " version: XYZ",
+					e.getMessage());
+		}
+		assertNull(parser.build());
+	}
+
+	@Test
+	public void parseCertFromPktLine() throws Exception {
+		PacketLineIn pckIn = newPacketLineIn(INPUT);
+		PushCertificateParser parser =
+				new PushCertificateParser(db, newEnabledConfig());
 		parser.receiveHeader(pckIn, false);
 		parser.addCommand(pckIn.readStringRaw());
 		assertEquals(PushCertificateParser.BEGIN_SIGNATURE, pckIn.readStringRaw());
@@ -117,9 +186,9 @@ public class PushCertificateParserTest {
 		assertEquals("6c2b981a177396fb47345b7df3e4d3f854c6bea7",
 				cmd.getNewId().name());
 
-		assertEquals(concatPacketLines(input, 0, 6), cert.toText());
+		assertEquals(concatPacketLines(INPUT, 0, 6), cert.toText());
 
-		String signature = concatPacketLines(input, 6, 17);
+		String signature = concatPacketLines(INPUT, 6, 17);
 		assertTrue(signature.startsWith(PushCertificateParser.BEGIN_SIGNATURE));
 		assertTrue(signature.endsWith(PushCertificateParser.END_SIGNATURE));
 		assertEquals(signature, cert.getSignature());

org.eclipse.jgit/src/org/eclipse/jgit/transport/BaseReceivePack.java

@@ -260,7 +260,8 @@ public abstract class BaseReceivePack {
 	 * <p>
 	 * Only valid after commands are read from the wire.
 	 *
-	 * @return the parsed certificate, or null if push certificates are disabled.
+	 * @return the parsed certificate, or null if push certificates are disabled
+	 *         or no cert was presented by the client.
 	 * @throws IOException if the certificate was present but invalid.
 	 * @since 4.1
 	 */

org.eclipse.jgit/src/org/eclipse/jgit/transport/PushCertificateParser.java

@@ -82,6 +82,7 @@ public class PushCertificateParser {
 
 	private static final String END_CERT = "push-cert-end\n"; //$NON-NLS-1$
 
+	private boolean received;
 	private String version;
 	private PushCertificateIdent pusher;
 	private String pushee;
@@ -135,7 +136,7 @@ public class PushCertificateParser {
 	 * @since 4.1
 	 */
 	public PushCertificate build() throws IOException {
-		if (nonceGenerator == null) {
+		if (!received || nonceGenerator == null) {
 			return null;
 		}
 		try {
@@ -210,6 +211,7 @@ public class PushCertificateParser {
 	 */
 	public void receiveHeader(PacketLineIn pckIn, boolean stateless)
 			throws IOException {
+		received = true;
 		try {
 			version = parseHeader(pckIn, VERSION);
 			if (!version.equals(VERSION_0_1)) {
@@ -255,6 +257,7 @@ public class PushCertificateParser {
 	 * @since 4.0
 	 */
 	public void receiveSignature(PacketLineIn pckIn) throws IOException {
+		received = true;
 		try {
 			StringBuilder sig = new StringBuilder(BEGIN_SIGNATURE);
 			String line;