auth: archived members' MCP bearers stop validating, and cannot be refreshed - #49
Conversation
…freshed 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.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes MCP OAuth now fails closed on users.archived_at for both bearer validation and refresh issuance, matching sessions and API keys.
- Bearer gate —
VerifyMCPBearer's OAuth branch joinsusersand requiresarchived_at IS NULL, so an archived member's agent token stops authenticating on/mcp. - Refresh gate —
tokenRefreshuses the same join, so archive cannot mint a fresh access+refresh pair (issuance-to-a-dead-account, not a/mcpbypass). - Test —
TestMCP_OAuthBearerRejectedWhenArchivedkeeps the rotated live pair as the post-archive subject, so neither assertion can pass vacuously. - Docs — ARCHITECTURE §6 now states that "no login" covers MCP OAuth bearers and refresh, without sweeping tokens (archive stays reversible).
Grok ｜ 𝕏
…out deleting The previous commit described archive as deleting a member's sessions and MCP tokens. Calnode#49 closed the hole differently: the OAuth bearer check and the refresh grant both refuse an archived member, and archive deletes nothing, so restore stays a true reverse. The sentence now says what Calnode#49 does. It still describes the tree once Calnode#49 is in, so this merges after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
One data point for the restore decision you've left open. I'd drafted the delete-on-archive version before this PR appeared, and measured it with the deletes turned off: restore brings back everything archive suspended. A restored member's old session cookie signs in again, their unexpired access token validates, and the refresh token mints a new pair. That's consistent with "reversible", so it may be exactly what you want. If you decide restore shouldn't bring agent or browser access back, |
2 participants
Companion to #41, and the hole its review found. ARCHITECTURE §6 documents "Offboarding = archive" as the complete story, but two MCP paths never checked
archived_at:VerifyMCPBearer's OAuth branch (mcp_oauth.go) selected onlyuser_id, expires_at— an archived member's connected agent bearer kept validating, while the session path (auth.go:103) and both API-key paths (auth.go:76,mcp_oauth.go:121) already filtered on it.mcp_oauth_authorize.go:tokenRefresh) would mint a fresh access+refresh pair for an archived user. The minted access token would not have validated on /mcp, so this was issuance-to-a-dead-account rather than a bypass — but it told the agent it was still authorized and left live credential material in the table.Both queries now
JOIN users … WHERE … AND u.archived_at IS NULL, mirroring the API-key fallback. Cost is a unique-index lookup plus a PK lookup (token_hash/refresh_hashare both UNIQUE-indexed). No FK exists tousers, so the INNER JOIN additionally fails closed on orphan rows rather than validating them.Verification:
TestMCP_OAuthBearerRejectedWhenArchivedcovers bearer rejection + refresh refusal, with pre-archive positive controls (including the rotated pair validating while live, so neither post-archive assertion can pass vacuously). Red-green checked by stashing both source hunks: both assertions fail on the unfixed code. Fullgo test ./internal/handler/,go vet,gofmt, `go build ./...$ all clean.Deliberately out of scope: deleting
oauth_access_tokensrows insideArchiveUser. Archive is documented as reversible, and with both gates closed there is no live access left to sweep — deletion would change restore semantics, which deserves its own decision. Unblocks #41.