Repository navigation
feat(auth): personal API tokens for scripts and tools (#617) - #664
Monkey7539 wants to merge 3 commits into
Conversation
|
@Monkey7539 yessss! |
0971b16 to
fa67b58
Compare
|
Rebased onto main 0441d23 (after 4.1.0), still one commit, and the description is updated to match. What the rebase changed:
Results on the rebased commit: |
maathimself
left a comment
There was a problem hiding this comment.
I reviewed the whole diff, ran the full suite against PostgreSQL 16 (4,233 pass; lint and lint:plugins clean), and had an independent second review done. The allowlist, the session isolation and the CSRF change hold up: neither review found a way for a token to reach anything outside its scopes. What needs changing is how a token's creation is confirmed, plus a few revocation gaps.
Needed before merge
- SSO confirmation does not make the user sign in again.
action=reauthsends the normal authorize request, with nopromptormax_age, and the callback does not look atauth_time, so with a live session at the provider the round trip completes without any input.rememberOidcSessionalso records a confirmation at every SSO login. So at an unlocked browser, or with a stolen MailFlow session cookie in the 10 minutes after a login, an SSO-only account can get a 30-day token, while an account with a password always has to enter it or a two-factor code. Please:- send
prompt=loginandmax_age=0on the reauth authorize request; - store when the reauth flow started in
oidcPending, and in the callback refuse an id_token whoseauth_timeis missing or earlier than that (allowing a minute of clock skew), since not every provider honorsprompt; - record a confirmation only from the reauth flow, not from ordinary logins. That drops "an SSO sign-in counts" from the design; the token form's Confirm button covers it.
- send
fromScheduledon token drafts.routes/draft.jsdeletes the scheduled message named byfromScheduled(from 4.1). A token cannot learn an edit token today, since onlyroutes/scheduled.jsreturns one, but a draft-scope request should not carry a delete at all. Please refusefromScheduledin the draft guard, with a test.- Revocation gaps. Please delete tokens in the same transaction as each of these:
POST /api/totp/disable: the user's tokens confirmed withtotp.DELETE /api/auth/oidc/identities/:id: the user's tokens confirmed through that identity's provider, when they have no other identity left at it.PATCH /api/admin/oidc/:idchangingissuer_urlorclient_id: the provider's tokens, as forenabled: false.
- Policy race.
reauthForreads the sign-in policy before bcrypt or TOTP runs, and nothing checks it again at insert. If an admin requires two-factor or turns off password sign-in in between, the revoking DELETE cannot see the uncommitted row, and a password-confirmed token survives the change. Re-checking inside the transaction is not enough on its own at READ COMMITTED. IncreateToken's transaction, readmfa_enforcementandinternal_auth_disabledwithSELECT … FOR SHARE(both rows are seeded by migrations), and refuse ifconfirmedWithno longer qualifies. The settings PATCH updates those rows, so the two then wait for each other in either order.
Optional
- The bulk-archive guard refuses only All Mail. Running the resolved archive destination through
isProtectedDestination, as bulk-move does, would also cover an archive mapping that points at Trash or Junk.
Separately, linking a new identity (action=link) needs only a signed-in session, so whoever holds a session can link an identity of their own and then use it to confirm. That predates this PR and I'll track it in its own issue; it doesn't block this one.
|
Thanks for the thorough review, and for the second one. All four are done, plus the optional archive check, as a second commit (8539946) on top of the rebased feature, so the delta is easy to see. The description is updated to match. 1. SSO confirmation is a sign-in just made. Agreed on every part, and the point about
One consequence to be aware of: a provider that never emits 2. 3. Revocation gaps, each in the same transaction as the change:
4. Policy race. Optional: archive. Done. The resolved archive folder goes through Each new check was proved by breaking it and watching its test fail (11 mutations, all caught). Results: |
…ps (maathimself#617) Review round 1 of maathimself#664. - The token form's SSO confirmation (action=reauth) sends prompt=login and max_age=0, records when it began, and refuses an id_token whose auth_time is missing or earlier than that, less a minute of clock skew. An ordinary SSO sign-in records no confirmation any more. - A token draft may not carry fromScheduled, which deletes the scheduled message a draft was made from. - Tokens are deleted in the same transaction when a user turns their own two-factor off (those it confirmed), when they unlink their last identity at a provider (those confirmed through it), and when an admin points a provider at another issuer or client (as when turning it off). - createToken reads mfa_enforcement and internal_auth_disabled FOR SHARE in its transaction and refuses a confirmation the policy no longer allows, so a policy change and a creation wait for each other instead of racing. - bulk-archive holds the resolved archive folder to a move's rules, so a mapping at Trash or Junk is refused as well as All Mail. Ported to dev, which keeps the fork's migrations 0067_api_tokens and 0068_api_tokens_confirmation rather than the PR's 0072: confirmed_with stays nullable there, revokeTokensForPolicy keeps its COALESCE for rows made before 0068, and confirmationAllowed reads a null confirmed_with the same way, as a password confirmation. Only the oidc.js imports conflicted. Co-Authored-By: Claude Fable 5.1 <[email protected]> (cherry picked from commit 8539946)
… button MailFlow's token form (maathimself/mailflow#664, review round 1) now makes the provider ask for credentials again, and an ordinary sign-in no longer counts, so step 3 of Create a token said something that is no longer true. The 401 note lists the revocation rules as they are now: turning your own two-factor off deletes the tokens it confirmed, an administrator's reset deletes all of yours, and a provider's tokens go when it is turned off, deleted or pointed at another issuer or client, or when you unlink your last identity at it. Co-Authored-By: Claude Fable 5.1 <[email protected]>
The backend of maathimself#664 as dev carries it: scoped personal API tokens for the REST API, confirmed as the sign-in policy requires (a fresh SSO sign-in where there is no password), with a one-year maximum, a route allowlist that fails closed, the 1 MB body limit, per-token and per-user rate limits, revocation when the sign-in policy changes or a password is reset, the folder-role lookups the token guards use, the tokens kept out of backups and redacted from diagnostics, and the Settings routes that list, create and revoke them. Migrations 0067_api_tokens and 0068_api_tokens_confirmation. Co-Authored-By: Claude Fable 5.1 <[email protected]>
…#617) The Settings UI for maathimself#664: list, create (with the scope and expiry picker, the SSO confirmation step and the warning of what Read can see) and revoke personal API tokens, with its strings in every locale. Co-Authored-By: Claude Fable 5.1 <[email protected]>
…nlink Linking a single sign-on identity needs only a signed-in session, and nothing recorded it, so someone holding a session could link their own account at a provider and keep signing in through it, past a password reset and without the account's two-factor code. - Linking and unlinking go to the security log as identity_linked and identity_unlinked, with the admin's name when an admin unlinks. - A password reset, self-service or set by an admin, removes the user's linked identities as it does sessions and trusted devices, while password sign-in is on. With it off the new password cannot sign in, so the identities stay. - Admins see a user's linked identities in the user editor and can unlink one, which also signs the user out. Neither an admin nor the user can remove the last identity of a user with no other way to sign in: with none left they need a password and password sign-in turned on. Confirming before linking (part 1 of #714) follows on top of the fresh sign-in confirmation in #664. Refs #714
|
All four from the last review are in, and they hold up: the fresh SSO sign-in with
|
A bearer credential for clients that are not a browser tab, such as an MCP server, so they no longer need the account password and keep working with two-factor sign-in on. This is the backend of maathimself#617, as agreed there. - api_tokens (migration 0072) stores only the SHA-256 of each token, its scopes, its expiry, and what confirmed its creation. An admin switch, api_tokens_enabled, is off by default. - Only `Authorization: Bearer mfat_...` makes a token request. Any other Authorization header goes to the cookie flow untouched, so installs behind an auth proxy that adds its own Bearer header are unaffected. - A token reaches only an explicit allowlist of routes, matched by method and exact path and grouped into the read, organize and draft scopes. Every other route answers 403, the scheduled-send routes included: a token may draft, never send. - Guards keep organize reversible: no moves into Trash, Junk, Drafts, Snoozed or folders MailFlow does not sync, no archive into Gmail's All Mail, and drafts are never moved, archived or snoozed. Token drafts are new and plain text only. - A token request gets a request-only session: no cookie and no Redis write. The CSRF check and the screen lock move into modules that know about tokens, and the upload re-check re-reads the token row. - Creating a token asks for the current two-factor code, or else the password. An account that cannot sign in with a password confirms with a sign-in through its SSO provider in the last 10 minutes; the new /auth/oidc/:slug/start?action=reauth records one without signing in again. Under required two-factor, a user without an authenticator sets one up first. Expiries are 7, 30, 90 or 365 days, and 7 or 30 for SSO. - A password reset, now one transaction, and an admin setting a password both delete the user's tokens. So does a change that tightens sign-in, in the same transaction: password login turned off keeps only SSO-confirmed tokens, required two-factor deletes those confirmed with only a password, an admin turning off a user's two-factor deletes that user's tokens, and a provider turned off or deleted takes the tokens it confirmed. Co-Authored-By: Claude Fable 5.1 <[email protected]>
…ps (maathimself#617) Review round 1 of maathimself#664. - The token form's SSO confirmation (action=reauth) sends prompt=login and max_age=0, records when it began, and refuses an id_token whose auth_time is missing or earlier than that, less a minute of clock skew. An ordinary SSO sign-in records no confirmation any more. - A token draft may not carry fromScheduled, which deletes the scheduled message a draft was made from. - Tokens are deleted in the same transaction when a user turns their own two-factor off (those it confirmed), when they unlink their last identity at a provider (those confirmed through it), and when an admin points a provider at another issuer or client (as when turning it off). - createToken reads mfa_enforcement and internal_auth_disabled FOR SHARE in its transaction and refuses a confirmation the policy no longer allows, so a policy change and a creation wait for each other instead of racing. - bulk-archive holds the resolved archive folder to a move's rules, so a mapping at Trash or Junk is refused as well as All Mail. Co-Authored-By: Claude Fable 5.1 <[email protected]>
… per SSO sign-in (maathimself#617) Review round 2 of maathimself#664. - createToken re-reads the users row it locks and refuses the token when the password hash, two-factor flag or secret differ from what the confirmation was checked against. For a single sign-on confirmation it also holds the provider row (FOR SHARE) and the user's identities there (FOR KEY SHARE), and refuses when the provider is off or points at another issuer or client, or no identity is left. A revocation that committed first now refuses the token; one still to come waits for the commit and then deletes it with the rest. A two-connection PostgreSQL test holds each lock and watches the other side wait. - A single sign-on confirmation makes one token: the session forgets it and Redis keeps its nonce as spent, so two requests carrying the same one cannot both pass. The record also names the user it vouches for and the issuer and client the id_token was checked against. - The two password-reset paths remove linked identities before deleting tokens, so they take the sign-in policy row before the tokens, as an admin's policy change does, and neither waits on the other. The provider PATCH takes `enabled` only as a boolean: the column reads the string "false" as off, past the lockout check and the revocation. - Tests for the admin's unlink (maathimself#714), which shares the token revocation in services/userIdentities.js, and for the identities service. Co-Authored-By: Claude Fable 5.1 <[email protected]>
1aa38df to
7142ce9
Compare
|
Thanks for round 2. All five are in, as a third commit (7142ce9) on top of the rebase, so the delta is easy to see; the description is updated to match. 1. Migration. Rebased onto dd7944f; the migration is 2. A revocation at the same moment. Tests: a unit test per changed column and per provider and identity case, each pinning the statement, its lock clause and the order; PostgreSQL tests for each path against the real rows; route tests for the 409; and a two-connection PostgreSQL test ( 3. One token per SSO confirmation. Creating a token deletes the record from the session. Two requests racing on one session would both still carry it, so 4. Rebase onto #714's first part. Done; the conflicts were where you said. The unlink revocation now lives in Confirming before a link: I'd do it as the next PR, right after this one, so this diff stays about tokens. The pieces are ready for it: Also, from a second pass over the round:
5. Docs. The Each new check was proved by breaking it and watching its test fail (24 mutations, all caught). Results: |
The backend of maathimself#664 as dev carries it: scoped personal API tokens for the REST API, confirmed as the sign-in policy requires (a fresh SSO sign-in where there is no password), with a one-year maximum, a route allowlist that fails closed, the 1 MB body limit, per-token and per-user rate limits, revocation when the sign-in policy changes or a password is reset, the folder-role lookups the token guards use, the tokens kept out of backups and redacted from diagnostics, and the Settings routes that list, create and revoke them. Migrations 0067_api_tokens and 0068_api_tokens_confirmation. Carries maathimself#664's review rounds 1 and 2 (the PR's commits c6946cc and 7142ce9 on upstream dd7944f): the SSO confirmation is a fresh sign-in proved by auth_time and makes one token; tokens are deleted in the same transaction as the two-factor, unlink, provider and password-reset changes that invalidate them, with the unlink revocation in services/userIdentities.js next to upstream's admin unlink; createToken re-checks the users row, the sign-in policy, the provider and the identity under locks before it inserts. Dev-only: the two migrations above instead of the PR's 0074, so a token made before confirmations were recorded reads as confirmed with a password (COALESCE), with the tests for those rows. Co-Authored-By: Claude Fable 5.1 <[email protected]>
…#617) The Settings UI for maathimself#664: list, create (with the scope and expiry picker, the SSO confirmation step and the warning of what Read can see) and revoke personal API tokens, with its strings in every locale. The SSO hint says what maathimself#664's review round 2 made true: the Confirm button sends you to the provider to sign in again, each confirmation makes one token, and the provider must send the sign-in's auth_time. Co-Authored-By: Claude Fable 5.1 <[email protected]>
The backend of maathimself#664 as dev carries it: scoped personal API tokens for the REST API, confirmed as the sign-in policy requires (a fresh SSO sign-in where there is no password), with a one-year maximum, a route allowlist that fails closed, the 1 MB body limit, per-token and per-user rate limits, revocation when the sign-in policy changes or a password is reset, the folder-role lookups the token guards use, the tokens kept out of backups and redacted from diagnostics, and the Settings routes that list, create and revoke them. Migrations 0067_api_tokens and 0068_api_tokens_confirmation. Carries maathimself#664's review rounds 1 and 2 (the PR's commits c6946cc and 7142ce9 on upstream dd7944f): the SSO confirmation is a fresh sign-in proved by auth_time and makes one token; tokens are deleted in the same transaction as the two-factor, unlink, provider and password-reset changes that invalidate them, with the unlink revocation in services/userIdentities.js next to upstream's admin unlink; createToken re-checks the users row, the sign-in policy, the provider and the identity under locks before it inserts. Dev-only: the two migrations above instead of the PR's 0074, so a token made before confirmations were recorded reads as confirmed with a password (COALESCE), with the tests for those rows. Co-Authored-By: Claude Fable 5.1 <[email protected]>
…#617) The Settings UI for maathimself#664: list, create (with the scope and expiry picker, the SSO confirmation step and the warning of what Read can see) and revoke personal API tokens, with its strings in every locale. The SSO hint says what maathimself#664's review round 2 made true: the Confirm button sends you to the provider to sign in again, each confirmation makes one token, and the provider must send the sign-in's auth_time. Co-Authored-By: Claude Fable 5.1 <[email protected]>
The backend of maathimself#664 as dev carries it: scoped personal API tokens for the REST API, confirmed as the sign-in policy requires (a fresh SSO sign-in where there is no password), with a one-year maximum, a route allowlist that fails closed, the 1 MB body limit, per-token and per-user rate limits, revocation when the sign-in policy changes or a password is reset, the folder-role lookups the token guards use, the tokens kept out of backups and redacted from diagnostics, and the Settings routes that list, create and revoke them. Migrations 0067_api_tokens and 0068_api_tokens_confirmation. Carries maathimself#664's review rounds 1 and 2 (the PR's commits c6946cc and 7142ce9 on upstream dd7944f): the SSO confirmation is a fresh sign-in proved by auth_time and makes one token; tokens are deleted in the same transaction as the two-factor, unlink, provider and password-reset changes that invalidate them, with the unlink revocation in services/userIdentities.js next to upstream's admin unlink; createToken re-checks the users row, the sign-in policy, the provider and the identity under locks before it inserts. Dev-only: the two migrations above instead of the PR's 0074, so a token made before confirmations were recorded reads as confirmed with a password (COALESCE), with the tests for those rows. Co-Authored-By: Claude Fable 5.1 <[email protected]>
…#617) The Settings UI for maathimself#664: list, create (with the scope and expiry picker, the SSO confirmation step and the warning of what Read can see) and revoke personal API tokens, with its strings in every locale. The SSO hint says what maathimself#664's review round 2 made true: the Confirm button sends you to the provider to sign in again, each confirmation makes one token, and the provider must send the sign-in's auth_time. Co-Authored-By: Claude Fable 5.1 <[email protected]>
Summary
This is the backend of #617: personal API tokens, as settled in that thread. With a token, a script or tool can use MailFlow's API without the account password, and keeps working with two-factor sign-in on. My own case is an MCP server for Claude, running on another machine.
This is the first of the two PRs #617 describes. The Settings UI (the token card, the admin switch and 11 locales) is the second, and I'll open it once this one merges. The MCP server itself stays outside MailFlow, at https://github.com/Monkey7539/mailflow-mcp.
Changes
Storage. Migration 0074 adds
api_tokens. For each token it holds:trusted_devicesand reset links do;password,totporoidc), and for single sign-on, the provider.api_tokens_enabledin system_settings defaults to off. It is read through a 30 s cache, which the admin settings PATCH clears.Which requests are token requests. Only
Authorization: Bearer mfat_…, as agreed on #617:mfat_value that doesn't check out gets a 401, and any cookie on the request is ignored;Allowlist (
services/apiTokenAllowlist.js). Each entry is an exact method and path, compared withreq.originalUrl:/api/health,/api/version,/api/api-tokens/selfEvery other route answers 403, the scheduled-send routes included.
Guards after parsing (
middleware/apiTokenGuards.js):folderRoleshelper;existing*fields,fromScheduled(which deletes the scheduled message a draft was made from), any truthybodyIsHtml, andquotedBodyHtmlare refused;Session plumbing:
req.session: no cookie and no Redis write.mountApiAuth, so a test can check its order: token auth, body parsers, session, CSRF, screen lock, guards.reloadSessionre-checks the token after a large upload.requireAuthanswers a token whose user is gone with 401, and doesn't update the user's last-seen time for token requests.requireAdminrefuses tokens outright.Management API (
routes/apiTokens.js):reauth,expiryChoicesandmaxTokens;/self, so a token can see its own name and scopes.Confirming creation (
services/apiTokenReauth.js), as settled on #617:totp:<uid>limit as the sign-in challenge.auth:<ip>./auth/oidc/:slug/start?action=reauth:prompt=loginandmax_age=0, so the provider asks for credentials again and must putauth_timein the id_token;auth_timeis missing or earlier than that, less a minute of clock skew, since not every provider honoursprompt;SET NX, as the two-factor replay guard does), so two requests carrying the same confirmation cannot both pass;auth_timecannot confirm tokens. Entra ID sends it only with theauth_timeoptional claim configured; the Settings UI PR says so beside the Confirm button.Expiries are 7, 30, 90 or 365 days, and 7 or 30 for SSO-confirmed tokens. A wrong code or password is logged as
totp_failorlogin_fail. The confirmation is never answered with a 401, which the web client would take for an expired session.Revocation.
api_tokensis not restored.unlinkUserIdentityinservices/userIdentities.js, which both routes call.A change that tightens sign-in deletes the tokens it no longer allows, in the transaction that makes the change. The
[admin]log line says how many went:Neither the policy nor a revocation can race a creation.
createTokentakes its locks in the order the revocations take theirs, and checks under each what the confirmation relied on:FOR UPDATE, which a password reset or set, two-factor turned off and an unlink lock first: the password hash, two-factor flag and secret must still be what the confirmation was checked against;FOR SHARE, which must still be on and point at the issuer and client the id_token was checked against (recorded with the confirmation), and the user's identities thereFOR KEY SHARE, of which one must remain.FOR KEY SHAREblocks the unlink's delete, not a sign-in updatinglast_used_at;mfa_enforcementandinternal_auth_disabled,FOR SHARE.A change that committed first shows in the rows and the token is refused with 409 ("Confirm again"); one still to come waits for the commit, then sees the token and deletes it with the rest. Deleting a provider waits the same way and cascades.
Limits (numbers as agreed on #617):
apitoken-fail:<ip>, not the sign-in budget. Only malformed or unknown tokens spend it, so a stale token in a tool can't lock its owner out of web sign-in. An expired token always gets 401 and spends nothing;A refused body is drained up to 1 MB, then the connection is closed.
Logging. There are three auth events:
api_token_created,api_token_revokedandapi_token_auth_fail. Neither the token nor its hash is ever logged, and the diagnostics scrubber redactsmfat_.Where this differs from #617's original text
The thread settled the Bearer rule, the move and archive guards, re-authentication and expiry, the policy changes and the limits. These details were not in it:
revoked_at, so no lookup can forget to check a flag. The auth events keep the history.quotedBodyHtmlis stored unsanitized today.GET /api/accountsreturns a narrower shape to tokens: no hosts, ports, TLS settings, login names, signatures, folder mappings or plugin settings.req.securereflects whatever the client claims, so the check would mean little.Testing
Suite:
npm testpasses 4,099 tests; the 251 skipped are the PostgreSQL-only ones, which your CI now runs withMAILFLOW_TEST_DATABASE_URL.npm run lintandnpm run lint:pluginsare clean.What the new tests cover:
mountApiAuthchain on a real express-session, Basic credentials included;prompt,max_age,auth_time, a plain sign-in confirming nothing), each revocation path (the admin's unlink included), one token per SSO confirmation (two requests at once included), the re-check of what confirmed a token under its locks (each row and lock clause, in order), the policy re-check, and the limits.I proved each security check by breaking the code and watching its test fail.
PostgreSQL 16 and Redis, in a temporary workflow on my fork: https://github.com/Monkey7539/mailflow/actions/runs/41511033774 (the round-2 commit)
Not checked: multi-replica deployments. The search limiter is per process, as it is today.
Contributor License Agreement
By submitting this pull request I confirm that:
third-party material and confirmed it is compatible with the CLA).
🤖 Generated with Claude Code