mike-jumper commented on code in PR #758:
URL: https://github.com/apache/guacamole-client/pull/758#discussion_r951966652
##########
guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java:
##########
@@ -462,25 +419,72 @@ public String authenticate(Credentials credentials,
String token)
else
existingSession = null;
- // Get up-to-date AuthenticatedUser and associated UserContexts
- AuthenticatedUser authenticatedUser =
getAuthenticatedUser(existingSession, credentials);
- List<DecoratedUserContext> userContexts =
getUserContexts(existingSession, authenticatedUser, credentials);
-
- // Update existing session, if it exists
+ AuthenticatedUser authenticatedUser;
String authToken;
- if (existingSession != null) {
- authToken = token;
- existingSession.setAuthenticatedUser(authenticatedUser);
- existingSession.setUserContexts(userContexts);
+
+ try {
+
+ // Get up-to-date AuthenticatedUser and associated UserContexts
+ authenticatedUser = getAuthenticatedUser(existingSession,
credentials);
+ List<DecoratedUserContext> userContexts =
getUserContexts(existingSession, authenticatedUser, credentials);
+
+ // Update existing session, if it exists
+ if (existingSession != null) {
+ authToken = token;
+ existingSession.setAuthenticatedUser(authenticatedUser);
+ existingSession.setUserContexts(userContexts);
+ }
+
+ // If no existing session, generate a new token/session pair
+ else {
+ authToken = authTokenGenerator.getToken();
+ tokenSessionMap.put(authToken, new
GuacamoleSession(environment, authenticatedUser, userContexts));
+ logger.debug("Login was successful for user \"{}\".",
authenticatedUser.getIdentifier());
+ }
+
+ // Report authentication success
+ try {
+ listenerService.handleEvent(new
AuthenticationSuccessEvent(authenticatedUser));
+ }
+ catch (GuacamoleException e) {
+ throw new GuacamoleAuthenticationProcessException("User "
+ + "authentication aborted by event listener.", null,
e);
+ }
+
}
- // If no existing session, generate a new token/session pair
- else {
- authToken = authTokenGenerator.getToken();
- tokenSessionMap.put(authToken, new GuacamoleSession(environment,
authenticatedUser, userContexts));
- logger.debug("Login was successful for user \"{}\".",
authenticatedUser.getIdentifier());
+ // Log and rethrow any authentication errors
+ catch (GuacamoleAuthenticationProcessException e) {
+
+ // Get request and username for sake of logging
+ HttpServletRequest request = credentials.getRequest();
+ String username = credentials.getUsername();
+
+ listenerService.handleEvent(new
AuthenticationFailureEvent(credentials,
+ e.getAuthenticationProvider(), e.getCause()));
+
+ // Log authentication failures with associated usernames
+ if (username != null) {
+ if (logger.isWarnEnabled())
+ logger.warn("Authentication attempt from {} for user
\"{}\" failed.",
+ getLoggableAddress(request), username);
+ }
+
+ // Log anonymous authentication failures
+ else if (logger.isDebugEnabled())
+ logger.debug("Anonymous authentication attempt from {}
failed.",
+ getLoggableAddress(request));
+
+ // Rethrow exception
+ throw e.getCauseAsGuacamoleException();
+
}
+ if (logger.isInfoEnabled())
Review Comment:
IIRC, these checks were added because of the call to `getLoggableAddress()`,
which performs string validation with a regex and is thus relatively heavy:
https://github.com/apache/guacamole-client/blob/843add93a5f2dc24e16b9370f8b95ad55eafe5bf/guacamole/src/main/java/org/apache/guacamole/rest/auth/AuthenticationService.java#L146-L156
If we took out these checks, `getLoggableAddress()` would be called in every
case because of how its return value is passed, even if the relevant log level
is disabled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
