diff --git a/src/main/java/fi/metatavu/keycloak/scim/server/AbstractController.java b/src/main/java/fi/metatavu/keycloak/scim/server/AbstractController.java index 9a36923..2ed10d1 100644 --- a/src/main/java/fi/metatavu/keycloak/scim/server/AbstractController.java +++ b/src/main/java/fi/metatavu/keycloak/scim/server/AbstractController.java @@ -33,6 +33,36 @@ protected fi.metatavu.keycloak.scim.server.model.Meta getMeta( return result; } + /** + * Whether the given SCIM attribute path refers to a read-only or + * structural core attribute that PATCH must ignore per RFC 7644 §3.5.2 + * (and the SCIM core schema, RFC 7643 §3.1). + * + * Concretely: "id" (server-assigned resource identifier), "meta" + * (server-assigned metadata), and "schemas" (structural). Servers MUST + * not error on these in PATCH payloads; clients (notably Okta on Group + * Push and user provisioning) echo them back when a PATCH value is + * constructed from a prior GET. Resource controllers consult this + * before resolving an attribute and silently skip the operation when + * it matches. + * + * Note: "externalId" is intentionally NOT in this list. Per RFC 7643 + * §3.1 externalId is an OPTIONAL, client-settable attribute and a + * legitimate target of PATCH. + * + * @param attrPath attribute path from a PatchOp (path-less or path-based) + * @return true if the attribute is read-only / structural and must be ignored + */ + protected static boolean isReadOnlyOrStructural(String attrPath) { + if (attrPath == null) { + return false; + } + return switch (attrPath) { + case "id", "meta", "schemas" -> true; + default -> false; + }; + } + /** * Returns date based on year, month and date * diff --git a/src/main/java/fi/metatavu/keycloak/scim/server/ScimErrors.java b/src/main/java/fi/metatavu/keycloak/scim/server/ScimErrors.java new file mode 100644 index 0000000..6d6ca40 --- /dev/null +++ b/src/main/java/fi/metatavu/keycloak/scim/server/ScimErrors.java @@ -0,0 +1,83 @@ +package fi.metatavu.keycloak.scim.server; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.node.ObjectNode; +import jakarta.ws.rs.core.Response; + +/** + * Helper for building SCIM 2.0 Error responses (RFC 7644 §3.12). + * + * Existing call sites returned plain-text bodies (e.g. "Unsupported group path", + * "Missing userName"), which broke clients that strictly parse error responses + * as JSON (Okta, Entra ID). All error responses go through this helper now and + * return a valid SCIM Error JSON document with the application/scim+json media + * type. + * + * The body is built via Jackson so any control character or quote that arrives + * in {@code detail} (typically from a user-supplied attribute path interpolated + * into an error message) is escaped correctly. A hand-rolled escape used to + * cover only `\\` and `"` and reintroduced the JSON parse failure on the client + * side as soon as a `\\n` or `\\t` reached `detail`. + */ +public final class ScimErrors { + + private static final ObjectMapper MAPPER = new ObjectMapper(); + private static final String ERROR_SCHEMA = "urn:ietf:params:scim:api:messages:2.0:Error"; + + private ScimErrors() { + // utility class + } + + /** + * Build a SCIM 2.0 Error response. + * + * @param status HTTP status (e.g. BAD_REQUEST) + * @param detail human-readable error detail; null is rendered as the empty string + * @return Response carrying a SCIM Error JSON body and application/scim+json type + */ + public static Response error(Response.Status status, String detail) { + ObjectNode node = MAPPER.createObjectNode(); + node.putArray("schemas").add(ERROR_SCHEMA); + node.put("status", Integer.toString(status.getStatusCode())); + node.put("detail", detail == null ? "" : detail); + String body; + try { + body = MAPPER.writeValueAsString(node); + } catch (JsonProcessingException e) { + // ObjectNode is always serializable; fall back to a static body if Jackson + // somehow fails so we still return SCIM-shaped JSON. + body = "{\"schemas\":[\"" + ERROR_SCHEMA + "\"],\"status\":\"" + + status.getStatusCode() + "\",\"detail\":\"\"}"; + } + return Response.status(status).type("application/scim+json").entity(body).build(); + } + + /** + * Convenience for HTTP 400 errors. + */ + public static Response badRequest(String detail) { + return error(Response.Status.BAD_REQUEST, detail); + } + + /** + * Convenience for HTTP 404 errors. + */ + public static Response notFound(String detail) { + return error(Response.Status.NOT_FOUND, detail); + } + + /** + * Convenience for HTTP 409 errors. + */ + public static Response conflict(String detail) { + return error(Response.Status.CONFLICT, detail); + } + + /** + * Convenience for HTTP 403 errors. + */ + public static Response forbidden(String detail) { + return error(Response.Status.FORBIDDEN, detail); + } +} diff --git a/src/main/java/fi/metatavu/keycloak/scim/server/ScimResources.java b/src/main/java/fi/metatavu/keycloak/scim/server/ScimResources.java index fffa0f5..da24c89 100644 --- a/src/main/java/fi/metatavu/keycloak/scim/server/ScimResources.java +++ b/src/main/java/fi/metatavu/keycloak/scim/server/ScimResources.java @@ -89,7 +89,7 @@ public Response listRealmUsers( scimFilter = parseFilter(filter); } catch (Exception e) { logger.warn(String.format("Failed to parse filter: '%s'", filter), e); - return Response.status(Response.Status.BAD_REQUEST).entity("Invalid filter").build(); + return ScimErrors.badRequest("Invalid filter"); } return realmScimServer.listUsers( @@ -213,7 +213,7 @@ public Response listRealmGroups( scimFilter = parseFilter(filter); } catch (Exception e) { logger.warn(String.format("Failed to parse filter: '%s'", filter), e); - return Response.status(Response.Status.BAD_REQUEST).entity("Invalid filter").build(); + return ScimErrors.badRequest("Invalid filter"); } return realmScimServer.listGroups( @@ -424,7 +424,7 @@ public Response listOrganizationUsers( scimFilter = parseFilter(filter); } catch (Exception e) { logger.warn(String.format("Failed to parse filter: '%s'", filter), e); - return Response.status(Response.Status.BAD_REQUEST).entity("Invalid filter").build(); + return ScimErrors.badRequest("Invalid filter"); } return getOrganizationScimServer().listUsers( @@ -554,7 +554,7 @@ public Response listOrganizationGroups( scimFilter = parseFilter(filter); } catch (Exception e) { logger.warn(String.format("Failed to parse filter: '%s'", filter), e); - return Response.status(Response.Status.BAD_REQUEST).entity("Invalid filter").build(); + return ScimErrors.badRequest("Invalid filter"); } return getOrganizationScimServer().listGroups( diff --git a/src/main/java/fi/metatavu/keycloak/scim/server/groups/GroupsController.java b/src/main/java/fi/metatavu/keycloak/scim/server/groups/GroupsController.java index 424c58c..40d9d0e 100644 --- a/src/main/java/fi/metatavu/keycloak/scim/server/groups/GroupsController.java +++ b/src/main/java/fi/metatavu/keycloak/scim/server/groups/GroupsController.java @@ -22,6 +22,7 @@ import org.keycloak.models.utils.ModelToRepresentation; import org.keycloak.representations.idm.GroupRepresentation; +import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -157,10 +158,7 @@ public fi.metatavu.keycloak.scim.server.model.Group patchGroup( ScimContext scimContext, GroupModel existing, fi.metatavu.keycloak.scim.server.model.PatchRequest patchRequest - ) throws UnsupportedGroupPath, UnsupportedPatchOperation { - KeycloakSession session = scimContext.getSession(); - RealmModel realm = scimContext.getRealm(); - + ) throws UnsupportedGroupPath, UnsupportedPatchOperation, InvalidGroupMemberReference { for (var operation : patchRequest.getOperations()) { PatchOperation op = PatchOperation.fromString(operation.getOp()); String path = operation.getPath(); @@ -171,11 +169,43 @@ public fi.metatavu.keycloak.scim.server.model.Group patchGroup( throw new UnsupportedPatchOperation("Unsupported patch operation: " + operation.getOp()); } + // RFC 7644 §3.5.2: when "path" is omitted, "value" carries a map of + // attribute -> value to apply to the resource. Okta's Group Push + // (add/remove members) emits this shape: + // {"op":"replace","value":{"members":[{"value":""}]}} + // Without this branch the code below would call findByScimPath(null), + // get null, and throw UnsupportedGroupPath, breaking Okta group pushes. + if (path == null) { + if (!(value instanceof Map valueMap)) { + throw new UnsupportedGroupPath("PatchOp without 'path' requires a map-valued 'value'"); + } + for (Map.Entry entry : valueMap.entrySet()) { + String attrPath = String.valueOf(entry.getKey()); + if (isReadOnlyOrStructural(attrPath)) { + // RFC 7644 §3.5.2 / §7.5: ignore read-only and + // structural attributes (id, meta, schemas) + // on PATCH. Okta echoes the resource id back inside + // 'value' on Group Push. + continue; + } + GroupAttribute attr = GroupAttribute.findByScimPath(attrPath); + if (attr == null) { + throw new UnsupportedGroupPath("Unsupported attribute: " + attrPath); + } + applyGroupPatch(scimContext, op, attr, attrPath, entry.getValue(), existing); + } + continue; + } + // Extract base attribute path (e.g., "members" from "members[value eq \"id\"]") - String attributePath = path != null && path.contains("[") + String attributePath = path.contains("[") ? path.substring(0, path.indexOf("[")) : path; + if (isReadOnlyOrStructural(attributePath)) { + continue; + } + GroupAttribute groupAttribute = GroupAttribute.findByScimPath(attributePath); if (groupAttribute == null) { throw new UnsupportedGroupPath("Unsupported patch path: " + path); @@ -187,75 +217,139 @@ public fi.metatavu.keycloak.scim.server.model.Group patchGroup( break; } - switch (op) { - case REPLACE, ADD -> { - switch (groupAttribute) { - case DISPLAY_NAME -> existing.setName((String) value); - case MEMBERS -> { - // Clear current members if REPLACE, just add if ADD - if (op == PatchOperation.REPLACE) { - session.users().getGroupMembersStream(realm, existing) - .forEach(user -> user.leaveGroup(existing)); - } - - for (Object obj : (List) value) { - if (!(obj instanceof Map memberMap)) { - logger.warn("Invalid member object: " + obj); - continue; - } - - String memberId = (String) memberMap.get("value"); - if (memberId == null) { - logger.warn("Member value missing: " + obj); - continue; - } - - UserModel user = scimContext.getSession().users().getUserById(scimContext.getRealm(), memberId); - if (user != null) { - user.joinGroup(existing); - dispatchGroupMembershipJoinEvent(scimContext, existing, user); - } - } + // For REMOVE with a path filter (e.g. members[value eq "id"]), extract + // the member ID from the filter and wrap it in list form so applyGroupPatch + // can handle it uniformly. + Object effectiveValue = value; + if (op == PatchOperation.REMOVE && groupAttribute == GroupAttribute.MEMBERS && path.contains("[")) { + String memberId = extractValueFromFilter(path); + if (memberId != null) { + effectiveValue = List.of(Map.of("value", memberId)); + } else { + throw new UnsupportedGroupPath("Unsupported members filter: " + path); + } + } + + applyGroupPatch(scimContext, op, groupAttribute, path, effectiveValue, existing); + } + + return translateGroup(scimContext, existing); + } + + /** + * Apply a single SCIM PatchOp on a Group. + * + *

Atomicity scope: per operation. If a PatchRequest contains multiple + * operations, each is applied independently in order. An earlier + * successful operation is NOT rolled back if a later one fails. + * + *

For MEMBERS modifications (ADD/REPLACE/REMOVE), every incoming member + * ID is resolved via {@link #resolveMembers} before any mutation. The + * first unresolved ID raises {@link InvalidGroupMemberReference}, so the + * group's membership stays intact for that operation. + * + *

Used by both the path-less and path-based PatchOp branches of + * {@link #patchGroup}. + */ + private void applyGroupPatch( + ScimContext scimContext, + PatchOperation op, + GroupAttribute attr, + String attrPath, + Object value, + GroupModel existing + ) throws InvalidGroupMemberReference, UnsupportedGroupPath { + KeycloakSession session = scimContext.getSession(); + RealmModel realm = scimContext.getRealm(); + + switch (op) { + case REPLACE, ADD -> { + switch (attr) { + case DISPLAY_NAME -> { + if (!(value instanceof String s)) { + throw new UnsupportedGroupPath("displayName requires a string value"); + } + existing.setName(s); + } + case MEMBERS -> { + List resolved = resolveMembers(session, realm, value); + if (op == PatchOperation.REPLACE) { + session.users().getGroupMembersStream(realm, existing) + .forEach(u -> { + u.leaveGroup(existing); + dispatchGroupMembershipLeaveEvent(scimContext, existing, u); + }); + } + for (UserModel u : resolved) { + u.joinGroup(existing); + dispatchGroupMembershipJoinEvent(scimContext, existing, u); } } } - - case REMOVE -> { - switch (groupAttribute) { - case DISPLAY_NAME -> existing.setName(null); - case MEMBERS -> { - // Handle path filter (e.g., "members[value eq \"user-id\"]") - if (path != null && path.contains("[")) { - String memberId = extractValueFromFilter(path); - if (memberId != null) { - UserModel user = session.users().getUserById(realm, memberId); - if (user != null) { - user.leaveGroup(existing); - dispatchGroupMembershipLeaveEvent(scimContext, existing, user); - } - } - } else if (value instanceof List list) { - // Handle direct value list - for (Object obj : list) { - if (obj instanceof Map memberMap) { - String memberId = (String) memberMap.get("value"); - if (memberId != null) { - UserModel user = session.users().getUserById(realm, memberId); - if (user != null) { - user.leaveGroup(existing); - dispatchGroupMembershipLeaveEvent(scimContext, existing, user); - } - } - } - } - } + } + case REMOVE -> { + switch (attr) { + case DISPLAY_NAME -> existing.setName(null); + case MEMBERS -> { + // REMOVE shares the strict resolution path with REPLACE/ADD: an unknown + // member id surfaces as 400 InvalidGroupMemberReference rather than a + // silent no-op. SCIM clients with stale state get an actionable error + // instead of believing the membership change went through. + List resolved = resolveMembers(session, realm, value); + for (UserModel u : resolved) { + u.leaveGroup(existing); + dispatchGroupMembershipLeaveEvent(scimContext, existing, u); } } } } } + } - return translateGroup(scimContext, existing); + /** + * Resolve every member-id-shaped entry in {@code value} into a UserModel, + * failing with {@link InvalidGroupMemberReference} on the first unknown ID + * before any mutation. Accepts a List of Maps each carrying a "value" key. + * Returns an empty list for any non-list input (tolerates null/empty values + * on REMOVE operations). + * + *

Atomicity scope: within a single operation only. Resolution runs in + * full before any group membership is modified, so a bad ID aborts the + * operation without partially applying changes. It does NOT span multiple + * operations in the same PatchRequest (see {@link #applyGroupPatch}). + * + *

REMOVE uses this same path: an unknown member ID returns 400 rather + * than silently no-oping, so SCIM clients with stale state receive an + * actionable error instead of a false success. + */ + private List resolveMembers( + KeycloakSession session, + RealmModel realm, + Object value + ) throws InvalidGroupMemberReference { + List out = new ArrayList<>(); + if (!(value instanceof List list)) { + return out; + } + for (Object obj : list) { + if (!(obj instanceof Map memberMap)) { + continue; + } + Object idObj = memberMap.get("value"); + if (!(idObj instanceof String rawMemberId)) { + continue; + } + String memberId = rawMemberId.strip(); + if (memberId.isEmpty()) { + continue; + } + UserModel user = session.users().getUserById(realm, memberId); + if (user == null) { + throw new InvalidGroupMemberReference(memberId); + } + out.add(user); + } + return out; } /** diff --git a/src/main/java/fi/metatavu/keycloak/scim/server/groups/InvalidGroupMemberReference.java b/src/main/java/fi/metatavu/keycloak/scim/server/groups/InvalidGroupMemberReference.java new file mode 100644 index 0000000..f83022b --- /dev/null +++ b/src/main/java/fi/metatavu/keycloak/scim/server/groups/InvalidGroupMemberReference.java @@ -0,0 +1,16 @@ +package fi.metatavu.keycloak.scim.server.groups; + +/** + * Thrown when a SCIM PATCH on a Group references one or more member IDs that + * do not resolve to a user in the realm. The entire patch is rejected without + * mutating membership, so the client receives an actionable 400 instead of an + * empty / truncated group with HTTP 200. + */ +public class InvalidGroupMemberReference extends Exception { + + private static final long serialVersionUID = 1L; + + public InvalidGroupMemberReference(String memberId) { + super("Unknown group member: " + memberId); + } +} diff --git a/src/main/java/fi/metatavu/keycloak/scim/server/metadata/BooleanUserAttribute.java b/src/main/java/fi/metatavu/keycloak/scim/server/metadata/BooleanUserAttribute.java index 190936f..a2bf3be 100644 --- a/src/main/java/fi/metatavu/keycloak/scim/server/metadata/BooleanUserAttribute.java +++ b/src/main/java/fi/metatavu/keycloak/scim/server/metadata/BooleanUserAttribute.java @@ -4,6 +4,7 @@ import org.keycloak.models.UserModel; import java.util.function.BiConsumer; +import java.util.function.Consumer; import java.util.function.Function; /** @@ -12,7 +13,7 @@ public class BooleanUserAttribute extends UserAttribute { /** - * Constructor + * Constructor without explicit remover. Defaults to {@code write(user, null)}. * * @param source source * @param sourceId source id @@ -27,5 +28,24 @@ public class BooleanUserAttribute extends UserAttribute { public BooleanUserAttribute(Source source, String sourceId, String scimPath, String description, SchemaAttribute.TypeEnum type, SchemaAttribute.MutabilityEnum mutability, SchemaAttribute.UniquenessEnum uniqueness, Function reader, BiConsumer writer) { super(source, sourceId, scimPath, description, type, mutability, uniqueness, reader, writer); } - + + /** + * Constructor with an explicit remover. Use when {@code write(user, null)} would be unsafe + * (e.g. boolean attributes backed by a primitive setter which would NPE on auto-unboxing null). + * + * @param source source + * @param sourceId source id + * @param scimPath SCIM path + * @param description description + * @param type type + * @param mutability mutability + * @param uniqueness uniqueness + * @param reader reader + * @param writer writer + * @param remover remover + */ + public BooleanUserAttribute(Source source, String sourceId, String scimPath, String description, SchemaAttribute.TypeEnum type, SchemaAttribute.MutabilityEnum mutability, SchemaAttribute.UniquenessEnum uniqueness, Function reader, BiConsumer writer, Consumer remover) { + super(source, sourceId, scimPath, description, type, mutability, uniqueness, reader, writer, remover); + } + } diff --git a/src/main/java/fi/metatavu/keycloak/scim/server/metadata/MetadataController.java b/src/main/java/fi/metatavu/keycloak/scim/server/metadata/MetadataController.java index 6ee21bf..6a3de0e 100644 --- a/src/main/java/fi/metatavu/keycloak/scim/server/metadata/MetadataController.java +++ b/src/main/java/fi/metatavu/keycloak/scim/server/metadata/MetadataController.java @@ -275,7 +275,8 @@ private List> getUserAttributeMappingList(ScimContext scimConte SchemaAttribute.MutabilityEnum.READWRITE, SchemaAttribute.UniquenessEnum.NONE, UserModel::isEnabled, - UserModel::setEnabled + UserModel::setEnabled, + user -> user.setEnabled(false) ) ); @@ -302,7 +303,8 @@ private List> getUserAttributeMappingList(ScimContext scimConte SchemaAttribute.MutabilityEnum.READWRITE, SchemaAttribute.UniquenessEnum.NONE, user -> user.getFirstAttribute(userProfileAttribute.getName()), - (user, value) -> user.setAttribute(userProfileAttribute.getName(), List.of(value)) + (user, value) -> user.setAttribute(userProfileAttribute.getName(), List.of(value)), + user -> user.removeAttribute(userProfileAttribute.getName()) )); } } diff --git a/src/main/java/fi/metatavu/keycloak/scim/server/metadata/StringUserAttribute.java b/src/main/java/fi/metatavu/keycloak/scim/server/metadata/StringUserAttribute.java index deedf44..7d65fec 100644 --- a/src/main/java/fi/metatavu/keycloak/scim/server/metadata/StringUserAttribute.java +++ b/src/main/java/fi/metatavu/keycloak/scim/server/metadata/StringUserAttribute.java @@ -4,6 +4,7 @@ import org.keycloak.models.UserModel; import java.util.function.BiConsumer; +import java.util.function.Consumer; import java.util.function.Function; /** @@ -12,7 +13,7 @@ public class StringUserAttribute extends UserAttribute { /** - * Constructor + * Constructor without explicit remover. Defaults to {@code write(user, null)}. * * @param source source * @param sourceId source id @@ -28,4 +29,22 @@ public StringUserAttribute(Source source, String sourceId, String scimPath, Stri super(source, sourceId, scimPath, description, type, mutability, uniqueness, reader, writer); } + /** + * Constructor with an explicit remover. + * + * @param source source + * @param sourceId source id + * @param scimPath SCIM path + * @param description description + * @param type type + * @param mutability mutability + * @param uniqueness uniqueness + * @param reader reader + * @param writer writer + * @param remover remover + */ + public StringUserAttribute(Source source, String sourceId, String scimPath, String description, SchemaAttribute.TypeEnum type, SchemaAttribute.MutabilityEnum mutability, SchemaAttribute.UniquenessEnum uniqueness, Function reader, BiConsumer writer, Consumer remover) { + super(source, sourceId, scimPath, description, type, mutability, uniqueness, reader, writer, remover); + } + } diff --git a/src/main/java/fi/metatavu/keycloak/scim/server/metadata/UserAttribute.java b/src/main/java/fi/metatavu/keycloak/scim/server/metadata/UserAttribute.java index 484bb38..4d43333 100644 --- a/src/main/java/fi/metatavu/keycloak/scim/server/metadata/UserAttribute.java +++ b/src/main/java/fi/metatavu/keycloak/scim/server/metadata/UserAttribute.java @@ -4,6 +4,7 @@ import org.keycloak.models.UserModel; import java.util.function.BiConsumer; +import java.util.function.Consumer; import java.util.function.Function; /** @@ -32,9 +33,10 @@ public enum Source { private final SchemaAttribute.UniquenessEnum uniqueness; private final Function reader; private final BiConsumer writer; + private final Consumer remover; /** - * Constructor + * Constructor without explicit remover. Defaults to {@code write(user, null)}. * * @param source attribute source * @param sourceId attribute source id @@ -56,6 +58,37 @@ public enum Source { SchemaAttribute.UniquenessEnum uniqueness, Function reader, BiConsumer writer + ) { + this(source, sourceId, scimPath, description, type, mutability, uniqueness, reader, writer, null); + } + + /** + * Constructor with an explicit remover. Use when {@code write(user, null)} would be unsafe + * (e.g. USER_PROFILE attributes backed by {@code user.setAttribute(name, List.of(value))} + * which throws NPE on {@code List.of(null)}). + * + * @param source attribute source + * @param sourceId attribute source id + * @param scimPath SCIM path + * @param description attribute description + * @param type attribute type + * @param mutability attribute mutability + * @param uniqueness attribute uniqueness + * @param reader attribute reader + * @param writer attribute writer + * @param remover attribute remover (nullable; falls back to {@code write(user, null)} when null) + */ + UserAttribute( + Source source, + String sourceId, + String scimPath, + String description, + SchemaAttribute.TypeEnum type, + SchemaAttribute.MutabilityEnum mutability, + SchemaAttribute.UniquenessEnum uniqueness, + Function reader, + BiConsumer writer, + Consumer remover ) { this.source = source; this.sourceId = sourceId; @@ -66,6 +99,7 @@ public enum Source { this.uniqueness = uniqueness; this.reader = reader; this.writer = writer; + this.remover = remover; } /** @@ -151,4 +185,21 @@ public void write(UserModel user, T value) { writer.accept(user, value); } + /** + * Removes this attribute from the user. + *

+ * Uses the explicit remover when one was provided at construction time; otherwise falls back + * to {@code write(user, null)}. USER_PROFILE attributes must supply an explicit remover + * because their writer uses {@code List.of(value)}, which throws NPE when value is null. + * + * @param user user + */ + public void clear(UserModel user) { + if (remover != null) { + remover.accept(user); + } else { + write(user, null); + } + } + } diff --git a/src/main/java/fi/metatavu/keycloak/scim/server/organization/OrganizationScimServer.java b/src/main/java/fi/metatavu/keycloak/scim/server/organization/OrganizationScimServer.java index 6865fa0..7b67682 100644 --- a/src/main/java/fi/metatavu/keycloak/scim/server/organization/OrganizationScimServer.java +++ b/src/main/java/fi/metatavu/keycloak/scim/server/organization/OrganizationScimServer.java @@ -1,6 +1,7 @@ package fi.metatavu.keycloak.scim.server.organization; import fi.metatavu.keycloak.scim.server.AbstractScimServer; +import fi.metatavu.keycloak.scim.server.ScimErrors; import fi.metatavu.keycloak.scim.server.config.ConfigurationError; import fi.metatavu.keycloak.scim.server.filter.ScimFilter; import fi.metatavu.keycloak.scim.server.jacoco.ExcludeFromJacocoGeneratedReport; @@ -36,12 +37,12 @@ public Response createUser(OrganizationScimContext scimContext, User createReque if (isBlank(createRequest.getUserName())) { logger.warn("Cannot create user: Missing userName"); - return Response.status(Response.Status.BAD_REQUEST).entity("Missing userName").build(); + return ScimErrors.badRequest("Missing userName"); } if (emailAsUsername && !isValidEmail(createRequest.getUserName())) { logger.warn("Cannot create user: Invalid email format for userName"); - return Response.status(Response.Status.BAD_REQUEST).entity("Invalid email format for userName").build(); + return ScimErrors.badRequest("Invalid email format for userName"); } UserAttributes userAttributes = metadataController.getUserAttributes(scimContext); @@ -68,19 +69,19 @@ public Response updateUser(OrganizationScimContext scimContext, String userId, f if (isBlank(username)) { logger.warn("Missing userName"); - return Response.status(Response.Status.BAD_REQUEST).entity("Missing userName").build(); + return ScimErrors.badRequest("Missing userName"); } if (emailAsUsername && !isValidEmail(updateRequest.getUserName())) { logger.warn("Cannot update user: Invalid email format for userName"); - return Response.status(Response.Status.BAD_REQUEST).entity("Invalid email format for userName").build(); + return ScimErrors.badRequest("Invalid email format for userName"); } if (emailAsUsername && updateRequest.getEmails() != null) { for (fi.metatavu.keycloak.scim.server.model.UserEmailsInner email : updateRequest.getEmails()) { if (!Objects.equals(email.getValue(), updateRequest.getUserName())) { logger.warn("Conflicting email and userName when emailAsUsername is enabled"); - return Response.status(Response.Status.BAD_REQUEST).entity("Username and email must match when emailAsUsername is enabled").build(); + return ScimErrors.badRequest("Username and email must match when emailAsUsername is enabled"); } } } @@ -89,7 +90,7 @@ public Response updateUser(OrganizationScimContext scimContext, String userId, f UserModel user = session.users().getUserById(realm, userId); if (user == null) { logger.warn(String.format("User not found: %s", userId)); - return Response.status(Response.Status.NOT_FOUND).entity("User not found").build(); + return ScimErrors.notFound("User not found"); } // Check if username is being changed to an already existing one @@ -102,7 +103,7 @@ public Response updateUser(OrganizationScimContext scimContext, String userId, f if (existing != null && !existing.getId().equals(userId)) { logger.warn(String.format("User name already taken: %s", updateRequest.getUserName())); - return Response.status(Response.Status.CONFLICT).entity("User name already taken").build(); + return ScimErrors.conflict("User name already taken"); } UserAttributes userAttributes = metadataController.getUserAttributes(scimContext); @@ -119,7 +120,7 @@ public Response patchUser(OrganizationScimContext scimContext, String userId, fi UserModel existing = session.users().getUserById(realm, userId); if (existing == null) { logger.warn(String.format("User not found: %s", userId)); - return Response.status(Response.Status.NOT_FOUND).entity("User not found").build(); + return ScimErrors.notFound("User not found"); } UserAttributes userAttributes = metadataController.getUserAttributes(scimContext); @@ -128,7 +129,7 @@ public Response patchUser(OrganizationScimContext scimContext, String userId, fi fi.metatavu.keycloak.scim.server.model.User result = organizationUserController.patchOrganizationUser(scimContext, userAttributes, existing, patchRequest); return Response.ok(result).build(); } catch (UnsupportedPatchOperation e) { - return Response.status(Response.Status.BAD_REQUEST).entity("Unsupported patch operation").build(); + return ScimErrors.badRequest("Unsupported patch operation"); } } @@ -173,7 +174,7 @@ public Response deleteUser(OrganizationScimContext scimContext, String userId) { RoleModel scimManagedRole = realm.getRole("scim-managed"); if (scimManagedRole != null && !user.hasRole(scimManagedRole)) { logger.warn(String.format("User is not SCIM-managed: %s", userId)); - return Response.status(Response.Status.FORBIDDEN).entity("User is not managed by SCIM").build(); + return ScimErrors.forbidden("User is not managed by SCIM"); } organizationUserController.deleteOrganizationUser(scimContext, user); diff --git a/src/main/java/fi/metatavu/keycloak/scim/server/organization/OrganizationUserController.java b/src/main/java/fi/metatavu/keycloak/scim/server/organization/OrganizationUserController.java index 3c3a131..6debf60 100644 --- a/src/main/java/fi/metatavu/keycloak/scim/server/organization/OrganizationUserController.java +++ b/src/main/java/fi/metatavu/keycloak/scim/server/organization/OrganizationUserController.java @@ -207,44 +207,7 @@ public fi.metatavu.keycloak.scim.server.model.User patchOrganizationUser( RealmModel realm = scimContext.getRealm(); ScimConfig config = scimContext.getConfig(); - for (var operation : patchRequest.getOperations()) { - PatchOperation op = PatchOperation.fromString(operation.getOp()); - if (op == null) { - logger.warn("Invalid patch operation: " + operation.getOp()); - throw new UnsupportedPatchOperation("Unsupported patch operation: " + operation.getOp()); - } - - UserAttribute userAttribute = userAttributes.findByScimPath(operation.getPath()); - Object value = operation.getValue(); - - if (userAttribute == null) { - throw new UnsupportedUserPath("Unsupported attribute: " + operation.getPath()); - } - - switch (op) { - case REPLACE, ADD -> { - switch (value) { - case null: - logger.warn("Value is null for patch operation: " + op); - break; - case String s when userAttribute instanceof StringUserAttribute: - ((StringUserAttribute) userAttribute).write(existing, s); - break; - case String s when userAttribute instanceof BooleanUserAttribute: - ((BooleanUserAttribute) userAttribute).write(existing, Boolean.parseBoolean(s)); - break; - case Boolean b when userAttribute instanceof BooleanUserAttribute: - ((BooleanUserAttribute) userAttribute).write(existing, b); - break; - default: - logger.warn("Unsupported value type for patch operation: " + value.getClass()); - break; - } - - } - case REMOVE -> userAttribute.write(existing, null); - } - } + applyPatchOperations(userAttributes, existing, patchRequest); fi.metatavu.keycloak.scim.server.model.User patchedUser = translateUser( scimContext, diff --git a/src/main/java/fi/metatavu/keycloak/scim/server/realm/RealmScimServer.java b/src/main/java/fi/metatavu/keycloak/scim/server/realm/RealmScimServer.java index 19d087c..38e9695 100644 --- a/src/main/java/fi/metatavu/keycloak/scim/server/realm/RealmScimServer.java +++ b/src/main/java/fi/metatavu/keycloak/scim/server/realm/RealmScimServer.java @@ -1,8 +1,10 @@ package fi.metatavu.keycloak.scim.server.realm; import fi.metatavu.keycloak.scim.server.AbstractScimServer; +import fi.metatavu.keycloak.scim.server.ScimErrors; import fi.metatavu.keycloak.scim.server.config.ConfigurationError; import fi.metatavu.keycloak.scim.server.filter.ScimFilter; +import fi.metatavu.keycloak.scim.server.groups.InvalidGroupMemberReference; import fi.metatavu.keycloak.scim.server.groups.UnsupportedGroupPath; import fi.metatavu.keycloak.scim.server.metadata.UserAttributes; import fi.metatavu.keycloak.scim.server.model.User; @@ -37,12 +39,12 @@ public Response createUser( if (isBlank(createRequest.getUserName())) { logger.warn("Cannot create user: Missing userName"); - return Response.status(Response.Status.BAD_REQUEST).entity("Missing userName").build(); + return ScimErrors.badRequest("Missing userName"); } UserModel existing = session.users().getUserByUsername(realm, createRequest.getUserName()); if (existing != null) { - return Response.status(Response.Status.CONFLICT).entity("User already exists").build(); + return ScimErrors.conflict("User already exists"); } UserAttributes userAttributes = metadataController.getUserAttributes(scimContext); @@ -68,19 +70,19 @@ public Response updateUser(RealmScimContext scimContext, String userId, fi.metat if (isBlank(updateRequest.getUserName())) { logger.warn("Missing userName"); - return Response.status(Response.Status.BAD_REQUEST).entity("Missing userName").build(); + return ScimErrors.badRequest("Missing userName"); } if (emailAsUsername && !isValidEmail(updateRequest.getUserName())) { logger.warn("Cannot update user: Invalid email format for userName"); - return Response.status(Response.Status.BAD_REQUEST).entity("Invalid email format for userName").build(); + return ScimErrors.badRequest("Invalid email format for userName"); } if (emailAsUsername && updateRequest.getEmails() != null) { for (fi.metatavu.keycloak.scim.server.model.UserEmailsInner email : updateRequest.getEmails()) { if (!Objects.equals(email.getValue(), updateRequest.getUserName())) { logger.warn("Conflicting email and userName when emailAsUsername is enabled"); - return Response.status(Response.Status.BAD_REQUEST).entity("Username and email must match when emailAsUsername is enabled").build(); + return ScimErrors.badRequest("Username and email must match when emailAsUsername is enabled"); } } } @@ -89,7 +91,7 @@ public Response updateUser(RealmScimContext scimContext, String userId, fi.metat UserModel user = session.users().getUserById(realm, userId); if (user == null) { logger.warn(String.format("User not found: %s", userId)); - return Response.status(Response.Status.NOT_FOUND).entity("User not found").build(); + return ScimErrors.notFound("User not found"); } // Check if username is being changed to an already existing one @@ -102,7 +104,7 @@ public Response updateUser(RealmScimContext scimContext, String userId, fi.metat if (existing != null && !existing.getId().equals(userId)) { logger.warn(String.format("User name already taken: %s", updateRequest.getUserName())); - return Response.status(Response.Status.CONFLICT).entity("User name already taken").build(); + return ScimErrors.conflict("User name already taken"); } UserAttributes userAttributes = metadataController.getUserAttributes(scimContext); @@ -119,7 +121,7 @@ public Response patchUser(RealmScimContext scimContext, String userId, fi.metata UserModel existing = session.users().getUserById(realm, userId); if (existing == null) { logger.warn(String.format("User not found: %s", userId)); - return Response.status(Response.Status.NOT_FOUND).entity("User not found").build(); + return ScimErrors.notFound("User not found"); } UserAttributes userAttributes = metadataController.getUserAttributes(scimContext); @@ -128,7 +130,7 @@ public Response patchUser(RealmScimContext scimContext, String userId, fi.metata fi.metatavu.keycloak.scim.server.model.User result = usersController.patchUser(scimContext, userAttributes, existing, patchRequest); return Response.ok(result).build(); } catch (UnsupportedPatchOperation e) { - return Response.status(Response.Status.BAD_REQUEST).entity("Unsupported patch operation").build(); + return ScimErrors.badRequest("Unsupported patch operation"); } } @@ -173,7 +175,7 @@ public Response deleteUser(RealmScimContext scimContext, String userId) { RoleModel scimManagedRole = realm.getRole("scim-managed"); if (scimManagedRole != null && !user.hasRole(scimManagedRole)) { logger.warn(String.format("User is not SCIM-managed: %s", userId)); - return Response.status(Response.Status.FORBIDDEN).entity("User is not managed by SCIM").build(); + return ScimErrors.forbidden("User is not managed by SCIM"); } usersController.deleteUser(scimContext, user); @@ -187,7 +189,7 @@ public Response createGroup(RealmScimContext scimContext, fi.metatavu.keycloak.s if (isBlank(createRequest.getDisplayName())) { logger.warn("Cannot create group: Missing displayName"); - return Response.status(Response.Status.BAD_REQUEST).entity("Missing displayName").build(); + return ScimErrors.badRequest("Missing displayName"); } fi.metatavu.keycloak.scim.server.model.Group created = groupsController.createGroup(scimContext, createRequest); @@ -225,7 +227,7 @@ public Response updateGroup(RealmScimContext scimContext, String id, fi.metatavu } if (!id.equals(existing.getId())) { - return Response.status(Response.Status.BAD_REQUEST).entity("Group ID mismatch").build(); + return ScimErrors.badRequest("Group ID mismatch"); } fi.metatavu.keycloak.scim.server.model.Group updated = groupsController.updateGroup( @@ -247,16 +249,18 @@ public Response patchGroup(RealmScimContext scimContext, String groupId, fi.meta } if (!groupId.equals(existing.getId())) { - return Response.status(Response.Status.BAD_REQUEST).entity("Group ID mismatch").build(); + return ScimErrors.badRequest("Group ID mismatch"); } try { fi.metatavu.keycloak.scim.server.model.Group updated = groupsController.patchGroup(scimContext, existing, patchRequest); return Response.ok(updated).build(); + } catch (InvalidGroupMemberReference e) { + return ScimErrors.badRequest(e.getMessage()); } catch (UnsupportedGroupPath e) { - return Response.status(Response.Status.BAD_REQUEST).entity("Unsupported group path").build(); + return ScimErrors.badRequest(e.getMessage() != null ? e.getMessage() : "Unsupported group path"); } catch (UnsupportedPatchOperation e) { - return Response.status(Response.Status.BAD_REQUEST).entity("Unsupported patch operation").build(); + return ScimErrors.badRequest("Unsupported patch operation"); } } diff --git a/src/main/java/fi/metatavu/keycloak/scim/server/users/UsersController.java b/src/main/java/fi/metatavu/keycloak/scim/server/users/UsersController.java index 97aa439..4ed2e1e 100644 --- a/src/main/java/fi/metatavu/keycloak/scim/server/users/UsersController.java +++ b/src/main/java/fi/metatavu/keycloak/scim/server/users/UsersController.java @@ -294,6 +294,41 @@ public fi.metatavu.keycloak.scim.server.model.User patchUser( UserAttributes userAttributes, UserModel existing, fi.metatavu.keycloak.scim.server.model.PatchRequest patchRequest + ) throws UnsupportedPatchOperation { + applyPatchOperations(userAttributes, existing, patchRequest); + + dispatchUserUpdateEvent(scimContext, existing); + + final User patchedUser = translateUser(scimContext, userAttributes, existing); + + if (scimContext.getConfig().getLinkIdp()) { + KeycloakSession session = scimContext.getSession(); + RealmModel realm = scimContext.getRealm(); + String scimUsername = patchedUser.getUserName(); + String externalId = getExternalId(patchedUser); + String idpAlias = scimContext.getConfig().getIdentityProviderAlias(); + linkUserIdp(session, realm, existing, scimUsername, externalId, idpAlias); + } + + + return patchedUser; + } + + /** + * Walk a PatchRequest's operations and apply each one to {@code existing}. + * Shared between {@link #patchUser} and + * {@link fi.metatavu.keycloak.scim.server.organization.OrganizationUserController#patchOrganizationUser} + * so the realm-scope and org-scope SCIM PATCH endpoints handle path-less / + * path-based shapes and read-only / structural attributes identically. + * + * @param userAttributes user attributes metadata + * @param existing user being patched + * @param patchRequest SCIM patch request + */ + protected void applyPatchOperations( + UserAttributes userAttributes, + UserModel existing, + fi.metatavu.keycloak.scim.server.model.PatchRequest patchRequest ) throws UnsupportedPatchOperation { for (var operation : patchRequest.getOperations()) { PatchOperation op = PatchOperation.fromString(operation.getOp()); @@ -314,6 +349,12 @@ public fi.metatavu.keycloak.scim.server.model.User patchUser( } for (Map.Entry entry : valueMap.entrySet()) { String attrPath = String.valueOf(entry.getKey()); + if (isReadOnlyOrStructural(attrPath)) { + // RFC 7644 §3.5.2 / §7.5: ignore read-only and + // structural attributes (id, meta, schemas) + // on PATCH. Clients (Okta) echo them back from a prior GET. + continue; + } UserAttribute ua = userAttributes.findByScimPath(attrPath); if (ua == null) { throw new UnsupportedUserPath("Unsupported attribute: " + attrPath); @@ -323,28 +364,16 @@ public fi.metatavu.keycloak.scim.server.model.User patchUser( continue; } + if (isReadOnlyOrStructural(path)) { + continue; + } + UserAttribute userAttribute = userAttributes.findByScimPath(path); if (userAttribute == null) { throw new UnsupportedUserPath("Unsupported attribute: " + path); } applyPatchValue(op, userAttribute, existing, value); } - - dispatchUserUpdateEvent(scimContext, existing); - - final User patchedUser = translateUser(scimContext, userAttributes, existing); - - if (scimContext.getConfig().getLinkIdp()) { - KeycloakSession session = scimContext.getSession(); - RealmModel realm = scimContext.getRealm(); - String scimUsername = patchedUser.getUserName(); - String externalId = getExternalId(patchedUser); - String idpAlias = scimContext.getConfig().getIdentityProviderAlias(); - linkUserIdp(session, realm, existing, scimUsername, externalId, idpAlias); - } - - - return patchedUser; } /** @@ -358,7 +387,7 @@ public fi.metatavu.keycloak.scim.server.model.User patchUser( * @param existing user being patched * @param value raw operation value */ - private void applyPatchValue( + protected void applyPatchValue( PatchOperation op, UserAttribute attr, UserModel existing, @@ -384,7 +413,7 @@ private void applyPatchValue( break; } } - case REMOVE -> attr.write(existing, null); + case REMOVE -> attr.clear(existing); } } diff --git a/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/OrganizationUserListTestsIT.java b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/OrganizationUserListTestsIT.java index 4c60e7e..d0890e5 100644 --- a/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/OrganizationUserListTestsIT.java +++ b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/OrganizationUserListTestsIT.java @@ -14,6 +14,7 @@ import java.util.ArrayList; import java.util.List; +import fi.metatavu.keycloak.scim.server.test.utils.ScimErrorAssertions; import static org.junit.jupiter.api.Assertions.*; /** @@ -228,7 +229,7 @@ void testInvalidFilterMissingOperator() { scimClient.listUsers("userName \"bob\"", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test @@ -239,7 +240,7 @@ void testInvalidFilterUnsupportedOperator() { scimClient.listUsers("userName gt \"bob\"", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test @@ -250,7 +251,7 @@ void testInvalidFilterUnquotedString() { scimClient.listUsers("userName eq bob", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test @@ -261,7 +262,7 @@ void testInvalidFilterBadLogicalStructure() { scimClient.listUsers("userName eq \"a\" and", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test diff --git a/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/OrganizationUserPatchTestsIT.java b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/OrganizationUserPatchTestsIT.java index 9a79662..547dd5c 100644 --- a/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/OrganizationUserPatchTestsIT.java +++ b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/OrganizationUserPatchTestsIT.java @@ -207,6 +207,37 @@ void testPatchAttributes() throws ApiException { deleteRealmUser(TestConsts.ORGANIZATIONS_REALM, created.getId()); } + /** + * Regression: SCIM REMOVE on a USER_PROFILE-backed attribute in org-scope previously threw NPE + * because applyOrgPatchValue called attr.write(user, null) -> List.of(null). After the fix both + * realm and org controllers share applyPatchValue which routes REMOVE through attr.clear(user). + */ + @Test + void testRemoveExternalIdDoesNotNpe() throws ApiException { + ScimClient scimClient = getAuthenticatedScimClient(TestConsts.ORGANIZATION_1_ID); + + User created = new User(); + created.setUserName("org-remove-extid-test"); + created.setActive(true); + created.putAdditionalProperty("externalId", "00uORGREMOVETEST"); + User u = scimClient.createUser(created); + + try { + PatchRequest patch = new PatchRequest(); + patch.setSchemas(List.of("urn:ietf:params:scim:api:messages:2.0:PatchOp")); + PatchRequestOperationsInner op = new PatchRequestOperationsInner(); + op.setOp("remove"); + op.setPath("externalId"); + patch.setOperations(List.of(op)); + + // Before this fix: applyOrgPatchValue called attr.write(user, null) -> NPE -> HTTP 500. + User after = scimClient.patchUser(u.getId(), patch); + assertNull(after.getAdditionalProperty("externalId")); + } finally { + deleteRealmUser(TestConsts.ORGANIZATIONS_REALM, u.getId()); + } + } + @Test void testPatchUsernameEmailAsUsername() throws ApiException { ScimClient scimClient = getAuthenticatedScimClient(TestConsts.ORGANIZATION_EMAIL_AS_USERNAME_ID); diff --git a/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmGroupListTestsIT.java b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmGroupListTestsIT.java index c7f72b2..2641535 100644 --- a/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmGroupListTestsIT.java +++ b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmGroupListTestsIT.java @@ -13,6 +13,7 @@ import java.util.Collections; import java.util.List; +import fi.metatavu.keycloak.scim.server.test.utils.ScimErrorAssertions; import static org.junit.jupiter.api.Assertions.*; /** @@ -200,7 +201,7 @@ void testInvalidFilterMissingOperator() throws ApiException { scimClient.listGroups("displayName \"test\"", 0, 10) ); - assertEquals("listGroups call failed with: 400 - Invalid filter", exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } finally { deleteGroup(scimClient, group.getId()); } @@ -218,7 +219,7 @@ void testInvalidFilterUnquotedString() throws ApiException { scimClient.listGroups("displayName eq test", 0, 10) ); - assertEquals("listGroups call failed with: 400 - Invalid filter", exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } finally { deleteGroup(scimClient, group.getId()); } @@ -236,7 +237,7 @@ void testInvalidFilterBadLogicalStructure() throws ApiException { scimClient.listGroups("displayName eq \"test\" and", 0, 10) ); - assertEquals("listGroups call failed with: 400 - Invalid filter", exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } finally { deleteGroup(scimClient, group.getId()); } diff --git a/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmGroupPatchTestsIT.java b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmGroupPatchTestsIT.java index 093b04e..4001f5f 100644 --- a/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmGroupPatchTestsIT.java +++ b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmGroupPatchTestsIT.java @@ -15,6 +15,7 @@ import java.io.IOException; import java.util.Collections; import java.util.List; +import java.util.Map; import static org.junit.jupiter.api.Assertions.*; @@ -210,6 +211,221 @@ void testRemoveGroupMemberAdminEvents() throws ApiException, IOException { deleteRealmGroup(TestConsts.TEST_REALM, group.getId()); } + /** + * A REPLACE operation that includes one valid and one unknown member ID must + * be rejected atomically: HTTP 400 with an error body naming the bad ID, and + * the group's original membership must be unchanged. + */ + @Test + void testReplaceMembersRejectsUnknownIdWithoutMutation() throws ApiException, IOException { + ScimClient scimClient = getAuthenticatedScimClient(); + + User user = createUser(scimClient, "atomic-1", "Atomic", "One"); + Group group = createGroup(scimClient, "atomic-group"); + + try { + // Seed with a known member + seedGroupWithMember(scimClient, group, user); + + // REPLACE with [known, unknown] -- must fail atomically with HTTP 400. + GroupMembersInner known = new GroupMembersInner(); + known.setValue(user.getId()); + GroupMembersInner unknown = new GroupMembersInner(); + unknown.setValue("00000000-0000-0000-0000-000000000000"); + PatchRequest replace = new PatchRequest(); + replace.setSchemas(List.of("urn:ietf:params:scim:api:messages:2.0:PatchOp")); + PatchRequestOperationsInner replaceOp = new PatchRequestOperationsInner(); + replaceOp.setOp("replace"); + replaceOp.setPath("members"); + replaceOp.setValue(List.of(known, unknown)); + replace.setOperations(List.of(replaceOp)); + + ApiException ex = assertThrows(ApiException.class, + () -> scimClient.patchGroup(group.getId(), replace)); + assertEquals(400, ex.getCode()); + + com.fasterxml.jackson.databind.JsonNode body = + new com.fasterxml.jackson.databind.ObjectMapper().readTree(ex.getResponseBody()); + assertEquals("400", body.get("status").asText()); + assertTrue(body.get("detail").asText().contains("00000000-0000-0000-0000-000000000000"), + "detail should name the unknown member id; got: " + body.get("detail").asText()); + + // Group state must be unchanged: original known member still present. + assertGroupHasOnlyMember(scimClient, group, user); + } finally { + deleteRealmUser(TestConsts.TEST_REALM, user.getId()); + deleteRealmGroup(TestConsts.TEST_REALM, group.getId()); + } + } + + /** + * Same atomicity guarantee for the path-less PatchOp shape (Okta Group Push): + * {"op":"replace","value":{"members":[...]}}. An unknown member ID must yield + * HTTP 400 without mutating the group. + */ + @Test + void testReplaceMembersPathLessRejectsUnknownIdWithoutMutation() throws ApiException, IOException { + ScimClient scimClient = getAuthenticatedScimClient(); + + User user = createUser(scimClient, "atomic-pathless-1", "Atomic", "Pathless"); + Group group = createGroup(scimClient, "atomic-pathless-group"); + + try { + // Seed with a known member via path-based ADD + seedGroupWithMember(scimClient, group, user); + + // Path-less REPLACE with one known + one unknown member. + // Shape: {"op":"replace","value":{"members":[{"value":""}, {"value":""}]}} + Map knownMap = Map.of("value", user.getId()); + Map unknownMap = Map.of("value", "00000000-0000-0000-0000-000000000000"); + PatchRequest replace = new PatchRequest(); + replace.setSchemas(List.of("urn:ietf:params:scim:api:messages:2.0:PatchOp")); + PatchRequestOperationsInner replaceOp = new PatchRequestOperationsInner(); + replaceOp.setOp("replace"); + // No path -- value is a map of attribute -> list, Okta Group Push shape + replaceOp.setValue(Map.of("members", List.of(knownMap, unknownMap))); + replace.setOperations(List.of(replaceOp)); + + ApiException ex = assertThrows(ApiException.class, + () -> scimClient.patchGroup(group.getId(), replace)); + assertEquals(400, ex.getCode()); + + com.fasterxml.jackson.databind.JsonNode body = + new com.fasterxml.jackson.databind.ObjectMapper().readTree(ex.getResponseBody()); + assertEquals("400", body.get("status").asText()); + assertTrue(body.get("detail").asText().contains("00000000-0000-0000-0000-000000000000"), + "detail should name the unknown member id; got: " + body.get("detail").asText()); + + // Group state must be unchanged: original known member still present. + assertGroupHasOnlyMember(scimClient, group, user); + } finally { + deleteRealmUser(TestConsts.TEST_REALM, user.getId()); + deleteRealmGroup(TestConsts.TEST_REALM, group.getId()); + } + } + + @Test + void testRemoveUnknownMemberIsRejected() throws ApiException, java.io.IOException { + ScimClient scimClient = getAuthenticatedScimClient(); + + User user = createUser(scimClient, "remove-unknown-1", "Remove", "Unknown"); + Group group = createGroup(scimClient, "remove-unknown-group"); + + try { + // Seed with the known member + seedGroupWithMember(scimClient, group, user); + + // REMOVE [unknown] should fail 400 atomically; the known member stays. + GroupMembersInner unknown = new GroupMembersInner(); + unknown.setValue("11111111-1111-1111-1111-111111111111"); + PatchRequest remove = new PatchRequest(); + remove.setSchemas(List.of("urn:ietf:params:scim:api:messages:2.0:PatchOp")); + PatchRequestOperationsInner removeOp = new PatchRequestOperationsInner(); + removeOp.setOp("remove"); + removeOp.setPath("members"); + removeOp.setValue(List.of(unknown)); + remove.setOperations(List.of(removeOp)); + + ApiException ex = assertThrows(ApiException.class, + () -> scimClient.patchGroup(group.getId(), remove)); + assertEquals(400, ex.getCode()); + com.fasterxml.jackson.databind.JsonNode body = + new com.fasterxml.jackson.databind.ObjectMapper().readTree(ex.getResponseBody()); + assertTrue(body.get("detail").asText().contains("11111111-1111-1111-1111-111111111111"), + "detail should name the unknown member id; got: " + body.get("detail").asText()); + + // Known member untouched + assertGroupHasOnlyMember(scimClient, group, user); + } finally { + deleteRealmUser(TestConsts.TEST_REALM, user.getId()); + deleteRealmGroup(TestConsts.TEST_REALM, group.getId()); + } + } + + /** + * REMOVE with an unquoted (malformed) filter value must return HTTP 400 + * with a SCIM error body, not silently succeed. + * Example malformed path: members[value eq abc] (no quotes around the id) + */ + @Test + void testRemoveMemberWithMalformedFilterReturns400() throws ApiException, IOException { + ScimClient scimClient = getAuthenticatedScimClient(); + + User user = createUser(scimClient, "malformed-filter-user", "Malformed", "Filter"); + Group group = createGroup(scimClient, "malformed-filter-group"); + + try { + // Seed with a known member so the group is non-empty + seedGroupWithMember(scimClient, group, user); + + // REMOVE with unquoted filter value -- must be rejected with 400. + PatchRequest remove = new PatchRequest(); + remove.setSchemas(List.of("urn:ietf:params:scim:api:messages:2.0:PatchOp")); + PatchRequestOperationsInner removeOp = new PatchRequestOperationsInner(); + removeOp.setOp("remove"); + // Intentionally malformed: value not quoted + removeOp.setPath("members[value eq " + user.getId() + "]"); + remove.setOperations(List.of(removeOp)); + + ApiException ex = assertThrows(ApiException.class, + () -> scimClient.patchGroup(group.getId(), remove)); + assertEquals(400, ex.getCode()); + + com.fasterxml.jackson.databind.JsonNode body = + new com.fasterxml.jackson.databind.ObjectMapper().readTree(ex.getResponseBody()); + assertEquals("400", body.get("status").asText()); + + // Group state must be unchanged: original member still present. + assertGroupHasOnlyMember(scimClient, group, user); + } finally { + deleteRealmUser(TestConsts.TEST_REALM, user.getId()); + deleteRealmGroup(TestConsts.TEST_REALM, group.getId()); + } + } + + /** + * Okta Group Push wire shape: path-less PatchOp where members are nested under the value map. + * Example body: {"op":"replace","value":{"members":[{"value":""}]}} + */ + @Test + void testAddMemberPathLessPatchOp() throws ApiException { + ScimClient scimClient = getAuthenticatedScimClient(); + + User user = createUser(scimClient, "pathless-add-1", "Pathless", "Add"); + Group group = createGroup(scimClient, "pathless-add-group"); + + try { + // Okta Group Push wire shape: no "path", members nested under the value map. + PatchRequest patchRequest = new PatchRequest(); + patchRequest.setSchemas(List.of("urn:ietf:params:scim:api:messages:2.0:PatchOp")); + + PatchRequestOperationsInner op = new PatchRequestOperationsInner(); + // Okta's Group Push uses op=replace (not add) for the path-less wire shape, + // even when populating an empty group for the first time. The members list + // inside `value` is the new authoritative set. + op.setOp("replace"); + op.setValue(Map.of("members", List.of(Map.of("value", user.getId())))); + + patchRequest.setOperations(List.of(op)); + + Group patched = scimClient.patchGroup(group.getId(), patchRequest); + + assertNotNull(patched); + assertNotNull(patched.getMembers()); + assertEquals(1, patched.getMembers().size()); + assertEquals(user.getId(), patched.getMembers().get(0).getValue()); + + // Verify via a fresh GET as well. + Group after = scimClient.findGroup(group.getId()); + assertNotNull(after.getMembers()); + assertEquals(1, after.getMembers().size()); + assertEquals(user.getId(), after.getMembers().get(0).getValue()); + } finally { + deleteRealmUser(TestConsts.TEST_REALM, user.getId()); + deleteRealmGroup(TestConsts.TEST_REALM, group.getId()); + } + } + /** * Regression: removing a group member whose email is null must not 500. * Map.of() rejects null values, so the admin event dispatch previously @@ -256,4 +472,36 @@ void testRemoveGroupMemberWithoutEmail() throws ApiException { deleteRealmUser(TestConsts.TEST_REALM, user.getId()); deleteRealmGroup(TestConsts.TEST_REALM, group.getId()); } + + // --- helpers shared by the atomic-resolution tests --- + + /** + * Seed a group with a single member via a path-based ADD members PatchOp. + * Mirrors what an upstream IdP would push to establish initial membership + * before the test exercises a subsequent REPLACE/REMOVE op. + */ + private void seedGroupWithMember(ScimClient scimClient, Group group, User user) throws ApiException { + PatchRequest seed = new PatchRequest(); + seed.setSchemas(List.of("urn:ietf:params:scim:api:messages:2.0:PatchOp")); + PatchRequestOperationsInner addOp = new PatchRequestOperationsInner(); + addOp.setOp("add"); + addOp.setPath("members"); + GroupMembersInner known = new GroupMembersInner(); + known.setValue(user.getId()); + addOp.setValue(List.of(known)); + seed.setOperations(List.of(addOp)); + scimClient.patchGroup(group.getId(), seed); + } + + /** + * Re-fetch the group and assert it has exactly one member, whose value + * matches the given user's id. Used to confirm membership is unchanged + * after a failed atomic PatchOp. + */ + private void assertGroupHasOnlyMember(ScimClient scimClient, Group group, User user) throws ApiException { + Group after = scimClient.findGroup(group.getId()); + assertNotNull(after.getMembers()); + assertEquals(1, after.getMembers().size()); + assertEquals(user.getId(), after.getMembers().get(0).getValue()); + } } diff --git a/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmUserListTestsIT.java b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmUserListTestsIT.java index 5f18358..acf5585 100644 --- a/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmUserListTestsIT.java +++ b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmUserListTestsIT.java @@ -12,6 +12,7 @@ import java.util.ArrayList; import java.util.List; +import fi.metatavu.keycloak.scim.server.test.utils.ScimErrorAssertions; import static org.junit.jupiter.api.Assertions.*; /** @@ -211,7 +212,7 @@ void testInvalidFilterMissingOperator() { scimClient.listUsers("userName \"bob\"", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test @@ -222,7 +223,7 @@ void testInvalidFilterUnsupportedOperator() { scimClient.listUsers("userName gt \"bob\"", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test @@ -233,7 +234,7 @@ void testInvalidFilterUnquotedString() { scimClient.listUsers("userName eq bob", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test @@ -244,7 +245,7 @@ void testInvalidFilterBadLogicalStructure() { scimClient.listUsers("userName eq \"a\" and", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test diff --git a/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmUserPatchTestsIT.java b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmUserPatchTestsIT.java index c2c4303..33a749c 100644 --- a/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmUserPatchTestsIT.java +++ b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmUserPatchTestsIT.java @@ -190,6 +190,37 @@ void testPatchUserAdminEvents() throws ApiException, IOException { deleteRealmUser(TestConsts.TEST_REALM, created.getId()); } + /** + * Regression: SCIM REMOVE on a USER_PROFILE-backed attribute (externalId, displayName, etc.) + * previously called attr.write(user, null) which passed null into List.of(value) and threw NPE, + * returning HTTP 500 instead of a clean removal. + */ + @Test + void testRemoveExternalIdDoesNotNpe() throws ApiException { + ScimClient scimClient = getAuthenticatedScimClient(); + + User created = new User(); + created.setUserName("remove-extid-test"); + created.setActive(true); + created.putAdditionalProperty("externalId", "00uREMOVETEST"); + User u = scimClient.createUser(created); + + try { + PatchRequest patch = new PatchRequest(); + patch.setSchemas(List.of("urn:ietf:params:scim:api:messages:2.0:PatchOp")); + PatchRequestOperationsInner op = new PatchRequestOperationsInner(); + op.setOp("remove"); + op.setPath("externalId"); + patch.setOperations(List.of(op)); + + // Before this fix: attr.write(user, null) -> List.of(null) -> NPE -> HTTP 500. + User after = scimClient.patchUser(u.getId(), patch); + assertNull(after.getAdditionalProperty("externalId")); + } finally { + deleteRealmUser(TestConsts.TEST_REALM, u.getId()); + } + } + /** * Okta's Deactivate User action emits a PATCH without a top-level "path", * carrying the attribute change inside a map-valued "value" (RFC 7644 diff --git a/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/unit/ScimErrorsTest.java b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/unit/ScimErrorsTest.java new file mode 100644 index 0000000..0289546 --- /dev/null +++ b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/unit/ScimErrorsTest.java @@ -0,0 +1,40 @@ +package fi.metatavu.keycloak.scim.server.test.tests.unit; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import fi.metatavu.keycloak.scim.server.ScimErrors; +import jakarta.ws.rs.core.Response; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +class ScimErrorsTest { + private static final ObjectMapper MAPPER = new ObjectMapper(); + + @Test + void encodesPlainDetail() throws Exception { + Response r = ScimErrors.badRequest("Unsupported attribute: members.value"); + assertEquals(400, r.getStatus()); + assertEquals("application/scim+json", r.getMediaType().toString()); + JsonNode body = MAPPER.readTree((String) r.getEntity()); + assertTrue(body.get("schemas").toString().contains("urn:ietf:params:scim:api:messages:2.0:Error")); + assertEquals("400", body.get("status").asText()); + assertEquals("Unsupported attribute: members.value", body.get("detail").asText()); + } + + @Test + void encodesDetailWithControlChars() throws Exception { + // Reviewer point #2: a newline / tab in detail must not produce malformed JSON. + Response r = ScimErrors.badRequest("Unsupported attribute: weird\n\tpath\"with\\quotes"); + JsonNode body = MAPPER.readTree((String) r.getEntity()); + assertEquals("Unsupported attribute: weird\n\tpath\"with\\quotes", body.get("detail").asText()); + } + + @Test + void nullDetailIsEmptyString() throws Exception { + Response r = ScimErrors.notFound(null); + JsonNode body = MAPPER.readTree((String) r.getEntity()); + assertEquals("", body.get("detail").asText()); + } +} diff --git a/src/test/java/fi/metatavu/keycloak/scim/server/test/utils/ScimErrorAssertions.java b/src/test/java/fi/metatavu/keycloak/scim/server/test/utils/ScimErrorAssertions.java new file mode 100644 index 0000000..d656a35 --- /dev/null +++ b/src/test/java/fi/metatavu/keycloak/scim/server/test/utils/ScimErrorAssertions.java @@ -0,0 +1,37 @@ +package fi.metatavu.keycloak.scim.server.test.utils; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import fi.metatavu.keycloak.scim.server.test.client.ApiException; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +public final class ScimErrorAssertions { + + private static final ObjectMapper MAPPER = new ObjectMapper(); + private static final String ERROR_SCHEMA = "urn:ietf:params:scim:api:messages:2.0:Error"; + + private ScimErrorAssertions() {} + + /** + * Assert the ApiException carries a SCIM 2.0 Error JSON body with the + * given HTTP status and a detail field that contains {@code detailSubstring}. + */ + public static void assertScimError(ApiException ex, int expectedStatus, String detailSubstring) { + assertEquals(expectedStatus, ex.getCode(), + "HTTP status mismatch; body=" + ex.getResponseBody()); + JsonNode body; + try { + body = MAPPER.readTree(ex.getResponseBody()); + } catch (Exception e) { + throw new AssertionError("ApiException body is not valid JSON: " + ex.getResponseBody(), e); + } + assertTrue(body.path("schemas").toString().contains(ERROR_SCHEMA), + "schemas should contain Error URN; got: " + body.path("schemas")); + assertEquals(Integer.toString(expectedStatus), body.path("status").asText(), + "SCIM Error status field mismatch; body=" + body); + assertTrue(body.path("detail").asText().contains(detailSubstring), + "detail should contain '" + detailSubstring + "'; got: " + body.path("detail")); + } +}