Github user mike-jumper commented on a diff in the pull request:
https://github.com/apache/incubator-guacamole-client/pull/68#discussion_r77440291
--- Diff:
extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java
---
@@ -205,38 +214,80 @@ private LDAPConnection bindAs(Credentials credentials)
* denied.
*/
public AuthenticatedUser authenticateUser(Credentials credentials)
- throws GuacamoleException {
-
- // Attempt bind
- LDAPConnection ldapConnection;
- try {
- ldapConnection = bindAs(credentials);
- }
- catch (GuacamoleException e) {
- logger.error("Cannot bind with LDAP server: {}",
e.getMessage());
- logger.debug("Error binding with LDAP server.", e);
- ldapConnection = null;
- }
-
- // If bind fails, permission to login is denied
- if (ldapConnection == null)
- throw new GuacamoleInvalidCredentialsException("Permission
denied.", CredentialsInfo.USERNAME_PASSWORD);
-
- try {
-
- // Return AuthenticatedUser if bind succeeds
- AuthenticatedUser authenticatedUser =
authenticatedUserProvider.get();
- authenticatedUser.init(credentials);
- return authenticatedUser;
-
- }
-
- // Always disconnect
- finally {
- ldapService.disconnect(ldapConnection);
- }
-
+ throws GuacamoleException {
+
+ // Attempt bind
+ LDAPConnection ldapConnection;
+ try {
+ ldapConnection = bindAs(credentials);
+ }
+ catch (GuacamoleException e) {
+ logger.error("Cannot bind with LDAP server: {}",
e.getMessage());
+ logger.debug("Error binding with LDAP server.", e);
+ ldapConnection = null;
+ }
+
+ // If bind fails, permission to login is denied
+ if (ldapConnection == null)
+ throw new
GuacamoleInvalidCredentialsException("Permission denied.",
CredentialsInfo.USERNAME_PASSWORD);
+
+ boolean authenticated=true;
+ // check if login in user also meet additional search filter.
+ if(confService.getAdditionalSearchFilter().length()>0)
+ {
+ authenticated=false;
+ for (String usernameAttribute :
confService.getUsernameAttributes()) {
+ try{
+ String ldapSearchFilter=
"(&(objectClass=*)(" +
escapingService.escapeLDAPSearchFilter(usernameAttribute) +
"="+credentials.getUsername()+")"
+
+confService.getAdditionalSearchFilter().trim()+")";
+
+ logger.debug("ldap search filter is
:"+ldapSearchFilter);
+ // Find all Guacamole users underneath
base DN
+ LDAPSearchResults results =
ldapConnection.search(
+
confService.getUserBaseDN(),
+
LDAPConnection.SCOPE_SUB,
+ ldapSearchFilter,
+ null,
+ false
+ );
+
+ // Read all visible users
+ while (results.hasMore()) {
+ authenticated=true;
+ logger.debug("ldap search find
at least one match with filter: "+ldapSearchFilter);
+ break;
+ }
+ }
+ catch (LDAPException e) {
+ logger.warn("ldap search failed with
additional filter"+confService.getAdditionalSearchFilter());
--- End diff --
Did you notice how the logger functions are invoked elsewhere in the code?
Consider:
```
logger.error("Cannot bind with LDAP server: {}", e.getMessage());
```
There is no need to manually concatenate strings. The logger will
substitute the `{}` within the message with the provided parameters, in order.
That said, this message would be very difficult to read anyway. Besides not
matching the formatting of other messages, there is no separation between the
message itself and the filter you intend to log. If the filter is "someFilter",
then your message will look like:
```
ldap search failed with additional filtersomeFilter
```
rather than:
```
LDAP search failed with additional filter: someFilter
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at [email protected] or file a JIRA ticket
with INFRA.
---
