[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
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...
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 search(LDAPConnection ldapConnection,
-String baseDN, String query) throws GuacamoleException {
+public List 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 entries = new ArrayList<>(results.getCount());
-while (results.hasMore()) {
+List 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...
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 {
+
+@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...
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 {
+
+@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...
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...
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 search(LDAPConnection ldapConnection,
-String baseDN, String query) throws GuacamoleException {
+public List 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 entries = new ArrayList<>(results.getCount());
-while (results.hasMore()) {
+List 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.
---
[GitHub] guacamole-client pull request #345: GUACAMOLE-234: Migrate to Apache Directo...
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...
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 {
+
+@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...
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 search(LDAPConnection ldapConnection,
-String baseDN, String query) throws GuacamoleException {
+public List 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 entries = new ArrayList<>(results.getCount());
-while (results.hasMore()) {
+List 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...
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 search(LDAPConnection ldapConnection,
-String baseDN, String query) throws GuacamoleException {
+public List 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 entries = new ArrayList<>(results.getCount());
-while (results.hasMore()) {
+List 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...
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...
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 search(LDAPConnection ldapConnection,
-String baseDN, String query) throws GuacamoleException {
+public List 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 entries = new ArrayList<>(results.getCount());
-while (results.hasMore()) {
+List 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...
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 Date: 2018-12-09T13:32:16Z GUACAMOLE-234: Convert LDAP extension to use Apache Directory LDAP API. commit 37022bb552e60631033f3630bc0a0545f9c6f1d7 Author: Nick Couchman Date: 2018-12-09T14:48:01Z GUACAMOLE-234: Clean up some LDAP implementation details. commit 2abd46b0e130548fe1219de7d6369b724c80fa55 Author: Nick Couchman Date: 2018-12-09T15:42:12Z GUACAMOLE-234: Clean up comments. commit 197f5fe7997cc1f8a636e9c114e2dd967b8ae4a1 Author: Nick Couchman Date: 2018-12-09T15:55:39Z GUACAMOLE-234: Exclude slf4j from Apache Directory dependency. ---
