[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
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...
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...
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...
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...
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 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...
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...
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...
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...
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 attributes = new HashMap();
--- 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...
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 attributes = new HashMap();
--- 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...
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 attributes = new HashMap();
--- End diff --
and currently `AuthenticatedUser` extends `Attributes`
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
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...
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 attributes = new HashMap();
--- End diff --
The problem with the `RemoteAuthenticatedUser` is it implements
AuthenticatedUser.
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
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...
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 attributes = new HashMap();
--- 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...
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...
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 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...
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 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...
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 getAttributes() {
+return attributes;
+}
+
+@Override
+public void setAttributes(Map 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 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...
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 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?
---
[GitHub] guacamole-client pull request #299: GUACAMOLE-524: Allow custom LDAP attribu...
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 getAttributes() {
+return attributes;
+}
+
+@Override
+public void setAttributes(Map 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 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...
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 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...
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 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...
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...
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...
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...
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...
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...
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 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...
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 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 getLDAPAttributes(LDAPConnection
ldapConnection,
+String username) throws GuacamoleException {
+
+// Get attributes from configuration information
+List 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 attrMap = new HashMap();
+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...
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 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...
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...
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...
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...
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...
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 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...
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 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 getLDAPAttributes(LDAPConnection
ldapConnection,
+String username) throws LDAPException {
+
+// Get attributes from configuration information
+List 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...
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...
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 getLDAPAttributes(LDAPConnection
ldapConnection,
+String username) throws LDAPException, GuacamoleException {
+
+// Get attributes from configuration information
+List 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 attrMap = new HashMap();
+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...
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 getLDAPAttributes(LDAPConnection
ldapConnection,
+String username) throws LDAPException, GuacamoleException {
+
+// Get attributes from configuration information
+List 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 attrMap = new HashMap();
+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...
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 getLDAPAttributes(LDAPConnection
ldapConnection,
+String username) throws LDAPException, GuacamoleException {
+
+// Get attributes from configuration information
+List 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 attrMap = new HashMap();
+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...
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 getLDAPAttributes(LDAPConnection
ldapConnection,
+String username) throws LDAPException, GuacamoleException {
+
+// Get attributes from configuration information
+List 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 attrMap = new HashMap();
+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...
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 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...
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...
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...
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...
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 getLDAPAttributes(LDAPConnection
ldapConnection,
+String username) throws LDAPException, GuacamoleException {
+
+// Get attributes from configuration information
+List 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...
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 getLDAPAttributes(LDAPConnection
ldapConnection,
+String username) throws LDAPException, GuacamoleException {
+
+// Get attributes from configuration information
+List 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 attrMap = new HashMap();
+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...
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...
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 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 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 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...
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 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 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 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...
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 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 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 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 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. ---
