Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
necouchman commented on PR #931: URL: https://github.com/apache/guacamole-client/pull/931#issuecomment-2041657719 @josnabattula You need to remove the merge commits from this PR - there are currently two: ca241ab2b2c17a198f7f312fb2c38d36eb6064c0 26f21166a3883624b5d83fa864a7c17a743fa5dc Also, you might want to consider squashing the others together into a smaller number - 9 commits seems a bit much for 60 lines of code change ;-). -- 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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
necouchman commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1554792021
##
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##
@@ -28,6 +28,9 @@
import java.util.List;
import java.util.Map;
import java.util.Set;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
Review Comment:
Please remove this extra line -we do not split `import` lines.
##
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##
@@ -308,25 +316,49 @@ 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 current LDAP domain token from the provided 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 username = credentials.getUsername();
+Pattern pattern = Pattern.compile("^(.+).*$|^.*@(.+)$");
Review Comment:
Might be nice to document what you're doing, here - regular expressions can
be obscure enough that it isn't obvious what this RegEx does, and would benefit
from a one line description of it.
##
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##
@@ -308,25 +316,49 @@ 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 current LDAP domain token from the provided 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 username = credentials.getUsername();
+Pattern pattern = Pattern.compile("^(.+).*$|^.*@(.+)$");
+Matcher matcher = pattern.matcher(username);
+if (matcher.find()) {
+return matcher.group(1) != null ? matcher.group(1) :
matcher.group(2);
+}
+return null;
+}
+
+/**
+ * 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.
*
* @param config
* The configuration of the LDAP server being queried.
*
+ * @param credentials
+ * The credentials to use for authentication.
+ *
* @return
- * A map of parameter tokens generated from attributes on the user
- * currently bound under the given LDAP connection, as a map of token
- * name to corresponding value, or an empty map if no attributes are
- * specified or none are found on the user object.
+ * A map of parameter tokens. These tokens are generated based on
+ * the attributes of the user currently bound under the given LDAP
connection
+ * and the user's credentials. The map's keys are the canonicalized
attribute
+ * names with an "LDAP_" prefix, and the values are the corresponding
attribute
+ * values. If the domain name is extracted from the user's
credentials, it is added
+ * to the map with the key "LDAP_DOMAIN". If no applicable tokens are
found,
+ * the method returns an empty map.
Review Comment:
Looks like these lines are all indented by an extra space?
##
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##
@@ -308,25 +316,49 @@ public LDAPAuthenticatedUser authenticateUser(Credentials
credentials)
}
/**
- * Returns parameter tokens gener
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula commented on PR #931: URL: https://github.com/apache/guacamole-client/pull/931#issuecomment-2033582166 @mike-jumper I have resolved your comments, could you review when you get a minute. Thanks. -- 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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1525573729
##
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:
Agree, I will do that.
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
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
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 getAttributeTokens(ConnectedLDAPConfiguration
config)
+private Map getAttributeTokens(ConnectedLDAPConfiguration
config, Credentials credentials)
Review Comment:
This function is specifically documented as:
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula commented on PR #931: URL: https://github.com/apache/guacamole-client/pull/931#issuecomment-1987383120 @necouchman if you have a minute, could you have look over this again, resolved all comments. Good to get this merged, waiting for this feature for a wee bit of time. -- 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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula commented on PR #931: URL: https://github.com/apache/guacamole-client/pull/931#issuecomment-1981797178 @necouchman I have resolved your comments. please have a review. thanks. -- 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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula commented on code in PR #931: URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1513567479 ## extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java: ## @@ -306,17 +311,43 @@ public LDAPAuthenticatedUser authenticateUser(Credentials credentials) } } +/** + * Returns parameter current ldap domain token generated from user credentials + * If no multiple LDAP are configured on GUACAMOLE_HOME such ldap-servers.yaml, + * a null is returned. Review Comment: Yep, I have updated code comments in code to be more clear as it works when there are situations where single LDAP configuration could return domain name if exists otherwise null. -- 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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
necouchman commented on code in PR #931: URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1510407891 ## extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java: ## @@ -306,17 +311,43 @@ public LDAPAuthenticatedUser authenticateUser(Credentials credentials) } } +/** + * Returns parameter current ldap domain token generated from user credentials + * If no multiple LDAP are configured on GUACAMOLE_HOME such ldap-servers.yaml, + * a null is returned. Review Comment: Is this actually the way it works - if multiple LDAP servers/domains are not enabled, will it _always_ return `null`? Or are there situations where a single LDAP configuration could still result in a value being returned by this function - such as if the `userPrincipalName` attribute is used when talking to Active Directory? -- 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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula commented on PR #931: URL: https://github.com/apache/guacamole-client/pull/931#issuecomment-1975369940 @necouchman @mike-jumper I have resolved all the comments. Can I get your tick? -- 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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula commented on PR #931: URL: https://github.com/apache/guacamole-client/pull/931#issuecomment-1935292080 @necouchman I have resolved your comments. -- 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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1483814298
##
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##
@@ -306,17 +306,42 @@ public LDAPAuthenticatedUser authenticateUser(Credentials
credentials)
}
}
+/**
+ * Returns parameter current ldap domain token generated from user
credentials
+ * If no multiple LDAP are configured on GUACAMOLE_HOME such
ldap-servers.yaml,
+ * a null is returned.
+ *
+ * @param credentials
+ * The credentials to use for authentication.
+ *
+ * @return
+ * Domain name by splitting logged username(domain/username) when
multiple LDAP configuration is available
+ * or null if no such configuration
+ */
+private String getDomainToken(Credentials credentials) {
+String ldapDomainName = null;
+if (credentials.getUsername().contains("\\")) {
+ldapDomainName = credentials.getUsername().split("")[0];
+} else if (credentials.getUsername().contains("@")) {
Review Comment:
Thanks for response, figured out :)
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
necouchman commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1483812180
##
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##
@@ -306,17 +306,42 @@ public LDAPAuthenticatedUser authenticateUser(Credentials
credentials)
}
}
+/**
+ * Returns parameter current ldap domain token generated from user
credentials
+ * If no multiple LDAP are configured on GUACAMOLE_HOME such
ldap-servers.yaml,
+ * a null is returned.
+ *
+ * @param credentials
+ * The credentials to use for authentication.
+ *
+ * @return
+ * Domain name by splitting logged username(domain/username) when
multiple LDAP configuration is available
+ * or null if no such configuration
+ */
+private String getDomainToken(Credentials credentials) {
+String ldapDomainName = null;
+if (credentials.getUsername().contains("\\")) {
+ldapDomainName = credentials.getUsername().split("")[0];
+} else if (credentials.getUsername().contains("@")) {
Review Comment:
@josnabattula Sure:
`} else if (something else) {`
This is known as "cuddling" tags. Code style for this project is:
```
}
else if (something else) {
```
Where the `}` is not on the same line as the `else if` statement.
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1483809679
##
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##
@@ -306,17 +306,42 @@ public LDAPAuthenticatedUser authenticateUser(Credentials
credentials)
}
}
+/**
+ * Returns parameter current ldap domain token generated from user
credentials
+ * If no multiple LDAP are configured on GUACAMOLE_HOME such
ldap-servers.yaml,
+ * a null is returned.
+ *
+ * @param credentials
+ * The credentials to use for authentication.
+ *
+ * @return
+ * Domain name by splitting logged username(domain/username) when
multiple LDAP configuration is available
+ * or null if no such configuration
+ */
+private String getDomainToken(Credentials credentials) {
+String ldapDomainName = null;
+if (credentials.getUsername().contains("\\")) {
+ldapDomainName = credentials.getUsername().split("")[0];
+} else if (credentials.getUsername().contains("@")) {
Review Comment:
@necouchman Could you give us more context, couldn't figure out what your
comments is. thanks.
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
necouchman commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1483790438
##
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##
@@ -356,6 +381,11 @@ private Map
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("domain_name",
LDAP_ATTRIBUTE_TOKEN_PREFIX);
Review Comment:
`domain_name` should probably get a constant.
##
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##
@@ -306,17 +306,42 @@ public LDAPAuthenticatedUser authenticateUser(Credentials
credentials)
}
}
+/**
+ * Returns parameter current ldap domain token generated from user
credentials
+ * If no multiple LDAP are configured on GUACAMOLE_HOME such
ldap-servers.yaml,
+ * a null is returned.
+ *
+ * @param credentials
+ * The credentials to use for authentication.
+ *
+ * @return
+ * Domain name by splitting logged username(domain/username) when
multiple LDAP configuration is available
+ * or null if no such configuration
+ */
+private String getDomainToken(Credentials credentials) {
+String ldapDomainName = null;
+if (credentials.getUsername().contains("\\")) {
+ldapDomainName = credentials.getUsername().split("")[0];
+} else if (credentials.getUsername().contains("@")) {
Review Comment:
Please follow established style guidelines - in particular, don't "cuddle"
the tags.
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula commented on PR #931: URL: https://github.com/apache/guacamole-client/pull/931#issuecomment-1935144138 @mike-jumper @necouchman could you look into my pull request, as I have move the changes into LDAP module as per the comments. -- 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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
necouchman commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1454061663
##
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java:
##
@@ -306,6 +311,26 @@ public LDAPAuthenticatedUser authenticateUser(Credentials
credentials)
}
}
+/**
+ * Returns parameter current ldap domain token generated from user
credentials
+ * If no multiple LDAP are configured on GUACAMOLE_HOME
+ * a null is returned.
+ *
+ * @param credentials
+ * The credentials to use for authentication.
+ *
+ * @return
+ * Domain name by splitting logged username when multiple LDAP
configuration is available
+ * or null if no such configuration
+ */
+private String getDomainToken(Credentials credentials) {
+ String ldapDomainName = null;
+// Creating custom LDAP attribute token - domain name - when
configured to multiple LDAP
+if (credentials.getUsername().contains("\\")) {
Review Comment:
I think we should probably deal with situations where the username is in the
UPN format, which is `USERNAME`@`DOMAIN`?
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula closed pull request #931: GUACAMOLE-1881: Adding new standard tokens userid and domain name URL: https://github.com/apache/guacamole-client/pull/931 -- 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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
fanovilla commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1411696129
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
I'm quite full-on now for the rest of the year, will leave it for you to
pickup in January.
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1409201752
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
Yep, @fanovilla you can.
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1409201752
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
Yep, you can.
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
fanovilla commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1398536854
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
thanks @mike-jumper ; @josnabattula not sure how but let me know if you want
me to try contribute to your PR branch..
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
mike-jumper commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1396549672
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
@fanovilla Yes, that function (`getAttributeTokens()`) is a reasonable place
to add LDAP-specific tokens. If you do so, the name and documentation for that
function would need to be adjusted, as it currently states:
> Returns parameter tokens generated from LDAP attributes on the user
currently bound under the given LDAP connection. ...
If a standard token for the user identifier is being added, as well,
`StandardTokenMap` is correct for that.
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
fanovilla commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1395110120
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
You're right @necouchman , main intent is to expose a token (agree should be
LDAP specific) containing only the domain, that can be used in connection
definitions.
> We've provided a mechanism within the LDAP module (and most of the other
modules, particularly SSO) to define tokens specific to those modules, but that
should be usable in connections stored in other modules.
To do that, I presume we can add tokens to what's already being collected
here?https://github.com/apache/guacamole-client/blob/284d1b24f37f06d380312e149d24d96497dd9af4/extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java#L355-L358
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
fanovilla commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1395110120
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
You're right @necouchman , main intent is to expose a token (agree should be
LDAP specific) containing only the domain (and another containing only the
username) that can be used in connection definitions.
> We've provided a mechanism within the LDAP module (and most of the other
modules, particularly SSO) to define tokens specific to those modules, but that
should be usable in connections stored in other modules.
To do that, I presume we can add tokens to what's already being collected
here?https://github.com/apache/guacamole-client/blob/284d1b24f37f06d380312e149d24d96497dd9af4/extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java#L355-L358
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
mike-jumper commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1394980110
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
> `authenticatedUser.getIdentifier()` only gives us which authentication
mechanism i have used for my guacamole instance.
What do you mean? The `AuthenticatedUser` is the object defining the user's
authenticated identity, with `getIdentifier()` returning the unique identifier
for that identity. In the case of LDAP, that is the username. This is exactly
the value that is returned as the username to be exposed in the UI:
https://github.com/apache/guacamole-client/blob/284d1b24f37f06d380312e149d24d96497dd9af4/guacamole/src/main/java/org/apache/guacamole/rest/auth/TokenRESTService.java#L188-L194
Providing the identifier as a token would allow the relevant username to be
included in the configuration independently of what the user enters in a login
form.
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
mike-jumper commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1394980110
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
> `authenticatedUser.getIdentifier()` only gives us which authentication
mechanism i have used for my guacamole instance.
What do you mean? The `AuthenticatedUser` is the object defining the user's
authenticated identity, with `getIdentifier()` returning the unique identifier
for that identity. In the case of LDAP, that is the username. This is exactly
the value that is returned as the username to be exposed in the UI:
https://github.com/apache/guacamole-client/blob/284d1b24f37f06d380312e149d24d96497dd9af4/guacamole/src/main/java/org/apache/guacamole/rest/auth/TokenRESTService.java#L188C1-L194
Providing the identifier as a token would allow the relevant username to be
included in the configuration independently of what the user enters in a login
form.
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1394850456
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
`if the domain portion is desired/required, it would need to be something
LDAP-specific` - Yes these variables we are looking are LDAP-specific.
`authenticatedUser.getIdentifier()` only gives us which authentication
mechanism i have used for my guacamole instance.
Also In the instance multiple LDAP configurations with `match-usernames`
`${GUAC_USERNAME` becomes `domain\username` which is not ideal for RDP
connections. Check below some parts of actual error
```
Failure Reason: Unknown user name or bad password.
Status: 0xC06D
Sub Status: 0xC064
Process Information:
Caller Process ID: 0x0
Caller Process Name:-
Detailed Authentication Information:
Logon Process: NtLmSsp
Authentication Package: NTLM
Transited Services: -
Package Name (NTLM only): -
Key Length: 0
```
It would be ideal to have DOMAIN and just user name which are again LDAP
sepcific, it makes easy for connection configuration without any hassle.
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
josnabattula commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1394850456
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
`if the domain portion is desired/required, it would need to be something
LDAP-specific` - Yes these variables we are looking are LDAP-specific.
`authenticatedUser.getIdentifier()` only gives us which authentication
mechanism i have used for my guacamole instance.
Also In the instance multiple LDAP configurations with `match-usernames`
`${GUAC_USERNAME` becomes `domain\username` which is not ideal for RDP
connections. Check below some parts error
```
Failure Reason: Unknown user name or bad password.
Status: 0xC06D
Sub Status: 0xC064
Process Information:
Caller Process ID: 0x0
Caller Process Name:-
Detailed Authentication Information:
Logon Process: NtLmSsp
Authentication Package: NTLM
Transited Services: -
Package Name (NTLM only): -
Key Length: 0
```
It would be ideal to have DOMAIN and just user name which are again LDAP
sepcific, it makes easy for connection configuration without any hassle.
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
necouchman commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1394541107
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
> If there is to be a new standard token, I think there's some value in
having one token (GUAC_IDENTIFIER? GUAC_USER_IDENTIFIER?) that represents
validated identity while the other (GUAC_USERNAME) represents the username
provided during the auth process, if any.
Sure, that makes sense - it doesn't resolve the issue of getting the domain
portion out of the overall username, so if there's a need to have the domain,
that would still have to be handled - which seems more appropriate to be
LDAP-specific.
> I think that much is already possible with the functionality provided by
the ldap-user-attributes property.
Yeah, I was looking at that code, but also did a quick search of my local AD
environment and figured out that the domain is not (obviously) present in any
attribute without splitting it out of another attribute. So, again, if the
domain portion is desired/required, it would need to be something LDAP-specific.
If the desire is just to get the Guacamole identifier, I'm good with the
`GUAC_IDENTIFIER` route.
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
mike-jumper commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1394536847
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
If there is to be a new standard token, I think there's some value in having
one token (`GUAC_IDENTIFIER`? `GUAC_USER_IDENTIFIER`?) that represents
validated identity while the other (`GUAC_USERNAME`) represents the username
provided during the auth process, if any.
I'd also have no issue if this were 100% purely LDAP-specific tokens.
> ... or just pull the LDAP attribute with the username?
I think that much is already possible with the functionality provided by the
`ldap-user-attributes` property.
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
necouchman commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1394378038
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
> I don't think decisions should be made about the format of the username at
the web application level...
I agree, and my question would be: Why do these need to be standard tokens
at all? We've provided a mechanism within the LDAP module (and most of the
other modules, particularly SSO) to define tokens specific to those modules,
but that should be usable in connections stored in other modules. Is there any
reason not to just do something like:
* LDAP_USER_REALM or LDAP_USER_DOMAIN
* LDAP_USER_IDENTIFIER, or just pull the LDAP attribute with the username?
? Or is there some other piece of this I'm missing?
--
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]
Re: [PR] GUACAMOLE-1881: Adding new standard tokens userid and domain name [guacamole-client]
mike-jumper commented on code in PR #931:
URL: https://github.com/apache/guacamole-client/pull/931#discussion_r1393809657
##
guacamole/src/main/java/org/apache/guacamole/tunnel/StandardTokenMap.java:
##
@@ -102,6 +115,13 @@ public StandardTokenMap(AuthenticatedUser
authenticatedUser) {
else
put(USERNAME_TOKEN, authenticatedUser.getIdentifier());
+if (get(USERNAME_TOKEN).contains("\\")) {
+put(USERNAME_DOMAIN_TOKEN, get(USERNAME_TOKEN).split("")[0]);
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN).split("")[1]);
+} else {
+put(USERNAME_DOMAIN_TOKEN, "");
+put(USERNAME_ID_TOKEN, get(USERNAME_TOKEN));
+}
Review Comment:
I don't think decisions should be made about the format of the username at
the web application level, nor that we should make specific assumptions about
the format here when the LDAP handling of username format is so flexible.
What about exposing `authenticatedUser.getIdentifier()` as something like
`GUAC_IDENTIFIER`? That would allow the unique identifier that the LDAP support
is already parsing out of the username to be exposed automatically without
forcing the webapp to be aware of LDAP-specific details.
If it is also necessary to pull the domain of a user authenticating with
LDAP and make that available with a token, it would make sense to do that
within the LDAP support, and it may be possible to extend the way
`match-usernames` works to provide that.
--
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]
