From 4bf35a2f7d015a87495eb140b337423b0c06dde3 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Tue, 12 May 2026 21:07:47 +0100 Subject: [PATCH 1/2] feat(auth): add revokeToken hook to attachLogoutCommand Optional pre-clear async hook so consumers can revoke the access token server-side as part of logout. Snapshot guard widened to fetch when either revokeToken or onCleared is supplied. Failures are swallowed so local clear always succeeds even when the server is unreachable. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 10 +++- src/auth/index.ts | 6 ++- src/auth/logout.test.ts | 110 ++++++++++++++++++++++++++++++++++++++++ src/auth/logout.ts | 33 +++++++++++- 4 files changed, 155 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index d27f97a..b3019db 100644 --- a/README.md +++ b/README.md @@ -182,6 +182,12 @@ import { attachLogoutCommand(auth, { store, + revokeToken: async ({ token }) => { + // Optional pre-clear server-side revocation. Errors are swallowed so + // local logout always succeeds — surface diagnostics via your own + // logging if you need them. + await api.revokeToken(token) + }, onCleared: ({ account, view }) => { // Optional follow-up — surface keyring-fallback warnings, etc. // Route extra prose to stderr in machine-output mode. @@ -207,11 +213,11 @@ attachTokenViewCommand(auth, { }) ``` -`attachLogoutCommand` snapshots `store.active()` (only when `onCleared` is supplied — skipped otherwise to avoid keyring / file I/O), calls `store.clear()`, then emits `✓ Logged out` (human) or `{ "ok": true }` (`--json`, silent under `--ndjson`) before firing `onCleared({ account, view, flags })`. +`attachLogoutCommand` snapshots `store.active()` (only when `revokeToken` or `onCleared` is supplied — skipped otherwise to avoid keyring / file I/O), optionally awaits `revokeToken({ token, account, view, flags })` for server-side revocation, calls `store.clear()`, then emits `✓ Logged out` (human) or `{ "ok": true }` (`--json`, silent under `--ndjson`) before firing `onCleared({ account, view, flags })`. `revokeToken` is skipped when no session is stored and any error it throws is swallowed so local logout always succeeds. `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. -Both attachers strip the standard `--json` / `--ndjson` registrar flags from the parsed options and pass the remainder to their callbacks as `flags` — same escape hatch `attachLoginCommand` uses, so a consumer can chain e.g. `.option('--user ')` and read it in `onCleared` / `renderText` / `fetchLive` / `renderJson` / `onNotAuthenticated`. +Both attachers strip the standard `--json` / `--ndjson` registrar flags from the parsed options and pass the remainder to their callbacks as `flags` — same escape hatch `attachLoginCommand` uses, so a consumer can chain e.g. `.option('--user ')` and read it in `revokeToken` / `onCleared` / `renderText` / `fetchLive` / `renderJson` / `onNotAuthenticated`. `attachTokenViewCommand` writes the bare stored token to stdout (no envelope, pipe-safe) and appends a trailing newline only when stdout is a TTY. When `envVarName` is set and the env var is populated, it throws `CliError('TOKEN_FROM_ENV', …)` to avoid disclosing a token the CLI did not manage. Defaults to subcommand name `token`; pass `name: 'view'` to nest under an existing `token` group. diff --git a/src/auth/index.ts b/src/auth/index.ts index 8423733..c60e527 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -4,7 +4,11 @@ export type { RunOAuthFlowOptions, RunOAuthFlowResult } from './flow.js' export { attachLoginCommand } from './login.js' export type { AttachLoginCommandOptions, AttachLoginContext } from './login.js' export { attachLogoutCommand } from './logout.js' -export type { AttachLogoutCommandOptions, AttachLogoutContext } from './logout.js' +export type { + AttachLogoutCommandOptions, + AttachLogoutContext, + AttachLogoutRevokeContext, +} from './logout.js' export { attachStatusCommand } from './status.js' export type { AttachStatusCommandOptions, AttachStatusContext } from './status.js' export { attachTokenViewCommand } from './token-view.js' diff --git a/src/auth/logout.test.ts b/src/auth/logout.test.ts index 36f7450..48e6a33 100644 --- a/src/auth/logout.test.ts +++ b/src/auth/logout.test.ts @@ -173,6 +173,116 @@ describe('attachLogoutCommand', () => { expect(order).toEqual(['active', 'clear', 'onCleared']) }) + it('invokes revokeToken with the snapshot before clear() runs', async () => { + const built = buildStore() + const order: string[] = [] + ;(built.store.active as ReturnType).mockImplementationOnce(async () => { + order.push('active') + return { token: 'tok', account } + }) + ;(built.store.clear as ReturnType).mockImplementationOnce(async () => { + order.push('clear') + }) + const revokeToken = vi.fn(async () => { + order.push('revoke') + }) + const onCleared = vi.fn(() => { + order.push('onCleared') + }) + const program = new Command() + program.exitOverride() + const auth = program.command('auth') + attachLogoutCommand(auth, { store: built.store, revokeToken, onCleared }) + + await program.parseAsync(['node', 'cli', 'auth', 'logout']) + + expect(revokeToken).toHaveBeenCalledWith({ + token: 'tok', + account, + view: { json: false, ndjson: false }, + flags: {}, + }) + expect(order).toEqual(['active', 'revoke', 'clear', 'onCleared']) + }) + + it('skips revokeToken when no prior session is stored but still clears', async () => { + const built = buildStore(null) + const revokeToken = vi.fn(async () => {}) + const program = new Command() + program.exitOverride() + const auth = program.command('auth') + attachLogoutCommand(auth, { store: built.store, revokeToken }) + + await program.parseAsync(['node', 'cli', 'auth', 'logout']) + + expect(revokeToken).not.toHaveBeenCalled() + expect(built.clearSpy).toHaveBeenCalledTimes(1) + }) + + it('swallows revokeToken failures and still clears and fires onCleared', async () => { + const built = buildStore() + const revokeToken = vi.fn(async () => { + throw new Error('network down') + }) + const onCleared = vi.fn() + const program = new Command() + program.exitOverride() + const auth = program.command('auth') + attachLogoutCommand(auth, { store: built.store, revokeToken, onCleared }) + + await expect(program.parseAsync(['node', 'cli', 'auth', 'logout'])).resolves.toBeDefined() + + expect(revokeToken).toHaveBeenCalledTimes(1) + expect(built.clearSpy).toHaveBeenCalledTimes(1) + expect(onCleared).toHaveBeenCalledWith({ + account, + view: { json: false, ndjson: false }, + flags: {}, + }) + }) + + it('reads store.active() when only revokeToken is supplied', async () => { + const built = buildStore() + const revokeToken = vi.fn(async () => {}) + const program = new Command() + program.exitOverride() + const auth = program.command('auth') + attachLogoutCommand(auth, { store: built.store, revokeToken }) + + await program.parseAsync(['node', 'cli', 'auth', 'logout']) + + expect(built.activeSpy).toHaveBeenCalledTimes(1) + expect(revokeToken).toHaveBeenCalledTimes(1) + expect(built.clearSpy).toHaveBeenCalledTimes(1) + }) + + it('exposes consumer-attached options in revokeToken flags', async () => { + const built = buildStore() + const revokeToken = vi.fn(async () => {}) + const program = new Command() + program.exitOverride() + const auth = program.command('auth') + const logout = attachLogoutCommand(auth, { store: built.store, revokeToken }) + logout.option('--user ', 'Multi-user selector') + + await program.parseAsync([ + 'node', + 'cli', + 'auth', + 'logout', + '--json', + '--user', + 'me@example', + ]) + + expect(revokeToken).toHaveBeenCalledWith({ + token: 'tok', + account, + view: { json: true, ndjson: false }, + flags: { user: 'me@example' }, + }) + }) + it('works without an onCleared callback', async () => { const program = new Command() program.exitOverride() diff --git a/src/auth/logout.ts b/src/auth/logout.ts index 347e187..43f25df 100644 --- a/src/auth/logout.ts +++ b/src/auth/logout.ts @@ -15,10 +15,28 @@ export type AttachLogoutContext = { flags: Record } +export type AttachLogoutRevokeContext = { + /** Live token from the snapshot — pass to the server-side revocation endpoint. */ + token: string + /** The account the token belongs to. Non-null because the hook is skipped when nothing is stored. */ + account: TAccount + view: Required + /** Same shape as `AttachLogoutContext.flags`. */ + flags: Record +} + export type AttachLogoutCommandOptions = { store: TokenStore /** Override the subcommand description. */ description?: string + /** + * Fires before `store.clear()` runs, when a prior session is stored. Use + * to call a server-side token-revocation endpoint. Errors are swallowed + * so local logout always succeeds even when the server is unreachable; + * surface diagnostics via your own logging if needed. Skipped entirely + * when `store.active()` returns `null`. + */ + revokeToken?(ctx: AttachLogoutRevokeContext): Promise /** * Fires after `store.clear()` resolves. Use to surface keyring-fallback * warnings or other CLI-specific follow-ups. Consumers in machine-output @@ -49,8 +67,21 @@ export function attachLogoutCommand( ndjson: Boolean(ndjson), } // Skip the keyring/file read when no callback consumes the snapshot. - const snapshot = options.onCleared ? await options.store.active() : null + const needsSnapshot = Boolean(options.revokeToken || options.onCleared) + const snapshot = needsSnapshot ? await options.store.active() : null const account = snapshot?.account ?? null + if (options.revokeToken && snapshot) { + try { + await options.revokeToken({ + token: snapshot.token, + account: snapshot.account, + view, + flags, + }) + } catch { + // Best-effort: server revoke failures must not block local clear. + } + } await options.store.clear() if (view.json) { console.log(formatJson({ ok: true })) From 0aba93847151d6d1d6285f4420d47d179bda7908 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Tue, 12 May 2026 21:21:12 +0100 Subject: [PATCH 2/2] refactor(auth): flip logout ordering and harden snapshot read Address PR review feedback: - Clear local store BEFORE awaiting revokeToken so a hung or failing server never blocks local logout. Token is preserved in the JS snapshot variable. - Catch store.active() failures and still proceed to clear() so keychain/file read errors don't block local cleanup. - Derive AttachLogoutRevokeContext from AttachLogoutContext via Omit so view/flags fields share a single source of truth. - Type revokeToken return as void | Promise to match onCleared. - Reduce test boilerplate via extended build() helper, drop unsafe vi.fn casts in favour of typed spies, and add a deferred-promise async-contract test plus a store.active()-throws case. - README "What's in it" auth row + prose now reflect the new ordering, swallow semantics, and the new exported context type. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 30 ++++---- src/auth/logout.test.ts | 152 +++++++++++++++++++++------------------- src/auth/logout.ts | 46 +++++++----- 3 files changed, 123 insertions(+), 105 deletions(-) diff --git a/README.md b/README.md index b3019db..030beba 100644 --- a/README.md +++ b/README.md @@ -12,20 +12,20 @@ npm install @doist/cli-core ## What's in it -| Module | Key exports | Purpose | -| -------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `auth` (subpath) | `attachLoginCommand`, `attachLogoutCommand`, `attachStatusCommand`, `attachTokenViewCommand`, `runOAuthFlow`, `createPkceProvider`, PKCE helpers, `AuthProvider` / `TokenStore` types | OAuth runtime plus the Commander attachers for ` [auth] login` / `logout` / `status` / `token`. Ships the standard public-client PKCE flow (`createPkceProvider`); `AuthProvider` and `TokenStore` are the escape hatches for DCR, OS-keychain, multi-account, etc. — consumers implement `TokenStore` directly (a single-user config-file version is ~30 LoC). `commander` (when using the attachers) and `open` (browser launch) are optional peer-deps. | -| `commands` (subpath) | `registerChangelogCommand`, `registerUpdateCommand` (+ semver helpers) | Commander wiring for cli-core's standard commands (e.g. ` changelog`, ` update`, ` update switch`). **Requires** `commander` as an optional peer-dep. | -| `config` | `getConfigPath`, `readConfig`, `readConfigStrict`, `writeConfig`, `updateConfig`, `CoreConfig`, `UpdateChannel` | Read / write a per-CLI JSON config file with typed error codes; `CoreConfig` is the shape of fields cli-core itself owns (extend it for per-CLI fields). | -| `empty` | `printEmpty` | Print an empty-state message gated on `--json` / `--ndjson` so machine consumers never see human strings on stdout. | -| `errors` | `CliError` | Typed CLI error class with `code` and exit-code mapping. | -| `global-args` | `parseGlobalArgs`, `createGlobalArgsStore`, `createAccessibleGate`, `createSpinnerGate`, `getProgressJsonlPath`, `isProgressJsonlEnabled` | Parse well-known global flags (`--json`, `--ndjson`, `--quiet`, `--verbose`, `--accessible`, `--no-spinner`, `--progress-jsonl`) and derive predicates from them. | -| `json` | `formatJson`, `formatNdjson` | Stable JSON / newline-delimited JSON formatting for stdout. | -| `markdown` (subpath) | `preloadMarkdown`, `renderMarkdown`, `TerminalRendererOptions` | Lazy-init terminal markdown renderer. **Requires** `marked` and `marked-terminal-renderer` as peer-deps — install only if your CLI uses this subpath. | -| `options` | `ViewOptions` | Type contract for `{ json?, ndjson? }` per-command options that machine-output gates derive from. | -| `spinner` | `createSpinner` | Loading spinner factory wrapping `yocto-spinner` with disable gates. | -| `terminal` | `isCI`, `isStderrTTY`, `isStdinTTY`, `isStdoutTTY` | TTY / CI detection helpers. | -| `testing` (subpath) | `describeEmptyMachineOutput` | Vitest helpers reusable by consuming CLIs (e.g. parametrised empty-state suite covering `--json` / `--ndjson` / human modes). | +| Module | Key exports | Purpose | +| -------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `auth` (subpath) | `attachLoginCommand`, `attachLogoutCommand`, `attachStatusCommand`, `attachTokenViewCommand`, `runOAuthFlow`, `createPkceProvider`, PKCE helpers, `AuthProvider` / `TokenStore` types, `AttachLogoutRevokeContext` | OAuth runtime plus the Commander attachers for ` [auth] login` / `logout` / `status` / `token`. `attachLogoutCommand` accepts an optional `revokeToken` hook for best-effort server-side token revocation. Ships the standard public-client PKCE flow (`createPkceProvider`); `AuthProvider` and `TokenStore` are the escape hatches for DCR, OS-keychain, multi-account, etc. — consumers implement `TokenStore` directly (a single-user config-file version is ~30 LoC). `commander` (when using the attachers) and `open` (browser launch) are optional peer-deps. | +| `commands` (subpath) | `registerChangelogCommand`, `registerUpdateCommand` (+ semver helpers) | Commander wiring for cli-core's standard commands (e.g. ` changelog`, ` update`, ` update switch`). **Requires** `commander` as an optional peer-dep. | +| `config` | `getConfigPath`, `readConfig`, `readConfigStrict`, `writeConfig`, `updateConfig`, `CoreConfig`, `UpdateChannel` | Read / write a per-CLI JSON config file with typed error codes; `CoreConfig` is the shape of fields cli-core itself owns (extend it for per-CLI fields). | +| `empty` | `printEmpty` | Print an empty-state message gated on `--json` / `--ndjson` so machine consumers never see human strings on stdout. | +| `errors` | `CliError` | Typed CLI error class with `code` and exit-code mapping. | +| `global-args` | `parseGlobalArgs`, `createGlobalArgsStore`, `createAccessibleGate`, `createSpinnerGate`, `getProgressJsonlPath`, `isProgressJsonlEnabled` | Parse well-known global flags (`--json`, `--ndjson`, `--quiet`, `--verbose`, `--accessible`, `--no-spinner`, `--progress-jsonl`) and derive predicates from them. | +| `json` | `formatJson`, `formatNdjson` | Stable JSON / newline-delimited JSON formatting for stdout. | +| `markdown` (subpath) | `preloadMarkdown`, `renderMarkdown`, `TerminalRendererOptions` | Lazy-init terminal markdown renderer. **Requires** `marked` and `marked-terminal-renderer` as peer-deps — install only if your CLI uses this subpath. | +| `options` | `ViewOptions` | Type contract for `{ json?, ndjson? }` per-command options that machine-output gates derive from. | +| `spinner` | `createSpinner` | Loading spinner factory wrapping `yocto-spinner` with disable gates. | +| `terminal` | `isCI`, `isStderrTTY`, `isStdinTTY`, `isStdoutTTY` | TTY / CI detection helpers. | +| `testing` (subpath) | `describeEmptyMachineOutput` | Vitest helpers reusable by consuming CLIs (e.g. parametrised empty-state suite covering `--json` / `--ndjson` / human modes). | ## Usage @@ -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), optionally awaits `revokeToken({ token, account, view, flags })` for server-side revocation, calls `store.clear()`, then emits `✓ Logged out` (human) or `{ "ok": true }` (`--json`, silent under `--ndjson`) before firing `onCleared({ account, view, flags })`. `revokeToken` is skipped when no session is stored and any error it throws is swallowed so local logout always succeeds. +`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. `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. diff --git a/src/auth/logout.test.ts b/src/auth/logout.test.ts index 48e6a33..8be90fc 100644 --- a/src/auth/logout.test.ts +++ b/src/auth/logout.test.ts @@ -6,6 +6,7 @@ import { attachLogoutCommand } from './logout.js' import type { TokenStore } from './types.js' type Account = { id: string; label?: string; email: string } +type LogoutOverrides = Partial>[1]> const account: Account = { id: '1', label: 'me', email: 'a@b' } @@ -27,11 +28,12 @@ function buildStore( } function build( - overrides: Partial>[1]> = {}, + overrides: LogoutOverrides = {}, storeOverride?: TokenStore, ): { program: Command store: TokenStore + logout: Command onCleared: ReturnType } { const { store } = storeOverride ? { store: storeOverride } : buildStore() @@ -39,12 +41,12 @@ function build( program.exitOverride() const auth = program.command('auth') const onCleared = vi.fn() - attachLogoutCommand(auth, { + const logout = attachLogoutCommand(auth, { store, onCleared, ...overrides, }) - return { program, store, onCleared } + return { program, store, logout, onCleared } } describe('attachLogoutCommand', () => { @@ -114,12 +116,9 @@ describe('attachLogoutCommand', () => { }) }) - it('skips store.active() when no onCleared callback is supplied', async () => { - const program = new Command() - program.exitOverride() - const auth = program.command('auth') + it('skips store.active() when no hook is supplied', async () => { const built = buildStore() - attachLogoutCommand(auth, { store: built.store }) + const { program } = build({ onCleared: undefined }, built.store) await program.parseAsync(['node', 'cli', 'auth', 'logout']) @@ -128,12 +127,8 @@ describe('attachLogoutCommand', () => { }) it('exposes consumer-attached options in flags but strips --json / --ndjson', async () => { - const program = new Command() - program.exitOverride() - const auth = program.command('auth') const built = buildStore() - const onCleared = vi.fn() - const logout = attachLogoutCommand(auth, { store: built.store, onCleared }) + const { program, logout, onCleared } = build({}, built.store) logout.option('--user ', 'Multi-user selector') await program.parseAsync([ @@ -153,65 +148,74 @@ describe('attachLogoutCommand', () => { }) }) - it('snapshots the active account before clear() runs', async () => { + it('runs hooks in order: active, clear, revoke, onCleared', async () => { const built = buildStore() const order: string[] = [] - ;(built.store.active as ReturnType).mockImplementationOnce(async () => { + built.activeSpy.mockImplementationOnce(async () => { order.push('active') return { token: 'tok', account } }) - ;(built.store.clear as ReturnType).mockImplementationOnce(async () => { + built.clearSpy.mockImplementationOnce(async () => { order.push('clear') }) + const revokeToken = vi.fn(async () => { + order.push('revoke') + }) const onCleared = vi.fn(() => { order.push('onCleared') }) - const { program } = build({ onCleared }, built.store) + const { program } = build({ revokeToken, onCleared }, built.store) await program.parseAsync(['node', 'cli', 'auth', 'logout']) - expect(order).toEqual(['active', 'clear', 'onCleared']) + expect(order).toEqual(['active', 'clear', 'revoke', 'onCleared']) }) - it('invokes revokeToken with the snapshot before clear() runs', async () => { + it('invokes revokeToken with the snapshot after clear() runs', async () => { const built = buildStore() - const order: string[] = [] - ;(built.store.active as ReturnType).mockImplementationOnce(async () => { - order.push('active') - return { token: 'tok', account } - }) - ;(built.store.clear as ReturnType).mockImplementationOnce(async () => { - order.push('clear') - }) - const revokeToken = vi.fn(async () => { - order.push('revoke') - }) - const onCleared = vi.fn(() => { - order.push('onCleared') - }) - const program = new Command() - program.exitOverride() - const auth = program.command('auth') - attachLogoutCommand(auth, { store: built.store, revokeToken, onCleared }) + const revokeToken = vi.fn(async () => {}) + const { program } = build({ revokeToken }, built.store) await program.parseAsync(['node', 'cli', 'auth', 'logout']) + expect(built.clearSpy).toHaveBeenCalledTimes(1) expect(revokeToken).toHaveBeenCalledWith({ token: 'tok', account, view: { json: false, ndjson: false }, flags: {}, }) - expect(order).toEqual(['active', 'revoke', 'clear', 'onCleared']) + }) + + it('awaits revokeToken before firing onCleared', async () => { + let resolveRevoke!: () => void + const revokeToken = vi.fn( + () => + new Promise((resolve) => { + resolveRevoke = resolve + }), + ) + const built = buildStore() + const { program, onCleared } = build({ revokeToken }, built.store) + + const parsing = program.parseAsync(['node', 'cli', 'auth', 'logout']) + // Flush microtasks so the action body advances past `store.clear()` into `revokeToken`. + await new Promise((resolve) => setImmediate(resolve)) + + expect(built.clearSpy).toHaveBeenCalledTimes(1) + expect(revokeToken).toHaveBeenCalledTimes(1) + expect(onCleared).not.toHaveBeenCalled() + + resolveRevoke() + await parsing + + expect(onCleared).toHaveBeenCalledTimes(1) }) it('skips revokeToken when no prior session is stored but still clears', async () => { const built = buildStore(null) const revokeToken = vi.fn(async () => {}) - const program = new Command() - program.exitOverride() - const auth = program.command('auth') - attachLogoutCommand(auth, { store: built.store, revokeToken }) + const { program } = build({ revokeToken, onCleared: undefined }, built.store) await program.parseAsync(['node', 'cli', 'auth', 'logout']) @@ -219,16 +223,12 @@ describe('attachLogoutCommand', () => { expect(built.clearSpy).toHaveBeenCalledTimes(1) }) - it('swallows revokeToken failures and still clears and fires onCleared', async () => { + it('swallows revokeToken failures and still fires onCleared', async () => { const built = buildStore() const revokeToken = vi.fn(async () => { throw new Error('network down') }) - const onCleared = vi.fn() - const program = new Command() - program.exitOverride() - const auth = program.command('auth') - attachLogoutCommand(auth, { store: built.store, revokeToken, onCleared }) + const { program, onCleared } = build({ revokeToken }, built.store) await expect(program.parseAsync(['node', 'cli', 'auth', 'logout'])).resolves.toBeDefined() @@ -241,13 +241,27 @@ describe('attachLogoutCommand', () => { }) }) + it('proceeds to clear when store.active() throws', async () => { + const built = buildStore() + built.activeSpy.mockRejectedValueOnce(new Error('keychain unavailable')) + const revokeToken = vi.fn(async () => {}) + const { program, onCleared } = build({ revokeToken }, built.store) + + await expect(program.parseAsync(['node', 'cli', 'auth', 'logout'])).resolves.toBeDefined() + + expect(built.clearSpy).toHaveBeenCalledTimes(1) + expect(revokeToken).not.toHaveBeenCalled() + expect(onCleared).toHaveBeenCalledWith({ + account: null, + view: { json: false, ndjson: false }, + flags: {}, + }) + }) + it('reads store.active() when only revokeToken is supplied', async () => { const built = buildStore() const revokeToken = vi.fn(async () => {}) - const program = new Command() - program.exitOverride() - const auth = program.command('auth') - attachLogoutCommand(auth, { store: built.store, revokeToken }) + const { program } = build({ revokeToken, onCleared: undefined }, built.store) await program.parseAsync(['node', 'cli', 'auth', 'logout']) @@ -259,10 +273,7 @@ describe('attachLogoutCommand', () => { it('exposes consumer-attached options in revokeToken flags', async () => { const built = buildStore() const revokeToken = vi.fn(async () => {}) - const program = new Command() - program.exitOverride() - const auth = program.command('auth') - const logout = attachLogoutCommand(auth, { store: built.store, revokeToken }) + const { program, logout } = build({ revokeToken, onCleared: undefined }, built.store) logout.option('--user ', 'Multi-user selector') await program.parseAsync([ @@ -283,34 +294,33 @@ describe('attachLogoutCommand', () => { }) }) + it('accepts a synchronous revokeToken', async () => { + const built = buildStore() + const revokeToken = vi.fn(() => {}) + const { program } = build({ revokeToken }, built.store) + + await program.parseAsync(['node', 'cli', 'auth', 'logout']) + + expect(revokeToken).toHaveBeenCalledTimes(1) + expect(built.clearSpy).toHaveBeenCalledTimes(1) + }) + it('works without an onCleared callback', async () => { - const program = new Command() - program.exitOverride() - const auth = program.command('auth') const built = buildStore() - attachLogoutCommand(auth, { store: built.store }) + const { program } = build({ onCleared: undefined }, built.store) await expect(program.parseAsync(['node', 'cli', 'auth', 'logout'])).resolves.toBeDefined() expect(built.clearSpy).toHaveBeenCalledTimes(1) }) it('returns the new Command so the consumer can chain', () => { - const program = new Command() - const auth = program.command('auth') - const built = buildStore() - const logout = attachLogoutCommand(auth, { store: built.store }) + const { logout } = build() expect(logout.name()).toBe('logout') }) it('honours a custom description', () => { - const program = new Command() - const auth = program.command('auth') - const built = buildStore() - const logout = attachLogoutCommand(auth, { - store: built.store, - description: 'Sign out of Todoist', - }) + const { logout } = build({ description: 'Sign out of Todoist' }) expect(logout.description()).toBe('Sign out of Todoist') }) diff --git a/src/auth/logout.ts b/src/auth/logout.ts index 43f25df..f106ec7 100644 --- a/src/auth/logout.ts +++ b/src/auth/logout.ts @@ -15,14 +15,14 @@ export type AttachLogoutContext = { flags: Record } -export type AttachLogoutRevokeContext = { +export type AttachLogoutRevokeContext = Omit< + AttachLogoutContext, + 'account' +> & { /** Live token from the snapshot — pass to the server-side revocation endpoint. */ token: string - /** The account the token belongs to. Non-null because the hook is skipped when nothing is stored. */ + /** Non-null because the hook is skipped when nothing was stored. */ account: TAccount - view: Required - /** Same shape as `AttachLogoutContext.flags`. */ - flags: Record } export type AttachLogoutCommandOptions = { @@ -30,15 +30,15 @@ export type AttachLogoutCommandOptions): Promise + revokeToken?(ctx: AttachLogoutRevokeContext): void | Promise /** - * Fires after `store.clear()` resolves. Use to surface keyring-fallback + * Fires after `revokeToken` settles. Use to surface keyring-fallback * warnings or other CLI-specific follow-ups. Consumers in machine-output * mode should route any extra prose to stderr to keep stdout parseable. */ @@ -46,10 +46,11 @@ export type AttachLogoutCommandOptions( parent: Command, @@ -68,8 +69,16 @@ export function attachLogoutCommand( } // Skip the keyring/file read when no callback consumes the snapshot. const needsSnapshot = Boolean(options.revokeToken || options.onCleared) - const snapshot = needsSnapshot ? await options.store.active() : null + let snapshot: { token: string; account: TAccount } | null = null + if (needsSnapshot) { + try { + snapshot = await options.store.active() + } catch { + // Snapshot lookup failures must not block local clear. + } + } const account = snapshot?.account ?? null + await options.store.clear() if (options.revokeToken && snapshot) { try { await options.revokeToken({ @@ -79,10 +88,9 @@ export function attachLogoutCommand( flags, }) } catch { - // Best-effort: server revoke failures must not block local clear. + // Best-effort: server revoke failures must not surface to the user. } } - await options.store.clear() if (view.json) { console.log(formatJson({ ok: true })) } else if (!view.ndjson) {