diff --git a/README.md b/README.md index 4717c5f..0bf9ff8 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` / `AccountRef` 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 (single-user store implements `list` / `setDefault` trivially against the one account). `logout` / `status` / `token` always attach `--user ` and thread the parsed ref to `store.active(ref)` (and `store.clear(ref)` on `logout`). `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`, `stripUserFlag`, `createGlobalArgsStore`, `createAccessibleGate`, `createSpinnerGate`, `getProgressJsonlPath`, `isProgressJsonlEnabled` | Parse well-known global flags (`--json`, `--ndjson`, `--quiet`, `--verbose`, `--accessible`, `--no-spinner`, `--progress-jsonl`, `--user `) and derive predicates from them. `stripUserFlag` removes `--user` tokens from argv so the cleaned array can be forwarded to Commander when the flag has no root-program attachment. | -| `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, `createKeyringTokenStore`, `createSecureStore`, `migrateLegacyAuth`, `AuthProvider` / `TokenStore` / `AccountRef` / `UserRecordStore` 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`) and a multi-account keyring-backed `TokenStore` (`createKeyringTokenStore`) that stores secrets in the OS credential manager and degrades to plaintext in the consumer's config when the keyring is unavailable (WSL/headless Linux/containers). `AuthProvider` and `TokenStore` remain the escape hatches for DCR or fully custom backends. `logout` / `status` / `token` always attach `--user ` and thread the parsed ref to `store.active(ref)` (and `store.clear(ref)` on `logout`). `commander` (when using the attachers), `open` (browser launch), and `@napi-rs/keyring` (when using the keyring store) are optional peer/optional 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`, `stripUserFlag`, `createGlobalArgsStore`, `createAccessibleGate`, `createSpinnerGate`, `getProgressJsonlPath`, `isProgressJsonlEnabled` | Parse well-known global flags (`--json`, `--ndjson`, `--quiet`, `--verbose`, `--accessible`, `--no-spinner`, `--progress-jsonl`, `--user `) and derive predicates from them. `stripUserFlag` removes `--user` tokens from argv so the cleaned array can be forwarded to Commander when the flag has no root-program attachment. | +| `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 @@ -286,7 +286,71 @@ export const tokenStore: TokenStore = { } ``` -For multi-account storage (OS keychain, per-user config slots, …), implement the same five methods against your backend and honour `ref` on `active` / `clear` / `setDefault`. `AccountRef` is an opaque `string` — cli-core does not constrain matching semantics (id-exact, email-case-insensitive, label, …). The store impl owns that. +For multi-account storage (OS keychain, per-user config slots, …), implement the same five methods against your backend and honour `ref` on `active` / `clear` / `setDefault`. `AccountRef` is an opaque `string` — cli-core does not constrain matching semantics (id-exact, email-case-insensitive, label, …). The store impl owns that. For the OS-keychain case, see the `createKeyringTokenStore` factory below. + +#### Multi-account keyring-backed `TokenStore` + +`createKeyringTokenStore` wires `@napi-rs/keyring` into the `TokenStore` contract for multi-account CLIs. Secrets live in the OS credential manager (Keychain / Credential Manager / libsecret); per-user metadata stays in the consumer's config via a small `UserRecordStore` port. When the keyring is unreachable — common on WSL without D-Bus, headless Linux, containers, and CI — the store transparently falls back to a `fallbackToken` field on the user record and exposes a warning on `getLastStorageResult()` for the login command to surface. + +```ts +import { createKeyringTokenStore, type UserRecordStore } from '@doist/cli-core/auth' + +type Account = { id: string; label?: string; email: string } + +// Adapter over the consumer's existing config.json shape. +const userRecords: UserRecordStore = { + async list() { + /* read from config */ + }, + async upsert(record) { + /* replace, do not merge — see UserRecordStore docs */ + }, + async remove(id) { + /* … */ + }, + async getDefaultId() { + /* … */ + }, + async setDefaultId(id) { + /* … */ + }, + describeLocation() { + return '~/.config/todoist-cli/config.json' + }, +} + +export const tokenStore = createKeyringTokenStore({ + serviceName: 'todoist-cli', + userRecords, +}) + +// In your login command's onSuccess: +const storage = tokenStore.getLastStorageResult() +if (storage?.warning) console.error('Warning:', storage.warning) +``` + +The returned store satisfies the full `TokenStore` contract — including `list()` / `setDefault(ref)` / `ref`-aware `active` / `clear` — so it plugs straight into the `logout` / `status` / `token` attachers. Default ref matching is `account.id === ref || account.label === ref`; override `matchAccount` to broaden it (e.g. email-case-insensitive). + +`@napi-rs/keyring` is already declared in cli-core's own `optionalDependencies`, so npm pulls it in transitively when you install `@doist/cli-core` — your consumer CLI does not need to add it explicitly. The library ships pre-built native binaries for Windows, macOS, and Linux (glibc + musl); cli-core dynamic-imports it so a missing binary on an exotic arch surfaces as `SecureStoreUnavailableError` and triggers the same plaintext fallback path rather than crashing module load. + +For one-time migration of a v1 single-user token into the v2 multi-user shape, use `migrateLegacyAuth` from a postinstall hook: + +```ts +import { migrateLegacyAuth } from '@doist/cli-core/auth' + +await migrateLegacyAuth({ + serviceName: 'todoist-cli', + legacyAccount: 'api-token', + userRecords, + loadLegacyPlaintextToken: async () => readConfig().api_token ?? null, + identifyAccount: async (token) => fetchUser(token), + cleanupLegacyConfig: async () => clearLegacyAuthFields(), + silent: true, + logPrefix: 'todoist-cli', +}) +``` + +For sync/lazy-decrypt or fully bespoke backends, implement `TokenStore` directly. #### `--user ` and multi-user wiring diff --git a/package-lock.json b/package-lock.json index 8b0389e..487a68a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -32,6 +32,9 @@ "engines": { "node": ">=20.18.1" }, + "optionalDependencies": { + "@napi-rs/keyring": "1.3.0" + }, "peerDependencies": { "commander": ">=14", "marked": ">=18", @@ -654,6 +657,226 @@ "dev": true, "license": "MIT" }, + "node_modules/@napi-rs/keyring": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring/-/keyring-1.3.0.tgz", + "integrity": "sha512-WrOw/bcXm0f9qHkumlT1QlArXSTWqaY9sunsDpOk+yCCorCKMxvWT/a3xko4EYHVdeZoh00yI2TydXn6eyICDA==", + "license": "MIT", + "optional": true, + "engines": { + "node": ">= 10" + }, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/Brooooooklyn" + }, + "optionalDependencies": { + "@napi-rs/keyring-darwin-arm64": "1.3.0", + "@napi-rs/keyring-darwin-x64": "1.3.0", + "@napi-rs/keyring-freebsd-x64": "1.3.0", + "@napi-rs/keyring-linux-arm-gnueabihf": "1.3.0", + "@napi-rs/keyring-linux-arm64-gnu": "1.3.0", + "@napi-rs/keyring-linux-arm64-musl": "1.3.0", + "@napi-rs/keyring-linux-riscv64-gnu": "1.3.0", + "@napi-rs/keyring-linux-x64-gnu": "1.3.0", + "@napi-rs/keyring-linux-x64-musl": "1.3.0", + "@napi-rs/keyring-win32-arm64-msvc": "1.3.0", + "@napi-rs/keyring-win32-ia32-msvc": "1.3.0", + "@napi-rs/keyring-win32-x64-msvc": "1.3.0" + } + }, + "node_modules/@napi-rs/keyring-darwin-arm64": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-darwin-arm64/-/keyring-darwin-arm64-1.3.0.tgz", + "integrity": "sha512-pl76hJvdYUBn6I24bXiOBMA9nbDapo3I5B+f3OorjDU4dUMSypXeKbOVehJe8fhgTiH24flMyTS3aAIy43xegQ==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-darwin-x64": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-darwin-x64/-/keyring-darwin-x64-1.3.0.tgz", + "integrity": "sha512-YcJtEV5LA3cvA4z3BurgxH5IhTsW1JfIvcAAcqcecwk06Si9F9NqkxbZVIfDwQ8oRHgaBmT3zZJnLAotCrVahw==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-freebsd-x64": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-freebsd-x64/-/keyring-freebsd-x64-1.3.0.tgz", + "integrity": "sha512-vlLf31TGhfRAaxLDBhg8b89ss0HHD/lyNmL5F3UjSaz5CUXElsJmKYq9fqA/B+cZKUEUcLHHGhF0I/CqcFdaVw==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "freebsd" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-arm-gnueabihf": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-arm-gnueabihf/-/keyring-linux-arm-gnueabihf-1.3.0.tgz", + "integrity": "sha512-KiWdMMu/Inz/bHHIAGrnF7r54FZDYXuHO6UFF/rhIrshUsxbMG1Rl9lEymNtqqsVo927G0VYcb02FzWQ3iBQRQ==", + "cpu": [ + "arm" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-arm64-gnu": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-arm64-gnu/-/keyring-linux-arm64-gnu-1.3.0.tgz", + "integrity": "sha512-eyKGpY40lm9Jvs1aD294XRH4y7+TlJM0YVAryZeXA6TX0mb4gMkxVXwSQv7MCwgah7raeUd0dKUb4BPAYIgcMg==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-arm64-musl": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-arm64-musl/-/keyring-linux-arm64-musl-1.3.0.tgz", + "integrity": "sha512-iIK6JWHXAJqDrEyLY3TmswwloVyt2vj+04TZnew+uSJ9gnDO8EwRbp3/iw3LpWaXiDO7VomGO6y8I0Id8uBZSw==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-riscv64-gnu": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-riscv64-gnu/-/keyring-linux-riscv64-gnu-1.3.0.tgz", + "integrity": "sha512-/PGqrwn6EwgtK6vccASSXJRfOSP4vN1F4ASsIQ+7MdrK6hNvAJ1FZPrIuD5gGGdxezo3F++To2Wq7DbuGIeuNQ==", + "cpu": [ + "riscv64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-x64-gnu": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-x64-gnu/-/keyring-linux-x64-gnu-1.3.0.tgz", + "integrity": "sha512-2PDK1WKWTu9lBGq9VvNEkSlQD3O7YwVpmnyN2M3cy4v7NJ/8gDMd9GXv3G+FVXN13uhp4gnnPBS+ScefmEeD2A==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-x64-musl": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-x64-musl/-/keyring-linux-x64-musl-1.3.0.tgz", + "integrity": "sha512-oJ2HkX8YUo46QBkn0pG+HuIKQNqr523q6vBobCn+P95s4C4K6/kLBqHY/1bg5J4ap31DzsznhnFKcfBNBsjCnw==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-win32-arm64-msvc": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-win32-arm64-msvc/-/keyring-win32-arm64-msvc-1.3.0.tgz", + "integrity": "sha512-tOd3c/uAaeoE4ycVlmAdSvygz0Zt3zdca6Y7gokBeIbaRDWpjDIUOpU3MvML59XAaqyuKGsVVu0F/DZb1lHPmw==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-win32-ia32-msvc": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-win32-ia32-msvc/-/keyring-win32-ia32-msvc-1.3.0.tgz", + "integrity": "sha512-sPSqeAFZMGqP1R++M2JTza7GQJJ/TpCo6JU6Vcd4jnebvOaEDs9b7eipakU1PJdSvhpC2yXMCNRk9gXfrhuwHQ==", + "cpu": [ + "ia32" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-win32-x64-msvc": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-win32-x64-msvc/-/keyring-win32-x64-msvc-1.3.0.tgz", + "integrity": "sha512-4DnCWXwDc0HRKwyRlG5y0VhKZW2tNRQfKKfyj6IX/KWfDNyq9hn4n+GL1auyDcOO/v8PwnhmYo2+rOOqCkvvOg==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, "node_modules/@napi-rs/wasm-runtime": { "version": "1.1.4", "resolved": "https://registry.npmjs.org/@napi-rs/wasm-runtime/-/wasm-runtime-1.1.4.tgz", diff --git a/package.json b/package.json index a7ed32e..2f83310 100644 --- a/package.json +++ b/package.json @@ -83,6 +83,9 @@ "chalk": "5.6.2", "yocto-spinner": "1.2.0" }, + "optionalDependencies": { + "@napi-rs/keyring": "1.3.0" + }, "peerDependencies": { "commander": ">=14", "marked": ">=18", 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/index.ts b/src/auth/index.ts index 579ceac..d5fbc0f 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -35,3 +35,22 @@ export type { TokenStore, ValidateInput, } from './types.js' +export { + SECURE_STORE_DESCRIPTION, + SecureStoreUnavailableError, + createKeyringTokenStore, + createSecureStore, + migrateLegacyAuth, +} from './keyring/index.js' +export type { + CreateKeyringTokenStoreOptions, + CreateSecureStoreOptions, + KeyringTokenStore, + MigrateAuthResult, + MigrateLegacyAuthOptions, + SecureStore, + TokenStorageLocation, + TokenStorageResult, + UserRecord, + UserRecordStore, +} from './keyring/index.js' diff --git a/src/auth/keyring/index.ts b/src/auth/keyring/index.ts new file mode 100644 index 0000000..741151b --- /dev/null +++ b/src/auth/keyring/index.ts @@ -0,0 +1,19 @@ +export { + SECURE_STORE_DESCRIPTION, + SecureStoreUnavailableError, + createSecureStore, +} from './secure-store.js' +export type { CreateSecureStoreOptions, SecureStore } from './secure-store.js' + +export { createKeyringTokenStore } from './token-store.js' +export type { CreateKeyringTokenStoreOptions, KeyringTokenStore } from './token-store.js' + +export { migrateLegacyAuth } from './migrate.js' +export type { MigrateAuthResult, MigrateLegacyAuthOptions } from './migrate.js' + +export type { + TokenStorageLocation, + TokenStorageResult, + UserRecord, + UserRecordStore, +} from './types.js' diff --git a/src/auth/keyring/migrate.test.ts b/src/auth/keyring/migrate.test.ts new file mode 100644 index 0000000..103e6a2 --- /dev/null +++ b/src/auth/keyring/migrate.test.ts @@ -0,0 +1,233 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { buildKeyringMap, buildUserRecords } from '../../test-support/keyring-mocks.js' +import { migrateLegacyAuth } from './migrate.js' +import { SecureStoreUnavailableError } from './secure-store.js' + +vi.mock('./secure-store.js', async () => { + const actual = await vi.importActual('./secure-store.js') + return { + ...actual, + createSecureStore: vi.fn(), + } +}) + +const { createSecureStore } = await import('./secure-store.js') +const mockedCreateSecureStore = vi.mocked(createSecureStore) + +type Account = { + id: string + label?: string + email: string +} + +const SERVICE = 'cli-core-test' +const LEGACY = 'api-token' + +describe('migrateLegacyAuth', () => { + beforeEach(() => { + mockedCreateSecureStore.mockReset() + }) + + it('returns already-migrated when user records already exist', async () => { + // No keyring mock needed — `migrateLegacyAuth` returns before ever calling `createSecureStore`. + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account: { id: '1', email: 'a@b' } }) + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => null, + identifyAccount: async () => ({ id: '1', email: 'a@b' }), + silent: true, + }) + + expect(result.status).toBe('already-migrated') + expect(mockedCreateSecureStore).not.toHaveBeenCalled() + }) + + it('returns no-legacy-state when neither slot has a token', async () => { + const km = buildKeyringMap() + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords } = buildUserRecords() + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => null, + identifyAccount: async () => ({ id: '1', email: 'a@b' }), + silent: true, + }) + + expect(result.status).toBe('no-legacy-state') + }) + + it('migrates a legacy keyring token into a per-user slot and clears the legacy entry', async () => { + const km = buildKeyringMap() + km.slots.set(LEGACY, { secret: 'legacy_tok' }) + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords, state, upsertSpy } = buildUserRecords() + const cleanup = vi.fn(async () => undefined) + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => null, + identifyAccount: async (token) => { + expect(token).toBe('legacy_tok') + return { id: '99', email: 'me@x.io', label: 'me@x.io' } + }, + cleanupLegacyConfig: cleanup, + silent: true, + }) + + expect(result.status).toBe('migrated') + expect(result.migratedAccount?.id).toBe('99') + expect(km.slots.get('user-99')?.secret).toBe('legacy_tok') + expect(km.slots.get(LEGACY)?.secret).toBeNull() + expect(state.records.get('99')?.fallbackToken).toBeUndefined() + expect(state.defaultId).toBe('99') + expect(upsertSpy).toHaveBeenCalledTimes(1) + expect(cleanup).toHaveBeenCalledTimes(1) + }) + + it('falls back to loadLegacyPlaintextToken when the legacy keyring slot is empty', async () => { + const km = buildKeyringMap() + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords, state } = buildUserRecords() + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => 'plain_legacy', + identifyAccount: async () => ({ id: '7', email: 'p@l.x' }), + silent: true, + }) + + expect(result.status).toBe('migrated') + expect(km.slots.get('user-7')?.secret).toBe('plain_legacy') + expect(state.records.get('7')?.fallbackToken).toBeUndefined() + }) + + it('migrates against an entirely offline keyring (WSL/headless)', async () => { + const km = buildKeyringMap() + // The whole keyring is dead: reading the legacy slot throws and + // writing the per-user slot would too. Migration must still complete + // by sourcing the token from the consumer's plaintext slot and + // parking it on the user record as `fallbackToken`. + km.slots.set(LEGACY, { + secret: null, + getErr: new SecureStoreUnavailableError('no dbus'), + }) + km.slots.set('user-7', { + secret: null, + setErr: new SecureStoreUnavailableError('no dbus'), + }) + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords, state } = buildUserRecords() + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => 'plain_legacy', + identifyAccount: async () => ({ id: '7', email: 'p@l.x' }), + silent: true, + }) + + expect(result.status).toBe('migrated') + expect(state.records.get('7')?.fallbackToken).toBe('plain_legacy') + expect(state.defaultId).toBe('7') + }) + + it('returns skipped when identifyAccount throws', async () => { + const km = buildKeyringMap() + km.slots.set(LEGACY, { secret: 'legacy_tok' }) + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords, state } = buildUserRecords() + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => null, + identifyAccount: async () => { + throw new Error('HTTP 401') + }, + silent: true, + }) + + expect(result.status).toBe('skipped') + expect(result.reason).toContain('HTTP 401') + expect(state.records.size).toBe(0) + // Legacy entry must remain so a retry can find it. + expect(km.slots.get(LEGACY)?.secret).toBe('legacy_tok') + }) +}) + +describe('migrateLegacyAuth — stderr privacy', () => { + let consoleError: ReturnType + beforeEach(() => { + mockedCreateSecureStore.mockReset() + consoleError = vi.spyOn(console, 'error').mockImplementation(() => undefined) + }) + afterEach(() => { + consoleError.mockRestore() + }) + + it('the success line carries only account.id (no label/email)', async () => { + const km = buildKeyringMap() + km.slots.set(LEGACY, { secret: 'legacy_tok' }) + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords } = buildUserRecords() + + await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => null, + identifyAccount: async () => ({ + id: 'user-99', + label: 'sensitive@email.example', + email: 'sensitive@email.example', + }), + silent: false, + logPrefix: 'td', + }) + + const lines = consoleError.mock.calls.flat().join('\n') + expect(lines).toContain('user-99') + expect(lines).not.toContain('sensitive@email.example') + }) + + it('the skip line is generic and does not echo the raw exception text', async () => { + const km = buildKeyringMap() + km.slots.set(LEGACY, { secret: 'legacy_tok' }) + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords } = buildUserRecords() + + const result = await migrateLegacyAuth({ + serviceName: SERVICE, + legacyAccount: LEGACY, + userRecords, + loadLegacyPlaintextToken: async () => null, + identifyAccount: async () => { + throw new Error('email leak: sensitive@email.example at /Users/me/.config/x') + }, + silent: false, + logPrefix: 'td', + }) + + expect(result.status).toBe('skipped') + const lines = consoleError.mock.calls.flat().join('\n') + expect(lines).toContain('could not identify user') + expect(lines).not.toContain('sensitive@email.example') + expect(lines).not.toContain('/Users/me/.config/x') + // The raw detail is preserved on the result for in-process callers. + expect(result.reason).toContain('sensitive@email.example') + }) +}) diff --git a/src/auth/keyring/migrate.ts b/src/auth/keyring/migrate.ts new file mode 100644 index 0000000..50479a1 --- /dev/null +++ b/src/auth/keyring/migrate.ts @@ -0,0 +1,218 @@ +import { getErrorMessage } from '../../errors.js' +import type { AuthAccount } from '../types.js' +import { writeRecordWithKeyringFallback } from './record-write.js' +import { + createSecureStore, + DEFAULT_ACCOUNT_FOR_USER, + SecureStoreUnavailableError, +} from './secure-store.js' +import type { UserRecordStore } from './types.js' + +export type MigrateLegacyAuthOptions = { + serviceName: string + /** Legacy single-user keyring account slug, e.g. `'api-token'`. */ + legacyAccount: string + /** v2 user-record store the migrated record is written into. */ + userRecords: UserRecordStore + /** Per-user keyring slug for the new entry. Defaults to `user-${id}`. */ + accountForUser?: (id: string) => string + /** + * Returns the v1 token from the consumer's *plaintext* config slot, or + * `null` if absent. cli-core handles the legacy keyring slot itself. + */ + loadLegacyPlaintextToken: () => Promise + /** + * Identifies the user behind the v1 token. Implementations typically hit + * the product API with the token to fetch the canonical `id` / `email` + * for the new account record. + */ + identifyAccount: (token: string) => Promise + /** + * Optional best-effort cleanup of v1-only config fields after a + * successful migration (e.g. unset legacy `api_token` / `auth_mode`). + * Failures are swallowed; the user record is the source of truth. + */ + cleanupLegacyConfig?: () => Promise + /** Suppress stderr output (postinstall hooks set this). */ + silent?: boolean + /** Label used in the stderr log line. Defaults to `'cli'`. */ + logPrefix?: string +} + +export type MigrateAuthResult = { + status: 'already-migrated' | 'no-legacy-state' | 'migrated' | 'skipped' + /** + * Internal-only failure detail for the call site (e.g. doctor commands). + * **Not** emitted to stderr — see `skipped()` for why. + */ + reason?: string + migratedAccount?: TAccount +} + +/** Internal `SkipReason` codes — kept generic for stderr so consumer-supplied error text can't leak PII to logs. */ +type SkipReason = + | 'identify-failed' + | 'keyring-write-failed' + | 'user-record-write-failed' + | 'cleanup-failed' + +const SKIP_REASON_MESSAGES: Record = { + 'identify-failed': 'could not identify user', + 'keyring-write-failed': 'failed to write user-scoped credential', + 'user-record-write-failed': 'failed to update user records', + 'cleanup-failed': 'failed to clean up legacy state', +} + +/** + * One-time migration of a v1 single-user auth state into a v2 multi-user + * shape. Best-effort: any failure (offline keyring, network error fetching + * the user, identifier mismatch, …) leaves the v1 state untouched so the + * consumer's runtime fallback can keep serving the legacy token until the + * next attempt. + * + * Steps: + * 1. Skip if `userRecords` already has any records. + * 2. Read the v1 token — legacy keyring slot first, then the consumer's + * plaintext slot. + * 3. Identify the user via `identifyAccount(token)`. + * 4. Write the token to the per-user keyring slot (or fall back to a + * `fallbackToken` on the record if keyring is unreachable). + * 5. Upsert the v2 record + set it as the default. + * 6. Best-effort delete of the legacy keyring slot. + * 7. Best-effort `cleanupLegacyConfig()`. + */ +export async function migrateLegacyAuth( + options: MigrateLegacyAuthOptions, +): Promise> { + const { + serviceName, + legacyAccount, + userRecords, + loadLegacyPlaintextToken, + identifyAccount, + cleanupLegacyConfig, + silent, + } = options + const accountForUser = options.accountForUser ?? DEFAULT_ACCOUNT_FOR_USER + const logPrefix = options.logPrefix ?? 'cli' + + const existing = await userRecords.list() + if (existing.length > 0) { + return { status: 'already-migrated' } + } + + const legacyToken = await readLegacyToken({ + serviceName, + legacyAccount, + loadLegacyPlaintextToken, + }) + if (!legacyToken) { + return { status: 'no-legacy-state' } + } + + let account: TAccount + try { + account = await identifyAccount(legacyToken) + } catch (error) { + return skipped(silent, logPrefix, 'identify-failed', getErrorMessage(error)) + } + + try { + await writeRecordWithKeyringFallback({ + serviceName, + accountForUser, + userRecords, + account, + token: legacyToken, + }) + } catch (error) { + // `writeRecordWithKeyringFallback` only rethrows non-keyring errors + // (i.e. `userRecords.upsert` failures or unexpected keyring errors). + // SecureStoreUnavailableError is internally caught by the helper and + // never propagates here. + return skipped( + silent, + logPrefix, + error instanceof SecureStoreUnavailableError + ? 'keyring-write-failed' + : 'user-record-write-failed', + getErrorMessage(error), + ) + } + + // Default promotion is best-effort — the user record is durable and + // ` account use` can fix things up later. + try { + await userRecords.setDefaultId(account.id) + } catch { + // non-fatal + } + + try { + const legacyStore = createSecureStore({ serviceName, account: legacyAccount }) + await legacyStore.deleteSecret() + } catch { + // best-effort — legacy slot may already be empty or the keyring may be offline. + } + + if (cleanupLegacyConfig) { + try { + await cleanupLegacyConfig() + } catch { + // best-effort — the user record is the source of truth. + } + } + + if (!silent) { + // Log the stable id only — `account.label` is typically an email or + // other user-facing identifier, and these diagnostics flow to stderr + // (and possibly to log aggregators) where PII shouldn't appear. + console.error(`${logPrefix}: migrated existing token to multi-user store (${account.id}).`) + } + + return { status: 'migrated', migratedAccount: account } +} + +async function readLegacyToken(opts: { + serviceName: string + legacyAccount: string + loadLegacyPlaintextToken: () => Promise +}): Promise { + try { + const legacyStore = createSecureStore({ + serviceName: opts.serviceName, + account: opts.legacyAccount, + }) + const stored = await legacyStore.getSecret() + if (stored?.trim()) return stored.trim() + } catch (error) { + if (!(error instanceof SecureStoreUnavailableError)) throw error + } + + const plaintext = await opts.loadLegacyPlaintextToken() + if (plaintext?.trim()) return plaintext.trim() + + return null +} + +/** + * Emit the migration skip line. The stderr text is a fixed phrase keyed off + * `SkipReason` so consumer-supplied callbacks (`identifyAccount`, the + * `UserRecordStore`, …) can't leak emails, paths, or auth diagnostics into + * logs. The raw error message is still attached to the returned + * `MigrateAuthResult.reason` for in-process callers that need it (doctor + * commands, tests). + */ +function skipped( + silent: boolean | undefined, + logPrefix: string, + reasonCode: SkipReason, + detail: string, +): MigrateAuthResult { + if (!silent) { + console.error( + `${logPrefix}: skipped legacy auth migration — ${SKIP_REASON_MESSAGES[reasonCode]}.`, + ) + } + return { status: 'skipped', reason: `${reasonCode}: ${detail}` } +} diff --git a/src/auth/keyring/record-write.ts b/src/auth/keyring/record-write.ts new file mode 100644 index 0000000..e506ab5 --- /dev/null +++ b/src/auth/keyring/record-write.ts @@ -0,0 +1,71 @@ +import type { AuthAccount } from '../types.js' +import { createSecureStore, type SecureStore, SecureStoreUnavailableError } from './secure-store.js' +import type { UserRecord, UserRecordStore } from './types.js' + +export type WriteRecordOptions = { + serviceName: string + accountForUser: (id: string) => string + userRecords: UserRecordStore + account: TAccount + token: string +} + +export type WriteRecordResult = { + /** `true` when the secret landed in the OS keyring; `false` when the keyring was unavailable and the token was written to `fallbackToken` on the user record. */ + storedSecurely: boolean +} + +/** + * Shared keyring-then-record write used by `createKeyringTokenStore.set` and + * `migrateLegacyAuth`. Encapsulates the order-of-operations contract that + * matters for credential safety: + * + * 1. Keyring `setSecret` first. On `SecureStoreUnavailableError`, swallow + * the failure and record a `fallbackToken` on the user record instead. + * Any other error rethrows. + * 2. `userRecords.upsert(record)`. On failure, best-effort rollback the + * keyring write so we don't leave an orphan credential for an account + * cli-core never managed to register. Original error rethrows. + * + * Default promotion (`setDefaultId`) is intentionally **not** in here — both + * call sites do it best-effort outside the critical section because it is a + * preference, not a correctness requirement, and an error there must not + * dirty up a successful credential write. + */ +export async function writeRecordWithKeyringFallback( + options: WriteRecordOptions, +): Promise { + const { serviceName, accountForUser, userRecords, account, token } = options + const trimmed = token.trim() + const secureStore: SecureStore = createSecureStore({ + serviceName, + account: accountForUser(account.id), + }) + + let storedSecurely = false + try { + await secureStore.setSecret(trimmed) + storedSecurely = true + } catch (error) { + if (!(error instanceof SecureStoreUnavailableError)) throw error + } + + const record: UserRecord = storedSecurely + ? { id: account.id, account } + : { id: account.id, account, fallbackToken: trimmed } + + try { + await userRecords.upsert(record) + } catch (error) { + if (storedSecurely) { + try { + await secureStore.deleteSecret() + } catch { + // best-effort — the user record failure is the real cause + } + } + throw error + } + + return { storedSecurely } +} diff --git a/src/auth/keyring/secure-store.test.ts b/src/auth/keyring/secure-store.test.ts new file mode 100644 index 0000000..0fb4875 --- /dev/null +++ b/src/auth/keyring/secure-store.test.ts @@ -0,0 +1,80 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const keyringMocks = vi.hoisted(() => { + const entry = { + getPassword: vi.fn(), + setPassword: vi.fn(), + deleteCredential: vi.fn(), + } + return { + AsyncEntry: vi.fn().mockImplementation(function AsyncEntry() { + return entry + }), + entry, + } +}) + +vi.mock('@napi-rs/keyring', () => ({ + AsyncEntry: keyringMocks.AsyncEntry, +})) + +const SERVICE = 'cli-core-test' +const ACCOUNT = 'user-42' + +describe('createSecureStore', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.resetModules() + }) + + it('reads, writes, and deletes secrets via @napi-rs/keyring with the configured service/account', async () => { + keyringMocks.entry.getPassword.mockResolvedValue('tok_abcdef') + keyringMocks.entry.setPassword.mockResolvedValue(undefined) + keyringMocks.entry.deleteCredential.mockResolvedValue(true) + + const { createSecureStore } = await import('./secure-store.js') + const store = createSecureStore({ serviceName: SERVICE, account: ACCOUNT }) + + await expect(store.getSecret()).resolves.toBe('tok_abcdef') + await expect(store.setSecret('tok_abcdef')).resolves.toBeUndefined() + await expect(store.deleteSecret()).resolves.toBe(true) + + expect(keyringMocks.AsyncEntry).toHaveBeenCalledWith(SERVICE, ACCOUNT) + expect(keyringMocks.entry.setPassword).toHaveBeenCalledWith('tok_abcdef') + }) + + it('wraps a keyring failure as SecureStoreUnavailableError', async () => { + keyringMocks.entry.getPassword.mockRejectedValue(new Error('Keychain locked')) + + const { createSecureStore, SecureStoreUnavailableError } = await import('./secure-store.js') + + await expect( + createSecureStore({ serviceName: SERVICE, account: ACCOUNT }).getSecret(), + ).rejects.toBeInstanceOf(SecureStoreUnavailableError) + }) +}) + +describe('createSecureStore — missing native binary', () => { + beforeEach(() => { + vi.resetModules() + }) + + // Ensure the throw-on-import mock is torn down even when the assertion + // inside the `it` body fails. Inline cleanup would otherwise be skipped + // and leave the mock active for later tests. + afterEach(() => { + vi.doUnmock('@napi-rs/keyring') + }) + + it('surfaces an import failure as SecureStoreUnavailableError instead of crashing module load', async () => { + vi.doMock('@napi-rs/keyring', () => { + throw new Error('no native binary for this arch') + }) + + const { createSecureStore, SecureStoreUnavailableError } = await import('./secure-store.js') + + await expect( + createSecureStore({ serviceName: SERVICE, account: ACCOUNT }).getSecret(), + ).rejects.toBeInstanceOf(SecureStoreUnavailableError) + }) +}) diff --git a/src/auth/keyring/secure-store.ts b/src/auth/keyring/secure-store.ts new file mode 100644 index 0000000..d4b660c --- /dev/null +++ b/src/auth/keyring/secure-store.ts @@ -0,0 +1,102 @@ +export const SECURE_STORE_DESCRIPTION = 'system credential manager' + +/** + * Default keyring account slug for a user id. Shared between the runtime + * `createKeyringTokenStore` and `migrateLegacyAuth` so a future rename can't + * silently park tokens in a slot the runtime no longer reads from. + */ +export const DEFAULT_ACCOUNT_FOR_USER = (id: string): string => `user-${id}` + +/** + * Thrown when the OS credential manager cannot be reached — missing native + * binary for the current architecture, libsecret/D-Bus unavailable + * (common in WSL / headless Linux / containers / CI), Keychain locked, or + * any other error returned by the underlying keyring backend. + * + * Callers that want to degrade to a plaintext fallback should catch this + * specific class rather than swallowing every `Error` — anything else + * coming out of `SecureStore` is a programmer error. + */ +export class SecureStoreUnavailableError extends Error { + constructor(message = 'System credential storage is unavailable') { + super(message) + this.name = 'SecureStoreUnavailableError' + } +} + +export type SecureStore = { + getSecret(): Promise + setSecret(secret: string): Promise + deleteSecret(): Promise +} + +export type CreateSecureStoreOptions = { + /** Stable per-application identifier (Keychain "service", Credential Manager "target prefix", libsecret "service"). */ + serviceName: string + /** Per-credential identifier within the service. For multi-account CLIs, typically `user-${id}`. */ + account: string +} + +/** + * Thin wrapper around `@napi-rs/keyring` that normalizes every failure mode + * into `SecureStoreUnavailableError`. `serviceName` + `account` together + * identify one credential slot in the OS keyring. + */ +export function createSecureStore(options: CreateSecureStoreOptions): SecureStore { + const { serviceName, account } = options + + return { + async getSecret(): Promise { + const entry = await getEntry(serviceName, account) + try { + return (await entry.getPassword()) ?? null + } catch (error) { + throw toUnavailableError(error) + } + }, + + async setSecret(secret: string): Promise { + const entry = await getEntry(serviceName, account) + try { + await entry.setPassword(secret) + } catch (error) { + throw toUnavailableError(error) + } + }, + + async deleteSecret(): Promise { + const entry = await getEntry(serviceName, account) + try { + return await entry.deleteCredential() + } catch (error) { + throw toUnavailableError(error) + } + }, + } +} + +async function getEntry( + serviceName: string, + account: string, +): Promise { + try { + // Dynamic import: `@napi-rs/keyring` is an optional dependency. On + // unsupported architectures (or when the native binary failed to + // install) a static import would crash module load before we get a + // chance to surface `SecureStoreUnavailableError` and let the caller + // fall back to plaintext config storage. + const { AsyncEntry } = await import('@napi-rs/keyring') + return new AsyncEntry(serviceName, account) + } catch (error) { + throw toUnavailableError(error) + } +} + +function toUnavailableError(error: unknown): SecureStoreUnavailableError { + if (error instanceof SecureStoreUnavailableError) { + return error + } + const message = + error instanceof Error ? error.message : 'System credential storage is unavailable' + return new SecureStoreUnavailableError(message) +} diff --git a/src/auth/keyring/token-store.test.ts b/src/auth/keyring/token-store.test.ts new file mode 100644 index 0000000..34a8a44 --- /dev/null +++ b/src/auth/keyring/token-store.test.ts @@ -0,0 +1,363 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import { + buildKeyringMap, + buildSingleSlot, + buildUserRecords, +} from '../../test-support/keyring-mocks.js' +import { SecureStoreUnavailableError } from './secure-store.js' +import { createKeyringTokenStore } from './token-store.js' + +vi.mock('./secure-store.js', async () => { + const actual = await vi.importActual('./secure-store.js') + return { + ...actual, + createSecureStore: vi.fn(), + } +}) + +const { createSecureStore } = await import('./secure-store.js') +const mockedCreateSecureStore = vi.mocked(createSecureStore) + +type Account = { + id: string + label?: string + email: string +} + +const SERVICE = 'cli-core-test' +const account: Account = { id: '42', label: 'me', email: 'a@b.c' } + +describe('createKeyringTokenStore', () => { + beforeEach(() => { + mockedCreateSecureStore.mockReset() + }) + + it('round-trips set → active → clear when the keyring is online', async () => { + const keyring = buildSingleSlot() + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state, upsertSpy } = buildUserRecords() + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.set(account, 'tok_secret') + expect(keyring.setSpy).toHaveBeenCalledWith('tok_secret') + expect(upsertSpy).toHaveBeenCalledWith({ id: '42', account }) + expect(state.defaultId).toBe('42') + expect(store.getLastStorageResult()).toEqual({ storage: 'secure-store' }) + + const active = await store.active() + expect(active).toEqual({ token: 'tok_secret', account }) + + await store.clear() + expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) + expect(state.records.size).toBe(0) + expect(state.defaultId).toBeNull() + expect(store.getLastClearResult()).toEqual({ storage: 'secure-store' }) + }) + + it('falls back to a plaintext token on the user record when the keyring is offline', async () => { + const keyring = buildSingleSlot() + keyring.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus')) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.set(account, 'tok_plain') + + const record = state.records.get('42') + expect(record?.fallbackToken).toBe('tok_plain') + expect(store.getLastStorageResult()).toEqual({ + storage: 'config-file', + warning: + 'system credential manager unavailable; token saved as plaintext in /tmp/fake/config.json', + }) + + const active = await store.active() + expect(active).toEqual({ token: 'tok_plain', account }) + expect(keyring.getSpy).not.toHaveBeenCalled() + }) + + it('rolls back the keyring write when the user record upsert fails', async () => { + const keyring = buildSingleSlot() + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, upsertSpy } = buildUserRecords() + upsertSpy.mockRejectedValueOnce(new Error('disk full')) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.set(account, 'tok')).rejects.toThrow('disk full') + expect(keyring.setSpy).toHaveBeenCalledWith('tok') + expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) + }) + + it('set() still succeeds when the best-effort default promotion fails', async () => { + const keyring = buildSingleSlot() + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state, setDefaultSpy } = buildUserRecords() + setDefaultSpy.mockRejectedValueOnce(new Error('default-write blew up')) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.set(account, 'tok')).resolves.toBeUndefined() + expect(state.records.get('42')?.account).toEqual(account) + // Default never got set because the write failed, but the user record is durable. + expect(state.defaultId).toBeNull() + }) + + it('throws AUTH_STORE_READ_FAILED when active() finds a record but the keyring is offline', async () => { + const keyring = buildSingleSlot({ secret: 'tok' }) + keyring.getSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('locked')) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.records.set('42', { id: '42', account }) + state.defaultId = '42' + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.active()).rejects.toMatchObject({ code: 'AUTH_STORE_READ_FAILED' }) + }) + + it('picks the lone user when no default is set', async () => { + const keyring = buildSingleSlot({ secret: 'tok' }) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.records.set('42', { id: '42', account }) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.active()).resolves.toEqual({ token: 'tok', account }) + }) + + it('returns null when multiple users exist and no default is set', async () => { + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account: { ...account, id: '1' } }) + state.records.set('2', { id: '2', account: { ...account, id: '2' } }) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.active()).resolves.toBeNull() + }) + + it('does not overwrite an existing default when a second user is added', async () => { + const keyring = buildSingleSlot() + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.defaultId = '1' + state.records.set('1', { id: '1', account: { ...account, id: '1' } }) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.set({ ...account, id: '2' }, 'tok_b') + + expect(state.defaultId).toBe('1') + }) + + it('clear() still calls the keyring delete when a fallbackToken is present (orphan cleanup)', async () => { + const keyring = buildSingleSlot({ secret: 'orphan_from_earlier_write' }) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.records.set('42', { id: '42', account, fallbackToken: 'tok_plain' }) + state.defaultId = '42' + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.clear() + + expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) + expect(state.records.size).toBe(0) + expect(store.getLastClearResult()).toEqual({ + storage: 'config-file', + warning: + 'system credential manager unavailable; local auth state cleared in /tmp/fake/config.json', + }) + }) + + it('clear() downgrades to a warning when the keyring delete fails', async () => { + const keyring = buildSingleSlot({ secret: 'tok' }) + keyring.deleteSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('offline')) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state } = buildUserRecords() + state.records.set('42', { id: '42', account }) + state.defaultId = '42' + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.clear() + + expect(state.records.size).toBe(0) + expect(store.getLastClearResult()).toMatchObject({ storage: 'config-file' }) + }) + + it('clear() still deletes the keyring slot even when setDefaultId(null) throws', async () => { + const keyring = buildSingleSlot({ secret: 'tok' }) + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords, state, setDefaultSpy } = buildUserRecords() + state.records.set('42', { id: '42', account }) + state.defaultId = '42' + setDefaultSpy.mockRejectedValueOnce(new Error('disk full')) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.clear() + + // Default pointer write blew up, but the keyring entry was still + // cleaned up — otherwise the record's old credential would become + // an unreachable orphan. + expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) + expect(state.records.size).toBe(0) + }) + + it('uses a custom accountForUser slug when provided', async () => { + const keyring = buildSingleSlot() + mockedCreateSecureStore.mockReturnValue(keyring) + const { store: userRecords } = buildUserRecords() + + const store = createKeyringTokenStore({ + serviceName: SERVICE, + userRecords, + accountForUser: (id) => `custom-${id}`, + }) + + await store.set(account, 'tok') + + expect(mockedCreateSecureStore).toHaveBeenCalledWith({ + serviceName: SERVICE, + account: 'custom-42', + }) + }) + + describe('AccountRef support (keyed per-user slots)', () => { + function buildMultiUser() { + const km = buildKeyringMap() + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords, state, removeSpy } = buildUserRecords() + state.records.set('1', { id: '1', account: { id: '1', label: 'alice', email: 'a@b' } }) + state.records.set('2', { + id: '2', + account: { id: '2', label: 'bob', email: 'c@d' }, + fallbackToken: 'tok_b', + }) + km.slots.set('user-1', { secret: 'tok_a' }) + return { km, userRecords, state, removeSpy } + } + + it('active(ref) reads from the matching per-user slot', async () => { + const { km, userRecords } = buildMultiUser() + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + const snapshot = await store.active('1') + expect(snapshot?.account.id).toBe('1') + expect(snapshot?.token).toBe('tok_a') + + // Sanity check: matched user 1 only — user 2's keyring slot was + // never touched (its record carries `fallbackToken`). + expect(km.slots.has('user-2')).toBe(false) + }) + + it('active(ref) prefers the fallbackToken over a stale keyring entry', async () => { + const { km, userRecords } = buildMultiUser() + // Simulate an orphan keyring entry left from a prior online write. + km.slots.set('user-2', { secret: 'tok_b_stale' }) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + const snapshot = await store.active('2') + expect(snapshot?.token).toBe('tok_b') + }) + + it('active(ref) returns null on a miss (attacher translates to ACCOUNT_NOT_FOUND)', async () => { + const { userRecords } = buildMultiUser() + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.active('does-not-exist')).resolves.toBeNull() + }) + + it('clear(ref) removes the matching record and deletes only its keyring slot', async () => { + const { km, userRecords, state } = buildMultiUser() + state.defaultId = '1' + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.clear('1') + + expect(state.records.has('1')).toBe(false) + expect(state.records.has('2')).toBe(true) + expect(state.defaultId).toBeNull() + // user-1's slot was cleared; user-2's slot was never touched. + expect(km.slots.get('user-1')?.secret).toBeNull() + expect((km.deleteCalls.get('user-1') ?? 0) > 0).toBe(true) + expect(km.deleteCalls.has('user-2')).toBe(false) + }) + + it('honours a custom matchAccount predicate', async () => { + const km = buildKeyringMap() + mockedCreateSecureStore.mockImplementation(km.create) + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account: { id: '1', email: 'Alice@x.io' } }) + km.slots.set('user-1', { secret: 'tok' }) + + const store = createKeyringTokenStore({ + serviceName: SERVICE, + userRecords, + matchAccount: (acc, ref) => acc.email.toLowerCase() === ref.toLowerCase(), + }) + + await expect(store.active('alice@x.io')).resolves.toMatchObject({ + account: { id: '1' }, + }) + }) + }) + + describe('list() + setDefault()', () => { + it('list() returns every account with the default marker', async () => { + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account: { id: '1', email: 'a@b' } }) + state.records.set('2', { id: '2', account: { id: '2', email: 'c@d' } }) + state.defaultId = '2' + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + const all = await store.list() + expect(all).toHaveLength(2) + expect(all.find((entry) => entry.account.id === '2')?.isDefault).toBe(true) + expect(all.find((entry) => entry.account.id === '1')?.isDefault).toBe(false) + }) + + it('list() marks a single record as default even when no defaultId is pinned (matches active())', async () => { + const { store: userRecords, state } = buildUserRecords() + state.records.set('42', { id: '42', account }) + // No defaultId set — active() treats the lone record as the + // implicit default, so list() must too. + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + const all = await store.list() + expect(all).toEqual([{ account, isDefault: true }]) + }) + + it('setDefault(ref) marks the matching account as default', async () => { + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account: { id: '1', label: 'a', email: 'a@b' } }) + state.records.set('2', { id: '2', account: { id: '2', label: 'b', email: 'c@d' } }) + state.defaultId = '1' + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await store.setDefault('b') + expect(state.defaultId).toBe('2') + expect(mockedCreateSecureStore).not.toHaveBeenCalled() + }) + + it('setDefault(ref) throws ACCOUNT_NOT_FOUND on a miss', async () => { + const { store: userRecords, state } = buildUserRecords() + state.records.set('1', { id: '1', account }) + + const store = createKeyringTokenStore({ serviceName: SERVICE, userRecords }) + + await expect(store.setDefault('nope')).rejects.toMatchObject({ + code: 'ACCOUNT_NOT_FOUND', + }) + }) + }) +}) diff --git a/src/auth/keyring/token-store.ts b/src/auth/keyring/token-store.ts new file mode 100644 index 0000000..441950b --- /dev/null +++ b/src/auth/keyring/token-store.ts @@ -0,0 +1,256 @@ +import { CliError } from '../../errors.js' +import type { AccountRef, AuthAccount, TokenStore } from '../types.js' +import { accountNotFoundError } from '../user-flag.js' +import { writeRecordWithKeyringFallback } from './record-write.js' +import { + createSecureStore, + DEFAULT_ACCOUNT_FOR_USER, + SECURE_STORE_DESCRIPTION, + SecureStoreUnavailableError, + type SecureStore, +} from './secure-store.js' +import type { TokenStorageResult, UserRecord, UserRecordStore } from './types.js' + +export type CreateKeyringTokenStoreOptions = { + /** Application identifier used for every keyring entry (e.g. `'todoist-cli'`). */ + serviceName: string + /** Consumer-owned per-user record store (typically backed by their config file). */ + userRecords: UserRecordStore + /** + * Builds the keyring `account` slug for a user id. Defaults to + * `user-${id}`. Override only when migrating from a legacy naming scheme. + */ + accountForUser?: (id: string) => string + /** + * Decides whether an account matches an `AccountRef` supplied via + * `--user `. Defaults to id-or-label equality. Override to broaden + * (e.g. case-insensitive email, alias map). + */ + matchAccount?: (account: TAccount, ref: AccountRef) => boolean +} + +export type KeyringTokenStore = TokenStore & { + /** Storage result from the most recent `set()` call, or `undefined` before any. */ + getLastStorageResult(): TokenStorageResult | undefined + /** Storage result from the most recent `clear()` call, or `undefined` before any. */ + getLastClearResult(): TokenStorageResult | undefined +} + +const DEFAULT_MATCH_ACCOUNT = ( + account: TAccount, + ref: AccountRef, +): boolean => account.id === ref || account.label === ref + +/** + * Multi-account `TokenStore` that keeps secrets in the OS credential manager + * and per-user metadata in the consumer's `UserRecordStore`. Falls back to a + * plaintext token on the user record when the keyring is unreachable (WSL + * without D-Bus, missing native binary, locked Keychain, …) so the CLI keeps + * working at the cost of a visible warning. + * + * Read order in `active()` is `fallbackToken` first, then the keyring. That + * matches the write semantics in `writeRecordWithKeyringFallback`: when the + * keyring is online the record is written with no `fallbackToken`, so the + * keyring read is the only path. When the keyring is offline the token is + * parked on the record and must be reachable on every subsequent read. + * + * Write order is keyring first, then `userRecords.upsert`. If the upsert + * fails after a successful keyring write, the keyring entry is rolled back + * via `deleteSecret()` to avoid orphan credentials for a user that cli-core + * never managed to record. + * + * Clear order is the inverse: record removal first (the source of truth that + * the rest of the CLI reads), then keyring delete. A keyring delete failure + * after a successful removal is downgraded to a warning — the orphan secret + * is harmless because no record references it anymore. + */ +export function createKeyringTokenStore( + options: CreateKeyringTokenStoreOptions, +): KeyringTokenStore { + const { serviceName, userRecords } = options + const accountForUser = options.accountForUser ?? DEFAULT_ACCOUNT_FOR_USER + const matchAccount = options.matchAccount ?? DEFAULT_MATCH_ACCOUNT + + let lastStorageResult: TokenStorageResult | undefined + let lastClearResult: TokenStorageResult | undefined + + function secureStoreFor(id: string): SecureStore { + return createSecureStore({ serviceName, account: accountForUser(id) }) + } + + type Snapshot = { records: UserRecord[]; defaultId: string | null } + + // `getDefaultId` is only needed when no ref is supplied — every + // authenticated command sits on `active()`, so skipping the extra config + // read on the `--user ` path matters for latency. + async function readSnapshot(needsDefault: boolean): Promise { + if (needsDefault) { + const [records, defaultId] = await Promise.all([ + userRecords.list(), + userRecords.getDefaultId(), + ]) + return { records, defaultId } + } + return { records: await userRecords.list(), defaultId: null } + } + + function findByRef(snapshot: Snapshot, ref: AccountRef): UserRecord | null { + return snapshot.records.find((record) => matchAccount(record.account, ref)) ?? null + } + + function resolveDefault(snapshot: Snapshot): UserRecord | null { + if (snapshot.defaultId) { + const found = snapshot.records.find((r) => r.id === snapshot.defaultId) + if (found) return found + } + return snapshot.records.length === 1 ? snapshot.records[0] : null + } + + function fallbackResult(action: string): TokenStorageResult { + return { + storage: 'config-file', + warning: buildFallbackWarning(action, userRecords.describeLocation()), + } + } + + return { + async active(ref) { + const snapshot = await readSnapshot(ref === undefined) + const record = ref === undefined ? resolveDefault(snapshot) : findByRef(snapshot, ref) + if (!record) return null + + if (record.fallbackToken?.trim()) { + return { token: record.fallbackToken.trim(), account: record.account } + } + + try { + const token = await secureStoreFor(record.id).getSecret() + if (token?.trim()) { + return { token: token.trim(), account: record.account } + } + return null + } catch (error) { + // A matching record exists but the keyring can't be read. + // Surface a typed failure instead of returning `null`, which + // would otherwise be indistinguishable from "no stored + // account" and trigger `ACCOUNT_NOT_FOUND` on `--user `. + // `attachLogoutCommand` catches this specific code so an + // explicit `logout --user ` can still clear the matching + // record without needing the unreadable token. + if (error instanceof SecureStoreUnavailableError) { + throw new CliError( + 'AUTH_STORE_READ_FAILED', + `${SECURE_STORE_DESCRIPTION} unavailable; could not read stored token (${error.message})`, + ) + } + throw error + } + }, + + async set(account, token) { + const { storedSecurely } = await writeRecordWithKeyringFallback({ + serviceName, + accountForUser, + userRecords, + account, + token, + }) + + // Best-effort default promotion: the record is already persisted, + // so a failure here must not turn into `AUTH_STORE_WRITE_FAILED` + // (the user can recover with ` account use`). + try { + const existingDefault = await userRecords.getDefaultId() + if (!existingDefault) { + await userRecords.setDefaultId(account.id) + } + } catch { + // best-effort + } + + lastStorageResult = storedSecurely + ? { storage: 'secure-store' } + : fallbackResult('token saved as plaintext in') + }, + + async clear(ref) { + const snapshot = await readSnapshot(ref === undefined) + const record = ref === undefined ? resolveDefault(snapshot) : findByRef(snapshot, ref) + if (!record) { + lastClearResult = undefined + return + } + + await userRecords.remove(record.id) + + // Default un-pinning is best-effort: a failure here must not + // skip the keyring delete below, otherwise we leave an + // unreachable orphan secret behind for the just-removed record. + const wasDefault = await readWasDefault(userRecords, record.id, snapshot) + if (wasDefault) { + try { + await userRecords.setDefaultId(null) + } catch { + // best-effort + } + } + + const fallbackClear = fallbackResult('local auth state cleared in') + + // Always attempt the keyring delete. Even when the record carried + // a `fallbackToken`, an older keyring entry may still be parked + // there from a prior keyring-online write that was later replaced + // by an offline-fallback write — skipping the delete would leak + // that orphan. + try { + await secureStoreFor(record.id).deleteSecret() + lastClearResult = + record.fallbackToken !== undefined ? fallbackClear : { storage: 'secure-store' } + } catch (error) { + if (!(error instanceof SecureStoreUnavailableError)) throw error + lastClearResult = fallbackClear + } + }, + + async list() { + const snapshot = await readSnapshot(true) + const implicitDefault = resolveDefault(snapshot) + return snapshot.records.map((record) => ({ + account: record.account, + isDefault: record.id === implicitDefault?.id, + })) + }, + + async setDefault(ref) { + const all = await userRecords.list() + const record = all.find((r) => matchAccount(r.account, ref)) + if (!record) { + throw accountNotFoundError(ref) + } + await userRecords.setDefaultId(record.id) + }, + + getLastStorageResult() { + return lastStorageResult + }, + + getLastClearResult() { + return lastClearResult + }, + } +} + +async function readWasDefault( + userRecords: UserRecordStore, + id: string, + snapshot: { defaultId: string | null }, +): Promise { + // `clear(ref)` skipped the default read in `readSnapshot`, so check now. + if (snapshot.defaultId !== null) return snapshot.defaultId === id + const current = await userRecords.getDefaultId() + return current === id +} + +function buildFallbackWarning(action: string, location: string): string { + return `${SECURE_STORE_DESCRIPTION} unavailable; ${action} ${location}` +} diff --git a/src/auth/keyring/types.ts b/src/auth/keyring/types.ts new file mode 100644 index 0000000..bdf724d --- /dev/null +++ b/src/auth/keyring/types.ts @@ -0,0 +1,56 @@ +import type { AuthAccount } from '../types.js' + +/** Where a token was (or wasn't) persisted on the most recent write/clear. */ +export type TokenStorageLocation = 'secure-store' | 'config-file' + +export type TokenStorageResult = { + storage: TokenStorageLocation + /** + * Present when the OS keyring was unavailable and the operation fell back + * to (or left state in) the consumer's config file. Suitable for surfacing + * to the user as a `Warning:` line on stderr. + */ + warning?: string +} + +export type UserRecord = { + id: string + account: TAccount + /** + * Plaintext token, present only when the keyring was unavailable at write + * time. The runtime reads it in preference to the keyring slot, so a + * stale fallback would mask a fresh keyring-backed write — consumers + * implementing `upsert` as replace-not-merge (per the contract below) + * guarantees the field is cleared on every successful keyring write. + * Surface its presence as security-relevant: it is the same material + * that would otherwise live in the OS credential manager. + */ + fallbackToken?: string +} + +/** + * Port the consumer implements to expose their per-user config records to + * cli-core's keyring-backed `TokenStore`. The shape of the record map (file + * format, path, schema versioning) stays in the consumer — cli-core only + * needs CRUD on these primitives plus a default-user pointer. + */ +export type UserRecordStore = { + list(): Promise[]> + /** + * **Replace**, do not merge. The persisted record must equal `record` field + * for field — an absent `fallbackToken` means "no plaintext token", and a + * merge-style implementation would let a stale plaintext token outlive a + * later keyring-backed write (the runtime preferentially reads + * `fallbackToken` over the keyring). + */ + upsert(record: UserRecord): Promise + remove(id: string): Promise + getDefaultId(): Promise + setDefaultId(id: string | null): Promise + /** + * Human-readable location used in the fallback warning text (e.g. + * `~/.config/todoist-cli/config.json`). Plain string; cli-core does not + * interpret it. + */ + describeLocation(): string +} diff --git a/src/auth/logout.test.ts b/src/auth/logout.test.ts index e161583..f4a4a7f 100644 --- a/src/auth/logout.test.ts +++ b/src/auth/logout.test.ts @@ -364,4 +364,56 @@ 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 activeSpy = vi.fn(async () => { + throw new CliError('AUTH_STORE_READ_FAILED', 'keyring offline') + }) + const clearSpy = vi.fn(async () => undefined) + const revokeSpy = vi.fn() + const store: TokenStore = { + active: activeSpy, + set: vi.fn(), + clear: clearSpy, + list: vi.fn(async () => [{ account, isDefault: true }]), + setDefault: vi.fn(), + } + const { program, onCleared } = build({ revokeToken: revokeSpy }, store) + + await program.parseAsync(['node', 'cli', 'auth', 'logout', '--user', 'me']) + + expect(clearSpy).toHaveBeenCalledWith('me') + expect(revokeSpy).not.toHaveBeenCalled() + expect(logSpy).toHaveBeenCalledWith('✓ Logged out') + expect(onCleared).toHaveBeenCalledWith({ + account: null, + view: { json: false, ndjson: false }, + flags: {}, + }) + }) + + it('still propagates non-read errors from the snapshot pre-flight', async () => { + const activeSpy = vi.fn(async () => { + throw new CliError('AUTH_STORE_WRITE_FAILED', 'something else') + }) + const clearSpy = vi.fn(async () => undefined) + const store: TokenStore = { + active: activeSpy, + set: vi.fn(), + clear: clearSpy, + list: vi.fn(async () => []), + setDefault: vi.fn(), + } + const { program } = build({ onCleared: undefined }, store) + + await expect( + program.parseAsync(['node', 'cli', 'auth', 'logout', '--user', 'me']), + ).rejects.toMatchObject({ code: 'AUTH_STORE_WRITE_FAILED' }) + expect(clearSpy).not.toHaveBeenCalled() + }) }) diff --git a/src/auth/logout.ts b/src/auth/logout.ts index 00a1026..88b3fc2 100644 --- a/src/auth/logout.ts +++ b/src/auth/logout.ts @@ -1,4 +1,5 @@ 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' @@ -73,7 +74,19 @@ export function attachLogoutCommand( let snapshot: { token: string; account: TAccount } | null = null if (needsSnapshot) { if (ref !== undefined) { - snapshot = await requireSnapshotForRef(options.store, ref) + try { + snapshot = await requireSnapshotForRef(options.store, ref) + } catch (error) { + // `AUTH_STORE_READ_FAILED` means the ref matched a stored + // record but the token couldn't be read (e.g. keyring + // offline). Local clear can still proceed without the + // token; we just skip the revoke and continue. Any other + // typed failure (notably `ACCOUNT_NOT_FOUND` from a + // genuine miss) propagates as before. + if (!(error instanceof CliError && error.code === 'AUTH_STORE_READ_FAILED')) { + throw error + } + } } else { try { snapshot = await options.store.active(ref) diff --git a/src/auth/status.test.ts b/src/auth/status.test.ts index d789064..35be11f 100644 --- a/src/auth/status.test.ts +++ b/src/auth/status.test.ts @@ -246,4 +246,21 @@ describe('attachStatusCommand', () => { code: 'ACCOUNT_NOT_FOUND', }) }) + + it('surfaces AUTH_STORE_READ_FAILED end-to-end (does not collapse to ACCOUNT_NOT_FOUND)', async () => { + const store: TokenStore = { + active: vi.fn(async () => { + throw new CliError('AUTH_STORE_READ_FAILED', 'keyring offline') + }), + set: vi.fn(), + clear: vi.fn(), + list: vi.fn(async () => [{ account, isDefault: true }]), + setDefault: vi.fn(), + } + const { program } = build({}, store) + + await expect( + program.parseAsync(['node', 'cli', 'auth', 'status', '--user', 'me']), + ).rejects.toMatchObject({ code: 'AUTH_STORE_READ_FAILED' }) + }) }) diff --git a/src/auth/user-flag.ts b/src/auth/user-flag.ts index 280402e..262e29d 100644 --- a/src/auth/user-flag.ts +++ b/src/auth/user-flag.ts @@ -18,6 +18,11 @@ export function extractUserRef(cmd: Record): AccountRef | undef return typeof cmd.user === 'string' ? cmd.user : undefined } +/** Shared constructor so `--user` attachers and the keyring `setDefault` can't drift on wording. */ +export function accountNotFoundError(ref: AccountRef): CliError { + return new CliError('ACCOUNT_NOT_FOUND', `No stored account matches "${ref}".`) +} + /** * Read `store.active(ref)` and throw `ACCOUNT_NOT_FOUND` if the explicit * `ref` doesn't match. With `ref === undefined` returns the snapshot @@ -29,7 +34,7 @@ export async function requireSnapshotForRef( ): Promise<{ token: string; account: TAccount } | null> { const snapshot = await store.active(ref) if (ref !== undefined && snapshot === null) { - throw new CliError('ACCOUNT_NOT_FOUND', `No stored account matches "${ref}".`) + throw accountNotFoundError(ref) } return snapshot } diff --git a/src/test-support/keyring-mocks.ts b/src/test-support/keyring-mocks.ts new file mode 100644 index 0000000..d125f65 --- /dev/null +++ b/src/test-support/keyring-mocks.ts @@ -0,0 +1,149 @@ +import { vi } from 'vitest' + +import type { SecureStore } from '../auth/keyring/secure-store.js' +import type { UserRecord, UserRecordStore } from '../auth/keyring/types.js' +import type { AuthAccount } from '../auth/types.js' + +// Test mocks shared between the keyring unit suites. Lives under +// `src/test-support/` so it's excluded from the build (per +// `tsconfig.build.json`) and never reaches consumers via `dist/`. + +export type KeyringSlot = { + secret: string | null + getErr?: unknown + setErr?: unknown + delErr?: unknown +} + +export type KeyringMap = { + create: (args: { serviceName: string; account: string }) => SecureStore + slots: Map + deleteCalls: Map +} + +/** + * Keyed multi-slot keyring mock. Each `account` (slug) gets its own state + * blob so tests can verify that `active()` / `clear()` actually route to the + * right per-user slot. Errors can be pre-seeded per slot. + */ +export function buildKeyringMap(): KeyringMap { + const slots = new Map() + const deleteCalls = new Map() + function getSlot(account: string): KeyringSlot { + let slot = slots.get(account) + if (!slot) { + slot = { secret: null } + slots.set(account, slot) + } + return slot + } + return { + slots, + deleteCalls, + create({ account }) { + return { + async getSecret() { + const slot = getSlot(account) + if (slot.getErr) throw slot.getErr + return slot.secret + }, + async setSecret(secret) { + const slot = getSlot(account) + if (slot.setErr) throw slot.setErr + slot.secret = secret + }, + async deleteSecret() { + deleteCalls.set(account, (deleteCalls.get(account) ?? 0) + 1) + const slot = getSlot(account) + if (slot.delErr) throw slot.delErr + const had = slot.secret !== null + slot.secret = null + return had + }, + } + }, + } +} + +export type SingleSlotMock = SecureStore & { + getSpy: ReturnType + setSpy: ReturnType + deleteSpy: ReturnType +} + +/** + * Simple single-slot keyring mock with spies on each method. Use for tests + * that don't care about per-user slot routing. + */ +export function buildSingleSlot(initial: { secret?: string | null } = {}): SingleSlotMock { + const state = { secret: initial.secret ?? null } + const getSpy = vi.fn(async () => state.secret) + const setSpy = vi.fn(async (secret: string) => { + state.secret = secret + }) + const deleteSpy = vi.fn(async () => { + const had = state.secret !== null + state.secret = null + return had + }) + return { + getSpy, + setSpy, + deleteSpy, + async getSecret() { + return getSpy() + }, + async setSecret(secret: string) { + return setSpy(secret) + }, + async deleteSecret() { + return deleteSpy() + }, + } +} + +export type UserRecordsHarness = { + store: UserRecordStore + state: { + records: Map> + defaultId: string | null + } + upsertSpy: ReturnType + removeSpy: ReturnType + setDefaultSpy: ReturnType +} + +/** In-memory `UserRecordStore` with spies on the mutating methods. */ +export function buildUserRecords( + options: { location?: string } = {}, +): UserRecordsHarness { + const location = options.location ?? '/tmp/fake/config.json' + const state = { + records: new Map>(), + defaultId: null as string | null, + } + const upsertSpy = vi.fn(async (record: UserRecord) => { + state.records.set(record.id, record) + }) + const removeSpy = vi.fn(async (id: string) => { + state.records.delete(id) + }) + const setDefaultSpy = vi.fn(async (id: string | null) => { + state.defaultId = id + }) + const store: UserRecordStore = { + async list() { + return [...state.records.values()] + }, + upsert: upsertSpy, + remove: removeSpy, + async getDefaultId() { + return state.defaultId + }, + setDefaultId: setDefaultSpy, + describeLocation() { + return location + }, + } + return { store, state, upsertSpy, removeSpy, setDefaultSpy } +}