diff --git a/README.md b/README.md index 8c3b595..f8eee3f 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`, `createSecureStore`, `createKeyringTokenStore`, `migrateLegacyAuth`, PKCE helpers, `AuthProvider` / `TokenStore` / `AccountRef` / `SecureStore` / `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`), a thin cross-platform OS-keyring wrapper (`createSecureStore`), 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 bespoke 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 `createSecureStore` or the keyring `TokenStore`) 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). | +| Module | Key exports | Purpose | +| -------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `auth` (subpath) | `attachLoginCommand`, `attachLogoutCommand`, `attachStatusCommand`, `attachTokenViewCommand`, `runOAuthFlow`, `createPkceProvider`, `createSecureStore`, `createKeyringTokenStore`, `migrateLegacyAuth`, `persistBundle`, PKCE helpers, `AuthProvider` / `TokenStore` / `TokenBundle` / `RefreshInput` / `AccountRef` / `SecureStore` / `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`), a thin cross-platform OS-keyring wrapper (`createSecureStore`), 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). The store contract supports an optional `setBundle(account, bundle)` write method (required on `KeyringTokenStore`) so consumers that need refresh-token persistence can opt in via `TokenBundle`; `active()` stays narrow (access token + account only) so callers that don't need refresh state don't pay extra keyring IPC. `AuthProvider` and `TokenStore` remain the escape hatches for DCR or fully bespoke 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 `createSecureStore` or the keyring `TokenStore`) 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 @@ -288,6 +288,14 @@ 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. +##### Refresh-token storage (`TokenBundle`) + +Stores that target servers issuing refresh tokens may implement the optional `setBundle(account, bundle, options?)` method. `TokenBundle` carries `{ accessToken, refreshToken?, accessTokenExpiresAt?, refreshTokenExpiresAt? }`. Stores that omit `setBundle` continue to work — cli-core helpers (`persistBundle`) fall back to `set(account, bundle.accessToken)` and silently drop refresh state. `KeyringTokenStore` implements `setBundle` as a required override and routes the refresh token to a sibling keyring slot. + +`active()` still returns the narrow `{ token, account }` snapshot — refresh-state material is stored but not surfaced on the hot read path so commands that only need the access token don't pay an extra keyring IPC. A bundle-aware read path lands alongside the silent-refresh helper in a follow-up PR. + +The `persistBundle({ store, account, bundle, promoteDefault? })` helper is the recommended write path for bundle-capable consumers — it prefers `setBundle` when available and falls back to `set` otherwise (the `set` fallback can't honour `promoteDefault`, so multi-account stores wanting silent-refresh-safe selection must implement `setBundle`). cli-core's own `runOAuthFlow` still persists via `store.set()` today; it switches to `persistBundle` when the refresh helper lands. + #### Keyring primitive (`createSecureStore`) When the OS credential manager is the right place for your token, `createSecureStore` is a thin cross-platform wrapper around `@napi-rs/keyring`. It exposes a three-method handle (`getSecret` / `setSecret` / `deleteSecret`) for one slot identified by `serviceName` + `account`: diff --git a/src/auth/index.ts b/src/auth/index.ts index 90858c9..e0595f6 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -20,6 +20,8 @@ export { generateVerifier, } from './pkce.js' export type { GenerateVerifierOptions } from './pkce.js' +export { persistBundle } from './persist.js' +export type { PersistBundleOptions } from './persist.js' export { createPkceProvider } from './providers/pkce.js' export type { PkceLazyString, PkceProviderOptions } from './providers/pkce.js' export type { @@ -32,6 +34,8 @@ export type { ExchangeResult, PrepareInput, PrepareResult, + RefreshInput, + TokenBundle, TokenStore, ValidateInput, } from './types.js' diff --git a/src/auth/keyring/record-write.test.ts b/src/auth/keyring/record-write.test.ts index 9d79478..476f73c 100644 --- a/src/auth/keyring/record-write.test.ts +++ b/src/auth/keyring/record-write.test.ts @@ -1,7 +1,8 @@ import { describe, expect, it } from 'vitest' import { buildSingleSlot, buildUserRecords } from '../../test-support/keyring-mocks.js' -import { writeRecordWithKeyringFallback } from './record-write.js' +import type { TokenBundle } from '../types.js' +import { writeBundleWithKeyringFallback, writeRecordWithKeyringFallback } from './record-write.js' import { SecureStoreUnavailableError } from './secure-store.js' type Account = { id: string; label?: string; email: string } @@ -22,7 +23,7 @@ describe('writeRecordWithKeyringFallback', () => { expect(result.storedSecurely).toBe(true) expect(secureStore.setSpy).toHaveBeenCalledWith('tok_secret') - expect(upsertSpy).toHaveBeenCalledWith({ account }) + expect(upsertSpy).toHaveBeenCalledWith({ account, hasRefreshToken: false }) expect(state.records.get('42')?.fallbackToken).toBeUndefined() }) @@ -95,3 +96,160 @@ describe('writeRecordWithKeyringFallback', () => { expect(secureStore.deleteSpy).not.toHaveBeenCalled() }) }) + +describe('writeBundleWithKeyringFallback', () => { + const bundle: TokenBundle = { + accessToken: 'tok_a', + refreshToken: 'tok_r', + accessTokenExpiresAt: 1_700_000_000_000, + refreshTokenExpiresAt: 1_701_000_000_000, + } + + function harness() { + const accessStore = buildSingleSlot() + const refreshStore = buildSingleSlot() + const records = buildUserRecords() + return { accessStore, refreshStore, ...records } + } + + it('writes both slots and persists the bundle metadata on the happy path', async () => { + const { accessStore, refreshStore, store: userRecords, state, upsertSpy } = harness() + + const result = await writeBundleWithKeyringFallback({ + accessStore, + refreshStore, + userRecords, + account, + bundle, + }) + + expect(result).toEqual({ accessStoredSecurely: true, refreshStoredSecurely: true }) + expect(accessStore.setSpy).toHaveBeenCalledWith('tok_a') + expect(refreshStore.setSpy).toHaveBeenCalledWith('tok_r') + expect(upsertSpy).toHaveBeenCalledWith({ + account, + accessTokenExpiresAt: 1_700_000_000_000, + refreshTokenExpiresAt: 1_701_000_000_000, + hasRefreshToken: true, + }) + expect(state.records.get('42')?.fallbackToken).toBeUndefined() + expect(state.records.get('42')?.fallbackRefreshToken).toBeUndefined() + }) + + it('clears the refresh slot when the bundle has no refresh token (no stale carryover)', async () => { + const { accessStore, refreshStore, store: userRecords, state } = harness() + + await writeBundleWithKeyringFallback({ + accessStore, + refreshStore, + userRecords, + account, + bundle: { accessToken: 'tok_a' }, + }) + + expect(refreshStore.deleteSpy).toHaveBeenCalledTimes(1) + expect(state.records.get('42')?.hasRefreshToken).toBe(false) + }) + + it('falls back to fallbackRefreshToken when the refresh slot is offline', async () => { + const { accessStore, refreshStore, store: userRecords, state } = harness() + refreshStore.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus')) + + const result = await writeBundleWithKeyringFallback({ + accessStore, + refreshStore, + userRecords, + account, + bundle, + }) + + expect(result).toEqual({ accessStoredSecurely: true, refreshStoredSecurely: false }) + expect(state.records.get('42')?.fallbackRefreshToken).toBe('tok_r') + expect(state.records.get('42')?.fallbackToken).toBeUndefined() + }) + + it('rolls back the access slot when a non-keyring refresh-slot setSecret error fires', async () => { + // Refresh-slot setSecret throws an unexpected error (not + // SecureStoreUnavailable) — leaving the access slot written would + // orphan a credential against a never-persisted record. + const { accessStore, refreshStore, store: userRecords, state } = harness() + refreshStore.setSpy.mockRejectedValueOnce(new Error('refresh slot blew up')) + + await expect( + writeBundleWithKeyringFallback({ + accessStore, + refreshStore, + userRecords, + account, + bundle, + }), + ).rejects.toThrow('refresh slot blew up') + + expect(accessStore.deleteSpy).toHaveBeenCalledTimes(1) + expect(state.records.size).toBe(0) + }) + + it('rolls back BOTH keyring slots when upsert fails after both writes succeeded', async () => { + const { accessStore, refreshStore, store: userRecords, upsertSpy } = harness() + upsertSpy.mockRejectedValueOnce(new Error('disk full')) + + await expect( + writeBundleWithKeyringFallback({ + accessStore, + refreshStore, + userRecords, + account, + bundle, + }), + ).rejects.toThrow('disk full') + + expect(accessStore.deleteSpy).toHaveBeenCalledTimes(1) + expect(refreshStore.deleteSpy).toHaveBeenCalledTimes(1) + }) + + it('falls back to fallbackToken when the access slot is offline (headless/WSL bundle path)', async () => { + // Real-world headless / WSL / locked-Keychain scenario: D-Bus is + // unavailable, so BOTH slot writes throw SecureStoreUnavailable. + // The record must persist both tokens as plaintext fallbacks. + const { accessStore, refreshStore, store: userRecords, state } = harness() + accessStore.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus')) + refreshStore.setSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('no dbus')) + + const result = await writeBundleWithKeyringFallback({ + accessStore, + refreshStore, + userRecords, + account, + bundle, + }) + + expect(result).toEqual({ accessStoredSecurely: false, refreshStoredSecurely: false }) + expect(state.records.get('42')?.fallbackToken).toBe('tok_a') + expect(state.records.get('42')?.fallbackRefreshToken).toBe('tok_r') + expect(state.records.get('42')?.hasRefreshToken).toBe(true) + }) + + it('defers the no-refresh slot wipe until after upsert succeeds', async () => { + // Regression: wiping before upsert would lose refresh state if the + // upsert then rejected. Order must be set-access → upsert → wipe. + const { accessStore, refreshStore, store: userRecords, upsertSpy } = harness() + const callOrder: string[] = [] + refreshStore.deleteSpy.mockImplementationOnce(async () => { + callOrder.push('refresh-delete') + return false + }) + upsertSpy.mockImplementationOnce(async () => { + callOrder.push('upsert') + }) + + await writeBundleWithKeyringFallback({ + accessStore, + refreshStore, + userRecords, + account, + bundle: { accessToken: 'tok_a' }, + }) + + expect(callOrder).toEqual(['upsert', 'refresh-delete']) + }) +}) diff --git a/src/auth/keyring/record-write.ts b/src/auth/keyring/record-write.ts index c8c850b..40039ca 100644 --- a/src/auth/keyring/record-write.ts +++ b/src/auth/keyring/record-write.ts @@ -1,10 +1,18 @@ -import type { AuthAccount } from '../types.js' +import { CliError } from '../../errors.js' +import type { AuthAccount, TokenBundle } from '../types.js' import { type SecureStore, SecureStoreUnavailableError } from './secure-store.js' import type { UserRecord, UserRecordStore } from './types.js' type WriteRecordOptions = { /** Per-account keyring slot, already configured by the caller (e.g. via `createSecureStore`). */ secureStore: SecureStore + /** + * Optional refresh-token keyring slot. When supplied, any orphan refresh + * material from a prior `setBundle` is wiped best-effort AFTER the user + * record is upserted (see the deferred-cleanup contract on + * `writeBundleWithKeyringFallback`). + */ + refreshStore?: SecureStore userRecords: UserRecordStore account: TAccount token: string @@ -15,53 +23,170 @@ type WriteRecordResult = { storedSecurely: boolean } +type WriteBundleOptions = { + /** Per-account access-token keyring slot. */ + accessStore: SecureStore + /** Per-account refresh-token keyring slot. */ + refreshStore: SecureStore + userRecords: UserRecordStore + account: TAccount + bundle: TokenBundle +} + +type WriteBundleResult = { + /** `true` when the access token landed in the OS keyring; `false` when it fell back to `fallbackToken`. */ + accessStoredSecurely: boolean + /** + * `true` when a refresh token landed in the OS keyring. `false` when it + * fell back to `fallbackRefreshToken`. `undefined` when the bundle + * carried no refresh token (nothing to store). + */ + refreshStoredSecurely: boolean | undefined +} + /** - * 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. + * Single-token write. Thin wrapper over `writeBundleWithKeyringFallback` + * passing a refresh-less bundle, so trim/validate, access-slot fallback, + * upsert rollback, and the deferred refresh-slot wipe all share one + * implementation. * - * 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. + * `refreshStore` is optional purely for legacy callers (`migrateLegacyAuth`) + * that don't have one wired; the migrate path never had refresh state so + * skipping the wipe is correct there. */ export async function writeRecordWithKeyringFallback( options: WriteRecordOptions, ): Promise { - const { secureStore, userRecords, account, token } = options - const trimmed = token.trim() + const { secureStore, refreshStore, userRecords, account, token } = options - let storedSecurely = false + const { accessStoredSecurely } = await writeBundleWithKeyringFallback({ + accessStore: secureStore, + // No-op store when the caller didn't wire one — the deferred wipe + // becomes inert and we don't accidentally create a refresh slot + // for legacy/migrate paths. + refreshStore: refreshStore ?? NOOP_SECURE_STORE, + userRecords, + account, + bundle: { accessToken: token }, + }) + + return { storedSecurely: accessStoredSecurely } +} + +/** + * Two-slot write. Order: access slot → refresh slot → upsert → deferred + * refresh wipe. + * + * 1. Validate `bundle.accessToken` (non-empty after trim). + * 2. `accessStore.setSecret`. `SecureStoreUnavailableError` degrades to + * `fallbackToken` on the record; any other error rethrows. + * 3. `refreshStore.setSecret` when `bundle.refreshToken` is present. + * `SecureStoreUnavailableError` degrades to `fallbackRefreshToken`. A + * non-keyring failure rolls back the access slot before rethrowing + * (no partial credentials left behind for an unregistered user). + * 4. `userRecords.upsert(record)`. On failure, best-effort + * `Promise.allSettled` rollback of any slot writes that succeeded. + * 5. Only after a successful upsert: if the bundle has no refresh token, + * wipe any orphan slot from a prior `setBundle` (best-effort). Doing + * this BEFORE the upsert would lose refresh state if the upsert then + * rejected — the new record's `hasRefreshToken` would still claim + * false but the old slot would be gone with no rollback path. + * + * Default promotion is external — preference, not correctness, and an + * error there must not dirty up a successful credential write. + */ +export async function writeBundleWithKeyringFallback( + options: WriteBundleOptions, +): Promise { + const { accessStore, refreshStore, userRecords, account, bundle } = options + const accessToken = bundle.accessToken.trim() + if (!accessToken) { + throw new CliError( + 'AUTH_STORE_WRITE_FAILED', + 'Refusing to persist a bundle with an empty access token.', + ) + } + const refreshToken = bundle.refreshToken?.trim() + + let accessStoredSecurely = false try { - await secureStore.setSecret(trimmed) - storedSecurely = true + await accessStore.setSecret(accessToken) + accessStoredSecurely = true } catch (error) { if (!(error instanceof SecureStoreUnavailableError)) throw error } - const record: UserRecord = storedSecurely - ? { account } - : { account, fallbackToken: trimmed } + let refreshStoredSecurely: boolean | undefined + if (refreshToken) { + try { + await refreshStore.setSecret(refreshToken) + refreshStoredSecurely = true + } catch (error) { + if (error instanceof SecureStoreUnavailableError) { + refreshStoredSecurely = false + } else { + if (accessStoredSecurely) { + try { + await accessStore.deleteSecret() + } catch { + // best-effort + } + } + throw error + } + } + } + + const record: UserRecord = { + account, + ...(accessStoredSecurely ? {} : { fallbackToken: accessToken }), + ...(refreshToken && refreshStoredSecurely === false + ? { fallbackRefreshToken: refreshToken } + : {}), + ...(bundle.accessTokenExpiresAt !== undefined + ? { accessTokenExpiresAt: bundle.accessTokenExpiresAt } + : {}), + ...(bundle.refreshTokenExpiresAt !== undefined + ? { refreshTokenExpiresAt: bundle.refreshTokenExpiresAt } + : {}), + hasRefreshToken: Boolean(refreshToken), + } try { await userRecords.upsert(record) } catch (error) { - if (storedSecurely) { - try { - await secureStore.deleteSecret() - } catch { - // best-effort — the user record failure is the real cause - } + const rollbacks: Promise[] = [] + if (accessStoredSecurely) rollbacks.push(accessStore.deleteSecret()) + if (refreshStoredSecurely === true) rollbacks.push(refreshStore.deleteSecret()) + if (rollbacks.length > 0) { + await Promise.allSettled(rollbacks) } throw error } - return { storedSecurely } + // Deferred: wipe any orphan refresh slot from a prior setBundle now + // that the new record (with `hasRefreshToken: false`) is durable. If + // this fails the gate already prevents readers from consulting it; the + // worst case is a stale keyring entry that `clear()` will pick up. + if (!refreshToken) { + try { + await refreshStore.deleteSecret() + } catch { + // best-effort + } + } + + return { accessStoredSecurely, refreshStoredSecurely } +} + +const NOOP_SECURE_STORE: SecureStore = { + async getSecret() { + return null + }, + async setSecret() { + // no-op + }, + async deleteSecret() { + return false + }, } diff --git a/src/auth/keyring/slot-naming.test.ts b/src/auth/keyring/slot-naming.test.ts new file mode 100644 index 0000000..a09d8f7 --- /dev/null +++ b/src/auth/keyring/slot-naming.test.ts @@ -0,0 +1,9 @@ +import { describe, expect, it } from 'vitest' + +import { refreshAccountSlot } from './slot-naming.js' + +describe('refreshAccountSlot', () => { + it('derives the refresh slot suffix from the access slot slug', () => { + expect(refreshAccountSlot('user-42')).toBe('user-42/refresh') + }) +}) diff --git a/src/auth/keyring/slot-naming.ts b/src/auth/keyring/slot-naming.ts new file mode 100644 index 0000000..ebae7f2 --- /dev/null +++ b/src/auth/keyring/slot-naming.ts @@ -0,0 +1,7 @@ +/** + * Derives the refresh slot name from the access slug. Single-sourced so the + * write and read paths can't drift onto different suffixes. Internal. + */ +export function refreshAccountSlot(accountSlug: string): string { + return `${accountSlug}/refresh` +} diff --git a/src/auth/keyring/token-store.test.ts b/src/auth/keyring/token-store.test.ts index cfbfec5..6bdb556 100644 --- a/src/auth/keyring/token-store.test.ts +++ b/src/auth/keyring/token-store.test.ts @@ -5,7 +5,9 @@ import { buildSingleSlot, buildUserRecords, } from '../../test-support/keyring-mocks.js' +import type { TokenBundle } from '../types.js' import { SecureStoreUnavailableError } from './secure-store.js' +import { refreshAccountSlot } from './slot-naming.js' import { type CreateKeyringTokenStoreOptions, createKeyringTokenStore } from './token-store.js' import type { UserRecord } from './types.js' @@ -69,24 +71,51 @@ function fixture( } } +/** + * Per-slot keyring routing — required whenever a test exercises both the + * access slot and the refresh slot (otherwise the single-slot mock would + * conflate them). + */ +function mapFixture( + records: Record> = {}, + defaultId: string | null = null, +) { + const km = buildKeyringMap() + mockedCreateSecureStore.mockImplementation(km.create) + const harness = buildUserRecords() + for (const [id, rec] of Object.entries(records)) { + harness.state.records.set(id, rec) + } + harness.state.defaultId = defaultId + const store = createKeyringTokenStore({ + serviceName: SERVICE, + userRecords: harness.store, + recordsLocation: LOCATION, + }) + return { km, store, state: harness.state, upsertSpy: harness.upsertSpy } +} + describe('createKeyringTokenStore', () => { beforeEach(() => { mockedCreateSecureStore.mockReset() }) it('round-trips set → active → clear when the keyring is online', async () => { - const { keyring, store, state, upsertSpy } = fixture() + // Per-slot map: set() wipes the refresh slot too, so a single-slot + // mock would clobber the access secret. + const { km, store, state, upsertSpy } = mapFixture() await store.set(account, 'tok_secret') - expect(keyring.setSpy).toHaveBeenCalledWith('tok_secret') - expect(upsertSpy).toHaveBeenCalledWith({ account }) + expect(km.slots.get('user-42')?.secret).toBe('tok_secret') + expect(upsertSpy).toHaveBeenCalledWith({ account, hasRefreshToken: false }) expect(state.defaultId).toBe('42') expect(store.getLastStorageResult()).toEqual({ storage: 'secure-store' }) await expect(store.active()).resolves.toEqual({ token: 'tok_secret', account }) await store.clear() - expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) + expect(km.slots.get('user-42')?.secret).toBeNull() + expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBeNull() expect(state.records.size).toBe(0) expect(state.defaultId).toBeNull() expect(store.getLastClearResult()).toEqual({ storage: 'secure-store' }) @@ -100,10 +129,13 @@ describe('createKeyringTokenStore', () => { await store.set(account, 'tok_plain') expect(state.records.get('42')?.fallbackToken).toBe('tok_plain') + // `set()` writes `hasRefreshToken: false` definitively, so the next + // `active()` skips the refresh-slot IPC entirely. + expect(state.records.get('42')?.hasRefreshToken).toBe(false) expect(store.getLastStorageResult()).toEqual({ storage: 'config-file', warning: - 'system credential manager unavailable; token saved as plaintext in /tmp/fake/config.json', + 'system credential manager unavailable; access token saved as plaintext in /tmp/fake/config.json', }) await expect(store.active()).resolves.toEqual({ token: 'tok_plain', account }) @@ -133,11 +165,19 @@ describe('createKeyringTokenStore', () => { it.each([ [ - 'the keyring read throws', + 'the keyring read throws SecureStoreUnavailableError', (k: SingleSlot) => { k.getSpy.mockRejectedValueOnce(new SecureStoreUnavailableError('locked')) }, ], + [ + 'the keyring read throws a non-keyring backend error', + (k: SingleSlot) => { + // Generic backend failure must also wrap into the typed code; + // a raw exception would crash the CLI with no exit signal. + k.getSpy.mockRejectedValueOnce(new Error('disk fried')) + }, + ], [ 'the keyring slot is empty (out-of-band deletion)', () => { @@ -154,8 +194,8 @@ describe('createKeyringTokenStore', () => { }) it('picks the lone user when no default is set', async () => { - const keyring = buildSingleSlot({ secret: 'tok' }) - const { store } = fixture({ keyring, records: { '42': { account } } }) + const { km, store } = mapFixture({ '42': { account } }) + km.slots.set('user-42', { secret: 'tok' }) await expect(store.active()).resolves.toEqual({ token: 'tok', account }) }) @@ -209,7 +249,8 @@ describe('createKeyringTokenStore', () => { await store.clear() - expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) + // Both slots wiped — access + refresh. Single-slot mock counts both. + expect(keyring.deleteSpy).toHaveBeenCalledTimes(2) expect(state.records.size).toBe(0) expect(store.getLastClearResult()).toEqual({ storage: 'config-file', @@ -255,16 +296,17 @@ describe('createKeyringTokenStore', () => { const keyring = buildSingleSlot({ secret: 'tok' }) const { store, state, setDefaultSpy } = fixture({ keyring, - records: { '42': { account } }, + records: { '42': { account, hasRefreshToken: false } }, defaultId: '42', }) setDefaultSpy.mockRejectedValueOnce(new Error('disk full')) await store.clear() - // Default pointer write blew up, but the keyring entry was still - // cleaned up — otherwise the credential becomes an unreachable orphan. - expect(keyring.deleteSpy).toHaveBeenCalledTimes(1) + // Default pointer write blew up, but the keyring entries were still + // cleaned up — otherwise the credentials become unreachable orphans. + // Both slots (access + refresh) are wiped. + expect(keyring.deleteSpy).toHaveBeenCalledTimes(2) expect(state.records.size).toBe(0) }) @@ -409,4 +451,74 @@ describe('createKeyringTokenStore', () => { }) }) }) + + describe('setBundle storage', () => { + const bundle: TokenBundle = { + accessToken: 'tok_a', + refreshToken: 'tok_r', + accessTokenExpiresAt: 1_700_000_000_000, + } + + it('persists access slot, refresh slot, and record metadata; active() returns the access snapshot', async () => { + const { km, store, state } = mapFixture() + + await store.setBundle(account, bundle, { promoteDefault: true }) + + // Storage: both slots written, record carries gate + expiry. + expect(km.slots.get('user-42')?.secret).toBe('tok_a') + expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBe('tok_r') + const record = state.records.get('42') + expect(record?.hasRefreshToken).toBe(true) + expect(record?.accessTokenExpiresAt).toBe(1_700_000_000_000) + expect(record?.fallbackToken).toBeUndefined() + expect(record?.fallbackRefreshToken).toBeUndefined() + expect(state.defaultId).toBe('42') + + // Read side stays narrow: active() returns only token + account. + // Reading the stored refresh state is a PR3 concern. + await expect(store.active()).resolves.toEqual({ token: 'tok_a', account }) + }) + + it('omits promoteDefault by default (silent-refresh path does not re-pin)', async () => { + const { store, state } = mapFixture({ + '42': { account, hasRefreshToken: false }, + }) + + await store.setBundle(account, bundle) + + expect(state.defaultId).toBeNull() + }) + + it('set() wipes the refresh slot left behind by a prior setBundle', async () => { + // Regression: `set(account, token)` is documented as "replacing + // any previous entry". A later set() must leave no orphan + // refresh material — otherwise a future bundle-aware reader + // would see stale data. + const { km, store, state } = mapFixture() + + await store.setBundle(account, bundle, { promoteDefault: true }) + expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBe('tok_r') + + await store.set(account, 'tok_a_replacement') + + expect(km.slots.get('user-42')?.secret).toBe('tok_a_replacement') + expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBeNull() + expect(state.records.get('42')?.hasRefreshToken).toBe(false) + }) + + it('clear() wipes both keyring slots', async () => { + const { km, store, state } = mapFixture({ + '42': { account, hasRefreshToken: true }, + }) + km.slots.set('user-42', { secret: 'tok_a' }) + km.slots.set(refreshAccountSlot('user-42'), { secret: 'tok_r' }) + state.defaultId = '42' + + await store.clear() + + expect(state.records.size).toBe(0) + expect(km.slots.get('user-42')?.secret).toBeNull() + expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBeNull() + }) + }) }) diff --git a/src/auth/keyring/token-store.ts b/src/auth/keyring/token-store.ts index 3a67895..a2cbd47 100644 --- a/src/auth/keyring/token-store.ts +++ b/src/auth/keyring/token-store.ts @@ -1,7 +1,7 @@ -import { CliError } from '../../errors.js' -import type { AccountRef, AuthAccount, TokenStore } from '../types.js' +import { CliError, getErrorMessage } from '../../errors.js' +import type { AccountRef, AuthAccount, TokenBundle, TokenStore } from '../types.js' import { accountNotFoundError } from '../user-flag.js' -import { writeRecordWithKeyringFallback } from './record-write.js' +import { writeBundleWithKeyringFallback, writeRecordWithKeyringFallback } from './record-write.js' import { createSecureStore, DEFAULT_ACCOUNT_FOR_USER, @@ -9,6 +9,7 @@ import { SecureStoreUnavailableError, type SecureStore, } from './secure-store.js' +import { refreshAccountSlot } from './slot-naming.js' import type { TokenStorageResult, UserRecord, UserRecordStore } from './types.js' export type CreateKeyringTokenStoreOptions = { @@ -36,7 +37,17 @@ export type CreateKeyringTokenStoreOptions = { } export type KeyringTokenStore = TokenStore & { - /** Storage result from the most recent `set()` call, or `undefined` before any (and reset to `undefined` when the most recent `set()` threw). */ + /** + * Override `setBundle` as required (not optional) — the keyring store + * always knows how to persist refresh state. Lets cli-core helpers + * (`persistBundle`) call it without a non-null assertion. + */ + setBundle( + account: TAccount, + bundle: TokenBundle, + options?: { promoteDefault?: boolean }, + ): Promise + /** Storage result from the most recent `set()` / `setBundle()` call, or `undefined` before any (and reset to `undefined` when the most recent write threw). */ getLastStorageResult(): TokenStorageResult | undefined /** Storage result from the most recent `clear()` call, or `undefined` before any (and reset to `undefined` when the most recent `clear()` threw or was a no-op). */ getLastClearResult(): TokenStorageResult | undefined @@ -86,6 +97,13 @@ export function createKeyringTokenStore( return createSecureStore({ serviceName, account: accountForUser(account.id) }) } + function refreshSecureStoreFor(account: TAccount): SecureStore { + return createSecureStore({ + serviceName, + account: refreshAccountSlot(accountForUser(account.id)), + }) + } + type Snapshot = { records: UserRecord[]; defaultId: string | null } /** @@ -147,6 +165,47 @@ export function createKeyringTokenStore( } } + /** + * Compose a storage result for a write that may have fallen back on + * either slot. `accessStored === false` indicates the access token went + * to `fallbackToken`; `refreshStored === false` indicates the refresh + * token went to `fallbackRefreshToken`. Either falsy slot downgrades + * the result to `config-file` so consumers see the warning — refresh + * plaintext is just as security-relevant as access plaintext. + */ + function bundleStorageResult( + accessStored: boolean, + refreshStored: boolean | undefined, + ): TokenStorageResult { + const accessFallback = !accessStored + const refreshFallback = refreshStored === false + if (!accessFallback && !refreshFallback) return { storage: 'secure-store' } + const subject = + accessFallback && refreshFallback + ? 'access + refresh tokens' + : accessFallback + ? 'access token' + : 'refresh token' + return fallbackResult(`${subject} saved as plaintext in`) + } + + /** + * Best-effort default promotion shared by `set` and `setBundle`. The + * record is already persisted, so a failure here must not surface as + * `AUTH_STORE_WRITE_FAILED` — the user can recover by setting a + * default later. + */ + async function promoteDefaultIfNeeded(accountId: string): Promise { + try { + const existingDefault = await userRecords.getDefaultId() + if (!existingDefault) { + await userRecords.setDefaultId(accountId) + } + } catch { + // best-effort + } + } + return { async active(ref) { // Ref-only path skips `getDefaultId()` — `resolveTarget` never @@ -159,42 +218,37 @@ export function createKeyringTokenStore( const record = resolveTarget(snapshot, ref) if (!record) return null + // Reads the access slot only. Refresh-state material lives in + // the keyring and on the record, but `active()` stays cheap and + // returns the pre-PR1 snapshot shape — a future bundle-aware + // read path lights up the refresh slot only when callers + // actually need it (silent refresh). const fallback = record.fallbackToken?.trim() - if (fallback) { - return { token: fallback, account: record.account } - } + if (fallback) return { token: fallback, account: record.account } let raw: string | null try { raw = await secureStoreFor(record.account).getSecret() } 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 + // Non-keyring backend failures wrap into the typed code too — + // a raw exception escaping `active()` would crash the CLI + // with no useful exit signal. + throw new CliError( + 'AUTH_STORE_READ_FAILED', + `Access-slot read failed (${getErrorMessage(error)})`, + ) } const token = raw?.trim() - if (token) { - return { token, account: record.account } - } + if (token) return { token, account: record.account } - // Record exists, no `fallbackToken`, and the keyring slot is - // empty — the credential was deleted out-of-band (user ran - // `security delete-generic-password`, `secret-tool clear`, …). - // This is corrupted state, not a miss; collapsing it to `null` - // would make `--user ` surface as `ACCOUNT_NOT_FOUND` and - // hide the real problem. + // Record exists, no `fallbackToken`, slot empty — corruption. throw new CliError( 'AUTH_STORE_READ_FAILED', `${SECURE_STORE_DESCRIPTION} returned no credential for the stored account; the keyring entry may have been removed externally.`, @@ -209,26 +263,36 @@ export function createKeyringTokenStore( const { storedSecurely } = await writeRecordWithKeyringFallback({ secureStore: secureStoreFor(account), + refreshStore: refreshSecureStoreFor(account), 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 by setting a default later). - try { - const existingDefault = await userRecords.getDefaultId() - if (!existingDefault) { - await userRecords.setDefaultId(account.id) - } - } catch { - // best-effort + await promoteDefaultIfNeeded(account.id) + + lastStorageResult = bundleStorageResult(storedSecurely, undefined) + }, + + async setBundle(account, bundle, options) { + lastStorageResult = undefined + + const { accessStoredSecurely, refreshStoredSecurely } = + await writeBundleWithKeyringFallback({ + accessStore: secureStoreFor(account), + refreshStore: refreshSecureStoreFor(account), + userRecords, + account, + bundle, + }) + + // Opt-in: silent refresh omits `promoteDefault` so it can't + // re-pin selection; login passes `true` to match `set()`. + if (options?.promoteDefault) { + await promoteDefaultIfNeeded(account.id) } - lastStorageResult = storedSecurely - ? { storage: 'secure-store' } - : fallbackResult('token saved as plaintext in') + lastStorageResult = bundleStorageResult(accessStoredSecurely, refreshStoredSecurely) }, async clear(ref) { @@ -259,21 +323,20 @@ export function createKeyringTokenStore( 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. Downgrade *any* failure to a warning: the record - // is already gone, so re-throwing would corrupt local state - // (caller sees an exception and assumes nothing was cleared, - // even though the next `account list` will show the user gone). - try { - await secureStoreFor(record.account).deleteSecret() - lastClearResult = - record.fallbackToken !== undefined ? fallbackClear : { storage: 'secure-store' } - } catch { - lastClearResult = fallbackClear - } + // Always attempt both deletes — a record's `fallbackToken` + // doesn't rule out an orphan keyring entry from a prior online + // write. Failures downgrade to a warning: the record is already + // gone, re-throwing would corrupt the caller's state. + const [accessOutcome, refreshOutcome] = await Promise.allSettled([ + secureStoreFor(record.account).deleteSecret(), + refreshSecureStoreFor(record.account).deleteSecret(), + ]) + const fellBack = + accessOutcome.status === 'rejected' || + refreshOutcome.status === 'rejected' || + record.fallbackToken !== undefined || + record.fallbackRefreshToken !== undefined + lastClearResult = fellBack ? fallbackClear : { storage: 'secure-store' } }, async list() { diff --git a/src/auth/keyring/types.ts b/src/auth/keyring/types.ts index c873dcc..53ce80a 100644 --- a/src/auth/keyring/types.ts +++ b/src/auth/keyring/types.ts @@ -25,6 +25,20 @@ export type UserRecord = { * that would otherwise live in the OS credential manager. */ fallbackToken?: string + /** Same lifecycle and security profile as `fallbackToken`, for the refresh slot. */ + fallbackRefreshToken?: string + /** Access-token expiry, unix-epoch ms. */ + accessTokenExpiresAt?: number + /** Refresh-token expiry, unix-epoch ms. */ + refreshTokenExpiresAt?: number + /** + * `true` when a refresh secret is stored (in the keyring or as + * `fallbackRefreshToken`); `false` when explicitly cleared by `set()` + * or by a no-refresh `setBundle`; `undefined` on legacy records that + * predate the bundle contract. Read by future bundle-aware accessors; + * `active()` itself doesn't consult it. + */ + hasRefreshToken?: boolean } /** @@ -43,6 +57,12 @@ export type UserRecordStore = { * `fallbackToken` over the keyring). Records are keyed by `account.id`. */ upsert(record: UserRecord): Promise + /** + * Optional atomic insert. Returns `true` on write, `false` if `account.id` + * already exists. Migration prefers it to eliminate the existence-check + * TOCTOU race; callers fall back to list-then-upsert when absent. + */ + tryInsert?(record: UserRecord): Promise /** Remove the record whose `account.id` matches. */ remove(id: string): Promise /** The pinned default's `account.id`, or `null` when nothing is pinned. */ diff --git a/src/auth/persist.test.ts b/src/auth/persist.test.ts new file mode 100644 index 0000000..8144a3c --- /dev/null +++ b/src/auth/persist.test.ts @@ -0,0 +1,82 @@ +import { describe, expect, it, vi } from 'vitest' + +import { CliError } from '../errors.js' +import { persistBundle } from './persist.js' +import type { AuthAccount, TokenBundle, TokenStore } from './types.js' + +type Account = AuthAccount & { id: string; email: string } + +const account: Account = { id: '42', email: 'a@b.c' } +const bundle: TokenBundle = { + accessToken: 'tok_access', + refreshToken: 'tok_refresh', + accessTokenExpiresAt: 1_700_000_000_000, +} + +function fakeStore(overrides: Partial> = {}): TokenStore { + return { + active: vi.fn(async () => null), + set: vi.fn(async () => undefined), + clear: vi.fn(async () => undefined), + list: vi.fn(async () => []), + setDefault: vi.fn(async () => undefined), + ...overrides, + } +} + +describe('persistBundle', () => { + it('prefers setBundle when the store implements it', async () => { + const setBundle = vi.fn(async () => undefined) + const set = vi.fn(async () => undefined) + const store = fakeStore({ setBundle, set }) + + await persistBundle({ store, account, bundle, promoteDefault: true }) + + expect(setBundle).toHaveBeenCalledWith(account, bundle, { promoteDefault: true }) + expect(set).not.toHaveBeenCalled() + }) + + it('falls back to set(accessToken) when the store does not implement setBundle', async () => { + const set = vi.fn(async () => undefined) + const store = fakeStore({ set }) + + await persistBundle({ store, account, bundle, promoteDefault: true }) + + expect(set).toHaveBeenCalledWith(account, 'tok_access') + }) + + it('omits the options argument entirely when promoteDefault is unset', async () => { + const setBundle = vi.fn(async () => undefined) + const store = fakeStore({ setBundle }) + + await persistBundle({ store, account, bundle }) + + // Presence-based handling: callers must be able to distinguish + // "default behaviour" from explicit opt-out via arg count. + expect(setBundle).toHaveBeenCalledTimes(1) + expect(setBundle.mock.calls[0]).toEqual([account, bundle]) + }) + + it('rethrows CliError without wrapping', async () => { + const cause = new CliError('AUTH_STORE_WRITE_FAILED', 'boom') + const store = fakeStore({ + setBundle: vi.fn(async () => { + throw cause + }), + }) + + await expect(persistBundle({ store, account, bundle })).rejects.toBe(cause) + }) + + it('wraps non-CliError failures as AUTH_STORE_WRITE_FAILED', async () => { + const store = fakeStore({ + setBundle: vi.fn(async () => { + throw new Error('disk full') + }), + }) + + await expect(persistBundle({ store, account, bundle })).rejects.toMatchObject({ + code: 'AUTH_STORE_WRITE_FAILED', + }) + }) +}) diff --git a/src/auth/persist.ts b/src/auth/persist.ts new file mode 100644 index 0000000..df5f389 --- /dev/null +++ b/src/auth/persist.ts @@ -0,0 +1,48 @@ +import { CliError, getErrorMessage } from '../errors.js' +import type { AuthAccount, TokenBundle, TokenStore } from './types.js' + +export type PersistBundleOptions = { + store: TokenStore + account: TAccount + bundle: TokenBundle + /** Forwarded to `setBundle` when present. See `TokenStore.setBundle`. */ + promoteDefault?: boolean +} + +/** + * Persist a bundle against any `TokenStore`. Prefers `setBundle` when the + * store implements it; otherwise falls back to `set(account, accessToken)` + * and silently drops refresh state. Wraps non-`CliError` failures as + * `AUTH_STORE_WRITE_FAILED`. + * + * `promoteDefault` is only honoured on the `setBundle` path — the base + * `set()` contract has no promotion control, so a custom multi-account + * store that opts out of `setBundle` will run its own promotion policy + * (typically first-account-wins). Multi-account stores that need + * silent-refresh-safe selection (no re-pinning on background rotation) + * MUST implement `setBundle`. + */ +export async function persistBundle( + options: PersistBundleOptions, +): Promise { + const { store, account, bundle, promoteDefault } = options + try { + if (store.setBundle) { + // Omit the options arg entirely when unset so presence-based + // handlers can distinguish "default" from explicit opt-out. + if (promoteDefault === undefined) { + await store.setBundle(account, bundle) + } else { + await store.setBundle(account, bundle, { promoteDefault }) + } + } else { + await store.set(account, bundle.accessToken) + } + } catch (error) { + if (error instanceof CliError) throw error + throw new CliError( + 'AUTH_STORE_WRITE_FAILED', + `Failed to persist token: ${getErrorMessage(error)}`, + ) + } +} diff --git a/src/auth/types.ts b/src/auth/types.ts index 4949389..934d9a9 100644 --- a/src/auth/types.ts +++ b/src/auth/types.ts @@ -46,8 +46,10 @@ export type ExchangeInput = { export type ExchangeResult = { accessToken: string refreshToken?: string - /** Unix-epoch ms. cli-core does not refresh today. */ + /** Access-token expiry, unix-epoch ms. */ expiresAt?: number + /** Refresh-token expiry, unix-epoch ms. */ + refreshTokenExpiresAt?: number /** Set when the token endpoint already identifies the account; skips `validateToken`. */ account?: TAccount } @@ -58,6 +60,12 @@ export type ValidateInput = { handshake: Record } +export type RefreshInput = { + refreshToken: string + /** Same shape as `ExchangeInput.handshake` — empty when called outside `runOAuthFlow`. */ + handshake: Record +} + /** * Strategy interface every auth method implements. cli-core ships * `createPkceProvider` for the standard public-client PKCE flow; bespoke @@ -70,6 +78,16 @@ export type AuthProvider = { exchangeCode(input: ExchangeInput): Promise> /** Skipped when `exchangeCode` already returned an `account`. */ validateToken(input: ValidateInput): Promise + /** Optional: exchange a refresh token for a fresh bundle. */ + refreshToken?(input: RefreshInput): Promise> +} + +/** Write-side bundle for `setBundle`. Time fields are unix-epoch ms. */ +export type TokenBundle = { + accessToken: string + refreshToken?: string + accessTokenExpiresAt?: number + refreshTokenExpiresAt?: number } /** Opaque account selector. Stores own the matching rule (id, email, label, …). */ @@ -93,6 +111,18 @@ export type TokenStore = { active(ref?: AccountRef): Promise<{ token: string; account: TAccount } | null> /** Persist `token` for `account`, replacing any previous entry. Throw `CliError` for typed failures; other thrown values become `AUTH_STORE_WRITE_FAILED`. */ set(account: TAccount, token: string): Promise + /** + * Persist a full bundle. Optional on the contract — stores that don't + * implement it get `bundle.accessToken` via `set()` instead (cli-core + * helpers handle the fallback). Pass `promoteDefault: true` on first + * login; omit on silent refresh so a background rotation can't re-pin + * account selection. + */ + setBundle?( + account: TAccount, + bundle: TokenBundle, + options?: { promoteDefault?: boolean }, + ): Promise /** Remove a stored credential. No-op when `ref` doesn't match. */ clear(ref?: AccountRef): Promise /** Every stored account with a default marker. */ diff --git a/src/test-support/keyring-mocks.ts b/src/test-support/keyring-mocks.ts index 7125b31..4d34a7d 100644 --- a/src/test-support/keyring-mocks.ts +++ b/src/test-support/keyring-mocks.ts @@ -18,6 +18,8 @@ type KeyringSlot = { type KeyringMap = { create: (args: { serviceName: string; account: string }) => SecureStore slots: Map + /** Per-slot count of `getSecret()` invocations. */ + getCalls: Map deleteCalls: Map } @@ -28,6 +30,7 @@ type KeyringMap = { */ export function buildKeyringMap(): KeyringMap { const slots = new Map() + const getCalls = new Map() const deleteCalls = new Map() function getSlot(account: string): KeyringSlot { let slot = slots.get(account) @@ -39,10 +42,12 @@ export function buildKeyringMap(): KeyringMap { } return { slots, + getCalls, deleteCalls, create({ account }) { return { async getSecret() { + getCalls.set(account, (getCalls.get(account) ?? 0) + 1) const slot = getSlot(account) if (slot.getErr) throw slot.getErr return slot.secret