From 773bb43b41713b21b4f9ad94e3ddc96bf84cfe5d Mon Sep 17 00:00:00 2001 From: mposolda Date: Thu, 25 Jun 2015 19:27:22 +0200 Subject: [PATCH] KEYCLOAK-1487 Fix LDAP case-sensitivity. Show warning in case of duplicate username --- .../keycloak/federation/ldap/LDAPUtils.java | 5 +-- .../federation/ldap/idm/model/LDAPObject.java | 41 ++++++++++++------- .../idm/query/internal/LDAPIdentityQuery.java | 3 +- .../idm/store/ldap/LDAPIdentityStore.java | 17 ++++---- .../mappers/FullNameLDAPFederationMapper.java | 2 +- .../mappers/RoleLDAPFederationMapper.java | 8 ++-- .../UserAttributeLDAPFederationMapper.java | 2 +- .../FederationProvidersIntegrationTest.java | 28 +++++++++++++ .../src/test/resources/ldap/users.ldif | 23 +++++++++++ 9 files changed, 95 insertions(+), 34 deletions(-) diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPUtils.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPUtils.java index 396d997dbe..27e8df3850 100755 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPUtils.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPUtils.java @@ -1,6 +1,5 @@ package org.keycloak.federation.ldap; -import java.util.List; import java.util.Set; import org.keycloak.federation.ldap.idm.model.LDAPDn; @@ -61,7 +60,7 @@ public class LDAPUtils { // ldapUser has filled attributes, but doesn't have filled dn. private static void computeAndSetDn(LDAPConfig config, LDAPObject ldapUser) { String rdnLdapAttrName = config.getRdnLdapAttribute(); - String rdnLdapAttrValue = ldapUser.getAttributeAsString(rdnLdapAttrName); + String rdnLdapAttrValue = ldapUser.getAttributeAsStringCaseInsensitive(rdnLdapAttrName); if (rdnLdapAttrValue == null) { throw new ModelException("RDN Attribute [" + rdnLdapAttrName + "] is not filled. Filled attributes: " + ldapUser.getAttributes()); } @@ -73,6 +72,6 @@ public class LDAPUtils { public static String getUsername(LDAPObject ldapUser, LDAPConfig config) { String usernameAttr = config.getUsernameLdapAttribute(); - return ldapUser.getAttributeAsString(usernameAttr); + return ldapUser.getAttributeAsStringCaseInsensitive(usernameAttr); } } diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/model/LDAPObject.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/model/LDAPObject.java index b7e6c0e36d..c449484cef 100644 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/model/LDAPObject.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/model/LDAPObject.java @@ -6,18 +6,29 @@ import java.util.LinkedList; import java.util.List; import java.util.Map; +import org.jboss.logging.Logger; + /** * @author Marek Posolda */ public class LDAPObject { + private static final Logger logger = Logger.getLogger(LDAPObject.class); + private String uuid; private LDAPDn dn; private String rdnAttributeName; - private final List objectClasses = new LinkedList(); - private final List readOnlyAttributeNames = new LinkedList(); - private final Map attributes = new HashMap(); + private final List objectClasses = new LinkedList<>(); + + // NOTE: names of read-only attributes are lower-cased to avoid case sensitivity issues + private final List readOnlyAttributeNames = new LinkedList<>(); + + private final Map attributes = new HashMap<>(); + + // Copy of "attributes" containing lower-cased keys + private final Map lowerCasedAttributes = new HashMap<>(); + public String getUuid() { return uuid; @@ -49,7 +60,7 @@ public class LDAPObject { } public void addReadOnlyAttributeName(String readOnlyAttribute) { - readOnlyAttributeNames.add(readOnlyAttribute); + readOnlyAttributeNames.add(readOnlyAttribute.toLowerCase()); } public String getRdnAttributeName() { @@ -62,21 +73,23 @@ public class LDAPObject { public void setAttribute(String attributeName, Object attributeValue) { attributes.put(attributeName, attributeValue); + lowerCasedAttributes.put(attributeName.toLowerCase(), attributeValue); } - public void removeAttribute(String name) { - attributes.remove(name); + public Object getAttributeCaseInsensitive(String name) { + return lowerCasedAttributes.get(name.toLowerCase()); } - - public Object getAttribute(String name) { - return attributes.get(name); - } - - public String getAttributeAsString(String name) { - Object attrValue = attributes.get(name); + public String getAttributeAsStringCaseInsensitive(String name) { + Object attrValue = lowerCasedAttributes.get(name.toLowerCase()); if (attrValue != null && !(attrValue instanceof String)) { - throw new IllegalStateException("Expected String but attribute was " + attrValue + " of type " + attrValue.getClass().getName()); + logger.warnf("Expected String but attribute '%s' has value '%s' of type '%s' ", name, attrValue, attrValue.getClass().getName()); + + if (attrValue instanceof Collection) { + Collection attrValues = (Collection) attrValue; + attrValue = attrValues.iterator().next(); + logger.warnf("Returning just first founded value '%s' from the collection", attrValue); + } } return (String) attrValue; diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/query/internal/LDAPIdentityQuery.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/query/internal/LDAPIdentityQuery.java index b3fe0f7f27..ee292b17e7 100644 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/query/internal/LDAPIdentityQuery.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/query/internal/LDAPIdentityQuery.java @@ -40,6 +40,7 @@ public class LDAPIdentityQuery { private final Set returningLdapAttributes = new LinkedHashSet(); // Contains just those returningLdapAttributes, which are read-only. They will be marked as read-only in returned LDAPObject instances as well + // NOTE: names of attributes are lower-cased to avoid case sensitivity issues (LDAP searching is usually case-insensitive, so we want to be as well) private final Set returningReadOnlyLdapAttributes = new LinkedHashSet(); private final Set objectClasses = new LinkedHashSet(); @@ -77,7 +78,7 @@ public class LDAPIdentityQuery { } public LDAPIdentityQuery addReturningReadOnlyLdapAttribute(String ldapAttributeName) { - this.returningReadOnlyLdapAttributes.add(ldapAttributeName); + this.returningReadOnlyLdapAttributes.add(ldapAttributeName.toLowerCase()); return this; } diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/store/ldap/LDAPIdentityStore.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/store/ldap/LDAPIdentityStore.java index 3dbfd0a399..64087ad139 100644 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/store/ldap/LDAPIdentityStore.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/store/ldap/LDAPIdentityStore.java @@ -4,11 +4,11 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; import java.util.Date; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.NoSuchElementException; import java.util.Set; -import java.util.TreeSet; import javax.naming.NamingEnumeration; import javax.naming.NamingException; @@ -382,12 +382,6 @@ public class LDAPIdentityStore implements IdentityStore { NamingEnumeration ldapAttributes = attributes.getAll(); - // Exact name of attributes might be different - List uppercasedReadOnlyAttrNames = new ArrayList<>(); - for (String readonlyAttr : readOnlyAttrNames) { - uppercasedReadOnlyAttrNames.add(readonlyAttr.toUpperCase()); - } - while (ldapAttributes.hasMore()) { Attribute ldapAttribute = ldapAttributes.next(); @@ -403,7 +397,7 @@ public class LDAPIdentityStore implements IdentityStore { Object uuidValue = ldapAttribute.get(); ldapObject.setUuid(this.operationManager.decodeEntryUUID(uuidValue)); } else { - Set attrValues = new TreeSet<>(); + Set attrValues = new LinkedHashSet<>(); NamingEnumeration enumm = ldapAttribute.getAll(); while (enumm.hasMoreElements()) { String attrVal = enumm.next().toString(); @@ -419,7 +413,8 @@ public class LDAPIdentityStore implements IdentityStore { ldapObject.setAttribute(ldapAttributeName, attrValues); } - if (uppercasedReadOnlyAttrNames.contains(ldapAttributeName.toUpperCase())) { + // readOnlyAttrNames are lower-cased + if (readOnlyAttrNames.contains(ldapAttributeName.toLowerCase())) { ldapObject.addReadOnlyAttributeName(ldapAttributeName); } } @@ -443,7 +438,9 @@ public class LDAPIdentityStore implements IdentityStore { for (Map.Entry attrEntry : ldapObject.getAttributes().entrySet()) { String attrName = attrEntry.getKey(); Object attrValue = attrEntry.getValue(); - if (!ldapObject.getReadOnlyAttributeNames().contains(attrName) && (isCreate || !ldapObject.getRdnAttributeName().equalsIgnoreCase(attrName))) { + + // ldapObject.getReadOnlyAttributeNames() are lower-cased + if (!ldapObject.getReadOnlyAttributeNames().contains(attrName.toLowerCase()) && (isCreate || !ldapObject.getRdnAttributeName().equalsIgnoreCase(attrName))) { if (String.class.isInstance(attrValue)) { if (attrValue.toString().trim().length() == 0) { diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/FullNameLDAPFederationMapper.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/FullNameLDAPFederationMapper.java index 7466778b5b..483cc06b68 100644 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/FullNameLDAPFederationMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/FullNameLDAPFederationMapper.java @@ -28,7 +28,7 @@ public class FullNameLDAPFederationMapper extends AbstractLDAPFederationMapper { @Override public void onImportUserFromLDAP(UserFederationMapperModel mapperModel, LDAPFederationProvider ldapProvider, LDAPObject ldapUser, UserModel user, RealmModel realm, boolean isCreate) { String ldapFullNameAttrName = getLdapFullNameAttrName(mapperModel); - String fullName = ldapUser.getAttributeAsString(ldapFullNameAttrName); + String fullName = ldapUser.getAttributeAsStringCaseInsensitive(ldapFullNameAttrName); fullName = fullName.trim(); if (fullName != null && !fullName.trim().isEmpty()) { int lastSpaceIndex = fullName.lastIndexOf(" "); diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/RoleLDAPFederationMapper.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/RoleLDAPFederationMapper.java index 165309c1e7..bc9fb05160 100644 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/RoleLDAPFederationMapper.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/RoleLDAPFederationMapper.java @@ -74,7 +74,7 @@ public class RoleLDAPFederationMapper extends AbstractLDAPFederationMapper { // Import role mappings from LDAP into Keycloak DB String roleNameAttr = getRoleNameLdapAttribute(mapperModel); for (LDAPObject ldapRole : ldapRoles) { - String roleName = ldapRole.getAttributeAsString(roleNameAttr); + String roleName = ldapRole.getAttributeAsStringCaseInsensitive(roleNameAttr); RoleContainerModel roleContainer = getTargetRoleContainer(mapperModel, realm); RoleModel role = roleContainer.getRole(roleName); @@ -103,7 +103,7 @@ public class RoleLDAPFederationMapper extends AbstractLDAPFederationMapper { RoleContainerModel roleContainer = getTargetRoleContainer(mapperModel, realm); String rolesRdnAttr = getRoleNameLdapAttribute(mapperModel); for (LDAPObject ldapRole : ldapRoles) { - String roleName = ldapRole.getAttributeAsString(rolesRdnAttr); + String roleName = ldapRole.getAttributeAsStringCaseInsensitive(rolesRdnAttr); if (roleContainer.getRole(roleName) == null) { logger.infof("Syncing role [%s] from LDAP to keycloak DB", roleName); @@ -249,7 +249,7 @@ public class RoleLDAPFederationMapper extends AbstractLDAPFederationMapper { protected Set getExistingMemberships(UserFederationMapperModel mapperModel, LDAPObject ldapRole) { String memberAttrName = getMembershipLdapAttribute(mapperModel); Set memberships = new TreeSet(); - Object existingMemberships = ldapRole.getAttribute(memberAttrName); + Object existingMemberships = ldapRole.getAttributeCaseInsensitive(memberAttrName); if (existingMemberships != null) { if (existingMemberships instanceof String) { @@ -411,7 +411,7 @@ public class RoleLDAPFederationMapper extends AbstractLDAPFederationMapper { Set roles = new HashSet(); String roleNameLdapAttr = getRoleNameLdapAttribute(mapperModel); for (LDAPObject role : ldapRoles) { - String roleName = role.getAttributeAsString(roleNameLdapAttr); + String roleName = role.getAttributeAsStringCaseInsensitive(roleNameLdapAttr); RoleModel modelRole = roleContainer.getRole(roleName); if (modelRole == null) { // Add role to local DB 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 dd139b0b1f..d5895f6c41 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 @@ -48,7 +48,7 @@ public class UserAttributeLDAPFederationMapper extends AbstractLDAPFederationMap String userModelAttrName = mapperModel.getConfig().get(USER_MODEL_ATTRIBUTE); String ldapAttrName = mapperModel.getConfig().get(LDAP_ATTRIBUTE); - Object ldapAttrValue = ldapUser.getAttribute(ldapAttrName); + Object ldapAttrValue = ldapUser.getAttributeCaseInsensitive(ldapAttrName); if (ldapAttrValue != null && !ldapAttrValue.toString().trim().isEmpty()) { Property userModelProperty = userModelProperties.get(userModelAttrName); 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 4ed0d28372..7f57aff315 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 @@ -262,6 +262,34 @@ public class FederationProvidersIntegrationTest { } } + @Test + public void testCaseSensitiveAttributeName() { + KeycloakSession session = keycloakRule.startSession(); + UserFederationMapperModel zipCodeMapper = null; + + try { + RealmModel appRealm = new RealmManager(session).getRealmByName("test"); + + LDAPFederationProvider ldapFedProvider = FederationTestUtils.getLdapProvider(session, ldapModel); + LDAPObject johnZip = FederationTestUtils.addLDAPUser(ldapFedProvider, appRealm, "johnzip", "John", "Zip", "johnzip@email.org", "12398"); + + // Remove default zipcode mapper + UserFederationMapperModel currentZipMapper = appRealm.getUserFederationMapperByName(ldapModel.getId(), "zipCodeMapper"); + appRealm.removeUserFederationMapper(currentZipMapper); + + // Add zipcode mapper for "POstalCode" + FederationTestUtils.addUserAttributeMapper(appRealm, ldapModel, "zipCodeMapper-cs", "postal_code", "POstalCode"); + + // Fetch user from LDAP and check that postalCode is filled + UserModel user = session.users().getUserByUsername("johnzip", appRealm); + String postalCode = user.getAttribute("postal_code"); + Assert.assertEquals("12398", postalCode); + + } finally { + keycloakRule.stopSession(session, false); + } + } + @Test public void testFullNameMapper() { KeycloakSession session = keycloakRule.startSession(); diff --git a/testsuite/integration/src/test/resources/ldap/users.ldif b/testsuite/integration/src/test/resources/ldap/users.ldif index 9450126233..dcb7904cf6 100644 --- a/testsuite/integration/src/test/resources/ldap/users.ldif +++ b/testsuite/integration/src/test/resources/ldap/users.ldif @@ -19,4 +19,27 @@ objectclass: top objectclass: organizationalUnit ou: FinanceRoles +dn: uid=jbrown,ou=People,dc=keycloak,dc=org +objectclass: top +objectclass: person +objectclass: organizationalPerson +objectclass: inetOrgPerson +uid: jbrown +cn: James +sn: Brown +mail: jbrown@keycloak.org +postalCode: 88441 + +dn: uid=bwilson,ou=People,dc=keycloak,dc=org +objectclass: top +objectclass: person +objectclass: organizationalPerson +objectclass: inetOrgPerson +uid: bwilson +cn: Bruce +sn: Wilson +mail: bwilson@keycloak.org +postalCode: 88441 +postalCode: 77332 +