[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #656: GUACAMOLE-1465: Allow different bind passwords for multi-LDAP

[GitBox <gi...@apache.org>]

mike-jumper commented on a change in pull request #656:
URL: https://github.com/apache/guacamole-client/pull/656#discussion_r757125086



##########
File path: extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java
##########
@@ -228,7 +228,7 @@ private UserLDAPConfiguration getLDAPConfiguration(String username,
             }
 
             // Attempt bind (authentication)
-            LdapNetworkConnection ldapConnection = ldapService.bindAs(config, bindDn.getName(), password);
+            LdapNetworkConnection ldapConnection = ldapService.bindAs(config, bindDn.getName(), (password == null || password.isEmpty()) ? config.getSearchBindPassword() : password);

Review comment:
       In this context, `password` is the password provided by the user during login, not the search bind password. The search bind password should definitely not be used as a fallback for a user's login attempt, which would potentially allow a user to successfully authenticate despite not providing a valid password.
   
   The only place that the search bind password should be pulled is in the context of the search bind DN (within `getUserBindDn()`). This should already be the case:
   
   https://github.com/apache/guacamole-client/blob/262643b2930aad5b6dc31df75cb928577b6a99a8/extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java#L131-L133




-- 
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