Do authentication re-tries on HTTP POST

There is at least one git server out there (GOGS) that does
not require authentication on the initial GET for
info/refs?service=git-receive-pack but that _does_ require
authentication for the subsequent POST to actually do the push.

This occurs on GOGS with public repositories; for private
repositories it wants authentication up front.

Handle this behavior by adding 401 handling to our POST request.
Note that this is suboptimal; we'll re-send the push data at
least twice if an authentication failure on POST occurs. It
would be much better if the server required authentication
up-front in the GET request.

Added authentication unit tests (using BASIC auth) to the
SmartClientSmartServerTest:

- clone with authentication
- clone with authentication but lacking CredentialsProvider
- clone with authentication and wrong password
- clone with authentication after redirect
- clone with authentication only on POST, but not on GET

Also tested manually in the wild using repositories at try.gogs.io.
That server offers only BASIC auth, so the other paths
(DIGEST, NEGOTIATE, fall back from DIGEST to BASIC) are untested
and I have no way to test them.

* public repository: GET unauthenticated, POST authenticated
  Also tested after clearing the credentials and then entering a
  wrong password: correctly asks three times during the HTTP
  POST for user name and password, then gives up.
* private repository: authentication already on GET; then gets
  applied correctly initially to the POST request, which succeeds.

Also fix the authentication to use the credentials for the redirected
URI if redirects had occurred. We must not present the credentials
for the original URI in that case. Consider a malicious redirect A->B:
this would allow server B to harvest the user credentials for server
A. The unit test for authentication after a redirect also tests for
this.

Bug: 513043
Change-Id: I97ee5058569efa1545a6c6f6edfd2b357c40592a
Signed-off-by: Thomas Wolf <thomas.wolf@paranor.ch>
Signed-off-by: Matthias Sohn <matthias.sohn@sap.com>

1 files changed, 29 insertions, 10 deletions

diff --git a/org.eclipse.jgit.junit.http/src/org/eclipse/jgit/junit/http/AppServer.java b/org.eclipse.jgit.junit.http/src/org/eclipse/jgit/junit/http/AppServer.java
index 69e2cd5957..e257cf65b6 100644
--- a/org.eclipse.jgit.junit.http/src/org/eclipse/jgit/junit/http/AppServer.java
+++ b/org.eclipse.jgit.junit.http/src/org/eclipse/jgit/junit/http/AppServer.java
@@ -55,6 +55,7 @@ import java.net.UnknownHostException;
 import java.nio.file.Files;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Locale;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentMap;
 
@@ -97,6 +98,9 @@ public class AppServer {
 	/** SSL keystore password; must have at least 6 characters. */
 	private static final String keyPassword = "mykeys";
 
+	/** Role for authentication. */
+	private static final String authRole = "can-access";
+
 	static {
 		// Install a logger that throws warning messages.
 		//
@@ -136,10 +140,10 @@ public class AppServer {
 
 	/**
 	 * @param port
-	 *            for https, may be zero to allocate a port dynamically
+	 *            for http, may be zero to allocate a port dynamically
 	 * @param sslPort
 	 *            for https,may be zero to allocate a port dynamically. If
-	 *            negative, the server will be set up without https support..
+	 *            negative, the server will be set up without https support.
 	 * @since 4.9
 	 */
 	public AppServer(int port, int sslPort) {
@@ -264,9 +268,10 @@ public class AppServer {
 		return ctx;
 	}
 
-	public ServletContextHandler authBasic(ServletContextHandler ctx) {
+	public ServletContextHandler authBasic(ServletContextHandler ctx,
+			String... methods) {
 		assertNotYetSetUp();
-		auth(ctx, new BasicAuthenticator());
+		auth(ctx, new BasicAuthenticator(), methods);
 		return ctx;
 	}
 
@@ -301,22 +306,36 @@ public class AppServer {
 		}
 	}
 
-	private void auth(ServletContextHandler ctx, Authenticator authType) {
-		final String role = "can-access";
-
-		AbstractLoginService users = new TestMappedLoginService(role);
+	private ConstraintMapping createConstraintMapping() {
 		ConstraintMapping cm = new ConstraintMapping();
 		cm.setConstraint(new Constraint());
 		cm.getConstraint().setAuthenticate(true);
 		cm.getConstraint().setDataConstraint(Constraint.DC_NONE);
-		cm.getConstraint().setRoles(new String[] { role });
+		cm.getConstraint().setRoles(new String[] { authRole });
 		cm.setPathSpec("/*");
+		return cm;
+	}
+
+	private void auth(ServletContextHandler ctx, Authenticator authType,
+			String... methods) {
+		AbstractLoginService users = new TestMappedLoginService(authRole);
+		List<ConstraintMapping> mappings = new ArrayList<>();
+		if (methods == null || methods.length == 0) {
+			mappings.add(createConstraintMapping());
+		} else {
+			for (String method : methods) {
+				ConstraintMapping cm = createConstraintMapping();
+				cm.setMethod(method.toUpperCase(Locale.ROOT));
+				mappings.add(cm);
+			}
+		}
 
 		ConstraintSecurityHandler sec = new ConstraintSecurityHandler();
 		sec.setRealmName(realm);
 		sec.setAuthenticator(authType);
 		sec.setLoginService(users);
-		sec.setConstraintMappings(new ConstraintMapping[] { cm });
+		sec.setConstraintMappings(
+				mappings.toArray(new ConstraintMapping[mappings.size()]));
 		sec.setHandler(ctx);
 
 		contexts.removeHandler(ctx);