necouchman commented on code in PR #987:
URL: https://github.com/apache/guacamole-client/pull/987#discussion_r1603384590
##########
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##########
@@ -354,14 +425,19 @@ private Map<String, String>
getAttributeTokens(ConnectedLDAPConfiguration config
// Convert each retrieved attribute into a corresponding token
for (Attribute attr : attributes) {
tokens.put(TokenName.canonicalize(attr.getId(),
- LDAP_ATTRIBUTE_TOKEN_PREFIX), attr.getString());
+ LDAP_TOKEN_PREFIX), attr.getString());
}
-
}
catch (LdapException e) {
throw new GuacamoleServerException("Could not query LDAP user
attributes.", e);
}
+ // Extract the domain (ie: Windows / Active Directory domain) from the
+ // user's credentials
+ String domainName = getUserDomain(credentials);
Review Comment:
What happens, here, if the `assert()` in line 367 gets triggered? Will the
`GuacamoleException` that this method throws get triggered? Or does it make
sense to do something specific to capture the `AssertionError` that will get
thrown by the `getUserDomain()` method?
##########
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##########
@@ -308,25 +339,65 @@ public LDAPAuthenticatedUser authenticateUser(Credentials
credentials)
}
/**
- * Returns parameter tokens generated from LDAP attributes on the user
- * currently bound under the given LDAP connection. The attributes to be
- * converted into parameter tokens must be explicitly listed in
- * guacamole.properties. If no attributes are specified or none are
- * found on the LDAP user object, an empty map is returned.
+ * Returns the Windows / Active Directory domain included in the username
+ * of from the provided user credentials. If the username does not contain
Review Comment:
Either the `of` or `from` is extra, here.
##########
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##########
@@ -63,7 +65,36 @@ public class AuthenticationProviderService {
/**
* The prefix that will be used when generating tokens.
*/
- public static final String LDAP_ATTRIBUTE_TOKEN_PREFIX = "LDAP_";
+ public static final String LDAP_TOKEN_PREFIX = "LDAP_";
+
+ /**
+ * Regular expression that extracts the Windows / Active Directory domain
+ * from a username in either of the following formats: down-level logon
+ * ("DOMAIN\\username") or UPN ("username@domain"). If the username is in
+ * "DOMAIN\\username" format, the domain will be stored in the first
Review Comment:
My experience with Windows is that the format is `DOMAIN\username` (with a
single back-slash), and the double-backslash is only required to escape the
single back-slash on non-Windows (UNIX, Linux, BSD - you know, standardized)
platforms. It might be good if we clarify what our expectation is for those
usernames being processed - do we expect that users will see/enter a
double-backslash? Or do we expect we'll get it from LDAP with the escaped
backslash?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
