From 4a0e9f310f8af0abdd7b377e9cf2324590b12de1 Mon Sep 17 00:00:00 2001
From: Michael Jumper <mjumper@apache.org>
Date: Thu, 25 Apr 2024 18:10:18 -0700
Subject: [PATCH] GUACAMOLE-1289: Ensure updates to credentials are accurately
 represented in failure events.

---
 .../rest/auth/AuthenticationService.java      | 83 +++++++------------
 1 file changed, 31 insertions(+), 52 deletions(-)

diff --git a/guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java b/guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java
index 7bb1e6fa8..5a0e4c5fe 100644
--- a/guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java
+++ b/guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java
@@ -332,43 +332,6 @@ public class AuthenticationService {
 
     }
 
-    /**
-     * Performs arbitrary and optional updates to the credentials supplied by
-     * the authenticating user as dictated by the {@link AuthenticationProvider#updateCredentials(org.apache.guacamole.net.auth.Credentials)}
-     * functions of any installed AuthenticationProvider. Each installed
-     * AuthenticationProvider is given the opportunity, in order, to make
-     * updates to the supplied credentials.
-     *
-     * @param credentials
-     *     The credentials to be updated.
-     *
-     * @return
-     *     The set of credentials that should be provided to all
-     *     AuthenticationProviders during authentication, now possibly updated
-     *     (or even replaced) by any number of installed
-     *     AuthenticationProviders.
-     *
-     * @throws GuacamoleAuthenticationProcessException 
-     *     If an error occurs while updating the supplied credentials.
-     */
-    private Credentials getUpdatedCredentials(Credentials credentials)
-            throws GuacamoleAuthenticationProcessException {
-
-        for (AuthenticationProvider authProvider : authProviders) {
-            try {
-                credentials = authProvider.updateCredentials(credentials);
-            }
-            catch (GuacamoleException | RuntimeException | Error e) {
-                throw new GuacamoleAuthenticationProcessException("User "
-                        + "authentication aborted during credential "
-                        + "update/revision.", authProvider, e);
-            }
-        }
-
-        return credentials;
-
-    }
-
     /**
      * Authenticates a user using the given credentials and optional
      * authentication token, returning the authentication token associated with
@@ -394,27 +357,38 @@ public class AuthenticationService {
     public String authenticate(Credentials credentials, String token)
             throws GuacamoleException {
 
-        // Fire pre-authentication event before ANY authn/authz occurs at all
-        listenerService.handleEvent((AuthenticationRequestReceivedEvent) () -> credentials);
-
-        // Pull existing session if token provided
-        GuacamoleSession existingSession;
-        if (token != null)
-            existingSession = tokenSessionMap.get(token);
-        else
-            existingSession = null;
-
-        AuthenticatedUser authenticatedUser;
         String authToken;
-
         try {
 
             // Allow extensions to make updated to credentials prior to
-            // actual authentication
-            Credentials updatedCredentials = getUpdatedCredentials(credentials);
+            // actual authentication (NOTE: We do this here instead of in a
+            // separate function to ensure that failure events accurately
+            // represent the credentials that failed when a chain of credential
+            // updates is involved)
+            for (AuthenticationProvider authProvider : authProviders) {
+                try {
+                    credentials = authProvider.updateCredentials(credentials);
+                }
+                catch (GuacamoleException | RuntimeException | Error e) {
+                    throw new GuacamoleAuthenticationProcessException("User "
+                            + "authentication aborted during credential "
+                            + "update/revision.", authProvider, e);
+                }
+            }
+
+            // Fire pre-authentication event before ANY authn/authz occurs at all
+            final Credentials updatedCredentials = credentials;
+            listenerService.handleEvent((AuthenticationRequestReceivedEvent) () -> updatedCredentials);
+
+            // Pull existing session if token provided
+            GuacamoleSession existingSession;
+            if (token != null)
+                existingSession = tokenSessionMap.get(token);
+            else
+                existingSession = null;
 
             // Get up-to-date AuthenticatedUser and associated UserContexts
-            authenticatedUser = getAuthenticatedUser(existingSession, updatedCredentials);
+            AuthenticatedUser authenticatedUser = getAuthenticatedUser(existingSession, updatedCredentials);
             List<DecoratedUserContext> userContexts = getUserContexts(existingSession, authenticatedUser, updatedCredentials);
 
             // Update existing session, if it exists
@@ -445,6 +419,11 @@ public class AuthenticationService {
         // Log and rethrow any authentication errors
         catch (GuacamoleAuthenticationProcessException e) {
 
+            // NOTE: The credentials referenced here are intentionally NOT the
+            // final updatedCredentials reference (though they may often be
+            // equivalent) to ensure that failure events accurately represent
+            // the credentials that failed if that failure occurs in the middle
+            // of a chain of credential updates via updateCredentials()
             listenerService.handleEvent(new AuthenticationFailureEvent(credentials,
                     e.getAuthenticationProvider(), e.getCause()));