You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by jaredfrees <gi...@git.apache.org> on 2018/06/08 19:15:31 UTC
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
GitHub user jaredfrees opened a pull request:
https://github.com/apache/guacamole-client/pull/299
GUACAMOLE-524: Allow custom LDAP attributes to be used as tokens
1. Add property for reading list of LDAP attributes in guacamole.properties.
2. Add LDAP attribute name and attribute value into credentials object.
3. Add attribute name and attribute value into Tokens from credentials object.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jaredfrees/guacamole-client master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/guacamole-client/pull/299.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 #299
----
commit 38eb97b42245699988cfa1fefe7bf80eeea3c56c
Author: Jared Frees <fr...@...>
Date: 2018-06-08T16:30:15Z
GUACAMOLE-524: Added reading of LDAP attributes in guacamole.properties named 'ldap-user-attributes'.
Added method getAttributes() in ConfigurationService to read environment property
LDAPGuacamoleProperties.LDAP_USER_ATTRIBUTES. These
attributes are arbitrary LDAP attributes that will
be mapped to the user in credentials and
tokens.
commit 5ca32a221afb9ff478e8b460e45fc14e790bcc5d
Author: Jared Frees <fr...@...>
Date: 2018-06-08T16:34:06Z
GUACAMOLE-524: Add LDAP attributes to credentials.
AuthenticationProviderService gets LDAP attributes
from confService and queries the LDAP server to
find values on user for specified attributes.
Added a Map<String, String> to Credentials named ldapAttrs
and a getLDAPAttributes() and setLDAPAttributes() to
manipulate ldapAttrs on credentials. Once
AuthenticationProviderService gets the values for the
LDAP attributes it sets ldapAttrs on the credentials object.
commit ad6be801311b3be14dde68be02f2b72dcdc1d8f9
Author: Jared Frees <fr...@...>
Date: 2018-06-08T16:40:02Z
GUACAMOLE-524: Add LDAP attribute tokens to StandardTokens.
In method addStandardTokens(TokenFilter, Credentials),
adds each LDAP attribute from credentials.getLDAPAttributes().
Name of token is "USER_ATTR:" + name of attribute and value
is the value of the attribute.
----
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194163526
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -221,6 +230,14 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
throw new GuacamoleInvalidCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD);
try {
+ try {
--- End diff --
Two things, here...
First, I don't think there's any reason to nest these `try` statements, is there? This may actually be moot (see comment below on the `getLDAPAttributes()` method), but you should be able to do something like:
try {
// Code to authenticate and retrieve attributes
}
catch (LDAPException e) {
throw...
}
finally {
ldapService.disconnect...
}
I've definitely encountered instances where the nesting was required, but I don't think this is one of them?
Second thing, I would actually put the attribute retrieval *after* the code to authenticate the user. It probably isn't worth retrieving the attributes if the authentication fails, so I'd only do that if authentication succeeds, and, thus, after.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by jaredfrees <gi...@git.apache.org>.
Github user jaredfrees commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r195733455
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/token/StandardTokens.java ---
@@ -74,6 +76,11 @@
*/
private static final String TIME_FORMAT = "HHmmss";
+ /**
+ * The prefix of the arbitrary attribute tokens.
+ */
+ public static final String ATTR_TOKEN_PREFIX = "GUAC_ATTR:";
--- End diff --
I think I'll change it so it fits into the current RegEx.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194525537
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/AuthenticatedUser.java ---
@@ -20,15 +20,19 @@
package org.apache.guacamole.auth.ldap.user;
import com.google.inject.Inject;
+import java.util.Map;
+import java.util.HashMap;
--- End diff --
HashMap > Map, at least in the alphabet.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by jaredfrees <gi...@git.apache.org>.
Github user jaredfrees commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194876880
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/AbstractAuthenticatedUser.java ---
@@ -41,4 +42,12 @@ public void invalidate() {
// Nothing to invalidate
}
+ public Map<String, String> getAttributes() {
--- End diff --
I think Option 2 is the way to go. Personally, I don't think we should mess so much with the other extensions.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by mike-jumper <gi...@git.apache.org>.
Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194158582
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Credentials.java ---
@@ -72,6 +72,29 @@
*/
private transient HttpSession session;
+ /**
+ * Arbitrary LDAP attributes specified in guacamole.properties
+ */
+ private Map<String, String> ldapAttrs;
+
+ /**
+ * Returns the lDAP attributes associated with this set of credentials.
+ * @return The LDAP attributes Map associated with this set of credentials,
+ * or null if no LDAP Attributes have been set.
+ */
+ public Map<String, String> getLDAPAttributes() {
+ return ldapAttrs;
+ }
+
+ /**
+ * Sets the LDAP attributes associated with this set of credentials.
+ * @param attributes The LDAP attributes to associate with this set of
+ * credentials.
+ */
+ public void setLDAPAttributes(Map<String, String> attributes) {
--- End diff --
This is too specific to LDAP. The `Credentials` object is intended to be generic, and shouldn't be tightly coupled to any one authentication mechanism.
If `Credentials` should include arbitrary attributes, I'd suggest implementing the `Attributes` interface: https://github.com/apache/guacamole-client/blob/7e6df7c1393be2df7c72f77d530a545edb473623/guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Attributes.java
Better perhaps than that would be having `AuthenticatedUser` implement `Attributes` (as, semantically, it is the *user* that has these attributes, not their credentials).
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194526588
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -22,10 +22,18 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
import com.novell.ldap.LDAPConnection;
+import com.novell.ldap.LDAPAttributeSet;
+import com.novell.ldap.LDAPEntry;
+import com.novell.ldap.LDAPAttribute;
+import com.novell.ldap.LDAPException;
+import java.util.HashMap;
import java.util.List;
+import java.util.Iterator;
+import java.util.Map;
--- End diff --
This is totally a nitpick, but this is *almost* in alphabetical order :-).
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194866759
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/AuthenticatedUser.java ---
@@ -53,6 +61,72 @@ public void init(Credentials credentials) {
setIdentifier(credentials.getUsername());
}
+ /**
+ * Get a map of attributes associated with this AuthenticatedUser.
+ *
+ * @return
+ * The Map of arbitrary attributes associated with this
+ * AuthenticatedUser object.
+ */
+ @Override
+ public Map<String, String> getAttributes() {
+ return attributes;
+ }
+
+ /**
+ * Sets a map of attributes associated with this AuthenticatedUser.
+ *
+ * @param attributes
+ * A map of attribute key/value pairs to add to this AuthenticatedUser.
+ */
--- End diff --
Same thing as above - since we're overriding another already-documented method, here, you should be able to leave out the Javadoc comments.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194525160
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/token/StandardTokens.java ---
@@ -23,6 +23,8 @@
import java.util.Date;
import org.apache.guacamole.net.auth.AuthenticatedUser;
import org.apache.guacamole.net.auth.Credentials;
+import java.util.Map;
+import java.util.Set;
--- End diff --
This should go above the `org.apache` entries, and be alphabetized in the correct order.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194870590
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -221,21 +229,82 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
throw new GuacamoleInvalidCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD);
try {
-
// Return AuthenticatedUser if bind succeeds
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
authenticatedUser.init(credentials);
+
+ //set attributes
--- End diff --
`// Set attributes`
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194525708
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -126,6 +127,10 @@
TokenFilter tokenFilter = new TokenFilter();
StandardTokens.addStandardTokens(tokenFilter, user);
+ // Add custom attribute tokens
+ Map<String, String> attrs = ( (org.apache.guacamole.auth.ldap.user.AuthenticatedUser) user).getAttributes();
--- End diff --
I'm not sure we want to have to do this type-casting, here, but see my comment below on the implementation within the LDAP-specific `AuthenticatedUser` class.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194867108
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/AbstractAuthenticatedUser.java ---
@@ -41,4 +42,12 @@ public void invalidate() {
// Nothing to invalidate
}
+ public Map<String, String> getAttributes() {
--- End diff --
So, I think there are two options here:
1. Don't implement these, here, since `AbstractAuthenticatedUser` implements `AuthenticatedUser`, which implements `Attributes`, these should already be defined at that level. If you don't implement them, here, any classes that extend `AbstractAuthenticatedUser` will need to override them.
2. Implement them, here, so that there's a base implementation, but use the `@Override` tag so that it's understood those are overriding methods from another class (`AuthenticatedUser`). This means you won't have to implement them in every single class that extends `AbstractAuthenticatedUser` except where you want to change this behavior.
Option 2 is definitely simpler, or at least requires fewer changes to other extensions, so that's probably the easiest to go with for the time being - that just means adding the `@Override` tag for this method and `setAttributes()`.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r197095272
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/token/StandardTokens.java ---
@@ -164,6 +172,33 @@ public static void addStandardTokens(TokenFilter filter, AuthenticatedUser user)
// Add tokens specific to credentials
addStandardTokens(filter, user.getCredentials());
+ // Add custom attribute tokens
+ addAttributeTokens(filter, user.getAttributes());
+ }
+
+ /**
+ * Add attribute tokens to StandardTokens. These are arbitrary
+ * key/value pairs that may be configured by the various authentication
+ * extensions.
+ *
+ * @param filter
+ * The TokenFilter to add attributes tokens to.
--- End diff --
"attribute tokens"
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by jaredfrees <gi...@git.apache.org>.
Github user jaredfrees commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r195750933
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/RemoteAuthenticatedUser.java ---
@@ -67,6 +69,21 @@
*/
private static final Pattern X_FORWARDED_FOR = Pattern.compile("^" + IP_ADDRESS_REGEX + "(, " + IP_ADDRESS_REGEX + ")*$");
+ /**
+ * Arbitrary attributes associated with this RemoteAuthenticatedUser object.
+ */
+ private Map<String, String> attributes = new HashMap<String, String>();
--- End diff --
Oh yes you are right. I didn't notice this. I am going to try to set the attributes in `ModeledAuthenticatedUser`
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194520899
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/connection/ConnectionService.java ---
@@ -36,6 +36,7 @@
import org.apache.guacamole.GuacamoleException;
import org.apache.guacamole.GuacamoleServerException;
import org.apache.guacamole.net.auth.AuthenticatedUser;
+//import org.apache.guacamole.auth.ldap.user.AuthenticatedUser;
--- End diff --
This should probably come out :smiley:
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r195792533
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledAuthenticatedUser.java ---
@@ -79,6 +84,7 @@ public ModeledAuthenticatedUser(AuthenticatedUser authenticatedUser,
super(authenticatedUser.getAuthenticationProvider(), authenticatedUser.getCredentials());
this.modelAuthenticationProvider = modelAuthenticationProvider;
this.user = user;
+ this.setAttributes(authenticatedUser.getAttributes());
--- End diff --
I think this properly should be:
super.setAttributes(authenticatedUser.getAttributes());
since `setAttributes()` is defined in `RemoteAuthenticatedUser`.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194864605
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -221,21 +229,82 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
throw new GuacamoleInvalidCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD);
try {
-
// Return AuthenticatedUser if bind succeeds
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
authenticatedUser.init(credentials);
+
+ //set attributes
+ String username = credentials.getUsername();
+ Map<String, String> attrs = getLDAPAttributes(ldapConnection, username);
+ authenticatedUser.setAttributes(attrs);
+
return authenticatedUser;
}
-
// Always disconnect
finally {
ldapService.disconnect(ldapConnection);
}
}
+ /**
+ * Returns all custom LDAP attributes on the user currently bound under
+ * the given LDAP connection. The custom attributes are specified in
+ * guacamole.properties.
+ *
+ * @param ldapConnection
+ * LDAP connection to find the custom LDAP attributes.
+ *
+ * @param username
+ * The username of the user whose attributes are queried.
+ *
+ * @return
+ * All attributes on the user currently bound under the
+ * given LDAP connection, as a map of attribute name to
+ * corresponding attribute value.
+ *
+ * @throws LDAPException
+ * If an error occurs while searching for the user attributes.
+ *
+ * @throws GuacamoleException
+ * If an error occurs retrieving the user DN.
+ */
+ private Map<String, String> getLDAPAttributes(LDAPConnection ldapConnection,
+ String username) throws GuacamoleException {
+
+ // Get attributes from configuration information
+ List<String> attrList = confService.getAttributes();
+
+ // If there are no attributes there is no reason to search LDAP
+ if (attrList == null || attrList.isEmpty())
+ return null;
+
+ // Build LDAP query parameters
+ String[] attrArray = attrList.toArray(new String[attrList.size()]);
+ String userDN = getUserBindDN(username);
+
+ Map<String, String> attrMap = new HashMap<String, String>();
+ try {
+ // Get LDAP attributes by querying LDAP
+ LDAPEntry userEntry = ldapConnection.read(userDN, attrArray);
+ LDAPAttributeSet attrSet = userEntry.getAttributeSet();
+
+ // Add each attribute into Map
+ for (Object attrObj : attrSet) {
+ LDAPAttribute attr = (LDAPAttribute)attrObj;
+ String attrName = attr.getName();
+ String attrValue = attr.getStringValue();
+ attrMap.put(attrName, attrValue);
--- End diff --
Can simplify this to:
attrMap.put(attr.getName(), attr.getStringValue());
and avoid the extra `String` variables that don't get used.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by jaredfrees <gi...@git.apache.org>.
Github user jaredfrees commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r195733813
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/RemoteAuthenticatedUser.java ---
@@ -67,6 +69,21 @@
*/
private static final Pattern X_FORWARDED_FOR = Pattern.compile("^" + IP_ADDRESS_REGEX + "(, " + IP_ADDRESS_REGEX + ")*$");
+ /**
+ * Arbitrary attributes associated with this RemoteAuthenticatedUser object.
+ */
+ private Map<String, String> attributes = new HashMap<String, String>();
--- End diff --
and currently `AuthenticatedUser` extends `Attributes`
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r195110827
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/AuthenticatedUser.java ---
@@ -20,7 +20,10 @@
package org.apache.guacamole.auth.ldap.user;
import com.google.inject.Inject;
+import java.util.HashMap;
+import java.util.Map;
import org.apache.guacamole.net.auth.AbstractAuthenticatedUser;
+import org.apache.guacamole.net.auth.Attributes;
--- End diff --
Unused import.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r197090964
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -21,11 +21,20 @@
import com.google.inject.Inject;
import com.google.inject.Provider;
+import com.novell.ldap.LDAPAttribute;
+import com.novell.ldap.LDAPAttributeSet;
import com.novell.ldap.LDAPConnection;
+import com.novell.ldap.LDAPEntry;
+import com.novell.ldap.LDAPException;
+import com.novell.ldap.LDAPReferralException;
+import java.util.HashMap;
+import java.util.Iterator;
--- End diff --
Unused import.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194167771
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/token/StandardTokens.java ---
@@ -23,6 +23,10 @@
import java.util.Date;
import org.apache.guacamole.net.auth.AuthenticatedUser;
import org.apache.guacamole.net.auth.Credentials;
+import java.util.Map;
+import java.util.Set;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
--- End diff --
Guessing you had these in here for debugging purposes, but doesn't look like there's any logging code left, so probably best to get rid of them. Also, these should probably be alphabetized.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by jaredfrees <gi...@git.apache.org>.
Github user jaredfrees commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r195794382
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledAuthenticatedUser.java ---
@@ -26,6 +26,11 @@
import org.apache.guacamole.net.auth.AuthenticationProvider;
import org.apache.guacamole.net.auth.Credentials;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.util.Map;
--- End diff --
yep, my bad.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by jaredfrees <gi...@git.apache.org>.
Github user jaredfrees commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194885846
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -221,21 +230,80 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
throw new GuacamoleInvalidCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD);
try {
-
// Return AuthenticatedUser if bind succeeds
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
authenticatedUser.init(credentials);
+
+ // Set attributes
+ String username = credentials.getUsername();
+ Map<String, String> attrs = getLDAPAttributes(ldapConnection, username);
+ authenticatedUser.setAttributes(attrs);
+
return authenticatedUser;
}
-
// Always disconnect
finally {
ldapService.disconnect(ldapConnection);
}
}
+ /**
+ * Returns all custom LDAP attributes on the user currently bound under
+ * the given LDAP connection. The custom attributes are specified in
+ * guacamole.properties.
+ *
+ * @param ldapConnection
+ * LDAP connection to find the custom LDAP attributes.
+ *
+ * @param username
+ * The username of the user whose attributes are queried.
+ *
+ * @return
+ * All attributes on the user currently bound under the
+ * given LDAP connection, as a map of attribute name to
+ * corresponding attribute value.
+ *
+ * @throws LDAPException
+ * If an error occurs while searching for the user attributes.
+ *
+ * @throws GuacamoleException
+ * If an error occurs retrieving the user DN.
--- End diff --
yes you're right that is a possibility also, I'll add that.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by jaredfrees <gi...@git.apache.org>.
Github user jaredfrees commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194410884
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -236,6 +253,58 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Returns all custom LDAP attributes on the user currently bound under
+ * the given LDAP connection. The custom attributes are specified in
+ * guacamole.properties.
+ *
+ * @param ldapConnection
+ * LDAP connection to find the custom LDAP attributes.
+ * @param username
+ * The username of the user whose attributes are queried.
+ *
+ * @return
+ * All attributes on the user currently bound under the
+ * given LDAP connection, as a map of attribute name to
+ * corresponding attribute value.
+ *
+ * @throws LDAPException
+ * If an error occurs while searching for the user attributes.
+ *
+ * @throws GuacamoleException
+ * If an error occurs retrieving the user DN.
+ */
+ private Map<String, String> getLDAPAttributes(LDAPConnection ldapConnection,
+ String username) throws LDAPException, GuacamoleException {
+
+ // Get attributes from configuration information
+ List<String> attrList = confService.getAttributes();
+
+ // If there are no attributes there is no reason to search LDAP
+ if (attrList.size() == 0)
+ return null;
+
+ // Build LDAP query parameters
+ String[] attrArray = attrList.toArray(new String[attrList.size()]);
+ String userDN = getUserBindDN(username);
+
+ // Get LDAP attributes by querying LDAP
+ LDAPEntry userEntry = ldapConnection.read(userDN, attrArray);
+ LDAPAttributeSet attrSet = userEntry.getAttributeSet();
+
+ // Add each attribute into Map
+ Map<String, String> attrMap = new HashMap<String, String>();
+ Iterator attrIterator = attrSet.iterator();
+ while (attrIterator.hasNext()) {
--- End diff --
Here is what I am doing now:
for (Object attrObj : attrMap) {
LDAPAttribute attr = (LDAPAttribute)attrObj;
.....
}
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r195712691
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/RemoteAuthenticatedUser.java ---
@@ -67,6 +69,21 @@
*/
private static final Pattern X_FORWARDED_FOR = Pattern.compile("^" + IP_ADDRESS_REGEX + "(, " + IP_ADDRESS_REGEX + ")*$");
+ /**
+ * Arbitrary attributes associated with this RemoteAuthenticatedUser object.
+ */
+ private Map<String, String> attributes = new HashMap<String, String>();
--- End diff --
I'm not sure if it's better to put this here, in this abstract class, or if this (along with the `@Override`s below) should only be done on the implementing classes, like `ModeledAuthenticatedUser`? @mike-jumper any guidance?
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194526230
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -221,21 +229,79 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
throw new GuacamoleInvalidCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD);
try {
-
// Return AuthenticatedUser if bind succeeds
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
authenticatedUser.init(credentials);
+
+ //set attributes
+ String username = credentials.getUsername();
+ Map<String, String> attrs = getLDAPAttributes(ldapConnection, username);
+ authenticatedUser.setAttributes(attrs);
+
return authenticatedUser;
}
-
+ catch (LDAPException e) {
+ throw new GuacamoleServerException("Error while querying for User Attributes.", e);
+ }
// Always disconnect
finally {
ldapService.disconnect(ldapConnection);
}
}
+ /**
+ * Returns all custom LDAP attributes on the user currently bound under
+ * the given LDAP connection. The custom attributes are specified in
+ * guacamole.properties.
+ *
+ * @param ldapConnection
+ * LDAP connection to find the custom LDAP attributes.
+ * @param username
+ * The username of the user whose attributes are queried.
+ *
+ * @return
+ * All attributes on the user currently bound under the
+ * given LDAP connection, as a map of attribute name to
+ * corresponding attribute value.
+ *
+ * @throws LDAPException
+ * If an error occurs while searching for the user attributes.
+ *
+ * @throws GuacamoleException
+ * If an error occurs retrieving the user DN.
+ */
+ private Map<String, String> getLDAPAttributes(LDAPConnection ldapConnection,
+ String username) throws LDAPException {
+
+ // Get attributes from configuration information
+ List<String> attrList = confService.getAttributes();
--- End diff --
Since `getAttributes()` throws a `GuacamoleException`, it will need to be caught, here. That's why I suggest having the `getLDAPAttributes()` method throw only `GuacamoleException`, and then using `try {} catch {}` within this method to catch and re-throw the `LDAPException` as a `GuacamoleException`. One way or the other, though, this needs to be handled.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194161639
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/net/auth/Credentials.java ---
@@ -72,6 +72,29 @@
*/
private transient HttpSession session;
+ /**
+ * Arbitrary LDAP attributes specified in guacamole.properties
+ */
+ private Map<String, String> ldapAttrs;
+
+ /**
+ * Returns the lDAP attributes associated with this set of credentials.
+ * @return The LDAP attributes Map associated with this set of credentials,
+ * or null if no LDAP Attributes have been set.
+ */
+ public Map<String, String> getLDAPAttributes() {
+ return ldapAttrs;
+ }
+
+ /**
+ * Sets the LDAP attributes associated with this set of credentials.
+ * @param attributes The LDAP attributes to associate with this set of
+ * credentials.
+ */
+ public void setLDAPAttributes(Map<String, String> attributes) {
--- End diff --
> Better perhaps than that would be having AuthenticatedUser implement Attributes (as, semantically, it is the user that has these attributes, not their credentials).
Ah, yes, that makes more sense. Sorry, @jaredfrees, I may have steered you wrong, here.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194165752
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -236,6 +253,58 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Returns all custom LDAP attributes on the user currently bound under
+ * the given LDAP connection. The custom attributes are specified in
+ * guacamole.properties.
+ *
+ * @param ldapConnection
+ * LDAP connection to find the custom LDAP attributes.
+ * @param username
+ * The username of the user whose attributes are queried.
+ *
+ * @return
+ * All attributes on the user currently bound under the
+ * given LDAP connection, as a map of attribute name to
+ * corresponding attribute value.
+ *
+ * @throws LDAPException
+ * If an error occurs while searching for the user attributes.
+ *
+ * @throws GuacamoleException
+ * If an error occurs retrieving the user DN.
+ */
+ private Map<String, String> getLDAPAttributes(LDAPConnection ldapConnection,
+ String username) throws LDAPException, GuacamoleException {
+
+ // Get attributes from configuration information
+ List<String> attrList = confService.getAttributes();
+
+ // If there are no attributes there is no reason to search LDAP
+ if (attrList.size() == 0)
--- End diff --
Two things:
- You probably also need to check `if attrList == null` here, since the configuration service doesn't have a default and so could return `null` if the attribute is not specified. The `StringListProperty` returns `null` if the value is not specified, so this will like trigger a `NullPointerException`.
- You can simplify the above to `attrList.isEmpty()`
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194866630
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/AuthenticatedUser.java ---
@@ -53,6 +61,72 @@ public void init(Credentials credentials) {
setIdentifier(credentials.getUsername());
}
+ /**
+ * Get a map of attributes associated with this AuthenticatedUser.
+ *
+ * @return
+ * The Map of arbitrary attributes associated with this
+ * AuthenticatedUser object.
+ */
--- End diff --
Since this is overriding another method, I think it doesn't need the Javadocs, no?
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194521920
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/AuthenticatedUser.java ---
@@ -20,15 +20,19 @@
package org.apache.guacamole.auth.ldap.user;
import com.google.inject.Inject;
+import java.util.Map;
+import java.util.HashMap;
import org.apache.guacamole.net.auth.AbstractAuthenticatedUser;
+import org.apache.guacamole.net.auth.Attributes;
import org.apache.guacamole.net.auth.AuthenticationProvider;
import org.apache.guacamole.net.auth.Credentials;
/**
* An LDAP-specific implementation of AuthenticatedUser, associating a
* particular set of credentials with the LDAP authentication provider.
*/
-public class AuthenticatedUser extends AbstractAuthenticatedUser {
+public class AuthenticatedUser extends AbstractAuthenticatedUser
--- End diff --
The way this is implemented, here, these attributes (tokens) would not be available to connections stored in the JDBC module, since the module you've extended, here, is the LDAP-specific one. In order for this to cover multiple authentication modules, you'll either have to implement this at one of the parent classes (`AbstractAuthenticatedUser` perhaps) or across multiple modules. Of course, that doesn't all have to be done with this one pull request - this can just tackle LDAP for now - but the base changes that go into it probably need to at least make it available for other extensions to implement.
The other advantage of at least creating the abstract methods within one of the parent classes is avoiding having to do the type-casting when trying to run the `getAttributes()` method.
@mike-jumper Any pointers on the best place to put the various attribute-related stuff such that it can be easily used across different authentication extensions?
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194162564
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -26,12 +26,21 @@
import org.apache.guacamole.auth.ldap.user.AuthenticatedUser;
import org.apache.guacamole.auth.ldap.user.UserContext;
import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleServerException;
import org.apache.guacamole.auth.ldap.user.UserService;
import org.apache.guacamole.net.auth.Credentials;
import org.apache.guacamole.net.auth.credentials.CredentialsInfo;
import org.apache.guacamole.net.auth.credentials.GuacamoleInvalidCredentialsException;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import java.util.HashMap;
--- End diff --
Generally in the Guacamole code the imports are organized alphabetically, so these ones should probably go at the top.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/guacamole-client/pull/299
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r195112180
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -221,21 +230,80 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
throw new GuacamoleInvalidCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD);
try {
-
// Return AuthenticatedUser if bind succeeds
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
authenticatedUser.init(credentials);
+
+ // Set attributes
+ String username = credentials.getUsername();
+ Map<String, String> attrs = getLDAPAttributes(ldapConnection, username);
+ authenticatedUser.setAttributes(attrs);
+
return authenticatedUser;
}
-
// Always disconnect
finally {
ldapService.disconnect(ldapConnection);
}
}
+ /**
+ * Returns all custom LDAP attributes on the user currently bound under
+ * the given LDAP connection. The custom attributes are specified in
+ * guacamole.properties.
+ *
+ * @param ldapConnection
+ * LDAP connection to find the custom LDAP attributes.
+ *
+ * @param username
+ * The username of the user whose attributes are queried.
+ *
+ * @return
+ * All attributes on the user currently bound under the
+ * given LDAP connection, as a map of attribute name to
+ * corresponding attribute value.
+ *
+ * @throws LDAPException
--- End diff --
Only throws `GuacamoleException` so this should be removed.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194409880
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -236,6 +253,58 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Returns all custom LDAP attributes on the user currently bound under
+ * the given LDAP connection. The custom attributes are specified in
+ * guacamole.properties.
+ *
+ * @param ldapConnection
+ * LDAP connection to find the custom LDAP attributes.
+ * @param username
+ * The username of the user whose attributes are queried.
+ *
+ * @return
+ * All attributes on the user currently bound under the
+ * given LDAP connection, as a map of attribute name to
+ * corresponding attribute value.
+ *
+ * @throws LDAPException
+ * If an error occurs while searching for the user attributes.
+ *
+ * @throws GuacamoleException
+ * If an error occurs retrieving the user DN.
+ */
+ private Map<String, String> getLDAPAttributes(LDAPConnection ldapConnection,
+ String username) throws LDAPException, GuacamoleException {
+
+ // Get attributes from configuration information
+ List<String> attrList = confService.getAttributes();
+
+ // If there are no attributes there is no reason to search LDAP
+ if (attrList.size() == 0)
+ return null;
+
+ // Build LDAP query parameters
+ String[] attrArray = attrList.toArray(new String[attrList.size()]);
+ String userDN = getUserBindDN(username);
+
+ // Get LDAP attributes by querying LDAP
+ LDAPEntry userEntry = ldapConnection.read(userDN, attrArray);
+ LDAPAttributeSet attrSet = userEntry.getAttributeSet();
+
+ // Add each attribute into Map
+ Map<String, String> attrMap = new HashMap<String, String>();
+ Iterator attrIterator = attrSet.iterator();
+ while (attrIterator.hasNext()) {
--- End diff --
Oh, I see what you mean - yeah, that could be a problem, as those custom classes (particularly ancient ones like the JLDAP ones :smile:) don't always plan nice with the for-each loop.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194870376
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -189,7 +197,7 @@ private LDAPConnection bindAs(Credentials credentials)
/**
* Returns an AuthenticatedUser representing the user authenticated by the
- * given credentials.
+ * given credentials. Also adds custom LDAP attributes to credentials object.
--- End diff --
Might want to update this comment to reflect the change from storing in `Credentials` to `AuthenticatedUser`
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r195743264
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/RemoteAuthenticatedUser.java ---
@@ -67,6 +69,21 @@
*/
private static final Pattern X_FORWARDED_FOR = Pattern.compile("^" + IP_ADDRESS_REGEX + "(, " + IP_ADDRESS_REGEX + ")*$");
+ /**
+ * Arbitrary attributes associated with this RemoteAuthenticatedUser object.
+ */
+ private Map<String, String> attributes = new HashMap<String, String>();
--- End diff --
Yes, but `RemoteAuthenticatedUser` is abstract, so the `getAttributes()` and `setAttributes()` methods do not have to be implemented in it - they can be, as you've done, here, but they can also be implemented by all of the implementing classes. Obviously implementing them in `RemoteAuthenticatedUser` requires fewer lines of code, as then you don't have to implement in both `SharedAuthenticatedUser` and `ModeledAuthenticatedUser`, I'm just not sure how "clean" this is, or if the `RemoteAuthenticatedUser` class is really the right place for implementing these attribute handlers.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194880927
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/AuthenticatedUser.java ---
@@ -53,6 +61,59 @@ public void init(Credentials credentials) {
setIdentifier(credentials.getUsername());
}
+ @Override
+ public Map<String, String> getAttributes() {
+ return attributes;
+ }
+
+ @Override
+ public void setAttributes(Map<String, String> attributes) {
+ this.attributes = attributes;
+ }
+
+ /**
+ * Add the Map of attributes to the current set, without completely
+ * replacing the existing set. However, if duplicate keys exist the new
+ * values will replace any existing ones.
+ *
+ * @param attributes
+ * A Map of attributes to add to the existing attributes, without
+ * completely overwriting them.
+ */
+ public void addAttributes(Map<String, String> attributes) {
--- End diff --
I think this method, the `getAttribute(String key)` method, and the `setAttribute(String key, String value)` method can all be removed, right? They don't actually ever get called, it doesn't look like.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r195791116
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/ModeledAuthenticatedUser.java ---
@@ -26,6 +26,11 @@
import org.apache.guacamole.net.auth.AuthenticationProvider;
import org.apache.guacamole.net.auth.Credentials;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import java.util.Map;
--- End diff --
Looks like some left-overs from debugging?
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194527078
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -221,21 +229,79 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
throw new GuacamoleInvalidCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD);
try {
-
// Return AuthenticatedUser if bind succeeds
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
authenticatedUser.init(credentials);
+
+ //set attributes
+ String username = credentials.getUsername();
+ Map<String, String> attrs = getLDAPAttributes(ldapConnection, username);
+ authenticatedUser.setAttributes(attrs);
+
return authenticatedUser;
}
-
+ catch (LDAPException e) {
+ throw new GuacamoleServerException("Error while querying for User Attributes.", e);
+ }
// Always disconnect
finally {
ldapService.disconnect(ldapConnection);
}
}
+ /**
+ * Returns all custom LDAP attributes on the user currently bound under
+ * the given LDAP connection. The custom attributes are specified in
+ * guacamole.properties.
+ *
+ * @param ldapConnection
+ * LDAP connection to find the custom LDAP attributes.
+ * @param username
--- End diff --
Space between `@param` tags.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by jaredfrees <gi...@git.apache.org>.
Github user jaredfrees commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r195732201
--- Diff: extensions/guacamole-auth-jdbc/modules/guacamole-auth-jdbc-base/src/main/java/org/apache/guacamole/auth/jdbc/user/RemoteAuthenticatedUser.java ---
@@ -67,6 +69,21 @@
*/
private static final Pattern X_FORWARDED_FOR = Pattern.compile("^" + IP_ADDRESS_REGEX + "(, " + IP_ADDRESS_REGEX + ")*$");
+ /**
+ * Arbitrary attributes associated with this RemoteAuthenticatedUser object.
+ */
+ private Map<String, String> attributes = new HashMap<String, String>();
--- End diff --
The problem with the `RemoteAuthenticatedUser` is it implements AuthenticatedUser.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by jaredfrees <gi...@git.apache.org>.
Github user jaredfrees commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194885363
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/user/AuthenticatedUser.java ---
@@ -53,6 +61,59 @@ public void init(Credentials credentials) {
setIdentifier(credentials.getUsername());
}
+ @Override
+ public Map<String, String> getAttributes() {
+ return attributes;
+ }
+
+ @Override
+ public void setAttributes(Map<String, String> attributes) {
+ this.attributes = attributes;
+ }
+
+ /**
+ * Add the Map of attributes to the current set, without completely
+ * replacing the existing set. However, if duplicate keys exist the new
+ * values will replace any existing ones.
+ *
+ * @param attributes
+ * A Map of attributes to add to the existing attributes, without
+ * completely overwriting them.
+ */
+ public void addAttributes(Map<String, String> attributes) {
--- End diff --
I just added them because I thought they might be useful to someone else in the future, but yeah they don't ever get called.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194412245
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -236,6 +253,58 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Returns all custom LDAP attributes on the user currently bound under
+ * the given LDAP connection. The custom attributes are specified in
+ * guacamole.properties.
+ *
+ * @param ldapConnection
+ * LDAP connection to find the custom LDAP attributes.
+ * @param username
+ * The username of the user whose attributes are queried.
+ *
+ * @return
+ * All attributes on the user currently bound under the
+ * given LDAP connection, as a map of attribute name to
+ * corresponding attribute value.
+ *
+ * @throws LDAPException
+ * If an error occurs while searching for the user attributes.
+ *
+ * @throws GuacamoleException
+ * If an error occurs retrieving the user DN.
+ */
+ private Map<String, String> getLDAPAttributes(LDAPConnection ldapConnection,
+ String username) throws LDAPException, GuacamoleException {
+
+ // Get attributes from configuration information
+ List<String> attrList = confService.getAttributes();
+
+ // If there are no attributes there is no reason to search LDAP
+ if (attrList.size() == 0)
+ return null;
+
+ // Build LDAP query parameters
+ String[] attrArray = attrList.toArray(new String[attrList.size()]);
+ String userDN = getUserBindDN(username);
+
+ // Get LDAP attributes by querying LDAP
+ LDAPEntry userEntry = ldapConnection.read(userDN, attrArray);
+ LDAPAttributeSet attrSet = userEntry.getAttributeSet();
+
+ // Add each attribute into Map
+ Map<String, String> attrMap = new HashMap<String, String>();
+ Iterator attrIterator = attrSet.iterator();
+ while (attrIterator.hasNext()) {
--- End diff --
Yep, that's fine. And, depending on what you're doing with it later, you might not even need that assignment, there, you could do:
String attrName = ((LDAPAttribute) attrObj).getName();
String attrValue = ((LDAPAttribute) attrObj).getStringValue();
I'm not certain what penalty there is, if any, to doing the cast twice like that, so it may actually make sense to go ahead and assign it to a new `LDAPAttribute` type. Either way is probably perfectly fine.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by jaredfrees <gi...@git.apache.org>.
Github user jaredfrees commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194407598
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -236,6 +253,58 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Returns all custom LDAP attributes on the user currently bound under
+ * the given LDAP connection. The custom attributes are specified in
+ * guacamole.properties.
+ *
+ * @param ldapConnection
+ * LDAP connection to find the custom LDAP attributes.
+ * @param username
+ * The username of the user whose attributes are queried.
+ *
+ * @return
+ * All attributes on the user currently bound under the
+ * given LDAP connection, as a map of attribute name to
+ * corresponding attribute value.
+ *
+ * @throws LDAPException
+ * If an error occurs while searching for the user attributes.
+ *
+ * @throws GuacamoleException
+ * If an error occurs retrieving the user DN.
+ */
+ private Map<String, String> getLDAPAttributes(LDAPConnection ldapConnection,
+ String username) throws LDAPException, GuacamoleException {
+
+ // Get attributes from configuration information
+ List<String> attrList = confService.getAttributes();
+
+ // If there are no attributes there is no reason to search LDAP
+ if (attrList.size() == 0)
+ return null;
+
+ // Build LDAP query parameters
+ String[] attrArray = attrList.toArray(new String[attrList.size()]);
+ String userDN = getUserBindDN(username);
+
+ // Get LDAP attributes by querying LDAP
+ LDAPEntry userEntry = ldapConnection.read(userDN, attrArray);
+ LDAPAttributeSet attrSet = userEntry.getAttributeSet();
+
+ // Add each attribute into Map
+ Map<String, String> attrMap = new HashMap<String, String>();
+ Iterator attrIterator = attrSet.iterator();
+ while (attrIterator.hasNext()) {
--- End diff --
the reason I did this is because I couldn't get the casting to work in the for-each loop, but I think I got the for-each loop working now.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194869441
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/token/StandardTokens.java ---
@@ -21,9 +21,10 @@
import java.text.SimpleDateFormat;
import java.util.Date;
+import java.util.Map;
+import java.util.Set;
import org.apache.guacamole.net.auth.AuthenticatedUser;
import org.apache.guacamole.net.auth.Credentials;
-
--- End diff --
Add this line back in - style-wise, there should be a line between the imports and the class documentation.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194865378
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/ConfigurationService.java ---
@@ -344,4 +344,20 @@ public int getOperationTimeout() throws GuacamoleException {
);
}
+ /**
+ * Returns names for custom LDAP user attributes.
+ *
+ * @return
+ * LDAP user attributes as defined in the guacamole.properties file
+ * as ldap-user-attributes: ''
--- End diff --
This comment just strikes me as a little odd - I would drop the example of its guacamole.properties definition and just allow that to be documented in the manual and make this more like the other `@return` tags for other properties.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r195711510
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/token/StandardTokens.java ---
@@ -74,6 +76,11 @@
*/
private static final String TIME_FORMAT = "HHmmss";
+ /**
+ * The prefix of the arbitrary attribute tokens.
+ */
+ public static final String ATTR_TOKEN_PREFIX = "GUAC_ATTR:";
--- End diff --
Okay, one of the things I figured out is that the RegEx for tokens won't currently match on ":", so, either one of two things will need to be done:
- Modify the RegEx in `TokenFilter` and add the match for ":" to that.
- Change this so that it fits into the current RegEx, which would probably mean using "GUAC_ATTR_" and then the tokens become GUAC_ATTR_MAIL, GUAC_ATTR_WORKSTATIONNAME, etc.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194164950
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -236,6 +253,58 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Returns all custom LDAP attributes on the user currently bound under
+ * the given LDAP connection. The custom attributes are specified in
+ * guacamole.properties.
+ *
+ * @param ldapConnection
+ * LDAP connection to find the custom LDAP attributes.
+ * @param username
+ * The username of the user whose attributes are queried.
+ *
+ * @return
+ * All attributes on the user currently bound under the
+ * given LDAP connection, as a map of attribute name to
+ * corresponding attribute value.
+ *
+ * @throws LDAPException
+ * If an error occurs while searching for the user attributes.
+ *
+ * @throws GuacamoleException
+ * If an error occurs retrieving the user DN.
+ */
+ private Map<String, String> getLDAPAttributes(LDAPConnection ldapConnection,
+ String username) throws LDAPException, GuacamoleException {
--- End diff --
Maybe @mike-jumper has opinions about this, but I'd suggest only throwing the `GuacamoleException` in this method and catching the `LDAPException`, here, and re-throwing it as a `GuacamoleException` (`GuacamoleServerException`, prehaps). This also clears up the nested try above...
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194167124
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -236,6 +253,58 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
}
+ /**
+ * Returns all custom LDAP attributes on the user currently bound under
+ * the given LDAP connection. The custom attributes are specified in
+ * guacamole.properties.
+ *
+ * @param ldapConnection
+ * LDAP connection to find the custom LDAP attributes.
+ * @param username
+ * The username of the user whose attributes are queried.
+ *
+ * @return
+ * All attributes on the user currently bound under the
+ * given LDAP connection, as a map of attribute name to
+ * corresponding attribute value.
+ *
+ * @throws LDAPException
+ * If an error occurs while searching for the user attributes.
+ *
+ * @throws GuacamoleException
+ * If an error occurs retrieving the user DN.
+ */
+ private Map<String, String> getLDAPAttributes(LDAPConnection ldapConnection,
+ String username) throws LDAPException, GuacamoleException {
+
+ // Get attributes from configuration information
+ List<String> attrList = confService.getAttributes();
+
+ // If there are no attributes there is no reason to search LDAP
+ if (attrList.size() == 0)
+ return null;
+
+ // Build LDAP query parameters
+ String[] attrArray = attrList.toArray(new String[attrList.size()]);
+ String userDN = getUserBindDN(username);
+
+ // Get LDAP attributes by querying LDAP
+ LDAPEntry userEntry = ldapConnection.read(userDN, attrArray);
+ LDAPAttributeSet attrSet = userEntry.getAttributeSet();
+
+ // Add each attribute into Map
+ Map<String, String> attrMap = new HashMap<String, String>();
+ Iterator attrIterator = attrSet.iterator();
+ while (attrIterator.hasNext()) {
--- End diff --
Might use the Java for-each loop, here - you used it below when dealing with the LDAP attributes in the token - no reason not to use it, here, too! :smile:
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194169785
--- Diff: guacamole-ext/src/main/java/org/apache/guacamole/token/StandardTokens.java ---
@@ -74,6 +78,12 @@
*/
private static final String TIME_FORMAT = "HHmmss";
+ /**
+ * Standard prefix to append to beginning of the name of each custom
+ * LDAP attribute before adding attributes as tokens.
+ */
+ private static final String LDAP_ATTR_PREFIX = "USER_ATTR:";
--- End diff --
As with @mike-jumper's suggestion above, I'd say make this prefix more generic.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r197091333
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -221,21 +231,77 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
throw new GuacamoleInvalidCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD);
try {
-
// Return AuthenticatedUser if bind succeeds
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
authenticatedUser.init(credentials);
+
+ // Set attributes
+ String username = credentials.getUsername();
+ Map<String, String> attrs = getLDAPAttributes(ldapConnection, username);
+ authenticatedUser.setAttributes(attrs);
--- End diff --
Doesn't seem to be much reason for the `attrs` object, here, or `username` - probably worth squashing these three lines down.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r197094045
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPGuacamoleProperties.java ---
@@ -205,4 +205,14 @@ private LDAPGuacamoleProperties() {}
};
+ /**
+ * Custom attribute or attributes in Guacamole user's record in the
--- End diff --
Maybe something more like:
> Custom attribute or attributes to query from Guacamole user's record...
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
Posted by necouchman <gi...@git.apache.org>.
Github user necouchman commented on a diff in the pull request:
https://github.com/apache/guacamole-client/pull/299#discussion_r194879510
--- Diff: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java ---
@@ -221,21 +230,80 @@ public AuthenticatedUser authenticateUser(Credentials credentials)
throw new GuacamoleInvalidCredentialsException("Permission denied.", CredentialsInfo.USERNAME_PASSWORD);
try {
-
// Return AuthenticatedUser if bind succeeds
AuthenticatedUser authenticatedUser = authenticatedUserProvider.get();
authenticatedUser.init(credentials);
+
+ // Set attributes
+ String username = credentials.getUsername();
+ Map<String, String> attrs = getLDAPAttributes(ldapConnection, username);
+ authenticatedUser.setAttributes(attrs);
+
return authenticatedUser;
}
-
// Always disconnect
finally {
ldapService.disconnect(ldapConnection);
}
}
+ /**
+ * Returns all custom LDAP attributes on the user currently bound under
+ * the given LDAP connection. The custom attributes are specified in
+ * guacamole.properties.
+ *
+ * @param ldapConnection
+ * LDAP connection to find the custom LDAP attributes.
+ *
+ * @param username
+ * The username of the user whose attributes are queried.
+ *
+ * @return
+ * All attributes on the user currently bound under the
+ * given LDAP connection, as a map of attribute name to
+ * corresponding attribute value.
+ *
+ * @throws LDAPException
+ * If an error occurs while searching for the user attributes.
+ *
+ * @throws GuacamoleException
+ * If an error occurs retrieving the user DN.
--- End diff --
...or an error occurs retrieving the attributes?
---