Handle retry exceptions as a 500 server response

This commit is contained in:
Alex Morel 2024-07-18 15:44:15 +02:00
parent 9c7c557b76
commit d8cba394b2
14 changed files with 95 additions and 59 deletions

View file

@ -113,7 +113,7 @@ public class ScimDispatcher {
if (matchingClient.isPresent()) { if (matchingClient.isPresent()) {
try { try {
operationToDispatch.acceptThrows(matchingClient.get()); 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) { } catch (ScimPropagationException e) {
exceptionHandler.handleException(matchingClient.get().getConfiguration(), e); exceptionHandler.handleException(matchingClient.get().getConfiguration(), e);
} }
@ -130,7 +130,7 @@ public class ScimDispatcher {
if (matchingClient.isPresent()) { if (matchingClient.isPresent()) {
try { try {
operationToDispatch.acceptThrows(matchingClient.get()); 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) { } catch (ScimPropagationException e) {
exceptionHandler.handleException(matchingClient.get().getConfiguration(), e); exceptionHandler.handleException(matchingClient.get().getConfiguration(), e);
} }

View file

@ -18,6 +18,7 @@ public class ScrimEndPointConfiguration {
private final String endPoint; private final String endPoint;
private final String id; private final String id;
private final String name;
private final String contentType; private final String contentType;
private final String authorizationHeaderValue; private final String authorizationHeaderValue;
private final ImportAction importAction; private final ImportAction importAction;
@ -42,6 +43,7 @@ public class ScrimEndPointConfiguration {
contentType = scimProviderConfiguration.get(CONF_KEY_CONTENT_TYPE, ""); contentType = scimProviderConfiguration.get(CONF_KEY_CONTENT_TYPE, "");
endPoint = scimProviderConfiguration.get(CONF_KEY_ENDPOINT, ""); endPoint = scimProviderConfiguration.get(CONF_KEY_ENDPOINT, "");
id = scimProviderConfiguration.getId(); id = scimProviderConfiguration.getId();
name = scimProviderConfiguration.getName();
importAction = ImportAction.valueOf(scimProviderConfiguration.get(CONF_KEY_SYNC_IMPORT_ACTION)); importAction = ImportAction.valueOf(scimProviderConfiguration.get(CONF_KEY_SYNC_IMPORT_ACTION));
pullFromScimSynchronisationActivated = scimProviderConfiguration.get(CONF_KEY_SYNC_IMPORT, false); pullFromScimSynchronisationActivated = scimProviderConfiguration.get(CONF_KEY_SYNC_IMPORT, false);
pushToScimSynchronisationActivated = scimProviderConfiguration.get(CONF_KEY_SYNC_REFRESH, false); pushToScimSynchronisationActivated = scimProviderConfiguration.get(CONF_KEY_SYNC_REFRESH, false);
@ -70,6 +72,10 @@ public class ScrimEndPointConfiguration {
return id; return id;
} }
public String getName() {
return name;
}
public ImportAction getImportAction() { public ImportAction getImportAction() {
return importAction; return importAction;
} }

View file

@ -2,16 +2,27 @@ package sh.libre.scim.core.exceptions;
import de.captaingoldfish.scim.sdk.client.response.ServerResponse; import de.captaingoldfish.scim.sdk.client.response.ServerResponse;
import java.util.Optional;
public class InvalidResponseFromScimEndpointException extends ScimPropagationException { public class InvalidResponseFromScimEndpointException extends ScimPropagationException {
private final ServerResponse response; private final transient Optional<ServerResponse> response;
public InvalidResponseFromScimEndpointException(ServerResponse response, String message) { public InvalidResponseFromScimEndpointException(ServerResponse response, String message) {
super(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<ServerResponse> getResponse() {
return response; return response;
} }

View file

@ -40,10 +40,16 @@ public enum RollbackApproach implements RollbackStrategy {
} }
private boolean shouldRollbackBecauseOfResponse(InvalidResponseFromScimEndpointException e) { private boolean shouldRollbackBecauseOfResponse(InvalidResponseFromScimEndpointException e) {
// We consider that 404 are acceptable, otherwise rollback // If we have a response
int httpStatus = e.getResponse().getHttpStatus(); return e.getResponse().map(r -> {
ArrayList<Integer> acceptableStatus = Lists.newArrayList(200, 204, 404); // We consider that 404 are acceptable, otherwise rollback
return !acceptableStatus.contains(httpStatus); ArrayList<Integer> 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
);
} }
} }
} }

View file

@ -32,10 +32,10 @@ public class ScimExceptionHandler {
* @param e the occuring exception * @param e the occuring exception
*/ */
public void handleException(ScrimEndPointConfiguration scimProviderConfiguration, ScimPropagationException e) { 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)) { if (rollbackStrategy.shouldRollback(scimProviderConfiguration, e)) {
session.getTransactionManager().rollback(); session.getTransactionManager().rollback();
LOGGER.error(errorMessage, e); LOGGER.error("TRANSACTION ROLLBACK - " + errorMessage, e);
} else { } else {
LOGGER.warn(errorMessage); LOGGER.warn(errorMessage);
} }

View file

@ -2,11 +2,11 @@ package sh.libre.scim.core.exceptions;
public abstract class ScimPropagationException extends Exception { public abstract class ScimPropagationException extends Exception {
public ScimPropagationException(String message) { protected ScimPropagationException(String message) {
super(message); super(message);
} }
public ScimPropagationException(String message, Exception e) { protected ScimPropagationException(String message, Exception e) {
super(message, e); super(message, e);
} }
} }

View file

@ -24,7 +24,7 @@ import java.util.TreeSet;
import java.util.stream.Stream; import java.util.stream.Stream;
public class GroupScimService extends AbstractScimService<GroupModel, Group> { public class GroupScimService extends AbstractScimService<GroupModel, Group> {
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) { public GroupScimService(KeycloakSession keycloakSession, ScrimEndPointConfiguration scimProviderConfiguration, SkipOrStopStrategy skipOrStopStrategy) {
super(keycloakSession, scimProviderConfiguration, ScimResourceType.GROUP, skipOrStopStrategy); super(keycloakSession, scimProviderConfiguration, ScimResourceType.GROUP, skipOrStopStrategy);

View file

@ -29,7 +29,9 @@ public class ScimClient<S extends ResourceNode> implements AutoCloseable {
private final ScimResourceType scimResourceType; private final ScimResourceType scimResourceType;
{ private ScimClient(ScimRequestBuilder scimRequestBuilder, ScimResourceType scimResourceType) {
this.scimRequestBuilder = scimRequestBuilder;
this.scimResourceType = scimResourceType;
RetryConfig retryConfig = RetryConfig.custom() RetryConfig retryConfig = RetryConfig.custom()
.maxAttempts(10) .maxAttempts(10)
.intervalFunction(IntervalFunction.ofExponentialBackoff()) .intervalFunction(IntervalFunction.ofExponentialBackoff())
@ -38,11 +40,6 @@ public class ScimClient<S extends ResourceNode> implements AutoCloseable {
retryRegistry = RetryRegistry.of(retryConfig); retryRegistry = RetryRegistry.of(retryConfig);
} }
private ScimClient(ScimRequestBuilder scimRequestBuilder, ScimResourceType scimResourceType) {
this.scimRequestBuilder = scimRequestBuilder;
this.scimResourceType = scimResourceType;
}
public static ScimClient open(ScrimEndPointConfiguration scimProviderConfiguration, ScimResourceType scimResourceType) { public static ScimClient open(ScrimEndPointConfiguration scimProviderConfiguration, ScimResourceType scimResourceType) {
String scimApplicationBaseUrl = scimProviderConfiguration.getEndPoint(); String scimApplicationBaseUrl = scimProviderConfiguration.getEndPoint();
Map<String, String> httpHeaders = new HashMap<>(); Map<String, String> httpHeaders = new HashMap<>();
@ -69,18 +66,23 @@ public class ScimClient<S extends ResourceNode> implements AutoCloseable {
"User to create should never have an existing id: %s %s".formatted(id, scimForCreationId.get()) "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 try {
Retry retry = retryRegistry.retry("create-%s".formatted(id.asString())); Retry retry = retryRegistry.retry("create-%s".formatted(id.asString()));
ServerResponse<S> response = retry.executeSupplier(() -> scimRequestBuilder ServerResponse<S> response = retry.executeSupplier(() -> scimRequestBuilder
.create(getResourceClass(), getScimEndpoint()) .create(getResourceClass(), getScimEndpoint())
.setResource(scimForCreation) .setResource(scimForCreation)
.sendRequest() .sendRequest()
); );
checkResponseIsSuccess(response); checkResponseIsSuccess(response);
S resource = response.getResource(); S resource = response.getResource();
return resource.getId() return resource.getId()
.map(EntityOnRemoteScimId::new) .map(EntityOnRemoteScimId::new)
.orElseThrow(() -> new InvalidResponseFromScimEndpointException(response, "Created SCIM resource does not have id")); .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<S> response) throws InvalidResponseFromScimEndpointException { private void checkResponseIsSuccess(ServerResponse<S> response) throws InvalidResponseFromScimEndpointException {
@ -99,23 +101,31 @@ public class ScimClient<S extends ResourceNode> implements AutoCloseable {
public void update(EntityOnRemoteScimId externalId, S scimForReplace) throws InvalidResponseFromScimEndpointException { public void update(EntityOnRemoteScimId externalId, S scimForReplace) throws InvalidResponseFromScimEndpointException {
Retry retry = retryRegistry.retry("replace-%s".formatted(externalId.asString())); Retry retry = retryRegistry.retry("replace-%s".formatted(externalId.asString()));
// TODO Exception handling : check that all exceptions are wrapped in server response try {
ServerResponse<S> response = retry.executeSupplier(() -> scimRequestBuilder ServerResponse<S> response = retry.executeSupplier(() -> scimRequestBuilder
.update(getResourceClass(), getScimEndpoint(), externalId.asString()) .update(getResourceClass(), getScimEndpoint(), externalId.asString())
.setResource(scimForReplace) .setResource(scimForReplace)
.sendRequest() .sendRequest()
); );
checkResponseIsSuccess(response); 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 { public void delete(EntityOnRemoteScimId externalId) throws InvalidResponseFromScimEndpointException {
Retry retry = retryRegistry.retry("delete-%s".formatted(externalId.asString())); Retry retry = retryRegistry.retry("delete-%s".formatted(externalId.asString()));
// TODO Exception handling : check that all exceptions are wrapped in server response try {
ServerResponse<S> response = retry.executeSupplier(() -> scimRequestBuilder ServerResponse<S> response = retry.executeSupplier(() -> scimRequestBuilder
.delete(getResourceClass(), getScimEndpoint(), externalId.asString()) .delete(getResourceClass(), getScimEndpoint(), externalId.asString())
.sendRequest() .sendRequest()
); );
checkResponseIsSuccess(response); checkResponseIsSuccess(response);
} catch (Exception e) {
LOGGER.warn(e);
throw new InvalidResponseFromScimEndpointException("Exception while retrying delete " + e.getMessage(), e);
}
} }
@Override @Override

View file

@ -20,11 +20,12 @@ import sh.libre.scim.core.exceptions.UnexpectedScimDataException;
import java.util.ArrayList; import java.util.ArrayList;
import java.util.List; import java.util.List;
import java.util.Objects;
import java.util.Optional; import java.util.Optional;
import java.util.stream.Stream; import java.util.stream.Stream;
public class UserScimService extends AbstractScimService<UserModel, User> { public class UserScimService extends AbstractScimService<UserModel, User> {
private final Logger LOGGER = Logger.getLogger(UserScimService.class); private static final Logger LOGGER = Logger.getLogger(UserScimService.class);
public UserScimService( public UserScimService(
KeycloakSession keycloakSession, KeycloakSession keycloakSession,
@ -56,7 +57,9 @@ public class UserScimService extends AbstractScimService<UserModel, User> {
if (matchedByUsername.isPresent() if (matchedByUsername.isPresent()
&& matchedByEmail.isPresent() && matchedByEmail.isPresent()
&& !matchedByUsername.equals(matchedByEmail)) { && !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()) { if (matchedByUsername.isPresent()) {
return matchedByUsername; return matchedByUsername;
@ -94,12 +97,12 @@ public class UserScimService extends AbstractScimService<UserModel, User> {
String firstAndLastName = String.format("%s %s", String firstAndLastName = String.format("%s %s",
StringUtils.defaultString(roleMapperModel.getFirstName()), StringUtils.defaultString(roleMapperModel.getFirstName()),
StringUtils.defaultString(roleMapperModel.getLastName())).trim(); StringUtils.defaultString(roleMapperModel.getLastName())).trim();
String displayName = StringUtils.defaultString(firstAndLastName, roleMapperModel.getUsername()); String displayName = Objects.toString(firstAndLastName, roleMapperModel.getUsername());
Stream<RoleModel> groupRoleModels = roleMapperModel.getGroupsStream().flatMap(RoleMapperModel::getRoleMappingsStream); Stream<RoleModel> groupRoleModels = roleMapperModel.getGroupsStream().flatMap(RoleMapperModel::getRoleMappingsStream);
Stream<RoleModel> roleModels = roleMapperModel.getRoleMappingsStream(); Stream<RoleModel> roleModels = roleMapperModel.getRoleMappingsStream();
Stream<RoleModel> allRoleModels = Stream.concat(groupRoleModels, roleModels); Stream<RoleModel> allRoleModels = Stream.concat(groupRoleModels, roleModels);
List<PersonRole> roles = allRoleModels List<PersonRole> roles = allRoleModels
.filter((r) -> BooleanUtils.TRUE.equals(r.getFirstAttribute("scim"))) .filter(r -> BooleanUtils.TRUE.equals(r.getFirstAttribute("scim")))
.map(RoleModel::getName) .map(RoleModel::getName)
.map(roleName -> { .map(roleName -> {
PersonRole personRole = new PersonRole(); PersonRole personRole = new PersonRole();

View file

@ -63,7 +63,7 @@ public class ScimBackgroundGroupMembershipUpdater {
private boolean isDirtyGroup(GroupModel g) { private boolean isDirtyGroup(GroupModel g) {
String groupDirtySinceAttribute = g.getFirstAttribute(GROUP_DIRTY_SINCE_ATTRIBUTE_NAME); String groupDirtySinceAttribute = g.getFirstAttribute(GROUP_DIRTY_SINCE_ATTRIBUTE_NAME);
try { try {
int groupDirtySince = Integer.parseInt(groupDirtySinceAttribute); long groupDirtySince = Long.parseLong(groupDirtySinceAttribute);
// Must be dirty for more than DEBOUNCE_DELAY_MS // Must be dirty for more than DEBOUNCE_DELAY_MS
// (otherwise update will be dispatched in next scheduled loop) // (otherwise update will be dispatched in next scheduled loop)
return System.currentTimeMillis() - groupDirtySince > DEBOUNCE_DELAY_MS; return System.currentTimeMillis() - groupDirtySince > DEBOUNCE_DELAY_MS;

View file

@ -91,7 +91,6 @@ public class ScimResourceDao {
} }
public void delete(ScimResourceMapping resource) { public void delete(ScimResourceMapping resource) {
EntityManager entityManager = getEntityManager();
entityManager.remove(resource); entityManager.remove(resource);
} }
} }

View file

@ -14,8 +14,8 @@ import sh.libre.scim.core.service.KeycloakId;
@IdClass(ScimResourceId.class) @IdClass(ScimResourceId.class)
@Table(name = "SCIM_RESOURCE_MAPPING") @Table(name = "SCIM_RESOURCE_MAPPING")
@NamedQueries({ @NamedQueries({
@NamedQuery(name = "findById", query = "from ScimResource where realmId = :realmId and componentId = :componentId and type = :type and id = :id"), @NamedQuery(name = "findById", query = "from ScimResourceMapping 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 = "findByExternalId", query = "from ScimResourceMapping where realmId = :realmId and componentId = :componentId and type = :type and externalId = :id")
}) })
public class ScimResourceMapping { public class ScimResourceMapping {

View file

@ -19,6 +19,7 @@ public class ScimResourceProvider implements JpaEntityProvider {
@Override @Override
public void close() { public void close() {
// Nothing to close
} }
@Override @Override

View file

@ -22,13 +22,13 @@
</column> </column>
</createTable> </createTable>
<addPrimaryKey constraintName="PK_SCIM_RESOURCE" tableName="SCIM_RESOURCE" <addPrimaryKey constraintName="PK_SCIM_RESOURCE_MAPPING" tableName="SCIM_RESOURCE_MAPPING"
columnNames="ID,REALM_ID,TYPE,COMPONENT_ID,EXTERNAL_ID"/> columnNames="ID,REALM_ID,TYPE,COMPONENT_ID,EXTERNAL_ID"/>
<addForeignKeyConstraint baseTableName="SCIM_RESOURCE" baseColumnNames="REALM_ID" <addForeignKeyConstraint baseTableName="SCIM_RESOURCE_MAPPING" baseColumnNames="REALM_ID"
constraintName="FK_SCIM_RESOURCE_REALM" referencedTableName="REALM" constraintName="FK_SCIM_RESOURCE_MAPPING_REALM" referencedTableName="REALM"
referencedColumnNames="ID" onDelete="CASCADE" onUpdate="CASCADE"/> referencedColumnNames="ID" onDelete="CASCADE" onUpdate="CASCADE"/>
<addForeignKeyConstraint baseTableName="SCIM_RESOURCE" baseColumnNames="COMPONENT_ID" <addForeignKeyConstraint baseTableName="SCIM_RESOURCE_MAPPING" baseColumnNames="COMPONENT_ID"
constraintName="FK_SCIM_RESOURCE_COMPONENT" referencedTableName="COMPONENT" constraintName="FK_SCIM_RESOURCE_MAPPING_COMPONENT" referencedTableName="COMPONENT"
referencedColumnNames="ID" onDelete="CASCADE" onUpdate="CASCADE"/> referencedColumnNames="ID" onDelete="CASCADE" onUpdate="CASCADE"/>
</changeSet> </changeSet>