From 3b62b6311cb5d67cd798e4cc55525440e8b60742 Mon Sep 17 00:00:00 2001 From: jesus-berri Date: Mon, 2 Nov 2026 09:14:10 -0800 Subject: [PATCH] fix(proxy): make /update/sso_settings a true PATCH and send only changed SSO fields from the UI (#49527) * fix(proxy): make /update/sso_settings a true PATCH and send only changed SSO fields from the UI * fix(proxy): mask SSO client secrets in the /update/sso_settings response * fix(proxy): keep SSO values that only live in the environment_variables row on a partial SSO PATCH The SSO PATCH strips every SSO env var from the environment_variables config row. Now that the UI only sends changed fields, an omitted value that lived only in that row was dropped from both tables and lost on restart. Carry those values into the SSO row before stripping them * test(ui): pin CLEARED_SSO_SETTINGS to exactly the backend SSOConfig fields With PATCH semantics an omitted key keeps its stored value, so a field missing from the Clear list would silently survive Clear SSO settings --- .../proxy_setting_endpoints.py | 88 ++++++--- .../test_proxy_setting_endpoints.py | 168 +++++++++++++++++- .../src/components/SSOModals.test.tsx | 23 +-- .../src/components/SSOModals.tsx | 134 +++++++------- .../Modals/AddSSOSettingsModal.tsx | 7 +- .../Modals/DeleteSSOSettingsModal.tsx | 26 +-- .../EditSSOSettingsModal.integration.test.tsx | 52 +++++- .../Modals/EditSSOSettingsModal.test.tsx | 2 + .../Modals/EditSSOSettingsModal.tsx | 7 +- .../AdminSettings/SSOSettings/utils.test-d.ts | 12 ++ .../AdminSettings/SSOSettings/utils.test.ts | 57 +++++- .../AdminSettings/SSOSettings/utils.ts | 43 +++++ 12 files changed, 458 insertions(+), 161 deletions(-) create mode 100644 ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/utils.test-d.ts diff --git a/litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py b/litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py index e5348684d78..6d31fa3f226 100644 --- a/litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py +++ b/litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py @@ -16,6 +16,7 @@ from typing import ( from urllib.parse import urlparse from fastapi import APIRouter, Body, Depends, File, HTTPException, UploadFile +from prisma.errors import PrismaError from pydantic import BaseModel, ConfigDict, JsonValue, TypeAdapter, ValidationError, create_model, field_validator from pydantic.fields import FieldInfo, PydanticUndefined from typing_extensions import NotRequired, ReadOnly, TypedDict @@ -26,6 +27,7 @@ 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.common_utils.encrypt_decrypt_utils import decrypt_value_helper from litellm.proxy.common_utils.validation_error_body import public_validation_errors from litellm.proxy.config_resolvers import FieldSource, SettingsStore, source_for from litellm.proxy.config_resolvers.settings_store import ConfigOwnedKeyError @@ -1247,6 +1249,37 @@ async def get_sso_settings(): } +_ENVIRONMENT_VARIABLES_PARAM: Final = TypeAdapter(dict[str, object]) + + +def _parse_environment_variables_param(param_value: str | Mapping[str, object] | None) -> Mapping[str, object]: + if isinstance(param_value, str): + return _ENVIRONMENT_VARIABLES_PARAM.validate_python(json.loads(param_value)) + return param_value or {} + + +async def _load_environment_variables_param( + repo: ConfigRepository, +) -> Mapping[str, object] | None | PrismaError | ValueError: + try: + entry: Final = await _config_param_db(repo).find_unique(where={"param_name": "environment_variables"}) + param_value: Final = None if entry is None else _parse_environment_variables_param(entry.param_value) + except (PrismaError, ValueError) as e: + return e + else: + return param_value + + +def _decrypted_sso_fields(environment_variables: Mapping[str, object]) -> Mapping[str, str | None]: + return MappingProxyType( + { + field_name: decrypt_value_helper(value=value, key=env_var, return_original_value=True) + for field_name, env_var in SSO_FIELD_ENV_VARS.items() + if isinstance(value := environment_variables.get(env_var), str) + } + ) + + @router.patch( "/update/sso_settings", tags=["SSO Settings"], @@ -1310,9 +1343,24 @@ async def update_sso_settings( if "general_settings" not in config: config["general_settings"] = {} - # Update environment variables in config and in memory - sso_data: Final = sso_config.model_dump() - for field_name, value in sso_data.items(): + # PATCH semantics: omitted fields keep their stored value, an explicit null clears it. SSO values that + # still live only in the environment_variables row are carried over, since that row is stripped below + environment_variables_param: Final = await _load_environment_variables_param( + ConfigRepository(prisma_client, use_writer=True) + ) + legacy_env_values: Final[Mapping[str, str | None]] = ( + _decrypted_sso_fields(environment_variables_param) + if isinstance(environment_variables_param, Mapping) + else MappingProxyType({}) + ) + stored_values: Final = {k: v for k, v in (before_sso_data or {}).items() if v is not None} + fallback_values: Final = {**legacy_env_values, **stored_values} + submitted: Final = sso_config.model_dump(include=sso_config.model_fields_set) + sso_data: Final = { + field_name: submitted[field_name] if field_name in submitted else fallback_values.get(field_name) + for field_name in SSOConfig.model_fields + } + for field_name, value in submitted.items(): if field_name in SSO_FIELD_ENV_VARS: env_var_name = SSO_FIELD_ENV_VARS[field_name] if value: @@ -1348,25 +1396,17 @@ async def update_sso_settings( ) ) - # Remove SSO-related env vars from config.environment_variables - try: - env_var_entry: Final = await _config_param_db(ConfigRepository(prisma_client, use_writer=True)).find_unique( - where={"param_name": "environment_variables"} + if isinstance(environment_variables_param, (PrismaError, ValueError)): + raise HTTPException( + status_code=500, + detail={"error": f"Error updating environment_variables: {environment_variables_param}"}, ) - # If no environment_variables entry exists, nothing to clean up - if env_var_entry is not None: - if env_var_entry.param_value is not None: - if isinstance(env_var_entry.param_value, str): - environment_variables: Mapping[str, object] = json.loads(env_var_entry.param_value) - else: - environment_variables = dict(env_var_entry.param_value) - else: - environment_variables = {} - + if environment_variables_param is not None: + try: env_vars_to_remove: Final = set(SSO_FIELD_ENV_VARS.values()) filtered_env_vars: Final = { - key: value for key, value in environment_variables.items() if key not in env_vars_to_remove + key: value for key, value in environment_variables_param.items() if key not in env_vars_to_remove } await _config_param_db(ConfigRepository(prisma_client)).update( @@ -1376,16 +1416,16 @@ async def update_sso_settings( }, ) await invalidate_config_param("environment_variables") - except Exception as e: - raise HTTPException( - status_code=500, - detail={"error": f"Error updating environment_variables: {e}"}, - ) + except Exception as e: + raise HTTPException( + status_code=500, + detail={"error": f"Error updating environment_variables: {e}"}, + ) return { "message": "SSO settings updated successfully", "status": "success", - "settings": sso_data, + "settings": mask_sensitive_keys(sso_data, set(SSO_SECRET_FIELDS)), } diff --git a/tests/unit/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py b/tests/unit/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py index ea39d5d59e2..e66050877df 100644 --- a/tests/unit/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py +++ b/tests/unit/proxy/ui_crud_endpoints/test_proxy_setting_endpoints.py @@ -689,16 +689,11 @@ class TestProxySettingEndpoints: # Verify settings were updated settings = data["settings"] assert settings["google_client_id"] == new_sso_settings["google_client_id"] - assert ( - settings["google_client_secret"] == new_sso_settings["google_client_secret"] - ) + assert settings["google_client_secret"] == "new_****************cret" assert ( settings["microsoft_client_id"] == new_sso_settings["microsoft_client_id"] ) - assert ( - settings["microsoft_client_secret"] - == new_sso_settings["microsoft_client_secret"] - ) + assert settings["microsoft_client_secret"] == "new_*******************cret" assert settings["proxy_base_url"] == new_sso_settings["proxy_base_url"] assert settings["user_email"] == new_sso_settings["user_email"] @@ -722,6 +717,163 @@ class TestProxySettingEndpoints: create_sso_settings = json.loads(create_data["sso_settings"]) assert create_sso_settings["google_client_id"] == "new_google_client_id" + def _patch_sso_settings(self, monkeypatch, stored, body, environment_variables=None): + """PATCH /update/sso_settings against a stored row; returns (response, saved row).""" + import json + from unittest.mock import AsyncMock, MagicMock + + monkeypatch.setattr("litellm.proxy.proxy_server.store_model_in_db", True) + mock_prisma = MagicMock() + stored_record = None + if stored is not None: + stored_record = MagicMock() + stored_record.sso_settings = stored + mock_prisma.db.litellm_ssoconfig.find_unique = AsyncMock( + return_value=stored_record + ) + mock_prisma.db.litellm_ssoconfig.upsert = AsyncMock() + env_var_entry = None + if environment_variables is not None: + env_var_entry = MagicMock() + env_var_entry.param_value = environment_variables + mock_prisma.writer_db.litellm_config.find_unique = AsyncMock(return_value=env_var_entry) + mock_prisma.db.litellm_config.update = AsyncMock() + monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", mock_prisma) + + from litellm.proxy.proxy_server import proxy_config + + monkeypatch.setattr( + proxy_config, "encrypt_env_variables", lambda environment_variables: environment_variables + ) + monkeypatch.setattr(proxy_config, "decrypt_db_variables", lambda variables: variables) + + response = client.patch("/update/sso_settings", json=body) + upsert_kwargs = mock_prisma.db.litellm_ssoconfig.upsert.call_args.kwargs + return response, json.loads(upsert_kwargs["data"]["update"]["sso_settings"]) + + _STORED_GENERIC_SSO = { + "generic_client_id": "stored-client-id", + "generic_client_secret": "stored-client-secret", + "generic_authorization_endpoint": "https://idp.example.com/authorize", + "user_email": "admin@example.com", + "role_mappings": { + "provider": "generic", + "group_claim": "groups", + "default_role": "internal_user", + "roles": {"proxy_admin": ["admins"]}, + }, + } + + def test_update_sso_settings_omitted_fields_keep_stored_values( + self, mock_proxy_config, mock_auth, monkeypatch + ): + monkeypatch.setenv("GENERIC_CLIENT_SECRET", "stored-client-secret") + new_role_mappings = { + "provider": "generic", + "group_claim": "roles", + "default_role": "internal_user", + "roles": {"proxy_admin": ["platform-admins"]}, + } + + response, saved = self._patch_sso_settings( + monkeypatch, self._STORED_GENERIC_SSO, {"role_mappings": new_role_mappings} + ) + + assert response.status_code == 200 + assert saved["generic_client_secret"] == "stored-client-secret" + assert saved["generic_client_id"] == "stored-client-id" + assert saved["user_email"] == "admin@example.com" + assert saved["role_mappings"]["group_claim"] == "roles" + assert os.environ["GENERIC_CLIENT_SECRET"] == "stored-client-secret" + + def test_update_sso_settings_null_clears_only_that_field( + self, mock_proxy_config, mock_auth, monkeypatch + ): + monkeypatch.setenv("GENERIC_CLIENT_SECRET", "stored-client-secret") + monkeypatch.setenv("GENERIC_CLIENT_ID", "stored-client-id") + + response, saved = self._patch_sso_settings( + monkeypatch, + self._STORED_GENERIC_SSO, + {"generic_client_secret": None, "role_mappings": None}, + ) + + assert response.status_code == 200 + assert saved["generic_client_secret"] is None + assert saved["role_mappings"] is None + assert saved["generic_client_id"] == "stored-client-id" + assert "GENERIC_CLIENT_SECRET" not in os.environ + assert os.environ["GENERIC_CLIENT_ID"] == "stored-client-id" + + def test_update_sso_settings_new_value_replaces_stored_value( + self, mock_proxy_config, mock_auth, monkeypatch + ): + response, saved = self._patch_sso_settings( + monkeypatch, self._STORED_GENERIC_SSO, {"generic_client_secret": "rotated-secret"} + ) + + assert response.status_code == 200 + assert saved["generic_client_secret"] == "rotated-secret" + assert saved["generic_client_id"] == "stored-client-id" + assert os.environ["GENERIC_CLIENT_SECRET"] == "rotated-secret" + + def test_update_sso_settings_omitted_field_leaves_env_only_value_alone( + self, mock_proxy_config, mock_auth, monkeypatch + ): + monkeypatch.setenv("GENERIC_CLIENT_SECRET", "env-only-secret") + + response, saved = self._patch_sso_settings( + monkeypatch, None, {"user_email": "admin@example.com"} + ) + + assert response.status_code == 200 + assert saved["user_email"] == "admin@example.com" + assert saved["generic_client_secret"] is None + assert os.environ["GENERIC_CLIENT_SECRET"] == "env-only-secret" + + def test_update_sso_settings_response_masks_stored_secrets( + self, mock_proxy_config, mock_auth, monkeypatch + ): + response, saved = self._patch_sso_settings( + monkeypatch, self._STORED_GENERIC_SSO, {} + ) + + assert response.status_code == 200 + assert saved["generic_client_secret"] == "stored-client-secret" + returned = response.json()["settings"] + assert returned["generic_client_id"] == "stored-client-id" + assert returned["generic_client_secret"] != "stored-client-secret" + assert "stored-client-secret" not in response.text + + def test_update_sso_settings_moves_omitted_env_row_values_into_sso_row( + self, mock_proxy_config, mock_auth, monkeypatch + ): + import json + + environment_variables = { + "MICROSOFT_TENANT": "env-row-tenant", + "MICROSOFT_CLIENT_ID": "env-row-client-id", + "GENERIC_CLIENT_ID": "env-row-generic-id", + "LANGFUSE_PUBLIC_KEY": "unrelated", + } + + response, saved = self._patch_sso_settings( + monkeypatch, + self._STORED_GENERIC_SSO, + {"user_email": "new@example.com", "microsoft_client_id": None}, + environment_variables=environment_variables, + ) + + from litellm.proxy.proxy_server import prisma_client + + assert response.status_code == 200 + assert saved["microsoft_tenant"] == "env-row-tenant" + assert saved["microsoft_client_id"] is None + assert saved["generic_client_id"] == "stored-client-id" + assert saved["user_email"] == "new@example.com" + env_row_update = prisma_client.db.litellm_config.update.call_args.kwargs + assert json.loads(env_row_update["data"]["param_value"]) == {"LANGFUSE_PUBLIC_KEY": "unrelated"} + def test_update_sso_settings_maps_saml_fields_to_env_vars( self, mock_proxy_config, mock_auth, monkeypatch ): @@ -804,7 +956,7 @@ class TestProxySettingEndpoints: mock_prisma.db.litellm_ssoconfig.find_unique = AsyncMock(return_value=None) mock_prisma.db.litellm_ssoconfig.upsert = AsyncMock() mock_prisma.db.litellm_config = MagicMock() - mock_prisma.db.litellm_config.find_unique = AsyncMock( + mock_prisma.writer_db.litellm_config.find_unique = AsyncMock( side_effect=ValueError("cleanup failed") ) mock_prisma.db.litellm_config.update = AsyncMock() diff --git a/ui/litellm-dashboard/src/components/SSOModals.test.tsx b/ui/litellm-dashboard/src/components/SSOModals.test.tsx index bee5f13c626..d78de8adaef 100644 --- a/ui/litellm-dashboard/src/components/SSOModals.test.tsx +++ b/ui/litellm-dashboard/src/components/SSOModals.test.tsx @@ -19,6 +19,7 @@ vi.mock("./shared/errorUtils", () => ({ import { toast } from "@/lib/toast"; import { getSSOSettings, updateSSOSettings } from "./networking"; +import { CLEARED_SSO_SETTINGS } from "./Settings/AdminSettings/SSOSettings/utils"; describe("SSOModals", () => { it("should render the SSOModals component", () => { @@ -515,27 +516,7 @@ describe("SSOModals", () => { // Verify the clear API was called with null values await waitFor(() => { - expect(updateSSOSettings).toHaveBeenCalledWith("test-token", { - google_client_id: null, - google_client_secret: null, - microsoft_client_id: null, - microsoft_client_secret: null, - microsoft_tenant: null, - generic_client_id: null, - generic_client_secret: null, - generic_authorization_endpoint: null, - generic_token_endpoint: null, - generic_userinfo_endpoint: null, - saml_idp_metadata_url: null, - saml_idp_metadata_xml: null, - saml_sp_entity_id: null, - saml_allow_unsolicited: null, - generic_scope: null, - proxy_base_url: null, - user_email: null, - sso_provider: null, - role_mappings: null, - }); + expect(updateSSOSettings).toHaveBeenCalledWith("test-token", CLEARED_SSO_SETTINGS); }); expect(toast.success).toHaveBeenCalledWith("SSO settings cleared successfully"); diff --git a/ui/litellm-dashboard/src/components/SSOModals.tsx b/ui/litellm-dashboard/src/components/SSOModals.tsx index 609c0efcd99..5c4825698a0 100644 --- a/ui/litellm-dashboard/src/components/SSOModals.tsx +++ b/ui/litellm-dashboard/src/components/SSOModals.tsx @@ -1,6 +1,7 @@ import React, { useEffect, useState } from "react"; import { FormProvider, useWatch, type UseFormReturn } from "react-hook-form"; import { getSSOSettings, updateSSOSettings } from "./networking"; +import { CLEARED_SSO_SETTINGS, diffSSOSettingsPayload } from "./Settings/AdminSettings/SSOSettings/utils"; import { toast } from "@/lib/toast"; import { parseErrorMessage } from "./shared/errorUtils"; import { Button } from "@/components/ui/button"; @@ -32,6 +33,60 @@ interface SSOModalsProps { ssoConfigured?: boolean; // Add optional prop to indicate if SSO is configured } +const buildLegacySSOPayload = (formValues: SSOSettingsFormValues): Record => { + const { + proxy_admin_teams, + admin_viewer_teams, + internal_user_teams, + internal_viewer_teams, + default_role, + group_claim, + use_role_mappings, + ...rest + } = formValues; + + const payload: Record = { + ...rest, + }; + + if (typeof payload.saml_allow_unsolicited === "boolean") { + payload.saml_allow_unsolicited = payload.saml_allow_unsolicited ? "true" : "false"; + } + + // Add role mappings if use_role_mappings is checked + if (use_role_mappings) { + // Helper function to split comma-separated string into array + const splitTeams = (teams: string | undefined): string[] => { + if (!teams || teams.trim() === "") return []; + return teams + .split(",") + .map((team) => team.trim()) + .filter((team) => team.length > 0); + }; + + // Map default role display values to backend values + const defaultRoleMapping: Record = { + internal_user_viewer: "internal_user_viewer", + internal_user: "internal_user", + proxy_admin_viewer: "proxy_admin_viewer", + proxy_admin: "proxy_admin", + }; + + payload.role_mappings = { + provider: "generic", + group_claim, + default_role: (default_role ? defaultRoleMapping[default_role] : undefined) || "internal_user", + roles: { + proxy_admin: splitTeams(proxy_admin_teams), + proxy_admin_viewer: splitTeams(admin_viewer_teams), + internal_user: splitTeams(internal_user_teams), + internal_user_viewer: splitTeams(internal_viewer_teams), + }, + }; + } + return payload; +}; + const detectSSOProvider = (values: Record): string | null => { if (values.google_client_id) return "google"; if (values.microsoft_client_id) return "microsoft"; @@ -134,56 +189,10 @@ const SSOModals: React.FC = ({ } try { - const { - proxy_admin_teams, - admin_viewer_teams, - internal_user_teams, - internal_viewer_teams, - default_role, - group_claim, - use_role_mappings, - ...rest - } = formValues; - - const payload: Record = { - ...rest, - }; - - if (typeof payload.saml_allow_unsolicited === "boolean") { - payload.saml_allow_unsolicited = payload.saml_allow_unsolicited ? "true" : "false"; - } - - // Add role mappings if use_role_mappings is checked - if (use_role_mappings) { - // Helper function to split comma-separated string into array - const splitTeams = (teams: string | undefined): string[] => { - if (!teams || teams.trim() === "") return []; - return teams - .split(",") - .map((team) => team.trim()) - .filter((team) => team.length > 0); - }; - - // Map default role display values to backend values - const defaultRoleMapping: Record = { - internal_user_viewer: "internal_user_viewer", - internal_user: "internal_user", - proxy_admin_viewer: "proxy_admin_viewer", - proxy_admin: "proxy_admin", - }; - - payload.role_mappings = { - provider: "generic", - group_claim, - default_role: (default_role ? defaultRoleMapping[default_role] : undefined) || "internal_user", - roles: { - proxy_admin: splitTeams(proxy_admin_teams), - proxy_admin_viewer: splitTeams(admin_viewer_teams), - internal_user: splitTeams(internal_user_teams), - internal_user_viewer: splitTeams(internal_viewer_teams), - }, - }; - } + const payload = diffSSOSettingsPayload( + buildLegacySSOPayload({ ...emptySSOSettingsFormValues, ...form.formState.defaultValues }), + buildLegacySSOPayload(formValues), + ); // Save SSO settings using the new API await updateSSOSettings(accessToken, payload); @@ -203,30 +212,7 @@ const SSOModals: React.FC = ({ } try { - // Clear all SSO settings - const clearSettings = { - google_client_id: null, - google_client_secret: null, - microsoft_client_id: null, - microsoft_client_secret: null, - microsoft_tenant: null, - generic_client_id: null, - generic_client_secret: null, - generic_authorization_endpoint: null, - generic_token_endpoint: null, - generic_userinfo_endpoint: null, - saml_idp_metadata_url: null, - saml_idp_metadata_xml: null, - saml_sp_entity_id: null, - saml_allow_unsolicited: null, - generic_scope: null, - proxy_base_url: null, - user_email: null, - sso_provider: null, - role_mappings: null, - }; - - await updateSSOSettings(accessToken, clearSettings); + await updateSSOSettings(accessToken, CLEARED_SSO_SETTINGS); // Clear the form form.reset(emptySSOSettingsFormValues); diff --git a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/AddSSOSettingsModal.tsx b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/AddSSOSettingsModal.tsx index 7fa16515bf5..a16598653cf 100644 --- a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/AddSSOSettingsModal.tsx +++ b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/AddSSOSettingsModal.tsx @@ -12,7 +12,7 @@ import BaseSSOSettingsForm, { import { Button } from "@/components/ui/button"; import { UiLoadingSpinner } from "@/components/ui/ui-loading-spinner"; import { useEditSSOSettings } from "@/app/(dashboard)/hooks/sso/useEditSSOSettings"; -import { processSSOSettingsPayload } from "../utils"; +import { diffSSOSettingsPayload, processSSOSettingsPayload } from "../utils"; import { Dialog, DialogContent, DialogFooter, DialogHeader, DialogTitle } from "@/components/ui/dialog"; interface AddSSOSettingsModalProps { @@ -26,7 +26,10 @@ const AddSSOSettingsModal: React.FC = ({ isVisible, on const { mutateAsync, isPending } = useEditSSOSettings(); const handleFormSubmit = async (formValues: SSOSettingsFormValues) => { - const payload = processSSOSettingsPayload(formValues); + const payload = diffSSOSettingsPayload( + processSSOSettingsPayload(emptySSOSettingsFormValues), + processSSOSettingsPayload(formValues), + ); await mutateAsync(payload, { onSuccess: () => { diff --git a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/DeleteSSOSettingsModal.tsx b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/DeleteSSOSettingsModal.tsx index 053dac499bd..416ec4d2432 100644 --- a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/DeleteSSOSettingsModal.tsx +++ b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/DeleteSSOSettingsModal.tsx @@ -4,7 +4,7 @@ import React from "react"; import DeleteResourceModal from "../../../../common_components/DeleteResourceModal"; import { toast } from "@/lib/toast"; import { parseErrorMessage } from "../../../../shared/errorUtils"; -import { detectSSOProvider } from "../utils"; +import { CLEARED_SSO_SETTINGS, detectSSOProvider } from "../utils"; interface DeleteSSOSettingsModalProps { isVisible: boolean; @@ -18,29 +18,7 @@ const DeleteSSOSettingsModal: React.FC = ({ isVisib // Handle clearing SSO settings const handleClearSSO = async () => { - const clearSettings = { - google_client_id: null, - google_client_secret: null, - microsoft_client_id: null, - microsoft_client_secret: null, - microsoft_tenant: null, - generic_client_id: null, - generic_client_secret: null, - generic_authorization_endpoint: null, - generic_token_endpoint: null, - generic_userinfo_endpoint: null, - saml_idp_metadata_url: null, - saml_idp_metadata_xml: null, - saml_sp_entity_id: null, - saml_allow_unsolicited: null, - proxy_base_url: null, - user_email: null, - sso_provider: null, - role_mappings: null, - team_mappings: null, - }; - - await editSSOSettings(clearSettings, { + await editSSOSettings(CLEARED_SSO_SETTINGS, { onSuccess: () => { toast.success("SSO settings cleared successfully"); onCancel(); diff --git a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/EditSSOSettingsModal.integration.test.tsx b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/EditSSOSettingsModal.integration.test.tsx index 14bfe5d6ac9..cddcffc6926 100644 --- a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/EditSSOSettingsModal.integration.test.tsx +++ b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/EditSSOSettingsModal.integration.test.tsx @@ -49,7 +49,7 @@ describe("EditSSOSettingsModal (real form tree)", () => { expect(mutateAsync).not.toHaveBeenCalled(); }); - it("sends only the mounted fields when the form is valid", async () => { + it("sends only the changed field and leaves the masked secret out", async () => { const user = userEvent.setup({ pointerEventsCheck: 0 }); renderWithProviders(); @@ -58,12 +58,54 @@ describe("EditSSOSettingsModal (real form tree)", () => { await user.click(saveButton()); await waitFor(() => expect(mutateAsync).toHaveBeenCalledTimes(1)); - expect(mutateAsync.mock.calls[0][0]).toEqual({ - sso_provider: "google", - google_client_id: "rotated-client-id", - google_client_secret: "stored-client-secret", + expect(mutateAsync.mock.calls[0][0]).toEqual({ google_client_id: "rotated-client-id" }); + }); + + it("sends an empty patch when nothing changed", async () => { + const user = userEvent.setup({ pointerEventsCheck: 0 }); + renderWithProviders(); + + await screen.findByLabelText("Google Client ID"); + await user.click(saveButton()); + + await waitFor(() => expect(mutateAsync).toHaveBeenCalledTimes(1)); + expect(mutateAsync.mock.calls[0][0]).toEqual({}); + }); + + it("sends a new secret when the admin types one", async () => { + const user = userEvent.setup({ pointerEventsCheck: 0 }); + renderWithProviders(); + + fireEvent.change(await screen.findByLabelText("Google Client Secret"), { target: { value: "new-secret" } }); + await user.click(saveButton()); + + await waitFor(() => expect(mutateAsync).toHaveBeenCalledTimes(1)); + expect(mutateAsync.mock.calls[0][0]).toEqual({ google_client_secret: "new-secret" }); + }); + + it("sends role_mappings null when the admin unchecks role mappings", async () => { + seed({ + generic_client_id: "stored-client-id", + generic_client_secret: "kc-s****6e0d", + generic_authorization_endpoint: "https://idp.example.com/authorize", + generic_token_endpoint: "https://idp.example.com/token", + generic_userinfo_endpoint: "https://idp.example.com/userinfo", user_email: "admin@example.com", proxy_base_url: "https://gateway.example.com", + role_mappings: { + provider: "generic", + group_claim: "groups", + default_role: "internal_user", + roles: { proxy_admin: ["admins"], proxy_admin_viewer: [], internal_user: [], internal_user_viewer: [] }, + }, }); + const user = userEvent.setup({ pointerEventsCheck: 0 }); + renderWithProviders(); + + await user.click((await screen.findAllByLabelText("Use Role Mappings"))[0]); + await user.click(saveButton()); + + await waitFor(() => expect(mutateAsync).toHaveBeenCalledTimes(1)); + expect(mutateAsync.mock.calls[0][0]).toEqual({ role_mappings: null }); }); }); diff --git a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/EditSSOSettingsModal.test.tsx b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/EditSSOSettingsModal.test.tsx index ef78e3827d8..132800faf9f 100644 --- a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/EditSSOSettingsModal.test.tsx +++ b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/EditSSOSettingsModal.test.tsx @@ -293,6 +293,8 @@ describe("EditSSOSettingsModal", () => { useEditSSOSettings: { mutateAsync: mockMutateAsync, isPending: false }, }); + (processSSOSettingsPayload as any).mockReturnValueOnce({}).mockReturnValueOnce(processedPayload); + renderComponent(); fireEvent.click(screen.getByTestId(TEST_IDS.TRIGGER_FORM_SUBMIT)); diff --git a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/EditSSOSettingsModal.tsx b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/EditSSOSettingsModal.tsx index 58c03eda979..706597e305b 100644 --- a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/EditSSOSettingsModal.tsx +++ b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/EditSSOSettingsModal.tsx @@ -11,7 +11,7 @@ import { Button } from "@/components/ui/button"; import { UiLoadingSpinner } from "@/components/ui/ui-loading-spinner"; import { toast } from "@/lib/toast"; import { parseErrorMessage } from "@/components/shared/errorUtils"; -import { detectSSOProvider, processSSOSettingsPayload } from "../utils"; +import { detectSSOProvider, diffSSOSettingsPayload, processSSOSettingsPayload } from "../utils"; import { useSSOSettings, type SSOSettingsValues } from "@/app/(dashboard)/hooks/sso/useSSOSettings"; import { useEditSSOSettings } from "@/app/(dashboard)/hooks/sso/useEditSSOSettings"; import { Dialog, DialogContent, DialogFooter, DialogHeader, DialogTitle } from "@/components/ui/dialog"; @@ -86,7 +86,10 @@ const EditSSOSettingsModal: React.FC = ({ isVisible, const handleFormSubmit = async (formValues: SSOSettingsFormValues) => { try { - const payload = processSSOSettingsPayload(formValues); + const payload = diffSSOSettingsPayload( + processSSOSettingsPayload(seededValues), + processSSOSettingsPayload(formValues), + ); await mutateAsync(payload, { onSuccess: () => { diff --git a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/utils.test-d.ts b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/utils.test-d.ts new file mode 100644 index 00000000000..c5f50e391ac --- /dev/null +++ b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/utils.test-d.ts @@ -0,0 +1,12 @@ +import { describe, expectTypeOf, test } from "vitest"; + +import type { components } from "@/lib/http/schema"; +import { CLEARED_SSO_SETTINGS } from "./utils"; + +describe("SSO settings utils types", () => { + test("clearing SSO settings sends null for exactly the fields the backend declares", () => { + expectTypeOf(CLEARED_SSO_SETTINGS).toEqualTypeOf<{ + readonly [K in keyof components["schemas"]["SSOConfig"]]-?: null; + }>(); + }); +}); diff --git a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/utils.test.ts b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/utils.test.ts index b280f5e92ec..b0d519afcbc 100644 --- a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/utils.test.ts +++ b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/utils.test.ts @@ -1,4 +1,4 @@ -import { detectSSOProvider, processSSOSettingsPayload } from "./utils"; +import { detectSSOProvider, diffSSOSettingsPayload, processSSOSettingsPayload } from "./utils"; import { describe, it, expect } from "vitest"; import type { SSOSettingsValues } from "@/app/(dashboard)/hooks/sso/useSSOSettings"; @@ -452,3 +452,58 @@ describe("processSSOSettingsPayload with SAML", () => { ).toBe("false"); }); }); + +describe("diffSSOSettingsPayload", () => { + const stored = { + sso_provider: "generic", + generic_client_id: "client-id", + generic_client_secret: "kc-s****6e0d", + user_email: "admin@example.com", + role_mappings: { group_claim: "groups", roles: { proxy_admin: ["admins"] } }, + }; + + it("returns an empty patch when nothing changed", () => { + expect(diffSSOSettingsPayload(stored, { ...stored })).toEqual({}); + }); + + it("leaves an untouched masked secret out of the patch", () => { + expect(diffSSOSettingsPayload(stored, { ...stored, user_email: "new@example.com" })).toEqual({ + user_email: "new@example.com", + }); + }); + + it("sends null for a field that was emptied", () => { + expect(diffSSOSettingsPayload(stored, { ...stored, generic_client_secret: "" })).toEqual({ + generic_client_secret: null, + }); + }); + + it("sends null for fields dropped from the payload, like another provider's or disabled mappings", () => { + const { generic_client_id, generic_client_secret, role_mappings, ...rest } = stored; + expect(diffSSOSettingsPayload(stored, { ...rest, sso_provider: "google", google_client_id: "g-id" })).toEqual({ + sso_provider: "google", + google_client_id: "g-id", + generic_client_id: null, + generic_client_secret: null, + role_mappings: null, + }); + }); + + it("sends the whole mapping object when one part of it changed", () => { + const role_mappings = { group_claim: "roles", roles: { proxy_admin: ["admins"] } }; + expect(diffSSOSettingsPayload(stored, { ...stored, role_mappings })).toEqual({ role_mappings }); + }); + + it("treats empty values on both sides as unchanged", () => { + expect(diffSSOSettingsPayload({ generic_scope: undefined, proxy_base_url: "" }, { proxy_base_url: null })).toEqual( + {}, + ); + }); + + it("treats an unchecked saml_allow_unsolicited as unset, but sends false when it was true", () => { + expect(diffSSOSettingsPayload({ saml_allow_unsolicited: "false" }, {})).toEqual({}); + expect(diffSSOSettingsPayload({ saml_allow_unsolicited: "true" }, { saml_allow_unsolicited: "false" })).toEqual({ + saml_allow_unsolicited: "false", + }); + }); +}); diff --git a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/utils.ts b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/utils.ts index 768fc7e8e0e..93af987adde 100644 --- a/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/utils.ts +++ b/ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/utils.ts @@ -71,6 +71,49 @@ export const processSSOSettingsPayload = (formValues: Record): Reco return payload; }; +const isEmptySSOValue = (value: unknown): boolean => value === undefined || value === null || value === ""; + +// An unchecked saml_allow_unsolicited is the same as never having set it +const comparableSSOValue = (key: string, value: unknown): unknown => + isEmptySSOValue(value) || (key === "saml_allow_unsolicited" && value === "false") ? undefined : value; + +// /update/sso_settings is a PATCH: send only the fields that changed, with null for the ones that were emptied +export const diffSSOSettingsPayload = ( + before: Record, + after: Record, +): Record => + Object.fromEntries( + [...new Set([...Object.keys(before), ...Object.keys(after)])] + .filter( + (key) => + JSON.stringify(comparableSSOValue(key, before[key])) !== JSON.stringify(comparableSSOValue(key, after[key])), + ) + .map((key) => [key, isEmptySSOValue(after[key]) ? null : after[key]]), + ); + +export const CLEARED_SSO_SETTINGS = { + google_client_id: null, + google_client_secret: null, + microsoft_client_id: null, + microsoft_client_secret: null, + microsoft_tenant: null, + generic_client_id: null, + generic_client_secret: null, + generic_authorization_endpoint: null, + generic_token_endpoint: null, + generic_userinfo_endpoint: null, + generic_scope: null, + saml_idp_metadata_url: null, + saml_idp_metadata_xml: null, + saml_sp_entity_id: null, + saml_allow_unsolicited: null, + proxy_base_url: null, + user_email: null, + ui_access_mode: null, + role_mappings: null, + team_mappings: null, +} as const; + // Determine the SSO provider based on the configuration export const detectSSOProvider = (values: SSOSettingsValues): string | null => { if (values.google_client_id) return "google";