Separate Dirty group update in a dedicated class

src/main/java/sh/libre/scim/core/ScimDispatcher.java

@@ -7,7 +7,9 @@ import sh.libre.scim.core.exceptions.ScimExceptionHandler;
 import sh.libre.scim.core.exceptions.ScimPropagationException;
 import sh.libre.scim.core.exceptions.SkipOrStopApproach;
 import sh.libre.scim.core.exceptions.SkipOrStopStrategy;
-import sh.libre.scim.storage.ScimEndpointConfigurationStorageProviderFactory;
+import sh.libre.scim.core.service.AbstractScimService;
+import sh.libre.scim.core.service.GroupScimService;
+import sh.libre.scim.core.service.UserScimService;
 
 import java.util.ArrayList;
 import java.util.LinkedHashSet;
@@ -85,7 +87,7 @@ public class ScimDispatcher {
                 exceptionHandler.handleException(userScimService.getConfiguration(), e);
             }
         });
-        // TODO we could iterate on servicesCorrectlyPropagated to undo modification
+        // TODO we could iterate on servicesCorrectlyPropagated to undo modification on already handled SCIM endpoints
         LOGGER.infof("[SCIM] User operation dispatched to %d SCIM server", servicesCorrectlyPropagated.size());
     }
 
@@ -100,7 +102,7 @@ public class ScimDispatcher {
                 exceptionHandler.handleException(groupScimService.getConfiguration(), e);
             }
         });
-        // TODO we could iterate on servicesCorrectlyPropagated to undo modification
+        // TODO we could iterate on servicesCorrectlyPropagated to undo modification on already handled SCIM endpoints
         LOGGER.infof("[SCIM] Group operation dispatched to %d SCIM server", servicesCorrectlyPropagated.size());
     }
 

src/main/java/sh/libre/scim/core/ScimEndpointConfigurationStorageProviderFactory.java

@@ -1,11 +1,10 @@
-package sh.libre.scim.storage;
+package sh.libre.scim.core;
 
 import de.captaingoldfish.scim.sdk.common.constants.HttpHeader;
 import jakarta.ws.rs.core.MediaType;
 import org.apache.commons.lang3.BooleanUtils;
 import org.jboss.logging.Logger;
 import org.keycloak.component.ComponentModel;
-import org.keycloak.models.GroupModel;
 import org.keycloak.models.KeycloakSession;
 import org.keycloak.models.KeycloakSessionFactory;
 import org.keycloak.models.RealmModel;
@@ -17,11 +16,8 @@ import org.keycloak.storage.UserStorageProviderFactory;
 import org.keycloak.storage.UserStorageProviderModel;
 import org.keycloak.storage.user.ImportSynchronization;
 import org.keycloak.storage.user.SynchronizationResult;
-import org.keycloak.timer.TimerProvider;
+import sh.libre.scim.event.ScimBackgroundGroupMembershipUpdater;
-import sh.libre.scim.core.ScimDispatcher;
-import sh.libre.scim.core.ScrimEndPointConfiguration;
 
-import java.time.Duration;
 import java.util.Date;
 import java.util.List;
 
