mike-jumper commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1524047638
##########
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##########
@@ -307,16 +312,39 @@ public LDAPAuthenticatedUser authenticateUser(Credentials
credentials)
}
+ /**
+ * Returns parameter current ldap domain token generated from user
credentials
+ * @param credentials
+ * The credentials used for authentication.
+ *
+ * @return
+ * Domain name by splitting login username or null if no domain is
detected.
+ */
+ private String getDomainToken(Credentials credentials) {
+ String ldapDomainName = null;
+ if (credentials.getUsername().contains("\\")) {
+ ldapDomainName = credentials.getUsername().split("\\\\")[0];
+ }
+ else if (credentials.getUsername().contains("@")) {
+ ldapDomainName = credentials.getUsername().split("@")[1];
+ }
+ return ldapDomainName;
+ }
Review Comment:
Hearkening back to the initial review, I'm still on the fence about this for
the same reason:
> I don't think ... that we should make specific assumptions about the
format here when the LDAP handling of username format is so flexible.
That said, I do see the utility of having this sort of token just be
available, given that this sort of matching is going to be so common.
If we're going to have a `${LDAP_DOMAIN}` token that is always defined for
any successful login that follows the `DOMAIN\username` or `username@domain`
formats, I think we should do this parsing a bit more efficiently. Currently,
the algorithm here is:
1. Search the username string for the first occurrence of a `\` character.
2. If such a character is found, search the username string again, this time
splitting the string into an array. Discard everything but the first element in
that array.
3. Search the username string yet again, this time for the first occurrence
of a `@` character.
4. If such a character is found, search the username string _again_, this
time splitting the string into an array. Discard everything but the first
element in that array.
This is suboptimal, as:
* It potentially searches the username string 4 times.
* It unnecessarily splits the string into an array, rather than just parsing
the interesting part of the string directly.
Instead, I'd recommend defining and using a `Pattern` that matches _either_
of these formats, extracting the domain. This would reduce things down to a
single operation that performs only one search and directly extracts the domain
at the same time.
##########
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##########
@@ -356,6 +384,11 @@ private Map<String, String>
getAttributeTokens(ConnectedLDAPConfiguration config
tokens.put(TokenName.canonicalize(attr.getId(),
LDAP_ATTRIBUTE_TOKEN_PREFIX), attr.getString());
}
+ String domainName = getDomainToken(credentials);
+ if (domainName != null) {
+ String tokenName = TokenName.canonicalize(LDAP_DOMAIN_TOKEN,
LDAP_ATTRIBUTE_TOKEN_PREFIX);
+ tokens.put(tokenName, domainName);
+ }
Review Comment:
This shouldn't be within the `try`/`catch` block that relates to LDAP errors
that may be encountered while retrieving attributes, as it's entirely
independent of all that. For readability, it should also be visibly separated
from the block that precedes it, which is already documented as having a
different purpose.
I'd recommend just moving this outside the `try`/`catch`, making sure to add:
* A blank line to visibly separate the block from everything else
* A high-level comment covering the intent of the block.
##########
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##########
@@ -65,6 +65,11 @@ public class AuthenticationProviderService {
*/
public static final String LDAP_ATTRIBUTE_TOKEN_PREFIX = "LDAP_";
+ /**
+ * The name of LDAP domain attribute
Review Comment:
JavaDoc standards require that the bodies of JavaDoc comments end with
periods.
##########
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##########
@@ -326,7 +354,7 @@ public LDAPAuthenticatedUser authenticateUser(Credentials
credentials)
* @throws GuacamoleException
* If an error occurs retrieving the user DN or the attributes.
*/
- private Map<String, String> getAttributeTokens(ConnectedLDAPConfiguration
config)
+ private Map<String, String> getAttributeTokens(ConnectedLDAPConfiguration
config, Credentials credentials)
Review Comment:
This function is specifically documented as:
> Returns parameter tokens generated from LDAP attributes on the user
currently bound under the given LDAP connection.
and is specifically named `getAttributeTokens()`. So long as that's the
case, it's bad practice to let injection of an entirely different kind of token
be a side-effect of this function.
I do see that the documentation here has been updated to clarify this a bit,
however that ultimately resulted in the following statement:
> The attributes to be converted into parameter tokens must be explicitly
listed in guacamole.properties or domain name of the LDAP connection when
multiple auth enabled.
As-written, this does not make sense to me, and the original issues with
primary documented scope and the naming of the function remain. If this
function is to serve as the basis for injection of the `LDAP_DOMAINS` token
(which I agree may make sense), then:
* The function should be renamed to capture its now-broadened scope
(`getAttributeTokens()` is no longer correct).
* The function's documentation should be updated to capture that scope
("Returns parameter tokens generated from LDAP attributes" is no longer
correct, and other documented aspects may also need adjusting).
Makes sense?
For example, if the function were renamed to something like
`getUserTokens()`, the following might be reasonable:
> Returns parameter tokens generated based on details specific to the user
currently bound under the given LDAP connection. Both LDAP attributes on the
user's associated LDAP object and the credentials submitted by the user to
Guacamole are considered. If any tokens are to be derived from LDAP attributes,
those attributes must be explicitly listed in guacamole.properties. If no
tokens are applicable, an empty map is returned.
##########
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##########
@@ -307,16 +312,39 @@ public LDAPAuthenticatedUser authenticateUser(Credentials
credentials)
}
+ /**
+ * Returns parameter current ldap domain token generated from user
credentials
+ * @param credentials
+ * The credentials used for authentication.
Review Comment:
To comply with JavaDoc standards and the code style we use for comments,
this should be formatted as:
```
/**
* Returns parameter current ldap domain token generated from user
credentials.
*
* @param credentials
* The credentials used for authentication.
```
That said, the phrase "Returns parameter current ldap domain token generated
from user credentials" does not make sense as written. It should probably be
rephrased.
--
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]
