From 28fd2d1c1584feebdb623f8fd85354d2631a91f7 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Wed, 5 Aug 2026 12:02:53 +0100 Subject: [PATCH] fix(api): stop advertising settings surfaces the routes refuse A settings capability is a promise the routes have to keep. Without an RBAC licence the authorizer allows every action, so capabilities derived from it alone reported true while the matching route stayed gated by ensureSettingsScope and in turn ensureAdminSession. canAccessPermissionSurface already refused to trust the authorizer for a non-admin proxy caller. The session half of that rule was never written, so a non-admin session on an unlicensed instance was told apiAccessRead, apiAccessWrite, singleSignOnRead and singleSignOnWrite were all available. The nav gates on exactly those flags, so the API Access and Single Sign-On tabs rendered, their first request came back 403, and the user got an error toast on a tab they were never able to use. Everything routed through canAccessAdminSurface was already correct, which is why authenticationRead and the audit surfaces behaved and these two did not. The fallback uses snapshot.sessionIsAdmin, which derives from the same sessionUserCarriesAdminPrivileges the routes enforce, so the capability now matches the answer the route will give. That keeps the OIDC-only pattern working, where SSO principals are the instance's only administrators. Nothing was reachable that should not have been. This is a capability reporting fix, not an access control one. Refs #1672 Contract-Neutral: settingsCapabilities JSON shape is unchanged (same 14 fields, same types); this corrects a wrong value returned to non-admin sessions when no RBAC authorizer is registered, no public-contract delta --- internal/api/security_status_capabilities.go | 23 ++- ...rity_status_capability_enforcement_test.go | 141 ++++++++++++++++++ 2 files changed, 161 insertions(+), 3 deletions(-) create mode 100644 internal/api/security_status_capability_enforcement_test.go diff --git a/internal/api/security_status_capabilities.go b/internal/api/security_status_capabilities.go index 41a88112d..34eab366e 100644 --- a/internal/api/security_status_capabilities.go +++ b/internal/api/security_status_capabilities.go @@ -182,9 +182,26 @@ func (r *Router) canAccessPermissionSurface(snapshot securityStatusAuthSnapshot, return false } - if snapshot.authMethod == "proxy" && !snapshot.proxyIsAdmin { - if _, isDefaultAuthorizer := r.authorizer.(*internalauth.DefaultAuthorizer); isDefaultAuthorizer { - return false + // Without a real RBAC authorizer, Authorize allows every action, so it + // cannot be the sole input to a capability. The routes these capabilities + // describe are still gated, by ensureSettingsScope and in turn + // ensureAdminSession, so reporting the capability from the authorizer alone + // advertises a surface the caller will be refused: the tab renders and its + // first request comes back 403. Fall back to the same admin identity the + // routes enforce, which snapshot.sessionIsAdmin already derives from + // sessionUserCarriesAdminPrivileges. Only the proxy half of this rule was + // ever written, so session and SSO callers were told they could reach API + // token management and SSO provider configuration when they could not. + if _, isDefaultAuthorizer := r.authorizer.(*internalauth.DefaultAuthorizer); isDefaultAuthorizer { + switch snapshot.authMethod { + case "proxy": + if !snapshot.proxyIsAdmin { + return false + } + case "session": + if !snapshot.sessionIsAdmin { + return false + } } } diff --git a/internal/api/security_status_capability_enforcement_test.go b/internal/api/security_status_capability_enforcement_test.go new file mode 100644 index 000000000..0a9d047ae --- /dev/null +++ b/internal/api/security_status_capability_enforcement_test.go @@ -0,0 +1,141 @@ +package api + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strconv" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/config" + "github.com/rcourtman/pulse-go-rewrite/pkg/auth" + "golang.org/x/crypto/bcrypt" +) + +func newCapabilityConfig(t *testing.T, adminUser string) *config.Config { + t.Helper() + tempDir := t.TempDir() + cfg := &config.Config{DataPath: tempDir, ConfigPath: tempDir} + if adminUser != "" { + hashed, err := bcrypt.GenerateFromPassword([]byte("capability-password"), bcrypt.DefaultCost) + if err != nil { + t.Fatalf("bcrypt: %v", err) + } + cfg.AuthUser = adminUser + cfg.AuthPass = string(hashed) + } + return cfg +} + +func capabilitySessionCookie(t *testing.T, username string) *http.Cookie { + t.Helper() + token := "capability-session-" + strconv.FormatInt(time.Now().UnixNano(), 10) + GetSessionStore().CreateSession(token, time.Hour, "browser", "127.0.0.1", username) + return &http.Cookie{Name: sessionCookieName(false), Value: token} +} + +func fetchSettingsCapabilities(t *testing.T, router *Router, cookie *http.Cookie) map[string]any { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/api/security/status", nil) + req.AddCookie(cookie) + rec := httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("security status = %d, want 200", rec.Code) + } + var payload map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &payload); err != nil { + t.Fatalf("unmarshal security status: %v", err) + } + caps, ok := payload["settingsCapabilities"].(map[string]any) + if !ok { + t.Fatalf("settingsCapabilities missing from %s", rec.Body.String()) + } + return caps +} + +// A settings capability is a promise the routes have to keep. Without an RBAC +// licence Authorize allows everything, so capabilities derived from it alone +// reported true while ensureSettingsScope refused the matching request, and the +// API Access and Single Sign-On tabs rendered for users who then got a 403 and +// an error toast. Refs discussion #1672. +func TestSettingsCapabilitiesMatchRouteEnforcementWithoutRBAC(t *testing.T) { + prevAuthorizer := auth.GetAuthorizer() + auth.SetAuthorizer(&auth.DefaultAuthorizer{}) + defer auth.SetAuthorizer(prevAuthorizer) + + cfg := newCapabilityConfig(t, "admin") + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + + caps := fetchSettingsCapabilities(t, router, capabilitySessionCookie(t, "sso:outsider@example.com")) + for _, key := range []string{ + "apiAccessRead", + "apiAccessWrite", + "singleSignOnRead", + "singleSignOnWrite", + "authenticationRead", + "authenticationWrite", + } { + if caps[key] != false { + t.Fatalf("%s = %v for a non-admin session, want false", key, caps[key]) + } + } + + // Every capability the non-admin was refused must in fact be refused by the + // route, otherwise the fix has swung into hiding a usable surface. + for _, probe := range []struct{ method, path string }{ + {http.MethodGet, "/api/security/tokens"}, + {http.MethodGet, "/api/security/sso/providers"}, + } { + req := httptest.NewRequest(probe.method, probe.path, nil) + req.AddCookie(capabilitySessionCookie(t, "sso:outsider@example.com")) + rec := httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + if rec.Code != http.StatusForbidden { + t.Fatalf("%s %s = %d, want 403 to match the withheld capability", probe.method, probe.path, rec.Code) + } + } +} + +// The configured admin must keep every surface, or the fix hides settings from +// the only account that can reach them. +func TestSettingsCapabilitiesGrantConfiguredAdminWithoutRBAC(t *testing.T) { + prevAuthorizer := auth.GetAuthorizer() + auth.SetAuthorizer(&auth.DefaultAuthorizer{}) + defer auth.SetAuthorizer(prevAuthorizer) + + cfg := newCapabilityConfig(t, "admin") + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + + caps := fetchSettingsCapabilities(t, router, capabilitySessionCookie(t, "admin")) + for _, key := range []string{"apiAccessRead", "apiAccessWrite", "singleSignOnRead", "singleSignOnWrite"} { + if caps[key] != true { + t.Fatalf("%s = %v for the configured admin, want true", key, caps[key]) + } + } +} + +// On the OIDC-only pattern there is no local admin, so SSO principals are the +// instance's administrators. They must keep the tabs, and the routes agree. +func TestSettingsCapabilitiesGrantSSOAdminWhenNoLocalAdminConfigured(t *testing.T) { + prevAuthorizer := auth.GetAuthorizer() + auth.SetAuthorizer(&auth.DefaultAuthorizer{}) + defer auth.SetAuthorizer(prevAuthorizer) + + cfg := newCapabilityConfig(t, "") + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + + caps := fetchSettingsCapabilities(t, router, capabilitySessionCookie(t, "sso:owner@example.com")) + if caps["apiAccessRead"] != true { + t.Fatalf("apiAccessRead = %v on an OIDC-only instance, want true", caps["apiAccessRead"]) + } + + req := httptest.NewRequest(http.MethodGet, "/api/security/tokens", nil) + req.AddCookie(capabilitySessionCookie(t, "sso:owner@example.com")) + rec := httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + if rec.Code == http.StatusForbidden { + t.Fatal("OIDC-only instance must not refuse its own SSO administrator") + } +}