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 424c58c..a4278f1 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,8 +171,31 @@ 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 + // 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; @@ -187,75 +210,130 @@ 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); - } - } - } - } - } + 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) { + 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); } /** 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 093b04e..6e8f93b 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,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.*; @@ -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 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 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()); + } }