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";