From cebacd65cf7061db3afe4eb96cf10a7cb95b6eb4 Mon Sep 17 00:00:00 2001 From: Ishaan Jaff Date: Mon, 15 Sep 2025 11:27:05 -0700 Subject: [PATCH] [Bug Fix] SCIM v2 - ensure group PUSH and PUT ops allow creating non-existent members (#14581) * fix: scim handle non existent members * test - scim v2 * test fix * fix: NewUserResponse --- .../management_endpoints/scim/scim_v2.py | 123 +++++++- .../scim/test_scim_v2_endpoints.py | 279 +++++++++++++++++- 2 files changed, 386 insertions(+), 16 deletions(-) diff --git a/litellm/proxy/management_endpoints/scim/scim_v2.py b/litellm/proxy/management_endpoints/scim/scim_v2.py index f84b4df42d..6720f0c3b7 100644 --- a/litellm/proxy/management_endpoints/scim/scim_v2.py +++ b/litellm/proxy/management_endpoints/scim/scim_v2.py @@ -18,6 +18,7 @@ from fastapi import ( Response, ) from typing_extensions import TypedDict +from pydantic import BaseModel import litellm from litellm._logging import verbose_proxy_logger @@ -29,6 +30,7 @@ from litellm.proxy._types import ( Member, NewTeamRequest, NewUserRequest, + NewUserResponse, TeamMemberAddRequest, TeamMemberDeleteRequest, UserAPIKeyAuth, @@ -101,6 +103,13 @@ class ScimUserData(TypedDict): active: Optional[bool] +class GroupMemberExtractionResult(BaseModel): + """Result of extracting and processing group members.""" + existing_member_ids: List[str] + created_users: List[NewUserResponse] + all_member_ids: List[str] # existing + newly created + + scim_router = APIRouter( prefix="/scim/v2", tags=["✨ SCIM v2 (Enterprise Only)"], @@ -190,21 +199,47 @@ def _build_scim_metadata(given_name: Optional[str], family_name: Optional[str], return metadata -async def _extract_group_member_ids(group: SCIMGroup) -> List[str]: - """Extract valid member IDs from SCIMGroup, verifying users exist.""" +async def _extract_group_member_ids(group: SCIMGroup) -> GroupMemberExtractionResult: + """ + Extract member IDs from SCIMGroup, creating users that don't exist. + + Returns: + GroupMemberExtractionResult with existing members, created users, and all member IDs + """ prisma_client = await _get_prisma_client_or_raise_exception() - member_ids = [] + existing_member_ids = [] + created_users = [] + all_member_ids = [] if group.members: for member in group.members: + user_id = member.value + # Check if user exists user = await prisma_client.db.litellm_usertable.find_unique( - where={"user_id": member.value} + where={"user_id": user_id} ) + if user: - member_ids.append(member.value) + existing_member_ids.append(user_id) + all_member_ids.append(user_id) + else: + # Create the user if they don't exist using our helper + created_user = await _create_user_if_not_exists( + user_id=user_id, + created_via="scim_group_membership" + ) + + if created_user: + created_users.append(created_user) + all_member_ids.append(user_id) + # If creation failed, user is skipped (logged in helper) - return member_ids + return GroupMemberExtractionResult( + existing_member_ids=existing_member_ids, + created_users=created_users, + all_member_ids=all_member_ids + ) async def _get_team_members_display(member_ids: List[str]) -> List[SCIMMember]: @@ -239,6 +274,51 @@ async def _handle_team_membership_changes(user_id: str, existing_teams: List[str ) +async def _create_user_if_not_exists(user_id: str, created_via: str = "scim_group") -> Optional[NewUserResponse]: + """ + Helper function to create a user if they don't exist. + + Args: + user_id: The user ID to create + created_via: Context for where the user was created from + + Returns: + LiteLLM_UserTable if user was created, None if creation failed + """ + from litellm.proxy.management_endpoints.internal_user_endpoints import new_user + + try: + # Get default role for new internal users + default_role: Optional[ + Literal[ + LitellmUserRoles.PROXY_ADMIN, + LitellmUserRoles.PROXY_ADMIN_VIEW_ONLY, + LitellmUserRoles.INTERNAL_USER, + LitellmUserRoles.INTERNAL_USER_VIEW_ONLY, + ] + ] = LitellmUserRoles.INTERNAL_USER_VIEW_ONLY + if litellm.default_internal_user_params: + default_role = litellm.default_internal_user_params.get("user_role") + + new_user_request = NewUserRequest( + 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 = await new_user(data=new_user_request) + verbose_proxy_logger.info(f"Created user {user_id} via {created_via}") + return created_user + + except Exception as e: + verbose_proxy_logger.exception(f"Failed to create user {user_id}: {e}") + return None + + async def _get_team_member_user_ids_from_team(team: LiteLLM_TeamTable) -> List[str]: """ Get the IDs of the members from a team. @@ -256,6 +336,8 @@ async def _get_team_member_user_ids_from_team(team: LiteLLM_TeamTable) -> List[s member_user_ids.append(user_id) return member_user_ids + + # Dependency to set the correct SCIM Content-Type async def set_scim_content_type(response: Response): """Sets the Content-Type header to application/scim+json""" @@ -914,9 +996,9 @@ async def create_group( detail={"error": f"Group already exists with ID: {team_id}"}, ) - # Extract valid member IDs - member_ids = await _extract_group_member_ids(group) - members_with_roles = [Member(user_id=member_id, role="user") for member_id in member_ids] + # Extract and process group members (creating users that don't exist) + member_result = await _extract_group_member_ids(group) + members_with_roles = [Member(user_id=member_id, role="user") for member_id in member_result.all_member_ids] # Create team in database created_team = await new_team( @@ -959,9 +1041,10 @@ async def update_group( prisma_client = await _get_prisma_client_or_raise_exception() existing_team = await _check_team_exists(group_id) - # Extract valid member IDs - member_ids = await _extract_group_member_ids(group) - verbose_proxy_logger.debug(f"SCIM PUT GROUP member_ids: {member_ids}") + # Extract and process group members (creating users that don't exist) + member_result = await _extract_group_member_ids(group) + verbose_proxy_logger.debug(f"SCIM PUT GROUP all_member_ids: {member_result.all_member_ids}") + verbose_proxy_logger.debug(f"SCIM PUT GROUP created_users: {len(member_result.created_users)}") # Prepare update data existing_metadata = existing_team.metadata if existing_team.metadata else {} @@ -978,10 +1061,10 @@ async def update_group( data=update_data, ) - # Handle user-team relationship changes using the same approach as patch_group + # Handle user-team relationship changes current_members = set(await _get_team_member_user_ids_from_team(existing_team)) verbose_proxy_logger.debug(f"SCIM PUT GROUP current_members: {current_members}") - final_members = set(member_ids) + final_members = set(member_result.all_member_ids) verbose_proxy_logger.debug(f"SCIM PUT GROUP final_members: {final_members}") await _handle_group_membership_changes( @@ -1075,7 +1158,7 @@ async def _process_group_patch_operations( elif path.startswith("members"): # Handle member operations member_values = _extract_group_values(value) - # Validate that users exist + # Create users that don't exist and get all valid member IDs valid_members = [] for member_id in member_values: user = await prisma_client.db.litellm_usertable.find_unique( @@ -1083,6 +1166,16 @@ async def _process_group_patch_operations( ) if user: valid_members.append(member_id) + else: + # Create the user if they don't exist using our helper + created_user = await _create_user_if_not_exists( + user_id=member_id, + created_via="scim_group_patch" + ) + + if created_user: + valid_members.append(member_id) + # If creation failed, user is skipped (logged in helper) if op_type == "replace": final_members = set(valid_members) 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 959275787c..5cbd602268 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 @@ -7,6 +7,7 @@ from litellm.proxy._types import LitellmUserRoles, NewUserRequest, ProxyExceptio from litellm.proxy.management_endpoints.scim.scim_v2 import ( UserProvisionerHelpers, _handle_team_membership_changes, + create_group, create_user, get_service_provider_config, patch_user, @@ -910,4 +911,280 @@ async def test_update_group_e2e(mocker): assert len(result.members) == 3 # Verify SCIM transformation was called with updated team - ScimTransformations.transform_litellm_team_to_scim_group.assert_called_once_with(updated_team) \ No newline at end of file + ScimTransformations.transform_litellm_team_to_scim_group.assert_called_once_with(updated_team) + + +@pytest.mark.asyncio +async def test_create_group_with_nonexistent_users_creates_users(mocker): + """ + Test that creating a group with non-existent users creates those users. + This tests the scenario: Group Push ['new user', existing users...] + """ + # Test data + group_id = "test-group-123" + scim_group = SCIMGroup( + schemas=["urn:ietf:params:scim:schemas:core:2.0:Group"], + id=group_id, + displayName="Test Group", + members=[ + SCIMMember(value="existing-user", display="Existing User"), # This user exists + SCIMMember(value="new-user-1", display="New User 1"), # This user doesn't exist + SCIMMember(value="new-user-2", display="New User 2"), # This user doesn't exist + ] + ) + + ######################################################### + # We expect new-user-1 and new-user-2 to be created + ######################################################### + + # Mock prisma client + mock_prisma_client = mocker.MagicMock() + mock_prisma_client.db = mocker.MagicMock() + mock_prisma_client.db.litellm_teamtable = mocker.MagicMock() + mock_prisma_client.db.litellm_usertable = mocker.MagicMock() + + # Mock team operations - team doesn't exist yet + mock_prisma_client.db.litellm_teamtable.find_unique = AsyncMock(return_value=None) + + # Mock user lookup - only existing-user exists + def mock_user_lookup(where): + user_id = where["user_id"] + if user_id == "existing-user": + mock_user = mocker.MagicMock() + mock_user.user_id = user_id + return mock_user + return None # new-user-1 and new-user-2 don't exist + + mock_prisma_client.db.litellm_usertable.find_unique = AsyncMock(side_effect=mock_user_lookup) + + # Mock dependencies + mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2._get_prisma_client_or_raise_exception", + AsyncMock(return_value=mock_prisma_client) + ) + + # Mock new_user function to track user creation + mock_new_user = mocker.patch( + "litellm.proxy.management_endpoints.internal_user_endpoints.new_user", + AsyncMock() + ) + + # Mock created users return values + def mock_new_user_side_effect(data): + from litellm.proxy._types import LiteLLM_UserTable + return LiteLLM_UserTable( + user_id=data.user_id, + user_email=data.user_email, + metadata=data.metadata, + teams=data.teams, + user_role=data.user_role + ) + + mock_new_user.side_effect = mock_new_user_side_effect + + # Mock new_team function + mock_created_team = mocker.MagicMock() + mock_created_team.team_id = group_id + mock_created_team.team_alias = "Test Group" + + mock_new_team = mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2.new_team", + AsyncMock(return_value=mock_created_team) + ) + + # Mock SCIM transformation + expected_scim_response = SCIMGroup( + schemas=["urn:ietf:params:scim:schemas:core:2.0:Group"], + id=group_id, + displayName="Test Group", + members=[ + SCIMMember(value="existing-user", display="existing-user"), + SCIMMember(value="new-user-1", display="new-user-1"), + SCIMMember(value="new-user-2", display="new-user-2") + ] + ) + mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2.ScimTransformations.transform_litellm_team_to_scim_group", + AsyncMock(return_value=expected_scim_response) + ) + + # Execute the create_group function + result = await create_group(group=scim_group) + + ######################################################### + # Assert that new-user-1 and new-user-2 were created + ######################################################### + + # Verify that new_user was called exactly twice (for new-user-1 and new-user-2) + assert mock_new_user.call_count == 2 + + # Check the user creation calls + created_user_ids = set() + for call in mock_new_user.call_args_list: + user_request = call.kwargs["data"] + created_user_ids.add(user_request.user_id) + assert user_request.metadata["created_via"] == "scim_group_membership" + assert user_request.user_role == LitellmUserRoles.INTERNAL_USER_VIEW_ONLY + assert user_request.auto_create_key is False + assert user_request.teams == [] # Teams added separately + + assert created_user_ids == {"new-user-1", "new-user-2"} + + # Verify team creation was called with all members (existing + created) + mock_new_team.assert_called_once() + team_request = mock_new_team.call_args.kwargs["data"] + assert team_request.team_id == group_id + assert team_request.team_alias == "Test Group" + + # Verify all members are in the team (existing + newly created) + member_user_ids = {member.user_id for member in team_request.members_with_roles} + assert member_user_ids == {"existing-user", "new-user-1", "new-user-2"} + + # Verify response + assert result.id == group_id + assert result.displayName == "Test Group" + assert len(result.members) == 3 + + +@pytest.mark.asyncio +async def test_update_group_with_nonexistent_users_creates_users(mocker): + """ + Test that updating a group with non-existent users creates those users. + This tests the scenario where a group is updated with members that don't exist in user table. + """ + # Test data + group_id = "existing-group-456" + + # Mock existing team + mock_existing_team = mocker.MagicMock() + mock_existing_team.team_id = group_id + mock_existing_team.team_alias = "Old Group Name" + mock_existing_team.members = ["old-user"] + mock_existing_team.members_with_roles = [{"user_id": "old-user", "role": "user"}] + mock_existing_team.metadata = {"existing": "data"} + + # SCIM group update request + scim_group_update = SCIMGroup( + schemas=["urn:ietf:params:scim:schemas:core:2.0:Group"], + id=group_id, + displayName="Updated Group Name", + members=[ + SCIMMember(value="existing-user", display="Existing User"), # This user exists + SCIMMember(value="new-user-3", display="New User 3"), # This user doesn't exist + SCIMMember(value="new-user-4", display="New User 4"), # This user doesn't exist + ] + ) + + # Mock prisma client + mock_prisma_client = mocker.MagicMock() + mock_prisma_client.db = mocker.MagicMock() + mock_prisma_client.db.litellm_teamtable = mocker.MagicMock() + mock_prisma_client.db.litellm_usertable = mocker.MagicMock() + + # Mock team operations + mock_prisma_client.db.litellm_teamtable.find_unique = AsyncMock(return_value=mock_existing_team) + + # Mock updated team response + mock_updated_team = mocker.MagicMock() + mock_updated_team.team_id = group_id + mock_updated_team.team_alias = "Updated Group Name" + mock_updated_team.members = ["existing-user", "new-user-3", "new-user-4"] + mock_prisma_client.db.litellm_teamtable.update = AsyncMock(return_value=mock_updated_team) + + # Mock user lookup - only existing-user exists + def mock_user_lookup(where): + user_id = where["user_id"] + if user_id == "existing-user": + mock_user = mocker.MagicMock() + mock_user.user_id = user_id + return mock_user + return None # new-user-3 and new-user-4 don't exist + + mock_prisma_client.db.litellm_usertable.find_unique = AsyncMock(side_effect=mock_user_lookup) + + # Mock dependencies + 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_team_exists", + AsyncMock(return_value=mock_existing_team) + ) + + # Mock new_user function to track user creation + mock_new_user = mocker.patch( + "litellm.proxy.management_endpoints.internal_user_endpoints.new_user", + AsyncMock() + ) + + # Mock created users return values + def mock_new_user_side_effect(data): + from litellm.proxy._types import LiteLLM_UserTable + return LiteLLM_UserTable( + user_id=data.user_id, + user_email=data.user_email, + metadata=data.metadata, + teams=data.teams, + user_role=data.user_role + ) + + mock_new_user.side_effect = mock_new_user_side_effect + + # Mock group membership changes + mock_handle_group_membership_changes = mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2._handle_group_membership_changes", + AsyncMock() + ) + + # Mock SCIM transformation + expected_scim_response = SCIMGroup( + schemas=["urn:ietf:params:scim:schemas:core:2.0:Group"], + id=group_id, + displayName="Updated Group Name", + members=[ + SCIMMember(value="existing-user", display="existing-user"), + SCIMMember(value="new-user-3", display="new-user-3"), + SCIMMember(value="new-user-4", display="new-user-4") + ] + ) + mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2.ScimTransformations.transform_litellm_team_to_scim_group", + AsyncMock(return_value=expected_scim_response) + ) + + # Execute the update_group function + result = await update_group(group_id=group_id, group=scim_group_update) + + # Verify that new_user was called exactly twice (for new-user-3 and new-user-4) + assert mock_new_user.call_count == 2 + + # Check the user creation calls + created_user_ids = set() + for call in mock_new_user.call_args_list: + user_request = call.kwargs["data"] + created_user_ids.add(user_request.user_id) + assert user_request.metadata["created_via"] == "scim_group_membership" + assert user_request.user_role == LitellmUserRoles.INTERNAL_USER_VIEW_ONLY + assert user_request.auto_create_key is False + assert user_request.teams == [] # Teams added separately + + assert created_user_ids == {"new-user-3", "new-user-4"} + + # Verify team update was called + mock_prisma_client.db.litellm_teamtable.update.assert_called_once() + update_call = mock_prisma_client.db.litellm_teamtable.update.call_args + assert update_call[1]["where"]["team_id"] == group_id + assert update_call[1]["data"]["team_alias"] == "Updated Group Name" + + # Verify group membership changes were handled with all members (existing + created) + mock_handle_group_membership_changes.assert_called_once() + membership_call = mock_handle_group_membership_changes.call_args + assert membership_call[1]["group_id"] == group_id + assert membership_call[1]["final_members"] == {"existing-user", "new-user-3", "new-user-4"} + + # Verify response + assert result.id == group_id + assert result.displayName == "Updated Group Name" + assert len(result.members) == 3 \ No newline at end of file