KEYCLOAK-1416 federation provider/mapper model fixes

model/jpa/src/main/java/org/keycloak/models/jpa/RealmAdapter.java

@@ -804,18 +804,23 @@ public class RealmAdapter implements RealmModel {
             if (entity.getId().equals(provider.getId())) {
 
                 session.users().preRemove(this, provider);
+                removeFederationMappersForProvider(provider.getId());
 
-                Set<UserFederationMapperEntity> mappers = getUserFederationMapperEntitiesByFederationProvider(provider.getId());
-                for (UserFederationMapperEntity mapper : mappers) {
-                    realm.getUserFederationMappers().remove(mapper);
-                    em.remove(mapper);
-                }
                 it.remove();
                 em.remove(entity);
                 return;
             }
         }
     }
+
+    private void removeFederationMappersForProvider(String federationProviderId) {
+        Set<UserFederationMapperEntity> mappers = getUserFederationMapperEntitiesByFederationProvider(federationProviderId);
+        for (UserFederationMapperEntity mapper : mappers) {
+            realm.getUserFederationMappers().remove(mapper);
+            em.remove(mapper);
+        }
+    }
+
     @Override
     public void updateUserFederationProvider(UserFederationProviderModel model) {
         KeycloakModelUtils.ensureUniqueDisplayName(model.getDisplayName(), model, getUserFederationProviders());
@@ -855,10 +860,9 @@ public class RealmAdapter implements RealmModel {
                     entity.setConfig(model.getConfig());
                     entity.setPriority(model.getPriority());
                     entity.setProviderName(model.getProviderName());
-                    entity.setPriority(model.getPriority());
                     String displayName = model.getDisplayName();
                     if (displayName != null) {
-                        entity.setDisplayName(model.getDisplayName());
+                        entity.setDisplayName(displayName);
                     }
                     entity.setFullSyncPeriod(model.getFullSyncPeriod());
                     entity.setChangedSyncPeriod(model.getChangedSyncPeriod());
@@ -871,6 +875,8 @@ public class RealmAdapter implements RealmModel {
             if (found) continue;
             session.users().preRemove(this, new UserFederationProviderModel(entity.getId(), entity.getProviderName(), entity.getConfig(), entity.getPriority(), entity.getDisplayName(),
                     entity.getFullSyncPeriod(), entity.getChangedSyncPeriod(), entity.getLastSync()));
+            removeFederationMappersForProvider(entity.getId());
+
             it.remove();
             em.remove(entity);
         }

model/mongo/src/main/java/org/keycloak/models/mongo/keycloak/adapters/RealmAdapter.java

@@ -877,11 +877,7 @@ public class RealmAdapter extends AbstractMongoAdapter<MongoRealmEntity> impleme
             if (entity.getId().equals(provider.getId())) {
                 session.users().preRemove(this, new UserFederationProviderModel(entity.getId(), entity.getProviderName(), entity.getConfig(), entity.getPriority(), entity.getDisplayName(),
                         entity.getFullSyncPeriod(), entity.getChangedSyncPeriod(), entity.getLastSync()));
-
+                removeFederationMappersForProvider(provider.getId());
-                Set<UserFederationMapperEntity> mappers = getUserFederationMapperEntitiesByFederationProvider(provider.getId());
-                for (UserFederationMapperEntity mapper : mappers) {
-                    getMongoEntity().getUserFederationMappers().remove(mapper);
-                }
 
                 it.remove();
             }
@@ -889,6 +885,13 @@ public class RealmAdapter extends AbstractMongoAdapter<MongoRealmEntity> impleme
         updateRealm();
     }
 
+    private void removeFederationMappersForProvider(String federationProviderId) {
+        Set<UserFederationMapperEntity> mappers = getUserFederationMapperEntitiesByFederationProvider(federationProviderId);
+        for (UserFederationMapperEntity mapper : mappers) {
+            getMongoEntity().getUserFederationMappers().remove(mapper);
+        }
+    }
+
     @Override
     public void updateUserFederationProvider(UserFederationProviderModel model) {
         KeycloakModelUtils.ensureUniqueDisplayName(model.getDisplayName(), model, getUserFederationProviders());
@@ -943,8 +946,52 @@ public class RealmAdapter extends AbstractMongoAdapter<MongoRealmEntity> impleme
             KeycloakModelUtils.ensureUniqueDisplayName(currentProvider.getDisplayName(), currentProvider, providers);
         }
 
-        List<UserFederationProviderEntity> entities = new LinkedList<UserFederationProviderEntity>();
+        List<UserFederationProviderEntity> existingProviders = realm.getUserFederationProviders();
+        List<UserFederationProviderEntity> toRemove = new LinkedList<>();
+        for (UserFederationProviderEntity entity : existingProviders) {
+            boolean found = false;
+            for (UserFederationProviderModel model : providers) {
+                if (entity.getId().equals(model.getId())) {
+                    entity.setConfig(model.getConfig());
+                    entity.setPriority(model.getPriority());
+                    entity.setProviderName(model.getProviderName());
+                    String displayName = model.getDisplayName();
+                    if (displayName != null) {
+                        entity.setDisplayName(displayName);
+                    }
+                    entity.setFullSyncPeriod(model.getFullSyncPeriod());
+                    entity.setChangedSyncPeriod(model.getChangedSyncPeriod());
+                    entity.setLastSync(model.getLastSync());
+                    found = true;
+                    break;
+                }
+
+            }
+            if (found) continue;
+            session.users().preRemove(this, new UserFederationProviderModel(entity.getId(), entity.getProviderName(), entity.getConfig(), entity.getPriority(), entity.getDisplayName(),
+                    entity.getFullSyncPeriod(), entity.getChangedSyncPeriod(), entity.getLastSync()));
+            removeFederationMappersForProvider(entity.getId());
+
+            toRemove.add(entity);
+        }
+
+        for (UserFederationProviderEntity entity : toRemove) {
+            realm.getUserFederationProviders().remove(entity);
+        }
+
+        List<UserFederationProviderModel> add = new LinkedList<UserFederationProviderModel>();
         for (UserFederationProviderModel model : providers) {
+            boolean found = false;
+            for (UserFederationProviderEntity entity : realm.getUserFederationProviders()) {
+                if (entity.getId().equals(model.getId())) {
+                    found = true;
+                    break;
+                }
+            }
+            if (!found) add.add(model);
+        }
+
+        for (UserFederationProviderModel model : add) {
             UserFederationProviderEntity entity = new UserFederationProviderEntity();
             if (model.getId() != null) {
                 entity.setId(model.getId());
@@ -964,12 +1011,11 @@ public class RealmAdapter extends AbstractMongoAdapter<MongoRealmEntity> impleme
             entity.setFullSyncPeriod(model.getFullSyncPeriod());
             entity.setChangedSyncPeriod(model.getChangedSyncPeriod());
             entity.setLastSync(model.getLastSync());
-            entities.add(entity);
+            realm.getUserFederationProviders().add(entity);
 
             session.getKeycloakSessionFactory().publish(new UserFederationProviderCreationEventImpl(this, model));
         }
 
-        realm.setUserFederationProviders(entities);
         updateRealm();
     }
 

services/src/main/java/org/keycloak/services/resources/admin/RealmAdminResource.java

@@ -193,8 +193,9 @@ public class RealmAdminResource {
         } catch (PatternSyntaxException e) {
             return ErrorResponse.error("Specified regex pattern(s) is invalid.", Response.Status.BAD_REQUEST);
         } catch (ModelDuplicateException e) {
-            return ErrorResponse.exists("Realm " + rep.getRealm() + " already exists.");
+            throw e;
-        }  catch (Exception e) {
+        } catch (Exception e) {
+            logger.error(e);
             return ErrorResponse.error("Failed to update " + rep.getRealm() + " Realm.", Response.Status.INTERNAL_SERVER_ERROR);
         }
     }

