From 54b195ad3eeed88f20774f2aa1a5d6b19f59b0d7 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Tue, 2 Jun 2015 16:13:16 -0700 Subject: [PATCH 1/4] Clarify description of ServiceNotAuthorizedException This exception's detail message states Service not permitted and according to the Javadoc it indicates that the current user does not have access to the service. In practice, though, callers handle this exception by presenting a '401 Unauthorized' response to the client, meaning that the user is unauthenticated and should authenticate. Clarify the documentation and detail message to match the practice. The exception message is not used anywhere except logs. No client-visible effect intended. Change-Id: I2c6be9cb74af932f0dcb121a381a64f2ad876766 Signed-off-by: Jonathan Nieder --- .../org/eclipse/jgit/internal/JGitText.properties | 2 +- .../src/org/eclipse/jgit/internal/JGitText.java | 2 +- .../resolver/ServiceNotAuthorizedException.java | 11 ++++++++--- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index db9a68458..dbe5973a7 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -531,7 +531,6 @@ selectingCommits=Selecting commits sequenceTooLargeForDiffAlgorithm=Sequence too large for difference algorithm. serviceNotEnabledNoName=Service not enabled serviceNotPermitted={0} not permitted -serviceNotPermittedNoName=Service not permitted shallowCommitsAlreadyInitialized=Shallow commits have already been initialized shortCompressedStreamAt=Short compressed stream at {0} shortReadOfBlock=Short read of block. @@ -602,6 +601,7 @@ unableToCheckConnectivity=Unable to check connectivity. unableToCreateNewObject=Unable to create new object: {0} unableToStore=Unable to store {0}. unableToWrite=Unable to write {0} +unauthorized=Unauthorized unencodeableFile=Unencodable file: {0} unexpectedCompareResult=Unexpected metadata comparison result: {0} unexpectedEndOfConfigFile=Unexpected end of config file diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index 9f6efef57..494114f70 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -590,7 +590,6 @@ public static JGitText get() { /***/ public String sequenceTooLargeForDiffAlgorithm; /***/ public String serviceNotEnabledNoName; /***/ public String serviceNotPermitted; - /***/ public String serviceNotPermittedNoName; /***/ public String shallowCommitsAlreadyInitialized; /***/ public String shortCompressedStreamAt; /***/ public String shortReadOfBlock; @@ -661,6 +660,7 @@ public static JGitText get() { /***/ public String unableToCreateNewObject; /***/ public String unableToStore; /***/ public String unableToWrite; + /***/ public String unauthorized; /***/ public String unencodeableFile; /***/ public String unexpectedCompareResult; /***/ public String unexpectedEndOfConfigFile; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/resolver/ServiceNotAuthorizedException.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/resolver/ServiceNotAuthorizedException.java index a2c6edc82..3c16556b4 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/resolver/ServiceNotAuthorizedException.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/resolver/ServiceNotAuthorizedException.java @@ -45,12 +45,17 @@ import org.eclipse.jgit.internal.JGitText; -/** Indicates the request service is not authorized for current user. */ +/** + * Indicates that the requested service requires authentication that + * the current user has not provided. + *

