Handle path-less PatchOp on Groups + return valid JSON errors

[dnl555]

Summary

Continues #97 (path-less PatchOp on Users, Okta Deactivate User flow) by closing two related gaps that block Okta's Group Push:

1. GroupsController.patchGroup did not handle path-less PatchOp shapes (RFC 7644 §3.5.2). Okta's Group membership push emits {\"op\":\"replace\",\"value\":{\"members\":[...]}} without a path field, which previously hit findByScimPath(null) and threw UnsupportedGroupPath.

2. Multiple error sites returned plain-text bodies (e.g. Unsupported group path, Missing userName, Invalid filter). Strict SCIM clients (Okta, Entra ID) parse error responses as JSON and fail. Concretely Okta's Jackson parser surfaces:

   Unrecognized token 'Unsupported': was expecting (JSON String, Number, Array,
   Object or token 'null', 'true' or 'false') at line: 1, column: 12
   

   masking the actual SCIM error from the IdP.

Fixes #100, refs #95, #97.

Changes

• New ScimErrors helper builds RFC 7644 §3.12-compliant Error JSON with application/scim+json media type, plus convenience methods badRequest, notFound, conflict, forbidden.
• All error response sites in RealmScimServer, OrganizationScimServer, and the filter handlers in ScimResources route through it.
• GroupsController.patchGroup: when path is null, iterate the map-valued value and apply each entry as a sub-operation via a new private helper applyGroupPatch (handles DISPLAY_NAME and MEMBERS for ADD/REPLACE/REMOVE).
• OrganizationUserController.patchOrganizationUser: same path-less treatment as Handle path-less PatchOp shape (Okta Deactivate User) #97 brought from UsersController.

No new public API. No version bump in this PR (you manage releases).

Verification

Reproduced the customer-facing failure against a realm running v1.5.0:

curl -s -o /tmp/p -w 'HTTP %{http_code}\n' -X PATCH \\
  -H 'Authorization: Bearer ...' -H 'Content-Type: application/scim+json' \\
  -d '{\"schemas\":[\"urn:ietf:params:scim:api:messages:2.0:PatchOp\"],\"Operations\":[{\"op\":\"replace\",\"value\":{\"members\":[{\"value\":\"...\"}]}}]}' \\
  \"\$BASE/Groups/\$GID\"

Before: HTTP 400 with body Unsupported group path (plain text). Okta's parser fails.

After this PR: HTTP 200 with the updated Group SCIM JSON, members list reflects the change. Negative inputs return HTTP 4xx with a parseable SCIM Error JSON body and application/scim+json Content-Type.

Verified end-to-end with a real Okta tenant pushing user-to-group memberships.

Tests

I haven't added new tests in this PR; happy to add an integration test along the lines of RealmUserPatchTestsIT#testDeactivateUserPathLessPatchOp if you'd like one specifically for RealmGroupPatchTestsIT#testAddMemberPathLessPatchOp. Let me know.

[dnl555]

Just pushed a follow-up commit to this PR that fixes a related issue surfaced after testing this PR's path-less branch end-to-end with Okta:

The path-less branch unblocked the request, but Okta's Group Push payload also includes id (and sometimes meta, schemas) inside value (echoed back from a prior GET). Per RFC 7644 §3.5.2 and the SCIM core schema, these are read-only / structural and servers MUST silently ignore them on PATCH.

The new commit adds an isReadOnlyOrStructural(path) filter on UsersController, GroupsController, and OrganizationUserController, consulted in both path-less and path-based branches before attribute resolution. Verified end-to-end against an Okta tenant pushing user-to-group memberships.

Happy to split this into a separate PR if you'd prefer.

[nicolamacoir]

Reviewed the diff. The Okta Group Push fix and the move to RFC 7644 §3.12 error bodies are both the right calls; flagging three items I'd like to discuss before merge, plus a few smaller ones.

1. REPLACE on members wipes existing membership before resolving the incoming IDs, and silently skips unknown IDs

In applyGroupPatch the REPLACE branch clears all members via leaveGroup first, then iterates the incoming list with if (user != null) and silently no-ops on any unknown ID:

keycloak-scim-server/src/main/java/fi/metatavu/keycloak/scim/server/groups/GroupsController.java

Lines 317 to 341 in 199ce36

	}
	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);
	}
	}
	}
	}
	}
	}

This overlaps with #108 (open), which proposes rejecting unknown member IDs in the path-based branch for the same reason — a silent 200 with a truncated/empty member list is indistinguishable from success on the client side. Worth coordinating the two PRs so the same approach applies to both branches: resolve all member IDs first, throw InvalidGroupMemberReference (or equivalent) if any is unknown, and only then mutate membership. If point 4 below (refactoring the two MEMBERS blocks into one) is taken, both paths get the fix for free.

2. ScimErrors.error JSON escaping isn't safe for arbitrary detail strings

keycloak-scim-server/src/main/java/fi/metatavu/keycloak/scim/server/ScimErrors.java

Lines 28 to 34 in 199ce36

	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();

Hand-rolled escape covers \ and " only. detail is fed from e.getMessage() for UnsupportedGroupPath at RealmScimServer.java:258, and that message embeds the attribute path from the client PATCH body — "Unsupported attribute: " + attrPath. A \n or \t in attrPath produces malformed JSON, reintroducing the same Jackson parse failure on the client side that this PR is fixing. Jackson is already on the classpath; building the body via ObjectNode (or Map<String,Object> + ObjectMapper) would be safer than extending the manual escape table.

