[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance

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

mike-jumper commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r531820921



##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
##########
@@ -122,16 +134,42 @@
             }
             
             // Convert remaining attributes that have values to Strings
+            String groupAttribute = confService.getGroupAttribute();
+            // Use cas-member-attribute to retrieve and set group memberships
+            String groupDnFormat = confService.getGroupDnFormat();
+            String groupTemplate = "";
+            if (groupDnFormat != null) {
+                // if CAS is backended to LDAP, groups come in as RFC4514 DN
+                // syntax.  If cas-group-dn-format is set, this strips
+                // an entry such as "CN=Foo,OU=Bar,DC=example,DC=com" to
+                // "Foo"
+                groupTemplate = groupDnFormat.replace("%s","([A-Za-z0-9_\\(\\)\\-\\.\\s+]+)");
+                // the underlying parser aggregates all instances of the same
+                // attribute, so we need to be able to parse them out
+                groupTemplate=groupTemplate+",*\\s*";
+            } else {
+               groupTemplate = "([A-Za-z0-9_\\(\\)\\-\\.\\s+]+,*\\s*)";
+            }
+            Pattern pattern = Pattern.compile(groupTemplate);

Review comment:
       Welp:
   
   > It looks like the `\Q` and `\E` special characters can be used to escape anything between them. So it might be possible to use those around the value of the `groupDnFormat` String to automatically escape anything within that ??
   
   Nope - that would be unsafe. What if the value of `groupDnFormat` were `"\E.*\Q"`?
   
   > Can we simply use the `Pattern.quote()` method as follows:
   >
   >     groupTemplate = Pattern.quote(groupDnFormat.replace("%s","([A-Za-z0-9_\(\)\-\.\s+]+)"));
   
   Not quite - that would escape the regex used as the replacement, as well. I think you have the right idea. Perhaps:
   
   ```java
   groupTemplate = Pattern.quote(groupDnFormat).replace("%s","([A-Za-z0-9_\(\)\-\.\s+]+)");
   ```
   
   BUT:
   
   * Definitely have some [Leaning Toothpick Syndrome](https://en.wikipedia.org/wiki/Leaning_toothpick_syndrome) going on with that regex. It's really tough to validate in the current context. Probably best to move that to a constant that can be separately documented, understood, and verified... that way this magic turns into something like: `Pattern.quote(groupDnFormat).replace("%s", REGEX_THAT_WE_KNOW_IS_CORRECT)`
   * I'm not 100% sure the regex _is_ correct ... depending on what it's supposed to match against (which I'm also not 100% sure of). Perhaps any added documentation will clarify that and allow for easier review.




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