mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-14 23:21:35 +00:00
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 "<id>"]). _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>
This commit is contained in:
milan
parent
214945a223
commit
423b5dd6e8
2 changed files with 108 additions and 30 deletions
|
|
@ -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":
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue