diff --git a/internal/api/discovery_handlers.go b/internal/api/discovery_handlers.go index dbd221519..6ff505886 100644 --- a/internal/api/discovery_handlers.go +++ b/internal/api/discovery_handlers.go @@ -201,15 +201,16 @@ func (h *DiscoveryHandlers) isAdminRequest(r *http.Request) bool { } } - // 3. Check for configured admin session (OIDC/SAML/local session) + // 3. Check for an admin session (OIDC/SAML/local session). The admin test is + // sessionUserCarriesAdminPrivileges, the same one the settings routes apply, + // so an RBAC admin grant and the SSO-principal-with-no-local-admin case both + // count. Comparing against h.config.AuthUser alone cannot match on an + // instance whose only administrators are SSO principals. if cookie, err := readSessionCookie(r); err == nil && cookie.Value != "" { if ValidateSession(cookie.Value) { - configuredAdmin := strings.TrimSpace(h.config.AuthUser) - if configuredAdmin != "" { - sessionUser := strings.TrimSpace(GetSessionUsername(cookie.Value)) - if strings.EqualFold(sessionUser, configuredAdmin) { - return true - } + sessionUser := strings.TrimSpace(GetSessionUsername(cookie.Value)) + if sessionUserCarriesAdminPrivileges(h.config, sessionUser) { + return true } } } diff --git a/internal/api/oidc_only_admin_parity_test.go b/internal/api/oidc_only_admin_parity_test.go new file mode 100644 index 000000000..efc6a9feb --- /dev/null +++ b/internal/api/oidc_only_admin_parity_test.go @@ -0,0 +1,130 @@ +package api + +import ( + "net/http" + "net/http/httptest" + "strconv" + "strings" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/config" + "github.com/rcourtman/pulse-go-rewrite/pkg/auth" + "golang.org/x/crypto/bcrypt" +) + +func adminParitySession(t *testing.T, user string) *http.Cookie { + t.Helper() + tok := "admin-parity-" + strconv.FormatInt(time.Now().UnixNano(), 10) + GetSessionStore().CreateSession(tok, time.Hour, "browser", "127.0.0.1", user) + return &http.Cookie{Name: sessionCookieName(false), Value: tok} +} + +func adminParityConfig(t *testing.T, adminUser string) *config.Config { + t.Helper() + dir := t.TempDir() + // An API token is present so the export/import path treats auth as required + // on the OIDC-only instance too, which is the shape that actually 403s. + record, err := config.NewAPITokenRecord("admin-parity-token-123.12345678", "parity", []string{config.ScopeSettingsWrite}) + if err != nil { + t.Fatalf("NewAPITokenRecord: %v", err) + } + cfg := &config.Config{DataPath: dir, ConfigPath: dir, APITokens: []config.APITokenRecord{*record}} + if adminUser != "" { + hashed, err := bcrypt.GenerateFromPassword([]byte("admin-parity-password"), bcrypt.DefaultCost) + if err != nil { + t.Fatalf("bcrypt: %v", err) + } + cfg.AuthUser = adminUser + cfg.AuthPass = string(hashed) + } + return cfg +} + +func exportStatusFor(t *testing.T, router *Router, user string) int { + t.Helper() + cookie := adminParitySession(t, user) + req := httptest.NewRequest(http.MethodPost, "/api/config/export", + strings.NewReader(`{"passphrase":"long-enough-passphrase"}`)) + req.Header.Set("Content-Type", "application/json") + req.AddCookie(cookie) + req.Header.Set("X-CSRF-Token", generateCSRFToken(cookie.Value)) + rec := httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + return rec.Code +} + +// On the OIDC-only pattern there is no local admin, so SSO principals are the +// instance's administrators and ensureAdminSession admits them. Several guards +// compared the session user against cfg.AuthUser instead, which is empty here, +// so they could admit nobody at all and locked the operator out of discovery, +// public URL capture and their own config export. +func TestOIDCOnlyAdminReachesEveryAdminGuard(t *testing.T) { + prev := auth.GetAuthorizer() + auth.SetAuthorizer(&auth.DefaultAuthorizer{}) + defer auth.SetAuthorizer(prev) + + cfg := adminParityConfig(t, "") + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + ssoAdmin := "sso:owner@example.com" + + if !sessionUserCarriesAdminPrivileges(cfg, ssoAdmin) { + t.Fatal("precondition: the canonical helper must treat this principal as an admin") + } + + req := httptest.NewRequest(http.MethodGet, "/api/health", nil) + req.AddCookie(adminParitySession(t, ssoAdmin)) + if !canCapturePublicURL(cfg, req) { + t.Error("canCapturePublicURL refused the OIDC-only administrator") + } + + h := &DiscoveryHandlers{config: cfg} + req2 := httptest.NewRequest(http.MethodGet, "/api/discovery/status", nil) + req2.AddCookie(adminParitySession(t, ssoAdmin)) + if !h.isAdminRequest(req2) { + t.Error("discovery isAdminRequest refused the OIDC-only administrator") + } + + if code := exportStatusFor(t, router, ssoAdmin); code == http.StatusForbidden { + t.Error("config export refused the OIDC-only administrator") + } +} + +// The parity fix must not widen anything. On an instance that does configure a +// local admin, an unrelated SSO principal is not an administrator and every one +// of these guards must still refuse them. +func TestUnrelatedSSOUserStillRefusedWhenLocalAdminConfigured(t *testing.T) { + prev := auth.GetAuthorizer() + auth.SetAuthorizer(&auth.DefaultAuthorizer{}) + defer auth.SetAuthorizer(prev) + + cfg := adminParityConfig(t, "admin") + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + outsider := "sso:outsider@example.com" + + if sessionUserCarriesAdminPrivileges(cfg, outsider) { + t.Fatal("precondition: an unrelated SSO user must not be an admin when a local admin is configured") + } + + req := httptest.NewRequest(http.MethodGet, "/api/health", nil) + req.AddCookie(adminParitySession(t, outsider)) + if canCapturePublicURL(cfg, req) { + t.Error("canCapturePublicURL admitted a non-admin SSO user") + } + + h := &DiscoveryHandlers{config: cfg} + req2 := httptest.NewRequest(http.MethodGet, "/api/discovery/status", nil) + req2.AddCookie(adminParitySession(t, outsider)) + if h.isAdminRequest(req2) { + t.Error("discovery isAdminRequest admitted a non-admin SSO user") + } + + if code := exportStatusFor(t, router, outsider); code != http.StatusForbidden { + t.Errorf("config export for a non-admin SSO user = %d, want 403", code) + } + + // The configured admin is unaffected. + if code := exportStatusFor(t, router, "admin"); code == http.StatusForbidden { + t.Error("config export refused the configured local admin") + } +} diff --git a/internal/api/router.go b/internal/api/router.go index 0b0bec8e0..1d5968ce1 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -5007,16 +5007,17 @@ func canCapturePublicURL(cfg *config.Config, req *http.Request) bool { } } - // Session (Browser): allow capture only for the configured local admin session. - // This prevents low-privilege session users from poisoning public URL auto-detection. + // Session (Browser): allow capture only for an admin session. This prevents + // low-privilege session users from poisoning public URL auto-detection. + // The admin test is sessionUserCarriesAdminPrivileges, the same one the + // settings routes apply, rather than a local comparison against + // cfg.AuthUser: that comparison cannot match on an instance whose only + // administrators are SSO principals, so it locked those operators out. if cookie, err := readSessionCookie(req); err == nil && cookie.Value != "" { if ValidateSession(cookie.Value) { - adminUser := strings.TrimSpace(cfg.AuthUser) - if adminUser != "" { - username := strings.TrimSpace(GetSessionUsername(cookie.Value)) - if constantTimeStringEqual(strings.ToLower(username), strings.ToLower(adminUser)) { - return true - } + username := strings.TrimSpace(GetSessionUsername(cookie.Value)) + if sessionUserCarriesAdminPrivileges(cfg, username) { + return true } } } diff --git a/internal/api/router_routes_registration.go b/internal/api/router_routes_registration.go index b18894426..ef0bba6b6 100644 --- a/internal/api/router_routes_registration.go +++ b/internal/api/router_routes_registration.go @@ -437,8 +437,12 @@ func (r *Router) registerConfigSystemRoutes(updateHandlers *UpdateHandlers) { hasValidSession = ValidateSession(cookie.Value) if hasValidSession { sessionUsername = strings.TrimSpace(GetSessionUsername(cookie.Value)) - configuredAdmin := strings.TrimSpace(r.config.AuthUser) - sessionIsAdmin = configuredAdmin != "" && strings.EqualFold(sessionUsername, configuredAdmin) + // Same admin test the settings routes apply. Comparing + // against r.config.AuthUser alone cannot match on an + // instance whose only administrators are SSO principals, + // which locked those operators out of their own config + // export and import. + sessionIsAdmin = sessionUserCarriesAdminPrivileges(r.config, sessionUsername) } } @@ -567,8 +571,12 @@ func (r *Router) registerConfigSystemRoutes(updateHandlers *UpdateHandlers) { hasValidSession = ValidateSession(cookie.Value) if hasValidSession { sessionUsername = strings.TrimSpace(GetSessionUsername(cookie.Value)) - configuredAdmin := strings.TrimSpace(r.config.AuthUser) - sessionIsAdmin = configuredAdmin != "" && strings.EqualFold(sessionUsername, configuredAdmin) + // Same admin test the settings routes apply. Comparing + // against r.config.AuthUser alone cannot match on an + // instance whose only administrators are SSO principals, + // which locked those operators out of their own config + // export and import. + sessionIsAdmin = sessionUserCarriesAdminPrivileges(r.config, sessionUsername) } }