From 387abc30f8b94d1ecb92e01c3c66ca6a848351b4 Mon Sep 17 00:00:00 2001 From: Alex Morel Date: Thu, 27 Jun 2024 11:15:59 +0200 Subject: [PATCH] Use enum interfaces for RollbackStrategy and SkipOrStopStrategy --- .../sh/libre/scim/core/ScimDispatcher.java | 6 +-- .../core/exceptions/RollbackApproach.java | 44 +++++++++++++++++++ ...RollbackOnlyForCriticalErrorsStrategy.java | 32 -------------- .../exceptions/RollbackStrategyFactory.java | 36 --------------- .../core/exceptions/ScimExceptionHandler.java | 2 +- ...gyFactory.java => SkipOrStopApproach.java} | 23 ++-------- 6 files changed, 51 insertions(+), 92 deletions(-) create mode 100644 src/main/java/sh/libre/scim/core/exceptions/RollbackApproach.java delete mode 100644 src/main/java/sh/libre/scim/core/exceptions/RollbackOnlyForCriticalErrorsStrategy.java delete mode 100644 src/main/java/sh/libre/scim/core/exceptions/RollbackStrategyFactory.java rename src/main/java/sh/libre/scim/core/exceptions/{SkipOrStopStrategyFactory.java => SkipOrStopApproach.java} (70%) diff --git a/src/main/java/sh/libre/scim/core/ScimDispatcher.java b/src/main/java/sh/libre/scim/core/ScimDispatcher.java index 0ab00fcc73..b0142ee4ae 100644 --- a/src/main/java/sh/libre/scim/core/ScimDispatcher.java +++ b/src/main/java/sh/libre/scim/core/ScimDispatcher.java @@ -5,8 +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.SkipOrStopApproach; import sh.libre.scim.core.exceptions.SkipOrStopStrategy; -import sh.libre.scim.core.exceptions.SkipOrStopStrategyFactory; import sh.libre.scim.storage.ScimEndpointConfigurationStorageProviderFactory; import java.util.ArrayList; @@ -34,9 +34,7 @@ public class ScimDispatcher { 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 - ); + this.skipOrStopStrategy = SkipOrStopApproach.ALWAYS_SKIP_AND_CONTINUE; } /** diff --git a/src/main/java/sh/libre/scim/core/exceptions/RollbackApproach.java b/src/main/java/sh/libre/scim/core/exceptions/RollbackApproach.java new file mode 100644 index 0000000000..fa5f5983dd --- /dev/null +++ b/src/main/java/sh/libre/scim/core/exceptions/RollbackApproach.java @@ -0,0 +1,44 @@ +package sh.libre.scim.core.exceptions; + +import sh.libre.scim.core.ScrimEndPointConfiguration; + + +public enum RollbackApproach implements RollbackStrategy { + ALWAYS_ROLLBACK { + @Override + public boolean shouldRollback(ScrimEndPointConfiguration configuration, ScimPropagationException e) { + return true; + } + }, + NEVER_ROLLBACK { + @Override + public boolean shouldRollback(ScrimEndPointConfiguration configuration, ScimPropagationException e) { + return false; + } + }, + CRITICAL_ONLY_ROLLBACK { + @Override + public boolean shouldRollback(ScrimEndPointConfiguration configuration, ScimPropagationException e) { + if (e instanceof InconsistentScimMappingException) { + // Occurs when mapping between a SCIM resource and a keycloak user failed (missing, ambiguous..) + // Log can be sufficient here, no rollback required + return false; + } + if (e instanceof UnexpectedScimDataException) { + // Occurs when a SCIM endpoint sends invalid date (e.g. group with empty name, user without ids...) + // No rollback required : we cannot recover. This needs to be fixed in the SCIM endpoint data + return false; + } + if (e instanceof InvalidResponseFromScimEndpointException invalidResponseFromScimEndpointException) { + return shouldRollbackBecauseOfResponse(invalidResponseFromScimEndpointException); + } + // Should not occur + throw new IllegalStateException("Unkown ScimPropagationException", e); + } + + private boolean shouldRollbackBecauseOfResponse(InvalidResponseFromScimEndpointException e) { + int httpStatus = e.getResponse().getHttpStatus(); + return httpStatus == 500; + } + } +} diff --git a/src/main/java/sh/libre/scim/core/exceptions/RollbackOnlyForCriticalErrorsStrategy.java b/src/main/java/sh/libre/scim/core/exceptions/RollbackOnlyForCriticalErrorsStrategy.java deleted file mode 100644 index fae0926e36..0000000000 --- a/src/main/java/sh/libre/scim/core/exceptions/RollbackOnlyForCriticalErrorsStrategy.java +++ /dev/null @@ -1,32 +0,0 @@ -package sh.libre.scim.core.exceptions; - -import sh.libre.scim.core.ScrimEndPointConfiguration; - -public class RollbackOnlyForCriticalErrorsStrategy implements RollbackStrategy { - - private boolean shouldRollback(InvalidResponseFromScimEndpointException e) { - int httpStatus = e.getResponse().getHttpStatus(); - return httpStatus == 500; - } - - @Override - public boolean shouldRollback(ScrimEndPointConfiguration configuration, ScimPropagationException e) { - if (e instanceof InvalidResponseFromScimEndpointException invalidResponseFromScimEndpointException) { - return shouldRollback(invalidResponseFromScimEndpointException); - } - if (e instanceof InconsistentScimMappingException) { - // Occurs when mapping between a SCIM resource and a keycloak user failed (missing, ambiguous..) - // Log can be sufficient here, no rollback required - return false; - } - if (e instanceof UnexpectedScimDataException) { - // Occurs when a SCIM endpoint sends invalid date (e.g. group with empty name, user without ids...) - // No rollback required : we cannot recover. This needs to be fixed in the SCIM endpoint data - return false; - } - // Should not occur - throw new IllegalStateException("Unkown 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 deleted file mode 100644 index 23651a7a38..0000000000 --- a/src/main/java/sh/libre/scim/core/exceptions/RollbackStrategyFactory.java +++ /dev/null @@ -1,36 +0,0 @@ -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(); - case CRITICAL_ONLY_ROLLBACK -> new RollbackOnlyForCriticalErrorsStrategy(); - }; - } - - public enum RollbackApproach { - ALWAYS_ROLLBACK, NEVER_ROLLBACK, CRITICAL_ONLY_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 7a6d6bf79d..b525ae1279 100644 --- a/src/main/java/sh/libre/scim/core/exceptions/ScimExceptionHandler.java +++ b/src/main/java/sh/libre/scim/core/exceptions/ScimExceptionHandler.java @@ -17,7 +17,7 @@ public class ScimExceptionHandler { private final RollbackStrategy rollbackStrategy; public ScimExceptionHandler(KeycloakSession session) { - this(session, RollbackStrategyFactory.create(RollbackStrategyFactory.RollbackApproach.CRITICAL_ONLY_ROLLBACK)); + this(session, RollbackApproach.CRITICAL_ONLY_ROLLBACK); } public ScimExceptionHandler(KeycloakSession session, RollbackStrategy rollbackStrategy) { diff --git a/src/main/java/sh/libre/scim/core/exceptions/SkipOrStopStrategyFactory.java b/src/main/java/sh/libre/scim/core/exceptions/SkipOrStopApproach.java similarity index 70% rename from src/main/java/sh/libre/scim/core/exceptions/SkipOrStopStrategyFactory.java rename to src/main/java/sh/libre/scim/core/exceptions/SkipOrStopApproach.java index 3bde1d4060..e0669d59db 100644 --- a/src/main/java/sh/libre/scim/core/exceptions/SkipOrStopStrategyFactory.java +++ b/src/main/java/sh/libre/scim/core/exceptions/SkipOrStopApproach.java @@ -2,22 +2,9 @@ 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 { +public enum SkipOrStopApproach implements SkipOrStopStrategy { + ALWAYS_SKIP_AND_CONTINUE { @Override public boolean allowPartialSynchronizationWhenPushingToScim(ScrimEndPointConfiguration configuration) { return false; @@ -42,10 +29,8 @@ public class SkipOrStopStrategyFactory { public boolean skipInvalidDataFromScimEndpoint(ScrimEndPointConfiguration configuration) { return false; } - } - - private static final class AlwaysSkipAndContinueStrategy implements SkipOrStopStrategy { - + }, + ALWAYS_STOP { @Override public boolean allowPartialSynchronizationWhenPushingToScim(ScrimEndPointConfiguration configuration) { return true;