From 0b39e5faf33685661ea2c0a5959cb3c4946bdfb4 Mon Sep 17 00:00:00 2001 From: jesus-berri Date: Tue, 3 Nov 2026 17:13:26 -0800 Subject: [PATCH] fix(ui): send only shown SSO fields when editing, and keep env-row SSO values behind a blank stored value (#49973) * fix(ui): send only shown SSO fields when editing, and keep env-row SSO values behind a blank stored value * fix(ui): clear every competing SSO provider when the admin switches provider Only the detected provider's fields were nulled on a switch, so a second stored provider (e.g. Microsoft behind Google) kept outranking the new one at sign-in --- .../proxy_setting_endpoints.py | 2 +- .../test_proxy_setting_endpoints.py | 19 ++++ .../src/components/SSOModals.tsx | 11 ++- .../Modals/BaseSSOSettingsForm.test.tsx | 93 +++++++++++++++++++ .../Modals/BaseSSOSettingsForm.tsx | 16 ++++ .../Modals/EditSSOSettingsModal.tsx | 8 +- 6 files changed, 139 insertions(+), 10 deletions(-) diff --git a/litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py b/litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py index 30df407e853..e68008135aa 100644 --- a/litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py +++ b/litellm/proxy/ui_crud_endpoints/proxy_setting_endpoints.py @@ -1360,7 +1360,7 @@ async def update_sso_settings( 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} + stored_values: Final = {k: v for k, v in (before_sso_data or {}).items() if v is not None and v != ""} fallback_values: Final = {**legacy_env_values, **stored_values} submitted: Final = sso_config.model_dump(include=sso_config.model_fields_set) sso_data: Final = { 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 4087c3e6413..77d0132ec72 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 @@ -895,6 +895,25 @@ class TestProxySettingEndpoints: 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_blank_stored_value_keeps_env_row_secret( + self, mock_proxy_config, mock_auth, monkeypatch + ): + import json + + response, saved = self._patch_sso_settings( + monkeypatch, + {**self._STORED_GENERIC_SSO, "generic_client_secret": ""}, + {"user_email": "new@example.com"}, + environment_variables={"GENERIC_CLIENT_SECRET": "env-row-secret", "LANGFUSE_PUBLIC_KEY": "unrelated"}, + ) + + from litellm.proxy.proxy_server import prisma_client + + assert response.status_code == 200, response.text + assert saved["generic_client_secret"] == "env-row-secret", saved + 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 ): diff --git a/ui/litellm-dashboard/src/components/SSOModals.tsx b/ui/litellm-dashboard/src/components/SSOModals.tsx index 4c48164ddb5..7415077d9df 100644 --- a/ui/litellm-dashboard/src/components/SSOModals.tsx +++ b/ui/litellm-dashboard/src/components/SSOModals.tsx @@ -2,7 +2,7 @@ import React, { useEffect, useState } from "react"; import { FormProvider, useWatch, type UseFormReturn } from "react-hook-form"; import { useTranslation } from "react-i18next"; import { getSSOSettings, updateSSOSettings } from "./networking"; -import { CLEARED_SSO_SETTINGS, diffSSOSettingsPayload } from "./Settings/AdminSettings/SSOSettings/utils"; +import { CLEARED_SSO_SETTINGS } from "./Settings/AdminSettings/SSOSettings/utils"; import { toast } from "@/lib/toast"; import { parseErrorMessage } from "./shared/errorUtils"; import { Button } from "@/components/ui/button"; @@ -14,6 +14,7 @@ import { ProxyBaseUrlField, RoleMappingTeamFields, SSOProviderSelectField, + buildSSOSettingsPatch, emptySSOSettingsFormValues, renderProviderFields, submitMountedSSOValues, @@ -191,9 +192,11 @@ const SSOModals: React.FC = ({ } try { - const payload = diffSSOSettingsPayload( - buildLegacySSOPayload({ ...emptySSOSettingsFormValues, ...form.formState.defaultValues }), - buildLegacySSOPayload(formValues), + const payload = buildSSOSettingsPatch( + { ...emptySSOSettingsFormValues, ...form.formState.defaultValues }, + formValues, + "admin-panel", + buildLegacySSOPayload, ); // Save SSO settings using the new API 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 eddc5716305..ed1f879e400 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 @@ -3,13 +3,16 @@ import userEvent from "@testing-library/user-event"; import { renderWithProviders } from "../../../../../../tests/test-utils"; import { afterEach, describe, expect, it, vi } from "vitest"; import BaseSSOSettingsForm, { + buildSSOSettingsPatch, emptySSOSettingsFormValues, + pickMountedSSOValues, renderProviderFields, ssoProviderConfigs, submitMountedSSOValues, useSSOSettingsForm, } from "./BaseSSOSettingsForm"; import { expectControlBesideLabel } from "../../../../../../tests/fieldOrientation"; +import { processSSOSettingsPayload } from "../utils"; const user = () => userEvent.setup({ pointerEventsCheck: 0 }); @@ -436,3 +439,93 @@ describe("renderProviderFields", () => { expect(await screen.findByLabelText("Default Role")).toHaveTextContent("Internal User"); }); }); + +describe("buildSSOSettingsPatch", () => { + const seeded = { + ...emptySSOSettingsFormValues, + sso_provider: "generic", + generic_client_id: "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", + generic_scope: "openid email profile groups", + saml_idp_metadata_url: "https://idp.example.com/saml/metadata", + saml_sp_entity_id: "litellm-sp", + microsoft_client_id: "ms-client-id", + microsoft_tenant: "ms-tenant", + user_email: "admin@example.com", + proxy_base_url: "https://gateway.example.com", + use_role_mappings: true, + group_claim: "groups", + default_role: "internal_user", + proxy_admin_teams: "admins", + }; + const submit = (values: typeof seeded) => pickMountedSSOValues(values, "sso-settings"); + const patch = (submitted: typeof seeded) => + buildSSOSettingsPatch(seeded, submit(submitted), "sso-settings", processSSOSettingsPayload); + + it("sends only the edited field, leaving SAML and another provider's settings the form never showed", () => { + expect(patch({ ...seeded, user_email: "new@example.com" })).toEqual({ user_email: "new@example.com" }); + }); + + it("sends null for a shown field the admin emptied", () => { + expect(patch({ ...seeded, generic_scope: "" })).toEqual({ generic_scope: null }); + }); + + it("clears role mappings when the admin unchecks them", () => { + expect(patch({ ...seeded, use_role_mappings: false })).toEqual({ role_mappings: null }); + }); + + it("clears the previous provider's credentials when the admin switches provider", () => { + const google = { sso_provider: "google", google_client_id: "g-id", google_client_secret: "g-secret" }; + const clearedGeneric = { + generic_client_id: null, + generic_client_secret: null, + generic_authorization_endpoint: null, + generic_token_endpoint: null, + generic_userinfo_endpoint: null, + generic_scope: null, + }; + const clearedOthers = { + microsoft_client_id: null, + microsoft_tenant: null, + saml_idp_metadata_url: null, + saml_sp_entity_id: null, + }; + expect(patch({ ...seeded, ...google })).toEqual({ ...google, ...clearedGeneric, ...clearedOthers }); + }); + + it("clears a stored provider that would outrank the new one at sign-in, not only the one the form showed", () => { + const googleAndMicrosoft = { + ...emptySSOSettingsFormValues, + sso_provider: "google", + google_client_id: "g-id", + microsoft_client_id: "ms-id", + user_email: "admin@example.com", + proxy_base_url: "https://gateway.example.com", + }; + const generic = { + ...googleAndMicrosoft, + sso_provider: "generic", + generic_client_id: "client-id", + generic_client_secret: "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", + }; + const result = buildSSOSettingsPatch( + googleAndMicrosoft, + pickMountedSSOValues(generic, "sso-settings"), + "sso-settings", + processSSOSettingsPayload, + ); + expect(result).toMatchObject({ google_client_id: null, microsoft_client_id: null, generic_client_id: "client-id" }); + }); + + it("keeps the shared generic credentials when the admin switches between okta and generic", () => { + const result = patch({ ...seeded, sso_provider: "okta" }); + expect(result.sso_provider).toBe("okta"); + expect(Object.keys(result).filter((key) => key.startsWith("generic_"))).toEqual([]); + }); +}); 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 f3f1332a093..804c6d192f6 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 @@ -6,6 +6,7 @@ import { FormProvider, useFormContext, useWatch, type UseFormReturn } from "reac import { useTranslation } from "react-i18next"; import { z } from "zod"; import { ssoProviderLogoMap, ssoProviderDisplayNames } from "../constants"; +import { diffSSOSettingsPayload } from "../utils"; import { Logo } from "@/components/molecules/logo/Logo"; import { FieldGroup } from "@/components/ui/field"; import { FormField } from "@/components/shared/form/FormField"; @@ -222,6 +223,21 @@ export const pickMountedSSOValues = (values: SSOSettingsFormValues, variant: SSO mountedSSOFieldNames(values, variant).map((name) => [name, values[name as keyof SSOSettingsFormValues]]), ); +export const buildSSOSettingsPatch = ( + seeded: SSOSettingsFormValues, + submitted: SSOSettingsFormValues, + variant: SSOFormVariant, + toPayload: (values: SSOSettingsFormValues) => Record, +): Record => { + const competingProviderFields = + seeded.sso_provider === submitted.sso_provider ? [] : Object.keys(ssoProviderConfigs).flatMap(providerFieldNames); + const editedFieldNames = new Set([...mountedSSOFieldNames(submitted, variant), ...competingProviderFields]); + const seededEditedValues: SSOSettingsFormValues = Object.fromEntries( + Object.entries(seeded).filter(([name]) => editedFieldNames.has(name)), + ); + return diffSSOSettingsPayload(toPayload(seededEditedValues), toPayload(submitted)); +}; + export const submitMountedSSOValues = ( form: UseFormReturn, 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 e775bb48dff..5151be7221e 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 @@ -3,6 +3,7 @@ import React, { useMemo } from "react"; import { useTranslation } from "react-i18next"; import BaseSSOSettingsForm, { + buildSSOSettingsPatch, emptySSOSettingsFormValues, submitMountedSSOValues, useSSOSettingsForm, @@ -12,7 +13,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, diffSSOSettingsPayload, processSSOSettingsPayload } from "../utils"; +import { detectSSOProvider, 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"; @@ -88,10 +89,7 @@ const EditSSOSettingsModal: React.FC = ({ isVisible, const handleFormSubmit = async (formValues: SSOSettingsFormValues) => { try { - const payload = diffSSOSettingsPayload( - processSSOSettingsPayload(seededValues), - processSSOSettingsPayload(formValues), - ); + const payload = buildSSOSettingsPatch(seededValues, formValues, "sso-settings", processSSOSettingsPayload); await mutateAsync(payload, { onSuccess: () => {