From 97400827d2f4e95a9576731c134c97f8d1f23157 Mon Sep 17 00:00:00 2001 From: Martin Idel Date: Mon, 27 Jul 2020 16:34:39 +0200 Subject: [PATCH] KEYCLOAK-14870: Fix bug where user is incorrectly imported Bug: SerializedBrokeredIdentityContext was changed to mirror UserModel changes. However, when creating the user in LDAP, the username must be provided first (everything else can be handled via attributes). --- .../UserAttributeLDAPStorageMapper.java | 28 +++++++++---------- .../models/cache/infinispan/UserAdapter.java | 7 +++++ .../storage/adapter/InMemoryUserAdapter.java | 8 ++++++ .../AbstractUserAdapterFederatedStorage.java | 8 +++--- .../IdpCreateUserIfUniqueAuthenticator.java | 7 ++--- .../ldap/LDAPProvidersIntegrationTest.java | 24 ++++++++++++++++ 6 files changed, 59 insertions(+), 23 deletions(-) diff --git a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/UserAttributeLDAPStorageMapper.java b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/UserAttributeLDAPStorageMapper.java index ff7ab9ef53..4754293891 100644 --- a/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/UserAttributeLDAPStorageMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/storage/ldap/mappers/UserAttributeLDAPStorageMapper.java @@ -22,6 +22,7 @@ import org.keycloak.component.ComponentModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.LDAPConstants; import org.keycloak.models.ModelDuplicateException; +import org.keycloak.models.ModelException; import org.keycloak.models.RealmModel; import org.keycloak.models.UserModel; import org.keycloak.models.utils.KeycloakModelUtils; @@ -43,8 +44,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import org.keycloak.models.ModelException; - /** * @author Marek Posolda */ @@ -192,24 +191,21 @@ public class UserAttributeLDAPStorageMapper extends AbstractLDAPStorageMapper { @Override public void setSingleAttribute(String name, String value) { if (UserModel.USERNAME.equals(name)) { - checkDuplicateUsername(userModelAttrName, value, realm, ldapProvider.getSession(), this); + setUsername(value); } else if (UserModel.EMAIL.equals(name)) { - checkDuplicateEmail(userModelAttrName, value, realm, ldapProvider.getSession(), this); - } - if (setLDAPAttribute(name, value)) { + setEmail(value); + } else if (setLDAPAttribute(name, value)) { super.setSingleAttribute(name, value); } } @Override public void setAttribute(String name, List values) { - String valueToSet = (values != null && values.size() > 0) ? values.get(0) : null; if (UserModel.USERNAME.equals(name)) { - checkDuplicateUsername(userModelAttrName, valueToSet, realm, ldapProvider.getSession(), this); + setUsername((values != null && values.size() > 0) ? values.get(0) : null); } else if (UserModel.EMAIL.equals(name)) { - checkDuplicateEmail(userModelAttrName, valueToSet, realm, ldapProvider.getSession(), this); - } - if (setLDAPAttribute(name, values)) { + setEmail((values != null && values.size() > 0) ? values.get(0) : null); + } else if (setLDAPAttribute(name, values)) { super.setAttribute(name, values); } } @@ -223,17 +219,19 @@ public class UserAttributeLDAPStorageMapper extends AbstractLDAPStorageMapper { @Override public void setUsername(String username) { - checkDuplicateUsername(userModelAttrName, username, realm, ldapProvider.getSession(), this); - setLDAPAttribute(UserModel.USERNAME, username); - super.setUsername(username); + String lowercaseUsername = KeycloakModelUtils.toLowerCaseSafe(username); + checkDuplicateUsername(userModelAttrName, lowercaseUsername, realm, ldapProvider.getSession(), this); + setLDAPAttribute(UserModel.USERNAME, lowercaseUsername); + super.setUsername(lowercaseUsername); } @Override public void setEmail(String email) { + String lowercaseEmail = KeycloakModelUtils.toLowerCaseSafe(email); checkDuplicateEmail(userModelAttrName, email, realm, ldapProvider.getSession(), this); setLDAPAttribute(UserModel.EMAIL, email); - super.setEmail(email); + super.setEmail(lowercaseEmail); } @Override diff --git a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserAdapter.java b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserAdapter.java index 938adec47c..74043f5829 100755 --- a/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserAdapter.java +++ b/model/infinispan/src/main/java/org/keycloak/models/cache/infinispan/UserAdapter.java @@ -167,12 +167,19 @@ public class UserAdapter implements CachedUserModel { @Override public void setSingleAttribute(String name, String value) { getDelegateForUpdate(); + if (UserModel.USERNAME.equals(name) || UserModel.EMAIL.equals(name)) { + value = KeycloakModelUtils.toLowerCaseSafe(value); + } updated.setSingleAttribute(name, value); } @Override public void setAttribute(String name, List values) { getDelegateForUpdate(); + if (UserModel.USERNAME.equals(name) || UserModel.EMAIL.equals(name)) { + String lowerCasedFirstValue = KeycloakModelUtils.toLowerCaseSafe((values != null && values.size() > 0) ? values.get(0) : null); + if (lowerCasedFirstValue != null) values.set(0, lowerCasedFirstValue); + } updated.setAttribute(name, values); } diff --git a/server-spi-private/src/main/java/org/keycloak/storage/adapter/InMemoryUserAdapter.java b/server-spi-private/src/main/java/org/keycloak/storage/adapter/InMemoryUserAdapter.java index a1704cac62..f2f541586f 100644 --- a/server-spi-private/src/main/java/org/keycloak/storage/adapter/InMemoryUserAdapter.java +++ b/server-spi-private/src/main/java/org/keycloak/storage/adapter/InMemoryUserAdapter.java @@ -26,6 +26,7 @@ import org.keycloak.models.RoleModel; import org.keycloak.models.UserModel; import org.keycloak.models.UserModelDefaultMethods; import org.keycloak.models.utils.DefaultRoles; +import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.models.utils.RoleUtils; import org.keycloak.storage.ReadOnlyException; @@ -123,6 +124,9 @@ public class InMemoryUserAdapter extends UserModelDefaultMethods { @Override public void setSingleAttribute(String name, String value) { checkReadonly(); + if (UserModel.USERNAME.equals(name) || UserModel.EMAIL.equals(name)) { + value = KeycloakModelUtils.toLowerCaseSafe(value); + } attributes.putSingle(name, value); } @@ -130,6 +134,10 @@ public class InMemoryUserAdapter extends UserModelDefaultMethods { @Override public void setAttribute(String name, List values) { checkReadonly(); + if (UserModel.USERNAME.equals(name) || UserModel.EMAIL.equals(name)) { + String lowerCasedFirstValue = KeycloakModelUtils.toLowerCaseSafe((values != null && values.size() > 0) ? values.get(0) : null); + if (lowerCasedFirstValue != null) values.set(0, lowerCasedFirstValue); + } attributes.put(name, values); } diff --git a/server-spi/src/main/java/org/keycloak/storage/adapter/AbstractUserAdapterFederatedStorage.java b/server-spi/src/main/java/org/keycloak/storage/adapter/AbstractUserAdapterFederatedStorage.java index 571379269e..7703d4d315 100644 --- a/server-spi/src/main/java/org/keycloak/storage/adapter/AbstractUserAdapterFederatedStorage.java +++ b/server-spi/src/main/java/org/keycloak/storage/adapter/AbstractUserAdapterFederatedStorage.java @@ -340,9 +340,9 @@ public abstract class AbstractUserAdapterFederatedStorage extends UserModelDefau public void setSingleAttribute(String name, String value) { if (UserModel.USERNAME.equals(name)) { setUsername(value); + } else { + getFederatedStorage().setSingleAttribute(realm, this.getId(), mapAttribute(name), value); } - getFederatedStorage().setSingleAttribute(realm, this.getId(), mapAttribute(name), value); - } @Override @@ -355,9 +355,9 @@ public abstract class AbstractUserAdapterFederatedStorage extends UserModelDefau public void setAttribute(String name, List values) { if (UserModel.USERNAME.equals(name)) { setUsername((values != null && values.size() > 0) ? values.get(0) : null); + } else { + getFederatedStorage().setAttribute(realm, this.getId(), mapAttribute(name), values); } - getFederatedStorage().setAttribute(realm, this.getId(), mapAttribute(name), values); - } @Override diff --git a/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpCreateUserIfUniqueAuthenticator.java b/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpCreateUserIfUniqueAuthenticator.java index e0b2363de3..e6ba64a492 100644 --- a/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpCreateUserIfUniqueAuthenticator.java +++ b/services/src/main/java/org/keycloak/authentication/authenticators/broker/IdpCreateUserIfUniqueAuthenticator.java @@ -74,12 +74,11 @@ public class IdpCreateUserIfUniqueAuthenticator extends AbstractIdpAuthenticator UserModel federatedUser = session.users().addUser(realm, username); federatedUser.setEnabled(true); - federatedUser.setEmail(brokerContext.getEmail()); - federatedUser.setFirstName(brokerContext.getFirstName()); - federatedUser.setLastName(brokerContext.getLastName()); for (Map.Entry> attr : serializedCtx.getAttributes().entrySet()) { - federatedUser.setAttribute(attr.getKey(), attr.getValue()); + if (!UserModel.USERNAME.equalsIgnoreCase(attr.getKey())) { + federatedUser.setAttribute(attr.getKey(), attr.getValue()); + } } AuthenticatorConfigModel config = context.getAuthenticatorConfig(); diff --git a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPProvidersIntegrationTest.java b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPProvidersIntegrationTest.java index 11b15da388..67b1c682f6 100755 --- a/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPProvidersIntegrationTest.java +++ b/testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/federation/ldap/LDAPProvidersIntegrationTest.java @@ -39,6 +39,7 @@ import org.keycloak.models.utils.KeycloakModelUtils; import org.keycloak.representations.AccessToken; import org.keycloak.representations.idm.ComponentRepresentation; import org.keycloak.representations.idm.CredentialRepresentation; +import org.keycloak.representations.idm.RealmRepresentation; import org.keycloak.representations.idm.UserRepresentation; import org.keycloak.services.managers.RealmManager; import org.keycloak.storage.ReadOnlyException; @@ -1143,4 +1144,27 @@ public class LDAPProvidersIntegrationTest extends AbstractLDAPTest { Assert.assertFalse(userNotVerified.get(0).isEmailVerified()); }); } + + @Test + public void testUserAttributeLDAPStorageMapperHandlingUsernameLowercasing() { + setEditingUsernameAllowed(false); + + testingClient.server().run(session -> { + LDAPTestContext ctx = LDAPTestContext.init(session); + RealmModel appRealm = ctx.getRealm(); + + UserModel johnkeycloak = session.users().getUserByUsername("johnkeycloak", appRealm); + // If the username was case sensitive in the username-cn mapper, then this would throw an exception + johnkeycloak.setSingleAttribute(UserModel.USERNAME, "JohnKeycloak"); + }); + + // Cleanup + setEditingUsernameAllowed(true); + } + + private void setEditingUsernameAllowed(boolean allowed) { + RealmRepresentation realmRepresentation = testRealm().toRepresentation(); + realmRepresentation.setEditUsernameAllowed(allowed); + testRealm().update(realmRepresentation); + } }