mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-07 02:59:05 +00:00
Merge pull request #39623 from BerriAI/litellm_scim_default_team_fallback
fix(scim): default-team fallback on create and keep memberships when PUT /Users has no groups
This commit is contained in:
commit
ab0478f068
3 changed files with 157 additions and 9 deletions
|
|
@ -1123,13 +1123,15 @@ async def _create_user_if_not_exists(user_id: str, created_via: str = "scim_grou
|
|||
user_id=user_id,
|
||||
user_email=user_id, # We don't have email from group membership
|
||||
user_alias=None,
|
||||
teams=[], # Teams will be added separately
|
||||
metadata={"created_via": created_via},
|
||||
auto_create_key=False,
|
||||
user_role=default_role,
|
||||
)
|
||||
|
||||
created_user: Final = await new_user(data=new_user_request)
|
||||
created_user: Final = await new_user(
|
||||
data=new_user_request,
|
||||
user_api_key_dict=UserAPIKeyAuth(user_role=LitellmUserRoles.PROXY_ADMIN),
|
||||
)
|
||||
verbose_proxy_logger.info("Created user %s via %s", user_id, created_via)
|
||||
return created_user
|
||||
|
||||
|
|
@ -1699,7 +1701,7 @@ async def create_user(
|
|||
user_id=user_id,
|
||||
user_email=user_data["user_email"],
|
||||
user_alias=user_data["user_alias"],
|
||||
teams=user_data["teams"],
|
||||
teams=user_data["teams"] or None,
|
||||
metadata=metadata,
|
||||
auto_create_key=False,
|
||||
user_role=resolved_role if admin_group is not None else default_role,
|
||||
|
|
@ -1717,6 +1719,7 @@ async def create_user(
|
|||
|
||||
created_user: Final = await new_user(
|
||||
data=new_user_request,
|
||||
user_api_key_dict=UserAPIKeyAuth(user_role=LitellmUserRoles.PROXY_ADMIN),
|
||||
)
|
||||
|
||||
scim_user: Final = await ScimTransformations.transform_litellm_user_to_scim_user(user=created_user)
|
||||
|
|
@ -1771,22 +1774,25 @@ async def update_user(
|
|||
roles=user_data["roles"],
|
||||
)
|
||||
|
||||
# SCIM User.groups is readOnly (RFC 7643 4.1.2): IdPs sync membership via /Groups and send
|
||||
# no groups or `groups: []` on profile PUTs, so empty means unspecified, not "remove from every team"
|
||||
target_teams: Final = user_data["teams"] or existing_user.teams
|
||||
await _handle_team_membership_changes(
|
||||
user_id=user_id,
|
||||
existing_teams=existing_user.teams or [],
|
||||
new_teams=user_data["teams"],
|
||||
existing_teams=existing_user.teams,
|
||||
new_teams=target_teams,
|
||||
)
|
||||
|
||||
update_data: Final = {
|
||||
"user_email": user_data["user_email"],
|
||||
"user_alias": user_data["user_alias"],
|
||||
"sso_user_id": user_data["sso_user_id"],
|
||||
"teams": user_data["teams"],
|
||||
"teams": target_teams,
|
||||
"metadata": safe_dumps(metadata),
|
||||
}
|
||||
|
||||
admin_group: Final = await _get_scim_admin_group()
|
||||
if admin_group is not None:
|
||||
if admin_group is not None and user_data["teams"]:
|
||||
update_data["user_role"] = _resolve_scim_user_role(
|
||||
user.groups or [], admin_group, _default_scim_user_role()
|
||||
)
|
||||
|
|
|
|||
|
|
@ -19,11 +19,13 @@ from litellm.proxy._types import (
|
|||
NewUserResponse,
|
||||
ProxyErrorTypes,
|
||||
ProxyException,
|
||||
UserAPIKeyAuth,
|
||||
)
|
||||
from litellm.proxy.management_endpoints.scim.scim_v2 import (
|
||||
SCIMRosterSyncError,
|
||||
UserProvisionerHelpers,
|
||||
_apply_group_patch_updates,
|
||||
_create_user_if_not_exists,
|
||||
_extract_group_member_ids,
|
||||
_extract_ids_from_path_filter,
|
||||
_handle_group_membership_changes,
|
||||
|
|
@ -37,8 +39,8 @@ from litellm.proxy.management_endpoints.scim.scim_v2 import (
|
|||
delete_group,
|
||||
delete_user,
|
||||
get_groups,
|
||||
get_users,
|
||||
get_service_provider_config,
|
||||
get_users,
|
||||
merge_placeholder,
|
||||
patch_group,
|
||||
patch_team_membership,
|
||||
|
|
@ -304,6 +306,85 @@ async def test_create_user_uses_default_internal_user_params_role(mocker, monkey
|
|||
assert called_args.user_role == LitellmUserRoles.PROXY_ADMIN
|
||||
|
||||
|
||||
def _mock_scim_create_user_deps(mocker: MockerFixture, scim_user: SCIMUser) -> AsyncMock:
|
||||
mock_prisma_client = mocker.MagicMock()
|
||||
mock_prisma_client.db.litellm_usertable.find_unique = AsyncMock(return_value=None)
|
||||
mock_prisma_client.db.litellm_usertable.find_many = AsyncMock(return_value=())
|
||||
mock_prisma_client.db.litellm_usertable.find_first = AsyncMock(return_value=None)
|
||||
mocker.patch( # test-quality-ok: endpoint collaborators are module-level, not injectable into create_user
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2._get_prisma_client_or_raise_exception",
|
||||
AsyncMock(return_value=mock_prisma_client),
|
||||
)
|
||||
mocker.patch( # test-quality-ok: endpoint collaborators are module-level, not injectable into create_user
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2.ScimTransformations.transform_litellm_user_to_scim_user",
|
||||
AsyncMock(return_value=scim_user),
|
||||
)
|
||||
return mocker.patch( # test-quality-ok: endpoint collaborators are module-level, not injectable into create_user
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2.new_user",
|
||||
AsyncMock(return_value=NewUserRequest(user_id=scim_user.userName)),
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_user_without_groups_defers_to_default_team(mocker: MockerFixture, monkeypatch):
|
||||
"""IdPs omit groups on POST /Users; teams must stay unset so new_user applies default_internal_user_params.teams"""
|
||||
scim_user = SCIMUser(
|
||||
schemas=["urn:ietf:params:scim:schemas:core:2.0:User"],
|
||||
userName="new-user",
|
||||
emails=[SCIMUserEmail(value="new@example.com")],
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
"litellm.default_internal_user_params",
|
||||
{"teams": [{"team_id": "default-team", "max_budget_in_team": 25}]},
|
||||
raising=False,
|
||||
)
|
||||
new_user_mock = _mock_scim_create_user_deps(mocker, scim_user)
|
||||
|
||||
await create_user(user=scim_user)
|
||||
|
||||
assert new_user_mock.call_args.kwargs["data"].teams is None
|
||||
assert new_user_mock.call_args.kwargs["user_api_key_dict"] == UserAPIKeyAuth(user_role=LitellmUserRoles.PROXY_ADMIN)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_user_with_groups_keeps_idp_teams(mocker: MockerFixture, monkeypatch):
|
||||
scim_user = SCIMUser(
|
||||
schemas=["urn:ietf:params:scim:schemas:core:2.0:User"],
|
||||
userName="new-user",
|
||||
emails=[SCIMUserEmail(value="new@example.com")],
|
||||
groups=[SCIMUserGroup(value="idp-team")],
|
||||
)
|
||||
monkeypatch.setattr(
|
||||
"litellm.default_internal_user_params",
|
||||
{"teams": [{"team_id": "default-team", "max_budget_in_team": 25}]},
|
||||
raising=False,
|
||||
)
|
||||
new_user_mock = _mock_scim_create_user_deps(mocker, scim_user)
|
||||
|
||||
await create_user(user=scim_user)
|
||||
|
||||
assert new_user_mock.call_args.kwargs["data"].teams == ["idp-team"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_user_if_not_exists_defers_to_default_team(mocker: MockerFixture, monkeypatch):
|
||||
monkeypatch.setattr(
|
||||
"litellm.default_internal_user_params",
|
||||
{"teams": [{"team_id": "default-team", "max_budget_in_team": 25}]},
|
||||
raising=False,
|
||||
)
|
||||
new_user_mock = mocker.patch( # test-quality-ok: new_user is imported inside the helper, not injectable
|
||||
"litellm.proxy.management_endpoints.internal_user_endpoints.new_user",
|
||||
AsyncMock(return_value=NewUserResponse(user_id="group-user", key="k")),
|
||||
)
|
||||
|
||||
created = await _create_user_if_not_exists(user_id="group-user")
|
||||
|
||||
assert created is not None
|
||||
assert new_user_mock.call_args.kwargs["data"].teams is None
|
||||
assert new_user_mock.call_args.kwargs["user_api_key_dict"] == UserAPIKeyAuth(user_role=LitellmUserRoles.PROXY_ADMIN)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_scim_create_user_respects_default_role_set_via_ui(mocker, monkeypatch):
|
||||
"""
|
||||
|
|
@ -1176,6 +1257,67 @@ async def test_update_user_success(mocker):
|
|||
assert call_args[1]["data"]["teams"] == ["new-team"]
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
@pytest.mark.parametrize("groups", [None, []], ids=["groups-omitted", "groups-empty"])
|
||||
async def test_update_user_without_groups_preserves_memberships_and_role(mocker, monkeypatch, groups):
|
||||
"""Okta profile PUTs carry no `groups` or `groups: []`; neither may drop teams (and their keys) or recompute role"""
|
||||
from litellm.proxy.proxy_server import proxy_config
|
||||
|
||||
async def mock_get_config():
|
||||
return {"litellm_settings": {"scim_admin_group": "litellm-admins"}}
|
||||
|
||||
monkeypatch.setattr(proxy_config, "get_config", mock_get_config)
|
||||
monkeypatch.setattr("litellm.default_internal_user_params", None, raising=False)
|
||||
|
||||
existing_user = mocker.MagicMock()
|
||||
existing_user.teams = ["litellm-admins", "engineering"]
|
||||
existing_user.metadata = {}
|
||||
|
||||
scim_user = SCIMUser(
|
||||
schemas=["urn:ietf:params:scim:schemas:core:2.0:User"],
|
||||
userName="okta-user",
|
||||
name=SCIMUserName(familyName="Renamed", givenName="Okta"),
|
||||
emails=[SCIMUserEmail(value="okta@example.com")],
|
||||
**({} if groups is None else {"groups": groups}),
|
||||
)
|
||||
response_scim_user = SCIMUser(
|
||||
schemas=["urn:ietf:params:scim:schemas:core:2.0:User"],
|
||||
id="okta-user",
|
||||
userName="okta-user",
|
||||
emails=[SCIMUserEmail(value="okta@example.com")],
|
||||
)
|
||||
|
||||
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(return_value={"user_id": "okta-user"})
|
||||
|
||||
mocker.patch( # test-quality-ok: update_user's collaborators are module-level, not injectable
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2._get_prisma_client_or_raise_exception",
|
||||
AsyncMock(return_value=mock_prisma_client),
|
||||
)
|
||||
mocker.patch( # test-quality-ok: update_user's collaborators are module-level, not injectable
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2._check_user_exists",
|
||||
AsyncMock(return_value=existing_user),
|
||||
)
|
||||
mocker.patch( # test-quality-ok: update_user's collaborators are module-level, not injectable
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2.ScimTransformations.transform_litellm_user_to_scim_user",
|
||||
AsyncMock(return_value=response_scim_user),
|
||||
)
|
||||
patch_membership = mocker.patch( # test-quality-ok: roster writes are module-level, not injectable
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2.patch_team_membership",
|
||||
AsyncMock(),
|
||||
)
|
||||
|
||||
result = await update_user(user_id="okta-user", user=scim_user)
|
||||
|
||||
assert result == response_scim_user
|
||||
patch_membership.assert_not_awaited()
|
||||
update_data = mock_prisma_client.db.litellm_usertable.update.call_args.kwargs["data"]
|
||||
assert update_data["teams"] == ["litellm-admins", "engineering"]
|
||||
assert "user_role" not in update_data
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_update_user_not_found(mocker):
|
||||
"""Should raise 404 when user doesn't exist"""
|
||||
|
|
|
|||
|
|
@ -3,7 +3,7 @@
|
|||
"limit": 22328
|
||||
},
|
||||
"LIT002": {
|
||||
"limit": 26760
|
||||
"limit": 26758
|
||||
},
|
||||
"LIT003": {
|
||||
"limit": 261
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue