GUACAMOLE-1881: Add parameter token containing the domain of LDAP/AD usernames. #987
Conversation
…rom user credentials
…to derive static name.
… domain is used for a token.
mike-jumper
force-pushed
the
ldap-domain-token
branch
from
20e7033
to
c330dbb
Compare
| * ("DOMAIN\\username") or UPN ("username@domain"). If the username is in | ||
| * "DOMAIN\\username" format, the domain will be stored in the first |
There was a problem hiding this 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?
| * 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 |
There was a problem hiding this comment.
Either the of or from is extra, here.
| } | ||
| 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); |
There was a problem hiding this 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?
|
@mike-jumper Can we get these changes into next release, its been months since I have raise pull request. |
This change is intended to supersede the changes proposed by poor @josnabattula via PR #931 that I have been very slow to review. It builds off the original commit (27bbd35) by:
TokenName.canonicalize()(there's no need to dynamically derive the token name of a static value).getDomainToken()renamed and redocumented asgetUserDomain()).Pattern(there's no need to repeatedly recompile the regex each time a user logs in - it never changes).