+ * This corresponds to response code + * {@link javax.servlet.http.HttpServletResponse#SC_UNAUTHORIZED}. + */ public class ServiceNotAuthorizedException extends Exception { private static final long serialVersionUID = 1L; - /** Indicates the request service is not available. */ public ServiceNotAuthorizedException() { - super(JGitText.get().serviceNotPermittedNoName); + super(JGitText.get().unauthorized); } } From 761e61f1ede1ec3ddeca9b7a6f959f43bb6ee8bb Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Tue, 2 Jun 2015 16:14:19 -0700 Subject: [PATCH 2/4] dumb HTTP: Clarify AsIsFilter by introducing req and res locals No functional change. Change-Id: I945ba18879c360f433e026aa125ef3f9f6a75793 Signed-off-by: Jonathan Nieder --- .../src/org/eclipse/jgit/http/server/AsIsFileFilter.java | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/AsIsFileFilter.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/AsIsFileFilter.java index b2250e3ef..e8d809659 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/AsIsFileFilter.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/AsIsFileFilter.java @@ -80,14 +80,16 @@ public void destroy() { public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { + HttpServletRequest req = (HttpServletRequest) request; + HttpServletResponse res = (HttpServletResponse) response; try { final Repository db = getRepository(request); - asIs.access((HttpServletRequest) request, db); + asIs.access(req, db); chain.doFilter(request, response); } catch (ServiceNotAuthorizedException e) { - ((HttpServletResponse) response).sendError(SC_UNAUTHORIZED); + res.sendError(SC_UNAUTHORIZED); } catch (ServiceNotEnabledException e) { - ((HttpServletResponse) response).sendError(SC_FORBIDDEN); + res.sendError(SC_FORBIDDEN); } } } From cc4f4f2fe14cb56611d42af34b0baa51bd6632aa Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Tue, 2 Jun 2015 16:40:23 -0700 Subject: [PATCH 3/4] Use message from ServiceNotAuthorizedException, ServiceNotEnabledException When sending an error response due to ServiceNotAuthorizedException or ServiceNotEnabledException, usually we send a default message. In the ServiceNotEnabledException case, we use 403 Git access forbidden except in a dumb-HTTP-specific filter where we use the servlet container's default 403 response: 403 Forbidden In the ServiceNotAuthorizedException case, we use the servlet container's default 401 response: 401 Unauthorized There is one exception: a ServiceNotEnabledException when handling a smart HTTP /info/refs request uses the message from the exception: 403 Service not enabled Be more consistent by always using the message from the exception. This way, authors of a RepositoryResolver, UploadPackFactory, or ReceivePackFactory can provide a more detailed message when appropriate. The defaults are 401 Unauthorized 403 Service not enabled Change-Id: Id1fe1c2042fb96487c3671c1965c8a65c4b8e1b8 Signed-off-by: Jonathan Nieder --- .../src/org/eclipse/jgit/http/server/AsIsFileFilter.java | 4 ++-- .../org/eclipse/jgit/http/server/ReceivePackServlet.java | 5 ++--- .../org/eclipse/jgit/http/server/RepositoryFilter.java | 4 ++-- .../eclipse/jgit/http/server/SmartServiceInfoRefs.java | 8 +++----- .../org/eclipse/jgit/http/server/UploadPackServlet.java | 5 ++--- 5 files changed, 11 insertions(+), 15 deletions(-) diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/AsIsFileFilter.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/AsIsFileFilter.java index e8d809659..d33362b4b 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/AsIsFileFilter.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/AsIsFileFilter.java @@ -87,9 +87,9 @@ public void doFilter(ServletRequest request, ServletResponse response, asIs.access(req, db); chain.doFilter(request, response); } catch (ServiceNotAuthorizedException e) { - res.sendError(SC_UNAUTHORIZED); + res.sendError(SC_UNAUTHORIZED, e.getMessage()); } catch (ServiceNotEnabledException e) { - res.sendError(SC_FORBIDDEN); + res.sendError(SC_FORBIDDEN, e.getMessage()); } } } diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java index 030287b5c..41217d997 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/ReceivePackServlet.java @@ -137,11 +137,10 @@ public void doFilter(ServletRequest request, ServletResponse response, try { rp = receivePackFactory.create(req, getRepository(req)); } catch (ServiceNotAuthorizedException e) { - rsp.sendError(SC_UNAUTHORIZED); + rsp.sendError(SC_UNAUTHORIZED, e.getMessage()); return; - } catch (ServiceNotEnabledException e) { - sendError(req, rsp, SC_FORBIDDEN); + sendError(req, rsp, SC_FORBIDDEN, e.getMessage()); return; } diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/RepositoryFilter.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/RepositoryFilter.java index 58404020c..a021c1ff5 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/RepositoryFilter.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/RepositoryFilter.java @@ -137,10 +137,10 @@ public void doFilter(final ServletRequest request, sendError(req, res, SC_NOT_FOUND); return; } catch (ServiceNotEnabledException e) { - sendError(req, res, SC_FORBIDDEN); + sendError(req, res, SC_FORBIDDEN, e.getMessage()); return; } catch (ServiceNotAuthorizedException e) { - res.sendError(SC_UNAUTHORIZED); + res.sendError(SC_UNAUTHORIZED, e.getMessage()); return; } catch (ServiceMayNotContinueException e) { sendError(req, res, SC_FORBIDDEN, e.getMessage()); diff --git a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/SmartServiceInfoRefs.java b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/SmartServiceInfoRefs.java index 51332194b..7d4f21b5c 100644 --- a/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/SmartServiceInfoRefs.java +++ b/org.eclipse.jgit.http.server/src/org/eclipse/jgit/http/server/SmartServiceInfoRefs.java @@ -98,7 +98,7 @@ public void doFilter(ServletRequest request, ServletResponse response, try { begin(req, db); } catch (ServiceNotAuthorizedException e) { - res.sendError(SC_UNAUTHORIZED); + res.sendError(SC_UNAUTHORIZED, e.getMessage()); return; } catch (ServiceNotEnabledException e) { sendError(req, res, SC_FORBIDDEN, e.getMessage()); @@ -132,11 +132,9 @@ private void service(ServletRequest request, ServletResponse response) advertise(req, new PacketLineOutRefAdvertiser(out)); buf.close(); } catch (ServiceNotAuthorizedException e) { - res.sendError(SC_UNAUTHORIZED); - + res.sendError(SC_UNAUTHORIZED, e.getMessage()); } catch (ServiceNotEnabledException e) { - sendError(req, res, SC_FORBIDDEN); - + sendError(req, res, SC_FORBIDDEN, e.getMessage()); } catch (ServiceMayNotContinueException e) { if (e.isOutput()) buf.close(); 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 44d4b1bcf..8c27b712b 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 @@ -137,11 +137,10 @@ public void doFilter(ServletRequest request, ServletResponse response, try { rp = uploadPackFactory.create(req, getRepository(req)); } catch (ServiceNotAuthorizedException e) { - rsp.sendError(SC_UNAUTHORIZED); + rsp.sendError(SC_UNAUTHORIZED, e.getMessage()); return; - } catch (ServiceNotEnabledException e) { - sendError(req, rsp, SC_FORBIDDEN); + sendError(req, rsp, SC_FORBIDDEN, e.getMessage()); return; } From 8c3fe215b447dcdc39cfdbf587b73592e726f2d5 Mon Sep 17 00:00:00 2001 From: Jonathan Nieder Date: Tue, 2 Jun 2015 16:52:21 -0700 Subject: [PATCH 4/4] Allow setting detail message and cause when constructing most exceptions In particular, this means a RepositoryResolver, UploadPackFactory, or ReceivePackFactory can set a detail message for ServiceNotAuthorizedException or ServiceNotEnabledException with information for the client about why access is not allowed. Change-Id: I38e1798e1e9d09b5e75cefacd9d85f25729235a9 Signed-off-by: Jonathan Nieder --- .../jgit/api/errors/RefNotFoundException.java | 9 ++++++++ .../errors/StashApplyFailureException.java | 9 ++++++++ .../api/errors/UnmergedPathsException.java | 9 ++++++++ .../jgit/errors/DiffInterruptedException.java | 21 +++++++++++++++++++ .../jgit/errors/LockFailedException.java | 14 +++++++++++++ .../ServiceNotAuthorizedException.java | 17 +++++++++++++++ .../resolver/ServiceNotEnabledException.java | 17 +++++++++++++++ 7 files changed, 96 insertions(+) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/errors/RefNotFoundException.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/errors/RefNotFoundException.java index c8d96a026..b9f2a5617 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/errors/RefNotFoundException.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/errors/RefNotFoundException.java @@ -43,6 +43,15 @@ public class RefNotFoundException extends GitAPIException { private static final long serialVersionUID = 1L; + /** + * @param message + * @param cause + * @since 4.1 + */ + public RefNotFoundException(String message, Throwable cause) { + super(message, cause); + } + /** * @param message */ diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/errors/StashApplyFailureException.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/errors/StashApplyFailureException.java index 25d7e4d47..1d54f77db 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/errors/StashApplyFailureException.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/errors/StashApplyFailureException.java @@ -9,6 +9,15 @@ public class StashApplyFailureException extends GitAPIException { private static final long serialVersionUID = 1L; + /** + * @param message + * @param cause + * @since 4.1 + */ + public StashApplyFailureException(String message, Throwable cause) { + super(message, cause); + } + /** * Create a StashApplyFailedException * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/api/errors/UnmergedPathsException.java b/org.eclipse.jgit/src/org/eclipse/jgit/api/errors/UnmergedPathsException.java index 099004015..082f94c65 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/api/errors/UnmergedPathsException.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/api/errors/UnmergedPathsException.java @@ -61,4 +61,13 @@ public UnmergedPathsException() { public UnmergedPathsException(Throwable cause) { super(JGitText.get().unmergedPaths, cause); } + + /** + * @param message + * @param cause + * @since 4.1 + */ + public UnmergedPathsException(String message, Throwable cause) { + super(message, cause); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/errors/DiffInterruptedException.java b/org.eclipse.jgit/src/org/eclipse/jgit/errors/DiffInterruptedException.java index e6ae685c0..94c8e5d1d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/errors/DiffInterruptedException.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/errors/DiffInterruptedException.java @@ -51,4 +51,25 @@ */ public class DiffInterruptedException extends RuntimeException { private static final long serialVersionUID = 1L; + + /** + * @param message + * @param cause + * @since 4.1 + */ + public DiffInterruptedException(String message, Throwable cause) { + super(message, cause); + } + + /** + * @param message + * @since 4.1 + */ + public DiffInterruptedException(String message) { + super(message); + } + + public DiffInterruptedException() { + super(); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/errors/LockFailedException.java b/org.eclipse.jgit/src/org/eclipse/jgit/errors/LockFailedException.java index 18aa9d978..0142e1763 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/errors/LockFailedException.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/errors/LockFailedException.java @@ -56,6 +56,20 @@ public class LockFailedException extends IOException { private File file; + /** + * @param file + * file that could not be locked + * @param message + * exception message + * @param cause + * cause, for later retrieval by {@link Throwable#getCause()} + * @since 4.1 + */ + public LockFailedException(File file, String message, Throwable cause) { + super(message, cause); + this.file = file; + } + /** * Construct a CannotLockException for the given file and message * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/resolver/ServiceNotAuthorizedException.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/resolver/ServiceNotAuthorizedException.java index 3c16556b4..a9e3e42ed 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/resolver/ServiceNotAuthorizedException.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/resolver/ServiceNotAuthorizedException.java @@ -55,6 +55,23 @@ public class ServiceNotAuthorizedException extends Exception { private static final long serialVersionUID = 1L; + /** + * @param message + * @param cause + * @since 4.1 + */ + public ServiceNotAuthorizedException(String message, Throwable cause) { + super(message, cause); + } + + /** + * @param message + * @since 4.1 + */ + public ServiceNotAuthorizedException(String message) { + super(message); + } + public ServiceNotAuthorizedException() { super(JGitText.get().unauthorized); } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/resolver/ServiceNotEnabledException.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/resolver/ServiceNotEnabledException.java index d0fa758a8..78ae303ed 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/resolver/ServiceNotEnabledException.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/resolver/ServiceNotEnabledException.java @@ -49,6 +49,23 @@ public class ServiceNotEnabledException extends Exception { private static final long serialVersionUID = 1L; + /** + * @param message + * @param cause + * @since 4.1 + */ + public ServiceNotEnabledException(String message, Throwable cause) { + super(message, cause); + } + + /** + * @param message + * @since 4.1 + */ + public ServiceNotEnabledException(String message) { + super(message); + } + /** Indicates the request service is not available. */ public ServiceNotEnabledException() { super(JGitText.get().serviceNotEnabledNoName);