From ebb0f7e4cf1bfde5f720d89b76c4311176851878 Mon Sep 17 00:00:00 2001 From: Krrish Dholakia Date: Wed, 15 Jul 2026 18:40:32 +0000 Subject: [PATCH 01/41] fix(responses): preserve reasoning through prompt hooks Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- litellm/responses/main.py | 18 +++- litellm/responses/utils.py | 50 ++++++++++ .../test_responses_prompt_management.py | 94 ++++++++++++++++--- 3 files changed, 148 insertions(+), 14 deletions(-) diff --git a/litellm/responses/main.py b/litellm/responses/main.py index 12f9be970c7..453676937fc 100644 --- a/litellm/responses/main.py +++ b/litellm/responses/main.py @@ -494,7 +494,14 @@ async def aresponses( prompt_label=kwargs.get("prompt_label", None), prompt_version=kwargs.get("prompt_version", None), ) - input = cast(Union[str, ResponseInputParam], merged_input) + input = cast( + Union[str, ResponseInputParam], + ResponsesAPIRequestUtils.merge_prompt_management_input( + original_input=input, + client_input=client_input, + merged_input=merged_input, + ), + ) if model != original_model: _, custom_llm_provider, _, _ = litellm.get_llm_provider(model=model) kwargs.pop("prompt_id", None) @@ -609,7 +616,14 @@ def _apply_prompt_management_to_responses_call( prompt_label=kwargs.get("prompt_label", None), prompt_version=kwargs.get("prompt_version", None), ) - input = cast(Union[str, ResponseInputParam], merged_input) + input = cast( + Union[str, ResponseInputParam], + ResponsesAPIRequestUtils.merge_prompt_management_input( + original_input=input, + client_input=client_input, + merged_input=merged_input, + ), + ) local_vars["input"] = input local_vars["model"] = model if model != original_model: diff --git a/litellm/responses/utils.py b/litellm/responses/utils.py index 234eb777aca..6d42e33a268 100644 --- a/litellm/responses/utils.py +++ b/litellm/responses/utils.py @@ -19,7 +19,9 @@ import litellm from litellm._logging import verbose_logger from litellm.llms.base_llm.responses.transformation import BaseResponsesAPIConfig from litellm.types.llms.openai import ( + AllMessageValues, ResponseAPIUsage, + ResponseInputParam, ResponsesAPIOptionalRequestParams, ResponsesAPIResponse, ResponseText, @@ -36,6 +38,54 @@ from litellm.types.utils import ( class ResponsesAPIRequestUtils: """Helper utils for constructing ResponseAPI requests""" + @staticmethod + def merge_prompt_management_input( + original_input: str | ResponseInputParam, + client_input: list[AllMessageValues], + merged_input: list[AllMessageValues], + ) -> list[object]: + if isinstance(original_input, str): + return [*merged_input] + + original_items = tuple(original_input) + client_item_ids = frozenset(id(item) for item in client_input) + message_positions = tuple(index for index, item in enumerate(original_items) if id(item) in client_item_ids) + + if len(message_positions) == len(original_items): + return [*merged_input] + if not message_positions: + return [*merged_input, *original_items] + + corresponding_messages = len(client_input) == len(merged_input) and all( + original.get("role") == merged.get("role") + and (not isinstance(original.get("id"), str) or original.get("id") == merged.get("id")) + for original, merged in zip(client_input, merged_input) + ) + if corresponding_messages: + merged_by_position = dict(zip(message_positions, merged_input)) + return [ + merged_by_position[index] if index in merged_by_position else item + for index, item in enumerate(original_items) + ] + + all_messages_preserved = all(any(original is merged for merged in merged_input) for original in client_input) + if all_messages_preserved: + prefixes = { + id(original_items[position]): original_items[ + message_positions[index - 1] + 1 if index else 0 : position + ] + for index, position in enumerate(message_positions) + } + trailing_items = original_items[message_positions[-1] + 1 :] + return [item for merged in merged_input for item in (*prefixes.get(id(merged), ()), merged)] + list( + trailing_items + ) + + verbose_logger.warning( + "Prompt management hook replaced Responses API messages; non-message input items were dropped" + ) + return [*merged_input] + @staticmethod def _check_valid_arg( supported_params: Optional[List[str]], diff --git a/tests/test_litellm/responses/test_responses_prompt_management.py b/tests/test_litellm/responses/test_responses_prompt_management.py index 84e98390268..e4207b292da 100644 --- a/tests/test_litellm/responses/test_responses_prompt_management.py +++ b/tests/test_litellm/responses/test_responses_prompt_management.py @@ -14,13 +14,19 @@ Covers: """ import asyncio -from typing import List +from typing import List, cast from unittest.mock import AsyncMock, MagicMock, patch import pytest +from litellm.integrations.anthropic_cache_control_hook import ( + AnthropicCacheControlHook, +) from litellm.litellm_core_utils.litellm_logging import Logging as LiteLLMLoggingObj -from litellm.types.llms.openai import AllMessageValues +from litellm.types.llms.openai import ( + AllMessageValues, + ResponseInputParam, +) # --------------------------------------------------------------------------- # Helpers @@ -54,18 +60,15 @@ def _patch_responses_dispatch(): return_value=("gpt-4o", "openai", None, None), ), patch( - "litellm.responses.mcp.litellm_proxy_mcp_handler." - "LiteLLM_Proxy_MCP_Handler._should_use_litellm_mcp_gateway", + "litellm.responses.mcp.litellm_proxy_mcp_handler.LiteLLM_Proxy_MCP_Handler._should_use_litellm_mcp_gateway", return_value=False, ), patch( - "litellm.responses.main.ProviderConfigManager" - ".get_provider_responses_api_config", + "litellm.responses.main.ProviderConfigManager.get_provider_responses_api_config", return_value=None, ), patch( - "litellm.responses.main.litellm_completion_transformation_handler" - ".response_api_handler", + "litellm.responses.main.litellm_completion_transformation_handler.response_api_handler", return_value=MagicMock(), ), ] @@ -77,7 +80,6 @@ def _patch_responses_dispatch(): class TestResponsesAPIPromptManagement: - def test_str_input_coerced_and_merged(self): """[A] str input is wrapped into a message list before being passed to the hook.""" template_messages: List[AllMessageValues] = [ @@ -108,9 +110,7 @@ class TestResponsesAPIPromptManagement: logging_obj.get_chat_completion_prompt.assert_called_once() call_kwargs = logging_obj.get_chat_completion_prompt.call_args.kwargs # str was coerced to a single user message before being passed to the hook - assert call_kwargs["messages"] == [ - {"role": "user", "content": "Tell me about AI."} - ] + assert call_kwargs["messages"] == [{"role": "user", "content": "Tell me about AI."}] assert call_kwargs["prompt_id"] == "summariser-prompt" def test_list_input_merged_with_template(self): @@ -256,6 +256,76 @@ class TestResponsesAPIPromptManagement: assert all(isinstance(m, dict) and "role" in m for m in passed_messages) assert len(passed_messages) == 1 + def test_cache_control_hook_preserves_reasoning_items(self): + system_message = cast( + AllMessageValues, + {"role": "system", "content": "Analyze the request"}, + ) + assistant_message = cast( + AllMessageValues, + { + "type": "message", + "id": "msg_1", + "role": "assistant", + "status": "completed", + "content": [ + { + "type": "output_text", + "text": "The code has a bug", + "annotations": [], + } + ], + }, + ) + user_message = cast( + AllMessageValues, + {"role": "user", "content": "Check for security issues"}, + ) + reasoning_item = { + "type": "reasoning", + "id": "rs_1", + "summary": [], + "encrypted_content": "encrypted", + } + original_input = cast( + ResponseInputParam, + [system_message, reasoning_item, assistant_message, user_message], + ) + _, merged_messages, _ = AnthropicCacheControlHook().get_chat_completion_prompt( + model="azure/gpt-5-codex", + messages=[system_message, assistant_message, user_message], + non_default_params={"cache_control_injection_points": [{"location": "message", "role": "system"}]}, + prompt_id=None, + prompt_variables=None, + dynamic_callback_params={}, + ) + logging_obj = _make_logging_obj( + merged_model="azure/gpt-5-codex", + merged_messages=merged_messages, + ) + + patches = _patch_responses_dispatch() + with patches[0], patches[1], patches[2], patches[3] as mock_handler: + import litellm + + litellm.responses( + input=original_input, + model="azure/gpt-5-codex", + litellm_logging_obj=logging_obj, + cache_control_injection_points=[{"location": "message", "role": "system"}], + ) + + sent_input = mock_handler.call_args.kwargs["input"] + assert [item.get("type") for item in sent_input] == [ + None, + "reasoning", + "message", + None, + ] + assert sent_input[0]["cache_control"] == {"type": "ephemeral"} + assert sent_input[1] == reasoning_item + assert sent_input[2]["id"] == "msg_1" + def test_model_override_re_resolves_provider(self): """[G] When the prompt template overrides the model to a different provider, custom_llm_provider is re-resolved so downstream routing uses the correct provider. From 4baee71bdd1e82db5225122ad5d9a1a7ae925af2 Mon Sep 17 00:00:00 2001 From: Krrish Dholakia Date: Wed, 15 Jul 2026 18:41:02 +0000 Subject: [PATCH 02/41] chore(responses): minimize regression test diff Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- .../responses/test_responses_prompt_management.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/test_litellm/responses/test_responses_prompt_management.py b/tests/test_litellm/responses/test_responses_prompt_management.py index e4207b292da..b3ba81ee2e8 100644 --- a/tests/test_litellm/responses/test_responses_prompt_management.py +++ b/tests/test_litellm/responses/test_responses_prompt_management.py @@ -60,15 +60,18 @@ def _patch_responses_dispatch(): return_value=("gpt-4o", "openai", None, None), ), patch( - "litellm.responses.mcp.litellm_proxy_mcp_handler.LiteLLM_Proxy_MCP_Handler._should_use_litellm_mcp_gateway", + "litellm.responses.mcp.litellm_proxy_mcp_handler." + "LiteLLM_Proxy_MCP_Handler._should_use_litellm_mcp_gateway", return_value=False, ), patch( - "litellm.responses.main.ProviderConfigManager.get_provider_responses_api_config", + "litellm.responses.main.ProviderConfigManager" + ".get_provider_responses_api_config", return_value=None, ), patch( - "litellm.responses.main.litellm_completion_transformation_handler.response_api_handler", + "litellm.responses.main.litellm_completion_transformation_handler" + ".response_api_handler", return_value=MagicMock(), ), ] @@ -80,6 +83,7 @@ def _patch_responses_dispatch(): class TestResponsesAPIPromptManagement: + def test_str_input_coerced_and_merged(self): """[A] str input is wrapped into a message list before being passed to the hook.""" template_messages: List[AllMessageValues] = [ @@ -110,7 +114,9 @@ class TestResponsesAPIPromptManagement: logging_obj.get_chat_completion_prompt.assert_called_once() call_kwargs = logging_obj.get_chat_completion_prompt.call_args.kwargs # str was coerced to a single user message before being passed to the hook - assert call_kwargs["messages"] == [{"role": "user", "content": "Tell me about AI."}] + assert call_kwargs["messages"] == [ + {"role": "user", "content": "Tell me about AI."} + ] assert call_kwargs["prompt_id"] == "summariser-prompt" def test_list_input_merged_with_template(self): From 4db0bdf465c9f07e8561136cf055ca9e6854f086 Mon Sep 17 00:00:00 2001 From: Krrish Dholakia Date: Wed, 15 Jul 2026 18:51:07 +0000 Subject: [PATCH 03/41] fix(responses): handle non-message-only prompt input Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- litellm/responses/utils.py | 5 +- .../test_responses_prompt_management.py | 154 +++++++++++++----- 2 files changed, 116 insertions(+), 43 deletions(-) diff --git a/litellm/responses/utils.py b/litellm/responses/utils.py index 6d42e33a268..a5203c4ee6a 100644 --- a/litellm/responses/utils.py +++ b/litellm/responses/utils.py @@ -54,7 +54,10 @@ class ResponsesAPIRequestUtils: if len(message_positions) == len(original_items): return [*merged_input] if not message_positions: - return [*merged_input, *original_items] + verbose_logger.warning( + "Prompt management hook returned messages without Responses API input messages; merged messages were ignored" + ) + return [*original_items] corresponding_messages = len(client_input) == len(merged_input) and all( original.get("role") == merged.get("role") diff --git a/tests/test_litellm/responses/test_responses_prompt_management.py b/tests/test_litellm/responses/test_responses_prompt_management.py index b3ba81ee2e8..7044d8384f8 100644 --- a/tests/test_litellm/responses/test_responses_prompt_management.py +++ b/tests/test_litellm/responses/test_responses_prompt_management.py @@ -77,6 +77,56 @@ def _patch_responses_dispatch(): ] +def _make_cache_control_case() -> tuple[ + ResponseInputParam, + list[AllMessageValues], + dict[str, object], +]: + system_message = cast( + AllMessageValues, + {"role": "system", "content": "Analyze the request"}, + ) + assistant_message = cast( + AllMessageValues, + { + "type": "message", + "id": "msg_1", + "role": "assistant", + "status": "completed", + "content": [ + { + "type": "output_text", + "text": "The code has a bug", + "annotations": [], + } + ], + }, + ) + user_message = cast( + AllMessageValues, + {"role": "user", "content": "Check for security issues"}, + ) + reasoning_item = { + "type": "reasoning", + "id": "rs_1", + "summary": [], + "encrypted_content": "encrypted", + } + original_input = cast( + ResponseInputParam, + [system_message, reasoning_item, assistant_message, user_message], + ) + _, merged_messages, _ = AnthropicCacheControlHook().get_chat_completion_prompt( + model="azure/gpt-5-codex", + messages=[system_message, assistant_message, user_message], + non_default_params={"cache_control_injection_points": [{"location": "message", "role": "system"}]}, + prompt_id=None, + prompt_variables=None, + dynamic_callback_params={}, + ) + return original_input, merged_messages, reasoning_item + + # --------------------------------------------------------------------------- # Tests # --------------------------------------------------------------------------- @@ -263,48 +313,7 @@ class TestResponsesAPIPromptManagement: assert len(passed_messages) == 1 def test_cache_control_hook_preserves_reasoning_items(self): - system_message = cast( - AllMessageValues, - {"role": "system", "content": "Analyze the request"}, - ) - assistant_message = cast( - AllMessageValues, - { - "type": "message", - "id": "msg_1", - "role": "assistant", - "status": "completed", - "content": [ - { - "type": "output_text", - "text": "The code has a bug", - "annotations": [], - } - ], - }, - ) - user_message = cast( - AllMessageValues, - {"role": "user", "content": "Check for security issues"}, - ) - reasoning_item = { - "type": "reasoning", - "id": "rs_1", - "summary": [], - "encrypted_content": "encrypted", - } - original_input = cast( - ResponseInputParam, - [system_message, reasoning_item, assistant_message, user_message], - ) - _, merged_messages, _ = AnthropicCacheControlHook().get_chat_completion_prompt( - model="azure/gpt-5-codex", - messages=[system_message, assistant_message, user_message], - non_default_params={"cache_control_injection_points": [{"location": "message", "role": "system"}]}, - prompt_id=None, - prompt_variables=None, - dynamic_callback_params={}, - ) + original_input, merged_messages, reasoning_item = _make_cache_control_case() logging_obj = _make_logging_obj( merged_model="azure/gpt-5-codex", merged_messages=merged_messages, @@ -332,6 +341,37 @@ class TestResponsesAPIPromptManagement: assert sent_input[1] == reasoning_item assert sent_input[2]["id"] == "msg_1" + def test_all_non_message_input_items_remain_unchanged(self): + reasoning_item = { + "type": "reasoning", + "id": "rs_1", + "summary": [], + "encrypted_content": "encrypted", + } + original_input = cast(ResponseInputParam, [reasoning_item]) + logging_obj = _make_logging_obj( + merged_model="openai/gpt-4o", + merged_messages=[ + cast( + AllMessageValues, + {"role": "system", "content": "Analyze the request"}, + ) + ], + ) + + patches = _patch_responses_dispatch() + with patches[0], patches[1], patches[2], patches[3] as mock_handler: + import litellm + + litellm.responses( + input=original_input, + model="gpt-4o", + prompt_id="all-non-message", + litellm_logging_obj=logging_obj, + ) + + assert mock_handler.call_args.kwargs["input"] == original_input + def test_model_override_re_resolves_provider(self): """[G] When the prompt template overrides the model to a different provider, custom_llm_provider is re-resolved so downstream routing uses the correct provider. @@ -469,3 +509,33 @@ class TestAsyncResponsesAPIPromptManagement: passed_messages = call_kwargs["messages"] assert all(isinstance(m, dict) and "role" in m for m in passed_messages) assert len(passed_messages) == 1 + + @pytest.mark.asyncio + async def test_async_cache_control_hook_preserves_reasoning_items(self): + original_input, merged_messages, reasoning_item = _make_cache_control_case() + logging_obj = _make_logging_obj( + merged_model="azure/gpt-5-codex", + merged_messages=merged_messages, + ) + + patches = _patch_responses_dispatch() + with patches[0], patches[1], patches[2], patches[3] as mock_handler: + import litellm + + await litellm.aresponses( + input=original_input, + model="azure/gpt-5-codex", + litellm_logging_obj=logging_obj, + cache_control_injection_points=[{"location": "message", "role": "system"}], + ) + + sent_input = mock_handler.call_args.kwargs["input"] + assert [item.get("type") for item in sent_input] == [ + None, + "reasoning", + "message", + None, + ] + assert sent_input[0]["cache_control"] == {"type": "ephemeral"} + assert sent_input[1] == reasoning_item + assert sent_input[2]["id"] == "msg_1" From 482f05190c7ad8b3e478e4d4caff38579367b308 Mon Sep 17 00:00:00 2001 From: Yassin Kortam Date: Wed, 22 Jul 2026 14:25:48 -0700 Subject: [PATCH 04/41] fix(scim): prune deleted user from teams' members_with_roles (#34180) SCIM delete_user removed the user from the legacy team.members column and deleted their team membership rows, but never pruned members_with_roles, which is the source of truth ScimTransformations reads for GET /Groups/{id}. A deleted user therefore lingered as a dangling member reference on every team they belonged to Prune each of the user's teams directly via team_member_delete before deleting the user row, and only for teams whose members_with_roles actually contain the user, so a real DB failure surfaces (the endpoint fails loudly and the user is kept; SCIM DELETE is idempotent, so the IdP retries) while a user who was never in a team's members_with_roles stays a no-op. patch_team_membership is left unchanged for its other callers --- .../management_endpoints/scim/scim_v2.py | 7 + .../scim/test_scim_v2_endpoints.py | 124 ++++++++++++++++++ 2 files changed, 131 insertions(+) diff --git a/litellm/proxy/management_endpoints/scim/scim_v2.py b/litellm/proxy/management_endpoints/scim/scim_v2.py index db2cf2b70dd..d1d1c5959d8 100644 --- a/litellm/proxy/management_endpoints/scim/scim_v2.py +++ b/litellm/proxy/management_endpoints/scim/scim_v2.py @@ -1317,6 +1317,13 @@ async def delete_user( where={"team_id": team.team_id}, data={"members": new_members} ) + team_row = LiteLLM_TeamTable(**team.model_dump()) + if any(member.user_id == user_id for member in team_row.members_with_roles or []): + await team_member_delete( + data=TeamMemberDeleteRequest(team_id=team_row.team_id, user_id=user_id), + user_api_key_dict=UserAPIKeyAuth(user_role=LitellmUserRoles.PROXY_ADMIN), + ) + await _set_user_keys_blocked(user_id=user_id, blocked=True) await _delete_rows_referencing_user(prisma_client, user_id=user_id) 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 f458645e51f..c31029eb54d 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 @@ -23,6 +23,7 @@ from litellm.proxy.management_endpoints.scim.scim_v2 import ( create_group, create_user, delete_group, + delete_user, get_groups, get_users, get_service_provider_config, @@ -3062,3 +3063,126 @@ async def test_apply_group_patch_updates_does_not_write_legacy_members(mocker): written = mock_prisma_client.db.litellm_teamtable.update.call_args.kwargs["data"] assert "members" not in written assert written["team_alias"] == "Renamed" + + +def _mock_prisma_for_delete_user(mocker, team): + mock_prisma_client = mocker.MagicMock() + mock_prisma_client.db = mocker.MagicMock() + mock_prisma_client.db.litellm_teamtable = mocker.MagicMock() + mock_prisma_client.db.litellm_teamtable.find_unique = AsyncMock(return_value=team) + mock_prisma_client.db.litellm_teamtable.update = AsyncMock() + mock_prisma_client.db.litellm_usertable = mocker.MagicMock() + mock_prisma_client.db.litellm_usertable.delete = AsyncMock() + return mock_prisma_client + + +def _patch_delete_user_dependencies(mocker, mock_prisma_client, existing_user): + 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_user_exists", + AsyncMock(return_value=existing_user), + ) + mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2._set_user_keys_blocked", + AsyncMock(), + ) + mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2._delete_rows_referencing_user", + AsyncMock(), + ) + + +@pytest.mark.asyncio +async def test_delete_user_prunes_members_with_roles(mocker): + """Deleting a SCIM user must remove them from every team they belong to via + team_member_delete, which prunes members_with_roles (the source of truth for + SCIM group membership) so GET /Groups no longer returns a dangling reference + to the now-deleted user.""" + user_id = "scim-del-user" + + existing_user = mocker.MagicMock() + existing_user.teams = ["team-1"] + + team = LiteLLM_TeamTable( + team_id="team-1", + members=[user_id, "other-user"], + members_with_roles=[Member(user_id=user_id, role="user"), Member(user_id="other-user", role="admin")], + ) + + mock_prisma_client = _mock_prisma_for_delete_user(mocker, team) + _patch_delete_user_dependencies(mocker, mock_prisma_client, existing_user) + team_member_delete_mock = mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2.team_member_delete", + AsyncMock(), + ) + + await delete_user(user_id=user_id) + + team_member_delete_mock.assert_awaited_once() + call = team_member_delete_mock.call_args + assert call.kwargs["data"].team_id == "team-1" + assert call.kwargs["data"].user_id == user_id + assert call.kwargs["user_api_key_dict"].user_role == LitellmUserRoles.PROXY_ADMIN + mock_prisma_client.db.litellm_usertable.delete.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_delete_user_surfaces_prune_failure_and_keeps_user(mocker): + """A genuine failure while pruning members_with_roles must surface: the + endpoint fails loudly and the user row is NOT deleted, so we never report a + successful delete while leaving a dangling member (SCIM DELETE is idempotent, + so the IdP retries).""" + user_id = "scim-del-user" + + existing_user = mocker.MagicMock() + existing_user.teams = ["team-1"] + + team = LiteLLM_TeamTable( + team_id="team-1", + members=[user_id], + members_with_roles=[Member(user_id=user_id, role="user")], + ) + + mock_prisma_client = _mock_prisma_for_delete_user(mocker, team) + _patch_delete_user_dependencies(mocker, mock_prisma_client, existing_user) + mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2.team_member_delete", + AsyncMock(side_effect=Exception("database connection lost")), + ) + + with pytest.raises(Exception): + await delete_user(user_id=user_id) + + mock_prisma_client.db.litellm_usertable.delete.assert_not_awaited() + + +@pytest.mark.asyncio +async def test_delete_user_skips_teams_where_not_a_member(mocker): + """If the user is not in a team's members_with_roles, deletion must treat that + team as a no-op (no team_member_delete call, no error) and still delete the + user, so a stale legacy membership can't block the delete.""" + user_id = "scim-del-user" + + existing_user = mocker.MagicMock() + existing_user.teams = ["team-1"] + + team = LiteLLM_TeamTable( + team_id="team-1", + members=[user_id], + members_with_roles=[Member(user_id="someone-else", role="admin")], + ) + + mock_prisma_client = _mock_prisma_for_delete_user(mocker, team) + _patch_delete_user_dependencies(mocker, mock_prisma_client, existing_user) + team_member_delete_mock = mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2.team_member_delete", + AsyncMock(), + ) + + await delete_user(user_id=user_id) + + team_member_delete_mock.assert_not_awaited() + mock_prisma_client.db.litellm_usertable.delete.assert_awaited_once() From 3f98f6274886ffa511b13e62765f2e2baac96682 Mon Sep 17 00:00:00 2001 From: Yassin Kortam Date: Wed, 22 Jul 2026 14:43:13 -0700 Subject: [PATCH 05/41] test(e2e): fail the run when a Rust gateway silently serves /messages through Python (#34208) A gateway whose native extension is unavailable falls back to the Python implementation without raising, so it answers /v1/messages normally and the only difference on the wire is the absent x-litellm-rust header. Nothing in the suite read that header, so a Rust deployment that had stopped running Rust produced a fully green e2e run. Assert the marker on the streamed Messages assertions when E2E_EXPECT_RUST is set. It stays opt-in because the same suite image also runs against the standard gateway, which has no Rust path and must keep passing; the two deployments are already separate Applications, so this is one value on the Rust instance rather than branching inside the tests. --- tests/e2e/e2e_config.py | 2 ++ .../llm_translation/test_messages_azure_foundry_e2e.py | 9 ++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/e2e/e2e_config.py b/tests/e2e/e2e_config.py index 30353b93dd6..e7c48690c0a 100644 --- a/tests/e2e/e2e_config.py +++ b/tests/e2e/e2e_config.py @@ -72,6 +72,8 @@ POLL_TIMEOUT = float(os.environ.get("E2E_POLL_TIMEOUT", "120")) POLL_INTERVAL = float(os.environ.get("E2E_POLL_INTERVAL", "5")) REQUEST_TIMEOUT = float(os.environ.get("E2E_REQUEST_TIMEOUT", "60")) +EXPECT_RUST = os.environ.get("E2E_EXPECT_RUST", "").strip().lower() in ("1", "true", "yes") + LOAD_USERS = int(os.environ.get("E2E_LOAD_USERS", "750")) LOAD_SPAWN_RATE = float(os.environ.get("E2E_LOAD_SPAWN_RATE", "50")) LOAD_DURATION_SECONDS = float(os.environ.get("E2E_LOAD_DURATION_SECONDS", "60")) diff --git a/tests/e2e/llm_translation/test_messages_azure_foundry_e2e.py b/tests/e2e/llm_translation/test_messages_azure_foundry_e2e.py index 3f2907f202e..d8d44820e80 100644 --- a/tests/e2e/llm_translation/test_messages_azure_foundry_e2e.py +++ b/tests/e2e/llm_translation/test_messages_azure_foundry_e2e.py @@ -12,7 +12,7 @@ from __future__ import annotations import pytest -from e2e_config import unique_marker +from e2e_config import EXPECT_RUST, unique_marker from e2e_http import StreamingResponse, require_successful_call, unwrap from endpoints_client import EndpointsClient from lifecycle import ResourceManager @@ -50,6 +50,13 @@ def _assert_streamed_ok(result: StreamingResponse) -> None: assert any("message_stop" in event for event in result.stream_events), ( "stream never reached message_stop" ) + if EXPECT_RUST: + assert result.headers.get("x-litellm-rust") == "true", ( + "E2E_EXPECT_RUST is set, so this gateway must serve /v1/messages through the " + "Rust path, but the response carried no x-litellm-rust marker. The request " + "still succeeded, which is exactly the failure mode: a gateway whose native " + f"extension is unavailable falls back to Python silently. headers={result.headers}" + ) class TestAzureFoundryMessages: From c6b2f111a684b8fc8a7bc2ba27234fcf6b2d1efa Mon Sep 17 00:00:00 2001 From: Yassin Kortam Date: Wed, 22 Jul 2026 14:47:22 -0700 Subject: [PATCH 06/41] fix(team): make team member add atomic to prevent concurrent-add member loss (#34185) _add_team_members_to_team reconciled membership by reading the complete_team_data snapshot captured at the start of team_member_add, appending in memory, and writing the whole members_with_roles array back. Two concurrent /team/member_add calls for the same team read the same snapshot, so the last write wins and one member is silently lost. This affects every concurrent team member add, including the SCIM group PATCH op:add path that routes through team_member_add Reconcile members_with_roles inside a transaction that locks the team row with SELECT ... FOR UPDATE before re-reading the current membership, so concurrent writers serialize on the row lock and each appends onto the other's committed result. The interactive transaction is exposed through a thin PrismaClient.tx() passthrough and the locked read is encapsulated in TeamRepository.get_members_with_roles_locked, and the SCIM group PATCH applies membership as deltas so concurrent adds are not clobbered --- .../management_endpoints/scim/scim_v2.py | 51 +++-- .../management_endpoints/team_endpoints.py | 37 ++-- litellm/proxy/utils.py | 9 + litellm/repositories/team_repository.py | 29 ++- tests/proxy_unit_tests/test_proxy_server.py | 83 +++++--- .../scim/test_scim_v2_endpoints.py | 197 +++++++++++++++++- .../test_team_endpoints.py | 72 +++++++ .../repositories/test_repositories.py | 38 +++- 8 files changed, 448 insertions(+), 68 deletions(-) diff --git a/litellm/proxy/management_endpoints/scim/scim_v2.py b/litellm/proxy/management_endpoints/scim/scim_v2.py index d1d1c5959d8..525ce9f0e89 100644 --- a/litellm/proxy/management_endpoints/scim/scim_v2.py +++ b/litellm/proxy/management_endpoints/scim/scim_v2.py @@ -1932,8 +1932,16 @@ async def delete_group( async def _process_group_patch_operations( patch_ops: SCIMPatchOp, existing_team, prisma_client -) -> Tuple[Dict[str, Any], Set[str]]: - """Process patch operations for a group and return update data and final members.""" +) -> Tuple[Dict[str, Any], Set[str], Set[str] | None]: + """Process patch operations for a group and return update data, final members + and, when the request contained a member ``replace`` op, the absolute target + roster it declared (``None`` otherwise). + + ``add``/``remove`` are deltas relative to the current roster, but ``replace`` + is absolute: it declares the roster is exactly this set, so the caller must + reconcile against it as a set-to-target rather than rebasing it onto a + concurrently-mutated roster. + """ update_data: Dict[str, Any] = {} # Create a fresh copy of existing metadata to avoid Prisma issues @@ -2019,7 +2027,12 @@ async def _process_group_patch_operations( if metadata: update_data["metadata"] = metadata - return update_data, final_members + member_replace_present = any( + op.op == "replace" and (op.path or "").lower().startswith("members") for op in patch_ops.Operations + ) + replace_target = set(final_members) if member_replace_present else None + + return update_data, final_members, replace_target async def _apply_group_patch_updates(group_id: str, update_data: Dict[str, Any], prisma_client): @@ -2090,27 +2103,29 @@ async def patch_group( existing_team = await _check_team_exists(group_id) # Process patch operations - update_data, final_members = await _process_group_patch_operations(patch_ops, existing_team, prisma_client) + update_data, final_members, replace_target = await _process_group_patch_operations( + patch_ops, existing_team, prisma_client + ) - # Track current members BEFORE update for comparison - current_members = set(await _get_team_member_user_ids_from_team(existing_team)) + snapshot_members = set(await _get_team_member_user_ids_from_team(existing_team)) + intended_add = final_members - snapshot_members + intended_remove = snapshot_members - final_members # Apply the metadata/displayName updates to the database updated_team = await _apply_group_patch_updates(group_id, update_data, prisma_client) - # Refresh team data from database to get the latest state after concurrent updates - # This prevents race conditions when multiple PATCH requests come in simultaneously refreshed_team = await TeamRepository(prisma_client).table.find_unique(where={"team_id": group_id}) - if refreshed_team: - # Re-read current members from refreshed team to account for concurrent updates - refreshed_current_members = set( - await _get_team_member_user_ids_from_team(LiteLLM_TeamTable(**refreshed_team.model_dump())) - ) - # Use the refreshed members for comparison - current_members = refreshed_current_members + refreshed_current = ( + set(await _get_team_member_user_ids_from_team(LiteLLM_TeamTable(**refreshed_team.model_dump()))) + if refreshed_team + else snapshot_members + ) - # Handle user-team relationship changes - await _handle_group_membership_changes(group_id, current_members, final_members) + effective_final = ( + replace_target if replace_target is not None else (refreshed_current | intended_add) - intended_remove + ) + + await _handle_group_membership_changes(group_id, refreshed_current, effective_final) # A rename can flip whether this group matches scim_admin_group by display # name, so retained members must be re-resolved too, not just the ones whose @@ -2119,7 +2134,7 @@ async def patch_group( alias_changed = new_alias != existing_team.team_alias await _recompute_scim_member_roles( prisma_client, - (current_members | final_members if alias_changed else current_members ^ final_members), + (refreshed_current | effective_final if alias_changed else refreshed_current ^ effective_final), ) # Refresh team one more time to get final state after membership changes diff --git a/litellm/proxy/management_endpoints/team_endpoints.py b/litellm/proxy/management_endpoints/team_endpoints.py index 70c002d2d2d..3a7e89aa20d 100644 --- a/litellm/proxy/management_endpoints/team_endpoints.py +++ b/litellm/proxy/management_endpoints/team_endpoints.py @@ -2375,7 +2375,15 @@ async def _add_team_members_to_team( user_api_key_dict: UserAPIKeyAuth, litellm_proxy_admin_name: str, ) -> Tuple[LiteLLM_TeamTable, List[LiteLLM_UserTable], List[LiteLLM_TeamMembership]]: - """Add team members to the team.""" + """Add team members to the team. + + The members_with_roles reconciliation runs inside a transaction that locks + the team row with ``SELECT ... FOR UPDATE`` before reading the current + membership. Concurrent /team/member_add calls for the same team therefore + serialize on the row lock and each appends onto the other's committed + result, instead of both rewriting the whole JSON array from a stale + snapshot (which silently drops one member on the losing write). + """ # Process and add new members updated_users, updated_team_memberships = await _process_team_members( data=data, @@ -2385,19 +2393,22 @@ async def _add_team_members_to_team( litellm_proxy_admin_name=litellm_proxy_admin_name, ) - # Update team members list - await _update_team_members_list( - data=data, - complete_team_data=complete_team_data, - updated_users=updated_users, - ) + async with prisma_client.tx() as tx: + complete_team_data.members_with_roles = await TeamRepository(prisma_client).get_members_with_roles_locked( + tx, data.team_id + ) - # ADD MEMBER TO TEAM - _db_team_members = [m.model_dump() for m in complete_team_data.members_with_roles] - updated_team = await TeamRepository(prisma_client).table.update( - where={"team_id": data.team_id}, - data={"members_with_roles": json.dumps(_db_team_members)}, # type: ignore - ) + await _update_team_members_list( + data=data, + complete_team_data=complete_team_data, + updated_users=updated_users, + ) + + _db_team_members = [m.model_dump() for m in complete_team_data.members_with_roles] + updated_team = await tx.litellm_teamtable.update( + where={"team_id": data.team_id}, + data={"members_with_roles": json.dumps(_db_team_members)}, + ) return updated_team, updated_users, updated_team_memberships diff --git a/litellm/proxy/utils.py b/litellm/proxy/utils.py index 43921a847a9..1dbc0ad1837 100644 --- a/litellm/proxy/utils.py +++ b/litellm/proxy/utils.py @@ -172,6 +172,7 @@ from litellm.types.utils import LLMResponseTypes, LoggedLiteLLMParams if TYPE_CHECKING: from opentelemetry.trace import Span as _Span + from prisma.client import TransactionManager from litellm.litellm_core_utils.litellm_logging import Logging as LiteLLMLoggingObj @@ -2922,6 +2923,14 @@ class PrismaClient: return self.db.writer return self.db + def tx(self) -> "TransactionManager": + """Open an interactive transaction on the writer. + + Callers go through this instead of reaching into ``self.db`` so writer + selection and read-replica routing stay encapsulated in the wrapper. + """ + return cast("TransactionManager", self.db.tx()) # cast-ok: wrappers delegate tx via __getattr__ (untyped) + def get_request_status(self, payload: Union[dict, SpendLogsPayload]) -> Literal["success", "failure"]: """ Determine if a request was successful or failed based on payload metadata. diff --git a/litellm/repositories/team_repository.py b/litellm/repositories/team_repository.py index 3227aa812ca..68875bd7972 100644 --- a/litellm/repositories/team_repository.py +++ b/litellm/repositories/team_repository.py @@ -4,11 +4,18 @@ Team repository for database operations on LiteLLM_TeamTable. import json from datetime import datetime -from typing import Any, Dict, List, Optional, Type +from typing import TYPE_CHECKING, Any, Dict, List, Optional, Type -from litellm.models.team import LiteLLM_TeamTable +from pydantic import TypeAdapter + +from litellm.models.team import LiteLLM_TeamTable, Member from litellm.repositories.base_repository import BaseRepository +if TYPE_CHECKING: + from prisma import Prisma + +_MEMBERS_WITH_ROLES_ADAPTER = TypeAdapter(list[Member]) + class TeamRepository(BaseRepository[LiteLLM_TeamTable]): """Repository for team database operations.""" @@ -46,6 +53,24 @@ class TeamRepository(BaseRepository[LiteLLM_TeamTable]): return LiteLLM_TeamTable(**data) + async def get_members_with_roles_locked(self, tx: "Prisma", team_id: str) -> List[Member]: + """Return the team's members_with_roles, locking the row FOR UPDATE. + + Must be called inside a transaction so the row lock is held until + commit. This serializes concurrent membership writers on the team row + so the losing writer appends onto the winner's committed result instead + of overwriting it from a stale snapshot. + """ + rows = await tx.query_raw( + 'SELECT members_with_roles FROM "LiteLLM_TeamTable" WHERE team_id = $1 FOR UPDATE', + team_id, + ) + raw_value = rows[0]["members_with_roles"] if rows else None + parsed = json.loads(raw_value) if isinstance(raw_value, str) else raw_value + if not parsed: + return [] + return _MEMBERS_WITH_ROLES_ADAPTER.validate_python(parsed) + async def find_by_id(self, team_id: str, id_field: str = "team_id") -> Optional[LiteLLM_TeamTable]: return await super().find_by_id(team_id, id_field) diff --git a/tests/proxy_unit_tests/test_proxy_server.py b/tests/proxy_unit_tests/test_proxy_server.py index 212f7772cad..bedd4dd1838 100644 --- a/tests/proxy_unit_tests/test_proxy_server.py +++ b/tests/proxy_unit_tests/test_proxy_server.py @@ -1252,6 +1252,17 @@ async def test_create_team_member_add(prisma_client, new_member_method): return_value=LiteLLM_TeamTableCachedObj(team_id="1234") ) + tx_mock = AsyncMock() + tx_mock.query_raw = AsyncMock(return_value=[{"members_with_roles": []}]) + tx_mock.litellm_teamtable = team_mock_client + tx_cm = MagicMock() + tx_cm.__aenter__ = AsyncMock(return_value=tx_mock) + tx_cm.__aexit__ = AsyncMock(return_value=None) + original_tx = litellm.proxy.proxy_server.prisma_client.tx + litellm.proxy.proxy_server.prisma_client.tx = MagicMock( + return_value=tx_cm + ) + print(f"team_member_add_request={team_member_add_request}") await team_member_add( data=team_member_add_request, @@ -1273,6 +1284,7 @@ async def test_create_team_member_add(prisma_client, new_member_method): ) litellm.proxy.proxy_server.prisma_client.db.litellm_teamtable = original_val + litellm.proxy.proxy_server.prisma_client.tx = original_tx @pytest.mark.parametrize("team_member_role", ["admin", "user"]) @@ -1434,42 +1446,51 @@ async def test_create_team_member_add_team_admin( mock_litellm_usertable.find_unique = AsyncMock(return_value=None) team_mock_client = AsyncMock() - original_val = getattr( - litellm.proxy.proxy_server.prisma_client.db, "litellm_teamtable" - ) - litellm.proxy.proxy_server.prisma_client.db.litellm_teamtable = team_mock_client - team_mock_client.update = AsyncMock( return_value=LiteLLM_TeamTableCachedObj(team_id="1234") ) - try: - await team_member_add( - data=team_member_add_request, - user_api_key_dict=valid_token, + tx_mock = AsyncMock() + tx_mock.query_raw = AsyncMock(return_value=[{"members_with_roles": []}]) + tx_mock.litellm_teamtable = team_mock_client + tx_cm = MagicMock() + tx_cm.__aenter__ = AsyncMock(return_value=tx_mock) + tx_cm.__aexit__ = AsyncMock(return_value=None) + + with ( + patch.object( + litellm.proxy.proxy_server.prisma_client.db, + "litellm_teamtable", + team_mock_client, + ), + patch.object( + litellm.proxy.proxy_server.prisma_client, + "tx", + MagicMock(return_value=tx_cm), + ), + ): + try: + await team_member_add( + data=team_member_add_request, + user_api_key_dict=valid_token, + ) + except HTTPException as e: + if user_role == "user": + assert e.status_code == 403 + return + else: + raise e + + mock_client.assert_called() + + assert ( + mock_client.call_args.kwargs["data"]["create"]["max_budget"] + == litellm.max_internal_user_budget + ) + assert ( + mock_client.call_args.kwargs["data"]["create"]["budget_duration"] + == litellm.internal_user_budget_duration ) - except HTTPException as e: - if user_role == "user": - assert e.status_code == 403 - return - else: - raise e - - mock_client.assert_called() - - print(f"mock_client.call_args: {mock_client.call_args}") - print("mock_client.call_args.kwargs: {}".format(mock_client.call_args.kwargs)) - - assert ( - mock_client.call_args.kwargs["data"]["create"]["max_budget"] - == litellm.max_internal_user_budget - ) - assert ( - mock_client.call_args.kwargs["data"]["create"]["budget_duration"] - == litellm.internal_user_budget_duration - ) - - litellm.proxy.proxy_server.prisma_client.db.litellm_teamtable = original_val @pytest.mark.asyncio 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 c31029eb54d..850b8fc7cfd 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 @@ -1909,7 +1909,7 @@ async def test_process_group_patch_operations_with_flag_true_creates_users(mocke ) # Execute the function - update_data, final_members = await _process_group_patch_operations( + update_data, final_members, _ = await _process_group_patch_operations( patch_ops=patch_ops, existing_team=mock_existing_team, prisma_client=mock_prisma_client, @@ -2948,7 +2948,7 @@ async def test_process_group_patch_operations_add_retains_existing_members( return_value=mocker.MagicMock(user_id="new-user") ) - _, final_members = await _process_group_patch_operations( + _, final_members, _ = await _process_group_patch_operations( patch_ops=patch_ops, existing_team=existing_team, prisma_client=mock_prisma_client, @@ -2996,7 +2996,7 @@ async def test_process_group_patch_operations_remove_uses_members_with_roles( return_value=mocker.MagicMock(user_id="drop-user") ) - _, final_members = await _process_group_patch_operations( + _, final_members, _ = await _process_group_patch_operations( patch_ops=patch_ops, existing_team=existing_team, prisma_client=mock_prisma_client, @@ -3186,3 +3186,194 @@ async def test_delete_user_skips_teams_where_not_a_member(mocker): team_member_delete_mock.assert_not_awaited() mock_prisma_client.db.litellm_usertable.delete.assert_awaited_once() + + +@pytest.mark.asyncio +async def test_patch_group_add_applies_delta_and_keeps_concurrent_add(mocker): + """A group PATCH op:add must be applied as a delta against the live roster, + not as a snapshot-based absolute target. + + When a concurrent PATCH has already added a member between this request's + initial read and its post-write refresh, that member shows up in the + refreshed roster but not in this request's snapshot-derived target. Diffing + the refreshed roster against the snapshot target would issue a spurious + team_member_delete for the concurrently-added member. Applying only this + request's intended delta on top of the refreshed roster must retain them. + """ + from litellm.proxy.management_endpoints.scim.scim_transformations import ( + ScimTransformations, + ) + + group_id = "team-concurrent" + + snapshot_team = LiteLLM_TeamTable( + team_id=group_id, + team_alias="Group", + members_with_roles=[Member(user_id="zed", role="user")], + metadata={"externalId": "grp-ext"}, + ) + refreshed_team = LiteLLM_TeamTable( + team_id=group_id, + team_alias="Group", + members_with_roles=[ + Member(user_id="zed", role="user"), + Member(user_id="alice", role="user"), + ], + metadata={"externalId": "grp-ext"}, + ) + final_team = LiteLLM_TeamTable( + team_id=group_id, + team_alias="Group", + members_with_roles=[ + Member(user_id="zed", role="user"), + Member(user_id="alice", role="user"), + Member(user_id="bob", role="user"), + ], + metadata={"externalId": "grp-ext"}, + ) + + patch_ops = SCIMPatchOp( + schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + Operations=[SCIMPatchOperation(op="add", path="members", value=[{"value": "bob"}])], + ) + + mock_prisma_client = mocker.MagicMock() + mock_prisma_client.db = mocker.MagicMock() + mock_prisma_client.db.litellm_teamtable = mocker.MagicMock() + mock_prisma_client.db.litellm_teamtable.find_unique = AsyncMock( + side_effect=[snapshot_team, refreshed_team, final_team] + ) + mock_prisma_client.db.litellm_teamtable.update = AsyncMock(return_value=final_team) + mock_prisma_client.db.litellm_usertable = mocker.MagicMock() + mock_prisma_client.db.litellm_usertable.find_unique = AsyncMock(return_value=mocker.MagicMock()) + + mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2._get_prisma_client_or_raise_exception", + AsyncMock(return_value=mock_prisma_client), + ) + patch_membership_mock = mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2.patch_team_membership", + AsyncMock(), + ) + mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2._recompute_scim_member_roles", + AsyncMock(), + ) + mocker.patch.object( + ScimTransformations, + "transform_litellm_team_to_scim_group", + AsyncMock( + return_value=SCIMGroup( + schemas=["urn:ietf:params:scim:schemas:core:2.0:Group"], + id=group_id, + displayName="Group", + ) + ), + ) + + await patch_group(group_id=group_id, patch_ops=patch_ops) + + calls = patch_membership_mock.call_args_list + + removed_user_ids = { + call.kwargs["user_id"] for call in calls if call.kwargs.get("teams_ids_to_remove_user_from") == [group_id] + } + assert removed_user_ids == set() + + added_user_ids = { + call.kwargs["user_id"] for call in calls if call.kwargs.get("teams_ids_to_add_user_to") == [group_id] + } + assert added_user_ids == {"bob"} + + +@pytest.mark.asyncio +async def test_patch_group_replace_stays_absolute_against_concurrent_roster(mocker): + """A group PATCH ``replace`` op declares the roster is exactly the given set, + so it must reconcile as a set-to-target, not as a delta. + + Unlike ``add``/``remove``, ``replace`` is absolute. A member that another + request added concurrently is present in the refreshed roster but not in the + replace target, and ``replace`` must drop it. Rebasing the replace onto the + refreshed roster (the delta behavior correct only for add/remove) would + wrongly retain that concurrently-added member. + """ + from litellm.proxy.management_endpoints.scim.scim_transformations import ( + ScimTransformations, + ) + + group_id = "team-replace-concurrent" + + snapshot_team = LiteLLM_TeamTable( + team_id=group_id, + team_alias="Group", + members_with_roles=[Member(user_id="zed", role="user")], + metadata={"externalId": "grp-ext"}, + ) + refreshed_team = LiteLLM_TeamTable( + team_id=group_id, + team_alias="Group", + members_with_roles=[ + Member(user_id="alice", role="user"), + Member(user_id="bob", role="user"), + ], + metadata={"externalId": "grp-ext"}, + ) + final_team = LiteLLM_TeamTable( + team_id=group_id, + team_alias="Group", + members_with_roles=[Member(user_id="alice", role="user")], + metadata={"externalId": "grp-ext"}, + ) + + patch_ops = SCIMPatchOp( + schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + Operations=[SCIMPatchOperation(op="replace", path="members", value=[{"value": "alice"}])], + ) + + mock_prisma_client = mocker.MagicMock() + mock_prisma_client.db = mocker.MagicMock() + mock_prisma_client.db.litellm_teamtable = mocker.MagicMock() + mock_prisma_client.db.litellm_teamtable.find_unique = AsyncMock( + side_effect=[snapshot_team, refreshed_team, final_team] + ) + mock_prisma_client.db.litellm_teamtable.update = AsyncMock(return_value=final_team) + mock_prisma_client.db.litellm_usertable = mocker.MagicMock() + mock_prisma_client.db.litellm_usertable.find_unique = AsyncMock(return_value=mocker.MagicMock()) + + mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2._get_prisma_client_or_raise_exception", + AsyncMock(return_value=mock_prisma_client), + ) + patch_membership_mock = mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2.patch_team_membership", + AsyncMock(), + ) + mocker.patch( + "litellm.proxy.management_endpoints.scim.scim_v2._recompute_scim_member_roles", + AsyncMock(), + ) + mocker.patch.object( + ScimTransformations, + "transform_litellm_team_to_scim_group", + AsyncMock( + return_value=SCIMGroup( + schemas=["urn:ietf:params:scim:schemas:core:2.0:Group"], + id=group_id, + displayName="Group", + ) + ), + ) + + await patch_group(group_id=group_id, patch_ops=patch_ops) + + calls = patch_membership_mock.call_args_list + + removed_user_ids = { + call.kwargs["user_id"] for call in calls if call.kwargs.get("teams_ids_to_remove_user_from") == [group_id] + } + assert removed_user_ids == {"bob"} + + added_user_ids = { + call.kwargs["user_id"] for call in calls if call.kwargs.get("teams_ids_to_add_user_to") == [group_id] + } + assert added_user_ids == set() diff --git a/tests/test_litellm/proxy/management_endpoints/test_team_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_team_endpoints.py index 50817b6a4c2..d5d61341c93 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_team_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_team_endpoints.py @@ -1693,6 +1693,78 @@ async def test_update_team_members_list_duplicate_prevention(): assert len(mock_team.members_with_roles) == 1 +@pytest.mark.asyncio +async def test_add_team_members_reconciles_against_freshly_locked_row(): + """ + Regression: _add_team_members_to_team must build the new members_with_roles + from the row it re-reads under a lock inside the write transaction, not from + the stale complete_team_data snapshot captured at the start of the request. + + Two concurrent /team/member_add calls for the same team read the same + snapshot; without the locked re-read the losing write rewrites the whole + JSON array from its stale copy and silently drops the member the other call + already committed. Here the snapshot holds only "zed", a concurrent writer + has already committed "alice" (returned by the locked SELECT), and this call + adds "bob". The write must contain all three. + """ + from litellm.proxy.management_endpoints.team_endpoints import ( + _add_team_members_to_team, + ) + + stale_snapshot = LiteLLM_TeamTable( + team_id="test-team-lock", + members_with_roles=[Member(user_id="zed", role="user")], + ) + + freshly_committed = [ + {"user_id": "zed", "user_email": None, "role": "user"}, + {"user_id": "alice", "user_email": None, "role": "user"}, + ] + + captured: dict = {} + + async def _capture_update(where, data): + captured["data"] = data + return LiteLLM_TeamTable( + team_id="test-team-lock", + members_with_roles=json.loads(data["members_with_roles"]), + ) + + tx = MagicMock() + tx.query_raw = AsyncMock(return_value=[{"members_with_roles": freshly_committed}]) + tx.litellm_teamtable.update = AsyncMock(side_effect=_capture_update) + + tx_cm = MagicMock() + tx_cm.__aenter__ = AsyncMock(return_value=tx) + tx_cm.__aexit__ = AsyncMock(return_value=None) + + prisma_client = MagicMock() + prisma_client.tx = MagicMock(return_value=tx_cm) + + with patch( + "litellm.proxy.management_endpoints.team_endpoints._process_team_members", + new=AsyncMock(return_value=([], [])), + ): + updated_team, _, _ = await _add_team_members_to_team( + data=TeamMemberAddRequest( + team_id="test-team-lock", + member=Member(user_id="bob", role="user"), + ), + complete_team_data=stale_snapshot, + prisma_client=cast(object, prisma_client), + user_api_key_dict=UserAPIKeyAuth(user_role=LitellmUserRoles.PROXY_ADMIN), + litellm_proxy_admin_name="admin", + ) + + written_ids = sorted(m["user_id"] for m in json.loads(captured["data"]["members_with_roles"])) + assert written_ids == ["alice", "bob", "zed"] + + lock_reads = [call for call in tx.query_raw.call_args_list if "FOR UPDATE" in str(call.args[0])] + assert lock_reads, "expected a SELECT ... FOR UPDATE row-lock read before the write" + + assert [m.user_id for m in updated_team.members_with_roles] == ["zed", "alice", "bob"] + + def test_add_new_models_to_team_with_existing_models(): """ Test add_new_models_to_team function with existing models diff --git a/tests/test_litellm/repositories/test_repositories.py b/tests/test_litellm/repositories/test_repositories.py index af2eea823f4..6308faf8fc7 100644 --- a/tests/test_litellm/repositories/test_repositories.py +++ b/tests/test_litellm/repositories/test_repositories.py @@ -5,7 +5,7 @@ Tests for gateway repository layer. import json from datetime import datetime from typing import Any, Dict, List, Optional -from unittest.mock import MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, patch import pytest @@ -499,6 +499,42 @@ class TestTeamRepository: assert team.team_id == "team-123" assert team.team_alias == "Engineering" + @pytest.mark.asyncio + @pytest.mark.parametrize( + "raw_value, expected_ids", + [ + ( + [ + {"user_id": "a", "role": "user"}, + {"user_id": "b", "role": "admin"}, + ], + ["a", "b"], + ), + (json.dumps([{"user_id": "a", "role": "user"}]), ["a"]), + ({}, []), + (None, []), + ], + ) + async def test_get_members_with_roles_locked(self, repo, raw_value, expected_ids): + tx = MagicMock() + tx.query_raw = AsyncMock(return_value=[{"members_with_roles": raw_value}]) + + members = await repo.get_members_with_roles_locked(tx, "team-1") + + assert [m.user_id for m in members] == expected_ids + sql = tx.query_raw.call_args.args[0] + assert "FOR UPDATE" in sql + assert tx.query_raw.call_args.args[1] == "team-1" + + @pytest.mark.asyncio + async def test_get_members_with_roles_locked_missing_row(self, repo): + tx = MagicMock() + tx.query_raw = AsyncMock(return_value=[]) + + members = await repo.get_members_with_roles_locked(tx, "missing") + + assert members == [] + @pytest.mark.asyncio async def test_create_team_all_fields(self, repo): team = await repo.create_team( From 9baea68f37d60e2b7c3be33841490600a5c13d4d Mon Sep 17 00:00:00 2001 From: tin-berri Date: Wed, 22 Jul 2026 14:47:35 -0700 Subject: [PATCH 07/41] fix(ui): resolve SSO and SMTP settings from a typed config object (#33576) The SSO and Email Server settings pages read only stored config, so a gateway configured entirely through environment variables rendered every field blank even though both features were live. Rather than add per-endpoint env fallback, resolve each setting through one typed config object. A FieldDescriptor names, for one setting, where it lives in the stored row (db_key), which process env var carries it (env_var), whether it is a secret, and its effective default. A pure resolve_fields reconciles a descriptor table against the stored row and the process environment with a fixed precedence and reports per-field provenance (db, env, default, or unset). The SSO descriptor table single-sources the field-to-env mapping that the read and write paths previously duplicated, so they can no longer drift. get_sso_settings and the /get/config/callbacks alerting block read through the resolver instead of their own inline fallbacks. get_sso_settings no longer decrypts stored values into os.environ; decryption happens once inside the resolver via the pure helper, so a GET stops mutating the process environment. The SSO response carries provenance so the UI can distinguish an env-sourced value from a stored one, and secrets are masked at the endpoint (the resolver returns them unmasked so the login path could consume them). os.environ remains the runtime carrier; the SSO login and mail-send paths are unchanged. The settings pages also submit only fields an admin actually edited, so a rendered mask or env-sourced value is never written back over a working secret, and generic_scope is a real SSO form field. Omitting a field from /update/sso_settings clears it, which provider switching relies on; the deeper write-path concern that behaviour points at is tracked in LIT-4498. --- .../workflows/test-unit-proxy-endpoints.yml | 1 + litellm/proxy/config_resolvers/__init__.py | 9 + .../proxy/config_resolvers/_descriptors.py | 73 ++++++++ litellm/proxy/config_resolvers/alerting.py | 25 +++ litellm/proxy/config_resolvers/sso.py | 94 ++++++++++ litellm/proxy/proxy_server.py | 38 ++--- .../proxy_setting_endpoints.py | 101 ++--------- .../proxy/management_endpoints/ui_sso.py | 4 + .../config_resolvers/test_config_resolvers.py | 105 ++++++++++++ tests/test_litellm/proxy/test_proxy_server.py | 107 ++++++++++++ .../test_proxy_setting_endpoints.py | 161 ++++++++++++++++-- .../(dashboard)/hooks/sso/useSSOSettings.ts | 1 + .../src/components/SSOModals.test.tsx | 1 + .../src/components/SSOModals.tsx | 97 +---------- .../Modals/BaseSSOSettingsForm.test.tsx | 63 ++++++- .../Modals/BaseSSOSettingsForm.tsx | 7 +- .../AdminSettings/SSOSettings/SSOSettings.tsx | 2 + .../src/components/email_settings.tsx | 12 +- ui/litellm-dashboard/src/lib/http/schema.d.ts | 9 + 19 files changed, 693 insertions(+), 217 deletions(-) create mode 100644 litellm/proxy/config_resolvers/__init__.py create mode 100644 litellm/proxy/config_resolvers/_descriptors.py create mode 100644 litellm/proxy/config_resolvers/alerting.py create mode 100644 litellm/proxy/config_resolvers/sso.py create mode 100644 tests/test_litellm/proxy/config_resolvers/test_config_resolvers.py diff --git a/.github/workflows/test-unit-proxy-endpoints.yml b/.github/workflows/test-unit-proxy-endpoints.yml index cbb36eebdb9..b3eb8f79a43 100644 --- a/.github/workflows/test-unit-proxy-endpoints.yml +++ b/.github/workflows/test-unit-proxy-endpoints.yml @@ -46,6 +46,7 @@ jobs: tests/test_litellm/proxy/rag_endpoints tests/test_litellm/proxy/realtime_endpoints tests/test_litellm/proxy/ui_crud_endpoints + tests/test_litellm/proxy/config_resolvers tests/test_litellm/proxy/utils workers: 2 reruns: 2 diff --git a/litellm/proxy/config_resolvers/__init__.py b/litellm/proxy/config_resolvers/__init__.py new file mode 100644 index 00000000000..88b4c3961f0 --- /dev/null +++ b/litellm/proxy/config_resolvers/__init__.py @@ -0,0 +1,9 @@ +"""Typed, provenance-aware resolution of proxy settings from DB then env.""" + +from litellm.proxy.config_resolvers._descriptors import ( + FieldDescriptor, + FieldSource, + resolve_fields, +) + +__all__ = ["FieldDescriptor", "FieldSource", "resolve_fields"] diff --git a/litellm/proxy/config_resolvers/_descriptors.py b/litellm/proxy/config_resolvers/_descriptors.py new file mode 100644 index 00000000000..f67a690f92f --- /dev/null +++ b/litellm/proxy/config_resolvers/_descriptors.py @@ -0,0 +1,73 @@ +"""Shared primitive for resolving a settings value from its sources. + +A ``FieldDescriptor`` names, for one setting, where it lives in the stored DB +row (``db_key``), which process env var carries it (``env_var``), whether it is +a secret, and its effective default. ``resolve_fields`` reconciles a set of +descriptors against a decrypted DB row and the process environment with a fixed +precedence, returning the resolved values plus per-field provenance so a caller +can tell whether a value came from the database, the environment, a default, or +is unset. +""" + +from collections.abc import Mapping, Sequence +from dataclasses import dataclass +from typing import Literal + +FieldSource = Literal["db", "env", "default", "unset"] + + +@dataclass(frozen=True, slots=True) +class FieldDescriptor: + field_name: str + db_key: str + env_var: str + is_secret: bool = False + default: str | None = None + + +def _db_is_set(db_value: object, empty_db_is_set: bool) -> bool: + if empty_db_is_set: + # A stored key that is present, even as "", is an explicit admin choice + # (e.g. clearing an alerting webhook) and must win over a stale env var. + return db_value is not None + # A blank stored value is treated as absent, so it falls through to env. This + # fits settings whose clear path also unsets the env var (e.g. SSO). + return isinstance(db_value, str) and bool(db_value.strip()) + + +def _resolve_one( + descriptor: FieldDescriptor, + db_values: Mapping[str, object], + env: Mapping[str, str], + empty_db_is_set: bool, +) -> tuple[str, str | None, FieldSource]: + db_value = db_values.get(descriptor.db_key) + if _db_is_set(db_value, empty_db_is_set): + return descriptor.field_name, db_value if isinstance(db_value, str) else str(db_value), "db" + env_value = env.get(descriptor.env_var) + if isinstance(env_value, str) and env_value.strip(): + return descriptor.field_name, env_value, "env" + if descriptor.default is not None: + return descriptor.field_name, descriptor.default, "default" + return descriptor.field_name, None, "unset" + + +def resolve_fields( + descriptors: Sequence[FieldDescriptor], + db_values: Mapping[str, object], + env: Mapping[str, str], + empty_db_is_set: bool = False, +) -> tuple[dict[str, str | None], dict[str, FieldSource]]: + """Resolve every descriptor to (values, provenance). + + Precedence per field: a set stored value wins, else a non-blank process env + var, else the descriptor default, else unset. ``empty_db_is_set`` selects + how a present-but-empty stored value is read: ``False`` treats it as absent + so it falls back to env (SSO, whose clear path also unsets the env var); + ``True`` treats it as an explicit clear that wins over env (alerting, whose + clear path stores "" without unsetting the env var). + """ + resolved = tuple(_resolve_one(descriptor, db_values, env, empty_db_is_set) for descriptor in descriptors) + values = {field_name: value for field_name, value, _ in resolved} + provenance = {field_name: source for field_name, _, source in resolved} + return values, provenance diff --git a/litellm/proxy/config_resolvers/alerting.py b/litellm/proxy/config_resolvers/alerting.py new file mode 100644 index 00000000000..3704ec09355 --- /dev/null +++ b/litellm/proxy/config_resolvers/alerting.py @@ -0,0 +1,25 @@ +"""Descriptor tables for the alerting settings surfaced by /get/config/callbacks. + +These reconcile the stored ``environment_variables`` blob (keyed by the +uppercase env-var names) with the process environment. SMTP_PORT and SMTP_TLS +carry the same effective defaults the mail-send path applies, so the settings +page shows the config that mail would actually use rather than a blank. +""" + +from litellm.proxy.config_resolvers._descriptors import FieldDescriptor + +EMAIL_DESCRIPTORS: tuple[FieldDescriptor, ...] = ( + FieldDescriptor("SMTP_HOST", "SMTP_HOST", "SMTP_HOST"), + FieldDescriptor("SMTP_PORT", "SMTP_PORT", "SMTP_PORT", default="587"), + FieldDescriptor("SMTP_TLS", "SMTP_TLS", "SMTP_TLS", default="True"), + FieldDescriptor("SMTP_USERNAME", "SMTP_USERNAME", "SMTP_USERNAME", is_secret=True), + FieldDescriptor("SMTP_PASSWORD", "SMTP_PASSWORD", "SMTP_PASSWORD", is_secret=True), + FieldDescriptor("SMTP_SENDER_EMAIL", "SMTP_SENDER_EMAIL", "SMTP_SENDER_EMAIL"), + FieldDescriptor("TEST_EMAIL_ADDRESS", "TEST_EMAIL_ADDRESS", "TEST_EMAIL_ADDRESS"), + FieldDescriptor("EMAIL_LOGO_URL", "EMAIL_LOGO_URL", "EMAIL_LOGO_URL"), + FieldDescriptor("EMAIL_SUPPORT_CONTACT", "EMAIL_SUPPORT_CONTACT", "EMAIL_SUPPORT_CONTACT"), +) + +SLACK_DESCRIPTORS: tuple[FieldDescriptor, ...] = ( + FieldDescriptor("SLACK_WEBHOOK_URL", "SLACK_WEBHOOK_URL", "SLACK_WEBHOOK_URL", is_secret=True), +) diff --git a/litellm/proxy/config_resolvers/sso.py b/litellm/proxy/config_resolvers/sso.py new file mode 100644 index 00000000000..3d83c06dd62 --- /dev/null +++ b/litellm/proxy/config_resolvers/sso.py @@ -0,0 +1,94 @@ +"""Resolved SSO config object. + +Reconciles the dedicated ``sso_config`` DB row (lowercase, per-value encrypted +keys) with the process environment (uppercase env vars) into a typed +``SSOConfig`` plus per-field provenance. This is the single source of truth for +the SSO field -> env-var mapping, used by both the read-back endpoint and the +save endpoint so the two can never drift. +""" + +from collections.abc import Mapping +from dataclasses import dataclass + +from litellm.proxy.common_utils.encrypt_decrypt_utils import decrypt_value_helper +from litellm.proxy.config_resolvers._descriptors import ( + FieldDescriptor, + FieldSource, + resolve_fields, +) +from litellm.types.proxy.management_endpoints.ui_sso import ( + RoleMappings, + SSOConfig, + TeamMappings, +) + +SSO_DESCRIPTORS: tuple[FieldDescriptor, ...] = ( + FieldDescriptor("google_client_id", "google_client_id", "GOOGLE_CLIENT_ID"), + FieldDescriptor("google_client_secret", "google_client_secret", "GOOGLE_CLIENT_SECRET", is_secret=True), + FieldDescriptor("microsoft_client_id", "microsoft_client_id", "MICROSOFT_CLIENT_ID"), + FieldDescriptor("microsoft_client_secret", "microsoft_client_secret", "MICROSOFT_CLIENT_SECRET", is_secret=True), + FieldDescriptor("microsoft_tenant", "microsoft_tenant", "MICROSOFT_TENANT"), + FieldDescriptor("generic_client_id", "generic_client_id", "GENERIC_CLIENT_ID"), + FieldDescriptor("generic_client_secret", "generic_client_secret", "GENERIC_CLIENT_SECRET", is_secret=True), + FieldDescriptor( + "generic_authorization_endpoint", "generic_authorization_endpoint", "GENERIC_AUTHORIZATION_ENDPOINT" + ), + FieldDescriptor("generic_token_endpoint", "generic_token_endpoint", "GENERIC_TOKEN_ENDPOINT"), + FieldDescriptor("generic_userinfo_endpoint", "generic_userinfo_endpoint", "GENERIC_USERINFO_ENDPOINT"), + FieldDescriptor("generic_scope", "generic_scope", "GENERIC_SCOPE", default="openid email profile"), + FieldDescriptor("proxy_base_url", "proxy_base_url", "PROXY_BASE_URL"), +) + +# Derived from the descriptor table so read (masking) and the field->env mapping +# never diverge from the resolver. +SSO_SECRET_FIELDS: frozenset[str] = frozenset(d.field_name for d in SSO_DESCRIPTORS if d.is_secret) +SSO_FIELD_ENV_VARS: dict[str, str] = {d.field_name: d.env_var for d in SSO_DESCRIPTORS} + +# Structured sub-objects stored on the SSO row that are not simple env-backed +# scalars; handled outside the descriptor resolution. +_STRUCTURED_KEYS = ("role_mappings", "team_mappings") + + +@dataclass(frozen=True, slots=True) +class ResolvedSSOConfig: + config: SSOConfig + provenance: dict[str, FieldSource] + + +def _decrypt(raw: Mapping[str, object]) -> dict[str, object]: + return { + key: ( + decrypt_value_helper(value=value, key=key, return_original_value=True) if isinstance(value, str) else value + ) + for key, value in raw.items() + } + + +def _parse_role_mappings(data: object) -> RoleMappings | None: + # The stored row is JSON, so mappings arrive as a dict (or are absent). + return RoleMappings(**data) if isinstance(data, dict) else None + + +def _parse_team_mappings(data: object) -> TeamMappings | None: + return TeamMappings(**data) if isinstance(data, dict) else None + + +def resolve_sso_config(sso_db_settings: Mapping[str, object] | None, env: Mapping[str, str]) -> ResolvedSSOConfig: + """Resolve the effective SSO config: stored row first, then process env. + + Decryption happens here, once, via the pure ``decrypt_value_helper``; this + function never writes ``os.environ`` (unlike the legacy read path). Values + are returned unmasked so the login path could consume them; the read-back + endpoint is responsible for masking secrets before responding to the UI. + """ + raw = dict(sso_db_settings) if sso_db_settings else {} + decrypted = _decrypt({key: value for key, value in raw.items() if key not in _STRUCTURED_KEYS}) + values, provenance = resolve_fields(SSO_DESCRIPTORS, decrypted, env) + structured = { + "user_email": decrypted.get("user_email"), + "ui_access_mode": decrypted.get("ui_access_mode"), + "role_mappings": _parse_role_mappings(raw.get("role_mappings")), + "team_mappings": _parse_team_mappings(raw.get("team_mappings")), + } + config = SSOConfig(**{**values, **structured}) + return ResolvedSSOConfig(config=config, provenance=provenance) diff --git a/litellm/proxy/proxy_server.py b/litellm/proxy/proxy_server.py index 7677ae51f9a..32845763f22 100644 --- a/litellm/proxy/proxy_server.py +++ b/litellm/proxy/proxy_server.py @@ -304,6 +304,11 @@ from litellm.proxy.common_utils.encrypt_decrypt_utils import ( encrypt_value_helper, ) from litellm.proxy.common_utils.html_forms.ui_login import build_ui_login_form +from litellm.proxy.config_resolvers import resolve_fields +from litellm.proxy.config_resolvers.alerting import ( + EMAIL_DESCRIPTORS, + SLACK_DESCRIPTORS, +) from litellm.proxy.common_utils.http_parsing_utils import ( _read_request_body, _safe_get_request_headers, @@ -1159,9 +1164,9 @@ _OPENAPI_HTTP_METHODS = { # Credentials surfaced by `/get/config/callbacks` in the alerting block: the # full Slack incoming-webhook URL is itself a credential, and the SMTP # password is a service password. Masked on read so plaintext never reaches -# the UI. Kept here at module scope to match the analogous -# `_SSO_SENSITIVE_FIELDS` / `_CACHE_SENSITIVE_FIELDS` constants in the SSO -# and cache endpoint files. +# the UI. Kept here at module scope to match the analogous descriptor +# `is_secret` flags in litellm.proxy.config_resolvers and the +# `_CACHE_SENSITIVE_FIELDS` constant in the cache endpoint file. _ALERTING_SENSITIVE_VARS: Set[str] = {"SLACK_WEBHOOK_URL", "SMTP_PASSWORD"} @@ -15491,14 +15496,10 @@ async def get_config( _alerting = _general_settings.get("alerting", []) alerting_data = [] if "slack" in _alerting: - _slack_vars = [ - "SLACK_WEBHOOK_URL", - ] - _slack_env_vars = { - _var: (value if (value := environment_variables.get(_var)) is not None else os.getenv(_var)) - for _var in _slack_vars - } - _slack_env_vars = _apply_alerting_env_role_gate(_slack_env_vars, is_full_admin) + _slack_values, _ = resolve_fields( + SLACK_DESCRIPTORS, environment_variables, os.environ, empty_db_is_set=True + ) + _slack_env_vars = _apply_alerting_env_role_gate(_slack_values, is_full_admin) _alerting_types = proxy_logging_obj.slack_alerting_instance.alert_types _all_alert_types = proxy_logging_obj.slack_alerting_instance._all_possible_alert_types() @@ -15514,19 +15515,8 @@ async def get_config( } ) # pass email alerting vars - _email_vars = [ - "SMTP_HOST", - "SMTP_PORT", - "SMTP_USERNAME", - "SMTP_PASSWORD", - "SMTP_SENDER_EMAIL", - "TEST_EMAIL_ADDRESS", - "EMAIL_LOGO_URL", - "EMAIL_SUPPORT_CONTACT", - ] - _email_env_vars = _apply_alerting_env_role_gate( - {_var: environment_variables.get(_var) for _var in _email_vars}, is_full_admin - ) + _email_values, _ = resolve_fields(EMAIL_DESCRIPTORS, environment_variables, os.environ, empty_db_is_set=True) + _email_env_vars = _apply_alerting_env_role_gate(_email_values, is_full_admin) alerting_data.append( { diff --git a/litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py b/litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py index 42111cf17f2..10c71c00110 100644 --- a/litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py +++ b/litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py @@ -15,6 +15,11 @@ from litellm._logging import verbose_proxy_logger from litellm.litellm_core_utils.sensitive_data_masker import mask_sensitive_keys from litellm.proxy._types import * from litellm.proxy.auth.user_api_key_auth import user_api_key_auth +from litellm.proxy.config_resolvers.sso import ( + SSO_FIELD_ENV_VARS, + SSO_SECRET_FIELDS, + resolve_sso_config, +) from litellm.repositories.config_repository import ConfigRepository from litellm.repositories.table_repositories import ( SSOConfigRepository, @@ -27,16 +32,6 @@ from litellm.types.proxy.management_endpoints.ui_sso import ( router = APIRouter() -# SSO secret fields returned by /get/sso_settings. These are masked on read so -# the UI can show "(set)" without ever transporting the plaintext OAuth secret -# off the server, matching the write-once + masked-on-read contract used for -# the HashiCorp Vault config override. -_SSO_SENSITIVE_FIELDS: Set[str] = { - "google_client_secret", - "microsoft_client_secret", - "generic_client_secret", -} - # Maps each UIThemeConfig field to the env var the UI branding path reads it # from. /update/ui_theme_settings writes both the stored ui_theme_config and # these env vars, so /get/ui_theme_settings resolves the same env vars to @@ -109,7 +104,8 @@ class SettingsResponse(BaseModel): class SSOSettingsResponse(SettingsResponse): """Response model for SSO settings""" - pass + provenance: Dict[str, str] = Field(default_factory=dict) + """Per-field source of each value: 'db', 'env', 'default', or 'unset'.""" class InternalUserSettingsResponse(SettingsResponse): @@ -757,7 +753,7 @@ async def get_sso_settings(): Returns a structured object with values and descriptions for UI display. """ - from litellm.proxy.proxy_server import prisma_client, proxy_config + from litellm.proxy.proxy_server import prisma_client if prisma_client is None: raise HTTPException( @@ -765,59 +761,12 @@ async def get_sso_settings(): detail={"error": "Database not connected. Please connect a database."}, ) - # Get SSO config from dedicated table + # Resolve the effective SSO config: the stored row wins, else the process + # environment, else each field's default. Unlike the legacy read path this + # does not write os.environ; a GET has no business mutating the environment. sso_db_record = await SSOConfigRepository(prisma_client).table.find_unique(where={"id": "sso_config"}) - - # Initialize with defaults - sso_settings_dict = {} - - if sso_db_record and sso_db_record.sso_settings: - # Load settings from database - sso_settings_dict = dict(sso_db_record.sso_settings) - - role_mappings_data = sso_settings_dict.pop("role_mappings", None) - role_mappings = None - if role_mappings_data: - from litellm.types.proxy.management_endpoints.ui_sso import RoleMappings - - if isinstance(role_mappings_data, dict): - role_mappings = RoleMappings(**role_mappings_data) - elif isinstance(role_mappings_data, RoleMappings): - role_mappings = role_mappings_data - - team_mappings_data = sso_settings_dict.pop("team_mappings", None) - team_mappings = None - if team_mappings_data: - from litellm.types.proxy.management_endpoints.ui_sso import TeamMappings - - if isinstance(team_mappings_data, dict): - team_mappings = TeamMappings(**team_mappings_data) - elif isinstance(team_mappings_data, TeamMappings): - team_mappings = team_mappings_data - - decrypted_sso_settings_dict = proxy_config._decrypt_and_set_db_env_variables( - environment_variables=sso_settings_dict - ) - - # Build SSO config with database values or environment fallback - - sso_config = SSOConfig( - google_client_id=decrypted_sso_settings_dict.get("google_client_id", None), - google_client_secret=decrypted_sso_settings_dict.get("google_client_secret", None), - microsoft_client_id=decrypted_sso_settings_dict.get("microsoft_client_id", None), - microsoft_client_secret=decrypted_sso_settings_dict.get("microsoft_client_secret", None), - microsoft_tenant=decrypted_sso_settings_dict.get("microsoft_tenant", None), - generic_client_id=decrypted_sso_settings_dict.get("generic_client_id", None), - generic_client_secret=decrypted_sso_settings_dict.get("generic_client_secret", None), - generic_authorization_endpoint=decrypted_sso_settings_dict.get("generic_authorization_endpoint", None), - generic_token_endpoint=decrypted_sso_settings_dict.get("generic_token_endpoint", None), - generic_userinfo_endpoint=decrypted_sso_settings_dict.get("generic_userinfo_endpoint", None), - proxy_base_url=decrypted_sso_settings_dict.get("proxy_base_url", None), - user_email=decrypted_sso_settings_dict.get("user_email"), - ui_access_mode=decrypted_sso_settings_dict.get("ui_access_mode"), - role_mappings=role_mappings, - team_mappings=team_mappings, - ) + sso_db_settings = dict(sso_db_record.sso_settings) if sso_db_record and sso_db_record.sso_settings else None + resolved = resolve_sso_config(sso_db_settings, os.environ) # Get the schema for UI display from pydantic import TypeAdapter @@ -826,11 +775,12 @@ async def get_sso_settings(): # Convert to dict for response, masking OAuth client secrets so plaintext # is never sent to the UI. - sso_dict = mask_sensitive_keys(sso_config.model_dump(), _SSO_SENSITIVE_FIELDS) + sso_dict = mask_sensitive_keys(resolved.config.model_dump(), set(SSO_SECRET_FIELDS)) # Add descriptions to the response result = { "values": sso_dict, + "provenance": resolved.provenance, "field_schema": { "description": schema.get("description", ""), "properties": {}, @@ -881,21 +831,6 @@ async def update_sso_settings( detail={"error": "Set `'STORE_MODEL_IN_DB='True'` in your env to enable this feature."}, ) - # Update environment variables - env_var_mapping = { - "google_client_id": "GOOGLE_CLIENT_ID", - "google_client_secret": "GOOGLE_CLIENT_SECRET", - "microsoft_client_id": "MICROSOFT_CLIENT_ID", - "microsoft_client_secret": "MICROSOFT_CLIENT_SECRET", - "microsoft_tenant": "MICROSOFT_TENANT", - "generic_client_id": "GENERIC_CLIENT_ID", - "generic_client_secret": "GENERIC_CLIENT_SECRET", - "generic_authorization_endpoint": "GENERIC_AUTHORIZATION_ENDPOINT", - "generic_token_endpoint": "GENERIC_TOKEN_ENDPOINT", - "generic_userinfo_endpoint": "GENERIC_USERINFO_ENDPOINT", - "proxy_base_url": "PROXY_BASE_URL", - } - # Read the existing SSO row first so the audit log captures a real # before/after diff. Stored values are encrypted; decrypt them so the # before-snapshot has the same shape as after_value, and rely on @@ -924,8 +859,8 @@ async def update_sso_settings( # Update environment variables in config and in memory sso_data = sso_config.model_dump() for field_name, value in sso_data.items(): - if field_name in env_var_mapping: - env_var_name = env_var_mapping[field_name] + if field_name in SSO_FIELD_ENV_VARS: + env_var_name = SSO_FIELD_ENV_VARS[field_name] if value: os.environ[env_var_name] = value else: @@ -975,7 +910,7 @@ async def update_sso_settings( else: environment_variables = {} - env_vars_to_remove = set(env_var_mapping.values()) + env_vars_to_remove = set(SSO_FIELD_ENV_VARS.values()) filtered_env_vars = { key: value for key, value in environment_variables.items() if key not in env_vars_to_remove } diff --git a/litellm/types/proxy/management_endpoints/ui_sso.py b/litellm/types/proxy/management_endpoints/ui_sso.py index 7234cc2650f..742e0f7818f 100644 --- a/litellm/types/proxy/management_endpoints/ui_sso.py +++ b/litellm/types/proxy/management_endpoints/ui_sso.py @@ -148,6 +148,10 @@ class SSOConfig(LiteLLMPydanticObjectBase): default=None, description="User info endpoint URL for generic OAuth provider", ) + generic_scope: Optional[str] = Field( + default=None, + description="Space-separated OAuth scopes requested from the generic provider, e.g. 'openid email profile'", + ) # Common settings proxy_base_url: Optional[str] = Field( diff --git a/tests/test_litellm/proxy/config_resolvers/test_config_resolvers.py b/tests/test_litellm/proxy/config_resolvers/test_config_resolvers.py new file mode 100644 index 00000000000..20bea98351f --- /dev/null +++ b/tests/test_litellm/proxy/config_resolvers/test_config_resolvers.py @@ -0,0 +1,105 @@ +import os + +from litellm.proxy.config_resolvers._descriptors import FieldDescriptor, resolve_fields +from litellm.proxy.config_resolvers.sso import ( + SSO_FIELD_ENV_VARS, + SSO_SECRET_FIELDS, + resolve_sso_config, +) + +_D = ( + FieldDescriptor("client_id", "client_id", "CLIENT_ID"), + FieldDescriptor("scope", "scope", "SCOPE", default="openid"), +) + + +def test_resolve_fields_db_wins_over_env(): + values, provenance = resolve_fields(_D, {"client_id": "from-db"}, {"CLIENT_ID": "from-env"}) + assert values["client_id"] == "from-db" + assert provenance["client_id"] == "db" + + +def test_resolve_fields_blank_db_falls_back_to_env(): + values, provenance = resolve_fields(_D, {"client_id": " "}, {"CLIENT_ID": "from-env"}) + assert values["client_id"] == "from-env" + assert provenance["client_id"] == "env" + + +def test_resolve_fields_blank_everywhere_falls_to_default(): + values, provenance = resolve_fields(_D, {}, {"SCOPE": ""}) + assert values["scope"] == "openid" + assert provenance["scope"] == "default" + + +def test_resolve_fields_unset_everywhere(): + values, provenance = resolve_fields(_D, {}, {}) + assert values["client_id"] is None + assert provenance["client_id"] == "unset" + + +def test_resolve_fields_empty_db_absent_by_default_falls_to_env(): + # SSO semantics: a present-but-empty stored value is absent, so env wins. + values, provenance = resolve_fields(_D, {"client_id": ""}, {"CLIENT_ID": "from-env"}) + assert values["client_id"] == "from-env" + assert provenance["client_id"] == "env" + + +def test_resolve_fields_empty_db_is_explicit_clear_when_flag_set(): + # Alerting semantics: a present-but-empty stored value is an explicit clear + # that must win over a stale env var. + values, provenance = resolve_fields( + _D, {"client_id": ""}, {"CLIENT_ID": "stale-env"}, empty_db_is_set=True + ) + assert values["client_id"] == "" + assert provenance["client_id"] == "db" + + +def test_sso_descriptor_mapping_is_single_sourced(): + # The write path and read path both consume this mapping; it must cover every + # env-backed SSO field and map to the uppercase env var. + assert SSO_FIELD_ENV_VARS["generic_client_id"] == "GENERIC_CLIENT_ID" + assert SSO_SECRET_FIELDS == frozenset( + {"google_client_secret", "microsoft_client_secret", "generic_client_secret"} + ) + + +def test_resolve_sso_config_returns_unmasked_secret_and_provenance(): + # The resolver hands back plaintext; masking is the endpoint's job. If the + # resolver masked, the login path would consume a masked secret and fail. + resolved = resolve_sso_config( + {"generic_client_secret": "super-secret-value"}, + {"GENERIC_CLIENT_ID": "env-id"}, + ) + assert resolved.config.generic_client_secret == "super-secret-value" + assert resolved.provenance["generic_client_secret"] == "db" + assert resolved.config.generic_client_id == "env-id" + assert resolved.provenance["generic_client_id"] == "env" + + +def test_resolve_sso_config_parses_structured_mappings(): + resolved = resolve_sso_config( + { + "generic_client_id": "id", + "role_mappings": { + "provider": "generic", + "group_claim": "groups", + "default_role": "internal_user", + "roles": {}, + }, + "team_mappings": {"team_ids_jwt_field": "teams"}, + }, + {}, + ) + assert resolved.config.role_mappings is not None + assert resolved.config.role_mappings.group_claim == "groups" + assert resolved.config.team_mappings is not None + assert resolved.config.team_mappings.team_ids_jwt_field == "teams" + + +def test_resolve_sso_config_does_not_mutate_os_environ(monkeypatch): + # Unlike the legacy read path, resolving must not write os.environ. + monkeypatch.delenv("GENERIC_CLIENT_ID", raising=False) + before = dict(os.environ) + resolve_sso_config({"generic_client_id": "id-from-db"}, os.environ) + assert dict(os.environ) == before + assert "GENERIC_CLIENT_ID" not in os.environ diff --git a/tests/test_litellm/proxy/test_proxy_server.py b/tests/test_litellm/proxy/test_proxy_server.py index 5effa3073ee..5b67780dc58 100644 --- a/tests/test_litellm/proxy/test_proxy_server.py +++ b/tests/test_litellm/proxy/test_proxy_server.py @@ -1174,6 +1174,113 @@ def test_get_config_returns_email_settings(monkeypatch): assert "*" in variables["SMTP_PASSWORD"] +def _get_email_alert_variables(monkeypatch, config_data): + from litellm.proxy.proxy_server import app, proxy_config, user_api_key_auth + + mock_router = MagicMock() + mock_router.get_settings.return_value = {} + monkeypatch.setattr("litellm.proxy.proxy_server.llm_router", mock_router) + monkeypatch.setattr(proxy_config, "get_config", AsyncMock(return_value=config_data)) + + original_overrides = app.dependency_overrides.copy() + app.dependency_overrides[user_api_key_auth] = lambda: UserAPIKeyAuth( + user_role=LitellmUserRoles.PROXY_ADMIN, api_key="sk-1234" + ) + + client = TestClient(app) + try: + response = client.get("/get/config/callbacks") + finally: + app.dependency_overrides = original_overrides + + assert response.status_code == 200 + email_alert = next((a for a in response.json()["alerts"] if a["name"] == "email"), None) + assert email_alert is not None + return email_alert["variables"] + + +def test_get_config_returns_email_settings_set_only_in_process_env(monkeypatch): + """ + Regression for LIT-4165. + + SMTP supplied purely as process env vars (helm/terraform, no UI writes) is + live at runtime because litellm/proxy/utils.py::send_email resolves every + field from os.getenv. The /get/config/callbacks email block only read the + config/DB environment_variables overlay though, so those deployments saw an + empty Email Server Settings page and could not tell SMTP was configured. + The slack block one branch above already fell back to os.getenv. + """ + smtp_password = "env-only-app-password" + monkeypatch.setenv("SMTP_HOST", "smtp.env-host.com") + monkeypatch.setenv("SMTP_PORT", "2525") + monkeypatch.setenv("SMTP_TLS", "False") + monkeypatch.setenv("SMTP_USERNAME", "env-user") + monkeypatch.setenv("SMTP_PASSWORD", smtp_password) + monkeypatch.setenv("SMTP_SENDER_EMAIL", "alerts@env-host.com") + monkeypatch.setenv("TEST_EMAIL_ADDRESS", "admin@env-host.com") + + variables = _get_email_alert_variables( + monkeypatch, + { + "litellm_settings": {}, + "general_settings": {"alerting": ["email"]}, + "environment_variables": {}, + }, + ) + + # Every one of these was None before the fix, despite SMTP working. + assert variables["SMTP_HOST"] == "smtp.env-host.com" + assert variables["SMTP_PORT"] == "2525" + assert variables["SMTP_TLS"] == "False" + assert variables["SMTP_USERNAME"] == "env-user" + assert variables["SMTP_SENDER_EMAIL"] == "alerts@env-host.com" + assert variables["TEST_EMAIL_ADDRESS"] == "admin@env-host.com" + + # An env-sourced secret is masked exactly like a stored one. + assert variables["SMTP_PASSWORD"] not in (None, smtp_password) + assert "*" in variables["SMTP_PASSWORD"] + + +def test_get_config_email_settings_prefer_stored_over_process_env(monkeypatch): + """ + Stored environment_variables win over the process environment, matching the + load order in ProxyConfig.get_config, which pushes stored values into + os.environ. Only a field with no stored entry falls back to os.getenv. + """ + monkeypatch.setenv("SMTP_HOST", "smtp.env-host.com") + monkeypatch.setenv("SMTP_SENDER_EMAIL", "alerts@env-host.com") + + variables = _get_email_alert_variables( + monkeypatch, + { + "litellm_settings": {}, + "general_settings": {"alerting": ["email"]}, + "environment_variables": {"SMTP_HOST": "smtp.stored-host.com"}, + }, + ) + + assert variables["SMTP_HOST"] == "smtp.stored-host.com" + assert variables["SMTP_SENDER_EMAIL"] == "alerts@env-host.com" + + +def test_get_config_email_settings_absent_everywhere_stay_none(monkeypatch): + """A field set in neither source is reported unset rather than invented.""" + for var in ("SMTP_HOST", "SMTP_PORT", "SMTP_TLS", "SMTP_USERNAME", "SMTP_PASSWORD", "SMTP_SENDER_EMAIL"): + monkeypatch.delenv(var, raising=False) + + variables = _get_email_alert_variables( + monkeypatch, + { + "litellm_settings": {}, + "general_settings": {"alerting": ["email"]}, + "environment_variables": {}, + }, + ) + + assert variables["SMTP_HOST"] is None + assert variables["SMTP_PASSWORD"] is None + + def test_get_config_returns_slack_webhook(monkeypatch): """ Same double-decryption regression as the email block (issue #19221): the diff --git a/tests/test_litellm/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py b/tests/test_litellm/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py index 805baed9e1e..85dbf70b452 100644 --- a/tests/test_litellm/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py +++ b/tests/test_litellm/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py @@ -396,6 +396,146 @@ class TestProxySettingEndpoints: call_args = mock_prisma.db.litellm_ssoconfig.find_unique.call_args assert call_args.kwargs["where"]["id"] == "sso_config" + def _mock_sso_db_record(self, monkeypatch, sso_settings): + """Point /get/sso_settings at a stored SSO row (or None for no row).""" + from unittest.mock import AsyncMock, MagicMock + + mock_prisma = MagicMock() + if sso_settings is None: + mock_db_record = None + else: + mock_db_record = MagicMock() + mock_db_record.sso_settings = sso_settings + mock_prisma.db.litellm_ssoconfig.find_unique = AsyncMock(return_value=mock_db_record) + monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", mock_prisma) + + # The resolver decrypts stored values via decrypt_value_helper; make it an + # identity so the plaintext fixtures round-trip. + monkeypatch.setattr( + "litellm.proxy.config_resolvers.sso.decrypt_value_helper", + lambda value, key, exception_type="error", return_original_value=False: value, + ) + + def test_get_sso_settings_falls_back_to_process_env( + self, mock_proxy_config, mock_auth, monkeypatch + ): + """ + Regression for LIT-4165. + + SSO configured purely as process env vars (helm/terraform, no UI writes) + logs users in successfully, because ui_sso.py resolves every setting from + os.environ. /get/sso_settings read only the sso_config table though, so + the Admin UI showed "not configured" for a working SSO deployment and hid + the Edit/Delete controls behind an empty-state placeholder. + """ + self._mock_sso_db_record(monkeypatch, None) + monkeypatch.setenv("GENERIC_CLIENT_ID", "env-client-id") + monkeypatch.setenv("GENERIC_CLIENT_SECRET", "env-client-secret-value") + monkeypatch.setenv("GENERIC_AUTHORIZATION_ENDPOINT", "https://idp.example.com/authorize") + monkeypatch.setenv("GENERIC_TOKEN_ENDPOINT", "https://idp.example.com/token") + monkeypatch.setenv("GENERIC_USERINFO_ENDPOINT", "https://idp.example.com/userinfo") + monkeypatch.setenv("GENERIC_SCOPE", "openid email profile groups") + monkeypatch.setenv("PROXY_BASE_URL", "https://gateway.example.com") + + response = client.get("/get/sso_settings") + + assert response.status_code == 200 + values = response.json()["values"] + + # Every one of these was None before the fix, despite SSO working. + assert values["generic_client_id"] == "env-client-id" + assert values["generic_authorization_endpoint"] == "https://idp.example.com/authorize" + assert values["generic_token_endpoint"] == "https://idp.example.com/token" + assert values["generic_userinfo_endpoint"] == "https://idp.example.com/userinfo" + assert values["generic_scope"] == "openid email profile groups" + assert values["proxy_base_url"] == "https://gateway.example.com" + + # An env-sourced secret is masked exactly like a stored one. + assert values["generic_client_secret"] not in (None, "env-client-secret-value") + assert "*" in values["generic_client_secret"] + + def test_get_sso_settings_does_not_mutate_os_environ( + self, mock_proxy_config, mock_auth, monkeypatch + ): + """A GET must not write os.environ. The legacy read path decrypted DB + values straight into the environment, so opening the settings page + repopulated env and masked any consumer that stopped reading it.""" + self._mock_sso_db_record(monkeypatch, {"generic_client_id": "db-only-id"}) + monkeypatch.delenv("GENERIC_CLIENT_ID", raising=False) + + response = client.get("/get/sso_settings") + + assert response.status_code == 200 + assert response.json()["values"]["generic_client_id"] == "db-only-id" + # The DB value must NOT have leaked into the process environment. + assert "GENERIC_CLIENT_ID" not in os.environ + + def test_get_sso_settings_prefers_stored_over_process_env( + self, mock_proxy_config, mock_auth, monkeypatch + ): + """A stored value wins; only fields absent from the row fall back to env.""" + self._mock_sso_db_record(monkeypatch, {"generic_client_id": "stored-client-id"}) + monkeypatch.setenv("GENERIC_CLIENT_ID", "env-client-id") + monkeypatch.setenv("GENERIC_TOKEN_ENDPOINT", "https://idp.example.com/token") + + response = client.get("/get/sso_settings") + + assert response.status_code == 200 + values = response.json()["values"] + assert values["generic_client_id"] == "stored-client-id" + assert values["generic_token_endpoint"] == "https://idp.example.com/token" + + def test_get_sso_settings_blank_stored_value_falls_back_to_process_env( + self, mock_proxy_config, mock_auth, monkeypatch + ): + """ + Blank means absent. update_sso_settings clears the env var for a blank + field, so a blank row entry cannot describe a live setting; os.environ is + the effective config and is what the UI must report. + """ + self._mock_sso_db_record(monkeypatch, {"generic_client_id": " ", "generic_token_endpoint": ""}) + monkeypatch.setenv("GENERIC_CLIENT_ID", "env-client-id") + monkeypatch.setenv("GENERIC_TOKEN_ENDPOINT", "https://idp.example.com/token") + + response = client.get("/get/sso_settings") + + assert response.status_code == 200 + values = response.json()["values"] + assert values["generic_client_id"] == "env-client-id" + assert values["generic_token_endpoint"] == "https://idp.example.com/token" + + def test_get_sso_settings_unset_everywhere_reports_source( + self, mock_proxy_config, mock_auth, monkeypatch + ): + """A field set in neither source is unset (or its effective default), + and provenance reports which.""" + self._mock_sso_db_record(monkeypatch, None) + for env_var in ( + "GENERIC_CLIENT_ID", + "GENERIC_CLIENT_SECRET", + "GENERIC_TOKEN_ENDPOINT", + "GENERIC_SCOPE", + "GOOGLE_CLIENT_ID", + "MICROSOFT_CLIENT_ID", + "PROXY_BASE_URL", + ): + monkeypatch.delenv(env_var, raising=False) + + response = client.get("/get/sso_settings") + + assert response.status_code == 200 + body = response.json() + values = body["values"] + provenance = body["provenance"] + assert values["generic_client_id"] is None + assert provenance["generic_client_id"] == "unset" + assert values["generic_client_secret"] is None + assert values["google_client_id"] is None + # generic_scope carries the same effective default the login path applies, + # so the settings page shows the scope logins would actually request. + assert values["generic_scope"] == "openid email profile" + assert provenance["generic_scope"] == "default" + def test_update_sso_settings(self, mock_proxy_config, mock_auth, monkeypatch): """Test updating the SSO settings to the dedicated database table""" import json @@ -1463,19 +1603,20 @@ class TestProxySettingEndpoints: monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", mock_prisma) - # Mock the decryption method to return decrypted values - def mock_decrypt_and_set(environment_variables): - return { - "google_client_id": "decrypted_google_id", - "google_client_secret": "decrypted_google_secret", - "microsoft_client_id": "decrypted_microsoft_id", - "proxy_base_url": "https://decrypted.example.com", - } + # The resolver decrypts each stored value via decrypt_value_helper; map + # the ciphertext fixtures to their plaintext. + decrypted_by_ciphertext = { + "encrypted_google_id": "decrypted_google_id", + "encrypted_google_secret": "decrypted_google_secret", + "encrypted_microsoft_id": "decrypted_microsoft_id", + "encrypted_proxy_url": "https://decrypted.example.com", + } - from litellm.proxy.proxy_server import proxy_config + def mock_decrypt(value, key, exception_type="error", return_original_value=False): + return decrypted_by_ciphertext.get(value, value) monkeypatch.setattr( - proxy_config, "_decrypt_and_set_db_env_variables", mock_decrypt_and_set + "litellm.proxy.config_resolvers.sso.decrypt_value_helper", mock_decrypt ) response = client.get("/get/sso_settings") diff --git a/ui/litellm-dashboard/src/app/(dashboard)/hooks/sso/useSSOSettings.ts b/ui/litellm-dashboard/src/app/(dashboard)/hooks/sso/useSSOSettings.ts index 0431a8d39f7..1a02e363de9 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/hooks/sso/useSSOSettings.ts +++ b/ui/litellm-dashboard/src/app/(dashboard)/hooks/sso/useSSOSettings.ts @@ -24,6 +24,7 @@ export interface SSOSettingsValues { generic_authorization_endpoint: string | null; generic_token_endpoint: string | null; generic_userinfo_endpoint: string | null; + generic_scope: string | null; proxy_base_url: string | null; user_email: string | null; ui_access_mode: string | null; diff --git a/ui/litellm-dashboard/src/components/SSOModals.test.tsx b/ui/litellm-dashboard/src/components/SSOModals.test.tsx index 792a964f01c..c5da4ee7064 100644 --- a/ui/litellm-dashboard/src/components/SSOModals.test.tsx +++ b/ui/litellm-dashboard/src/components/SSOModals.test.tsx @@ -462,6 +462,7 @@ describe("SSOModals", () => { generic_authorization_endpoint: null, generic_token_endpoint: null, generic_userinfo_endpoint: null, + generic_scope: null, proxy_base_url: null, user_email: null, sso_provider: null, diff --git a/ui/litellm-dashboard/src/components/SSOModals.tsx b/ui/litellm-dashboard/src/components/SSOModals.tsx index 88ce72573d7..637abbf4a81 100644 --- a/ui/litellm-dashboard/src/components/SSOModals.tsx +++ b/ui/litellm-dashboard/src/components/SSOModals.tsx @@ -1,11 +1,12 @@ import React, { useEffect, useState } from "react"; -import { Modal, Form, Input, Button as Button2, Select, Checkbox } from "antd"; +import { Modal, Form, Button as Button2, Select, Checkbox } from "antd"; import { Text, TextInput } from "@tremor/react"; import { getSSOSettings, updateSSOSettings } from "./networking"; import NotificationsManager from "./molecules/notifications_manager"; import { parseErrorMessage } from "./shared/errorUtils"; import { Logo } from "@/components/molecules/logo/Logo"; import { ssoProviderDisplayNames, ssoProviderLogoMap } from "./Settings/AdminSettings/SSOSettings/constants"; +import { renderProviderFields } from "./Settings/AdminSettings/SSOSettings/Modals/BaseSSOSettingsForm"; interface SSOModalsProps { isAddSSOModalVisible: boolean; @@ -20,82 +21,6 @@ interface SSOModalsProps { ssoConfigured?: boolean; // Add optional prop to indicate if SSO is configured } -// Define the SSO provider configuration type -interface SSOProviderConfig { - envVarMap: Record; - fields: Array<{ - label: string; - name: string; - placeholder?: string; - }>; -} - -// Define configurations for each SSO provider -const ssoProviderConfigs: Record = { - google: { - envVarMap: { - google_client_id: "GOOGLE_CLIENT_ID", - google_client_secret: "GOOGLE_CLIENT_SECRET", - }, - fields: [ - { label: "Google Client ID", name: "google_client_id" }, - { label: "Google Client Secret", name: "google_client_secret" }, - ], - }, - microsoft: { - envVarMap: { - microsoft_client_id: "MICROSOFT_CLIENT_ID", - microsoft_client_secret: "MICROSOFT_CLIENT_SECRET", - microsoft_tenant: "MICROSOFT_TENANT", - }, - fields: [ - { label: "Microsoft Client ID", name: "microsoft_client_id" }, - { label: "Microsoft Client Secret", name: "microsoft_client_secret" }, - { label: "Microsoft Tenant", name: "microsoft_tenant" }, - ], - }, - okta: { - envVarMap: { - generic_client_id: "GENERIC_CLIENT_ID", - generic_client_secret: "GENERIC_CLIENT_SECRET", - generic_authorization_endpoint: "GENERIC_AUTHORIZATION_ENDPOINT", - generic_token_endpoint: "GENERIC_TOKEN_ENDPOINT", - generic_userinfo_endpoint: "GENERIC_USERINFO_ENDPOINT", - }, - fields: [ - { label: "Generic Client ID", name: "generic_client_id" }, - { label: "Generic Client Secret", name: "generic_client_secret" }, - { - label: "Authorization Endpoint", - name: "generic_authorization_endpoint", - placeholder: "https://your-domain/authorize", - }, - { label: "Token Endpoint", name: "generic_token_endpoint", placeholder: "https://your-domain/token" }, - { - label: "Userinfo Endpoint", - name: "generic_userinfo_endpoint", - placeholder: "https://your-domain/userinfo", - }, - ], - }, - generic: { - envVarMap: { - generic_client_id: "GENERIC_CLIENT_ID", - generic_client_secret: "GENERIC_CLIENT_SECRET", - generic_authorization_endpoint: "GENERIC_AUTHORIZATION_ENDPOINT", - generic_token_endpoint: "GENERIC_TOKEN_ENDPOINT", - generic_userinfo_endpoint: "GENERIC_USERINFO_ENDPOINT", - }, - fields: [ - { label: "Generic Client ID", name: "generic_client_id" }, - { label: "Generic Client Secret", name: "generic_client_secret" }, - { label: "Authorization Endpoint", name: "generic_authorization_endpoint" }, - { label: "Token Endpoint", name: "generic_token_endpoint" }, - { label: "Userinfo Endpoint", name: "generic_userinfo_endpoint" }, - ], - }, -}; - const SSOModals: React.FC = ({ isAddSSOModalVisible, isInstructionsModalVisible, @@ -266,6 +191,7 @@ const SSOModals: React.FC = ({ generic_authorization_endpoint: null, generic_token_endpoint: null, generic_userinfo_endpoint: null, + generic_scope: null, proxy_base_url: null, user_email: null, sso_provider: null, @@ -291,22 +217,6 @@ const SSOModals: React.FC = ({ }; // Helper function to render provider fields - const renderProviderFields = (provider: string) => { - const config = ssoProviderConfigs[provider]; - if (!config) return null; - - return config.fields.map((field) => ( - - {field.name.includes("client") ? : } - - )); - }; - return ( <> = ({ ); }; -export { ssoProviderConfigs }; // Export for use in other components export default SSOModals; diff --git a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/BaseSSOSettingsForm.test.tsx b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/BaseSSOSettingsForm.test.tsx index 21132fff63b..2f9c49dfa56 100644 --- a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/BaseSSOSettingsForm.test.tsx +++ b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/BaseSSOSettingsForm.test.tsx @@ -2,7 +2,7 @@ import { Form } from "antd"; import { act, fireEvent, screen, waitFor } from "@testing-library/react"; import { renderWithProviders } from "../../../../../../tests/test-utils"; import { afterEach, describe, expect, it, vi } from "vitest"; -import BaseSSOSettingsForm, { renderProviderFields } from "./BaseSSOSettingsForm"; +import BaseSSOSettingsForm, { renderProviderFields, ssoProviderConfigs } from "./BaseSSOSettingsForm"; describe("BaseSSOSettingsForm", () => { afterEach(() => { @@ -285,13 +285,70 @@ describe("renderProviderFields", () => { it("should return fields for okta provider", () => { const result = renderProviderFields("okta"); expect(result).not.toBeNull(); - expect(result?.length).toBe(5); + expect(result?.length).toBe(6); }); it("should return fields for generic provider", () => { const result = renderProviderFields("generic"); expect(result).not.toBeNull(); - expect(result?.length).toBe(5); + expect(result?.length).toBe(6); + }); + + it.each(["okta", "generic"])( + "renders an optional generic_scope field for %s so editing cannot clear it", + (provider) => { + const scopeField = ssoProviderConfigs[provider].fields.find((field) => field.name === "generic_scope"); + expect(scopeField).toBeDefined(); + expect(scopeField?.required).toBe(false); + expect(ssoProviderConfigs[provider].envVarMap.generic_scope).toBe("GENERIC_SCOPE"); + }, + ); + + it("submits generic_scope untouched, so saving an unrelated edit cannot clear GENERIC_SCOPE", async () => { + // update_sso_settings clears the env var for any mapped field its payload + // omits, and antd only submits mounted fields. So the Scopes field being + // present is what stops an unrelated edit from downgrading a custom scope + // to the provider default. Dropping the field from ssoProviderConfigs must + // fail here rather than silently in production. + const handleSubmit = vi.fn(); + let form: any; + const TestWrapper = () => { + const [formInstance] = Form.useForm(); + form = formInstance; + return ; + }; + + renderWithProviders(); + + // Mirror EditSSOSettingsModal hydrating the form from the GET response. + await act(async () => { + form.setFieldsValue({ + sso_provider: "generic", + generic_client_id: "client-id", + generic_client_secret: "client-secret", + generic_authorization_endpoint: "https://idp.example.com/authorize", + generic_token_endpoint: "https://idp.example.com/token", + generic_userinfo_endpoint: "https://idp.example.com/userinfo", + generic_scope: "openid email profile groups", + proxy_base_url: "https://gateway.example.com", + user_email: "admin@example.com", + }); + }); + + // The admin edits something else entirely and saves. + await act(async () => { + form.setFieldsValue({ generic_token_endpoint: "https://idp.example.com/token/v2" }); + form.submit(); + }); + + await waitFor(() => { + expect(handleSubmit).toHaveBeenCalledWith( + expect.objectContaining({ + generic_token_endpoint: "https://idp.example.com/token/v2", + generic_scope: "openid email profile groups", + }), + ); + }); }); it("renders provider logos in the dropdown and falls back to a letter avatar on load error", async () => { diff --git a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/BaseSSOSettingsForm.tsx b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/BaseSSOSettingsForm.tsx index 6971c107a73..caa6ff4f1e8 100644 --- a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/BaseSSOSettingsForm.tsx +++ b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/BaseSSOSettingsForm.tsx @@ -18,6 +18,7 @@ export interface SSOProviderConfig { label: string; name: string; placeholder?: string; + required?: boolean; }>; } @@ -52,6 +53,7 @@ export const ssoProviderConfigs: Record = { generic_authorization_endpoint: "GENERIC_AUTHORIZATION_ENDPOINT", generic_token_endpoint: "GENERIC_TOKEN_ENDPOINT", generic_userinfo_endpoint: "GENERIC_USERINFO_ENDPOINT", + generic_scope: "GENERIC_SCOPE", }, fields: [ { label: "Generic Client ID", name: "generic_client_id" }, @@ -67,6 +69,7 @@ export const ssoProviderConfigs: Record = { name: "generic_userinfo_endpoint", placeholder: "https://your-domain/userinfo", }, + { label: "Scopes", name: "generic_scope", placeholder: "openid email profile", required: false }, ], }, generic: { @@ -76,6 +79,7 @@ export const ssoProviderConfigs: Record = { generic_authorization_endpoint: "GENERIC_AUTHORIZATION_ENDPOINT", generic_token_endpoint: "GENERIC_TOKEN_ENDPOINT", generic_userinfo_endpoint: "GENERIC_USERINFO_ENDPOINT", + generic_scope: "GENERIC_SCOPE", }, fields: [ { label: "Generic Client ID", name: "generic_client_id" }, @@ -83,6 +87,7 @@ export const ssoProviderConfigs: Record = { { label: "Authorization Endpoint", name: "generic_authorization_endpoint" }, { label: "Token Endpoint", name: "generic_token_endpoint" }, { label: "Userinfo Endpoint", name: "generic_userinfo_endpoint" }, + { label: "Scopes", name: "generic_scope", placeholder: "openid email profile", required: false }, ], }, }; @@ -97,7 +102,7 @@ export const renderProviderFields = (provider: string) => { key={field.name} label={field.label} name={field.name} - rules={[{ required: true, message: `Please enter the ${field.label.toLowerCase()}` }]} + rules={[{ required: field.required !== false, message: `Please enter the ${field.label.toLowerCase()}` }]} > {field.name.includes("client") ? : } diff --git a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/SSOSettings.tsx b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/SSOSettings.tsx index e3361050422..849921c1076 100644 --- a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/SSOSettings.tsx +++ b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/SSOSettings.tsx @@ -111,6 +111,7 @@ export default function SSOSettings() { label: "User Info Endpoint", render: (values: SSOSettingsValues) => renderEndpointValue(values.generic_userinfo_endpoint), }, + { label: "Scopes", render: (values: SSOSettingsValues) => renderSimpleValue(values.generic_scope) }, { label: "Proxy Base URL", render: (values: SSOSettingsValues) => renderSimpleValue(values.proxy_base_url) }, isTeamMappingsEnabled ? { @@ -143,6 +144,7 @@ export default function SSOSettings() { label: "User Info Endpoint", render: (values: SSOSettingsValues) => renderEndpointValue(values.generic_userinfo_endpoint), }, + { label: "Scopes", render: (values: SSOSettingsValues) => renderSimpleValue(values.generic_scope) }, { label: "Proxy Base URL", render: (values: SSOSettingsValues) => renderSimpleValue(values.proxy_base_url) }, isTeamMappingsEnabled ? { diff --git a/ui/litellm-dashboard/src/components/email_settings.tsx b/ui/litellm-dashboard/src/components/email_settings.tsx index 83968a458b9..2a7988832ea 100644 --- a/ui/litellm-dashboard/src/components/email_settings.tsx +++ b/ui/litellm-dashboard/src/components/email_settings.tsx @@ -26,9 +26,17 @@ const EmailSettings: React.FC = ({ accessToken, premiumUser, .forEach((alert) => { Object.entries(alert.variables ?? {}).forEach(([key, value]) => { const inputElement = document.querySelector(`input[name="${key}"]`) as HTMLInputElement; - if (inputElement && inputElement.value) { - updatedVariables[key] = inputElement?.value; + if (!inputElement || !inputElement.value) { + return; } + // Only send fields the admin actually edited. Values rendered from the + // server are masked (SMTP_PASSWORD) or sourced from the process + // environment, so re-submitting an untouched field would persist a mask + // or copy env-managed config into the database. + if (inputElement.value === (value == null ? "" : String(value))) { + return; + } + updatedVariables[key] = inputElement.value; }); }); diff --git a/ui/litellm-dashboard/src/lib/http/schema.d.ts b/ui/litellm-dashboard/src/lib/http/schema.d.ts index 203057f23f1..d109fc1783d 100644 --- a/ui/litellm-dashboard/src/lib/http/schema.d.ts +++ b/ui/litellm-dashboard/src/lib/http/schema.d.ts @@ -30686,6 +30686,11 @@ export interface components { * @description Generic OAuth Client Secret for SSO authentication */ generic_client_secret?: string | null; + /** + * Generic Scope + * @description Space-separated OAuth scopes requested from the generic provider, e.g. 'openid email profile' + */ + generic_scope?: string | null; /** * Generic Token Endpoint * @description Token endpoint URL for generic OAuth provider @@ -30750,6 +30755,10 @@ export interface components { field_schema: { [key: string]: unknown; }; + /** Provenance */ + provenance?: { + [key: string]: string; + }; /** Values */ values: { [key: string]: unknown; From 8d217a4d5f9f5511fc6068a61e5718d42a2b8200 Mon Sep 17 00:00:00 2001 From: Yassin Kortam Date: Wed, 22 Jul 2026 15:15:37 -0700 Subject: [PATCH 08/41] fix(scim): parse membership id from filtered PATCH path when value omitted (#34181) Okta commonly sends SCIM membership removals as a filtered path with no request body value, e.g. Groups PATCH members[value eq "uid"] and Users PATCH groups[value eq "tid"]. The patch handlers pulled ids only from op.value, so these removes were a silent no-op and the member or team was never dropped Add a linear-time filter parser reused by both the Groups members path and the Users groups path so the id is taken from the [value eq "..."] filter when op.value is absent, for add and remove ops. The eq operator is matched case-insensitively, both quote styles are accepted, and the quoted value is unescaped. The path-filter fallback only fires when the request body value is omitted, so an explicit empty value no longer resurrects the filter id, and the compared value must be quoted per the SCIM filter grammar --- .../management_endpoints/scim/scim_v2.py | 33 +++- .../scim/test_scim_patch_user.py | 55 +++++++ .../scim/test_scim_v2_endpoints.py | 142 ++++++++++++++++++ 3 files changed, 228 insertions(+), 2 deletions(-) diff --git a/litellm/proxy/management_endpoints/scim/scim_v2.py b/litellm/proxy/management_endpoints/scim/scim_v2.py index 525ce9f0e89..90eae5bbb21 100644 --- a/litellm/proxy/management_endpoints/scim/scim_v2.py +++ b/litellm/proxy/management_endpoints/scim/scim_v2.py @@ -1353,6 +1353,31 @@ def _extract_group_values(value: Any) -> List[str]: return group_values +def _extract_ids_from_path_filter(path: str | None, attribute: str) -> List[str]: + """Return ids from a SCIM filtered path like ``members[value eq "id"]``. + + Okta commonly sends membership removals as a filtered path and omits the + request body ``value``, so the id lives only inside the ``[value eq "..."]`` + filter. The ``eq`` operator is matched case-insensitively per the SCIM + spec; the id keeps its original case. Per the SCIM filter grammar the + compared value must be quoted (single or double), so malformed unquoted + filters yield no id. A quoted id may contain escaped quotes and + backslashes (``\\"`` and ``\\\\``), which are unescaped before use. + ``path`` must be the raw, case-preserving path from the patch op. + """ + if not path: + return [] + match = re.match( + rf"""\s*{re.escape(attribute)}\s*\[\s*value\s+eq\s+(['"])((?:\\.|[^\\])*?)\1\s*\]\s*$""", + path, + flags=re.IGNORECASE, + ) + if not match: + return [] + extracted = re.sub(r"\\(.)", r"\1", match.group(2)) + return [extracted] if extracted else [] + + def _handle_displayname_update(op_type: str, value: Any, update_data: Dict[str, Any]) -> None: """Handle displayname updates.""" if op_type == "remove": @@ -1396,9 +1421,11 @@ def _handle_name_update(path: str, op_type: str, value: Any, scim_metadata: Dict scim_metadata["familyName"] = str(value) -def _handle_group_operations(op_type: str, value: Any, teams_set: Set[str]) -> Optional[Set[str]]: +def _handle_group_operations(op_type: str, value: Any, teams_set: Set[str], path: str | None) -> Set[str] | None: """Handle group/team membership operations.""" group_values = _extract_group_values(value) + if not group_values and value is None: + group_values = _extract_ids_from_path_filter(path, "groups") if op_type == "replace": return set(group_values) elif op_type == "add": @@ -1511,7 +1538,7 @@ def _apply_patch_ops( elif _multi_valued_attribute_base(path) in SCIM_MULTI_VALUED_ATTRIBUTE_METADATA_KEYS: _handle_multi_valued_attribute_update(path, op_type, value, metadata) elif path.startswith("groups"): - new_replace_set = _handle_group_operations(op_type, value, teams_set) + new_replace_set = _handle_group_operations(op_type, value, teams_set, op.path) if new_replace_set is not None: replace_team_set = new_replace_set else: @@ -1975,6 +2002,8 @@ async def _process_group_patch_operations( elif path.startswith("members"): # Handle member operations member_values = _extract_group_values(value) + if not member_values and value is None: + member_values = _extract_ids_from_path_filter(op.path, "members") # Check the feature flag scim_upsert_user = await _get_scim_upsert_user_setting() # Validate all users exist or create them based on feature flag 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 f8995a6f4da..c3af4208d37 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 @@ -465,3 +465,58 @@ def test_apply_patch_ops_filtered_path_raises_400_instead_of_junk_metadata(): ) assert exc_info.value.status_code == 400 + + +def test_apply_patch_ops_remove_group_filtered_path_without_value(): + """Okta removes a user from a team with groups[value eq "..."] and no body + value; the team id must be parsed from the filter so the remove takes effect""" + user = LiteLLM_UserTable( + user_id="user-fp", + user_email="fp@example.com", + teams=["team-1", "team-2"], + metadata={}, + ) + patch_ops = SCIMPatchOp( + Operations=[SCIMPatchOperation(op="remove", path='groups[value eq "team-1"]')] + ) + + _, final_team_set = _apply_patch_ops(existing_user=user, patch_ops=patch_ops) + + assert final_team_set == {"team-2"} + + +def test_apply_patch_ops_add_group_filtered_path_without_value(): + """A filtered add path with no body value adds the team id from the filter.""" + user = LiteLLM_UserTable( + user_id="user-fp", + user_email="fp@example.com", + teams=["team-1"], + metadata={}, + ) + patch_ops = SCIMPatchOp( + Operations=[SCIMPatchOperation(op="add", path="groups[value eq 'team-3']")] + ) + + _, final_team_set = _apply_patch_ops(existing_user=user, patch_ops=patch_ops) + + assert final_team_set == {"team-1", "team-3"} + + +def test_apply_patch_ops_replace_groups_empty_value_does_not_use_path_filter(): + """A filtered replace with an explicit empty value must not resurrect the + filter id; the team set is replaced with the empty value as given.""" + user = LiteLLM_UserTable( + user_id="user-fp", + user_email="fp@example.com", + teams=["team-1", "team-2"], + metadata={}, + ) + patch_ops = SCIMPatchOp( + Operations=[ + SCIMPatchOperation(op="replace", path='groups[value eq "team-1"]', value=[]) + ] + ) + + _, final_team_set = _apply_patch_ops(existing_user=user, patch_ops=patch_ops) + + assert final_team_set == set() 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 850b8fc7cfd..7bb74285ac6 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 @@ -1,3 +1,4 @@ +import time from unittest.mock import AsyncMock import pytest @@ -17,6 +18,7 @@ from litellm.proxy.management_endpoints.scim.scim_v2 import ( UserProvisionerHelpers, _apply_group_patch_updates, _extract_group_member_ids, + _extract_ids_from_path_filter, _handle_team_membership_changes, _process_group_patch_operations, _recompute_scim_member_roles, @@ -3377,3 +3379,143 @@ async def test_patch_group_replace_stays_absolute_against_concurrent_roster(mock call.kwargs["user_id"] for call in calls if call.kwargs.get("teams_ids_to_add_user_to") == [group_id] } assert added_user_ids == set() + + +@pytest.mark.parametrize( + "path, attribute, expected", + [ + ('members[value eq "user-1"]', "members", ["user-1"]), + ("members[value eq 'user-1']", "members", ["user-1"]), + ('members[value EQ "user-1"]', "members", ["user-1"]), + ('members[ value eq "user-1" ]', "members", ["user-1"]), + ('groups[value eq "team-1"]', "groups", ["team-1"]), + ('members[value eq "Mixed-CASE-Id"]', "members", ["Mixed-CASE-Id"]), + ('members[value eq "a\\"b"]', "members", ['a"b']), + ('members[value eq "a\\\\b"]', "members", ["a\\b"]), + ("members[value eq 'a\\'b']", "members", ["a'b"]), + ("members", "members", []), + ('groups[value eq "team-1"]', "members", []), + (None, "members", []), + ('members[value eq ""]', "members", []), + ("members[value eq user-1]", "members", []), + ("members[value eq unintendeduser]", "members", []), + ], +) +def test_extract_ids_from_path_filter(path, attribute, expected): + assert _extract_ids_from_path_filter(path, attribute) == expected + + +def test_extract_ids_from_path_filter_unterminated_is_linear(): + """A pathological unterminated quoted filter must not trigger super-linear + backtracking; it returns no id and completes near-instantly.""" + pathological = 'members[value eq "' + ("\\" * 200) + + start = time.perf_counter() + result = _extract_ids_from_path_filter(pathological, "members") + elapsed = time.perf_counter() - start + + assert result == [] + assert elapsed < 1.0 + + +@pytest.mark.asyncio +async def test_process_group_patch_remove_filtered_path_without_value(mocker): + """Okta sends group membership removals as a filtered path with no request + body value; the member id must be parsed out of members[value eq "..."]""" + patch_ops = SCIMPatchOp( + schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + Operations=[SCIMPatchOperation(op="remove", path='members[value eq "user-1"]')], + ) + + existing_team = LiteLLM_TeamTable( + team_id="team-1", + team_alias="Team One", + members=[], + members_with_roles=[ + Member(user_id="user-1", role="user"), + Member(user_id="user-2", role="user"), + ], + ) + + prisma_client = mocker.MagicMock() + prisma_client.db = mocker.MagicMock() + prisma_client.db.litellm_usertable = mocker.MagicMock() + prisma_client.db.litellm_usertable.find_unique = AsyncMock( + return_value=LiteLLM_UserTable(user_id="user-1") + ) + + _, final_members, _ = await _process_group_patch_operations( + patch_ops=patch_ops, + existing_team=existing_team, + prisma_client=prisma_client, + ) + + assert final_members == {"user-2"} + + +@pytest.mark.asyncio +async def test_process_group_patch_add_filtered_path_without_value(mocker): + """A filtered add path with no body value adds the id parsed from the filter.""" + patch_ops = SCIMPatchOp( + schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + Operations=[SCIMPatchOperation(op="add", path='members[value eq "user-3"]')], + ) + + existing_team = LiteLLM_TeamTable( + team_id="team-1", + team_alias="Team One", + members=[], + members_with_roles=[Member(user_id="user-1", role="user")], + ) + + prisma_client = mocker.MagicMock() + prisma_client.db = mocker.MagicMock() + prisma_client.db.litellm_usertable = mocker.MagicMock() + prisma_client.db.litellm_usertable.find_unique = AsyncMock( + return_value=LiteLLM_UserTable(user_id="user-3") + ) + + _, final_members, _ = await _process_group_patch_operations( + patch_ops=patch_ops, + existing_team=existing_team, + prisma_client=prisma_client, + ) + + assert final_members == {"user-1", "user-3"} + + +@pytest.mark.asyncio +async def test_process_group_patch_replace_empty_value_does_not_use_path_filter(mocker): + """An explicit empty replace value must clear membership rather than pull an + id from the filtered path, which would retain one member and drop the rest.""" + patch_ops = SCIMPatchOp( + schemas=["urn:ietf:params:scim:api:messages:2.0:PatchOp"], + Operations=[ + SCIMPatchOperation(op="replace", path='members[value eq "user-1"]', value=[]) + ], + ) + + existing_team = LiteLLM_TeamTable( + team_id="team-1", + team_alias="Team One", + members=[], + members_with_roles=[ + Member(user_id="user-1", role="user"), + Member(user_id="user-2", role="user"), + ], + ) + + prisma_client = mocker.MagicMock() + prisma_client.db = mocker.MagicMock() + prisma_client.db.litellm_usertable = mocker.MagicMock() + prisma_client.db.litellm_usertable.find_unique = AsyncMock( + return_value=LiteLLM_UserTable(user_id="user-1") + ) + + _, final_members, _ = await _process_group_patch_operations( + patch_ops=patch_ops, + existing_team=existing_team, + prisma_client=prisma_client, + ) + + assert final_members == set() From 0b4446edccf282cc63a29d7f6b8415211b13fc3f Mon Sep 17 00:00:00 2001 From: "devin-ai-integration[bot]" <158243242+devin-ai-integration[bot]@users.noreply.github.com> Date: Wed, 22 Jul 2026 15:16:52 -0700 Subject: [PATCH 09/41] fix(ui): remove misleading os.environ tooltip from logging settings (#34305) Co-authored-by: yuneng Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- ui/litellm-dashboard/src/components/team/LoggingSettings.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/ui/litellm-dashboard/src/components/team/LoggingSettings.tsx b/ui/litellm-dashboard/src/components/team/LoggingSettings.tsx index 3572082872e..80bea3ef2d2 100644 --- a/ui/litellm-dashboard/src/components/team/LoggingSettings.tsx +++ b/ui/litellm-dashboard/src/components/team/LoggingSettings.tsx @@ -117,9 +117,6 @@ const LoggingSettings: React.FC = ({
= ({ accessToken, userRole }) => { /> {promptToDelete && ( - { + if (!open && !isDeleting) handleDeleteCancel(); + }} > -

Are you sure you want to delete prompt: {promptToDelete.name} ?

-

This action cannot be undone.

- + + + Delete Prompt + + Are you sure you want to delete prompt: {promptToDelete.name} ? This action cannot be undone. + + + + Cancel + + + + )} ); From 692e6d48e983832939d7e3e26c911e21c9ee7ef3 Mon Sep 17 00:00:00 2001 From: yuneng-jiang Date: Wed, 22 Jul 2026 17:01:13 -0700 Subject: [PATCH 18/41] refactor(ui): migrate old-usage to shadcn (#34304) * test(ui): characterise the old usage page before migrating it Role- and text-based coverage of the route as it behaves on Tremor, so the shadcn migration has a regression net it did not get to write. Pins the DISABLE_EXPENSIVE_DB_QUERIES branch (warning copy, the docs link and its target, and that every expensive query is skipped), the admin vs non-admin tab set, the cost cards, and the provider and customer tables * refactor(ui): migrate old-usage to shadcn Replaces Tremor with the installed shadcn primitives and the shared recharts wrappers on the only file the route owns. Tabs, cards, tables, the key select and the tag multi-select come from src/components/ui; the bar, area and donut charts come from src/components/shared/charts. Tremor BarList has no shared equivalent, so Total Spend Per Team is composed from ui/meter, which also means the per-team totals stay numbers in state instead of pre-formatted strings; a team total of 1,000 or more used to make the bar widths NaN. The Database Query Limit Reached warning moves with it: same copy, same docs link, still short-circuiting every expensive query. Drops the file's no-restricted-imports suppression and the dead customTooltip, getTopKeys, DataDict and UserData symbols. The characterisation test from the previous commit is unchanged and green on both sides --- ui/litellm-dashboard/eslint-suppressions.json | 3 - .../old-usage/_components/usage.test.tsx | 202 +++++ .../old-usage/_components/usage.tsx | 855 +++++++++--------- 3 files changed, 622 insertions(+), 438 deletions(-) create mode 100644 ui/litellm-dashboard/src/app/(dashboard)/old-usage/_components/usage.test.tsx diff --git a/ui/litellm-dashboard/eslint-suppressions.json b/ui/litellm-dashboard/eslint-suppressions.json index f8927becba0..b9b27e6cbb5 100644 --- a/ui/litellm-dashboard/eslint-suppressions.json +++ b/ui/litellm-dashboard/eslint-suppressions.json @@ -664,9 +664,6 @@ } }, "src/app/(dashboard)/old-usage/_components/usage.tsx": { - "no-restricted-imports": { - "count": 2 - }, "react-hooks/immutability": { "count": 1 }, diff --git a/ui/litellm-dashboard/src/app/(dashboard)/old-usage/_components/usage.test.tsx b/ui/litellm-dashboard/src/app/(dashboard)/old-usage/_components/usage.test.tsx new file mode 100644 index 00000000000..e3db50b7300 --- /dev/null +++ b/ui/litellm-dashboard/src/app/(dashboard)/old-usage/_components/usage.test.tsx @@ -0,0 +1,202 @@ +import React from "react"; +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { screen, waitFor, within } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { renderWithProviders } from "../../../../../tests/test-utils"; +import UsagePage from "./usage"; + +const networking = vi.hoisted(() => ({ + adminSpendLogsCall: vi.fn(), + adminTopKeysCall: vi.fn(), + adminTopModelsCall: vi.fn(), + adminTopEndUsersCall: vi.fn(), + teamSpendLogsCall: vi.fn(), + tagsSpendLogsCall: vi.fn(), + allTagNamesCall: vi.fn(), + adminspendByProvider: vi.fn(), + adminGlobalActivity: vi.fn(), + adminGlobalActivityPerModel: vi.fn(), + getProxyUISettings: vi.fn(), + modelAvailableCall: vi.fn(), + keyInfoV1Call: vi.fn(), +})); + +vi.mock("@/components/networking", () => networking); +vi.mock("../../../../components/networking", () => networking); + +vi.mock("@/app/(dashboard)/hooks/useAuthorized", () => ({ + default: () => ({ + accessToken: "sk-test", + token: "tok", + userRole: "Admin", + userId: "u1", + premiumUser: true, + }), +})); + +const UNLIMITED_SETTINGS = { DISABLE_EXPENSIVE_DB_QUERIES: false, NUM_SPEND_LOGS_ROWS: 10 }; + +const renderUsage = (overrides: Partial> = {}) => + renderWithProviders( + , + ); + +beforeEach(() => { + vi.clearAllMocks(); + networking.getProxyUISettings.mockResolvedValue(UNLIMITED_SETTINGS); + networking.adminSpendLogsCall.mockResolvedValue([{ date: "2026-07-01", spend: 12.5 }]); + networking.adminTopKeysCall.mockResolvedValue([ + { api_key: "sk-abcdefghijk", key_alias: "prod-key", total_spend: 9.5 }, + ]); + networking.adminTopModelsCall.mockResolvedValue([{ model: "gpt-5.1", total_spend: 7.25 }]); + networking.adminTopEndUsersCall.mockResolvedValue([ + { end_user: "customer-alpha", total_spend: 3.5, total_count: 42 }, + ]); + networking.teamSpendLogsCall.mockResolvedValue({ + daily_spend: [{ date: "2026-07-01", "team-a": 5 }], + teams: ["team-a"], + total_spend_per_team: [{ team_id: "team-a", total_spend: 5 }], + }); + networking.tagsSpendLogsCall.mockResolvedValue({ spend_per_tag: [{ name: "prod", spend: 4 }] }); + networking.allTagNamesCall.mockResolvedValue({ tag_names: ["prod", "staging"] }); + networking.adminspendByProvider.mockResolvedValue([{ provider: "openai", spend: 6.75 }]); + networking.adminGlobalActivity.mockResolvedValue({ + sum_api_requests: 120, + sum_total_tokens: 4500, + daily_data: [{ date: "2026-07-01", api_requests: 120, total_tokens: 4500 }], + }); + networking.adminGlobalActivityPerModel.mockResolvedValue([]); + networking.modelAvailableCall.mockResolvedValue({ data: [] }); + networking.keyInfoV1Call.mockResolvedValue({ info: {} }); +}); + +describe("old usage page", () => { + describe("when the proxy has disabled expensive DB queries", () => { + beforeEach(() => { + networking.getProxyUISettings.mockResolvedValue({ + DISABLE_EXPENSIVE_DB_QUERIES: true, + NUM_SPEND_LOGS_ROWS: 2500000, + }); + }); + + it("shows the database query limit warning instead of the usage dashboard", async () => { + renderUsage(); + + expect(await screen.findByText("Database Query Limit Reached")).toBeInTheDocument(); + expect(screen.getByText(/SpendLogs in DB has/)).toHaveTextContent("2500000"); + expect(screen.getByText(/Please follow our guide to view usage when SpendLogs has more than 1M rows/i)); + expect(screen.queryByRole("tab", { name: "All Up" })).not.toBeInTheDocument(); + }); + + it("links to the cost tracking guide in a new tab", async () => { + renderUsage(); + + const link = await screen.findByRole("link", { name: "View Usage Guide" }); + expect(link).toHaveAttribute("href", "https://docs.litellm.ai/docs/proxy/cost_tracking"); + expect(link).toHaveAttribute("target", "_blank"); + }); + + it("skips every expensive usage query", async () => { + renderUsage(); + + await screen.findByText("Database Query Limit Reached"); + await waitFor(() => expect(networking.getProxyUISettings).toHaveBeenCalled()); + + expect(networking.adminSpendLogsCall).not.toHaveBeenCalled(); + expect(networking.adminspendByProvider).not.toHaveBeenCalled(); + expect(networking.adminTopKeysCall).not.toHaveBeenCalled(); + expect(networking.adminTopModelsCall).not.toHaveBeenCalled(); + expect(networking.adminGlobalActivity).not.toHaveBeenCalled(); + expect(networking.adminGlobalActivityPerModel).not.toHaveBeenCalled(); + expect(networking.teamSpendLogsCall).not.toHaveBeenCalled(); + expect(networking.adminTopEndUsersCall).not.toHaveBeenCalled(); + expect(networking.tagsSpendLogsCall).not.toHaveBeenCalled(); + }); + }); + + describe("as an admin", () => { + it("renders the admin tabs", async () => { + renderUsage(); + + expect(await screen.findByRole("tab", { name: "All Up" })).toBeInTheDocument(); + expect(screen.getByRole("tab", { name: "Team Based Usage" })).toBeInTheDocument(); + expect(screen.getByRole("tab", { name: "Customer Usage" })).toBeInTheDocument(); + expect(screen.getByRole("tab", { name: "Tag Based Usage" })).toBeInTheDocument(); + }); + + it("renders the cost panel cards", async () => { + renderUsage(); + + expect(await screen.findByText("Monthly Spend")).toBeInTheDocument(); + expect(screen.getByText("Top Virtual Keys")).toBeInTheDocument(); + expect(screen.getByText("Top Models")).toBeInTheDocument(); + expect(screen.getByText("Spend by Provider")).toBeInTheDocument(); + }); + + it("lists spend by provider in a table", async () => { + renderUsage(); + + const providerCell = await screen.findByText("openai"); + const row = providerCell.closest("tr"); + expect(row).not.toBeNull(); + expect(within(row as HTMLElement).getByText("$6.75")).toBeInTheDocument(); + expect(screen.getByRole("columnheader", { name: "Provider" })).toBeInTheDocument(); + }); + + it("shows the customer usage table when its tab is selected", async () => { + const user = userEvent.setup(); + renderUsage(); + + await user.click(await screen.findByRole("tab", { name: "Customer Usage" })); + + const customerCell = await screen.findByText("customer-alpha"); + const row = customerCell.closest("tr"); + expect(row).not.toBeNull(); + expect(within(row as HTMLElement).getByText("$3.50")).toBeInTheDocument(); + expect(within(row as HTMLElement).getByText("42")).toBeInTheDocument(); + expect(screen.getByRole("columnheader", { name: "Total Events" })).toBeInTheDocument(); + }); + + it("shows the tag spend panel when its tab is selected", async () => { + const user = userEvent.setup(); + renderUsage(); + + await user.click(await screen.findByRole("tab", { name: "Tag Based Usage" })); + + expect(await screen.findByText("Spend Per Tag")).toBeInTheDocument(); + }); + + it("shows the team spend panel when its tab is selected", async () => { + const user = userEvent.setup(); + renderUsage(); + + await user.click(await screen.findByRole("tab", { name: "Team Based Usage" })); + + expect(await screen.findByText("Total Spend Per Team")).toBeInTheDocument(); + expect(screen.getByText("Daily Spend Per Team")).toBeInTheDocument(); + }); + }); + + describe("as a non-admin", () => { + it("renders only the All Up tab and skips admin-only queries", async () => { + renderUsage({ userRole: "Internal User" }); + + expect(await screen.findByRole("tab", { name: "All Up" })).toBeInTheDocument(); + expect(screen.queryByRole("tab", { name: "Team Based Usage" })).not.toBeInTheDocument(); + expect(screen.queryByRole("tab", { name: "Customer Usage" })).not.toBeInTheDocument(); + expect(screen.queryByRole("tab", { name: "Tag Based Usage" })).not.toBeInTheDocument(); + + await waitFor(() => expect(networking.adminSpendLogsCall).toHaveBeenCalled()); + expect(networking.teamSpendLogsCall).not.toHaveBeenCalled(); + expect(networking.adminTopEndUsersCall).not.toHaveBeenCalled(); + }); + }); +}); diff --git a/ui/litellm-dashboard/src/app/(dashboard)/old-usage/_components/usage.tsx b/ui/litellm-dashboard/src/app/(dashboard)/old-usage/_components/usage.tsx index 01f8cb1cd45..3d55f9bb698 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/old-usage/_components/usage.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/old-usage/_components/usage.tsx @@ -1,40 +1,26 @@ -import { - BarChart, - BarList, - Card, - Title, - Table, - TableHead, - TableHeaderCell, - TableRow, - TableCell, - TableBody, - Subtitle, -} from "@tremor/react"; - import React, { useState, useEffect } from "react"; import ViewUserSpend from "@/components/view_user_spend"; import { ProxySettings } from "@/components/user_dashboard"; import UsageDatePicker from "@/components/shared/usage_date_picker"; +import { Button } from "@/components/ui/button"; +import { Card, CardContent, CardHeader, CardTitle } from "@/components/ui/card"; import { - Grid, - Col, - Text, - TabPanel, - TabPanels, - TabGroup, - TabList, - Tab, - Select, - SelectItem, - DateRangePickerValue, - DonutChart, - AreaChart, - Button, - MultiSelect, - MultiSelectItem, -} from "@tremor/react"; + Combobox, + ComboboxChip, + ComboboxChips, + ComboboxChipsInput, + ComboboxContent, + ComboboxEmpty, + ComboboxItem, + ComboboxList, + ComboboxValue, +} from "@/components/ui/combobox"; +import { Meter, MeterIndicator, MeterTrack } from "@/components/ui/meter"; +import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from "@/components/ui/select"; +import { Table, TableBody, TableCell, TableHead, TableHeader, TableRow } from "@/components/ui/table"; +import { Tabs, TabsContent, TabsList, TabsTrigger } from "@/components/ui/tabs"; +import { AreaChart, BarChart, DonutChart } from "@/components/shared/charts"; import { adminSpendLogsCall, @@ -68,69 +54,41 @@ interface GlobalActivityData { daily_data: { date: string; api_requests: number; total_tokens: number }[]; } -type CustomTooltipTypeBar = { - payload: any; - active: boolean | undefined; - label: any; -}; +type UsageDateRange = { from?: Date; to?: Date }; -const customTooltip = (props: CustomTooltipTypeBar) => { - const { payload, active } = props; - if (!active || !payload) return null; +type TeamSpendTotal = { name: string; value: number }; - const value = payload[0].payload; - const date = value["startTime"]; - const model_values = value["models"]; - const entries: [string, number][] = Object.entries(model_values).map(([key, value]) => [key, value as number]); +type TagOption = { value: string; label: string; disabled: boolean }; - entries.sort((a, b) => b[1] - a[1]); - const topEntries = entries.slice(0, 5); - - return ( -
- {date} - {topEntries.map(([key, value]) => ( -
-
-

- {key} - {":"} - - {" "} - {value ? `$${formatNumberWithCommas(value, 2)}` : ""} - -

-
-
- ))} -
- ); -}; - -function getTopKeys(data: Array<{ [key: string]: unknown }>): any[] { - const spendKeys: { key: string; spend: unknown }[] = []; - - data.forEach((dict) => { - Object.entries(dict).forEach(([key, value]) => { - if (key !== "spend" && key !== "startTime" && key !== "models" && key !== "users") { - spendKeys.push({ key, spend: value }); - } - }); - }); - - spendKeys.sort((a, b) => Number(b.spend) - Number(a.spend)); - - const topKeys = spendKeys.slice(0, 5).map((k) => k.key); - return topKeys; -} -type DataDict = { [key: string]: unknown }; -type UserData = { user_id: string; spend: number }; +const ALL_TAGS = "all-tags"; const isAdminOrAdminViewer = (role: string | null): boolean => { if (role === null) return false; return role === "Admin" || role === "Admin Viewer"; }; +const TeamSpendBarList: React.FC<{ data: TeamSpendTotal[] }> = ({ data }) => { + const max = Math.max(0, ...data.map((team) => team.value)); + + return ( +
+ {data.map((team) => ( +
+

{team.name}

+ + + + + +

+ {formatNumberWithCommas(team.value, 2)} +

+
+ ))} +
+ ); +}; + const UsagePage: React.FC = ({ accessToken, token, userRole, userID, keys, premiumUser }) => { const currentDate = new Date(); const [keySpendData, setKeySpendData] = useState([]); @@ -141,13 +99,13 @@ const UsagePage: React.FC = ({ accessToken, token, userRole, use const [topTagsData, setTopTagsData] = useState([]); const [allTagNames, setAllTagNames] = useState([]); const [uniqueTeamIds, setUniqueTeamIds] = useState([]); - const [totalSpendPerTeam, setTotalSpendPerTeam] = useState([]); + const [totalSpendPerTeam, setTotalSpendPerTeam] = useState([]); const [spendByProvider, setSpendByProvider] = useState([]); const [globalActivity, setGlobalActivity] = useState({} as GlobalActivityData); const [globalActivityPerModel, setGlobalActivityPerModel] = useState([]); - const [selectedKeyID, setSelectedKeyID] = useState(""); - const [selectedTags, setSelectedTags] = useState(["all-tags"]); - const [dateValue, setDateValue] = useState({ + const [selectedKeyToken, setSelectedKeyToken] = useState(null); + const [selectedTags, setSelectedTags] = useState([ALL_TAGS]); + const [dateValue, setDateValue] = useState({ from: new Date(Date.now() - 7 * 24 * 60 * 60 * 1000), to: new Date(), }); @@ -160,6 +118,21 @@ const UsagePage: React.FC = ({ accessToken, token, userRole, use let startTime = formatDate(firstDay); let endTime = formatDate(lastDay); + const selectableKeys: { token: string; alias: string }[] = (keys ?? []) + .filter((key: any) => key && typeof key["key_alias"] === "string" && key["key_alias"].length > 0) + .map((key: any) => ({ token: String(key["token"]), alias: String(key["key_alias"]) })); + + const tagOptions: TagOption[] = [ + { value: ALL_TAGS, label: "All Tags", disabled: false }, + ...allTagNames + .filter((tag) => tag !== ALL_TAGS) + .map((tag) => ({ + value: tag, + label: premiumUser ? tag : `✨ ${tag} (Enterprise only Feature)`, + disabled: !premiumUser, + })), + ]; + function valueFormatterNumbers(number: number) { const formatter = new Intl.NumberFormat("en-US", { maximumFractionDigits: 0, @@ -405,7 +378,7 @@ const UsagePage: React.FC = ({ accessToken, token, userRole, use setUniqueTeamIds(teamSpend.teams); return teamSpend.total_spend_per_team.map((tspt: any) => ({ name: tspt["team_id"] || "", - value: formatNumberWithCommas(tspt["total_spend"] || 0, 2), + value: Number(tspt["total_spend"] || 0), })); }, setTotalSpendPerTeam, @@ -524,223 +497,252 @@ const UsagePage: React.FC = ({ accessToken, token, userRole, use if (proxySettings?.DISABLE_EXPENSIVE_DB_QUERIES) { return ( -
+
- Database Query Limit Reached - - SpendLogs in DB has {proxySettings.NUM_SPEND_LOGS_ROWS} rows. -

- Please follow our guide to view usage when SpendLogs has more than 1M rows. - - + + Database Query Limit Reached + + +

+ SpendLogs in DB has {proxySettings.NUM_SPEND_LOGS_ROWS} rows. +

+ Please follow our guide to view usage when SpendLogs has more than 1M rows. +

+
); } return ( -
- - - All Up +
+ + + All Up - {isAdminOrAdminViewer(userRole) ? ( + {isAdminOrAdminViewer(userRole) && ( <> - Team Based Usage - Customer Usage - Tag Based Usage - - ) : ( - <> -
+ Team Based Usage + Customer Usage + Tag Based Usage )} - - - - - - Cost - Activity - - - - - - - Project Spend {new Date().toLocaleString("default", { month: "long" })} 1 -{" "} - {new Date(new Date().getFullYear(), new Date().getMonth() + 1, 0).getDate()} - - - - - - Monthly Spend - + + + + + Cost + Activity + + + +
+
+

+ Project Spend {new Date().toLocaleString("default", { month: "long" })} 1 -{" "} + {new Date(new Date().getFullYear(), new Date().getMonth() + 1, 0).getDate()} +

+ +
+
+ + + Monthly Spend + + + + + +
+
+ + + Top Virtual Keys + + + {}} /> + + +
+
+ + + Top Models + + + `$${formatNumberWithCommas(value, 2)}`} + /> + + +
+
+
+ + + Spend by Provider + + +
+
+ `$${formatNumberWithCommas(value, 2)}`} + /> +
+
+ + + + Provider + Spend + + + + {spendByProvider.map((provider) => ( + + {provider.provider} + + + + + ))} + +
+
+
+ + +
+
+ + + +
+ + + All Up + + +
+
+

+ API Requests {valueFormatterNumbers(globalActivity.sum_api_requests)} +

+ - - - - - Top Virtual Keys - {}} /> - - - - - Top Models +
+
+

+ Tokens {valueFormatterNumbers(globalActivity.sum_total_tokens)} +

`$${formatNumberWithCommas(value, 2)}`} + categories={["total_tokens"]} /> - - - - - - Spend by Provider - <> - - - `$${formatNumberWithCommas(value, 2)}`} - /> - - - - - - Provider - Spend - - - - {spendByProvider.map((provider) => ( - - {provider.provider} - - - - - ))} - -
- - - - - - - - - - - All Up - - - +
+
+ + + + {globalActivityPerModel.map((globalActivity, index) => ( + + + {globalActivity.model} + + +
+
+

API Requests {valueFormatterNumbers(globalActivity.sum_api_requests)} - +

- - - +
+
+

Tokens {valueFormatterNumbers(globalActivity.sum_total_tokens)} - +

- - - +
+
+ + + ))} +
+ + + - <> - {globalActivityPerModel.map((globalActivity, index) => ( - - {globalActivity.model} - - - - API Requests {valueFormatterNumbers(globalActivity.sum_api_requests)} - - - - - - Tokens {valueFormatterNumbers(globalActivity.sum_total_tokens)} - - - - - - ))} - - - - - - - - - - - Total Spend Per Team - - - - Daily Spend Per Team + +
+
+ + + Total Spend Per Team + + + + + + + + Daily Spend Per Team + + = ({ accessToken, token, userRole, use yAxisWidth={80} stack={true} /> - - - - - - -

- Customers of your LLM API calls. Tracked when a `user` param is passed in your LLM calls{" "} - - docs here - -

- - - { - setDateValue(value); - updateEndUserData(value.from, value.to, null); - }} - /> - - - Select Key - - - + + +
+
+ - - - - - Customer - Spend - Total Events - - - - - {topUsers?.map((user: any, index: number) => ( - - {user.end_user} - - - - {user.total_count} - + +

+ Customers of your LLM API calls. Tracked when a `user` param is passed in your LLM calls{" "} + + docs here + +

+
+
+ { + setDateValue(value); + updateEndUserData(value.from, value.to, null); + }} + /> +
+
+

Select Key

+
- - - - - - { - setDateValue(value); - updateTagSpendData(value.from, value.to); - }} - /> - + + +
+
- - {premiumUser ? ( -
- setSelectedTags(value as string[])}> - setSelectedTags(["all-tags"])} - > - All Tags - - {allTagNames && - allTagNames - .filter((tag) => tag !== "all-tags") - .map((tag: any, index: number) => { - return ( - - {tag} - - ); - })} - -
- ) : ( -
- setSelectedTags(value as string[])}> - setSelectedTags(["all-tags"])} - > - All Tags - - {allTagNames && - allTagNames - .filter((tag) => tag !== "all-tags") - .map((tag: any, index: number) => { - return ( - - ✨ {tag} (Enterprise only Feature) - - ); - })} - -
- )} - - - - - - Spend Per Tag - + + +
+ + + + Customer + Spend + Total Events + + + + + {topUsers?.map((user: any, index: number) => ( + + {user.end_user} + + + + {user.total_count} + + ))} + +
+
+ + + + + +
+
+ { + setDateValue(value); + updateTagSpendData(value.from, value.to); + }} + /> +
+ +
+ selectedTags.includes(option.value))} + onValueChange={(options: TagOption[]) => setSelectedTags(options.map((option) => option.value))} + isItemEqualToValue={(a: TagOption, b: TagOption) => a.value === b.value} + itemToStringLabel={(option: TagOption) => option.label} + > + + + {(options: TagOption[]) => + options.map((option) => ( + + {option.label} + + )) + } + + + + + No tags found + + {(option: TagOption) => ( + + {option.label} + + )} + + + +
+
+
+
+ + + Spend Per Tag + + +

Get Started by Tracking cost per tag{" "} here - - - - - - - - - +

+ + + +
+
+ +
); }; From 169ba0e287e9993f0b2f1a5d229c26d3d64d39bb Mon Sep 17 00:00:00 2001 From: yuneng-jiang Date: Wed, 22 Jul 2026 17:01:41 -0700 Subject: [PATCH 19/41] refactor(ui): migrate transform-request to shadcn (#34303) * test(ui): characterise transform-request panel behaviour before migration * refactor(ui): migrate transform-request to shadcn * fix(ui): keep transform-request panels within the fixed-height content fold * fix(ui): let transform-request flow naturally so the shell scrolls instead of clipping * test(ui): select the copy button by its accessible name --- ui/litellm-dashboard/eslint-suppressions.json | 5 - .../TransformRequestPanel.test.tsx | 160 ++++++++++++++ .../TransformRequestPanel.tsx | 205 ++++++------------ 3 files changed, 226 insertions(+), 144 deletions(-) create mode 100644 ui/litellm-dashboard/src/app/(dashboard)/transform-request/TransformRequestPanel.test.tsx diff --git a/ui/litellm-dashboard/eslint-suppressions.json b/ui/litellm-dashboard/eslint-suppressions.json index b9b27e6cbb5..269348ec054 100644 --- a/ui/litellm-dashboard/eslint-suppressions.json +++ b/ui/litellm-dashboard/eslint-suppressions.json @@ -1098,11 +1098,6 @@ "count": 1 } }, - "src/app/(dashboard)/transform-request/TransformRequestPanel.tsx": { - "no-restricted-imports": { - "count": 1 - } - }, "src/app/(dashboard)/ui-theme/UIThemeSettings.tsx": { "no-restricted-imports": { "count": 1 diff --git a/ui/litellm-dashboard/src/app/(dashboard)/transform-request/TransformRequestPanel.test.tsx b/ui/litellm-dashboard/src/app/(dashboard)/transform-request/TransformRequestPanel.test.tsx new file mode 100644 index 00000000000..a0add153116 --- /dev/null +++ b/ui/litellm-dashboard/src/app/(dashboard)/transform-request/TransformRequestPanel.test.tsx @@ -0,0 +1,160 @@ +import { render, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import TransformRequestPanel from "./TransformRequestPanel"; +import { transformRequestCall } from "@/components/networking"; +import NotificationsManager from "@/components/molecules/notifications_manager"; + +vi.mock("@/components/networking", () => ({ + transformRequestCall: vi.fn(), +})); + +vi.mock("@/components/molecules/notifications_manager", () => ({ + default: { + success: vi.fn(), + info: vi.fn(), + fromBackend: vi.fn(), + }, +})); + +const transformRequestCallMock = vi.mocked(transformRequestCall); +const notify = vi.mocked(NotificationsManager); + +const ACCESS_TOKEN = "sk-test-token"; + +const getRequestTextarea = () => screen.getByPlaceholderText(/press cmd\/ctrl \+ enter to transform/i); + +const getTransformButton = () => screen.getByRole("button", { name: /transform/i }); + +const getCopyButton = () => screen.getByRole("button", { name: /copy to clipboard/i }); + +describe("TransformRequestPanel", () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + it("renders both panels, the prefilled request and the placeholder curl", () => { + render(); + + expect(screen.getByText("Original Request")).toBeInTheDocument(); + expect(screen.getByText("Transformed Request")).toBeInTheDocument(); + expect(screen.getByText(/sensitive headers are not shown/i)).toBeInTheDocument(); + + expect((getRequestTextarea() as HTMLTextAreaElement).value).toContain('"model": "openai/gpt-4o"'); + expect(screen.getByText(/https:\/\/api\.openai\.com\/v1\/chat\/completions/)).toBeInTheDocument(); + + expect(screen.getByRole("link", { name: /here/i })).toHaveAttribute( + "href", + "https://github.com/BerriAI/litellm/issues", + ); + }); + + it("sends the edited request body as a completion call and renders the returned curl", async () => { + const user = userEvent.setup(); + transformRequestCallMock.mockResolvedValue({ + raw_request_api_base: "https://api.anthropic.com/v1/messages", + raw_request_body: { model: "claude-opus-4-8", max_tokens: 42 }, + raw_request_headers: { "x-api-key": "redacted" }, + }); + + render(); + + const textarea = getRequestTextarea(); + await user.clear(textarea); + await user.type(textarea, '{{"model": "claude-opus-4-8"}'); + + await user.click(getTransformButton()); + + await waitFor(() => expect(transformRequestCallMock).toHaveBeenCalledTimes(1)); + expect(transformRequestCallMock).toHaveBeenCalledWith(ACCESS_TOKEN, { + call_type: "completion", + request_body: { model: "claude-opus-4-8" }, + }); + + const output = await screen.findByText(/api\.anthropic\.com\/v1\/messages/); + expect(output.textContent).toContain("curl -X POST"); + expect(output.textContent).toContain("-H 'x-api-key: redacted'"); + expect(output.textContent).toContain('"model": "claude-opus-4-8"'); + expect(output.textContent).toContain('"max_tokens": 42'); + expect(notify.success).toHaveBeenCalledWith("Request transformed successfully"); + }); + + it("transforms on Cmd/Ctrl + Enter without clicking the button", async () => { + const user = userEvent.setup(); + transformRequestCallMock.mockResolvedValue({ + raw_request_api_base: "https://api.openai.com/v1/chat/completions", + raw_request_body: { model: "gpt-4o" }, + raw_request_headers: {}, + }); + + render(); + + getRequestTextarea().focus(); + await user.keyboard("{Meta>}{Enter}{/Meta}"); + + await waitFor(() => expect(transformRequestCallMock).toHaveBeenCalledTimes(1)); + }); + + it("rejects invalid JSON without calling the backend", async () => { + const user = userEvent.setup(); + + render(); + + const textarea = getRequestTextarea(); + await user.clear(textarea); + await user.type(textarea, "not json"); + await user.click(getTransformButton()); + + await waitFor(() => expect(notify.fromBackend).toHaveBeenCalledWith("Invalid JSON in request body")); + expect(transformRequestCallMock).not.toHaveBeenCalled(); + }); + + it("does not call the backend when there is no access token", async () => { + const user = userEvent.setup(); + + render(); + + await user.click(getTransformButton()); + + await waitFor(() => expect(notify.fromBackend).toHaveBeenCalledWith("No access token found")); + expect(transformRequestCallMock).not.toHaveBeenCalled(); + }); + + it("reports a failed transform and leaves the placeholder curl in place", async () => { + const user = userEvent.setup(); + vi.spyOn(console, "error").mockImplementation(() => {}); + transformRequestCallMock.mockRejectedValue(new Error("boom")); + + render(); + + await user.click(getTransformButton()); + + await waitFor(() => expect(notify.fromBackend).toHaveBeenCalledWith("Failed to transform request")); + expect(screen.getByText(/https:\/\/api\.openai\.com\/v1\/chat\/completions/)).toBeInTheDocument(); + }); + + it("copies the transformed request to the clipboard", async () => { + const user = userEvent.setup(); + const writeText = vi.spyOn(navigator.clipboard, "writeText"); + transformRequestCallMock.mockResolvedValue({ + raw_request_api_base: "https://api.anthropic.com/v1/messages", + raw_request_body: { model: "claude-opus-4-8" }, + raw_request_headers: {}, + }); + + render(); + + await user.click(getTransformButton()); + await screen.findByText(/api\.anthropic\.com\/v1\/messages/); + + await user.click(getCopyButton()); + + expect(writeText).toHaveBeenCalledTimes(1); + expect(writeText.mock.calls[0]?.[0]).toContain("https://api.anthropic.com/v1/messages"); + expect(notify.success).toHaveBeenCalledWith("Copied to clipboard"); + }); +}); diff --git a/ui/litellm-dashboard/src/app/(dashboard)/transform-request/TransformRequestPanel.tsx b/ui/litellm-dashboard/src/app/(dashboard)/transform-request/TransformRequestPanel.tsx index 04d1701de3f..0c41547b9b7 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/transform-request/TransformRequestPanel.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/transform-request/TransformRequestPanel.tsx @@ -1,9 +1,12 @@ import React, { useState } from "react"; -import { Button } from "antd"; -import { CopyOutlined } from "@ant-design/icons"; -import { Title } from "@tremor/react"; +import { ArrowRight, Copy } from "lucide-react"; +import { Button } from "@/components/ui/button"; +import { Card, CardContent, CardDescription, CardFooter, CardHeader, CardTitle } from "@/components/ui/card"; +import { Textarea } from "@/components/ui/textarea"; +import { UiLoadingSpinner } from "@/components/ui/ui-loading-spinner"; import { transformRequestCall } from "@/components/networking"; import NotificationsManager from "@/components/molecules/notifications_manager"; + interface TransformRequestPanelProps { accessToken: string | null; } @@ -128,130 +131,50 @@ ${formattedBody} }; return ( -
- Playground -

See how LiteLLM transforms your request for the specified provider.

-
+
+

Playground

+

+ See how LiteLLM transforms your request for the specified provider. +

+
{/* Original Request Panel */} -
-
-

Original Request

-

- The request you would send to LiteLLM /chat/completions endpoint. -

-
+ + + Original Request + The request you would send to LiteLLM /chat/completions endpoint. + -