mirror of
https://github.com/BerriAI/litellm.git
synced 2026-11-03 07:23:33 +00:00
fix(proxy): make /update/sso_settings a true PATCH and send only changed SSO fields from the UI (#49527)
* fix(proxy): make /update/sso_settings a true PATCH and send only changed SSO fields from the UI * fix(proxy): mask SSO client secrets in the /update/sso_settings response * fix(proxy): keep SSO values that only live in the environment_variables row on a partial SSO PATCH The SSO PATCH strips every SSO env var from the environment_variables config row. Now that the UI only sends changed fields, an omitted value that lived only in that row was dropped from both tables and lost on restart. Carry those values into the SSO row before stripping them * test(ui): pin CLEARED_SSO_SETTINGS to exactly the backend SSOConfig fields With PATCH semantics an omitted key keeps its stored value, so a field missing from the Clear list would silently survive Clear SSO settings
This commit is contained in:
parent
6f3251c7e3
commit
3b62b6311c
12 changed files with 458 additions and 161 deletions
|
|
@ -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)),
|
||||
}
|
||||
|
||||
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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");
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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: () => {
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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 });
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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));
|
||||
|
|
|
|||
|
|
@ -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: () => {
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
}>();
|
||||
});
|
||||
});
|
||||
|
|
@ -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",
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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";
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue