Merge branch 'stable-6.2'

* stable-6.2:
  HTTP Smart: set correct HTTP status on error

Change-Id: I7bf99b0c720f9dabb65da5cc777281a1d227f5a8

4 files changed, 56 insertions, 23 deletions

diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/GitSmartHttpTools.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/GitSmartHttpTools.java
index 012f9a3dc0..f1155dcf57 100644
--- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/GitSmartHttpTools.java
+++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/GitSmartHttpTools.java
@@ -158,11 +158,11 @@ public class GitSmartHttpTools {
 		}
 
 		if (isInfoRefs(req)) {
-			sendInfoRefsError(req, res, textForGit);
+			sendInfoRefsError(req, res, textForGit, httpStatus);
 		} else if (isUploadPack(req)) {
-			sendUploadPackError(req, res, textForGit);
+			sendUploadPackError(req, res, textForGit, httpStatus);
 		} else if (isReceivePack(req)) {
-			sendReceivePackError(req, res, textForGit);
+			sendReceivePackError(req, res, textForGit, httpStatus);
 		} else {
 			if (httpStatus < 400)
 				ServletUtils.consumeRequestBody(req);
@@ -171,29 +171,32 @@ public class GitSmartHttpTools {
 	}
 
 	private static void sendInfoRefsError(HttpServletRequest req,
-			HttpServletResponse res, String textForGit) throws IOException {
+			HttpServletResponse res, String textForGit, int httpStatus)
+			throws IOException {
 		ByteArrayOutputStream buf = new ByteArrayOutputStream(128);
 		PacketLineOut pck = new PacketLineOut(buf);
 		String svc = req.getParameter("service");
 		pck.writeString("# service=" + svc + "\n");
 		pck.end();
 		pck.writeString("ERR " + textForGit);
-		send(req, res, infoRefsResultType(svc), buf.toByteArray());
+		send(req, res, infoRefsResultType(svc), buf.toByteArray(), httpStatus);
 	}
 
 	private static void sendUploadPackError(HttpServletRequest req,
-			HttpServletResponse res, String textForGit) throws IOException {
+			HttpServletResponse res, String textForGit, int httpStatus)
+			throws IOException {
 		// Do not use sideband. Sideband is acceptable only while packfile is
 		// being sent. Other places, like acknowledgement section, do not
 		// support sideband. Use an error packet.
 		ByteArrayOutputStream buf = new ByteArrayOutputStream(128);
 		PacketLineOut pckOut = new PacketLineOut(buf);
 		writePacket(pckOut, textForGit);
-		send(req, res, UPLOAD_PACK_RESULT_TYPE, buf.toByteArray());
+		send(req, res, UPLOAD_PACK_RESULT_TYPE, buf.toByteArray(), httpStatus);
 	}
 
 	private static void sendReceivePackError(HttpServletRequest req,
-			HttpServletResponse res, String textForGit) throws IOException {
+			HttpServletResponse res, String textForGit, int httpStatus)
+			throws IOException {
 		ByteArrayOutputStream buf = new ByteArrayOutputStream(128);
 		PacketLineOut pckOut = new PacketLineOut(buf);
 
@@ -212,7 +215,7 @@ public class GitSmartHttpTools {
 			writeSideBand(buf, textForGit);
 		else
 			writePacket(pckOut, textForGit);
-		send(req, res, RECEIVE_PACK_RESULT_TYPE, buf.toByteArray());
+		send(req, res, RECEIVE_PACK_RESULT_TYPE, buf.toByteArray(), httpStatus);
 	}
 
 	private static boolean isReceivePackSideBand(HttpServletRequest req) {
@@ -246,9 +249,9 @@ public class GitSmartHttpTools {
 	}
 
 	private static void send(HttpServletRequest req, HttpServletResponse res,
-			String type, byte[] buf) throws IOException {
+			String type, byte[] buf, int httpStatus) throws IOException {
 		ServletUtils.consumeRequestBody(req);
-		res.setStatus(HttpServletResponse.SC_OK);
+		res.setStatus(httpStatus);
 		res.setContentType(type);
 		res.setContentLength(buf.length);
 		try (OutputStream os = res.getOutputStream()) {
diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java
index 77ed09cdfb..d3437b7eb9 100644
--- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java
+++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/UploadPackServlet.java
@@ -10,6 +10,7 @@
 
 package org.eclipse.jgit.http.server;
 
+import static javax.servlet.http.HttpServletResponse.SC_BAD_REQUEST;
 import static javax.servlet.http.HttpServletResponse.SC_FORBIDDEN;
 import static javax.servlet.http.HttpServletResponse.SC_INTERNAL_SERVER_ERROR;
 import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED;
@@ -48,6 +49,7 @@ import org.eclipse.jgit.transport.RefAdvertiser.PacketLineOutRefAdvertiser;
 import org.eclipse.jgit.transport.ServiceMayNotContinueException;
 import org.eclipse.jgit.transport.UploadPack;
 import org.eclipse.jgit.transport.UploadPackInternalServerErrorException;
+import org.eclipse.jgit.transport.WantNotValidException;
 import org.eclipse.jgit.transport.resolver.ServiceNotAuthorizedException;
 import org.eclipse.jgit.transport.resolver.ServiceNotEnabledException;
 import org.eclipse.jgit.transport.resolver.UploadPackFactory;
@@ -151,6 +153,16 @@ class UploadPackServlet extends HttpServlet {
 		}
 	}
 
+	private static int statusCodeForThrowable(Throwable error) {
+		if (error instanceof ServiceNotEnabledException) {
+			return SC_FORBIDDEN;
+		}
+		if (error instanceof WantNotValidException) {
+			return SC_BAD_REQUEST;
+		}
+		return SC_INTERNAL_SERVER_ERROR;
+	}
+
 	private final UploadPackErrorHandler handler;
 
 	UploadPackServlet(@Nullable UploadPackErrorHandler handler) {
@@ -216,9 +228,12 @@ class UploadPackServlet extends HttpServlet {
 			log(up.getRepository(), e);
 			if (!rsp.isCommitted()) {
 				rsp.reset();
-				String msg = e instanceof PackProtocolException ? e.getMessage()
-						: null;
-				sendError(req, rsp, SC_INTERNAL_SERVER_ERROR, msg);
+				String msg = null;
+				if (e instanceof PackProtocolException
+						|| e instanceof ServiceNotEnabledException) {
+					msg = e.getMessage();
+				}
+				sendError(req, rsp, statusCodeForThrowable(e), msg);
 			}
 		}
 	}
diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HttpClientTests.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HttpClientTests.java
index df093c185b..3438c52c8d 100644
--- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HttpClientTests.java
+++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/HttpClientTests.java
@@ -23,6 +23,7 @@ import java.io.File;
 import java.io.OutputStream;
 import java.net.URI;
 import java.net.URL;
+import java.text.MessageFormat;
 import java.util.List;
 
 import javax.servlet.http.HttpServletRequest;
@@ -308,7 +309,10 @@ public class HttpClientTests extends AllFactoriesHttpTestCase {
 				fail("connection opened even though service disabled");
 			} catch (TransportException err) {
 				String exp = smartAuthNoneURI + ": "
-						+ JGitText.get().serviceNotEnabledNoName;
+						+ MessageFormat.format(
+								JGitText.get().serviceNotPermitted,
+								smartAuthNoneURI.toString() + "/",
+								"git-upload-pack");
 				assertEquals(exp, err.getMessage());
 			}
 		}
diff --git a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
index 887e970a0c..8f3888e4d5 100644
--- a/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
+++ b/org.eclipse.jgit.http.test/tst/org/eclipse/jgit/http/test/SmartClientSmartServerTest.java
@@ -57,7 +57,7 @@ import org.eclipse.jetty.servlet.ServletContextHandler;
 import org.eclipse.jetty.servlet.ServletHolder;
 import org.eclipse.jgit.api.Git;
 import org.eclipse.jgit.api.TransportConfigCallback;
-import org.eclipse.jgit.errors.RemoteRepositoryException;
+import org.eclipse.jgit.errors.NoRemoteRepositoryException;
 import org.eclipse.jgit.errors.TransportException;
 import org.eclipse.jgit.errors.UnsupportedCredentialItem;
 import org.eclipse.jgit.http.server.GitServlet;
@@ -496,8 +496,9 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 			try {
 				t.openFetch();
 				fail("fetch connection opened");
-			} catch (RemoteRepositoryException notFound) {
-				assertEquals(uri + ": Git repository not found",
+			} catch (NoRemoteRepositoryException notFound) {
+				assertEquals(uri + ": " + uri
+						+ "/info/refs?service=git-upload-pack not found: Not Found",
 						notFound.getMessage());
 			}
 		}
@@ -510,7 +511,7 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 		assertEquals(join(uri, "info/refs"), info.getPath());
 		assertEquals(1, info.getParameters().size());
 		assertEquals("git-upload-pack", info.getParameter("service"));
-		assertEquals(200, info.getStatus());
+		assertEquals(404, info.getStatus());
 		assertEquals("application/x-git-upload-pack-advertisement",
 				info.getResponseHeader(HDR_CONTENT_TYPE));
 	}
@@ -536,8 +537,9 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 							Collections.singletonList(
 									new RefSpec(unreachableCommit.name()))));
 			assertTrue(e.getMessage().contains(
-					"want " + unreachableCommit.name() + " not valid"));
+					"Bad Request"));
 		}
