From 349070ebb5c4a68971db531754171e62df9a3cad Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Tue, 19 May 2026 15:21:16 +0100 Subject: [PATCH 1/2] feat(auth): silent refresh helper + PKCE refreshToken via oauth4webapi MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR3 of the refresh-token feature split. Login persists the full `TokenBundle` (refresh token + expiry) and `refreshAccessToken` rotates the access token proactively (skew window) or reactively (`force: true` after a 401), serialised via an `O_EXCL` file lock so concurrent CLI invocations don't issue parallel refresh grants. The PKCE provider implements `refreshToken` via `oauth4webapi`, declared as an optional peer dep — only refresh-capable consumers install it. `invalid_grant` (any status) maps to `AUTH_REFRESH_EXPIRED`; other failures to `AUTH_REFRESH_TRANSIENT`; missing peer / no refresh token / no `activeBundle` to `AUTH_REFRESH_UNAVAILABLE`. `KeyringTokenStore.activeBundle` parallel-reads both slots, honouring the `hasRefreshToken: false` record gate so access-only records skip the refresh-slot IPC. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 46 ++-- package-lock.json | 15 ++ package.json | 5 + src/auth/errors.ts | 3 + src/auth/flow.test.ts | 83 ++++++- src/auth/flow.ts | 16 +- src/auth/index.ts | 5 +- src/auth/keyring/internal.test.ts | 72 ++++++ src/auth/keyring/internal.ts | 44 ++++ src/auth/keyring/token-store.test.ts | 54 +++++ src/auth/keyring/token-store.ts | 64 ++++- src/auth/persist.ts | 26 ++- src/auth/providers/pkce.test.ts | 106 ++++++++- src/auth/providers/pkce.ts | 97 ++++++++ src/auth/refresh.test.ts | 334 +++++++++++++++++++++++++++ src/auth/refresh.ts | 207 +++++++++++++++++ src/auth/status.ts | 31 ++- src/auth/types.ts | 18 ++ 18 files changed, 1194 insertions(+), 32 deletions(-) create mode 100644 src/auth/keyring/internal.test.ts create mode 100644 src/auth/refresh.test.ts create mode 100644 src/auth/refresh.ts diff --git a/README.md b/README.md index f8eee3f..2ce51f0 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`, `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). | +| Module | Key exports | Purpose | +| -------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `auth` (subpath) | `attachLoginCommand`, `attachLogoutCommand`, `attachStatusCommand`, `attachTokenViewCommand`, `runOAuthFlow`, `refreshAccessToken`, `createPkceProvider`, `createSecureStore`, `createKeyringTokenStore`, `migrateLegacyAuth`, `persistBundle`, `bundleFromExchange`, PKCE helpers, `AuthProvider` / `TokenStore` / `TokenBundle` / `ActiveBundleSnapshot` / `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 @@ -292,9 +292,23 @@ For multi-account storage (OS keychain, per-user config slots, …), implement t 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. +`active()` stays narrow — `{ token, account }` only — so commands that only need the access token don't pay extra keyring IPC. The companion read method `activeBundle?(ref)` returns the full bundle (`{ account, bundle }`) and is the read path the silent-refresh helper requires. Optional on the contract; `KeyringTokenStore` implements it as a required override and parallel-reads both keyring slots, honouring the `hasRefreshToken: false` record gate to skip the refresh-slot IPC on access-only records. -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. +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`). `runOAuthFlow` persists the full bundle through `persistBundle` with `promoteDefault: true`. + +##### Silent refresh (`refreshAccessToken`) + +`refreshAccessToken({ store, provider, lockPath, skewMs?, force?, ref? })` rotates the access token using the stored refresh token. Use **proactively** before each authenticated call (skew defaults to 60s) or **reactively** with `force: true` after a 401. Persists the rotated bundle via `persistBundle` _without_ `promoteDefault` so a background rotation can't re-pin account selection. + +`lockPath` is caller-provided (cli-core doesn't interpret `~` or know where your config lives) — `O_EXCL` on that path serialises concurrent CLI invocations so only one POSTs and the rest re-read the rotated bundle. Recommended path: `${getConfigPath(serviceName)}.refresh.lock`. + +Error contract: + +- `AUTH_REFRESH_EXPIRED` — server rejected the refresh token (`invalid_grant`, including 400 and 401 since some reverse proxies remap). Caller should prompt re-login. +- `AUTH_REFRESH_TRANSIENT` — 5xx, network, non-JSON body, lock timeout. Caller may retry. +- `AUTH_REFRESH_UNAVAILABLE` — refresh isn't possible in the current setup: no refresh token stored, store doesn't implement `activeBundle`, provider doesn't implement `refreshToken`, or the optional `oauth4webapi` peer dep isn't installed. + +The PKCE provider (`createPkceProvider`) implements `refreshToken` via the [`oauth4webapi`](https://github.com/panva/oauth4webapi) library, declared as an **optional peer dependency** — only CLIs that opt into refresh need to install it (`npm install oauth4webapi`). Providers built directly against the `AuthProvider` interface (e.g. DCR, device code) implement the `refreshToken?` hook themselves; the storage and helper contract is identical. #### Keyring primitive (`createSecureStore`) diff --git a/package-lock.json b/package-lock.json index 2a36f8a..d619bf8 100644 --- a/package-lock.json +++ b/package-lock.json @@ -22,6 +22,7 @@ "lefthook": "2.1.6", "marked": "18.0.3", "marked-terminal-renderer": "2.2.0", + "oauth4webapi": "3.8.6", "open": "11.0.0", "oxfmt": "0.49.0", "oxlint": "1.64.0", @@ -39,6 +40,7 @@ "commander": ">=14", "marked": ">=18", "marked-terminal-renderer": ">=2", + "oauth4webapi": ">=3", "open": ">=10", "vitest": ">=4.1" }, @@ -52,6 +54,9 @@ "marked-terminal-renderer": { "optional": true }, + "oauth4webapi": { + "optional": true + }, "open": { "optional": true }, @@ -7919,6 +7924,16 @@ "node": ">=18" } }, + "node_modules/oauth4webapi": { + "version": "3.8.6", + "resolved": "https://registry.npmjs.org/oauth4webapi/-/oauth4webapi-3.8.6.tgz", + "integrity": "sha512-iwemM91xz8nryHti2yTmg5fhyEMVOkOXwHNqbvcATjyajb5oQxCQzrNOA6uElRHuMhQQTKUyFKV9y/CNyg25BQ==", + "dev": true, + "license": "MIT", + "funding": { + "url": "https://github.com/sponsors/panva" + } + }, "node_modules/object-assign": { "version": "4.1.1", "resolved": "https://registry.npmjs.org/object-assign/-/object-assign-4.1.1.tgz", diff --git a/package.json b/package.json index 6a9c428..6aaec21 100644 --- a/package.json +++ b/package.json @@ -72,6 +72,7 @@ "lefthook": "2.1.6", "marked": "18.0.3", "marked-terminal-renderer": "2.2.0", + "oauth4webapi": "3.8.6", "open": "11.0.0", "oxfmt": "0.49.0", "oxlint": "1.64.0", @@ -90,6 +91,7 @@ "commander": ">=14", "marked": ">=18", "marked-terminal-renderer": ">=2", + "oauth4webapi": ">=3", "open": ">=10", "vitest": ">=4.1" }, @@ -103,6 +105,9 @@ "marked-terminal-renderer": { "optional": true }, + "oauth4webapi": { + "optional": true + }, "open": { "optional": true }, diff --git a/src/auth/errors.ts b/src/auth/errors.ts index bf77311..439e987 100644 --- a/src/auth/errors.ts +++ b/src/auth/errors.ts @@ -10,6 +10,9 @@ export type AuthErrorCode = | 'AUTH_TOKEN_EXCHANGE_FAILED' | 'AUTH_STORE_WRITE_FAILED' | 'AUTH_STORE_READ_FAILED' + | 'AUTH_REFRESH_EXPIRED' + | 'AUTH_REFRESH_TRANSIENT' + | 'AUTH_REFRESH_UNAVAILABLE' | 'NOT_AUTHENTICATED' | 'TOKEN_FROM_ENV' | 'NO_ACCOUNT_SELECTED' diff --git a/src/auth/flow.test.ts b/src/auth/flow.test.ts index a510304..1d37bf5 100644 --- a/src/auth/flow.test.ts +++ b/src/auth/flow.test.ts @@ -22,7 +22,7 @@ import { execFile } from 'node:child_process' import { readFileSync } from 'node:fs' import openBrowserModule from 'open' import { type RunOAuthFlowOptions, runOAuthFlow } from './flow.js' -import type { AuthProvider, TokenStore } from './types.js' +import type { AuthProvider, TokenBundle, TokenStore } from './types.js' type Account = { id: string; label?: string; email: string } @@ -360,6 +360,87 @@ describe('runOAuthFlow', () => { expect(result.token).toBe('tok-1') }) + it('persists the full bundle via setBundle when the store implements it (promoteDefault: true)', async () => { + const setBundle = vi.fn( + async ( + _account: Account, + _bundle: TokenBundle, + _options?: { promoteDefault?: boolean }, + ) => undefined, + ) + const set = vi.fn(async () => undefined) + const store: TokenStore = { + async active() { + return null + }, + set, + setBundle, + async clear() {}, + async list() { + return [] + }, + async setDefault() {}, + } + const { provider, getRedirect } = instrument({ + exchangeCode: async () => ({ + accessToken: 'tok-1', + refreshToken: 'r-1', + expiresAt: 1_700_000_000_000, + refreshTokenExpiresAt: 1_800_000_000_000, + }), + }) + + await runOAuthFlow( + flowOptions({ + provider, + store, + openBrowser: driveCallback(getRedirect), + onAuthorizeUrl: () => undefined, + }), + ) + + expect(set).not.toHaveBeenCalled() + expect(setBundle).toHaveBeenCalledTimes(1) + const [, persistedBundle, opts] = setBundle.mock.calls[0] + expect(persistedBundle).toEqual({ + accessToken: 'tok-1', + refreshToken: 'r-1', + accessTokenExpiresAt: 1_700_000_000_000, + refreshTokenExpiresAt: 1_800_000_000_000, + }) + expect(opts).toEqual({ promoteDefault: true }) + }) + + it('falls back to set(accessToken) when the store does not implement setBundle (refresh state dropped)', async () => { + const set = vi.fn(async () => undefined) + const store: TokenStore = { + async active() { + return null + }, + set, + async clear() {}, + async list() { + return [] + }, + async setDefault() {}, + } + const { provider, getRedirect } = instrument({ + exchangeCode: async () => ({ accessToken: 'tok-1', refreshToken: 'r-1' }), + }) + + const result = await runOAuthFlow( + flowOptions({ + provider, + store, + openBrowser: driveCallback(getRedirect), + onAuthorizeUrl: () => undefined, + }), + ) + + expect(result.token).toBe('tok-1') + expect(set).toHaveBeenCalledWith(expect.objectContaining({ id: '1' }), 'tok-1') + }) + it('wraps non-CliError store.set failures in AUTH_STORE_WRITE_FAILED', async () => { const { provider, getRedirect } = instrument() const store: TokenStore = { diff --git a/src/auth/flow.ts b/src/auth/flow.ts index d95611b..7a09237 100644 --- a/src/auth/flow.ts +++ b/src/auth/flow.ts @@ -4,6 +4,7 @@ import { type IncomingMessage, type Server, type ServerResponse, createServer } import { promisify } from 'node:util' import { CliError, getErrorMessage } from '../errors.js' import { isStdoutTTY } from '../terminal.js' +import { bundleFromExchange, persistBundle } from './persist.js' import { generateState } from './pkce.js' import type { AuthAccount, AuthProvider, TokenStore } from './types.js' @@ -187,15 +188,12 @@ export async function runOAuthFlow( })) checkAborted() - try { - await options.store.set(account, exchange.accessToken) - } catch (error) { - if (error instanceof CliError) throw error - throw new CliError( - 'AUTH_STORE_WRITE_FAILED', - `Failed to persist token: ${getErrorMessage(error)}`, - ) - } + await persistBundle({ + store: options.store, + account, + bundle: bundleFromExchange(exchange), + promoteDefault: true, + }) return { token: exchange.accessToken, account } } finally { diff --git a/src/auth/index.ts b/src/auth/index.ts index e0595f6..4616185 100644 --- a/src/auth/index.ts +++ b/src/auth/index.ts @@ -20,12 +20,15 @@ export { generateVerifier, } from './pkce.js' export type { GenerateVerifierOptions } from './pkce.js' -export { persistBundle } from './persist.js' +export { bundleFromExchange, 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 { refreshAccessToken } from './refresh.js' +export type { RefreshAccessTokenOptions, RefreshAccessTokenResult } from './refresh.js' export type { AccountRef, + ActiveBundleSnapshot, AuthAccount, AuthorizeInput, AuthorizeResult, diff --git a/src/auth/keyring/internal.test.ts b/src/auth/keyring/internal.test.ts new file mode 100644 index 0000000..3b36ac8 --- /dev/null +++ b/src/auth/keyring/internal.test.ts @@ -0,0 +1,72 @@ +import { describe, expect, it, vi } from 'vitest' + +import type { AuthAccount } from '../types.js' +import { readRefreshTokenForRecord } from './internal.js' +import { type SecureStore, SecureStoreUnavailableError } from './secure-store.js' +import type { UserRecord } from './types.js' + +type Account = AuthAccount & { id: string } +const account: Account = { id: '42' } + +function fakeStore(impl: Partial): SecureStore { + return { + async getSecret() { + return null + }, + async setSecret() {}, + async deleteSecret() { + return false + }, + ...impl, + } +} + +describe('readRefreshTokenForRecord', () => { + it('short-circuits to not-present when hasRefreshToken is false', async () => { + const getSpy = vi.fn(async () => 'should-not-be-read') + const store = fakeStore({ getSecret: getSpy }) + const record: UserRecord = { account, hasRefreshToken: false } + + const outcome = await readRefreshTokenForRecord(record, store) + expect(outcome).toEqual({ ok: false, reason: 'not-present' }) + expect(getSpy).not.toHaveBeenCalled() + }) + + it('returns fallbackRefreshToken in preference to a (possibly stale) keyring slot', async () => { + const getSpy = vi.fn(async () => 'stale') + const store = fakeStore({ getSecret: getSpy }) + const record: UserRecord = { + account, + hasRefreshToken: true, + fallbackRefreshToken: 'plaintext_fallback', + } + + const outcome = await readRefreshTokenForRecord(record, store) + expect(outcome).toEqual({ ok: true, token: 'plaintext_fallback' }) + expect(getSpy).not.toHaveBeenCalled() + }) + + it('reads the keyring slot when no fallback is present', async () => { + const store = fakeStore({ + async getSecret() { + return 'from_keyring' + }, + }) + const record: UserRecord = { account, hasRefreshToken: true } + + const outcome = await readRefreshTokenForRecord(record, store) + expect(outcome).toEqual({ ok: true, token: 'from_keyring' }) + }) + + it('maps SecureStoreUnavailableError to slot-unavailable', async () => { + const store = fakeStore({ + async getSecret() { + throw new SecureStoreUnavailableError('no dbus') + }, + }) + const record: UserRecord = { account, hasRefreshToken: true } + + const outcome = await readRefreshTokenForRecord(record, store) + expect(outcome).toMatchObject({ ok: false, reason: 'slot-unavailable' }) + }) +}) diff --git a/src/auth/keyring/internal.ts b/src/auth/keyring/internal.ts index 8145f75..0d76789 100644 --- a/src/auth/keyring/internal.ts +++ b/src/auth/keyring/internal.ts @@ -13,6 +13,18 @@ export type ReadAccessTokenOutcome = | { ok: true; token: string } | { ok: false; reason: 'slot-empty' | 'slot-unavailable' | 'slot-error'; detail: string } +/** + * Outcome of resolving the refresh token for a record. Mirrors + * `ReadAccessTokenOutcome`, plus an extra `not-present` variant for records + * the store knows carry no refresh state (`hasRefreshToken: false`) — the + * gate lets `activeBundle` skip the slot IPC entirely on access-only + * records. + */ +export type ReadRefreshTokenOutcome = + | { ok: true; token: string } + | { ok: false; reason: 'not-present' } + | { 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 @@ -42,3 +54,35 @@ export async function readAccessTokenForRecord( return { ok: false, reason: 'slot-error', detail: getErrorMessage(error) } } } + +/** + * Refresh-side analogue of `readAccessTokenForRecord`. Honours the + * `hasRefreshToken: false` gate — a record that knows it has no refresh + * material short-circuits to `not-present` without touching the keyring. + * Legacy records (`hasRefreshToken === undefined`) probe the slot once. + */ +export async function readRefreshTokenForRecord( + record: UserRecord, + refreshStore: SecureStore, +): Promise { + if (record.hasRefreshToken === false) return { ok: false, reason: 'not-present' } + + const fallback = record.fallbackRefreshToken?.trim() + if (fallback) return { ok: true, token: fallback } + + try { + const raw = await refreshStore.getSecret() + const trimmed = raw?.trim() + if (trimmed) return { ok: true, token: trimmed } + return { + ok: false, + reason: 'slot-empty', + detail: 'keyring refresh 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/token-store.test.ts b/src/auth/keyring/token-store.test.ts index 6bdb556..4406602 100644 --- a/src/auth/keyring/token-store.test.ts +++ b/src/auth/keyring/token-store.test.ts @@ -506,6 +506,60 @@ describe('createKeyringTokenStore', () => { expect(state.records.get('42')?.hasRefreshToken).toBe(false) }) + it('activeBundle round-trips the persisted bundle (account + access + refresh + expiry)', async () => { + const { km, store } = mapFixture() + await store.setBundle(account, bundle, { promoteDefault: true }) + + const snapshot = await store.activeBundle() + expect(snapshot).toEqual({ + account, + bundle: { + accessToken: 'tok_a', + refreshToken: 'tok_r', + accessTokenExpiresAt: 1_700_000_000_000, + }, + }) + // Sanity: both slots read, refresh slot was actually populated. + expect(km.slots.get(refreshAccountSlot('user-42'))?.secret).toBe('tok_r') + }) + + it('activeBundle skips the refresh-slot IPC when hasRefreshToken is false', async () => { + const { km, store } = mapFixture({ + '42': { account, hasRefreshToken: false }, + }) + km.slots.set('user-42', { secret: 'tok_a' }) + + const snapshot = await store.activeBundle() + expect(snapshot?.bundle).toEqual({ accessToken: 'tok_a' }) + // Refresh slot must not have been read (no IPC). + expect(km.getCalls.get(refreshAccountSlot('user-42'))).toBeUndefined() + }) + + it('activeBundle returns a refresh-less bundle when the refresh slot is unavailable', async () => { + // Legacy record (no hasRefreshToken gate) where the refresh slot + // is offline (SecureStoreUnavailableError). The bundle returns + // without refreshToken; the silent-refresh helper translates + // that to AUTH_REFRESH_UNAVAILABLE. + const km = buildKeyringMap() + mockedCreateSecureStore.mockImplementation(km.create) + const harness = buildUserRecords() + harness.state.records.set('42', { account }) + km.slots.set('user-42', { secret: 'tok_a' }) + km.slots.set(refreshAccountSlot('user-42'), { + secret: null, + getErr: new SecureStoreUnavailableError('locked'), + }) + + const store = createKeyringTokenStore({ + serviceName: SERVICE, + userRecords: harness.store, + recordsLocation: LOCATION, + }) + + const snapshot = await store.activeBundle() + expect(snapshot?.bundle).toEqual({ accessToken: 'tok_a' }) + }) + it('clear() wipes both keyring slots', async () => { const { km, store, state } = mapFixture({ '42': { account, hasRefreshToken: true }, diff --git a/src/auth/keyring/token-store.ts b/src/auth/keyring/token-store.ts index 8fb61bd..3b89df0 100644 --- a/src/auth/keyring/token-store.ts +++ b/src/auth/keyring/token-store.ts @@ -1,7 +1,13 @@ import { CliError } from '../../errors.js' -import type { AccountRef, AuthAccount, TokenBundle, TokenStore } from '../types.js' +import type { + AccountRef, + ActiveBundleSnapshot, + AuthAccount, + TokenBundle, + TokenStore, +} from '../types.js' import { accountNotFoundError } from '../user-flag.js' -import { readAccessTokenForRecord } from './internal.js' +import { readAccessTokenForRecord, readRefreshTokenForRecord } from './internal.js' import { writeBundleWithKeyringFallback, writeRecordWithKeyringFallback } from './record-write.js' import { createSecureStore, @@ -47,6 +53,12 @@ export type KeyringTokenStore = TokenStore + /** + * Override `activeBundle` as required (not optional) — the keyring store + * always knows how to read refresh state. Lets cli-core helpers + * (`refreshAccessToken`) call it without a non-null assertion. + */ + activeBundle(ref?: AccountRef): Promise | null> /** 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). */ @@ -235,6 +247,54 @@ export function createKeyringTokenStore( throw new CliError('AUTH_STORE_READ_FAILED', message) }, + async activeBundle(ref) { + const snapshot: Snapshot = + ref === undefined + ? await readFullSnapshot() + : { records: await userRecords.list(), defaultId: null } + const record = resolveTarget(snapshot, ref) + if (!record) return null + + // Read both slots in parallel. The refresh side honours the + // `hasRefreshToken: false` gate inside `readRefreshTokenForRecord`, + // so an access-only record short-circuits without a refresh-slot + // IPC. Access-slot failures map to the same typed error as + // `active()`; a refresh-slot failure degrades silently — the + // bundle returns without `refreshToken` and the silent-refresh + // helper translates that to `AUTH_REFRESH_UNAVAILABLE`. + const [accessOutcome, refreshOutcome] = await Promise.all([ + readAccessTokenForRecord(record, secureStoreFor(record.account)), + readRefreshTokenForRecord(record, refreshSecureStoreFor(record.account)), + ]) + if (!accessOutcome.ok) { + const message = + accessOutcome.reason === 'slot-empty' + ? `${SECURE_STORE_DESCRIPTION} returned no credential for the stored account; the keyring entry may have been removed externally.` + : accessOutcome.reason === 'slot-unavailable' + ? `${SECURE_STORE_DESCRIPTION} unavailable; could not read stored token (${accessOutcome.detail})` + : `Access-slot read failed (${accessOutcome.detail})` + throw new CliError('AUTH_STORE_READ_FAILED', message) + } + if (refreshOutcome.ok === false && refreshOutcome.reason === 'slot-error') { + throw new CliError( + 'AUTH_STORE_READ_FAILED', + `Refresh-slot read failed (${refreshOutcome.detail})`, + ) + } + + const bundle: TokenBundle = { + accessToken: accessOutcome.token, + ...(refreshOutcome.ok ? { refreshToken: refreshOutcome.token } : {}), + ...(record.accessTokenExpiresAt !== undefined + ? { accessTokenExpiresAt: record.accessTokenExpiresAt } + : {}), + ...(record.refreshTokenExpiresAt !== undefined + ? { refreshTokenExpiresAt: record.refreshTokenExpiresAt } + : {}), + } + return { account: record.account, bundle } + }, + async set(account, token) { // Reset the cached storage result up front so a caller that // catches a thrown `set()` doesn't observe the previous call's diff --git a/src/auth/persist.ts b/src/auth/persist.ts index df5f389..aa3aac5 100644 --- a/src/auth/persist.ts +++ b/src/auth/persist.ts @@ -1,5 +1,5 @@ import { CliError, getErrorMessage } from '../errors.js' -import type { AuthAccount, TokenBundle, TokenStore } from './types.js' +import type { AuthAccount, ExchangeResult, TokenBundle, TokenStore } from './types.js' export type PersistBundleOptions = { store: TokenStore @@ -22,6 +22,30 @@ export type PersistBundleOptions = { * silent-refresh-safe selection (no re-pinning on background rotation) * MUST implement `setBundle`. */ +/** + * Build a `TokenBundle` from an `ExchangeResult`. `prev` carries forward + * any refresh-side state the server omitted from the response — most + * servers don't reissue the refresh token on a refresh-grant, and few + * include `refresh_token_expires_in`, so the previous values stay + * authoritative until the server sends an explicit replacement. + */ +export function bundleFromExchange( + exchange: ExchangeResult, + prev?: TokenBundle, +): TokenBundle { + const refreshToken = exchange.refreshToken ?? prev?.refreshToken + return { + accessToken: exchange.accessToken, + ...(refreshToken !== undefined ? { refreshToken } : {}), + ...(exchange.expiresAt !== undefined ? { accessTokenExpiresAt: exchange.expiresAt } : {}), + ...(exchange.refreshTokenExpiresAt !== undefined + ? { refreshTokenExpiresAt: exchange.refreshTokenExpiresAt } + : prev?.refreshTokenExpiresAt !== undefined + ? { refreshTokenExpiresAt: prev.refreshTokenExpiresAt } + : {}), + } +} + export async function persistBundle( options: PersistBundleOptions, ): Promise { diff --git a/src/auth/providers/pkce.test.ts b/src/auth/providers/pkce.test.ts index 6bfebbf..e4ba826 100644 --- a/src/auth/providers/pkce.test.ts +++ b/src/auth/providers/pkce.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' import { createPkceProvider } from './pkce.js' @@ -145,3 +145,107 @@ describe('createPkceProvider', () => { ).rejects.toMatchObject({ code: 'AUTH_TOKEN_EXCHANGE_FAILED' }) }) }) + +// `oauth4webapi` is lazy-imported by provider.refreshToken and uses the +// global `fetch`. Each refresh test stubs `globalThis.fetch` to drive the +// request; real `oauth4webapi` enforces https for the token endpoint, so +// all URLs below use https. +describe('createPkceProvider.refreshToken', () => { + const tokenUrl = 'https://example.com/oauth/token' + + function refreshProvider() { + return createPkceProvider({ + authorizeUrl: 'https://example.com/oauth/authorize', + tokenUrl, + clientId: 'client-xyz', + validate, + }) + } + + function stubFetch(impl: typeof fetch): ReturnType { + return vi.spyOn(globalThis, 'fetch').mockImplementation(impl) + } + + afterEach(() => { + vi.restoreAllMocks() + }) + + it('POSTs the refresh grant and returns the rotated bundle (no client_secret)', async () => { + let captured: { url: string; body: string } | undefined + stubFetch((async (input: RequestInfo | URL, init: RequestInit = {}) => { + captured = { url: String(input), body: String(init.body ?? '') } + return new Response( + JSON.stringify({ + access_token: 'tok-new', + refresh_token: 'r-new', + expires_in: 3600, + token_type: 'bearer', + }), + { status: 200, headers: { 'Content-Type': 'application/json' } }, + ) + }) as typeof fetch) + + const result = await refreshProvider().refreshToken!({ + refreshToken: 'r-old', + handshake: {}, + }) + + expect(result.accessToken).toBe('tok-new') + expect(result.refreshToken).toBe('r-new') + expect(result.expiresAt).toBeGreaterThan(Date.now()) + expect(captured?.url).toBe(tokenUrl) + const body = new URLSearchParams(captured!.body) + expect(body.get('grant_type')).toBe('refresh_token') + expect(body.get('refresh_token')).toBe('r-old') + expect(body.get('client_id')).toBe('client-xyz') + expect(body.has('client_secret')).toBe(false) + }) + + it.each([ + ['400', 400], + ['401 (reverse-proxy remap)', 401], + ])('maps invalid_grant %s to AUTH_REFRESH_EXPIRED', async (_label, status) => { + stubFetch( + (async () => + new Response(JSON.stringify({ error: 'invalid_grant' }), { + status, + headers: { 'Content-Type': 'application/json' }, + })) as typeof fetch, + ) + + await expect( + refreshProvider().refreshToken!({ refreshToken: 'r-old', handshake: {} }), + ).rejects.toMatchObject({ code: 'AUTH_REFRESH_EXPIRED' }) + }) + + it.each([ + [ + '500', + async () => + new Response(JSON.stringify({ error: 'server_error' }), { + status: 500, + headers: { 'Content-Type': 'application/json' }, + }), + ], + [ + 'non-JSON 2xx', + async () => + new Response('oops', { + status: 200, + headers: { 'Content-Type': 'text/html' }, + }), + ], + [ + 'network failure', + async () => { + throw new Error('connection reset') + }, + ], + ])('maps %s to AUTH_REFRESH_TRANSIENT', async (_label, impl) => { + stubFetch(impl as typeof fetch) + + await expect( + refreshProvider().refreshToken!({ refreshToken: 'r-old', handshake: {} }), + ).rejects.toMatchObject({ code: 'AUTH_REFRESH_TRANSIENT' }) + }) +}) diff --git a/src/auth/providers/pkce.ts b/src/auth/providers/pkce.ts index 2e641a7..6838ad7 100644 --- a/src/auth/providers/pkce.ts +++ b/src/auth/providers/pkce.ts @@ -1,3 +1,4 @@ +import type { AuthorizationServer, Client, TokenEndpointResponse } from 'oauth4webapi' import { CliError, getErrorMessage } from '../../errors.js' import { deriveChallenge, generateVerifier } from '../pkce.js' import type { @@ -7,6 +8,7 @@ import type { AuthorizeResult, ExchangeInput, ExchangeResult, + RefreshInput, ValidateInput, } from '../types.js' @@ -160,6 +162,37 @@ export function createPkceProvider( }, validateToken: options.validate, + + async refreshToken(input: RefreshInput): Promise> { + const oauth = await loadOauth4webapi() + // RefreshInput.handshake is empty by default; the helper has no + // flags context during silent rotation, so resolvers that need + // per-flow flags should encode the relevant state in the + // handshake the caller supplies (or be constant). + const flags = (input.handshake.flags as Record | undefined) ?? {} + const tokenUrlResolved = resolve(options.tokenUrl, input.handshake, flags) + const clientIdResolved = resolve(options.clientId, input.handshake, flags) + const as: AuthorizationServer = { + issuer: tokenUrlResolved, + token_endpoint: tokenUrlResolved, + } + const client: Client = { + client_id: clientIdResolved, + token_endpoint_auth_method: 'none', + } + try { + const response = await oauth.refreshTokenGrantRequest( + as, + client, + oauth.None(), + input.refreshToken, + ) + const result = await oauth.processRefreshTokenResponse(as, client, response) + return mapRefreshResponse(result) + } catch (error) { + throw translateRefreshError(oauth, error) + } + }, } } @@ -171,6 +204,70 @@ function resolve( return typeof resolver === 'function' ? resolver({ handshake, flags }) : resolver } +/** + * Lazy-import `oauth4webapi`. It's an optional peer dep — only refresh + * consumers install it. Missing module → `AUTH_REFRESH_UNAVAILABLE` with an + * actionable hint; any other import failure rethrows as `CliError` of the + * same code (the user can't recover beyond "install the dep" either way). + */ +type Oauth4WebApi = typeof import('oauth4webapi') + +async function loadOauth4webapi(): Promise { + try { + return await import('oauth4webapi') + } catch (error) { + const code = (error as NodeJS.ErrnoException | undefined)?.code + if (code === 'ERR_MODULE_NOT_FOUND' || code === 'MODULE_NOT_FOUND') { + throw new CliError( + 'AUTH_REFRESH_UNAVAILABLE', + 'oauth4webapi is required for refresh-token support.', + { hints: ['Run `npm install oauth4webapi` in your CLI.'] }, + ) + } + throw new CliError( + 'AUTH_REFRESH_UNAVAILABLE', + `Failed to load oauth4webapi: ${getErrorMessage(error)}`, + ) + } +} + +function mapRefreshResponse( + response: TokenEndpointResponse, +): ExchangeResult { + return { + accessToken: response.access_token, + refreshToken: response.refresh_token, + expiresAt: + typeof response.expires_in === 'number' + ? Date.now() + response.expires_in * 1000 + : undefined, + } +} + +/** + * Translate `oauth4webapi` failures to the typed refresh contract. + * + * - `ResponseBodyError` with `error === 'invalid_grant'` → `AUTH_REFRESH_EXPIRED` + * regardless of status (some reverse proxies remap 400 → 401). + * - Everything else (other `ResponseBodyError` codes, network failures, 5xx, + * non-JSON bodies, `WWWAuthenticateChallengeError`) → `AUTH_REFRESH_TRANSIENT`. + */ +function translateRefreshError(oauth: Oauth4WebApi, error: unknown): CliError { + if (error instanceof CliError) return error + if (error instanceof oauth.ResponseBodyError && error.error === 'invalid_grant') { + return new CliError( + 'AUTH_REFRESH_EXPIRED', + `Refresh token rejected: ${error.error_description ?? error.error}`, + { hints: ['Re-run the login command to reauthorize.'] }, + ) + } + return new CliError( + 'AUTH_REFRESH_TRANSIENT', + `Refresh request failed: ${getErrorMessage(error)}`, + { hints: ['Try again.'] }, + ) +} + async function safeReadText(response: Response): Promise { try { const text = (await response.text()).trim() diff --git a/src/auth/refresh.test.ts b/src/auth/refresh.test.ts new file mode 100644 index 0000000..c9f25f0 --- /dev/null +++ b/src/auth/refresh.test.ts @@ -0,0 +1,334 @@ +import { mkdtemp, rm, writeFile } from 'node:fs/promises' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { refreshAccessToken } from './refresh.js' +import type { + ActiveBundleSnapshot, + AuthProvider, + ExchangeResult, + TokenBundle, + TokenStore, +} from './types.js' + +type Account = { id: string; label?: string; email: string } + +const account: Account = { id: '42', email: 'a@b' } + +function bundle(overrides: Partial = {}): TokenBundle { + return { + accessToken: 'tok_a', + refreshToken: 'r_a', + accessTokenExpiresAt: Date.now() + 10_000, + ...overrides, + } +} + +type StoreState = { + snapshot: ActiveBundleSnapshot | null + activeBundleSpy: ReturnType + setBundleSpy: ReturnType + setBundleCalls: { account: Account; bundle: TokenBundle; options?: unknown }[] +} + +function fakeStore( + initial: ActiveBundleSnapshot | null, + overrides: Partial> = {}, +): { store: TokenStore; state: StoreState } { + const setBundleCalls: StoreState['setBundleCalls'] = [] + const state: StoreState = { + snapshot: initial, + activeBundleSpy: vi.fn(), + setBundleSpy: vi.fn(), + setBundleCalls, + } + state.activeBundleSpy.mockImplementation(async () => state.snapshot) + state.setBundleSpy.mockImplementation( + async (acc: Account, b: TokenBundle, options?: unknown) => { + setBundleCalls.push({ account: acc, bundle: b, options }) + state.snapshot = { account: acc, bundle: b } + }, + ) + const store: TokenStore = { + async active() { + return state.snapshot + ? { token: state.snapshot.bundle.accessToken, account: state.snapshot.account } + : null + }, + activeBundle: state.activeBundleSpy as unknown as TokenStore['activeBundle'], + async set() {}, + setBundle: state.setBundleSpy as unknown as TokenStore['setBundle'], + async clear() {}, + async list() { + return [] + }, + async setDefault() {}, + ...overrides, + } + return { store, state } +} + +function fakeProvider( + refreshImpl?: (input: { refreshToken: string }) => Promise>, +): { provider: AuthProvider; refreshSpy: ReturnType } { + const refreshSpy = vi.fn( + refreshImpl ?? + (async () => ({ + accessToken: 'tok_new', + refreshToken: 'r_new', + expiresAt: Date.now() + 60_000, + })), + ) + const provider: AuthProvider = { + async authorize() { + return { authorizeUrl: '', handshake: {} } + }, + async exchangeCode() { + return { accessToken: '' } + }, + async validateToken() { + return account + }, + refreshToken: refreshSpy as unknown as AuthProvider['refreshToken'], + } + return { provider, refreshSpy } +} + +describe('refreshAccessToken', () => { + let lockDir: string + let lockPath: string + + beforeEach(async () => { + lockDir = await mkdtemp(join(tmpdir(), 'cli-core-refresh-')) + lockPath = join(lockDir, 'refresh.lock') + }) + + afterEach(async () => { + await rm(lockDir, { recursive: true, force: true }) + }) + + it('rotates the bundle when access expiry is inside the skew window', async () => { + const { store, state } = fakeStore({ + account, + bundle: bundle({ accessTokenExpiresAt: Date.now() + 1_000 }), + }) + const { provider, refreshSpy } = fakeProvider() + + const result = await refreshAccessToken({ + store, + provider, + skewMs: 5_000, + lockPath, + }) + + expect(result.rotated).toBe(true) + expect(result.bundle.accessToken).toBe('tok_new') + expect(refreshSpy).toHaveBeenCalledWith({ refreshToken: 'r_a', handshake: {} }) + // setBundle was called without promoteDefault — silent rotation must + // not re-pin account selection. The helper omits the options arg + // entirely so presence-based handlers see the same shape they would + // for "no caller preference". + expect(state.setBundleCalls).toHaveLength(1) + expect(state.setBundleCalls[0].options).toBeUndefined() + }) + + it('returns rotated:false without POSTing when access expiry is outside the skew window', async () => { + const { store } = fakeStore({ + account, + bundle: bundle({ accessTokenExpiresAt: Date.now() + 60_000 }), + }) + const { provider, refreshSpy } = fakeProvider() + + const result = await refreshAccessToken({ + store, + provider, + skewMs: 5_000, + lockPath, + }) + + expect(result.rotated).toBe(false) + expect(refreshSpy).not.toHaveBeenCalled() + }) + + it('force:true rotates regardless of expiry', async () => { + const { store } = fakeStore({ + account, + bundle: bundle({ accessTokenExpiresAt: Date.now() + 60_000 }), + }) + const { provider, refreshSpy } = fakeProvider() + + const result = await refreshAccessToken({ + store, + provider, + force: true, + lockPath, + }) + + expect(result.rotated).toBe(true) + expect(refreshSpy).toHaveBeenCalledTimes(1) + }) + + it('returns rotated:false when accessTokenExpiresAt is missing (consumer reactive-refreshes on 401)', async () => { + const { store } = fakeStore({ + account, + bundle: { accessToken: 'tok_a', refreshToken: 'r_a' }, + }) + const { provider, refreshSpy } = fakeProvider() + + const result = await refreshAccessToken({ store, provider, lockPath }) + + expect(result.rotated).toBe(false) + expect(refreshSpy).not.toHaveBeenCalled() + }) + + it('throws AUTH_REFRESH_UNAVAILABLE when the bundle has no refresh token', async () => { + const { store } = fakeStore({ + account, + bundle: { accessToken: 'tok_a', accessTokenExpiresAt: Date.now() + 1_000 }, + }) + const { provider } = fakeProvider() + + await expect( + refreshAccessToken({ store, provider, skewMs: 5_000, lockPath }), + ).rejects.toMatchObject({ code: 'AUTH_REFRESH_UNAVAILABLE' }) + }) + + it('throws AUTH_REFRESH_UNAVAILABLE when the store has no snapshot', async () => { + const { store } = fakeStore(null) + const { provider } = fakeProvider() + + await expect( + refreshAccessToken({ store, provider, force: true, lockPath }), + ).rejects.toMatchObject({ code: 'AUTH_REFRESH_UNAVAILABLE' }) + }) + + it('throws AUTH_REFRESH_UNAVAILABLE when the provider does not implement refreshToken', async () => { + const { store } = fakeStore({ account, bundle: bundle() }) + const provider: AuthProvider = { + async authorize() { + return { authorizeUrl: '', handshake: {} } + }, + async exchangeCode() { + return { accessToken: '' } + }, + async validateToken() { + return account + }, + } + + await expect( + refreshAccessToken({ store, provider, force: true, lockPath }), + ).rejects.toMatchObject({ code: 'AUTH_REFRESH_UNAVAILABLE' }) + }) + + it('throws AUTH_REFRESH_UNAVAILABLE when the store does not implement activeBundle', async () => { + const store: TokenStore = { + async active() { + return { token: 'tok_a', account } + }, + async set() {}, + async clear() {}, + async list() { + return [] + }, + async setDefault() {}, + } + const { provider } = fakeProvider() + + await expect( + refreshAccessToken({ store, provider, force: true, lockPath }), + ).rejects.toMatchObject({ code: 'AUTH_REFRESH_UNAVAILABLE' }) + }) + + it('lock contention: returns the rotated bundle when the holder rotated during the wait', async () => { + // Pre-create the lock file to simulate a holder. + await writeFile(lockPath, '', { flag: 'wx' }) + + const initial: ActiveBundleSnapshot = { + account, + bundle: bundle({ accessTokenExpiresAt: Date.now() + 1_000 }), + } + const { store, state } = fakeStore(initial) + const { provider, refreshSpy } = fakeProvider() + + // After ~120ms the "holder" releases the lock and persists a rotated bundle. + setTimeout(async () => { + state.snapshot = { + account, + bundle: { accessToken: 'tok_held', refreshToken: 'r_held' }, + } + await rm(lockPath, { force: true }) + }, 120) + + const result = await refreshAccessToken({ + store, + provider, + skewMs: 5_000, + lockPath, + }) + + expect(result.rotated).toBe(true) + expect(result.bundle.accessToken).toBe('tok_held') + // Waiter must not have POSTed — the holder already did. + expect(refreshSpy).not.toHaveBeenCalled() + }) + + it('lock contention: throws AUTH_REFRESH_TRANSIENT when the holder times out without rotating', async () => { + await writeFile(lockPath, '', { flag: 'wx' }) + + const { store } = fakeStore({ + account, + bundle: bundle({ accessTokenExpiresAt: Date.now() + 1_000 }), + }) + const { provider, refreshSpy } = fakeProvider() + + await expect( + refreshAccessToken({ store, provider, skewMs: 5_000, lockPath }), + ).rejects.toMatchObject({ code: 'AUTH_REFRESH_TRANSIENT' }) + expect(refreshSpy).not.toHaveBeenCalled() + }, 5_000) + + it('propagates AUTH_REFRESH_EXPIRED from the provider unchanged', async () => { + const { store } = fakeStore({ + account, + bundle: bundle({ accessTokenExpiresAt: Date.now() + 1_000 }), + }) + const { provider } = fakeProvider(async () => { + const { CliError } = await import('../errors.js') + throw new CliError('AUTH_REFRESH_EXPIRED', 'rejected by server') + }) + + await expect( + refreshAccessToken({ store, provider, skewMs: 5_000, lockPath }), + ).rejects.toMatchObject({ code: 'AUTH_REFRESH_EXPIRED' }) + }) + + it('carries the previous refresh token forward when the server omits it from the response', async () => { + const { store, state } = fakeStore({ + account, + bundle: bundle({ + refreshToken: 'r_existing', + accessTokenExpiresAt: Date.now() + 1_000, + refreshTokenExpiresAt: 9_999_999_999_999, + }), + }) + const { provider } = fakeProvider(async () => ({ + accessToken: 'tok_new', + expiresAt: Date.now() + 60_000, + // no refresh_token in response — most servers + })) + + const result = await refreshAccessToken({ + store, + provider, + force: true, + lockPath, + }) + + expect(result.bundle.refreshToken).toBe('r_existing') + expect(result.bundle.refreshTokenExpiresAt).toBe(9_999_999_999_999) + // Persisted shape matches the returned bundle. + expect(state.setBundleCalls[0].bundle.refreshToken).toBe('r_existing') + }) +}) diff --git a/src/auth/refresh.ts b/src/auth/refresh.ts new file mode 100644 index 0000000..a12313f --- /dev/null +++ b/src/auth/refresh.ts @@ -0,0 +1,207 @@ +import { open, unlink } from 'node:fs/promises' +import { setTimeout as sleep } from 'node:timers/promises' +import { CliError, getErrorMessage } from '../errors.js' +import { bundleFromExchange, persistBundle } from './persist.js' +import type { + AccountRef, + ActiveBundleSnapshot, + AuthAccount, + AuthProvider, + TokenBundle, + TokenStore, +} from './types.js' + +export type RefreshAccessTokenOptions = { + store: TokenStore + provider: AuthProvider + ref?: AccountRef + /** + * Refresh proactively when the access token's expiry is within this many + * ms of now. Default 60_000 (60s). Ignored when `force: true`. + */ + skewMs?: number + /** + * Reactive path: caller hit a 401 and wants a rotation regardless of + * expiry. Skips the skew check; still honours all the "unavailable" + * gates (no refresh token, no provider hook, no `activeBundle`). + */ + force?: boolean + /** + * Path to the O_EXCL concurrency lock file. Required — cli-core does not + * interpret `~` or know where the consumer's config lives. Recommended: + * `${getConfigPath(serviceName)}.refresh.lock`. + */ + lockPath: string +} + +export type RefreshAccessTokenResult = { + rotated: boolean + bundle: TokenBundle + account: TAccount +} + +const DEFAULT_SKEW_MS = 60_000 +const LOCK_WAIT_TIMEOUT_MS = 2_000 +const LOCK_POLL_INTERVAL_MS = 50 + +/** + * Rotate the access token using the stored refresh token. Proactive when + * `accessTokenExpiresAt` is within `skewMs` of now; reactive when `force: + * true`. Uses an `O_EXCL` file lock at `lockPath` so concurrent CLI + * invocations don't issue parallel refresh-token grants — one POSTs, the + * others re-read the rotated bundle from the store. + * + * Throws `AUTH_REFRESH_UNAVAILABLE` when refresh isn't possible in the + * current setup: no refresh token stored, store doesn't implement + * `activeBundle`, provider doesn't implement `refreshToken`, or + * `oauth4webapi` isn't installed. Server-side rejections surface as + * `AUTH_REFRESH_EXPIRED` (re-login required) or `AUTH_REFRESH_TRANSIENT` + * (retryable). + */ +export async function refreshAccessToken( + options: RefreshAccessTokenOptions, +): Promise> { + const { store, provider, ref, force, lockPath } = options + const skewMs = options.skewMs ?? DEFAULT_SKEW_MS + + if (!store.activeBundle) { + throw new CliError( + 'AUTH_REFRESH_UNAVAILABLE', + 'TokenStore does not implement activeBundle; refresh is not supported.', + { hints: ['Re-run the login command to reauthorize.'] }, + ) + } + + const snapshot = await store.activeBundle(ref) + if (!snapshot) { + throw new CliError('AUTH_REFRESH_UNAVAILABLE', 'No stored credential to refresh.', { + hints: ['Re-run the login command to reauthorize.'], + }) + } + + if (!force && !needsRefresh(snapshot.bundle, skewMs)) { + return { rotated: false, bundle: snapshot.bundle, account: snapshot.account } + } + + if (!snapshot.bundle.refreshToken) { + throw new CliError('AUTH_REFRESH_UNAVAILABLE', 'Stored credential has no refresh token.', { + hints: ['Re-run the login command to reauthorize.'], + }) + } + if (!provider.refreshToken) { + throw new CliError( + 'AUTH_REFRESH_UNAVAILABLE', + 'Auth provider does not implement refreshToken.', + ) + } + + const lock = await acquireLock(lockPath, store, ref, snapshot) + if (lock.kind === 'rotated-by-holder') { + return { rotated: true, bundle: lock.snapshot.bundle, account: lock.snapshot.account } + } + if (lock.kind === 'timeout') { + throw new CliError( + 'AUTH_REFRESH_TRANSIENT', + 'Timed out waiting for a concurrent refresh to complete.', + { hints: ['Try again.'] }, + ) + } + + try { + const exchange = await provider.refreshToken({ + refreshToken: snapshot.bundle.refreshToken, + handshake: {}, + }) + + const account = exchange.account ?? snapshot.account + const bundle = bundleFromExchange(exchange, snapshot.bundle) + + await persistBundle({ store, account, bundle }) + return { rotated: true, bundle, account } + } finally { + await releaseLock(lockPath) + } +} + +function needsRefresh(bundle: TokenBundle, skewMs: number): boolean { + // No expiry tracked → can't proactively refresh; defer to reactive 401. + if (bundle.accessTokenExpiresAt === undefined) return false + return bundle.accessTokenExpiresAt - Date.now() < skewMs +} + +type LockOutcome = + | { kind: 'acquired' } + | { kind: 'rotated-by-holder'; snapshot: ActiveBundleSnapshot } + | { kind: 'timeout' } + +/** + * Try to acquire the `O_EXCL` lock. On `EEXIST`, poll for the lock file to + * disappear (up to `LOCK_WAIT_TIMEOUT_MS`). Whether the wait ends via + * acquisition, lock-released, or timeout, re-read the bundle: if the holder + * has rotated, return the new snapshot so the waiter doesn't POST. + */ +async function acquireLock( + lockPath: string, + store: TokenStore, + ref: AccountRef | undefined, + snapshotBefore: ActiveBundleSnapshot, +): Promise> { + if (await tryCreateLockFile(lockPath)) { + return { kind: 'acquired' } + } + + const deadline = Date.now() + LOCK_WAIT_TIMEOUT_MS + while (Date.now() < deadline) { + await sleep(LOCK_POLL_INTERVAL_MS) + if (await tryCreateLockFile(lockPath)) { + // Lock acquired — but the holder may have completed a rotation + // before releasing. Re-check the store before POSTing. + const fresh = await store.activeBundle?.(ref) + if (fresh && hasRotated(snapshotBefore.bundle, fresh.bundle)) { + await releaseLock(lockPath) + return { kind: 'rotated-by-holder', snapshot: fresh } + } + return { kind: 'acquired' } + } + } + + // Timed out: holder didn't release. Re-read once more — they may have + // rotated then crashed before unlinking, in which case the waiter should + // still benefit from the new bundle. + const fresh = await store.activeBundle?.(ref) + if (fresh && hasRotated(snapshotBefore.bundle, fresh.bundle)) { + return { kind: 'rotated-by-holder', snapshot: fresh } + } + return { kind: 'timeout' } +} + +function hasRotated(before: TokenBundle, after: TokenBundle): boolean { + if (after.accessToken !== before.accessToken) return true + if (after.accessTokenExpiresAt !== before.accessTokenExpiresAt) return true + return false +} + +async function tryCreateLockFile(lockPath: string): Promise { + try { + const handle = await open(lockPath, 'wx') + await handle.close() + return true + } catch (error) { + const code = (error as NodeJS.ErrnoException).code + if (code === 'EEXIST') return false + throw new CliError( + 'AUTH_REFRESH_TRANSIENT', + `Failed to acquire refresh lock: ${getErrorMessage(error)}`, + { hints: ['Try again.'] }, + ) + } +} + +async function releaseLock(lockPath: string): Promise { + try { + await unlink(lockPath) + } catch { + // best-effort: a missing lock file (manual cleanup, crash, …) must + // not surface as a refresh failure. + } +} diff --git a/src/auth/status.ts b/src/auth/status.ts index c27ff09..2a40669 100644 --- a/src/auth/status.ts +++ b/src/auth/status.ts @@ -2,9 +2,28 @@ import type { Command } from 'commander' import { CliError } from '../errors.js' import { formatJson, formatNdjson } from '../json.js' import type { ViewOptions } from '../options.js' -import type { AuthAccount, TokenStore } from './types.js' +import type { AccountRef, AuthAccount, TokenBundle, TokenStore } from './types.js' import { attachUserFlag, extractUserRef, requireSnapshotForRef } from './user-flag.js' +/** + * Opportunistic bundle read for `fetchLive`. Stores that don't implement + * `activeBundle` get `undefined`; any error during the bundle read is + * swallowed so a refresh-slot fault can't break the status command, which + * is the user's first port of call when something is wrong. + */ +async function readBundleBestEffort( + store: TokenStore, + ref: AccountRef | undefined, +): Promise { + if (!store.activeBundle) return undefined + try { + const snapshot = await store.activeBundle(ref) + return snapshot?.bundle + } catch { + return undefined + } +} + export type AttachStatusContext = { account: TAccount /** `--json` / `--ndjson` flag values, both present (defaulted to `false`). */ @@ -25,6 +44,12 @@ export type AttachStatusCommandOptions flags: Record @@ -80,10 +105,14 @@ export function attachStatusCommand( } throw new CliError('NOT_AUTHENTICATED', 'Not signed in.') } + const bundle = options.fetchLive + ? await readBundleBestEffort(options.store, ref) + : undefined const account = options.fetchLive ? await options.fetchLive({ account: snapshot.account, token: snapshot.token, + ...(bundle ? { bundle } : {}), view, flags, }) diff --git a/src/auth/types.ts b/src/auth/types.ts index 934d9a9..7379be9 100644 --- a/src/auth/types.ts +++ b/src/auth/types.ts @@ -90,6 +90,12 @@ export type TokenBundle = { refreshTokenExpiresAt?: number } +/** Read-side snapshot returned by `activeBundle`. Mirrors `setBundle`'s write side. */ +export type ActiveBundleSnapshot = { + account: TAccount + bundle: TokenBundle +} + /** Opaque account selector. Stores own the matching rule (id, email, label, …). */ export type AccountRef = string @@ -123,6 +129,18 @@ export type TokenStore = { bundle: TokenBundle, options?: { promoteDefault?: boolean }, ): Promise + /** + * Full-bundle read for refresh-capable consumers. Returns the matching + * account + bundle, or `null` on miss. Optional on the contract — the + * silent-refresh helper throws `AUTH_REFRESH_UNAVAILABLE` when a custom + * store doesn't implement it. `KeyringTokenStore` overrides this as + * required so cli-core helpers can call it without a non-null assertion. + * + * Stores MAY throw `CliError('AUTH_STORE_READ_FAILED', …)` on the same + * conditions as `active()` (e.g. keyring offline while a matching + * record exists). + */ + activeBundle?(ref?: AccountRef): Promise | null> /** Remove a stored credential. No-op when `ref` doesn't match. */ clear(ref?: AccountRef): Promise /** Every stored account with a default marker. */ From f97cb74231db7ebdacd6b0e4e0400cef00750555 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Tue, 19 May 2026 15:28:49 +0100 Subject: [PATCH 2/2] refactor(auth): trim refresh-helper comment + test bloat MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The whole point of using oauth4webapi was to write less code — the initial PR3 commit didn't fully lean on that. Cuts: - `mapRefreshResponse` + `translateRefreshError` extracted as named functions called exactly once each. Inlined into `refreshToken`; the surrounding code reads top-to-bottom. - `loadOauth4webapi` was distinguishing `ERR_MODULE_NOT_FOUND` from other errors and surfacing the same `AUTH_REFRESH_UNAVAILABLE` code either way. Collapsed to one branch. - Verbose JSDoc on internal `acquireLock` / `translateRefreshError` restated what the code already says. Kept only the WHY-comments (re-read invariant on both acquire and timeout). - Three test-bloat cuts: provider 400-vs-401 + transient-variant it.eaches (oauth4webapi treats them identically — passes by construction); `refresh.test.ts#propagates AUTH_REFRESH_EXPIRED` (helper does no wrapping — `await` propagates by default); `internal.test.ts` redundant slot-read + slot-unavailable cases (covered by `activeBundle` integration tests in token-store). 412 tests still pass; net -128 LOC. --- src/auth/keyring/internal.test.ts | 26 +------- src/auth/providers/pkce.test.ts | 84 +++++++----------------- src/auth/providers/pkce.ts | 104 ++++++++++-------------------- src/auth/refresh.test.ts | 24 +------ src/auth/refresh.ts | 18 ++---- 5 files changed, 64 insertions(+), 192 deletions(-) diff --git a/src/auth/keyring/internal.test.ts b/src/auth/keyring/internal.test.ts index 3b36ac8..4afbf52 100644 --- a/src/auth/keyring/internal.test.ts +++ b/src/auth/keyring/internal.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it, vi } from 'vitest' import type { AuthAccount } from '../types.js' import { readRefreshTokenForRecord } from './internal.js' -import { type SecureStore, SecureStoreUnavailableError } from './secure-store.js' +import type { SecureStore } from './secure-store.js' import type { UserRecord } from './types.js' type Account = AuthAccount & { id: string } @@ -45,28 +45,4 @@ describe('readRefreshTokenForRecord', () => { expect(outcome).toEqual({ ok: true, token: 'plaintext_fallback' }) expect(getSpy).not.toHaveBeenCalled() }) - - it('reads the keyring slot when no fallback is present', async () => { - const store = fakeStore({ - async getSecret() { - return 'from_keyring' - }, - }) - const record: UserRecord = { account, hasRefreshToken: true } - - const outcome = await readRefreshTokenForRecord(record, store) - expect(outcome).toEqual({ ok: true, token: 'from_keyring' }) - }) - - it('maps SecureStoreUnavailableError to slot-unavailable', async () => { - const store = fakeStore({ - async getSecret() { - throw new SecureStoreUnavailableError('no dbus') - }, - }) - const record: UserRecord = { account, hasRefreshToken: true } - - const outcome = await readRefreshTokenForRecord(record, store) - expect(outcome).toMatchObject({ ok: false, reason: 'slot-unavailable' }) - }) }) diff --git a/src/auth/providers/pkce.test.ts b/src/auth/providers/pkce.test.ts index e4ba826..ed995d9 100644 --- a/src/auth/providers/pkce.test.ts +++ b/src/auth/providers/pkce.test.ts @@ -146,69 +146,52 @@ describe('createPkceProvider', () => { }) }) -// `oauth4webapi` is lazy-imported by provider.refreshToken and uses the -// global `fetch`. Each refresh test stubs `globalThis.fetch` to drive the -// request; real `oauth4webapi` enforces https for the token endpoint, so -// all URLs below use https. describe('createPkceProvider.refreshToken', () => { - const tokenUrl = 'https://example.com/oauth/token' - function refreshProvider() { return createPkceProvider({ authorizeUrl: 'https://example.com/oauth/authorize', - tokenUrl, + tokenUrl: 'https://example.com/oauth/token', clientId: 'client-xyz', validate, }) } - function stubFetch(impl: typeof fetch): ReturnType { - return vi.spyOn(globalThis, 'fetch').mockImplementation(impl) + function stubFetch(impl: typeof fetch): void { + vi.spyOn(globalThis, 'fetch').mockImplementation(impl) } afterEach(() => { vi.restoreAllMocks() }) - it('POSTs the refresh grant and returns the rotated bundle (no client_secret)', async () => { - let captured: { url: string; body: string } | undefined - stubFetch((async (input: RequestInfo | URL, init: RequestInit = {}) => { - captured = { url: String(input), body: String(init.body ?? '') } - return new Response( - JSON.stringify({ - access_token: 'tok-new', - refresh_token: 'r-new', - expires_in: 3600, - token_type: 'bearer', - }), - { status: 200, headers: { 'Content-Type': 'application/json' } }, - ) - }) as typeof fetch) + it('POSTs the refresh grant and returns the rotated bundle', async () => { + stubFetch( + (async () => + new Response( + JSON.stringify({ + access_token: 'tok-new', + refresh_token: 'r-new', + expires_in: 3600, + token_type: 'bearer', + }), + { status: 200, headers: { 'Content-Type': 'application/json' } }, + )) as typeof fetch, + ) const result = await refreshProvider().refreshToken!({ refreshToken: 'r-old', handshake: {}, }) - expect(result.accessToken).toBe('tok-new') - expect(result.refreshToken).toBe('r-new') + expect(result).toMatchObject({ accessToken: 'tok-new', refreshToken: 'r-new' }) expect(result.expiresAt).toBeGreaterThan(Date.now()) - expect(captured?.url).toBe(tokenUrl) - const body = new URLSearchParams(captured!.body) - expect(body.get('grant_type')).toBe('refresh_token') - expect(body.get('refresh_token')).toBe('r-old') - expect(body.get('client_id')).toBe('client-xyz') - expect(body.has('client_secret')).toBe(false) }) - it.each([ - ['400', 400], - ['401 (reverse-proxy remap)', 401], - ])('maps invalid_grant %s to AUTH_REFRESH_EXPIRED', async (_label, status) => { + it('maps invalid_grant to AUTH_REFRESH_EXPIRED (any HTTP status — proxies remap 400/401)', async () => { stubFetch( (async () => new Response(JSON.stringify({ error: 'invalid_grant' }), { - status, + status: 401, headers: { 'Content-Type': 'application/json' }, })) as typeof fetch, ) @@ -218,31 +201,10 @@ describe('createPkceProvider.refreshToken', () => { ).rejects.toMatchObject({ code: 'AUTH_REFRESH_EXPIRED' }) }) - it.each([ - [ - '500', - async () => - new Response(JSON.stringify({ error: 'server_error' }), { - status: 500, - headers: { 'Content-Type': 'application/json' }, - }), - ], - [ - 'non-JSON 2xx', - async () => - new Response('oops', { - status: 200, - headers: { 'Content-Type': 'text/html' }, - }), - ], - [ - 'network failure', - async () => { - throw new Error('connection reset') - }, - ], - ])('maps %s to AUTH_REFRESH_TRANSIENT', async (_label, impl) => { - stubFetch(impl as typeof fetch) + it('maps everything else (network, 5xx, non-JSON, other OAuth errors) to AUTH_REFRESH_TRANSIENT', async () => { + stubFetch((async () => { + throw new Error('connection reset') + }) as typeof fetch) await expect( refreshProvider().refreshToken!({ refreshToken: 'r-old', handshake: {} }), diff --git a/src/auth/providers/pkce.ts b/src/auth/providers/pkce.ts index 6838ad7..2d5496a 100644 --- a/src/auth/providers/pkce.ts +++ b/src/auth/providers/pkce.ts @@ -1,4 +1,4 @@ -import type { AuthorizationServer, Client, TokenEndpointResponse } from 'oauth4webapi' +import type { AuthorizationServer, Client } from 'oauth4webapi' import { CliError, getErrorMessage } from '../../errors.js' import { deriveChallenge, generateVerifier } from '../pkce.js' import type { @@ -165,19 +165,10 @@ export function createPkceProvider( async refreshToken(input: RefreshInput): Promise> { const oauth = await loadOauth4webapi() - // RefreshInput.handshake is empty by default; the helper has no - // flags context during silent rotation, so resolvers that need - // per-flow flags should encode the relevant state in the - // handshake the caller supplies (or be constant). - const flags = (input.handshake.flags as Record | undefined) ?? {} - const tokenUrlResolved = resolve(options.tokenUrl, input.handshake, flags) - const clientIdResolved = resolve(options.clientId, input.handshake, flags) - const as: AuthorizationServer = { - issuer: tokenUrlResolved, - token_endpoint: tokenUrlResolved, - } + const tokenUrl = resolve(options.tokenUrl, input.handshake, {}) + const as: AuthorizationServer = { issuer: tokenUrl, token_endpoint: tokenUrl } const client: Client = { - client_id: clientIdResolved, + client_id: resolve(options.clientId, input.handshake, {}), token_endpoint_auth_method: 'none', } try { @@ -188,9 +179,31 @@ export function createPkceProvider( input.refreshToken, ) const result = await oauth.processRefreshTokenResponse(as, client, response) - return mapRefreshResponse(result) + return { + accessToken: result.access_token, + refreshToken: result.refresh_token, + expiresAt: + typeof result.expires_in === 'number' + ? Date.now() + result.expires_in * 1000 + : undefined, + } } catch (error) { - throw translateRefreshError(oauth, error) + // `invalid_grant` (any status — some proxies remap 400 → 401) + // is the only retryable-by-relogin signal; everything else + // (network, 5xx, non-JSON, other OAuth error codes) is + // transient from cli-core's POV. + if (error instanceof oauth.ResponseBodyError && error.error === 'invalid_grant') { + throw new CliError( + 'AUTH_REFRESH_EXPIRED', + `Refresh token rejected: ${error.error_description ?? error.error}`, + { hints: ['Re-run the login command to reauthorize.'] }, + ) + } + throw new CliError( + 'AUTH_REFRESH_TRANSIENT', + `Refresh request failed: ${getErrorMessage(error)}`, + { hints: ['Try again.'] }, + ) } }, } @@ -204,68 +217,17 @@ function resolve( return typeof resolver === 'function' ? resolver({ handshake, flags }) : resolver } -/** - * Lazy-import `oauth4webapi`. It's an optional peer dep — only refresh - * consumers install it. Missing module → `AUTH_REFRESH_UNAVAILABLE` with an - * actionable hint; any other import failure rethrows as `CliError` of the - * same code (the user can't recover beyond "install the dep" either way). - */ -type Oauth4WebApi = typeof import('oauth4webapi') - -async function loadOauth4webapi(): Promise { +// Optional peer dep — only refresh consumers install it. +async function loadOauth4webapi(): Promise { try { return await import('oauth4webapi') - } catch (error) { - const code = (error as NodeJS.ErrnoException | undefined)?.code - if (code === 'ERR_MODULE_NOT_FOUND' || code === 'MODULE_NOT_FOUND') { - throw new CliError( - 'AUTH_REFRESH_UNAVAILABLE', - 'oauth4webapi is required for refresh-token support.', - { hints: ['Run `npm install oauth4webapi` in your CLI.'] }, - ) - } + } catch { throw new CliError( 'AUTH_REFRESH_UNAVAILABLE', - `Failed to load oauth4webapi: ${getErrorMessage(error)}`, - ) - } -} - -function mapRefreshResponse( - response: TokenEndpointResponse, -): ExchangeResult { - return { - accessToken: response.access_token, - refreshToken: response.refresh_token, - expiresAt: - typeof response.expires_in === 'number' - ? Date.now() + response.expires_in * 1000 - : undefined, - } -} - -/** - * Translate `oauth4webapi` failures to the typed refresh contract. - * - * - `ResponseBodyError` with `error === 'invalid_grant'` → `AUTH_REFRESH_EXPIRED` - * regardless of status (some reverse proxies remap 400 → 401). - * - Everything else (other `ResponseBodyError` codes, network failures, 5xx, - * non-JSON bodies, `WWWAuthenticateChallengeError`) → `AUTH_REFRESH_TRANSIENT`. - */ -function translateRefreshError(oauth: Oauth4WebApi, error: unknown): CliError { - if (error instanceof CliError) return error - if (error instanceof oauth.ResponseBodyError && error.error === 'invalid_grant') { - return new CliError( - 'AUTH_REFRESH_EXPIRED', - `Refresh token rejected: ${error.error_description ?? error.error}`, - { hints: ['Re-run the login command to reauthorize.'] }, + 'oauth4webapi is required for refresh-token support.', + { hints: ['Run `npm install oauth4webapi` in your CLI.'] }, ) } - return new CliError( - 'AUTH_REFRESH_TRANSIENT', - `Refresh request failed: ${getErrorMessage(error)}`, - { hints: ['Try again.'] }, - ) } async function safeReadText(response: Response): Promise { diff --git a/src/auth/refresh.test.ts b/src/auth/refresh.test.ts index c9f25f0..0718c88 100644 --- a/src/auth/refresh.test.ts +++ b/src/auth/refresh.test.ts @@ -125,10 +125,9 @@ describe('refreshAccessToken', () => { expect(result.rotated).toBe(true) expect(result.bundle.accessToken).toBe('tok_new') expect(refreshSpy).toHaveBeenCalledWith({ refreshToken: 'r_a', handshake: {} }) - // setBundle was called without promoteDefault — silent rotation must - // not re-pin account selection. The helper omits the options arg - // entirely so presence-based handlers see the same shape they would - // for "no caller preference". + // promoteDefault omitted (not just `false`): a silent rotation must + // not re-pin selection, and the helper distinguishes "absent" from + // "explicit opt-out" via arg count. expect(state.setBundleCalls).toHaveLength(1) expect(state.setBundleCalls[0].options).toBeUndefined() }) @@ -289,21 +288,6 @@ describe('refreshAccessToken', () => { expect(refreshSpy).not.toHaveBeenCalled() }, 5_000) - it('propagates AUTH_REFRESH_EXPIRED from the provider unchanged', async () => { - const { store } = fakeStore({ - account, - bundle: bundle({ accessTokenExpiresAt: Date.now() + 1_000 }), - }) - const { provider } = fakeProvider(async () => { - const { CliError } = await import('../errors.js') - throw new CliError('AUTH_REFRESH_EXPIRED', 'rejected by server') - }) - - await expect( - refreshAccessToken({ store, provider, skewMs: 5_000, lockPath }), - ).rejects.toMatchObject({ code: 'AUTH_REFRESH_EXPIRED' }) - }) - it('carries the previous refresh token forward when the server omits it from the response', async () => { const { store, state } = fakeStore({ account, @@ -316,7 +300,6 @@ describe('refreshAccessToken', () => { const { provider } = fakeProvider(async () => ({ accessToken: 'tok_new', expiresAt: Date.now() + 60_000, - // no refresh_token in response — most servers })) const result = await refreshAccessToken({ @@ -328,7 +311,6 @@ describe('refreshAccessToken', () => { expect(result.bundle.refreshToken).toBe('r_existing') expect(result.bundle.refreshTokenExpiresAt).toBe(9_999_999_999_999) - // Persisted shape matches the returned bundle. expect(state.setBundleCalls[0].bundle.refreshToken).toBe('r_existing') }) }) diff --git a/src/auth/refresh.ts b/src/auth/refresh.ts index a12313f..338b8eb 100644 --- a/src/auth/refresh.ts +++ b/src/auth/refresh.ts @@ -134,28 +134,21 @@ type LockOutcome = | { kind: 'rotated-by-holder'; snapshot: ActiveBundleSnapshot } | { kind: 'timeout' } -/** - * Try to acquire the `O_EXCL` lock. On `EEXIST`, poll for the lock file to - * disappear (up to `LOCK_WAIT_TIMEOUT_MS`). Whether the wait ends via - * acquisition, lock-released, or timeout, re-read the bundle: if the holder - * has rotated, return the new snapshot so the waiter doesn't POST. - */ +// Re-reading the bundle after a contested wait — on acquire OR timeout — +// is load-bearing: the holder may have rotated then crashed before +// unlinking, and the waiter should still benefit from the new bundle. async function acquireLock( lockPath: string, store: TokenStore, ref: AccountRef | undefined, snapshotBefore: ActiveBundleSnapshot, ): Promise> { - if (await tryCreateLockFile(lockPath)) { - return { kind: 'acquired' } - } + if (await tryCreateLockFile(lockPath)) return { kind: 'acquired' } const deadline = Date.now() + LOCK_WAIT_TIMEOUT_MS while (Date.now() < deadline) { await sleep(LOCK_POLL_INTERVAL_MS) if (await tryCreateLockFile(lockPath)) { - // Lock acquired — but the holder may have completed a rotation - // before releasing. Re-check the store before POSTing. const fresh = await store.activeBundle?.(ref) if (fresh && hasRotated(snapshotBefore.bundle, fresh.bundle)) { await releaseLock(lockPath) @@ -165,9 +158,6 @@ async function acquireLock( } } - // Timed out: holder didn't release. Re-read once more — they may have - // rotated then crashed before unlinking, in which case the waiter should - // still benefit from the new bundle. const fresh = await store.activeBundle?.(ref) if (fresh && hasRotated(snapshotBefore.bundle, fresh.bundle)) { return { kind: 'rotated-by-holder', snapshot: fresh }