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/10/23 13:50:53 UTC
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
GitHub user necouchman opened a pull request:
https://github.com/apache/incubator-guacamole-client/pull/202
GUACAMOLE-243: Allow LDAP referral following to be configured
This PR fixes the JIRA issue 243, where errors in following referrals block LDAP authentication from succeeding, by allowing referral support to be configured.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/necouchman/incubator-guacamole-client GUACAMOLE-243-WORKING
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-guacamole-client/pull/202.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 #202
----
commit d98cdd29178b5623032b7d340658221549114361
Author: Nick Couchman <ni...@yahoo.com>
Date: 2017-03-17T20:16:04Z
GUACAMOLE-243: Implement LDAP referral handling in Guacamole LDAP extension.
commit 242cfbaf852b4380395103b8ca8e574cf30c323f
Author: Nick Couchman <ni...@yahoo.com>
Date: 2017-03-18T15:08:56Z
GUACAMOLE-243: Finish up changes to deal with LDAP referrals, both in UserServer and ConnectionServer classes, along with global changes in LDAPConnectionService class.
commit 9c99905a1dd01e7ab6b8b49f23c5623e71df5123
Author: Nick Couchman <vn...@apache.org>
Date: 2017-10-23T13:48:20Z
GUACAMOLE-243: Change warning message to include failed referral.
----
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149184453
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -267,8 +285,23 @@ private String generateLDAPQuery(String username)
// Add all DNs for found users
while (results.hasMore()) {
- LDAPEntry entry = results.next();
- userDNs.add(entry.getDN());
+ try {
+ LDAPEntry entry = results.next();
+ userDNs.add(entry.getDN());
+ }
+
+ // Deal with errors following referrals
+ catch (LDAPReferralException e) {
+ if (confService.getFollowReferrals()) {
+ logger.error("Error trying to follow a referral.", e.getMessage());
--- End diff --
Missing `{}` in log message.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149181098
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java ---
@@ -0,0 +1,80 @@
+/*
+ * 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;
+
+import com.google.inject.Inject;
+import com.novell.ldap.LDAPAuthHandler;
+import com.novell.ldap.LDAPAuthProvider;
+import com.novell.ldap.LDAPConnection;
+import java.io.UnsupportedEncodingException;
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class that implements the necessary authentication handling
+ * for following referrals in LDAP connections.
+ */
+public class ReferralAuthHandler implements LDAPAuthHandler {
+
+ /**
+ * Logger for this class.
+ */
+ private final Logger logger = LoggerFactory.getLogger(ReferralAuthHandler.class);
+
+ /**
+ * The LDAPAuthProvider object that will be set and returned to the referral handler.
+ */
+ private final LDAPAuthProvider ldapAuth;
+
+ /**
+ * Creates a ReferralAuthHandler object to handle authentication when
+ * following referrals in a LDAP connection, using the provided dn and
+ * password.
+ *
+ * @throws GuacamoleException
+ * If exceptions are caught while converting the password from a string
+ * into a byte array.
+ */
+ public ReferralAuthHandler(String dn, String password) throws GuacamoleException {
+ 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);
+ throw new GuacamoleException("Could not set password due to missing UTF-8 support.");
--- End diff --
In general:
1. Rather than throw the base `GuacamoleException`, we should be throwing an appropriate subclass. `GuacamoleServerException` would be a reasonable choice in this case.
2. For any exception caused by another, [the constructor which includes the causing exception](http://guacamole.incubator.apache.org/doc/guacamole-common/org/apache/guacamole/GuacamoleServerException.html#GuacamoleServerException-java.lang.String-java.lang.Throwable-) should be used (rather than `new GuacamoleServerException("message")`, use `new GuacamoleServerException("message", e)`).
That said, since UTF-8 support is guaranteed by Java, there's no need for checked exceptions here. Something like `UnsupportedOperationException` might be more appropriate. For example:
https://github.com/apache/incubator-guacamole-client/blob/0611fe8fff694dff7e3cb6a62918f6c90668728c/extensions/guacamole-auth-cas/src/main/java/org/apache/guacamole/auth/cas/form/CASTicketField.java#L80-L83
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149186459
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPGuacamoleProperties.java ---
@@ -174,4 +175,34 @@ private LDAPGuacamoleProperties() {}
};
+ /**
+ * Whether or not we should follow referrals
+ */
+ public static final BooleanGuacamoleProperty LDAP_FOLLOW_REFERRALS = new BooleanGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "ldap-follow-referrals"; }
+
+ };
+
+ /**
+ * Maximum number of referral hops to follow
+ */
+ public static final IntegerGuacamoleProperty LDAP_MAX_REFERRAL_HOPS = new IntegerGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "ldap-max-referral-hops"; }
+
+ };
+
+ /**
+ * Number of seconds to wait for LDAP operations to complete
--- End diff --
JavaDoc formatting: missing period.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149186316
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -292,4 +327,21 @@ public String getUserSearchFilter() throws GuacamoleException {
);
}
+ /**
+ * Returns the maximum number of seconds to wait for LDAP operations
--- End diff --
JavaDoc formatting: missing period.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146660309
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -251,8 +270,23 @@ private String getConnectionSearchFilter(String userDN,
// The guacConfig group uses the seeAlso attribute to refer
// to these other groups
while (userRoleGroupResults.hasMore()) {
- LDAPEntry entry = userRoleGroupResults.next();
- connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ try {
+ LDAPEntry entry = userRoleGroupResults.next();
+ connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ }
+
+ catch (LDAPReferralException e) {
+ if (confService.getFollowReferrals()) {
+ logger.error("Could not follow referral.", e.getMessage());
+ logger.debug("Error encountered trying to follow referral.", e);
+ throw new GuacamoleServerException("Could not follow LDAP referral.", e);
--- End diff --
My thinking is that if you explicitly enable referral following, an error following the referrals should cause an abort, and if you disable it, you can log it and ignore it. I'm not opposed to a non-fatal logging, here, though, if that makes more sense. I don't know off the top of my head what the generally accepted behavior is for other implementations.
In the case of the issue with Active Directory, disabling referral following completely allows you to query it via LDAP correctly - having it disabled doesn't cut off expected results or anything like that.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149263723
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -129,62 +130,79 @@
Map<String, Connection> connections = new HashMap<String, Connection>();
while (results.hasMore()) {
- LDAPEntry entry = results.next();
+ try {
- // Get common name (CN)
- LDAPAttribute cn = entry.getAttribute("cn");
- if (cn == null) {
- logger.warn("guacConfigGroup is missing a cn.");
- continue;
- }
+ LDAPEntry entry = results.next();
- // Get associated protocol
- LDAPAttribute protocol = entry.getAttribute("guacConfigProtocol");
- if (protocol == null) {
- logger.warn("guacConfigGroup \"{}\" is missing the "
- + "required \"guacConfigProtocol\" attribute.",
- cn.getStringValue());
- continue;
- }
+ // Get common name (CN)
+ LDAPAttribute cn = entry.getAttribute("cn");
+ if (cn == null) {
+ logger.warn("guacConfigGroup is missing a cn.");
+ continue;
+ }
+
+ // Get associated protocol
+ LDAPAttribute protocol = entry.getAttribute("guacConfigProtocol");
+ if (protocol == null) {
+ logger.warn("guacConfigGroup \"{}\" is missing the "
+ + "required \"guacConfigProtocol\" attribute.",
+ cn.getStringValue());
+ continue;
+ }
- // Set protocol
- GuacamoleConfiguration config = new GuacamoleConfiguration();
- config.setProtocol(protocol.getStringValue());
+ // Set protocol
+ GuacamoleConfiguration config = new GuacamoleConfiguration();
+ config.setProtocol(protocol.getStringValue());
- // Get parameters, if any
- LDAPAttribute parameterAttribute = entry.getAttribute("guacConfigParameter");
- if (parameterAttribute != null) {
+ // Get parameters, if any
+ LDAPAttribute parameterAttribute = entry.getAttribute("guacConfigParameter");
+ if (parameterAttribute != null) {
- // For each parameter
- Enumeration<?> parameters = parameterAttribute.getStringValues();
- while (parameters.hasMoreElements()) {
+ // For each parameter
+ Enumeration<?> parameters = parameterAttribute.getStringValues();
+ while (parameters.hasMoreElements()) {
- String parameter = (String) parameters.nextElement();
+ String parameter = (String) parameters.nextElement();
- // Parse parameter
- int equals = parameter.indexOf('=');
- if (equals != -1) {
+ // Parse parameter
+ int equals = parameter.indexOf('=');
+ if (equals != -1) {
- // Parse name
- String name = parameter.substring(0, equals);
- String value = parameter.substring(equals+1);
+ // Parse name
+ String name = parameter.substring(0, equals);
+ String value = parameter.substring(equals+1);
- config.setParameter(name, value);
+ config.setParameter(name, value);
+
+ }
}
}
- }
+ // Filter the configuration, substituting all defined tokens
+ tokenFilter.filterValues(config.getParameters());
- // Filter the configuration, substituting all defined tokens
- tokenFilter.filterValues(config.getParameters());
+ // Store connection using cn for both identifier and name
+ String name = cn.getStringValue();
+ Connection connection = new SimpleConnection(name, name, config);
+ connection.setParentIdentifier(LDAPAuthenticationProvider.ROOT_CONNECTION_GROUP);
+ connections.put(name, connection);
- // Store connection using cn for both identifier and name
- String name = cn.getStringValue();
- Connection connection = new SimpleConnection(name, name, config);
- connection.setParentIdentifier(LDAPAuthenticationProvider.ROOT_CONNECTION_GROUP);
- connections.put(name, connection);
+ }
+
+ // Deal with issues following LDAP referrals
+ 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.", e.getMessage());
--- End diff --
Fixed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146657553
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -107,19 +108,37 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
// Read all visible users
while (results.hasMore()) {
- LDAPEntry entry = results.next();
+ try {
+
+ LDAPEntry entry = results.next();
+
+ // Get username from record
+ LDAPAttribute username = entry.getAttribute(usernameAttribute);
+ if (username == null) {
+ logger.warn("Queried user is missing the username attribute \"{}\".", usernameAttribute);
+ continue;
+ }
+
+ // Store user using their username as the identifier
+ String identifier = username.getStringValue();
+ if (users.put(identifier, new SimpleUser(identifier)) != null)
+ logger.warn("Possibly ambiguous user account: \"{}\".", identifier);
- // Get username from record
- LDAPAttribute username = entry.getAttribute(usernameAttribute);
- if (username == null) {
- logger.warn("Queried user is missing the username attribute \"{}\".", usernameAttribute);
- continue;
}
- // Store user using their username as the identifier
- String identifier = username.getStringValue();
- if (users.put(identifier, new SimpleUser(identifier)) != null)
- logger.warn("Possibly ambiguous user account: \"{}\".", identifier);
+ // Deal with errors trying to follow referrals
+ 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.", e.getMessage());
+ logger.debug("Got a referral, but configured to not follow them.", e);
+ continue;
--- End diff --
Being the last statement in the loop, this `continue` has no effect.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146654775
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java ---
@@ -0,0 +1,97 @@
+/*
+ * 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;
+
+import com.google.inject.Inject;
+import com.novell.ldap.LDAPAuthHandler;
+import com.novell.ldap.LDAPAuthProvider;
+import com.novell.ldap.LDAPConnection;
+import java.io.UnsupportedEncodingException;
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReferralAuthHandler implements LDAPAuthHandler {
+
+ /**
+ * Logger for this class.
+ */
+ private final Logger logger = LoggerFactory.getLogger(ReferralAuthHandler.class);
+
+ /**
+ * The LDAPAuthProvider object that will be set and returned to the referral handler.
+ */
+ private final LDAPAuthProvider ldapAuth;
+
+ /**
+ * Service for retrieving LDAP server configuration information.
+ */
+ @Inject
+ private ConfigurationService confService;
+
+
+ public ReferralAuthHandler() throws GuacamoleException {
+ String binddn = confService.getSearchBindDN();
+ String password = confService.getSearchBindPassword();
+ 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);
+ throw new GuacamoleException("Could not set password due to missing support for UTF-8 encoding.");
+ }
+
+ ldapAuth = new LDAPAuthProvider(binddn, passwordBytes);
+
+ }
+
+ public ReferralAuthHandler(String dn, String password) throws GuacamoleException {
--- End diff --
Missing JavaDoc.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149186347
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -292,4 +327,21 @@ public String getUserSearchFilter() throws GuacamoleException {
);
}
+ /**
+ * Returns the maximum number of seconds to wait for LDAP operations
+ *
+ * @return
+ * The maximum number of seconds to wait for LDAP operations
+ * as configured in guacamole.properties
--- End diff --
JavaDoc formatting: missing period.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146658786
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -251,8 +270,23 @@ private String getConnectionSearchFilter(String userDN,
// The guacConfig group uses the seeAlso attribute to refer
// to these other groups
while (userRoleGroupResults.hasMore()) {
- LDAPEntry entry = userRoleGroupResults.next();
- connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ try {
+ LDAPEntry entry = userRoleGroupResults.next();
+ connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ }
+
+ catch (LDAPReferralException e) {
+ if (confService.getFollowReferrals()) {
+ logger.error("Could not follow referral.", e.getMessage());
--- End diff --
Corrected.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149264150
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -252,6 +252,24 @@ private DereferenceAliasesMode getDereferenceAliases() throws GuacamoleException
}
/**
+ * Returns the boolean value for whether the connection should
+ * follow referrals or not. By default, it will not.
+ *
+ * @return
+ * The boolean value of whether to follow referrals
+ * as configured in guacamole.properties
--- End diff --
Fixed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149263677
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -267,8 +285,23 @@ private String generateLDAPQuery(String username)
// Add all DNs for found users
while (results.hasMore()) {
- LDAPEntry entry = results.next();
- userDNs.add(entry.getDN());
+ try {
+ LDAPEntry entry = results.next();
+ userDNs.add(entry.getDN());
+ }
+
+ // Deal with errors following referrals
+ catch (LDAPReferralException e) {
+ if (confService.getFollowReferrals()) {
+ logger.error("Error trying to follow a referral.", e.getMessage());
+ logger.debug("Encountered an error trying to follow a referral.", e);
+ throw new GuacamoleServerException("Failed while trying to follow referrals.", e);
+ }
+ else {
+ logger.warn("Given a referral, not following it.", e.getMessage());
--- End diff --
Fixed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146655739
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java ---
@@ -0,0 +1,97 @@
+/*
+ * 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;
+
+import com.google.inject.Inject;
+import com.novell.ldap.LDAPAuthHandler;
+import com.novell.ldap.LDAPAuthProvider;
+import com.novell.ldap.LDAPConnection;
+import java.io.UnsupportedEncodingException;
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReferralAuthHandler implements LDAPAuthHandler {
+
+ /**
+ * Logger for this class.
+ */
+ private final Logger logger = LoggerFactory.getLogger(ReferralAuthHandler.class);
+
+ /**
+ * The LDAPAuthProvider object that will be set and returned to the referral handler.
+ */
+ private final LDAPAuthProvider ldapAuth;
+
+ /**
+ * Service for retrieving LDAP server configuration information.
+ */
+ @Inject
+ private ConfigurationService confService;
--- End diff --
Beware that, despite the `@Inject` annotation, this will not happen automatically upon construction. Guice will only perform the injection via a `Provider`, or if the various constructors are properly annotated for assisted injection, which is complex.
If the default constructor is the only place this is used, and that constructor is not called anyway, I'd suggest simply getting rid of both.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146658662
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java ---
@@ -0,0 +1,97 @@
+/*
+ * 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;
+
+import com.google.inject.Inject;
+import com.novell.ldap.LDAPAuthHandler;
+import com.novell.ldap.LDAPAuthProvider;
+import com.novell.ldap.LDAPConnection;
+import java.io.UnsupportedEncodingException;
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReferralAuthHandler implements LDAPAuthHandler {
+
+ /**
+ * Logger for this class.
+ */
+ private final Logger logger = LoggerFactory.getLogger(ReferralAuthHandler.class);
+
+ /**
+ * The LDAPAuthProvider object that will be set and returned to the referral handler.
+ */
+ private final LDAPAuthProvider ldapAuth;
+
+ /**
+ * Service for retrieving LDAP server configuration information.
+ */
+ @Inject
+ private ConfigurationService confService;
+
+
+ public ReferralAuthHandler() throws GuacamoleException {
+ String binddn = confService.getSearchBindDN();
+ String password = confService.getSearchBindPassword();
+ 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);
+ throw new GuacamoleException("Could not set password due to missing support for UTF-8 encoding.");
+ }
+
+ ldapAuth = new LDAPAuthProvider(binddn, passwordBytes);
+
+ }
+
+ public ReferralAuthHandler(String dn, String password) throws GuacamoleException {
--- End diff --
Added.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146654730
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java ---
@@ -0,0 +1,97 @@
+/*
+ * 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;
+
+import com.google.inject.Inject;
+import com.novell.ldap.LDAPAuthHandler;
+import com.novell.ldap.LDAPAuthProvider;
+import com.novell.ldap.LDAPConnection;
+import java.io.UnsupportedEncodingException;
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReferralAuthHandler implements LDAPAuthHandler {
--- End diff --
JavaDoc missing for this class.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149264123
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -292,4 +327,21 @@ public String getUserSearchFilter() throws GuacamoleException {
);
}
+ /**
+ * Returns the maximum number of seconds to wait for LDAP operations
+ *
+ * @return
+ * The maximum number of seconds to wait for LDAP operations
+ * as configured in guacamole.properties
--- End diff --
Fixed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149264116
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPGuacamoleProperties.java ---
@@ -174,4 +175,34 @@ private LDAPGuacamoleProperties() {}
};
+ /**
+ * Whether or not we should follow referrals
--- End diff --
Fixed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149183803
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -129,62 +130,79 @@
Map<String, Connection> connections = new HashMap<String, Connection>();
while (results.hasMore()) {
- LDAPEntry entry = results.next();
+ try {
- // Get common name (CN)
- LDAPAttribute cn = entry.getAttribute("cn");
- if (cn == null) {
- logger.warn("guacConfigGroup is missing a cn.");
- continue;
- }
+ LDAPEntry entry = results.next();
- // Get associated protocol
- LDAPAttribute protocol = entry.getAttribute("guacConfigProtocol");
- if (protocol == null) {
- logger.warn("guacConfigGroup \"{}\" is missing the "
- + "required \"guacConfigProtocol\" attribute.",
- cn.getStringValue());
- continue;
- }
+ // Get common name (CN)
+ LDAPAttribute cn = entry.getAttribute("cn");
+ if (cn == null) {
+ logger.warn("guacConfigGroup is missing a cn.");
+ continue;
+ }
+
+ // Get associated protocol
+ LDAPAttribute protocol = entry.getAttribute("guacConfigProtocol");
+ if (protocol == null) {
+ logger.warn("guacConfigGroup \"{}\" is missing the "
+ + "required \"guacConfigProtocol\" attribute.",
+ cn.getStringValue());
+ continue;
+ }
- // Set protocol
- GuacamoleConfiguration config = new GuacamoleConfiguration();
- config.setProtocol(protocol.getStringValue());
+ // Set protocol
+ GuacamoleConfiguration config = new GuacamoleConfiguration();
+ config.setProtocol(protocol.getStringValue());
- // Get parameters, if any
- LDAPAttribute parameterAttribute = entry.getAttribute("guacConfigParameter");
- if (parameterAttribute != null) {
+ // Get parameters, if any
+ LDAPAttribute parameterAttribute = entry.getAttribute("guacConfigParameter");
+ if (parameterAttribute != null) {
- // For each parameter
- Enumeration<?> parameters = parameterAttribute.getStringValues();
- while (parameters.hasMoreElements()) {
+ // For each parameter
+ Enumeration<?> parameters = parameterAttribute.getStringValues();
+ while (parameters.hasMoreElements()) {
- String parameter = (String) parameters.nextElement();
+ String parameter = (String) parameters.nextElement();
- // Parse parameter
- int equals = parameter.indexOf('=');
- if (equals != -1) {
+ // Parse parameter
+ int equals = parameter.indexOf('=');
+ if (equals != -1) {
- // Parse name
- String name = parameter.substring(0, equals);
- String value = parameter.substring(equals+1);
+ // Parse name
+ String name = parameter.substring(0, equals);
+ String value = parameter.substring(equals+1);
- config.setParameter(name, value);
+ config.setParameter(name, value);
+
+ }
}
}
- }
+ // Filter the configuration, substituting all defined tokens
+ tokenFilter.filterValues(config.getParameters());
- // Filter the configuration, substituting all defined tokens
- tokenFilter.filterValues(config.getParameters());
+ // Store connection using cn for both identifier and name
+ String name = cn.getStringValue();
+ Connection connection = new SimpleConnection(name, name, config);
+ connection.setParentIdentifier(LDAPAuthenticationProvider.ROOT_CONNECTION_GROUP);
+ connections.put(name, connection);
- // Store connection using cn for both identifier and name
- String name = cn.getStringValue();
- Connection connection = new SimpleConnection(name, name, config);
- connection.setParentIdentifier(LDAPAuthenticationProvider.ROOT_CONNECTION_GROUP);
- connections.put(name, connection);
+ }
+
+ // Deal with issues following LDAP referrals
+ catch (LDAPReferralException e) {
+ if (confService.getFollowReferrals()) {
+ logger.error("Could not follow referral.", e.getFailedReferral());
--- End diff --
If the intent is to include `e.getFailedReferral()` within the logged message, then the `{}` string must be present in the message at the location the value should be inserted.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146655137
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java ---
@@ -0,0 +1,97 @@
+/*
+ * 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;
+
+import com.google.inject.Inject;
+import com.novell.ldap.LDAPAuthHandler;
+import com.novell.ldap.LDAPAuthProvider;
+import com.novell.ldap.LDAPConnection;
+import java.io.UnsupportedEncodingException;
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReferralAuthHandler implements LDAPAuthHandler {
+
+ /**
+ * Logger for this class.
+ */
+ private final Logger logger = LoggerFactory.getLogger(ReferralAuthHandler.class);
+
+ /**
+ * The LDAPAuthProvider object that will be set and returned to the referral handler.
+ */
+ private final LDAPAuthProvider ldapAuth;
+
+ /**
+ * Service for retrieving LDAP server configuration information.
+ */
+ @Inject
+ private ConfigurationService confService;
+
+
+ public ReferralAuthHandler() throws GuacamoleException {
+ String binddn = confService.getSearchBindDN();
+ String password = confService.getSearchBindPassword();
+ byte[] passwordBytes;
--- End diff --
Rather than copy the entire body of the other constructor, you can invoke it here. You would do:
this(confService.getSearchBindDN(), confService.getSearchBindPassword());
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149186411
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPGuacamoleProperties.java ---
@@ -174,4 +175,34 @@ private LDAPGuacamoleProperties() {}
};
+ /**
+ * Whether or not we should follow referrals
--- End diff --
JavaDoc formatting: missing period.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146653140
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java ---
@@ -111,6 +113,29 @@ public LDAPConnection bindAs(String userDN, String password)
// Obtain appropriately-configured LDAPConnection instance
LDAPConnection ldapConnection = createLDAPConnection();
+ // Configure LDAP connection constraints
+ LDAPConstraints ldapConstraints = ldapConnection.getConstraints();
+ if (ldapConstraints == null)
+ ldapConstraints = new LDAPConstraints();
+
+ // Set whether or not we follow referrals
+ ldapConstraints.setReferralFollowing(confService.getFollowReferrals());
+
+ // If the referral auth method is set to bind, we set it using the existing
+ // username and password.
+ String refAuthMethod = confService.getReferralAuthentication();
+ if (refAuthMethod != null && refAuthMethod.equals("bind"))
+ ldapConstraints.setReferralHandler(new ReferralAuthHandler(userDN, password));
--- End diff --
Ah, OK then.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-guacamole-client/pull/202
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149263698
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -107,19 +108,36 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
// Read all visible users
while (results.hasMore()) {
- LDAPEntry entry = results.next();
+ try {
+
+ LDAPEntry entry = results.next();
+
+ // Get username from record
+ LDAPAttribute username = entry.getAttribute(usernameAttribute);
+ if (username == null) {
+ logger.warn("Queried user is missing the username attribute \"{}\".", usernameAttribute);
+ continue;
+ }
+
+ // Store user using their username as the identifier
+ String identifier = username.getStringValue();
+ if (users.put(identifier, new SimpleUser(identifier)) != null)
+ logger.warn("Possibly ambiguous user account: \"{}\".", identifier);
- // Get username from record
- LDAPAttribute username = entry.getAttribute(usernameAttribute);
- if (username == null) {
- logger.warn("Queried user is missing the username attribute \"{}\".", usernameAttribute);
- continue;
}
- // Store user using their username as the identifier
- String identifier = username.getStringValue();
- if (users.put(identifier, new SimpleUser(identifier)) != null)
- logger.warn("Possibly ambiguous user account: \"{}\".", identifier);
+ // Deal with errors trying to follow referrals
+ 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.", e.getMessage());
--- End diff --
Fixed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r147561005
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -251,8 +270,23 @@ private String getConnectionSearchFilter(String userDN,
// The guacConfig group uses the seeAlso attribute to refer
// to these other groups
while (userRoleGroupResults.hasMore()) {
- LDAPEntry entry = userRoleGroupResults.next();
- connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ try {
+ LDAPEntry entry = userRoleGroupResults.next();
+ connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ }
+
+ catch (LDAPReferralException e) {
+ if (confService.getFollowReferrals()) {
+ logger.error("Could not follow referral.", e.getMessage());
+ logger.debug("Error encountered trying to follow referral.", e);
+ throw new GuacamoleServerException("Could not follow LDAP referral.", e);
--- End diff --
@mike-jumper Any further thoughts on this, or opinions on which way it should go? I'm not sure one way or the other - I do tend toward not wanting to break things if a single operation fails; however, I also think that if you're explicitly enabling it and it fails, you want a very clear indication that something is not working. I see both sides, so I'm good going either way - it's easy enough to change down the road, and will get tweaked, anyway, when we switch LDAP APIs.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149264141
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -273,6 +291,23 @@ public LDAPSearchConstraints getLDAPSearchConstraints() throws GuacamoleExceptio
}
/**
+ * Returns the maximum number of referral hops to follow.
+ *
+ * @return
+ * The maximum number of referral hops to follow
+ * as configured in guacamole.properties
--- End diff --
Fixed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149264135
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -292,4 +327,21 @@ public String getUserSearchFilter() throws GuacamoleException {
);
}
+ /**
+ * Returns the maximum number of seconds to wait for LDAP operations
--- End diff --
Fixed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146416080
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java ---
@@ -111,6 +113,29 @@ public LDAPConnection bindAs(String userDN, String password)
// Obtain appropriately-configured LDAPConnection instance
LDAPConnection ldapConnection = createLDAPConnection();
+ // Configure LDAP connection constraints
+ LDAPConstraints ldapConstraints = ldapConnection.getConstraints();
+ if (ldapConstraints == null)
+ ldapConstraints = new LDAPConstraints();
+
+ // Set whether or not we follow referrals
+ ldapConstraints.setReferralFollowing(confService.getFollowReferrals());
+
+ // If the referral auth method is set to bind, we set it using the existing
+ // username and password.
+ String refAuthMethod = confService.getReferralAuthentication();
+ if (refAuthMethod != null && refAuthMethod.equals("bind"))
+ ldapConstraints.setReferralHandler(new ReferralAuthHandler(userDN, password));
--- End diff --
So, what's the idea behind configuring whether referrals auth with the same credentials?
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149264107
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPGuacamoleProperties.java ---
@@ -174,4 +175,34 @@ private LDAPGuacamoleProperties() {}
};
+ /**
+ * Whether or not we should follow referrals
+ */
+ public static final BooleanGuacamoleProperty LDAP_FOLLOW_REFERRALS = new BooleanGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "ldap-follow-referrals"; }
+
+ };
+
+ /**
+ * Maximum number of referral hops to follow
--- End diff --
Fixed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146657406
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -251,8 +270,23 @@ private String getConnectionSearchFilter(String userDN,
// The guacConfig group uses the seeAlso attribute to refer
// to these other groups
while (userRoleGroupResults.hasMore()) {
- LDAPEntry entry = userRoleGroupResults.next();
- connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ try {
+ LDAPEntry entry = userRoleGroupResults.next();
+ connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ }
+
+ catch (LDAPReferralException e) {
+ if (confService.getFollowReferrals()) {
+ logger.error("Could not follow referral.", e.getMessage());
--- End diff --
If you want this message to be logged, you'll need to include `{}` within the string to define the location for the value to be inserted. For example:
logger.error("Could not follow referral: {}", e.getMessage());
Same thing for the other new logging statements within these changes.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146658733
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java ---
@@ -0,0 +1,97 @@
+/*
+ * 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;
+
+import com.google.inject.Inject;
+import com.novell.ldap.LDAPAuthHandler;
+import com.novell.ldap.LDAPAuthProvider;
+import com.novell.ldap.LDAPConnection;
+import java.io.UnsupportedEncodingException;
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReferralAuthHandler implements LDAPAuthHandler {
+
+ /**
+ * Logger for this class.
+ */
+ private final Logger logger = LoggerFactory.getLogger(ReferralAuthHandler.class);
+
+ /**
+ * The LDAPAuthProvider object that will be set and returned to the referral handler.
+ */
+ private final LDAPAuthProvider ldapAuth;
+
+ /**
+ * Service for retrieving LDAP server configuration information.
+ */
+ @Inject
+ private ConfigurationService confService;
+
+
+ public ReferralAuthHandler() throws GuacamoleException {
--- End diff --
Removed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149186256
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -252,6 +252,24 @@ private DereferenceAliasesMode getDereferenceAliases() throws GuacamoleException
}
/**
+ * Returns the boolean value for whether the connection should
+ * follow referrals or not. By default, it will not.
+ *
+ * @return
+ * The boolean value of whether to follow referrals
+ * as configured in guacamole.properties
--- End diff --
JavaDoc formatting: missing period.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146657508
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -251,8 +270,23 @@ private String getConnectionSearchFilter(String userDN,
// The guacConfig group uses the seeAlso attribute to refer
// to these other groups
while (userRoleGroupResults.hasMore()) {
- LDAPEntry entry = userRoleGroupResults.next();
- connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ try {
+ LDAPEntry entry = userRoleGroupResults.next();
+ connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ }
+
+ catch (LDAPReferralException e) {
+ if (confService.getFollowReferrals()) {
+ logger.error("Could not follow referral.", e.getMessage());
+ 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.", e.getMessage());
+ logger.debug("Got a referral, but configured to not follow them.", e);
+ continue;
--- End diff --
Being the last statement in the loop, this `continue` has no effect.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146655204
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java ---
@@ -0,0 +1,97 @@
+/*
+ * 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;
+
+import com.google.inject.Inject;
+import com.novell.ldap.LDAPAuthHandler;
+import com.novell.ldap.LDAPAuthProvider;
+import com.novell.ldap.LDAPConnection;
+import java.io.UnsupportedEncodingException;
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReferralAuthHandler implements LDAPAuthHandler {
+
+ /**
+ * Logger for this class.
+ */
+ private final Logger logger = LoggerFactory.getLogger(ReferralAuthHandler.class);
+
+ /**
+ * The LDAPAuthProvider object that will be set and returned to the referral handler.
+ */
+ private final LDAPAuthProvider ldapAuth;
+
+ /**
+ * Service for retrieving LDAP server configuration information.
+ */
+ @Inject
+ private ConfigurationService confService;
+
+
+ public ReferralAuthHandler() throws GuacamoleException {
--- End diff --
I don't see this constructor used anywhere. Perhaps it's no longer needed?
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146444185
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java ---
@@ -111,6 +113,29 @@ public LDAPConnection bindAs(String userDN, String password)
// Obtain appropriately-configured LDAPConnection instance
LDAPConnection ldapConnection = createLDAPConnection();
+ // Configure LDAP connection constraints
+ LDAPConstraints ldapConstraints = ldapConnection.getConstraints();
+ if (ldapConstraints == null)
+ ldapConstraints = new LDAPConstraints();
+
+ // Set whether or not we follow referrals
+ ldapConstraints.setReferralFollowing(confService.getFollowReferrals());
+
+ // If the referral auth method is set to bind, we set it using the existing
+ // username and password.
+ String refAuthMethod = confService.getReferralAuthentication();
+ if (refAuthMethod != null && refAuthMethod.equals("bind"))
+ ldapConstraints.setReferralHandler(new ReferralAuthHandler(userDN, password));
--- End diff --
Okay, I configured it to just use the search credentials.
The one situation that I thought about here that you might want the ability to configure separate referral credentials is where you know you have a situation where your base LDAP tree points to another system, and the credentials are different on the referred to system than they are on the base system. This is somewhat of a corner case, and I think it unlikely there are just huge numbers of people running that, but it's worth keeping in mind for future discussions. In that case, however, just having this parameter - ldap-referral-auth - wouldn't be enough - you'd also want to ability to configure a separate username and password for the referrals, so you'd end up adding a couple more parameters.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149263732
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -129,62 +130,79 @@
Map<String, Connection> connections = new HashMap<String, Connection>();
while (results.hasMore()) {
- LDAPEntry entry = results.next();
+ try {
- // Get common name (CN)
- LDAPAttribute cn = entry.getAttribute("cn");
- if (cn == null) {
- logger.warn("guacConfigGroup is missing a cn.");
- continue;
- }
+ LDAPEntry entry = results.next();
- // Get associated protocol
- LDAPAttribute protocol = entry.getAttribute("guacConfigProtocol");
- if (protocol == null) {
- logger.warn("guacConfigGroup \"{}\" is missing the "
- + "required \"guacConfigProtocol\" attribute.",
- cn.getStringValue());
- continue;
- }
+ // Get common name (CN)
+ LDAPAttribute cn = entry.getAttribute("cn");
+ if (cn == null) {
+ logger.warn("guacConfigGroup is missing a cn.");
+ continue;
+ }
+
+ // Get associated protocol
+ LDAPAttribute protocol = entry.getAttribute("guacConfigProtocol");
+ if (protocol == null) {
+ logger.warn("guacConfigGroup \"{}\" is missing the "
+ + "required \"guacConfigProtocol\" attribute.",
+ cn.getStringValue());
+ continue;
+ }
- // Set protocol
- GuacamoleConfiguration config = new GuacamoleConfiguration();
- config.setProtocol(protocol.getStringValue());
+ // Set protocol
+ GuacamoleConfiguration config = new GuacamoleConfiguration();
+ config.setProtocol(protocol.getStringValue());
- // Get parameters, if any
- LDAPAttribute parameterAttribute = entry.getAttribute("guacConfigParameter");
- if (parameterAttribute != null) {
+ // Get parameters, if any
+ LDAPAttribute parameterAttribute = entry.getAttribute("guacConfigParameter");
+ if (parameterAttribute != null) {
- // For each parameter
- Enumeration<?> parameters = parameterAttribute.getStringValues();
- while (parameters.hasMoreElements()) {
+ // For each parameter
+ Enumeration<?> parameters = parameterAttribute.getStringValues();
+ while (parameters.hasMoreElements()) {
- String parameter = (String) parameters.nextElement();
+ String parameter = (String) parameters.nextElement();
- // Parse parameter
- int equals = parameter.indexOf('=');
- if (equals != -1) {
+ // Parse parameter
+ int equals = parameter.indexOf('=');
+ if (equals != -1) {
- // Parse name
- String name = parameter.substring(0, equals);
- String value = parameter.substring(equals+1);
+ // Parse name
+ String name = parameter.substring(0, equals);
+ String value = parameter.substring(equals+1);
- config.setParameter(name, value);
+ config.setParameter(name, value);
+
+ }
}
}
- }
+ // Filter the configuration, substituting all defined tokens
+ tokenFilter.filterValues(config.getParameters());
- // Filter the configuration, substituting all defined tokens
- tokenFilter.filterValues(config.getParameters());
+ // Store connection using cn for both identifier and name
+ String name = cn.getStringValue();
+ Connection connection = new SimpleConnection(name, name, config);
+ connection.setParentIdentifier(LDAPAuthenticationProvider.ROOT_CONNECTION_GROUP);
+ connections.put(name, connection);
- // Store connection using cn for both identifier and name
- String name = cn.getStringValue();
- Connection connection = new SimpleConnection(name, name, config);
- connection.setParentIdentifier(LDAPAuthenticationProvider.ROOT_CONNECTION_GROUP);
- connections.put(name, connection);
+ }
+
+ // Deal with issues following LDAP referrals
+ catch (LDAPReferralException e) {
+ if (confService.getFollowReferrals()) {
+ logger.error("Could not follow referral.", e.getFailedReferral());
--- End diff --
Fixed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149264099
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPGuacamoleProperties.java ---
@@ -174,4 +175,34 @@ private LDAPGuacamoleProperties() {}
};
+ /**
+ * Whether or not we should follow referrals
+ */
+ public static final BooleanGuacamoleProperty LDAP_FOLLOW_REFERRALS = new BooleanGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "ldap-follow-referrals"; }
+
+ };
+
+ /**
+ * Maximum number of referral hops to follow
+ */
+ public static final IntegerGuacamoleProperty LDAP_MAX_REFERRAL_HOPS = new IntegerGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "ldap-max-referral-hops"; }
+
+ };
+
+ /**
+ * Number of seconds to wait for LDAP operations to complete
--- End diff --
Fixed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149263712
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -251,8 +269,22 @@ private String getConnectionSearchFilter(String userDN,
// The guacConfig group uses the seeAlso attribute to refer
// to these other groups
while (userRoleGroupResults.hasMore()) {
- LDAPEntry entry = userRoleGroupResults.next();
- connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ try {
+ LDAPEntry entry = userRoleGroupResults.next();
+ connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ }
+
+ 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.", e.getMessage());
--- End diff --
Fixed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146658839
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -107,19 +108,37 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
// Read all visible users
while (results.hasMore()) {
- LDAPEntry entry = results.next();
+ try {
+
+ LDAPEntry entry = results.next();
+
+ // Get username from record
+ LDAPAttribute username = entry.getAttribute(usernameAttribute);
+ if (username == null) {
+ logger.warn("Queried user is missing the username attribute \"{}\".", usernameAttribute);
+ continue;
+ }
+
+ // Store user using their username as the identifier
+ String identifier = username.getStringValue();
+ if (users.put(identifier, new SimpleUser(identifier)) != null)
+ logger.warn("Possibly ambiguous user account: \"{}\".", identifier);
- // Get username from record
- LDAPAttribute username = entry.getAttribute(usernameAttribute);
- if (username == null) {
- logger.warn("Queried user is missing the username attribute \"{}\".", usernameAttribute);
- continue;
}
- // Store user using their username as the identifier
- String identifier = username.getStringValue();
- if (users.put(identifier, new SimpleUser(identifier)) != null)
- logger.warn("Possibly ambiguous user account: \"{}\".", identifier);
+ // Deal with errors trying to follow referrals
+ 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.", e.getMessage());
+ logger.debug("Got a referral, but configured to not follow them.", e);
+ continue;
--- End diff --
Removed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146651500
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java ---
@@ -111,6 +113,29 @@ public LDAPConnection bindAs(String userDN, String password)
// Obtain appropriately-configured LDAPConnection instance
LDAPConnection ldapConnection = createLDAPConnection();
+ // Configure LDAP connection constraints
+ LDAPConstraints ldapConstraints = ldapConnection.getConstraints();
+ if (ldapConstraints == null)
+ ldapConstraints = new LDAPConstraints();
+
+ // Set whether or not we follow referrals
+ ldapConstraints.setReferralFollowing(confService.getFollowReferrals());
+
+ // If the referral auth method is set to bind, we set it using the existing
+ // username and password.
+ String refAuthMethod = confService.getReferralAuthentication();
+ if (refAuthMethod != null && refAuthMethod.equals("bind"))
+ ldapConstraints.setReferralHandler(new ReferralAuthHandler(userDN, password));
--- End diff --
Yeah, I see what you're saying. The changes here should be consistent with that - the referral authentication just uses what is passed in to the bindAs method for userDN and password, which, for the initial user search will be the search credentials, and subsequently will be the retrieved userDN and the password used for authentication.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149184417
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -107,19 +108,36 @@ private void putAllUsers(Map<String, User> users, LDAPConnection ldapConnection,
// Read all visible users
while (results.hasMore()) {
- LDAPEntry entry = results.next();
+ try {
+
+ LDAPEntry entry = results.next();
+
+ // Get username from record
+ LDAPAttribute username = entry.getAttribute(usernameAttribute);
+ if (username == null) {
+ logger.warn("Queried user is missing the username attribute \"{}\".", usernameAttribute);
+ continue;
+ }
+
+ // Store user using their username as the identifier
+ String identifier = username.getStringValue();
+ if (users.put(identifier, new SimpleUser(identifier)) != null)
+ logger.warn("Possibly ambiguous user account: \"{}\".", identifier);
- // Get username from record
- LDAPAttribute username = entry.getAttribute(usernameAttribute);
- if (username == null) {
- logger.warn("Queried user is missing the username attribute \"{}\".", usernameAttribute);
- continue;
}
- // Store user using their username as the identifier
- String identifier = username.getStringValue();
- if (users.put(identifier, new SimpleUser(identifier)) != null)
- logger.warn("Possibly ambiguous user account: \"{}\".", identifier);
+ // Deal with errors trying to follow referrals
+ 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.", e.getMessage());
--- End diff --
Missing `{}` in log message.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149184317
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -251,8 +269,22 @@ private String getConnectionSearchFilter(String userDN,
// The guacConfig group uses the seeAlso attribute to refer
// to these other groups
while (userRoleGroupResults.hasMore()) {
- LDAPEntry entry = userRoleGroupResults.next();
- connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ try {
+ LDAPEntry entry = userRoleGroupResults.next();
+ connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ }
+
+ 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.", e.getMessage());
--- End diff --
Missing `{}` in log message.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149186435
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPGuacamoleProperties.java ---
@@ -174,4 +175,34 @@ private LDAPGuacamoleProperties() {}
};
+ /**
+ * Whether or not we should follow referrals
+ */
+ public static final BooleanGuacamoleProperty LDAP_FOLLOW_REFERRALS = new BooleanGuacamoleProperty() {
+
+ @Override
+ public String getName() { return "ldap-follow-referrals"; }
+
+ };
+
+ /**
+ * Maximum number of referral hops to follow
--- End diff --
JavaDoc formatting: missing period.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149263746
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java ---
@@ -0,0 +1,80 @@
+/*
+ * 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;
+
+import com.google.inject.Inject;
+import com.novell.ldap.LDAPAuthHandler;
+import com.novell.ldap.LDAPAuthProvider;
+import com.novell.ldap.LDAPConnection;
+import java.io.UnsupportedEncodingException;
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class that implements the necessary authentication handling
+ * for following referrals in LDAP connections.
+ */
+public class ReferralAuthHandler implements LDAPAuthHandler {
+
+ /**
+ * Logger for this class.
+ */
+ private final Logger logger = LoggerFactory.getLogger(ReferralAuthHandler.class);
+
+ /**
+ * The LDAPAuthProvider object that will be set and returned to the referral handler.
+ */
+ private final LDAPAuthProvider ldapAuth;
+
+ /**
+ * Creates a ReferralAuthHandler object to handle authentication when
+ * following referrals in a LDAP connection, using the provided dn and
+ * password.
+ *
+ * @throws GuacamoleException
+ * If exceptions are caught while converting the password from a string
+ * into a byte array.
+ */
+ public ReferralAuthHandler(String dn, String password) throws GuacamoleException {
+ 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);
+ throw new GuacamoleException("Could not set password due to missing UTF-8 support.");
--- End diff --
Changed as suggested.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146658764
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java ---
@@ -0,0 +1,97 @@
+/*
+ * 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;
+
+import com.google.inject.Inject;
+import com.novell.ldap.LDAPAuthHandler;
+import com.novell.ldap.LDAPAuthProvider;
+import com.novell.ldap.LDAPConnection;
+import java.io.UnsupportedEncodingException;
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReferralAuthHandler implements LDAPAuthHandler {
+
+ /**
+ * Logger for this class.
+ */
+ private final Logger logger = LoggerFactory.getLogger(ReferralAuthHandler.class);
+
+ /**
+ * The LDAPAuthProvider object that will be set and returned to the referral handler.
+ */
+ private final LDAPAuthProvider ldapAuth;
+
+ /**
+ * Service for retrieving LDAP server configuration information.
+ */
+ @Inject
+ private ConfigurationService confService;
--- End diff --
Removed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149264082
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java ---
@@ -0,0 +1,80 @@
+/*
+ * 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;
+
+import com.google.inject.Inject;
+import com.novell.ldap.LDAPAuthHandler;
+import com.novell.ldap.LDAPAuthProvider;
+import com.novell.ldap.LDAPConnection;
+import java.io.UnsupportedEncodingException;
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class that implements the necessary authentication handling
+ * for following referrals in LDAP connections.
+ */
+public class ReferralAuthHandler implements LDAPAuthHandler {
+
+ /**
+ * Logger for this class.
+ */
+ private final Logger logger = LoggerFactory.getLogger(ReferralAuthHandler.class);
+
+ /**
+ * The LDAPAuthProvider object that will be set and returned to the referral handler.
+ */
+ private final LDAPAuthProvider ldapAuth;
+
+ /**
+ * Creates a ReferralAuthHandler object to handle authentication when
+ * following referrals in a LDAP connection, using the provided dn and
+ * password.
+ *
+ * @throws GuacamoleException
+ * If exceptions are caught while converting the password from a string
--- End diff --
Removed since using `UnsupportedOperationException`.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146647996
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java ---
@@ -111,6 +113,29 @@ public LDAPConnection bindAs(String userDN, String password)
// Obtain appropriately-configured LDAPConnection instance
LDAPConnection ldapConnection = createLDAPConnection();
+ // Configure LDAP connection constraints
+ LDAPConstraints ldapConstraints = ldapConnection.getConstraints();
+ if (ldapConstraints == null)
+ ldapConstraints = new LDAPConstraints();
+
+ // Set whether or not we follow referrals
+ ldapConstraints.setReferralFollowing(confService.getFollowReferrals());
+
+ // If the referral auth method is set to bind, we set it using the existing
+ // username and password.
+ String refAuthMethod = confService.getReferralAuthentication();
+ if (refAuthMethod != null && refAuthMethod.equals("bind"))
+ ldapConstraints.setReferralHandler(new ReferralAuthHandler(userDN, password));
--- End diff --
Well, to clarify a bit, there are two types of searches which may occur in the LDAP auth:
* The search performed to resolve the user's DN. This is the only case where the search DN and password are used.
* The search to retrieve the users and/or connections defined within LDAP and accessible to the user that authenticated. These searches naturally **must** use the user's own DN and password, since part of the point of the LDAP auth is to allow the LDAP directory's own access control to take effect.
If users or connections cannot be retrieved via LDAP because the authenticated user cannot access them, then they shouldn't have access, referral or not.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146658634
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java ---
@@ -0,0 +1,97 @@
+/*
+ * 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;
+
+import com.google.inject.Inject;
+import com.novell.ldap.LDAPAuthHandler;
+import com.novell.ldap.LDAPAuthProvider;
+import com.novell.ldap.LDAPConnection;
+import java.io.UnsupportedEncodingException;
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReferralAuthHandler implements LDAPAuthHandler {
--- End diff --
Added.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149184513
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -267,8 +285,23 @@ private String generateLDAPQuery(String username)
// Add all DNs for found users
while (results.hasMore()) {
- LDAPEntry entry = results.next();
- userDNs.add(entry.getDN());
+ try {
+ LDAPEntry entry = results.next();
+ userDNs.add(entry.getDN());
+ }
+
+ // Deal with errors following referrals
+ catch (LDAPReferralException e) {
+ if (confService.getFollowReferrals()) {
+ logger.error("Error trying to follow a referral.", e.getMessage());
+ logger.debug("Encountered an error trying to follow a referral.", e);
+ throw new GuacamoleServerException("Failed while trying to follow referrals.", e);
+ }
+ else {
+ logger.warn("Given a referral, not following it.", e.getMessage());
--- End diff --
Missing `{}` in log message.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146417245
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java ---
@@ -111,6 +113,29 @@ public LDAPConnection bindAs(String userDN, String password)
// Obtain appropriately-configured LDAPConnection instance
LDAPConnection ldapConnection = createLDAPConnection();
+ // Configure LDAP connection constraints
+ LDAPConstraints ldapConstraints = ldapConnection.getConstraints();
+ if (ldapConstraints == null)
+ ldapConstraints = new LDAPConstraints();
+
+ // Set whether or not we follow referrals
+ ldapConstraints.setReferralFollowing(confService.getFollowReferrals());
+
+ // If the referral auth method is set to bind, we set it using the existing
+ // username and password.
+ String refAuthMethod = confService.getReferralAuthentication();
+ if (refAuthMethod != null && refAuthMethod.equals("bind"))
+ ldapConstraints.setReferralHandler(new ReferralAuthHandler(userDN, password));
--- End diff --
I believe it's the difference between using the credentials that the user logged in with vs. using the credentials specified in the guacamole.properties file, if any. So, in some cases it'll be the difference between an anonymous bind and an authenticated bind, in other cases it'll be the difference between binding with the configured search user vs. the logged in user.
There are cases where the user who logged in may not have access to the entire tree, or even the base of the tree, so you'd want the search & referral following to happen under that user account instead of the logged in account.
At least, that's the theory, I think...whether or not I've actually implemented it that way, I probably should go back and take another look and make sure.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146658688
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java ---
@@ -0,0 +1,97 @@
+/*
+ * 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;
+
+import com.google.inject.Inject;
+import com.novell.ldap.LDAPAuthHandler;
+import com.novell.ldap.LDAPAuthProvider;
+import com.novell.ldap.LDAPConnection;
+import java.io.UnsupportedEncodingException;
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ReferralAuthHandler implements LDAPAuthHandler {
+
+ /**
+ * Logger for this class.
+ */
+ private final Logger logger = LoggerFactory.getLogger(ReferralAuthHandler.class);
+
+ /**
+ * The LDAPAuthProvider object that will be set and returned to the referral handler.
+ */
+ private final LDAPAuthProvider ldapAuth;
+
+ /**
+ * Service for retrieving LDAP server configuration information.
+ */
+ @Inject
+ private ConfigurationService confService;
+
+
+ public ReferralAuthHandler() throws GuacamoleException {
+ String binddn = confService.getSearchBindDN();
+ String password = confService.getSearchBindPassword();
+ byte[] passwordBytes;
--- End diff --
Removed this constructor.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146658609
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java ---
@@ -111,6 +113,28 @@ public LDAPConnection bindAs(String userDN, String password)
// Obtain appropriately-configured LDAPConnection instance
LDAPConnection ldapConnection = createLDAPConnection();
+ // Configure LDAP connection constraints
+ LDAPConstraints ldapConstraints = ldapConnection.getConstraints();
+ if (ldapConstraints == null)
+ ldapConstraints = new LDAPConstraints();
+
+ // Set whether or not we follow referrals
+ ldapConstraints.setReferralFollowing(confService.getFollowReferrals());
+
+ // If the referral auth method is set to bind, we set it using the existing
--- End diff --
Corrected.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149184261
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -129,62 +130,79 @@
Map<String, Connection> connections = new HashMap<String, Connection>();
while (results.hasMore()) {
- LDAPEntry entry = results.next();
+ try {
- // Get common name (CN)
- LDAPAttribute cn = entry.getAttribute("cn");
- if (cn == null) {
- logger.warn("guacConfigGroup is missing a cn.");
- continue;
- }
+ LDAPEntry entry = results.next();
- // Get associated protocol
- LDAPAttribute protocol = entry.getAttribute("guacConfigProtocol");
- if (protocol == null) {
- logger.warn("guacConfigGroup \"{}\" is missing the "
- + "required \"guacConfigProtocol\" attribute.",
- cn.getStringValue());
- continue;
- }
+ // Get common name (CN)
+ LDAPAttribute cn = entry.getAttribute("cn");
+ if (cn == null) {
+ logger.warn("guacConfigGroup is missing a cn.");
+ continue;
+ }
+
+ // Get associated protocol
+ LDAPAttribute protocol = entry.getAttribute("guacConfigProtocol");
+ if (protocol == null) {
+ logger.warn("guacConfigGroup \"{}\" is missing the "
+ + "required \"guacConfigProtocol\" attribute.",
+ cn.getStringValue());
+ continue;
+ }
- // Set protocol
- GuacamoleConfiguration config = new GuacamoleConfiguration();
- config.setProtocol(protocol.getStringValue());
+ // Set protocol
+ GuacamoleConfiguration config = new GuacamoleConfiguration();
+ config.setProtocol(protocol.getStringValue());
- // Get parameters, if any
- LDAPAttribute parameterAttribute = entry.getAttribute("guacConfigParameter");
- if (parameterAttribute != null) {
+ // Get parameters, if any
+ LDAPAttribute parameterAttribute = entry.getAttribute("guacConfigParameter");
+ if (parameterAttribute != null) {
- // For each parameter
- Enumeration<?> parameters = parameterAttribute.getStringValues();
- while (parameters.hasMoreElements()) {
+ // For each parameter
+ Enumeration<?> parameters = parameterAttribute.getStringValues();
+ while (parameters.hasMoreElements()) {
- String parameter = (String) parameters.nextElement();
+ String parameter = (String) parameters.nextElement();
- // Parse parameter
- int equals = parameter.indexOf('=');
- if (equals != -1) {
+ // Parse parameter
+ int equals = parameter.indexOf('=');
+ if (equals != -1) {
- // Parse name
- String name = parameter.substring(0, equals);
- String value = parameter.substring(equals+1);
+ // Parse name
+ String name = parameter.substring(0, equals);
+ String value = parameter.substring(equals+1);
- config.setParameter(name, value);
+ config.setParameter(name, value);
+
+ }
}
}
- }
+ // Filter the configuration, substituting all defined tokens
+ tokenFilter.filterValues(config.getParameters());
- // Filter the configuration, substituting all defined tokens
- tokenFilter.filterValues(config.getParameters());
+ // Store connection using cn for both identifier and name
+ String name = cn.getStringValue();
+ Connection connection = new SimpleConnection(name, name, config);
+ connection.setParentIdentifier(LDAPAuthenticationProvider.ROOT_CONNECTION_GROUP);
+ connections.put(name, connection);
- // Store connection using cn for both identifier and name
- String name = cn.getStringValue();
- Connection connection = new SimpleConnection(name, name, config);
- connection.setParentIdentifier(LDAPAuthenticationProvider.ROOT_CONNECTION_GROUP);
- connections.put(name, connection);
+ }
+
+ // Deal with issues following LDAP referrals
+ 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.", e.getMessage());
--- End diff --
As above, if the intent is to include `e.getMessage()` within the logged message, then the `{}` string must be present in the message at the location the value should be inserted.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146658810
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -251,8 +270,23 @@ private String getConnectionSearchFilter(String userDN,
// The guacConfig group uses the seeAlso attribute to refer
// to these other groups
while (userRoleGroupResults.hasMore()) {
- LDAPEntry entry = userRoleGroupResults.next();
- connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ try {
+ LDAPEntry entry = userRoleGroupResults.next();
+ connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ }
+
+ catch (LDAPReferralException e) {
+ if (confService.getFollowReferrals()) {
+ logger.error("Could not follow referral.", e.getMessage());
+ 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.", e.getMessage());
+ logger.debug("Got a referral, but configured to not follow them.", e);
+ continue;
--- End diff --
Removed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149182139
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ReferralAuthHandler.java ---
@@ -0,0 +1,80 @@
+/*
+ * 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;
+
+import com.google.inject.Inject;
+import com.novell.ldap.LDAPAuthHandler;
+import com.novell.ldap.LDAPAuthProvider;
+import com.novell.ldap.LDAPConnection;
+import java.io.UnsupportedEncodingException;
+import org.apache.guacamole.GuacamoleException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Class that implements the necessary authentication handling
+ * for following referrals in LDAP connections.
+ */
+public class ReferralAuthHandler implements LDAPAuthHandler {
+
+ /**
+ * Logger for this class.
+ */
+ private final Logger logger = LoggerFactory.getLogger(ReferralAuthHandler.class);
+
+ /**
+ * The LDAPAuthProvider object that will be set and returned to the referral handler.
+ */
+ private final LDAPAuthProvider ldapAuth;
+
+ /**
+ * Creates a ReferralAuthHandler object to handle authentication when
+ * following referrals in a LDAP connection, using the provided dn and
+ * password.
+ *
+ * @throws GuacamoleException
+ * If exceptions are caught while converting the password from a string
--- End diff --
"If exceptions are caught" doesn't really provide much information, and "converting into a byte array" is an internal concern. This may be a moot point if switching over to `UnsupportedOperationException` (see below), as the exception need not be declared in that case, but otherwise perhaps there is a way to describe the cases where exceptions will be thrown which does not reference internal implementation details?
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146654624
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java ---
@@ -111,6 +113,28 @@ public LDAPConnection bindAs(String userDN, String password)
// Obtain appropriately-configured LDAPConnection instance
LDAPConnection ldapConnection = createLDAPConnection();
+ // Configure LDAP connection constraints
+ LDAPConstraints ldapConstraints = ldapConnection.getConstraints();
+ if (ldapConstraints == null)
+ ldapConstraints = new LDAPConstraints();
+
+ // Set whether or not we follow referrals
+ ldapConstraints.setReferralFollowing(confService.getFollowReferrals());
+
+ // If the referral auth method is set to bind, we set it using the existing
--- End diff --
With the removal of the auth method property, this comment is no longer correct.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r146658052
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -251,8 +270,23 @@ private String getConnectionSearchFilter(String userDN,
// The guacConfig group uses the seeAlso attribute to refer
// to these other groups
while (userRoleGroupResults.hasMore()) {
- LDAPEntry entry = userRoleGroupResults.next();
- connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ try {
+ LDAPEntry entry = userRoleGroupResults.next();
+ connectionSearchFilter.append("(seeAlso=").append(escapingService.escapeLDAPSearchFilter(entry.getDN())).append(")");
+ }
+
+ catch (LDAPReferralException e) {
+ if (confService.getFollowReferrals()) {
+ logger.error("Could not follow referral.", e.getMessage());
+ logger.debug("Error encountered trying to follow referral.", e);
+ throw new GuacamoleServerException("Could not follow LDAP referral.", e);
--- End diff --
Recalling the changes proposed via #129, is it desirable that the entire search will abort with a failure if a referral error occurs? Or should the error be non-fatal and simply logged?
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149263692
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/UserService.java ---
@@ -267,8 +285,23 @@ private String generateLDAPQuery(String username)
// Add all DNs for found users
while (results.hasMore()) {
- LDAPEntry entry = results.next();
- userDNs.add(entry.getDN());
+ try {
+ LDAPEntry entry = results.next();
+ userDNs.add(entry.getDN());
+ }
+
+ // Deal with errors following referrals
+ catch (LDAPReferralException e) {
+ if (confService.getFollowReferrals()) {
+ logger.error("Error trying to follow a referral.", e.getMessage());
--- End diff --
Fixed.
---
[GitHub] incubator-guacamole-client pull request #202: GUACAMOLE-243: Allow LDAP refe...
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/202#discussion_r149186298
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -273,6 +291,23 @@ public LDAPSearchConstraints getLDAPSearchConstraints() throws GuacamoleExceptio
}
/**
+ * Returns the maximum number of referral hops to follow.
+ *
+ * @return
+ * The maximum number of referral hops to follow
+ * as configured in guacamole.properties
--- End diff --
JavaDoc formatting: missing period.
---