KEYCLOAK-1728 NPE during LDAP sync when some LDAP user doesn't have username

federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java

@@ -258,16 +258,14 @@ public class LDAPFederationProvider implements UserFederationProvider {
     protected UserModel importUserFromLDAP(KeycloakSession session, RealmModel realm, LDAPObject ldapUser) {
         String ldapUsername = LDAPUtils.getUsername(ldapUser, ldapIdentityStore.getConfig());
 
-        if (ldapUsername == null) {
-            throw new ModelException("User returned from LDAP has null username! Check configuration of your LDAP mappings. Mapped username LDAP attribute: " +
-                    ldapIdentityStore.getConfig().getUsernameLdapAttribute() + ", attributes from LDAP: " + ldapUser.getAttributes());
-        }
-
         UserModel imported = session.userStorage().addUser(realm, ldapUsername);
         imported.setEnabled(true);
 
         Set<UserFederationMapperModel> federationMappers = realm.getUserFederationMappersByFederationProvider(getModel().getId());
         for (UserFederationMapperModel mapperModel : federationMappers) {
+            if (logger.isTraceEnabled()) {
+                logger.tracef("Using mapper %s during import user from LDAP", mapperModel);
+            }
             LDAPFederationMapper ldapMapper = getMapper(mapperModel);
             ldapMapper.onImportUserFromLDAP(mapperModel, this, ldapUser, imported, realm, true);
         }

federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProviderFactory.java

@@ -332,10 +332,17 @@ public class LDAPFederationProviderFactory extends UserFederationEventAwareProvi
                         public void run(KeycloakSession session) {
                             LDAPFederationProvider ldapFedProvider = getInstance(session, fedModel);
                             RealmModel currentRealm = session.realms().getRealm(realmId);
-                            String username = LDAPUtils.getUsername(ldapUser, ldapFedProvider.getLdapIdentityStore().getConfig());
+                            String username = null;
-                            UserModel existing = session.userStorage().getUserByUsername(username, currentRealm);
+                            try {
-                            if (existing != null) {
+                                username = LDAPUtils.getUsername(ldapUser, ldapFedProvider.getLdapIdentityStore().getConfig());
-                                session.userStorage().removeUser(currentRealm, existing);
+                            } catch (ModelException ignore) {
+                            }
+
+                            if (username != null) {
+                                UserModel existing = session.userStorage().getUserByUsername(username, currentRealm);
+                                if (existing != null) {
+                                    session.userStorage().removeUser(currentRealm, existing);
+                                }
                             }
                         }
 

federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPUtils.java

@@ -72,6 +72,13 @@ public class LDAPUtils {
 
     public static String getUsername(LDAPObject ldapUser, LDAPConfig config) {
         String usernameAttr = config.getUsernameLdapAttribute();
-        return ldapUser.getAttributeAsString(usernameAttr);
+        String ldapUsername = ldapUser.getAttributeAsString(usernameAttr);
+
+        if (ldapUsername == null) {
+            throw new ModelException("User returned from LDAP has null username! Check configuration of your LDAP mappings. Mapped username LDAP attribute: " +
+                    config.getUsernameLdapAttribute() + ", user DN: " + ldapUser.getDn() + ", attributes from LDAP: " + ldapUser.getAttributes());
+        }
+
+        return ldapUsername;
     }
 }

federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/model/LDAPObject.java

@@ -129,4 +129,9 @@ public class LDAPObject {
         result = 31 * result + (getUuid() != null ? getUuid().hashCode() : 0);
         return result;
     }
+
+    @Override
+    public String toString() {
+        return "LDAP Object [ dn: " + dn + " , uuid: " + uuid + ", attributes: " + attributes + ", readOnly attribute names: " + readOnlyAttributeNames + " ]";
+    }
 }

federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/store/ldap/LDAPIdentityStore.java

@@ -81,8 +81,8 @@ public class LDAPIdentityStore implements IdentityStore {
         this.operationManager.createSubContext(entryDN, ldapAttributes);
         ldapObject.setUuid(getEntryIdentifier(ldapObject));
 
-        if (logger.isTraceEnabled()) {
+        if (logger.isDebugEnabled()) {
-            logger.tracef("Type with identifier [%s] and dn [%s] successfully added to LDAP store.", ldapObject.getUuid(), entryDN);
+            logger.debugf("Type with identifier [%s] and dn [%s] successfully added to LDAP store.", ldapObject.getUuid(), entryDN);
         }
     }
 
@@ -94,8 +94,8 @@ public class LDAPIdentityStore implements IdentityStore {
         String entryDn = ldapObject.getDn().toString();
         this.operationManager.modifyAttributes(entryDn, attributes);
 
-        if (logger.isTraceEnabled()) {
+        if (logger.isDebugEnabled()) {
-            logger.tracef("Type with identifier [%s] and DN [%s] successfully updated to LDAP store.", ldapObject.getUuid(), entryDn);
+            logger.debugf("Type with identifier [%s] and DN [%s] successfully updated to LDAP store.", ldapObject.getUuid(), entryDn);
         }
     }
 
@@ -103,8 +103,8 @@ public class LDAPIdentityStore implements IdentityStore {
     public void remove(LDAPObject ldapObject) {
         this.operationManager.removeEntry(ldapObject.getDn().toString());
 
-        if (logger.isTraceEnabled()) {
+        if (logger.isDebugEnabled()) {
-            logger.tracef("Type with identifier [%s] and DN [%s] successfully removed from LDAP store.", ldapObject.getUuid(), ldapObject.getDn().toString());
+            logger.debugf("Type with identifier [%s] and DN [%s] successfully removed from LDAP store.", ldapObject.getUuid(), ldapObject.getDn().toString());
         }
     }
 
@@ -429,7 +429,7 @@ public class LDAPIdentityStore implements IdentityStore {
             }
 
             if (logger.isTraceEnabled()) {
-                logger.tracef("Found ldap object [%s] and populated with the attributes [%s]. Read-only attributes are [%s]", ldapObject.getDn().toString(), ldapObject.getAttributes(), ldapObject.getReadOnlyAttributeNames());
+                logger.tracef("Found ldap object and populated with the attributes. LDAP Object: %s", ldapObject.toString());
             }
             return ldapObject;
 

model/api/src/main/java/org/keycloak/models/UserFederationMapperModel.java

@@ -76,4 +76,12 @@ public class UserFederationMapperModel implements Serializable {
     public int hashCode() {
         return id.hashCode();
     }
+
+    @Override
+    public String toString() {
+        return new StringBuilder(" { name=" + name)
+                .append(", federationMapperType=" + federationMapperType)
+                .append(", config=" + config)
+                .append(" } ").toString();
+    }
 }

testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationTestUtils.java

@@ -102,14 +102,14 @@ class FederationTestUtils {
         addUserAttributeMapper(realm, providerModel, "zipCodeMapper", "postal_code", LDAPConstants.POSTAL_CODE);
     }
 
-    public static void addUserAttributeMapper(RealmModel realm, UserFederationProviderModel providerModel, String mapperName, String userModelAttributeName, String ldapAttributeName) {
+    public static UserFederationMapperModel addUserAttributeMapper(RealmModel realm, UserFederationProviderModel providerModel, String mapperName, String userModelAttributeName, String ldapAttributeName) {
         UserFederationMapperModel mapperModel = KeycloakModelUtils.createUserFederationMapperModel(mapperName, providerModel.getId(), UserAttributeLDAPFederationMapperFactory.PROVIDER_ID,
                 UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, userModelAttributeName,
                 UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE, ldapAttributeName,
                 UserAttributeLDAPFederationMapper.READ_ONLY, "false",
                 UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, "false",
                 UserAttributeLDAPFederationMapper.IS_MANDATORY_IN_LDAP, "false");
-        realm.addUserFederationMapper(mapperModel);
+        return realm.addUserFederationMapper(mapperModel);
     }
 
     public static void addOrUpdateRoleLDAPMappers(RealmModel realm, UserFederationProviderModel providerModel, RoleLDAPFederationMapper.Mode mode) {

testsuite/integration/src/test/java/org/keycloak/testsuite/federation/SyncProvidersTest.java

@@ -11,10 +11,12 @@ import org.keycloak.federation.ldap.LDAPConfig;
 import org.keycloak.federation.ldap.LDAPFederationProvider;
 import org.keycloak.federation.ldap.LDAPFederationProviderFactory;
 import org.keycloak.federation.ldap.idm.model.LDAPObject;
+import org.keycloak.federation.ldap.mappers.UserAttributeLDAPFederationMapper;
 import org.keycloak.models.KeycloakSession;
 import org.keycloak.models.KeycloakSessionFactory;
 import org.keycloak.models.LDAPConstants;
 import org.keycloak.models.RealmModel;
+import org.keycloak.models.UserFederationMapperModel;
 import org.keycloak.models.UserFederationProvider;
 import org.keycloak.models.UserFederationProviderModel;
 import org.keycloak.models.UserFederationSyncResult;
@@ -287,6 +289,68 @@ public class SyncProvidersTest {
         }
     }
 
+    // KEYCLOAK-1728
+    @Test
+    public void test04MissingLDAPUsernameSync() {
+        KeycloakSession session = keycloakRule.startSession();
+        String origUsernameAttrName;
+
+        try {
+            RealmModel testRealm = session.realms().getRealm("test");
+
+            // Remove all users from model
+            for (UserModel user : session.userStorage().getUsers(testRealm, true)) {
+                session.userStorage().removeUser(testRealm, user);
+            }
+
+            UserFederationProviderModel providerModel = KeycloakModelUtils.findUserFederationProviderByDisplayName(ldapModel.getDisplayName(), testRealm);
+
+            // Add street mapper and add some user including street
+            UserFederationMapperModel streetMapper = FederationTestUtils.addUserAttributeMapper(testRealm, ldapModel, "streetMapper", "street", LDAPConstants.STREET);
+            LDAPFederationProvider ldapFedProvider = FederationTestUtils.getLdapProvider(session, ldapModel);
+            LDAPObject streetUser = FederationTestUtils.addLDAPUser(ldapFedProvider, testRealm, "user8", "User8FN", "User8LN", "user8@email.org", "user8street", "126");
+
+            // Change name of username attribute name to street
+            origUsernameAttrName = providerModel.getConfig().get(LDAPConstants.USERNAME_LDAP_ATTRIBUTE);
+            providerModel.getConfig().put(LDAPConstants.USERNAME_LDAP_ATTRIBUTE, "street");
+
+            // Need to change this due to ApacheDS pagination bug (For other LDAP servers, pagination works fine) TODO: Remove once ApacheDS upgraded and pagination is fixed
+            providerModel.getConfig().put(LDAPConstants.BATCH_SIZE_FOR_SYNC, "10");
+            testRealm.updateUserFederationProvider(providerModel);
+
+        } finally {
+            keycloakRule.stopSession(session, true);
+        }
+
+        // Just user8 synced. All others failed to sync
+        session = keycloakRule.startSession();
+        try {
+            RealmModel testRealm = session.realms().getRealm("test");
+            UserFederationProviderModel providerModel = KeycloakModelUtils.findUserFederationProviderByDisplayName(ldapModel.getDisplayName(), testRealm);
+
+            KeycloakSessionFactory sessionFactory = session.getKeycloakSessionFactory();
+            UserFederationSyncResult syncResult = new UsersSyncManager().syncAllUsers(sessionFactory, "test", providerModel);
+            Assert.assertEquals(1, syncResult.getAdded());
+            Assert.assertTrue(syncResult.getFailed() > 0);
+        } finally {
+            keycloakRule.stopSession(session, false);
+        }
+
+        session = keycloakRule.startSession();
+        try {
+            RealmModel testRealm = session.realms().getRealm("test");
+
+            // Revert config changes
+            UserFederationProviderModel providerModel = KeycloakModelUtils.findUserFederationProviderByDisplayName(ldapModel.getDisplayName(), testRealm);
+            providerModel.getConfig().put(LDAPConstants.USERNAME_LDAP_ATTRIBUTE, origUsernameAttrName);
+            testRealm.updateUserFederationProvider(providerModel);
+            UserFederationMapperModel streetMapper = testRealm.getUserFederationMapperByName(providerModel.getId(), "streetMapper");
+            testRealm.removeUserFederationMapper(streetMapper);
+        } finally {
+            keycloakRule.stopSession(session, true);
+        }
+    }
+
     @Test
     public void testPeriodicSync() {
         KeycloakSession session = keycloakRule.startSession();