From 199ce36a714d9b7d649e829c305ceadff444fdf4 Mon Sep 17 00:00:00 2001 From: Danilo Acquaviva Date: Tue, 12 May 2026 14:17:44 +0100 Subject: [PATCH 1/9] Handle path-less PatchOp on Groups + return valid JSON errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three closely-related fixes that together let Okta-driven SCIM Group Push and User provisioning round-trip through the realm and organization-scoped endpoints. 1. Path-less PatchOp on Groups. RFC 7644 §3.5.2 says that when "path" is omitted on a PatchOp, "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":""}]}} The previous code called findByScimPath(null), got null, and threw UnsupportedGroupPath, breaking every Okta Group Push. A path-less branch in patchGroup expands the map into one logical operation per entry. 2. Valid JSON error bodies on every patch / create / update / delete path. Per RFC 7644 §3.12 a SCIM Service Provider error response is a JSON object with schemas, status, and detail. Several handlers were returning plain-text strings, which Okta surfaces in its System Log as a malformed JSON parsing error and which other SCIM clients reject before showing the operator anything useful. A new ScimErrors helper centralises the envelope and is wired into the Groups, Users, OrganizationUser, RealmScimServer, and OrganizationScimServer entry points. 3. Silently ignore read-only / structural attrs on PATCH. Per RFC 7644 §3.5.2 and the SCIM core schema (RFC 7643 §3.1), servers MUST not error on read-only / structural attributes appearing in PATCH payloads. Okta and other clients echo "id", "meta", and "schemas" inside the PatchOp value when it is constructed from a prior GET. isReadOnlyOrStructural lives on AbstractController and is consulted in both path-less and path-based branches before attribute resolution. externalId is intentionally NOT in the list per RFC 7643 §3.1 (client-settable). Test assertions in *ListTestsIT switched from assertEquals on the exact pre-change plain-text error string to assertTrue substring on "Invalid filter", so they remain correct regardless of whether the body is plain text or JSON. --- .../scim/server/AbstractController.java | 30 +++++ .../keycloak/scim/server/ScimErrors.java | 64 +++++++++++ .../keycloak/scim/server/ScimResources.java | 8 +- .../scim/server/groups/GroupsController.java | 105 ++++++++++++++++++ .../organization/OrganizationScimServer.java | 21 ++-- .../OrganizationUserController.java | 85 ++++++++++---- .../scim/server/realm/RealmScimServer.java | 36 +++--- .../scim/server/users/UsersController.java | 10 ++ .../OrganizationUserListTestsIT.java | 12 +- .../functional/RealmGroupListTestsIT.java | 9 +- .../functional/RealmUserListTestsIT.java | 12 +- 11 files changed, 328 insertions(+), 64 deletions(-) create mode 100644 src/main/java/fi/metatavu/keycloak/scim/server/ScimErrors.java 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..cc53dd0 --- /dev/null +++ b/src/main/java/fi/metatavu/keycloak/scim/server/ScimErrors.java @@ -0,0 +1,64 @@ +package fi.metatavu.keycloak.scim.server; + +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. + */ +public final class ScimErrors { + + 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 + * @return Response carrying a SCIM Error JSON body and application/scim+json type + */ + public static Response error(Response.Status status, String detail) { + String safeDetail = detail == null + ? "" + : detail.replace("\\", "\\\\").replace("\"", "\\\""); + String body = "{\"schemas\":[\"urn:ietf:params:scim:api:messages:2.0:Error\"]" + + ",\"status\":\"" + status.getStatusCode() + "\"" + + ",\"detail\":\"" + safeDetail + "\"}"; + 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 96adb9c..610c891 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 @@ -171,11 +171,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("[") ? 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); @@ -258,6 +290,79 @@ public fi.metatavu.keycloak.scim.server.model.Group patchGroup( return translateGroup(scimContext, existing); } + /** + * Apply a single attribute patch to the given group. + * + * Used by the path-less PatchOp branch of {@link #patchGroup} to expand + * each entry of the map-valued 'value' into one logical operation. + */ + private void applyGroupPatch( + ScimContext scimContext, + PatchOperation op, + GroupAttribute attr, + String attrPath, + Object value, + GroupModel existing + ) { + KeycloakSession session = scimContext.getSession(); + RealmModel realm = scimContext.getRealm(); + + switch (op) { + case REPLACE, ADD -> { + switch (attr) { + case DISPLAY_NAME -> { + if (value instanceof String s) { + existing.setName(s); + } + } + case MEMBERS -> { + if (op == PatchOperation.REPLACE) { + session.users().getGroupMembersStream(realm, existing) + .forEach(user -> user.leaveGroup(existing)); + } + if (value instanceof List list) { + for (Object obj : list) { + if (!(obj instanceof Map memberMap)) { + continue; + } + String memberId = (String) memberMap.get("value"); + if (memberId == null) { + continue; + } + UserModel user = session.users().getUserById(realm, memberId); + if (user != null) { + user.joinGroup(existing); + dispatchGroupMembershipJoinEvent(scimContext, existing, user); + } + } + } + } + } + } + case REMOVE -> { + switch (attr) { + case DISPLAY_NAME -> existing.setName(null); + case MEMBERS -> { + if (value instanceof List 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); + } + } + } + } + } + } + } + } + } + } + /** * Deletes a group * 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..e1b2dd7 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 @@ -214,36 +214,44 @@ public fi.metatavu.keycloak.scim.server.model.User patchOrganizationUser( throw new UnsupportedPatchOperation("Unsupported patch operation: " + operation.getOp()); } - UserAttribute userAttribute = userAttributes.findByScimPath(operation.getPath()); + String path = operation.getPath(); Object value = operation.getValue(); - if (userAttribute == null) { - throw new UnsupportedUserPath("Unsupported attribute: " + operation.getPath()); + // RFC 7644 §3.5.2: when "path" is omitted, "value" carries a map of + // attribute -> value to apply to the resource. Same shape Okta uses + // for Deactivate User on the realm scope; mirror the realm-scope + // handling here so org-scope users do not throw UnsupportedUserPath. + if (path == null) { + if (!(value instanceof java.util.Map valueMap)) { + throw new UnsupportedUserPath("PatchOp without 'path' requires a map-valued 'value'"); + } + for (java.util.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). + continue; + } + UserAttribute ua = userAttributes.findByScimPath(attrPath); + if (ua == null) { + throw new UnsupportedUserPath("Unsupported attribute: " + attrPath); + } + applyOrgPatchValue(op, ua, existing, entry.getValue()); + } + continue; } - 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; - } + if (isReadOnlyOrStructural(path)) { + continue; + } - } - case REMOVE -> userAttribute.write(existing, null); + UserAttribute userAttribute = userAttributes.findByScimPath(path); + + if (userAttribute == null) { + throw new UnsupportedUserPath("Unsupported attribute: " + path); } + + applyOrgPatchValue(op, userAttribute, existing, value); } fi.metatavu.keycloak.scim.server.model.User patchedUser = translateUser( @@ -264,6 +272,35 @@ public fi.metatavu.keycloak.scim.server.model.User patchOrganizationUser( return patchedUser; } + /** + * Apply a single attribute patch to the given org-scope user. + * Used by both the path-based and path-less branches of patchOrganizationUser. + */ + private void applyOrgPatchValue(PatchOperation op, UserAttribute ua, UserModel existing, Object value) { + switch (op) { + case REPLACE, ADD -> { + switch (value) { + case null: + logger.warn("Value is null for patch operation: " + op); + break; + case String s when ua instanceof StringUserAttribute: + ((StringUserAttribute) ua).write(existing, s); + break; + case String s when ua instanceof BooleanUserAttribute: + ((BooleanUserAttribute) ua).write(existing, Boolean.parseBoolean(s)); + break; + case Boolean b when ua instanceof BooleanUserAttribute: + ((BooleanUserAttribute) ua).write(existing, b); + break; + default: + logger.warn("Unsupported value type for patch operation: " + value.getClass()); + break; + } + } + case REMOVE -> ua.write(existing, null); + } + } + /** * Finds a user * 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..aa4870a 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,6 +1,7 @@ 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.UnsupportedGroupPath; @@ -37,12 +38,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 +69,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 +90,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 +103,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 +120,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 scimError(Response.Status.NOT_FOUND, "User not found"); } UserAttributes userAttributes = metadataController.getUserAttributes(scimContext); @@ -128,7 +129,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 scimError(Response.Status.BAD_REQUEST, "Unsupported patch operation"); } } @@ -173,7 +174,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 +188,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 +226,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,19 +248,24 @@ 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 scimError(Response.Status.BAD_REQUEST, "Group ID mismatch"); } try { fi.metatavu.keycloak.scim.server.model.Group updated = groupsController.patchGroup(scimContext, existing, patchRequest); return Response.ok(updated).build(); } catch (UnsupportedGroupPath e) { - return Response.status(Response.Status.BAD_REQUEST).entity("Unsupported group path").build(); + return scimError(Response.Status.BAD_REQUEST, e.getMessage() != null ? e.getMessage() : "Unsupported group path"); } catch (UnsupportedPatchOperation e) { - return Response.status(Response.Status.BAD_REQUEST).entity("Unsupported patch operation").build(); + return scimError(Response.Status.BAD_REQUEST, "Unsupported patch operation"); } } + /** Backwards-compat shim around {@link ScimErrors#error(Response.Status, String)}. */ + private static Response scimError(Response.Status status, String detail) { + return ScimErrors.error(status, detail); + } + @Override public Response deleteGroup(RealmScimContext scimContext, String id) { KeycloakSession session = scimContext.getSession(); 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..f8fdece 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 @@ -314,6 +314,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,6 +329,10 @@ 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); 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..f6e3bf4 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 @@ -228,7 +228,8 @@ void testInvalidFilterMissingOperator() { scimClient.listUsers("userName \"bob\"", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + assertTrue(exception.getMessage().contains("Invalid filter"), + "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); } @Test @@ -239,7 +240,8 @@ void testInvalidFilterUnsupportedOperator() { scimClient.listUsers("userName gt \"bob\"", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + assertTrue(exception.getMessage().contains("Invalid filter"), + "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); } @Test @@ -250,7 +252,8 @@ void testInvalidFilterUnquotedString() { scimClient.listUsers("userName eq bob", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + assertTrue(exception.getMessage().contains("Invalid filter"), + "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); } @Test @@ -261,7 +264,8 @@ void testInvalidFilterBadLogicalStructure() { scimClient.listUsers("userName eq \"a\" and", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + assertTrue(exception.getMessage().contains("Invalid filter"), + "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); } @Test 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..879536e 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 @@ -200,7 +200,8 @@ void testInvalidFilterMissingOperator() throws ApiException { scimClient.listGroups("displayName \"test\"", 0, 10) ); - assertEquals("listGroups call failed with: 400 - Invalid filter", exception.getMessage()); + assertTrue(exception.getMessage().contains("Invalid filter"), + "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); } finally { deleteGroup(scimClient, group.getId()); } @@ -218,7 +219,8 @@ void testInvalidFilterUnquotedString() throws ApiException { scimClient.listGroups("displayName eq test", 0, 10) ); - assertEquals("listGroups call failed with: 400 - Invalid filter", exception.getMessage()); + assertTrue(exception.getMessage().contains("Invalid filter"), + "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); } finally { deleteGroup(scimClient, group.getId()); } @@ -236,7 +238,8 @@ void testInvalidFilterBadLogicalStructure() throws ApiException { scimClient.listGroups("displayName eq \"test\" and", 0, 10) ); - assertEquals("listGroups call failed with: 400 - Invalid filter", exception.getMessage()); + assertTrue(exception.getMessage().contains("Invalid filter"), + "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); } finally { deleteGroup(scimClient, group.getId()); } 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..b350554 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 @@ -211,7 +211,8 @@ void testInvalidFilterMissingOperator() { scimClient.listUsers("userName \"bob\"", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + assertTrue(exception.getMessage().contains("Invalid filter"), + "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); } @Test @@ -222,7 +223,8 @@ void testInvalidFilterUnsupportedOperator() { scimClient.listUsers("userName gt \"bob\"", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + assertTrue(exception.getMessage().contains("Invalid filter"), + "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); } @Test @@ -233,7 +235,8 @@ void testInvalidFilterUnquotedString() { scimClient.listUsers("userName eq bob", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + assertTrue(exception.getMessage().contains("Invalid filter"), + "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); } @Test @@ -244,7 +247,8 @@ void testInvalidFilterBadLogicalStructure() { scimClient.listUsers("userName eq \"a\" and", 0, 10) ); - assertEquals("listUsers call failed with: 400 - Invalid filter", exception.getMessage()); + assertTrue(exception.getMessage().contains("Invalid filter"), + "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); } @Test From a34e3e0aa3c08f70aa34c34039c3a74446842f58 Mon Sep 17 00:00:00 2001 From: Danilo Acquaviva Date: Mon, 25 May 2026 17:24:38 +0100 Subject: [PATCH 2/9] Build SCIM Error JSON via Jackson so detail is safely escaped --- .../keycloak/scim/server/ScimErrors.java | 33 +++++++++++---- .../test/tests/unit/ScimErrorsTest.java | 40 +++++++++++++++++++ 2 files changed, 66 insertions(+), 7 deletions(-) create mode 100644 src/test/java/fi/metatavu/keycloak/scim/server/test/tests/unit/ScimErrorsTest.java diff --git a/src/main/java/fi/metatavu/keycloak/scim/server/ScimErrors.java b/src/main/java/fi/metatavu/keycloak/scim/server/ScimErrors.java index cc53dd0..6d6ca40 100644 --- a/src/main/java/fi/metatavu/keycloak/scim/server/ScimErrors.java +++ b/src/main/java/fi/metatavu/keycloak/scim/server/ScimErrors.java @@ -1,5 +1,8 @@ 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; /** @@ -10,9 +13,18 @@ * 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 } @@ -21,16 +33,23 @@ private ScimErrors() { * Build a SCIM 2.0 Error response. * * @param status HTTP status (e.g. BAD_REQUEST) - * @param detail human-readable error detail + * @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) { - String safeDetail = detail == null - ? "" - : detail.replace("\\", "\\\\").replace("\"", "\\\""); - String body = "{\"schemas\":[\"urn:ietf:params:scim:api:messages:2.0:Error\"]" - + ",\"status\":\"" + status.getStatusCode() + "\"" - + ",\"detail\":\"" + safeDetail + "\"}"; + 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(); } 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()); + } +} From 0e76edadf00efa9bf55a52cc768d22bade5453a6 Mon Sep 17 00:00:00 2001 From: Danilo Acquaviva Date: Mon, 25 May 2026 17:35:34 +0100 Subject: [PATCH 3/9] Inline RealmScimServer.scimError shim through ScimErrors directly --- .../scim/server/realm/RealmScimServer.java | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) 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 aa4870a..110343a 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 @@ -120,7 +120,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 scimError(Response.Status.NOT_FOUND, "User not found"); + return ScimErrors.notFound("User not found"); } UserAttributes userAttributes = metadataController.getUserAttributes(scimContext); @@ -129,7 +129,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 scimError(Response.Status.BAD_REQUEST, "Unsupported patch operation"); + return ScimErrors.badRequest("Unsupported patch operation"); } } @@ -248,24 +248,19 @@ public Response patchGroup(RealmScimContext scimContext, String groupId, fi.meta } if (!groupId.equals(existing.getId())) { - return scimError(Response.Status.BAD_REQUEST, "Group ID mismatch"); + 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 (UnsupportedGroupPath e) { - return scimError(Response.Status.BAD_REQUEST, e.getMessage() != null ? e.getMessage() : "Unsupported group path"); + return ScimErrors.badRequest(e.getMessage() != null ? e.getMessage() : "Unsupported group path"); } catch (UnsupportedPatchOperation e) { - return scimError(Response.Status.BAD_REQUEST, "Unsupported patch operation"); + return ScimErrors.badRequest("Unsupported patch operation"); } } - /** Backwards-compat shim around {@link ScimErrors#error(Response.Status, String)}. */ - private static Response scimError(Response.Status status, String detail) { - return ScimErrors.error(status, detail); - } - @Override public Response deleteGroup(RealmScimContext scimContext, String id) { KeycloakSession session = scimContext.getSession(); From b39aa18fcd973587a849886ac46fefadab04dc2c Mon Sep 17 00:00:00 2001 From: Danilo Acquaviva Date: Mon, 25 May 2026 17:51:59 +0100 Subject: [PATCH 4/9] Share applyPatchValue between realm + org controllers; fix REMOVE NPE on USER_PROFILE attrs Promote UsersController.applyPatchValue to protected and delete the near-identical OrganizationUserController.applyOrgPatchValue, routing both controllers through the single shared method. Add UserAttribute.clear(UserModel) with an optional Consumer remover registered at construction time. USER_PROFILE-backed attributes (externalId, displayName, etc.) register user -> user.removeAttribute(name) so REMOVE operations no longer pass null into List.of(value), which threw NPE on Java 9+ and returned HTTP 500. --- .../server/metadata/BooleanUserAttribute.java | 24 ++++++++- .../server/metadata/MetadataController.java | 6 ++- .../server/metadata/StringUserAttribute.java | 21 +++++++- .../scim/server/metadata/UserAttribute.java | 53 ++++++++++++++++++- .../OrganizationUserController.java | 33 +----------- .../scim/server/users/UsersController.java | 4 +- .../OrganizationUserPatchTestsIT.java | 31 +++++++++++ .../functional/RealmUserPatchTestsIT.java | 31 +++++++++++ 8 files changed, 164 insertions(+), 39 deletions(-) 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/OrganizationUserController.java b/src/main/java/fi/metatavu/keycloak/scim/server/organization/OrganizationUserController.java index e1b2dd7..5a0f470 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 @@ -236,7 +236,7 @@ public fi.metatavu.keycloak.scim.server.model.User patchOrganizationUser( if (ua == null) { throw new UnsupportedUserPath("Unsupported attribute: " + attrPath); } - applyOrgPatchValue(op, ua, existing, entry.getValue()); + applyPatchValue(op, ua, existing, entry.getValue()); } continue; } @@ -251,7 +251,7 @@ public fi.metatavu.keycloak.scim.server.model.User patchOrganizationUser( throw new UnsupportedUserPath("Unsupported attribute: " + path); } - applyOrgPatchValue(op, userAttribute, existing, value); + applyPatchValue(op, userAttribute, existing, value); } fi.metatavu.keycloak.scim.server.model.User patchedUser = translateUser( @@ -272,35 +272,6 @@ public fi.metatavu.keycloak.scim.server.model.User patchOrganizationUser( return patchedUser; } - /** - * Apply a single attribute patch to the given org-scope user. - * Used by both the path-based and path-less branches of patchOrganizationUser. - */ - private void applyOrgPatchValue(PatchOperation op, UserAttribute ua, UserModel existing, Object value) { - switch (op) { - case REPLACE, ADD -> { - switch (value) { - case null: - logger.warn("Value is null for patch operation: " + op); - break; - case String s when ua instanceof StringUserAttribute: - ((StringUserAttribute) ua).write(existing, s); - break; - case String s when ua instanceof BooleanUserAttribute: - ((BooleanUserAttribute) ua).write(existing, Boolean.parseBoolean(s)); - break; - case Boolean b when ua instanceof BooleanUserAttribute: - ((BooleanUserAttribute) ua).write(existing, b); - break; - default: - logger.warn("Unsupported value type for patch operation: " + value.getClass()); - break; - } - } - case REMOVE -> ua.write(existing, null); - } - } - /** * Finds a user * 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 f8fdece..06adc09 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 @@ -368,7 +368,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, @@ -394,7 +394,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/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/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 From 5d7adc0fe087948aa72e934a68daea7bc384dd5b Mon Sep 17 00:00:00 2001 From: Danilo Acquaviva Date: Mon, 25 May 2026 18:15:33 +0100 Subject: [PATCH 5/9] Unify group MEMBERS handling, resolve atomically, reject non-String displayName --- .../scim/server/groups/GroupsController.java | 197 ++++++++-------- .../groups/InvalidGroupMemberReference.java | 16 ++ .../scim/server/realm/RealmScimServer.java | 3 + .../functional/RealmGroupPatchTestsIT.java | 218 ++++++++++++++++++ 4 files changed, 330 insertions(+), 104 deletions(-) create mode 100644 src/main/java/fi/metatavu/keycloak/scim/server/groups/InvalidGroupMemberReference.java 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 610c891..e4dc04e 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(); @@ -200,7 +198,7 @@ public fi.metatavu.keycloak.scim.server.model.Group patchGroup( } // 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; @@ -219,82 +217,39 @@ 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); - } - } - } - } - } - - 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); - } - } - } - } - } - } - } + // 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 attribute patch to the given group. + * 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. * - * Used by the path-less PatchOp branch of {@link #patchGroup} to expand - * each entry of the map-valued 'value' into one logical operation. + *

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, @@ -303,7 +258,7 @@ private void applyGroupPatch( String attrPath, Object value, GroupModel existing - ) { + ) throws InvalidGroupMemberReference, UnsupportedGroupPath { KeycloakSession session = scimContext.getSession(); RealmModel realm = scimContext.getRealm(); @@ -311,30 +266,23 @@ private void applyGroupPatch( case REPLACE, ADD -> { switch (attr) { case DISPLAY_NAME -> { - if (value instanceof String s) { - existing.setName(s); + 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(user -> user.leaveGroup(existing)); + .forEach(u -> { + u.leaveGroup(existing); + dispatchGroupMembershipLeaveEvent(scimContext, existing, u); + }); } - if (value instanceof List list) { - for (Object obj : list) { - if (!(obj instanceof Map memberMap)) { - continue; - } - String memberId = (String) memberMap.get("value"); - if (memberId == null) { - continue; - } - UserModel user = session.users().getUserById(realm, memberId); - if (user != null) { - user.joinGroup(existing); - dispatchGroupMembershipJoinEvent(scimContext, existing, user); - } - } + for (UserModel u : resolved) { + u.joinGroup(existing); + dispatchGroupMembershipJoinEvent(scimContext, existing, u); } } } @@ -343,19 +291,14 @@ private void applyGroupPatch( switch (attr) { case DISPLAY_NAME -> existing.setName(null); case MEMBERS -> { - if (value instanceof List 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); - } - } - } - } + // 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); } } } @@ -363,6 +306,52 @@ private void applyGroupPatch( } } + /** + * 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; + } + /** * Deletes a group * 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/realm/RealmScimServer.java b/src/main/java/fi/metatavu/keycloak/scim/server/realm/RealmScimServer.java index 110343a..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 @@ -4,6 +4,7 @@ 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; @@ -254,6 +255,8 @@ public Response patchGroup(RealmScimContext scimContext, String groupId, fi.meta 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 ScimErrors.badRequest(e.getMessage() != null ? e.getMessage() : "Unsupported group path"); } catch (UnsupportedPatchOperation e) { 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 671533e..d86762b 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 @@ -14,6 +14,7 @@ import java.io.IOException; import java.util.Collections; import java.util.List; +import java.util.Map; import static org.junit.jupiter.api.Assertions.*; @@ -208,4 +209,221 @@ void testRemoveGroupMemberAdminEvents() throws ApiException, IOException { deleteRealmUser(TestConsts.TEST_REALM, user.getId()); 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 + GroupMembersInner known = new GroupMembersInner(); + known.setValue(user.getId()); + 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"); + addOp.setValue(List.of(known)); + seed.setOperations(List.of(addOp)); + scimClient.patchGroup(group.getId(), seed); + + // REPLACE with [known, unknown] -- must fail atomically with HTTP 400. + 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. + 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()); + } + } + + /** + * 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 + GroupMembersInner known = new GroupMembersInner(); + known.setValue(user.getId()); + 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"); + addOp.setValue(List.of(known)); + seed.setOperations(List.of(addOp)); + scimClient.patchGroup(group.getId(), seed); + + // 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. + 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()); + } + } + + @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 + GroupMembersInner known = new GroupMembersInner(); + known.setValue(user.getId()); + 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"); + addOp.setValue(List.of(known)); + seed.setOperations(List.of(addOp)); + scimClient.patchGroup(group.getId(), seed); + + // 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 + Group after = scimClient.findGroup(group.getId()); + 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()); + } + } + + /** + * 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 + GroupMembersInner member = new GroupMembersInner(); + member.setValue(user.getId()); + 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"); + addOp.setValue(List.of(member)); + seed.setOperations(List.of(addOp)); + scimClient.patchGroup(group.getId(), seed); + + // 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. + 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()); + } + } } From 673fac3dcf5d34f0cf7007a16a18a5e95018262e Mon Sep 17 00:00:00 2001 From: Danilo Acquaviva Date: Mon, 25 May 2026 18:42:02 +0100 Subject: [PATCH 6/9] Assert SCIM Error status + body structure in list-filter tests --- .../OrganizationUserListTestsIT.java | 13 +++---- .../functional/RealmGroupListTestsIT.java | 10 ++--- .../functional/RealmUserListTestsIT.java | 13 +++---- .../test/utils/ScimErrorAssertions.java | 37 +++++++++++++++++++ 4 files changed, 51 insertions(+), 22 deletions(-) create mode 100644 src/test/java/fi/metatavu/keycloak/scim/server/test/utils/ScimErrorAssertions.java 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 f6e3bf4..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,8 +229,7 @@ void testInvalidFilterMissingOperator() { scimClient.listUsers("userName \"bob\"", 0, 10) ); - assertTrue(exception.getMessage().contains("Invalid filter"), - "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test @@ -240,8 +240,7 @@ void testInvalidFilterUnsupportedOperator() { scimClient.listUsers("userName gt \"bob\"", 0, 10) ); - assertTrue(exception.getMessage().contains("Invalid filter"), - "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test @@ -252,8 +251,7 @@ void testInvalidFilterUnquotedString() { scimClient.listUsers("userName eq bob", 0, 10) ); - assertTrue(exception.getMessage().contains("Invalid filter"), - "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test @@ -264,8 +262,7 @@ void testInvalidFilterBadLogicalStructure() { scimClient.listUsers("userName eq \"a\" and", 0, 10) ); - assertTrue(exception.getMessage().contains("Invalid filter"), - "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test 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 879536e..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,8 +201,7 @@ void testInvalidFilterMissingOperator() throws ApiException { scimClient.listGroups("displayName \"test\"", 0, 10) ); - assertTrue(exception.getMessage().contains("Invalid filter"), - "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } finally { deleteGroup(scimClient, group.getId()); } @@ -219,8 +219,7 @@ void testInvalidFilterUnquotedString() throws ApiException { scimClient.listGroups("displayName eq test", 0, 10) ); - assertTrue(exception.getMessage().contains("Invalid filter"), - "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } finally { deleteGroup(scimClient, group.getId()); } @@ -238,8 +237,7 @@ void testInvalidFilterBadLogicalStructure() throws ApiException { scimClient.listGroups("displayName eq \"test\" and", 0, 10) ); - assertTrue(exception.getMessage().contains("Invalid filter"), - "Expected error message to contain 'Invalid filter'; got: " + 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/RealmUserListTestsIT.java b/src/test/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmUserListTestsIT.java index b350554..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,8 +212,7 @@ void testInvalidFilterMissingOperator() { scimClient.listUsers("userName \"bob\"", 0, 10) ); - assertTrue(exception.getMessage().contains("Invalid filter"), - "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test @@ -223,8 +223,7 @@ void testInvalidFilterUnsupportedOperator() { scimClient.listUsers("userName gt \"bob\"", 0, 10) ); - assertTrue(exception.getMessage().contains("Invalid filter"), - "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test @@ -235,8 +234,7 @@ void testInvalidFilterUnquotedString() { scimClient.listUsers("userName eq bob", 0, 10) ); - assertTrue(exception.getMessage().contains("Invalid filter"), - "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test @@ -247,8 +245,7 @@ void testInvalidFilterBadLogicalStructure() { scimClient.listUsers("userName eq \"a\" and", 0, 10) ); - assertTrue(exception.getMessage().contains("Invalid filter"), - "Expected error message to contain 'Invalid filter'; got: " + exception.getMessage()); + ScimErrorAssertions.assertScimError(exception, 400, "Invalid filter"); } @Test 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")); + } +} From c966217872d5146f6d96d4e921f47a0900f2e167 Mon Sep 17 00:00:00 2001 From: Danilo Acquaviva Date: Mon, 25 May 2026 18:57:18 +0100 Subject: [PATCH 7/9] Cover path-less Group PatchOp add-member with an integration test --- .../functional/RealmGroupPatchTestsIT.java | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) 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 d86762b..75087b5 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 @@ -426,4 +426,47 @@ void testRemoveMemberWithMalformedFilterReturns400() throws ApiException, IOExce 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()); + } + } } From 0cca738beb07b18007247b4187a663ce1af4087d Mon Sep 17 00:00:00 2001 From: Danilo Acquaviva Date: Mon, 25 May 2026 19:17:33 +0100 Subject: [PATCH 8/9] Extract seed + assert helpers in group-patch atomic tests --- .../functional/RealmGroupPatchTestsIT.java | 97 ++++++++----------- 1 file changed, 42 insertions(+), 55 deletions(-) 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 7267575..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 @@ -225,18 +225,11 @@ void testReplaceMembersRejectsUnknownIdWithoutMutation() throws ApiException, IO try { // Seed with a known member - GroupMembersInner known = new GroupMembersInner(); - known.setValue(user.getId()); - 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"); - addOp.setValue(List.of(known)); - seed.setOperations(List.of(addOp)); - scimClient.patchGroup(group.getId(), seed); + 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(); @@ -258,10 +251,7 @@ void testReplaceMembersRejectsUnknownIdWithoutMutation() throws ApiException, IO "detail should name the unknown member id; got: " + body.get("detail").asText()); // Group state must be unchanged: original known member still present. - Group after = scimClient.findGroup(group.getId()); - assertNotNull(after.getMembers()); - assertEquals(1, after.getMembers().size()); - assertEquals(user.getId(), after.getMembers().get(0).getValue()); + assertGroupHasOnlyMember(scimClient, group, user); } finally { deleteRealmUser(TestConsts.TEST_REALM, user.getId()); deleteRealmGroup(TestConsts.TEST_REALM, group.getId()); @@ -282,16 +272,7 @@ void testReplaceMembersPathLessRejectsUnknownIdWithoutMutation() throws ApiExcep try { // Seed with a known member via path-based ADD - GroupMembersInner known = new GroupMembersInner(); - known.setValue(user.getId()); - 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"); - addOp.setValue(List.of(known)); - seed.setOperations(List.of(addOp)); - scimClient.patchGroup(group.getId(), seed); + seedGroupWithMember(scimClient, group, user); // Path-less REPLACE with one known + one unknown member. // Shape: {"op":"replace","value":{"members":[{"value":""}, {"value":""}]}} @@ -316,10 +297,7 @@ void testReplaceMembersPathLessRejectsUnknownIdWithoutMutation() throws ApiExcep "detail should name the unknown member id; got: " + body.get("detail").asText()); // Group state must be unchanged: original known member still present. - Group after = scimClient.findGroup(group.getId()); - assertNotNull(after.getMembers()); - assertEquals(1, after.getMembers().size()); - assertEquals(user.getId(), after.getMembers().get(0).getValue()); + assertGroupHasOnlyMember(scimClient, group, user); } finally { deleteRealmUser(TestConsts.TEST_REALM, user.getId()); deleteRealmGroup(TestConsts.TEST_REALM, group.getId()); @@ -335,16 +313,7 @@ void testRemoveUnknownMemberIsRejected() throws ApiException, java.io.IOExceptio try { // Seed with the known member - GroupMembersInner known = new GroupMembersInner(); - known.setValue(user.getId()); - 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"); - addOp.setValue(List.of(known)); - seed.setOperations(List.of(addOp)); - scimClient.patchGroup(group.getId(), seed); + seedGroupWithMember(scimClient, group, user); // REMOVE [unknown] should fail 400 atomically; the known member stays. GroupMembersInner unknown = new GroupMembersInner(); @@ -366,9 +335,7 @@ void testRemoveUnknownMemberIsRejected() throws ApiException, java.io.IOExceptio "detail should name the unknown member id; got: " + body.get("detail").asText()); // Known member untouched - Group after = scimClient.findGroup(group.getId()); - assertEquals(1, after.getMembers().size()); - assertEquals(user.getId(), after.getMembers().get(0).getValue()); + assertGroupHasOnlyMember(scimClient, group, user); } finally { deleteRealmUser(TestConsts.TEST_REALM, user.getId()); deleteRealmGroup(TestConsts.TEST_REALM, group.getId()); @@ -389,16 +356,7 @@ void testRemoveMemberWithMalformedFilterReturns400() throws ApiException, IOExce try { // Seed with a known member so the group is non-empty - GroupMembersInner member = new GroupMembersInner(); - member.setValue(user.getId()); - 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"); - addOp.setValue(List.of(member)); - seed.setOperations(List.of(addOp)); - scimClient.patchGroup(group.getId(), seed); + seedGroupWithMember(scimClient, group, user); // REMOVE with unquoted filter value -- must be rejected with 400. PatchRequest remove = new PatchRequest(); @@ -418,10 +376,7 @@ void testRemoveMemberWithMalformedFilterReturns400() throws ApiException, IOExce assertEquals("400", body.get("status").asText()); // Group state must be unchanged: original member still present. - Group after = scimClient.findGroup(group.getId()); - assertNotNull(after.getMembers()); - assertEquals(1, after.getMembers().size()); - assertEquals(user.getId(), after.getMembers().get(0).getValue()); + assertGroupHasOnlyMember(scimClient, group, user); } finally { deleteRealmUser(TestConsts.TEST_REALM, user.getId()); deleteRealmGroup(TestConsts.TEST_REALM, group.getId()); @@ -517,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()); + } } From 936a5436134bce62ad203d309f77b1b120aecbb3 Mon Sep 17 00:00:00 2001 From: Danilo Acquaviva Date: Mon, 25 May 2026 19:27:54 +0100 Subject: [PATCH 9/9] Extract applyPatchOperations to share PATCH loop between realm + org controllers --- .../OrganizationUserController.java | 47 +---------------- .../scim/server/users/UsersController.java | 51 +++++++++++++------ 2 files changed, 36 insertions(+), 62 deletions(-) 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 5a0f470..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,52 +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()); - } - - String path = operation.getPath(); - Object value = operation.getValue(); - - // RFC 7644 §3.5.2: when "path" is omitted, "value" carries a map of - // attribute -> value to apply to the resource. Same shape Okta uses - // for Deactivate User on the realm scope; mirror the realm-scope - // handling here so org-scope users do not throw UnsupportedUserPath. - if (path == null) { - if (!(value instanceof java.util.Map valueMap)) { - throw new UnsupportedUserPath("PatchOp without 'path' requires a map-valued 'value'"); - } - for (java.util.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). - continue; - } - UserAttribute ua = userAttributes.findByScimPath(attrPath); - if (ua == null) { - throw new UnsupportedUserPath("Unsupported attribute: " + attrPath); - } - applyPatchValue(op, ua, existing, entry.getValue()); - } - continue; - } - - if (isReadOnlyOrStructural(path)) { - continue; - } - - UserAttribute userAttribute = userAttributes.findByScimPath(path); - - if (userAttribute == null) { - throw new UnsupportedUserPath("Unsupported attribute: " + path); - } - - applyPatchValue(op, userAttribute, existing, value); - } + 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/users/UsersController.java b/src/main/java/fi/metatavu/keycloak/scim/server/users/UsersController.java index 06adc09..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()); @@ -339,22 +374,6 @@ public fi.metatavu.keycloak.scim.server.model.User patchUser( } 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; } /**