Merge branch 'stable-6.1' into stable-6.2 * stable-6.1: HTTP Smart: set correct HTTP status on error Change-Id: I792d6cdfe9e76a3d2e6d1e01ec1dc96805ed2900
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 012f9a3..f1155dc 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 static void sendError(HttpServletRequest req, } 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 static void sendError(HttpServletRequest req, } 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 @@ private static void sendReceivePackError(HttpServletRequest req, 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 @@ private static void writePacket(PacketLineOut pckOut, String textForGit) } 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 77ed09c..d3437b7 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.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 @@ public void destroy() { } } + 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 @@ private void defaultUploadPackHandler(HttpServletRequest req, 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 df093c1..3438c52 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.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 void testListRemote_Smart_UploadPackDisabled() throws Exception { 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 887e970..8f3888e 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.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 void testListRemote_BadName() throws IOException, URISyntaxException { 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 void testListRemote_BadName() throws IOException, URISyntaxException { 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 void testFetchBySHA1Unreachable() throws Exception { Collections.singletonList( new RefSpec(unreachableCommit.name())))); assertTrue(e.getMessage().contains( - "want " + unreachableCommit.name() + " not valid")); + "Bad Request")); } + assertLastRequestStatusCode(400); } @Test @@ -558,8 +560,9 @@ protected Map<String, Ref> getAdvertisedRefs(Repository repository, () -> 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 void testInitialClone_RedirectOnPostForbidden() throws Exception { } catch (TransportException e) { assertTrue(e.getMessage().contains("301")); } + assertLastRequestStatusCode(301); } @Test @@ -934,6 +938,7 @@ public void testInitialClone_RedirectForbidden() throws Exception { assertTrue( e.getMessage().contains("http.followRedirects is false")); } + assertLastRequestStatusCode(301); } private void assertFetchRequests(List<AccessEvent> requests, int index) { @@ -1605,8 +1610,9 @@ public void testInvalidWant() throws Exception { 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 void testFetch_RefsUnreadableOnUpload() throws Exception { 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 void testPush_ChunkedEncoding() throws Exception { .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);