diff --git a/litellm/proxy/management_endpoints/scim/scim_v2.py b/litellm/proxy/management_endpoints/scim/scim_v2.py index 5bce2807487..4ec13177e0b 100644 --- a/litellm/proxy/management_endpoints/scim/scim_v2.py +++ b/litellm/proxy/management_endpoints/scim/scim_v2.py @@ -206,20 +206,35 @@ def _build_scim_metadata( async def _extract_group_member_ids(group: SCIMGroup) -> GroupMemberExtractionResult: """ - Extract member IDs from SCIMGroup, creating users that don't exist. + Extract member IDs from SCIMGroup, validating that all users exist. + + Per SCIM 2.0 protocol, groups should only reference existing users. + Users must be created via POST /Users before being added to groups. Returns: - GroupMemberExtractionResult with existing members, created users, and all member IDs + GroupMemberExtractionResult with existing members and all member IDs + + Raises: + HTTPException: If any member user does not exist (400 Bad Request) """ prisma_client = await _get_prisma_client_or_raise_exception() existing_member_ids = [] - created_users = [] + created_users = [] # Always empty - users must exist before group membership all_member_ids = [] if group.members: for member in group.members: user_id = member.value + # Validate user_id is not empty + if not user_id or not user_id.strip(): + raise HTTPException( + status_code=400, + detail={ + "error": "Invalid member: user ID cannot be empty." + }, + ) + # Check if user exists user = await prisma_client.db.litellm_usertable.find_unique( where={"user_id": user_id} @@ -229,16 +244,17 @@ async def _extract_group_member_ids(group: SCIMGroup) -> GroupMemberExtractionRe 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" + # User doesn't exist - reject per SCIM 2.0 protocol + # This prevents security issues where users not assigned to app + # get provisioned via group membership + raise HTTPException( + status_code=400, + detail={ + "error": f"User with ID '{user_id}' does not exist. " + "Please create the user first via POST /Users before adding to group." + }, ) - if created_user: - created_users.append(created_user) - all_member_ids.append(user_id) - # If creation failed, user is skipped (logged in helper) - return GroupMemberExtractionResult( existing_member_ids=existing_member_ids, created_users=created_users, @@ -1024,7 +1040,7 @@ async def create_group( detail={"error": f"Group already exists with ID: {team_id}"}, ) - # Extract and process group members (creating users that don't exist) + # Extract and validate group members (all users must exist) member_result = await _extract_group_member_ids(group) members_with_roles = [ Member(user_id=member_id, role="user") @@ -1072,7 +1088,7 @@ async def update_group( prisma_client = await _get_prisma_client_or_raise_exception() existing_team = await _check_team_exists(group_id) - # Extract and process group members (creating users that don't exist) + # Extract and validate group members (all users must 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}" @@ -1189,24 +1205,33 @@ async def _process_group_patch_operations( elif path.startswith("members"): # Handle member operations member_values = _extract_group_values(value) - # Create users that don't exist and get all valid member IDs + # Validate all users exist - per SCIM 2.0, users must exist before group membership valid_members = [] for member_id in member_values: + # Validate member_id is not empty + if not member_id or not member_id.strip(): + raise HTTPException( + status_code=400, + detail={ + "error": "Invalid member: user ID cannot be empty." + }, + ) + user = await prisma_client.db.litellm_usertable.find_unique( where={"user_id": member_id} ) 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" + # User doesn't exist - reject per SCIM 2.0 protocol + raise HTTPException( + status_code=400, + detail={ + "error": f"User with ID '{member_id}' does not exist. " + "Please create the user first via POST /Users before adding to group." + }, ) - 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) elif op_type == "add": 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 230e251a5d0..15c4272fa90 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 @@ -915,10 +915,11 @@ async def test_update_group_e2e(mocker): @pytest.mark.asyncio -async def test_create_group_with_nonexistent_users_creates_users(mocker): +async def test_create_group_with_nonexistent_users_rejects(mocker): """ - Test that creating a group with non-existent users creates those users. - This tests the scenario: Group Push ['new user', existing users...] + Test that creating a group with non-existent users is rejected. + Per SCIM 2.0 protocol, users must exist before being added to groups. + This prevents security issues where users not assigned to app get provisioned via group membership. """ # Test data group_id = "test-group-123" @@ -934,7 +935,7 @@ async def test_create_group_with_nonexistent_users_creates_users(mocker): ) ######################################################### - # We expect new-user-1 and new-user-2 to be created + # We expect the request to be rejected with 400 error ######################################################### # Mock prisma client @@ -963,95 +964,21 @@ async def test_create_group_with_nonexistent_users_creates_users(mocker): 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() - ) + # Execute the create_group function - should raise HTTPException + with pytest.raises(HTTPException) as exc_info: + await create_group(group=scim_group) - # Mock created users return values - def mock_new_user_side_effect(data): - from litellm.proxy._types import NewUserResponse - return NewUserResponse( - key="sk-test-key-" + data.user_id, # Required field from GenerateKeyResponse - 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 + # Verify it's a 400 Bad Request + assert exc_info.value.status_code == 400 + assert "does not exist" in str(exc_info.value.detail) + assert "new-user-1" in str(exc_info.value.detail) or "new-user-2" in str(exc_info.value.detail) @pytest.mark.asyncio -async def test_update_group_with_nonexistent_users_creates_users(mocker): +async def test_update_group_with_nonexistent_users_rejects(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 that updating a group with non-existent users is rejected. + Per SCIM 2.0 protocol, users must exist before being added to groups. """ # Test data group_id = "existing-group-456" @@ -1114,79 +1041,11 @@ async def test_update_group_with_nonexistent_users_creates_users(mocker): 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() - ) + # Execute the update_group function - should raise HTTPException + with pytest.raises(HTTPException) as exc_info: + await update_group(group_id=group_id, group=scim_group_update) - # Mock created users return values - def mock_new_user_side_effect(data): - from litellm.proxy._types import NewUserResponse - return NewUserResponse( - key="sk-test-key-" + data.user_id, # Required field from GenerateKeyResponse - 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 + # Verify it's a 400 Bad Request + assert exc_info.value.status_code == 400 + assert "does not exist" in str(exc_info.value.detail) + assert "new-user-3" in str(exc_info.value.detail) or "new-user-4" in str(exc_info.value.detail) \ No newline at end of file