siacali 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_r361699508
##########
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:
Ok, the problem with your suggestion is that the `Map<String, String>`
**_presently_** returned by `validateTicket()` doesn't contain valid
information for me to derive groups (from anything other than a group attribute
such as memberOf, doing so, I realized I’ve implemented in code I’m playing
with in-house but didn’t actually submit in my pull request Doh! - so by now
you probably think I’m either nuts or stupid - I apologize for that confusion).
In particular, it returns capitalized keys with a string prepended
(CAS-FOOBAR), and groups is case sensitive (So, CAS-FOOBAR, would not be enough
for me to differentiate groups such as FooBar from fooBar - I had mentioned
this before). So, to do the processing within `authenticateUser()`, I would
need to:
- Refactor the previous author's code to return what I need ("raw" SAML
attributes released by CAS)
- Move/restructure their code to create tokens from SAML attributes within
`authenticateUser()`
- Derive both tokens and groups in `authenticateUser()`
Certainly possible, but refactoring/restructuring someone elses code (and
then having to QA it) does seem like a different task and a bit more risk than
adding a feature to existing code.
Also, does that really fit into the philosophy of "one jira ticket, one
change?" or is it "ok" to "fix" someone elses non-broken code as part of adding
a feature? I'm not even a Java programmer and I can see a lot of things I'd
like to "fix" given a license like that... :-)
Give me the final word and I'll do it (given the time to do so), if that's
what you'd like to see. Otherwise I’ll do a version that only considers
actual group membership type attributes (moving the group stuff to
`authenticateUser()`, and open a Jira to address the more full-featured version.
All the other changes you've suggested are "no brainers" - I'm on it.
----------------------------------------------------------------
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
