GUACAMOLE-197: Move some RADIUS code into specific functions; tighten up code, improve efficiency, address some style issues.

extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java

@@ -75,6 +75,46 @@ public class AuthenticationProviderService {
     @Inject
     private Provider<AuthenticatedUser> authenticatedUserProvider;
 
+    /**
+     * Returns the expected credentials from a RADIUS challenge.
+     *
+     * @param challengePacket
+     *     The AccessChallenge RadiusPacket received from the RADIUS 
+     *     server.
+     *
+     * @return
+     *     A CredentialsInfo object that represents fields that need to
+     *     be presented to the user in order to complete authentication.
+     *     One of these must be the RADIUS state.
+     */
+    private CredentialsInfo getRadiusChallenge(RadiusPacket challengePacket) {
+
+        // Try to get the state attribute - if it's not there, we have a problem
+        RadiusAttribute stateAttr = challengePacket.findAttribute(Attr_State.TYPE);
+        if (stateAttr == null) {
+            logger.error("Something went wrong, state attribute not present.");
+            logger.debug("State Attribute turned up null, which shouldn't happen in AccessChallenge.");
+            return null;
+        }
+
+        // We need to get the reply message so we know what to ask the user
+        RadiusAttribute replyAttr = challengePacket.findAttribute(Attr_ReplyMessage.TYPE);
+        if (replyAttr == null) {
+            logger.error("No reply message received from the server.");
+            logger.debug("Expecting a Attr_ReplyMessage attribute on this packet, and did not get one.");
+            return null;
+        }
+
+        // We have the required attributes - convert to strings and then generate the additional login box/field
+        String replyMsg = replyAttr.toString();
+        String radiusState = new String(stateAttr.getValue().getBytes());
+        Field radiusResponseField = new RadiusChallengeResponseField(replyMsg);
+        Field radiusStateField = new RadiusStateField(radiusState);
+
+        // Return the CredentialsInfo object that has the state and the expected response.
+        return new CredentialsInfo(Arrays.asList(radiusResponseField,radiusStateField));
+    }
+
     /**
      * Returns an AuthenticatedUser representing the user authenticated by the
      * given credentials.
@@ -93,12 +133,6 @@ public class AuthenticationProviderService {
     public AuthenticatedUser authenticateUser(Credentials credentials)
             throws GuacamoleException {
 
-        // Grab the HTTP Request from the credentials object
-        HttpServletRequest request = credentials.getRequest();
-
-        // Set up RadiusPacket object
-        RadiusPacket radPack;
-
         // Ignore anonymous users
         if (credentials.getUsername() == null || credentials.getUsername().isEmpty())
             return null;
@@ -107,13 +141,19 @@ public class AuthenticationProviderService {
         if (credentials.getPassword() == null || credentials.getPassword().isEmpty())
             return null;
 
+        // Grab the HTTP Request from the credentials object
+        HttpServletRequest request = credentials.getRequest();
+
         // Try to get parameters to see if this is a post-challenge attempt
         String challengeResponse = request.getParameter(RadiusChallengeResponseField.PARAMETER_NAME);
 
-        // We do not have a challenge response, so we proceed normally
+        // RadiusPacket object to store response from server.
+        RadiusPacket radPack;
+
+        // We do not have a challenge response, proceed with username/password authentication.
         if (challengeResponse == null) {
 
-            // Initialize Radius Packet and try to authenticate
+            // Attempt RADIUS authentication with username/password.
             try {
                 radPack = radiusService.authenticate(credentials.getUsername(),
                                                 credentials.getPassword());
@@ -124,7 +164,7 @@ public class AuthenticationProviderService {
                 throw new GuacamoleInvalidCredentialsException("Authentication error.", CredentialsInfo.USERNAME_PASSWORD);
             }
 
-            // If configure fails, permission to login is denied
+            // If no RadiusPacket is returned, we've encountered an error.
             if (radPack == null) {
                 logger.debug("Nothing in the RADIUS packet.");
                 throw new GuacamoleInvalidCredentialsException("Authentication error.", CredentialsInfo.USERNAME_PASSWORD);
@@ -133,45 +173,23 @@ public class AuthenticationProviderService {
             // If we get back an AccessReject packet, login is denied.
             else if (radPack instanceof AccessReject) {
                 logger.debug("Login has been rejected by RADIUS server.");
-                throw new GuacamoleInvalidCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD);
+                throw new GuacamoleInvalidCredentialsException("Authentication failed.", CredentialsInfo.USERNAME_PASSWORD);
             }
 
             // If we receive an AccessChallenge package, the server needs more information -
             // We create a new form/field with the challenge message.
             else if (radPack instanceof AccessChallenge) {
+                CredentialsInfo expectedCredentials = getRadiusChallenge(radPack);
 
-                // Try to get the state attribute - if it's not there, we have a problem
+                if (expectedCredentials == null)
-                RadiusAttribute stateAttr = radPack.findAttribute(Attr_State.TYPE);
-                if (stateAttr == null) {
-                    logger.error("Something went wrong, state attribute not present.");
-                    logger.debug("State Attribute turned up null, which shouldn't happen in AccessChallenge.");
                     throw new GuacamoleInvalidCredentialsException("Authentication error.", CredentialsInfo.USERNAME_PASSWORD);
-                }
 
-                // We need to get the reply message so we know what to ask the user
-                RadiusAttribute replyAttr = radPack.findAttribute(Attr_ReplyMessage.TYPE);
-                if (replyAttr == null) {
-                    logger.error("No reply message received from the server.");
-                    logger.debug("Expecting a Attr_ReplyMessage attribute on this packet, and did not get one.");
-                    throw new GuacamoleInvalidCredentialsException("Authentication error.", CredentialsInfo.USERNAME_PASSWORD);
-                }
-
-                // We have the required attributes - convert to strings and then generate the additional login box/field
-                String replyMsg = replyAttr.toString();
-                String radiusState = new String(stateAttr.getValue().getBytes());
-                Field radiusResponseField = new RadiusChallengeResponseField(replyMsg);
-                Field radiusStateField = new RadiusStateField(radiusState);
-                CredentialsInfo expectedCredentials = new CredentialsInfo(Arrays.asList(radiusResponseField,radiusStateField));
                 throw new GuacamoleInsufficientCredentialsException("LOGIN.INFO_RADIUS_ADDL_REQUIRED", expectedCredentials);
-
             }
 
             // If we receive AccessAccept, authentication has succeeded
             else if (radPack instanceof AccessAccept) {
-
                 try {
-
-                    // Return AuthenticatedUser if bind succeeds
                     AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
                     authenticatedUser.init(credentials);
                     return authenticatedUser;
@@ -181,9 +199,9 @@ public class AuthenticationProviderService {
                 }
             }
 
-            // Something unanticipated happened, so we panic
+            // Something unanticipated happened, so we panic and go back to login.
             else {
-                logger.error("Unexpected authentication failure talking to RADIUS server.");
+                logger.error("Unexpected failure authenticating with RADIUS server.");
                 throw new GuacamoleInvalidCredentialsException("Unknown error trying to authenticate.", CredentialsInfo.USERNAME_PASSWORD);
             }
         }
@@ -191,7 +209,7 @@ public class AuthenticationProviderService {
         // The user responded to the challenge, send that back to the server
         else {
 
-            // Initialize Radius Packet and try to authenticate
+            // Attempt to authenticate with response to challenge.
             try {
                 radPack = radiusService.authenticate(credentials.getUsername(),
                                                      request.getParameter(RadiusStateField.PARAMETER_NAME),
@@ -200,7 +218,7 @@ public class AuthenticationProviderService {
             catch (GuacamoleException e) {
                 logger.error("Cannot configure RADIUS server: {}", e.getMessage());
                 logger.debug("Error configuring RADIUS server.", e);
-                radPack = null;
+                throw new GuacamoleInvalidCredentialsException("Authentication error.", CredentialsInfo.USERNAME_PASSWORD);
             }
             finally {
                 radiusService.disconnect();
@@ -216,8 +234,8 @@ public class AuthenticationProviderService {
             // Authentication failed
             else {
                 logger.warn("RADIUS Challenge/Response authentication failed.");
-                logger.debug("Did not receive a RADIUS AccessAccept packet back from server.");
+                logger.debug("Received something other than AccessAccept packet from the RADIUS server.");
-                throw new GuacamoleInvalidCredentialsException("Failed to authenticate to RADIUS.", CredentialsInfo.USERNAME_PASSWORD);
+                throw new GuacamoleInvalidCredentialsException("Authentication failed.", CredentialsInfo.USERNAME_PASSWORD);
             }
         }
     }

extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/RadiusConnectionService.java

@@ -109,7 +109,6 @@ public class RadiusConnectionService {
      *     A new RadiusAuthenticator instance which has been configured
      *     with parameters from guacamole.properties, or null if
      *     configuration fails.
-     *
      */
     private RadiusAuthenticator setupRadiusAuthenticator() throws GuacamoleException {
 
@@ -151,17 +150,14 @@ public class RadiusConnectionService {
             ((EAPTLSAuthenticator)radAuth).setKeyFile((new File(guacHome, keyFile)).toString());
             ((EAPTLSAuthenticator)radAuth).setKeyFileType(confService.getRadiusKeyType());
             ((EAPTLSAuthenticator)radAuth).setTrustAll(confService.getRadiusTrustAll());
-
         }
 
         // If we're using EAP-TTLS, we need to define tunneled protocol
         if (radAuth instanceof EAPTTLSAuthenticator) {
-
             if (innerProtocol == null)
                 throw new GuacamoleException("Trying to use EAP-TTLS, but no inner protocol specified.");
 
             ((EAPTTLSAuthenticator)radAuth).setInnerProtocol(innerProtocol);
-
         }
 
         return radAuth;
@@ -186,14 +182,6 @@ public class RadiusConnectionService {
     public RadiusPacket authenticate(String username, String password) 
             throws GuacamoleException {
 
-        // Create the connection and load the attribute dictionary
-        createRadiusConnection();
-        AttributeFactory.loadAttributeDictionary("net.jradius.dictionary.AttributeDictionaryImpl");
-
-        // If the client is null, we return null - something has gone wrong
-        if (radiusClient == null)
-            return null;
-
         // If a username hasn't been provided, stop
         if (username == null || username.isEmpty()) {
             logger.warn("Anonymous access not allowed with RADIUS client.");
@@ -206,6 +194,14 @@ public class RadiusConnectionService {
             return null;
         }
 
+        // Create the connection and load the attribute dictionary
+        createRadiusConnection();
+        AttributeFactory.loadAttributeDictionary("net.jradius.dictionary.AttributeDictionaryImpl");
+
+        // If the client is null, we return null - something has gone wrong
+        if (radiusClient == null)
+            return null;
+
         RadiusAuthenticator radAuth = setupRadiusAuthenticator();
 
         if (radAuth == null)
@@ -272,14 +268,6 @@ public class RadiusConnectionService {
     public RadiusPacket authenticate(String username, String state, String response)
             throws GuacamoleException {
 
-        // Create the RADIUS connection and set up the dictionary
-        createRadiusConnection();
-        AttributeFactory.loadAttributeDictionary("net.jradius.dictionary.AttributeDictionaryImpl");
-
-        // Client failed to set up, so we return null
-        if (radiusClient == null)
-            return null;
-
         // If a username wasn't passed, we quit
         if (username == null || username.isEmpty()) {
             logger.warn("Anonymous access not allowed with RADIUS client.");
@@ -298,6 +286,14 @@ public class RadiusConnectionService {
             return null;
         }
 
+        // Create the RADIUS connection and set up the dictionary
+        createRadiusConnection();
+        AttributeFactory.loadAttributeDictionary("net.jradius.dictionary.AttributeDictionaryImpl");
+
+        // Client failed to set up, so we return null
+        if (radiusClient == null)
+            return null;
+
         // Set up the RadiusAuthenticator
         RadiusAuthenticator radAuth = setupRadiusAuthenticator();
         if (radAuth == null)
@@ -345,9 +341,7 @@ public class RadiusConnectionService {
     }
 
     /**
-     * Disconnects the given RADIUS connection, logging any failure to do so
+     * Disconnects the current RADIUS connection.
-     * appropriately.
-     *
      */
     public void disconnect() {