Skip to content

Sign out everywhere, including the MCP connectors - #41

Merged
shockalotti merged 5 commits into
Calnode:mainfrom
distronode-corporation:feat/sign-out-everywhere
Sep 21, 2026
Merged

shockalotti merged 5 commits into
Calnode:mainfrom
distronode-corporation:feat/sign-out-everywhere

Conversation

@distronode-com

Copy link
Copy Markdown
Contributor

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 tiers roles.go already 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_memberCannotProbeForUserIDs holds 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_tokens delete replaced by a no-op, TestRevokeAllSessions_cutsMCPTokensToo and TestRevokeAllSessions_adminRevokesAMember both fail. Restored, they pass.

go vet ./... and go 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 owner on 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

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ 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_tokens rows 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.

ℹ️ 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

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using Grok | 𝕏

Comment thread CHANGELOG.md
Comment thread internal/handler/session.go
Third of the features split out of #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
distronode-com force-pushed the feat/sign-out-everywhere branch from c1e1722 to be05370 Compare September 9, 2026 22:52

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

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_session when extractAPIKey(r) == "", matching RequireAuth’s key-first precedence; a stale cookie on an API-key call no longer leaves a session alive.
  • Regression test — TestRevokeAllSessions_apiKeyCallerWithAStaleCookieStillRevokesEveryone covers the dual-credential case the old key-only test could not catch.

Pullfrog  | View workflow run | Using Grok | 𝕏

@shockalotti shockalotti left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The endpoint itself is solid — RBAC tiers mirror roles.go, the 404-before-tier-check kills the user-id oracle (pinned by test, including the same-body assertion), the API-key-precedence handling matches RequireAuth's confused-deputy guard, and the single-transaction session+token wipe is the right call. I want to merge this. What blocks it is the sentence it sits next to:

ARCHITECTURE §6 documents "Offboarding = archive" as the complete story, and this PR adds its revoke-all bullet directly above the archive bullet without touching it. I checked the tree, and archive currently leaves MCP access live:

  • internal/handler/mcp_oauth.go:105 (VerifyMCPBearer, OAuth branch) selects only user_id, expires_at from oauth_access_tokens — no archived_at check, while the session path (auth.go:103), the API-key path (auth.go:76), and the MCP API-key fallback (mcp_oauth.go:121) all filter on it.
  • ArchiveUser (internal/handler/archive.go:85) only sets archived_at; it deletes no sessions, keys, or tokens.

So today an archived member's connected agent bearer keeps working, and after this PR an operator reading §6 would archive someone and reasonably believe offboarding is done. Two-part ask:

  1. Close the hole (small, and arguably its own PR): add the archived_at IS NULL join to the OAuth branch of VerifyMCPBearer, mirroring the API-key fallback two lines below it. That is the actual bypass; everything else is belt-and-suspenders. Optionally also delete oauth_access_tokens inside ArchiveUser's transaction — note archive is documented as reversible, so say in one line whether restore resurrects agent access (it shouldn't).
  2. Then this PR's §6 hunk needs one honest sentence next to the archive bullet: archive alone does not end MCP access until (1) lands, or (after (1)) that it does.