3. applyOrgPatchValue REMOVE → ua.write(existing, null) NPEs on custom profile attributes

keycloak-scim-server/src/main/java/fi/metatavu/keycloak/scim/server/organization/OrganizationUserController.java

Lines 298 to 301 in 199ce36

	}
	}
	case REMOVE -> ua.write(existing, null);
	}

The writer registered for USER_PROFILE-sourced attributes is (user, value) -> user.setAttribute(name, List.of(value)) (see MetadataController.java). List.of(null) throws NPE on Java 9+, so a SCIM REMOVE targeting e.g. externalId on an org-scope user surfaces as an unhandled 500. Same defect exists in UsersController.applyPatchValue and is pre-existing there, but since the new helper duplicates the pattern it's worth fixing in both at once — user.removeAttribute(name) (or setAttribute(name, Collections.emptyList())) for USER_PROFILE attributes.

4. SonarCloud duplication

The 3.6% new-code duplication that failed the quality gate is concentrated in two pairs:

• applyOrgPatchValue is nearly byte-identical to UsersController.applyPatchValue. OrganizationUserController extends UsersController — promoting applyPatchValue from private to protected lets you delete applyOrgPatchValue entirely.
• applyGroupPatch and the path-based MEMBERS switch inside patchGroup share the REPLACE/ADD/REMOVE × DISPLAY_NAME/MEMBERS matrix. Routing both branches through applyGroupPatch would cut the duplication and naturally unify the unknown-member-id handling from point 1.

Smaller stuff

• RealmScimServer.scimError (lines 264-267) is a private one-line alias for ScimErrors.error. 12 sites in the same file use ScimErrors.* directly; only 4 use the shim. Worth removing for consistency since there are no other callers.
• applyGroupPatch DISPLAY_NAME REPLACE/ADD silently no-ops when value isn't a String (line ~313), but the path-based branch hard-casts and throws CCE. Different behavior for the same operation — pick one (preferably reject with UnsupportedGroupPath).
• The weakened test assertions (e.g. RealmGroupListTestsIT.java:201 and siblings) drop from exact match to substring contains("Invalid filter") — necessary because the body shape changed, but they no longer assert the HTTP 400 status or the JSON detail field. Suggest asserting on the parsed body + status separately so a regression that swaps 400→500 or moves the message into a different field still fails.
• Re: tests — yes please on RealmGroupPatchTestsIT#testAddMemberPathLessPatchOp. The new path-less code currently has no coverage.

[dnl555]

Thanks for your comments, I will have some time to review that during the next week.

[sonarqubecloud]

Quality Gate passed

Issues
11 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
1.3% Duplication on New Code

See analysis details on SonarQube Cloud

[dnl555]

@nicolamacoir thanks for the careful review. Pushed a follow-up that addresses each item:

• Atomic group member resolution + path-based MEMBERS unification + DISPLAY_NAME consistency (Support additional properties #1, #4b, smaller item): resolveMembers collects every member id up front, throws a new InvalidGroupMemberReference on the first unknown one, and only then mutates membership. The path-based MEMBERS switch in patchGroup now routes through the same applyGroupPatch helper, so both shapes share the atomic-resolution path. DISPLAY_NAME REPLACE/ADD throws UnsupportedGroupPath for non-String values in both branches.

• Safe JSON encoding in ScimErrors (Feature 2 additional properties #2): rebuilt via Jackson ObjectMapper + ObjectNode. Unit test covers \n / \t / " / \ in detail.

• REMOVE NPE on USER_PROFILE attrs (Organization support #3): UserAttribute gained a clear(UserModel) method backed by a per-attribute Consumer<UserModel> remover. MetadataController registers user -> user.removeAttribute(name) for USER_PROFILE-sourced attributes; the BooleanUserAttribute (active) registers user -> user.setEnabled(false). Regression tests in both realm and org *PatchTestsIT.

• Duplication (#4a + #4b): promoted UsersController.applyPatchValue to protected and deleted OrganizationUserController.applyOrgPatchValue. Also extracted the operations-walking loop into UsersController.applyPatchOperations, so both patchUser and patchOrganizationUser share it. Sonar new-code duplication is now at 1.3%, gate passing.

• Smaller items:

  • RealmScimServer.scimError shim removed; the 5 call sites use ScimErrors.notFound / ScimErrors.badRequest directly.
  • The weakened contains("Invalid filter") assertions in the three list-test files are replaced with a new ScimErrorAssertions.assertScimError(ex, 400, "Invalid filter") helper that parses the body and asserts status + Error schema URN + detail substring separately.
  • Added testAddMemberPathLessPatchOp covering the Okta Group Push wire shape, plus path-based and path-less negative tests for the atomic-resolution behavior (testReplaceMembersRejectsUnknownIdWithoutMutation, testReplaceMembersPathLessRejectsUnknownIdWithoutMutation, testRemoveUnknownMemberIsRejected).

Heads-up on one deliberate extension: resolveMembers is also called from the REMOVE branch, so a REMOVE op with an unknown member id surfaces as 400 rather than a silent 200 no-op. Pinned with testRemoveUnknownMemberIsRejected and documented inline. Happy to relax it to lenient if you prefer the RFC §3.5.2 "delete is idempotent" reading.

Also merged in origin/develop so the branch is clean to merge.

Ready for another look when you have time.

[nicolamacoir]

LGTM! 🚀