Fix OIDC provider detail persistence

Refs #1521
2026-07-09 16:00:59 +00:00 · 2026-07-06 11:18:18 +01:00 · 2026-07-06 11:18:18 +01:00 · caa9b41834
commit caa9b41834
parent fce4317176
6 changed files with 301 additions and 15 deletions
--- 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
--- 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
--- 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,
--- 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) {
--- 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
 // ============================================================================
--- 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)