[GitHub] [guacamole-client] necouchman commented on a change in pull request #564: GUACAMOLE-1172: add logic to retrieve groups from OIDC token

[GitBox <gi...@apache.org>]

necouchman commented on a change in pull request #564:
URL: https://github.com/apache/guacamole-client/pull/564#discussion_r515491002



##########
File path: extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/AuthenticationProviderService.java
##########
@@ -91,13 +93,19 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
             throws GuacamoleException {
 
         String username = null;
+        Set<String> groups = null;
 
         // Validate OpenID token in request, if present, and derive username
         HttpServletRequest request = credentials.getRequest();
         if (request != null) {
             String token = request.getParameter(TokenField.PARAMETER_NAME);
-            if (token != null)
-                username = tokenService.processUsername(token);
+            if (token != null) {
+                JwtClaims claims = tokenService.validateToken(token);
+                if ( null != claims ) {

Review comment:
       A couple of issues with this one line:
   * From a style perspective, there are extra spaces: `if (null != claims)`
   * I'm not sure why the sudden switch to putting `null` as the first part of the condition - generally throughout the code we put the variable, first.

##########
File path: extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java
##########
@@ -132,9 +122,85 @@ public String processUsername(String token) throws GuacamoleException {
             logger.debug("Malformed claim within received JWT.", e);
         }
 
+        return claims;
+    }
+
+    /**
+     * Parses the given JwtClaims, returning the username contained
+     * therein, as defined by the username claim type given in
+     * guacamole.properties. If the username claim type is missing or 
+     * is invalid, null is returned.
+     *
+     * @param claims
+     *     A valid JwtClaims to extract the username from.
+     *
+     * @return
+     *     The username contained within the given JwtClaims, or null if the
+     *     claim is not valid or the username claim type is missing,
+     *
+     * @throws GuacamoleException
+     *     If guacamole.properties could not be parsed.
+     */
+    public String processUsername(JwtClaims claims) throws GuacamoleException {
+        String usernameClaim = confService.getUsernameClaimType();
+
+        if ( null != claims ) {
+            try {
+                // Pull username from claims
+                String username = claims.getStringClaimValue(usernameClaim);
+                if (username != null)
+                    return username;
+            }
+            catch (MalformedClaimException e) {
+                logger.info("Rejected OpenID token with malformed claim: {}", e.getMessage());
+                logger.debug("Malformed claim within received JWT.", e);
+            }
+
+            // Warn if username was not present in token, as it likely means
+            // the system is not set up correctly
+            logger.warn("Username claim \"{}\" missing from token. Perhaps the "
+                    + "OpenID scope and/or username claim type are "
+                    + "misconfigured?", usernameClaim);
+        }
+
         // Could not retrieve username from JWT
         return null;
-
     }
 
+    /**
+     * Parses the given JwtClaims, returning the groups contained
+     * therein, as defined by the groups claim type given in
+     * guacamole.properties. If the groups claim type is missing or
+     * is invalid, an empty set is returned.
+     *
+     * @param claims
+     *     A valid JwtClaims to extract groups from.
+     *
+     * @return
+     *     A Set of String representing the groups the user is member of
+     *     from the OpenID provider point of view, or an empty Set if
+     *     claim is not valid or the groups claim type is missing,
+     *
+     * @throws GuacamoleException
+     *     If guacamole.properties could not be parsed.
+     */
+    public Set<String> processGroups(JwtClaims claims) throws GuacamoleException {
+        String groupsClaim = confService.getGroupsClaimType();
+
+        if ( null != claims ) {

Review comment:
       And here...

##########
File path: extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java
##########
@@ -132,9 +122,85 @@ public String processUsername(String token) throws GuacamoleException {
             logger.debug("Malformed claim within received JWT.", e);
         }
 
+        return claims;
+    }
+
+    /**
+     * Parses the given JwtClaims, returning the username contained
+     * therein, as defined by the username claim type given in
+     * guacamole.properties. If the username claim type is missing or 
+     * is invalid, null is returned.
+     *
+     * @param claims
+     *     A valid JwtClaims to extract the username from.
+     *
+     * @return
+     *     The username contained within the given JwtClaims, or null if the
+     *     claim is not valid or the username claim type is missing,
+     *
+     * @throws GuacamoleException
+     *     If guacamole.properties could not be parsed.
+     */
+    public String processUsername(JwtClaims claims) throws GuacamoleException {
+        String usernameClaim = confService.getUsernameClaimType();
+
+        if ( null != claims ) {

Review comment:
       Similar to above comment:
   * Remove extra spaces
   * Re-order the comparison




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org