From d8cba394b2de0c679aac070261a90bef788b1b07 Mon Sep 17 00:00:00 2001 From: Alex Morel Date: Thu, 18 Jul 2024 15:44:15 +0200 Subject: [PATCH] Handle retry exceptions as a 500 server response --- .../sh/libre/scim/core/ScimDispatcher.java | 4 +- .../scim/core/ScrimEndPointConfiguration.java | 6 ++ ...alidResponseFromScimEndpointException.java | 19 +++-- .../core/exceptions/RollbackApproach.java | 14 ++-- .../core/exceptions/ScimExceptionHandler.java | 4 +- .../exceptions/ScimPropagationException.java | 4 +- .../scim/core/service/GroupScimService.java | 2 +- .../libre/scim/core/service/ScimClient.java | 72 +++++++++++-------- .../scim/core/service/UserScimService.java | 11 +-- .../ScimBackgroundGroupMembershipUpdater.java | 2 +- .../sh/libre/scim/jpa/ScimResourceDao.java | 1 - .../libre/scim/jpa/ScimResourceMapping.java | 4 +- .../libre/scim/jpa/ScimResourceProvider.java | 1 + .../META-INF/scim-resource-changelog.xml | 10 +-- 14 files changed, 95 insertions(+), 59 deletions(-) diff --git a/src/main/java/sh/libre/scim/core/ScimDispatcher.java b/src/main/java/sh/libre/scim/core/ScimDispatcher.java index 4146c30119..d3d675108b 100644 --- a/src/main/java/sh/libre/scim/core/ScimDispatcher.java +++ b/src/main/java/sh/libre/scim/core/ScimDispatcher.java @@ -113,7 +113,7 @@ 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().getName()); } catch (ScimPropagationException e) { exceptionHandler.handleException(matchingClient.get().getConfiguration(), e); } @@ -130,7 +130,7 @@ 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().getName()); } catch (ScimPropagationException e) { exceptionHandler.handleException(matchingClient.get().getConfiguration(), e); } diff --git a/src/main/java/sh/libre/scim/core/ScrimEndPointConfiguration.java b/src/main/java/sh/libre/scim/core/ScrimEndPointConfiguration.java index 0df9cedacc..23755fdfa0 100644 --- a/src/main/java/sh/libre/scim/core/ScrimEndPointConfiguration.java +++ b/src/main/java/sh/libre/scim/core/ScrimEndPointConfiguration.java @@ -18,6 +18,7 @@ public class ScrimEndPointConfiguration { private final String endPoint; private final String id; + private final String name; private final String contentType; private final String authorizationHeaderValue; private final ImportAction importAction; @@ -42,6 +43,7 @@ public class ScrimEndPointConfiguration { contentType = scimProviderConfiguration.get(CONF_KEY_CONTENT_TYPE, ""); endPoint = scimProviderConfiguration.get(CONF_KEY_ENDPOINT, ""); id = scimProviderConfiguration.getId(); + name = scimProviderConfiguration.getName(); importAction = ImportAction.valueOf(scimProviderConfiguration.get(CONF_KEY_SYNC_IMPORT_ACTION)); pullFromScimSynchronisationActivated = scimProviderConfiguration.get(CONF_KEY_SYNC_IMPORT, false); pushToScimSynchronisationActivated = scimProviderConfiguration.get(CONF_KEY_SYNC_REFRESH, false); @@ -70,6 +72,10 @@ public class ScrimEndPointConfiguration { return id; } + public String getName() { + return name; + } + public ImportAction getImportAction() { return importAction; } diff --git a/src/main/java/sh/libre/scim/core/exceptions/InvalidResponseFromScimEndpointException.java b/src/main/java/sh/libre/scim/core/exceptions/InvalidResponseFromScimEndpointException.java index 27c5c97b81..079443622b 100644 --- a/src/main/java/sh/libre/scim/core/exceptions/InvalidResponseFromScimEndpointException.java +++ b/src/main/java/sh/libre/scim/core/exceptions/InvalidResponseFromScimEndpointException.java @@ -2,16 +2,27 @@ package sh.libre.scim.core.exceptions; import de.captaingoldfish.scim.sdk.client.response.ServerResponse; +import java.util.Optional; + public class InvalidResponseFromScimEndpointException extends ScimPropagationException { - - private final ServerResponse response; + + private final transient Optional response; public InvalidResponseFromScimEndpointException(ServerResponse response, String message) { super(message); - this.response = response; + this.response = Optional.of(response); } - public ServerResponse getResponse() { + public InvalidResponseFromScimEndpointException(String message, Exception e) { + super(message, e); + this.response = Optional.empty(); + } + + + /** + * Empty response can occur if a major exception was thrown while retrying the request. + */ + public Optional getResponse() { return response; } diff --git a/src/main/java/sh/libre/scim/core/exceptions/RollbackApproach.java b/src/main/java/sh/libre/scim/core/exceptions/RollbackApproach.java index aeca5e930c..d1fb108930 100644 --- a/src/main/java/sh/libre/scim/core/exceptions/RollbackApproach.java +++ b/src/main/java/sh/libre/scim/core/exceptions/RollbackApproach.java @@ -40,10 +40,16 @@ public enum RollbackApproach implements RollbackStrategy { } private boolean shouldRollbackBecauseOfResponse(InvalidResponseFromScimEndpointException e) { - // We consider that 404 are acceptable, otherwise rollback - int httpStatus = e.getResponse().getHttpStatus(); - ArrayList acceptableStatus = Lists.newArrayList(200, 204, 404); - return !acceptableStatus.contains(httpStatus); + // If we have a response + return e.getResponse().map(r -> { + // We consider that 404 are acceptable, otherwise rollback + ArrayList acceptableStatus = Lists.newArrayList(200, 204, 404); + return !acceptableStatus.contains(r.getHttpStatus()); + }).orElse( + // Never got an answer, server was either misconfigured or unreachable + // No rollback in that case. + false + ); } } } 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 b525ae1279..993f287ff5 100644 --- a/src/main/java/sh/libre/scim/core/exceptions/ScimExceptionHandler.java +++ b/src/main/java/sh/libre/scim/core/exceptions/ScimExceptionHandler.java @@ -32,10 +32,10 @@ public class ScimExceptionHandler { * @param e the occuring exception */ public void handleException(ScrimEndPointConfiguration scimProviderConfiguration, ScimPropagationException e) { - String errorMessage = "[SCIM] Error while propagating to SCIM endpoint " + scimProviderConfiguration.getId(); + String errorMessage = "[SCIM] Error while propagating to SCIM endpoint " + scimProviderConfiguration.getName(); if (rollbackStrategy.shouldRollback(scimProviderConfiguration, e)) { session.getTransactionManager().rollback(); - LOGGER.error(errorMessage, e); + LOGGER.error("TRANSACTION ROLLBACK - " + errorMessage, e); } else { LOGGER.warn(errorMessage); } diff --git a/src/main/java/sh/libre/scim/core/exceptions/ScimPropagationException.java b/src/main/java/sh/libre/scim/core/exceptions/ScimPropagationException.java index 7b1a747b21..bee5ee18fd 100644 --- a/src/main/java/sh/libre/scim/core/exceptions/ScimPropagationException.java +++ b/src/main/java/sh/libre/scim/core/exceptions/ScimPropagationException.java @@ -2,11 +2,11 @@ package sh.libre.scim.core.exceptions; public abstract class ScimPropagationException extends Exception { - public ScimPropagationException(String message) { + protected ScimPropagationException(String message) { super(message); } - public ScimPropagationException(String message, Exception e) { + protected ScimPropagationException(String message, Exception e) { super(message, e); } } diff --git a/src/main/java/sh/libre/scim/core/service/GroupScimService.java b/src/main/java/sh/libre/scim/core/service/GroupScimService.java index 5297acdac3..bd09f3e9b2 100644 --- a/src/main/java/sh/libre/scim/core/service/GroupScimService.java +++ b/src/main/java/sh/libre/scim/core/service/GroupScimService.java @@ -24,7 +24,7 @@ import java.util.TreeSet; import java.util.stream.Stream; public class GroupScimService extends AbstractScimService { - private final Logger LOGGER = Logger.getLogger(GroupScimService.class); + private static final Logger LOGGER = Logger.getLogger(GroupScimService.class); public GroupScimService(KeycloakSession keycloakSession, ScrimEndPointConfiguration scimProviderConfiguration, SkipOrStopStrategy skipOrStopStrategy) { super(keycloakSession, scimProviderConfiguration, ScimResourceType.GROUP, skipOrStopStrategy); diff --git a/src/main/java/sh/libre/scim/core/service/ScimClient.java b/src/main/java/sh/libre/scim/core/service/ScimClient.java index 7fcda90091..90bcfcf459 100644 --- a/src/main/java/sh/libre/scim/core/service/ScimClient.java +++ b/src/main/java/sh/libre/scim/core/service/ScimClient.java @@ -29,7 +29,9 @@ public class ScimClient implements AutoCloseable { private final ScimResourceType scimResourceType; - { + private ScimClient(ScimRequestBuilder scimRequestBuilder, ScimResourceType scimResourceType) { + this.scimRequestBuilder = scimRequestBuilder; + this.scimResourceType = scimResourceType; RetryConfig retryConfig = RetryConfig.custom() .maxAttempts(10) .intervalFunction(IntervalFunction.ofExponentialBackoff()) @@ -38,11 +40,6 @@ public class ScimClient implements AutoCloseable { retryRegistry = RetryRegistry.of(retryConfig); } - private ScimClient(ScimRequestBuilder scimRequestBuilder, ScimResourceType scimResourceType) { - this.scimRequestBuilder = scimRequestBuilder; - this.scimResourceType = scimResourceType; - } - public static ScimClient open(ScrimEndPointConfiguration scimProviderConfiguration, ScimResourceType scimResourceType) { String scimApplicationBaseUrl = scimProviderConfiguration.getEndPoint(); Map httpHeaders = new HashMap<>(); @@ -69,18 +66,23 @@ public class ScimClient implements AutoCloseable { "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()) - .setResource(scimForCreation) - .sendRequest() - ); - checkResponseIsSuccess(response); - S resource = response.getResource(); - return resource.getId() - .map(EntityOnRemoteScimId::new) - .orElseThrow(() -> new InvalidResponseFromScimEndpointException(response, "Created SCIM resource does not have id")); + try { + Retry retry = retryRegistry.retry("create-%s".formatted(id.asString())); + ServerResponse response = retry.executeSupplier(() -> scimRequestBuilder + .create(getResourceClass(), getScimEndpoint()) + .setResource(scimForCreation) + .sendRequest() + ); + checkResponseIsSuccess(response); + S resource = response.getResource(); + return resource.getId() + .map(EntityOnRemoteScimId::new) + .orElseThrow(() -> new InvalidResponseFromScimEndpointException(response, "Created SCIM resource does not have id")); + + } catch (Exception e) { + LOGGER.warn(e); + throw new InvalidResponseFromScimEndpointException("Exception while retrying create " + e.getMessage(), e); + } } private void checkResponseIsSuccess(ServerResponse response) throws InvalidResponseFromScimEndpointException { @@ -99,23 +101,31 @@ public class ScimClient implements AutoCloseable { 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 - .update(getResourceClass(), getScimEndpoint(), externalId.asString()) - .setResource(scimForReplace) - .sendRequest() - ); - checkResponseIsSuccess(response); + try { + ServerResponse response = retry.executeSupplier(() -> scimRequestBuilder + .update(getResourceClass(), getScimEndpoint(), externalId.asString()) + .setResource(scimForReplace) + .sendRequest() + ); + checkResponseIsSuccess(response); + } catch (Exception e) { + LOGGER.warn(e); + throw new InvalidResponseFromScimEndpointException("Exception while retrying update " + e.getMessage(), e); + } } 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 - .delete(getResourceClass(), getScimEndpoint(), externalId.asString()) - .sendRequest() - ); - checkResponseIsSuccess(response); + try { + ServerResponse response = retry.executeSupplier(() -> scimRequestBuilder + .delete(getResourceClass(), getScimEndpoint(), externalId.asString()) + .sendRequest() + ); + checkResponseIsSuccess(response); + } catch (Exception e) { + LOGGER.warn(e); + throw new InvalidResponseFromScimEndpointException("Exception while retrying delete " + e.getMessage(), e); + } } @Override diff --git a/src/main/java/sh/libre/scim/core/service/UserScimService.java b/src/main/java/sh/libre/scim/core/service/UserScimService.java index cfd4a826ad..c0262f3d4a 100644 --- a/src/main/java/sh/libre/scim/core/service/UserScimService.java +++ b/src/main/java/sh/libre/scim/core/service/UserScimService.java @@ -20,11 +20,12 @@ import sh.libre.scim.core.exceptions.UnexpectedScimDataException; import java.util.ArrayList; import java.util.List; +import java.util.Objects; import java.util.Optional; import java.util.stream.Stream; public class UserScimService extends AbstractScimService { - private final Logger LOGGER = Logger.getLogger(UserScimService.class); + private static final Logger LOGGER = Logger.getLogger(UserScimService.class); public UserScimService( KeycloakSession keycloakSession, @@ -56,7 +57,9 @@ public class UserScimService extends AbstractScimService { if (matchedByUsername.isPresent() && matchedByEmail.isPresent() && !matchedByUsername.equals(matchedByEmail)) { - throw new InconsistentScimMappingException("Found 2 possible users for remote user " + matchedByUsername.get() + " - " + matchedByEmail.get()); + String inconstencyErrorMessage = "Found 2 possible users for remote user " + matchedByUsername.get() + " - " + matchedByEmail.get(); + LOGGER.warn(inconstencyErrorMessage); + throw new InconsistentScimMappingException(inconstencyErrorMessage); } if (matchedByUsername.isPresent()) { return matchedByUsername; @@ -94,12 +97,12 @@ public class UserScimService extends AbstractScimService { String firstAndLastName = String.format("%s %s", StringUtils.defaultString(roleMapperModel.getFirstName()), StringUtils.defaultString(roleMapperModel.getLastName())).trim(); - String displayName = StringUtils.defaultString(firstAndLastName, roleMapperModel.getUsername()); + String displayName = Objects.toString(firstAndLastName, roleMapperModel.getUsername()); Stream groupRoleModels = roleMapperModel.getGroupsStream().flatMap(RoleMapperModel::getRoleMappingsStream); Stream roleModels = roleMapperModel.getRoleMappingsStream(); Stream allRoleModels = Stream.concat(groupRoleModels, roleModels); List roles = allRoleModels - .filter((r) -> BooleanUtils.TRUE.equals(r.getFirstAttribute("scim"))) + .filter(r -> BooleanUtils.TRUE.equals(r.getFirstAttribute("scim"))) .map(RoleModel::getName) .map(roleName -> { PersonRole personRole = new PersonRole(); diff --git a/src/main/java/sh/libre/scim/event/ScimBackgroundGroupMembershipUpdater.java b/src/main/java/sh/libre/scim/event/ScimBackgroundGroupMembershipUpdater.java index d7a0731573..4c49f74f67 100644 --- a/src/main/java/sh/libre/scim/event/ScimBackgroundGroupMembershipUpdater.java +++ b/src/main/java/sh/libre/scim/event/ScimBackgroundGroupMembershipUpdater.java @@ -63,7 +63,7 @@ public class ScimBackgroundGroupMembershipUpdater { private boolean isDirtyGroup(GroupModel g) { String groupDirtySinceAttribute = g.getFirstAttribute(GROUP_DIRTY_SINCE_ATTRIBUTE_NAME); try { - int groupDirtySince = Integer.parseInt(groupDirtySinceAttribute); + long groupDirtySince = Long.parseLong(groupDirtySinceAttribute); // Must be dirty for more than DEBOUNCE_DELAY_MS // (otherwise update will be dispatched in next scheduled loop) return System.currentTimeMillis() - groupDirtySince > DEBOUNCE_DELAY_MS; diff --git a/src/main/java/sh/libre/scim/jpa/ScimResourceDao.java b/src/main/java/sh/libre/scim/jpa/ScimResourceDao.java index 3824ad44d8..4deec373b9 100644 --- a/src/main/java/sh/libre/scim/jpa/ScimResourceDao.java +++ b/src/main/java/sh/libre/scim/jpa/ScimResourceDao.java @@ -91,7 +91,6 @@ public class ScimResourceDao { } public void delete(ScimResourceMapping resource) { - EntityManager entityManager = getEntityManager(); entityManager.remove(resource); } } diff --git a/src/main/java/sh/libre/scim/jpa/ScimResourceMapping.java b/src/main/java/sh/libre/scim/jpa/ScimResourceMapping.java index fabd266444..ade6848ccd 100644 --- a/src/main/java/sh/libre/scim/jpa/ScimResourceMapping.java +++ b/src/main/java/sh/libre/scim/jpa/ScimResourceMapping.java @@ -14,8 +14,8 @@ import sh.libre.scim.core.service.KeycloakId; @IdClass(ScimResourceId.class) @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") + @NamedQuery(name = "findById", query = "from ScimResourceMapping where realmId = :realmId and componentId = :componentId and type = :type and id = :id"), + @NamedQuery(name = "findByExternalId", query = "from ScimResourceMapping where realmId = :realmId and componentId = :componentId and type = :type and externalId = :id") }) public class ScimResourceMapping { diff --git a/src/main/java/sh/libre/scim/jpa/ScimResourceProvider.java b/src/main/java/sh/libre/scim/jpa/ScimResourceProvider.java index 3dc284e22f..6ef55a060e 100644 --- a/src/main/java/sh/libre/scim/jpa/ScimResourceProvider.java +++ b/src/main/java/sh/libre/scim/jpa/ScimResourceProvider.java @@ -19,6 +19,7 @@ public class ScimResourceProvider implements JpaEntityProvider { @Override public void close() { + // Nothing to close } @Override diff --git a/src/main/resources/META-INF/scim-resource-changelog.xml b/src/main/resources/META-INF/scim-resource-changelog.xml index cf300faad5..d3e2687a57 100644 --- a/src/main/resources/META-INF/scim-resource-changelog.xml +++ b/src/main/resources/META-INF/scim-resource-changelog.xml @@ -22,13 +22,13 @@ - - -