From 6f6b31f392b9a6e866baa62472c90dd385bf24e5 Mon Sep 17 00:00:00 2001 From: Wynne Pirini Date: Thu, 17 Sep 2026 11:54:20 +1200 Subject: [PATCH] auth: archived members' MCP bearers stop validating, and cannot be refreshed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit VerifyMCPBearer's OAuth branch never checked users.archived_at, so archiving a member — the documented offboarding path — left their connected agent bearer working, while sessions and API keys already filtered on it. The refresh grant had the same gap: it would mint fresh tokens for a dead account. Both queries now join users and require archived_at IS NULL, mirroring the API-key fallback directly below. Pinned by TestMCP_OAuthBearerRejectedWhenArchived (red-green verified against the unfixed code on both assertions) and one ARCHITECTURE §6 sentence stating the invariant. --- docs/ARCHITECTURE.md | 4 ++ internal/handler/mcp_oauth.go | 11 +++- internal/handler/mcp_oauth_authorize.go | 7 ++- internal/handler/mcp_oauth_test.go | 75 +++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 4 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e91d5db6..b79052a5 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -189,6 +189,10 @@ the platform/recovery secret doesn't expose secrets. - **Offboarding = archive** (`users.archived_at`), never hard-delete — preserves bookings, event-type ownership, team links. Archived ⇒ no login, hidden from lists, skipped in routing/slots, event types deactivated. Reversible (restore). + "No login" holds on every auth path: sessions, API keys, and MCP OAuth bearers + all filter on `archived_at`, and refresh grants are refused for archived users + too — so archiving ends a member's agent access and leaves no usable + credential material behind. Archiving is blocked while the member has upcoming (primary-host) bookings; a resolve-meetings flow makes the admin reassign/cancel each first. diff --git a/internal/handler/mcp_oauth.go b/internal/handler/mcp_oauth.go index 330b9f13..bd2f1638 100644 --- a/internal/handler/mcp_oauth.go +++ b/internal/handler/mcp_oauth.go @@ -99,10 +99,15 @@ func (h *Handler) MCPCallerMiddleware(next http.Handler) http.Handler { func (h *Handler) VerifyMCPBearer(ctx context.Context, token string, _ *http.Request) (*auth.TokenInfo, error) { hash := hashAPIKey(token) - // OAuth access token? + // OAuth access token? Archived users authenticate with nothing: the session + // path (auth.go) and both API-key paths (below, and auth.go) already filter + // on users.archived_at, and without the same check here an archived member's + // connected agent bearer would keep validating after offboarding. var userID, expiresAt string - err := h.db.QueryRowContext(ctx, - `SELECT user_id, expires_at FROM oauth_access_tokens WHERE token_hash = ?`, hash). + err := h.db.QueryRowContext(ctx, ` + SELECT t.user_id, t.expires_at FROM oauth_access_tokens t + JOIN users u ON u.id = t.user_id + WHERE t.token_hash = ? AND u.archived_at IS NULL`, hash). Scan(&userID, &expiresAt) if err == nil { exp, _ := time.Parse(time.RFC3339, expiresAt) diff --git a/internal/handler/mcp_oauth_authorize.go b/internal/handler/mcp_oauth_authorize.go index 8ab139f2..a7b2e481 100644 --- a/internal/handler/mcp_oauth_authorize.go +++ b/internal/handler/mcp_oauth_authorize.go @@ -246,8 +246,13 @@ func (h *Handler) tokenRefresh(w http.ResponseWriter, r *http.Request) { return } var id, userID, storedClient, scope, resource string + // Archived users refresh nothing: their access tokens no longer validate + // (VerifyMCPBearer filters on archived_at), so minting fresh ones here + // would hand live credential material to a dead account. err := h.db.QueryRowContext(r.Context(), ` - SELECT id, user_id, client_id, scope, resource FROM oauth_access_tokens WHERE refresh_hash = ?`, + SELECT t.id, t.user_id, t.client_id, t.scope, t.resource FROM oauth_access_tokens t + JOIN users u ON u.id = t.user_id + WHERE t.refresh_hash = ? AND u.archived_at IS NULL`, hashAPIKey(refresh)).Scan(&id, &userID, &storedClient, &scope, &resource) if err != nil { writeOAuthError(w, http.StatusBadRequest, "invalid_grant", "unknown refresh token") diff --git a/internal/handler/mcp_oauth_test.go b/internal/handler/mcp_oauth_test.go index 59e5042d..a51dd3b1 100644 --- a/internal/handler/mcp_oauth_test.go +++ b/internal/handler/mcp_oauth_test.go @@ -297,3 +297,78 @@ func TestMCP_OAuthDeny(t *testing.T) { t.Errorf("deny: code=%d location=%q; want 302 with error=access_denied", rec.Code, loc) } } + +// TestMCP_OAuthBearerRejectedWhenArchived pins the offboarding half of +// "Offboarding = archive" (§6) for the MCP OAuth path. The session path and +// both API-key paths already filter on users.archived_at; the OAuth branch of +// VerifyMCPBearer did not, so an archived member's connected agent bearer kept +// validating after they were offboarded. +func TestMCP_OAuthBearerRejectedWhenArchived(t *testing.T) { + h, database, _, userID := setupWorkspaceWithDB(t) + + const rawToken = "archived-user-bearer" + const rawRefresh = "archived-user-refresh" + now := time.Now().UTC() + if _, err := database.Exec(` + INSERT INTO oauth_access_tokens (id, token_hash, refresh_hash, client_id, user_id, expires_at, created_at) + VALUES (?, ?, ?, 'client-1', ?, ?, ?)`, + "tok-archived", sha256HexForTest(rawToken), sha256HexForTest(rawRefresh), userID, + now.Add(time.Hour).Format(time.RFC3339), now.Format(time.RFC3339Nano)); err != nil { + t.Fatalf("seed token: %v", err) + } + refresh := func(t *testing.T, raw string) *httptest.ResponseRecorder { + t.Helper() + form := url.Values{"grant_type": {"refresh_token"}, "refresh_token": {raw}} + req := httptest.NewRequest(http.MethodPost, "/oauth/token", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rec := httptest.NewRecorder() + h.TokenMCP(rec, req) + return rec + } + + // Before archiving, the bearer validates and the refresh token rotates. + // The rotated pair is kept: it is the agent's *current* credential, which + // is exactly what must die on archive (testing the pre-rotation values + // post-archive would pass vacuously — rotation already replaced them). + if info, err := h.VerifyMCPBearer(context.Background(), rawToken, nil); err != nil || info.UserID != userID { + t.Fatalf("VerifyMCPBearer before archive = %+v, %v; want UserID=%s", info, err, userID) + } + rec := refresh(t, rawRefresh) + if rec.Code != http.StatusOK { + t.Fatalf("refresh before archive = %d; want 200 — %s", rec.Code, rec.Body.String()) + } + var rotated map[string]any + if err := json.Unmarshal(rec.Body.Bytes(), &rotated); err != nil { + t.Fatalf("decode rotated pair: %v", err) + } + access, _ := rotated["access_token"].(string) + refreshTok, _ := rotated["refresh_token"].(string) + if access == "" || refreshTok == "" { + t.Fatalf("rotated pair missing tokens: %v", rotated) + } + // The rotated access token validates while the user is live — so its + // post-archive rejection below proves the archive check, not a broken + // rotation. + if info, err := h.VerifyMCPBearer(context.Background(), access, nil); err != nil || info.UserID != userID { + t.Fatalf("VerifyMCPBearer(rotated) before archive = %+v, %v; want UserID=%s", info, err, userID) + } + + // Archive the user — the documented offboarding path — and the current + // bearer must stop validating, exactly like the user's sessions and API + // keys do. + if _, err := database.Exec(`UPDATE users SET archived_at = ? WHERE id = ?`, + now.Format(time.RFC3339Nano), userID); err != nil { + t.Fatalf("archive user: %v", err) + } + if info, err := h.VerifyMCPBearer(context.Background(), access, nil); err == nil { + t.Errorf("VerifyMCPBearer after archive = %+v; want rejection — an archived member's agent bearer must not survive offboarding", info) + } + + // The refresh token must not mint fresh credentials for a dead account + // either: the rotated access token would not validate, but issuance itself + // tells the agent it is still authorized and leaves live credential + // material in the table. + if rec := refresh(t, refreshTok); rec.Code == http.StatusOK { + t.Errorf("refresh after archive issued tokens to an archived user: %s", rec.Body.String()) + } +}