+		assertLastRequestStatusCode(400);
 	}
 
 	@Test
@@ -558,8 +560,9 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 					() -> t.fetch(NullProgressMonitor.INSTANCE,
 							Collections.singletonList(new RefSpec(A.name()))));
 			assertTrue(
-					e.getMessage().contains("want " + A.name() + " not valid"));
+					e.getMessage().contains("Bad Request"));
 		}
+		assertLastRequestStatusCode(400);
 	}
 
 	@Test
@@ -916,6 +919,7 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 		} catch (TransportException e) {
 			assertTrue(e.getMessage().contains("301"));
 		}
+		assertLastRequestStatusCode(301);
 	}
 
 	@Test
@@ -934,6 +938,7 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 			assertTrue(
 					e.getMessage().contains("http.followRedirects is false"));
 		}
+		assertLastRequestStatusCode(301);
 	}
 
 	private void assertFetchRequests(List<AccessEvent> requests, int index) {
@@ -1605,8 +1610,9 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 			fail("Server accepted want " + id.name());
 		} catch (TransportException err) {
 			assertTrue(err.getMessage()
-					.contains("want " + id.name() + " not valid"));
+					.contains("Bad Request"));
 		}
+		assertLastRequestStatusCode(400);
 	}
 
 	@Test
@@ -1650,7 +1656,7 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 				fail("Successfully served ref with value " + c.getRef(master));
 			} catch (TransportException err) {
 				assertTrue("Unexpected exception message " + err.getMessage(),
-						err.getMessage().contains("Internal server error"));
+						err.getMessage().contains("Server Error"));
 			}
 		} finally {
 			noRefServer.tearDown();
@@ -1821,6 +1827,11 @@ public class SmartClientSmartServerTest extends AllProtocolsHttpTestCase {
 				.getResponseHeader(HDR_CONTENT_TYPE));
 	}
 
+	private void assertLastRequestStatusCode(int statusCode) {
+		List<AccessEvent> requests = getRequests();
+		assertEquals(statusCode, requests.get(requests.size() - 1).getStatus());
+	}
+
 	private void enableReceivePack() throws IOException {
 		final StoredConfig cfg = remoteRepository.getConfig();
 		cfg.setBoolean("http", null, "receivepack", true);