Exception Handler - Step 2: logging uniformisation and add //TODOs

This commit is contained in:
Alex Morel 2024-06-25 09:36:20 +02:00
parent 72e597a09c
commit 399dfbd17e
8 changed files with 77 additions and 78 deletions

View file

@ -82,10 +82,12 @@ public abstract class AbstractScimService<RMM extends RoleMapperModel, S extends
entityOnRemoteScimId entityOnRemoteScimId
.ifPresentOrElse( .ifPresentOrElse(
externalId -> doReplace(roleMapperModel, externalId), externalId -> 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) { } catch (Exception e) {
LOGGER.error(e);
throw new ScimPropagationException("[SCIM] Error while replacing SCIM resource", e); throw new ScimPropagationException("[SCIM] Error while replacing SCIM resource", e);
} }
} }
@ -106,21 +108,21 @@ public abstract class AbstractScimService<RMM extends RoleMapperModel, S extends
} }
public void refreshResources(SynchronizationResult syncRes) throws ScimPropagationException { public void refreshResources(SynchronizationResult syncRes) throws ScimPropagationException {
LOGGER.info("Refresh resources"); LOGGER.info("[SCIM] Refresh resources for endpoint " + this.getConfiguration().getEndPoint());
try (Stream<RMM> resourcesStream = getResourceStream()) { try (Stream<RMM> resourcesStream = getResourceStream()) {
Set<RMM> resources = resourcesStream.collect(Collectors.toUnmodifiableSet()); Set<RMM> resources = resourcesStream.collect(Collectors.toUnmodifiableSet());
for (RMM resource : resources) { for (RMM resource : resources) {
KeycloakId id = getId(resource); KeycloakId id = getId(resource);
LOGGER.infof("Reconciling local resource %s", id); LOGGER.infof("[SCIM] Reconciling local resource %s", id);
if (isSkipRefresh(resource)) { if (isSkipRefresh(resource)) {
LOGGER.infof("Skip local resource %s", id); LOGGER.infof("[SCIM] Skip local resource %s", id);
continue; continue;
} }
if (findById(id).isPresent()) { if (findById(id).isPresent()) {
LOGGER.info("Replacing it"); LOGGER.info("[SCIM] Replacing it");
replace(resource); replace(resource);
} else { } else {
LOGGER.info("Creating it"); LOGGER.info("[SCIM] Creating it");
create(resource); create(resource);
} }
syncRes.increaseUpdated(); syncRes.increaseUpdated();
@ -133,11 +135,11 @@ public abstract class AbstractScimService<RMM extends RoleMapperModel, S extends
protected abstract Stream<RMM> getResourceStream(); protected abstract Stream<RMM> getResourceStream();
public void importResources(SynchronizationResult syncRes) throws ScimPropagationException { public void importResources(SynchronizationResult syncRes) throws ScimPropagationException {
LOGGER.info("Import"); LOGGER.info("[SCIM] Import resources for scim endpoint " + this.getConfiguration().getEndPoint());
try { try {
for (S resource : scimClient.listResources()) { for (S resource : scimClient.listResources()) {
try { try {
LOGGER.infof("Reconciling remote resource %s", resource); LOGGER.infof("[SCIM] Reconciling remote resource %s", resource);
EntityOnRemoteScimId externalId = resource.getId() EntityOnRemoteScimId externalId = resource.getId()
.map(EntityOnRemoteScimId::new) .map(EntityOnRemoteScimId::new)
.orElseThrow(() -> new ScimPropagationException("remote SCIM resource doesn't have an id")); .orElseThrow(() -> new ScimPropagationException("remote SCIM resource doesn't have an id"));
@ -145,49 +147,50 @@ public abstract class AbstractScimService<RMM extends RoleMapperModel, S extends
if (optionalMapping.isPresent()) { if (optionalMapping.isPresent()) {
ScimResource mapping = optionalMapping.get(); ScimResource mapping = optionalMapping.get();
if (entityExists(mapping.getIdAsKeycloakId())) { if (entityExists(mapping.getIdAsKeycloakId())) {
LOGGER.info("Valid mapping found, skipping"); LOGGER.info("[SCIM] Valid mapping found, skipping");
continue; continue;
} else { } else {
LOGGER.info("Delete a dangling mapping"); LOGGER.info("[SCIM] Delete a dangling mapping");
getScimResourceDao().delete(mapping); getScimResourceDao().delete(mapping);
} }
} }
Optional<KeycloakId> mapped = tryToMap(resource); Optional<KeycloakId> mapped = tryToMap(resource);
if (mapped.isPresent()) { if (mapped.isPresent()) {
LOGGER.info("Matched"); LOGGER.info("[SCIM] Matched");
createMapping(mapped.get(), externalId); createMapping(mapped.get(), externalId);
} else { } else {
switch (scimProviderConfiguration.getImportAction()) { switch (scimProviderConfiguration.getImportAction()) {
case CREATE_LOCAL: case CREATE_LOCAL:
LOGGER.info("Create local resource"); LOGGER.info("[SCIM] Create local resource");
try { try {
KeycloakId id = createEntity(resource); KeycloakId id = createEntity(resource);
createMapping(id, externalId); createMapping(id, externalId);
syncRes.increaseAdded(); syncRes.increaseAdded();
} catch (Exception e) { } catch (Exception e) {
// TODO ExceptionHandling should we stop and throw ScimPropagationException here ?
LOGGER.error(e); LOGGER.error(e);
} }
break; break;
case DELETE_REMOTE: case DELETE_REMOTE:
LOGGER.info("Delete remote resource"); LOGGER.info("[SCIM] Delete remote resource");
this.scimClient.delete(externalId); this.scimClient.delete(externalId);
syncRes.increaseRemoved(); syncRes.increaseRemoved();
break; break;
case NOTHING: case NOTHING:
LOGGER.info("Import action set to NOTHING"); LOGGER.info("[SCIM] Import action set to NOTHING");
break; break;
} }
} }
} catch (Exception e) { } catch (Exception e) {
// TODO should we stop and throw ScimPropagationException here ? // TODO ExceptionHandling should we stop and throw ScimPropagationException here ?
LOGGER.error(e); LOGGER.error(e);
e.printStackTrace(); e.printStackTrace();
syncRes.increaseFailed(); syncRes.increaseFailed();
} }
} }
} catch (ResponseException e) { } catch (ResponseException e) {
// TODO should we stop and throw ScimPropagationException here ? // TODO ExceptionHandling should we stop and throw ScimPropagationException here ?
LOGGER.error(e); LOGGER.error(e);
e.printStackTrace(); e.printStackTrace();
syncRes.increaseFailed(); syncRes.increaseFailed();

View file

@ -20,7 +20,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 final Logger LOGGER = Logger.getLogger(GroupScimService.class);
public GroupScimService(KeycloakSession keycloakSession, ScrimProviderConfiguration scimProviderConfiguration) { public GroupScimService(KeycloakSession keycloakSession, ScrimProviderConfiguration scimProviderConfiguration) {
super(keycloakSession, scimProviderConfiguration, ScimResourceType.GROUP); super(keycloakSession, scimProviderConfiguration, ScimResourceType.GROUP);
@ -95,13 +95,13 @@ public class GroupScimService extends AbstractScimService<GroupModel, Group> {
if (optionalGroupMemberMapping.isPresent()) { if (optionalGroupMemberMapping.isPresent()) {
ScimResource groupMemberMapping = optionalGroupMemberMapping.get(); ScimResource groupMemberMapping = optionalGroupMemberMapping.get();
EntityOnRemoteScimId externalIdAsEntityOnRemoteScimId = groupMemberMapping.getExternalIdAsEntityOnRemoteScimId(); EntityOnRemoteScimId externalIdAsEntityOnRemoteScimId = groupMemberMapping.getExternalIdAsEntityOnRemoteScimId();
logger.debugf("found mapping for group member %s as %s", member, externalIdAsEntityOnRemoteScimId);
groupMember.setValue(externalIdAsEntityOnRemoteScimId.asString()); groupMember.setValue(externalIdAsEntityOnRemoteScimId.asString());
URI ref = getUri(ScimResourceType.USER, externalIdAsEntityOnRemoteScimId); URI ref = getUri(ScimResourceType.USER, externalIdAsEntityOnRemoteScimId);
groupMember.setRef(ref.toString()); groupMember.setRef(ref.toString());
group.addMember(groupMember); group.addMember(groupMember);
} else { } 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; return group;

View file

@ -85,6 +85,7 @@ public class ScimClient<S extends ResourceNode> implements AutoCloseable {
private void checkResponseIsSuccess(ServerResponse<S> response) { private void checkResponseIsSuccess(ServerResponse<S> response) {
if (!response.isSuccess()) { if (!response.isSuccess()) {
// TODO Exception handling : we should throw a SCIM Progagation exception here
LOGGER.warn("[SCIM] Issue on SCIM Server response "); LOGGER.warn("[SCIM] Issue on SCIM Server response ");
LOGGER.warn(response.getResponseBody()); LOGGER.warn(response.getResponseBody());
LOGGER.warn(response.getHttpStatus()); LOGGER.warn(response.getHttpStatus());

View file

@ -16,7 +16,7 @@ import java.util.Set;
*/ */
public class ScimDispatcher { 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 KeycloakSession session;
private final ScimExceptionHandler exceptionHandler; private final ScimExceptionHandler exceptionHandler;
@ -34,42 +34,32 @@ public class ScimDispatcher {
* Lists all active ScimStorageProviderFactory and create new ScimClients for each of them * Lists all active ScimStorageProviderFactory and create new ScimClients for each of them
*/ */
public void refreshActiveScimEndpoints() { public void refreshActiveScimEndpoints() {
try { // Step 1: close existing clients (as configuration may have changed)
// Step 1: close existing clients groupScimServices.forEach(GroupScimService::close);
for (GroupScimService c : groupScimServices) { groupScimServices.clear();
c.close(); userScimServices.forEach(UserScimService::close);
} userScimServices.clear();
groupScimServices.clear();
for (UserScimService c : userScimServices) {
c.close();
}
userScimServices.clear();
// Step 2: Get All SCIM endpoints defined in Admin Console (enabled ScimStorageProviderFactory) // Step 2: Get All SCIM endpoints defined in Admin Console (enabled ScimStorageProviderFactory)
session.getContext().getRealm().getComponentsStream() session.getContext().getRealm().getComponentsStream()
.filter(m -> ScimEndpointConfigurationStorageProviderFactory.ID.equals(m.getProviderId()) .filter(m -> ScimEndpointConfigurationStorageProviderFactory.ID.equals(m.getProviderId())
&& m.get("enabled", true)) && m.get("enabled", true))
.forEach(scimEndpointConfigurationRaw -> { .forEach(scimEndpointConfigurationRaw -> {
ScrimProviderConfiguration scrimProviderConfiguration = new ScrimProviderConfiguration(scimEndpointConfigurationRaw); ScrimProviderConfiguration scrimProviderConfiguration = new ScrimProviderConfiguration(scimEndpointConfigurationRaw);
try { try {
// Step 3 : create scim clients for each endpoint // Step 3 : create scim clients for each endpoint
if (scimEndpointConfigurationRaw.get(ScrimProviderConfiguration.CONF_KEY_PROPAGATION_GROUP, false)) { if (scimEndpointConfigurationRaw.get(ScrimProviderConfiguration.CONF_KEY_PROPAGATION_GROUP, false)) {
GroupScimService groupScimService = new GroupScimService(session, scrimProviderConfiguration); GroupScimService groupScimService = new GroupScimService(session, scrimProviderConfiguration);
groupScimServices.add(groupScimService); 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 ?
} }
}); if (scimEndpointConfigurationRaw.get(ScrimProviderConfiguration.CONF_KEY_PROPAGATION_USER, false)) {
} catch (Exception e) { UserScimService userScimService = new UserScimService(session, scrimProviderConfiguration);
logger.error("[SCIM] Error while refreshing scim clients ", e); userScimServices.add(userScimService);
// TODO : how to handle exception here ? }
} } catch (Exception e) {
exceptionHandler.handleInvalidEndpointConfiguration(scimEndpointConfigurationRaw, e);
}
});
} }
public void dispatchUserModificationToAll(SCIMPropagationConsumer<UserScimService> operationToDispatch) { public void dispatchUserModificationToAll(SCIMPropagationConsumer<UserScimService> operationToDispatch) {
@ -84,7 +74,7 @@ public class ScimDispatcher {
} }
}); });
// TODO we could iterate on servicesCorrectlyPropagated to undo modification // 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<GroupScimService> operationToDispatch) { public void dispatchGroupModificationToAll(SCIMPropagationConsumer<GroupScimService> operationToDispatch) {
@ -99,7 +89,7 @@ public class ScimDispatcher {
} }
}); });
// TODO we could iterate on servicesCorrectlyPropagated to undo modification // 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<UserScimService> operationToDispatch) { public void dispatchUserModificationToOne(ComponentModel scimServerConfiguration, SCIMPropagationConsumer<UserScimService> operationToDispatch) {
@ -109,12 +99,12 @@ 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().getId());
} catch (ScimPropagationException e) { } catch (ScimPropagationException e) {
exceptionHandler.handleException(matchingClient.get().getConfiguration(), e); exceptionHandler.handleException(matchingClient.get().getConfiguration(), e);
} }
} else { } 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()) { 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().getId());
} catch (ScimPropagationException e) { } catch (ScimPropagationException e) {
exceptionHandler.handleException(matchingClient.get().getConfiguration(), e); exceptionHandler.handleException(matchingClient.get().getConfiguration(), e);
} }
} else { } 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());
} }
} }

View file

@ -1,6 +1,7 @@
package sh.libre.scim.core; package sh.libre.scim.core;
import org.jboss.logging.Logger; import org.jboss.logging.Logger;
import org.keycloak.component.ComponentModel;
import org.keycloak.models.KeycloakSession; import org.keycloak.models.KeycloakSession;
/** /**
@ -10,8 +11,8 @@ import org.keycloak.models.KeycloakSession;
* - The thrown exception itself * - The thrown exception itself
*/ */
public class ScimExceptionHandler { 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; private final KeycloakSession session;
public ScimExceptionHandler(KeycloakSession session) { public ScimExceptionHandler(KeycloakSession session) {
@ -25,7 +26,14 @@ public class ScimExceptionHandler {
* @param e the occuring exception * @param e the occuring exception
*/ */
public void handleException(ScrimProviderConfiguration scimProviderConfiguration, ScimPropagationException e) { public void handleException(ScrimProviderConfiguration scimProviderConfiguration, ScimPropagationException e) {
logger.error("[SCIM] Error while propagating to SCIM endpoint " + scimProviderConfiguration.getId(), e); LOGGER.error("[SCIM] Error while propagating to SCIM endpoint %s", scimProviderConfiguration.getId(), e);
// TODO session.getTransactionManager().rollback(); // 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
} }
} }

View file

@ -20,7 +20,7 @@ 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 final Logger LOGGER = Logger.getLogger(UserScimService.class);
public UserScimService( public UserScimService(
KeycloakSession keycloakSession, KeycloakSession keycloakSession,
@ -41,17 +41,18 @@ public class UserScimService extends AbstractScimService<UserModel, User> {
@Override @Override
protected Optional<KeycloakId> tryToMap(User resource) { protected Optional<KeycloakId> tryToMap(User resource) {
Optional<KeycloakId> matchedByUsername = resource.getUserName() Optional<KeycloakId> matchedByUsername = resource.getUserName()
.map(getKeycloakDao()::getUserByUsername) .map(getKeycloakDao()::getUserByUsername)
.map(this::getId); .map(this::getId);
Optional<KeycloakId> matchedByEmail = resource.getEmails().stream() Optional<KeycloakId> matchedByEmail = resource.getEmails().stream()
.findFirst() .findFirst()
.flatMap(MultiComplexNode::getValue) .flatMap(MultiComplexNode::getValue)
.map(getKeycloakDao()::getUserByEmail) .map(getKeycloakDao()::getUserByEmail)
.map(this::getId); .map(this::getId);
if (matchedByUsername.isPresent() if (matchedByUsername.isPresent()
&& matchedByEmail.isPresent() && matchedByEmail.isPresent()
&& !matchedByUsername.equals(matchedByEmail)) { && !matchedByUsername.equals(matchedByEmail)) {
logger.warnf("found 2 possible users for remote user %s %s", matchedByUsername.get(), matchedByEmail.get()); // 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(); return Optional.empty();
} }
if (matchedByUsername.isPresent()) { if (matchedByUsername.isPresent()) {

View file

@ -228,11 +228,7 @@ public class ScimEventListenerProvider implements EventListenerProvider {
@Override @Override
public void close() { public void close() {
try { dispatcher.close();
dispatcher.close();
} catch (Exception e) {
LOGGER.error("Error while closing dispatcher", e);
}
} }

View file

@ -31,7 +31,7 @@ import java.util.List;
public class ScimEndpointConfigurationStorageProviderFactory public class ScimEndpointConfigurationStorageProviderFactory
implements UserStorageProviderFactory<ScimEndpointConfigurationStorageProviderFactory.ScimEndpointConfigurationStorageProvider>, ImportSynchronization { implements UserStorageProviderFactory<ScimEndpointConfigurationStorageProviderFactory.ScimEndpointConfigurationStorageProvider>, ImportSynchronization {
public static final String ID = "scim"; public static final String ID = "scim";
private final Logger logger = Logger.getLogger(ScimEndpointConfigurationStorageProviderFactory.class); private final Logger LOGGER = Logger.getLogger(ScimEndpointConfigurationStorageProviderFactory.class);
@Override @Override
public String getId() { public String getId() {
@ -43,7 +43,7 @@ public class ScimEndpointConfigurationStorageProviderFactory
public SynchronizationResult sync(KeycloakSessionFactory sessionFactory, String realmId, public SynchronizationResult sync(KeycloakSessionFactory sessionFactory, String realmId,
UserStorageProviderModel model) { UserStorageProviderModel model) {
// TODO if this should be kept here, better document purpose & usage // 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(); SynchronizationResult result = new SynchronizationResult();
KeycloakModelUtils.runJobInTransaction(sessionFactory, session -> { KeycloakModelUtils.runJobInTransaction(sessionFactory, session -> {
RealmModel realm = session.realms().getRealm(realmId); RealmModel realm = session.realms().getRealm(realmId);
@ -78,7 +78,7 @@ public class ScimEndpointConfigurationStorageProviderFactory
ScimDispatcher dispatcher = new ScimDispatcher(session); ScimDispatcher dispatcher = new ScimDispatcher(session);
for (GroupModel group : session.groups().getGroupsStream(realm) for (GroupModel group : session.groups().getGroupsStream(realm)
.filter(x -> BooleanUtils.TRUE.equals(x.getFirstAttribute("scim-dirty"))).toList()) { .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)); dispatcher.dispatchGroupModificationToAll(client -> client.replace(group));
group.removeAttribute("scim-dirty"); group.removeAttribute("scim-dirty");
} }