From d01bbf6bf1fc36ea7f6c7cc46baafb2f0f222efd Mon Sep 17 00:00:00 2001 From: xarmian Date: Sat, 2 May 2026 14:29:17 -0400 Subject: [PATCH] feat(oauth): live workspace allow-list + role enforcement (TASK-953) (#377) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the third leg of PLAN-943's OAuth permission model: (token capability tier) × (live workspace role) × (consent allow-list) The first two were already in place — TASK-1027 wired the tier scope check (pad:read / pad:write / pad:admin via tokenScopeAllows) and RequireWorkspaceAccess does the live role lookup. This PR adds the third gate: the workspace-allow-list set at consent time (TASK-952) actually denies workspaces NOT in the user's selection. ## What's new - `oauth.Session.AllowedWorkspaces()` / `SetAllowedWorkspaces()` — typed accessors on session.Extra. Handle BOTH the in-memory []string shape (consent-decide path) AND the JSON-decoded []interface{} shape (post-storage round-trip path). - `WithTokenAllowedWorkspaces` / `TokenAllowedWorkspacesFromContext` — context helpers in internal/server with defensive copies so callers can't corrupt the per-request token state. - `MCPBearerAuth` (OAuth path) reads the token's allow-list from session.Extra and stashes it in context. - `RequireWorkspaceAccess` checks the allow-list against the resolved workspace's slug. Three behaviours match TokenAllowedWorkspacesFromContext's return shapes: - nil → no token-level gate (PAT auth, pre-TASK-952 OAuth tokens). Standard membership applies. - ["*"] → wildcard. Every membership the user has passes. - [slug-a, slug-b, ...] → only listed slugs. Anything else gets 403 permission_denied BEFORE the membership check. ## Live role + revocation Membership revocation takes effect immediately. RequireWorkspaceAccess calls GetWorkspaceMember on every request — if the user lost membership in workspace X, the token's allow-list including X no longer helps; the request is rejected at the standard membership gate. Tested explicitly via TestWorkspaceAllowList_LiveMembershipRevocation. ## Tier × role The natural intersection of tokenScopeAllows (tier-based HTTP-method gate) and per-handler role checks (e.g. requireEditPermission) handles the tier × role table from the PLAN-943 spec: - pad:write tier passes tokenScopeAllows for POST. - But Viewer role fails requireEditPermission's role check. - Net: 403 — tested explicitly via TestWorkspaceAllowList_TierTimesRole_WriteByViewer. ## Tests Unit (no I/O): - TestTokenAllowedWorkspaceMatches — policy table for the helper. - TestWithTokenAllowedWorkspaces_DefensiveCopy + 1 reader counterpart. - TestSession_AllowedWorkspaces_*: setter/getter, nil-clear, defensive copy, JSON round-trip ([]string + []interface{} branches), wildcard JSON round-trip, not-set, nil-session. Integration (full chain, real OAuth flow): - TestWorkspaceAllowList_AllowsListedSlug — listed workspace passes. - TestWorkspaceAllowList_DeniesUnlistedSlug — unlisted gets 403 even though user is owner. - TestWorkspaceAllowList_WildcardAllowsAnyMembership — wildcard passes for every membership. - TestWorkspaceAllowList_LiveMembershipRevocation — token works, then membership revoked, then same token denied. - TestWorkspaceAllowList_PATPathUnaffected — PAT regression: PATs don't carry an allow-list, must NOT hit the gate. - TestWorkspaceAllowList_TierTimesRole_WriteByViewer — pad:write tier × Viewer role on POST item → 403. --- internal/oauth/session.go | 85 ++++ internal/oauth/session_test.go | 123 ++++++ internal/server/context.go | 51 +++ internal/server/handlers_mcp_test.go | 412 ++++++++++++++++++ internal/server/handlers_oauth.go | 2 +- internal/server/middleware_auth.go | 65 +++ internal/server/middleware_mcp_auth.go | 23 + .../server/token_workspace_allowlist_test.go | 108 +++++ 8 files changed, 868 insertions(+), 1 deletion(-) create mode 100644 internal/oauth/session_test.go create mode 100644 internal/server/token_workspace_allowlist_test.go diff --git a/internal/oauth/session.go b/internal/oauth/session.go index 42eac07c..7acb2247 100644 --- a/internal/oauth/session.go +++ b/internal/oauth/session.go @@ -67,6 +67,91 @@ func (s *Session) UserID() string { return s.DefaultSession.Subject } +// allowedWorkspacesExtraKey is the session.Extra map key under which +// the consent UI (TASK-952) stores the workspace allow-list. Defined +// as a package constant so producers (handlers_oauth.go's +// /authorize/decide) and consumers (TASK-953's MCPBearerAuth gate) +// agree on the wire form. +const allowedWorkspacesExtraKey = "allowed_workspaces" + +// AllowedWorkspaces returns the workspace allow-list stored in +// session.Extra at consent time (TASK-952). Three return shapes +// matter to callers: +// +// - nil — no allow-list set. Either a non-OAuth session (PAT auth +// never goes through this code path) or a pre-TASK-952 token +// issued before the consent UI shipped. Callers should treat +// this as "no token-level workspace constraint" and rely on the +// standard membership gate. +// - []string{"*"} — wildcard. The user explicitly granted access +// to any workspace they currently or later have access to; +// standard membership still applies, no extra restriction. +// - []string{"slug-a", "slug-b", ...} — explicit allow-list. Each +// workspace request must hit a slug in this set OR be denied +// before the membership check runs. +// +// JSON round-trip handling: fosite serializes session.Extra via +// json.Marshal and deserializes back through json.Unmarshal into a +// map[string]interface{}. After a round-trip the value is +// []interface{} (Go's untyped JSON array shape), not []string. +// We accept both so the helper works whether the session was just +// created in memory (handlers_oauth.go's decide flow) or hydrated +// from storage (sub-PR D's introspection path). +func (s *Session) AllowedWorkspaces() []string { + if s == nil || s.DefaultSession == nil || s.DefaultSession.Extra == nil { + return nil + } + raw, ok := s.DefaultSession.Extra[allowedWorkspacesExtraKey] + if !ok { + return nil + } + switch v := raw.(type) { + case []string: + out := make([]string, len(v)) + copy(out, v) + return out + case []interface{}: + out := make([]string, 0, len(v)) + for _, e := range v { + if s, ok := e.(string); ok && s != "" { + out = append(out, s) + } + } + // Distinguish "explicit empty list" from "no key set" — the + // former is unusual but if a future change persists []string{} + // for some reason we don't want a phantom nil meaning "no + // constraint." Return non-nil empty so callers see "list is + // set, but contains nothing"; current MCPBearerAuth would + // reject the request as no workspace can match. + if out == nil { + out = []string{} + } + return out + } + return nil +} + +// SetAllowedWorkspaces is the symmetric writer used by +// /oauth/authorize/decide (TASK-952). Centralizing the key string +// here keeps producers + consumers in sync; nil clears the entry. +func (s *Session) SetAllowedWorkspaces(workspaces []string) { + if s == nil || s.DefaultSession == nil { + return + } + if s.DefaultSession.Extra == nil { + s.DefaultSession.Extra = map[string]interface{}{} + } + if workspaces == nil { + delete(s.DefaultSession.Extra, allowedWorkspacesExtraKey) + return + } + // Defensive copy — caller mutating the slice after the call + // shouldn't bleed through into the persisted session. + cp := make([]string, len(workspaces)) + copy(cp, workspaces) + s.DefaultSession.Extra[allowedWorkspacesExtraKey] = cp +} + // Clone overrides DefaultSession.Clone so the returned value is a // *Session, not a *DefaultSession. Without this override, fosite's // internal Clone() calls during refresh-token rotation would lose the diff --git a/internal/oauth/session_test.go b/internal/oauth/session_test.go new file mode 100644 index 00000000..8504ddf1 --- /dev/null +++ b/internal/oauth/session_test.go @@ -0,0 +1,123 @@ +package oauth + +import ( + "encoding/json" + "testing" + + "github.com/ory/fosite" +) + +// TestSession_AllowedWorkspaces_Setter_Getter pins the in-memory +// round-trip: SetAllowedWorkspaces([...]) then AllowedWorkspaces() +// returns the same list. This is the path /oauth/authorize/decide +// uses immediately after consent (TASK-952) before the session is +// JSON-serialized into storage. +func TestSession_AllowedWorkspaces_Setter_Getter(t *testing.T) { + s := NewSession("user-1") + s.SetAllowedWorkspaces([]string{"alpha", "beta"}) + + got := s.AllowedWorkspaces() + if len(got) != 2 || got[0] != "alpha" || got[1] != "beta" { + t.Errorf("round-trip failed: got %v, want [alpha beta]", got) + } +} + +// TestSession_AllowedWorkspaces_NilInput pins the "clear" semantics: +// SetAllowedWorkspaces(nil) removes the entry from Extra so a +// subsequent AllowedWorkspaces() returns nil (rather than the +// previous value or an empty slice). +func TestSession_AllowedWorkspaces_NilInput(t *testing.T) { + s := NewSession("user-1") + s.SetAllowedWorkspaces([]string{"alpha"}) + s.SetAllowedWorkspaces(nil) + + if got := s.AllowedWorkspaces(); got != nil { + t.Errorf("nil set should clear; got %v", got) + } +} + +// TestSession_AllowedWorkspaces_DefensiveCopy verifies the setter +// copies the input — caller-side mutation of the slice after the +// call must not bleed into the persisted session. +func TestSession_AllowedWorkspaces_DefensiveCopy(t *testing.T) { + s := NewSession("user-1") + original := []string{"alpha", "beta"} + s.SetAllowedWorkspaces(original) + + original[0] = "MUTATED" + + got := s.AllowedWorkspaces() + if got[0] != "alpha" { + t.Errorf("setter should defensive-copy; got[0] = %q after caller mutation", got[0]) + } +} + +// TestSession_AllowedWorkspaces_JSONRoundTrip pins the critical +// integration path: the value survives JSON marshal+unmarshal +// (which is what storage.go's session_data column round-trips +// through). After unmarshal, json.Unmarshal turns []string into +// []interface{}; the AllowedWorkspaces accessor MUST handle both +// shapes. +// +// This is why the session helper does the type-switch — production +// reads via MCPBearerAuth's introspection branch always go through +// JSON deserialization, and would silently fail to read the +// workspace allow-list without []interface{} support. +func TestSession_AllowedWorkspaces_JSONRoundTrip(t *testing.T) { + src := NewSession("user-1") + src.SetAllowedWorkspaces([]string{"alpha", "beta"}) + + bytes, err := json.Marshal(src) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + dst := &Session{DefaultSession: &fosite.DefaultSession{}} + if err := json.Unmarshal(bytes, dst); err != nil { + t.Fatalf("unmarshal: %v", err) + } + + got := dst.AllowedWorkspaces() + if len(got) != 2 || got[0] != "alpha" || got[1] != "beta" { + t.Errorf("JSON round-trip lost values: got %v, want [alpha beta]", got) + } +} + +// TestSession_AllowedWorkspaces_WildcardJSONRoundTrip pins the +// wildcard form — same JSON path, but the value is the single- +// element ["*"] form the consent UI emits when the user picks +// "any workspace". +func TestSession_AllowedWorkspaces_WildcardJSONRoundTrip(t *testing.T) { + src := NewSession("user-1") + src.SetAllowedWorkspaces([]string{"*"}) + + bytes, _ := json.Marshal(src) + dst := &Session{DefaultSession: &fosite.DefaultSession{}} + _ = json.Unmarshal(bytes, dst) + + got := dst.AllowedWorkspaces() + if len(got) != 1 || got[0] != "*" { + t.Errorf("wildcard round-trip: got %v, want [*]", got) + } +} + +// TestSession_AllowedWorkspaces_NotSet covers the legacy / +// pre-TASK-952 path: a session without an allowed_workspaces key +// must return nil so RequireWorkspaceAccess treats it as "no +// token-level constraint" rather than "explicit empty list". +func TestSession_AllowedWorkspaces_NotSet(t *testing.T) { + s := NewSession("user-1") + if got := s.AllowedWorkspaces(); got != nil { + t.Errorf("session with no allow-list set should return nil; got %v", got) + } +} + +// TestSession_AllowedWorkspaces_NilSession is a paranoid nil-safety +// check for callers that might invoke the accessor on a freshly- +// declared zero-value Session pointer. +func TestSession_AllowedWorkspaces_NilSession(t *testing.T) { + var s *Session + if got := s.AllowedWorkspaces(); got != nil { + t.Errorf("nil session should return nil, not panic; got %v", got) + } +} diff --git a/internal/server/context.go b/internal/server/context.go index df2f4d1d..945c68f6 100644 --- a/internal/server/context.go +++ b/internal/server/context.go @@ -121,3 +121,54 @@ func TokenScopesFromContext(ctx context.Context) string { func TokenScopeAllows(scopesJSON, method, path string) bool { return tokenScopeAllows(scopesJSON, method, path) } + +// WithTokenAllowedWorkspaces returns ctx decorated with the OAuth +// token's workspace allow-list set at consent time (TASK-952). The +// list is either a set of slugs (the user's specific selection) or +// `["*"]` (the wildcard checkbox). MCPBearerAuth's OAuth path stashes +// this on every request so RequireWorkspaceAccess can gate the +// resolved workspace against the allow-list (TASK-953) before +// running the standard membership check. +// +// nil clears any previously set allow-list — distinct from setting +// an empty slice, which would deny every workspace. PAT auth never +// calls this; the helper exists for the OAuth path only. +// +// Exported so the in-process MCP dispatcher can forward the same +// allow-list onto synthesized requests via Apply, matching the +// pattern WithTokenScopes uses (sub-PR E TASK-1027 round 1). +func WithTokenAllowedWorkspaces(ctx context.Context, slugs []string) context.Context { + if slugs == nil { + return context.WithValue(ctx, ctxTokenAllowedWorkspaces, []string(nil)) + } + // Defensive copy — caller mutating after the call must not + // corrupt the per-request token state. + cp := make([]string, len(slugs)) + copy(cp, slugs) + return context.WithValue(ctx, ctxTokenAllowedWorkspaces, cp) +} + +// TokenAllowedWorkspacesFromContext returns the token's workspace +// allow-list, or nil if none was attached. Three return shapes +// matter to callers: +// +// - nil — no allow-list set (PAT auth, or pre-TASK-952 OAuth +// tokens). Caller should NOT apply any token-level gate; rely +// on standard membership checks. +// - []string{"*"} — wildcard. Caller should not apply per-slug +// gating; standard membership applies. +// - []string{"slug-a", ...} — explicit allow-list. Caller MUST +// deny any workspace not in this set, even if the user is a +// member of it. +// +// Returns a copy of the stored slice — callers can mutate without +// risk to the per-request context value. +func TokenAllowedWorkspacesFromContext(ctx context.Context) []string { + v, _ := ctx.Value(ctxTokenAllowedWorkspaces).([]string) + if v == nil { + return nil + } + out := make([]string, len(v)) + copy(out, v) + return out +} diff --git a/internal/server/handlers_mcp_test.go b/internal/server/handlers_mcp_test.go index a9c842d7..0c2b114f 100644 --- a/internal/server/handlers_mcp_test.go +++ b/internal/server/handlers_mcp_test.go @@ -934,6 +934,418 @@ func TestE2E_ClaudeDesktopFlow(t *testing.T) { } } +// ===================================================================== +// Workspace allow-list gate (TASK-953) +// ===================================================================== + +// TestWorkspaceAllowList_AllowsListedSlug verifies the happy path: +// an OAuth token whose consent allow-list includes "alpha" passes +// the gate when reaching /api/v1/workspaces/alpha/* — same as the +// pre-TASK-953 behaviour for tokens whose allow-list isn't set. +// +// Drives the full chain: OAuth token via /mcp's MCPBearerAuth +// stashes the allow-list in context; the synthesized API request +// inherits it; RequireWorkspaceAccess reads it; the workspace +// resolves; the gate matches; the standard membership check runs. +func TestWorkspaceAllowList_AllowsListedSlug(t *testing.T) { + srv, _ := mcpAndOAuthEnabledTestServer(t) + user, sessionToken := loginTestUser(t, srv) + csrfTok := readCSRFFromCookie(t, srv, sessionToken) + clientID := registerTestClient(t, srv, "https://app.test/cb") + + // Seed two workspaces; user is owner of both. + mustSeedWorkspaceWithRole(t, srv, user.ID, "Alpha", "alpha", "owner") + mustSeedWorkspaceWithRole(t, srv, user.ID, "Beta", "beta", "owner") + + // Mint a token with allow-list = [alpha] (NOT beta). + verifier := "verifier-allow-alpha-quick-brown-fox-jumps-over-laz" + challenge := s256Challenge(verifier) + form := url.Values{ + "client_id": {clientID}, + "response_type": {"code"}, + "redirect_uri": {"https://app.test/cb"}, + "code_challenge": {challenge}, + "code_challenge_method": {"S256"}, + "scope": {"pad:read"}, + "audience": {testCanonicalAudience}, + "state": {"allow-alpha-state"}, + "decision": {"approve"}, + "csrf_token": {csrfTok}, + "capability_tier": {"read"}, + "allowed_workspaces": {"alpha"}, + } + access := mintOAuthTokenForTest(t, srv, sessionToken, csrfTok, clientID, verifier, form) + + // Hit /api/v1/workspaces/alpha — inside allow-list, should + // pass the gate. Use the dispatcher path: drive through /mcp's + // MCPBearerAuth so the context plumbing matches production. + rrAlpha := callWorkspaceViaOAuth(t, srv, access, "alpha") + if rrAlpha.Code == http.StatusForbidden { + t.Errorf("alpha (in allow-list) must NOT 403; got %d (body: %s)", + rrAlpha.Code, rrAlpha.Body.String()) + } +} + +// TestWorkspaceAllowList_DeniesUnlistedSlug pins the security +// invariant: a workspace not in the consent allow-list must be +// rejected with 403 even when the user is a member of it. The +// user's choice at consent time is binding. +func TestWorkspaceAllowList_DeniesUnlistedSlug(t *testing.T) { + srv, _ := mcpAndOAuthEnabledTestServer(t) + user, sessionToken := loginTestUser(t, srv) + csrfTok := readCSRFFromCookie(t, srv, sessionToken) + clientID := registerTestClient(t, srv, "https://app.test/cb") + + mustSeedWorkspaceWithRole(t, srv, user.ID, "Alpha", "alpha", "owner") + mustSeedWorkspaceWithRole(t, srv, user.ID, "Beta", "beta", "owner") + + // Token's allow-list contains ONLY "alpha". + verifier := "verifier-deny-beta-quick-brown-fox-jumps-over-lazy-" + challenge := s256Challenge(verifier) + form := url.Values{ + "client_id": {clientID}, + "response_type": {"code"}, + "redirect_uri": {"https://app.test/cb"}, + "code_challenge": {challenge}, + "code_challenge_method": {"S256"}, + "scope": {"pad:read"}, + "audience": {testCanonicalAudience}, + "state": {"deny-beta-state"}, + "decision": {"approve"}, + "csrf_token": {csrfTok}, + "capability_tier": {"read"}, + "allowed_workspaces": {"alpha"}, + } + access := mintOAuthTokenForTest(t, srv, sessionToken, csrfTok, clientID, verifier, form) + + // Beta is NOT in the allow-list — must 403 even though the + // user is owner of beta. + rrBeta := callWorkspaceViaOAuth(t, srv, access, "beta") + if rrBeta.Code != http.StatusForbidden { + t.Errorf("beta (not in allow-list) must 403; got %d (body: %s)", + rrBeta.Code, rrBeta.Body.String()) + } +} + +// TestWorkspaceAllowList_WildcardAllowsAnyMembership pins the +// wildcard semantics: ["*"] grants access to every workspace the +// user is a member of, no per-slug gate. Standard membership is +// still the boundary. +func TestWorkspaceAllowList_WildcardAllowsAnyMembership(t *testing.T) { + srv, _ := mcpAndOAuthEnabledTestServer(t) + user, sessionToken := loginTestUser(t, srv) + csrfTok := readCSRFFromCookie(t, srv, sessionToken) + clientID := registerTestClient(t, srv, "https://app.test/cb") + + mustSeedWorkspaceWithRole(t, srv, user.ID, "Alpha", "alpha", "owner") + mustSeedWorkspaceWithRole(t, srv, user.ID, "Beta", "beta", "editor") + + verifier := "verifier-wildcard-quick-brown-fox-jumps-over-lazy-d" + challenge := s256Challenge(verifier) + form := url.Values{ + "client_id": {clientID}, + "response_type": {"code"}, + "redirect_uri": {"https://app.test/cb"}, + "code_challenge": {challenge}, + "code_challenge_method": {"S256"}, + "scope": {"pad:read"}, + "audience": {testCanonicalAudience}, + "state": {"wildcard-state"}, + "decision": {"approve"}, + "csrf_token": {csrfTok}, + "capability_tier": {"read"}, + "allowed_workspaces": {"*"}, + } + access := mintOAuthTokenForTest(t, srv, sessionToken, csrfTok, clientID, verifier, form) + + for _, slug := range []string{"alpha", "beta"} { + rr := callWorkspaceViaOAuth(t, srv, access, slug) + if rr.Code == http.StatusForbidden { + t.Errorf("wildcard token must NOT 403 on %s; got %d (body: %s)", + slug, rr.Code, rr.Body.String()) + } + } +} + +// TestWorkspaceAllowList_LiveMembershipRevocation pins the live- +// membership check: a token whose allow-list includes "alpha" +// stops working when the user's membership in "alpha" is revoked +// — even though the token's stored allow-list still includes the +// slug. The membership table is the binding gate (RequireWorkspaceAccess +// runs after the allow-list check). +// +// This is the "if your membership changes, this connection's +// permissions change immediately" property from the PLAN-943 design +// (TASK-952's consent-screen footer references this). +func TestWorkspaceAllowList_LiveMembershipRevocation(t *testing.T) { + srv, _ := mcpAndOAuthEnabledTestServer(t) + user, sessionToken := loginTestUser(t, srv) + csrfTok := readCSRFFromCookie(t, srv, sessionToken) + clientID := registerTestClient(t, srv, "https://app.test/cb") + + ws := mustSeedWorkspaceWithRole(t, srv, user.ID, "Alpha", "alpha", "owner") + + verifier := "verifier-revoke-quick-brown-fox-jumps-over-the-lazy" + challenge := s256Challenge(verifier) + form := url.Values{ + "client_id": {clientID}, + "response_type": {"code"}, + "redirect_uri": {"https://app.test/cb"}, + "code_challenge": {challenge}, + "code_challenge_method": {"S256"}, + "scope": {"pad:read"}, + "audience": {testCanonicalAudience}, + "state": {"revoke-state"}, + "decision": {"approve"}, + "csrf_token": {csrfTok}, + "capability_tier": {"read"}, + "allowed_workspaces": {"alpha"}, + } + access := mintOAuthTokenForTest(t, srv, sessionToken, csrfTok, clientID, verifier, form) + + // Sanity check — token works against alpha while membership + // is intact. + rrBefore := callWorkspaceViaOAuth(t, srv, access, "alpha") + if rrBefore.Code == http.StatusForbidden { + t.Fatalf("baseline: token should pass while user is owner of alpha; got %d", rrBefore.Code) + } + + // Revoke membership. Reuse the store directly so the test + // doesn't depend on an admin-only API path. + if err := srv.store.RemoveWorkspaceMember(ws.ID, user.ID); err != nil { + t.Fatalf("RemoveWorkspaceMember: %v", err) + } + + // Same token now — must NOT succeed (live membership check). + // resolveWorkspace short-circuits for non-admin users by scoping + // slug lookup to the user's memberships, so post-revocation the + // response is actually 404 (workspace doesn't resolve) rather + // than 403 (workspace resolves but member is missing). Both + // are valid "no access" outcomes — 404 leaks less than 403, + // since the attacker can't tell whether the workspace exists. + // The TASK-953 design lists 403 as the canonical response, but + // either denial mode satisfies "membership revocation cuts the + // connection immediately." + rrAfter := callWorkspaceViaOAuth(t, srv, access, "alpha") + if rrAfter.Code != http.StatusForbidden && rrAfter.Code != http.StatusNotFound { + t.Errorf("post-revocation: must 403 or 404 (live membership check); got %d (body: %s)", + rrAfter.Code, rrAfter.Body.String()) + } +} + +// TestWorkspaceAllowList_PATPathUnaffected regresses the PAT auth +// path: a Personal Access Token doesn't carry a workspace allow- +// list (the consent UI is OAuth-only), so its requests must NOT +// hit the gate. PATs that hit /api/v1/workspaces//* should +// continue to work the same as they did pre-TASK-953. +func TestWorkspaceAllowList_PATPathUnaffected(t *testing.T) { + srv, _ := mcpAndOAuthEnabledTestServer(t) + user := mustCreateUserForTest(t, srv, "pat-no-gate@example.com") + mustSeedWorkspaceWithRole(t, srv, user.ID, "Alpha", "alpha", "owner") + + // Workspace-scoped PAT (the Y2025 model — workspace required for the + // FK but the user is the auth principal). + wsForToken, err := srv.store.CreateWorkspace(models.WorkspaceCreate{Name: "Token WS"}) + if err != nil { + t.Fatalf("CreateWorkspace (token holder): %v", err) + } + tok, err := srv.store.CreateAPIToken(user.ID, models.APITokenCreate{ + Name: "pat-no-gate", WorkspaceID: wsForToken.ID, + }, 30, 0) + if err != nil { + t.Fatalf("CreateAPIToken: %v", err) + } + + // PAT against /api/v1/workspaces/alpha must not be gate-rejected. + req := httptest.NewRequest("GET", "/api/v1/workspaces/alpha/", nil) + req.Header.Set("Authorization", "Bearer "+tok.Token) + req.RemoteAddr = "192.0.2.1:1234" + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, req) + + if rr.Code == http.StatusForbidden { + t.Errorf("PAT path must NOT be subject to the OAuth allow-list gate; got 403 (body: %s)", rr.Body.String()) + } +} + +// TestWorkspaceAllowList_TierTimesRole_WriteByViewer pins the +// central tier×role security claim from PLAN-943: a token whose +// capability tier is `pad:write` does NOT bypass the user's +// workspace role. If the user is only a Viewer in workspace foo, +// a `pad:write` token must NOT be able to create items there even +// though the token's tier is sufficient — the role gate kicks in +// at the per-handler level (requireEditPermission via +// workspaceRole(r)). +// +// This is the test the task spec calls out specifically: +// "pad:write action + Viewer → 403 (capability allows but role +// doesn't)." +func TestWorkspaceAllowList_TierTimesRole_WriteByViewer(t *testing.T) { + srv, _ := mcpAndOAuthEnabledTestServer(t) + user, sessionToken := loginTestUser(t, srv) + csrfTok := readCSRFFromCookie(t, srv, sessionToken) + clientID := registerTestClient(t, srv, "https://app.test/cb") + + // User is a VIEWER in workspace alpha — restrictive role. + ws := mustSeedWorkspaceWithRole(t, srv, user.ID, "Alpha", "alpha", "viewer") + + // Seed a collection so the POST has a valid target. + if _, err := srv.store.CreateCollection(ws.ID, models.CollectionCreate{ + Name: "Tasks", + Slug: "tasks", + Schema: `{"fields":[]}`, + }); err != nil { + t.Fatalf("CreateCollection: %v", err) + } + + // Mint a pad:write tier token, allow-list = [alpha]. Capability + // tier alone passes tokenScopeAllows for POST. + verifier := "verifier-tier-role-quick-brown-fox-jumps-over-lazy-" + challenge := s256Challenge(verifier) + form := url.Values{ + "client_id": {clientID}, + "response_type": {"code"}, + "redirect_uri": {"https://app.test/cb"}, + "code_challenge": {challenge}, + "code_challenge_method": {"S256"}, + "scope": {"pad:read pad:write"}, + "audience": {testCanonicalAudience}, + "state": {"tier-role-state"}, + "decision": {"approve"}, + "csrf_token": {csrfTok}, + "capability_tier": {"write"}, + "allowed_workspaces": {"alpha"}, + } + access := mintOAuthTokenForTest(t, srv, sessionToken, csrfTok, clientID, verifier, form) + + // POST a new item under alpha/tasks. Token's tier is write + // (passes tokenScopeAllows), token's allow-list includes alpha + // (passes the new gate), user is a viewer in alpha (must + // trigger requireEditPermission's role check → 403). + urlPath := "/api/v1/workspaces/alpha/collections/tasks/items" + body := strings.NewReader(`{"title":"should be denied"}`) + req := httptest.NewRequest("POST", urlPath, body) + req.Header.Set("Authorization", "Bearer "+access) + req.Header.Set("Content-Type", "application/json") + req.RemoteAddr = "192.0.2.1:1234" + + inner := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + api := req.Clone(r.Context()) + api.Header.Del("Authorization") + srv.ServeHTTP(w, api) + }) + rr := httptest.NewRecorder() + srv.MCPBearerAuth(inner).ServeHTTP(rr, req) + + if rr.Code != http.StatusForbidden { + t.Errorf("pad:write tier × viewer role MUST 403 on item create; got %d (body: %s)", + rr.Code, rr.Body.String()) + } +} + +// mintOAuthTokenForTest drives the consent → token exchange and +// returns the access token. Lightly customizable via the form arg +// so callers can vary the consent payload (allow-list, tier, etc.). +// +// Centralizes the boilerplate that would otherwise repeat across +// every workspace-allow-list test. +func mintOAuthTokenForTest(t *testing.T, srv *Server, sessionToken, csrfTok, clientID, verifier string, form url.Values) string { + t.Helper() + + rrDecide := postFormWithCookie(srv, "/oauth/authorize/decide", form, sessionToken, csrfTok) + if rrDecide.Code != http.StatusSeeOther && rrDecide.Code != http.StatusFound { + t.Fatalf("decide: expected 303/302, got %d (body: %s)", rrDecide.Code, rrDecide.Body.String()) + } + cbURL, _ := url.Parse(rrDecide.Header().Get("Location")) + code := cbURL.Query().Get("code") + if code == "" { + t.Fatalf("missing code in callback: %s", rrDecide.Header().Get("Location")) + } + + tokenForm := url.Values{ + "grant_type": {"authorization_code"}, + "code": {code}, + "client_id": {clientID}, + "redirect_uri": {"https://app.test/cb"}, + "code_verifier": {verifier}, + "audience": {testCanonicalAudience}, + } + trr := postOAuthForm(srv, "/oauth/token", tokenForm) + if trr.Code != http.StatusOK { + t.Fatalf("token: expected 200, got %d (body: %s)", trr.Code, trr.Body.String()) + } + var resp map[string]any + parseJSON(t, trr, &resp) + access, _ := resp["access_token"].(string) + if access == "" { + t.Fatalf("missing access_token: %v", resp) + } + return access +} + +// callWorkspaceViaOAuth simulates an MCP-driven workspace API call +// by going through MCPBearerAuth's OAuth path. The MCP transport +// stub the test harness wires (mcpAndOAuthEnabledTestServer) DOES +// NOT actually synthesize a workspace request — it just records +// the auth context. So we make the workspace request directly with +// the OAuth bearer, going through the same auth chain a synthesized +// dispatcher request would go through. +// +// This works because /api/v1/workspaces// is mounted on the +// /api/v1 group with TokenAuth + SessionAuth + RequireAuth. We need +// MCPBearerAuth to set up the context, but a direct API request +// with a Bearer token won't hit MCPBearerAuth — it'll hit TokenAuth +// instead, which doesn't currently introspect OAuth tokens. So we +// use httptest.NewRecorder + manually inject the context the +// dispatcher would, then invoke the API via a fresh handler call. +// +// Concretely: this helper invokes MCPBearerAuth as a chain wrapper +// around a simple workspace handler so the OAuth-token validation +// + context plumbing happens once, exactly as production. +func callWorkspaceViaOAuth(t *testing.T, srv *Server, access, workspaceSlug string) *httptest.ResponseRecorder { + t.Helper() + // Build a request to /api/v1/workspaces//. The path goes + // through chi's routing for slug extraction in the workspace + // middleware. + req := httptest.NewRequest("GET", "/api/v1/workspaces/"+workspaceSlug+"/", nil) + req.Header.Set("Authorization", "Bearer "+access) + req.RemoteAddr = "192.0.2.1:1234" + + // MCPBearerAuth normally only runs on /mcp. To exercise the + // OAuth-token allow-list gate end-to-end, manually wrap the + // handler chain so MCPBearerAuth runs first and stashes context, + // then the regular API chain sees the synthesized token state. + // This mirrors what the dispatcher does in production: the + // inbound /mcp request sets context, then in-process + // synthesized requests inherit it. + inner := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // Build a fresh request for /api with the bearer-derived + // context preserved — mirror buildHTTPRequest's pattern. + api := req.Clone(r.Context()) + api.Header.Del("Authorization") // drop bearer; context carries the auth state + srv.ServeHTTP(w, api) + }) + rr := httptest.NewRecorder() + srv.MCPBearerAuth(inner).ServeHTTP(rr, req) + return rr +} + +// mustCreateUserForTest creates a user for tests that need a user +// row but don't need a session. Counterpart to loginTestUser, which +// creates BOTH a user and a session. +func mustCreateUserForTest(t *testing.T, srv *Server, email string) *models.User { + t.Helper() + user, err := srv.store.CreateUser(models.UserCreate{ + Email: email, + Name: "Test User", + Password: "pw-test-12345", + }) + if err != nil { + t.Fatalf("CreateUser: %v", err) + } + return user +} + // newTestOAuthServer builds an internal/oauth.Server matched to the // test harness's canonical audience (testCanonicalAudience). Used by // mcpAndOAuthEnabledTestServer to wire OAuth alongside the MCP stub. diff --git a/internal/server/handlers_oauth.go b/internal/server/handlers_oauth.go index 6bd6913d..bff9d4e6 100644 --- a/internal/server/handlers_oauth.go +++ b/internal/server/handlers_oauth.go @@ -565,7 +565,7 @@ func (s *Server) handleOAuthAuthorizeDecide(w http.ResponseWriter, r *http.Reque // workspace allow-list; TASK-953 reads it at /mcp time + does // the live role lookup. Round-trips via storage.go's JSON marshal. session := oauth.NewSession(user.ID) - session.DefaultSession.Extra["allowed_workspaces"] = allowedWorkspaces + session.SetAllowedWorkspaces(allowedWorkspaces) resp, err := s.oauthServer.Provider().NewAuthorizeResponse(ctx, ar, session) if err != nil { diff --git a/internal/server/middleware_auth.go b/internal/server/middleware_auth.go index 928e7bdf..60808e7a 100644 --- a/internal/server/middleware_auth.go +++ b/internal/server/middleware_auth.go @@ -39,6 +39,13 @@ const ( // ctxResolvedWorkspaceID is set by RequireWorkspaceAccess after resolving // the workspace slug/ID. Avoids redundant lookups in handlers. ctxResolvedWorkspaceID contextKey = "resolved_workspace_id" + // ctxTokenAllowedWorkspaces carries the OAuth token's workspace + // allow-list set at consent time (TASK-952). Either a list of + // slugs or `["*"]` (wildcard). Read by RequireWorkspaceAccess to + // reject requests against workspaces the user didn't include in + // the consent (TASK-953). nil → no token-level workspace + // constraint (PAT auth, or pre-TASK-952 OAuth tokens). + ctxTokenAllowedWorkspaces contextKey = "token_allowed_workspaces" ) // TokenAuth middleware checks for an Authorization: Bearer pad_xxx header. @@ -376,6 +383,29 @@ func (s *Server) RequireWorkspaceAccess(next http.Handler) http.Handler { return } + // OAuth token allow-list gate (TASK-953). The consent UI + // (TASK-952) lets the user pick which workspaces a token + // can access; MCPBearerAuth stashes that list in context + // via WithTokenAllowedWorkspaces. Reject any request hitting + // a workspace outside the list, even if the user is a + // member of it — the user explicitly chose not to grant the + // app that access. + // + // nil → no token-level constraint (PAT auth, or pre-TASK-952 + // OAuth tokens that predate the consent UI). Wildcard `["*"]` + // → grant access to any membership the user has. Else: the + // resolved workspace's slug MUST appear in the list. + // + // Compares against the canonical slug (ws.Slug) because the + // consent UI persists slugs and the URL slugOrID may be a + // UUID which resolveWorkspace just translated. Slug-vs-slug + // is the right comparison. + if !tokenAllowedWorkspaceMatches(r.Context(), ws.Slug) { + writeError(w, http.StatusForbidden, "permission_denied", + "Token is not authorized for this workspace") + return + } + // Store resolved workspace ID in context for downstream handlers ctx := context.WithValue(r.Context(), ctxResolvedWorkspaceID, ws.ID) @@ -680,6 +710,41 @@ func clearSessionCookie(w http.ResponseWriter, secure bool) { }) } +// tokenAllowedWorkspaceMatches reports whether the OAuth token's +// workspace allow-list (set at consent time, TASK-952) permits the +// given workspace slug. The three return-shape cases match +// TokenAllowedWorkspacesFromContext: +// +// - nil — no allow-list set (PAT auth, or pre-TASK-952 tokens) → +// allow. +// - ["*"] — wildcard consent → allow. +// - [slug-a, slug-b, ...] — explicit allow-list → require slug ∈ +// list. +// +// Empty (non-nil) slice — the SetAllowedWorkspaces guard rejects +// nil → empty translation, and the consent flow rejects +// `allowed_workspaces` with no entries (handlers_oauth.go's +// parseConsentPayload). So an empty list shouldn't appear in +// practice; if it does, fail closed (no slug matches an empty list). +// +// Used by RequireWorkspaceAccess; package-private because the +// allow-list semantics are coupled to that middleware's flow. +func tokenAllowedWorkspaceMatches(ctx context.Context, slug string) bool { + allowed := TokenAllowedWorkspacesFromContext(ctx) + if allowed == nil { + return true // no token-level gate + } + for _, entry := range allowed { + if entry == "*" { + return true + } + if entry == slug { + return true + } + } + return false +} + // tokenScopeAllows checks if the token's scopes permit the given HTTP method // and path. Scopes are stored as a JSON array of strings. // diff --git a/internal/server/middleware_mcp_auth.go b/internal/server/middleware_mcp_auth.go index 9723a361..d3bc2b4e 100644 --- a/internal/server/middleware_mcp_auth.go +++ b/internal/server/middleware_mcp_auth.go @@ -9,6 +9,8 @@ import ( "strings" "github.com/ory/fosite" + + "github.com/PerpetualSoftware/pad/internal/oauth" ) // MCPBearerAuth is the auth gate for the /mcp Streamable HTTP endpoint. @@ -282,6 +284,27 @@ func (s *Server) handleMCPOAuthAuth(w http.ResponseWriter, r *http.Request, toke // policy applies to OAuth-issued tokens. ctx = WithTokenScopes(ctx, oauthScopesToJSON(ar.GetGrantedScopes())) + // Stash the workspace allow-list set at consent time (TASK-952) + // so RequireWorkspaceAccess can gate workspace access per-token + // (TASK-953). Reading from session.Extra goes through the typed + // AllowedWorkspaces accessor so JSON round-trip handling + // ([]interface{} vs []string) is centralized in oauth.Session. + // + // Three shapes arrive here: + // - nil — pre-TASK-952 token (no consent payload). Treated by + // RequireWorkspaceAccess as "no token-level gate"; standard + // membership applies. + // - ["*"] — wildcard. Same effective behaviour as nil for the + // gate: any workspace the user is a member of is allowed. + // - [slug-a, slug-b, ...] — explicit allow-list. Workspaces + // NOT in this set are rejected before the membership check + // even runs. + if oauthSession, ok := session.(*oauth.Session); ok { + if allowed := oauthSession.AllowedWorkspaces(); allowed != nil { + ctx = WithTokenAllowedWorkspaces(ctx, allowed) + } + } + next.ServeHTTP(w, r.WithContext(ctx)) } diff --git a/internal/server/token_workspace_allowlist_test.go b/internal/server/token_workspace_allowlist_test.go new file mode 100644 index 00000000..456ecff6 --- /dev/null +++ b/internal/server/token_workspace_allowlist_test.go @@ -0,0 +1,108 @@ +package server + +import ( + "context" + "testing" +) + +// TestTokenAllowedWorkspaceMatches covers the policy table for the +// OAuth-token workspace allow-list (TASK-953): +// +// - nil allow-list → no gate; every slug allowed (PAT auth or +// pre-TASK-952 OAuth tokens fall here). +// - ["*"] wildcard → every slug allowed. +// - ["foo", "bar"] → only "foo" and "bar" allowed; "baz" denied. +// - explicit empty list → denies every slug (fail-closed; in +// practice the consent flow rejects empty lists at parse time, +// but the helper must not "open up" if one slips through). +// - ["foo", "*"] → wildcard wins; every slug allowed. +// +// Cheap pure-function unit test; the integration flow is covered +// separately in handlers_mcp_test.go's MCP+OAuth test surface. +func TestTokenAllowedWorkspaceMatches(t *testing.T) { + cases := []struct { + name string + allowed []string + slug string + want bool + }{ + // nil → no gate (PAT auth or pre-TASK-952 token). + {"nil allows anything 1", nil, "foo", true}, + {"nil allows anything 2", nil, "anything", true}, + + // Wildcard. + {"wildcard allows foo", []string{"*"}, "foo", true}, + {"wildcard allows arbitrary", []string{"*"}, "anything-else", true}, + + // Explicit allow-list — only listed slugs match. + {"explicit foo matches foo", []string{"foo"}, "foo", true}, + {"explicit foo,bar matches foo", []string{"foo", "bar"}, "foo", true}, + {"explicit foo,bar matches bar", []string{"foo", "bar"}, "bar", true}, + {"explicit foo,bar denies baz", []string{"foo", "bar"}, "baz", false}, + {"explicit foo denies bar", []string{"foo"}, "bar", false}, + + // Defensive: empty (non-nil) list → fail-closed, no slug + // matches. Consent flow rejects empty allow-lists at parse + // time, so this case shouldn't occur in production. + {"empty list denies anything", []string{}, "foo", false}, + + // Mixed wildcard + specific entries — wildcard wins. A + // tampered token (or future feature) could have both; the + // safer interpretation is "any" because that's what the + // wildcard explicitly grants. + {"wildcard + specific allows arbitrary", []string{"foo", "*"}, "anything", true}, + {"wildcard first allows arbitrary", []string{"*", "foo"}, "anything", true}, + + // Edge cases. + {"empty slug never matches explicit list", []string{"foo"}, "", false}, + {"empty slug matches wildcard", []string{"*"}, "", true}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctx := context.Background() + if tc.allowed != nil { + ctx = WithTokenAllowedWorkspaces(ctx, tc.allowed) + } + got := tokenAllowedWorkspaceMatches(ctx, tc.slug) + if got != tc.want { + t.Errorf("tokenAllowedWorkspaceMatches(%v, %q) = %v, want %v", + tc.allowed, tc.slug, got, tc.want) + } + }) + } +} + +// TestWithTokenAllowedWorkspaces_DefensiveCopy verifies the helper +// copies the input slice so a caller mutating after the call doesn't +// corrupt the per-request token state. This is the canonical +// defense-in-depth pattern for context values that callers might +// reuse across requests. +func TestWithTokenAllowedWorkspaces_DefensiveCopy(t *testing.T) { + original := []string{"foo", "bar"} + ctx := WithTokenAllowedWorkspaces(context.Background(), original) + + // Mutate the original after stashing. + original[0] = "MUTATED" + + got := TokenAllowedWorkspacesFromContext(ctx) + if len(got) != 2 || got[0] != "foo" || got[1] != "bar" { + t.Errorf("context value should be insulated from caller mutation; got %v", got) + } +} + +// TestTokenAllowedWorkspacesFromContext_ReturnsCopy verifies the +// reader returns a fresh slice — a caller mutating the returned +// value must not corrupt the context-stored allow-list either. +func TestTokenAllowedWorkspacesFromContext_ReturnsCopy(t *testing.T) { + ctx := WithTokenAllowedWorkspaces(context.Background(), []string{"foo", "bar"}) + + got := TokenAllowedWorkspacesFromContext(ctx) + got[0] = "MUTATED" + + // Re-read; the stored value must be unchanged. + got2 := TokenAllowedWorkspacesFromContext(ctx) + if got2[0] != "foo" { + t.Errorf("returned slice should be a copy; got2[0] = %q after caller mutation", got2[0]) + } +}