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.
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 6c6474b..9855cd6 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 @@
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 @@
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 @@
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 9117947..e6c9b4c 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.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 @@
}
// 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);
