[GitHub] [guacamole-client] necouchman commented on a change in pull request #579: GUACAMOLE-793: Add support for retrieving effective groups from CAS.

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

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



##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java
##########
@@ -85,4 +91,102 @@ public PrivateKey getClearpassKey() throws GuacamoleException {
         return environment.getProperty(CASGuacamoleProperties.CAS_CLEARPASS_KEY);
     }
 
+    /**
+     * Returns the CAS attribute that should be used to determine group
+     * memberships in CAS, such as "memberOf". If no attribute has been
+     * specified, null is returned.
+     *
+     * @return
+     *     The attribute name used to determine group memberships in CAS,
+     *     null if not defined.
+     *
+     * @throws GuacamoleException
+     *     If guacamole.properties cannot be parsed.
+     */
+    public String getGroupAttribute() throws GuacamoleException {
+        return environment.getProperty(CASGuacamoleProperties.CAS_GROUP_ATTRIBUTE);
+    }
+
+    /**
+     * Returns the format that CAS is expected to use for its group names, such
+     * as {@link GroupFormat#PLAIN} (simple plain-text names) or
+     * {@link GroupFormat#LDAP} (fully-qualified LDAP DNs). If not specified,
+     * PLAIN is used by default.
+     *
+     * @return
+     *     The format that CAS is expected to use for its group names.
+     *
+     * @throws GuacamoleException
+     *     If the format specified within guacamole.properties is not valid.
+     */
+    public GroupFormat getGroupFormat() throws GuacamoleException {
+        return environment.getProperty(CASGuacamoleProperties.CAS_GROUP_FORMAT, GroupFormat.PLAIN);
+    }
+
+    /**
+     * Returns the base DN that all LDAP-formatted CAS groups must reside
+     * beneath. Any groups that are not beneath this base DN should be ignored.
+     * If no such base DN is provided, the tree structure of the ancestors of
+     * LDAP-formatted CAS groups should not be considered.

Review comment:
       This seems slightly confusing as to what the practical outcome of this is. If the "tree structure of the ancestors...should not be considered", does that effectively mean that all groups will be allowed or all will be ignored?

##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
##########
@@ -34,13 +34,20 @@
 import org.apache.guacamole.auth.cas.ticket.TicketValidationService;
 import org.apache.guacamole.auth.cas.user.CASAuthenticatedUser;
 import org.apache.guacamole.language.TranslatableMessage;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * Service providing convenience functions for the CAS AuthenticationProvider
  * implementation.
  */
 public class AuthenticationProviderService {
 
+    /**
+     * Logger for this class.
+     */
+    private static final Logger logger = LoggerFactory.getLogger(AuthenticationProviderService.class);
+

Review comment:
       Is there a reason for adding the logger? It doesn't look like there were any code additions for actual logging??




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