From 8d4f8d55d3d14b51983f951568a2f4e4ff64b324 Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Wed, 17 Oct 2018 16:52:30 -0700 Subject: [PATCH 1/2] UploadPack: Return correct peer user agent on v2 requests UploadPack.getPeerUserAgent() doesn't produce the expected results for protocol v2 requests. In v2, the agent reported in the request (in an "agent=" line) is not in the clientCapabilities but in a field on its own. This makes getPeerUserAgent default to the transport user agent. Making "agent" a shared property between protocol v0/v1 and v2 fixes the problem, simplifies the function and harmonizes the implementation between protocol versions. In a follow up commit the "agent" will be identified on parsing time, instead of taking it from the client capabilities. Change-Id: Idf9825ec4e0b81a1458c8e3701f3e28aafd8a32a Signed-off-by: Ivan Frade --- .../jgit/transport/UploadPackTest.java | 70 +++++++++++++++---- .../eclipse/jgit/transport/FetchRequest.java | 24 ++++++- .../jgit/transport/FetchV0Request.java | 29 ++++++-- .../jgit/transport/FetchV2Request.java | 15 +--- .../eclipse/jgit/transport/UploadPack.java | 7 +- 5 files changed, 109 insertions(+), 36 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java index aaef45883..b74dcca58 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java @@ -429,17 +429,7 @@ public class UploadPackTest { RefFilter refFilter, ProtocolV2Hook hook, String... inputLines) throws Exception { - ByteArrayOutputStream send = new ByteArrayOutputStream(); - PacketLineOut pckOut = new PacketLineOut(send); - for (String line : inputLines) { - if (line == PacketLineIn.END) { - pckOut.end(); - } else if (line == PacketLineIn.DELIM) { - pckOut.writeDelim(); - } else { - pckOut.writeString(line); - } - } + ByteArrayInputStream send = linesAsInputStream(inputLines); server.getConfig().setString("protocol", null, "version", "2"); UploadPack up = new UploadPack(server); @@ -453,11 +443,28 @@ public class UploadPackTest { } ByteArrayOutputStream recv = new ByteArrayOutputStream(); - up.upload(new ByteArrayInputStream(send.toByteArray()), recv, null); + up.upload(send, recv, null); return new ByteArrayInputStream(recv.toByteArray()); } + private static ByteArrayInputStream linesAsInputStream(String... inputLines) + throws IOException { + try (ByteArrayOutputStream send = new ByteArrayOutputStream()) { + PacketLineOut pckOut = new PacketLineOut(send); + for (String line : inputLines) { + if (line == PacketLineIn.END) { + pckOut.end(); + } else if (line == PacketLineIn.DELIM) { + pckOut.writeDelim(); + } else { + pckOut.writeString(line); + } + } + return new ByteArrayInputStream(send.toByteArray()); + } + } + /* * Invokes UploadPack with protocol v2 and sends it the given lines. * Returns UploadPack's output stream, not including the capability @@ -1552,6 +1559,45 @@ public class UploadPackTest { assertTrue(client.hasObject(three.toObjectId())); } + @Test + public void testGetPeerAgentProtocolV0() throws Exception { + RevCommit one = remote.commit().message("1").create(); + remote.update("one", one); + + UploadPack up = new UploadPack(server); + ByteArrayInputStream send = linesAsInputStream( + "want " + one.getName() + " agent=JGit-test/1.2.3\n", + PacketLineIn.END, + "have 11cedf1b796d44207da702f7d420684022fc0f09\n", "done\n"); + + ByteArrayOutputStream recv = new ByteArrayOutputStream(); + up.upload(send, recv, null); + + assertEquals(up.getPeerUserAgent(), "JGit-test/1.2.3"); + } + + @Test + public void testGetPeerAgentProtocolV2() throws Exception { + server.getConfig().setString("protocol", null, "version", "2"); + + RevCommit one = remote.commit().message("1").create(); + remote.update("one", one); + + UploadPack up = new UploadPack(server); + up.setExtraParameters(Sets.of("version=2")); + + ByteArrayInputStream send = linesAsInputStream( + "command=fetch\n", "agent=JGit-test/1.2.4\n", + PacketLineIn.DELIM, "want " + one.getName() + "\n", + "have 11cedf1b796d44207da702f7d420684022fc0f09\n", "done\n", + PacketLineIn.END); + + ByteArrayOutputStream recv = new ByteArrayOutputStream(); + up.upload(send, recv, null); + + assertEquals(up.getPeerUserAgent(), "JGit-test/1.2.4"); + } + private static class RejectAllRefFilter implements RefFilter { @Override public Map filter(Map refs) { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchRequest.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchRequest.java index 5d28a4d9f..40ba3a3ad 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchRequest.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchRequest.java @@ -48,6 +48,7 @@ import java.util.List; import java.util.Set; import org.eclipse.jgit.annotations.NonNull; +import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.lib.ObjectId; /** @@ -69,6 +70,9 @@ abstract class FetchRequest { final List deepenNotRefs; + @Nullable + final String agent; + /** * Initialize the common fields of a fetch request. * @@ -88,11 +92,13 @@ abstract class FetchRequest { * @param deepenSince * Requests that the shallow clone/fetch should be cut at a * specific time, instead of depth + * @param agent + * agent as reported by the client in the request body */ FetchRequest(@NonNull Set wantIds, int depth, @NonNull Set clientShallowCommits, long filterBlobLimit, @NonNull Set clientCapabilities, int deepenSince, - @NonNull List deepenNotRefs) { + @NonNull List deepenNotRefs, @Nullable String agent) { this.wantIds = requireNonNull(wantIds); this.depth = depth; this.clientShallowCommits = requireNonNull(clientShallowCommits); @@ -100,6 +106,7 @@ abstract class FetchRequest { this.clientCapabilities = requireNonNull(clientCapabilities); this.deepenSince = deepenSince; this.deepenNotRefs = requireNonNull(deepenNotRefs); + this.agent = agent; } /** @@ -146,7 +153,11 @@ abstract class FetchRequest { * These options are listed and well-defined in the git protocol * specification. * - * @return capabilities sent by the client + * The agent capability is not included in this set. It can be retrieved via + * {@link #getAgent()}. + * + * @return capabilities sent by the client (excluding the "agent" + * capability) */ @NonNull Set getClientCapabilities() { @@ -171,4 +182,13 @@ abstract class FetchRequest { List getDeepenNotRefs() { return deepenNotRefs; } + + /** + * @return string identifying the agent (as sent in the request body by the + * client) + */ + @Nullable + String getAgent() { + return agent; + } } \ No newline at end of file diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV0Request.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV0Request.java index bb358c886..5a97036eb 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV0Request.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV0Request.java @@ -48,6 +48,7 @@ import java.util.HashSet; import java.util.Set; import org.eclipse.jgit.annotations.NonNull; +import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.lib.ObjectId; /** @@ -57,9 +58,9 @@ final class FetchV0Request extends FetchRequest { FetchV0Request(@NonNull Set wantIds, int depth, @NonNull Set clientShallowCommits, long filterBlobLimit, - @NonNull Set clientCapabilities) { + @NonNull Set clientCapabilities, @Nullable String agent) { super(wantIds, depth, clientShallowCommits, filterBlobLimit, - clientCapabilities, 0, Collections.emptyList()); + clientCapabilities, 0, Collections.emptyList(), agent); } static final class Builder { @@ -74,6 +75,8 @@ final class FetchV0Request extends FetchRequest { final Set clientCaps = new HashSet<>(); + String agent; + /** * @param objectId * object id received in a "want" line @@ -111,7 +114,24 @@ final class FetchV0Request extends FetchRequest { * @return this builder */ Builder addClientCapabilities(Collection clientCapabilities) { - clientCaps.addAll(clientCapabilities); + for (String cap: clientCapabilities) { + // TODO(ifrade): Do this is done on parse time + if (cap.startsWith("agent=")) { //$NON-NLS-1$ + agent = cap.substring("agent=".length()); //$NON-NLS-1$ + } else { + clientCaps.add(cap); + } + } + return this; + } + + /** + * @param clientAgent + * agent line sent by the client in the request body + * @return this builder + */ + Builder setAgent(String clientAgent) { + agent = clientAgent; return this; } @@ -127,7 +147,8 @@ final class FetchV0Request extends FetchRequest { FetchV0Request build() { return new FetchV0Request(wantIds, depth, clientShallowCommits, - filterBlobLimit, clientCaps); + filterBlobLimit, clientCaps, agent); } + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java index 951d2d79e..8e36a109e 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java @@ -71,9 +71,6 @@ public final class FetchV2Request extends FetchRequest { private final boolean doneReceived; - @Nullable - private final String agent; - @NonNull private final List serverOptions; @@ -86,11 +83,10 @@ public final class FetchV2Request extends FetchRequest { boolean doneReceived, @NonNull Set clientCapabilities, @Nullable String agent, @NonNull List serverOptions) { super(wantIds, depth, clientShallowCommits, filterBlobLimit, - clientCapabilities, deepenSince, deepenNotRefs); + clientCapabilities, deepenSince, deepenNotRefs, agent); this.peerHas = requireNonNull(peerHas); this.wantedRefs = requireNonNull(wantedRefs); this.doneReceived = doneReceived; - this.agent = agent; this.serverOptions = requireNonNull(serverOptions); } @@ -117,15 +113,6 @@ public final class FetchV2Request extends FetchRequest { return doneReceived; } - /** - * @return string identifying the agent (as sent in the request body by the - * client) - */ - @Nullable - String getAgent() { - return agent; - } - /** * Options received in server-option lines. The caller can choose to act on * these in an application-specific way diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index 535c914fc..fde9ce917 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -1369,12 +1369,11 @@ public class UploadPack { * @since 4.0 */ public String getPeerUserAgent() { - if (currentRequest == null) { - return userAgent; + if (currentRequest != null && currentRequest.getAgent() != null) { + return currentRequest.getAgent(); } - return UserAgent.getAgent(currentRequest.getClientCapabilities(), - userAgent); + return userAgent; } private boolean negotiate(FetchRequest req, From 94a3d8bae9ff9a04e32ef5b82539ba70af0649bf Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Thu, 18 Oct 2018 10:54:14 -0700 Subject: [PATCH 2/2] UploadPack v0: Extract "agent" client capability at parse time The request receives a list of capabilities and takes out the "agent" to offer it on its own setter (getAgent). Do this at parse time: when reading the line if the capability is "agent" set it directly in the builder. This makes the treatment of "agent" consistent in v0/v1 and v2. Change-Id: Ie4f9f2cad8639adeeaef4921df49a30a8ce5b42f Signed-off-by: Ivan Frade --- .../transport/parser/FirstWantTest.java | 15 ++++++-- .../internal/transport/parser/FirstWant.java | 38 ++++++++++++++++--- .../jgit/transport/FetchV0Request.java | 9 +---- .../jgit/transport/ProtocolV0Parser.java | 1 + .../eclipse/jgit/transport/UploadPack.java | 5 +++ 5 files changed, 51 insertions(+), 17 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/parser/FirstWantTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/parser/FirstWantTest.java index 627079eac..b877c598e 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/parser/FirstWantTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/transport/parser/FirstWantTest.java @@ -43,6 +43,7 @@ package org.eclipse.jgit.internal.transport.parser; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; @@ -66,9 +67,9 @@ public class FirstWantTest { r.getLine()); Set capabilities = r.getCapabilities(); Set expectedCapabilities = new HashSet<>( - Arrays.asList("no-progress", "include-tag", "ofs-delta", - "agent=JGit/unknown")); + Arrays.asList("no-progress", "include-tag", "ofs-delta")); assertEquals(expectedCapabilities, capabilities); + assertEquals("JGit/unknown", r.getAgent()); } @Test @@ -79,6 +80,7 @@ public class FirstWantTest { assertEquals("want b9d4d1eb2f93058814480eae9e1b67550f46ee38", r.getLine()); assertTrue(r.getCapabilities().isEmpty()); + assertNull(r.getAgent()); } private String makeFirstWantLine(String capability) { @@ -110,7 +112,7 @@ public class FirstWantTest { List validNames = Arrays.asList( "c", "cap", "C", "CAP", "1", "1cap", "cap-64k_test", "-", "-cap", - "_", "_cap", "agent=pack.age/Version"); + "_", "_cap"); for (String capability: validNames) { FirstWant r = FirstWant.fromLine(makeFirstWantLine(capability)); @@ -118,4 +120,11 @@ public class FirstWantTest { assertTrue(r.getCapabilities().contains(capability)); } } + + @Test + public void testFirstWantValidAgentName() throws PackProtocolException { + FirstWant r = FirstWant.fromLine(makeFirstWantLine("agent=pack.age/Version")); + assertEquals(r.getCapabilities().size(), 0); + assertEquals("pack.age/Version", r.getAgent()); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/parser/FirstWant.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/parser/FirstWant.java index 1ac9b1887..2dae02170 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/parser/FirstWant.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/transport/parser/FirstWant.java @@ -42,11 +42,13 @@ */ package org.eclipse.jgit.internal.transport.parser; -import java.util.Arrays; +import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_AGENT; + import java.util.Collections; import java.util.HashSet; import java.util.Set; +import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.errors.PackProtocolException; import org.eclipse.jgit.internal.JGitText; @@ -72,6 +74,11 @@ public class FirstWant { private final Set capabilities; + @Nullable + private final String agent; + + private static final String AGENT_PREFIX = OPTION_AGENT + '='; + /** * Parse the first want line in the protocol v0/v1 pack negotiation. * @@ -84,6 +91,7 @@ public class FirstWant { public static FirstWant fromLine(String line) throws PackProtocolException { String wantLine; Set capabilities; + String agent = null; if (line.length() > 45) { String opt = line.substring(45); @@ -91,8 +99,15 @@ public class FirstWant { throw new PackProtocolException(JGitText.get().wantNoSpaceWithCapabilities); } opt = opt.substring(1); - HashSet opts = new HashSet<>( - Arrays.asList(opt.split(" "))); //$NON-NLS-1$ + + HashSet opts = new HashSet<>(); + for (String clientCapability : opt.split(" ")) { //$NON-NLS-1$ + if (clientCapability.startsWith(AGENT_PREFIX)) { + agent = clientCapability.substring(AGENT_PREFIX.length()); + } else { + opts.add(clientCapability); + } + } wantLine = line.substring(0, 45); capabilities = Collections.unmodifiableSet(opts); } else { @@ -100,12 +115,14 @@ public class FirstWant { capabilities = Collections.emptySet(); } - return new FirstWant(wantLine, capabilities); + return new FirstWant(wantLine, capabilities, agent); } - private FirstWant(String line, Set capabilities) { + private FirstWant(String line, Set capabilities, + @Nullable String agent) { this.line = line; this.capabilities = capabilities; + this.agent = agent; } /** @return non-capabilities part of the line. */ @@ -113,8 +130,17 @@ public class FirstWant { return line; } - /** @return capabilities parsed from the line as an immutable set. */ + /** + * @return capabilities parsed from the line as an immutable set (excluding + * agent). + */ public Set getCapabilities() { return capabilities; } + + /** @return client user agent parsed from the line. */ + @Nullable + public String getAgent() { + return agent; + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV0Request.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV0Request.java index 5a97036eb..05f4a8155 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV0Request.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV0Request.java @@ -114,14 +114,7 @@ final class FetchV0Request extends FetchRequest { * @return this builder */ Builder addClientCapabilities(Collection clientCapabilities) { - for (String cap: clientCapabilities) { - // TODO(ifrade): Do this is done on parse time - if (cap.startsWith("agent=")) { //$NON-NLS-1$ - agent = cap.substring("agent=".length()); //$NON-NLS-1$ - } else { - clientCaps.add(cap); - } - } + clientCaps.addAll(clientCapabilities); return this; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV0Parser.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV0Parser.java index 60d5fff6d..21498d6f5 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV0Parser.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV0Parser.java @@ -143,6 +143,7 @@ final class ProtocolV0Parser { if (line.length() > 45) { FirstWant firstLine = FirstWant.fromLine(line); reqBuilder.addClientCapabilities(firstLine.getCapabilities()); + reqBuilder.setAgent(firstLine.getAgent()); line = firstLine.getLine(); } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java index fde9ce917..b69f2cd92 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java @@ -209,6 +209,11 @@ public class UploadPack { /** @return capabilities parsed from the line. */ public Set getOptions() { + if (firstWant.getAgent() != null) { + Set caps = new HashSet<>(firstWant.getCapabilities()); + caps.add(OPTION_AGENT + '=' + firstWant.getAgent()); + return caps; + } return firstWant.getCapabilities(); } }