From 10e47a19fff26ba5eef53db5bc6bd54bd2213ca0 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Mon, 17 Aug 2020 16:48:52 -0700 Subject: [PATCH 1/2] 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); From 2ffe8d9705f72386997402b30e9f70f626a84665 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Wed, 19 Aug 2020 14:32:45 -0700 Subject: [PATCH 2/2] GUACAMOLE-1152: Ensure field values accurately represent login state. If expected fields are deleted rather than reset to empty, those fields will not be resubmitted in future requests, resulting in the content of those requests not accurately representing true client-side login state. For example, if a user receives an insufficient credentials error due to their password expiring, failing to provide any new password should result in at least the following fields: 1. Their original username (part of the initial login attempt) 2. Their original password (part of the initial login attempt) 3. Their new password (empty) If fields are incorrectly reset to null, those fields will not be submitted, resulting instead in a request containing only: 1. Their original username (part of the initial login attempt) 2. Their original password (part of the initial login attempt) which is indistinguishable from a normal login attempt. --- .../main/webapp/app/login/directives/login.js | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/guacamole/src/main/webapp/app/login/directives/login.js b/guacamole/src/main/webapp/app/login/directives/login.js index a4145483b..b7967d798 100644 --- a/guacamole/src/main/webapp/app/login/directives/login.js +++ b/guacamole/src/main/webapp/app/login/directives/login.js @@ -71,6 +71,23 @@ angular.module('login').directive('guacLogin', [function guacLogin() { var authenticationService = $injector.get('authenticationService'); var requestService = $injector.get('requestService'); + /** + * The initial value for all login fields. Note that this value must + * not be null. If null, empty fields may not be submitted back to the + * server at all, causing the request to misrepresent true login state. + * + * For example, if a user receives an insufficient credentials error + * due to their password expiring, failing to provide that new password + * should result in the user submitting their username, original + * password, and empty new password. If only the username and original + * password are sent, the invalid password reset request will be + * indistinguishable from a normal login attempt. + * + * @constant + * @type String + */ + var DEFAULT_FIELD_VALUE = ''; + /** * A description of the error that occurred during login, if any. * @@ -148,7 +165,7 @@ angular.module('login').directive('guacLogin', [function guacLogin() { // Set default values for all unset fields angular.forEach($scope.remainingFields, function setDefault(field) { if (!$scope.enteredValues[field.name]) - $scope.enteredValues[field.name] = ''; + $scope.enteredValues[field.name] = DEFAULT_FIELD_VALUE; }); $scope.relevantField = getRelevantField(); @@ -195,13 +212,11 @@ angular.module('login').directive('guacLogin', [function guacLogin() { else $scope.loginError = error.translatableMessage; - // Clear all remaining fields that are not username fields + // Reset all remaining fields to default values, but + // preserve any usernames angular.forEach($scope.remainingFields, function clearEnteredValueIfPassword(field) { - - // If field is not username field, delete it. if (field.type !== Field.Type.USERNAME && field.name in $scope.enteredValues) - delete $scope.enteredValues[field.name]; - + $scope.enteredValues[field.name] = DEFAULT_FIELD_VALUE; }); }