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/04/03 23:35:39 UTC
[GitHub] [guacamole-client] echu2013 opened a new pull request #496:
GUACAMOLE-996
echu2013 opened a new pull request #496: GUACAMOLE-996
URL: https://github.com/apache/guacamole-client/pull/496
This provides ldap-group-search-filter functionallity. Tested and working OK!
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] echu2013 commented on a change in pull request
#496: GUACAMOLE-996
Posted by GitBox <gi...@apache.org>.
echu2013 commented on a change in pull request #496: GUACAMOLE-996
URL: https://github.com/apache/guacamole-client/pull/496#discussion_r403395864
##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/group/UserGroupService.java
##########
@@ -79,17 +79,17 @@
* @throws GuacamoleException
* If guacamole.properties cannot be parsed.
*/
- private ExprNode getGroupSearchFilter() throws GuacamoleException {
+ private ExprNode buildGroupSearchFilter() throws GuacamoleException {
// Explicitly exclude guacConfigGroup object class only if it should
// be assumed to be defined (query may fail due to no such object
// class existing otherwise)
if (confService.getConfigurationBaseDN() != null)
return new NotNode(new EqualityNode("objectClass","guacConfigGroup"));
- // Read any object as a group if LDAP is not being used for connection
+ // Read objects from LDAP with filter defined by "ldap-group-search-filter" as a group if LDAP is not being used for connection
Review comment:
@necouchman sorry, i am pretty newie with contributing online and I make some mistakes. You are right about The out of scope stuff.. And I will rewrite the "pretty long" comment, how could I update this pull request?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] echu2013 commented on issue #496: GUACAMOLE-996:
Add support for configuring group filter.
Posted by GitBox <gi...@apache.org>.
echu2013 commented on issue #496: GUACAMOLE-996: Add support for configuring group filter.
URL: https://github.com/apache/guacamole-client/pull/496#issuecomment-609093316
Closing in favor of #497 , sorry for the mess.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] necouchman commented on a change in pull request
#496: GUACAMOLE-996
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #496: GUACAMOLE-996
URL: https://github.com/apache/guacamole-client/pull/496#discussion_r403391139
##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/group/UserGroupService.java
##########
@@ -79,17 +79,17 @@
* @throws GuacamoleException
* If guacamole.properties cannot be parsed.
*/
- private ExprNode getGroupSearchFilter() throws GuacamoleException {
+ private ExprNode buildGroupSearchFilter() throws GuacamoleException {
// Explicitly exclude guacConfigGroup object class only if it should
// be assumed to be defined (query may fail due to no such object
// class existing otherwise)
if (confService.getConfigurationBaseDN() != null)
return new NotNode(new EqualityNode("objectClass","guacConfigGroup"));
- // Read any object as a group if LDAP is not being used for connection
+ // Read objects from LDAP with filter defined by "ldap-group-search-filter" as a group if LDAP is not being used for connection
Review comment:
This should probably just be made a multi-line comment - it's pretty long.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] necouchman commented on a change in pull request
#496: GUACAMOLE-996
Posted by GitBox <gi...@apache.org>.
necouchman commented on a change in pull request #496: GUACAMOLE-996
URL: https://github.com/apache/guacamole-client/pull/496#discussion_r403390730
##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/conf/ConfigurationService.java
##########
@@ -317,7 +318,27 @@ public int getMaxReferralHops() throws GuacamoleException {
public ExprNode getUserSearchFilter() throws GuacamoleException {
return environment.getProperty(
LDAPGuacamoleProperties.LDAP_USER_SEARCH_FILTER,
- new PresenceNode("objectClass")
+ new EqualityNode("objectClass","user")
Review comment:
This seems out-of-scope for this particular pull request. We might discuss the merits of doing this at some point, but I don't think it should be included in an issue to implement the group filter.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] mike-jumper commented on a change in pull
request #496: GUACAMOLE-996
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #496: GUACAMOLE-996
URL: https://github.com/apache/guacamole-client/pull/496#discussion_r403405947
##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/group/UserGroupService.java
##########
@@ -79,17 +79,17 @@
* @throws GuacamoleException
* If guacamole.properties cannot be parsed.
*/
- private ExprNode getGroupSearchFilter() throws GuacamoleException {
+ private ExprNode buildGroupSearchFilter() throws GuacamoleException {
Review comment:
Why is this being renamed?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] echu2013 closed pull request #496:
GUACAMOLE-996: Add support for configuring group filter.
Posted by GitBox <gi...@apache.org>.
echu2013 closed pull request #496: GUACAMOLE-996: Add support for configuring group filter.
URL: https://github.com/apache/guacamole-client/pull/496
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] echu2013 commented on a change in pull request
#496: GUACAMOLE-996: Add support for configuring group filter.
Posted by GitBox <gi...@apache.org>.
echu2013 commented on a change in pull request #496: GUACAMOLE-996: Add support for configuring group filter.
URL: https://github.com/apache/guacamole-client/pull/496#discussion_r403761407
##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/conf/ConfigurationService.java
##########
@@ -317,7 +318,27 @@ public int getMaxReferralHops() throws GuacamoleException {
public ExprNode getUserSearchFilter() throws GuacamoleException {
return environment.getProperty(
LDAPGuacamoleProperties.LDAP_USER_SEARCH_FILTER,
- new PresenceNode("objectClass")
+ new EqualityNode("objectClass","user")
Review comment:
> +1
>
> Adding the option to narrow by `objectClass` would be a separate feature, the value that is used should not be hard-coded to `user`, and we should avoid breaking compatibility with existing deployments which have been relying on the former `*` filter for ages.
>
> Same for the group filter - if the deployment in question needs to narrow things, then specifying custom filters will allow that. Beyond the filters, the different base DNs should be the only restrictions applied unless the administrator configures otherwise.
Please note that description inside code states the following:
` /**
* Returns the search filter that should be used when querying the
* LDAP server for Guacamole users. If no filter is specified,
* a default of "(objectClass=user)" is returned.
*
* @return
* The search filter that should be used when querying the
* LDAP server for users that are valid in Guacamole, or
* "(objectClass=user)" if not specified.
*
* @throws GuacamoleException
* If guacamole.properties cannot be parsed.
*/`
**If no filter is specified, a default of "(objectClass=user)" is returned.**
So, that's why I corrected 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
With regards,
Apache Git Services
[GitHub] [guacamole-client] echu2013 commented on a change in pull request
#496: GUACAMOLE-996: Add support for configuring group filter.
Posted by GitBox <gi...@apache.org>.
echu2013 commented on a change in pull request #496: GUACAMOLE-996: Add support for configuring group filter.
URL: https://github.com/apache/guacamole-client/pull/496#discussion_r403524577
##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/group/UserGroupService.java
##########
@@ -79,17 +79,17 @@
* @throws GuacamoleException
* If guacamole.properties cannot be parsed.
*/
- private ExprNode getGroupSearchFilter() throws GuacamoleException {
+ private ExprNode buildGroupSearchFilter() throws GuacamoleException {
Review comment:
I though it was more descriptive this name rather than get, also to avoid confusion with
` return confService.getGroupSearchFilter();
`
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [guacamole-client] mike-jumper commented on a change in pull
request #496: GUACAMOLE-996
Posted by GitBox <gi...@apache.org>.
mike-jumper commented on a change in pull request #496: GUACAMOLE-996
URL: https://github.com/apache/guacamole-client/pull/496#discussion_r403405847
##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/conf/ConfigurationService.java
##########
@@ -317,7 +318,27 @@ public int getMaxReferralHops() throws GuacamoleException {
public ExprNode getUserSearchFilter() throws GuacamoleException {
return environment.getProperty(
LDAPGuacamoleProperties.LDAP_USER_SEARCH_FILTER,
- new PresenceNode("objectClass")
+ new EqualityNode("objectClass","user")
Review comment:
+1
Adding the option to narrow by `objectClass` would be a separate feature, the value that is used should not be hard-coded to `user`, and we should avoid breaking compatibility with existing deployments which have been relying on the former `*` filter for ages.
Same for the group filter - if the deployment in question needs to narrow things, then specifying custom filters will allow that. Beyond the filters, the different base DNs should be the only restrictions applied unless the administrator configures otherwise.
----------------------------------------------------------------
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
With regards,
Apache Git Services