mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-07 08:26:10 +00:00
fix(scim): propagate team roster write failures on group and user writes (#37700)
SCIM roster writes were swallowed, so a group or user push returned 200 while the team roster never received the membership. Surfacing the failure fixes that, but aborting on the first failed write leaves the rest of the batch unattempted on top of unrolled-back, which is worse than what it replaces. Every roster write in a reconciliation is now attempted, and the ones that did not land are reported together, naming each failed add and remove. Rollback would be the other option and it is not safe here: the compensating write can fail too, and it can strip a membership that pre-dated the push. SCIM reconciliation is idempotent, so a named partial failure is what the IdP's next push needs to close the gap. The reported status still follows the failures, so a unanimous 404 stays a 404 and only a batch whose failures disagree falls back to 500. Co-authored-by: yassin <yassin@berri.ai> Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
This commit is contained in:
devin-ai-integration[bot]
GitHub
parent
286c75f69d
commit
c008d5e2bd
2 changed files with 416 additions and 131 deletions
|
|
@ -5,7 +5,9 @@ This is an enterprise feature and requires a premium license.
|
|||
"""
|
||||
|
||||
import re
|
||||
from collections.abc import Iterable, Mapping, Sequence
|
||||
from collections.abc import Awaitable, Callable, Iterable, Mapping, Sequence
|
||||
from dataclasses import dataclass
|
||||
from functools import partial
|
||||
from itertools import chain
|
||||
from typing import TYPE_CHECKING, Final, NamedTuple, Protocol, overload
|
||||
|
||||
|
|
@ -206,7 +208,6 @@ class UserProvisionerHelpers:
|
|||
user_id=existing_user.user_id,
|
||||
existing_teams=existing_user.teams or [],
|
||||
new_teams=new_teams,
|
||||
raise_on_error=True,
|
||||
)
|
||||
|
||||
updated_user: Final = await _table(UserRepository(prisma_client)).update(
|
||||
|
|
@ -759,9 +760,12 @@ async def _handle_team_membership_changes(
|
|||
user_id: str,
|
||||
existing_teams: list[str],
|
||||
new_teams: list[str],
|
||||
raise_on_error: bool = False,
|
||||
) -> None:
|
||||
"""Handle adding/removing user from teams based on changes."""
|
||||
"""Handle adding/removing user from teams based on changes.
|
||||
|
||||
Roster write failures propagate so the SCIM endpoint returns an error the IdP
|
||||
retries, instead of persisting a ``teams`` array the roster never received.
|
||||
"""
|
||||
existing_teams_set: Final = set(existing_teams)
|
||||
new_teams_set: Final = set(new_teams)
|
||||
|
||||
|
|
@ -773,7 +777,7 @@ async def _handle_team_membership_changes(
|
|||
user_id=user_id,
|
||||
teams_ids_to_add_user_to=list(teams_to_add),
|
||||
teams_ids_to_remove_user_from=list(teams_to_remove),
|
||||
raise_on_error=raise_on_error,
|
||||
raise_on_error=True,
|
||||
)
|
||||
|
||||
|
||||
|
|
@ -1896,6 +1900,87 @@ def _is_user_not_in_team_error(exc: HTTPException) -> bool:
|
|||
return isinstance(detail, dict) and detail.get("error") == "User not found in team"
|
||||
|
||||
|
||||
@dataclass(frozen=True, slots=True)
|
||||
class RosterWriteFailure:
|
||||
description: str
|
||||
status_code: int
|
||||
|
||||
|
||||
def _roster_write_status(exc: Exception) -> int:
|
||||
if isinstance(exc, HTTPException):
|
||||
return exc.status_code
|
||||
if isinstance(exc, ProxyException):
|
||||
return int(exc.code) if exc.code.isdigit() else 500
|
||||
return 500
|
||||
|
||||
|
||||
class SCIMRosterSyncError(Exception):
|
||||
"""Every roster write in the batch was attempted; these are the ones that did not land.
|
||||
|
||||
Rolling the successful ones back is not safe, since the compensating write can fail
|
||||
too and can strip a membership that pre-dated the push. Naming the exact failures
|
||||
instead lets the IdP's next push, which is idempotent, close the gap. handle_exception_on_proxy
|
||||
reads ``status_code`` off this, so a unanimous failure keeps its own status and a mixed
|
||||
batch reports 500.
|
||||
"""
|
||||
|
||||
def __init__(self, failures: tuple[RosterWriteFailure, ...], attempted: int) -> None:
|
||||
statuses: Final = frozenset(failure.status_code for failure in failures)
|
||||
self.failures: Final[tuple[RosterWriteFailure, ...]] = failures
|
||||
self.status_code: Final[int] = next(iter(statuses)) if len(statuses) == 1 else 500
|
||||
super().__init__(
|
||||
f"SCIM roster sync failed on {len(failures)} of {attempted} team membership writes, "
|
||||
f"leaving the roster partially updated. Retry the push to reconcile it. "
|
||||
f"Failed writes: {'; '.join(failure.description for failure in failures)}"
|
||||
)
|
||||
|
||||
|
||||
async def _attempt_roster_write(label: str, write: Callable[[], Awaitable[object]]) -> tuple[RosterWriteFailure, ...]:
|
||||
"""Run one roster write and return what failed, so the caller can keep going."""
|
||||
try:
|
||||
await write()
|
||||
except SCIMRosterSyncError as e:
|
||||
return e.failures
|
||||
except Exception as e: # noqa: BLE001 # this boundary turns any write failure into a value so the batch continues
|
||||
verbose_proxy_logger.exception("SCIM roster write failed (%s): %s", label, e)
|
||||
return (RosterWriteFailure(description=f"{label}: {e}", status_code=_roster_write_status(e)),)
|
||||
return ()
|
||||
|
||||
|
||||
async def _collect_roster_write_failures(
|
||||
writes: Sequence[tuple[str, Callable[[], Awaitable[object]]]],
|
||||
) -> tuple[RosterWriteFailure, ...]:
|
||||
per_write: Final = tuple([await _attempt_roster_write(label, write) for label, write in writes])
|
||||
return tuple(chain.from_iterable(per_write))
|
||||
|
||||
|
||||
async def _add_user_to_team(user_id: str, team_id: str) -> None:
|
||||
try:
|
||||
await team_member_add(
|
||||
data=TeamMemberAddRequest(
|
||||
team_id=team_id,
|
||||
member=Member(user_id=user_id, role="user"),
|
||||
),
|
||||
user_api_key_dict=UserAPIKeyAuth(user_role=LitellmUserRoles.PROXY_ADMIN),
|
||||
)
|
||||
except ProxyException as e:
|
||||
if e.type != ProxyErrorTypes.team_member_already_in_team:
|
||||
raise
|
||||
verbose_proxy_logger.debug("User %s is already in team %s, skipping add", user_id, team_id)
|
||||
|
||||
|
||||
async def _remove_user_from_team(user_id: str, team_id: str) -> None:
|
||||
try:
|
||||
await team_member_delete(
|
||||
data=TeamMemberDeleteRequest(team_id=team_id, user_id=user_id),
|
||||
user_api_key_dict=UserAPIKeyAuth(user_role=LitellmUserRoles.PROXY_ADMIN),
|
||||
)
|
||||
except HTTPException as e:
|
||||
if not _is_user_not_in_team_error(e):
|
||||
raise
|
||||
verbose_proxy_logger.debug("User %s is not in team %s, skipping remove", user_id, team_id)
|
||||
|
||||
|
||||
async def patch_team_membership(
|
||||
user_id: str,
|
||||
teams_ids_to_add_user_to: list[str],
|
||||
|
|
@ -1909,49 +1994,26 @@ async def patch_team_membership(
|
|||
A user already being in a team (on add) or already absent from it (on
|
||||
remove) is treated as a no-op, not an error.
|
||||
|
||||
When ``raise_on_error`` is True a genuine add or remove failure (anything
|
||||
other than those idempotent no-ops) propagates instead of being swallowed,
|
||||
so a caller can avoid persisting a teams array the roster never received.
|
||||
Every team is attempted before anything is reported, so one failing team cannot
|
||||
strand the others unattempted. When ``raise_on_error`` is True the writes that did
|
||||
not land are reported together, instead of a teams array the roster never received
|
||||
being persisted as a success.
|
||||
"""
|
||||
for _team_id in teams_ids_to_add_user_to:
|
||||
try:
|
||||
await team_member_add(
|
||||
data=TeamMemberAddRequest(
|
||||
team_id=_team_id,
|
||||
member=Member(user_id=user_id, role="user"),
|
||||
),
|
||||
user_api_key_dict=UserAPIKeyAuth(user_role=LitellmUserRoles.PROXY_ADMIN),
|
||||
)
|
||||
except ProxyException as e:
|
||||
# Handle duplicate membership gracefully - this is idempotent
|
||||
if e.type == ProxyErrorTypes.team_member_already_in_team:
|
||||
verbose_proxy_logger.debug("User %s is already in team %s, skipping add", user_id, _team_id)
|
||||
elif raise_on_error:
|
||||
raise
|
||||
else:
|
||||
verbose_proxy_logger.exception("Error adding user to team %s: %s", _team_id, e)
|
||||
except Exception as e:
|
||||
if raise_on_error:
|
||||
raise
|
||||
verbose_proxy_logger.exception("Error adding user to team %s: %s", _team_id, e)
|
||||
|
||||
for _team_id in teams_ids_to_remove_user_from:
|
||||
try:
|
||||
await team_member_delete(
|
||||
data=TeamMemberDeleteRequest(team_id=_team_id, user_id=user_id),
|
||||
user_api_key_dict=UserAPIKeyAuth(user_role=LitellmUserRoles.PROXY_ADMIN),
|
||||
)
|
||||
except HTTPException as e:
|
||||
if _is_user_not_in_team_error(e):
|
||||
verbose_proxy_logger.debug("User %s is not in team %s, skipping remove", user_id, _team_id)
|
||||
elif raise_on_error:
|
||||
raise
|
||||
else:
|
||||
verbose_proxy_logger.exception("Error removing user from team %s: %s", _team_id, e)
|
||||
except Exception as e:
|
||||
if raise_on_error:
|
||||
raise
|
||||
verbose_proxy_logger.exception("Error removing user from team %s: %s", _team_id, e)
|
||||
writes: Final = tuple(
|
||||
chain(
|
||||
(
|
||||
(f"add {user_id} to {team_id}", partial(_add_user_to_team, user_id, team_id))
|
||||
for team_id in teams_ids_to_add_user_to
|
||||
),
|
||||
(
|
||||
(f"remove {user_id} from {team_id}", partial(_remove_user_from_team, user_id, team_id))
|
||||
for team_id in teams_ids_to_remove_user_from
|
||||
),
|
||||
)
|
||||
)
|
||||
failures: Final = await _collect_roster_write_failures(writes)
|
||||
if failures and raise_on_error:
|
||||
raise SCIMRosterSyncError(failures, attempted=len(writes))
|
||||
|
||||
return True
|
||||
|
||||
|
|
@ -2414,35 +2476,52 @@ async def _apply_group_patch_updates(group_id: str, update_data: dict[str, objec
|
|||
return await TeamRepository(prisma_client).table.find_unique(where={"team_id": group_id})
|
||||
|
||||
|
||||
async def _handle_group_membership_changes(group_id: str, current_members: set[str], final_members: set[str]):
|
||||
"""Handle adding/removing members from the group.
|
||||
async def _handle_group_membership_changes(group_id: str, current_members: set[str], final_members: set[str]) -> None:
|
||||
"""Reconcile the group roster, attempting every member before reporting failures.
|
||||
|
||||
Runs strict: a genuine add or remove failure propagates so the group request
|
||||
fails and the identity provider retries, instead of reporting success for a
|
||||
member the roster never received. Idempotent no-ops (already in / already out
|
||||
of the team) are still swallowed by patch_team_membership.
|
||||
Aborting on the first failure would leave the remaining members unattempted on top
|
||||
of unrolled-back, so every member is written and the ones that failed are named for
|
||||
the IdP's next push to reconcile.
|
||||
"""
|
||||
members_to_add: Final = final_members - current_members
|
||||
members_to_remove: Final = current_members - final_members
|
||||
members_to_add: Final = sorted(final_members - current_members)
|
||||
members_to_remove: Final = sorted(current_members - final_members)
|
||||
|
||||
verbose_proxy_logger.debug("members_to_add: %s", members_to_add)
|
||||
verbose_proxy_logger.debug("members_to_remove: %s", members_to_remove)
|
||||
|
||||
for member_id in members_to_add:
|
||||
await patch_team_membership(
|
||||
user_id=member_id,
|
||||
teams_ids_to_add_user_to=[group_id],
|
||||
teams_ids_to_remove_user_from=[],
|
||||
raise_on_error=True,
|
||||
)
|
||||
|
||||
for member_id in members_to_remove:
|
||||
await patch_team_membership(
|
||||
user_id=member_id,
|
||||
teams_ids_to_add_user_to=[],
|
||||
teams_ids_to_remove_user_from=[group_id],
|
||||
raise_on_error=True,
|
||||
writes: Final = tuple(
|
||||
chain(
|
||||
(
|
||||
(
|
||||
f"add {member_id} to {group_id}",
|
||||
partial(
|
||||
patch_team_membership,
|
||||
user_id=member_id,
|
||||
teams_ids_to_add_user_to=[group_id],
|
||||
teams_ids_to_remove_user_from=[],
|
||||
raise_on_error=True,
|
||||
),
|
||||
)
|
||||
for member_id in members_to_add
|
||||
),
|
||||
(
|
||||
(
|
||||
f"remove {member_id} from {group_id}",
|
||||
partial(
|
||||
patch_team_membership,
|
||||
user_id=member_id,
|
||||
teams_ids_to_add_user_to=[],
|
||||
teams_ids_to_remove_user_from=[group_id],
|
||||
raise_on_error=True,
|
||||
),
|
||||
)
|
||||
for member_id in members_to_remove
|
||||
),
|
||||
)
|
||||
)
|
||||
failures: Final = await _collect_roster_write_failures(writes)
|
||||
if failures:
|
||||
raise SCIMRosterSyncError(failures, attempted=len(writes))
|
||||
|
||||
|
||||
@scim_router.patch(
|
||||
|
|
|
|||
|
|
@ -15,6 +15,7 @@ from litellm.proxy._types import (
|
|||
ProxyException,
|
||||
)
|
||||
from litellm.proxy.management_endpoints.scim.scim_v2 import (
|
||||
SCIMRosterSyncError,
|
||||
UserProvisionerHelpers,
|
||||
_apply_group_patch_updates,
|
||||
_extract_group_member_ids,
|
||||
|
|
@ -33,6 +34,7 @@ from litellm.proxy.management_endpoints.scim.scim_v2 import (
|
|||
get_users,
|
||||
get_service_provider_config,
|
||||
patch_group,
|
||||
patch_team_membership,
|
||||
patch_user,
|
||||
update_group,
|
||||
update_user,
|
||||
|
|
@ -626,7 +628,6 @@ async def test_handle_existing_user_by_email_existing_user_updated(mocker):
|
|||
user_id="old-user-id",
|
||||
existing_teams=["old-team"],
|
||||
new_teams=["new-team"],
|
||||
raise_on_error=True,
|
||||
)
|
||||
|
||||
mock_transform.assert_called_once_with(updated_user)
|
||||
|
|
@ -732,7 +733,6 @@ async def test_handle_existing_user_by_email_syncs_roster_and_dedups_teams(mocke
|
|||
user_id="same-id",
|
||||
existing_teams=[],
|
||||
new_teams=["team-a", "team-b"],
|
||||
raise_on_error=True,
|
||||
)
|
||||
|
||||
update_calls = mock_prisma_client.db.litellm_usertable.update.call_args_list
|
||||
|
|
@ -777,12 +777,13 @@ async def test_handle_existing_user_by_email_roster_add_failure_blocks_teams_wri
|
|||
auto_create_key=False,
|
||||
)
|
||||
|
||||
with pytest.raises(HTTPException):
|
||||
with pytest.raises(SCIMRosterSyncError) as exc_info:
|
||||
await UserProvisionerHelpers.handle_existing_user_by_email(
|
||||
prisma_client=mock_prisma_client, new_user_request=new_user_request
|
||||
)
|
||||
|
||||
mock_team_member_add.assert_awaited_once()
|
||||
assert "add uid to missing-team" in str(exc_info.value)
|
||||
assert mock_prisma_client.db.litellm_usertable.update.await_count == 0
|
||||
|
||||
|
||||
|
|
@ -869,12 +870,13 @@ async def test_handle_existing_user_by_email_roster_remove_failure_blocks_teams_
|
|||
auto_create_key=False,
|
||||
)
|
||||
|
||||
with pytest.raises(HTTPException):
|
||||
with pytest.raises(SCIMRosterSyncError) as exc_info:
|
||||
await UserProvisionerHelpers.handle_existing_user_by_email(
|
||||
prisma_client=mock_prisma_client, new_user_request=new_user_request
|
||||
)
|
||||
|
||||
mock_team_member_delete.assert_awaited_once()
|
||||
assert "remove uid from old-team" in str(exc_info.value)
|
||||
assert mock_prisma_client.db.litellm_usertable.update.await_count == 0
|
||||
|
||||
|
||||
|
|
@ -1287,6 +1289,11 @@ async def test_update_group_metadata_serialization_issue(mocker):
|
|||
AsyncMock(return_value=mock_prisma_client),
|
||||
)
|
||||
|
||||
mocker.patch(
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2.patch_team_membership",
|
||||
AsyncMock(),
|
||||
)
|
||||
|
||||
# Mock the transformation function
|
||||
mock_scim_group_response = SCIMGroup(
|
||||
schemas=["urn:ietf:params:scim:schemas:core:2.0:Group"],
|
||||
|
|
@ -2978,9 +2985,7 @@ async def test_patch_group_rename_recomputes_retained_members(mocker):
|
|||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_process_group_patch_operations_add_retains_existing_members(
|
||||
mocker, monkeypatch
|
||||
):
|
||||
async def test_process_group_patch_operations_add_retains_existing_members(mocker, monkeypatch):
|
||||
"""A SCIM group ``add`` operation must not drop members already in the team.
|
||||
|
||||
Team membership lives in members_with_roles; team creation leaves the legacy
|
||||
|
|
@ -3005,18 +3010,14 @@ async def test_process_group_patch_operations_add_retains_existing_members(
|
|||
)
|
||||
patch_ops = SCIMPatchOp(
|
||||
schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"],
|
||||
Operations=[
|
||||
SCIMPatchOperation(op="add", path="members", value=[{"value": "new-user"}])
|
||||
],
|
||||
Operations=[SCIMPatchOperation(op="add", path="members", value=[{"value": "new-user"}])],
|
||||
)
|
||||
|
||||
mock_prisma_client = mocker.MagicMock()
|
||||
mock_prisma_client.db = mocker.MagicMock()
|
||||
mock_prisma_client.db.litellm_usertable = mocker.MagicMock()
|
||||
# new-user already exists in the DB
|
||||
mock_prisma_client.db.litellm_usertable.find_unique = AsyncMock(
|
||||
return_value=mocker.MagicMock(user_id="new-user")
|
||||
)
|
||||
mock_prisma_client.db.litellm_usertable.find_unique = AsyncMock(return_value=mocker.MagicMock(user_id="new-user"))
|
||||
|
||||
_, final_members, _ = await _process_group_patch_operations(
|
||||
patch_ops=patch_ops,
|
||||
|
|
@ -3028,9 +3029,7 @@ async def test_process_group_patch_operations_add_retains_existing_members(
|
|||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_process_group_patch_operations_remove_uses_members_with_roles(
|
||||
mocker, monkeypatch
|
||||
):
|
||||
async def test_process_group_patch_operations_remove_uses_members_with_roles(mocker, monkeypatch):
|
||||
"""A ``remove`` op must diff against members_with_roles, so removing one
|
||||
member leaves the rest of the team intact rather than emptying it."""
|
||||
|
||||
|
|
@ -3052,19 +3051,13 @@ async def test_process_group_patch_operations_remove_uses_members_with_roles(
|
|||
)
|
||||
patch_ops = SCIMPatchOp(
|
||||
schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"],
|
||||
Operations=[
|
||||
SCIMPatchOperation(
|
||||
op="remove", path="members", value=[{"value": "drop-user"}]
|
||||
)
|
||||
],
|
||||
Operations=[SCIMPatchOperation(op="remove", path="members", value=[{"value": "drop-user"}])],
|
||||
)
|
||||
|
||||
mock_prisma_client = mocker.MagicMock()
|
||||
mock_prisma_client.db = mocker.MagicMock()
|
||||
mock_prisma_client.db.litellm_usertable = mocker.MagicMock()
|
||||
mock_prisma_client.db.litellm_usertable.find_unique = AsyncMock(
|
||||
return_value=mocker.MagicMock(user_id="drop-user")
|
||||
)
|
||||
mock_prisma_client.db.litellm_usertable.find_unique = AsyncMock(return_value=mocker.MagicMock(user_id="drop-user"))
|
||||
|
||||
_, final_members, _ = await _process_group_patch_operations(
|
||||
patch_ops=patch_ops,
|
||||
|
|
@ -3508,9 +3501,7 @@ async def test_process_group_patch_remove_filtered_path_without_value(mocker):
|
|||
prisma_client = mocker.MagicMock()
|
||||
prisma_client.db = mocker.MagicMock()
|
||||
prisma_client.db.litellm_usertable = mocker.MagicMock()
|
||||
prisma_client.db.litellm_usertable.find_unique = AsyncMock(
|
||||
return_value=LiteLLM_UserTable(user_id="user-1")
|
||||
)
|
||||
prisma_client.db.litellm_usertable.find_unique = AsyncMock(return_value=LiteLLM_UserTable(user_id="user-1"))
|
||||
|
||||
_, final_members, _ = await _process_group_patch_operations(
|
||||
patch_ops=patch_ops,
|
||||
|
|
@ -3539,9 +3530,7 @@ async def test_process_group_patch_add_filtered_path_without_value(mocker):
|
|||
prisma_client = mocker.MagicMock()
|
||||
prisma_client.db = mocker.MagicMock()
|
||||
prisma_client.db.litellm_usertable = mocker.MagicMock()
|
||||
prisma_client.db.litellm_usertable.find_unique = AsyncMock(
|
||||
return_value=LiteLLM_UserTable(user_id="user-3")
|
||||
)
|
||||
prisma_client.db.litellm_usertable.find_unique = AsyncMock(return_value=LiteLLM_UserTable(user_id="user-3"))
|
||||
|
||||
_, final_members, _ = await _process_group_patch_operations(
|
||||
patch_ops=patch_ops,
|
||||
|
|
@ -3558,9 +3547,7 @@ async def test_process_group_patch_replace_empty_value_does_not_use_path_filter(
|
|||
id from the filtered path, which would retain one member and drop the rest."""
|
||||
patch_ops = SCIMPatchOp(
|
||||
schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"],
|
||||
Operations=[
|
||||
SCIMPatchOperation(op="replace", path='members[value eq "user-1"]', value=[])
|
||||
],
|
||||
Operations=[SCIMPatchOperation(op="replace", path='members[value eq "user-1"]', value=[])],
|
||||
)
|
||||
|
||||
existing_team = LiteLLM_TeamTable(
|
||||
|
|
@ -3576,9 +3563,7 @@ async def test_process_group_patch_replace_empty_value_does_not_use_path_filter(
|
|||
prisma_client = mocker.MagicMock()
|
||||
prisma_client.db = mocker.MagicMock()
|
||||
prisma_client.db.litellm_usertable = mocker.MagicMock()
|
||||
prisma_client.db.litellm_usertable.find_unique = AsyncMock(
|
||||
return_value=LiteLLM_UserTable(user_id="user-1")
|
||||
)
|
||||
prisma_client.db.litellm_usertable.find_unique = AsyncMock(return_value=LiteLLM_UserTable(user_id="user-1"))
|
||||
|
||||
_, final_members, _ = await _process_group_patch_operations(
|
||||
patch_ops=patch_ops,
|
||||
|
|
@ -3607,9 +3592,7 @@ def _member_resolution_prisma(mocker, *, users: set, teams: set, unmanaged_teams
|
|||
prisma_client.db = mocker.MagicMock()
|
||||
prisma_client.db.litellm_usertable = mocker.MagicMock()
|
||||
prisma_client.db.litellm_usertable.find_unique = AsyncMock(
|
||||
side_effect=lambda where: (
|
||||
LiteLLM_UserTable(user_id=where["user_id"]) if where["user_id"] in users else None
|
||||
)
|
||||
side_effect=lambda where: LiteLLM_UserTable(user_id=where["user_id"]) if where["user_id"] in users else None
|
||||
)
|
||||
prisma_client.db.litellm_teamtable = mocker.MagicMock()
|
||||
prisma_client.db.litellm_teamtable.find_unique = AsyncMock(side_effect=lambda where: team_row(where["team_id"]))
|
||||
|
|
@ -3780,9 +3763,7 @@ async def test_process_group_patch_operations_ignores_lowercase_group_type(mocke
|
|||
nested_group_id = "8f1e9d70-0000-4a0e-9a1e-nested"
|
||||
patch_ops = SCIMPatchOp(
|
||||
schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"],
|
||||
Operations=[
|
||||
SCIMPatchOperation(op="add", path="members", value=[{"value": nested_group_id, "type": "group"}])
|
||||
],
|
||||
Operations=[SCIMPatchOperation(op="add", path="members", value=[{"value": nested_group_id, "type": "group"}])],
|
||||
)
|
||||
existing_team = LiteLLM_TeamTable(
|
||||
team_id="parent-group",
|
||||
|
|
@ -3835,9 +3816,7 @@ async def test_process_group_patch_operations_skips_member_matching_existing_tea
|
|||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_process_group_patch_operations_prefers_user_over_team_for_colliding_id(
|
||||
mocker, scim_upsert_user_enabled
|
||||
):
|
||||
async def test_process_group_patch_operations_prefers_user_over_team_for_colliding_id(mocker, scim_upsert_user_enabled):
|
||||
"""Nothing stops a user id from also being a team id, so the user lookup has to
|
||||
win; ordering the team check first would silently stop syncing that user."""
|
||||
patch_ops = SCIMPatchOp(
|
||||
|
|
@ -4460,6 +4439,250 @@ async def test_get_groups_members_are_typed_as_users(mocker):
|
|||
assert [m.type for m in response.Resources[0].members] == ["User"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_user_roster_add_failure_propagates_and_skips_teams_write(mocker):
|
||||
"""PUT /Users must surface a genuine roster add failure instead of returning 200.
|
||||
|
||||
Regression: the failure was swallowed, the IdP recorded the push as successful
|
||||
and never retried, and the user row was still written with a teams array the
|
||||
team roster never received.
|
||||
"""
|
||||
existing_user = mocker.MagicMock()
|
||||
existing_user.teams = ["old-team"]
|
||||
|
||||
scim_user = SCIMUser(
|
||||
schemas=["urn:ietf:params:scim:schemas:core:2.0:User"],
|
||||
userName="test-user",
|
||||
name=SCIMUserName(familyName="User", givenName="Updated"),
|
||||
emails=[SCIMUserEmail(value="updated@example.com")],
|
||||
groups=[SCIMUserGroup(value="new-team")],
|
||||
)
|
||||
|
||||
mock_prisma_client = mocker.MagicMock()
|
||||
mock_prisma_client.db = mocker.MagicMock()
|
||||
mock_prisma_client.db.litellm_usertable = mocker.MagicMock()
|
||||
mock_prisma_client.db.litellm_usertable.update = AsyncMock()
|
||||
|
||||
mocker.patch(
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2._get_prisma_client_or_raise_exception",
|
||||
AsyncMock(return_value=mock_prisma_client),
|
||||
)
|
||||
mocker.patch(
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2._check_user_exists",
|
||||
AsyncMock(return_value=existing_user),
|
||||
)
|
||||
mocker.patch(
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2.team_member_add",
|
||||
AsyncMock(side_effect=HTTPException(status_code=404, detail={"error": "Team not found"})),
|
||||
)
|
||||
delete_mock = mocker.patch("litellm.proxy.management_endpoints.scim.scim_v2.team_member_delete", AsyncMock())
|
||||
|
||||
with pytest.raises(ProxyException) as exc_info:
|
||||
await update_user(user_id="test-user", user=scim_user)
|
||||
|
||||
delete_mock.assert_awaited_once()
|
||||
assert exc_info.value.code == "404"
|
||||
assert "add test-user to new-team" in exc_info.value.message
|
||||
mock_prisma_client.db.litellm_usertable.update.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_patch_user_roster_remove_failure_propagates_and_skips_teams_write(mocker):
|
||||
"""PATCH /Users must surface a genuine roster remove failure instead of returning 200."""
|
||||
existing_user = mocker.MagicMock()
|
||||
existing_user.teams = ["team1", "team2"]
|
||||
existing_user.metadata = {}
|
||||
|
||||
patch_ops = SCIMPatchOp(
|
||||
schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"],
|
||||
Operations=[SCIMPatchOperation(op="remove", path="groups", value=[{"value": "team2"}])],
|
||||
)
|
||||
|
||||
mock_prisma_client = mocker.MagicMock()
|
||||
mock_prisma_client.db = mocker.MagicMock()
|
||||
mock_prisma_client.db.litellm_usertable = mocker.MagicMock()
|
||||
mock_prisma_client.db.litellm_usertable.update = AsyncMock()
|
||||
|
||||
mocker.patch(
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2._get_prisma_client_or_raise_exception",
|
||||
AsyncMock(return_value=mock_prisma_client),
|
||||
)
|
||||
mocker.patch(
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2._check_user_exists",
|
||||
AsyncMock(return_value=existing_user),
|
||||
)
|
||||
mocker.patch(
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2.team_member_delete",
|
||||
AsyncMock(side_effect=HTTPException(status_code=500, detail={"error": "db unavailable"})),
|
||||
)
|
||||
|
||||
with pytest.raises(ProxyException):
|
||||
await patch_user(user_id="test-user", patch_ops=patch_ops)
|
||||
|
||||
mock_prisma_client.db.litellm_usertable.update.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("failing_member", ["user0", "user1", "user2", "user3"])
|
||||
async def test_handle_group_membership_changes_attempts_every_member_and_names_failures(mocker, failing_member):
|
||||
"""One failing member must not strand the rest of the roster unattempted.
|
||||
|
||||
Regression: reconciliation stopped at the first failure, so a group push carrying
|
||||
several membership changes left the later ones neither written nor reported, and the
|
||||
IdP got one opaque error. Every member is attempted now and only the writes that
|
||||
actually failed are named, so the next push closes exactly that gap.
|
||||
"""
|
||||
|
||||
async def add_member(**kwargs):
|
||||
if kwargs["data"].member.user_id == failing_member:
|
||||
raise HTTPException(status_code=500, detail={"error": "db unavailable"})
|
||||
|
||||
async def remove_member(**kwargs):
|
||||
if kwargs["data"].user_id == failing_member:
|
||||
raise HTTPException(status_code=500, detail={"error": "db unavailable"})
|
||||
|
||||
add_mock = AsyncMock(side_effect=add_member)
|
||||
delete_mock = AsyncMock(side_effect=remove_member)
|
||||
mocker.patch("litellm.proxy.management_endpoints.scim.scim_v2.team_member_add", add_mock)
|
||||
mocker.patch("litellm.proxy.management_endpoints.scim.scim_v2.team_member_delete", delete_mock)
|
||||
|
||||
with pytest.raises(SCIMRosterSyncError) as exc_info:
|
||||
await _handle_group_membership_changes(
|
||||
group_id="group-1",
|
||||
current_members={"user0"},
|
||||
final_members={"user1", "user2", "user3"},
|
||||
)
|
||||
|
||||
assert [call.kwargs["data"].member.user_id for call in add_mock.call_args_list] == ["user1", "user2", "user3"]
|
||||
assert [call.kwargs["data"].user_id for call in delete_mock.call_args_list] == ["user0"]
|
||||
|
||||
message = str(exc_info.value)
|
||||
assert "1 of 4 team membership writes" in message
|
||||
failed_write = "remove user0 from group-1" if failing_member == "user0" else f"add {failing_member} to group-1"
|
||||
assert failed_write in message
|
||||
all_writes = {
|
||||
"remove user0 from group-1",
|
||||
"add user1 to group-1",
|
||||
"add user2 to group-1",
|
||||
"add user3 to group-1",
|
||||
}
|
||||
assert not [write for write in all_writes - {failed_write} if write in message]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize(
|
||||
"first_status, second_status, expected_status",
|
||||
[(404, 404, 404), (404, 500, 500), (500, 500, 500)],
|
||||
)
|
||||
async def test_roster_sync_error_status_follows_unanimous_failures(
|
||||
mocker, first_status, second_status, expected_status
|
||||
):
|
||||
"""Aggregating several failures must not flatten a unanimous 4xx into a 500.
|
||||
|
||||
A push naming a team that does not exist is not retryable, so the IdP has to keep
|
||||
seeing the 404. Only a batch whose failures disagree falls back to 500.
|
||||
"""
|
||||
|
||||
async def add_member(**kwargs):
|
||||
status = first_status if kwargs["data"].team_id == "team-a" else second_status
|
||||
raise HTTPException(status_code=status, detail={"error": "nope"})
|
||||
|
||||
mocker.patch("litellm.proxy.management_endpoints.scim.scim_v2.team_member_add", AsyncMock(side_effect=add_member))
|
||||
|
||||
with pytest.raises(SCIMRosterSyncError) as exc_info:
|
||||
await patch_team_membership(
|
||||
user_id="user1",
|
||||
teams_ids_to_add_user_to=["team-a", "team-b"],
|
||||
teams_ids_to_remove_user_from=[],
|
||||
raise_on_error=True,
|
||||
)
|
||||
|
||||
assert exc_info.value.status_code == expected_status
|
||||
assert "2 of 2 team membership writes" in str(exc_info.value)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("failing_team", ["team-a", "team-b", "team-c"])
|
||||
async def test_patch_team_membership_attempts_every_team_before_reporting(mocker, failing_team):
|
||||
"""A failing team must not strand the same user's remaining adds and removes.
|
||||
|
||||
Regression: the add loop bailed on the first failure, which skipped both the later
|
||||
adds and every removal, so a multi-team SCIM push reconciled only a prefix of the
|
||||
requested changes while reporting one failure.
|
||||
"""
|
||||
|
||||
async def add_member(**kwargs):
|
||||
if kwargs["data"].team_id == failing_team:
|
||||
raise HTTPException(status_code=500, detail={"error": "db unavailable"})
|
||||
|
||||
add_mock = AsyncMock(side_effect=add_member)
|
||||
delete_mock = AsyncMock()
|
||||
mocker.patch("litellm.proxy.management_endpoints.scim.scim_v2.team_member_add", add_mock)
|
||||
mocker.patch("litellm.proxy.management_endpoints.scim.scim_v2.team_member_delete", delete_mock)
|
||||
|
||||
with pytest.raises(SCIMRosterSyncError) as exc_info:
|
||||
await patch_team_membership(
|
||||
user_id="user1",
|
||||
teams_ids_to_add_user_to=["team-a", "team-b", "team-c"],
|
||||
teams_ids_to_remove_user_from=["team-d"],
|
||||
raise_on_error=True,
|
||||
)
|
||||
|
||||
assert [call.kwargs["data"].team_id for call in add_mock.call_args_list] == ["team-a", "team-b", "team-c"]
|
||||
assert [call.kwargs["data"].team_id for call in delete_mock.call_args_list] == ["team-d"]
|
||||
|
||||
message = str(exc_info.value)
|
||||
assert "1 of 4 team membership writes" in message
|
||||
assert f"add user1 to {failing_team}" in message
|
||||
assert not [team for team in {"team-a", "team-b", "team-c"} - {failing_team} if f"add user1 to {team}" in message]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_group_roster_failure_propagates(mocker):
|
||||
"""PUT /Groups must fail loudly when a member roster write fails, instead of
|
||||
reporting a successful membership sync to the IdP."""
|
||||
group_id = "test-team-123"
|
||||
existing_team = LiteLLM_TeamTable(
|
||||
team_id=group_id,
|
||||
team_alias="Engineering",
|
||||
members_with_roles=[Member(user_id="user1", role="user")],
|
||||
metadata={},
|
||||
)
|
||||
scim_group = SCIMGroup(
|
||||
schemas=["urn:ietf:params:scim:schemas:core:2.0:Group"],
|
||||
id=group_id,
|
||||
displayName="Engineering",
|
||||
members=[SCIMMember(value="user1"), SCIMMember(value="user2")],
|
||||
)
|
||||
|
||||
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)
|
||||
mock_prisma_client.db.litellm_usertable = mocker.MagicMock()
|
||||
mock_prisma_client.db.litellm_usertable.find_unique = AsyncMock(return_value=mocker.MagicMock())
|
||||
|
||||
mocker.patch(
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2._get_prisma_client_or_raise_exception",
|
||||
AsyncMock(return_value=mock_prisma_client),
|
||||
)
|
||||
mocker.patch(
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2.team_member_add",
|
||||
AsyncMock(side_effect=HTTPException(status_code=500, detail={"error": "db unavailable"})),
|
||||
)
|
||||
recompute_mock = mocker.patch(
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2._recompute_scim_member_roles",
|
||||
AsyncMock(),
|
||||
)
|
||||
|
||||
with pytest.raises(ProxyException) as exc_info:
|
||||
await update_group(group_id=group_id, group=scim_group)
|
||||
|
||||
assert "add user2 to test-team-123" in exc_info.value.message
|
||||
recompute_mock.assert_not_called()
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_resolve_group_member_ids_raises_when_creation_fails(mocker, scim_upsert_user_enabled):
|
||||
"""A member whose user row can neither be found nor created must fail the
|
||||
|
|
@ -4506,23 +4729,6 @@ async def test_resolve_group_member_ids_admits_member_created_concurrently(mocke
|
|||
assert len(result.created_users) == 0
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_handle_group_membership_changes_propagates_add_failure(mocker):
|
||||
"""A genuine roster add failure must fail the group request so the IdP retries.
|
||||
Regression: patch_team_membership ran with raise_on_error=False here, so a
|
||||
failed team_member_add was logged and swallowed and the SCIM group sync
|
||||
reported success with members missing from the team."""
|
||||
mocker.patch(
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2.team_member_add",
|
||||
AsyncMock(side_effect=HTTPException(status_code=500, detail={"error": "db write failed"})),
|
||||
)
|
||||
|
||||
with pytest.raises(HTTPException):
|
||||
await _handle_group_membership_changes(
|
||||
group_id="group-1", current_members=set(), final_members={"user-1"}
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_handle_group_membership_changes_already_in_team_is_noop(mocker):
|
||||
"""The strict path must keep treating an already-enrolled member as a no-op
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue