fix: reject PATCH /Groups operations targeting nonexistent users#108
Open
nicolamacoir wants to merge 1 commit into
Open
fix: reject PATCH /Groups operations targeting nonexistent users#108nicolamacoir wants to merge 1 commit into
nicolamacoir wants to merge 1 commit into
Conversation
GroupsController.patchGroup silently no-op'd add/remove/replace operations whose member IDs did not resolve to a user — every site ran getUserById(...) followed by an "if (user != null)" that just dropped the request. SCIM clients then received HTTP 200 with the group unchanged, indistinguishable from a successful op. The replace variant additionally cleared the existing membership before the lookup loop, so a single bad ID in a replace silently wiped the group's members. - Introduce InvalidGroupMemberReference (checked exception) and throw it from every member lookup site instead of the silent null check. - For REPLACE, resolve every member ID up front and only swap membership if all references are valid — preserves existing members when one of the new IDs is bogus. - Map the new exception to HTTP 400 in RealmScimServer.patchGroup, matching how UnsupportedGroupPath / UnsupportedPatchOperation are already handled. Adds regression tests covering the add, remove (filter form), and replace cases, plus a specific assertion that REPLACE with a bad ID does not wipe existing members. Closes Metatavu#107
|
This was referenced May 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PATCH /Groupsadd/remove/replace operations against a nonexistent member ID silently returned HTTP 200, and thereplacevariant additionally wiped the existing membership before the (failing) lookup.InvalidGroupMemberReference(checked exception) and throws it from every member lookup site inGroupsController.patchGroup, replacing the silentif (user != null)no-op.REPLACE, all member IDs are now resolved up front; the existing membership is only cleared if every reference is valid. A bad ID inside aREPLACEno longer wipes the group.RealmScimServer.patchGroupcatches the new exception and maps it to HTTP 400, matching the existing handling ofUnsupportedGroupPath/UnsupportedPatchOperation.Changes
groups/InvalidGroupMemberReference.java— new checked exception carrying the offending member ID.groups/GroupsController.javapatchGroupthrowsInvalidGroupMemberReference.ADD/REPLACEresolve all member IDs up front (List<UserModel> resolved) and throw on any unresolved ID before mutating membership.REMOVE(filter form and value-list form) throws when the referenced member does not exist.realm/RealmScimServer.java— newcatch (InvalidGroupMemberReference e)branch returning400 Bad Requestwith the offending ID.RealmGroupPatchTestsIT— three new regression tests:testAddNonexistentGroupMemberReturnsBadRequesttestRemoveNonexistentGroupMemberReturnsBadRequesttestReplaceMembersWithNonexistentUserDoesNotWipeMembership— seeds a real member, sendsreplacewith a bad ID, asserts 400 and that the original member is still in the group.The organization-scope
OrganizationScimServer.patchGroupcurrently returns501 Not Implemented, so no parallel change is needed there.Test plan
./gradlew test --tests "fi.metatavu.keycloak.scim.server.test.tests.functional.RealmGroupPatchTestsIT"passes (existing happy-path tests + three new regressions)replacewith a bogus ID leaves existing membership intactNotes
Returns plain-text
"Invalid group member reference: <id>"to match the existing pattern inRealmScimServer.patchGroup. If you'd prefer a structured SCIM error body ({"schemas":[...], "status":"400", "scimType":"invalidValue", "detail":"..."}), happy to switch over in this PR or in a follow-up that uniformly upgrades the error responses.