From c71a4ac4e8d321ecf7aec1ac41ea3a943561b3d0 Mon Sep 17 00:00:00 2001 From: mposolda Date: Fri, 17 Jul 2015 18:38:59 +0200 Subject: [PATCH] KEYCLOAK-1545 KEYCLOAK-1551 Ensure that username and email are always saved to DB lowercased --- .../ldap/LDAPFederationProvider.java | 7 -- .../UserAttributeLDAPFederationMapper.java | 4 + .../migration/migrators/MigrateTo1_4_0.java | 18 ++++- .../models/utils/KeycloakModelUtils.java | 3 + .../models/file/adapter/UserAdapter.java | 4 + .../keycloak/models/cache/UserAdapter.java | 3 + .../org/keycloak/models/jpa/UserAdapter.java | 2 + .../mongo/keycloak/adapters/UserAdapter.java | 4 + .../keycloak/testsuite/admin/UserTest.java | 21 +++++- .../FederationProvidersIntegrationTest.java | 74 +++++++++++++++++-- .../federation/SyncProvidersTest.java | 2 +- 11 files changed, 123 insertions(+), 19 deletions(-) diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java index 597f9fe0d9..1fa601d0e6 100755 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java @@ -431,13 +431,6 @@ public class LDAPFederationProvider implements UserFederationProvider { return null; } - // KEYCLOAK-808: Should we allow case-sensitivity to be configurable? - String ldapUsername = LDAPUtils.getUsername(ldapUser, ldapIdentityStore.getConfig()); - if (!username.equals(ldapUsername)) { - logger.warnf("User found in LDAP but with different username. LDAP username: %s, Searched username: %s", username, ldapUsername); - return null; - } - return ldapUser; } diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/UserAttributeLDAPFederationMapper.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/UserAttributeLDAPFederationMapper.java index 438c57d729..536753fb60 100644 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/UserAttributeLDAPFederationMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/UserAttributeLDAPFederationMapper.java @@ -23,6 +23,7 @@ import org.keycloak.models.RealmModel; import org.keycloak.models.UserFederationMapperModel; import org.keycloak.models.UserFederationProvider; import org.keycloak.models.UserModel; +import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.models.utils.UserModelDelegate; import org.keycloak.models.utils.reflection.Property; import org.keycloak.models.utils.reflection.PropertyCriteria; @@ -138,6 +139,9 @@ public class UserAttributeLDAPFederationMapper extends AbstractLDAPFederationMap // throw ModelDuplicateException if there is different user in model with same email protected void checkDuplicateEmail(String userModelAttrName, String email, RealmModel realm, KeycloakSession session, UserModel user) { if (UserModel.EMAIL.equalsIgnoreCase(userModelAttrName)) { + // lowercase before search + email = KeycloakModelUtils.toLowerCaseSafe(email); + UserModel that = session.userStorage().getUserByEmail(email, realm); if (that != null && !that.getId().equals(user.getId())) { session.getTransaction().setRollbackOnly(); diff --git a/model/api/src/main/java/org/keycloak/migration/migrators/MigrateTo1_4_0.java b/model/api/src/main/java/org/keycloak/migration/migrators/MigrateTo1_4_0.java index 29f3d4a11e..e2ef2f66fc 100755 --- a/model/api/src/main/java/org/keycloak/migration/migrators/MigrateTo1_4_0.java +++ b/model/api/src/main/java/org/keycloak/migration/migrators/MigrateTo1_4_0.java @@ -1,16 +1,14 @@ package org.keycloak.migration.migrators; import org.keycloak.migration.ModelVersion; -import org.keycloak.models.AuthenticationExecutionModel; -import org.keycloak.models.AuthenticationFlowModel; import org.keycloak.models.ImpersonationConstants; import org.keycloak.models.KeycloakSession; import org.keycloak.models.RealmModel; -import org.keycloak.models.RequiredCredentialModel; +import org.keycloak.models.UserModel; import org.keycloak.models.utils.DefaultAuthenticationFlows; import org.keycloak.models.utils.DefaultRequiredActions; +import org.keycloak.models.utils.KeycloakModelUtils; -import java.util.HashSet; import java.util.List; /** @@ -28,7 +26,19 @@ public class MigrateTo1_4_0 { DefaultRequiredActions.addActions(realm); } ImpersonationConstants.setupImpersonationService(session, realm); + migrateUsers(session, realm); } } + + public void migrateUsers(KeycloakSession session, RealmModel realm) { + List users = session.userStorage().getUsers(realm); + for (UserModel user : users) { + String email = user.getEmail(); + email = KeycloakModelUtils.toLowerCaseSafe(email); + if (email != null && !email.equals(user.getEmail())) { + user.setEmail(email); + } + } + } } diff --git a/model/api/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java b/model/api/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java index f26b4e9ba6..c98a114509 100755 --- a/model/api/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java +++ b/model/api/src/main/java/org/keycloak/models/utils/KeycloakModelUtils.java @@ -350,4 +350,7 @@ public final class KeycloakModelUtils { return mapperModel; } + public static String toLowerCaseSafe(String str) { + return str==null ? str : str.toLowerCase(); + } } diff --git a/model/file/src/main/java/org/keycloak/models/file/adapter/UserAdapter.java b/model/file/src/main/java/org/keycloak/models/file/adapter/UserAdapter.java index f037f011bb..5db8f93506 100755 --- a/model/file/src/main/java/org/keycloak/models/file/adapter/UserAdapter.java +++ b/model/file/src/main/java/org/keycloak/models/file/adapter/UserAdapter.java @@ -86,6 +86,8 @@ public class UserAdapter implements UserModel, Comparable { @Override public void setUsername(String username) { + username = KeycloakModelUtils.toLowerCaseSafe(username); + if (getUsername() == null) { user.setUsername(username); return; @@ -145,6 +147,8 @@ public class UserAdapter implements UserModel, Comparable { @Override public void setEmail(String email) { + email = KeycloakModelUtils.toLowerCaseSafe(email); + if (email == null) { user.setEmail(email); return; diff --git a/model/invalidation-cache/model-adapters/src/main/java/org/keycloak/models/cache/UserAdapter.java b/model/invalidation-cache/model-adapters/src/main/java/org/keycloak/models/cache/UserAdapter.java index 46130a9999..b075ea118c 100755 --- a/model/invalidation-cache/model-adapters/src/main/java/org/keycloak/models/cache/UserAdapter.java +++ b/model/invalidation-cache/model-adapters/src/main/java/org/keycloak/models/cache/UserAdapter.java @@ -10,6 +10,7 @@ import org.keycloak.models.UserCredentialModel; import org.keycloak.models.UserCredentialValueModel; import org.keycloak.models.UserModel; import org.keycloak.models.cache.entities.CachedUser; +import org.keycloak.models.utils.KeycloakModelUtils; import java.util.Collections; import java.util.HashSet; @@ -57,6 +58,7 @@ public class UserAdapter implements UserModel { @Override public void setUsername(String username) { getDelegateForUpdate(); + username = KeycloakModelUtils.toLowerCaseSafe(username); updated.setUsername(username); } @@ -189,6 +191,7 @@ public class UserAdapter implements UserModel { @Override public void setEmail(String email) { getDelegateForUpdate(); + email = KeycloakModelUtils.toLowerCaseSafe(email); updated.setEmail(email); } diff --git a/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java b/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java index 0e10110231..ca0c284b45 100755 --- a/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java +++ b/model/jpa/src/main/java/org/keycloak/models/jpa/UserAdapter.java @@ -74,6 +74,7 @@ public class UserAdapter implements UserModel { @Override public void setUsername(String username) { + username = KeycloakModelUtils.toLowerCaseSafe(username); user.setUsername(username); } @@ -266,6 +267,7 @@ public class UserAdapter implements UserModel { @Override public void setEmail(String email) { + email = KeycloakModelUtils.toLowerCaseSafe(email); user.setEmail(email); } diff --git a/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/UserAdapter.java b/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/UserAdapter.java index f63494b266..a8408134d5 100755 --- a/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/UserAdapter.java +++ b/model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/UserAdapter.java @@ -67,6 +67,8 @@ public class UserAdapter extends AbstractMongoAdapter implement @Override public void setUsername(String username) { + username = KeycloakModelUtils.toLowerCaseSafe(username); + user.setUsername(username); updateUser(); } @@ -121,6 +123,8 @@ public class UserAdapter extends AbstractMongoAdapter implement @Override public void setEmail(String email) { + email = KeycloakModelUtils.toLowerCaseSafe(email); + user.setEmail(email); updateUser(); } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/admin/UserTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/admin/UserTest.java index ab644cdcdb..aa5c3925a1 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/admin/UserTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/admin/UserTest.java @@ -27,9 +27,13 @@ import static org.junit.Assert.fail; public class UserTest extends AbstractClientTest { public String createUser() { + return createUser("user1", "user1@localhost"); + } + + public String createUser(String username, String email) { UserRepresentation user = new UserRepresentation(); - user.setUsername("user1"); - user.setEmail("user1@localhost"); + user.setUsername(username); + user.setEmail(email); Response response = realm.users().create(user); String createdId = ApiUtil.getCreatedId(response); @@ -115,6 +119,19 @@ public class UserTest extends AbstractClientTest { assertEquals(409, response.getStatus()); response.close(); } + + @Test + public void createDuplicatedUser7() { + createUser("user1", "USer1@Localhost"); + + UserRepresentation user = new UserRepresentation(); + user.setUsername("user2"); + user.setEmail("user1@localhost"); + Response response = realm.users().create(user); + assertEquals(409, response.getStatus()); + response.close(); + + } private void createUsers() { for (int i = 1; i < 10; i++) { diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationProvidersIntegrationTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationProvidersIntegrationTest.java index 708da5b473..b91ee08ee9 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationProvidersIntegrationTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationProvidersIntegrationTest.java @@ -39,6 +39,7 @@ import org.keycloak.testsuite.rule.WebResource; import org.keycloak.testsuite.rule.WebRule; import org.openqa.selenium.WebDriver; +import java.util.List; import java.util.Map; /** @@ -113,12 +114,75 @@ public class FederationProvidersIntegrationTest { @Test - public void caseSensitiveSearch() { - loginPage.open(); + public void caseInSensitiveImport() { + KeycloakSession session = keycloakRule.startSession(); + try { + RealmManager manager = new RealmManager(session); + RealmModel appRealm = manager.getRealm("test"); + LDAPFederationProvider ldapFedProvider = FederationTestUtils.getLdapProvider(session, ldapModel); + LDAPObject jbrown2 = FederationTestUtils.addLDAPUser(ldapFedProvider, appRealm, "JBrown2", "John", "Brown2", "jbrown2@email.org", null, "1234"); + ldapFedProvider.getLdapIdentityStore().updatePassword(jbrown2, "password"); + LDAPObject jbrown3 = FederationTestUtils.addLDAPUser(ldapFedProvider, appRealm, "jbrown3", "John", "Brown3", "JBrown3@email.org", null, "1234"); + ldapFedProvider.getLdapIdentityStore().updatePassword(jbrown3, "password"); + } finally { + keycloakRule.stopSession(session, true); + } - // This should fail for now due to case-sensitivity - loginPage.login("johnKeycloak", "Password1"); - Assert.assertEquals("Invalid username or password.", loginPage.getError()); + loginSuccessAndLogout("jbrown2", "password"); + loginSuccessAndLogout("JBrown2", "password"); + loginSuccessAndLogout("jbrown2@email.org", "password"); + loginSuccessAndLogout("JBrown2@email.org", "password"); + + loginSuccessAndLogout("jbrown3", "password"); + loginSuccessAndLogout("JBrown3", "password"); + loginSuccessAndLogout("jbrown3@email.org", "password"); + loginSuccessAndLogout("JBrown3@email.org", "password"); + } + + private void loginSuccessAndLogout(String username, String password) { + loginPage.open(); + loginPage.login(username, password); + Assert.assertEquals(AppPage.RequestType.AUTH_RESPONSE, appPage.getRequestType()); + Assert.assertNotNull(oauth.getCurrentQuery().get(OAuth2Constants.CODE)); + oauth.openLogout(); + } + + @Test + public void caseInsensitiveSearch() { + KeycloakSession session = keycloakRule.startSession(); + try { + RealmManager manager = new RealmManager(session); + RealmModel appRealm = manager.getRealm("test"); + LDAPFederationProvider ldapFedProvider = FederationTestUtils.getLdapProvider(session, ldapModel); + LDAPObject jbrown2 = FederationTestUtils.addLDAPUser(ldapFedProvider, appRealm, "JBrown4", "John", "Brown4", "jbrown4@email.org", null, "1234"); + ldapFedProvider.getLdapIdentityStore().updatePassword(jbrown2, "password"); + LDAPObject jbrown3 = FederationTestUtils.addLDAPUser(ldapFedProvider, appRealm, "jbrown5", "John", "Brown5", "JBrown5@Email.org", null, "1234"); + ldapFedProvider.getLdapIdentityStore().updatePassword(jbrown3, "password"); + } finally { + keycloakRule.stopSession(session, true); + } + + session = keycloakRule.startSession(); + try { + RealmManager manager = new RealmManager(session); + RealmModel appRealm = manager.getRealm("test"); + + // search by username + List users = session.users().searchForUser("JBROwn4", appRealm); + Assert.assertEquals(1, users.size()); + UserModel user4 = users.get(0); + Assert.assertEquals("jbrown4", user4.getUsername()); + Assert.assertEquals("jbrown4@email.org", user4.getEmail()); + + // search by email + users = session.users().searchForUser("JBROwn5@eMAil.org", appRealm); + Assert.assertEquals(1, users.size()); + UserModel user5 = users.get(0); + Assert.assertEquals("jbrown5", user5.getUsername()); + Assert.assertEquals("jbrown5@email.org", user5.getEmail()); + } finally { + keycloakRule.stopSession(session, true); + } } @Test diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/SyncProvidersTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/SyncProvidersTest.java index fd1c28dbb7..e50caf845c 100755 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/SyncProvidersTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/SyncProvidersTest.java @@ -151,7 +151,7 @@ public class SyncProvidersTest { RealmModel testRealm = session.realms().getRealm("test"); UserProvider userProvider = session.userStorage(); // Assert users updated in local provider - FederationTestUtils.assertUserImported(userProvider, testRealm, "user5", "User5FN", "User5LN", "user5Updated@email.org", "521"); + FederationTestUtils.assertUserImported(userProvider, testRealm, "user5", "User5FN", "User5LN", "user5updated@email.org", "521"); FederationTestUtils.assertUserImported(userProvider, testRealm, "user6", "User6FN", "User6LN", "user6@email.org", "126"); } finally { keycloakRule.stopSession(session, false);