@@ -31,7 +27,7 @@ import java.util.List;
 public class ScimEndpointConfigurationStorageProviderFactory
         implements UserStorageProviderFactory<ScimEndpointConfigurationStorageProviderFactory.ScimEndpointConfigurationStorageProvider>, ImportSynchronization {
     public static final String ID = "scim";
-    private final Logger LOGGER = Logger.getLogger(ScimEndpointConfigurationStorageProviderFactory.class);
+    private static final Logger LOGGER = Logger.getLogger(ScimEndpointConfigurationStorageProviderFactory.class);
 
     @Override
     public String getId() {
@@ -42,7 +38,7 @@ public class ScimEndpointConfigurationStorageProviderFactory
     @Override
     public SynchronizationResult sync(KeycloakSessionFactory sessionFactory, String realmId,
                                       UserStorageProviderModel model) {
-        // TODO if this should be kept here, better document purpose & usage
+        // Manually Launch a synchronization between keycloack and the SCIM endpoint described in the given model
         LOGGER.infof("[SCIM] Sync from ScimStorageProvider - Realm %s - Model %s", realmId, model.getId());
         SynchronizationResult result = new SynchronizationResult();
         KeycloakModelUtils.runJobInTransaction(sessionFactory, session -> {
@@ -68,25 +64,8 @@ public class ScimEndpointConfigurationStorageProviderFactory
 
     @Override
     public void postInit(KeycloakSessionFactory factory) {
-        // TODO : find a better way to handle scim dirty (use a QUEUE for SCIM queries ?)
+        ScimBackgroundGroupMembershipUpdater scimBackgroundGroupMembershipUpdater = new ScimBackgroundGroupMembershipUpdater(factory);
-        try (KeycloakSession keycloakSession = factory.create()) {
+        scimBackgroundGroupMembershipUpdater.startBackgroundUpdates();
-            TimerProvider timer = keycloakSession.getProvider(TimerProvider.class);
-            timer.scheduleTask(taskSession -> {
-                for (RealmModel realm : taskSession.realms().getRealmsStream().toList()) {
-                    KeycloakModelUtils.runJobInTransaction(factory, session -> {
-                        session.getContext().setRealm(realm);
-                        ScimDispatcher dispatcher = new ScimDispatcher(session);
-                        for (GroupModel group : session.groups().getGroupsStream(realm)
-                                .filter(x -> BooleanUtils.TRUE.equals(x.getFirstAttribute("scim-dirty"))).toList()) {
-                            LOGGER.infof("[SCIM] Dirty group: %s", group.getName());
-                            dispatcher.dispatchGroupModificationToAll(client -> client.update(group));
-                            group.removeAttribute("scim-dirty");
-                        }
-                        dispatcher.close();
-                    });
-                }
-            }, Duration.ofSeconds(30).toMillis(), "scim-background");
-        }
     }
 
     @Override

src/main/java/sh/libre/scim/core/exceptions/RollbackApproach.java

@@ -1,7 +1,10 @@
 package sh.libre.scim.core.exceptions;
 
+import com.google.common.collect.Lists;
 import sh.libre.scim.core.ScrimEndPointConfiguration;
 
+import java.util.ArrayList;
+
 
 public enum RollbackApproach implements RollbackStrategy {
     ALWAYS_ROLLBACK {
@@ -37,8 +40,10 @@ public enum RollbackApproach implements RollbackStrategy {
         }
 
         private boolean shouldRollbackBecauseOfResponse(InvalidResponseFromScimEndpointException e) {
+            // We consider that 404 are acceptable, otherwise rollback
             int httpStatus = e.getResponse().getHttpStatus();
-            return httpStatus == 500;
+            ArrayList<Integer> acceptableStatus = Lists.newArrayList(200, 204, 404);
+            return !acceptableStatus.contains(httpStatus);
         }
     }
 }

src/main/java/sh/libre/scim/core/service/AbstractScimService.java

@@ -1,4 +1,4 @@
-package sh.libre.scim.core;
+package sh.libre.scim.core.service;
 
 import de.captaingoldfish.scim.sdk.common.resources.ResourceNode;
 import de.captaingoldfish.scim.sdk.common.resources.complex.Meta;
@@ -6,6 +6,7 @@ import org.jboss.logging.Logger;
 import org.keycloak.models.KeycloakSession;
 import org.keycloak.models.RoleMapperModel;
 import org.keycloak.storage.user.SynchronizationResult;
+import sh.libre.scim.core.ScrimEndPointConfiguration;
 import sh.libre.scim.core.exceptions.InconsistentScimMappingException;
 import sh.libre.scim.core.exceptions.InvalidResponseFromScimEndpointException;
 import sh.libre.scim.core.exceptions.SkipOrStopStrategy;

src/main/java/sh/libre/scim/core/service/EntityOnRemoteScimId.java

@@ -1,4 +1,4 @@
-package sh.libre.scim.core;
+package sh.libre.scim.core.service;
 
 public record EntityOnRemoteScimId(
         String asString

src/main/java/sh/libre/scim/core/service/GroupScimService.java

@@ -1,4 +1,4 @@
-package sh.libre.scim.core;
+package sh.libre.scim.core.service;
 
 import de.captaingoldfish.scim.sdk.common.resources.Group;
 import de.captaingoldfish.scim.sdk.common.resources.complex.Meta;
@@ -10,6 +10,7 @@ import org.jboss.logging.Logger;
 import org.keycloak.models.GroupModel;
 import org.keycloak.models.KeycloakSession;
 import org.keycloak.models.UserModel;
+import sh.libre.scim.core.ScrimEndPointConfiguration;
 import sh.libre.scim.core.exceptions.InconsistentScimMappingException;
 import sh.libre.scim.core.exceptions.SkipOrStopStrategy;
 import sh.libre.scim.core.exceptions.UnexpectedScimDataException;

src/main/java/sh/libre/scim/core/service/KeycloakDao.java

@@ -1,4 +1,4 @@
-package sh.libre.scim.core;
+package sh.libre.scim.core.service;
 
 import org.keycloak.models.GroupModel;
 import org.keycloak.models.KeycloakSession;

src/main/java/sh/libre/scim/core/service/KeycloakId.java

@@ -1,6 +1,5 @@
-package sh.libre.scim.core;
+package sh.libre.scim.core.service;
 
-// TODO rename this
 public record KeycloakId(
         String asString
 ) {

src/main/java/sh/libre/scim/core/service/ScimClient.java

@@ -1,4 +1,4 @@
-package sh.libre.scim.core;
+package sh.libre.scim.core.service;
 
 import com.google.common.net.HttpHeaders;
 import de.captaingoldfish.scim.sdk.client.ScimClientConfig;
@@ -12,9 +12,9 @@ import io.github.resilience4j.retry.RetryConfig;
 import io.github.resilience4j.retry.RetryRegistry;
 import jakarta.ws.rs.ProcessingException;
 import org.jboss.logging.Logger;
+import sh.libre.scim.core.ScrimEndPointConfiguration;
 import sh.libre.scim.core.exceptions.InvalidResponseFromScimEndpointException;
 
-import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -53,8 +53,6 @@ public class ScimClient<S extends ResourceNode> implements AutoCloseable {
                 .connectTimeout(5)
                 .requestTimeout(5)
                 .socketTimeout(5)
-                .expectedHttpResponseHeaders(Collections.emptyMap()) // strange, useful?
-                // TODO Question Indiehoster : should we really allow connection with TLS ? .hostnameVerifier((s, sslSession) -> true)
                 .build();
         ScimRequestBuilder scimRequestBuilder =
                 new ScimRequestBuilder(

src/main/java/sh/libre/scim/core/service/ScimResourceType.java

@@ -1,4 +1,4 @@
-package sh.libre.scim.core;
+package sh.libre.scim.core.service;
 
 import de.captaingoldfish.scim.sdk.common.resources.Group;
 import de.captaingoldfish.scim.sdk.common.resources.ResourceNode;

src/main/java/sh/libre/scim/core/service/UserScimService.java

@@ -1,4 +1,4 @@
-package sh.libre.scim.core;
+package sh.libre.scim.core.service;
 
 import de.captaingoldfish.scim.sdk.common.resources.User;
 import de.captaingoldfish.scim.sdk.common.resources.complex.Meta;
@@ -13,6 +13,7 @@ import org.keycloak.models.KeycloakSession;
 import org.keycloak.models.RoleMapperModel;
 import org.keycloak.models.RoleModel;
 import org.keycloak.models.UserModel;
+import sh.libre.scim.core.ScrimEndPointConfiguration;
 import sh.libre.scim.core.exceptions.InconsistentScimMappingException;
 import sh.libre.scim.core.exceptions.SkipOrStopStrategy;
 import sh.libre.scim.core.exceptions.UnexpectedScimDataException;

src/main/java/sh/libre/scim/event/ScimBackgroundGroupMembershipUpdater.java

@@ -0,0 +1,74 @@
+package sh.libre.scim.event;
+
+import org.jboss.logging.Logger;
+import org.keycloak.models.GroupModel;
+import org.keycloak.models.KeycloakSession;
+import org.keycloak.models.KeycloakSessionFactory;
+import org.keycloak.models.RealmModel;
+import org.keycloak.models.utils.KeycloakModelUtils;
+import org.keycloak.timer.TimerProvider;
+import sh.libre.scim.core.ScimDispatcher;
+
+import java.time.Duration;
+
+/**
+ * In charge of making background checks and sent
+ * UPDATE requests from group for which membership information has changed.
+ * <p>
+ * This is required to avoid immediate group membership updates which could cause
+ * to incorrect group members list in case of concurrent group membership changes.
+ */
+public class ScimBackgroundGroupMembershipUpdater {
+    public static final String GROUP_DIRTY_SINCE_ATTRIBUTE_NAME = "scim-dirty-since";
+
+    private static final Logger LOGGER = Logger.getLogger(ScimBackgroundGroupMembershipUpdater.class);
+    // Update check loop will run every time this delay has passed
+    private static final long UPDATE_CHECK_DELAY_MS = 2000;
+    // If a group is marked dirty since less that this debounce delay, wait for the next update check loop
+    private static final long DEBOUNCE_DELAY_MS = 1200;
+    private final KeycloakSessionFactory sessionFactory;
+
+    public ScimBackgroundGroupMembershipUpdater(KeycloakSessionFactory sessionFactory) {
+        this.sessionFactory = sessionFactory;
+    }
+
+    public void startBackgroundUpdates() {
+        // Every UPDATE_CHECK_DELAY_MS, check for dirty groups and send updates if required
+        try (KeycloakSession keycloakSession = sessionFactory.create()) {
+            TimerProvider timer = keycloakSession.getProvider(TimerProvider.class);
+            timer.scheduleTask(taskSession -> {
+                for (RealmModel realm : taskSession.realms().getRealmsStream().toList()) {
+                    dispatchDirtyGroupsUpdates(realm);
+                }
+            }, Duration.ofMillis(UPDATE_CHECK_DELAY_MS).toMillis(), "scim-background");
+        }
+    }
+
+    private void dispatchDirtyGroupsUpdates(RealmModel realm) {
+        KeycloakModelUtils.runJobInTransaction(sessionFactory, session -> {
+            session.getContext().setRealm(realm);
+            ScimDispatcher dispatcher = new ScimDispatcher(session);
+            // Identify groups marked as dirty by the ScimEventListenerProvider
+            for (GroupModel group : session.groups().getGroupsStream(realm)
+                    .filter(this::isDirtyGroup).toList()) {
+                LOGGER.infof("[SCIM] Group %s is dirty, dispatch an update", group.getName());
+                // If dirty : dispatch a group update to all clients and mark it clean
+                dispatcher.dispatchGroupModificationToAll(client -> client.update(group));
+                group.removeAttribute(GROUP_DIRTY_SINCE_ATTRIBUTE_NAME);
+            }
+            dispatcher.close();
+        });
+    }
+
+    private boolean isDirtyGroup(GroupModel g) {
+        String groupDirtySinceAttribute = g.getFirstAttribute(GROUP_DIRTY_SINCE_ATTRIBUTE_NAME);
+        try {
+            int groupDirtySince = Integer.parseInt(groupDirtySinceAttribute);
+            // Must be dirty for more than DEBOUNCE_DELAY_MS
+            // (otherwise update will be dispatched in next scheduled loop)
+            return System.currentTimeMillis() - groupDirtySince > DEBOUNCE_DELAY_MS;
+        } catch (NumberFormatException e) {
+            return false;
+        }
+    }
+}

src/main/java/sh/libre/scim/event/ScimEventListenerProvider.java

@@ -1,6 +1,5 @@
 package sh.libre.scim.event;
 
-import org.apache.commons.lang3.BooleanUtils;
 import org.jboss.logging.Logger;
 import org.keycloak.events.Event;
 import org.keycloak.events.EventListenerProvider;
@@ -11,10 +10,10 @@ import org.keycloak.events.admin.ResourceType;
 import org.keycloak.models.GroupModel;
 import org.keycloak.models.KeycloakSession;
 import org.keycloak.models.UserModel;
-import sh.libre.scim.core.KeycloakDao;
-import sh.libre.scim.core.KeycloakId;
 import sh.libre.scim.core.ScimDispatcher;
-import sh.libre.scim.core.ScimResourceType;
+import sh.libre.scim.core.service.KeycloakDao;
+import sh.libre.scim.core.service.KeycloakId;
+import sh.libre.scim.core.service.ScimResourceType;
 
 import java.util.Map;
 import java.util.regex.Matcher;
@@ -35,7 +34,7 @@ public class ScimEventListenerProvider implements EventListenerProvider {
 
     private final KeycloakDao keycloakDao;
 
-    private final Map<ResourceType, Pattern> patterns = Map.of(
+    private final Map<ResourceType, Pattern> listenedEventPathPatterns = Map.of(
             ResourceType.USER, Pattern.compile("users/(.+)"),
             ResourceType.GROUP, Pattern.compile("groups/([\\w-]+)(/children)?"),
             ResourceType.GROUP_MEMBERSHIP, Pattern.compile("users/(.+)/groups/(.+)"),
@@ -79,7 +78,7 @@ public class ScimEventListenerProvider implements EventListenerProvider {
     @Override
     public void onEvent(AdminEvent event, boolean includeRepresentation) {
         // Step 1: check if event is relevant for propagation through SCIM
-        Pattern pattern = patterns.get(event.getResourceType());
+        Pattern pattern = listenedEventPathPatterns.get(event.getResourceType());
         if (pattern == null)
             return;
         Matcher matcher = pattern.matcher(event.getResourcePath());
@@ -171,10 +170,16 @@ public class ScimEventListenerProvider implements EventListenerProvider {
 
     private void handleGroupMemberShipEvent(AdminEvent groupMemberShipEvent, KeycloakId userId, KeycloakId groupId) {
         LOGGER.infof("[SCIM] Propagate GroupMemberShip %s - User %s Group %s", groupMemberShipEvent.getOperationType(), userId, groupId);
+        // Step 1: update USER immediately
         GroupModel group = getGroup(groupId);
-        group.setSingleAttribute("scim-dirty", BooleanUtils.TRUE);
         UserModel user = getUser(userId);
         dispatcher.dispatchUserModificationToAll(client -> client.update(user));
+
+        // Step 2: delayed GROUP update :
+        // if several users are added to the group simultaneously in different Keycloack sessions
+        // update the group in the context of the current session may not reflect those other changes
+        // We trigger a delayed update by setting an attribute on the group (that will be handled by ScimBackgroundGroupMembershipUpdaters)
+        group.setSingleAttribute(ScimBackgroundGroupMembershipUpdater.GROUP_DIRTY_SINCE_ATTRIBUTE_NAME, "" + System.currentTimeMillis());
     }
 
     private void handleRoleMappingEvent(AdminEvent roleMappingEvent, ScimResourceType type, KeycloakId id) {

src/main/java/sh/libre/scim/jpa/ScimResourceDao.java

@@ -5,9 +5,9 @@ import jakarta.persistence.NoResultException;
 import jakarta.persistence.TypedQuery;
 import org.keycloak.connections.jpa.JpaConnectionProvider;
 import org.keycloak.models.KeycloakSession;
-import sh.libre.scim.core.EntityOnRemoteScimId;
+import sh.libre.scim.core.service.EntityOnRemoteScimId;
-import sh.libre.scim.core.KeycloakId;
+import sh.libre.scim.core.service.KeycloakId;
-import sh.libre.scim.core.ScimResourceType;
+import sh.libre.scim.core.service.ScimResourceType;
 
 import java.util.Optional;
 

src/main/java/sh/libre/scim/jpa/ScimResourceId.java

@@ -67,10 +67,8 @@ public class ScimResourceId implements Serializable {
     public boolean equals(Object other) {
         if (this == other)
             return true;
-        if (!(other instanceof ScimResourceId))
+        if (!(other instanceof ScimResourceId o))
             return false;
-        ScimResourceId o = (ScimResourceId) other;
-        // TODO
         return (StringUtils.equals(o.id, id) &&
                 StringUtils.equals(o.realmId, realmId) &&
                 StringUtils.equals(o.componentId, componentId) &&

src/main/java/sh/libre/scim/jpa/ScimResourceMapping.java

@@ -7,8 +7,8 @@ import jakarta.persistence.IdClass;
 import jakarta.persistence.NamedQueries;
 import jakarta.persistence.NamedQuery;
 import jakarta.persistence.Table;
-import sh.libre.scim.core.EntityOnRemoteScimId;
+import sh.libre.scim.core.service.EntityOnRemoteScimId;
-import sh.libre.scim.core.KeycloakId;
+import sh.libre.scim.core.service.KeycloakId;
 
 @Entity
 @IdClass(ScimResourceId.class)

src/main/resources/META-INF/services/org.keycloak.storage.UserStorageProviderFactory

@@ -1 +1 @@
-sh.libre.scim.storage.ScimEndpointConfigurationStorageProviderFactory
+sh.libre.scim.core.ScimEndpointConfigurationStorageProviderFactory