necouchman commented on a change in pull request #454: GUACAMOLE-793: CAS
Provider returns Group - like LDAP Provider
URL: https://github.com/apache/guacamole-client/pull/454#discussion_r361686355
##########
File path:
extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
##########
@@ -88,7 +92,7 @@
* If the ID ticket is not valid or guacamole.properties could
* not be parsed.
*/
- public Map<String, String> validateTicket(String ticket,
+ public TokensAndGroups validateTicket(String ticket,
Review comment:
I hate to be a pain about this, but I'm still in favor of having this
`validateTicket()` method just return the `Map<String, String>` data, and
parsing it out in the body of the `authenticateUser()` method in the
`AutenticationProviderService` class. This leaves the `validateTicket()`
method to do ticket validation, keeping it free from the extra logic introduced
here.
I'd suggest doing one of two things:
* Keep the `TokenAndGroups` utility class, but, rather than using it to
store the information, implement a static method (or two) that takes in the
`Map<String, String>` input from `validateTicket()` and parses out the groups,
returning the `List` of effective groups. Call this method within the
`authenticateUser()` method using the output received from `validateTicket()`
and using the output to set up the effective groups
* Get rid of the `TokensAndGroups` class and implement the parser in a
`private` method within the `AuthenticationProviderService` class that does the
same as above.
I'm completely on-board with leaving the group data in the tokens, as we
discussed earlier.
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
