You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@guacamole.apache.org by GitBox <gi...@apache.org> on 2022/01/21 23:33:53 UTC
[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #683: Added support for specifying LDAP UPN in properties file
mike-jumper commented on a change in pull request #683:
URL: https://github.com/apache/guacamole-client/pull/683#discussion_r789899794
##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java
##########
@@ -120,17 +120,28 @@
* If required properties are missing, and thus the user DN cannot be
* determined.
*/
- private Dn getUserBindDN(LDAPConfiguration config, String username)
- throws GuacamoleException {
+ private Dn getUserBindDN(LDAPConfiguration config, String username, String password) throws GuacamoleException {
// If a search DN is provided, search the LDAP directory for the DN
// corresponding to the given username
- String searchBindLogon = config.getSearchBindDN();
+
+ String searchBindLogon;
+ String searchBindPassword;
+
+ if(confService.getUPNDomain() != "" && confService.getUPNDomain() != null){
+ searchBindLogon = username + "@" + confService.getUPNDomain();
+ searchBindPassword = password;
Review comment:
I don't think this should be done. The search bind DN is a very specific thing - it's the service account that Guacamole uses to resolve the DN of a user. It shouldn't be replaced with the user's own account.
If the idea here is to allow logins via UPN and avoid the search bind DN entirely, then the solution would be to allow user authentication to proceed without a user search by directly binding with the provided UPN - essentially an alternative to the direct user mapping already supported.
##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java
##########
@@ -120,17 +120,28 @@
* If required properties are missing, and thus the user DN cannot be
* determined.
*/
- private Dn getUserBindDN(LDAPConfiguration config, String username)
- throws GuacamoleException {
+ private Dn getUserBindDN(LDAPConfiguration config, String username, String password) throws GuacamoleException {
// If a search DN is provided, search the LDAP directory for the DN
// corresponding to the given username
- String searchBindLogon = config.getSearchBindDN();
+
+ String searchBindLogon;
+ String searchBindPassword;
+
+ if(confService.getUPNDomain() != "" && confService.getUPNDomain() != null){
Review comment:
The test `confService.getUPNDomain() != ""` performs a by-reference comparison against the `String` object for `""`, not a comparison of the content of those strings. The proper test for whether a `String` is empty is `isEmpty()`, however the `null` check shown here would have to occur first for that call to not throw a `NullPointerException` in the event the string is indeed `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: dev-unsubscribe@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org