From c6fdd9058cb74009521a88981dc778a7b50852f5 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Tue, 7 Jul 2026 22:04:46 +0100 Subject: [PATCH] Allow clearing SSO provider restriction fields from Settings Empty allowedGroups, allowedDomains, allowedEmails, groupsClaim and groupRoleMappings in a provider PUT were silently restored from the existing config, so an admin could never clear them. The guards shielded against lossy round-trips that no longer exist: the detail GET and the flat list response both carry these fields, so an empty value is an intentional clear. Nested OIDC/SAML config and the client secret stay preserved since toggle payloads omit them and secrets are never echoed in reads. Also expose groupsClaim and groupRoleMappings in the shared extensions list response so the enterprise list matches the core one, and make the Settings enable/disable toggle send only writable fields; the old list-item spread included computed response fields that the enterprise strict PUT decoder rejects. --- .../components/Settings/ssoProvidersModel.ts | 2 + .../Settings/useSSOProvidersState.ts | 19 ++++++- internal/api/identity_sso_handlers.go | 19 ++----- internal/api/sso_handlers_crud_test.go | 49 ++++++++++++++++++- pkg/extensions/sso_admin.go | 40 ++++++++------- 5 files changed, 92 insertions(+), 37 deletions(-) diff --git a/frontend-modern/src/components/Settings/ssoProvidersModel.ts b/frontend-modern/src/components/Settings/ssoProvidersModel.ts index 4a5e7e52c..139fcb5f9 100644 --- a/frontend-modern/src/components/Settings/ssoProvidersModel.ts +++ b/frontend-modern/src/components/Settings/ssoProvidersModel.ts @@ -16,6 +16,8 @@ export interface SSOProvider { allowedGroups?: string[]; allowedDomains?: string[]; allowedEmails?: string[]; + groupsClaim?: string; + groupRoleMappings?: Record; } export interface SSOProvidersResponse { diff --git a/frontend-modern/src/components/Settings/useSSOProvidersState.ts b/frontend-modern/src/components/Settings/useSSOProvidersState.ts index 7f9085b7e..4093acfc4 100644 --- a/frontend-modern/src/components/Settings/useSSOProvidersState.ts +++ b/frontend-modern/src/components/Settings/useSSOProvidersState.ts @@ -185,10 +185,27 @@ export const useSSOProvidersState = (props: SSOProvidersPanelProps) => { } try { const { apiFetch } = await import('@/utils/apiClient'); + // Send only writable provider fields: the list item also carries + // computed response fields (oidcLoginUrl, samlAcsUrl, ...) that the + // strict PUT decoder rejects. Nested oidc/saml config and the client + // secret are intentionally absent; the backend preserves them. const response = await apiFetch(`/api/security/sso/providers/${provider.id}`, { method: 'PUT', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ ...provider, enabled: !provider.enabled }), + body: JSON.stringify({ + id: provider.id, + name: provider.name, + type: provider.type, + enabled: !provider.enabled, + displayName: provider.displayName, + iconUrl: provider.iconUrl, + priority: provider.priority, + allowedGroups: provider.allowedGroups ?? [], + allowedDomains: provider.allowedDomains ?? [], + allowedEmails: provider.allowedEmails ?? [], + groupsClaim: provider.groupsClaim, + groupRoleMappings: provider.groupRoleMappings, + }), }); if (!response.ok) { diff --git a/internal/api/identity_sso_handlers.go b/internal/api/identity_sso_handlers.go index 9359bfa78..01ce29fb9 100644 --- a/internal/api/identity_sso_handlers.go +++ b/internal/api/identity_sso_handlers.go @@ -500,21 +500,10 @@ func (r *Router) handleUpdateSSOProvider(w http.ResponseWriter, req *http.Reques if updated.Type == config.SSOProviderTypeSAML && updated.SAML == nil && existing.SAML != nil { updated.SAML = existing.SAML } - if updated.GroupsClaim == "" && existing.GroupsClaim != "" { - updated.GroupsClaim = existing.GroupsClaim - } - if len(updated.GroupRoleMappings) == 0 && len(existing.GroupRoleMappings) > 0 { - updated.GroupRoleMappings = existing.GroupRoleMappings - } - if len(updated.AllowedGroups) == 0 && len(existing.AllowedGroups) > 0 { - updated.AllowedGroups = existing.AllowedGroups - } - if len(updated.AllowedDomains) == 0 && len(existing.AllowedDomains) > 0 { - updated.AllowedDomains = existing.AllowedDomains - } - if len(updated.AllowedEmails) == 0 && len(existing.AllowedEmails) > 0 { - updated.AllowedEmails = existing.AllowedEmails - } + // groupsClaim, groupRoleMappings, allowedGroups, allowedDomains and + // allowedEmails are NOT preserved on empty: both the detail GET and the + // flat list response round-trip them, so an empty value in a PUT is an + // intentional clear. // Preserve secrets if not provided in update if updated.Type == config.SSOProviderTypeOIDC && updated.OIDC != nil && existing.OIDC != nil { diff --git a/internal/api/sso_handlers_crud_test.go b/internal/api/sso_handlers_crud_test.go index af2adedb2..28e9ff9bb 100644 --- a/internal/api/sso_handlers_crud_test.go +++ b/internal/api/sso_handlers_crud_test.go @@ -13,8 +13,6 @@ import ( "github.com/stretchr/testify/require" ) -// ... (skipping unchanged parts until test) - func TestSanitizeProviderName(t *testing.T) { tests := []struct { input string @@ -243,6 +241,53 @@ func TestSSOProviderDetailRoundTripPreservesOIDCEditableSettings(t *testing.T) { assert.Equal(t, map[string]string{"admins": "admin"}, stored.GroupRoleMappings) } +func TestUpdateSSOProvider_EmptyValuesClearRestrictionFields(t *testing.T) { + router, _ := setupTestRouter(t) + + provider := config.SSOProvider{ + ID: "corp-oidc", + Name: "Corporate OIDC", + Type: config.SSOProviderTypeOIDC, + Enabled: true, + AllowedGroups: []string{"admins"}, + AllowedDomains: []string{"example.com"}, + AllowedEmails: []string{"owner@example.com"}, + GroupsClaim: "groups", + GroupRoleMappings: map[string]string{ + "admins": "admin", + }, + OIDC: &config.OIDCProviderConfig{ + IssuerURL: "https://idp.example.com/realms/pulse", + ClientID: "pulse-client", + ClientSecret: "super-secret", + }, + } + require.NoError(t, router.ssoConfig.AddProvider(provider)) + require.NoError(t, router.saveSSOConfig()) + + // An intentionally emptied field in a PUT clears it; only the nested + // config (absent from toggle payloads) and the client secret (never + // echoed in reads) are preserved. + body := `{"name":"Corporate OIDC","type":"oidc","enabled":true,"allowedGroups":[],"allowedDomains":[],"allowedEmails":[],"groupsClaim":"","groupRoleMappings":{}}` + req := httptest.NewRequest(http.MethodPut, "/api/security/sso/providers/corp-oidc", strings.NewReader(body)) + w := httptest.NewRecorder() + router.handleSSOProvider(w, req) + require.Equal(t, http.StatusOK, w.Code, w.Body.String()) + + loadedConfig, err := router.persistence.LoadSSOConfig() + require.NoError(t, err) + stored := loadedConfig.GetProvider("corp-oidc") + require.NotNil(t, stored) + assert.Empty(t, stored.AllowedGroups) + assert.Empty(t, stored.AllowedDomains) + assert.Empty(t, stored.AllowedEmails) + assert.Empty(t, stored.GroupsClaim) + assert.Empty(t, stored.GroupRoleMappings) + require.NotNil(t, stored.OIDC) + assert.Equal(t, "https://idp.example.com/realms/pulse", stored.OIDC.IssuerURL) + assert.Equal(t, "super-secret", stored.OIDC.ClientSecret) +} + func TestCreateSSOProvider_Validation(t *testing.T) { router, _ := setupTestRouter(t) diff --git a/pkg/extensions/sso_admin.go b/pkg/extensions/sso_admin.go index c3470033b..bd2bfbd34 100644 --- a/pkg/extensions/sso_admin.go +++ b/pkg/extensions/sso_admin.go @@ -94,25 +94,27 @@ type SSOConfigSnapshot struct { // SSOProviderResponse is the API response shape for SSO providers. type SSOProviderResponse struct { - ID string `json:"id"` - Name string `json:"name"` - Type string `json:"type"` - Enabled bool `json:"enabled"` - DisplayName string `json:"displayName,omitempty"` - IconURL string `json:"iconUrl,omitempty"` - Priority int `json:"priority"` - OIDCIssuerURL string `json:"oidcIssuerUrl,omitempty"` - OIDCClientID string `json:"oidcClientId,omitempty"` - OIDCClientSecretSet bool `json:"oidcClientSecretSet,omitempty"` - OIDCLoginURL string `json:"oidcLoginUrl,omitempty"` - OIDCCallbackURL string `json:"oidcCallbackUrl,omitempty"` - SAMLIDPEntityID string `json:"samlIdpEntityId,omitempty"` - SAMLSPEntityID string `json:"samlSpEntityId,omitempty"` - SAMLMetadataURL string `json:"samlMetadataUrl,omitempty"` - SAMLACSURL string `json:"samlAcsUrl,omitempty"` - AllowedGroups []string `json:"allowedGroups,omitempty"` - AllowedDomains []string `json:"allowedDomains,omitempty"` - AllowedEmails []string `json:"allowedEmails,omitempty"` + ID string `json:"id"` + Name string `json:"name"` + Type string `json:"type"` + Enabled bool `json:"enabled"` + DisplayName string `json:"displayName,omitempty"` + IconURL string `json:"iconUrl,omitempty"` + Priority int `json:"priority"` + OIDCIssuerURL string `json:"oidcIssuerUrl,omitempty"` + OIDCClientID string `json:"oidcClientId,omitempty"` + OIDCClientSecretSet bool `json:"oidcClientSecretSet,omitempty"` + OIDCLoginURL string `json:"oidcLoginUrl,omitempty"` + OIDCCallbackURL string `json:"oidcCallbackUrl,omitempty"` + SAMLIDPEntityID string `json:"samlIdpEntityId,omitempty"` + SAMLSPEntityID string `json:"samlSpEntityId,omitempty"` + SAMLMetadataURL string `json:"samlMetadataUrl,omitempty"` + SAMLACSURL string `json:"samlAcsUrl,omitempty"` + AllowedGroups []string `json:"allowedGroups,omitempty"` + AllowedDomains []string `json:"allowedDomains,omitempty"` + AllowedEmails []string `json:"allowedEmails,omitempty"` + GroupsClaim string `json:"groupsClaim,omitempty"` + GroupRoleMappings map[string]string `json:"groupRoleMappings,omitempty"` } // SSOProvidersListResponse is the API response shape for list requests.