From 2cb0efeda6ec05a65c649f3550ba4c52dbe95f39 Mon Sep 17 00:00:00 2001 From: Michael Jumper Date: Wed, 22 Jan 2020 23:05:03 -0800 Subject: [PATCH] GUACAMOLE-937: Return null on bindAs() failures. Rely on caller to interpret and handle failures. Throwing GuacamoleInvalidCredentialsException breaks separation of concerns (bindAs() shouldn't assume that it's being used during login and that credentials given are the Guacamole user's credentials), and has unintended side effects (throwing subclasses of GuacamoleUnauthorizedException causes implicit session invalidation). --- .../ldap/AuthenticationProviderService.java | 12 ++++++++++++ .../auth/ldap/LDAPConnectionService.java | 19 +++++++++---------- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java b/extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java index a9f39cf7f..c047b91af 100644 --- a/extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java +++ b/extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/AuthenticationProviderService.java @@ -204,6 +204,10 @@ public class AuthenticationProviderService { // Attempt bind LdapNetworkConnection ldapConnection = ldapService.bindAs(bindDn, password); + if (ldapConnection == null) + throw new GuacamoleInvalidCredentialsException("Invalid login.", + CredentialsInfo.USERNAME_PASSWORD); + try { // Retrieve group membership of the user that just authenticated @@ -309,8 +313,16 @@ public class AuthenticationProviderService { // Bind using credentials associated with AuthenticatedUser Credentials credentials = authenticatedUser.getCredentials(); if (authenticatedUser instanceof LDAPAuthenticatedUser) { + Dn bindDn = ((LDAPAuthenticatedUser) authenticatedUser).getBindDn(); LdapNetworkConnection ldapConnection = ldapService.bindAs(bindDn, credentials.getPassword()); + if (ldapConnection == null) { + logger.debug("LDAP bind succeeded for \"{}\" during " + + "authentication but failed during data retrieval.", + authenticatedUser.getIdentifier()); + throw new GuacamoleInvalidCredentialsException("Invalid login.", + CredentialsInfo.USERNAME_PASSWORD); + } try { diff --git a/extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java b/extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java index 7141a79cc..7e88f8abe 100644 --- a/extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java +++ b/extensions/guacamole-auth-ldap/src/main/java/org/apache/guacamole/auth/ldap/LDAPConnectionService.java @@ -144,13 +144,12 @@ public class LDAPConnectionService { bindRequest.setDn(userDN); bindRequest.setCredentials(password); BindResponse bindResponse = ldapConnection.bind(bindRequest); - if (bindResponse.getLdapResult().getResultCode() == ResultCodeEnum.SUCCESS) - return ldapConnection; - - else - throw new GuacamoleInvalidCredentialsException("Error binding" - + " to server: " + bindResponse.toString(), - CredentialsInfo.USERNAME_PASSWORD); + + if (bindResponse.getLdapResult().getResultCode() != ResultCodeEnum.SUCCESS) { + ldapConnection.close(); + logger.debug("LDAP bind attempt failed: {}", bindResponse.toString()); + return null; + } } @@ -158,11 +157,11 @@ public class LDAPConnectionService { catch (LdapException e) { ldapConnection.close(); logger.debug("Unable to bind to LDAP server.", e); - throw new GuacamoleInvalidCredentialsException( - "Unable to bind to the LDAP server.", - CredentialsInfo.USERNAME_PASSWORD); + return null; } + return ldapConnection; + } /**