keycloak-aplcache

KEYCLOAK-1487 Fix LDAP case-sensitivity. Show warning in

6/25/2015 2:27:22 PM

Details

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 b7e6c0e..c449484 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 <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
  */
 public class LDAPObject {
 
+    private static final Logger logger = Logger.getLogger(LDAPObject.class);
+
     private String uuid;
     private LDAPDn dn;
     private String rdnAttributeName;
 
-    private final List<String> objectClasses = new LinkedList<String>();
-    private final List<String> readOnlyAttributeNames = new LinkedList<String>();
-    private final Map<String, Object> attributes = new HashMap<String, Object>();
+    private final List<String> objectClasses = new LinkedList<>();
+
+    // NOTE: names of read-only attributes are lower-cased to avoid case sensitivity issues
+    private final List<String> readOnlyAttributeNames = new LinkedList<>();
+
+    private final Map<String, Object> attributes = new HashMap<>();
+
+    // Copy of "attributes" containing lower-cased keys
+    private final Map<String, Object> 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<String> attrValues = (Collection<String>) 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 b3fe0f7..ee292b1 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<String> returningLdapAttributes = new LinkedHashSet<String>();
 
     // 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<String> returningReadOnlyLdapAttributes = new LinkedHashSet<String>();
     private final Set<String> objectClasses = new LinkedHashSet<String>();
 
@@ -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 3dbfd0a..64087ad 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<? extends Attribute> ldapAttributes = attributes.getAll();
 
-            // Exact name of attributes might be different
-            List<String> 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<String> attrValues = new TreeSet<>();
+                    Set<String> 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<String, Object> 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/LDAPUtils.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPUtils.java
index 396d997..27e8df3 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/mappers/FullNameLDAPFederationMapper.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/FullNameLDAPFederationMapper.java
index 7466778..483cc06 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 165309c..bc9fb05 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<String> getExistingMemberships(UserFederationMapperModel mapperModel, LDAPObject ldapRole) {
         String memberAttrName = getMembershipLdapAttribute(mapperModel);
         Set<String> memberships = new TreeSet<String>();
-        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<RoleModel> roles = new HashSet<RoleModel>();
             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 dd139b0..d5895f6 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<Object> 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 4ed0d28..7f57aff 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
@@ -263,6 +263,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();
         UserFederationMapperModel firstNameMapper = null;
diff --git a/testsuite/integration/src/test/resources/ldap/users.ldif b/testsuite/integration/src/test/resources/ldap/users.ldif
index 9450126..dcb7904 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
+