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 2018/12/09 16:08:56 UTC
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
GitHub user necouchman opened a pull request:
https://github.com/apache/guacamole-client/pull/345
GUACAMOLE-234: Migrate to Apache Directory API for LDAP Extension
This change request takes a new stab at migrating from the legacy Novell JLDAP API to the Apache Directory API for LDAP access. I've tried to make sure we're using as much of the API as possible for parsing and verification and eliminated overlapping functionality. I'm sure there are some more optimizations that could be done, but this pretty well takes care of things for now, I think.
I have tested it against ActiveDirectory and it seems to work fine, but it'd be nice if others could test it against other directories.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/guacamole-client jira/234
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-client/pull/345.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 #345
----
commit 6643923a61fbd97866d889eea78194e090c0a374
Author: Nick Couchman <vn...@...>
Date: 2018-12-09T13:32:16Z
GUACAMOLE-234: Convert LDAP extension to use Apache Directory LDAP API.
commit 37022bb552e60631033f3630bc0a0545f9c6f1d7
Author: Nick Couchman <vn...@...>
Date: 2018-12-09T14:48:01Z
GUACAMOLE-234: Clean up some LDAP implementation details.
commit 2abd46b0e130548fe1219de7d6369b724c80fa55
Author: Nick Couchman <vn...@...>
Date: 2018-12-09T15:42:12Z
GUACAMOLE-234: Clean up comments.
commit 197f5fe7997cc1f8a636e9c114e2dd967b8ae4a1
Author: Nick Couchman <vn...@...>
Date: 2018-12-09T15:55:39Z
GUACAMOLE-234: Exclude slf4j from Apache Directory dependency.
----
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241916443
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -240,17 +244,24 @@ public LDAPAuthenticatedUser authenticateUser(Credentials credentials)
try {
+ LdapConnectionConfig ldapConnectionConfig =
+ ((LdapNetworkConnection) ldapConnection).getConfig();
+ Dn authDn = new Dn(ldapConnectionConfig.getName());
--- End diff --
I'll take a look and try to figure something out.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241928420
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java ---
@@ -156,38 +146,84 @@ public LDAPConnection bindAs(String userDN, String password)
// Bind using provided credentials
try {
- byte[] passwordBytes;
- try {
-
- // Convert password into corresponding byte array
- if (password != null)
- passwordBytes = password.getBytes("UTF-8");
- else
- passwordBytes = null;
-
- }
- catch (UnsupportedEncodingException e) {
- logger.error("Unexpected lack of support for UTF-8: {}", e.getMessage());
- logger.debug("Support for UTF-8 (as required by Java spec) not found.", e);
- disconnect(ldapConnection);
- return null;
- }
-
- // Bind as user
- ldapConnection.bind(LDAPConnection.LDAP_V3, userDN, passwordBytes);
+ BindRequest bindRequest = new BindRequestImpl();
+ bindRequest.setDn(userDN);
+ bindRequest.setCredentials(password);
+ ldapConnection.bind(bindRequest);
}
// Disconnect if an error occurs during bind
- catch (LDAPException e) {
- logger.debug("LDAP bind failed.", e);
+ catch (LdapException e) {
+ logger.debug("Unable to bind to LDAP server.", e);
disconnect(ldapConnection);
return null;
}
return ldapConnection;
}
+
+ /**
+ * Generate a new LdapConnection object for following a referral
+ * with the given LdapUrl, and copy the username and password
+ * from the original connection.
+ *
+ * @param referralUrl
+ * The LDAP URL to follow.
+ *
+ * @param ldapConfig
+ * The connection configuration to use to retrieve username and
+ * password.
+ *
+ * @param hop
+ * The current hop number of this referral - once the configured
+ * limit is reached, this method will throw an exception.
+ *
+ * @return
+ * A LdapConnection object that points at the location
+ * specified in the referralUrl.
+ *
+ * @throws GuacamoleException
+ * If an error occurs parsing out the LdapUrl object or the
+ * maximum number of referral hops is reached.
+ */
+ public LdapConnection referralConnection(LdapUrl referralUrl,
+ LdapConnectionConfig ldapConfig, Integer hop)
--- End diff --
Fixed.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241928473
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/conf/LdapDnGuacamoleProperty.java ---
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.ldap.conf;
+
+import org.apache.directory.api.ldap.model.exception.LdapInvalidDnException;
+import org.apache.directory.api.ldap.model.name.Dn;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.properties.GuacamoleProperty;
+
+/**
+ * A GuacamoleProperty that converts a string to a Dn that can be used
+ * in LDAP connections. An exception is thrown if the provided DN is invalid
+ * and cannot be parsed.
+ */
+public abstract class LdapDnGuacamoleProperty implements GuacamoleProperty<Dn> {
+
+ @Override
+ public Dn parseValue(String value) throws GuacamoleException {
+
+ if (value == null)
+ return null;
+
+ try {
+ return new Dn(value);
+ }
+ catch (LdapInvalidDnException e) {
+ throw new GuacamoleServerException("Invalid DN specified in configuration.", e);
--- End diff --
Reworded.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241958169
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -240,17 +244,24 @@ public LDAPAuthenticatedUser authenticateUser(Credentials credentials)
try {
+ LdapConnectionConfig ldapConnectionConfig =
+ ((LdapNetworkConnection) ldapConnection).getConfig();
+ Dn authDn = new Dn(ldapConnectionConfig.getName());
--- End diff --
This does seem to work (now that I cleared up the other things that were wrong), but let me know if it's a reasonable approach or if I should try something else.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241907562
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ObjectQueryService.java ---
@@ -188,46 +183,50 @@ public String generateQuery(String filter,
* information required to execute the query cannot be read from
* guacamole.properties.
*/
- public List<LDAPEntry> search(LDAPConnection ldapConnection,
- String baseDN, String query) throws GuacamoleException {
+ public List<Entry> search(LdapConnection ldapConnection,
+ Dn baseDN, ExprNode query) throws GuacamoleException {
logger.debug("Searching \"{}\" for objects matching \"{}\".", baseDN, query);
try {
+ LdapConnectionConfig ldapConnectionConfig =
+ ((LdapNetworkConnection) ldapConnection).getConfig();
+
// Search within subtree of given base DN
- LDAPSearchResults results = ldapConnection.search(baseDN,
- LDAPConnection.SCOPE_SUB, query, null, false,
- confService.getLDAPSearchConstraints());
+ SearchRequest request = ldapService.getSearchRequest(baseDN,
+ query);
+
+ SearchCursor results = ldapConnection.search(request);
// Produce list of all entries in the search result, automatically
// following referrals if configured to do so
- List<LDAPEntry> entries = new ArrayList<>(results.getCount());
- while (results.hasMore()) {
+ List<Entry> entries = new ArrayList<>();
+ while (results.next()) {
- try {
- entries.add(results.next());
+ Response response = results.get();
+ if (response instanceof SearchResultEntry) {
--- End diff --
Any way to avoid all the `instanceof` and typecasts? I see functions like `isReferral()` and `getReferral()` on `SearchCursor`.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241928458
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/conf/LdapFilterGuacamoleProperty.java ---
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.ldap.conf;
+
+import java.text.ParseException;
+import org.apache.directory.api.ldap.model.filter.ExprNode;
+import org.apache.directory.api.ldap.model.filter.FilterParser;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.properties.GuacamoleProperty;
+
+/**
+ * A GuacamoleProperty with a value of an ExprNode query filter. The string
+ * provided is passed through the FilterParser returning the ExprNode object,
+ * or an exception is thrown if the filter is invalid and cannot be correctly
+ * parsed.
+ */
+public abstract class LdapFilterGuacamoleProperty implements GuacamoleProperty<ExprNode> {
+
+ @Override
+ public ExprNode parseValue(String value) throws GuacamoleException {
+
+ // No value provided, so return null.
+ if (value == null)
+ return null;
+
+ try {
+ return FilterParser.parse(value);
+ }
+ catch (ParseException e) {
+ throw new GuacamoleServerException("Error parsing filter", e);
--- End diff --
Reworded.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241916700
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/conf/LdapFilterGuacamoleProperty.java ---
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.ldap.conf;
+
+import java.text.ParseException;
+import org.apache.directory.api.ldap.model.filter.ExprNode;
+import org.apache.directory.api.ldap.model.filter.FilterParser;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.properties.GuacamoleProperty;
+
+/**
+ * A GuacamoleProperty with a value of an ExprNode query filter. The string
+ * provided is passed through the FilterParser returning the ExprNode object,
+ * or an exception is thrown if the filter is invalid and cannot be correctly
+ * parsed.
+ */
+public abstract class LdapFilterGuacamoleProperty implements GuacamoleProperty<ExprNode> {
+
+ @Override
+ public ExprNode parseValue(String value) throws GuacamoleException {
+
+ // No value provided, so return null.
+ if (value == null)
+ return null;
+
+ try {
+ return FilterParser.parse(value);
+ }
+ catch (ParseException e) {
+ throw new GuacamoleServerException("Error parsing filter", e);
--- End diff --
Will reword.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241919019
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -240,17 +244,24 @@ public LDAPAuthenticatedUser authenticateUser(Credentials credentials)
try {
+ LdapConnectionConfig ldapConnectionConfig =
+ ((LdapNetworkConnection) ldapConnection).getConfig();
--- End diff --
Unfortunately a different LdapConnection implementation does get used - LdapReferralConnection. I might be able to clean up some of it, but I think some of the typecasting will have to stay. We'll see.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241932591
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -240,17 +244,24 @@ public LDAPAuthenticatedUser authenticateUser(Credentials credentials)
try {
+ LdapConnectionConfig ldapConnectionConfig =
+ ((LdapNetworkConnection) ldapConnection).getConfig();
+ Dn authDn = new Dn(ldapConnectionConfig.getName());
--- End diff --
So I took one stab at a way to do this, storing a `Dn` object called `bindDn` in the `LDAPAuthenticatedUser` class. I'm not sure it's the right way to go (and haven't tested it to make sure it works), but let me know if that seems reasonable or if I should try another route.
I also thought about creating a new `LDAPCredentials` class that extends `Credentials` and adding the bindDn into that, but that would require importing other things (JAX-RS) into the guacamole-auth-ldap module, and that seemed like a lot of extra stuff just to find a place to store the bind DN.
I'm open to other suggestions.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241916742
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/conf/LdapDnGuacamoleProperty.java ---
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.ldap.conf;
+
+import org.apache.directory.api.ldap.model.exception.LdapInvalidDnException;
+import org.apache.directory.api.ldap.model.name.Dn;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.properties.GuacamoleProperty;
+
+/**
+ * A GuacamoleProperty that converts a string to a Dn that can be used
+ * in LDAP connections. An exception is thrown if the provided DN is invalid
+ * and cannot be parsed.
+ */
+public abstract class LdapDnGuacamoleProperty implements GuacamoleProperty<Dn> {
+
+ @Override
+ public Dn parseValue(String value) throws GuacamoleException {
+
+ if (value == null)
+ return null;
+
+ try {
+ return new Dn(value);
+ }
+ catch (LdapInvalidDnException e) {
+ throw new GuacamoleServerException("Invalid DN specified in configuration.", e);
--- End diff --
Sounds good, I'll rework this error a bit.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241902907
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -240,17 +244,24 @@ public LDAPAuthenticatedUser authenticateUser(Credentials credentials)
try {
+ LdapConnectionConfig ldapConnectionConfig =
+ ((LdapNetworkConnection) ldapConnection).getConfig();
--- End diff --
Is this typecast necessary? If `bindAs()` will always return an `LdapNetworkConnection` and we need to use that object as an `LdapNetworkConnection`, then `bindAs()` should be declared as such.
... This may not be relevant depending on the answer to the comment regarding `new Dn(ldapConnectionConfig.getName())`, though.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241928405
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -240,17 +244,24 @@ public LDAPAuthenticatedUser authenticateUser(Credentials credentials)
try {
+ LdapConnectionConfig ldapConnectionConfig =
+ ((LdapNetworkConnection) ldapConnection).getConfig();
--- End diff --
Nevermind...I was able to clean this stuff up.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241916665
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ObjectQueryService.java ---
@@ -188,46 +183,50 @@ public String generateQuery(String filter,
* information required to execute the query cannot be read from
* guacamole.properties.
*/
- public List<LDAPEntry> search(LDAPConnection ldapConnection,
- String baseDN, String query) throws GuacamoleException {
+ public List<Entry> search(LdapConnection ldapConnection,
+ Dn baseDN, ExprNode query) throws GuacamoleException {
logger.debug("Searching \"{}\" for objects matching \"{}\".", baseDN, query);
try {
+ LdapConnectionConfig ldapConnectionConfig =
+ ((LdapNetworkConnection) ldapConnection).getConfig();
+
// Search within subtree of given base DN
- LDAPSearchResults results = ldapConnection.search(baseDN,
- LDAPConnection.SCOPE_SUB, query, null, false,
- confService.getLDAPSearchConstraints());
+ SearchRequest request = ldapService.getSearchRequest(baseDN,
+ query);
+
+ SearchCursor results = ldapConnection.search(request);
// Produce list of all entries in the search result, automatically
// following referrals if configured to do so
- List<LDAPEntry> entries = new ArrayList<>(results.getCount());
- while (results.hasMore()) {
+ List<Entry> entries = new ArrayList<>();
+ while (results.next()) {
- try {
- entries.add(results.next());
+ Response response = results.get();
+ if (response instanceof SearchResultEntry) {
+ entries.add(((SearchResultEntry) response).getEntry());
}
-
- // Warn if referrals cannot be followed
- catch (LDAPReferralException e) {
- if (confService.getFollowReferrals()) {
- logger.error("Could not follow referral: {}", e.getFailedReferral());
- logger.debug("Error encountered trying to follow referral.", e);
- throw new GuacamoleServerException("Could not follow LDAP referral.", e);
- }
- else {
- logger.warn("Given a referral, but referrals are disabled. Error was: {}", e.getMessage());
- logger.debug("Got a referral, but configured to not follow them.", e);
+ else if (response instanceof SearchResultReference &&
+ request.isFollowReferrals()) {
+
+ Referral referral = ((SearchResultReference) response).getReferral();
+ int referralHop = 0;
+ for (String url : referral.getLdapUrls()) {
+ LdapConnection referralConnection = ldapService.referralConnection(
+ new LdapUrl(url), ldapConnectionConfig, referralHop++);
+ entries.addAll(search(referralConnection, baseDN, query));
--- End diff --
Yeah, you're probably right. The entire recursion of this function was kind of a wild stab, so I'm not surprised I made a few mistakes along the way.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241916353
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -240,17 +244,24 @@ public LDAPAuthenticatedUser authenticateUser(Credentials credentials)
try {
+ LdapConnectionConfig ldapConnectionConfig =
+ ((LdapNetworkConnection) ldapConnection).getConfig();
--- End diff --
I'll see what makes the most sense - maybe just converting everything to LdapNetworkConnection is the most straight-forward, since that's really the only implementation that ever gets used.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241908453
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ObjectQueryService.java ---
@@ -188,46 +183,50 @@ public String generateQuery(String filter,
* information required to execute the query cannot be read from
* guacamole.properties.
*/
- public List<LDAPEntry> search(LDAPConnection ldapConnection,
- String baseDN, String query) throws GuacamoleException {
+ public List<Entry> search(LdapConnection ldapConnection,
+ Dn baseDN, ExprNode query) throws GuacamoleException {
logger.debug("Searching \"{}\" for objects matching \"{}\".", baseDN, query);
try {
+ LdapConnectionConfig ldapConnectionConfig =
+ ((LdapNetworkConnection) ldapConnection).getConfig();
+
// Search within subtree of given base DN
- LDAPSearchResults results = ldapConnection.search(baseDN,
- LDAPConnection.SCOPE_SUB, query, null, false,
- confService.getLDAPSearchConstraints());
+ SearchRequest request = ldapService.getSearchRequest(baseDN,
+ query);
+
+ SearchCursor results = ldapConnection.search(request);
// Produce list of all entries in the search result, automatically
// following referrals if configured to do so
- List<LDAPEntry> entries = new ArrayList<>(results.getCount());
- while (results.hasMore()) {
+ List<Entry> entries = new ArrayList<>();
+ while (results.next()) {
- try {
- entries.add(results.next());
+ Response response = results.get();
+ if (response instanceof SearchResultEntry) {
+ entries.add(((SearchResultEntry) response).getEntry());
}
-
- // Warn if referrals cannot be followed
- catch (LDAPReferralException e) {
- if (confService.getFollowReferrals()) {
- logger.error("Could not follow referral: {}", e.getFailedReferral());
- logger.debug("Error encountered trying to follow referral.", e);
- throw new GuacamoleServerException("Could not follow LDAP referral.", e);
- }
- else {
- logger.warn("Given a referral, but referrals are disabled. Error was: {}", e.getMessage());
- logger.debug("Got a referral, but configured to not follow them.", e);
+ else if (response instanceof SearchResultReference &&
+ request.isFollowReferrals()) {
+
+ Referral referral = ((SearchResultReference) response).getReferral();
+ int referralHop = 0;
+ for (String url : referral.getLdapUrls()) {
+ LdapConnection referralConnection = ldapService.referralConnection(
+ new LdapUrl(url), ldapConnectionConfig, referralHop++);
+ entries.addAll(search(referralConnection, baseDN, query));
--- End diff --
I'm not sure this is correct. Each call to `search()` will effectively reset that `referralHop` counter. Doesn't this need to be tracked recursively such that it limits the depth of the referral tree?
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241905520
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -240,17 +244,24 @@ public LDAPAuthenticatedUser authenticateUser(Credentials credentials)
try {
+ LdapConnectionConfig ldapConnectionConfig =
+ ((LdapNetworkConnection) ldapConnection).getConfig();
+ Dn authDn = new Dn(ldapConnectionConfig.getName());
--- End diff --
Is there a better way of retrieving the `Dn` than reparsing it from a string? Perhaps it could be stored somewhere by the relevant `bindAs()` implementation? Or perhaps we should be using our own object that provides access to both the `LdapConnection` and the bind `Dn`?
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241910894
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/conf/LdapFilterGuacamoleProperty.java ---
@@ -0,0 +1,53 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.ldap.conf;
+
+import java.text.ParseException;
+import org.apache.directory.api.ldap.model.filter.ExprNode;
+import org.apache.directory.api.ldap.model.filter.FilterParser;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.properties.GuacamoleProperty;
+
+/**
+ * A GuacamoleProperty with a value of an ExprNode query filter. The string
+ * provided is passed through the FilterParser returning the ExprNode object,
+ * or an exception is thrown if the filter is invalid and cannot be correctly
+ * parsed.
+ */
+public abstract class LdapFilterGuacamoleProperty implements GuacamoleProperty<ExprNode> {
+
+ @Override
+ public ExprNode parseValue(String value) throws GuacamoleException {
+
+ // No value provided, so return null.
+ if (value == null)
+ return null;
+
+ try {
+ return FilterParser.parse(value);
+ }
+ catch (ParseException e) {
+ throw new GuacamoleServerException("Error parsing filter", e);
--- End diff --
I think this error should be clearer. "Error parsing filter" is a bit vague, as the important fact for the user/admin is that filter X is invalid. In this case, that fact (filter is invalid) is hidden here behind what is an internal detail (an error occurred parsing it). If it's possible to tell the user why something is failing without requiring them to deduce the reason for the failure, we should do so.
There's a big difference in pain level when troubleshooting `Error parsing filter` vs. something like `"(dn=invalid))" is not a valid LDAP filter`.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241916587
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ObjectQueryService.java ---
@@ -188,46 +183,50 @@ public String generateQuery(String filter,
* information required to execute the query cannot be read from
* guacamole.properties.
*/
- public List<LDAPEntry> search(LDAPConnection ldapConnection,
- String baseDN, String query) throws GuacamoleException {
+ public List<Entry> search(LdapConnection ldapConnection,
+ Dn baseDN, ExprNode query) throws GuacamoleException {
logger.debug("Searching \"{}\" for objects matching \"{}\".", baseDN, query);
try {
+ LdapConnectionConfig ldapConnectionConfig =
+ ((LdapNetworkConnection) ldapConnection).getConfig();
+
// Search within subtree of given base DN
- LDAPSearchResults results = ldapConnection.search(baseDN,
- LDAPConnection.SCOPE_SUB, query, null, false,
- confService.getLDAPSearchConstraints());
+ SearchRequest request = ldapService.getSearchRequest(baseDN,
+ query);
+
+ SearchCursor results = ldapConnection.search(request);
// Produce list of all entries in the search result, automatically
// following referrals if configured to do so
- List<LDAPEntry> entries = new ArrayList<>(results.getCount());
- while (results.hasMore()) {
+ List<Entry> entries = new ArrayList<>();
+ while (results.next()) {
- try {
- entries.add(results.next());
+ Response response = results.get();
+ if (response instanceof SearchResultEntry) {
--- End diff --
I'll take a closer look.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241916521
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java ---
@@ -156,38 +146,84 @@ public LDAPConnection bindAs(String userDN, String password)
// Bind using provided credentials
try {
- byte[] passwordBytes;
- try {
-
- // Convert password into corresponding byte array
- if (password != null)
- passwordBytes = password.getBytes("UTF-8");
- else
- passwordBytes = null;
-
- }
- catch (UnsupportedEncodingException e) {
- logger.error("Unexpected lack of support for UTF-8: {}", e.getMessage());
- logger.debug("Support for UTF-8 (as required by Java spec) not found.", e);
- disconnect(ldapConnection);
- return null;
- }
-
- // Bind as user
- ldapConnection.bind(LDAPConnection.LDAP_V3, userDN, passwordBytes);
+ BindRequest bindRequest = new BindRequestImpl();
+ bindRequest.setDn(userDN);
+ bindRequest.setCredentials(password);
+ ldapConnection.bind(bindRequest);
}
// Disconnect if an error occurs during bind
- catch (LDAPException e) {
- logger.debug("LDAP bind failed.", e);
+ catch (LdapException e) {
+ logger.debug("Unable to bind to LDAP server.", e);
disconnect(ldapConnection);
return null;
}
return ldapConnection;
}
+
+ /**
+ * Generate a new LdapConnection object for following a referral
+ * with the given LdapUrl, and copy the username and password
+ * from the original connection.
+ *
+ * @param referralUrl
+ * The LDAP URL to follow.
+ *
+ * @param ldapConfig
+ * The connection configuration to use to retrieve username and
+ * password.
+ *
+ * @param hop
+ * The current hop number of this referral - once the configured
+ * limit is reached, this method will throw an exception.
+ *
+ * @return
+ * A LdapConnection object that points at the location
+ * specified in the referralUrl.
+ *
+ * @throws GuacamoleException
+ * If an error occurs parsing out the LdapUrl object or the
+ * maximum number of referral hops is reached.
+ */
+ public LdapConnection referralConnection(LdapUrl referralUrl,
+ LdapConnectionConfig ldapConfig, Integer hop)
--- End diff --
Bad habits?! :smile:
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241911374
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/conf/LdapDnGuacamoleProperty.java ---
@@ -0,0 +1,50 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.auth.ldap.conf;
+
+import org.apache.directory.api.ldap.model.exception.LdapInvalidDnException;
+import org.apache.directory.api.ldap.model.name.Dn;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleServerException;
+import org.apache.guacamole.properties.GuacamoleProperty;
+
+/**
+ * A GuacamoleProperty that converts a string to a Dn that can be used
+ * in LDAP connections. An exception is thrown if the provided DN is invalid
+ * and cannot be parsed.
+ */
+public abstract class LdapDnGuacamoleProperty implements GuacamoleProperty<Dn> {
+
+ @Override
+ public Dn parseValue(String value) throws GuacamoleException {
+
+ if (value == null)
+ return null;
+
+ try {
+ return new Dn(value);
+ }
+ catch (LdapInvalidDnException e) {
+ throw new GuacamoleServerException("Invalid DN specified in configuration.", e);
--- End diff --
To avoid confusion, probably better to include the DN in question. Maybe `Specified DN "thednhere" is not valid` or `"thednhere" is not a valid DN.`, etc.?
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241933797
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ObjectQueryService.java ---
@@ -188,46 +183,50 @@ public String generateQuery(String filter,
* information required to execute the query cannot be read from
* guacamole.properties.
*/
- public List<LDAPEntry> search(LDAPConnection ldapConnection,
- String baseDN, String query) throws GuacamoleException {
+ public List<Entry> search(LdapConnection ldapConnection,
+ Dn baseDN, ExprNode query) throws GuacamoleException {
logger.debug("Searching \"{}\" for objects matching \"{}\".", baseDN, query);
try {
+ LdapConnectionConfig ldapConnectionConfig =
+ ((LdapNetworkConnection) ldapConnection).getConfig();
+
// Search within subtree of given base DN
- LDAPSearchResults results = ldapConnection.search(baseDN,
- LDAPConnection.SCOPE_SUB, query, null, false,
- confService.getLDAPSearchConstraints());
+ SearchRequest request = ldapService.getSearchRequest(baseDN,
+ query);
+
+ SearchCursor results = ldapConnection.search(request);
// Produce list of all entries in the search result, automatically
// following referrals if configured to do so
- List<LDAPEntry> entries = new ArrayList<>(results.getCount());
- while (results.hasMore()) {
+ List<Entry> entries = new ArrayList<>();
+ while (results.next()) {
- try {
- entries.add(results.next());
+ Response response = results.get();
+ if (response instanceof SearchResultEntry) {
+ entries.add(((SearchResultEntry) response).getEntry());
}
-
- // Warn if referrals cannot be followed
- catch (LDAPReferralException e) {
- if (confService.getFollowReferrals()) {
- logger.error("Could not follow referral: {}", e.getFailedReferral());
- logger.debug("Error encountered trying to follow referral.", e);
- throw new GuacamoleServerException("Could not follow LDAP referral.", e);
- }
- else {
- logger.warn("Given a referral, but referrals are disabled. Error was: {}", e.getMessage());
- logger.debug("Got a referral, but configured to not follow them.", e);
+ else if (response instanceof SearchResultReference &&
+ request.isFollowReferrals()) {
+
+ Referral referral = ((SearchResultReference) response).getReferral();
+ int referralHop = 0;
+ for (String url : referral.getLdapUrls()) {
+ LdapConnection referralConnection = ldapService.referralConnection(
+ new LdapUrl(url), ldapConnectionConfig, referralHop++);
+ entries.addAll(search(referralConnection, baseDN, query));
--- End diff --
Took a stab at this, as well - not entirely sure this is the best/cleanest way, but let me know what you think. As usual, I'm open to suggestions for how to approach it.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241906662
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java ---
@@ -156,38 +146,84 @@ public LDAPConnection bindAs(String userDN, String password)
// Bind using provided credentials
try {
- byte[] passwordBytes;
- try {
-
- // Convert password into corresponding byte array
- if (password != null)
- passwordBytes = password.getBytes("UTF-8");
- else
- passwordBytes = null;
-
- }
- catch (UnsupportedEncodingException e) {
- logger.error("Unexpected lack of support for UTF-8: {}", e.getMessage());
- logger.debug("Support for UTF-8 (as required by Java spec) not found.", e);
- disconnect(ldapConnection);
- return null;
- }
-
- // Bind as user
- ldapConnection.bind(LDAPConnection.LDAP_V3, userDN, passwordBytes);
+ BindRequest bindRequest = new BindRequestImpl();
+ bindRequest.setDn(userDN);
+ bindRequest.setCredentials(password);
+ ldapConnection.bind(bindRequest);
}
// Disconnect if an error occurs during bind
- catch (LDAPException e) {
- logger.debug("LDAP bind failed.", e);
+ catch (LdapException e) {
+ logger.debug("Unable to bind to LDAP server.", e);
disconnect(ldapConnection);
return null;
}
return ldapConnection;
}
+
+ /**
+ * Generate a new LdapConnection object for following a referral
+ * with the given LdapUrl, and copy the username and password
+ * from the original connection.
+ *
+ * @param referralUrl
+ * The LDAP URL to follow.
+ *
+ * @param ldapConfig
+ * The connection configuration to use to retrieve username and
+ * password.
+ *
+ * @param hop
+ * The current hop number of this referral - once the configured
+ * limit is reached, this method will throw an exception.
+ *
+ * @return
+ * A LdapConnection object that points at the location
+ * specified in the referralUrl.
+ *
+ * @throws GuacamoleException
+ * If an error occurs parsing out the LdapUrl object or the
+ * maximum number of referral hops is reached.
+ */
+ public LdapConnection referralConnection(LdapUrl referralUrl,
+ LdapConnectionConfig ldapConfig, Integer hop)
--- End diff --
Why `Integer` and not `int`?
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/345#discussion_r241928440
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ObjectQueryService.java ---
@@ -188,46 +183,50 @@ public String generateQuery(String filter,
* information required to execute the query cannot be read from
* guacamole.properties.
*/
- public List<LDAPEntry> search(LDAPConnection ldapConnection,
- String baseDN, String query) throws GuacamoleException {
+ public List<Entry> search(LdapConnection ldapConnection,
+ Dn baseDN, ExprNode query) throws GuacamoleException {
logger.debug("Searching \"{}\" for objects matching \"{}\".", baseDN, query);
try {
+ LdapConnectionConfig ldapConnectionConfig =
+ ((LdapNetworkConnection) ldapConnection).getConfig();
+
// Search within subtree of given base DN
- LDAPSearchResults results = ldapConnection.search(baseDN,
- LDAPConnection.SCOPE_SUB, query, null, false,
- confService.getLDAPSearchConstraints());
+ SearchRequest request = ldapService.getSearchRequest(baseDN,
+ query);
+
+ SearchCursor results = ldapConnection.search(request);
// Produce list of all entries in the search result, automatically
// following referrals if configured to do so
- List<LDAPEntry> entries = new ArrayList<>(results.getCount());
- while (results.hasMore()) {
+ List<Entry> entries = new ArrayList<>();
+ while (results.next()) {
- try {
- entries.add(results.next());
+ Response response = results.get();
+ if (response instanceof SearchResultEntry) {
--- End diff --
Reworked to avoid this using the calls you mentioned.
---