fix(proxy): make /update/sso_settings a true PATCH and send only changed SSO fields from the UI

This commit is contained in:
jesus 2026-10-31 23:08:41 +00:00
parent a7425101bb
commit 9331e97d36
11 changed files with 339 additions and 132 deletions

View file

@ -1310,9 +1310,14 @@ 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
submitted: Final = sso_config.model_dump(include=sso_config.model_fields_set)
stored_values: Final = before_sso_data or {}
sso_data: Final = {
field_name: submitted[field_name] if field_name in submitted else stored_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:

View file

@ -722,6 +722,115 @@ 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):
"""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()
mock_prisma.writer_db.litellm_config.find_unique = AsyncMock(return_value=None)
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_maps_saml_fields_to_env_vars(
self, mock_proxy_config, mock_auth, monkeypatch
):

View file

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

View file

@ -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<string, unknown> => {
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<string, unknown> = {
...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<string, string> = {
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, unknown>): string | null => {
if (values.google_client_id) return "google";
if (values.microsoft_client_id) return "microsoft";
@ -134,56 +189,10 @@ const SSOModals: React.FC<SSOModalsProps> = ({
}
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<string, unknown> = {
...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<string, string> = {
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<SSOModalsProps> = ({
}
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);

View file

@ -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<AddSSOSettingsModalProps> = ({ 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: () => {

View file

@ -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<DeleteSSOSettingsModalProps> = ({ 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();

View file

@ -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(<EditSSOSettingsModal isVisible={true} onCancel={vi.fn()} onSuccess={vi.fn()} />);
@ -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(<EditSSOSettingsModal isVisible={true} onCancel={vi.fn()} onSuccess={vi.fn()} />);
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(<EditSSOSettingsModal isVisible={true} onCancel={vi.fn()} onSuccess={vi.fn()} />);
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(<EditSSOSettingsModal isVisible={true} onCancel={vi.fn()} onSuccess={vi.fn()} />);
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 });
});
});

View file

@ -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));

View file

@ -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<EditSSOSettingsModalProps> = ({ isVisible,
const handleFormSubmit = async (formValues: SSOSettingsFormValues) => {
try {
const payload = processSSOSettingsPayload(formValues);
const payload = diffSSOSettingsPayload(
processSSOSettingsPayload(seededValues),
processSSOSettingsPayload(formValues),
);
await mutateAsync(payload, {
onSuccess: () => {

View file

@ -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",
});
});
});

View file

@ -71,6 +71,49 @@ export const processSSOSettingsPayload = (formValues: Record<string, any>): 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<string, unknown>,
after: Record<string, unknown>,
): Record<string, unknown> =>
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";