diff --git a/litellm/proxy/management_endpoints/scim/scim_v2.py b/litellm/proxy/management_endpoints/scim/scim_v2.py index 8efa2c06998..8d255859571 100644 --- a/litellm/proxy/management_endpoints/scim/scim_v2.py +++ b/litellm/proxy/management_endpoints/scim/scim_v2.py @@ -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( 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 f29069f7f3c..5caca95095f 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 @@ -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