Add null checks after retrieving user from LDAP for validation to pre…

…vent NPE when user is removed in LDAP. Closes keycloak#28523 Signed-off-by: Stefan Guilhen <[email protected]>
brianredbeard · Apr 11, 2024 · e6b9d28 · e6b9d28
1 parent d31f128
commit e6b9d28
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 1 deletion.
diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java
@@ -790,6  790,10 @@ public boolean validPassword(RealmModel realm, UserModel user, String password)
         } else {
             // Use Naming LDAP API
             LDAPObject ldapUser = loadAndValidateUser(realm, user);
             if (ldapUser == null) {
                 // user was removed from ldap - password verification must fail.
                 return false;
             }
 
             try {
                 ldapIdentityStore.validatePassword(ldapUser, password);
@@ -821,6  825,10 @@ public boolean updateCredential(RealmModel realm, UserModel user, CredentialInpu
             LDAPIdentityStore ldapIdentityStore = getLdapIdentityStore();
             String password = input.getChallengeResponse();
             LDAPObject ldapUser = loadAndValidateUser(realm, user);
             if (ldapUser == null) {
                 logger.warnf("User '%s' can't be updated in LDAP as it doesn't exist there", user.getUsername());
                 return false;
             }
             if (ldapIdentityStore.getConfig().isValidatePasswordPolicy()) {
                 PolicyError error = session.getProvider(PasswordPolicyManagerProvider.class).validate(realm, user, password);
                 if (error != null) throw new ModelException(error.getMessage(), error.getParameters());
@@ -967,7  975,8 @@ protected UserModel findOrCreateAuthenticatedUser(RealmModel realm, KerberosPrin
                 return null;
             } else {
                 LDAPObject ldapObject = loadAndValidateUser(realm, user);
-                if (kerberosPrincipalAttrName != null && !kerberosPrincipal.toString().equalsIgnoreCase(ldapObject.getAttributeAsString(kerberosPrincipalAttrName))) {
                 if (kerberosPrincipalAttrName != null && ldapObject != null &&
                         !kerberosPrincipal.toString().equalsIgnoreCase(ldapObject.getAttributeAsString(kerberosPrincipalAttrName))) {
                     logger.warnf("User with username [%s] aready exists and is linked to provider [%s] but is not valid. Authenticated kerberos principal is [%s], but LDAP user has different kerberos principal [%s]",
                             user.getUsername(),  model.getName(), kerberosPrincipal, ldapObject.getAttributeAsString(kerberosPrincipalAttrName));
                     ldapObject = null;

diff --git a/...an/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPUserLoginTest.java b/...an/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPUserLoginTest.java
@@ -324,4  324,31 @@ public void loginLDAPUserCredentialVaultAuthenticationNoneEncryptionStartTLS() {
         verifyConnectionUrlProtocolPrefix("ldap://");
         runLDAPLoginTest();
     }
 
     // Check that login fails as expected when an LDAP user that has already authenticated is removed from LDAP and attempts to authenticate again.
     // See https://github.com/keycloak/keycloak/issues/28523
     @Test
     @LDAPConnectionParameters(bindType=LDAPConnectionParameters.BindType.SIMPLE, encryption=LDAPConnectionParameters.Encryption.NONE)
     public void loginLDAPUserAuthenticationSimpleDeleteLDAPUser() {
         // create another user for this test.
         getTestingClient().server().run(session -> {
             LDAPTestContext ctx = LDAPTestContext.init(session);
             RealmModel appRealm = ctx.getRealm();
             LDAPObject jane = LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "janedoe", "Jane",
                             "Doe", "[email protected]", "2nd Avenue", "09283");
             LDAPTestUtils.updateLDAPPassword(ctx.getLdapProvider(), jane, DEFAULT_TEST_USERS.get("VALID_USER_PASSWORD"));
         });
         // login with the new user, then logout - user is now cached in Keycloak.
         this.verifyLoginSucceededAndLogout("janedoe", DEFAULT_TEST_USERS.get("VALID_USER_PASSWORD"));
 
         // now remove the user directly in LDAP.
         getTestingClient().server().run(session -> {
             LDAPTestContext ctx = LDAPTestContext.init(session);
             RealmModel appRealm = ctx.getRealm();
             LDAPTestUtils.removeLDAPUserByUsername(ctx.getLdapProvider(), appRealm, ctx.getLdapProvider().getLdapIdentityStore().getConfig(), "janedoe");
         });
 
         // attempt to login again with the deleted user should fail with the proper message.
         this.verifyLoginFailed("janedoe", DEFAULT_TEST_USERS.get("VALID_USER_PASSWORD"));
     }
 }