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 2021/08/22 01:31:56 UTC
[GitHub] [guacamole-client] necouchman opened a new pull request #640: GUACAMOLE-1130: Limit LDAP attribute retrieval to only those required or requested
necouchman opened a new pull request #640:
URL: https://github.com/apache/guacamole-client/pull/640
This builds on #550 by @echu2013, incorporating a couple of cleanup changes to the code that limits the retrieval of LDAP attributes to ones that are specifically required by the code or requested by the Guacamole configuration, making LDAP searches more efficient.
This closes #550.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on a change in pull request #640: GUACAMOLE-1130: Limit LDAP attribute retrieval to only those required or requested
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #640:
URL: https://github.com/apache/guacamole-client/pull/640#discussion_r694408052
##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/group/UserGroupService.java
##########
@@ -214,13 +215,17 @@ private ExprNode getGroupSearchFilter() throws GuacamoleException {
}
}
+ // Gather all attributes relevant for a group
+ List<String> groupAttributes = confService.getGroupNameAttributes();
+ groupAttributes.add(confService.getMemberAttribute());
+
// Get all groups the user is a member of starting at the groupBaseDN,
// excluding guacConfigGroups
return queryService.search(
ldapConnection,
groupBaseDN,
getGroupSearchFilter(),
- Collections.singleton(confService.getMemberAttribute()),
+ groupAttributes,
Review comment:
Ugh, now that I look at this, I think this is not going to result in intended behavior at all. This change, here, will make it so that the query that's run to match membership gets executed against all of the group attributes that are identified as "relevant" attributes, via this code, here:
https://github.com/necouchman/guacamole-client/blob/d6c161c1d67c349a1796fb346a5c33cb744073a0/extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ObjectQueryService.java#L336-L341
I'm pretty sure that's not what we want to happen - we want certain attributes to be retrieved, not searched against. So, this is going to take a little more reworking...
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper commented on pull request #640: GUACAMOLE-1130: Limit LDAP attribute retrieval to only those required or requested
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on pull request #640:
URL: https://github.com/apache/guacamole-client/pull/640#issuecomment-943106852
Looks pretty good! I will re-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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] mike-jumper merged pull request #640: GUACAMOLE-1130: Limit LDAP attribute retrieval to only those required or requested
Posted by GitBox <gi...@apache.org>.
mike-jumper merged pull request #640:
URL: https://github.com/apache/guacamole-client/pull/640
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on pull request #640: GUACAMOLE-1130: Limit LDAP attribute retrieval to only those required or requested
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #640:
URL: https://github.com/apache/guacamole-client/pull/640#issuecomment-904252091
Okay, I've reworked this a bit, but ***have not tested, yet*** - I'll give it a go when I get a chance. I may have gone a bit overboard reworking the `ConnectionService` class, in particular - I can back out some of those constants and such if that's just too complicating to the code.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on a change in pull request #640: GUACAMOLE-1130: Limit LDAP attribute retrieval to only those required or requested
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #640:
URL: https://github.com/apache/guacamole-client/pull/640#discussion_r694404263
##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/group/UserGroupService.java
##########
@@ -192,7 +192,8 @@ private ExprNode getGroupSearchFilter() throws GuacamoleException {
ldapConnection,
userDN,
confService.getUserSearchFilter(),
- 0);
+ 0,
+ null);
Review comment:
I'll take a look and see - probably can be restricted.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on pull request #640: GUACAMOLE-1130: Limit LDAP attribute retrieval to only those required or requested
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #640:
URL: https://github.com/apache/guacamole-client/pull/640#issuecomment-939145676
Okay, fixed a couple of issues and did some testing, looks clean to me.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [guacamole-client] necouchman commented on pull request #640: GUACAMOLE-1130: Limit LDAP attribute retrieval to only those required or requested
Posted by GitBox <gi...@apache.org>.
necouchman commented on pull request #640:
URL: https://github.com/apache/guacamole-client/pull/640#issuecomment-904234642
Yeah, I'll see about applying this to users and `guacConfigGroup`s, as well.
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
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 #640: GUACAMOLE-1130: Limit LDAP attribute retrieval to only those required or requested
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #640:
URL: https://github.com/apache/guacamole-client/pull/640#discussion_r694396228
##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/group/UserGroupService.java
##########
@@ -192,7 +192,8 @@ private ExprNode getGroupSearchFilter() throws GuacamoleException {
ldapConnection,
userDN,
confService.getUserSearchFilter(),
- 0);
+ 0,
+ null);
Review comment:
Why `null` here vs. a more restricted set of attributes?
--
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.
To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org