Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zivisaiah wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
208 changes: 143 additions & 65 deletions src/main/java/fi/metatavu/keycloak/scim/server/groups/GroupsController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@
KeycloakSession session = scimContext.getSession();
RealmModel realm = scimContext.getRealm();

for (var operation : patchRequest.getOperations()) {

Check warning on line 164 in src/main/java/fi/metatavu/keycloak/scim/server/groups/GroupsController.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Reduce the total number of break and continue statements in this loop to use at most one. 


See more on https://sonarcloud.io/project/issues?id=Metatavu_keycloak-scim-server&issues=AZ5Theq4kDEmer1ny3kY&open=AZ5Theq4kDEmer1ny3kY&pullRequest=113
PatchOperation op = PatchOperation.fromString(operation.getOp());
String path = operation.getPath();
Object value = operation.getValue();
Expand All @@ -171,8 +171,31 @@
throw new UnsupportedPatchOperation("Unsupported patch operation: " + operation.getOp());
}

// RFC 7644 §3.5.2: when "path" is omitted, "value" carries a
// partial resource (map of attribute -> value) to apply to the
// target. Okta emits this for group metadata reconciliation:
// {"op":"replace","value":{"id":"...","displayName":"..."}}.
// Iterate the map and apply each known attribute; ignore
// read-only / unknown keys so spurious metadata fields like "id"
// don't fail the request.
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 attrName = String.valueOf(entry.getKey());
GroupAttribute attr = GroupAttribute.findByScimPath(attrName);
if (attr == null) {
logger.debugf("Ignoring unsupported group attribute in path-less PATCH: %s", attrName);
continue;
}
applyGroupPatchValue(scimContext, op, attr, null, existing, entry.getValue());
}
continue;
}

// Extract base attribute path (e.g., "members" from "members[value eq \"id\"]")
String attributePath = path != null && path.contains("[")
String attributePath = path.contains("[")
? path.substring(0, path.indexOf("["))
: path;

Expand All @@ -187,75 +210,130 @@
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);
}
}
}
}
}
applyGroupPatchValue(scimContext, op, groupAttribute, path, existing, value);
}

return translateGroup(scimContext, existing);
}

/**
* Applies a single PATCH operation to a group attribute. Shared by the
* path-based and path-less PATCH branches so the membership and
* displayName mutation logic stays in one place.
*/
private void applyGroupPatchValue(
ScimContext scimContext,
PatchOperation op,
GroupAttribute groupAttribute,
String path,
GroupModel existing,
Object value
) {
switch (op) {
case REPLACE, ADD -> applyAddOrReplace(scimContext, op, groupAttribute, existing, value);
case REMOVE -> applyRemove(scimContext, groupAttribute, path, existing, value);
}
}

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);
}
}
}
}
}
}
private void applyAddOrReplace(
ScimContext scimContext,
PatchOperation op,
GroupAttribute groupAttribute,
GroupModel existing,
Object value
) {
switch (groupAttribute) {
case DISPLAY_NAME -> existing.setName((String) value);
case MEMBERS -> addOrReplaceMembers(scimContext, op, existing, (List<?>) value);
}
}

private void addOrReplaceMembers(
ScimContext scimContext,
PatchOperation op,
GroupModel existing,
List<?> members
) {
KeycloakSession session = scimContext.getSession();
RealmModel realm = scimContext.getRealm();

if (op == PatchOperation.REPLACE) {
session.users().getGroupMembersStream(realm, existing)
.forEach(user -> user.leaveGroup(existing));
}

for (Object obj : members) {

Check warning on line 265 in src/main/java/fi/metatavu/keycloak/scim/server/groups/GroupsController.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Reduce the total number of break and continue statements in this loop to use at most one. 


See more on https://sonarcloud.io/project/issues?id=Metatavu_keycloak-scim-server&issues=AZ5fsC4yIwemmfnNyfjJ&open=AZ5fsC4yIwemmfnNyfjJ&pullRequest=113
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 = session.users().getUserById(realm, memberId);
if (user != null) {
user.joinGroup(existing);
dispatchGroupMembershipJoinEvent(scimContext, existing, user);
}
}
}

private void applyRemove(
ScimContext scimContext,
GroupAttribute groupAttribute,
String path,
GroupModel existing,
Object value
) {
switch (groupAttribute) {
case DISPLAY_NAME -> existing.setName(null);
case MEMBERS -> removeMembers(scimContext, path, existing, value);
}
}

