diff --git a/src/main/java/sh/libre/scim/core/AbstractScimService.java b/src/main/java/sh/libre/scim/core/AbstractScimService.java index 9fc896bbe0..dda7b72687 100644 --- a/src/main/java/sh/libre/scim/core/AbstractScimService.java +++ b/src/main/java/sh/libre/scim/core/AbstractScimService.java @@ -8,7 +8,7 @@ import org.keycloak.models.RoleMapperModel; import org.keycloak.storage.user.SynchronizationResult; import sh.libre.scim.core.exceptions.ErrorForScimEndpointException; import sh.libre.scim.core.exceptions.InconsistentScimDataException; -import sh.libre.scim.core.exceptions.ScimPropagationException; +import sh.libre.scim.core.exceptions.SkipOrStopStrategy; import sh.libre.scim.core.exceptions.UnexpectedScimDataException; import sh.libre.scim.jpa.ScimResourceDao; import sh.libre.scim.jpa.ScimResourceMapping; @@ -20,24 +20,29 @@ import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; -// TODO Document K and S +/** + * A service in charge of synchronisation (CRUD) between + * a Keykloak Role (UserModel, GroupModel) and a SCIM Resource (User,Group). + * + * @param The Keycloack Model (e.g. UserModel, GroupModel) + * @param The SCIM Resource (e.g. User, Group) + */ public abstract class AbstractScimService implements AutoCloseable { private static final Logger LOGGER = Logger.getLogger(AbstractScimService.class); private final KeycloakSession keycloakSession; - - private final ScrimProviderConfiguration scimProviderConfiguration; - + protected final SkipOrStopStrategy skipOrStopStrategy; + private final ScrimEndPointConfiguration scimProviderConfiguration; private final ScimResourceType type; - private final ScimClient scimClient; - protected AbstractScimService(KeycloakSession keycloakSession, ScrimProviderConfiguration scimProviderConfiguration, ScimResourceType type) { + protected AbstractScimService(KeycloakSession keycloakSession, ScrimEndPointConfiguration scimProviderConfiguration, ScimResourceType type, SkipOrStopStrategy skipOrStopStrategy) { this.keycloakSession = keycloakSession; this.scimProviderConfiguration = scimProviderConfiguration; this.type = type; this.scimClient = ScimClient.open(scimProviderConfiguration, type); + this.skipOrStopStrategy = skipOrStopStrategy; } public void create(K roleMapperModel) throws InconsistentScimDataException, ErrorForScimEndpointException { @@ -68,7 +73,7 @@ public abstract class AbstractScimService resourcesStream = getResourceStream()) { Set resources = resourcesStream.collect(Collectors.toUnmodifiableSet()); for (K resource : resources) { - try { - KeycloakId id = getId(resource); - LOGGER.infof("[SCIM] Reconciling local resource %s", id); - if (shouldIgnoreForScimSynchronization(resource)) { - LOGGER.infof("[SCIM] Skip local resource %s", id); - continue; - } - if (findMappingById(id).isPresent()) { - LOGGER.info("[SCIM] Replacing it"); - update(resource); - } else { - LOGGER.info("[SCIM] Creating it"); - create(resource); - } - syncRes.increaseUpdated(); - } catch (ErrorForScimEndpointException e) { - // TODO Error handling use strategy SOUPLE or STICT to determine if we re-trhoguh or log & continue - throw e; - } + KeycloakId id = getId(resource); + pushSingleResourceToScim(syncRes, resource, id); } } } - - public void pullResourcesFromScim(SynchronizationResult syncRes) { - LOGGER.info("[SCIM] Import resources from endpoint " + this.getConfiguration().getEndPoint()); + public void pullAllResourcesFromScim(SynchronizationResult syncRes) throws UnexpectedScimDataException, InconsistentScimDataException, ErrorForScimEndpointException { + LOGGER.info("[SCIM] Pull resources from endpoint " + this.getConfiguration().getEndPoint()); for (S resource : scimClient.listResources()) { - try { - LOGGER.infof("[SCIM] Reconciling remote resource %s", resource); - EntityOnRemoteScimId externalId = resource.getId() - .map(EntityOnRemoteScimId::new) - .orElseThrow(() -> new UnexpectedScimDataException("Remote SCIM resource doesn't have an id, cannot import it in Keycloak")); - Optional optionalMapping = getScimResourceDao().findByExternalId(externalId, type); + pullSingleResourceFromScim(syncRes, resource); + } + } - // If an existing mapping exists, delete potential dangling references - if (optionalMapping.isPresent()) { - ScimResourceMapping mapping = optionalMapping.get(); - if (entityExists(mapping.getIdAsKeycloakId())) { - LOGGER.debug("[SCIM] Valid mapping found, skipping"); - continue; - } else { - LOGGER.info("[SCIM] Delete a dangling mapping"); - getScimResourceDao().delete(mapping); - } - } - - // Here no keycloak user/group matching the SCIM external id exists - // Try to match existing keycloak resource by properties (username, email, name) - Optional mapped = matchKeycloakMappingByScimProperties(resource); - if (mapped.isPresent()) { - LOGGER.info("[SCIM] Matched SCIM resource " + externalId + " from properties with keycloak entity " + mapped.get()); - createMapping(mapped.get(), externalId); - syncRes.increaseUpdated(); - } else { - switch (scimProviderConfiguration.getImportAction()) { - case CREATE_LOCAL -> { - LOGGER.info("[SCIM] Create local resource for SCIM resource " + externalId); - KeycloakId id = createEntity(resource); - createMapping(id, externalId); - syncRes.increaseAdded(); - } - case DELETE_REMOTE -> { - LOGGER.info("[SCIM] Delete remote resource " + externalId); - scimClient.delete(externalId); - } - case NOTHING -> LOGGER.info("[SCIM] Import action set to NOTHING"); - } - } - } catch (ScimPropagationException e) { - throw new RuntimeException(e); + private void pushSingleResourceToScim(SynchronizationResult syncRes, K resource, KeycloakId id) throws ErrorForScimEndpointException, InconsistentScimDataException { + try { + LOGGER.infof("[SCIM] Reconciling local resource %s", id); + if (shouldIgnoreForScimSynchronization(resource)) { + LOGGER.infof("[SCIM] Skip local resource %s", id); + return; + } + if (findMappingById(id).isPresent()) { + LOGGER.info("[SCIM] Replacing it"); + update(resource); + } else { + LOGGER.info("[SCIM] Creating it"); + create(resource); + } + syncRes.increaseUpdated(); + } catch (ErrorForScimEndpointException e) { + if (skipOrStopStrategy.allowPartialSynchronizationWhenPushingToScim(this.getConfiguration())) { + LOGGER.warn("Error while syncing " + id + " to endpoint " + getConfiguration().getEndPoint()); + } else { + throw e; + } + } catch (InconsistentScimDataException e) { + if (skipOrStopStrategy.allowPartialSynchronizationWhenPushingToScim(this.getConfiguration())) { + LOGGER.warn("Inconsistent data for element " + id + " and endpoint " + getConfiguration().getEndPoint()); + } else { + throw e; } - } } - protected abstract S scimRequestBodyForCreate(K roleMapperModel); + private void pullSingleResourceFromScim(SynchronizationResult syncRes, S resource) throws UnexpectedScimDataException, InconsistentScimDataException, ErrorForScimEndpointException { + try { + LOGGER.infof("[SCIM] Reconciling remote resource %s", resource); + EntityOnRemoteScimId externalId = resource.getId() + .map(EntityOnRemoteScimId::new) + .orElseThrow(() -> new UnexpectedScimDataException("Remote SCIM resource doesn't have an id, cannot import it in Keycloak")); + Optional optionalMapping = getScimResourceDao().findByExternalId(externalId, type); + + // If an existing mapping exists, delete potential dangling references + if (optionalMapping.isPresent()) { + ScimResourceMapping mapping = optionalMapping.get(); + if (entityExists(mapping.getIdAsKeycloakId())) { + LOGGER.debug("[SCIM] Valid mapping found, skipping"); + return; + } else { + LOGGER.info("[SCIM] Delete a dangling mapping"); + getScimResourceDao().delete(mapping); + } + } + + // Here no keycloak user/group matching the SCIM external id exists + // Try to match existing keycloak resource by properties (username, email, name) + Optional mapped = matchKeycloakMappingByScimProperties(resource); + if (mapped.isPresent()) { + LOGGER.info("[SCIM] Matched SCIM resource " + externalId + " from properties with keycloak entity " + mapped.get()); + createMapping(mapped.get(), externalId); + syncRes.increaseUpdated(); + } else { + switch (scimProviderConfiguration.getImportAction()) { + case CREATE_LOCAL -> { + LOGGER.info("[SCIM] Create local resource for SCIM resource " + externalId); + KeycloakId id = createEntity(resource); + createMapping(id, externalId); + syncRes.increaseAdded(); + } + case DELETE_REMOTE -> { + LOGGER.info("[SCIM] Delete remote resource " + externalId); + scimClient.delete(externalId); + } + case NOTHING -> LOGGER.info("[SCIM] Import action set to NOTHING"); + } + } + } catch (UnexpectedScimDataException e) { + if (skipOrStopStrategy.skipInvalidDataFromScimEndpoint(getConfiguration())) { + LOGGER.warn("[SCIM] Skipping element synchronisation because of invalid Scim Data for element " + resource.getId() + " : " + e.getMessage()); + } else { + throw e; + } + } catch (InconsistentScimDataException e) { + if (skipOrStopStrategy.allowPartialSynchronizationWhenPullingFromScim(getConfiguration())) { + LOGGER.warn("[SCIM] Skipping element synchronisation because of inconsistent mapping for element " + resource.getId() + " : " + e.getMessage()); + } else { + throw e; + } + } catch (ErrorForScimEndpointException e) { + // Can only occur in case of a DELETE_REMOTE conflict action + if (skipOrStopStrategy.allowPartialSynchronizationWhenPullingFromScim(getConfiguration())) { + LOGGER.warn("[SCIM] Could not delete SCIM resource " + resource.getId() + " during synchronisation"); + } else { + throw e; + } + } + + } + + + protected abstract S scimRequestBodyForCreate(K roleMapperModel) throws InconsistentScimDataException; protected abstract KeycloakId getId(K roleMapperModel); @@ -192,12 +231,12 @@ public abstract class AbstractScimService { private final Logger LOGGER = Logger.getLogger(GroupScimService.class); - public GroupScimService(KeycloakSession keycloakSession, ScrimProviderConfiguration scimProviderConfiguration) { - super(keycloakSession, scimProviderConfiguration, ScimResourceType.GROUP); + public GroupScimService(KeycloakSession keycloakSession, ScrimEndPointConfiguration scimProviderConfiguration, SkipOrStopStrategy skipOrStopStrategy) { + super(keycloakSession, scimProviderConfiguration, ScimResourceType.GROUP, skipOrStopStrategy); } @Override @@ -66,6 +68,7 @@ public class GroupScimService extends AbstractScimService { .orElseThrow(() -> new UnexpectedScimDataException("can't create group member for group '%s' without id: ".formatted(displayName) + resource)); KeycloakId userId = getScimResourceDao().findUserByExternalId(externalId) .map(ScimResourceMapping::getIdAsKeycloakId) + // TODO Exception handling : here if think this is a InconsistentScimData : Scim member is valid, but not mapped yet in our keycloak .orElseThrow(() -> new UnexpectedScimDataException("can't find mapping for group member %s".formatted(externalId))); UserModel userModel = getKeycloakDao().getUserById(userId); userModel.joinGroup(group); @@ -85,7 +88,7 @@ public class GroupScimService extends AbstractScimService { } @Override - protected Group scimRequestBodyForCreate(GroupModel groupModel) { + protected Group scimRequestBodyForCreate(GroupModel groupModel) throws InconsistentScimDataException { Set members = getKeycloakDao().getGroupMembers(groupModel); Group group = new Group(); group.setExternalId(groupModel.getId()); @@ -101,15 +104,19 @@ public class GroupScimService extends AbstractScimService { groupMember.setRef(ref.toString()); group.addMember(groupMember); } else { - // TODO Exception Handling : should we throw an exception when some group members can't be found ? - LOGGER.warnf("member %s not found for group %s", member, groupModel.getId()); + String message = "Unmapped member " + member + " for group " + groupModel.getId(); + if (skipOrStopStrategy.allowMissingMembersWhenPushingGroupToScim(this.getConfiguration())) { + LOGGER.warn(message); + } else { + throw new InconsistentScimDataException(message); + } } } return group; } @Override - protected Group scimRequestBodyForUpdate(GroupModel groupModel, EntityOnRemoteScimId externalId) { + protected Group scimRequestBodyForUpdate(GroupModel groupModel, EntityOnRemoteScimId externalId) throws InconsistentScimDataException { Group group = scimRequestBodyForCreate(groupModel); group.setId(externalId.asString()); Meta meta = newMetaLocation(externalId); diff --git a/src/main/java/sh/libre/scim/core/ScimClient.java b/src/main/java/sh/libre/scim/core/ScimClient.java index 9bdf3cce88..9e0d136871 100644 --- a/src/main/java/sh/libre/scim/core/ScimClient.java +++ b/src/main/java/sh/libre/scim/core/ScimClient.java @@ -43,7 +43,7 @@ public class ScimClient implements AutoCloseable { this.scimResourceType = scimResourceType; } - public static ScimClient open(ScrimProviderConfiguration scimProviderConfiguration, ScimResourceType scimResourceType) { + public static ScimClient open(ScrimEndPointConfiguration scimProviderConfiguration, ScimResourceType scimResourceType) { String scimApplicationBaseUrl = scimProviderConfiguration.getEndPoint(); Map httpHeaders = new HashMap<>(); httpHeaders.put(HttpHeaders.AUTHORIZATION, scimProviderConfiguration.getAuthorizationHeaderValue()); diff --git a/src/main/java/sh/libre/scim/core/ScimDispatcher.java b/src/main/java/sh/libre/scim/core/ScimDispatcher.java index 927d0f9e02..0ab00fcc73 100644 --- a/src/main/java/sh/libre/scim/core/ScimDispatcher.java +++ b/src/main/java/sh/libre/scim/core/ScimDispatcher.java @@ -5,6 +5,8 @@ import org.keycloak.component.ComponentModel; import org.keycloak.models.KeycloakSession; import sh.libre.scim.core.exceptions.ScimExceptionHandler; import sh.libre.scim.core.exceptions.ScimPropagationException; +import sh.libre.scim.core.exceptions.SkipOrStopStrategy; +import sh.libre.scim.core.exceptions.SkipOrStopStrategyFactory; import sh.libre.scim.storage.ScimEndpointConfigurationStorageProviderFactory; import java.util.ArrayList; @@ -22,6 +24,7 @@ public class ScimDispatcher { private final KeycloakSession session; private final ScimExceptionHandler exceptionHandler; + private final SkipOrStopStrategy skipOrStopStrategy; private boolean clientsInitialized = false; private final List userScimServices = new ArrayList<>(); private final List groupScimServices = new ArrayList<>(); @@ -30,6 +33,10 @@ public class ScimDispatcher { public ScimDispatcher(KeycloakSession session) { this.session = session; this.exceptionHandler = new ScimExceptionHandler(session); + // By default, use a permissive Skip or Stop strategy + this.skipOrStopStrategy = SkipOrStopStrategyFactory.create( + SkipOrStopStrategyFactory.SkipOrStopApproach.ALWAYS_SKIP_AND_CONTINUE + ); } /** @@ -47,19 +54,24 @@ public class ScimDispatcher { .filter(m -> ScimEndpointConfigurationStorageProviderFactory.ID.equals(m.getProviderId()) && m.get("enabled", true)) .forEach(scimEndpointConfigurationRaw -> { - ScrimProviderConfiguration scrimProviderConfiguration = new ScrimProviderConfiguration(scimEndpointConfigurationRaw); try { + ScrimEndPointConfiguration scrimEndPointConfiguration = new ScrimEndPointConfiguration(scimEndpointConfigurationRaw); + // Step 3 : create scim clients for each endpoint - if (scimEndpointConfigurationRaw.get(ScrimProviderConfiguration.CONF_KEY_PROPAGATION_GROUP, false)) { - GroupScimService groupScimService = new GroupScimService(session, scrimProviderConfiguration); + if (scimEndpointConfigurationRaw.get(ScrimEndPointConfiguration.CONF_KEY_PROPAGATION_GROUP, false)) { + GroupScimService groupScimService = new GroupScimService(session, scrimEndPointConfiguration, skipOrStopStrategy); groupScimServices.add(groupScimService); } - if (scimEndpointConfigurationRaw.get(ScrimProviderConfiguration.CONF_KEY_PROPAGATION_USER, false)) { - UserScimService userScimService = new UserScimService(session, scrimProviderConfiguration); + if (scimEndpointConfigurationRaw.get(ScrimEndPointConfiguration.CONF_KEY_PROPAGATION_USER, false)) { + UserScimService userScimService = new UserScimService(session, scrimEndPointConfiguration, skipOrStopStrategy); userScimServices.add(userScimService); } - } catch (Exception e) { - exceptionHandler.handleInvalidEndpointConfiguration(scimEndpointConfigurationRaw, e); + } catch (IllegalArgumentException e) { + if (skipOrStopStrategy.allowInvalidEndpointConfiguration()) { + LOGGER.warn("[SCIM] Invalid Endpoint configuration " + scimEndpointConfigurationRaw.getId(), e); + } else { + throw e; + } } }); } diff --git a/src/main/java/sh/libre/scim/core/ScrimProviderConfiguration.java b/src/main/java/sh/libre/scim/core/ScrimEndPointConfiguration.java similarity index 81% rename from src/main/java/sh/libre/scim/core/ScrimProviderConfiguration.java rename to src/main/java/sh/libre/scim/core/ScrimEndPointConfiguration.java index 6cd5431dc4..0df9cedacc 100644 --- a/src/main/java/sh/libre/scim/core/ScrimProviderConfiguration.java +++ b/src/main/java/sh/libre/scim/core/ScrimEndPointConfiguration.java @@ -3,7 +3,7 @@ package sh.libre.scim.core; import de.captaingoldfish.scim.sdk.client.http.BasicAuth; import org.keycloak.component.ComponentModel; -public class ScrimProviderConfiguration { +public class ScrimEndPointConfiguration { // Configuration keys : also used in Admin Console page public static final String CONF_KEY_AUTH_MODE = "auth-mode"; public static final String CONF_KEY_AUTH_PASSWORD = "auth-pass"; @@ -21,10 +21,10 @@ public class ScrimProviderConfiguration { private final String contentType; private final String authorizationHeaderValue; private final ImportAction importAction; - private final boolean syncImport; - private final boolean syncRefresh; + private final boolean pullFromScimSynchronisationActivated; + private final boolean pushToScimSynchronisationActivated; - public ScrimProviderConfiguration(ComponentModel scimProviderConfiguration) { + public ScrimEndPointConfiguration(ComponentModel scimProviderConfiguration) { try { AuthMode authMode = AuthMode.valueOf(scimProviderConfiguration.get(CONF_KEY_AUTH_MODE)); @@ -43,19 +43,19 @@ public class ScrimProviderConfiguration { endPoint = scimProviderConfiguration.get(CONF_KEY_ENDPOINT, ""); id = scimProviderConfiguration.getId(); importAction = ImportAction.valueOf(scimProviderConfiguration.get(CONF_KEY_SYNC_IMPORT_ACTION)); - syncImport = scimProviderConfiguration.get(CONF_KEY_SYNC_IMPORT, false); - syncRefresh = scimProviderConfiguration.get(CONF_KEY_SYNC_REFRESH, false); + pullFromScimSynchronisationActivated = scimProviderConfiguration.get(CONF_KEY_SYNC_IMPORT, false); + pushToScimSynchronisationActivated = scimProviderConfiguration.get(CONF_KEY_SYNC_REFRESH, false); } catch (IllegalArgumentException e) { throw new IllegalArgumentException("authMode '" + scimProviderConfiguration.get(CONF_KEY_AUTH_MODE) + "' is not supported"); } } - public boolean isSyncRefresh() { - return syncRefresh; + public boolean isPushToScimSynchronisationActivated() { + return pushToScimSynchronisationActivated; } - public boolean isSyncImport() { - return syncImport; + public boolean isPullFromScimSynchronisationActivated() { + return pullFromScimSynchronisationActivated; } public String getContentType() { diff --git a/src/main/java/sh/libre/scim/core/UserScimService.java b/src/main/java/sh/libre/scim/core/UserScimService.java index 23b20795b2..b74e0b85ff 100644 --- a/src/main/java/sh/libre/scim/core/UserScimService.java +++ b/src/main/java/sh/libre/scim/core/UserScimService.java @@ -14,6 +14,7 @@ import org.keycloak.models.RoleMapperModel; import org.keycloak.models.RoleModel; import org.keycloak.models.UserModel; import sh.libre.scim.core.exceptions.InconsistentScimDataException; +import sh.libre.scim.core.exceptions.SkipOrStopStrategy; import sh.libre.scim.core.exceptions.UnexpectedScimDataException; import java.util.ArrayList; @@ -26,8 +27,9 @@ public class UserScimService extends AbstractScimService { public UserScimService( KeycloakSession keycloakSession, - ScrimProviderConfiguration scimProviderConfiguration) { - super(keycloakSession, scimProviderConfiguration, ScimResourceType.USER); + ScrimEndPointConfiguration scimProviderConfiguration, + SkipOrStopStrategy skipOrStopStrategy) { + super(keycloakSession, scimProviderConfiguration, ScimResourceType.USER, skipOrStopStrategy); } @Override diff --git a/src/main/java/sh/libre/scim/core/exceptions/RollbackStrategy.java b/src/main/java/sh/libre/scim/core/exceptions/RollbackStrategy.java new file mode 100644 index 0000000000..90d859305c --- /dev/null +++ b/src/main/java/sh/libre/scim/core/exceptions/RollbackStrategy.java @@ -0,0 +1,22 @@ +package sh.libre.scim.core.exceptions; + +import sh.libre.scim.core.ScrimEndPointConfiguration; + +/** + * In charge of deciding, when facing a SCIM-related issue during an operation (e.g User creation), + * whether we should : + * - Log the issue and let the operation succeed in Keycloack database (potentially unsynchronising + * Keycloack with the SCIM servers) + * - Rollback the whole operation + */ +public interface RollbackStrategy { + + /** + * Indicates whether we should rollback the whole transaction because of the given exception. + * + * @param configuration The SCIM Endpoint configuration for which the exception occured + * @param e the exception that we have to handle + * @return true if transaction should be rolled back, false if we should log and continue operation + */ + boolean shouldRollback(ScrimEndPointConfiguration configuration, ScimPropagationException e); +} diff --git a/src/main/java/sh/libre/scim/core/exceptions/RollbackStrategyFactory.java b/src/main/java/sh/libre/scim/core/exceptions/RollbackStrategyFactory.java new file mode 100644 index 0000000000..b2df15567c --- /dev/null +++ b/src/main/java/sh/libre/scim/core/exceptions/RollbackStrategyFactory.java @@ -0,0 +1,34 @@ +package sh.libre.scim.core.exceptions; + +import sh.libre.scim.core.ScrimEndPointConfiguration; + +public class RollbackStrategyFactory { + + public static RollbackStrategy create(RollbackApproach approach) { + // We could imagine more fine-grained rollback strategies (e.g. based on each Scim endpoint configuration) + return switch (approach) { + case ALWAYS_ROLLBACK -> new AlwaysRollbackStrategy(); + case NEVER_ROLLBACK -> new NeverRollbackStrategy(); + }; + } + + public enum RollbackApproach { + ALWAYS_ROLLBACK, NEVER_ROLLBACK + } + + private static final class AlwaysRollbackStrategy implements RollbackStrategy { + + @Override + public boolean shouldRollback(ScrimEndPointConfiguration configuration, ScimPropagationException e) { + return true; + } + } + + private static final class NeverRollbackStrategy implements RollbackStrategy { + + @Override + public boolean shouldRollback(ScrimEndPointConfiguration configuration, ScimPropagationException e) { + return true; + } + } +} diff --git a/src/main/java/sh/libre/scim/core/exceptions/ScimExceptionHandler.java b/src/main/java/sh/libre/scim/core/exceptions/ScimExceptionHandler.java index 81afeeade0..626c04e8fc 100644 --- a/src/main/java/sh/libre/scim/core/exceptions/ScimExceptionHandler.java +++ b/src/main/java/sh/libre/scim/core/exceptions/ScimExceptionHandler.java @@ -1,9 +1,8 @@ package sh.libre.scim.core.exceptions; import org.jboss.logging.Logger; -import org.keycloak.component.ComponentModel; import org.keycloak.models.KeycloakSession; -import sh.libre.scim.core.ScrimProviderConfiguration; +import sh.libre.scim.core.ScrimEndPointConfiguration; /** * In charge of dealing with SCIM exceptions by ignoring, logging or rollback transaction according to : @@ -15,9 +14,15 @@ public class ScimExceptionHandler { private static final Logger LOGGER = Logger.getLogger(ScimExceptionHandler.class); private final KeycloakSession session; + private final RollbackStrategy rollbackStrategy; public ScimExceptionHandler(KeycloakSession session) { + this(session, RollbackStrategyFactory.create(RollbackStrategyFactory.RollbackApproach.NEVER_ROLLBACK)); + } + + public ScimExceptionHandler(KeycloakSession session, RollbackStrategy rollbackStrategy) { this.session = session; + this.rollbackStrategy = rollbackStrategy; } /** @@ -26,15 +31,13 @@ public class ScimExceptionHandler { * @param scimProviderConfiguration the configuration of the endpoint for which the propagation exception occured * @param e the occuring exception */ - public void handleException(ScrimProviderConfiguration scimProviderConfiguration, ScimPropagationException e) { - LOGGER.error("[SCIM] Error while propagating to SCIM endpoint %s", scimProviderConfiguration.getId(), e); - // TODO Exception Handling : rollback only for critical operations, if configuration says so - // session.getTransactionManager().rollback(); - } - - public void handleInvalidEndpointConfiguration(ComponentModel scimEndpointConfigurationRaw, Exception e) { - LOGGER.error("[SCIM] Invalid Endpoint configuration " + scimEndpointConfigurationRaw.getId(), e); - // TODO Exception Handling is it ok to ignore an invalid Scim endpoint Configuration ? - // IF not, we should propagate the exception here + public void handleException(ScrimEndPointConfiguration scimProviderConfiguration, ScimPropagationException e) { + String errorMessage = "[SCIM] Error while propagating to SCIM endpoint " + scimProviderConfiguration.getId(); + if (rollbackStrategy.shouldRollback(scimProviderConfiguration, e)) { + session.getTransactionManager().rollback(); + LOGGER.error(errorMessage, e); + } else { + LOGGER.warn(errorMessage); + } } } diff --git a/src/main/java/sh/libre/scim/core/exceptions/SkipOrStopStrategy.java b/src/main/java/sh/libre/scim/core/exceptions/SkipOrStopStrategy.java new file mode 100644 index 0000000000..8ad46c7ff9 --- /dev/null +++ b/src/main/java/sh/libre/scim/core/exceptions/SkipOrStopStrategy.java @@ -0,0 +1,66 @@ +package sh.libre.scim.core.exceptions; + +import sh.libre.scim.core.ScrimEndPointConfiguration; + +/** + * In charge of deciding, when facing a SCIM-related issue, whether we should : + * - log a warning, skip the problematic element and continue the rest of the operation + * - stop immediately the whole operation (typically, a synchronisation between SCIM and Keycloack) + */ +public interface SkipOrStopStrategy { + /** + * Indicates if, during a synchronisation from Keycloack to a SCIM endpoint, we should : + * - cancel the whole synchronisation if an element CRUD fail, or + * - keep on with synchronisation, allowing a partial synchronisation + * + * @param configuration the configuration of the endpoint in which the error occurred + * @return true if a partial synchronisation is allowed, + * false if we should stop the whole synchronisation at first issue + */ + boolean allowPartialSynchronizationWhenPushingToScim(ScrimEndPointConfiguration configuration); + + /** + * Indicates if, during a synchronisation from a SCIM endpoint to Keycloack, we should : + * - cancel the whole synchronisation if an element CRUD fail, or + * - keep on with synchronisation, allowing a partial synchronisation + * + * @param configuration the configuration of the endpoint in which the error occurred + * @return true if a partial synchronisation is allowed, + * false if we should interrupt the whole synchronisation at first issue + */ + boolean allowPartialSynchronizationWhenPullingFromScim(ScrimEndPointConfiguration configuration); + + + /** + * Indicates if, when we propagate a group creation or update to a SCIM endpoint and some + * of its members are not mapped to SCIM, we should allow partial group update or interrupt completely. + * + * @param configuration the configuration of the endpoint in which the error occurred + * @return true if a partial group update is allowed, + * false if we should interrupt the group update in case of any unmapped member + */ + boolean allowMissingMembersWhenPushingGroupToScim(ScrimEndPointConfiguration configuration); + + /** + * Indicates if, when facing an invalid SCIM endpoint configuration (resulting in a unreachable SCIM server), + * we should stop or ignore this configuration. + * + * @return true the invalid endpoint should be ignored, + * * false if we should interrupt the rest of the synchronisation + */ + boolean allowInvalidEndpointConfiguration(); + + /** + * Indicates if, when trying to pull User or Groups from a SCIM endpoint, + * we encounter a invalid data (e.g. group with empty name), we should : + * - Skip the invalid element pull and continue + * - Cancel the whole synchronisation + * + * @param configuration the configuration of the endpoint in which the error occurred + * @return true if we should skip the invalid data synchronisation and pursue, + * false if we should interrupt immediately the whole synchronisation + */ + boolean skipInvalidDataFromScimEndpoint(ScrimEndPointConfiguration configuration); + + +} diff --git a/src/main/java/sh/libre/scim/core/exceptions/SkipOrStopStrategyFactory.java b/src/main/java/sh/libre/scim/core/exceptions/SkipOrStopStrategyFactory.java new file mode 100644 index 0000000000..3bde1d4060 --- /dev/null +++ b/src/main/java/sh/libre/scim/core/exceptions/SkipOrStopStrategyFactory.java @@ -0,0 +1,74 @@ +package sh.libre.scim.core.exceptions; + +import sh.libre.scim.core.ScrimEndPointConfiguration; + +public class SkipOrStopStrategyFactory { + + public static SkipOrStopStrategy create(SkipOrStopApproach approach) { + // We could imagine more fine-grained strategies (e.g. based on each Scim endpoint configuration) + return switch (approach) { + case ALWAYS_STOP -> new AlwaysStopStrategy(); + case ALWAYS_SKIP_AND_CONTINUE -> new AlwaysSkipAndContinueStrategy(); + }; + } + + public enum SkipOrStopApproach { + ALWAYS_SKIP_AND_CONTINUE, ALWAYS_STOP + } + + private static final class AlwaysStopStrategy implements SkipOrStopStrategy { + + @Override + public boolean allowPartialSynchronizationWhenPushingToScim(ScrimEndPointConfiguration configuration) { + return false; + } + + @Override + public boolean allowPartialSynchronizationWhenPullingFromScim(ScrimEndPointConfiguration configuration) { + return false; + } + + @Override + public boolean allowMissingMembersWhenPushingGroupToScim(ScrimEndPointConfiguration configuration) { + return false; + } + + @Override + public boolean allowInvalidEndpointConfiguration() { + return false; + } + + @Override + public boolean skipInvalidDataFromScimEndpoint(ScrimEndPointConfiguration configuration) { + return false; + } + } + + private static final class AlwaysSkipAndContinueStrategy implements SkipOrStopStrategy { + + @Override + public boolean allowPartialSynchronizationWhenPushingToScim(ScrimEndPointConfiguration configuration) { + return true; + } + + @Override + public boolean allowPartialSynchronizationWhenPullingFromScim(ScrimEndPointConfiguration configuration) { + return true; + } + + @Override + public boolean allowMissingMembersWhenPushingGroupToScim(ScrimEndPointConfiguration configuration) { + return true; + } + + @Override + public boolean allowInvalidEndpointConfiguration() { + return true; + } + + @Override + public boolean skipInvalidDataFromScimEndpoint(ScrimEndPointConfiguration configuration) { + return true; + } + } +} diff --git a/src/main/java/sh/libre/scim/storage/ScimEndpointConfigurationStorageProviderFactory.java b/src/main/java/sh/libre/scim/storage/ScimEndpointConfigurationStorageProviderFactory.java index 8bc99d6211..f825ab6651 100644 --- a/src/main/java/sh/libre/scim/storage/ScimEndpointConfigurationStorageProviderFactory.java +++ b/src/main/java/sh/libre/scim/storage/ScimEndpointConfigurationStorageProviderFactory.java @@ -19,7 +19,7 @@ import org.keycloak.storage.user.ImportSynchronization; import org.keycloak.storage.user.SynchronizationResult; import org.keycloak.timer.TimerProvider; import sh.libre.scim.core.ScimDispatcher; -import sh.libre.scim.core.ScrimProviderConfiguration; +import sh.libre.scim.core.ScrimEndPointConfiguration; import java.time.Duration; import java.util.Date; @@ -94,7 +94,7 @@ public class ScimEndpointConfigurationStorageProviderFactory // These Config Properties will be use to generate configuration page in Admin Console return ProviderConfigurationBuilder.create() .property() - .name(ScrimProviderConfiguration.CONF_KEY_ENDPOINT) + .name(ScrimEndPointConfiguration.CONF_KEY_ENDPOINT) .type(ProviderConfigProperty.STRING_TYPE) .required(true) .label("SCIM 2.0 endpoint") @@ -102,7 +102,7 @@ public class ScimEndpointConfigurationStorageProviderFactory "URL (/ServiceProviderConfig /Schemas and /ResourcesTypes should be accessible)") .add() .property() - .name(ScrimProviderConfiguration.CONF_KEY_CONTENT_TYPE) + .name(ScrimEndPointConfiguration.CONF_KEY_CONTENT_TYPE) .type(ProviderConfigProperty.LIST_TYPE) .label("Endpoint content type") .helpText("Only used when endpoint doesn't support application/scim+json") @@ -110,7 +110,7 @@ public class ScimEndpointConfigurationStorageProviderFactory .defaultValue(HttpHeader.SCIM_CONTENT_TYPE) .add() .property() - .name(ScrimProviderConfiguration.CONF_KEY_AUTH_MODE) + .name(ScrimEndPointConfiguration.CONF_KEY_AUTH_MODE) .type(ProviderConfigProperty.LIST_TYPE) .label("Auth mode") .helpText("Select the authorization mode") @@ -118,38 +118,38 @@ public class ScimEndpointConfigurationStorageProviderFactory .defaultValue("NONE") .add() .property() - .name(ScrimProviderConfiguration.CONF_KEY_AUTH_USER) + .name(ScrimEndPointConfiguration.CONF_KEY_AUTH_USER) .type(ProviderConfigProperty.STRING_TYPE) .label("Auth username") .helpText("Required for basic authentication.") .add() .property() - .name(ScrimProviderConfiguration.CONF_KEY_AUTH_PASSWORD) + .name(ScrimEndPointConfiguration.CONF_KEY_AUTH_PASSWORD) .type(ProviderConfigProperty.PASSWORD) .label("Auth password/token") .helpText("Password or token required for basic or bearer authentication.") .add() .property() - .name(ScrimProviderConfiguration.CONF_KEY_PROPAGATION_USER) + .name(ScrimEndPointConfiguration.CONF_KEY_PROPAGATION_USER) .type(ProviderConfigProperty.BOOLEAN_TYPE) .label("Enable user propagation") .helpText("Should operation on users be propagated to this provider?") .defaultValue(BooleanUtils.TRUE) .add() .property() - .name(ScrimProviderConfiguration.CONF_KEY_PROPAGATION_GROUP) + .name(ScrimEndPointConfiguration.CONF_KEY_PROPAGATION_GROUP) .type(ProviderConfigProperty.BOOLEAN_TYPE) .label("Enable group propagation") .helpText("Should operation on groups be propagated to this provider?") .defaultValue(BooleanUtils.TRUE) .add() .property() - .name(ScrimProviderConfiguration.CONF_KEY_SYNC_IMPORT) + .name(ScrimEndPointConfiguration.CONF_KEY_SYNC_IMPORT) .type(ProviderConfigProperty.BOOLEAN_TYPE) .label("Enable import during sync") .add() .property() - .name(ScrimProviderConfiguration.CONF_KEY_SYNC_IMPORT_ACTION) + .name(ScrimEndPointConfiguration.CONF_KEY_SYNC_IMPORT_ACTION) .type(ProviderConfigProperty.LIST_TYPE) .label("Import action") .helpText("What to do when the user doesn't exists in Keycloak.") @@ -157,7 +157,7 @@ public class ScimEndpointConfigurationStorageProviderFactory .defaultValue("CREATE_LOCAL") .add() .property() - .name(ScrimProviderConfiguration.CONF_KEY_SYNC_REFRESH) + .name(ScrimEndPointConfiguration.CONF_KEY_SYNC_REFRESH) .type(ProviderConfigProperty.BOOLEAN_TYPE) .label("Enable refresh during sync") .add()