keycloak-uncached
Changes
federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/store/ldap/LDAPIdentityStore.java 14(+7 -7)
federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProviderFactory.java 15(+11 -4)
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 81f058d..cbb28f9 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
@@ -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 + " ]";
+ }
}
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 43057d1..dc93742 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
@@ -81,8 +81,8 @@ public class LDAPIdentityStore implements IdentityStore {
this.operationManager.createSubContext(entryDN, ldapAttributes);
ldapObject.setUuid(getEntryIdentifier(ldapObject));
- if (logger.isTraceEnabled()) {
- logger.tracef("Type with identifier [%s] and dn [%s] successfully added to LDAP store.", ldapObject.getUuid(), entryDN);
+ if (logger.isDebugEnabled()) {
+ 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()) {
- logger.tracef("Type with identifier [%s] and DN [%s] successfully updated to LDAP store.", ldapObject.getUuid(), entryDn);
+ if (logger.isDebugEnabled()) {
+ 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()) {
- logger.tracef("Type with identifier [%s] and DN [%s] successfully removed from LDAP store.", ldapObject.getUuid(), ldapObject.getDn().toString());
+ if (logger.isDebugEnabled()) {
+ 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;
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 4edbc83..102393c 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
@@ -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);
}
diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProviderFactory.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProviderFactory.java
index 3198255..256af7e 100755
--- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProviderFactory.java
+++ b/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());
- UserModel existing = session.userStorage().getUserByUsername(username, currentRealm);
- if (existing != null) {
- session.userStorage().removeUser(currentRealm, existing);
+ String username = null;
+ try {
+ username = LDAPUtils.getUsername(ldapUser, ldapFedProvider.getLdapIdentityStore().getConfig());
+ } catch (ModelException ignore) {
+ }
+
+ if (username != null) {
+ UserModel existing = session.userStorage().getUserByUsername(username, currentRealm);
+ if (existing != null) {
+ session.userStorage().removeUser(currentRealm, existing);
+ }
}
}
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 c731330..4c4e65b 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
@@ -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;
}
}
diff --git a/model/api/src/main/java/org/keycloak/models/UserFederationMapperModel.java b/model/api/src/main/java/org/keycloak/models/UserFederationMapperModel.java
index 80f22be..ea08a58 100644
--- a/model/api/src/main/java/org/keycloak/models/UserFederationMapperModel.java
+++ b/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();
+ }
}
diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationTestUtils.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationTestUtils.java
index 939fe24..63a27e6 100644
--- a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationTestUtils.java
+++ b/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) {
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 e5e893a..f946825 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
@@ -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();