KEYCLOAK-1532 LDAP sync fixes and other bugfixing

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
      *

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,31 +238,16 @@ public class LDAPFederationProviderFactory extends UserFederationEventAwareProvi
                 userQuery.setLimit(pageSize);
                 final List<LDAPObject> users = userQuery.getResultList();
                 nextPage = userQuery.getPaginationContext() != null;
-
+                UserFederationSyncResult currentPageSync = importLdapUsers(sessionFactory, realmId, fedModel, users);
-                KeycloakModelUtils.runJobInTransaction(sessionFactory, new KeycloakSessionTask() {
-
-                    @Override
-                    public void run(KeycloakSession session) {
-                        UserFederationSyncResult currentPageSync = importLdapUsers(session, 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() {
+            UserFederationSyncResult currentSync = importLdapUsers(sessionFactory, realmId, fedModel, users);
-
-                @Override
-                public void run(KeycloakSession session) {
-                    UserFederationSyncResult currentSync = importLdapUsers(session, 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();
 
-    protected UserFederationSyncResult importLdapUsers(KeycloakSession session, String realmId, UserFederationProviderModel fedModel, List<LDAPObject> ldapUsers) {
+        class BooleanHolder {
-        RealmModel realm = session.realms().getRealm(realmId);
+            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);
-        return ldapFedProvider.importLDAPUsers(realm, ldapUsers, 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);
+                            }
+                        }
+
+                    });
+                }
+            }
+        }
+
+        return syncResult;
     }
 
     protected SPNEGOAuthenticator createSPNEGOAuthenticator(String spnegoToken, CommonKerberosConfig kerberosConfig) {

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
+                    // Shouldn't happen
-                    attr.add(LDAPConstants.EMPTY_ATTRIBUTE_VALUE);
+                    logger.warnf("Attribute '%s' is null on LDAP object '%s' . Using empty value to be saved to LDAP", attrName, ldapObject.getDn().toString());
-                } else {
+                    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);
             }
         }

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);
         }

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,15 +187,23 @@ 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;
+                            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);
                         }

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

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);

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

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,6 +60,9 @@ public class DefaultKeycloakTransactionManager implements KeycloakTransactionMan
                 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();
@@ -64,6 +70,16 @@ public class DefaultKeycloakTransactionManager implements KeycloakTransactionMan
                     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;

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);
+
+        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

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);

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);
     }
 

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

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);
     }
 }