From ad60363cbdce5eaef9f177f13e26dd8fef37d502 Mon Sep 17 00:00:00 2001
From: Brendan Le Ny <bleny@codelutin.com>
Date: Mon, 24 Jun 2024 11:39:32 +0200
Subject: [PATCH] Remove all unsafe Optional.get calls
---
.../libre/scim/core/AbstractScimService.java | 86 ++++++++++---------
.../sh/libre/scim/core/GroupScimService.java | 65 +++++++-------
.../sh/libre/scim/core/UserScimService.java | 44 ++++------
3 files changed, 94 insertions(+), 101 deletions(-)
diff --git a/src/main/java/sh/libre/scim/core/AbstractScimService.java b/src/main/java/sh/libre/scim/core/AbstractScimService.java
index a0939ad..9586a23 100644
--- a/src/main/java/sh/libre/scim/core/AbstractScimService.java
+++ b/src/main/java/sh/libre/scim/core/AbstractScimService.java
@@ -12,8 +12,9 @@ import sh.libre.scim.jpa.ScimResourceDao;
import java.net.URI;
import java.net.URISyntaxException;
-import java.util.NoSuchElementException;
import java.util.Optional;
+import java.util.Set;
+import java.util.stream.Collectors;
import java.util.stream.Stream;
public abstract class AbstractScimService<RMM extends RoleMapperModel, S extends ResourceNode> implements AutoCloseable {
@@ -76,52 +77,55 @@ public abstract class AbstractScimService<RMM extends RoleMapperModel, S extends
if (isSkip(roleMapperModel))
return;
KeycloakId id = getId(roleMapperModel);
- ScimResource scimResource = findById(id).get();
- EntityOnRemoteScimId externalId = scimResource.getExternalIdAsEntityOnRemoteScimId();
- S scimForReplace = toScimForReplace(roleMapperModel, externalId);
- scimClient.replace(externalId, scimForReplace);
- } catch (NoSuchElementException e) {
- LOGGER.warnf("failed to replace resource %s, scim mapping not found", getId(roleMapperModel));
+ Optional<EntityOnRemoteScimId> entityOnRemoteScimId = findById(id)
+ .map(ScimResource::getExternalIdAsEntityOnRemoteScimId);
+ entityOnRemoteScimId
+ .ifPresentOrElse(
+ externalId -> doReplace(roleMapperModel, externalId),
+ () -> 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);
}
}
+ private void doReplace(RMM roleMapperModel, EntityOnRemoteScimId externalId) {
+ S scimForReplace = toScimForReplace(roleMapperModel, externalId);
+ scimClient.replace(externalId, scimForReplace);
+ }
+
protected abstract S toScimForReplace(RMM roleMapperModel, EntityOnRemoteScimId externalId);
public void delete(KeycloakId id) throws ScimPropagationException {
- try {
- ScimResource resource = findById(id).get();
- EntityOnRemoteScimId externalId = resource.getExternalIdAsEntityOnRemoteScimId();
- scimClient.delete(externalId);
- getScimResourceDao().delete(resource);
- } catch (NoSuchElementException e) {
- throw new ScimPropagationException("Failed to delete resource %s, scim mapping not found: ".formatted(id), e);
- }
+ 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);
}
public void refreshResources(SynchronizationResult syncRes) throws ScimPropagationException {
LOGGER.info("Refresh resources");
- getResourceStream().forEach(resource -> {
- KeycloakId id = getId(resource);
- LOGGER.infof("Reconciling local resource %s", id);
- if (!isSkipRefresh(resource)) {
- try {
- try {
- findById(id).get();
- LOGGER.info("Replacing it");
- replace(resource);
- } catch (NoSuchElementException e) {
- LOGGER.info("Creating it");
- create(resource);
- }
- syncRes.increaseUpdated();
- } catch (ScimPropagationException e) {
- // TODO handle exception
+ try (Stream<RMM> resourcesStream = getResourceStream()) {
+ Set<RMM> resources = resourcesStream.collect(Collectors.toUnmodifiableSet());
+ for (RMM resource : resources) {
+ KeycloakId id = getId(resource);
+ LOGGER.infof("Reconciling local resource %s", id);
+ if (isSkipRefresh(resource)) {
+ LOGGER.infof("Skip local resource %s", id);
+ continue;
}
+ if (findById(id).isPresent()) {
+ LOGGER.info("Replacing it");
+ replace(resource);
+ } else {
+ LOGGER.info("Creating it");
+ create(resource);
+ }
+ syncRes.increaseUpdated();
}
- });
+ }
}
protected abstract boolean isSkipRefresh(RMM resource);
@@ -130,14 +134,16 @@ public abstract class AbstractScimService<RMM extends RoleMapperModel, S extends
public void importResources(SynchronizationResult syncRes) {
LOGGER.info("Import");
- ScimClient<S> scimClient = this.scimClient;
try {
for (S resource : scimClient.listResources()) {
try {
LOGGER.infof("Reconciling remote resource %s", resource);
- EntityOnRemoteScimId externalId = resource.getId().map(EntityOnRemoteScimId::new).get();
- try {
- ScimResource mapping = getScimResourceDao().findByExternalId(externalId, type).get();
+ EntityOnRemoteScimId externalId = resource.getId()
+ .map(EntityOnRemoteScimId::new)
+ .orElseThrow(() -> new ScimPropagationException("remote SCIM resource doesn't have an id"));
+ Optional<ScimResource> optionalMapping = getScimResourceDao().findByExternalId(externalId, type);
+ if (optionalMapping.isPresent()) {
+ ScimResource mapping = optionalMapping.get();
if (entityExists(mapping.getIdAsKeycloakId())) {
LOGGER.info("Valid mapping found, skipping");
continue;
@@ -145,8 +151,6 @@ public abstract class AbstractScimService<RMM extends RoleMapperModel, S extends
LOGGER.info("Delete a dangling mapping");
getScimResourceDao().delete(mapping);
}
- } catch (NoSuchElementException e) {
- // nothing to do
}
Optional<KeycloakId> mapped = tryToMap(resource);
@@ -167,7 +171,7 @@ public abstract class AbstractScimService<RMM extends RoleMapperModel, S extends
break;
case DELETE_REMOTE:
LOGGER.info("Delete remote resource");
- scimClient.delete(externalId);
+ this.scimClient.delete(externalId);
syncRes.increaseRemoved();
break;
case NOTHING:
@@ -186,9 +190,9 @@ public abstract class AbstractScimService<RMM extends RoleMapperModel, S extends
}
}
- protected abstract KeycloakId createEntity(S resource);
+ protected abstract KeycloakId createEntity(S resource) throws ScimPropagationException;
- protected abstract Optional<KeycloakId> tryToMap(S resource);
+ protected abstract Optional<KeycloakId> tryToMap(S resource) throws ScimPropagationException;
protected abstract boolean entityExists(KeycloakId keycloakId);
diff --git a/src/main/java/sh/libre/scim/core/GroupScimService.java b/src/main/java/sh/libre/scim/core/GroupScimService.java
index 99c8c5c..687d52a 100644
--- a/src/main/java/sh/libre/scim/core/GroupScimService.java
+++ b/src/main/java/sh/libre/scim/core/GroupScimService.java
@@ -3,9 +3,9 @@ package sh.libre.scim.core;
import de.captaingoldfish.scim.sdk.common.resources.Group;
import de.captaingoldfish.scim.sdk.common.resources.complex.Meta;
import de.captaingoldfish.scim.sdk.common.resources.multicomplex.Member;
-import jakarta.persistence.NoResultException;
import org.apache.commons.collections4.CollectionUtils;
import org.apache.commons.lang3.BooleanUtils;
+import org.apache.commons.lang3.StringUtils;
import org.jboss.logging.Logger;
import org.keycloak.models.GroupModel;
import org.keycloak.models.KeycloakSession;
@@ -15,9 +15,9 @@ import sh.libre.scim.jpa.ScimResource;
import java.net.URI;
import java.net.URISyntaxException;
import java.util.List;
-import java.util.NoSuchElementException;
import java.util.Optional;
import java.util.Set;
+import java.util.TreeSet;
import java.util.stream.Stream;
public class GroupScimService extends AbstractScimService<GroupModel, Group> {
@@ -39,9 +39,9 @@ public class GroupScimService extends AbstractScimService<GroupModel, Group> {
@Override
protected Optional<KeycloakId> tryToMap(Group resource) {
- String externalId = resource.getId().get();
- String displayName = resource.getDisplayName().get();
- Set<String> names = Set.of(externalId, displayName);
+ Set<String> names = new TreeSet<>();
+ resource.getId().ifPresent(names::add);
+ resource.getDisplayName().ifPresent(names::add);
Optional<GroupModel> group = getKeycloakDao().getGroupsStream()
.filter(groupModel -> names.contains(groupModel.getName()))
.findFirst();
@@ -49,28 +49,22 @@ public class GroupScimService extends AbstractScimService<GroupModel, Group> {
}
@Override
- protected KeycloakId createEntity(Group resource) {
- String displayName = resource.getDisplayName().get();
+ protected KeycloakId createEntity(Group resource) throws ScimPropagationException {
+ String displayName = resource.getDisplayName()
+ .filter(StringUtils::isNotBlank)
+ .orElseThrow(() -> new ScimPropagationException("can't create group without name: " + resource));
GroupModel group = getKeycloakDao().createGroup(displayName);
List<Member> groupMembers = resource.getMembers();
if (CollectionUtils.isNotEmpty(groupMembers)) {
for (Member groupMember : groupMembers) {
- try {
- EntityOnRemoteScimId externalId = new EntityOnRemoteScimId(groupMember.getValue().get());
- ScimResource userMapping = getScimResourceDao().findUserByExternalId(externalId).get();
- KeycloakId userId = userMapping.getIdAsKeycloakId();
- try {
- UserModel user = getKeycloakDao().getUserById(userId);
- if (user == null) {
- throw new NoResultException();
- }
- user.joinGroup(group);
- } catch (Exception e) {
- logger.warn(e);
- }
- } catch (NoSuchElementException e) {
- logger.warnf("member %s not found for scim group %s", groupMember.getValue().get(), resource.getId().get());
- }
+ EntityOnRemoteScimId externalId = groupMember.getValue()
+ .map(EntityOnRemoteScimId::new)
+ .orElseThrow(() -> new ScimPropagationException("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)));
+ UserModel userModel = getKeycloakDao().getUserById(userId);
+ userModel.joinGroup(group);
}
}
return new KeycloakId(group.getId());
@@ -94,19 +88,22 @@ public class GroupScimService extends AbstractScimService<GroupModel, Group> {
group.setDisplayName(groupModel.getName());
for (KeycloakId member : members) {
Member groupMember = new Member();
- try {
- ScimResource userMapping = getScimResourceDao().findUserById(member).get();
- logger.debug(userMapping.getExternalIdAsEntityOnRemoteScimId());
- logger.debug(userMapping.getIdAsKeycloakId());
- groupMember.setValue(userMapping.getExternalIdAsEntityOnRemoteScimId().asString());
- String refString = String.format("Users/%s", userMapping.getExternalIdAsEntityOnRemoteScimId().asString());
- URI ref = new URI(refString);
- groupMember.setRef(ref.toString());
+ Optional<ScimResource> optionalGroupMemberMapping = getScimResourceDao().findUserById(member);
+ 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());
+ try {
+ String refString = String.format("Users/%s", externalIdAsEntityOnRemoteScimId.asString());
+ URI ref = new URI(refString);
+ groupMember.setRef(ref.toString());
+ } catch (URISyntaxException e) {
+ logger.warnf("bad ref uri for member " + member);
+ }
group.addMember(groupMember);
- } catch (NoSuchElementException e) {
+ } else {
logger.warnf("member %s not found for group %s", member, groupModel.getId());
- } catch (URISyntaxException e) {
- logger.warnf("bad ref uri for member " + member);
}
}
return group;
diff --git a/src/main/java/sh/libre/scim/core/UserScimService.java b/src/main/java/sh/libre/scim/core/UserScimService.java
index 9a65849..3c30023 100644
--- a/src/main/java/sh/libre/scim/core/UserScimService.java
+++ b/src/main/java/sh/libre/scim/core/UserScimService.java
@@ -40,45 +40,37 @@ public class UserScimService extends AbstractScimService<UserModel, User> {
@Override
protected Optional<KeycloakId> tryToMap(User resource) {
- String username = resource.getUserName().get();
- String email = resource.getEmails().stream()
+ Optional<KeycloakId> matchedByUsername = resource.getUserName()
+ .map(getKeycloakDao()::getUserByUsername)
+ .map(this::getId);
+ Optional<KeycloakId> matchedByEmail = resource.getEmails().stream()
.findFirst()
.flatMap(MultiComplexNode::getValue)
- .orElse(null);
- UserModel sameUsernameUser = null;
- UserModel sameEmailUser = null;
- if (username != null) {
- sameUsernameUser = getKeycloakDao().getUserByUsername(username);
- }
- if (email != null) {
- sameEmailUser = getKeycloakDao().getUserByEmail(email);
- }
- if ((sameUsernameUser != null && sameEmailUser != null)
- && (!StringUtils.equals(sameUsernameUser.getId(), sameEmailUser.getId()))) {
- logger.warnf("found 2 possible users for remote user %s %s", username, email);
+ .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());
return Optional.empty();
}
- if (sameUsernameUser != null) {
- return Optional.of(getId(sameUsernameUser));
+ if (matchedByUsername.isPresent()) {
+ return matchedByUsername;
}
- if (sameEmailUser != null) {
- return Optional.of(getId(sameEmailUser));
- }
- return Optional.empty();
+ return matchedByEmail;
}
@Override
- protected KeycloakId createEntity(User resource) {
- String username = resource.getUserName().get();
- if (StringUtils.isEmpty(username)) {
- throw new IllegalArgumentException("can't create user with empty username");
- }
+ protected KeycloakId createEntity(User resource) throws ScimPropagationException {
+ String username = resource.getUserName()
+ .filter(StringUtils::isNotBlank)
+ .orElseThrow(() -> new ScimPropagationException("can't create user with empty username, 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().get());
+ user.setEnabled(resource.isActive().orElseThrow(() -> new ScimPropagationException("can't create user with undefined 'active', resource id = %s".formatted(resource.getId()))));
return new KeycloakId(user.getId());
}
--
GitLab