fix(proxy): clear provider and mapping fields the SSO form drops, keep env out of the row

Omitted fields now keep only their stored value, so a partial save no longer copies env-only values into the row or touches their env vars. The dashboard SSO forms send null for other providers' fields and unchecked role or team mappings, Clear also nulls team_mappings and ui_access_mode, and an unreadable stored row no longer 500s the update
This commit is contained in:
jesus 2026-10-30 01:40:29 +00:00
parent b5080cd1d4
commit f6ecc40326
11 changed files with 228 additions and 59 deletions

View file

@ -9,6 +9,7 @@ save endpoint so the two can never drift.
from collections.abc import Mapping
from dataclasses import dataclass
from types import MappingProxyType
from typing import Final
from litellm.proxy.common_utils.encrypt_decrypt_utils import decrypt_value_helper
@ -97,3 +98,15 @@ def resolve_sso_config(sso_db_settings: Mapping[str, object] | None, env: Mappin
}
config: Final = SSOConfig(**{**values, **structured})
return ResolvedSSOConfig(config=config, provenance=provenance)
_SSO_SECRET_DESCRIPTORS: Final = tuple(d for d in SSO_DESCRIPTORS if d.is_secret)
def resolve_sso_secrets(sso_db_settings: Mapping[str, object] | None, env: Mapping[str, str]) -> Mapping[str, str]:
"""Effective client secrets (stored row, then env), decrypted. Reads only the secret keys so a
malformed role or team mapping on the row cannot block a settings update."""
raw: Final = dict(sso_db_settings) if sso_db_settings else {}
stored: Final = _decrypt({d.db_key: raw[d.db_key] for d in _SSO_SECRET_DESCRIPTORS if d.db_key in raw})
values, _ = resolve_fields(_SSO_SECRET_DESCRIPTORS, stored, env)
return MappingProxyType({name: value for name, value in values.items() if value})

View file