Happy to take (1) as a fast follow-up and merge this the moment it lands, rather than growing this PR — but I don't want the §6 "offboarding" story to say two things that aren't both true at any point. (Also noted, no action: revoke-all deliberately leaves cno_ API keys alive, which is fine only because archived keys are already dead via the auth.go check — worth that one doc sentence so the next reader doesn't have to re-derive it.)

Review asked that §6 not tell two stories about offboarding. The revoke-all
bullet sat directly above "Offboarding = archive" while archive still left an
MCP client's OAuth bearer working, so an operator reading the pair could
archive someone and believe the agent was cut off too.

The companion pull request closes that: VerifyMCPBearer refuses an archived
member's OAuth token, and archive deletes the member's sessions, MCP tokens
and pending authorization codes. This bullet now says revoke-all signs
someone out rather than offboarding them, that archive ends MCP access by
itself, and why revoke-all leaving cno_ API keys alive is safe: an archived
member's keys are already refused on the key path in auth.go.

The sentence describes the tree once the companion lands, so this should
merge after it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The new §6 paragraph claims archive already ends MCP access. Neither cited mechanism exists on this branch.

Reviewed changes Delta since the prior pullfrog review (be05370 → e814e0f): documentation only, answering the request that §6 not describe offboarding as complete while archive leaves MCP live.

  • ARCHITECTURE §6 sign-out vs archive — Added that revoke-all is sign-out, not offboarding, and that archive is the offboarding path. The same sentences claim the OAuth bearer check refuses archived members and that archive deletes sessions and MCP tokens.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using Grok | 𝕏

Comment thread docs/ARCHITECTURE.md
…eting

The previous commit described archive as deleting a member's sessions and
MCP tokens. #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 #49 does.

It still describes the tree once #49 is in, so this merges after it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@distronode-com

Copy link
Copy Markdown
Contributor Author

Thanks for checking the tree, and for picking up (1) yourself in #49.

For (2), the §6 hunk here now says that revoke-all signs someone out rather than offboarding them, and that archive ends MCP access on its own: the OAuth bearer check and the refresh grant both refuse an archived member. It also says why leaving cno_ keys alive here is safe: the key path in auth.go already refuses an archived member (e814e0f, then 57db7dc to match #49, which deletes nothing on archive). That describes the tree once #49 is in, so this should merge after it.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Important

The rewritten §6 sentence still describes #49, which is not on this branch.

Reviewed changes Delta since the prior pullfrog review (e814e0f → 57db7dc): documentation only.

  • ARCHITECTURE §6 archive/MCP wording — Dropped the false claim that ArchiveUser deletes sessions and MCP tokens. Replaced it with a claim that the OAuth bearer check and the refresh grant already refuse archived members.

⚠️ Merge order with #49

That replacement matches companion PR #49, which is still open. This tree still validates an archived member's OAuth bearer (VerifyMCPBearer selects only user_id, expires_at) and still mints a refresh pair for them (tokenRefresh has no archived_at filter). Merging this first leaves operators with the same false offboarding story.

Technical details
# §6 must match this branch, not #49

## Affected sites
- `docs/ARCHITECTURE.md:201-203` — claims archive ends MCP access via bearer + refresh refusal
- `internal/handler/mcp_oauth.go` (`VerifyMCPBearer` OAuth branch) — no `users.archived_at` join
- `internal/handler/mcp_oauth_authorize.go` (`tokenRefresh`) — no archive filter
- PR #49 — implements those joins; state is open, not in this ancestry

## Required outcome
- §6 describes this branch until #49 lands, or this PR waits on #49 and then restates the positive claim.

## Suggested approach (optional)
- Keep the dropped delete claim gone (that part was right). Describe the remaining hole, or rebase onto merged #49.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using Grok | 𝕏

Comment thread docs/ARCHITECTURE.md
distronode-com and others added 2 commits September 16, 2026 22:20
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The branch was cut before 0.9.0, when its entry sat under [Unreleased].
0.9.0 renamed that section, and merging main moved the entry with it
without a conflict, so it read as something 0.9.0 shipped. Moved back
under [Unreleased], text unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

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 (57db7dc → 89c1e39): changelog placement after 0.9.0 shipped, and merge of main which lands #49 on this tree.

  • CHANGELOG Unreleased — Moved the Sign-out entry out of shipped [0.9.0] into [Unreleased], so 0.9.0 no longer claims this endpoint.
  • Merge of main — Brings #49 (eb04dca): VerifyMCPBearer and tokenRefresh now refuse archived members, so the §6 sentence is true of this tree.

Pullfrog  | View workflow run | Using Grok | 𝕏

@distronode-com

Copy link
Copy Markdown
Contributor Author

#49 is in, so the §6 text now describes main. Merged main in (78a7398). Like #39, this branch predates 0.9.0, and git filed its CHANGELOG entry under 0.9.0 without a conflict; 89c1e39 moves it back under [Unreleased], text unchanged. go vet and the full Go suite pass.

@shockalotti
shockalotti merged commit deb35c9 into Calnode:main Sep 21, 2026
5 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 21, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants