From 821405e1753bfca657c24e16632bf91e82d61419 Mon Sep 17 00:00:00 2001 From: mposolda Date: Wed, 8 Apr 2020 12:28:40 +0200 Subject: [PATCH] KEYCLOAK-10852 Inconsistency when using 'forgot password' after changing email directly in LDAP --- .../storage/ldap/LDAPStorageProvider.java | 4 +- .../keycloak/storage/UserStorageManager.java | 6 +- .../main/module.xml | 1 + .../keycloak/testsuite/util/MailUtils.java | 12 + .../federation/ldap/LDAPNoCacheTest.java | 214 ++++++++++++++++++ 5 files changed, 235 insertions(+), 2 deletions(-) create mode 100644 testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPNoCacheTest.java 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 index 6d4ae22de5..70499a55e6 100755 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/LDAPStorageProvider.java @@ -563,7 +563,9 @@ public class LDAPStorageProvider implements UserStorageProvider, if (user != null) { LDAPUtils.checkUuid(ldapUser, ldapIdentityStore.getConfig()); // If email attribute mapper is set to "Always Read Value From LDAP" the user may be in Keycloak DB with an old email address - if (ldapUser.getUuid().equals(user.getFirstAttribute(LDAPConstants.LDAP_ID))) return user; + if (ldapUser.getUuid().equals(user.getFirstAttribute(LDAPConstants.LDAP_ID))) { + return proxy(realm, user, ldapUser); + } throw new ModelDuplicateException("User with username '" + ldapUsername + "' already exists in Keycloak. It conflicts with LDAP user with email '" + email + "'"); } diff --git a/services/src/main/java/org/keycloak/storage/UserStorageManager.java b/services/src/main/java/org/keycloak/storage/UserStorageManager.java index f7240711ef..a5acd40ee5 100755 --- a/services/src/main/java/org/keycloak/storage/UserStorageManager.java +++ b/services/src/main/java/org/keycloak/storage/UserStorageManager.java @@ -412,7 +412,11 @@ public class UserStorageManager implements UserProvider, OnUserCache, OnCreateCo public UserModel getUserByEmail(String email, RealmModel realm) { UserModel user = localStorage().getUserByEmail(email, realm); if (user != null) { - return importValidation(realm, user); + user = importValidation(realm, user); + // Case when email was changed directly in the userStorage and doesn't correspond anymore to the email from local DB + if (email.equalsIgnoreCase(user.getEmail())) { + return user; + } } for (UserLookupProvider provider : getEnabledStorageProviders(session, realm, UserLookupProvider.class)) { user = provider.getUserByEmail(email, realm); diff --git a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/resources/org/keycloak/testsuite/integration-arquillian-testsuite-providers/main/module.xml b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/resources/org/keycloak/testsuite/integration-arquillian-testsuite-providers/main/module.xml index cb844e206e..e29f10cbf1 100644 --- a/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/resources/org/keycloak/testsuite/integration-arquillian-testsuite-providers/main/module.xml +++ b/testsuite/integration-arquillian/servers/auth-server/services/testsuite-providers/src/main/resources/org/keycloak/testsuite/integration-arquillian-testsuite-providers/main/module.xml @@ -22,6 +22,7 @@ + diff --git a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/MailUtils.java b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/MailUtils.java index 1083148c9f..8734bd2a6a 100644 --- a/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/MailUtils.java +++ b/testsuite/integration-arquillian/tests/base/src/main/java/org/keycloak/testsuite/util/MailUtils.java @@ -17,6 +17,7 @@ package org.keycloak.testsuite.util; +import javax.mail.Address; import javax.mail.MessagingException; import javax.mail.Multipart; import javax.mail.internet.MimeMessage; @@ -45,6 +46,17 @@ public class MailUtils { return getPasswordResetEmailLink(new EmailBody(message)); } + /** + * + * @param message email message + * @return first recipient of the email message + * @throws MessagingException + */ + public static String getRecipient(MimeMessage message) throws MessagingException { + Address[] recipients = message.getRecipients(MimeMessage.RecipientType.TO); + return recipients[0].toString(); + } + public static String getPasswordResetEmailLink(EmailBody body) throws IOException { final String textChangePwdUrl = getLink(body.getText()); String htmlChangePwdUrl = getLink(body.getHtml()); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPNoCacheTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPNoCacheTest.java new file mode 100644 index 0000000000..c84cd40e9c --- /dev/null +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPNoCacheTest.java @@ -0,0 +1,214 @@ +/* + * Copyright 2019 Red Hat, Inc. and/or its affiliates + * and other contributors as indicated by the @author tags. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +package org.keycloak.testsuite.federation.ldap; + +import java.io.IOException; +import java.util.List; + +import javax.mail.MessagingException; +import javax.mail.internet.MimeMessage; + +import org.hamcrest.Matchers; +import org.jboss.arquillian.graphene.page.Page; +import org.junit.ClassRule; +import org.junit.FixMethodOrder; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runners.MethodSorters; +import org.keycloak.component.ComponentModel; +import org.keycloak.models.KeycloakSession; +import org.keycloak.models.LDAPConstants; +import org.keycloak.models.RealmModel; +import org.keycloak.representations.idm.UserRepresentation; +import org.keycloak.storage.UserStorageProviderModel; +import org.keycloak.storage.ldap.LDAPStorageProvider; +import org.keycloak.storage.ldap.idm.model.LDAPObject; +import org.keycloak.storage.ldap.mappers.UserAttributeLDAPStorageMapper; +import org.keycloak.storage.ldap.mappers.UserAttributeLDAPStorageMapperFactory; +import org.keycloak.testsuite.Assert; +import org.keycloak.testsuite.client.KeycloakTestingClient; +import org.keycloak.testsuite.pages.AppPage; +import org.keycloak.testsuite.pages.LoginPage; +import org.keycloak.testsuite.pages.LoginPasswordResetPage; +import org.keycloak.testsuite.pages.LoginPasswordUpdatePage; +import org.keycloak.testsuite.util.GreenMailRule; +import org.keycloak.testsuite.util.LDAPRule; +import org.keycloak.testsuite.util.LDAPTestUtils; +import org.keycloak.testsuite.util.MailUtils; + +import static org.junit.Assert.assertEquals; + +/** + * Test for the scenarios with disabled cache for LDAP provider. This involves scenarios when something is changed directly in LDAP server + * and changes are supposed to be immediately visible on Keycloak side + * + * @author Marek Posolda + */ +@FixMethodOrder(MethodSorters.NAME_ASCENDING) +public class LDAPNoCacheTest extends AbstractLDAPTest { + + @ClassRule + public static LDAPRule ldapRule = new LDAPRule(); + + @Override + protected LDAPRule getLDAPRule() { + return ldapRule; + } + + @Override + protected void afterImportTestRealm() { + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + + // Switch to NO_CACHE + RealmModel appRealm = ctx.getRealm(); + ctx.getLdapModel().setCachePolicy(UserStorageProviderModel.CachePolicy.NO_CACHE); + appRealm.updateComponent(ctx.getLdapModel()); + + // Switch mappers to "Always read value from LDAP". Changed attributes in LDAP should be immediately visible on Keycloak side + List ldapMappers = appRealm.getComponents(ctx.getLdapModel().getId()); + ldapMappers.stream() + .filter(mapper -> UserAttributeLDAPStorageMapperFactory.PROVIDER_ID.equals(mapper.getProviderId())) + .forEach(mapper -> { + + mapper.put(UserAttributeLDAPStorageMapper.ALWAYS_READ_VALUE_FROM_LDAP, true); + appRealm.updateComponent(mapper); + + }); + + // Delete all LDAP users and add some new for testing + LDAPTestUtils.removeAllLDAPUsers(ctx.getLdapProvider(), appRealm); + + LDAPObject john = LDAPTestUtils.addLDAPUser(ctx.getLdapProvider(), appRealm, "johnkeycloak", "John", "Doe", "john_old@email.org", null, "1234"); + LDAPTestUtils.updateLDAPPassword(ctx.getLdapProvider(), john, "Password1"); + + }); + } + + @Rule + public GreenMailRule greenMail = new GreenMailRule(); + + @Page + protected AppPage appPage; + + @Page + protected LoginPage loginPage; + + @Page + protected LoginPasswordResetPage resetPasswordPage; + + @Page + protected LoginPasswordUpdatePage updatePasswordPage; + + // KEYCLOAK-10852 + @Test + public void resetPasswordLink() throws IOException, MessagingException { + // Trigger reset password from the login page + loginPage.open(); + + // Send and email to the current email address of john. This will sync john to the Keycloak DB + triggerForgetPasswordForUser("john_old@email.org", 1, "john_old@email.org"); + + // Change the email address of user directly in LDAP + changeEmailAddressInLDAP(testingClient,"john_new@email.org"); + + try { + // Search for the user and check email is new address + UserRepresentation john = testRealm().users().search("johnkeycloak").get(0); + Assert.assertEquals("john_new@email.org", john.getEmail()); + + // Test 1 - Use username on the ResetPassword form. Mail should be sent to new address + triggerForgetPasswordForUser("johnkeycloak", 2, "john_new@email.org"); + + // Test 2 - Use old email on the ResetPassword form. Mail should NOT be sent and count of messages should be still the same + triggerForgetPasswordForUser("john_old@email.org", 2, "john_new@email.org"); + + // Test 3 - Use new email on the ResetPassword form. Mail should be sent to new address + triggerForgetPasswordForUser("john_new@email.org", 3, "john_new@email.org"); + } finally { + // Revert email address in LDAP + changeEmailAddressInLDAP(testingClient, "john_old@email.org"); + } + } + + + @Test + public void resetPasswordLinkCheckOldAddressLast() throws IOException, MessagingException { + // Trigger reset password from the login page + loginPage.open(); + + triggerForgetPasswordForUser("john_old@email.org", 1, "john_old@email.org"); + + changeEmailAddressInLDAP(testingClient,"john_new@email.org"); + + try { + // Test 1 - Use username on the ResetPassword form. Mail should be sent to new address + triggerForgetPasswordForUser("johnkeycloak", 2, "john_new@email.org"); + + // Test 2 - Use new email on the ResetPassword form. Mail should be sent to new address + triggerForgetPasswordForUser("john_new@email.org", 3, "john_new@email.org"); + + // Test 3 - Use old email on the ResetPassword form. Mail should NOT be sent and count of messages should be still the same + triggerForgetPasswordForUser("john_old@email.org", 3, "john_new@email.org"); + } finally { + // Revert email address in LDAP + changeEmailAddressInLDAP(testingClient, "john_old@email.org"); + } + } + + /** + * Trigger "Forget password" for the user and test mail was sent to expected address. + * Assumption is that browser (webDriver) is on loginPage when this method is triggered + * + * @param usernameInput username or email, which will be used on "Reset Password" form + * @param expectedCountOfMessages expected count of delivered messages. Important to test if new message was sent or not + * @param expectedEmail Expected email address where the last email message was sent + * + */ + private void triggerForgetPasswordForUser(String usernameInput, int expectedCountOfMessages, String expectedEmail) throws MessagingException { + loginPage.resetPassword(); + + resetPasswordPage.assertCurrent(); + resetPasswordPage.changePassword(usernameInput); + + loginPage.assertCurrent(); + assertEquals("You should receive an email shortly with further instructions.", loginPage.getSuccessMessage()); + + MimeMessage[] messages = greenMail.getReceivedMessages(); + Assert.assertEquals(expectedCountOfMessages, messages.length); + MimeMessage message = greenMail.getReceivedMessages()[expectedCountOfMessages - 1]; + + String emailAddress = MailUtils.getRecipient(message); + Assert.assertEquals(expectedEmail, emailAddress); + } + + private static void changeEmailAddressInLDAP(KeycloakTestingClient testingClient, String newEmail) { + testingClient.server().run((KeycloakSession session) -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + + RealmModel realm = ctx.getRealm(); + LDAPStorageProvider ldapProvider = ctx.getLdapProvider(); + LDAPObject ldapUser = ldapProvider.loadLDAPUserByUsername(realm, "johnkeycloak"); + ldapUser.setSingleAttribute(LDAPConstants.EMAIL, newEmail); + ctx.getLdapProvider().getLdapIdentityStore().update(ldapUser); + + }); + } + +}