From 71ea61e7a6a233b9b218fe0022cab069df46cf03 Mon Sep 17 00:00:00 2001 From: mposolda Date: Sat, 4 Jul 2015 18:15:00 +0200 Subject: [PATCH] KEYCLOAK-1532 LDAP sync fixes and other bugfixing --- .../ldap/LDAPFederationProvider.java | 43 +----- .../ldap/LDAPFederationProviderFactory.java | 131 +++++++++++++----- .../idm/store/ldap/LDAPIdentityStore.java | 29 ++-- .../mappers/RoleLDAPFederationMapper.java | 2 +- .../UserAttributeLDAPFederationMapper.java | 37 ++++- ...rAttributeLDAPFederationMapperFactory.java | 6 +- .../models/UserFederationManager.java | 2 +- .../models/UserFederationSyncResult.java | 20 ++- .../DefaultKeycloakTransactionManager.java | 26 +++- .../messages/AdminMessagesProvider.java | 8 +- .../FederationProvidersIntegrationTest.java | 17 ++- .../federation/FederationTestUtils.java | 9 +- .../LDAPMultipleAttributesTest.java | 2 +- .../federation/SyncProvidersTest.java | 70 +++++++++- 14 files changed, 295 insertions(+), 107 deletions(-) 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 66665bf8f7..b5598e935b 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 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 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 2b60ff83b9..a71b84d1fd 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 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 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 ldapUsers) { + final UserFederationSyncResult syncResult = new UserFederationSyncResult(); - protected UserFederationSyncResult importLdapUsers(KeycloakSession session, String realmId, UserFederationProviderModel fedModel, List ldapUsers) { - RealmModel realm = session.realms().getRealm(realmId); - LDAPFederationProvider ldapFedProvider = getInstance(session, fedModel); - return ldapFedProvider.importLDAPUsers(realm, ldapUsers, fedModel); + 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 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); + } + } + + }); + } + } + } + + return syncResult; } protected SPNEGOAuthenticator createSPNEGOAuthenticator(String spnegoToken, CommonKerberosConfig kerberosConfig) { 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 67e92c7e2c..141ae387d7 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/mappers/RoleLDAPFederationMapper.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/mappers/RoleLDAPFederationMapper.java index 4565f885ca..e29ab6f9a0 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 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 24bf4509e5..283089a0ba 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 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()); + } + } else { + ldapUser.setSingleAttribute(ldapAttrName, attrValue.toString()); + } } else { // we don't have java property. Let's set attribute List 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()); + } } 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()); + } } else if (value instanceof String) { ldapUser.setSingleAttribute(ldapAttrName, (String) value); } else { List asList = (List) 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 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 1b1b44d2bd..c14d0e89b3 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 7d9e7ca434..8bca31c624 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 e6d465b219..b06348d318 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 75f609689f..c39bc8a947 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 transactions = new LinkedList(); private List afterCompletion = new LinkedList(); 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 1da7bfb9d8..7b6b3ed967 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 5ce1b12c5e..1945701fb1 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 7e06bc67b5..939fe24351 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 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 556053daa0..edd210cb41 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 ab2f46e239..e29a2b8e9a 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); } @@ -154,6 +154,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(); @@ -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); } }