From caa9b418342a97cc1ee7f3a1c2cb1c747674c083 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Mon, 6 Jul 2026 11:18:18 +0100 Subject: [PATCH] Fix OIDC provider detail persistence Refs #1521 --- .../v6/internal/subsystems/agent-lifecycle.md | 6 + .../v6/internal/subsystems/api-contracts.md | 20 +++- .../internal/subsystems/storage-recovery.md | 6 + internal/api/contract_test.go | 106 ++++++++++++++++++ internal/api/identity_sso_handlers.go | 102 ++++++++++++++--- internal/api/sso_handlers_crud_test.go | 76 +++++++++++++ 6 files changed, 301 insertions(+), 15 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index a63ba9d52..502431fe1 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -1597,6 +1597,12 @@ list representation, and masked-echo preservation on update are notifications/API-contract owned and grant no agent install, enrollment, setup-token, or fleet command authority. +SSO provider-detail payload changes on shared `internal/api/identity_sso_handlers.go` +are API-contract/security-settings owned. Nested OIDC/SAML edit fields, +restriction lists, role mappings, and masked secret-presence markers create no +agent install, enrollment, setup-token, command, fleet liveness, or agent +profile semantics. + Alert delivery diagnosis on shared `internal/api/alerts.go` is likewise adjacent only: `/api/alerts/delivery-diagnosis` is alerts/API-contract owned read-only notification-policy evidence and must not be interpreted as agent diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index c9995cc27..77713989a 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -3093,7 +3093,16 @@ a new API state machine, queue contract, or verification-accounting field. inputs before any outbound request is attempted, and OIDC discovery must append `/.well-known/openid-configuration` beneath the configured issuer base path instead of resetting to the origin root. -32. Keep config-archive import reloads fail-closed on the shared API/runtime +32. Keep SSO provider-detail payloads edit-complete and non-secret: + `GET /api/security/sso/providers/{id}` may keep flat list/card fields for + summary compatibility, but it must also return the nested OIDC/SAML fields + used by settings edit forms, plus restrictions and role mappings, without + echoing raw OIDC client secrets or SAML private keys. The same detail + payload must round-trip through `PUT /api/security/sso/providers/{id}` + without clearing existing OIDC redirect, logout, scopes, restrictions, + role mappings, or stored secret markers unless the update explicitly + replaces them. +33. Keep config-archive import reloads fail-closed on the shared API/runtime boundary. `internal/api/config_export_import_handlers.go`, `internal/api/contract_test.go`, and adjacent config/runtime helpers must tolerate absent notification managers and other optional runtime managers @@ -4147,6 +4156,15 @@ That same SSO boundary also owns manual SAML endpoint validation payloads. through the same validated absolute HTTP(S) helpers instead of letting the manual logout URL drift out of the request model or bypass the governed URL normalization path. +That same SSO provider-detail boundary must return the non-secret nested +provider configuration used by the settings edit form. `GET +/api/security/sso/providers/{id}` may keep the flat list/card fields for +summary compatibility, but it must also include nested OIDC/SAML edit fields, +group restrictions, and role mappings while never echoing raw OIDC client +secrets or SAML private keys. Saving that detail payload back through `PUT +/api/security/sso/providers/{id}` must preserve configured OIDC redirect, +logout, scopes, restrictions, mappings, and existing secrets unless the update +explicitly replaces them. That same runtime SSO contract also owns the Pulse-side public URL that feeds SAML service-provider metadata and auth requests. `internal/api/saml_handlers.go`, `internal/api/saml_service.go`, and the SAML regression tests must rebind diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 60edea525..6c697009d 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -1593,6 +1593,12 @@ likewise adjacent only: the webhook `signingSecret` payload field and its masking semantics are notifications/API-contract owned and create no storage, recovery-point, or backup-surface semantics. +SSO provider-detail payload changes on shared `internal/api/identity_sso_handlers.go` +are API-contract/security-settings owned. Nested OIDC/SAML edit fields, +restriction lists, role mappings, and masked secret-presence markers create no +storage health, recovery-point, backup verification, restore authorization, or +provider-coverage semantics. + Alert delivery diagnosis on shared `internal/api/alerts.go` is likewise adjacent only: `/api/alerts/delivery-diagnosis` exposes alerts/API-contract read-only notification-policy evidence and creates no storage health, diff --git a/internal/api/contract_test.go b/internal/api/contract_test.go index c4c2dd427..02e88cbe7 100644 --- a/internal/api/contract_test.go +++ b/internal/api/contract_test.go @@ -3239,6 +3239,112 @@ func TestContract_SSOTestOIDCDiscoveryKeepsIssuerBasePath(t *testing.T) { } } +func TestContract_SSOProviderDetailRoundTripKeepsEditableOIDCConfig(t *testing.T) { + tmp := t.TempDir() + persistence := config.NewConfigPersistence(tmp) + router := &Router{ + config: &config.Config{ + DataPath: tmp, + PublicURL: "https://pulse.example.com", + }, + persistence: persistence, + ssoConfig: config.NewSSOConfig(), + samlManager: NewSAMLServiceManager("https://pulse.example.com"), + oidcManager: NewOIDCServiceManager(), + } + if err := persistence.SaveSSOConfig(router.ssoConfig); err != nil { + t.Fatalf("save initial sso config: %v", err) + } + + provider := config.SSOProvider{ + ID: "corp-oidc", + Name: "Corporate OIDC", + Type: config.SSOProviderTypeOIDC, + Enabled: true, + AllowedGroups: []string{"admins", "operators"}, + 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", + RedirectURL: "https://pulse.example.com/api/oidc/corp-oidc/callback", + LogoutURL: "https://idp.example.com/logout", + Scopes: []string{"openid", "profile", "email", "groups"}, + }, + } + if err := router.ssoConfig.AddProvider(provider); err != nil { + t.Fatalf("add provider: %v", err) + } + if err := router.saveSSOConfig(); err != nil { + t.Fatalf("save provider: %v", err) + } + + req := httptest.NewRequest(http.MethodGet, "/api/security/sso/providers/corp-oidc", nil) + rec := httptest.NewRecorder() + router.handleSSOProvider(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("get status=%d, want %d: %s", rec.Code, http.StatusOK, rec.Body.String()) + } + if strings.Contains(rec.Body.String(), "super-secret") { + t.Fatal("provider detail response exposed raw OIDC client secret") + } + + var detail SSOProviderResponse + if err := json.Unmarshal(rec.Body.Bytes(), &detail); err != nil { + t.Fatalf("unmarshal detail response: %v", err) + } + if detail.OIDC == nil { + t.Fatal("provider detail response missing nested OIDC edit config") + } + if detail.OIDC.RedirectURL != provider.OIDC.RedirectURL { + t.Fatalf("redirect URL = %q, want %q", detail.OIDC.RedirectURL, provider.OIDC.RedirectURL) + } + if detail.OIDC.LogoutURL != provider.OIDC.LogoutURL { + t.Fatalf("logout URL = %q, want %q", detail.OIDC.LogoutURL, provider.OIDC.LogoutURL) + } + if !reflect.DeepEqual(detail.OIDC.Scopes, provider.OIDC.Scopes) { + t.Fatalf("scopes = %#v, want %#v", detail.OIDC.Scopes, provider.OIDC.Scopes) + } + if detail.GroupsClaim != "groups" || !reflect.DeepEqual(detail.GroupRoleMappings, provider.GroupRoleMappings) { + t.Fatalf("group mapping detail lost: claim=%q mappings=%#v", detail.GroupsClaim, detail.GroupRoleMappings) + } + + body, err := json.Marshal(detail) + if err != nil { + t.Fatalf("marshal detail response: %v", err) + } + req = httptest.NewRequest(http.MethodPut, "/api/security/sso/providers/corp-oidc", bytes.NewReader(body)) + rec = httptest.NewRecorder() + router.handleSSOProvider(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("put status=%d, want %d: %s", rec.Code, http.StatusOK, rec.Body.String()) + } + + loaded, err := persistence.LoadSSOConfig() + if err != nil { + t.Fatalf("load stored sso config: %v", err) + } + stored := loaded.GetProvider("corp-oidc") + if stored == nil || stored.OIDC == nil { + t.Fatalf("stored provider missing OIDC config: %#v", stored) + } + if stored.OIDC.ClientSecret != "super-secret" || !stored.OIDC.ClientSecretSet { + t.Fatal("OIDC edit round-trip did not preserve existing client secret marker") + } + if stored.OIDC.RedirectURL != provider.OIDC.RedirectURL || stored.OIDC.LogoutURL != provider.OIDC.LogoutURL || !reflect.DeepEqual(stored.OIDC.Scopes, provider.OIDC.Scopes) { + t.Fatalf("OIDC edit fields were not preserved: %#v", stored.OIDC) + } + if !reflect.DeepEqual(stored.AllowedGroups, provider.AllowedGroups) || !reflect.DeepEqual(stored.AllowedDomains, provider.AllowedDomains) || !reflect.DeepEqual(stored.AllowedEmails, provider.AllowedEmails) { + t.Fatalf("SSO restrictions were not preserved: %#v", stored) + } + if stored.GroupsClaim != provider.GroupsClaim || !reflect.DeepEqual(stored.GroupRoleMappings, provider.GroupRoleMappings) { + t.Fatalf("SSO role mappings were not preserved: %#v", stored) + } +} + func TestContract_SSOTestRejectsCrossOriginSAMLMetadataRedirect(t *testing.T) { targetCalled := make(chan struct{}, 1) target := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { diff --git a/internal/api/identity_sso_handlers.go b/internal/api/identity_sso_handlers.go index 92171b5fc..0fa585730 100644 --- a/internal/api/identity_sso_handlers.go +++ b/internal/api/identity_sso_handlers.go @@ -99,22 +99,49 @@ type SSOProviderResponse struct { Priority int `json:"priority"` // OIDC-specific (only present for OIDC providers) - 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"` + 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"` + OIDC *SSOProviderOIDCResponse `json:"oidc,omitempty"` // SAML-specific (only present for SAML providers) - SAMLIDPEntityID string `json:"samlIdpEntityId,omitempty"` - SAMLSPEntityID string `json:"samlSpEntityId,omitempty"` - SAMLMetadataURL string `json:"samlMetadataUrl,omitempty"` - SAMLACSURL string `json:"samlAcsUrl,omitempty"` + SAMLIDPEntityID string `json:"samlIdpEntityId,omitempty"` + SAMLSPEntityID string `json:"samlSpEntityId,omitempty"` + SAMLMetadataURL string `json:"samlMetadataUrl,omitempty"` + SAMLACSURL string `json:"samlAcsUrl,omitempty"` + SAML *SSOProviderSAMLResponse `json:"saml,omitempty"` // Common restrictions - AllowedGroups []string `json:"allowedGroups"` - AllowedDomains []string `json:"allowedDomains"` - AllowedEmails []string `json:"allowedEmails"` + AllowedGroups []string `json:"allowedGroups"` + AllowedDomains []string `json:"allowedDomains"` + AllowedEmails []string `json:"allowedEmails"` + GroupsClaim string `json:"groupsClaim,omitempty"` + GroupRoleMappings map[string]string `json:"groupRoleMappings,omitempty"` +} + +type SSOProviderOIDCResponse struct { + IssuerURL string `json:"issuerUrl,omitempty"` + ClientID string `json:"clientId,omitempty"` + RedirectURL string `json:"redirectUrl,omitempty"` + LogoutURL string `json:"logoutUrl,omitempty"` + Scopes []string `json:"scopes"` + ClientSecretSet bool `json:"clientSecretSet,omitempty"` +} + +type SSOProviderSAMLResponse struct { + IDPMetadataURL string `json:"idpMetadataUrl,omitempty"` + IDPMetadataXML string `json:"idpMetadataXml,omitempty"` + IDPSSOURL string `json:"idpSsoUrl,omitempty"` + IDPEntityID string `json:"idpEntityId,omitempty"` + IDPCertificate string `json:"idpCertificate,omitempty"` + SPEntityID string `json:"spEntityId,omitempty"` + SignRequests bool `json:"signRequests,omitempty"` + AllowIDPInitiated bool `json:"allowIdpInitiated,omitempty"` + UsernameAttr string `json:"usernameAttr,omitempty"` + EmailAttr string `json:"emailAttr,omitempty"` + GroupsAttr string `json:"groupsAttr,omitempty"` } func EmptySSOProviderResponse() SSOProviderResponse { @@ -131,6 +158,12 @@ func (r SSOProviderResponse) NormalizeCollections() SSOProviderResponse { if r.AllowedEmails == nil { r.AllowedEmails = []string{} } + if r.GroupRoleMappings == nil { + r.GroupRoleMappings = map[string]string{} + } + if r.OIDC != nil && r.OIDC.Scopes == nil { + r.OIDC.Scopes = []string{} + } return r } @@ -244,7 +277,7 @@ func (r *Router) handleGetSSOProvider(w http.ResponseWriter, req *http.Request, } w.Header().Set("Content-Type", "application/json") - json.NewEncoder(w).Encode(providerToResponse(provider, r.config.PublicURL).NormalizeCollections()) + json.NewEncoder(w).Encode(providerToDetailResponse(provider, r.config.PublicURL).NormalizeCollections()) } func (r *Router) handleCreateSSOProvider(w http.ResponseWriter, req *http.Request) { @@ -300,6 +333,9 @@ func (r *Router) handleCreateSSOProvider(w http.ResponseWriter, req *http.Reques writeErrorResponse(w, http.StatusBadRequest, "validation_error", "Invalid OIDC redirect URL", nil) return } + if provider.OIDC.ClientSecret != "" { + provider.OIDC.ClientSecretSet = true + } } // Security: Validate SAML configuration @@ -470,9 +506,12 @@ func (r *Router) handleUpdateSSOProvider(w http.ResponseWriter, req *http.Reques // Preserve secrets if not provided in update if updated.Type == config.SSOProviderTypeOIDC && updated.OIDC != nil && existing.OIDC != nil { - if updated.OIDC.ClientSecret == "" && existing.OIDC.ClientSecretSet { + existingHasSecret := existing.OIDC.ClientSecretSet || existing.OIDC.ClientSecret != "" + if updated.OIDC.ClientSecret == "" && existingHasSecret { updated.OIDC.ClientSecret = existing.OIDC.ClientSecret updated.OIDC.ClientSecretSet = true + } else if updated.OIDC.ClientSecret != "" { + updated.OIDC.ClientSecretSet = true } } @@ -599,6 +638,8 @@ func providerToResponse(p *config.SSOProvider, publicURL string) SSOProviderResp resp.AllowedGroups = p.AllowedGroups resp.AllowedDomains = p.AllowedDomains resp.AllowedEmails = p.AllowedEmails + resp.GroupsClaim = p.GroupsClaim + resp.GroupRoleMappings = p.GroupRoleMappings if resp.DisplayName == "" { resp.DisplayName = p.Name @@ -633,6 +674,39 @@ func providerToResponse(p *config.SSOProvider, publicURL string) SSOProviderResp return resp.NormalizeCollections() } +func providerToDetailResponse(p *config.SSOProvider, publicURL string) SSOProviderResponse { + resp := providerToResponse(p, publicURL) + + if p.Type == config.SSOProviderTypeOIDC && p.OIDC != nil { + resp.OIDC = &SSOProviderOIDCResponse{ + IssuerURL: p.OIDC.IssuerURL, + ClientID: p.OIDC.ClientID, + RedirectURL: p.OIDC.RedirectURL, + LogoutURL: p.OIDC.LogoutURL, + Scopes: append([]string{}, p.OIDC.Scopes...), + ClientSecretSet: p.OIDC.ClientSecretSet || p.OIDC.ClientSecret != "", + } + } + + if p.Type == config.SSOProviderTypeSAML && p.SAML != nil { + resp.SAML = &SSOProviderSAMLResponse{ + IDPMetadataURL: p.SAML.IDPMetadataURL, + IDPMetadataXML: p.SAML.IDPMetadataXML, + IDPSSOURL: p.SAML.IDPSSOURL, + IDPEntityID: p.SAML.IDPEntityID, + IDPCertificate: p.SAML.IDPCertificate, + SPEntityID: p.SAML.SPEntityID, + SignRequests: p.SAML.SignRequests, + AllowIDPInitiated: p.SAML.AllowIDPInitiated, + UsernameAttr: p.SAML.UsernameAttr, + EmailAttr: p.SAML.EmailAttr, + GroupsAttr: p.SAML.GroupsAttr, + } + } + + return resp.NormalizeCollections() +} + // ============================================================================ // SSO Provider Connection Testing // ============================================================================ diff --git a/internal/api/sso_handlers_crud_test.go b/internal/api/sso_handlers_crud_test.go index 156fb52c2..af2adedb2 100644 --- a/internal/api/sso_handlers_crud_test.go +++ b/internal/api/sso_handlers_crud_test.go @@ -167,6 +167,82 @@ func TestSSOProviderCRUD(t *testing.T) { assert.Nil(t, loadedConfig.GetProvider("test-oidc")) } +func TestSSOProviderDetailRoundTripPreservesOIDCEditableSettings(t *testing.T) { + router, _ := setupTestRouter(t) + + provider := config.SSOProvider{ + ID: "corp-oidc", + Name: "Corporate OIDC", + Type: config.SSOProviderTypeOIDC, + Enabled: true, + DisplayName: "Sign in with Corporate SSO", + Priority: 2, + AllowedGroups: []string{ + "admins", + "operators", + }, + 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", + RedirectURL: "https://pulse.example.com/api/oidc/corp-oidc/callback", + LogoutURL: "https://idp.example.com/logout", + Scopes: []string{"openid", "profile", "email", "groups"}, + }, + } + require.NoError(t, router.ssoConfig.AddProvider(provider)) + require.NoError(t, router.saveSSOConfig()) + + req := httptest.NewRequest(http.MethodGet, "/api/security/sso/providers/corp-oidc", nil) + w := httptest.NewRecorder() + router.handleSSOProvider(w, req) + require.Equal(t, http.StatusOK, w.Code) + require.NotContains(t, w.Body.String(), "super-secret") + + var detail SSOProviderResponse + require.NoError(t, json.Unmarshal(w.Body.Bytes(), &detail)) + require.NotNil(t, detail.OIDC) + assert.Equal(t, "https://idp.example.com/realms/pulse", detail.OIDC.IssuerURL) + assert.Equal(t, "pulse-client", detail.OIDC.ClientID) + assert.Equal(t, "https://pulse.example.com/api/oidc/corp-oidc/callback", detail.OIDC.RedirectURL) + assert.Equal(t, "https://idp.example.com/logout", detail.OIDC.LogoutURL) + assert.Equal(t, []string{"openid", "profile", "email", "groups"}, detail.OIDC.Scopes) + assert.True(t, detail.OIDC.ClientSecretSet) + assert.Equal(t, "groups", detail.GroupsClaim) + assert.Equal(t, map[string]string{"admins": "admin"}, detail.GroupRoleMappings) + + body, err := json.Marshal(detail) + require.NoError(t, err) + req = httptest.NewRequest(http.MethodPut, "/api/security/sso/providers/corp-oidc", bytes.NewReader(body)) + w = httptest.NewRecorder() + router.handleSSOProvider(w, req) + require.Equal(t, http.StatusOK, w.Code) + + loadedConfig, err := router.persistence.LoadSSOConfig() + require.NoError(t, err) + stored := loadedConfig.GetProvider("corp-oidc") + require.NotNil(t, stored) + require.NotNil(t, stored.OIDC) + assert.Equal(t, "https://idp.example.com/realms/pulse", stored.OIDC.IssuerURL) + assert.Equal(t, "pulse-client", stored.OIDC.ClientID) + assert.Equal(t, "super-secret", stored.OIDC.ClientSecret) + assert.True(t, stored.OIDC.ClientSecretSet) + assert.Equal(t, "https://pulse.example.com/api/oidc/corp-oidc/callback", stored.OIDC.RedirectURL) + assert.Equal(t, "https://idp.example.com/logout", stored.OIDC.LogoutURL) + assert.Equal(t, []string{"openid", "profile", "email", "groups"}, stored.OIDC.Scopes) + assert.Equal(t, []string{"admins", "operators"}, stored.AllowedGroups) + assert.Equal(t, []string{"example.com"}, stored.AllowedDomains) + assert.Equal(t, []string{"owner@example.com"}, stored.AllowedEmails) + assert.Equal(t, "groups", stored.GroupsClaim) + assert.Equal(t, map[string]string{"admins": "admin"}, stored.GroupRoleMappings) +} + func TestCreateSSOProvider_Validation(t *testing.T) { router, _ := setupTestRouter(t)