-
Notifications
You must be signed in to change notification settings - Fork 8
Sign out everywhere, including the MCP connectors - #41
Open
distronode-com wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i️ Two rough edges worth a look — handler/RBAC core looks solid.
Reviewed changes One new authenticated endpoint that revokes sessions (and MCP OAuth tokens) for the caller or, with admin tiers, for another user.
POST /v1/auth/sessions/revoke-all— Self mode keeps the calling session and wipes the rest; targeting another user follows the existing owner/admin/member ladder, with the admin check before the target load so members cannot 404-oracle ids.- MCP token sweep in the same transaction — Deletes the target's
oauth_access_tokensrows with the session deletes so a partial failure cannot report success while an agent bearer still works. - Tests — Cover keep-current, API-key wipe-all, MCP cut, RBAC, same status+body for existing vs unknown targets (member), and self-via-
user_id.
i️ Archive still does not cut MCP agents
VerifyMCPBearer's OAuth path never checks users.archived_at (the API-key path does), and ArchiveUser does not delete oauth_access_tokens. So the documented primary offboarding path (archive) still leaves a connected agent holding a bearer until something else deletes the rows. This PR correctly adds the wipe; operators who only archive remain exposed. Worth either deleting tokens inside ArchiveUser (same transaction as archived_at) or stating the two-step offboarding sequence next to both bullets in ARCHITECTURE.md.
Technical details
# Archive vs MCP bearer lifetime ## Affected sites - `internal/handler/mcp_oauth.go` (`VerifyMCPBearer`) — OAuth branch selects only `user_id, expires_at` from `oauth_access_tokens`; no join/filter on `users.archived_at` - `internal/handler/archive.go` (`ArchiveUser`) — sets `archived_at`, deactivates event types; does not touch sessions or oauth tokens - `docs/ARCHITECTURE.md` §6 — adjacent "Sign out everywhere" and "Offboarding = archive" bullets ## Required outcome - Either archive alone ends MCP access for that user, or docs make the dual-step (archive + revoke-all) explicit so operators do not assume archive is sufficient ## Suggested approach (optional) - Prefer deleting `oauth_access_tokens` (and optionally sessions) inside the archive transaction; mirrors what this endpoint already does for the credential half of offboarding
Grok | X
Third of the features split out of Calnode#30. POST /v1/auth/sessions/revoke-all. With no body it drops all of the caller's sessions EXCEPT the one that made the request — "sign out my other devices", as distinct from POST /v1/auth/logout, which ends the current one. An API-key caller has no current session, so for them every session goes. Losing a laptop no longer means waiting out a 30-day cookie. With {"user_id": "..."} it is an offboarding tool, gated on the same tiers roles.go already uses: an admin may revoke a member, only the owner may revoke another admin, and the owner's sessions are reachable only by the owner. ⛔ The actor's tier is checked BEFORE the target is loaded. Otherwise the 404 for an unknown id and the 403 for a protected one are distinguishable, and the endpoint becomes a way for a member to enumerate user ids. ⛔ It also deletes the target's rows in oauth_access_tokens, and that is the part that makes this an offboarding tool rather than a convenience. An MCP connector authenticates with a bearer token, not the session cookie, so ending the sessions alone would leave a connected agent holding exactly the access that was just withdrawn. Both deletes are in one transaction, so "revoked" is never half-true. TestRevokeAllSessions_memberCannotProbeForUserIDs holds the ordering, and it had to be written for this: the neighbouring member-cannot-target test names a REAL user, so it passes with the two checks in either order. The new one asks for an existing id and a made-up one and requires the same status AND the same body, since a differing message is the same oracle in prose. Moving the tier check below the target load fails it, and fails nothing else in the package. ⛔ Self-mode spares the session the caller AUTHENTICATED WITH, which is not the same as the one it happened to send. The cookie is read only when no API key is present, mirroring RequireAuth's own precedence — it tries the key first, so a request carrying both is an API-key request and its cookie authenticated nothing. Without that test a script holding a key and a stale cookie would ask to end all its sessions, be told it had, and leave one alive; the response counts what was deleted, not what was kept, so nothing would say so. The existing API-key test sends no cookie, so it passes either way. Mutation-checked rather than assumed: with the oauth_access_tokens delete replaced by a no-op, TestRevokeAllSessions_cutsMCPTokensToo and TestRevokeAllSessions_adminRevokesAMember both fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
distronode-com
force-pushed
the
feat/sign-out-everywhere
branch
from
September 9, 2026 22:52
c1e1722 to
be05370
Compare
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ No new issues found.
Reviewed changes Delta since the prior pullfrog review (c1e1722 → be05370): both prior inline findings fixed cleanly.
- CHANGELOG placement — Moved the Sign-out bullet to after the full Duplicate entry so Duplicate’s continuation paragraphs stay under the right feature.
- API-key + cookie spare — Self-mode only reads
calnode_sessionwhenextractAPIKey(r) == "", matchingRequireAuth’s key-first precedence; a stale cookie on an API-key call no longer leaves a session alive. - Regression test —
TestRevokeAllSessions_apiKeyCallerWithAStaleCookieStillRevokesEveryonecovers the dual-credential case the old key-only test could not catch.
Grok | X
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third of the features split out of #30.
POST /v1/auth/sessions/revoke-all.Two things behind one endpoint
With no body it drops all of the caller's sessions except the one that made the request. That is "sign out my other devices", as distinct from
POST /v1/auth/logout, which ends the current one. Losing a laptop no longer means waiting out a 30-day cookie. (An API-key caller has no current session, so for them every session goes — which is the right reading of "all of mine" for that caller.)With
{"user_id": "..."}it is an offboarding tool, gated on the tiersroles.goalready establishes: an admin may revoke a member, only the owner may revoke another admin, and the owner's sessions are reachable only by the owner.⛔ The ordering check is load-bearing
The actor's tier is checked before the target is loaded. If the target were loaded first, the 404 for an unknown id and the 403 for a protected one would be distinguishable, and the endpoint becomes a way for a member to enumerate user ids one guess at a time.
TestRevokeAllSessions_memberCannotProbeForUserIDsholds that, and it had to be written for this pull request rather than coming across with the rest. The neighbouring "member cannot target another user" test names a real user, so it passes with the two checks in either order and says nothing about the ordering at all. The new one asks for an existing id and a made-up one and requires the same status and the same body, because a differing message is the same oracle in prose.Moving the tier check below the target load fails that test and fails nothing else in the package, which is the evidence the gap was real rather than theoretical.
⛔ It cuts the MCP tokens too, and that is the point
The delete also covers the target's rows in
oauth_access_tokens, which is what makes this an offboarding tool rather than a convenience. An MCP connector (§19) authenticates with a bearer token, not the session cookie — so revoking sessions alone would leave a connected agent holding exactly the access that was just withdrawn, and the person who ran the revoke would have no way to know.Both deletes run in one transaction, so "revoked" is never half-true. A session sweep that succeeded while the token sweep failed is the worst possible outcome: it looks done and is not.
Verification
Mutation-checked rather than assumed: with the
oauth_access_tokensdelete replaced by a no-op,TestRevokeAllSessions_cutsMCPTokensTooandTestRevokeAllSessions_adminRevokesAMemberboth fail. Restored, they pass.go vet ./...andgo test ./...are clean on this branch — 26 packages, zero failures.Five files, +444. One new route, no schema change, no new dependency.
Not in this pull request
The commit this is split from also carried the SSO hand-off's documentation, since the two were written together. That half is removed here — it belongs on its own thread, as you asked on #30, and it will arrive changed rather than as you saw it. The owner bootstrap in that version was check-then-act, so two concurrent hand-offs claiming
owneron an unowned instance could both succeed. It needs a partial unique index rather than a code fix, which is a schema change and deserves its own discussion.🤖 Generated with Claude Code