diff --git a/README.md b/README.md index 1ea057f..249554a 100644 --- a/README.md +++ b/README.md @@ -213,7 +213,7 @@ attachTokenViewCommand(auth, { }) ``` -`attachLogoutCommand` snapshots `store.active()` (only when `revokeToken` or `onCleared` is supplied — skipped otherwise to avoid keyring / file I/O), calls `store.clear()`, then awaits `revokeToken({ token, account, view, flags })` for best-effort server-side revocation, emits `✓ Logged out` (human) or `{ "ok": true }` (`--json`, silent under `--ndjson`), and finally fires `onCleared({ account, view, flags })`. Local clear runs first so logout always succeeds locally even if the snapshot read fails or the server is unreachable; both `store.active()` and `revokeToken` errors are swallowed (`revokeToken` is also skipped when no session was stored). The exported `AttachLogoutRevokeContext` is the ctx type for typing standalone revoke implementations. +`attachLogoutCommand` snapshots `store.active(ref)` when either `--user ` is supplied or one of the consumer hooks (`revokeToken` / `onCleared`) needs the prior account, calls `store.clear(ref)`, awaits `revokeToken({ token, account, ref, view, flags })` for best-effort server-side revocation, emits `✓ Logged out` (human) or `{ "ok": true }` (`--json`, silent under `--ndjson`), and finally fires `onCleared({ account, ref, view, flags })`. `ref` is the parsed `--user` argument (or `undefined`) so consumers can distinguish "nothing was stored" (`account: null`, `ref: undefined`) from "cleared an unreadable record by ref" (`account: null`, `ref: "me"`). `revokeToken` failures are always swallowed; the pre-flight snapshot's error contract is covered in the `--user ` section below. The exported `AttachLogoutRevokeContext` is the ctx type for typing standalone revoke implementations. `attachStatusCommand` reads `store.active()`, optionally runs `fetchLive` (consumer translates 401 → `CliError('NO_TOKEN', …)`), then dispatches to `renderJson` (`--json` / `--ndjson` via `formatJson` / `formatNdjson`, defaults to the account itself, **only invoked in machine-output mode**) or `renderText` (human mode, string or array of lines). When the store is empty it throws `CliError('NOT_AUTHENTICATED', 'Not signed in.')` unless `onNotAuthenticated` is supplied. @@ -338,6 +338,8 @@ Account-selection resolvers (env > `--user` > default > single-only > error), `a `ACCOUNT_NOT_FOUND` is thrown by the account-touching attachers when `--user ` was supplied but `store.active(ref)` returned `null`. `NO_ACCOUNT_SELECTED` is reserved for consumer-thrown resolver failures (multiple accounts stored, no default, no `--user`); cli-core does not throw it itself. +A `TokenStore` MAY throw `CliError('AUTH_STORE_READ_FAILED', …)` from `active(ref)` when a matching record exists but the token itself can't be read (e.g. an OS keyring backing the store is offline). `attachLogoutCommand` catches this specific code on the explicit-ref path and proceeds with `clear(ref)` — local logout doesn't need the token, and the `revokeToken` hook is skipped because there's no token to send. Every other error from `active(ref)` (notably `ACCOUNT_NOT_FOUND` from a genuine ref miss, plus any consumer-thrown code) still propagates so a real miss isn't masked. Without `--user`, the logout pre-flight swallows any snapshot read failure so the local clear always runs. `attachStatusCommand` and `attachTokenViewCommand` propagate `AUTH_STORE_READ_FAILED` since they have no way to render or print without the token. + #### Custom `AuthProvider` (non-PKCE flows) Implement `AuthProvider` directly for Dynamic Client Registration, device code, magic-link, etc. The four hooks fire in this order during `runOAuthFlow`: diff --git a/src/auth/errors.ts b/src/auth/errors.ts index 26d6318..bf77311 100644 --- a/src/auth/errors.ts +++ b/src/auth/errors.ts @@ -9,6 +9,7 @@ export type AuthErrorCode = | 'AUTH_PORT_BIND_FAILED' | 'AUTH_TOKEN_EXCHANGE_FAILED' | 'AUTH_STORE_WRITE_FAILED' + | 'AUTH_STORE_READ_FAILED' | 'NOT_AUTHENTICATED' | 'TOKEN_FROM_ENV' | 'NO_ACCOUNT_SELECTED' diff --git a/src/auth/logout.test.ts b/src/auth/logout.test.ts index e161583..d16c060 100644 --- a/src/auth/logout.test.ts +++ b/src/auth/logout.test.ts @@ -74,6 +74,7 @@ describe('attachLogoutCommand', () => { expect(logSpy).toHaveBeenCalledWith('✓ Logged out') expect(onCleared).toHaveBeenCalledWith({ account, + ref: undefined, view: { json: false, ndjson: false }, flags: {}, }) @@ -87,6 +88,7 @@ describe('attachLogoutCommand', () => { expect(logSpy).toHaveBeenCalledWith(formatJson({ ok: true })) expect(onCleared).toHaveBeenCalledWith({ account, + ref: undefined, view: { json: true, ndjson: false }, flags: {}, }) @@ -100,6 +102,7 @@ describe('attachLogoutCommand', () => { expect(logSpy).not.toHaveBeenCalled() expect(onCleared).toHaveBeenCalledWith({ account, + ref: undefined, view: { json: false, ndjson: true }, flags: {}, }) @@ -114,6 +117,7 @@ describe('attachLogoutCommand', () => { expect(built.clearSpy).toHaveBeenCalledTimes(1) expect(onCleared).toHaveBeenCalledWith({ account: null, + ref: undefined, view: { json: false, ndjson: false }, flags: {}, }) @@ -149,6 +153,7 @@ describe('attachLogoutCommand', () => { expect(built.clearSpy).toHaveBeenCalledWith('me@example') expect(onCleared).toHaveBeenCalledWith({ account, + ref: 'me@example', view: { json: true, ndjson: false }, flags: { full: true }, }) @@ -188,6 +193,7 @@ describe('attachLogoutCommand', () => { expect(revokeToken).toHaveBeenCalledWith({ token: 'tok', account, + ref: undefined, view: { json: false, ndjson: false }, flags: {}, }) @@ -242,6 +248,7 @@ describe('attachLogoutCommand', () => { expect(built.clearSpy).toHaveBeenCalledTimes(1) expect(onCleared).toHaveBeenCalledWith({ account, + ref: undefined, view: { json: false, ndjson: false }, flags: {}, }) @@ -259,6 +266,7 @@ describe('attachLogoutCommand', () => { expect(revokeToken).not.toHaveBeenCalled() expect(onCleared).toHaveBeenCalledWith({ account: null, + ref: undefined, view: { json: false, ndjson: false }, flags: {}, }) @@ -297,6 +305,7 @@ describe('attachLogoutCommand', () => { expect(revokeToken).toHaveBeenCalledWith({ token: 'tok', account, + ref: 'me@example', view: { json: true, ndjson: false }, flags: { full: true }, }) @@ -343,6 +352,7 @@ describe('attachLogoutCommand', () => { expect(built.clearSpy).toHaveBeenCalledWith('alice@example') expect(onCleared).toHaveBeenCalledWith({ account, + ref: 'alice@example', view: { json: false, ndjson: false }, flags: {}, }) @@ -364,4 +374,64 @@ describe('attachLogoutCommand', () => { expect(built.clearSpy).not.toHaveBeenCalled() expect(logSpy).not.toHaveBeenCalled() }) + + it('proceeds with clear(ref) when active(ref) throws AUTH_STORE_READ_FAILED', async () => { + // A matching record exists but the keyring is offline, so the + // pre-flight can't return a snapshot. `logout --user me` should + // still clear the record (it doesn't need the token); only the + // optional `revokeToken` is skipped because there's no token to + // send to the server. + const built = buildStore() + built.activeSpy.mockRejectedValueOnce( + new CliError('AUTH_STORE_READ_FAILED', 'keyring offline'), + ) + const revokeSpy = vi.fn() + const { program, onCleared } = build({ revokeToken: revokeSpy }, built.store) + + await program.parseAsync(['node', 'cli', 'auth', 'logout', '--user', 'me']) + + expect(built.clearSpy).toHaveBeenCalledWith('me') + expect(revokeSpy).not.toHaveBeenCalled() + expect(logSpy).toHaveBeenCalledWith('✓ Logged out') + // `account` is null (no readable snapshot) but `ref` is populated, so + // consumers can distinguish "nothing was stored" from "cleared an + // unreadable record". + expect(onCleared).toHaveBeenCalledWith({ + account: null, + ref: 'me', + view: { json: false, ndjson: false }, + flags: {}, + }) + }) + + it('tolerates AUTH_STORE_READ_FAILED when --user alone triggers the pre-flight (no hooks)', async () => { + // With neither `revokeToken` nor `onCleared` set, the snapshot only + // runs because `--user ` was supplied. The recovery branch must + // still kick in there — otherwise `logout --user me` would abort with + // `AUTH_STORE_READ_FAILED` in the bare-config case. + const built = buildStore() + built.activeSpy.mockRejectedValueOnce( + new CliError('AUTH_STORE_READ_FAILED', 'keyring offline'), + ) + const { program } = build({ onCleared: undefined }, built.store) + + await program.parseAsync(['node', 'cli', 'auth', 'logout', '--user', 'me']) + + expect(built.clearSpy).toHaveBeenCalledWith('me') + expect(logSpy).toHaveBeenCalledWith('✓ Logged out') + }) + + it('still propagates non-read errors from the snapshot pre-flight', async () => { + const thrown = new CliError('AUTH_STORE_WRITE_FAILED', 'something else') + const built = buildStore() + built.activeSpy.mockRejectedValueOnce(thrown) + const { program } = build({ onCleared: undefined }, built.store) + + // Exact-instance match (`toBe`) — a wrap-and-rethrow with the same + // code would otherwise pass. + await expect( + program.parseAsync(['node', 'cli', 'auth', 'logout', '--user', 'me']), + ).rejects.toBe(thrown) + expect(built.clearSpy).not.toHaveBeenCalled() + }) }) diff --git a/src/auth/logout.ts b/src/auth/logout.ts index 00a1026..941debc 100644 --- a/src/auth/logout.ts +++ b/src/auth/logout.ts @@ -1,12 +1,20 @@ import type { Command } from 'commander' +import { CliError } from '../errors.js' import { formatJson } from '../json.js' import type { ViewOptions } from '../options.js' -import type { AuthAccount, TokenStore } from './types.js' +import type { AccountRef, AuthAccount, TokenStore } from './types.js' import { attachUserFlag, extractUserRef, requireSnapshotForRef } from './user-flag.js' export type AttachLogoutContext = { - /** The account that was active immediately before `clear()` ran, or `null` if nothing was stored. */ + /** + * The account that was active immediately before `clear()` ran, or + * `null` if nothing was stored. Also `null` on the `AUTH_STORE_READ_FAILED` + * recovery path (matching record existed but the token wasn't readable); + * `ref` is still populated in that case so consumers can distinguish. + */ account: TAccount | null + /** The `--user ` value, or `undefined` when not supplied. Always present so consumers can tell "no stored account" from "cleared an unreadable account by ref". */ + ref: AccountRef | undefined /** `--json` / `--ndjson` flag values, both present (defaulted to `false`). */ view: Required /** Consumer-attached options. The registrar flags (`--json`, `--ndjson`, `--user`) are stripped. */ @@ -66,19 +74,31 @@ export function attachLogoutCommand( ndjson: Boolean(ndjson), } const ref = extractUserRef(cmd) - // Explicit ref must surface a typed miss before `clear()` runs — - // `clear(ref)` is contractually a no-op on miss, so otherwise - // `logout --user mistake` would print `✓ Logged out`. + // Snapshot only when something downstream needs it: + // - an explicit `--user ` must surface a typed miss before + // `clear()` runs (otherwise `logout --user mistake` would print + // `✓ Logged out` after a no-op clear); + // - either consumer hook is supplied and wants the prior account. + // `requireSnapshotForRef` handles both `ref === undefined` (returns + // the snapshot directly) and the explicit-ref path (throws + // `ACCOUNT_NOT_FOUND` on a null snapshot), so one call covers both. const needsSnapshot = ref !== undefined || Boolean(options.revokeToken || options.onCleared) let snapshot: { token: string; account: TAccount } | null = null if (needsSnapshot) { - if (ref !== undefined) { + try { snapshot = await requireSnapshotForRef(options.store, ref) - } else { - try { - snapshot = await options.store.active(ref) - } catch { - // Snapshot lookup failures must not block local clear. + } catch (error) { + // Without an explicit ref, any snapshot read failure must + // not block local clear (we just lose the hooks' account + // context). With an explicit ref, `AUTH_STORE_READ_FAILED` + // is the only recoverable case — clear can still run + // without the token. Everything else (notably + // `ACCOUNT_NOT_FOUND` from a genuine ref miss) propagates. + if ( + ref !== undefined && + !(error instanceof CliError && error.code === 'AUTH_STORE_READ_FAILED') + ) { + throw error } } } @@ -89,6 +109,7 @@ export function attachLogoutCommand( await options.revokeToken({ token: snapshot.token, account: snapshot.account, + ref, view, flags, }) @@ -101,6 +122,6 @@ export function attachLogoutCommand( } else if (!view.ndjson) { console.log('✓ Logged out') } - await options.onCleared?.({ account, view, flags }) + await options.onCleared?.({ account, ref, view, flags }) }) } diff --git a/src/auth/status.test.ts b/src/auth/status.test.ts index d789064..9af64ee 100644 --- a/src/auth/status.test.ts +++ b/src/auth/status.test.ts @@ -246,4 +246,19 @@ describe('attachStatusCommand', () => { code: 'ACCOUNT_NOT_FOUND', }) }) + + it('surfaces AUTH_STORE_READ_FAILED end-to-end (does not collapse to ACCOUNT_NOT_FOUND)', async () => { + const thrown = new CliError('AUTH_STORE_READ_FAILED', 'keyring offline') + const built = buildStore() + built.activeSpy.mockRejectedValueOnce(thrown) + const { program } = build({}, built.store) + + // Assert the exact thrown instance bubbles through unchanged — a + // future refactor that recreates the error with the same code + // (losing the original `cause` / stack) would still satisfy an + // `objectContaining({ code })` check. + await expect( + program.parseAsync(['node', 'cli', 'auth', 'status', '--user', 'me']), + ).rejects.toBe(thrown) + }) }) diff --git a/src/auth/types.ts b/src/auth/types.ts index e398b25..4949389 100644 --- a/src/auth/types.ts +++ b/src/auth/types.ts @@ -81,7 +81,15 @@ export type AccountRef = string * the README example). */ export type TokenStore = { - /** Active snapshot, or `null` when nothing matches (the attachers translate a ref miss into `ACCOUNT_NOT_FOUND`). */ + /** + * Active snapshot, or `null` when nothing matches (the attachers translate + * a ref miss into `ACCOUNT_NOT_FOUND`). A store MAY throw + * `CliError('AUTH_STORE_READ_FAILED', …)` when a matching record exists + * but the token itself can't be read (e.g. an OS keyring backing the + * store is offline) — `attachLogoutCommand` catches that code on the + * explicit-ref path and proceeds with `clear(ref)`; `attachStatusCommand` + * and `attachTokenViewCommand` propagate it. + */ active(ref?: AccountRef): Promise<{ token: string; account: TAccount } | null> /** Persist `token` for `account`, replacing any previous entry. Throw `CliError` for typed failures; other thrown values become `AUTH_STORE_WRITE_FAILED`. */ set(account: TAccount, token: string): Promise