You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2020/08/27 16:57:52 UTC
[GitHub] [guacamole-client] doctorfree opened a new pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
doctorfree opened a new pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563
This is a revision of the pull request by @siacali at https://github.com/apache/guacamole-client/pull/454 which provides a functioning solution to this PR. The revised approach is to modify validateTicket() to return the CASAuthenticatedUser instance rather than just a token so CAS Provider can return Group - like LDAP Provider. This alleviates the need for a new class which was the objection to the previous pull request.
This is my first pull request to this project so please feel free to help me comply with your standards. I've tried to incorporate changes that address all of the comments in the previous pull request but may still need to address other issues I may have overlooked. Thanks for any feedback.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] doctorfree commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
doctorfree commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r516797787
##########
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);
+
for (Entry <String, Object> attr : ticketAttrs.entrySet()) {
String tokenName = TokenName.canonicalize(attr.getKey(),
CAS_ATTRIBUTE_TOKEN_PREFIX);
Object value = attr.getValue();
- if (value != null)
+ if (value != null) {
tokens.put(tokenName, value.toString());
+ if (attr.getKey().equals(groupAttribute)) {
+ String valueWithoutBrackets = value.toString().substring(1,value.toString().length()-1);
+ Matcher matcher = pattern.matcher(valueWithoutBrackets);
+ while (matcher.find()) {
+ effectiveGroups.add(matcher.group(1));
+ }
+ }
+ }
Review comment:
Thanks for identifying this. I moved the initialization of the CAS authenticated user object into validateTicket as you suggest.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] lchanouha commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
lchanouha commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r537158175
##########
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:
I think LdapName is the way:
http://cr.openjdk.java.net/~iris/se/12/latestSpec/api/java.naming/javax/naming/ldap/LdapName.html#%3Cinit%3E(java.lang.String)
(see my example below)
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r531821036
##########
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:
Also ... what if the DN format needs to contain a literal "%s"?
----------------------------------------------------------------
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
[GitHub] [guacamole-client] doctorfree commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
doctorfree commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r516792244
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/CASGuacamoleProperties.java
##########
@@ -68,5 +69,30 @@ private CASGuacamoleProperties() {}
public String getName() { return "cas-clearpass-key"; }
};
+
+ /**
+ * The attribute used for group membership
+ * example: memberOf (case sensitive)
+ */
+ public static final StringGuacamoleProperty CAS_GROUP_ATTRIBUTE =
+ new StringGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "cas-group-attribute"; }
+
+ };
+ /**
+ * The attribute used for group DN Format
+ * such as CN=%s,OU=myou,DC=example,DC=com (case sensitive)
+ * used to strip all but %s. This is only necessary when
+ * CAS backend is LDAP.
+ */
Review comment:
Modified the comment in latest commits to clarify this. Thanks for identifying this unclear comment.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] necouchman commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r527919325
##########
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:
@mike-jumper Thoughts, here? RegEx still causes my mind to twist in ways I didn't think were possible...
----------------------------------------------------------------
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
[GitHub] [guacamole-client] necouchman commented on pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#issuecomment-744399914
Closed by #579
----------------------------------------------------------------
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
[GitHub] [guacamole-client] necouchman closed pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
necouchman closed pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r536968813
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
##########
@@ -122,16 +134,44 @@
}
// 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);
+
for (Entry <String, Object> attr : ticketAttrs.entrySet()) {
String tokenName = TokenName.canonicalize(attr.getKey(),
CAS_ATTRIBUTE_TOKEN_PREFIX);
Object value = attr.getValue();
- if (value != null)
- tokens.put(tokenName, value.toString());
+ if (value != null) {
+ String attrValue = value.toString();
+ tokens.put(tokenName, attrValue);
+ if (attr.getKey().equals(groupAttribute)) {
+ Matcher matcher =
+ pattern.matcher(attrValue.substring(1,attrValue.length()-1));
+ while (matcher.find()) {
+ effectiveGroups.add(matcher.group(1));
+ }
+ }
Review comment:
I believe it was mentioned before that the format of the `memberOf` CAS attribute was `['GROUP1', 'GROUP2', ...]`. Do we know:
* Is this _truly_ the case? Or is it the result of `value.toString()` on some type that could be iterated cleanly? Is there any way to know whether the CAS client implementation returns an actual iterable type?
* If it does indeed provide only a string, where is that format dictated? How does it handle the possible presence of meaningful characters like `[`, `,`, or `]`?
----------------------------------------------------------------
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
[GitHub] [guacamole-client] lchanouha commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
lchanouha commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r537153649
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
##########
@@ -122,16 +134,44 @@
}
// 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);
+
for (Entry <String, Object> attr : ticketAttrs.entrySet()) {
String tokenName = TokenName.canonicalize(attr.getKey(),
CAS_ATTRIBUTE_TOKEN_PREFIX);
Object value = attr.getValue();
- if (value != null)
- tokens.put(tokenName, value.toString());
+ if (value != null) {
+ String attrValue = value.toString();
+ tokens.put(tokenName, attrValue);
+ if (attr.getKey().equals(groupAttribute)) {
+ Matcher matcher =
+ pattern.matcher(attrValue.substring(1,attrValue.length()-1));
+ while (matcher.find()) {
+ effectiveGroups.add(matcher.group(1));
+ }
+ }
Review comment:
Is this truly the case?
=> no, the toString() is converting the collection to something displayable, not iterable (actually this is the toString() of Collection/AbstractCollection: https://www.geeksforgeeks.org/abstractcollection-in-java-with-examples/). This is the CAS java client implementation. instanceof Collection is the way to test if CAS attribute is multivalued.
In the CAS 2.0 protocol, every value of attribute is separated in a xml tag.
This should look like this (not tested):
```
if (attr.getKey().equals(groupAttribute)) {
if (attr.getValue() instanceof Collection) {
for (final Object value : (Collection) attr.getValue()) {
effectiveGroups.add(value.toString());
}
} else {
effectiveGroups.add(attr.getValue().toString()):
}
}
```
(wihout dn strip logic)
```
if (attr.getKey().equals(groupAttribute)) {
if (attr.getValue() instanceof Collection) {
for (final Object value : (Collection) attr.getValue()) {
addEffectiveGroup(value.toString());
}
} else {
addEffectiveGroup(attr.getValue().toString()):
}
}
[...]
private void addEffectiveGroup(String dn){
try {
dn = new LdapName(subject.getName());
for (Rdn rdn : dn.getRdns()) {
// if (rdn.getType().compareToIgnoreCase("cn") == 0) {
effectiveGroups.add(rdn.getValue().toString()):
// }
break;
}
} catch (InvalidNameException e) {
effectiveGroups.add(dn):
}
}
```
IMHO
I will try to provide a full fix next week, I'm still using ugly patch with commas.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r536927311
##########
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:
Also:
* What if the relevant part of the DN in an actual group contains characters that must be escaped for the sake of LDAP (like a comma)?
* What if the relevant groups are nested in different subtrees of a common base DN, or involve different attributes?
It seems to me that the code transforming a DN received from CAS into a generic group name would need to be LDAP syntax-aware, rather than employing pattern matching.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] lchanouha commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
lchanouha commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r537158175
##########
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:
I think LdapName is the way:
http://cr.openjdk.java.net/~iris/se/12/latestSpec/api/java.naming/javax/naming/ldap/LdapName.html#%3Cinit%3E(java.lang.String)
----------------------------------------------------------------
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
[GitHub] [guacamole-client] lchanouha commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
lchanouha commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r534404101
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
##########
@@ -122,16 +134,44 @@
}
// 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);
+
for (Entry <String, Object> attr : ticketAttrs.entrySet()) {
String tokenName = TokenName.canonicalize(attr.getKey(),
CAS_ATTRIBUTE_TOKEN_PREFIX);
Object value = attr.getValue();
- if (value != null)
- tokens.put(tokenName, value.toString());
+ if (value != null) {
+ String attrValue = value.toString();
+ tokens.put(tokenName, attrValue);
+ if (attr.getKey().equals(groupAttribute)) {
+ Matcher matcher =
+ pattern.matcher(attrValue.substring(1,attrValue.length()-1));
Review comment:
My cas (apereo 6.2) is returning groups names as short names, not names
i ended with group "employee," with the comma included. I added replace(",","") to get it working
The toString line 159 seems to be the source of the problem.
Here is an example on how to do it when multivalued:
```
if (entry.getValue() instanceof Collection) {
for (final Object value : (Collection) entry.getValue()) {
attributeValues.add(new StringAttributeValue(value.toString()));
}
} else {
attributeValues.add(new StringAttributeValue(entry.getValue().toString()));
}
```
Taken from here: https://github.com/Unicon/shib-cas-authn3/blob/a8f296b1fd54e940d2b0cd5f4a5f6f5ddaa50bc0/src/main/java/net/unicon/idp/externalauth/AuthenticatedNameTranslator.java#L79
----------------------------------------------------------------
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
[GitHub] [guacamole-client] necouchman commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r516794487
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
##########
@@ -122,16 +134,44 @@
}
// 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*)";
+ }
Review comment:
* Please don't cuddle the `if/else` tags.
* `groupTemplate` line has a 3 space indent.
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/AuthenticationProviderService.java
##########
@@ -82,13 +83,9 @@ public CASAuthenticatedUser authenticateUser(Credentials credentials)
if (request != null) {
String ticket = request.getParameter(CASTicketField.PARAMETER_NAME);
if (ticket != null) {
- Map<String, String> tokens = ticketService.validateTicket(ticket, credentials);
- String username = credentials.getUsername();
- if (username != null) {
- CASAuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
- authenticatedUser.init(username, credentials, tokens);
- return authenticatedUser;
- }
+ CASAuthenticatedUser authenticatedUser =
+ ticketService.validateTicket(ticket, credentials);
+ return authenticatedUser;
Review comment:
Maybe just `return ticketService.validateTicket(ticket, credentials);` here?
##########
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:
I'm not 100% certain if this will work, but, based on this page:
https://www.baeldung.com/java-regexp-escape-char
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 ??
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
##########
@@ -81,14 +93,13 @@
* password values in.
*
* @return
- * A Map all of tokens for the user parsed from attributes returned
- * by the CAS server.
+ * The CASAuthenticatedUser instance returned by the CAS server.
Review comment:
I'm not sure this actually addresses Mike's comment, here. Mike is pointing out that it is inaccurate to say that the "CASAuthenticatedUser instance returned by the CAS server", because the CAS server is not returning a `CASAuthenticatedUser` instance - in fact, the CAS server does not know anything about the `CASAuthenticatedUser` class. So, a more accurate comment would be:
> A CASAuthenticatedUser instance containing the ticket data returned by the CAS server.
Since the ticket _is_ what the CAS server returns.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] lchanouha commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
lchanouha commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r537153649
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
##########
@@ -122,16 +134,44 @@
}
// 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);
+
for (Entry <String, Object> attr : ticketAttrs.entrySet()) {
String tokenName = TokenName.canonicalize(attr.getKey(),
CAS_ATTRIBUTE_TOKEN_PREFIX);
Object value = attr.getValue();
- if (value != null)
- tokens.put(tokenName, value.toString());
+ if (value != null) {
+ String attrValue = value.toString();
+ tokens.put(tokenName, attrValue);
+ if (attr.getKey().equals(groupAttribute)) {
+ Matcher matcher =
+ pattern.matcher(attrValue.substring(1,attrValue.length()-1));
+ while (matcher.find()) {
+ effectiveGroups.add(matcher.group(1));
+ }
+ }
Review comment:
Is this truly the case?
=> no, the toString() is converting the collection to something displayable, not iterable. This is the CAS java client implementation. instanceof Collection is the way to test if CAS attribute is multivalued.
In the CAS 2.0 protocol, every value of attribute is separated in a xml tag.
This should look like this (not tested):
```
if (attr.getKey().equals(groupAttribute)) {
if (attr.getValue() instanceof Collection) {
for (final Object value : (Collection) attr.getValue()) {
effectiveGroups.add(value.toString()); }
} else {
effectiveGroups.add(attr.getValue().toString()):
}
}
}
```
(wihout dn strip logic)
IMHO
I will try to provide a full fix next week, I'm still using ugly patch with commas.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] lchanouha commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
lchanouha commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r537153649
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
##########
@@ -122,16 +134,44 @@
}
// 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);
+
for (Entry <String, Object> attr : ticketAttrs.entrySet()) {
String tokenName = TokenName.canonicalize(attr.getKey(),
CAS_ATTRIBUTE_TOKEN_PREFIX);
Object value = attr.getValue();
- if (value != null)
- tokens.put(tokenName, value.toString());
+ if (value != null) {
+ String attrValue = value.toString();
+ tokens.put(tokenName, attrValue);
+ if (attr.getKey().equals(groupAttribute)) {
+ Matcher matcher =
+ pattern.matcher(attrValue.substring(1,attrValue.length()-1));
+ while (matcher.find()) {
+ effectiveGroups.add(matcher.group(1));
+ }
+ }
Review comment:
Is this truly the case?
=> no, the toString() is converting the collection to something displayable, not iterable. This is the CAS java client implementation. instanceof Collection is the way to test if CAS attribute is multivalued.
In the CAS 2.0 protocol, every value of attribute is separated in a xml tag.
This should look like this (not tested):
```
if (attr.getKey().equals(groupAttribute)) {
if (attr.getValue() instanceof Collection) {
for (final Object value : (Collection) attr.getValue()) {
effectiveGroups.add(value.toString()); }
} else {
effectiveGroups.add(attr.getValue().toString()):
}
}
}
```
(wihout dn strip logic)
IMHO
----------------------------------------------------------------
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
[GitHub] [guacamole-client] doctorfree commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
doctorfree commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r516793331
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java
##########
@@ -85,4 +85,40 @@ public PrivateKey getClearpassKey() throws GuacamoleException {
return environment.getProperty(CASGuacamoleProperties.CAS_CLEARPASS_KEY);
}
+ /**
+ * Returns the attribute used to determine group memberships
+ * in CAS, or null if not defined.
+ *
+ * @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 attribute used to format group DN's
+ * in CAS, or null if not defined. If CAS is backed by LDAP, it will
+ * return an LDAP DN, such as CN=foo,OU=bar,DC=example,DC=com.
+ * This attribute may be set to CN=%s,OU=bar,DC=example,DC=com and given
+ * the example above, would result in a group called "foo." CAS backed
+ * by something other than LDAP (such as a database) would likely not
+ * need this.
Review comment:
Clarified the comment and fixed line wrapping
----------------------------------------------------------------
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
[GitHub] [guacamole-client] lchanouha commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
lchanouha commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r537153649
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
##########
@@ -122,16 +134,44 @@
}
// 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);
+
for (Entry <String, Object> attr : ticketAttrs.entrySet()) {
String tokenName = TokenName.canonicalize(attr.getKey(),
CAS_ATTRIBUTE_TOKEN_PREFIX);
Object value = attr.getValue();
- if (value != null)
- tokens.put(tokenName, value.toString());
+ if (value != null) {
+ String attrValue = value.toString();
+ tokens.put(tokenName, attrValue);
+ if (attr.getKey().equals(groupAttribute)) {
+ Matcher matcher =
+ pattern.matcher(attrValue.substring(1,attrValue.length()-1));
+ while (matcher.find()) {
+ effectiveGroups.add(matcher.group(1));
+ }
+ }
Review comment:
Is this truly the case?
=> no, the toString() is converting the collection to something displayable, not iterable (actually this is the toString() of Collection/AbstractCollection: https://www.geeksforgeeks.org/abstractcollection-in-java-with-examples/). This is the CAS java client implementation. instanceof Collection is the way to test if CAS attribute is multivalued.
In the CAS 2.0 protocol, every value of attribute is separated in a xml tag.
This should look like this (not tested):
```
if (attr.getKey().equals(groupAttribute)) {
if (attr.getValue() instanceof Collection) {
for (final Object value : (Collection) attr.getValue()) {
effectiveGroups.add(value.toString()); }
} else {
effectiveGroups.add(attr.getValue().toString()):
}
}
}
```
(wihout dn strip logic)
IMHO
I will try to provide a full fix next week, I'm still using ugly patch with commas.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] doctorfree commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
doctorfree commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r516941589
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
##########
@@ -81,14 +93,13 @@
* password values in.
*
* @return
- * A Map all of tokens for the user parsed from attributes returned
- * by the CAS server.
+ * The CASAuthenticatedUser instance returned by the CAS server.
Review comment:
Committed the suggested JavaDoc change to the description of the return value. However, my latest commits are not showing up here after rebasing on staging/1.3.0 so maybe I need to go back to git school.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by 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
[GitHub] [guacamole-client] doctorfree commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
doctorfree commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r516792809
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/conf/ConfigurationService.java
##########
@@ -85,4 +85,40 @@ public PrivateKey getClearpassKey() throws GuacamoleException {
return environment.getProperty(CASGuacamoleProperties.CAS_CLEARPASS_KEY);
}
+ /**
+ * Returns the attribute used to determine group memberships
+ * in CAS, or null if not defined.
+ *
+ * @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 attribute used to format group DN's
+ * in CAS, or null if not defined. If CAS is backed by LDAP, it will
+ * return an LDAP DN, such as CN=foo,OU=bar,DC=example,DC=com.
+ * This attribute may be set to CN=%s,OU=bar,DC=example,DC=com and given
+ * the example above, would result in a group called "foo." CAS backed
+ * by something other than LDAP (such as a database) would likely not
+ * need this.
+ *
+ * @return
+ * The attribute name
Review comment:
Added the period
----------------------------------------------------------------
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
[GitHub] [guacamole-client] doctorfree commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
doctorfree commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r516795167
##########
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:
This is the only feedback I have not yet addressed in commits. I am not sure if this is a problem or, if it is, how best to address it. I will look into this later this week if I have time. Please feel free to suggest or advise on best approach.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] doctorfree commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
doctorfree commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r519920968
##########
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:
Although best practices would prohibit special characters in the DN it does not guarantee that. I believe your suggested use of \Q and \E to escape any special characters in the stripped DN is a good idea. Can we simply use the Pattern.quote() method as follows:
groupTemplate = Pattern.quote(groupDnFormat.replace("%s","([A-Za-z0-9_\\(\\)\\-\\.\\s+]+)"));
Or am I mistaken about where to escape the stripped DN?
----------------------------------------------------------------
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
[GitHub] [guacamole-client] doctorfree commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
doctorfree commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r516790601
##########
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);
+
for (Entry <String, Object> attr : ticketAttrs.entrySet()) {
String tokenName = TokenName.canonicalize(attr.getKey(),
CAS_ATTRIBUTE_TOKEN_PREFIX);
Object value = attr.getValue();
- if (value != null)
+ if (value != null) {
tokens.put(tokenName, value.toString());
+ if (attr.getKey().equals(groupAttribute)) {
+ String valueWithoutBrackets = value.toString().substring(1,value.toString().length()-1);
+ Matcher matcher = pattern.matcher(valueWithoutBrackets);
+ while (matcher.find()) {
+ effectiveGroups.add(matcher.group(1));
+ }
+ }
+ }
Review comment:
These two changes are in the latest commits, value.toString() is assigned to a String variable and the substring expression is used directly in the call to pattern.matcher(). Thanks for the suggested code improvements.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] lchanouha commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
lchanouha commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r537153649
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
##########
@@ -122,16 +134,44 @@
}
// 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);
+
for (Entry <String, Object> attr : ticketAttrs.entrySet()) {
String tokenName = TokenName.canonicalize(attr.getKey(),
CAS_ATTRIBUTE_TOKEN_PREFIX);
Object value = attr.getValue();
- if (value != null)
- tokens.put(tokenName, value.toString());
+ if (value != null) {
+ String attrValue = value.toString();
+ tokens.put(tokenName, attrValue);
+ if (attr.getKey().equals(groupAttribute)) {
+ Matcher matcher =
+ pattern.matcher(attrValue.substring(1,attrValue.length()-1));
+ while (matcher.find()) {
+ effectiveGroups.add(matcher.group(1));
+ }
+ }
Review comment:
Is this truly the case?
=> no, the toString() is converting the collection to something displayable, not iterable (actually this is the toString() of Collection/AbstractCollection: https://www.geeksforgeeks.org/abstractcollection-in-java-with-examples/). This is the CAS java client implementation. instanceof Collection is the way to test if CAS attribute is multivalued.
In the CAS 2.0 protocol, every value of attribute is separated in a xml tag.
This should look like this (not tested):
```
if (attr.getKey().equals(groupAttribute)) {
if (attr.getValue() instanceof Collection) {
for (final Object value : (Collection) attr.getValue()) {
effectiveGroups.add(value.toString());
}
} else {
effectiveGroups.add(attr.getValue().toString()):
}
}
```
and with DN strip logic:
```
if (attr.getKey().equals(groupAttribute)) {
if (attr.getValue() instanceof Collection) {
for (final Object value : (Collection) attr.getValue()) {
addEffectiveGroup(value.toString());
}
} else {
addEffectiveGroup(attr.getValue().toString()):
}
}
[...]
private void addEffectiveGroup(String dn){
try {
dn = new LdapName(subject.getName());
for (Rdn rdn : dn.getRdns()) {
// if (rdn.getType().compareToIgnoreCase("cn") == 0) {
effectiveGroups.add(rdn.getValue().toString()):
// }
break;
}
} catch (InvalidNameException e) {
effectiveGroups.add(dn):
}
}
```
IMHO. What do you think about it ?
I will try to provide a full fix next week, I'm still using ugly patch with commas.
----------------------------------------------------------------
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
[GitHub] [guacamole-client] doctorfree commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
doctorfree commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r516793894
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/ticket/TicketValidationService.java
##########
@@ -81,14 +93,13 @@
* password values in.
*
* @return
- * A Map all of tokens for the user parsed from attributes returned
- * by the CAS server.
+ * The CASAuthenticatedUser instance returned by the CAS server.
Review comment:
Latest commits should resolve this and an instance of CASAuthenticatedUser should be returned now
----------------------------------------------------------------
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
[GitHub] [guacamole-client] doctorfree commented on a change in pull request #563: GUACAMOLE-793: validateTicket() returns the CASAuthenticatedUser instance
Posted by GitBox <gi...@apache.org>.
doctorfree commented on a change in pull request #563:
URL: https://github.com/apache/guacamole-client/pull/563#discussion_r516795652
##########
File path: extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/user/CASAuthenticatedUser.java
##########
@@ -50,6 +51,11 @@
*/
private Map<String, String> tokens;
+ /**
+ * The unique identifiers of all user groups which a user is a member of.
Review comment:
Corrected. Yes, it should have been "this user"
----------------------------------------------------------------
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