From bcdc0ef38510be4793e452b7fb0573bb5e62b86c Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 31 Aug 2026 04:55:47 +0000 Subject: [PATCH] =?UTF-8?q?fix(admin):=20owner=20gate=20consumes=20the=20c?= =?UTF-8?q?ontext=20role=20=E2=80=94=20no=20second=20lookup=20(OC-0379)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OC-0345's fix kept the redundant GetRoleByID its own title named (its suggestedFix says why: reading adminRoleKey broke two tests that injected only the user). Finish the job: ownerOnlyMiddleware now consumes the *db.Role adminAuthMiddleware stored, exactly like requirePerm — missing role fails closed as 401, position below Owner stays 403, and the query plus its 503 branch are gone (a role read fault surfaces once, at the perimeter). The signature drops *db.DB at all nine call sites, so a reintroduced lookup is a compile-visible change. Test-first: TestOwnerOnlyMiddleware_NoSecondRoleLookup renames the roles table away with the role in context and demands 200 — red 503 against the old code, green now. The old RoleLookupFailureIs503 test guarded a branch that no longer exists in any form; a tombstone comment records where its contract lives on (the perimeter's default branch). Added below-owner 403 and user-without-role 401 pins; the blackbox owner-route tests pass unchanged. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01B8dwVLEihnGZYtH9X631F4 --- Server/admin/api.go | 18 +-- Server/admin/middleware.go | 31 ++--- Server/admin/middleware_and_spawn_test.go | 137 ++++++++++++++-------- 3 files changed, 105 insertions(+), 81 deletions(-) diff --git a/Server/admin/api.go b/Server/admin/api.go index 2840c275..93545061 100644 --- a/Server/admin/api.go +++ b/Server/admin/api.go @@ -147,13 +147,13 @@ func NewAdminAPI(database *db.DB, version string, hub HubBroadcaster, u *updater // API tokens — Owner-only. Minting a network-reachable, revocation- // surviving bearer credential is gated like backups/updates. r.Get("/tokens", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - ownerOnlyMiddleware(database, handleListAPITokens(database)).ServeHTTP(w, req) + ownerOnlyMiddleware(handleListAPITokens(database)).ServeHTTP(w, req) })) r.Post("/tokens", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - ownerOnlyMiddleware(database, handleCreateAPIToken(database)).ServeHTTP(w, req) + ownerOnlyMiddleware(handleCreateAPIToken(database)).ServeHTTP(w, req) })) r.Delete("/tokens/{id}", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - ownerOnlyMiddleware(database, handleRevokeAPIToken(database)).ServeHTTP(w, req) + ownerOnlyMiddleware(handleRevokeAPIToken(database)).ServeHTTP(w, req) })) r.Group(func(r chi.Router) { r.Use(requirePerm(permissions.ManageServer)) @@ -161,22 +161,22 @@ func NewAdminAPI(database *db.DB, version string, hub HubBroadcaster, u *updater r.Patch("/settings", handlePatchSettings(database)) }) r.Post("/backup", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - ownerOnlyMiddleware(database, handleBackup(database)).ServeHTTP(w, req) + ownerOnlyMiddleware(handleBackup(database)).ServeHTTP(w, req) })) r.Get("/backups", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - ownerOnlyMiddleware(database, handleListBackups()).ServeHTTP(w, req) + ownerOnlyMiddleware(handleListBackups()).ServeHTTP(w, req) })) r.Delete("/backups/{name}", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - ownerOnlyMiddleware(database, handleDeleteBackup(database)).ServeHTTP(w, req) + ownerOnlyMiddleware(handleDeleteBackup(database)).ServeHTTP(w, req) })) r.Post("/backups/{name}/restore", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - ownerOnlyMiddleware(database, handleRestoreBackup(database, hub)).ServeHTTP(w, req) + ownerOnlyMiddleware(handleRestoreBackup(database, hub)).ServeHTTP(w, req) })) r.Get("/updates", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - ownerOnlyMiddleware(database, handleCheckUpdate(u)).ServeHTTP(w, req) + ownerOnlyMiddleware(handleCheckUpdate(u)).ServeHTTP(w, req) })) r.Post("/updates/apply", http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - ownerOnlyMiddleware(database, handleApplyUpdate(u, hub, version)).ServeHTTP(w, req) + ownerOnlyMiddleware(handleApplyUpdate(u, hub, version)).ServeHTTP(w, req) })) }) diff --git a/Server/admin/middleware.go b/Server/admin/middleware.go index 5da213b9..b0300148 100644 --- a/Server/admin/middleware.go +++ b/Server/admin/middleware.go @@ -116,31 +116,22 @@ func requirePerm(perm int64) func(http.Handler) http.Handler { } } -// ownerOnlyMiddleware wraps a handler to require Owner role (position == 100). -// It reads the user from context (set by adminAuthMiddleware) rather than -// re-authenticating, avoiding redundant DB queries and session-expiry gaps. -func ownerOnlyMiddleware(database *db.DB, next http.Handler) http.Handler { +// ownerOnlyMiddleware wraps a handler to require the Owner role +// (position == permissions.OwnerRolePosition). It consumes the *db.Role that +// adminAuthMiddleware resolved and stored in the request context — the same +// contract as requirePerm, so no second role read runs and no read-fault +// error mapping exists here at all: OC-0345's 503 branch died with the query +// it served, and OC-0379 pins the absence (a role read fault now surfaces +// once, at the perimeter, as its 503). A request that somehow arrives without +// the context role fails closed as unauthenticated. +func ownerOnlyMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - user, ok := r.Context().Value(adminUserKey).(*db.User) - if !ok || user == nil { + role, ok := r.Context().Value(adminRoleKey).(*db.Role) + if !ok || role == nil { writeErr(w, http.StatusUnauthorized, "UNAUTHORIZED", "not authenticated") return } - role, err := database.GetRoleByID(r.Context(), user.RoleID) - if err != nil { - // A read fault is an outage, not a missing role: answering 403 - // would tell the Owner they lack the Owner role. Mirror the - // perimeter's contract above — log it, report 503 (OC-0345). - slog.ErrorContext(r.Context(), "admin: owner role lookup failed", "error", err) - writeErr(w, http.StatusServiceUnavailable, "SERVICE_UNAVAILABLE", "authorization service temporarily unavailable") - return - } - if role == nil { - writeErr(w, http.StatusForbidden, "FORBIDDEN", "role not found") - return - } - if role.Position < permissions.OwnerRolePosition { writeErr(w, http.StatusForbidden, "FORBIDDEN", "owner role required") return diff --git a/Server/admin/middleware_and_spawn_test.go b/Server/admin/middleware_and_spawn_test.go index b856ab54..60c8ca7d 100644 --- a/Server/admin/middleware_and_spawn_test.go +++ b/Server/admin/middleware_and_spawn_test.go @@ -138,38 +138,42 @@ CREATE TABLE IF NOT EXISTS invites ( // ─── ownerOnlyMiddleware whitebox tests ────────────────────────────────────── // TestOwnerOnlyMiddleware_NoUserInContext verifies that ownerOnlyMiddleware -// returns 401 when there is no user stored in the request context. +// returns 401 when the request context carries no authenticated principal — +// simulates a call bypassing adminAuthMiddleware, which is what stores the +// role the gate consumes. func TestOwnerOnlyMiddleware_NoUserInContext(t *testing.T) { - database := openWhiteboxTestDB(t) - reached := false next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { reached = true w.WriteHeader(http.StatusOK) }) - handler := ownerOnlyMiddleware(database, next) + handler := ownerOnlyMiddleware(next) - // Request with NO user in context — simulates a call bypassing adminAuthMiddleware. req := httptest.NewRequest(http.MethodPost, "/backup", nil) w := httptest.NewRecorder() handler.ServeHTTP(w, req) if reached { - t.Error("next handler was reached despite missing user in context") + t.Error("next handler was reached despite an unauthenticated context") } if w.Code != http.StatusUnauthorized { t.Errorf("status = %d, want 401", w.Code) } } -// TestOwnerOnlyMiddleware_RoleNotFound verifies that ownerOnlyMiddleware -// returns 403 when the user's role_id does not exist in the database. -func TestOwnerOnlyMiddleware_RoleNotFound(t *testing.T) { +// TestOwnerOnlyMiddleware_UserWithoutRoleInContext verifies the gate fails +// closed as 401 when the context carries a user but no role. Through the full +// stack this cannot happen — adminAuthMiddleware stores both or refuses the +// request (a genuinely missing role is its 401, a role read fault its 503) — +// so a half-populated context means the perimeter did not run, and the gate +// must treat that as unauthenticated rather than consult the database itself. +// (The pre-OC-0379 middleware answered this shape by re-reading the role; the +// old TestOwnerOnlyMiddleware_RoleNotFound covered that lookup's miss, a +// branch that no longer exists.) +func TestOwnerOnlyMiddleware_UserWithoutRoleInContext(t *testing.T) { database := openWhiteboxTestDB(t) - // Create a user initially with a valid role, then mutate role_id to a - // nonexistent value (disabling FK checks temporarily so SQLite allows it). uid, err := database.CreateUser(context.Background(), "orphanuser", "$2a$12$x", 1) if err != nil { t.Fatalf("CreateUser: %v", err) @@ -179,45 +183,60 @@ func TestOwnerOnlyMiddleware_RoleNotFound(t *testing.T) { t.Fatalf("GetUserByID: %v", err) } - // Disable FK enforcement, update role_id, re-enable. - if _, err := database.ExecContext(context.Background(), `PRAGMA foreign_keys=OFF`); err != nil { - t.Fatalf("disable FK: %v", err) - } - if _, err := database.ExecContext(context.Background(), `UPDATE users SET role_id = 9999 WHERE id = ?`, uid); err != nil { - t.Fatalf("UPDATE role_id: %v", err) - } - if _, err := database.ExecContext(context.Background(), `PRAGMA foreign_keys=ON`); err != nil { - t.Fatalf("re-enable FK: %v", err) - } - user.RoleID = 9999 // mirror the DB value in our in-memory struct - reached := false next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { reached = true w.WriteHeader(http.StatusOK) }) - handler := ownerOnlyMiddleware(database, next) + handler := ownerOnlyMiddleware(next) - // Inject user into context as adminAuthMiddleware would. + // User injected, role deliberately absent. ctx := context.WithValue(context.Background(), adminUserKey, user) req := httptest.NewRequest(http.MethodPost, "/backup", nil).WithContext(ctx) w := httptest.NewRecorder() handler.ServeHTTP(w, req) if reached { - t.Error("next handler was reached despite missing role") + t.Error("next handler was reached despite no role in context") } - if w.Code != http.StatusForbidden { - t.Errorf("status = %d, want 403 (role not found)", w.Code) + if w.Code != http.StatusUnauthorized { + t.Errorf("status = %d, want 401 (no role in context is unauthenticated)", w.Code) } var resp map[string]string if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { t.Fatalf("unmarshal: %v", err) } - if resp["error"] != "FORBIDDEN" { - t.Errorf("error = %q, want FORBIDDEN", resp["error"]) + if resp["error"] != "UNAUTHORIZED" { + t.Errorf("error = %q, want UNAUTHORIZED", resp["error"]) + } +} + +// TestOwnerOnlyMiddleware_BelowOwnerForbidden verifies a role below the Owner +// position is refused with 403 "owner role required". The role comes straight +// from the context — the middleware reads nothing else, so a plain struct is +// the whole setup. +func TestOwnerOnlyMiddleware_BelowOwnerForbidden(t *testing.T) { + reached := false + next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + reached = true + w.WriteHeader(http.StatusOK) + }) + + handler := ownerOnlyMiddleware(next) + + mod := &db.Role{ID: 2, Name: "Moderator", Position: 50} + ctx := context.WithValue(context.Background(), adminRoleKey, mod) + req := httptest.NewRequest(http.MethodPost, "/backup", nil).WithContext(ctx) + w := httptest.NewRecorder() + handler.ServeHTTP(w, req) + + if reached { + t.Error("next handler was reached for a below-owner role") + } + if w.Code != http.StatusForbidden { + t.Errorf("status = %d, want 403 (owner role required)", w.Code) } } @@ -234,6 +253,10 @@ func TestOwnerOnlyMiddleware_OwnerPassesThrough(t *testing.T) { if err != nil || user == nil { t.Fatalf("GetUserByID: %v", err) } + role, err := database.GetRoleByID(context.Background(), user.RoleID) + if err != nil || role == nil { + t.Fatalf("GetRoleByID: %v", err) + } reached := false next := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -241,9 +264,11 @@ func TestOwnerOnlyMiddleware_OwnerPassesThrough(t *testing.T) { w.WriteHeader(http.StatusOK) }) - handler := ownerOnlyMiddleware(database, next) + handler := ownerOnlyMiddleware(next) + // Both keys, as adminAuthMiddleware stores them. ctx := context.WithValue(context.Background(), adminUserKey, user) + ctx = context.WithValue(ctx, adminRoleKey, role) req := httptest.NewRequest(http.MethodPost, "/backup", nil).WithContext(ctx) w := httptest.NewRecorder() handler.ServeHTTP(w, req) @@ -523,16 +548,26 @@ func TestSpawnDetached_CommandConstruction(t *testing.T) { } } -// TestOwnerOnlyMiddleware_RoleLookupFailureIs503 pins OC-0345: a database -// fault on the owner gate's role read is an outage, not a missing role, so the -// Owner must get 503 SERVICE_UNAVAILABLE — never the 403 "role not found" a -// genuinely absent role earns. Whitebox on purpose: through the full stack -// adminAuthMiddleware reads the role first and would answer its own 503, so -// the branch under test would never run. -func TestOwnerOnlyMiddleware_RoleLookupFailureIs503(t *testing.T) { +// TestOwnerOnlyMiddleware_RoleLookupFailureIs503 pinned OC-0345's 503 mapping +// on the owner gate's own role read. OC-0379 removed that read entirely — the +// gate consumes adminRoleKey and issues no query, so the branch this test +// exercised no longer exists in any form. The 503-on-read-fault contract it +// protected still holds where the one remaining role read lives: the +// perimeter's default branch in adminAuthMiddleware (middleware.go), covered +// by its own tests. TestOwnerOnlyMiddleware_NoSecondRoleLookup below is the +// replacement pin: it renames the roles table away and requires the owner +// path to succeed anyway. + +// TestOwnerOnlyMiddleware_NoSecondRoleLookup pins OC-0379 (OC-0345's residue): +// the owner gate consumes the role adminAuthMiddleware already resolved into +// the request context and performs no role read of its own. The roles table is +// renamed away exactly as the OC-0345 fault test did — if the middleware still +// issues a role query, that query fails and the request cannot reach 200, so +// this test is red for as long as the second lookup exists. +func TestOwnerOnlyMiddleware_NoSecondRoleLookup(t *testing.T) { database := openWhiteboxTestDB(t) - uid, err := database.CreateUser(context.Background(), "ownerfault", "$2a$12$x", 1) + uid, err := database.CreateUser(context.Background(), "ownerctx", "$2a$12$x", 1) if err != nil { t.Fatalf("CreateUser: %v", err) } @@ -540,7 +575,11 @@ func TestOwnerOnlyMiddleware_RoleLookupFailureIs503(t *testing.T) { if err != nil || user == nil { t.Fatalf("GetUserByID: %v", err) } - // Every query against roles now fails with a non-sentinel error. + role, err := database.GetRoleByID(context.Background(), user.RoleID) + if err != nil || role == nil { + t.Fatalf("GetRoleByID: %v", err) + } + // After this, any role read fails: the only way to 200 is the context role. if _, err := database.ExecContext(context.Background(), `ALTER TABLE roles RENAME TO roles_gone`); err != nil { t.Fatalf("hide roles: %v", err) } @@ -550,24 +589,18 @@ func TestOwnerOnlyMiddleware_RoleLookupFailureIs503(t *testing.T) { reached = true w.WriteHeader(http.StatusOK) }) - handler := ownerOnlyMiddleware(database, next) + handler := ownerOnlyMiddleware(next) ctx := context.WithValue(context.Background(), adminUserKey, user) + ctx = context.WithValue(ctx, adminRoleKey, role) req := httptest.NewRequest(http.MethodPost, "/backup", nil).WithContext(ctx) w := httptest.NewRecorder() handler.ServeHTTP(w, req) - if reached { - t.Error("next handler was reached although the role could not be read") + if !reached { + t.Error("next handler was not reached: the owner gate performed a role lookup instead of consuming the context role") } - if w.Code != http.StatusServiceUnavailable { - t.Errorf("status = %d, want 503 (a role read fault is not a missing role)", w.Code) - } - var resp map[string]string - if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { - t.Fatalf("unmarshal: %v", err) - } - if resp["error"] != "SERVICE_UNAVAILABLE" { - t.Errorf("error = %q, want SERVICE_UNAVAILABLE", resp["error"]) + if w.Code != http.StatusOK { + t.Errorf("status = %d, want 200 with no role read", w.Code) } }