From 10e47a19fff26ba5eef53db5bc6bd54bd2213ca0 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Mon, 17 Aug 2020 16:48:52 -0700 Subject: [PATCH] GUACAMOLE-1152: Correctly differentiate between client errors and server errors. By definition, a client error is not an internal error, but an intentional refusal of the server to handle a malformed or otherwise invalid request. These should not be handled in the same way as server errors which unexpectedly block processing of a request and should be corrected by an administrator. In the case of GUACAMOLE-1152, client errors should not be ignored even if failures are explicitly configured as tolerated for the associated authentication provider. --- .../AuthenticationProviderFacade.java | 70 +++---------------- .../guacamole/rest/RESTExceptionMapper.java | 16 ++++- 2 files changed, 23 insertions(+), 63 deletions(-) diff --git a/guacamole/src/main/java/org/apache/guacamole/extension/AuthenticationProviderFacade.java b/guacamole/src/main/java/org/apache/guacamole/extension/AuthenticationProviderFacade.java index 6c6474b45..9855cd6fd 100644 --- a/guacamole/src/main/java/org/apache/guacamole/extension/AuthenticationProviderFacade.java +++ b/guacamole/src/main/java/org/apache/guacamole/extension/AuthenticationProviderFacade.java @@ -21,12 +21,12 @@ package org.apache.guacamole.extension; import java.util.Set; import java.util.UUID; +import org.apache.guacamole.GuacamoleClientException; import org.apache.guacamole.GuacamoleException; import org.apache.guacamole.net.auth.AuthenticatedUser; import org.apache.guacamole.net.auth.AuthenticationProvider; import org.apache.guacamole.net.auth.Credentials; import org.apache.guacamole.net.auth.UserContext; -import org.apache.guacamole.net.auth.credentials.GuacamoleCredentialsException; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -190,41 +190,15 @@ public class AuthenticationProviderFacade implements AuthenticationProvider { return authProvider.authenticateUser(credentials); } - // Pass through credential exceptions untouched, as these are not - // internal failures - catch (GuacamoleCredentialsException e) { + // Pass through client exceptions untouched, including credential + // exceptions, as these are not internal failures + catch (GuacamoleClientException e) { throw e; } // Pass through all other exceptions (aborting authentication entirely) // only if not configured to ignore such failures - catch (GuacamoleException e) { - - // Skip using this authentication provider if configured to ignore - // internal failures during auth - if (isFailureTolerated()) { - warnAuthProviderSkipped(e); - return null; - } - - warnAuthAborted(); - throw e; - - } - catch (RuntimeException e) { - - // Skip using this authentication provider if configured to ignore - // internal failures during auth - if (isFailureTolerated()) { - warnAuthProviderSkipped(e); - return null; - } - - warnAuthAborted(); - throw e; - - } - catch (Error e) { + catch (GuacamoleException | RuntimeException | Error e) { // Skip using this authentication provider if configured to ignore // internal failures during auth @@ -274,41 +248,15 @@ public class AuthenticationProviderFacade implements AuthenticationProvider { return authProvider.getUserContext(authenticatedUser); } - // Pass through credential exceptions untouched, as these are not - // internal failures - catch (GuacamoleCredentialsException e) { + // Pass through client exceptions untouched, including credential + // exceptions, as these are not internal failures + catch (GuacamoleClientException e) { throw e; } // Pass through all other exceptions (aborting authentication entirely) // only if not configured to ignore such failures - catch (GuacamoleException e) { - - // Skip using this authentication provider if configured to ignore - // internal failures during auth - if (isFailureTolerated()) { - warnAuthProviderSkipped(e); - return null; - } - - warnAuthAborted(); - throw e; - - } - catch (RuntimeException e) { - - // Skip using this authentication provider if configured to ignore - // internal failures during auth - if (isFailureTolerated()) { - warnAuthProviderSkipped(e); - return null; - } - - warnAuthAborted(); - throw e; - - } - catch (Error e) { + catch (GuacamoleException | RuntimeException | Error e) { // Skip using this authentication provider if configured to ignore // internal failures during auth diff --git a/guacamole/src/main/java/org/apache/guacamole/rest/RESTExceptionMapper.java b/guacamole/src/main/java/org/apache/guacamole/rest/RESTExceptionMapper.java index 91179473a..e6c9b4c5b 100644 --- a/guacamole/src/main/java/org/apache/guacamole/rest/RESTExceptionMapper.java +++ b/guacamole/src/main/java/org/apache/guacamole/rest/RESTExceptionMapper.java @@ -27,6 +27,7 @@ import javax.ws.rs.core.MediaType; import javax.ws.rs.core.Response; import javax.ws.rs.ext.ExceptionMapper; import javax.ws.rs.ext.Provider; +import org.apache.guacamole.GuacamoleClientException; import org.apache.guacamole.GuacamoleException; import org.apache.guacamole.GuacamoleUnauthorizedException; import org.apache.guacamole.rest.auth.AuthenticationService; @@ -91,14 +92,25 @@ public class RESTExceptionMapper implements ExceptionMapper { } // Translate GuacamoleException subclasses to HTTP error codes - if (t instanceof GuacamoleException) + if (t instanceof GuacamoleException) { + + // Always log the human-readable details of GuacacamoleExceptions + // for the benefit of the administrator + if (t instanceof GuacamoleClientException) + logger.debug("Client request rejected: {}", t.getMessage()); + else { + logger.error("Request could not be processed: {}", t.getMessage()); + logger.debug("Processing of request aborted by extension.", t); + } + return Response .status(((GuacamoleException) t).getHttpStatusCode()) .entity(new APIError((GuacamoleException) t)) .type(MediaType.APPLICATION_JSON) .build(); + } - // Rethrow unchecked exceptions such that they are properly wrapped + // Wrap unchecked exceptions String message = t.getMessage(); if (message != null) logger.error("Unexpected internal error: {}", message);