private void removeMembers(ScimContext scimContext, String path, GroupModel existing, Object value) {
KeycloakSession session = scimContext.getSession();
RealmModel realm = scimContext.getRealm();

if (path != null && path.contains("[")) {
removeMemberByFilter(session, realm, scimContext, path, existing);
} else if (value instanceof List<?> list) {
removeMembersFromList(session, realm, scimContext, list, existing);
}
}

private void removeMemberByFilter(
KeycloakSession session, RealmModel realm, ScimContext scimContext,
String path, GroupModel existing
) {
String memberId = extractValueFromFilter(path);
if (memberId != null) {
UserModel user = session.users().getUserById(realm, memberId);
if (user != null) {
user.leaveGroup(existing);
dispatchGroupMembershipLeaveEvent(scimContext, existing, user);
}
}
}

private void removeMembersFromList(
KeycloakSession session, RealmModel realm, ScimContext scimContext,
List<?> list, GroupModel existing
) {
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);
}
}
}
}

return translateGroup(scimContext, existing);
}

/**
Expand Down
79 changes: 79 additions & 0 deletions ...t/java/fi/metatavu/keycloak/scim/server/test/tests/functional/RealmGroupPatchTestsIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,9 @@

import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import static org.junit.jupiter.api.Assertions.*;

Expand Down Expand Up @@ -256,4 +258,81 @@ void testRemoveGroupMemberWithoutEmail() throws ApiException {
deleteRealmUser(TestConsts.TEST_REALM, user.getId());
deleteRealmGroup(TestConsts.TEST_REALM, group.getId());
}

/**
* Regression test for path-less PATCH on Groups per RFC 7644 §3.5.2.
*
* Okta's SCIM client refreshes group metadata after each push by sending
* a PATCH where the Operation omits "path" and supplies a partial
* resource as "value":
*
* {"op":"replace","value":{"id":"...","displayName":"..."}}
*
* Before this fix, GroupsController#patchGroup rejected such Operations
* with `UnsupportedGroupPath`, which Okta interpreted as a group-push
* failure and stopped pushing memberships. The fix mirrors the
* path-less branch already present in UsersController#patchUser:
* iterate the value map, apply each known attribute, and ignore
* unknown / read-only keys (id, schemas, meta, externalId) without
* failing.
*/
@Test
void testPatchGroupWithoutPathReplacesDisplayName() throws ApiException {
ScimClient scimClient = getAuthenticatedScimClient();
Group group = createGroup(scimClient, "pathless-original-name");

PatchRequest patchRequest = new PatchRequest();
patchRequest.setSchemas(List.of("urn:ietf:params:scim:api:messages:2.0:PatchOp"));

PatchRequestOperationsInner operation = new PatchRequestOperationsInner();
operation.setOp("replace");
// path intentionally omitted to exercise the path-less branch.

Map<String, Object> partialResource = new HashMap<>();
partialResource.put("id", group.getId()); // read-only, must be ignored
partialResource.put("displayName", "pathless-updated-name"); // must be applied
operation.setValue(partialResource);

patchRequest.setOperations(List.of(operation));

Group patched = scimClient.patchGroup(group.getId(), patchRequest);

assertEquals("pathless-updated-name", patched.getDisplayName());

deleteRealmGroup(TestConsts.TEST_REALM, group.getId());
}

/**
* Path-less PATCH where the value map contains only attributes the
* server treats as read-only / unknown (id, schemas) must succeed
* silently — applying nothing — rather than throwing
* UnsupportedGroupPath. This mirrors Okta's idempotent metadata
* reconciliation calls where the server returns the same data Okta
* just pushed.
*/
@Test
void testPatchGroupWithoutPathIgnoresReadOnlyAttributes() throws ApiException {
ScimClient scimClient = getAuthenticatedScimClient();
Group group = createGroup(scimClient, "pathless-readonly-name");
String originalDisplayName = group.getDisplayName();

PatchRequest patchRequest = new PatchRequest();
patchRequest.setSchemas(List.of("urn:ietf:params:scim:api:messages:2.0:PatchOp"));

PatchRequestOperationsInner operation = new PatchRequestOperationsInner();
operation.setOp("replace");

Map<String, Object> partialResource = new HashMap<>();
partialResource.put("id", group.getId());
partialResource.put("schemas", List.of("urn:ietf:params:scim:schemas:core:2.0:Group"));
operation.setValue(partialResource);

patchRequest.setOperations(List.of(operation));

Group patched = scimClient.patchGroup(group.getId(), patchRequest);

assertEquals(originalDisplayName, patched.getDisplayName());

deleteRealmGroup(TestConsts.TEST_REALM, group.getId());
}
}