testsuite/integration/src/test/java/org/keycloak/testsuite/model/UserFederationModelTest.java

@@ -1,5 +1,9 @@
 package org.keycloak.testsuite.model;
 
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.TreeMap;
@@ -97,6 +101,43 @@ public class UserFederationModelTest extends AbstractModelTest {
         commit();
     }
 
+    @Test
+    public void federationProvidersSetTest() {
+        RealmModel realm = realmManager.createRealm("test-realm");
+        UserFederationProviderModel ldapProvider = new UserFederationProviderModel(null, "ldap", new TreeMap<String, String>(), 1, "my-cool-provider", -1, -1, 0);
+        realm.setUserFederationProviders(Arrays.asList(ldapProvider));
+
+        commit();
+
+        realm = realmManager.getRealmByName("test-realm");
+        List<UserFederationProviderModel> fedProviders = realm.getUserFederationProviders();
+        Assert.assertEquals(1, fedProviders.size());
+        ldapProvider = fedProviders.get(0);
+        Set<UserFederationMapperModel> fedMappers = realmManager.getRealmByName("test-realm").getUserFederationMappersByFederationProvider(ldapProvider.getId());
+
+        UserFederationProviderModel dummyProvider = new UserFederationProviderModel(null, "dummy", new TreeMap<String, String>(), 1, "my-cool-provider", -1, -1, 0);
+        try {
+            realm.setUserFederationProviders(Arrays.asList(ldapProvider, dummyProvider));
+            commit();
+            Assert.fail("Don't expect to end here");
+        } catch (ModelDuplicateException expected) {
+        }
+
+        dummyProvider.setDisplayName("my-cool-provider2");
+        realm.setUserFederationProviders(Arrays.asList(ldapProvider, dummyProvider));
+
+        commit();
+
+        realm = realmManager.getRealmByName("test-realm");
+        Assert.assertEquals(fedMappers.size(), realm.getUserFederationMappersByFederationProvider(ldapProvider.getId()).size());
+        realm.setUserFederationProviders(new ArrayList<UserFederationProviderModel>());
+
+        commit();
+
+        realm = realmManager.getRealmByName("test-realm");
+        Assert.assertTrue(realm.getUserFederationMappersByFederationProvider(ldapProvider.getId()).isEmpty());
+    }
+
     private UserFederationMapperModel createMapper(String name, String fedProviderId, String... config) {
         UserFederationMapperModel mapperModel = new UserFederationMapperModel();
         mapperModel.setName(name);