mirror of
https://github.com/BerriAI/litellm.git
synced 2026-11-10 08:28:25 +00:00
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
This commit is contained in:
parent
570f41daad
commit
0b39e5faf3
6 changed files with 139 additions and 10 deletions
|
|
@ -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 = {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
):
|
||||
|
|
|
|||
|
|
@ -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<SSOModalsProps> = ({
|
|||
}
|
||||
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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([]);
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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<string, unknown>,
|
||||
): Record<string, unknown> => {
|
||||
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<SSOSettingsFormValues>,
|
||||
|
|
|
|||
|
|
@ -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<EditSSOSettingsModalProps> = ({ 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: () => {
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue