You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by necouchman <gi...@git.apache.org> on 2017/03/21 02:40:10 UTC
[GitHub] incubator-guacamole-client pull request #132: GUACAMOLE-101: Allow Filtering...
GitHub user necouchman opened a pull request:
https://github.com/apache/incubator-guacamole-client/pull/132
GUACAMOLE-101: Allow Filtering of LDAP Users
Took a stab at implementing two LDAP filter configuration properties for user and connection searches.
This was a very quick stab at implementing it - there are a couple of things that might be worth adding:
- Error checking on the LDAP query - not sure if there's an easy way to parse out the filters to make sure that they're valid before actually running the searches?? The LDAPException should handle this - there are specific messages for filter errors.
- The JIRA issue mentions implementing multiple search bases - this does not do anything with that.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/incubator-guacamole-client GUACAMOLE-101
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-client/pull/132.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #132
----
commit e5daa1fc7d50d1dc1cccf7c6756a3f572edde1d7
Author: Nick Couchman <ni...@yahoo.com>
Date: 2017-03-21T02:15:14Z
GUACAMOLE-101: Impelement properties for controller user and connection search filters.
commit e8629237b8ffee941eb2bf9eda62bb3e8e341571
Author: Nick Couchman <ni...@yahoo.com>
Date: 2017-03-21T02:29:55Z
GUACAMOLE-101: Update comments to some of the changes.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #132: GUACAMOLE-101: Allow Filtering...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/132#discussion_r108457046
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -85,11 +85,22 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
try {
+ // Build a filter using the configured or default user search filter
+ // to find all user objects in the LDAP tree
+ StringBuilder userSearchFilter = new StringBuilder();
+ userSearchFilter.append("(&");
+ userSearchFilter.append(confService.getUserSearchFilter());
+ userSearchFilter.append("(");
+ userSearchFilter.append(escapingService.escapeLDAPSearchFilter(usernameAttribute));
+ userSearchFilter.append("=*)");
+ userSearchFilter.append(")");
+
+
--- End diff --
Please kill one of these blank lines - separating things by two blanks is somewhat unusual compared to the surrounding code.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #132: GUACAMOLE-101: Allow Filtering...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/132#discussion_r108327237
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -270,7 +270,46 @@ public LDAPSearchConstraints getLDAPSearchConstraints() throws GuacamoleExceptio
constraints.setDereference(getDereferenceAliases().DEREF_VALUE);
return constraints;
+ }
+
+ /**
+ * 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=* is returned.
+ *
+ * @return
+ * The search filter that should be used when querying the
+ * LDAP server for users that are valid in Guacamole, or
+ * objectClass=* if not specified.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public String getUserSearchFilter() throws GuacamoleException {
+ return environment.getProperty(
+ LDAPGuacamoleProperties.LDAP_USER_SEARCH_FILTER,
+ "(objectClass=*)"
+ );
+ }
+ /**
+ * Returns the search filter that should be used when querying the
+ * LDAP server for Guacamole connections. If no filter is specified,
+ * the default of objectClass=guacConfigGroup is returned.
+ *
+ * @return
+ * The search filter that should be used when querying the
+ * LDAP server for connections for Guacamole, or
+ * objectClass=guacConfigGroup if no filter is specified.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public String getConnectionSearchFilter() throws GuacamoleException {
+ return environment.getProperty(
+ LDAPGuacamoleProperties.LDAP_CONNECTION_SEARCH_FILTER,
+ "(objectClass=guacConfigGroup)"
--- End diff --
If we're going to allow connection filters to be defined, `(objectClass=guacConfigGroup)` should probably be of the filter in all cases. Unlike the objects returned by the user search, the connection search really must be strictly `guacConfigGroup` objects as defined by the schema modifications included with the LDAP extension.
Since connections returned by the search using this filter will already be limited by membership (users must be explicit members of the group to see the connection), this probably simply isn't necessary and can be simply removed.
Given that the summary of [GUACAMOLE-101](https://issues.apache.org/jira/browse/GUACAMOLE-101) is "Allow arbitrary filtering of LDAP users", it is probably also out of scope.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #132: GUACAMOLE-101: Allow Filtering...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/132#discussion_r108327688
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -270,7 +270,46 @@ public LDAPSearchConstraints getLDAPSearchConstraints() throws GuacamoleExceptio
constraints.setDereference(getDereferenceAliases().DEREF_VALUE);
return constraints;
+ }
+
+ /**
+ * 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=* is returned.
--- End diff --
a default of "(objectClass=*)" is returned.
The parenthesis are a critical part of LDAP filter syntax.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #132: GUACAMOLE-101: Allow Filtering...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/132#discussion_r108456437
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -85,11 +85,22 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
try {
+ // Build a filter using the configured or default user search filter
+ // to find all user objects in the LDAP tree
+ StringBuilder userSearchFilter = new StringBuilder();
+ userSearchFilter.append("(&");
+ userSearchFilter.append(confService.getUserSearchFilter());
+ userSearchFilter.append("(");
+ userSearchFilter.append(escapingService.escapeLDAPSearchFilter(usernameAttribute));
+ userSearchFilter.append("=*)");
+ userSearchFilter.append(")");
--- End diff --
> userSearchFilter.append("=*)");
> userSearchFilter.append(")");
Really should be ```userSearchFilter.append("=*))")``` - no need for splitting across multiple `append()` calls what is otherwise a single string constant.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #132: GUACAMOLE-101: Allow Filtering...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/132#discussion_r108458000
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -85,11 +85,22 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
try {
+ // Build a filter using the configured or default user search filter
+ // to find all user objects in the LDAP tree
+ StringBuilder userSearchFilter = new StringBuilder();
+ userSearchFilter.append("(&");
+ userSearchFilter.append(confService.getUserSearchFilter());
+ userSearchFilter.append("(");
+ userSearchFilter.append(escapingService.escapeLDAPSearchFilter(usernameAttribute));
+ userSearchFilter.append("=*)");
+ userSearchFilter.append(")");
+
+
--- End diff --
Fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #132: GUACAMOLE-101: Allow Filtering...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/132#discussion_r108454222
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -85,11 +85,20 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
try {
+ // Build a filter using the configured or default user search filter
+ // to find all user objects in the LDAP tree
+ StringBuilder userSearchFilter = new StringBuilder();
+ userSearchFilter.append("(&");
+ userSearchFilter.append(confService.getUserSearchFilter());
+ userSearchFilter.append("(" + escapingService.escapeLDAPSearchFilter(usernameAttribute) + "=*)");
--- End diff --
If using a `StringBuilder` (good), you shouldn't be doing string concatenation with `+`. The inline concatenation here will actually result in Java creating a temporary `StringBuilder` for the concatenation operation.
You should either do the whole thing with a single line of chained `+`:
"(&" + confService.getUserSearchFilter() + "(" + escapingService.escapeLDAPSearchFilter(usernameAttribute) + "=*))"
or use only the `StringBuilder` you've created:
StringBuilder userSearchFilter = new StringBuilder();
userSearchFilter.append("(&");
userSearchFilter.append(confService.getUserSearchFilter());
userSearchFilter.append("(");
userSearchFilter.append(escapingService.escapeLDAPSearchFilter(usernameAttribute));
userSearchFilter.append("=*))");
FYI: In the case of the construction of the other query below (```StringBuilder ldapQuery = ...```), the use of a `StringBuilder` is absolutely necessary, since parts of the concatenation operation are conditional, and using Java's `+` operator would result in unnecessary creation of temporary `String` and `StringBuilder` objects for intermediate results.
In this case, you're safe either way, but using both is wasteful.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #132: GUACAMOLE-101: Allow Filtering...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/132#discussion_r108399810
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -270,7 +270,46 @@ public LDAPSearchConstraints getLDAPSearchConstraints() throws GuacamoleExceptio
constraints.setDereference(getDereferenceAliases().DEREF_VALUE);
return constraints;
+ }
+
+ /**
+ * 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=* is returned.
+ *
+ * @return
+ * The search filter that should be used when querying the
+ * LDAP server for users that are valid in Guacamole, or
+ * objectClass=* if not specified.
--- End diff --
Fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #132: GUACAMOLE-101: Allow Filtering...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/132#discussion_r108327714
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -270,7 +270,46 @@ public LDAPSearchConstraints getLDAPSearchConstraints() throws GuacamoleExceptio
constraints.setDereference(getDereferenceAliases().DEREF_VALUE);
return constraints;
+ }
+
+ /**
+ * 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=* is returned.
+ *
+ * @return
+ * The search filter that should be used when querying the
+ * LDAP server for users that are valid in Guacamole, or
+ * objectClass=* if not specified.
--- End diff --
Same here - should be "(objectClass=*)".
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #132: GUACAMOLE-101: Allow Filtering...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/132#discussion_r108457964
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -85,11 +85,22 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
try {
+ // Build a filter using the configured or default user search filter
+ // to find all user objects in the LDAP tree
+ StringBuilder userSearchFilter = new StringBuilder();
+ userSearchFilter.append("(&");
+ userSearchFilter.append(confService.getUserSearchFilter());
+ userSearchFilter.append("(");
+ userSearchFilter.append(escapingService.escapeLDAPSearchFilter(usernameAttribute));
+ userSearchFilter.append("=*)");
+ userSearchFilter.append(")");
--- End diff --
Fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #132: GUACAMOLE-101: Allow Filtering...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/132#discussion_r108455321
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -85,11 +85,20 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
try {
+ // Build a filter using the configured or default user search filter
+ // to find all user objects in the LDAP tree
+ StringBuilder userSearchFilter = new StringBuilder();
+ userSearchFilter.append("(&");
+ userSearchFilter.append(confService.getUserSearchFilter());
+ userSearchFilter.append("(" + escapingService.escapeLDAPSearchFilter(usernameAttribute) + "=*)");
--- End diff --
Switched it all to StringBuilder, as you suggested.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #132: GUACAMOLE-101: Allow Filtering...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-guacamole-client/pull/132
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #132: GUACAMOLE-101: Allow Filtering...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/132#discussion_r108399836
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -270,7 +270,46 @@ public LDAPSearchConstraints getLDAPSearchConstraints() throws GuacamoleExceptio
constraints.setDereference(getDereferenceAliases().DEREF_VALUE);
return constraints;
+ }
+
+ /**
+ * 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=* is returned.
+ *
+ * @return
+ * The search filter that should be used when querying the
+ * LDAP server for users that are valid in Guacamole, or
+ * objectClass=* if not specified.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public String getUserSearchFilter() throws GuacamoleException {
+ return environment.getProperty(
+ LDAPGuacamoleProperties.LDAP_USER_SEARCH_FILTER,
+ "(objectClass=*)"
+ );
+ }
+ /**
+ * Returns the search filter that should be used when querying the
+ * LDAP server for Guacamole connections. If no filter is specified,
+ * the default of objectClass=guacConfigGroup is returned.
+ *
+ * @return
+ * The search filter that should be used when querying the
+ * LDAP server for connections for Guacamole, or
+ * objectClass=guacConfigGroup if no filter is specified.
+ *
+ * @throws GuacamoleException
+ * If guacamole.properties cannot be parsed.
+ */
+ public String getConnectionSearchFilter() throws GuacamoleException {
+ return environment.getProperty(
+ LDAPGuacamoleProperties.LDAP_CONNECTION_SEARCH_FILTER,
+ "(objectClass=guacConfigGroup)"
--- End diff --
Removed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-guacamole-client pull request #132: GUACAMOLE-101: Allow Filtering...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/132#discussion_r108399796
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -270,7 +270,46 @@ public LDAPSearchConstraints getLDAPSearchConstraints() throws GuacamoleExceptio
constraints.setDereference(getDereferenceAliases().DEREF_VALUE);
return constraints;
+ }
+
+ /**
+ * 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=* is returned.
--- End diff --
Fixed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---