mirror of
https://github.com/BerriAI/litellm.git
synced 2026-08-28 05:25:59 +00:00
fix(scim): keep the matched user_id on POST /Users email match (#37701)
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:
parent
1d7f675e52
commit
387a948263
2 changed files with 75 additions and 15 deletions
|
|
@ -164,6 +164,12 @@ class UserProvisionerHelpers:
|
|||
"""
|
||||
Check if a user with the given email already exists and update them if found.
|
||||
|
||||
The matched row keeps its existing user_id even when the SCIM userName differs.
|
||||
Virtual keys, team rosters, team/organization memberships and spend logs all
|
||||
reference that id, so re-keying the user row would strand every one of them and
|
||||
make removals against rosters holding the old id no-op. SCIM ids are opaque to
|
||||
the client, which reads the stable id back from the response.
|
||||
|
||||
When admin_group is configured the resolved global role on new_user_request
|
||||
is persisted too, so re-upserting an existing email demotes a user who is no
|
||||
longer in the admin group instead of leaving the stale role.
|
||||
|
|
@ -189,20 +195,22 @@ class UserProvisionerHelpers:
|
|||
new_teams: Final = list(dict.fromkeys(new_user_request.teams or []))
|
||||
|
||||
if new_user_request.user_id != existing_user.user_id:
|
||||
await _table(UserRepository(prisma_client)).update(
|
||||
where={"user_id": existing_user.user_id},
|
||||
data={"user_id": new_user_request.user_id},
|
||||
verbose_proxy_logger.info(
|
||||
"SCIM: email %s already provisioned as user_id=%s, keeping that id instead of re-keying to %s",
|
||||
new_user_request.user_email,
|
||||
existing_user.user_id,
|
||||
new_user_request.user_id,
|
||||
)
|
||||
|
||||
await _handle_team_membership_changes(
|
||||
user_id=new_user_request.user_id,
|
||||
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(
|
||||
where={"user_id": new_user_request.user_id},
|
||||
where={"user_id": existing_user.user_id},
|
||||
data={
|
||||
"user_email": new_user_request.user_email,
|
||||
"user_alias": new_user_request.user_alias,
|
||||
|
|
|
|||
|
|
@ -548,7 +548,12 @@ async def test_handle_existing_user_by_email_no_existing_user(mocker):
|
|||
|
||||
@pytest.mark.asyncio
|
||||
async def test_handle_existing_user_by_email_existing_user_updated(mocker):
|
||||
"""Should rename the existing user, sync team roster, and return SCIMUser"""
|
||||
"""Should keep the existing user_id, sync team roster, and return SCIMUser
|
||||
|
||||
Regression: a SCIM userName differing from the matched row's user_id used to
|
||||
re-key the user row, orphaning virtual keys, team rosters, memberships and
|
||||
spend logs that still referenced the old id.
|
||||
"""
|
||||
existing_user = mocker.MagicMock()
|
||||
existing_user.user_id = "old-user-id"
|
||||
existing_user.user_email = "test@example.com"
|
||||
|
|
@ -557,7 +562,7 @@ async def test_handle_existing_user_by_email_existing_user_updated(mocker):
|
|||
existing_user.metadata = {"old": "data"}
|
||||
|
||||
updated_user = {
|
||||
"user_id": "new-user-id",
|
||||
"user_id": "old-user-id",
|
||||
"user_email": "test@example.com",
|
||||
"user_alias": "New Name",
|
||||
"teams": ["new-team"],
|
||||
|
|
@ -566,8 +571,8 @@ async def test_handle_existing_user_by_email_existing_user_updated(mocker):
|
|||
|
||||
mock_scim_user = SCIMUser(
|
||||
schemas=["urn:ietf:params:scim:schemas:core:2.0:User"],
|
||||
id="new-user-id",
|
||||
userName="new-user-id",
|
||||
id="old-user-id",
|
||||
userName="test@example.com",
|
||||
name=SCIMUserName(familyName="Name", givenName="New"),
|
||||
emails=[SCIMUserEmail(value="test@example.com")],
|
||||
)
|
||||
|
|
@ -605,13 +610,9 @@ async def test_handle_existing_user_by_email_existing_user_updated(mocker):
|
|||
mock_prisma_client.db.litellm_usertable.find_first.assert_called_once_with(where={"user_email": "test@example.com"})
|
||||
|
||||
update_calls = mock_prisma_client.db.litellm_usertable.update.call_args_list
|
||||
assert len(update_calls) == 2
|
||||
assert len(update_calls) == 1
|
||||
assert update_calls[0].kwargs == {
|
||||
"where": {"user_id": "old-user-id"},
|
||||
"data": {"user_id": "new-user-id"},
|
||||
}
|
||||
assert update_calls[1].kwargs == {
|
||||
"where": {"user_id": "new-user-id"},
|
||||
"data": {
|
||||
"user_email": "test@example.com",
|
||||
"user_alias": "New Name",
|
||||
|
|
@ -621,7 +622,7 @@ async def test_handle_existing_user_by_email_existing_user_updated(mocker):
|
|||
}
|
||||
|
||||
mock_membership.assert_awaited_once_with(
|
||||
user_id="new-user-id",
|
||||
user_id="old-user-id",
|
||||
existing_teams=["old-team"],
|
||||
new_teams=["new-team"],
|
||||
raise_on_error=True,
|
||||
|
|
@ -630,6 +631,57 @@ async def test_handle_existing_user_by_email_existing_user_updated(mocker):
|
|||
mock_transform.assert_called_once_with(updated_user)
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_handle_existing_user_by_email_roster_changes_use_existing_user_id(mocker):
|
||||
"""Roster add/remove must be issued for the matched row's user_id, not the SCIM userName.
|
||||
|
||||
Regression: the rename made removals run against the new id, so a roster still
|
||||
holding the old id reported "User not found in team" and the stale entry survived.
|
||||
"""
|
||||
existing_user = mocker.MagicMock()
|
||||
existing_user.user_id = "oidc-sub-123"
|
||||
existing_user.user_email = "member@example.com"
|
||||
existing_user.user_alias = "Member"
|
||||
existing_user.teams = ["old-team"]
|
||||
existing_user.metadata = {}
|
||||
|
||||
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_first = AsyncMock(return_value=existing_user)
|
||||
mock_prisma_client.db.litellm_usertable.update = AsyncMock(return_value={})
|
||||
|
||||
mock_team_member_add = mocker.patch(
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2.team_member_add",
|
||||
AsyncMock(),
|
||||
)
|
||||
mock_team_member_delete = mocker.patch(
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2.team_member_delete",
|
||||
AsyncMock(),
|
||||
)
|
||||
mocker.patch(
|
||||
"litellm.proxy.management_endpoints.scim.scim_v2.ScimTransformations.transform_litellm_user_to_scim_user",
|
||||
AsyncMock(return_value=None),
|
||||
)
|
||||
|
||||
new_user_request = NewUserRequest(
|
||||
user_id="scim-username",
|
||||
user_email="member@example.com",
|
||||
user_alias="Member",
|
||||
teams=["new-team"],
|
||||
metadata={},
|
||||
auto_create_key=False,
|
||||
)
|
||||
|
||||
await UserProvisionerHelpers.handle_existing_user_by_email(
|
||||
prisma_client=mock_prisma_client, new_user_request=new_user_request
|
||||
)
|
||||
|
||||
assert mock_team_member_add.await_args.kwargs["data"].member.user_id == "oidc-sub-123"
|
||||
assert mock_team_member_delete.await_args.kwargs["data"].user_id == "oidc-sub-123"
|
||||
assert mock_prisma_client.db.litellm_usertable.update.await_args.kwargs["where"] == {"user_id": "oidc-sub-123"}
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_handle_existing_user_by_email_syncs_roster_and_dedups_teams(mocker):
|
||||
"""Existing-email upsert must add the user to the team roster via the shared
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue