From 399dfbd17ed6c591a27f2c94313b06a2a04afd8f Mon Sep 17 00:00:00 2001 From: Alex Morel Date: Tue, 25 Jun 2024 09:36:20 +0200 Subject: [PATCH] Exception Handler - Step 2: logging uniformisation and add //TODOs --- .../libre/scim/core/AbstractScimService.java | 37 +++++----- .../sh/libre/scim/core/GroupScimService.java | 6 +- .../java/sh/libre/scim/core/ScimClient.java | 1 + .../sh/libre/scim/core/ScimDispatcher.java | 72 ++++++++----------- .../libre/scim/core/ScimExceptionHandler.java | 14 +++- .../sh/libre/scim/core/UserScimService.java | 13 ++-- .../scim/event/ScimEventListenerProvider.java | 6 +- ...ntConfigurationStorageProviderFactory.java | 6 +- 8 files changed, 77 insertions(+), 78 deletions(-) diff --git a/src/main/java/sh/libre/scim/core/AbstractScimService.java b/src/main/java/sh/libre/scim/core/AbstractScimService.java index 637283c043..b72bef678d 100644 --- a/src/main/java/sh/libre/scim/core/AbstractScimService.java +++ b/src/main/java/sh/libre/scim/core/AbstractScimService.java @@ -82,10 +82,12 @@ public abstract class AbstractScimService doReplace(roleMapperModel, externalId), - () -> LOGGER.warnf("failed to replace resource %s, scim mapping not found", id) + () -> { + // TODO Exception Handling : should we throw a ScimPropagationException here ? + LOGGER.warnf("failed to replace resource %s, scim mapping not found", id); + } ); } catch (Exception e) { - LOGGER.error(e); throw new ScimPropagationException("[SCIM] Error while replacing SCIM resource", e); } } @@ -106,21 +108,21 @@ public abstract class AbstractScimService resourcesStream = getResourceStream()) { Set resources = resourcesStream.collect(Collectors.toUnmodifiableSet()); for (RMM resource : resources) { KeycloakId id = getId(resource); - LOGGER.infof("Reconciling local resource %s", id); + LOGGER.infof("[SCIM] Reconciling local resource %s", id); if (isSkipRefresh(resource)) { - LOGGER.infof("Skip local resource %s", id); + LOGGER.infof("[SCIM] Skip local resource %s", id); continue; } if (findById(id).isPresent()) { - LOGGER.info("Replacing it"); + LOGGER.info("[SCIM] Replacing it"); replace(resource); } else { - LOGGER.info("Creating it"); + LOGGER.info("[SCIM] Creating it"); create(resource); } syncRes.increaseUpdated(); @@ -133,11 +135,11 @@ public abstract class AbstractScimService getResourceStream(); public void importResources(SynchronizationResult syncRes) throws ScimPropagationException { - LOGGER.info("Import"); + LOGGER.info("[SCIM] Import resources for scim endpoint " + this.getConfiguration().getEndPoint()); try { for (S resource : scimClient.listResources()) { try { - LOGGER.infof("Reconciling remote resource %s", resource); + 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")); @@ -145,49 +147,50 @@ public abstract class AbstractScimService mapped = tryToMap(resource); if (mapped.isPresent()) { - LOGGER.info("Matched"); + LOGGER.info("[SCIM] Matched"); createMapping(mapped.get(), externalId); } else { switch (scimProviderConfiguration.getImportAction()) { case CREATE_LOCAL: - LOGGER.info("Create local resource"); + 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("Delete remote resource"); + LOGGER.info("[SCIM] Delete remote resource"); this.scimClient.delete(externalId); syncRes.increaseRemoved(); break; case NOTHING: - LOGGER.info("Import action set to NOTHING"); + LOGGER.info("[SCIM] Import action set to NOTHING"); break; } } } catch (Exception e) { - // TODO should we stop and throw ScimPropagationException here ? + // TODO ExceptionHandling should we stop and throw ScimPropagationException here ? LOGGER.error(e); e.printStackTrace(); syncRes.increaseFailed(); } } } catch (ResponseException e) { - // TODO should we stop and throw ScimPropagationException here ? + // TODO ExceptionHandling should we stop and throw ScimPropagationException here ? LOGGER.error(e); e.printStackTrace(); syncRes.increaseFailed(); diff --git a/src/main/java/sh/libre/scim/core/GroupScimService.java b/src/main/java/sh/libre/scim/core/GroupScimService.java index e5b2315552..c448a9a00a 100644 --- a/src/main/java/sh/libre/scim/core/GroupScimService.java +++ b/src/main/java/sh/libre/scim/core/GroupScimService.java @@ -20,7 +20,7 @@ import java.util.TreeSet; import java.util.stream.Stream; public class GroupScimService extends AbstractScimService { - private final Logger logger = Logger.getLogger(GroupScimService.class); + private final Logger LOGGER = Logger.getLogger(GroupScimService.class); public GroupScimService(KeycloakSession keycloakSession, ScrimProviderConfiguration scimProviderConfiguration) { super(keycloakSession, scimProviderConfiguration, ScimResourceType.GROUP); @@ -95,13 +95,13 @@ public class GroupScimService extends AbstractScimService { if (optionalGroupMemberMapping.isPresent()) { ScimResource groupMemberMapping = optionalGroupMemberMapping.get(); EntityOnRemoteScimId externalIdAsEntityOnRemoteScimId = groupMemberMapping.getExternalIdAsEntityOnRemoteScimId(); - logger.debugf("found mapping for group member %s as %s", member, externalIdAsEntityOnRemoteScimId); groupMember.setValue(externalIdAsEntityOnRemoteScimId.asString()); URI ref = getUri(ScimResourceType.USER, externalIdAsEntityOnRemoteScimId); groupMember.setRef(ref.toString()); group.addMember(groupMember); } else { - logger.warnf("member %s not found for group %s", member, groupModel.getId()); + // 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()); } } return group; diff --git a/src/main/java/sh/libre/scim/core/ScimClient.java b/src/main/java/sh/libre/scim/core/ScimClient.java index ea89c60415..caa318a60c 100644 --- a/src/main/java/sh/libre/scim/core/ScimClient.java +++ b/src/main/java/sh/libre/scim/core/ScimClient.java @@ -85,6 +85,7 @@ public class ScimClient implements AutoCloseable { private void checkResponseIsSuccess(ServerResponse response) { 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()); diff --git a/src/main/java/sh/libre/scim/core/ScimDispatcher.java b/src/main/java/sh/libre/scim/core/ScimDispatcher.java index 54c4843998..2ec51aa924 100644 --- a/src/main/java/sh/libre/scim/core/ScimDispatcher.java +++ b/src/main/java/sh/libre/scim/core/ScimDispatcher.java @@ -16,7 +16,7 @@ import java.util.Set; */ public class ScimDispatcher { - private static final Logger logger = Logger.getLogger(ScimDispatcher.class); + private static final Logger LOGGER = Logger.getLogger(ScimDispatcher.class); private final KeycloakSession session; private final ScimExceptionHandler exceptionHandler; @@ -34,42 +34,32 @@ public class ScimDispatcher { * Lists all active ScimStorageProviderFactory and create new ScimClients for each of them */ public void refreshActiveScimEndpoints() { - try { - // Step 1: close existing clients - for (GroupScimService c : groupScimServices) { - c.close(); - } - groupScimServices.clear(); - for (UserScimService c : userScimServices) { - c.close(); - } - userScimServices.clear(); + // Step 1: close existing clients (as configuration may have changed) + groupScimServices.forEach(GroupScimService::close); + groupScimServices.clear(); + userScimServices.forEach(UserScimService::close); + userScimServices.clear(); - // Step 2: Get All SCIM endpoints defined in Admin Console (enabled ScimStorageProviderFactory) - session.getContext().getRealm().getComponentsStream() - .filter(m -> ScimEndpointConfigurationStorageProviderFactory.ID.equals(m.getProviderId()) - && m.get("enabled", true)) - .forEach(scimEndpointConfigurationRaw -> { - ScrimProviderConfiguration scrimProviderConfiguration = new ScrimProviderConfiguration(scimEndpointConfigurationRaw); - try { - // Step 3 : create scim clients for each endpoint - if (scimEndpointConfigurationRaw.get(ScrimProviderConfiguration.CONF_KEY_PROPAGATION_GROUP, false)) { - GroupScimService groupScimService = new GroupScimService(session, scrimProviderConfiguration); - groupScimServices.add(groupScimService); - } - if (scimEndpointConfigurationRaw.get(ScrimProviderConfiguration.CONF_KEY_PROPAGATION_USER, false)) { - UserScimService userScimService = new UserScimService(session, scrimProviderConfiguration); - userScimServices.add(userScimService); - } - } catch (Exception e) { - logger.warnf("[SCIM] Invalid Endpoint configuration %s: %s", scimEndpointConfigurationRaw.getId(), e.getMessage()); - // TODO is it ok to log and try to create the other clients ? + // Step 2: Get All SCIM endpoints defined in Admin Console (enabled ScimStorageProviderFactory) + session.getContext().getRealm().getComponentsStream() + .filter(m -> ScimEndpointConfigurationStorageProviderFactory.ID.equals(m.getProviderId()) + && m.get("enabled", true)) + .forEach(scimEndpointConfigurationRaw -> { + ScrimProviderConfiguration scrimProviderConfiguration = new ScrimProviderConfiguration(scimEndpointConfigurationRaw); + try { + // Step 3 : create scim clients for each endpoint + if (scimEndpointConfigurationRaw.get(ScrimProviderConfiguration.CONF_KEY_PROPAGATION_GROUP, false)) { + GroupScimService groupScimService = new GroupScimService(session, scrimProviderConfiguration); + groupScimServices.add(groupScimService); } - }); - } catch (Exception e) { - logger.error("[SCIM] Error while refreshing scim clients ", e); - // TODO : how to handle exception here ? - } + if (scimEndpointConfigurationRaw.get(ScrimProviderConfiguration.CONF_KEY_PROPAGATION_USER, false)) { + UserScimService userScimService = new UserScimService(session, scrimProviderConfiguration); + userScimServices.add(userScimService); + } + } catch (Exception e) { + exceptionHandler.handleInvalidEndpointConfiguration(scimEndpointConfigurationRaw, e); + } + }); } public void dispatchUserModificationToAll(SCIMPropagationConsumer operationToDispatch) { @@ -84,7 +74,7 @@ public class ScimDispatcher { } }); // TODO we could iterate on servicesCorrectlyPropagated to undo modification - logger.infof("[SCIM] User operation dispatched to %d SCIM server", servicesCorrectlyPropagated.size()); + LOGGER.infof("[SCIM] User operation dispatched to %d SCIM server", servicesCorrectlyPropagated.size()); } public void dispatchGroupModificationToAll(SCIMPropagationConsumer operationToDispatch) { @@ -99,7 +89,7 @@ public class ScimDispatcher { } }); // TODO we could iterate on servicesCorrectlyPropagated to undo modification - logger.infof("[SCIM] Group operation dispatched to %d SCIM server", servicesCorrectlyPropagated.size()); + LOGGER.infof("[SCIM] Group operation dispatched to %d SCIM server", servicesCorrectlyPropagated.size()); } public void dispatchUserModificationToOne(ComponentModel scimServerConfiguration, SCIMPropagationConsumer operationToDispatch) { @@ -109,12 +99,12 @@ public class ScimDispatcher { if (matchingClient.isPresent()) { try { operationToDispatch.acceptThrows(matchingClient.get()); - logger.infof("[SCIM] User operation dispatched to SCIM server %s", matchingClient.get().getConfiguration().getId()); + LOGGER.infof("[SCIM] User operation dispatched to SCIM server %s", matchingClient.get().getConfiguration().getId()); } catch (ScimPropagationException e) { exceptionHandler.handleException(matchingClient.get().getConfiguration(), e); } } else { - logger.error("[SCIM] Could not find a Scim Client matching User endpoint configuration" + scimServerConfiguration.getId()); + LOGGER.error("[SCIM] Could not find a Scim Client matching User endpoint configuration" + scimServerConfiguration.getId()); } } @@ -126,12 +116,12 @@ public class ScimDispatcher { if (matchingClient.isPresent()) { try { operationToDispatch.acceptThrows(matchingClient.get()); - logger.infof("[SCIM] Group operation dispatched to SCIM server %s", matchingClient.get().getConfiguration().getId()); + LOGGER.infof("[SCIM] Group operation dispatched to SCIM server %s", matchingClient.get().getConfiguration().getId()); } catch (ScimPropagationException e) { exceptionHandler.handleException(matchingClient.get().getConfiguration(), e); } } else { - logger.error("[SCIM] Could not find a Scim Client matching Group endpoint configuration" + scimServerConfiguration.getId()); + LOGGER.error("[SCIM] Could not find a Scim Client matching Group endpoint configuration" + scimServerConfiguration.getId()); } } diff --git a/src/main/java/sh/libre/scim/core/ScimExceptionHandler.java b/src/main/java/sh/libre/scim/core/ScimExceptionHandler.java index 44d65ff0e1..47b8c718da 100644 --- a/src/main/java/sh/libre/scim/core/ScimExceptionHandler.java +++ b/src/main/java/sh/libre/scim/core/ScimExceptionHandler.java @@ -1,6 +1,7 @@ package sh.libre.scim.core; import org.jboss.logging.Logger; +import org.keycloak.component.ComponentModel; import org.keycloak.models.KeycloakSession; /** @@ -10,8 +11,8 @@ import org.keycloak.models.KeycloakSession; * - The thrown exception itself */ public class ScimExceptionHandler { + private static final Logger LOGGER = Logger.getLogger(ScimExceptionHandler.class); - private static final Logger logger = Logger.getLogger(ScimDispatcher.class); private final KeycloakSession session; public ScimExceptionHandler(KeycloakSession session) { @@ -25,7 +26,14 @@ public class ScimExceptionHandler { * @param e the occuring exception */ public void handleException(ScrimProviderConfiguration scimProviderConfiguration, ScimPropagationException e) { - logger.error("[SCIM] Error while propagating to SCIM endpoint " + scimProviderConfiguration.getId(), e); - // TODO session.getTransactionManager().rollback(); + 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 } } diff --git a/src/main/java/sh/libre/scim/core/UserScimService.java b/src/main/java/sh/libre/scim/core/UserScimService.java index 3c30023c64..fbae36236b 100644 --- a/src/main/java/sh/libre/scim/core/UserScimService.java +++ b/src/main/java/sh/libre/scim/core/UserScimService.java @@ -20,7 +20,7 @@ import java.util.Optional; import java.util.stream.Stream; public class UserScimService extends AbstractScimService { - private final Logger logger = Logger.getLogger(UserScimService.class); + private final Logger LOGGER = Logger.getLogger(UserScimService.class); public UserScimService( KeycloakSession keycloakSession, @@ -41,17 +41,18 @@ public class UserScimService extends AbstractScimService { @Override protected Optional tryToMap(User resource) { Optional matchedByUsername = resource.getUserName() - .map(getKeycloakDao()::getUserByUsername) - .map(this::getId); + .map(getKeycloakDao()::getUserByUsername) + .map(this::getId); Optional matchedByEmail = resource.getEmails().stream() .findFirst() .flatMap(MultiComplexNode::getValue) .map(getKeycloakDao()::getUserByEmail) .map(this::getId); if (matchedByUsername.isPresent() - && matchedByEmail.isPresent() - && !matchedByUsername.equals(matchedByEmail)) { - logger.warnf("found 2 possible users for remote user %s %s", matchedByUsername.get(), matchedByEmail.get()); + && 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(); } if (matchedByUsername.isPresent()) { diff --git a/src/main/java/sh/libre/scim/event/ScimEventListenerProvider.java b/src/main/java/sh/libre/scim/event/ScimEventListenerProvider.java index 4195e6b618..5465222940 100644 --- a/src/main/java/sh/libre/scim/event/ScimEventListenerProvider.java +++ b/src/main/java/sh/libre/scim/event/ScimEventListenerProvider.java @@ -228,11 +228,7 @@ public class ScimEventListenerProvider implements EventListenerProvider { @Override public void close() { - try { - dispatcher.close(); - } catch (Exception e) { - LOGGER.error("Error while closing dispatcher", e); - } + dispatcher.close(); } diff --git a/src/main/java/sh/libre/scim/storage/ScimEndpointConfigurationStorageProviderFactory.java b/src/main/java/sh/libre/scim/storage/ScimEndpointConfigurationStorageProviderFactory.java index 00a527fe3e..724fa925b7 100644 --- a/src/main/java/sh/libre/scim/storage/ScimEndpointConfigurationStorageProviderFactory.java +++ b/src/main/java/sh/libre/scim/storage/ScimEndpointConfigurationStorageProviderFactory.java @@ -31,7 +31,7 @@ import java.util.List; public class ScimEndpointConfigurationStorageProviderFactory implements UserStorageProviderFactory, ImportSynchronization { public static final String ID = "scim"; - private final Logger logger = Logger.getLogger(ScimEndpointConfigurationStorageProviderFactory.class); + private final Logger LOGGER = Logger.getLogger(ScimEndpointConfigurationStorageProviderFactory.class); @Override public String getId() { @@ -43,7 +43,7 @@ public class ScimEndpointConfigurationStorageProviderFactory public SynchronizationResult sync(KeycloakSessionFactory sessionFactory, String realmId, UserStorageProviderModel model) { // TODO if this should be kept here, better document purpose & usage - logger.infof("[SCIM] Sync from ScimStorageProvider - Realm %s - Model %s", realmId, model.getId()); + LOGGER.infof("[SCIM] Sync from ScimStorageProvider - Realm %s - Model %s", realmId, model.getId()); SynchronizationResult result = new SynchronizationResult(); KeycloakModelUtils.runJobInTransaction(sessionFactory, session -> { RealmModel realm = session.realms().getRealm(realmId); @@ -78,7 +78,7 @@ public class ScimEndpointConfigurationStorageProviderFactory ScimDispatcher dispatcher = new ScimDispatcher(session); for (GroupModel group : session.groups().getGroupsStream(realm) .filter(x -> BooleanUtils.TRUE.equals(x.getFirstAttribute("scim-dirty"))).toList()) { - logger.infof("[SCIM] Dirty group: %s", group.getName()); + LOGGER.infof("[SCIM] Dirty group: %s", group.getName()); dispatcher.dispatchGroupModificationToAll(client -> client.replace(group)); group.removeAttribute("scim-dirty"); }