From 423b5dd6e8a12268a8c68f3aef00a10bf0739bb9 Mon Sep 17 00:00:00 2001 From: milan Date: Wed, 29 Jul 2026 17:00:42 +0000 Subject: [PATCH] fix(scim): honor Okta filtered member remove on PATCH /Groups Okta removes a single group member with a filtered path and no value body (members[value eq ""]). _process_group_patch_operations only read op.value and seeded its member baseline from the denormalized team.members column, so the targeted remove was a no-op (user kept on the team) or, when that column was empty, wiped the whole group. Parse the member id from the path filter and seed from the real membership so exactly the targeted member is removed Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../management_endpoints/scim/scim_v2.py | 65 +++++++++-------- .../scim/test_scim_v2_endpoints.py | 73 +++++++++++++++++++ 2 files changed, 108 insertions(+), 30 deletions(-) diff --git a/litellm/proxy/management_endpoints/scim/scim_v2.py b/litellm/proxy/management_endpoints/scim/scim_v2.py index fa123b7d76c..6c0a424c378 100644 --- a/litellm/proxy/management_endpoints/scim/scim_v2.py +++ b/litellm/proxy/management_endpoints/scim/scim_v2.py @@ -1327,6 +1327,20 @@ def _extract_group_values(value: Any) -> List[str]: return group_values +def _extract_member_ids_from_path(path: str) -> list[str]: + """Return member ids from a SCIM member path filter such as members[value eq "id"].""" + match = re.search(r"\[(.+)\]", path) + if match is None: + return [] + parsed = _parse_scim_eq_filter(match.group(1)) + if parsed is None: + return [] + attribute, member_id = parsed + if attribute != "value": + return [] + return [member_id] + + def _handle_displayname_update(op_type: str, value: Any, update_data: Dict[str, Any]) -> None: """Handle displayname updates.""" if op_type == "remove": @@ -1886,8 +1900,7 @@ async def _process_group_patch_operations( metadata = dict(existing_metadata) if existing_metadata else {} # Track member changes - current_members = set(existing_team.members or []) - final_members = current_members.copy() + final_members = set(await _get_team_member_user_ids_from_team(existing_team)) # Process each patch operation for op in patch_ops.Operations: @@ -1906,34 +1919,29 @@ async def _process_group_patch_operations( else: metadata["externalId"] = str(value) elif path.startswith("members"): - # Handle member operations - member_values = _extract_group_values(value) - # Check the feature flag - scim_upsert_user = await _get_scim_upsert_user_setting() - # Validate all users exist or create them based on feature flag - valid_members = [] - for member_id in member_values: - # Validate member_id is not empty - if not member_id or not member_id.strip(): - raise HTTPException( - status_code=400, - detail={"error": "Invalid member: user ID cannot be empty."}, - ) + member_values = _extract_group_values(value) or _extract_member_ids_from_path(op.path or "") + if op_type == "remove": + final_members = final_members - set(member_values) if member_values else set() + else: + scim_upsert_user = await _get_scim_upsert_user_setting() + valid_members = [] + for member_id in member_values: + if not member_id or not member_id.strip(): + raise HTTPException( + status_code=400, + detail={"error": "Invalid member: user ID cannot be empty."}, + ) - user = await UserRepository(prisma_client).table.find_unique(where={"user_id": member_id}) - if user: - valid_members.append(member_id) - else: - if scim_upsert_user: - # Create the user if they don't exist (backward compatible behavior) + user = await UserRepository(prisma_client).table.find_unique(where={"user_id": member_id}) + if user: + valid_members.append(member_id) + elif scim_upsert_user: created_user = await _create_user_if_not_exists( user_id=member_id, created_via="scim_group_patch" ) if created_user: valid_members.append(member_id) - # If creation failed, user is skipped (logged in helper) else: - # User doesn't exist - reject per SCIM 2.0 protocol raise HTTPException( status_code=400, detail={ @@ -1942,13 +1950,10 @@ async def _process_group_patch_operations( }, ) - if op_type == "replace": - final_members = set(valid_members) - elif op_type == "add": - final_members.update(valid_members) - elif op_type == "remove": - for member_id in valid_members: - final_members.discard(member_id) + if op_type == "replace": + final_members = set(valid_members) + elif op_type == "add": + final_members = final_members | set(valid_members) else: # Handle other generic metadata if op_type == "remove": diff --git a/tests/test_litellm/proxy/management_endpoints/scim/test_scim_v2_endpoints.py b/tests/test_litellm/proxy/management_endpoints/scim/test_scim_v2_endpoints.py index f27f1197090..f2428b1fa03 100644 --- a/tests/test_litellm/proxy/management_endpoints/scim/test_scim_v2_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/scim/test_scim_v2_endpoints.py @@ -2501,6 +2501,79 @@ async def test_patch_group_recomputes_roles_for_changed_members(mocker): assert set(recompute_mock.call_args[0][1]) == {"user1"} +@pytest.mark.asyncio +async def test_patch_group_removes_only_targeted_member_via_okta_filter_path(mocker): + """Okta removes a single member with a filtered path and no value body, + e.g. `members[value eq "user1"]`. PATCH /Groups must remove exactly that + member from the team and leave the others in place.""" + from litellm.proxy._types import LiteLLM_TeamTable, Member + from litellm.proxy.management_endpoints.scim.scim_transformations import ( + ScimTransformations, + ) + + group_id = "test-team-123" + existing_team = LiteLLM_TeamTable( + team_id=group_id, + team_alias="Admins", + members=["user1", "user2"], + members_with_roles=[ + Member(user_id="user1", role="user"), + Member(user_id="user2", role="user"), + ], + metadata={}, + ) + patch_ops = SCIMPatchOp( + schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + Operations=[SCIMPatchOperation(op="remove", path='members[value eq "user1"]')], + ) + + mock_prisma_client = mocker.MagicMock() + mock_prisma_client.db = mocker.MagicMock() + mock_prisma_client.db.litellm_teamtable = mocker.MagicMock() + mock_prisma_client.db.litellm_teamtable.find_unique = AsyncMock( + return_value=existing_team + ) + mock_prisma_client.db.litellm_teamtable.update = AsyncMock( + return_value=existing_team + ) + + mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2._get_prisma_client_or_raise_exception", + AsyncMock(return_value=mock_prisma_client), + ) + patch_membership_mock = mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2.patch_team_membership", + AsyncMock(), + ) + mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2._recompute_scim_member_roles", + AsyncMock(), + ) + mocker.patch.object( + ScimTransformations, + "transform_litellm_team_to_scim_group", + AsyncMock( + return_value=SCIMGroup( + schemas=["urn:ietf:params:scim:schemas:core:2.0:Group"], + id=group_id, + displayName="Admins", + ) + ), + ) + + await patch_group(group_id=group_id, patch_ops=patch_ops) + + patch_membership_mock.assert_awaited_once_with( + user_id="user1", + teams_ids_to_add_user_to=[], + teams_ids_to_remove_user_from=[group_id], + ) + written_members = set( + mock_prisma_client.db.litellm_teamtable.update.call_args.kwargs["data"]["members"] + ) + assert written_members == {"user2"} + + @pytest.mark.asyncio async def test_delete_group_recomputes_roles_for_members(mocker): """DELETE /Groups must recompute the global role for the team's members, so