@ -33,8 +33,8 @@ from litellm.proxy.config_resolvers.settings_store import ConfigOwnedKeyError
from litellm.proxy.config_resolvers.sso import (
SSO_FIELD_ENV_VARS,
SSO_SECRET_FIELDS,
ResolvedSSOConfig,
resolve_sso_config,
resolve_sso_secrets,
)
from litellm.proxy.management_endpoints.team_admin_field_permissions import (
SUPPORTED_TEAM_ADMIN_PERMISSIONS,
@ -1238,38 +1238,33 @@ def _stored_sso_settings(record: _StoredSsoSettingsRow | None) -> Mapping[str, J
if record is None or not record.sso_settings:
return None
stored: Final = record.sso_settings
decoded: Final = _DECODED_JSON.validate_python(json.loads(stored)) if isinstance(stored, str) else stored
try:
decoded: Final = _DECODED_JSON.validate_python(json.loads(stored)) if isinstance(stored, str) else stored
return _SSO_SETTINGS_VALUES.validate_python(decoded)
except ValidationError:
except ValueError:
return None
def _current_sso_value(current: ResolvedSSOConfig, field_name: str, value: JsonValue) -> JsonValue:
return None if current.provenance.get(field_name) in ("default", "unset") else value
def _merged_sso_field(field_name: str, incoming: JsonValue, current: JsonValue, submitted: bool) -> JsonValue:
if not submitted:
return current
if field_name not in SSO_SECRET_FIELDS or not isinstance(current, str) or not current:
def _kept_sso_secret(field_name: str, incoming: JsonValue, current_secret: str | None) -> JsonValue:
if current_secret is None:
return incoming
if incoming == mask_sensitive_keys({field_name: current}, {field_name})[field_name]:
return current
return incoming
shown_secret: Final = mask_sensitive_keys({field_name: current_secret}, {field_name})[field_name]
return current_secret if incoming == shown_secret else incoming
def merge_sso_settings_update(sso_config: SSOConfig, current: ResolvedSSOConfig) -> Mapping[str, JsonValue]:
"""Fields the request omits keep their effective value, and a client secret sent back
as the mask /get/sso_settings returned keeps the real secret. An explicit null or empty
string still clears a field."""
def merge_sso_settings_update(
sso_config: SSOConfig, stored: Mapping[str, JsonValue], current_secrets: Mapping[str, str]
) -> Mapping[str, JsonValue]:
"""Fields the request omits keep their stored value (never an env-only value, so a save does not
freeze env into the row), and a client secret sent back as the mask /get/sso_settings returned
keeps the real secret. An explicit null or empty string still clears a field."""
incoming: Final = _SSO_SETTINGS_VALUES.validate_python(sso_config.model_dump(mode="json"))
current_values: Final = {
name: _current_sso_value(current, name, value)
for name, value in _SSO_SETTINGS_VALUES.validate_python(current.config.model_dump(mode="json")).items()
}
return {
name: _merged_sso_field(name, value, current_values[name], name in sso_config.model_fields_set)
name: (
_kept_sso_secret(name, value, current_secrets.get(name))
if name in sso_config.model_fields_set
else stored.get(name)
)
for name, value in incoming.items()
}
@ -1335,9 +1330,11 @@ async def update_sso_settings(
config["general_settings"] = {}
# Update environment variables in config and in memory
sso_data: Final = merge_sso_settings_update(sso_config, resolve_sso_config(stored_sso_settings, os.environ))
sso_data: Final = merge_sso_settings_update(
sso_config, before_sso_data or {}, resolve_sso_secrets(stored_sso_settings, os.environ)
)
for field_name, value in sso_data.items():
if field_name in SSO_FIELD_ENV_VARS:
if field_name in SSO_FIELD_ENV_VARS and field_name in sso_config.model_fields_set:
env_var_name = SSO_FIELD_ENV_VARS[field_name]
if isinstance(value, str) and value:
os.environ[env_var_name] = value

View file

@ -1137,7 +1137,6 @@ class TestProxySettingEndpoints:
assert create_sso_settings["user_email"] is None
def _mock_sso_update_db(self, monkeypatch, stored_sso_settings):
"""Point /update/sso_settings at a stored SSO row (or None) with encryption as identity."""
from unittest.mock import AsyncMock, MagicMock
from litellm.proxy.proxy_server import proxy_config
@ -1145,27 +1144,29 @@ class TestProxySettingEndpoints:
monkeypatch.setenv("LITELLM_SALT_KEY", "test_salt_key")
monkeypatch.setattr("litellm.proxy.proxy_server.store_model_in_db", True)
mock_prisma = MagicMock()
if stored_sso_settings is None:
record = None
else:
record = None
if stored_sso_settings is not None:
record = MagicMock()
record.sso_settings = stored_sso_settings
record.sso_settings = (
stored_sso_settings
if isinstance(stored_sso_settings, str)
else proxy_config.encrypt_env_variables(environment_variables=stored_sso_settings)
)
mock_prisma.db.litellm_ssoconfig.find_unique = AsyncMock(return_value=record)
mock_prisma.db.litellm_ssoconfig.upsert = AsyncMock()
mock_prisma.writer_db.litellm_config.find_unique = AsyncMock(return_value=None)
mock_prisma.db.litellm_config.update = AsyncMock()
monkeypatch.setattr("litellm.proxy.proxy_server.prisma_client", mock_prisma)
monkeypatch.setattr(proxy_config, "encrypt_env_variables", lambda environment_variables: environment_variables)
identity = lambda value, key=None, exception_type="error", return_original_value=False: value # noqa: E731
monkeypatch.setattr("litellm.proxy.config_resolvers.sso.decrypt_value_helper", identity)
monkeypatch.setattr("litellm.proxy.proxy_server.decrypt_value_helper", identity)
return mock_prisma
@staticmethod
def _upserted_sso_settings(mock_prisma):
import json
return json.loads(mock_prisma.db.litellm_ssoconfig.upsert.call_args.kwargs["data"]["update"]["sso_settings"])
from litellm.proxy.proxy_server import proxy_config
stored = json.loads(mock_prisma.db.litellm_ssoconfig.upsert.call_args.kwargs["data"]["update"]["sso_settings"])
return proxy_config.decrypt_db_variables(stored)
@pytest.mark.parametrize(
"secret_field,env_var",
@ -1178,8 +1179,6 @@ class TestProxySettingEndpoints:
def test_update_sso_settings_round_tripped_mask_keeps_stored_secret(
self, mock_proxy_config, mock_auth, monkeypatch, secret_field, env_var
):
"""Regression for LIT-8683: saving the settings page (which only ever holds the masked
secret from /get/sso_settings) must not overwrite the stored client secret with the mask."""
monkeypatch.delenv(env_var, raising=False)
stored_secret = "stored-client-secret-0123456789"
mock_prisma = self._mock_sso_update_db(monkeypatch, {secret_field: stored_secret})
@ -1199,8 +1198,6 @@ class TestProxySettingEndpoints:
def test_update_sso_settings_round_tripped_mask_keeps_env_secret(
self, mock_proxy_config, mock_auth, monkeypatch
):
"""A secret that only lives in the process env is shown masked too, and saving the
page persists that secret rather than the mask."""
monkeypatch.setenv("GENERIC_CLIENT_SECRET", "env-only-secret-0123456789")
mock_prisma = self._mock_sso_update_db(monkeypatch, None)
@ -1214,14 +1211,13 @@ class TestProxySettingEndpoints:
def test_update_sso_settings_omitted_fields_keep_stored_values(
self, mock_proxy_config, mock_auth, monkeypatch
):
"""The UI access control form sends only ui_access_mode. That must not null out the
rest of the stored SSO configuration."""
monkeypatch.delenv("GENERIC_CLIENT_SECRET", raising=False)
monkeypatch.setenv("GENERIC_CLIENT_SECRET", "stored-client-secret-0123456789")
stored = {
"generic_client_id": "stored-client-id",
"generic_client_secret": "stored-client-secret-0123456789",
"generic_token_endpoint": "https://idp.example.com/token",
"role_mappings": {"provider": "generic", "group_claim": "groups", "roles": {"proxy_admin": ["admins"]}},
"team_mappings": {"team_ids_jwt_field": "teams"},
}
mock_prisma = self._mock_sso_update_db(monkeypatch, stored)
@ -1230,12 +1226,55 @@ class TestProxySettingEndpoints:
assert response.status_code == 200, response.text
persisted = self._upserted_sso_settings(mock_prisma)
assert persisted["ui_access_mode"] == "none"
assert persisted["generic_client_id"] == "stored-client-id"
assert persisted["generic_client_secret"] == "stored-client-secret-0123456789"
assert persisted["generic_token_endpoint"] == "https://idp.example.com/token"
assert persisted["role_mappings"]["roles"]["proxy_admin"] == ["admins"]
assert {name: persisted[name] for name in stored} == stored
assert os.environ["GENERIC_CLIENT_SECRET"] == "stored-client-secret-0123456789"
def test_update_sso_settings_omitted_env_only_field_is_not_frozen_into_row(
self, mock_proxy_config, mock_auth, monkeypatch
):
monkeypatch.setenv("GENERIC_CLIENT_ID", "env-client-id")
mock_prisma = self._mock_sso_update_db(monkeypatch, {"user_email": "admin@example.com"})
response = client.patch("/update/sso_settings", json={"ui_access_mode": "none"})
assert response.status_code == 200, response.text
persisted = self._upserted_sso_settings(mock_prisma)
assert persisted["user_email"] == "admin@example.com"
assert persisted["generic_client_id"] is None
assert os.environ["GENERIC_CLIENT_ID"] == "env-client-id"
def test_update_sso_settings_explicit_nulls_clear_previous_provider_and_mappings(
self, mock_proxy_config, mock_auth, monkeypatch
):
monkeypatch.setenv("GOOGLE_CLIENT_ID", "stored-google-id")
mock_prisma = self._mock_sso_update_db(
monkeypatch,
{
"google_client_id": "stored-google-id",
"google_client_secret": "stored-google-secret-0123456789",
"role_mappings": {"provider": "generic", "group_claim": "groups", "roles": {"proxy_admin": ["a"]}},
"team_mappings": {"team_ids_jwt_field": "teams"},
},
)
response = client.patch(
"/update/sso_settings",
json={
"microsoft_client_id": "new-microsoft-id",
"google_client_id": None,
"google_client_secret": None,
"role_mappings": None,
"team_mappings": None,
},
)
assert response.status_code == 200, response.text
persisted = self._upserted_sso_settings(mock_prisma)
assert persisted["microsoft_client_id"] == "new-microsoft-id"
cleared = ("google_client_id", "google_client_secret", "role_mappings", "team_mappings")
assert {name: persisted[name] for name in cleared} == dict.fromkeys(cleared)
assert "GOOGLE_CLIENT_ID" not in os.environ
def test_update_sso_settings_new_secret_replaces_stored_secret(
self, mock_proxy_config, mock_auth, monkeypatch
):
@ -1250,6 +1289,20 @@ class TestProxySettingEndpoints:
assert os.environ["GENERIC_CLIENT_SECRET"] == "rotated-secret-9876543210"
assert "rotated-secret-9876543210" not in response.text
def test_update_sso_settings_mask_of_another_field_secret_is_not_swapped(
self, mock_proxy_config, mock_auth, monkeypatch
):
monkeypatch.delenv("GOOGLE_CLIENT_SECRET", raising=False)
mock_prisma = self._mock_sso_update_db(
monkeypatch, {"generic_client_secret": "stored-client-secret-0123456789"}
)
generic_mask = client.get("/get/sso_settings").json()["values"]["generic_client_secret"]
response = client.patch("/update/sso_settings", json={"google_client_secret": generic_mask})
assert response.status_code == 200, response.text
assert self._upserted_sso_settings(mock_prisma)["google_client_secret"] == generic_mask
@pytest.mark.parametrize("cleared", [None, ""])
def test_update_sso_settings_explicit_clear_drops_stored_secret(
self, mock_proxy_config, mock_auth, monkeypatch, cleared
@ -1264,6 +1317,23 @@ class TestProxySettingEndpoints:
assert not self._upserted_sso_settings(mock_prisma)["generic_client_secret"]
assert "GENERIC_CLIENT_SECRET" not in os.environ
@pytest.mark.parametrize(
"stored_row",
[
"{not json",
{"generic_client_secret": "stored-client-secret-0123456789", "role_mappings": {"roles": "not-a-dict"}},
],
)
def test_update_sso_settings_overwrites_unreadable_stored_row(
self, mock_proxy_config, mock_auth, monkeypatch, stored_row
):
mock_prisma = self._mock_sso_update_db(monkeypatch, stored_row)
response = client.patch("/update/sso_settings", json={"generic_client_id": "new-client-id"})
assert response.status_code == 200, response.text
assert self._upserted_sso_settings(mock_prisma)["generic_client_id"] == "new-client-id"
def test_get_ui_theme_settings(self, mock_proxy_config):
"""Test getting UI theme settings without authentication"""
response = client.get("/get/ui_theme_settings")

View file

@ -389,6 +389,15 @@ describe("SSOModals", () => {
generic_authorization_endpoint: "https://example.okta.com/authorize",
generic_token_endpoint: "https://example.okta.com/token",
generic_userinfo_endpoint: "https://example.okta.com/userinfo",
google_client_id: null,
google_client_secret: null,
microsoft_client_id: null,
microsoft_client_secret: null,
microsoft_tenant: null,
saml_idp_metadata_url: null,
saml_idp_metadata_xml: null,
saml_sp_entity_id: null,
saml_allow_unsolicited: null,
role_mappings: {
provider: "generic",
group_claim: "groups",
@ -535,6 +544,8 @@ describe("SSOModals", () => {
user_email: null,
sso_provider: null,
role_mappings: null,
team_mappings: null,
ui_access_mode: null,
});
});

View file

@ -224,6 +224,8 @@ const SSOModals: React.FC<SSOModalsProps> = ({
user_email: null,
sso_provider: null,
role_mappings: null,
team_mappings: null,
ui_access_mode: null,
};
await updateSSOSettings(accessToken, clearSettings);

View file

@ -4,6 +4,7 @@ import { renderWithProviders } from "../../../../../../tests/test-utils";
import { afterEach, describe, expect, it, vi } from "vitest";
import BaseSSOSettingsForm, {
emptySSOSettingsFormValues,
pickMountedSSOValues,
renderProviderFields,
ssoProviderConfigs,
submitMountedSSOValues,
@ -436,3 +437,27 @@ describe("renderProviderFields", () => {
expect(await screen.findByLabelText("Default Role")).toHaveTextContent("Internal User");
});
});
describe("pickMountedSSOValues", () => {
it("nulls the previous provider's fields after a provider switch so the save clears them", () => {
const picked = pickMountedSSOValues(
{
sso_provider: "microsoft",
google_client_id: "old-google-id",
google_client_secret: "old-google-secret",
microsoft_client_id: "new-microsoft-id",
user_email: "admin@example.com",
},
"sso-settings",
);
expect(picked).toMatchObject({
sso_provider: "microsoft",
microsoft_client_id: "new-microsoft-id",
google_client_id: null,
google_client_secret: null,
generic_client_id: null,
saml_idp_metadata_url: null,
});
});
});

View file

@ -199,10 +199,17 @@ export const mountedSSOFieldNames = (values: SSOSettingsFormValues, variant: SSO
];
};
export const pickMountedSSOValues = (values: SSOSettingsFormValues, variant: SSOFormVariant): SSOSettingsFormValues =>
Object.fromEntries(
mountedSSOFieldNames(values, variant).map((name) => [name, values[name as keyof SSOSettingsFormValues]]),
);
const ALL_PROVIDER_FIELD_NAMES: readonly string[] = [
...new Set(Object.values(ssoProviderConfigs).flatMap((config) => config.fields.map((field) => field.name))),
];
export const pickMountedSSOValues = (values: SSOSettingsFormValues, variant: SSOFormVariant): SSOSettingsFormValues => {
const mounted = mountedSSOFieldNames(values, variant);
return Object.fromEntries([
...ALL_PROVIDER_FIELD_NAMES.filter((name) => !mounted.includes(name)).map((name) => [name, null]),
...mounted.map((name) => [name, values[name as keyof SSOSettingsFormValues]]),
]);
};
export const submitMountedSSOValues =
(

View file

@ -38,6 +38,7 @@ const DeleteSSOSettingsModal: React.FC<DeleteSSOSettingsModalProps> = ({ isVisib
sso_provider: null,
role_mappings: null,
team_mappings: null,
ui_access_mode: null,
};
await editSSOSettings(clearSettings, {

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 the mounted fields and clears every other provider's fields and the unchecked mappings", async () => {
const user = userEvent.setup({ pointerEventsCheck: 0 });
renderWithProviders(<EditSSOSettingsModal isVisible={true} onCancel={vi.fn()} onSuccess={vi.fn()} />);
@ -64,6 +64,21 @@ describe("EditSSOSettingsModal (real form tree)", () => {
google_client_secret: "stored-client-secret",
user_email: "admin@example.com",
proxy_base_url: "https://gateway.example.com",
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,
role_mappings: null,
team_mappings: null,
});
});
});

View file

@ -24,9 +24,9 @@ describe("processSSOSettingsPayload", () => {
expect(result).toEqual({
other_field: "value",
another_field: 123,
role_mappings: null,
team_mappings: null,
});
expect(result.role_mappings).toBeUndefined();
expect(result.team_mappings).toBeUndefined();
});
it("should return all fields except role mapping fields when use_role_mappings is not present", () => {
@ -46,9 +46,29 @@ describe("processSSOSettingsPayload", () => {
expect(result).toEqual({
other_field: "value",
role_mappings: null,
team_mappings: null,
});
expect(result.role_mappings).toBeUndefined();
expect(result.team_mappings).toBeUndefined();
});
it("clears mappings the user unchecked on a provider that supports them", () => {
const result = processSSOSettingsPayload({
sso_provider: "generic",
use_role_mappings: false,
use_team_mappings: false,
group_claim: "groups",
team_ids_jwt_field: "teams",
});
expect(result.role_mappings).toBeNull();
expect(result.team_mappings).toBeNull();
});
it("leaves mappings alone when the form has no toggle for them", () => {
const result = processSSOSettingsPayload({ sso_provider: "generic", use_role_mappings: false });
expect(result.role_mappings).toBeNull();
expect(result).not.toHaveProperty("team_mappings");
});
});
@ -274,8 +294,8 @@ describe("processSSOSettingsPayload", () => {
expect(result).toEqual({
sso_provider: "okta",
other_field: "value",
team_mappings: null,
});
expect(result.team_mappings).toBeUndefined();
});
it("should return all fields except team mapping fields when use_team_mappings is not present", () => {
@ -307,8 +327,9 @@ describe("processSSOSettingsPayload", () => {
expect(result).toEqual({
sso_provider: "google",
other_field: "value",
role_mappings: null,
team_mappings: null,
});
expect(result.team_mappings).toBeUndefined();
});
it("should not include team mappings for microsoft provider even when use_team_mappings is true", () => {
@ -324,8 +345,9 @@ describe("processSSOSettingsPayload", () => {
expect(result).toEqual({
sso_provider: "microsoft",
other_field: "value",
role_mappings: null,
team_mappings: null,
});
expect(result.team_mappings).toBeUndefined();
});
});
@ -401,7 +423,7 @@ describe("processSSOSettingsPayload", () => {
it("should handle empty form values", () => {
const result = processSSOSettingsPayload({});
expect(result).toEqual({});
expect(result).toEqual({ role_mappings: null, team_mappings: null });
});
it("should preserve other fields in the payload", () => {
@ -425,6 +447,8 @@ describe("processSSOSettingsPayload", () => {
redirect_url: "http://example.com",
custom_field: { nested: "value" },
array_field: [1, 2, 3],
role_mappings: null,
team_mappings: null,
});
});
});

View file

@ -58,6 +58,8 @@ export const processSSOSettingsPayload = (formValues: Record<string, any>): Reco
internal_user_viewer: splitTeams(internal_viewer_teams),
},
};
} else if (!supportsRoleMappings || "use_role_mappings" in formValues) {
payload.role_mappings = null;
}
// Add team mappings only if use_team_mappings is checked AND provider supports team mappings
@ -66,6 +68,8 @@ export const processSSOSettingsPayload = (formValues: Record<string, any>): Reco
payload.team_mappings = {
team_ids_jwt_field: team_ids_jwt_field,
};
} else if (!supportsTeamMappings || "use_team_mappings" in formValues) {
payload.team_mappings = null;
}
return payload;