keycloak-aplcache
Changes
federation/ldap/src/main/java/org/keycloak/federation/ldap/idm/store/ldap/LDAPIdentityStore.java 27(+18 -9)
federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProviderFactory.java 131(+99 -32)
federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/RoleLDAPFederationMapper.java 2(+1 -1)
federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/UserAttributeLDAPFederationMapper.java 37(+31 -6)
federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/UserAttributeLDAPFederationMapperFactory.java 6(+5 -1)
testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationProvidersIntegrationTest.java 17(+13 -4)
testsuite/integration/src/test/java/org/keycloak/testsuite/federation/FederationTestUtils.java 9(+5 -4)
Details
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 67e92c7..141ae38 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
@@ -3,6 +3,7 @@ package org.keycloak.federation.ldap.idm.store.ldap;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
+import java.util.Collections;
import java.util.Date;
import java.util.LinkedHashSet;
import java.util.List;
@@ -437,18 +438,26 @@ public class LDAPIdentityStore implements IdentityStore {
// ldapObject.getReadOnlyAttributeNames() are lower-cased
if (!ldapObject.getReadOnlyAttributeNames().contains(attrName.toLowerCase()) && (isCreate || !ldapObject.getRdnAttributeName().equalsIgnoreCase(attrName))) {
- BasicAttribute attr = new BasicAttribute(attrName);
+
if (attrValue == null) {
- // Adding empty value as we don't know if attribute is mandatory in LDAP
- attr.add(LDAPConstants.EMPTY_ATTRIBUTE_VALUE);
- } else {
- for (String val : attrValue) {
- if (val == null || val.toString().trim().length() == 0) {
- val = LDAPConstants.EMPTY_ATTRIBUTE_VALUE;
- }
- attr.add(val);
+ // Shouldn't happen
+ logger.warnf("Attribute '%s' is null on LDAP object '%s' . Using empty value to be saved to LDAP", attrName, ldapObject.getDn().toString());
+ attrValue = Collections.emptySet();
+ }
+
+ // Ignore empty attributes during create
+ if (isCreate && attrValue.isEmpty()) {
+ continue;
+ }
+
+ BasicAttribute attr = new BasicAttribute(attrName);
+ for (String val : attrValue) {
+ if (val == null || val.toString().trim().length() == 0) {
+ val = LDAPConstants.EMPTY_ATTRIBUTE_VALUE;
}
+ attr.add(val);
}
+
entryAttributes.put(attr);
}
}
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 66665bf..b5598e9 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
@@ -13,7 +13,9 @@ import org.keycloak.federation.ldap.kerberos.LDAPProviderKerberosConfig;
import org.keycloak.federation.ldap.mappers.LDAPFederationMapper;
import org.keycloak.models.CredentialValidationOutput;
import org.keycloak.models.KeycloakSession;
+import org.keycloak.models.KeycloakSessionTask;
import org.keycloak.models.LDAPConstants;
+import org.keycloak.models.ModelDuplicateException;
import org.keycloak.models.ModelException;
import org.keycloak.models.RealmModel;
import org.keycloak.models.RoleModel;
@@ -26,6 +28,7 @@ import org.keycloak.models.UserFederationProviderModel;
import org.keycloak.models.UserFederationSyncResult;
import org.keycloak.models.UserModel;
import org.keycloak.constants.KerberosConstants;
+import org.keycloak.models.utils.KeycloakModelUtils;
import java.util.ArrayList;
import java.util.Arrays;
@@ -176,7 +179,7 @@ public class LDAPFederationProvider implements UserFederationProvider {
for (LDAPObject ldapUser : ldapUsers) {
String ldapUsername = LDAPUtils.getUsername(ldapUser, this.ldapIdentityStore.getConfig());
if (session.userStorage().getUserByUsername(ldapUsername, realm) == null) {
- UserModel imported = importUserFromLDAP(realm, ldapUser);
+ UserModel imported = importUserFromLDAP(session, realm, ldapUser);
searchResults.add(imported);
}
}
@@ -249,10 +252,10 @@ public class LDAPFederationProvider implements UserFederationProvider {
return null;
}
- return importUserFromLDAP(realm, ldapUser);
+ return importUserFromLDAP(session, realm, ldapUser);
}
- protected UserModel importUserFromLDAP(RealmModel realm, LDAPObject ldapUser) {
+ protected UserModel importUserFromLDAP(KeycloakSession session, RealmModel realm, LDAPObject ldapUser) {
String ldapUsername = LDAPUtils.getUsername(ldapUser, ldapIdentityStore.getConfig());
if (ldapUsername == null) {
@@ -298,7 +301,7 @@ public class LDAPFederationProvider implements UserFederationProvider {
return null;
}
- return importUserFromLDAP(realm, ldapUser);
+ return importUserFromLDAP(session, realm, ldapUser);
}
@Override
@@ -383,38 +386,6 @@ public class LDAPFederationProvider implements UserFederationProvider {
public void close() {
}
- protected UserFederationSyncResult importLDAPUsers(RealmModel realm, List<LDAPObject> ldapUsers, UserFederationProviderModel fedModel) {
- UserFederationSyncResult syncResult = new UserFederationSyncResult();
-
- for (LDAPObject ldapUser : ldapUsers) {
- String username = LDAPUtils.getUsername(ldapUser, ldapIdentityStore.getConfig());
- UserModel currentUser = session.userStorage().getUserByUsername(username, realm);
-
- if (currentUser == null) {
- // Add new user to Keycloak
- importUserFromLDAP(realm, ldapUser);
- syncResult.increaseAdded();
- } else {
- if ((fedModel.getId().equals(currentUser.getFederationLink())) && (ldapUser.getUuid().equals(currentUser.getFirstAttribute(LDAPConstants.LDAP_ID)))) {
-
- // Update keycloak user
- Set<UserFederationMapperModel> federationMappers = realm.getUserFederationMappersByFederationProvider(model.getId());
- for (UserFederationMapperModel mapperModel : federationMappers) {
- LDAPFederationMapper ldapMapper = getMapper(mapperModel);
- ldapMapper.onImportUserFromLDAP(mapperModel, this, ldapUser, currentUser, realm, false);
- }
-
- logger.debugf("Updated user from LDAP: %s", currentUser.getUsername());
- syncResult.increaseUpdated();
- } else {
- logger.warnf("User '%s' is not updated during sync as he is not linked to federation provider '%s'", username, fedModel.getDisplayName());
- }
- }
- }
-
- return syncResult;
- }
-
/**
* Called after successful kerberos authentication
*
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 2b60ff8..a71b84d 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
@@ -14,12 +14,15 @@ import org.keycloak.federation.ldap.idm.query.internal.LDAPQueryConditionsBuilde
import org.keycloak.federation.ldap.idm.store.ldap.LDAPIdentityStore;
import org.keycloak.federation.ldap.mappers.FullNameLDAPFederationMapper;
import org.keycloak.federation.ldap.mappers.FullNameLDAPFederationMapperFactory;
+import org.keycloak.federation.ldap.mappers.LDAPFederationMapper;
import org.keycloak.federation.ldap.mappers.UserAttributeLDAPFederationMapper;
import org.keycloak.federation.ldap.mappers.UserAttributeLDAPFederationMapperFactory;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.KeycloakSessionFactory;
import org.keycloak.models.KeycloakSessionTask;
import org.keycloak.models.LDAPConstants;
+import org.keycloak.models.ModelDuplicateException;
+import org.keycloak.models.ModelException;
import org.keycloak.models.RealmModel;
import org.keycloak.models.UserFederationEventAwareProviderFactory;
import org.keycloak.models.UserFederationMapperModel;
@@ -94,7 +97,8 @@ public class LDAPFederationProviderFactory extends UserFederationEventAwareProvi
UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, UserModel.USERNAME,
UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE, usernameLdapAttribute,
UserAttributeLDAPFederationMapper.READ_ONLY, readOnly,
- UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, "false");
+ UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, "false",
+ UserAttributeLDAPFederationMapper.IS_MANDATORY_IN_LDAP, "true");
realm.addUserFederationMapper(mapperModel);
// CN is typically used as RDN for Active Directory deployments
@@ -107,7 +111,8 @@ public class LDAPFederationProviderFactory extends UserFederationEventAwareProvi
UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, UserModel.FIRST_NAME,
UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE, LDAPConstants.GIVENNAME,
UserAttributeLDAPFederationMapper.READ_ONLY, readOnly,
- UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, alwaysReadValueFromLDAP);
+ UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, alwaysReadValueFromLDAP,
+ UserAttributeLDAPFederationMapper.IS_MANDATORY_IN_LDAP, "true");
realm.addUserFederationMapper(mapperModel);
} else {
@@ -118,14 +123,16 @@ public class LDAPFederationProviderFactory extends UserFederationEventAwareProvi
UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, UserModel.FIRST_NAME,
UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE, LDAPConstants.GIVENNAME,
UserAttributeLDAPFederationMapper.READ_ONLY, readOnly,
- UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, alwaysReadValueFromLDAP);
+ UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, alwaysReadValueFromLDAP,
+ UserAttributeLDAPFederationMapper.IS_MANDATORY_IN_LDAP, "true");
realm.addUserFederationMapper(mapperModel);
mapperModel = KeycloakModelUtils.createUserFederationMapperModel("username-cn", newProviderModel.getId(), UserAttributeLDAPFederationMapperFactory.PROVIDER_ID,
UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, UserModel.USERNAME,
UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE, LDAPConstants.CN,
UserAttributeLDAPFederationMapper.READ_ONLY, readOnly,
- UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, "false");
+ UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, "false",
+ UserAttributeLDAPFederationMapper.IS_MANDATORY_IN_LDAP, "true");
realm.addUserFederationMapper(mapperModel);
} else {
@@ -141,7 +148,8 @@ public class LDAPFederationProviderFactory extends UserFederationEventAwareProvi
UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, UserModel.FIRST_NAME,
UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE, LDAPConstants.CN,
UserAttributeLDAPFederationMapper.READ_ONLY, readOnly,
- UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, alwaysReadValueFromLDAP);
+ UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, alwaysReadValueFromLDAP,
+ UserAttributeLDAPFederationMapper.IS_MANDATORY_IN_LDAP, "true");
realm.addUserFederationMapper(mapperModel);
}
@@ -149,14 +157,16 @@ public class LDAPFederationProviderFactory extends UserFederationEventAwareProvi
UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, UserModel.LAST_NAME,
UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE, LDAPConstants.SN,
UserAttributeLDAPFederationMapper.READ_ONLY, readOnly,
- UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, alwaysReadValueFromLDAP);
+ UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, alwaysReadValueFromLDAP,
+ UserAttributeLDAPFederationMapper.IS_MANDATORY_IN_LDAP, "true");
realm.addUserFederationMapper(mapperModel);
mapperModel = KeycloakModelUtils.createUserFederationMapperModel("email", newProviderModel.getId(), UserAttributeLDAPFederationMapperFactory.PROVIDER_ID,
UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, UserModel.EMAIL,
UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE, LDAPConstants.EMAIL,
UserAttributeLDAPFederationMapper.READ_ONLY, readOnly,
- UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, alwaysReadValueFromLDAP);
+ UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, "false",
+ UserAttributeLDAPFederationMapper.IS_MANDATORY_IN_LDAP, "false");
realm.addUserFederationMapper(mapperModel);
String createTimestampLdapAttrName = activeDirectory ? "whenCreated" : LDAPConstants.CREATE_TIMESTAMP;
@@ -167,7 +177,8 @@ public class LDAPFederationProviderFactory extends UserFederationEventAwareProvi
UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, LDAPConstants.CREATE_TIMESTAMP,
UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE, createTimestampLdapAttrName,
UserAttributeLDAPFederationMapper.READ_ONLY, "true",
- UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, alwaysReadValueFromLDAP);
+ UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, alwaysReadValueFromLDAP,
+ UserAttributeLDAPFederationMapper.IS_MANDATORY_IN_LDAP, "false");
realm.addUserFederationMapper(mapperModel);
// map modifyTimeStamp as read-only
@@ -175,7 +186,8 @@ public class LDAPFederationProviderFactory extends UserFederationEventAwareProvi
UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, LDAPConstants.MODIFY_TIMESTAMP,
UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE, modifyTimestampLdapAttrName,
UserAttributeLDAPFederationMapper.READ_ONLY, "true",
- UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, alwaysReadValueFromLDAP);
+ UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, alwaysReadValueFromLDAP,
+ UserAttributeLDAPFederationMapper.IS_MANDATORY_IN_LDAP, "false");
realm.addUserFederationMapper(mapperModel);
}
@@ -226,29 +238,14 @@ public class LDAPFederationProviderFactory extends UserFederationEventAwareProvi
userQuery.setLimit(pageSize);
final List<LDAPObject> users = userQuery.getResultList();
nextPage = userQuery.getPaginationContext() != null;
-
- KeycloakModelUtils.runJobInTransaction(sessionFactory, new KeycloakSessionTask() {
-
- @Override
- public void run(KeycloakSession session) {
- UserFederationSyncResult currentPageSync = importLdapUsers(session, realmId, fedModel, users);
- syncResult.add(currentPageSync);
- }
-
- });
+ UserFederationSyncResult currentPageSync = importLdapUsers(sessionFactory, realmId, fedModel, users);
+ syncResult.add(currentPageSync);
}
} else {
// LDAP pagination not available. Do everything in single transaction
final List<LDAPObject> users = userQuery.getResultList();
- KeycloakModelUtils.runJobInTransaction(sessionFactory, new KeycloakSessionTask() {
-
- @Override
- public void run(KeycloakSession session) {
- UserFederationSyncResult currentSync = importLdapUsers(session, realmId, fedModel, users);
- syncResult.add(currentSync);
- }
-
- });
+ UserFederationSyncResult currentSync = importLdapUsers(sessionFactory, realmId, fedModel, users);
+ syncResult.add(currentSync);
}
return syncResult;
@@ -273,11 +270,81 @@ public class LDAPFederationProviderFactory extends UserFederationEventAwareProvi
return queryHolder.query;
}
+ protected UserFederationSyncResult importLdapUsers(KeycloakSessionFactory sessionFactory, final String realmId, final UserFederationProviderModel fedModel, List<LDAPObject> ldapUsers) {
+ final UserFederationSyncResult syncResult = new UserFederationSyncResult();
+
+ class BooleanHolder {
+ private boolean value = true;
+ }
+ final BooleanHolder exists = new BooleanHolder();
+
+ for (final LDAPObject ldapUser : ldapUsers) {
+
+ try {
+
+ // Process each user in it's own transaction to avoid global fail
+ KeycloakModelUtils.runJobInTransaction(sessionFactory, new KeycloakSessionTask() {
+
+ @Override
+ 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 currentUser = session.userStorage().getUserByUsername(username, currentRealm);
+
+ if (currentUser == null) {
+
+ // Add new user to Keycloak
+ exists.value = false;
+ ldapFedProvider.importUserFromLDAP(session, currentRealm, ldapUser);
+ syncResult.increaseAdded();
+
+ } else {
+ if ((fedModel.getId().equals(currentUser.getFederationLink())) && (ldapUser.getUuid().equals(currentUser.getFirstAttribute(LDAPConstants.LDAP_ID)))) {
+
+ // Update keycloak user
+ Set<UserFederationMapperModel> federationMappers = currentRealm.getUserFederationMappersByFederationProvider(fedModel.getId());
+ for (UserFederationMapperModel mapperModel : federationMappers) {
+ LDAPFederationMapper ldapMapper = ldapFedProvider.getMapper(mapperModel);
+ ldapMapper.onImportUserFromLDAP(mapperModel, ldapFedProvider, ldapUser, currentUser, currentRealm, false);
+ }
+
+ logger.debugf("Updated user from LDAP: %s", currentUser.getUsername());
+ syncResult.increaseUpdated();
+ } else {
+ logger.warnf("User '%s' is not updated during sync as he already exists in Keycloak database but is not linked to federation provider '%s'", username, fedModel.getDisplayName());
+ syncResult.increaseFailed();
+ }
+ }
+ }
+
+ });
+ } catch (ModelException me) {
+ logger.error("Failed during import user from LDAP", me);
+ syncResult.increaseFailed();
+
+ // Remove user if we already added him during this transaction
+ if (!exists.value) {
+ KeycloakModelUtils.runJobInTransaction(sessionFactory, new KeycloakSessionTask() {
+
+ @Override
+ 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);
+ }
+ }
+
+ });
+ }
+ }
+ }
- protected UserFederationSyncResult importLdapUsers(KeycloakSession session, String realmId, UserFederationProviderModel fedModel, List<LDAPObject> ldapUsers) {
- RealmModel realm = session.realms().getRealm(realmId);
- LDAPFederationProvider ldapFedProvider = getInstance(session, fedModel);
- return ldapFedProvider.importLDAPUsers(realm, ldapUsers, fedModel);
+ return syncResult;
}
protected SPNEGOAuthenticator createSPNEGOAuthenticator(String spnegoToken, CommonKerberosConfig kerberosConfig) {
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 4565f88..e29ab6f 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
@@ -239,7 +239,7 @@ public class RoleLDAPFederationMapper extends AbstractLDAPFederationMapper {
Set<String> memberships = getExistingMemberships(mapperModel, ldapRole);
memberships.remove(ldapUser.getDn().toString());
- // Some membership placeholder needs to be always here as "member" is mandatory attribute on some LDAP servers. But on active directory! (Empty membership is not allowed here)
+ // Some membership placeholder needs to be always here as "member" is mandatory attribute on some LDAP servers. But not on active directory! (Empty membership is not allowed here)
if (memberships.size() == 0 && !ldapProvider.getLdapIdentityStore().getConfig().isActiveDirectory()) {
memberships.add(LDAPConstants.EMPTY_MEMBER_ATTRIBUTE_VALUE);
}
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 24bf450..283089a 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
@@ -3,6 +3,7 @@ package org.keycloak.federation.ldap.mappers;
import java.lang.reflect.Method;
import java.util.ArrayList;
import java.util.Collection;
+import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
@@ -15,6 +16,7 @@ import org.keycloak.federation.ldap.idm.model.LDAPObject;
import org.keycloak.federation.ldap.idm.query.Condition;
import org.keycloak.federation.ldap.idm.query.QueryParameter;
import org.keycloak.federation.ldap.idm.query.internal.LDAPQuery;
+import org.keycloak.models.LDAPConstants;
import org.keycloak.models.RealmModel;
import org.keycloak.models.UserFederationMapperModel;
import org.keycloak.models.UserFederationProvider;
@@ -58,6 +60,7 @@ public class UserAttributeLDAPFederationMapper extends AbstractLDAPFederationMap
public static final String LDAP_ATTRIBUTE = "ldap.attribute";
public static final String READ_ONLY = "read.only";
public static final String ALWAYS_READ_VALUE_FROM_LDAP = "always.read.value.from.ldap";
+ public static final String IS_MANDATORY_IN_LDAP = "is.mandatory.in.ldap";
@Override
@@ -88,6 +91,7 @@ public class UserAttributeLDAPFederationMapper extends AbstractLDAPFederationMap
public void onRegisterUserToLDAP(UserFederationMapperModel mapperModel, LDAPFederationProvider ldapProvider, LDAPObject ldapUser, UserModel localUser, RealmModel realm) {
String userModelAttrName = mapperModel.getConfig().get(USER_MODEL_ATTRIBUTE);
String ldapAttrName = mapperModel.getConfig().get(LDAP_ATTRIBUTE);
+ boolean isMandatoryInLdap = parseBooleanParameter(mapperModel, IS_MANDATORY_IN_LDAP);
Property<Object> userModelProperty = userModelProperties.get(userModelAttrName.toLowerCase());
@@ -95,15 +99,27 @@ public class UserAttributeLDAPFederationMapper extends AbstractLDAPFederationMap
// we have java property on UserModel. Assuming we support just properties of simple types
Object attrValue = userModelProperty.getValue(localUser);
- String valueAsString = (attrValue == null) ? null : attrValue.toString();
- ldapUser.setSingleAttribute(ldapAttrName, valueAsString);
+
+ if (attrValue == null) {
+ if (isMandatoryInLdap) {
+ ldapUser.setSingleAttribute(ldapAttrName, LDAPConstants.EMPTY_ATTRIBUTE_VALUE);
+ } else {
+ ldapUser.setAttribute(ldapAttrName, new LinkedHashSet<String>());
+ }
+ } else {
+ ldapUser.setSingleAttribute(ldapAttrName, attrValue.toString());
+ }
} else {
// we don't have java property. Let's set attribute
List<String> attrValues = localUser.getAttribute(userModelAttrName);
if (attrValues.size() == 0) {
- ldapUser.setAttribute(ldapAttrName, null);
+ if (isMandatoryInLdap) {
+ ldapUser.setSingleAttribute(ldapAttrName, LDAPConstants.EMPTY_ATTRIBUTE_VALUE);
+ } else {
+ ldapUser.setAttribute(ldapAttrName, new LinkedHashSet<String>());
+ }
} else {
ldapUser.setAttribute(ldapAttrName, new LinkedHashSet<>(attrValues));
}
@@ -119,6 +135,7 @@ public class UserAttributeLDAPFederationMapper extends AbstractLDAPFederationMap
final String userModelAttrName = mapperModel.getConfig().get(USER_MODEL_ATTRIBUTE);
final String ldapAttrName = mapperModel.getConfig().get(LDAP_ATTRIBUTE);
boolean isAlwaysReadValueFromLDAP = parseBooleanParameter(mapperModel, ALWAYS_READ_VALUE_FROM_LDAP);
+ final boolean isMandatoryInLdap = parseBooleanParameter(mapperModel, IS_MANDATORY_IN_LDAP);
// For writable mode, we want to propagate writing of attribute to LDAP as well
if (ldapProvider.getEditMode() == UserFederationProvider.EditMode.WRITABLE && !isReadOnly(mapperModel)) {
@@ -170,12 +187,20 @@ public class UserAttributeLDAPFederationMapper extends AbstractLDAPFederationMap
ensureTransactionStarted();
if (value == null) {
- ldapUser.setAttribute(ldapAttrName, null);
+ if (isMandatoryInLdap) {
+ ldapUser.setSingleAttribute(ldapAttrName, LDAPConstants.EMPTY_ATTRIBUTE_VALUE);
+ } else {
+ ldapUser.setAttribute(ldapAttrName, new LinkedHashSet<String>());
+ }
} else if (value instanceof String) {
ldapUser.setSingleAttribute(ldapAttrName, (String) value);
} else {
List<String> asList = (List<String>) value;
- ldapUser.setAttribute(ldapAttrName, new LinkedHashSet<>(asList));
+ if (asList.isEmpty() && isMandatoryInLdap) {
+ ldapUser.setSingleAttribute(ldapAttrName, LDAPConstants.EMPTY_ATTRIBUTE_VALUE);
+ } else {
+ ldapUser.setAttribute(ldapAttrName, new LinkedHashSet<>(asList));
+ }
}
}
}
@@ -203,7 +228,7 @@ public class UserAttributeLDAPFederationMapper extends AbstractLDAPFederationMap
if (name.equalsIgnoreCase(userModelAttrName)) {
Collection<String> ldapAttrValue = ldapUser.getAttributeAsSet(ldapAttrName);
if (ldapAttrValue == null) {
- return null;
+ return Collections.emptyList();
} else {
return new ArrayList<>(ldapAttrValue);
}
diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/UserAttributeLDAPFederationMapperFactory.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/UserAttributeLDAPFederationMapperFactory.java
index 1b1b44d..c14d0e8 100644
--- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/UserAttributeLDAPFederationMapperFactory.java
+++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/UserAttributeLDAPFederationMapperFactory.java
@@ -31,9 +31,13 @@ public class UserAttributeLDAPFederationMapperFactory extends AbstractLDAPFedera
"Read-only attribute is imported from LDAP to Keycloak DB, but it's not saved back to LDAP when user is updated in Keycloak.", ProviderConfigProperty.BOOLEAN_TYPE, "false");
configProperties.add(readOnly);
- ProviderConfigProperty alwaysReadValueFromLDAP = createConfigProperty(UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, "Always read value from LDAP",
+ ProviderConfigProperty alwaysReadValueFromLDAP = createConfigProperty(UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, "Always Read Value From LDAP",
"If on, then during reading of the user will be value of attribute from LDAP always used instead of the value from Keycloak DB", ProviderConfigProperty.BOOLEAN_TYPE, "false");
configProperties.add(alwaysReadValueFromLDAP);
+
+ ProviderConfigProperty isMandatoryInLdap = createConfigProperty(UserAttributeLDAPFederationMapper.IS_MANDATORY_IN_LDAP, "Is Mandatory In LDAP",
+ "If true, attribute is mandatory in LDAP. Hence if there is no value in Keycloak DB, the empty value will be set to be propagated to LDAP", ProviderConfigProperty.BOOLEAN_TYPE, "false");
+ configProperties.add(isMandatoryInLdap);
}
@Override
diff --git a/model/api/src/main/java/org/keycloak/models/UserFederationManager.java b/model/api/src/main/java/org/keycloak/models/UserFederationManager.java
index 7d9e7ca..8bca31c 100755
--- a/model/api/src/main/java/org/keycloak/models/UserFederationManager.java
+++ b/model/api/src/main/java/org/keycloak/models/UserFederationManager.java
@@ -128,7 +128,7 @@ public class UserFederationManager implements UserProvider {
if (link != null) {
UserModel validatedProxyUser = link.validateAndProxy(realm, user);
if (validatedProxyUser != null) {
- managedUsers.put(user.getId(), user);
+ managedUsers.put(user.getId(), validatedProxyUser);
return validatedProxyUser;
} else {
deleteInvalidUser(realm, user);
diff --git a/model/api/src/main/java/org/keycloak/models/UserFederationSyncResult.java b/model/api/src/main/java/org/keycloak/models/UserFederationSyncResult.java
index e6d465b..b06348d 100644
--- a/model/api/src/main/java/org/keycloak/models/UserFederationSyncResult.java
+++ b/model/api/src/main/java/org/keycloak/models/UserFederationSyncResult.java
@@ -8,6 +8,7 @@ public class UserFederationSyncResult {
private int added;
private int updated;
private int removed;
+ private int failed;
public int getAdded() {
return added;
@@ -33,6 +34,14 @@ public class UserFederationSyncResult {
this.removed = removed;
}
+ public int getFailed() {
+ return failed;
+ }
+
+ public void setFailed(int failed) {
+ this.failed = failed;
+ }
+
public void increaseAdded() {
added++;
}
@@ -45,14 +54,23 @@ public class UserFederationSyncResult {
removed++;
}
+ public void increaseFailed() {
+ failed++;
+ }
+
public void add(UserFederationSyncResult other) {
added += other.added;
updated += other.updated;
removed += other.removed;
+ failed += other.failed;
}
public String getStatus() {
- return String.format("%d imported users, %d updated users, %d removed users", added, updated, removed);
+ String status = String.format("%d imported users, %d updated users, %d removed users", added, updated, removed);
+ if (failed != 0) {
+ status += String.format(", %d users failed sync! See server log for more details", failed);
+ }
+ return status;
}
@Override
diff --git a/services/src/main/java/org/keycloak/services/DefaultKeycloakTransactionManager.java b/services/src/main/java/org/keycloak/services/DefaultKeycloakTransactionManager.java
index 75f6096..c39bc8a 100755
--- a/services/src/main/java/org/keycloak/services/DefaultKeycloakTransactionManager.java
+++ b/services/src/main/java/org/keycloak/services/DefaultKeycloakTransactionManager.java
@@ -1,5 +1,6 @@
package org.keycloak.services;
+import org.jboss.logging.Logger;
import org.keycloak.models.KeycloakTransaction;
import org.keycloak.models.KeycloakTransactionManager;
@@ -11,6 +12,8 @@ import java.util.List;
*/
public class DefaultKeycloakTransactionManager implements KeycloakTransactionManager {
+ public static final Logger logger = Logger.getLogger(DefaultKeycloakTransactionManager.class);
+
private List<KeycloakTransaction> transactions = new LinkedList<KeycloakTransaction>();
private List<KeycloakTransaction> afterCompletion = new LinkedList<KeycloakTransaction>();
private boolean active;
@@ -57,13 +60,26 @@ public class DefaultKeycloakTransactionManager implements KeycloakTransactionMan
exception = exception == null ? e : exception;
}
}
- for (KeycloakTransaction tx : afterCompletion) {
- try {
- tx.commit();
- } catch (RuntimeException e) {
- exception = exception == null ? e : exception;
+
+ // Don't commit "afterCompletion" if commit of some main transaction failed
+ if (exception == null) {
+ for (KeycloakTransaction tx : afterCompletion) {
+ try {
+ tx.commit();
+ } catch (RuntimeException e) {
+ exception = exception == null ? e : exception;
+ }
+ }
+ } else {
+ for (KeycloakTransaction tx : afterCompletion) {
+ try {
+ tx.rollback();
+ } catch (RuntimeException e) {
+ logger.error("Exception during rollback", e);
+ }
}
}
+
active = false;
if (exception != null) {
throw exception;
diff --git a/services/src/main/java/org/keycloak/services/messages/AdminMessagesProvider.java b/services/src/main/java/org/keycloak/services/messages/AdminMessagesProvider.java
index 1da7bfb..7b6b3ed 100644
--- a/services/src/main/java/org/keycloak/services/messages/AdminMessagesProvider.java
+++ b/services/src/main/java/org/keycloak/services/messages/AdminMessagesProvider.java
@@ -29,7 +29,13 @@ public class AdminMessagesProvider implements MessagesProvider {
@Override
public String getMessage(String messageKey, Object... parameters) {
String message = messagesBundle.getProperty(messageKey, messageKey);
- return new MessageFormat(message, locale).format(parameters);
+
+ try {
+ return new MessageFormat(message, locale).format(parameters);
+ } catch (Exception e) {
+ logger.warnf("Failed to format message due to: %s", e.getMessage());
+ return message;
+ }
}
@Override
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 5ce1b12..1945701 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
@@ -307,9 +307,18 @@ public class FederationProvidersIntegrationTest {
johnDirect.setSingleAttribute(LDAPConstants.SN, "DirectLDAPUpdated");
ldapFedProvider.getLdapIdentityStore().update(johnDirect);
+ } finally {
+ keycloakRule.stopSession(session, true);
+ }
+
+ session = keycloakRule.startSession();
+ try {
+ RealmModel appRealm = new RealmManager(session).getRealmByName("test");
+ UserModel user = session.users().getUserByUsername("johndirect", appRealm);
+
// Verify that postalCode is still the same as we read it's value from Keycloak DB
user = session.users().getUserByUsername("johndirect", appRealm);
- postalCode = user.getFirstAttribute("postal_code");
+ String postalCode = user.getFirstAttribute("postal_code");
Assert.assertEquals("12399", postalCode);
// Check user.getAttributes()
@@ -381,9 +390,6 @@ public class FederationProvidersIntegrationTest {
FullNameLDAPFederationMapper.LDAP_FULL_NAME_ATTRIBUTE, ldapFirstNameAttributeName,
UserAttributeLDAPFederationMapper.READ_ONLY, "false");
appRealm.addUserFederationMapper(fullNameMapperModel);
-
- // Assert user is successfully imported in Keycloak DB now with correct firstName and lastName
- FederationTestUtils.assertUserImported(session.users(), appRealm, "fullname", "James", "Dee", "fullname@email.org", "4578");
} finally {
keycloakRule.stopSession(session, true);
}
@@ -392,6 +398,9 @@ public class FederationProvidersIntegrationTest {
try {
RealmModel appRealm = new RealmManager(session).getRealmByName("test");
+ // Assert user is successfully imported in Keycloak DB now with correct firstName and lastName
+ FederationTestUtils.assertUserImported(session.users(), appRealm, "fullname", "James", "Dee", "fullname@email.org", "4578");
+
// Remove "fullnameUser" to assert he is removed from LDAP. Revert mappers to previous state
UserModel fullnameUser = session.users().getUserByUsername("fullname", appRealm);
session.users().removeUser(appRealm, fullnameUser);
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 7e06bc6..939fe24 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
@@ -34,7 +34,7 @@ import org.keycloak.representations.idm.CredentialRepresentation;
class FederationTestUtils {
public static UserModel addLocalUser(KeycloakSession session, RealmModel realm, String username, String email, String password) {
- UserModel user = session.users().addUser(realm, username);
+ UserModel user = session.userStorage().addUser(realm, username);
user.setEmail(email);
user.setEnabled(true);
@@ -72,9 +72,9 @@ class FederationTestUtils {
@Override
public List<String> getAttribute(String name) {
- if ("postal_code".equals(name)) {
+ if ("postal_code".equals(name) && postalCode != null && postalCode.length > 0) {
return Arrays.asList(postalCode);
- } else if ("street".equals(name)) {
+ } else if ("street".equals(name) && street != null) {
return Arrays.asList(street);
} else {
return Collections.emptyList();
@@ -107,7 +107,8 @@ class FederationTestUtils {
UserAttributeLDAPFederationMapper.USER_MODEL_ATTRIBUTE, userModelAttributeName,
UserAttributeLDAPFederationMapper.LDAP_ATTRIBUTE, ldapAttributeName,
UserAttributeLDAPFederationMapper.READ_ONLY, "false",
- UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, "false");
+ UserAttributeLDAPFederationMapper.ALWAYS_READ_VALUE_FROM_LDAP, "false",
+ UserAttributeLDAPFederationMapper.IS_MANDATORY_IN_LDAP, "false");
realm.addUserFederationMapper(mapperModel);
}
diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/LDAPMultipleAttributesTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/LDAPMultipleAttributesTest.java
index 556053d..edd210c 100644
--- a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/LDAPMultipleAttributesTest.java
+++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/LDAPMultipleAttributesTest.java
@@ -68,7 +68,7 @@ public class LDAPMultipleAttributesTest {
LDAPFederationProvider ldapFedProvider = FederationTestUtils.getLdapProvider(session, ldapModel);
FederationTestUtils.removeAllLDAPUsers(ldapFedProvider, appRealm);
- LDAPObject james = FederationTestUtils.addLDAPUser(ldapFedProvider, appRealm, "jbrown", "James", "Brown", "jbrown@keycloak.org", "", "88441");
+ LDAPObject james = FederationTestUtils.addLDAPUser(ldapFedProvider, appRealm, "jbrown", "James", "Brown", "jbrown@keycloak.org", null, "88441");
ldapFedProvider.getLdapIdentityStore().updatePassword(james, "password");
// User for testing duplicating surname and postalCode
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 ab2f46e..e29a2b8 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
@@ -81,7 +81,7 @@ public class SyncProvidersTest {
// }
@Test
- public void testLDAPSync() {
+ public void test01LDAPSync() {
UsersSyncManager usersSyncManager = new UsersSyncManager();
// wait a bit
@@ -91,7 +91,7 @@ public class SyncProvidersTest {
try {
KeycloakSessionFactory sessionFactory = session.getKeycloakSessionFactory();
UserFederationSyncResult syncResult = usersSyncManager.syncAllUsers(sessionFactory, "test", ldapModel);
- assertSyncEquals(syncResult, 5, 0, 0);
+ assertSyncEquals(syncResult, 5, 0, 0, 0);
} finally {
keycloakRule.stopSession(session, false);
}
@@ -137,7 +137,7 @@ public class SyncProvidersTest {
// Trigger partial sync
KeycloakSessionFactory sessionFactory = session.getKeycloakSessionFactory();
UserFederationSyncResult syncResult = usersSyncManager.syncChangedUsers(sessionFactory, "test", ldapModel);
- assertSyncEquals(syncResult, 1, 1, 0);
+ assertSyncEquals(syncResult, 1, 1, 0, 0);
} finally {
keycloakRule.stopSession(session, false);
}
@@ -155,6 +155,67 @@ public class SyncProvidersTest {
}
@Test
+ public void test02duplicateUsernameSync() {
+ LDAPObject duplicatedLdapUser;
+
+ KeycloakSession session = keycloakRule.startSession();
+ try {
+ RealmModel testRealm = session.realms().getRealm("test");
+
+ FederationTestUtils.addLocalUser(session, testRealm, "user7", "user7@email.org", "password");
+ LDAPFederationProvider ldapFedProvider = FederationTestUtils.getLdapProvider(session, ldapModel);
+
+ // Add user to LDAP with duplicated username "user7"
+ duplicatedLdapUser = FederationTestUtils.addLDAPUser(ldapFedProvider, testRealm, "user7", "User7FN", "User7LN", "user7-something@email.org", null, "126");
+
+ // Add user to LDAP with duplicated email "user7@email.org"
+ //FederationTestUtils.addLDAPUser(ldapFedProvider, testRealm, "user7-something", "User7FNN", "User7LNL", "user7@email.org", null, "126");
+ } finally {
+ keycloakRule.stopSession(session, true);
+ }
+
+ session = keycloakRule.startSession();
+ try {
+ RealmModel testRealm = session.realms().getRealm("test");
+
+ // Assert syncing from LDAP fails due to duplicated username
+ UserFederationSyncResult result = new UsersSyncManager().syncAllUsers(session.getKeycloakSessionFactory(), "test", ldapModel);
+ Assert.assertEquals(1, result.getFailed());
+
+ // Remove "user7" from LDAP
+ LDAPFederationProvider ldapFedProvider = FederationTestUtils.getLdapProvider(session, ldapModel);
+ ldapFedProvider.getLdapIdentityStore().remove(duplicatedLdapUser);
+
+ // Add user to LDAP with duplicated email "user7@email.org"
+ duplicatedLdapUser = FederationTestUtils.addLDAPUser(ldapFedProvider, testRealm, "user7-something", "User7FNN", "User7LNL", "user7@email.org", null, "126");
+ } finally {
+ keycloakRule.stopSession(session, true);
+ }
+
+ session = keycloakRule.startSession();
+ try {
+ RealmModel testRealm = session.realms().getRealm("test");
+
+ // Assert syncing from LDAP fails due to duplicated email
+ UserFederationSyncResult result = new UsersSyncManager().syncAllUsers(session.getKeycloakSessionFactory(), "test", ldapModel);
+ Assert.assertEquals(1, result.getFailed());
+ Assert.assertNull(session.userStorage().getUserByUsername("user7-something", testRealm));
+
+ // Update LDAP user to avoid duplicated email
+ duplicatedLdapUser.setSingleAttribute(LDAPConstants.EMAIL, "user7-changed@email.org");
+ LDAPFederationProvider ldapFedProvider = FederationTestUtils.getLdapProvider(session, ldapModel);
+ ldapFedProvider.getLdapIdentityStore().update(duplicatedLdapUser);
+
+ // Assert user successfully synced now
+ result = new UsersSyncManager().syncAllUsers(session.getKeycloakSessionFactory(), "test", ldapModel);
+ Assert.assertEquals(0, result.getFailed());
+ FederationTestUtils.assertUserImported(session.userStorage(), testRealm, "user7-something", "User7FNN", "User7LNL", "user7-changed@email.org", "126");
+ } finally {
+ keycloakRule.stopSession(session, true);
+ }
+ }
+
+ @Test
public void testPeriodicSync() {
KeycloakSession session = keycloakRule.startSession();
try {
@@ -193,9 +254,10 @@ public class SyncProvidersTest {
}
}
- private void assertSyncEquals(UserFederationSyncResult syncResult, int expectedAdded, int expectedUpdated, int expectedRemoved) {
+ private void assertSyncEquals(UserFederationSyncResult syncResult, int expectedAdded, int expectedUpdated, int expectedRemoved, int expectedFailed) {
Assert.assertEquals(syncResult.getAdded(), expectedAdded);
Assert.assertEquals(syncResult.getUpdated(), expectedUpdated);
Assert.assertEquals(syncResult.getRemoved(), expectedRemoved);
+ Assert.assertEquals(syncResult.getFailed(), expectedFailed);
}
}