diff --git a/src/auth/keyring/internal.ts b/src/auth/keyring/internal.ts new file mode 100644 index 0000000..8145f75 --- /dev/null +++ b/src/auth/keyring/internal.ts @@ -0,0 +1,44 @@ +import { getErrorMessage } from '../../errors.js' +import type { AuthAccount } from '../types.js' +import { type SecureStore, SecureStoreUnavailableError } from './secure-store.js' +import type { UserRecord } from './types.js' + +/** + * Outcome of resolving the access token for a record. Callers map the + * structured failure variants to whatever error contract they expose — + * `KeyringTokenStore.active()` throws `CliError('AUTH_STORE_READ_FAILED', …)`; + * `migrateLegacyAuth` translates each variant into a `MigrateSkipReason`. + */ +export type ReadAccessTokenOutcome = + | { ok: true; token: string } + | { ok: false; reason: 'slot-empty' | 'slot-unavailable' | 'slot-error'; detail: string } + +/** + * `fallbackToken` first (so an offline-keyring write is preferred over a + * stale slot), then the keyring slot. Single-source for "is this record + * readable in the current environment" — `KeyringTokenStore.active()` and + * `migrateLegacyAuth`'s readability probe both call this. + */ +export async function readAccessTokenForRecord( + record: UserRecord, + secureStore: SecureStore, +): Promise { + const fallback = record.fallbackToken?.trim() + if (fallback) return { ok: true, token: fallback } + + try { + const raw = await secureStore.getSecret() + const trimmed = raw?.trim() + if (trimmed) return { ok: true, token: trimmed } + return { + ok: false, + reason: 'slot-empty', + detail: 'keyring slot returned no credential', + } + } catch (error) { + if (error instanceof SecureStoreUnavailableError) { + return { ok: false, reason: 'slot-unavailable', detail: error.message } + } + return { ok: false, reason: 'slot-error', detail: getErrorMessage(error) } + } +} diff --git a/src/auth/keyring/migrate.test.ts b/src/auth/keyring/migrate.test.ts index 0204295..233eaaa 100644 --- a/src/auth/keyring/migrate.test.ts +++ b/src/auth/keyring/migrate.test.ts @@ -42,6 +42,8 @@ async function runMigration( seedRecords?: Record> seedDefaultId?: string marker?: MarkerState + /** Opt the harness into the atomic `tryInsert?` path. */ + withTryInsert?: boolean options?: Partial> } = {}, ) { @@ -51,7 +53,7 @@ async function runMigration( } mockedCreateSecureStore.mockImplementation(km.create) - const harness = buildUserRecords() + const harness = buildUserRecords({ withTryInsert: opts.withTryInsert }) for (const [id, rec] of Object.entries(opts.seedRecords ?? {})) { harness.state.records.set(id, rec) } @@ -110,7 +112,7 @@ describe('migrateLegacyAuth', () => { it('migrates a legacy keyring token into a per-user slot and clears the legacy entry', async () => { const cleanup = vi.fn(async () => undefined) - const { km, state, harness, result, marker, markMigrated } = await runMigration({ + const { km, state, result, marker, markMigrated } = await runMigration({ slots: { [LEGACY]: { secret: 'legacy_tok' } }, options: { identifyAccount: async (token) => { @@ -127,7 +129,6 @@ describe('migrateLegacyAuth', () => { expect(km.slots.get(LEGACY)?.secret).toBeNull() expect(state.records.get('99')?.fallbackToken).toBeUndefined() expect(state.defaultId).toBe('99') - expect(harness.upsertSpy).toHaveBeenCalledTimes(1) expect(cleanup).toHaveBeenCalledTimes(1) expect(markMigrated).toHaveBeenCalledTimes(1) expect(marker.migrated).toBe(true) @@ -298,6 +299,207 @@ describe('migrateLegacyAuth', () => { expect(km.slots.get('user-99')?.secret).toBeUndefined() }) + it('uses tryInsert (atomic) and never calls list() when the store implements it', async () => { + // Atomic path must avoid the list-then-upsert TOCTOU race entirely. + const { km, harness, result } = await runMigration({ + slots: { [LEGACY]: { secret: 'legacy_tok' } }, + withTryInsert: true, + options: { + identifyAccount: async () => ({ id: '99', email: 'me@x.io' }), + }, + }) + + expect(result.status).toBe('migrated') + expect(harness.tryInsertSpy).toHaveBeenCalledTimes(1) + // Phase 2 promotes the record to the clean (no-fallback) shape. + expect(harness.upsertSpy).toHaveBeenCalledTimes(1) + // Atomic insert means `list()` is never consulted on Phase 1. + expect(harness.listSpy).not.toHaveBeenCalled() + expect(km.slots.get('user-99')?.secret).toBe('legacy_tok') + }) + + it.each([true, false])( + 'skips Phase 2 when a v2 record already exists (withTryInsert=%s)', + async (withTryInsert) => { + const existing: UserRecord = { + account: { id: '99', email: 'me@x.io', label: 'updated-label' }, + hasRefreshToken: false, + } + const { km, state, result, marker, harness } = await runMigration({ + slots: { + [LEGACY]: { secret: 'legacy_tok' }, + 'user-99': { secret: 'fresh_login_tok' }, + }, + seedRecords: { '99': existing }, + withTryInsert, + options: { + identifyAccount: async () => ({ id: '99', email: 'me@x.io' }), + }, + }) + + expect(result.status).toBe('migrated') + expect(harness.upsertSpy).not.toHaveBeenCalled() + expect(state.records.get('99')).toBe(existing) + expect(km.slots.get('user-99')?.secret).toBe('fresh_login_tok') + expect(km.slots.get(LEGACY)?.secret).toBeNull() + expect(marker.migrated).toBe(true) + }, + ) + + it('returns skipped(user-keyring-unreachable) when an existing v2 record cannot be read', async () => { + // A clean v2 record from a prior set/setBundle has no fallbackToken + // and depends on the keyring being reachable. With the keyring + // offline, cleaning up the legacy state would brick the user — we + // must abort the migration and leave the legacy token in place. + const existing: UserRecord = { + account: { id: '99', email: 'me@x.io' }, + hasRefreshToken: false, + } + const { km, state, result, marker } = await runMigration({ + slots: { + [LEGACY]: { secret: 'legacy_tok' }, + 'user-99': { getErr: new SecureStoreUnavailableError('no dbus') }, + }, + seedRecords: { '99': existing }, + options: { + identifyAccount: async () => ({ id: '99', email: 'me@x.io' }), + }, + }) + + expect(result).toMatchObject({ + status: 'skipped', + reason: 'user-keyring-unreachable', + }) + expect(marker.migrated).toBe(false) + expect(km.slots.get(LEGACY)?.secret).toBe('legacy_tok') + expect(state.records.get('99')).toBe(existing) + }) + + it('returns skipped(user-record-write-failed) when an existing v2 record has empty fallback AND empty slot', async () => { + // Corrupted state: the record carries no plaintext fallback and + // the per-user keyring slot is empty (deleted out of band). We + // must NOT cleanup legacy — the user has no readable credential. + const existing: UserRecord = { + account: { id: '99', email: 'me@x.io' }, + hasRefreshToken: false, + } + const { km, state, result, marker } = await runMigration({ + slots: { + [LEGACY]: { secret: 'legacy_tok' }, + // No `secret` and no `getErr` → getSecret returns null. + 'user-99': {}, + }, + seedRecords: { '99': existing }, + options: { + identifyAccount: async () => ({ id: '99', email: 'me@x.io' }), + }, + }) + + expect(result).toMatchObject({ + status: 'skipped', + reason: 'user-record-write-failed', + }) + expect(marker.migrated).toBe(false) + expect(km.slots.get(LEGACY)?.secret).toBe('legacy_tok') + expect(state.records.get('99')).toBe(existing) + }) + + it('skips the readability check when an existing v2 record carries an external fallbackToken', async () => { + // External record's fallback differs from the legacy token → not a + // prior-run Phase 1 → don't attempt Phase 2. The fallback alone + // makes the record readable, so the per-user slot is never probed. + const existing: UserRecord = { + account: { id: '99', email: 'me@x.io' }, + fallbackToken: 'external_plaintext', + hasRefreshToken: false, + } + const { km, result, marker } = await runMigration({ + slots: { + [LEGACY]: { secret: 'legacy_tok' }, + 'user-99': { getErr: new SecureStoreUnavailableError('no dbus') }, + }, + seedRecords: { '99': existing }, + options: { + identifyAccount: async () => ({ id: '99', email: 'me@x.io' }), + }, + }) + + expect(result.status).toBe('migrated') + expect(marker.migrated).toBe(true) + expect(km.slots.get(LEGACY)?.secret).toBeNull() + // Per-user slot must NEVER have been read — the fallback short-circuits. + expect(km.getCalls.has('user-99')).toBe(false) + }) + + it('retries Phase 2 when the existing record carries our legacy token (prior incomplete migration)', async () => { + // Scenario: a previous run wrote the Phase 1 fallback record but + // Phase 2 failed (transient backend error). Marker stayed unset. + // On retry, the existence check finds the record, recognises the + // fallback token as our own legacy token, and finishes Phase 2. + const existing: UserRecord = { + account: { id: '99', email: 'me@x.io' }, + fallbackToken: 'legacy_tok', + hasRefreshToken: false, + } + const { km, state, result, marker, harness } = await runMigration({ + slots: { [LEGACY]: { secret: 'legacy_tok' } }, + seedRecords: { '99': existing }, + options: { + identifyAccount: async () => ({ id: '99', email: 'me@x.io' }), + }, + }) + + expect(result.status).toBe('migrated') + expect(km.slots.get('user-99')?.secret).toBe('legacy_tok') + expect(state.records.get('99')?.fallbackToken).toBeUndefined() + expect(marker.migrated).toBe(true) + // Phase 2 ran exactly once: setSecret + clean upsert. + expect(harness.upsertSpy).toHaveBeenCalledTimes(1) + }) + + it('Phase 2 SecureStoreUnavailable falls back silently; Phase 1 record survives', async () => { + // Headless / WSL: the per-user slot can't accept a write. Phase 1's + // fallback record is self-sufficient, so the migration completes + // with the token in plaintext (until a future login upgrades it). + const { km, state, result, marker } = await runMigration({ + slots: { + [LEGACY]: { secret: 'legacy_tok' }, + 'user-99': { setErr: new SecureStoreUnavailableError('no dbus') }, + }, + options: { + identifyAccount: async () => ({ id: '99', email: 'me@x.io' }), + }, + }) + + expect(result.status).toBe('migrated') + expect(state.records.get('99')?.fallbackToken).toBe('legacy_tok') + expect(km.slots.get('user-99')?.secret).toBeNull() + expect(marker.migrated).toBe(true) + expect(km.slots.get(LEGACY)?.secret).toBeNull() + }) + + it('Phase 2 non-keyring setSecret failure surfaces as skipped (marker stays unset)', async () => { + // A transient backend error must not be silently swallowed — the + // operator needs visibility, and the marker stays unset so a future + // retry can re-attempt Phase 2. + const { result, marker, km } = await runMigration({ + slots: { + [LEGACY]: { secret: 'legacy_tok' }, + 'user-99': { setErr: new Error('keyring backend exploded') }, + }, + options: { + identifyAccount: async () => ({ id: '99', email: 'me@x.io' }), + }, + }) + + expect(result).toMatchObject({ + status: 'skipped', + reason: 'user-record-write-failed', + }) + expect(marker.migrated).toBe(false) + expect(km.slots.get(LEGACY)?.secret).toBe('legacy_tok') + }) + it('prefers the legacy keyring token over the plaintext fallback when both are populated', async () => { // Locks the keyring-first precedence — a refactor that flipped the // order would silently surface a stale plaintext token even when a diff --git a/src/auth/keyring/migrate.ts b/src/auth/keyring/migrate.ts index 9b407ba..488c2fa 100644 --- a/src/auth/keyring/migrate.ts +++ b/src/auth/keyring/migrate.ts @@ -1,13 +1,14 @@ import { getErrorMessage } from '../../errors.js' import type { AuthAccount } from '../types.js' -import { writeRecordWithKeyringFallback } from './record-write.js' +import { readAccessTokenForRecord } from './internal.js' +import { buildSingleTokenRecord } from './record-write.js' import { createSecureStore, DEFAULT_ACCOUNT_FOR_USER, type SecureStore, SecureStoreUnavailableError, } from './secure-store.js' -import type { UserRecordStore } from './types.js' +import type { UserRecord, UserRecordStore } from './types.js' export type MigrateLegacyAuthOptions = { serviceName: string @@ -59,18 +60,21 @@ export type MigrateLegacyAuthOptions = { } /** - * Stable skip reasons. `legacy-keyring-unreachable` is retryable (a later - * run with the keyring online would succeed); the others are diagnostic. + * Stable skip reasons. `*-keyring-unreachable` variants are retryable (a + * later run with the keyring online would succeed); the others are + * diagnostic. */ export type MigrateSkipReason = | 'identify-failed' | 'legacy-keyring-unreachable' + | 'user-keyring-unreachable' | 'user-record-write-failed' | 'marker-write-failed' const SKIP_REASON_MESSAGES: Record = { 'identify-failed': 'could not identify user', 'legacy-keyring-unreachable': 'legacy credential is unreachable (keyring offline)', + 'user-keyring-unreachable': 'per-user credential slot is unreachable (keyring offline)', 'user-record-write-failed': 'failed to update user records', 'marker-write-failed': 'failed to persist migration marker', } @@ -93,22 +97,23 @@ type LegacyTokenResult = /** * 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, …) leaves the v1 state untouched so the consumer's runtime - * fallback can keep serving the legacy token until the next attempt. + * shape. Best-effort: any failure leaves v1 untouched so the runtime + * fallback keeps serving the legacy token until the next attempt. * - * Order of operations is deliberate so the migration is genuinely one-way: + * Order is deliberate so the migration is one-way AND safe under retry: * - * 1. `hasMigrated()` short-circuits if the durable marker is already set. - * 2. Read the v1 token (legacy keyring slot first, then plaintext). - * 3. `identifyAccount(token)` resolves the v2 `account` shape. - * 4. `writeRecordWithKeyringFallback` writes the v2 record. - * 5. Best-effort `setDefaultId(account.id)` so the new record is active. - * 6. `markMigrated()` persists the marker. **If this fails, we return - * `skipped` even though the v2 record is on disk** — the marker is - * what prevents re-migration on the next run. - * 7. Best-effort legacy keyring delete + `cleanupLegacyConfig()` run - * concurrently. Failures here are harmless because the marker is set. + * 1. `hasMigrated()` short-circuits when the marker is set. + * 2. Read the v1 token (legacy keyring first, then plaintext). + * 3. `identifyAccount(token)` resolves the v2 `account`. + * 4. **Phase 1** — `ensureV2Record` writes a fallback-bearing record (or + * no-ops when a v2 record already exists). + * 5. **Phase 2** — when Phase 1 wrote: move the token to the per-user + * keyring slot and upsert the clean record. When Phase 1 didn't: + * verify the existing record is readable before retiring legacy. + * 6. Best-effort `setDefaultId(account.id)`. + * 7. `markMigrated()` — the one-way gate. Failure here surfaces as + * `skipped(marker-write-failed)` so the caller retries. + * 8. Best-effort legacy cleanup runs concurrently. */ export async function migrateLegacyAuth( options: MigrateLegacyAuthOptions, @@ -131,8 +136,6 @@ export async function migrateLegacyAuth( return { status: 'already-migrated' } } - // One legacy-keyring handle covers both the initial read and the - // post-success cleanup delete. const legacyStore = createSecureStore({ serviceName, account: legacyAccount }) const legacyToken = await readLegacyToken(legacyStore, loadLegacyPlaintextToken) @@ -153,28 +156,44 @@ export async function migrateLegacyAuth( return skipped(silent, logPrefix, 'identify-failed', getErrorMessage(error)) } - // `writeRecordWithKeyringFallback` swallows `SecureStoreUnavailableError` - // internally (writing to `fallbackToken` instead), so any error here is - // a non-keyring failure — typically a `userRecords.upsert` rejection. + let phase1: Phase1Result try { - await writeRecordWithKeyringFallback({ - secureStore: createSecureStore({ - serviceName, - account: accountForUser(account.id), - }), - userRecords, - account, - token: legacyToken.token, - }) + phase1 = await ensureV2Record(userRecords, account, legacyToken.token) } catch (error) { return skipped(silent, logPrefix, 'user-record-write-failed', getErrorMessage(error)) } - // Default promotion is best-effort and **only fires when nothing is - // already pinned**. A retry after a previous `markMigrated()` failure - // can land on a store where the user has since logged in to a different - // account and picked it as default — blindly setting the legacy account - // back as default would silently switch the user. + const userSlot = createSecureStore({ + serviceName, + account: accountForUser(account.id), + }) + + // Run Phase 2 when EITHER Phase 1 just wrote the fallback record OR + // the existing record's fallback matches our legacy token — that's a + // prior-run Phase 1 we owe an upgrade. Other existing records are + // external state and get a readability check instead. + const isOurPriorPhase1 = + !phase1.written && phase1.existing.fallbackToken?.trim() === legacyToken.token + if (phase1.written || isOurPriorPhase1) { + const phase2Error = await runPhase2(userRecords, userSlot, account, legacyToken.token) + if (phase2Error) { + return skipped(silent, logPrefix, phase2Error.reason, phase2Error.detail) + } + } else { + // External record — cleaning up legacy is safe only if it can be + // read in the current environment. + const outcome = await readAccessTokenForRecord(phase1.existing, userSlot) + if (!outcome.ok) { + const reason: MigrateSkipReason = + outcome.reason === 'slot-unavailable' + ? 'user-keyring-unreachable' + : 'user-record-write-failed' + return skipped(silent, logPrefix, reason, outcome.detail) + } + } + + // Only promote when nothing is pinned — a retry must not overwrite a + // default the user chose between attempts. try { const existingDefault = await userRecords.getDefaultId() if (!existingDefault) { @@ -184,39 +203,98 @@ export async function migrateLegacyAuth( // best-effort } - // Persist the one-way marker BEFORE legacy cleanup. If this fails, the - // v2 record is already written but the gate is unset — surface as - // `skipped` so the caller retries. Without this ordering, a later - // `logout` could let the next run re-migrate the stale v1 token. + // Marker BEFORE cleanup: the gate, not cleanup, is what prevents the + // next run from re-migrating after a later `logout`. try { await markMigrated() } catch (error) { return skipped(silent, logPrefix, 'marker-write-failed', getErrorMessage(error)) } - // Best-effort legacy cleanup — runs concurrently so we don't pay - // keyring latency followed by config-write latency on every install. - // The marker is already set, so a failure here can't cause - // re-migration on the next run. The `Promise.resolve().then(...)` - // wrappers convert any *synchronous* throw from a consumer-supplied - // `cleanupLegacyConfig` (or an oddly-implemented `SecureStore`) into - // a rejected promise that `Promise.allSettled` can swallow. + // `Promise.resolve().then(...)` converts any *synchronous* throw from + // a consumer's `cleanupLegacyConfig` into a rejection that + // `allSettled` can swallow. await Promise.allSettled([ Promise.resolve().then(() => legacyStore.deleteSecret()), Promise.resolve().then(() => cleanupLegacyConfig?.()), ]) if (!silent) { - // No identifier in the log line — `account.id` is typed as `string` - // but consumers can legitimately use an email or other PII there. - // Callers that need richer telemetry can compose it from the - // returned `account`. + // Account id may carry PII (email, etc.) — keep it out of logs. console.error(`${logPrefix}: migrated existing token to multi-user store.`) } return { status: 'migrated', account } } +type Phase1Result = + | { written: true } + | { written: false; existing: UserRecord } + +/** + * Phase 1. Writes a `fallbackToken`-bearing record so a crash before + * Phase 2 still leaves a working credential. Returns `{ written: true }` + * when this call wrote, or `{ written: false, existing }` when a v2 + * record already existed — the existing record is returned so callers + * decide whether to upgrade it (Phase 2 retry) or treat it as external + * state, without paying a second `list()`. + */ +async function ensureV2Record( + userRecords: UserRecordStore, + account: TAccount, + legacyToken: string, +): Promise> { + const record = buildSingleTokenRecord(account, legacyToken) + if (userRecords.tryInsert) { + const wrote = await userRecords.tryInsert(record) + if (wrote) return { written: true } + const existing = (await userRecords.list()).find((r) => r.account.id === account.id) + if (!existing) { + throw new Error('tryInsert returned false but no matching record was listed') + } + return { written: false, existing } + } + // Non-atomic path. Narrow time-of-check, time-of-use race between + // `list()` and `upsert()`; acceptable for one-time migration since + // concurrent runs would write the same shape. + const all = await userRecords.list() + const existing = all.find((r) => r.account.id === account.id) + if (existing) return { written: false, existing } + await userRecords.upsert(record) + return { written: true } +} + +/** + * Phase 2: move the legacy token into the per-user keyring slot and + * upsert a clean (no `fallbackToken`) record. Inlined rather than + * delegating to `writeRecordWithKeyringFallback` so the offline-keyring + * branch doesn't double-upsert the same fallback record Phase 1 just + * wrote. Returns `null` on success (including the silently-handled + * SecureStoreUnavailable case); a skip descriptor when a non-keyring + * failure leaves the marker unset for retry. + */ +async function runPhase2( + userRecords: UserRecordStore, + userSlot: SecureStore, + account: TAccount, + legacyToken: string, +): Promise<{ reason: MigrateSkipReason; detail: string } | null> { + try { + await userSlot.setSecret(legacyToken) + } catch (error) { + if (error instanceof SecureStoreUnavailableError) { + return null // Phase 1 fallback record continues to serve reads. + } + return { reason: 'user-record-write-failed', detail: getErrorMessage(error) } + } + try { + await userRecords.upsert(buildSingleTokenRecord(account)) + } catch (error) { + return { reason: 'user-record-write-failed', detail: getErrorMessage(error) } + } + return null +} + async function readLegacyToken( legacyStore: SecureStore, loadLegacyPlaintextToken: () => Promise, diff --git a/src/auth/keyring/record-write.ts b/src/auth/keyring/record-write.ts index 40039ca..4c82d39 100644 --- a/src/auth/keyring/record-write.ts +++ b/src/auth/keyring/record-write.ts @@ -179,6 +179,28 @@ export async function writeBundleWithKeyringFallback( + account: TAccount, + fallbackToken?: string, +): UserRecord { + return { + account, + ...(fallbackToken ? { fallbackToken } : {}), + hasRefreshToken: false, + } +} + const NOOP_SECURE_STORE: SecureStore = { async getSecret() { return null diff --git a/src/auth/keyring/token-store.ts b/src/auth/keyring/token-store.ts index a2cbd47..8fb61bd 100644 --- a/src/auth/keyring/token-store.ts +++ b/src/auth/keyring/token-store.ts @@ -1,12 +1,12 @@ -import { CliError, getErrorMessage } from '../../errors.js' +import { CliError } from '../../errors.js' import type { AccountRef, AuthAccount, TokenBundle, TokenStore } from '../types.js' import { accountNotFoundError } from '../user-flag.js' +import { readAccessTokenForRecord } from './internal.js' import { writeBundleWithKeyringFallback, writeRecordWithKeyringFallback } from './record-write.js' import { createSecureStore, DEFAULT_ACCOUNT_FOR_USER, SECURE_STORE_DESCRIPTION, - SecureStoreUnavailableError, type SecureStore, } from './secure-store.js' import { refreshAccountSlot } from './slot-naming.js' @@ -223,36 +223,16 @@ export function createKeyringTokenStore( // 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 } - - let raw: string | null - try { - raw = await secureStoreFor(record.account).getSecret() - } catch (error) { - if (error instanceof SecureStoreUnavailableError) { - throw new CliError( - 'AUTH_STORE_READ_FAILED', - `${SECURE_STORE_DESCRIPTION} unavailable; could not read stored token (${error.message})`, - ) - } - // 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 } - - // 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.`, - ) + const outcome = await readAccessTokenForRecord(record, secureStoreFor(record.account)) + if (outcome.ok) return { token: outcome.token, account: record.account } + // Map structured outcomes to the typed error contract. + const message = + outcome.reason === 'slot-empty' + ? `${SECURE_STORE_DESCRIPTION} returned no credential for the stored account; the keyring entry may have been removed externally.` + : outcome.reason === 'slot-unavailable' + ? `${SECURE_STORE_DESCRIPTION} unavailable; could not read stored token (${outcome.detail})` + : `Access-slot read failed (${outcome.detail})` + throw new CliError('AUTH_STORE_READ_FAILED', message) }, async set(account, token) { diff --git a/src/test-support/keyring-mocks.ts b/src/test-support/keyring-mocks.ts index 4d34a7d..c471daf 100644 --- a/src/test-support/keyring-mocks.ts +++ b/src/test-support/keyring-mocks.ts @@ -113,17 +113,27 @@ type UserRecordsHarness = { records: Map> defaultId: string | null } + listSpy: ReturnType upsertSpy: ReturnType removeSpy: ReturnType setDefaultSpy: ReturnType + /** Defined only when the harness was built with `withTryInsert: true`. */ + tryInsertSpy?: ReturnType } -/** In-memory `UserRecordStore` with spies on the mutating methods. */ -export function buildUserRecords(): UserRecordsHarness { +/** + * In-memory `UserRecordStore` with spies on the mutating methods. Opt into + * the atomic `tryInsert` path via `withTryInsert: true` — the default + * preserves the legacy list-then-upsert shape exercised by older tests. + */ +export function buildUserRecords( + options: { withTryInsert?: boolean } = {}, +): UserRecordsHarness { const state = { records: new Map>(), defaultId: null as string | null, } + const listSpy = vi.fn(async () => [...state.records.values()]) const upsertSpy = vi.fn(async (record: UserRecord) => { state.records.set(record.account.id, record) }) @@ -133,16 +143,22 @@ export function buildUserRecords(): UserRecordsHar const setDefaultSpy = vi.fn(async (id: string | null) => { state.defaultId = id }) + const tryInsertSpy = options.withTryInsert + ? vi.fn(async (record: UserRecord) => { + if (state.records.has(record.account.id)) return false + state.records.set(record.account.id, record) + return true + }) + : undefined const store: UserRecordStore = { - async list() { - return [...state.records.values()] - }, + list: listSpy, upsert: upsertSpy, remove: removeSpy, async getDefaultId() { return state.defaultId }, setDefaultId: setDefaultSpy, + ...(tryInsertSpy ? { tryInsert: tryInsertSpy } : {}), } - return { store, state, upsertSpy, removeSpy, setDefaultSpy } + return { store, state, listSpy, upsertSpy, removeSpy, setDefaultSpy, tryInsertSpy } }