fix(groups): accept path-less PATCH Operations per RFC 7644 §3.5.2

[zivisaiah]

Summary

RFC 7644 §3.5.2.1 / §3.5.2.3 allow PATCH Operations to omit path — in which case value is a partial resource that should be merged into the target. UsersController.patchUser already handles this branch; the Group equivalent did not, returning 400 Unsupported group path for any path-less Operation.

Okta's SCIM client uses this exact shape for group metadata reconciliation immediately after each create:

{
  "Operations": [{
    "op": "replace",
    "value": {"id": "...", "displayName": "..."}
  }]
}

The 400 caused Okta to mark the entire group push as Error and stop pushing group memberships — breaking provisioning end-to-end against the OAuth Bearer Token variant of Okta's SCIM 2.0 Test App.

Change

GroupsController.patchGroup now handles path-less Operations the same way UsersController.patchUser does:

• if path is null and value is a Map, iterate entries and apply each known attribute
• read-only / unknown attributes (id, schemas, meta, externalId, …) are skipped with a debug log rather than throwing, matching the RFC's "extend gracefully" tone and matching Okta's real payloads
• the existing path-based switch is refactored into a private applyGroupPatchValue helper so both branches share it

Tests

Two new regression tests in RealmGroupPatchTestsIT:

• testPatchGroupWithoutPathReplacesDisplayName — happy path. Sends {op: replace, value: {id, displayName}} and verifies the new displayName lands while id is silently ignored.
• testPatchGroupWithoutPathIgnoresReadOnlyAttributes — edge case. Sends {op: replace, value: {id, schemas}} (no mutable attributes) and verifies the call succeeds without throwing.

Validation

Validated end-to-end against a live Okta SCIM 2.0 Test App (OAuth Bearer Token). After this fix, Okta proceeds past metadata reconciliation and pushes group memberships via PATCH op=add path=members value=[...], which the existing handler accepts cleanly. Full validation evidence is documented in the upstream consumer's spike findings.

Related

Builds on #112 (member reconciliation on PUT + UserCache eviction). Both PRs are required for Okta SCIM 2.0 Test App (OAuth Bearer Token) compatibility against Keycloak 26.6.

Draft until #112 lands or until a maintainer signals they'd like to land them together.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

[nicolamacoir]

Thanks @zivisaiah for the careful Okta-against-live-tenant validation write-up — that level of evidence is genuinely useful. Heads-up that develop has moved underneath this PR; please see below.

Overview

The PR teaches GroupsController.patchGroup to handle SCIM PatchOps that omit path and carry a partial-resource Map in value — the shape Okta emits during group-metadata reconciliation. It also refactors the previously deeply nested switch (op) → switch (attr) → … block into six small helpers (applyGroupPatchValue, applyAddOrReplace, addOrReplaceMembers, applyRemove, removeMembers, removeMemberByFilter, removeMembersFromList). Two new integration tests exercise the path-less branch.

🚨 Critical: Already on develop — PR is superseded

develop already implements path-less PATCH handling on Groups. This is why the PR is currently mergeStateStatus: DIRTY / CONFLICTING.

