From 9d52c0eb4b676ffa70b3efe76755cd04f82e0fc0 Mon Sep 17 00:00:00 2001 From: Alex Morel Date: Tue, 25 Jun 2024 10:35:56 +0200 Subject: [PATCH] Exception Handler - Step 3 : exception categorization --- .../libre/scim/core/AbstractScimService.java | 273 +++++++++--------- .../sh/libre/scim/core/GroupScimService.java | 29 +- .../java/sh/libre/scim/core/KeycloakId.java | 1 + .../java/sh/libre/scim/core/ScimClient.java | 23 +- .../sh/libre/scim/core/ScimDispatcher.java | 2 + .../sh/libre/scim/core/UserScimService.java | 25 +- .../ErrorForScimEndpointException.java | 8 + .../InconsistentScimDataException.java | 7 + .../ScimExceptionHandler.java | 3 +- .../ScimPropagationException.java | 4 +- .../UnexpectedScimDataException.java | 7 + .../scim/event/ScimEventListenerProvider.java | 24 +- .../sh/libre/scim/jpa/ScimResourceDao.java | 16 +- ...Resource.java => ScimResourceMapping.java} | 4 +- .../libre/scim/jpa/ScimResourceProvider.java | 2 +- ...ntConfigurationStorageProviderFactory.java | 2 +- 16 files changed, 224 insertions(+), 206 deletions(-) create mode 100644 src/main/java/sh/libre/scim/core/exceptions/ErrorForScimEndpointException.java create mode 100644 src/main/java/sh/libre/scim/core/exceptions/InconsistentScimDataException.java rename src/main/java/sh/libre/scim/core/{ => exceptions}/ScimExceptionHandler.java (94%) rename src/main/java/sh/libre/scim/core/{ => exceptions}/ScimPropagationException.java (64%) create mode 100644 src/main/java/sh/libre/scim/core/exceptions/UnexpectedScimDataException.java rename src/main/java/sh/libre/scim/jpa/{ScimResource.java => ScimResourceMapping.java} (96%) diff --git a/src/main/java/sh/libre/scim/core/AbstractScimService.java b/src/main/java/sh/libre/scim/core/AbstractScimService.java index b72bef678d..9fc896bbe0 100644 --- a/src/main/java/sh/libre/scim/core/AbstractScimService.java +++ b/src/main/java/sh/libre/scim/core/AbstractScimService.java @@ -1,14 +1,17 @@ package sh.libre.scim.core; -import de.captaingoldfish.scim.sdk.common.exceptions.ResponseException; import de.captaingoldfish.scim.sdk.common.resources.ResourceNode; import de.captaingoldfish.scim.sdk.common.resources.complex.Meta; 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.jpa.ScimResource; +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.UnexpectedScimDataException; import sh.libre.scim.jpa.ScimResourceDao; +import sh.libre.scim.jpa.ScimResourceMapping; import java.net.URI; import java.net.URISyntaxException; @@ -17,7 +20,8 @@ import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; -public abstract class AbstractScimService implements AutoCloseable { +// TODO Document K and S +public abstract class AbstractScimService implements AutoCloseable { private static final Logger LOGGER = Logger.getLogger(AbstractScimService.class); @@ -36,25 +40,130 @@ public abstract class AbstractScimService new InconsistentScimDataException("Failed to find SCIM mapping for " + keycloakId)); + S scimForReplace = scimRequestBodyForUpdate(roleMapperModel, entityOnRemoteScimId); + scimClient.update(entityOnRemoteScimId, scimForReplace); + } - protected abstract KeycloakId getId(RMM roleMapperModel); + protected abstract S scimRequestBodyForUpdate(K roleMapperModel, EntityOnRemoteScimId externalId); - protected abstract boolean isSkip(RMM roleMapperModel); + public void delete(KeycloakId id) throws InconsistentScimDataException, ErrorForScimEndpointException { + ScimResourceMapping resource = findMappingById(id) + .orElseThrow(() -> new InconsistentScimDataException("Failed to delete resource %s, scim mapping not found: ".formatted(id))); + EntityOnRemoteScimId externalId = resource.getExternalIdAsEntityOnRemoteScimId(); + scimClient.delete(externalId); + getScimResourceDao().delete(resource); + } + + public void pushResourcesToScim(SynchronizationResult syncRes) throws ErrorForScimEndpointException, InconsistentScimDataException { + LOGGER.info("[SCIM] Push resources to endpoint " + this.getConfiguration().getEndPoint()); + try (Stream 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; + } + } + } + } + + + public void pullResourcesFromScim(SynchronizationResult syncRes) { + LOGGER.info("[SCIM] Import 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); + + // 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); + } + + } + } + + + protected abstract S scimRequestBodyForCreate(K roleMapperModel); + + protected abstract KeycloakId getId(K roleMapperModel); + + protected abstract boolean isMarkedToIgnore(K roleMapperModel); private void createMapping(KeycloakId keycloakId, EntityOnRemoteScimId externalId) { getScimResourceDao().create(keycloakId, externalId, type); @@ -64,7 +173,7 @@ public abstract class AbstractScimService findById(KeycloakId keycloakId) { + private Optional findMappingById(KeycloakId keycloakId) { return getScimResourceDao().findById(keycloakId, type); } @@ -72,143 +181,23 @@ public abstract class AbstractScimService entityOnRemoteScimId = findById(id) - .map(ScimResource::getExternalIdAsEntityOnRemoteScimId); - entityOnRemoteScimId - .ifPresentOrElse( - externalId -> doReplace(roleMapperModel, externalId), - () -> { - // TODO Exception Handling : should we throw a ScimPropagationException here ? - LOGGER.warnf("failed to replace resource %s, scim mapping not found", id); - } - ); - } catch (Exception e) { - throw new ScimPropagationException("[SCIM] Error while replacing SCIM resource", e); - } - } - private void doReplace(RMM roleMapperModel, EntityOnRemoteScimId externalId) { - S scimForReplace = toScimForReplace(roleMapperModel, externalId); - scimClient.replace(externalId, scimForReplace); - } + protected abstract boolean shouldIgnoreForScimSynchronization(K resource); - protected abstract S toScimForReplace(RMM roleMapperModel, EntityOnRemoteScimId externalId); + protected abstract Stream getResourceStream(); - public void delete(KeycloakId id) throws ScimPropagationException { - ScimResource resource = findById(id) - .orElseThrow(() -> new ScimPropagationException("Failed to delete resource %s, scim mapping not found: ".formatted(id))); - EntityOnRemoteScimId externalId = resource.getExternalIdAsEntityOnRemoteScimId(); - scimClient.delete(externalId); - getScimResourceDao().delete(resource); - } + protected abstract KeycloakId createEntity(S resource) throws UnexpectedScimDataException; - public void refreshResources(SynchronizationResult syncRes) throws ScimPropagationException { - LOGGER.info("[SCIM] Refresh resources for endpoint " + this.getConfiguration().getEndPoint()); - try (Stream resourcesStream = getResourceStream()) { - Set resources = resourcesStream.collect(Collectors.toUnmodifiableSet()); - for (RMM resource : resources) { - KeycloakId id = getId(resource); - LOGGER.infof("[SCIM] Reconciling local resource %s", id); - if (isSkipRefresh(resource)) { - LOGGER.infof("[SCIM] Skip local resource %s", id); - continue; - } - if (findById(id).isPresent()) { - LOGGER.info("[SCIM] Replacing it"); - replace(resource); - } else { - LOGGER.info("[SCIM] Creating it"); - create(resource); - } - syncRes.increaseUpdated(); - } - } - } - - protected abstract boolean isSkipRefresh(RMM resource); - - protected abstract Stream getResourceStream(); - - public void importResources(SynchronizationResult syncRes) throws ScimPropagationException { - LOGGER.info("[SCIM] Import resources for scim endpoint " + this.getConfiguration().getEndPoint()); - try { - for (S resource : scimClient.listResources()) { - try { - LOGGER.infof("[SCIM] Reconciling remote resource %s", resource); - EntityOnRemoteScimId externalId = resource.getId() - .map(EntityOnRemoteScimId::new) - .orElseThrow(() -> new ScimPropagationException("remote SCIM resource doesn't have an id")); - Optional optionalMapping = getScimResourceDao().findByExternalId(externalId, type); - if (optionalMapping.isPresent()) { - ScimResource mapping = optionalMapping.get(); - if (entityExists(mapping.getIdAsKeycloakId())) { - LOGGER.info("[SCIM] Valid mapping found, skipping"); - continue; - } else { - LOGGER.info("[SCIM] Delete a dangling mapping"); - getScimResourceDao().delete(mapping); - } - } - - Optional mapped = tryToMap(resource); - if (mapped.isPresent()) { - LOGGER.info("[SCIM] Matched"); - createMapping(mapped.get(), externalId); - } else { - switch (scimProviderConfiguration.getImportAction()) { - case CREATE_LOCAL: - LOGGER.info("[SCIM] Create local resource"); - try { - KeycloakId id = createEntity(resource); - createMapping(id, externalId); - syncRes.increaseAdded(); - } catch (Exception e) { - // TODO ExceptionHandling should we stop and throw ScimPropagationException here ? - LOGGER.error(e); - } - break; - case DELETE_REMOTE: - LOGGER.info("[SCIM] Delete remote resource"); - this.scimClient.delete(externalId); - syncRes.increaseRemoved(); - break; - case NOTHING: - LOGGER.info("[SCIM] Import action set to NOTHING"); - break; - } - } - } catch (Exception e) { - // TODO ExceptionHandling should we stop and throw ScimPropagationException here ? - LOGGER.error(e); - e.printStackTrace(); - syncRes.increaseFailed(); - } - } - } catch (ResponseException e) { - // TODO ExceptionHandling should we stop and throw ScimPropagationException here ? - LOGGER.error(e); - e.printStackTrace(); - syncRes.increaseFailed(); - } - } - - protected abstract KeycloakId createEntity(S resource) throws ScimPropagationException; - - protected abstract Optional tryToMap(S resource) throws ScimPropagationException; + protected abstract Optional matchKeycloakMappingByScimProperties(S resource) throws InconsistentScimDataException; protected abstract boolean entityExists(KeycloakId keycloakId); - public void sync(SynchronizationResult syncRes) throws ScimPropagationException { + public void sync(SynchronizationResult syncRes) throws InconsistentScimDataException, ErrorForScimEndpointException { if (this.scimProviderConfiguration.isSyncImport()) { - this.importResources(syncRes); + this.pullResourcesFromScim(syncRes); } if (this.scimProviderConfiguration.isSyncRefresh()) { - this.refreshResources(syncRes); + this.pushResourcesToScim(syncRes); } } diff --git a/src/main/java/sh/libre/scim/core/GroupScimService.java b/src/main/java/sh/libre/scim/core/GroupScimService.java index c448a9a00a..4679725256 100644 --- a/src/main/java/sh/libre/scim/core/GroupScimService.java +++ b/src/main/java/sh/libre/scim/core/GroupScimService.java @@ -10,7 +10,8 @@ import org.jboss.logging.Logger; import org.keycloak.models.GroupModel; import org.keycloak.models.KeycloakSession; import org.keycloak.models.UserModel; -import sh.libre.scim.jpa.ScimResource; +import sh.libre.scim.core.exceptions.UnexpectedScimDataException; +import sh.libre.scim.jpa.ScimResourceMapping; import java.net.URI; import java.util.List; @@ -37,7 +38,7 @@ public class GroupScimService extends AbstractScimService { } @Override - protected Optional tryToMap(Group resource) { + protected Optional matchKeycloakMappingByScimProperties(Group resource) { Set names = new TreeSet<>(); resource.getId().ifPresent(names::add); resource.getDisplayName().ifPresent(names::add); @@ -52,20 +53,20 @@ public class GroupScimService extends AbstractScimService { } @Override - protected KeycloakId createEntity(Group resource) throws ScimPropagationException { + protected KeycloakId createEntity(Group resource) throws UnexpectedScimDataException { String displayName = resource.getDisplayName() .filter(StringUtils::isNotBlank) - .orElseThrow(() -> new ScimPropagationException("can't create group without name: " + resource)); + .orElseThrow(() -> new UnexpectedScimDataException("Remote Scim group has empty name, can't create. Resource id = %s".formatted(resource.getId()))); GroupModel group = getKeycloakDao().createGroup(displayName); List groupMembers = resource.getMembers(); if (CollectionUtils.isNotEmpty(groupMembers)) { for (Member groupMember : groupMembers) { EntityOnRemoteScimId externalId = groupMember.getValue() .map(EntityOnRemoteScimId::new) - .orElseThrow(() -> new ScimPropagationException("can't create group member for group '%s' without id: ".formatted(displayName) + resource)); + .orElseThrow(() -> new UnexpectedScimDataException("can't create group member for group '%s' without id: ".formatted(displayName) + resource)); KeycloakId userId = getScimResourceDao().findUserByExternalId(externalId) - .map(ScimResource::getIdAsKeycloakId) - .orElseThrow(() -> new ScimPropagationException("can't find mapping for group member %s".formatted(externalId))); + .map(ScimResourceMapping::getIdAsKeycloakId) + .orElseThrow(() -> new UnexpectedScimDataException("can't find mapping for group member %s".formatted(externalId))); UserModel userModel = getKeycloakDao().getUserById(userId); userModel.joinGroup(group); } @@ -74,7 +75,7 @@ public class GroupScimService extends AbstractScimService { } @Override - protected boolean isSkip(GroupModel groupModel) { + protected boolean isMarkedToIgnore(GroupModel groupModel) { return BooleanUtils.TRUE.equals(groupModel.getFirstAttribute("scim-skip")); } @@ -84,16 +85,16 @@ public class GroupScimService extends AbstractScimService { } @Override - protected Group toScimForCreation(GroupModel groupModel) { + protected Group scimRequestBodyForCreate(GroupModel groupModel) { Set members = getKeycloakDao().getGroupMembers(groupModel); Group group = new Group(); group.setExternalId(groupModel.getId()); group.setDisplayName(groupModel.getName()); for (KeycloakId member : members) { Member groupMember = new Member(); - Optional optionalGroupMemberMapping = getScimResourceDao().findUserById(member); + Optional optionalGroupMemberMapping = getScimResourceDao().findUserById(member); if (optionalGroupMemberMapping.isPresent()) { - ScimResource groupMemberMapping = optionalGroupMemberMapping.get(); + ScimResourceMapping groupMemberMapping = optionalGroupMemberMapping.get(); EntityOnRemoteScimId externalIdAsEntityOnRemoteScimId = groupMemberMapping.getExternalIdAsEntityOnRemoteScimId(); groupMember.setValue(externalIdAsEntityOnRemoteScimId.asString()); URI ref = getUri(ScimResourceType.USER, externalIdAsEntityOnRemoteScimId); @@ -108,8 +109,8 @@ public class GroupScimService extends AbstractScimService { } @Override - protected Group toScimForReplace(GroupModel groupModel, EntityOnRemoteScimId externalId) { - Group group = toScimForCreation(groupModel); + protected Group scimRequestBodyForUpdate(GroupModel groupModel, EntityOnRemoteScimId externalId) { + Group group = scimRequestBodyForCreate(groupModel); group.setId(externalId.asString()); Meta meta = newMetaLocation(externalId); group.setMeta(meta); @@ -117,7 +118,7 @@ public class GroupScimService extends AbstractScimService { } @Override - protected boolean isSkipRefresh(GroupModel resource) { + protected boolean shouldIgnoreForScimSynchronization(GroupModel resource) { return false; } } diff --git a/src/main/java/sh/libre/scim/core/KeycloakId.java b/src/main/java/sh/libre/scim/core/KeycloakId.java index f35817dfbe..432892e6f1 100644 --- a/src/main/java/sh/libre/scim/core/KeycloakId.java +++ b/src/main/java/sh/libre/scim/core/KeycloakId.java @@ -1,5 +1,6 @@ package sh.libre.scim.core; +// TODO rename this public record KeycloakId( String asString ) { diff --git a/src/main/java/sh/libre/scim/core/ScimClient.java b/src/main/java/sh/libre/scim/core/ScimClient.java index caa318a60c..9bdf3cce88 100644 --- a/src/main/java/sh/libre/scim/core/ScimClient.java +++ b/src/main/java/sh/libre/scim/core/ScimClient.java @@ -12,6 +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 java.util.Collections; import java.util.HashMap; @@ -63,13 +64,14 @@ public class ScimClient implements AutoCloseable { return new ScimClient(scimRequestBuilder, scimResourceType); } - public EntityOnRemoteScimId create(KeycloakId id, S scimForCreation) throws ScimPropagationException { + public EntityOnRemoteScimId create(KeycloakId id, S scimForCreation) throws ErrorForScimEndpointException { Optional scimForCreationId = scimForCreation.getId(); if (scimForCreationId.isPresent()) { - throw new ScimPropagationException( - "%s is already created on remote with id %s".formatted(id, scimForCreationId.get()) + throw new IllegalArgumentException( + "User to create should never have an existing id: %s %s".formatted(id, scimForCreationId.get()) ); } + // TODO Exception handling : check that all exceptions are wrapped in server response Retry retry = retryRegistry.retry("create-%s".formatted(id.asString())); ServerResponse response = retry.executeSupplier(() -> scimRequestBuilder .create(getResourceClass(), getScimEndpoint()) @@ -80,15 +82,12 @@ public class ScimClient implements AutoCloseable { S resource = response.getResource(); return resource.getId() .map(EntityOnRemoteScimId::new) - .orElseThrow(() -> new IllegalStateException("created resource does not have id")); + .orElseThrow(() -> new ErrorForScimEndpointException("Created SCIM resource does not have id")); } - private void checkResponseIsSuccess(ServerResponse response) { + private void checkResponseIsSuccess(ServerResponse response) throws ErrorForScimEndpointException { if (!response.isSuccess()) { - // TODO Exception handling : we should throw a SCIM Progagation exception here - LOGGER.warn("[SCIM] Issue on SCIM Server response "); - LOGGER.warn(response.getResponseBody()); - LOGGER.warn(response.getHttpStatus()); + throw new ErrorForScimEndpointException("Server answered with status " + response.getResponseBody() + ": " + response.getResponseBody()); } } @@ -100,8 +99,9 @@ public class ScimClient implements AutoCloseable { return scimResourceType.getResourceClass(); } - public void replace(EntityOnRemoteScimId externalId, S scimForReplace) { + public void update(EntityOnRemoteScimId externalId, S scimForReplace) throws ErrorForScimEndpointException { 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 .update(getResourceClass(), getScimEndpoint(), externalId.asString()) .setResource(scimForReplace) @@ -110,8 +110,9 @@ public class ScimClient implements AutoCloseable { checkResponseIsSuccess(response); } - public void delete(EntityOnRemoteScimId externalId) { + public void delete(EntityOnRemoteScimId externalId) throws ErrorForScimEndpointException { 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 .delete(getResourceClass(), getScimEndpoint(), externalId.asString()) .sendRequest() diff --git a/src/main/java/sh/libre/scim/core/ScimDispatcher.java b/src/main/java/sh/libre/scim/core/ScimDispatcher.java index 2ec51aa924..927d0f9e02 100644 --- a/src/main/java/sh/libre/scim/core/ScimDispatcher.java +++ b/src/main/java/sh/libre/scim/core/ScimDispatcher.java @@ -3,6 +3,8 @@ package sh.libre.scim.core; import org.jboss.logging.Logger; 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.storage.ScimEndpointConfigurationStorageProviderFactory; import java.util.ArrayList; diff --git a/src/main/java/sh/libre/scim/core/UserScimService.java b/src/main/java/sh/libre/scim/core/UserScimService.java index fbae36236b..23b20795b2 100644 --- a/src/main/java/sh/libre/scim/core/UserScimService.java +++ b/src/main/java/sh/libre/scim/core/UserScimService.java @@ -13,6 +13,8 @@ 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.UnexpectedScimDataException; import java.util.ArrayList; import java.util.List; @@ -39,7 +41,7 @@ public class UserScimService extends AbstractScimService { } @Override - protected Optional tryToMap(User resource) { + protected Optional matchKeycloakMappingByScimProperties(User resource) throws InconsistentScimDataException { Optional matchedByUsername = resource.getUserName() .map(getKeycloakDao()::getUserByUsername) .map(this::getId); @@ -51,9 +53,7 @@ public class UserScimService extends AbstractScimService { if (matchedByUsername.isPresent() && matchedByEmail.isPresent() && !matchedByUsername.equals(matchedByEmail)) { - // TODO Exception Handling : what should we do here ? - LOGGER.warnf("found 2 possible users for remote user %s %s", matchedByUsername.get(), matchedByEmail.get()); - return Optional.empty(); + throw new InconsistentScimDataException("Found 2 possible users for remote user " + matchedByUsername.get() + " - " + matchedByEmail.get()); } if (matchedByUsername.isPresent()) { return matchedByUsername; @@ -62,21 +62,22 @@ public class UserScimService extends AbstractScimService { } @Override - protected KeycloakId createEntity(User resource) throws ScimPropagationException { + protected KeycloakId createEntity(User resource) throws UnexpectedScimDataException { String username = resource.getUserName() .filter(StringUtils::isNotBlank) - .orElseThrow(() -> new ScimPropagationException("can't create user with empty username, resource id = %s".formatted(resource.getId()))); + .orElseThrow(() -> new UnexpectedScimDataException("Remote Scim user has empty username, can't create. Resource id = %s".formatted(resource.getId()))); UserModel user = getKeycloakDao().addUser(username); resource.getEmails().stream() .findFirst() .flatMap(MultiComplexNode::getValue) .ifPresent(user::setEmail); - user.setEnabled(resource.isActive().orElseThrow(() -> new ScimPropagationException("can't create user with undefined 'active', resource id = %s".formatted(resource.getId())))); + boolean userEnabled = resource.isActive().orElse(false); + user.setEnabled(userEnabled); return new KeycloakId(user.getId()); } @Override - protected boolean isSkip(UserModel userModel) { + protected boolean isMarkedToIgnore(UserModel userModel) { return BooleanUtils.TRUE.equals(userModel.getFirstAttribute("scim-skip")); } @@ -86,7 +87,7 @@ public class UserScimService extends AbstractScimService { } @Override - protected User toScimForCreation(UserModel roleMapperModel) { + protected User scimRequestBodyForCreate(UserModel roleMapperModel) { String firstAndLastName = String.format("%s %s", StringUtils.defaultString(roleMapperModel.getFirstName()), StringUtils.defaultString(roleMapperModel.getLastName())).trim(); @@ -123,8 +124,8 @@ public class UserScimService extends AbstractScimService { } @Override - protected User toScimForReplace(UserModel userModel, EntityOnRemoteScimId externalId) { - User user = toScimForCreation(userModel); + protected User scimRequestBodyForUpdate(UserModel userModel, EntityOnRemoteScimId externalId) { + User user = scimRequestBodyForCreate(userModel); user.setId(externalId.asString()); Meta meta = newMetaLocation(externalId); user.setMeta(meta); @@ -132,7 +133,7 @@ public class UserScimService extends AbstractScimService { } @Override - protected boolean isSkipRefresh(UserModel userModel) { + protected boolean shouldIgnoreForScimSynchronization(UserModel userModel) { return "admin".equals(userModel.getUsername()); } } diff --git a/src/main/java/sh/libre/scim/core/exceptions/ErrorForScimEndpointException.java b/src/main/java/sh/libre/scim/core/exceptions/ErrorForScimEndpointException.java new file mode 100644 index 0000000000..1705e9981d --- /dev/null +++ b/src/main/java/sh/libre/scim/core/exceptions/ErrorForScimEndpointException.java @@ -0,0 +1,8 @@ +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 new file mode 100644 index 0000000000..947a9f1eb3 --- /dev/null +++ b/src/main/java/sh/libre/scim/core/exceptions/InconsistentScimDataException.java @@ -0,0 +1,7 @@ +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/ScimExceptionHandler.java b/src/main/java/sh/libre/scim/core/exceptions/ScimExceptionHandler.java similarity index 94% rename from src/main/java/sh/libre/scim/core/ScimExceptionHandler.java rename to src/main/java/sh/libre/scim/core/exceptions/ScimExceptionHandler.java index 47b8c718da..81afeeade0 100644 --- a/src/main/java/sh/libre/scim/core/ScimExceptionHandler.java +++ b/src/main/java/sh/libre/scim/core/exceptions/ScimExceptionHandler.java @@ -1,8 +1,9 @@ -package sh.libre.scim.core; +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; /** * In charge of dealing with SCIM exceptions by ignoring, logging or rollback transaction according to : diff --git a/src/main/java/sh/libre/scim/core/ScimPropagationException.java b/src/main/java/sh/libre/scim/core/exceptions/ScimPropagationException.java similarity index 64% rename from src/main/java/sh/libre/scim/core/ScimPropagationException.java rename to src/main/java/sh/libre/scim/core/exceptions/ScimPropagationException.java index 135b555961..7b1a747b21 100644 --- a/src/main/java/sh/libre/scim/core/ScimPropagationException.java +++ b/src/main/java/sh/libre/scim/core/exceptions/ScimPropagationException.java @@ -1,6 +1,6 @@ -package sh.libre.scim.core; +package sh.libre.scim.core.exceptions; -public class ScimPropagationException extends Exception { +public abstract class ScimPropagationException extends Exception { public ScimPropagationException(String message) { super(message); diff --git a/src/main/java/sh/libre/scim/core/exceptions/UnexpectedScimDataException.java b/src/main/java/sh/libre/scim/core/exceptions/UnexpectedScimDataException.java new file mode 100644 index 0000000000..918127ef0b --- /dev/null +++ b/src/main/java/sh/libre/scim/core/exceptions/UnexpectedScimDataException.java @@ -0,0 +1,7 @@ +package sh.libre.scim.core.exceptions; + +public class UnexpectedScimDataException extends ScimPropagationException { + public UnexpectedScimDataException(String message) { + super(message); + } +} diff --git a/src/main/java/sh/libre/scim/event/ScimEventListenerProvider.java b/src/main/java/sh/libre/scim/event/ScimEventListenerProvider.java index 5465222940..4d6f7f470f 100644 --- a/src/main/java/sh/libre/scim/event/ScimEventListenerProvider.java +++ b/src/main/java/sh/libre/scim/event/ScimEventListenerProvider.java @@ -33,7 +33,7 @@ public class ScimEventListenerProvider implements EventListenerProvider { private final KeycloakSession session; - private final KeycloakDao keycloackDao; + private final KeycloakDao keycloakDao; private final Map patterns = Map.of( ResourceType.USER, Pattern.compile("users/(.+)"), @@ -45,7 +45,7 @@ public class ScimEventListenerProvider implements EventListenerProvider { public ScimEventListenerProvider(KeycloakSession session) { this.session = session; - this.keycloackDao = new KeycloakDao(session); + this.keycloakDao = new KeycloakDao(session); this.dispatcher = new ScimDispatcher(session); } @@ -63,7 +63,7 @@ public class ScimEventListenerProvider implements EventListenerProvider { case UPDATE_EMAIL, UPDATE_PROFILE -> { LOGGER.infof("[SCIM] Propagate User %s - %s", eventType, eventUserId); UserModel user = getUser(eventUserId); - dispatcher.dispatchUserModificationToAll(client -> client.replace(user)); + dispatcher.dispatchUserModificationToAll(client -> client.update(user)); } case DELETE_ACCOUNT -> { LOGGER.infof("[SCIM] Propagate User deletion - %s", eventUserId); @@ -131,12 +131,12 @@ public class ScimEventListenerProvider implements EventListenerProvider { UserModel user = getUser(userId); dispatcher.dispatchUserModificationToAll(client -> client.create(user)); user.getGroupsStream().forEach(group -> - dispatcher.dispatchGroupModificationToAll(client -> client.replace(group) + dispatcher.dispatchGroupModificationToAll(client -> client.update(group) )); } case UPDATE -> { UserModel user = getUser(userId); - dispatcher.dispatchUserModificationToAll(client -> client.replace(user)); + dispatcher.dispatchUserModificationToAll(client -> client.update(user)); } case DELETE -> dispatcher.dispatchUserModificationToAll(client -> client.delete(userId)); default -> { @@ -160,7 +160,7 @@ public class ScimEventListenerProvider implements EventListenerProvider { } case UPDATE -> { GroupModel group = getGroup(groupId); - dispatcher.dispatchGroupModificationToAll(client -> client.replace(group)); + dispatcher.dispatchGroupModificationToAll(client -> client.update(group)); } case DELETE -> dispatcher.dispatchGroupModificationToAll(client -> client.delete(groupId)); default -> { @@ -174,7 +174,7 @@ public class ScimEventListenerProvider implements EventListenerProvider { GroupModel group = getGroup(groupId); group.setSingleAttribute("scim-dirty", BooleanUtils.TRUE); UserModel user = getUser(userId); - dispatcher.dispatchUserModificationToAll(client -> client.replace(user)); + dispatcher.dispatchUserModificationToAll(client -> client.update(user)); } private void handleRoleMappingEvent(AdminEvent roleMappingEvent, ScimResourceType type, KeycloakId id) { @@ -182,14 +182,14 @@ public class ScimEventListenerProvider implements EventListenerProvider { switch (type) { case USER -> { UserModel user = getUser(id); - dispatcher.dispatchUserModificationToAll(client -> client.replace(user)); + dispatcher.dispatchUserModificationToAll(client -> client.update(user)); } case GROUP -> { GroupModel group = getGroup(id); session.users() .getGroupMembersStream(session.getContext().getRealm(), group) .forEach(user -> - dispatcher.dispatchUserModificationToAll(client -> client.replace(user) + dispatcher.dispatchUserModificationToAll(client -> client.update(user) )); } default -> { @@ -204,7 +204,7 @@ public class ScimEventListenerProvider implements EventListenerProvider { // In case of a component deletion if (event.getOperationType() == OperationType.DELETE) { // Check if it was a Scim endpoint configuration, and forward deletion if so - // TODO : determine if deleted element is of ScimStorageProvider class and only delete in that case + // TODO : determine if deleted element is of ScimStorageProvider class and only refresh in that case dispatcher.refreshActiveScimEndpoints(); } else { // In case of CREATE or UPDATE, we can directly use the string representation @@ -219,11 +219,11 @@ public class ScimEventListenerProvider implements EventListenerProvider { private UserModel getUser(KeycloakId id) { - return keycloackDao.getUserById(id); + return keycloakDao.getUserById(id); } private GroupModel getGroup(KeycloakId id) { - return keycloackDao.getGroupById(id); + return keycloakDao.getGroupById(id); } @Override diff --git a/src/main/java/sh/libre/scim/jpa/ScimResourceDao.java b/src/main/java/sh/libre/scim/jpa/ScimResourceDao.java index 7d96b2819d..97db33b9ea 100644 --- a/src/main/java/sh/libre/scim/jpa/ScimResourceDao.java +++ b/src/main/java/sh/libre/scim/jpa/ScimResourceDao.java @@ -44,7 +44,7 @@ public class ScimResourceDao { } public void create(KeycloakId id, EntityOnRemoteScimId externalId, ScimResourceType type) { - ScimResource entity = new ScimResource(); + ScimResourceMapping entity = new ScimResourceMapping(); entity.setType(type.name()); entity.setExternalId(externalId.asString()); entity.setComponentId(componentId); @@ -53,16 +53,16 @@ public class ScimResourceDao { entityManager.persist(entity); } - private TypedQuery getScimResourceTypedQuery(String queryName, String id, ScimResourceType type) { + private TypedQuery getScimResourceTypedQuery(String queryName, String id, ScimResourceType type) { return getEntityManager() - .createNamedQuery(queryName, ScimResource.class) + .createNamedQuery(queryName, ScimResourceMapping.class) .setParameter("type", type.name()) .setParameter("realmId", getRealmId()) .setParameter("componentId", getComponentId()) .setParameter("id", id); } - public Optional findByExternalId(EntityOnRemoteScimId externalId, ScimResourceType type) { + public Optional findByExternalId(EntityOnRemoteScimId externalId, ScimResourceType type) { try { return Optional.of( getScimResourceTypedQuery("findByExternalId", externalId.asString(), type).getSingleResult() @@ -72,7 +72,7 @@ public class ScimResourceDao { } } - public Optional findById(KeycloakId keycloakId, ScimResourceType type) { + public Optional findById(KeycloakId keycloakId, ScimResourceType type) { try { return Optional.of( getScimResourceTypedQuery("findById", keycloakId.asString(), type).getSingleResult() @@ -82,15 +82,15 @@ public class ScimResourceDao { } } - public Optional findUserById(KeycloakId id) { + public Optional findUserById(KeycloakId id) { return findById(id, ScimResourceType.USER); } - public Optional findUserByExternalId(EntityOnRemoteScimId externalId) { + public Optional findUserByExternalId(EntityOnRemoteScimId externalId) { return findByExternalId(externalId, ScimResourceType.USER); } - public void delete(ScimResource resource) { + public void delete(ScimResourceMapping resource) { EntityManager entityManager = getEntityManager(); entityManager.remove(resource); } diff --git a/src/main/java/sh/libre/scim/jpa/ScimResource.java b/src/main/java/sh/libre/scim/jpa/ScimResourceMapping.java similarity index 96% rename from src/main/java/sh/libre/scim/jpa/ScimResource.java rename to src/main/java/sh/libre/scim/jpa/ScimResourceMapping.java index 5536a6dd96..26eafa04c2 100644 --- a/src/main/java/sh/libre/scim/jpa/ScimResource.java +++ b/src/main/java/sh/libre/scim/jpa/ScimResourceMapping.java @@ -12,12 +12,12 @@ import sh.libre.scim.core.KeycloakId; @Entity @IdClass(ScimResourceId.class) -@Table(name = "SCIM_RESOURCE") +@Table(name = "SCIM_RESOURCE_MAPPING") @NamedQueries({ @NamedQuery(name = "findById", query = "from ScimResource where realmId = :realmId and componentId = :componentId and type = :type and id = :id"), @NamedQuery(name = "findByExternalId", query = "from ScimResource where realmId = :realmId and componentId = :componentId and type = :type and externalId = :id") }) -public class ScimResource { +public class ScimResourceMapping { @Id @Column(name = "ID", nullable = false) diff --git a/src/main/java/sh/libre/scim/jpa/ScimResourceProvider.java b/src/main/java/sh/libre/scim/jpa/ScimResourceProvider.java index ab12d4cc43..3dc284e22f 100644 --- a/src/main/java/sh/libre/scim/jpa/ScimResourceProvider.java +++ b/src/main/java/sh/libre/scim/jpa/ScimResourceProvider.java @@ -9,7 +9,7 @@ public class ScimResourceProvider implements JpaEntityProvider { @Override public List> getEntities() { - return Collections.singletonList(ScimResource.class); + return Collections.singletonList(ScimResourceMapping.class); } @Override diff --git a/src/main/java/sh/libre/scim/storage/ScimEndpointConfigurationStorageProviderFactory.java b/src/main/java/sh/libre/scim/storage/ScimEndpointConfigurationStorageProviderFactory.java index 724fa925b7..8bc99d6211 100644 --- a/src/main/java/sh/libre/scim/storage/ScimEndpointConfigurationStorageProviderFactory.java +++ b/src/main/java/sh/libre/scim/storage/ScimEndpointConfigurationStorageProviderFactory.java @@ -79,7 +79,7 @@ public class ScimEndpointConfigurationStorageProviderFactory 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.replace(group)); + dispatcher.dispatchGroupModificationToAll(client -> client.update(group)); group.removeAttribute("scim-dirty"); } dispatcher.close();