[GitHub] [guacamole-client] jmuehlner commented on a diff in pull request #808: GUACAMOLE-1219: Add support for disabling TOTP for specific users and groups.

["jmuehlner (via GitHub)" <gi...@apache.org>]

jmuehlner commented on code in PR #808:
URL: https://github.com/apache/guacamole-client/pull/808#discussion_r1140510806


##########
extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/UserVerificationService.java:
##########
@@ -203,6 +207,65 @@ private boolean setKey(UserContext context, UserTOTPKey key)
         return true;
 
     }
+    
+    /**
+     * Checks the user in question, via both UserContext and AuthenticatedUser,
+     * to see if TOTP has been disabled for this user, either directly or via
+     * membership in a group that has had TOTP marked as disabled.
+     * 
+     * @param context
+     *     The UserContext for the user being verified.
+     * 
+     * @param authenticatedUser
+     *     The AuthenticatedUser for the user being verified.
+     * 
+     * @return
+     *     True if TOTP access has been disabled for the user, otherwise
+     *     false.
+     * 
+     * @throws GuacamoleException
+     *     If the extension handling storage fails internally while attempting
+     *     to update the user.
+     */
+    private boolean totpDisabled(UserContext context,
+            AuthenticatedUser authenticatedUser)
+            throws GuacamoleException {
+        
+        // If TOTP is disabled for this user, return, allowing login to continue
+        if (TOTPUser.TRUTH_VALUE.equals(
+                context.self().getAttributes().get(
+                        TOTPUser.TOTP_KEY_DISABLED_ATTRIBUTE_NAME
+                ))) {
+            logger.warn("TOTP validation has been disabled for user \"{}\"",
+                    context.self().getIdentifier());
+            return true;
+        }
+        
+        // Check if any effective user groups have TOTP marked as disabled
+        Set<String> userGroups = authenticatedUser.getEffectiveUserGroups();
+        Directory<UserGroup> directoryGroups = context.getPrivileged().getUserGroupDirectory();
+        for (String userGroup : userGroups) {
+            UserGroup thisGroup = directoryGroups.get(userGroup);
+            if (thisGroup == null)
+                continue;
+            
+            Map<String, String> attributes = thisGroup.getAttributes();
+            if (attributes != null 
+                    && !attributes.isEmpty() 

Review Comment:
   This check doesn't seem necessary - if a map is empty it will never contain the key. For that matter, line 255 shouldn't be needed either - if the key doesn't exist in the map, it'll return `null`, and that will fail the check on line 256.



-- 
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: dev-unsubscribe@guacamole.apache.org

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