From 620c9e84bb107d94a8cf6895f23234a2735f88b1 Mon Sep 17 00:00:00 2001 From: Alex Morel Date: Tue, 25 Jun 2024 16:45:35 +0200 Subject: [PATCH] Exception Handler - Step 5 : basic Rolback Strategy implementation --- .../libre/scim/core/AbstractScimService.java | 42 +++++++++---------- .../sh/libre/scim/core/GroupScimService.java | 13 +++--- .../java/sh/libre/scim/core/ScimClient.java | 14 +++---- .../sh/libre/scim/core/UserScimService.java | 6 +-- .../ErrorForScimEndpointException.java | 8 ---- .../InconsistentScimDataException.java | 7 ---- .../InconsistentScimMappingException.java | 7 ++++ ...alidResponseFromScimEndpointException.java | 18 ++++++++ ...RollbackOnlyForCriticalErrorsStrategy.java | 32 ++++++++++++++ .../exceptions/RollbackStrategyFactory.java | 4 +- .../core/exceptions/ScimExceptionHandler.java | 2 +- .../META-INF/scim-resource-changelog.xml | 27 +++++++----- 12 files changed, 115 insertions(+), 65 deletions(-) delete mode 100644 src/main/java/sh/libre/scim/core/exceptions/ErrorForScimEndpointException.java delete mode 100644 src/main/java/sh/libre/scim/core/exceptions/InconsistentScimDataException.java create mode 100644 src/main/java/sh/libre/scim/core/exceptions/InconsistentScimMappingException.java create mode 100644 src/main/java/sh/libre/scim/core/exceptions/InvalidResponseFromScimEndpointException.java create mode 100644 src/main/java/sh/libre/scim/core/exceptions/RollbackOnlyForCriticalErrorsStrategy.java diff --git a/src/main/java/sh/libre/scim/core/AbstractScimService.java b/src/main/java/sh/libre/scim/core/AbstractScimService.java index dda7b72687..c350702911 100644 --- a/src/main/java/sh/libre/scim/core/AbstractScimService.java +++ b/src/main/java/sh/libre/scim/core/AbstractScimService.java @@ -6,8 +6,8 @@ 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.exceptions.ErrorForScimEndpointException; -import sh.libre.scim.core.exceptions.InconsistentScimDataException; +import sh.libre.scim.core.exceptions.InconsistentScimMappingException; +import sh.libre.scim.core.exceptions.InvalidResponseFromScimEndpointException; import sh.libre.scim.core.exceptions.SkipOrStopStrategy; import sh.libre.scim.core.exceptions.UnexpectedScimDataException; import sh.libre.scim.jpa.ScimResourceDao; @@ -45,7 +45,7 @@ public abstract class AbstractScimService new InconsistentScimDataException("Failed to find SCIM mapping for " + keycloakId)); + .orElseThrow(() -> new InconsistentScimMappingException("Failed to find SCIM mapping for " + keycloakId)); S scimForReplace = scimRequestBodyForUpdate(roleMapperModel, entityOnRemoteScimId); scimClient.update(entityOnRemoteScimId, scimForReplace); } - protected abstract S scimRequestBodyForUpdate(K roleMapperModel, EntityOnRemoteScimId externalId) throws InconsistentScimDataException; + protected abstract S scimRequestBodyForUpdate(K roleMapperModel, EntityOnRemoteScimId externalId) throws InconsistentScimMappingException; - public void delete(KeycloakId id) throws InconsistentScimDataException, ErrorForScimEndpointException { + public void delete(KeycloakId id) throws InconsistentScimMappingException, InvalidResponseFromScimEndpointException { ScimResourceMapping resource = findMappingById(id) - .orElseThrow(() -> new InconsistentScimDataException("Failed to delete resource %s, scim mapping not found: ".formatted(id))); + .orElseThrow(() -> new InconsistentScimMappingException("Failed to delete resource %s, scim mapping not found: ".formatted(id))); EntityOnRemoteScimId externalId = resource.getExternalIdAsEntityOnRemoteScimId(); scimClient.delete(externalId); getScimResourceDao().delete(resource); } - public void pushAllResourcesToScim(SynchronizationResult syncRes) throws ErrorForScimEndpointException, InconsistentScimDataException { + public void pushAllResourcesToScim(SynchronizationResult syncRes) throws InvalidResponseFromScimEndpointException, InconsistentScimMappingException { LOGGER.info("[SCIM] Push resources to endpoint " + this.getConfiguration().getEndPoint()); try (Stream resourcesStream = getResourceStream()) { Set resources = resourcesStream.collect(Collectors.toUnmodifiableSet()); @@ -94,14 +94,14 @@ public abstract class AbstractScimService getResourceStream(); - protected abstract KeycloakId createEntity(S resource) throws UnexpectedScimDataException; + protected abstract KeycloakId createEntity(S resource) throws UnexpectedScimDataException, InconsistentScimMappingException; - protected abstract Optional matchKeycloakMappingByScimProperties(S resource) throws InconsistentScimDataException; + protected abstract Optional matchKeycloakMappingByScimProperties(S resource) throws InconsistentScimMappingException; protected abstract boolean entityExists(KeycloakId keycloakId); - public void sync(SynchronizationResult syncRes) throws InconsistentScimDataException, ErrorForScimEndpointException, UnexpectedScimDataException { + public void sync(SynchronizationResult syncRes) throws InconsistentScimMappingException, InvalidResponseFromScimEndpointException, UnexpectedScimDataException { if (this.scimProviderConfiguration.isPullFromScimSynchronisationActivated()) { this.pullAllResourcesFromScim(syncRes); } diff --git a/src/main/java/sh/libre/scim/core/GroupScimService.java b/src/main/java/sh/libre/scim/core/GroupScimService.java index 92857ec2fd..35daece518 100644 --- a/src/main/java/sh/libre/scim/core/GroupScimService.java +++ b/src/main/java/sh/libre/scim/core/GroupScimService.java @@ -10,7 +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.exceptions.InconsistentScimDataException; +import sh.libre.scim.core.exceptions.InconsistentScimMappingException; import sh.libre.scim.core.exceptions.SkipOrStopStrategy; import sh.libre.scim.core.exceptions.UnexpectedScimDataException; import sh.libre.scim.jpa.ScimResourceMapping; @@ -55,7 +55,7 @@ public class GroupScimService extends AbstractScimService { } @Override - protected KeycloakId createEntity(Group resource) throws UnexpectedScimDataException { + protected KeycloakId createEntity(Group resource) throws UnexpectedScimDataException, InconsistentScimMappingException { String displayName = resource.getDisplayName() .filter(StringUtils::isNotBlank) .orElseThrow(() -> new UnexpectedScimDataException("Remote Scim group has empty name, can't create. Resource id = %s".formatted(resource.getId()))); @@ -68,8 +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))); + .orElseThrow(() -> new InconsistentScimMappingException("can't find mapping for group member %s".formatted(externalId))); UserModel userModel = getKeycloakDao().getUserById(userId); userModel.joinGroup(group); } @@ -88,7 +87,7 @@ public class GroupScimService extends AbstractScimService { } @Override - protected Group scimRequestBodyForCreate(GroupModel groupModel) throws InconsistentScimDataException { + protected Group scimRequestBodyForCreate(GroupModel groupModel) throws InconsistentScimMappingException { Set members = getKeycloakDao().getGroupMembers(groupModel); Group group = new Group(); group.setExternalId(groupModel.getId()); @@ -108,7 +107,7 @@ public class GroupScimService extends AbstractScimService { if (skipOrStopStrategy.allowMissingMembersWhenPushingGroupToScim(this.getConfiguration())) { LOGGER.warn(message); } else { - throw new InconsistentScimDataException(message); + throw new InconsistentScimMappingException(message); } } } @@ -116,7 +115,7 @@ public class GroupScimService extends AbstractScimService { } @Override - protected Group scimRequestBodyForUpdate(GroupModel groupModel, EntityOnRemoteScimId externalId) throws InconsistentScimDataException { + protected Group scimRequestBodyForUpdate(GroupModel groupModel, EntityOnRemoteScimId externalId) throws InconsistentScimMappingException { 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 9e0d136871..7649ccb03c 100644 --- a/src/main/java/sh/libre/scim/core/ScimClient.java +++ b/src/main/java/sh/libre/scim/core/ScimClient.java @@ -12,7 +12,7 @@ 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.exceptions.ErrorForScimEndpointException; +import sh.libre.scim.core.exceptions.InvalidResponseFromScimEndpointException; import java.util.Collections; import java.util.HashMap; @@ -64,7 +64,7 @@ public class ScimClient implements AutoCloseable { return new ScimClient(scimRequestBuilder, scimResourceType); } - public EntityOnRemoteScimId create(KeycloakId id, S scimForCreation) throws ErrorForScimEndpointException { + public EntityOnRemoteScimId create(KeycloakId id, S scimForCreation) throws InvalidResponseFromScimEndpointException { Optional scimForCreationId = scimForCreation.getId(); if (scimForCreationId.isPresent()) { throw new IllegalArgumentException( @@ -82,12 +82,12 @@ public class ScimClient implements AutoCloseable { S resource = response.getResource(); return resource.getId() .map(EntityOnRemoteScimId::new) - .orElseThrow(() -> new ErrorForScimEndpointException("Created SCIM resource does not have id")); + .orElseThrow(() -> new InvalidResponseFromScimEndpointException(response, "Created SCIM resource does not have id")); } - private void checkResponseIsSuccess(ServerResponse response) throws ErrorForScimEndpointException { + private void checkResponseIsSuccess(ServerResponse response) throws InvalidResponseFromScimEndpointException { if (!response.isSuccess()) { - throw new ErrorForScimEndpointException("Server answered with status " + response.getResponseBody() + ": " + response.getResponseBody()); + throw new InvalidResponseFromScimEndpointException(response, "Server answered with status " + response.getResponseBody() + ": " + response.getResponseBody()); } } @@ -99,7 +99,7 @@ public class ScimClient implements AutoCloseable { return scimResourceType.getResourceClass(); } - public void update(EntityOnRemoteScimId externalId, S scimForReplace) throws ErrorForScimEndpointException { + public void update(EntityOnRemoteScimId externalId, S scimForReplace) throws InvalidResponseFromScimEndpointException { Retry retry = retryRegistry.retry("replace-%s".formatted(externalId.asString())); // TODO Exception handling : check that all exceptions are wrapped in server response ServerResponse response = retry.executeSupplier(() -> scimRequestBuilder @@ -110,7 +110,7 @@ public class ScimClient implements AutoCloseable { checkResponseIsSuccess(response); } - public void delete(EntityOnRemoteScimId externalId) throws ErrorForScimEndpointException { + public void delete(EntityOnRemoteScimId externalId) throws InvalidResponseFromScimEndpointException { Retry retry = retryRegistry.retry("delete-%s".formatted(externalId.asString())); // TODO Exception handling : check that all exceptions are wrapped in server response ServerResponse response = retry.executeSupplier(() -> scimRequestBuilder diff --git a/src/main/java/sh/libre/scim/core/UserScimService.java b/src/main/java/sh/libre/scim/core/UserScimService.java index b74e0b85ff..54c8345a20 100644 --- a/src/main/java/sh/libre/scim/core/UserScimService.java +++ b/src/main/java/sh/libre/scim/core/UserScimService.java @@ -13,7 +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.exceptions.InconsistentScimDataException; +import sh.libre.scim.core.exceptions.InconsistentScimMappingException; import sh.libre.scim.core.exceptions.SkipOrStopStrategy; import sh.libre.scim.core.exceptions.UnexpectedScimDataException; @@ -43,7 +43,7 @@ public class UserScimService extends AbstractScimService { } @Override - protected Optional matchKeycloakMappingByScimProperties(User resource) throws InconsistentScimDataException { + protected Optional matchKeycloakMappingByScimProperties(User resource) throws InconsistentScimMappingException { Optional matchedByUsername = resource.getUserName() .map(getKeycloakDao()::getUserByUsername) .map(this::getId); @@ -55,7 +55,7 @@ public class UserScimService extends AbstractScimService { if (matchedByUsername.isPresent() && matchedByEmail.isPresent() && !matchedByUsername.equals(matchedByEmail)) { - throw new InconsistentScimDataException("Found 2 possible users for remote user " + matchedByUsername.get() + " - " + matchedByEmail.get()); + throw new InconsistentScimMappingException("Found 2 possible users for remote user " + matchedByUsername.get() + " - " + matchedByEmail.get()); } if (matchedByUsername.isPresent()) { return matchedByUsername; diff --git a/src/main/java/sh/libre/scim/core/exceptions/ErrorForScimEndpointException.java b/src/main/java/sh/libre/scim/core/exceptions/ErrorForScimEndpointException.java deleted file mode 100644 index 1705e9981d..0000000000 --- a/src/main/java/sh/libre/scim/core/exceptions/ErrorForScimEndpointException.java +++ /dev/null @@ -1,8 +0,0 @@ -package sh.libre.scim.core.exceptions; - -public class ErrorForScimEndpointException extends ScimPropagationException { - - public ErrorForScimEndpointException(String message) { - super(message); - } -} diff --git a/src/main/java/sh/libre/scim/core/exceptions/InconsistentScimDataException.java b/src/main/java/sh/libre/scim/core/exceptions/InconsistentScimDataException.java deleted file mode 100644 index 947a9f1eb3..0000000000 --- a/src/main/java/sh/libre/scim/core/exceptions/InconsistentScimDataException.java +++ /dev/null @@ -1,7 +0,0 @@ -package sh.libre.scim.core.exceptions; - -public class InconsistentScimDataException extends ScimPropagationException { - public InconsistentScimDataException(String message) { - super(message); - } -} diff --git a/src/main/java/sh/libre/scim/core/exceptions/InconsistentScimMappingException.java b/src/main/java/sh/libre/scim/core/exceptions/InconsistentScimMappingException.java new file mode 100644 index 0000000000..44f7eb46e6 --- /dev/null +++ b/src/main/java/sh/libre/scim/core/exceptions/InconsistentScimMappingException.java @@ -0,0 +1,7 @@ +package sh.libre.scim.core.exceptions; + +public class InconsistentScimMappingException extends ScimPropagationException { + public InconsistentScimMappingException(String message) { + super(message); + } +} diff --git a/src/main/java/sh/libre/scim/core/exceptions/InvalidResponseFromScimEndpointException.java b/src/main/java/sh/libre/scim/core/exceptions/InvalidResponseFromScimEndpointException.java new file mode 100644 index 0000000000..27c5c97b81 --- /dev/null +++ b/src/main/java/sh/libre/scim/core/exceptions/InvalidResponseFromScimEndpointException.java @@ -0,0 +1,18 @@ +package sh.libre.scim.core.exceptions; + +import de.captaingoldfish.scim.sdk.client.response.ServerResponse; + +public class InvalidResponseFromScimEndpointException extends ScimPropagationException { + + private final ServerResponse response; + + public InvalidResponseFromScimEndpointException(ServerResponse response, String message) { + super(message); + this.response = response; + } + + public ServerResponse getResponse() { + return response; + } + +} diff --git a/src/main/java/sh/libre/scim/core/exceptions/RollbackOnlyForCriticalErrorsStrategy.java b/src/main/java/sh/libre/scim/core/exceptions/RollbackOnlyForCriticalErrorsStrategy.java new file mode 100644 index 0000000000..fae0926e36 --- /dev/null +++ b/src/main/java/sh/libre/scim/core/exceptions/RollbackOnlyForCriticalErrorsStrategy.java @@ -0,0 +1,32 @@ +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 index b2df15567c..23651a7a38 100644 --- a/src/main/java/sh/libre/scim/core/exceptions/RollbackStrategyFactory.java +++ b/src/main/java/sh/libre/scim/core/exceptions/RollbackStrategyFactory.java @@ -9,13 +9,15 @@ public class RollbackStrategyFactory { 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 + ALWAYS_ROLLBACK, NEVER_ROLLBACK, CRITICAL_ONLY_ROLLBACK } + private static final class AlwaysRollbackStrategy implements RollbackStrategy { @Override 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 626c04e8fc..7a6d6bf79d 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.NEVER_ROLLBACK)); + this(session, RollbackStrategyFactory.create(RollbackStrategyFactory.RollbackApproach.CRITICAL_ONLY_ROLLBACK)); } public ScimExceptionHandler(KeycloakSession session, RollbackStrategy rollbackStrategy) { diff --git a/src/main/resources/META-INF/scim-resource-changelog.xml b/src/main/resources/META-INF/scim-resource-changelog.xml index 45be732f89..cf300faad5 100644 --- a/src/main/resources/META-INF/scim-resource-changelog.xml +++ b/src/main/resources/META-INF/scim-resource-changelog.xml @@ -1,28 +1,35 @@ - + - + - + - + - + - + - + - - - + + + \ No newline at end of file