• Commit 199ce36 (merged as Handle path-less PatchOp on Groups + return valid JSON errors #101, Handle path-less PatchOp on Groups + return valid JSON errors) added the same if (path == null) { … } branch.
• Commit 5d7adc0 (Unify group MEMBERS handling, resolve atomically …) then refactored the controller and added atomicity guarantees.
• Commit 936a543 further extracted applyPatchOperations to share the PATCH loop between realm + org SCIM servers.

A side-by-side comparison of the path-less branches:

Concern	This PR	develop (via #101)
Unknown attribute key	debug log + skip silently	Throws UnsupportedGroupPath
Read-only / structural keys (id, meta, schemas)	Skipped (same path as unknown)	Explicit isReadOnlyOrStructural(attrPath) allow-list, then skip; unknown attrs still throw
Atomicity of member resolution	None — bad ID silently dropped, REPLACE wipes membership before lookup	All member IDs resolved up front via resolveMembers; bad ID → HTTP 400, group untouched

develop's version is strictly safer. The "ignore everything we don't recognize" stance in this PR is looser than the RFC needs and would mask client bugs. If this PR landed over the existing implementation, it would lose the atomicity guarantees on the MEMBERS path.

Recommendation: rebase onto current develop, observe that the path-less branch is already there, then either close the PR as superseded or extract just the additive value (see Test coverage below) into a small follow-up PR.

Code correctness (assuming hypothetical merge against the pre-#101 base)

• applyAddOrReplace casts value to List<?> unconditionally at the MEMBERS arm (addOrReplaceMembers(..., (List<?>) value)). A non-list value for ADD/REPLACE members raises ClassCastException propagated as 500. develop's version uses instanceof pattern matching. Minor robustness gap.
• String memberId = (String) memberMap.get("value") — same fragility carried over from the pre-existing code: a non-String value cell yields 500 rather than 400. Not a regression, but worth fixing while you're in there.
• String.valueOf(entry.getKey()) returns "null" for a null key (unlikely for JSON-derived maps, but worth a defensive null skip).
• path.contains("[") after the new if (path == null) … block is correct — the null check is now exhaustive, so the prior path != null && path.contains("[") simplification is right.
• if (value == null && op != PatchOperation.REMOVE) { … break; } uses break rather than continue, silently swallowing the rest of the PatchRequest's operations after the first null value. Pre-existing, not introduced by this PR, but worth flagging while the surrounding code is being touched.

Refactor quality

• The 6-method split is consistent and the names read well; removeMemberByFilter / removeMembersFromList parallel structure is clear.
• Repeated (KeycloakSession session, RealmModel realm, ScimContext scimContext, …) signatures are slightly noisy — ScimContext already carries both. Could be (ScimContext scimContext, …) and let helpers pull the session/realm themselves, matching the rest of the controller's style.
• The "reduce cognitive complexity" justification in the commit body is worth surfacing in the PR description — reviewers reading the PR alone don't see it.

Test coverage

The two new tests are well-shaped:

• testPatchGroupWithoutPathReplacesDisplayName — asserts displayName is applied and the id key in the same map is silently ignored. Good combined coverage.
• testPatchGroupWithoutPathIgnoresReadOnlyAttributes — asserts a no-op call (only read-only keys) succeeds without throwing and without mutating state.

Gap vs. develop: develop's tests cover the more interesting Okta wire shape — path-less PatchOp where value nests a members list (testReplaceMembersPathLessRejectsUnknownIdWithoutMutation, testAddMemberPathLessPatchOp). The new tests here don't exercise that nested-members shape, which is the actually-load-bearing path for Okta group push (metadata-only reconciliation is the easier case).

develop is missing an explicit test for the "path-less PATCH with displayName" happy path, though. If you wanted to salvage something from this PR, testPatchGroupWithoutPathReplacesDisplayName is a small, focused, and genuinely additive contribution.

Security / performance

• No new persistence, no new auth surface — same risk profile as the existing handler.
• getUserById per member is unchanged from the original loop; develop's resolveMembers doesn't fix this either, so no regression.
• Silently swallowing unknown attribute keys is slightly worse for operability than develop's "throw on unknown" stance: misconfigured clients get no feedback that an attribute they think they're updating is being dropped on the floor.

Style nits

• Test file imports HashMap and Map — both used.
• Comment block in the new branch is excellent — RFC reference, concrete Okta example, and rationale. Matches the project's existing comment style.
• The String memberId = (String) memberMap.get("value"); chain inside removeMembersFromList is nearly identical to the one in addOrReplaceMembers. Worth pulling into a small extractMemberId(Map) if you continue the refactor.

Bottom line

Requesting changes because the fix is already on develop via #101 and develop's implementation is stricter and atomic. Suggested paths forward:

1. Close this PR as superseded — the production behavior you're after is already shipped.
2. Or, rebase onto current develop, observe the overlap, and re-scope to just the missing happy-path test (testPatchGroupWithoutPathReplacesDisplayName) as a small additive contribution.

The refactor itself is reasonable but competes with the refactor already merged via #101 — relitigating that is unlikely to be productive.