From 2a48418bb48ae8d4d087e3d21c2d0069bd0d6ec7 Mon Sep 17 00:00:00 2001 From: Joshua Valluru <326636767+joshua-berri@users.noreply.github.com> Date: Sat, 26 Sep 2026 12:27:11 -0700 Subject: [PATCH] fix(scim): keep human email ownership consistent with local writes --- .../scim/human_provisioning.py | 28 +++-- .../management_endpoints/scim/scim_v2.py | 4 +- .../scim/test_human_provisioning.py | 114 +++++++++++++++++- .../scim/test_scim_patch_user.py | 24 ++-- 4 files changed, 138 insertions(+), 32 deletions(-) diff --git a/litellm/proxy/management_endpoints/scim/human_provisioning.py b/litellm/proxy/management_endpoints/scim/human_provisioning.py index dff1f03adbe..7d43e5f1fa1 100644 --- a/litellm/proxy/management_endpoints/scim/human_provisioning.py +++ b/litellm/proxy/management_endpoints/scim/human_provisioning.py @@ -17,13 +17,12 @@ from prisma.types import ( ) from pydantic import TypeAdapter -from litellm.proxy._types import LiteLLM_UserTable as UserPolicy from litellm.proxy._types import LitellmUserRoles from litellm.proxy.management_endpoints.internal_user_endpoints import check_user_license_capacity from litellm.proxy.utils import PrismaClient from litellm.repositories.base_repository import is_unique_violation from litellm.types.proxy.management_endpoints.scim_agent_provisioning import canonical_directory_id -from litellm.types.proxy.management_endpoints.scim_v2 import SCIMPatchOp, SCIMPatchOperation, SCIMUser +from litellm.types.proxy.management_endpoints.scim_v2 import SCIMPatchOp, SCIMPatchOperation, SCIMUser, SCIMUserEmail def human_email(user: SCIMUser) -> str: @@ -43,7 +42,16 @@ def changes_readonly_attribute(operation: SCIMPatchOperation) -> bool: return any(attribute.startswith(("groups", "externalid")) for attribute in attributes) +def changes_email_attribute(operation: SCIMPatchOperation) -> bool: + value: Final = operation.value + fields: Final = TypeAdapter(dict[str, object]).validate_python(value) if isinstance(value, dict) else {} + attributes: Final = (operation.path,) if operation.path else tuple(fields) + return any(attribute.casefold().split("[", 1)[0].split(".", 1)[0] == "emails" for attribute in attributes) + + def validate_human_patch(patch: SCIMPatchOp) -> None: + if any(changes_email_attribute(operation) for operation in patch.Operations): + raise HTTPException(400, "Use PUT to replace a provisioned human's email") if any(changes_readonly_attribute(operation) for operation in patch.Operations): raise HTTPException(400, "externalId is immutable; update group membership through this source's Groups") @@ -165,19 +173,15 @@ class SourceHumanProvisioner: raise HTTPException(409, "The human local identity was removed; automatic recreation is not permitted") if isinstance(change, SCIMUser): await self.claim_email(row, human_email(change)) - if isinstance(change, SCIMPatchOp): - async with self.client.tx() as tx: - current: Final = await tx.litellm_usertable.find_unique(where=local_filter) - if current is None: - raise HTTPException(409, "The human local identity was removed during provisioning") - preview, _ = scim_v2.apply_scim_user_patch(UserPolicy.model_validate(current.model_dump()), change) - email: Final = TypeAdapter[str | None](str | None).validate_python(preview.get("user_email")) - if email: - await self.claim_email(row, email.casefold()) result: Final = ( await scim_v2.patch_user(user_id=row.local_id, patch_ops=change) if isinstance(change, SCIMPatchOp) - else await scim_v2.update_user(user_id=row.local_id, user=change.model_copy(update={"groups": None})) + else await scim_v2.update_user( + user_id=row.local_id, + user=change.model_copy( + update={"groups": None, "emails": [SCIMUserEmail(value=human_email(change), primary=True)]} + ), + ) ) document: Final = result.model_copy(update={"id": row.id, "externalId": row.external_id, "userName": username}) resource_filter: Final[LiteLLM_SCIMResourceWhereUniqueInput] = {"id": row.id} diff --git a/litellm/proxy/management_endpoints/scim/scim_v2.py b/litellm/proxy/management_endpoints/scim/scim_v2.py index 65665920099..0cf201b3a00 100644 --- a/litellm/proxy/management_endpoints/scim/scim_v2.py +++ b/litellm/proxy/management_endpoints/scim/scim_v2.py @@ -2133,7 +2133,7 @@ def _handle_generic_metadata(path: str, op_type: str, value: object, metadata: d metadata[path] = value -def apply_scim_user_patch( +def _apply_patch_ops( existing_user: LiteLLM_UserTable, patch_ops: SCIMPatchOp, ) -> tuple[dict[str, object], set[str]]: @@ -2347,7 +2347,7 @@ async def patch_user( prev_active: Final = _user_scim_active(existing_user) - update_data, final_team_set = apply_scim_user_patch( + update_data, final_team_set = _apply_patch_ops( existing_user=existing_user, patch_ops=patch_ops, ) diff --git a/tests/test_litellm/proxy/management_endpoints/scim/test_human_provisioning.py b/tests/test_litellm/proxy/management_endpoints/scim/test_human_provisioning.py index 05f3a422bf9..48f878776a6 100644 --- a/tests/test_litellm/proxy/management_endpoints/scim/test_human_provisioning.py +++ b/tests/test_litellm/proxy/management_endpoints/scim/test_human_provisioning.py @@ -72,6 +72,81 @@ def test_human_ownership_email_uses_primary_and_normalizes_case(emails: object, assert human_email(user) == expected +def test_missing_human_ownership_identifier_is_rejected() -> None: + with pytest.raises(HTTPException) as failure: + human_email(SCIMUser(schemas=[])) + assert failure.value.status_code == 400 + + +@pytest.mark.asyncio +@pytest.mark.parametrize("missing", ["externalId", "userName"]) +async def test_direct_reservation_requires_complete_directory_identity(missing: str) -> None: + service, tx, _, user = human_fixture() + with pytest.raises(HTTPException) as failure: + await service.reserve(user.model_copy(update={missing: None})) + assert failure.value.status_code == 400 + tx.litellm_scimresource.create.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_human_create_does_not_hide_storage_failure() -> None: + service, tx, _, user = human_fixture() + tx.litellm_scimresource.find_unique.side_effect = ConnectionError("unavailable") + with pytest.raises(ConnectionError, match="unavailable"): + await service.create(user) + tx.litellm_usertable.create.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_incomplete_human_record_cannot_be_updated() -> None: + service, tx, row, user = human_fixture() + with pytest.raises(HTTPException) as failure: + await service.update(row.model_copy(update={"local_id": None}), user) + assert failure.value.status_code == 409 + tx.litellm_usertable.find_unique.assert_not_awaited() + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "path,value", + [ + ("emails", [{"value": "NEW@example.com"}]), + ('emails[type eq "work"].value', "NEW@example.com"), + (None, {"emails": [{"value": "NEW@example.com"}]}), + ], +) +async def test_scoped_email_patch_requires_put_before_mutation(path: str | None, value: object) -> None: + service, tx, row, _ = human_fixture() + change: Final = SCIMPatchOp(Operations=[{"op": "replace", "path": path, "value": value}]) + with pytest.raises(HTTPException, match="PUT") as failure: + await service.update(row, change) + assert failure.value.status_code == 400 + tx.litellm_usertable.find_unique.assert_not_awaited() + tx.litellm_scimresource.update.assert_not_awaited() + + +@pytest.mark.asyncio +@pytest.mark.parametrize( + "emails", [None, [{"value": "FIRST@example.com"}, {"value": "PRIMARY@example.com", "primary": True}]] +) +async def test_human_put_claims_the_same_email_it_writes(emails: object, monkeypatch: pytest.MonkeyPatch) -> None: + service, tx, row, user = human_fixture() + tx.litellm_usertable.find_unique.return_value = SimpleNamespace(user_id=row.local_id) + change: Final = SCIMUser.model_validate({**user.model_dump(), "emails": emails}) + expected: Final = human_email(change) + + async def apply_update(*, user_id: str, user: SCIMUser) -> SCIMUser: + assert user_id == row.local_id + assert user.emails and user.emails[0].value == expected + tx.litellm_scimresource.update.assert_awaited_once_with(where={"id": row.id}, data={"human_email": expected}) + return user + + monkeypatch.setattr(scim_v2, "update_user", apply_update) + result: Final = await service.update(row, change) + assert result.id == row.id + assert result.emails and result.emails[0].value == expected + + @pytest.mark.asyncio async def test_reservation_replay_preserves_identity_before_creating_a_local_user() -> None: service, tx, row, user = human_fixture() @@ -90,10 +165,14 @@ async def test_reservation_claims_email_subject_and_local_identity() -> None: assert data["human_email"] == user.userName assert data["human_subject_key"] == f"{TENANT}:{SUBJECT}" assert data["id"] == data["document"].data["id"] - tx.litellm_usertable.create.assert_awaited_once_with(data={ - "user_id": user.userName, "user_email": user.userName, - "user_role": "internal_user_viewer", "teams": [], - }) + tx.litellm_usertable.create.assert_awaited_once_with( + data={ + "user_id": user.userName, + "user_email": user.userName, + "user_role": "internal_user_viewer", + "teams": [], + } + ) @pytest.mark.asyncio @@ -194,7 +273,7 @@ async def test_ownership_collision_is_a_conflict_before_legacy_user_mutation( @pytest.mark.asyncio -async def test_human_patch_preserves_scim_id_and_claims_the_changed_email(monkeypatch: pytest.MonkeyPatch) -> None: +async def test_human_patch_preserves_scim_id_and_updates_activity(monkeypatch: pytest.MonkeyPatch) -> None: from litellm.proxy._types import LiteLLM_UserTable service, tx, row, user = human_fixture() @@ -291,7 +370,9 @@ async def test_email_update_cannot_claim_an_unrelated_local_human(monkeypatch: p update: Final = AsyncMock() monkeypatch.setattr(scim_v2, "update_user", update) with pytest.raises(HTTPException) as failure: - await service.update(row, SCIMUser.model_validate({**user.model_dump(), "emails": [{"value": "ADMIN@example.com"}]})) + await service.update( + row, SCIMUser.model_validate({**user.model_dump(), "emails": [{"value": "ADMIN@example.com"}]}) + ) assert failure.value.status_code == 409 update.assert_not_awaited() tx.litellm_scimresource.update.assert_not_awaited() @@ -322,3 +403,24 @@ async def test_replayed_human_does_not_consume_another_license_seat(monkeypatch: assert await service.reserve(user) == row license_check.is_over_limit.assert_not_called() tx.litellm_usertable.create.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_existing_native_agent_cannot_be_reclassified_as_human() -> None: + from litellm.types.proxy.management_endpoints.scim_v2 import SCIM_AGENT_USER_SCHEMA + + service, tx, row, user = human_fixture() + native: Final = SCIMUser.model_validate( + { + **user.model_dump(), + SCIM_AGENT_USER_SCHEMA: {"identityParentId": TENANT}, + } + ) + tx.litellm_scimresource.find_unique.return_value = row.model_copy( + update={"document": native.model_dump(by_alias=True, mode="json")} + ) + with pytest.raises(HTTPException) as failure: + await service.create(user) + assert failure.value.status_code == 409 + tx.litellm_usertable.find_unique.assert_not_awaited() + tx.litellm_scimresource.update.assert_not_awaited() diff --git a/tests/test_litellm/proxy/management_endpoints/scim/test_scim_patch_user.py b/tests/test_litellm/proxy/management_endpoints/scim/test_scim_patch_user.py index 95e022f1ec4..edcce16ab41 100644 --- a/tests/test_litellm/proxy/management_endpoints/scim/test_scim_patch_user.py +++ b/tests/test_litellm/proxy/management_endpoints/scim/test_scim_patch_user.py @@ -4,7 +4,7 @@ import pytest from fastapi import HTTPException from litellm.proxy._types import LiteLLM_UserTable -from litellm.proxy.management_endpoints.scim.scim_v2 import apply_scim_user_patch, patch_user +from litellm.proxy.management_endpoints.scim.scim_v2 import _apply_patch_ops, patch_user from litellm.types.proxy.management_endpoints.scim_v2 import ( SCIMPatchOp, SCIMPatchOperation, @@ -355,7 +355,7 @@ def test_apply_patch_ops_replace_entitlements_writes_canonical_key(): ] ) - update_data, _ = apply_scim_user_patch( + update_data, _ = _apply_patch_ops( existing_user=_user_with_metadata({}), patch_ops=patch_ops ) @@ -373,7 +373,7 @@ def test_apply_patch_ops_add_roles_appends_to_existing(): ] ) - update_data, _ = apply_scim_user_patch( + update_data, _ = _apply_patch_ops( existing_user=_user_with_metadata({"scim_roles": [{"value": "viewer"}]}), patch_ops=patch_ops, ) @@ -389,7 +389,7 @@ def test_apply_patch_ops_remove_entitlements_clears_canonical_key(): Operations=[SCIMPatchOperation(op="remove", path="entitlements")] ) - update_data, _ = apply_scim_user_patch( + update_data, _ = _apply_patch_ops( existing_user=_user_with_metadata( {"scim_entitlements": [{"value": "jira-software"}]} ), @@ -409,7 +409,7 @@ def test_apply_patch_ops_pathless_value_dict_handles_roles(): ] ) - update_data, _ = apply_scim_user_patch( + update_data, _ = _apply_patch_ops( existing_user=_user_with_metadata({}), patch_ops=patch_ops ) @@ -428,7 +428,7 @@ def test_apply_patch_ops_invalid_entitlements_value_raises_400(): ) with pytest.raises(HTTPException) as exc_info: - apply_scim_user_patch(existing_user=_user_with_metadata({}), patch_ops=patch_ops) + _apply_patch_ops(existing_user=_user_with_metadata({}), patch_ops=patch_ops) assert exc_info.value.status_code == 400 @@ -442,7 +442,7 @@ def test_apply_patch_ops_replace_entitlements_without_value_member_is_stored_as_ ] ) - update_data, _ = apply_scim_user_patch( + update_data, _ = _apply_patch_ops( existing_user=_user_with_metadata({}), patch_ops=patch_ops ) @@ -455,7 +455,7 @@ def test_apply_patch_ops_add_without_value_raises_400_naming_value_member(): ) with pytest.raises(HTTPException) as exc_info: - apply_scim_user_patch(existing_user=_user_with_metadata({}), patch_ops=patch_ops) + _apply_patch_ops(existing_user=_user_with_metadata({}), patch_ops=patch_ops) assert exc_info.value.status_code == 400 assert "value" in str(exc_info.value.detail) @@ -473,7 +473,7 @@ def test_apply_patch_ops_filtered_path_raises_400_instead_of_junk_metadata(): ) with pytest.raises(HTTPException) as exc_info: - apply_scim_user_patch( + _apply_patch_ops( existing_user=_user_with_metadata( {"scim_roles": [{"value": "engineering-admin"}]} ), @@ -496,7 +496,7 @@ def test_apply_patch_ops_remove_group_filtered_path_without_value(): Operations=[SCIMPatchOperation(op="remove", path='groups[value eq "team-1"]')] ) - _, final_team_set = apply_scim_user_patch(existing_user=user, patch_ops=patch_ops) + _, final_team_set = _apply_patch_ops(existing_user=user, patch_ops=patch_ops) assert final_team_set == {"team-2"} @@ -513,7 +513,7 @@ def test_apply_patch_ops_add_group_filtered_path_without_value(): Operations=[SCIMPatchOperation(op="add", path="groups[value eq 'team-3']")] ) - _, final_team_set = apply_scim_user_patch(existing_user=user, patch_ops=patch_ops) + _, final_team_set = _apply_patch_ops(existing_user=user, patch_ops=patch_ops) assert final_team_set == {"team-1", "team-3"} @@ -533,6 +533,6 @@ def test_apply_patch_ops_replace_groups_empty_value_does_not_use_path_filter(): ] ) - _, final_team_set = apply_scim_user_patch(existing_user=user, patch_ops=patch_ops) + _, final_team_set = _apply_patch_ops(existing_user=user, patch_ops=patch_ops) assert final_team_set == set()