From 394d552bcc56437491c4011b491205ffb08417ca Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Thu, 21 May 2026 10:35:06 +0100 Subject: [PATCH 1/2] feat(auth): silent OAuth token refresh Adopts cli-core 0.19.0's refresh machinery so `ol` transparently rotates expired access tokens instead of forcing `ol auth login` every time the workspace's short-lived token expires. - Replaces the bespoke AuthProvider with `createPkceProvider` (cli-core now supports async resolvers + a custom-transport `fetchImpl`), so the refresh grant is inherited rather than hand-rolled. `exchangeCode` now captures `refresh_token` + `expires_in` instead of dropping them. - API client refreshes proactively (within the expiry skew, before a request) and reactively (force-refresh + single retry on a 401). `OUTLINE_API_TOKEN` env tokens are never refreshed. - `ol auth status` routes through the managed path so it self-heals an expired-but-refreshable token rather than reporting "expired". - `StoredUser` round-trips the bundle metadata (refresh_token, access_token_expires_at, refresh_token_expires_at, has_refresh_token); refresh tokens live in a sibling keyring slot. Requires the Outline OAuth app to be a public client (refresh sends no client_secret; a confidential client rejects the refresh grant). Co-Authored-By: Claude Opus 4.7 (1M context) --- package-lock.json | 22 +++- package.json | 3 +- src/__tests__/api.test.ts | 82 +++++++++++---- src/__tests__/auth-command.test.ts | 4 +- src/__tests__/auth-provider.test.ts | 129 ++++++++++++++++++++---- src/__tests__/auth.test.ts | 31 ++++++ src/__tests__/user-records.test.ts | 35 +++++++ src/commands/auth.ts | 15 +-- src/lib/api.ts | 48 ++++++--- src/lib/auth-provider.ts | 151 +++++++++++++--------------- src/lib/auth.ts | 61 ++++++++++- src/lib/config.ts | 9 ++ src/lib/user-records.ts | 30 ++++-- 13 files changed, 463 insertions(+), 157 deletions(-) diff --git a/package-lock.json b/package-lock.json index 444c773..e7476aa 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10,11 +10,12 @@ "hasInstallScript": true, "license": "MIT", "dependencies": { - "@doist/cli-core": "0.16.1", + "@doist/cli-core": "0.19.0", "chalk": "5.6.2", "commander": "14.0.2", "marked": "18.0.3", "marked-terminal-renderer": "2.2.0", + "oauth4webapi": "3.8.6", "open": "10.2.0", "undici": "7.22.0" }, @@ -132,9 +133,9 @@ } }, "node_modules/@doist/cli-core": { - "version": "0.16.1", - "resolved": "https://registry.npmjs.org/@doist/cli-core/-/cli-core-0.16.1.tgz", - "integrity": "sha512-OoHWYM8Rv+eoPhVR1BUMwFIejZGk5Wyxheva4Cp9x3zTh4RURE+OIJ03G95oBegbXCA2O9LRtZZ9Ww0D6vU1dQ==", + "version": "0.19.0", + "resolved": "https://registry.npmjs.org/@doist/cli-core/-/cli-core-0.19.0.tgz", + "integrity": "sha512-+CswqzGwcFC78v8oH7uC5poS9Ptqxpw7UKIXs3/xLIbgpGMJB+LKYT88lq8LGw2ZJ2bevsRN867lApgnWdJ4vw==", "license": "MIT", "dependencies": { "chalk": "5.6.2", @@ -150,6 +151,7 @@ "commander": ">=14", "marked": ">=18", "marked-terminal-renderer": ">=2", + "oauth4webapi": ">=3", "open": ">=10", "vitest": ">=4.1" }, @@ -163,6 +165,9 @@ "marked-terminal-renderer": { "optional": true }, + "oauth4webapi": { + "optional": true + }, "open": { "optional": true }, @@ -7662,6 +7667,15 @@ "node": ">=18" } }, + "node_modules/oauth4webapi": { + "version": "3.8.6", + "resolved": "https://registry.npmjs.org/oauth4webapi/-/oauth4webapi-3.8.6.tgz", + "integrity": "sha512-iwemM91xz8nryHti2yTmg5fhyEMVOkOXwHNqbvcATjyajb5oQxCQzrNOA6uElRHuMhQQTKUyFKV9y/CNyg25BQ==", + "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 4361127..ed6b1b3 100644 --- a/package.json +++ b/package.json @@ -48,11 +48,12 @@ "node": ">=20.18.1" }, "dependencies": { - "@doist/cli-core": "0.16.1", + "@doist/cli-core": "0.19.0", "chalk": "5.6.2", "commander": "14.0.2", "marked": "18.0.3", "marked-terminal-renderer": "2.2.0", + "oauth4webapi": "3.8.6", "open": "10.2.0", "undici": "7.22.0" }, diff --git a/src/__tests__/api.test.ts b/src/__tests__/api.test.ts index 9d33aa9..4389d86 100644 --- a/src/__tests__/api.test.ts +++ b/src/__tests__/api.test.ts @@ -1,24 +1,34 @@ -import { afterEach, describe, expect, it, vi } from 'vitest' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -vi.mock('../lib/auth.js', () => ({ - getApiToken: async () => 'test-token', - getBaseUrl: async () => 'https://test.outline.com', +const authMocks = vi.hoisted(() => ({ + getApiToken: vi.fn(async () => 'test-token'), + getBaseUrl: vi.fn(async () => 'https://test.outline.com'), + proactiveRefresh: vi.fn(async () => undefined), + reactiveRefresh: vi.fn(async () => false), })) +vi.mock('../lib/auth.js', () => authMocks) + vi.mock('../transport/fetch-with-retry.js', () => ({ fetchWithRetry: vi.fn(), })) describe('apiRequest', () => { + beforeEach(() => { + delete process.env.OUTLINE_API_TOKEN + authMocks.getApiToken.mockReset().mockResolvedValue('test-token') + authMocks.getBaseUrl.mockReset().mockResolvedValue('https://test.outline.com') + authMocks.proactiveRefresh.mockReset().mockResolvedValue(undefined) + authMocks.reactiveRefresh.mockReset().mockResolvedValue(false) + }) + afterEach(() => { vi.clearAllMocks() + vi.unstubAllEnvs() }) it('uses fetchWithRetry for API requests', async () => { - const mockResponse = { - ok: true, - json: async () => ({ data: { id: '123' } }), - } + const mockResponse = { ok: true, json: async () => ({ data: { id: '123' } }) } const { fetchWithRetry } = await import('../transport/fetch-with-retry.js') ;(fetchWithRetry as ReturnType).mockResolvedValue(mockResponse) @@ -41,10 +51,7 @@ describe('apiRequest', () => { it('returns data and pagination', async () => { const mockResponse = { ok: true, - json: async () => ({ - data: [{ id: '1' }], - pagination: { offset: 0, limit: 25 }, - }), + json: async () => ({ data: [{ id: '1' }], pagination: { offset: 0, limit: 25 } }), } const { fetchWithRetry } = await import('../transport/fetch-with-retry.js') ;(fetchWithRetry as ReturnType).mockResolvedValue(mockResponse) @@ -59,18 +66,15 @@ describe('apiRequest', () => { it('throws on non-ok response with API message', async () => { const mockResponse = { ok: false, - status: 401, - statusText: 'Unauthorized', - json: async () => ({ - error: 'auth_required', - message: 'Authentication required', - }), + status: 500, + statusText: 'Internal Server Error', + json: async () => ({ error: 'server_error', message: 'Server exploded' }), } const { fetchWithRetry } = await import('../transport/fetch-with-retry.js') ;(fetchWithRetry as ReturnType).mockResolvedValue(mockResponse) const { apiRequest } = await import('../lib/api.js') - await expect(apiRequest('auth.info')).rejects.toThrow('Authentication required') + await expect(apiRequest('documents.list')).rejects.toThrow('Server exploded') }) it('throws generic message when no API error body', async () => { @@ -90,4 +94,44 @@ describe('apiRequest', () => { 'API error: 500 Internal Server Error', ) }) + + it('force-refreshes and retries once when a managed token gets a 401', async () => { + const { fetchWithRetry } = await import('../transport/fetch-with-retry.js') + const f = fetchWithRetry as ReturnType + f.mockResolvedValueOnce({ + ok: false, + status: 401, + statusText: 'Unauthorized', + json: async () => ({}), + }) + f.mockResolvedValueOnce({ ok: true, json: async () => ({ data: { id: 'ok' } }) }) + authMocks.reactiveRefresh.mockResolvedValueOnce(true) + authMocks.getApiToken + .mockResolvedValueOnce('stale-token') + .mockResolvedValueOnce('rotated-token') + + const { apiRequest } = await import('../lib/api.js') + const result = await apiRequest('documents.info', { id: 'abc' }) + + expect(result.data).toEqual({ id: 'ok' }) + expect(authMocks.reactiveRefresh).toHaveBeenCalledTimes(1) + expect(f).toHaveBeenCalledTimes(2) + const retryHeaders = f.mock.calls[1][0].options.headers as Record + expect(retryHeaders.Authorization).toBe('Bearer rotated-token') + }) + + it('does not refresh when OUTLINE_API_TOKEN is set (unmanaged token)', async () => { + vi.stubEnv('OUTLINE_API_TOKEN', 'env-tok') + const { fetchWithRetry } = await import('../transport/fetch-with-retry.js') + ;(fetchWithRetry as ReturnType).mockResolvedValue({ + ok: true, + json: async () => ({ data: {} }), + }) + + const { apiRequest } = await import('../lib/api.js') + await apiRequest('documents.list') + + expect(authMocks.proactiveRefresh).not.toHaveBeenCalled() + expect(authMocks.reactiveRefresh).not.toHaveBeenCalled() + }) }) diff --git a/src/__tests__/auth-command.test.ts b/src/__tests__/auth-command.test.ts index 9c87d11..83d8a2f 100644 --- a/src/__tests__/auth-command.test.ts +++ b/src/__tests__/auth-command.test.ts @@ -118,10 +118,12 @@ describe('auth status subcommand', () => { const program = await buildProgram() await program.parseAsync(['node', 'ol', 'auth', 'status']) + // status routes through the managed request path (no token override) + // so an expired-but-refreshable token is rotated before the check. expect(apiRequest).toHaveBeenCalledWith( 'auth.info', {}, - { token: 'env-token', baseUrl: 'https://test.outline.com' }, + { baseUrl: 'https://test.outline.com' }, ) expect(logs.some((l) => l.includes('Authenticated'))).toBe(true) expect(logs.some((l) => l.includes('Team:') && l.includes('Analytics'))).toBe(true) diff --git a/src/__tests__/auth-provider.test.ts b/src/__tests__/auth-provider.test.ts index 2f666f4..6100e8b 100644 --- a/src/__tests__/auth-provider.test.ts +++ b/src/__tests__/auth-provider.test.ts @@ -1,5 +1,7 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { + AUTH_INFO, + errResponse, LEGACY_CLEAR_PAYLOAD, LEGACY_CONFIG, okResponse, @@ -20,7 +22,9 @@ const keyringMocks = vi.hoisted(() => ({ createKeyringTokenStore: vi.fn(), inner: { active: vi.fn(), + activeBundle: vi.fn(), set: vi.fn(), + setBundle: vi.fn(), clear: vi.fn(), list: vi.fn(), setDefault: vi.fn(), @@ -94,15 +98,14 @@ describe('createOutlineAuthProvider', () => { }) expect(url.searchParams.get('code_challenge')).toMatch(/^[A-Za-z0-9_-]+$/) + // createPkceProvider carries the verifier + resolved client id through + // the handshake (base URL rides the provider closure, not the handshake). const handshake = result.handshake as Record - expect(handshake).toMatchObject({ - baseUrl: 'https://wiki.example.com', - clientId: 'cid-xyz', - }) + expect(handshake.clientId).toBe('cid-xyz') expect(handshake.codeVerifier?.length).toBeGreaterThan(40) }) - it('exchangeCode posts via fetchWithRetry and surfaces provider errors', async () => { + it('exchangeCode posts to the token endpoint via outline transport; maps failures to the typed code', async () => { const { fetchWithRetry } = await import('../transport/fetch-with-retry.js') vi.mocked(fetchWithRetry).mockResolvedValueOnce(okResponse({ access_token: 'tok-abc' })) @@ -124,7 +127,7 @@ describe('createOutlineAuthProvider', () => { const args = vi.mocked(fetchWithRetry).mock.calls[0][0] expect(args.url).toBe('https://wiki.example.com/oauth/token') - const body = new URLSearchParams(args.options.body as string) + const body = new URLSearchParams(args.options?.body as string) expect(Object.fromEntries(body)).toEqual({ grant_type: 'authorization_code', client_id: 'cid-xyz', @@ -133,11 +136,9 @@ describe('createOutlineAuthProvider', () => { code: 'auth-code', }) - vi.mocked(fetchWithRetry).mockResolvedValueOnce({ - ok: false, - statusText: 'Bad Request', - json: async () => ({ error_description: 'Authorization code expired' }), - } as Response) + vi.mocked(fetchWithRetry).mockResolvedValueOnce( + errResponse(400, 'Bad Request', { error_description: 'Authorization code expired' }), + ) await expect( provider.exchangeCode({ code: 'c', @@ -145,22 +146,29 @@ describe('createOutlineAuthProvider', () => { redirectUri: 'http://localhost:54969/callback', handshake, }), - ).rejects.toThrow('OAuth token exchange failed: Authorization code expired') + ).rejects.toMatchObject({ code: 'AUTH_TOKEN_EXCHANGE_FAILED' }) }) - it('validateToken calls auth.info with the unsaved token and builds an OutlineAccount', async () => { + it('validateToken builds an OutlineAccount using the base URL captured at authorize', async () => { const { apiRequest } = await import('../lib/api.js') - vi.mocked(apiRequest).mockResolvedValue({ - data: { - user: { id: 'user-uuid', name: 'Ada Lovelace', email: 'ada@example.com' }, - team: { name: 'Analytics', subdomain: 'analytics' }, - }, - }) + vi.mocked(apiRequest).mockResolvedValue({ data: AUTH_INFO }) const { createOutlineAuthProvider } = await import('../lib/auth-provider.js') - const account = await createOutlineAuthProvider().validateToken({ + const provider = createOutlineAuthProvider() + // authorize seeds the provider's base-URL closure (from the flag), + // which validate then reuses instead of a handshake field. + await provider.authorize({ + redirectUri: 'http://localhost:54969/callback', + state: 's', + scopes: [], + readOnly: false, + flags: { baseUrl: 'https://wiki.example.com/', clientId: 'cid-xyz' }, + handshake: {}, + }) + + const account = await provider.validateToken({ token: 'tok-abc', - handshake: { baseUrl: 'https://wiki.example.com', clientId: 'cid-xyz' }, + handshake: { clientId: 'cid-xyz' }, }) expect(account).toEqual({ @@ -176,6 +184,43 @@ describe('createOutlineAuthProvider', () => { { token: 'tok-abc', baseUrl: 'https://wiki.example.com' }, ) }) + + it('refreshToken rotates the bundle, resolving base URL + client id from stored config (no prompt)', async () => { + // A fresh provider used only for silent refresh never runs authorize, + // so the resolvers fall back to the logged-in record — never prompting. + configMocks.getConfig.mockResolvedValue({ + users: [ + { + id: 'user-uuid', + name: 'Ada', + base_url: 'https://wiki.example.com', + oauth_client_id: 'cid-xyz', + }, + ], + default_user_id: 'user-uuid', + }) + const { fetchWithRetry } = await import('../transport/fetch-with-retry.js') + vi.mocked(fetchWithRetry).mockResolvedValue( + Response.json({ + access_token: 'tok-new', + refresh_token: 'r-new', + expires_in: 3600, + token_type: 'bearer', + }), + ) + + const { createOutlineAuthProvider } = await import('../lib/auth-provider.js') + const result = await createOutlineAuthProvider().refreshToken?.({ + refreshToken: 'r-old', + handshake: {}, + }) + + expect(result?.accessToken).toBe('tok-new') + expect(result?.refreshToken).toBe('r-new') + const called = vi.mocked(fetchWithRetry).mock.calls[0][0] + const calledUrl = called.url instanceof Request ? called.url.url : String(called.url) + expect(calledUrl).toBe('https://wiki.example.com/oauth/token') + }) }) describe('createOutlineTokenStore', () => { @@ -184,7 +229,9 @@ describe('createOutlineTokenStore', () => { delete process.env.OUTLINE_URL keyringMocks.createKeyringTokenStore.mockClear() keyringMocks.inner.active.mockReset().mockResolvedValue(null) + keyringMocks.inner.activeBundle.mockReset().mockResolvedValue(null) keyringMocks.inner.set.mockReset().mockResolvedValue(undefined) + keyringMocks.inner.setBundle.mockReset().mockResolvedValue(undefined) keyringMocks.inner.clear.mockReset().mockResolvedValue(undefined) keyringMocks.inner.list.mockReset().mockResolvedValue([]) keyringMocks.inner.setDefault.mockReset().mockResolvedValue(undefined) @@ -397,6 +444,46 @@ describe('createOutlineTokenStore', () => { expect(callOrder).toEqual(['migrate', 'set']) }) + + it('activeBundle env short-circuit returns an access-only bundle and bypasses inner', async () => { + vi.stubEnv(TOKEN_ENV_VAR, 'env_token_value') + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + + const snapshot = await createOutlineTokenStore().activeBundle() + + expect(snapshot?.bundle).toEqual({ accessToken: 'env_token_value' }) + expect(keyringMocks.inner.activeBundle).not.toHaveBeenCalled() + }) + + it('activeBundle forwards to the v2 store, preserving refresh state', async () => { + keyringMocks.inner.activeBundle.mockResolvedValue({ + account: STORED_ACCOUNT, + bundle: { accessToken: 'a', refreshToken: 'r', accessTokenExpiresAt: 123 }, + }) + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + + const snapshot = await createOutlineTokenStore().activeBundle() + + expect(snapshot?.bundle).toEqual({ + accessToken: 'a', + refreshToken: 'r', + accessTokenExpiresAt: 123, + }) + expect(keyringMocks.inner.activeBundle).toHaveBeenCalledTimes(1) + }) + + it('setBundle forwards to inner and discharges legacy state when migration is inconclusive', async () => { + migrateMocks.runMigrateLegacyAuth.mockResolvedValue(SKIPPED_RESULT) + const bundle = { accessToken: 'a', refreshToken: 'r' } + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + + await createOutlineTokenStore().setBundle(STORED_ACCOUNT, bundle, { promoteDefault: true }) + + expect(keyringMocks.inner.setBundle).toHaveBeenCalledWith(STORED_ACCOUNT, bundle, { + promoteDefault: true, + }) + expect(configMocks.updateConfig).toHaveBeenCalledWith(LEGACY_CLEAR_PAYLOAD) + }) }) describe('matchOutlineAccount', () => { diff --git a/src/__tests__/auth.test.ts b/src/__tests__/auth.test.ts index c4dea1d..43f04a2 100644 --- a/src/__tests__/auth.test.ts +++ b/src/__tests__/auth.test.ts @@ -123,4 +123,35 @@ describe('auth', () => { const { getOAuthClientId } = await import('../lib/auth.js') await expect(getOAuthClientId()).resolves.toBe('cid-record') }) + + it('reactiveRefresh maps an unrefreshable token to NoTokenError (prompts re-login)', async () => { + // A stored access token with no refresh token can't be rotated, so the + // real refreshAccessToken throws AUTH_REFRESH_UNAVAILABLE — which the + // helper surfaces as a re-login prompt rather than a raw refresh error. + writeFileSync( + TEST_CONFIG_PATH, + JSON.stringify({ + config_version: 2, + users: [ + { + id: 'u', + name: 'Ada', + base_url: 'https://wiki.example.com', + oauth_client_id: 'cid', + token: 'plain-access', + }, + ], + default_user_id: 'u', + }), + ) + const { reactiveRefresh } = await import('../lib/auth.js') + + let caught: unknown + try { + await reactiveRefresh() + } catch (e) { + caught = e + } + expect((caught as { code?: string }).code).toBe('NO_TOKEN') + }) }) diff --git a/src/__tests__/user-records.test.ts b/src/__tests__/user-records.test.ts index acce45d..2a589f3 100644 --- a/src/__tests__/user-records.test.ts +++ b/src/__tests__/user-records.test.ts @@ -98,6 +98,41 @@ describe('createOutlineUserRecordStore', () => { }) }) + it('round-trips the bundle metadata (expiry + refresh flag + refresh fallback)', async () => { + // Regression: these fields were dropped by the StoredUser mapping, so + // proactive refresh never knew the token's expiry and a keyring-down + // refresh token was lost. + const { createOutlineUserRecordStore } = await import('../lib/user-records.js') + + await createOutlineUserRecordStore().upsert({ + account: STORED_ACCOUNT, + accessTokenExpiresAt: 1_700_000_000_000, + refreshTokenExpiresAt: 1_800_000_000_000, + hasRefreshToken: true, + fallbackRefreshToken: 'plain-refresh', + }) + + const stored = { + ...ADA, + refresh_token: 'plain-refresh', + access_token_expires_at: 1_700_000_000_000, + refresh_token_expires_at: 1_800_000_000_000, + has_refresh_token: true, + } + expect(configMock.updateConfig).toHaveBeenCalledWith({ users: [stored] }) + + // ...and back out through list() + configMock.getConfig.mockResolvedValue({ users: [stored] }) + const [record] = await createOutlineUserRecordStore().list() + expect(record).toEqual({ + account: STORED_ACCOUNT, + accessTokenExpiresAt: 1_700_000_000_000, + refreshTokenExpiresAt: 1_800_000_000_000, + hasRefreshToken: true, + fallbackRefreshToken: 'plain-refresh', + }) + }) + it('remove() drops the matching record', async () => { configMock.getConfig.mockResolvedValue({ users: [ADA, GRACE] }) const { createOutlineUserRecordStore } = await import('../lib/user-records.js') diff --git a/src/commands/auth.ts b/src/commands/auth.ts index a2dc509..37824e3 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -104,14 +104,17 @@ export function registerAuthCommand(program: Command): void { attachStatusCommand(auth, { store, description: 'Show current authentication state', - async fetchLive({ token, account }) { + async fetchLive({ account }) { try { const [{ data: info }, source] = await Promise.all([ - apiRequest( - 'auth.info', - {}, - { token, baseUrl: account.baseUrl }, - ), + // No token override → the managed request path, so an + // expired-but-refreshable token is rotated before the + // check rather than reported as expired. (Outline access + // tokens last ~an hour; without this, `auth status` can't + // self-heal even though normal commands do.) A `--user` + // targeting a non-default account resolves the default + // here — multi-account status is a separate concern. + apiRequest('auth.info', {}, { baseUrl: account.baseUrl }), getActiveTokenSource(), ]) statusData = { email: info.user.email, source } diff --git a/src/lib/api.ts b/src/lib/api.ts index 833f2a4..4afc274 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -1,5 +1,6 @@ import { fetchWithRetry } from '../transport/fetch-with-retry.js' -import { getApiToken, getBaseUrl } from './auth.js' +import { TOKEN_ENV_VAR } from './auth-constants.js' +import { getApiToken, getBaseUrl, proactiveRefresh, reactiveRefresh } from './auth.js' import { type SpinnerOptions, withSpinner } from './spinner.js' /** @@ -60,22 +61,37 @@ async function rawApiRequest( body: object = {}, overrides: ApiRequestOverrides = {}, ): Promise> { - const [resolvedBaseUrl, resolvedToken] = await Promise.all([ - overrides.baseUrl ? Promise.resolve(overrides.baseUrl.replace(/\/$/, '')) : getBaseUrl(), - overrides.token ? Promise.resolve(overrides.token) : getApiToken(), - ]) - - const res = await fetchWithRetry({ - url: `${resolvedBaseUrl}/api/${path}`, - options: { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - Authorization: `Bearer ${resolvedToken}`, + // Only stored credentials can be refreshed: a caller-supplied token + // override or the `OUTLINE_API_TOKEN` env var is taken as-is. + const managed = !overrides.token && !process.env[TOKEN_ENV_VAR]?.trim() + + if (managed) await proactiveRefresh() + + const resolvedBaseUrl = overrides.baseUrl + ? overrides.baseUrl.replace(/\/$/, '') + : await getBaseUrl() + + const performRequest = (token: string) => + fetchWithRetry({ + url: `${resolvedBaseUrl}/api/${path}`, + options: { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Authorization: `Bearer ${token}`, + }, + body: JSON.stringify(body), }, - body: JSON.stringify(body), - }, - }) + }) + + let res = await performRequest(overrides.token ?? (await getApiToken())) + + // Reactive path: a 401 on a managed token triggers a forced rotation and + // a single retry. `reactiveRefresh` throws `NoTokenError` when the refresh + // token is gone, so an unrecoverable 401 surfaces the re-login hint. + if (res.status === 401 && managed && (await reactiveRefresh())) { + res = await performRequest(await getApiToken()) + } if (!res.ok) { let message = `API error: ${res.status} ${res.statusText}` diff --git a/src/lib/auth-provider.ts b/src/lib/auth-provider.ts index 24045ad..748252b 100644 --- a/src/lib/auth-provider.ts +++ b/src/lib/auth-provider.ts @@ -1,12 +1,13 @@ import { createInterface } from 'node:readline/promises' import { type AccountRef, + type ActiveBundleSnapshot, type AuthProvider, createKeyringTokenStore, - deriveChallenge, - generateVerifier, + createPkceProvider, type KeyringTokenStore, type MigrateAuthResult, + type TokenBundle, } from '@doist/cli-core/auth' import { fetchWithRetry } from '../transport/fetch-with-retry.js' import { apiRequest } from './api.js' @@ -26,16 +27,6 @@ export type AuthInfoResponse = { export type OutlineTokenStore = KeyringTokenStore -type OutlineHandshake = Record & { - baseUrl: string - clientId: string - codeVerifier?: string -} - -function asHandshake(value: Record): OutlineHandshake { - return value as OutlineHandshake -} - function stringFlag(flags: Record, key: string): string | undefined { const value = flags[key] return typeof value === 'string' && value.trim() ? value.trim() : undefined @@ -75,88 +66,52 @@ async function resolveClientId(flags: Record): Promise return answered } -export function createOutlineAuthProvider(): AuthProvider { - return { - async authorize({ redirectUri, state, flags }) { - const baseUrl = await resolveBaseUrl(flags) - const clientId = await resolveClientId(flags) - const codeVerifier = generateVerifier() - const codeChallenge = deriveChallenge(codeVerifier) - - const url = new URL(`${baseUrl}/oauth/authorize`) - url.searchParams.set('client_id', clientId) - url.searchParams.set('response_type', 'code') - url.searchParams.set('code_challenge', codeChallenge) - url.searchParams.set('code_challenge_method', 'S256') - url.searchParams.set('redirect_uri', redirectUri) - url.searchParams.set('state', state) +/** + * Routes cli-core's OAuth HTTP through outline's transport so the proxy / + * decompression dispatcher applies — to the token exchange AND the + * oauth4webapi refresh grant (cli-core threads this into oauth4webapi's + * `customFetch`), which would otherwise capture the bare global `fetch`. + */ +const outlineFetch: typeof fetch = (input, init) => + fetchWithRetry({ url: input as RequestInfo | URL, options: init ?? {} }) - const handshake: OutlineHandshake = { baseUrl, clientId, codeVerifier } - return { authorizeUrl: url.toString(), handshake } +export function createOutlineAuthProvider(): AuthProvider { + // Captured at `authorize` (which may prompt for it) and reused by + // `exchangeCode` / `validate` so a single login can't double-prompt. A + // provider built only for silent refresh never runs `authorize`, so this + // stays undefined there and `tokenUrl` falls back to stored config — the + // refresh path must never prompt. + let baseUrl: string | undefined + return createPkceProvider({ + authorizeUrl: async ({ flags }) => { + baseUrl = await resolveBaseUrl(flags) + return `${baseUrl}/oauth/authorize` }, - - async exchangeCode({ code, redirectUri, handshake }) { - const hs = asHandshake(handshake) - if (!hs.codeVerifier) { - throw new Error('Missing PKCE code verifier from authorize step.') - } - - const params = new URLSearchParams({ - grant_type: 'authorization_code', - client_id: hs.clientId, - redirect_uri: redirectUri, - code_verifier: hs.codeVerifier, - code, - }) - - const res = await fetchWithRetry({ - url: `${hs.baseUrl}/oauth/token`, - options: { - method: 'POST', - headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, - body: params.toString(), - }, - }) - - const json = (await res.json().catch(() => ({}))) as { - access_token?: string - error?: string - error_description?: string - message?: string - } - - if (!res.ok) { - const message = - json.error_description || json.message || json.error || res.statusText - throw new Error(`OAuth token exchange failed: ${message}`) - } - - if (!json.access_token) { - throw new Error('OAuth token exchange did not return an access token.') - } - - return { accessToken: json.access_token } + tokenUrl: async ({ handshake }) => { + const base = + baseUrl ?? (handshake.baseUrl as string | undefined) ?? (await getBaseUrl()) + return `${base}/oauth/token` }, - - async validateToken({ token, handshake }) { - const hs = asHandshake(handshake) + // `resolveClientId` only prompts when there's no flag AND no stored id, + // so it's safe on the refresh path (the logged-in record carries one). + clientId: ({ flags }) => resolveClientId(flags), + validate: async ({ token, handshake }) => { + const base = baseUrl ?? (await getBaseUrl()) const { data } = await apiRequest( 'auth.info', {}, - { - token, - baseUrl: hs.baseUrl, - }, + { token, baseUrl: base }, ) return makeOutlineAccount({ id: data.user.id, label: data.user.name, - baseUrl: hs.baseUrl, - oauthClientId: hs.clientId, + baseUrl: base, + oauthClientId: handshake.clientId as string, teamName: data.team.name, }) }, - } + fetchImpl: outlineFetch, + }) } /** @@ -298,6 +253,33 @@ export function createOutlineTokenStore(): OutlineTokenStore { } return null }, + async activeBundle(ref?: AccountRef): Promise | null> { + // Env and legacy tokens carry no refresh material, so they surface + // as access-only bundles — the refresh helper then reports + // `AUTH_REFRESH_UNAVAILABLE`, which is the correct answer for them. + if (ref === undefined) { + const envToken = process.env[TOKEN_ENV_VAR]?.trim() + if (envToken) { + return { + account: makeOutlineAccount({ + id: '', + label: '', + baseUrl: await getBaseUrl(), + }), + bundle: { accessToken: envToken }, + } + } + } + await ensureMigrated() + const fromStore = await inner.activeBundle(ref) + if (fromStore) return fromStore + + const legacy = await readLegacyTokenSnapshot() + if (legacy && (ref === undefined || matchOutlineAccount(legacy.account, ref))) { + return { account: legacy.account, bundle: { accessToken: legacy.token } } + } + return null + }, async set(account: OutlineAccount, token: string) { await ensureMigrated() await inner.set(account, token) @@ -307,6 +289,13 @@ export function createOutlineTokenStore(): OutlineTokenStore { await dischargeLegacyState().catch(() => undefined) } }, + async setBundle(account: OutlineAccount, bundle: TokenBundle, options) { + await ensureMigrated() + await inner.setBundle(account, bundle, options) + if (await migrationIsInconclusive()) { + await dischargeLegacyState().catch(() => undefined) + } + }, async clear(ref?: AccountRef) { await ensureMigrated() await inner.clear(ref) diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 6a40df0..65bff5a 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -1,11 +1,12 @@ -import { SecureStoreUnavailableError } from '@doist/cli-core/auth' +import { refreshAccessToken, SecureStoreUnavailableError } from '@doist/cli-core/auth' import { TOKEN_ENV_VAR } from './auth-constants.js' import { + createOutlineAuthProvider, createOutlineTokenStore, getActiveTokenSource, type OutlineTokenStore, } from './auth-provider.js' -import { getConfig } from './config.js' +import { getConfig, getConfigPath } from './config.js' import { CliError } from './errors.js' import { DEFAULT_BASE_URL } from './outline-account.js' import { getDefaultUserRecord } from './user-records.js' @@ -35,6 +36,62 @@ function tokenStore(): OutlineTokenStore { return storeSingleton } +let providerSingleton: ReturnType | undefined +function authProvider(): ReturnType { + if (!providerSingleton) providerSingleton = createOutlineAuthProvider() + return providerSingleton +} + +// Caller-provided O_EXCL lock so concurrent `ol` invocations don't issue +// parallel refresh grants. Resolved per-call (not cached) to honour test +// config-path mocking, mirroring `getConfigPath`. +function refreshLockPath(): string { + return `${getConfigPath()}.refresh.lock` +} + +/** + * Best-effort proactive rotation before a request. Swallows every failure — + * the request and its 401 path are authoritative, so a transient or + * not-refreshable outcome here must not block an otherwise-valid token. + */ +export async function proactiveRefresh(): Promise { + try { + await refreshAccessToken({ + store: tokenStore(), + provider: authProvider(), + lockPath: refreshLockPath(), + }) + } catch { + // reactive 401 path owns the authoritative outcome + } +} + +/** + * Reactive rotation after a 401. Returns `true` when the token rotated (the + * caller retries once). A rejected/absent refresh token surfaces as + * `NoTokenError` (re-login); a transient failure propagates unchanged. + */ +export async function reactiveRefresh(): Promise { + try { + const result = await refreshAccessToken({ + store: tokenStore(), + provider: authProvider(), + lockPath: refreshLockPath(), + force: true, + }) + return result.rotated + } catch (err) { + // Match on the structural `.code` rather than `instanceof`: cli-core + // throws its own CliError, and class identity isn't reliable across + // package boundaries (duplicate module instances under linking). + const code = (err as { code?: unknown } | null)?.code + if (code === 'AUTH_REFRESH_EXPIRED' || code === 'AUTH_REFRESH_UNAVAILABLE') { + throw new NoTokenError() + } + throw err + } +} + /** * Read the active token. Hot path: when `OUTLINE_API_TOKEN` is set we * return it directly without consulting the token store, since diff --git a/src/lib/config.ts b/src/lib/config.ts index 46ce074..32467b5 100644 --- a/src/lib/config.ts +++ b/src/lib/config.ts @@ -19,7 +19,16 @@ export type StoredUser = { base_url?: string oauth_client_id?: string team_name?: string + /** Plaintext access-token fallback, present only when the keyring was unavailable at write time. */ token?: string + /** Plaintext refresh-token fallback, same lifecycle as `token`. */ + refresh_token?: string + /** Access-token expiry (unix-epoch ms); drives proactive refresh. */ + access_token_expires_at?: number + /** Refresh-token expiry (unix-epoch ms), when the server returns one. */ + refresh_token_expires_at?: number + /** Whether a refresh token is stored (keyring slot or `refresh_token`). */ + has_refresh_token?: boolean } export type Config = CoreConfig & { diff --git a/src/lib/user-records.ts b/src/lib/user-records.ts index 4e7f7c9..7b114a0 100644 --- a/src/lib/user-records.ts +++ b/src/lib/user-records.ts @@ -64,16 +64,24 @@ function toRecord(user: StoredUser): UserRecord { oauthClientId: user.oauth_client_id, teamName: user.team_name, }) - const trimmed = user.token?.trim() const record: UserRecord = { account } - if (trimmed) record.fallbackToken = trimmed + const token = user.token?.trim() + if (token) record.fallbackToken = token + const refresh = user.refresh_token?.trim() + if (refresh) record.fallbackRefreshToken = refresh + if (user.access_token_expires_at !== undefined) { + record.accessTokenExpiresAt = user.access_token_expires_at + } + if (user.refresh_token_expires_at !== undefined) { + record.refreshTokenExpiresAt = user.refresh_token_expires_at + } + if (user.has_refresh_token !== undefined) record.hasRefreshToken = user.has_refresh_token return record } function fromRecord(record: UserRecord): StoredUser { - // Replace, don't merge: an absent `fallbackToken` strips the plaintext - // slot so it can't shadow a fresh keyring-backed write. cli-core contract. - const trimmed = record.fallbackToken?.trim() + // Replace, don't merge: absent fields strip the corresponding slots so a + // stale value can't shadow a fresh keyring-backed write. cli-core contract. const next: StoredUser = { id: record.account.id, name: record.account.label, @@ -81,6 +89,16 @@ function fromRecord(record: UserRecord): StoredUser { if (record.account.baseUrl) next.base_url = record.account.baseUrl if (record.account.oauthClientId) next.oauth_client_id = record.account.oauthClientId if (record.account.teamName) next.team_name = record.account.teamName - if (trimmed && trimmed.length > 0) next.token = trimmed + const token = record.fallbackToken?.trim() + if (token) next.token = token + const refresh = record.fallbackRefreshToken?.trim() + if (refresh) next.refresh_token = refresh + if (record.accessTokenExpiresAt !== undefined) { + next.access_token_expires_at = record.accessTokenExpiresAt + } + if (record.refreshTokenExpiresAt !== undefined) { + next.refresh_token_expires_at = record.refreshTokenExpiresAt + } + if (record.hasRefreshToken !== undefined) next.has_refresh_token = record.hasRefreshToken return next } From 376f54da2a335308e932dd4143b248b9f673e579 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Thu, 21 May 2026 11:29:45 +0100 Subject: [PATCH 2/2] =?UTF-8?q?fix(auth):=20address=20PR=20review=20?= =?UTF-8?q?=E2=80=94=20secrets,=20account-aware=20status,=20dedup?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security: - Never persist the refresh token to config in plaintext. It's a long-lived credential, so it stays in the secure store only; when the keyring is offline the account fails closed (re-auth on next expiry) rather than writing the token at rest. (Doist secrets standard.) Correctness: - `auth status` now refreshes the *selected* account (scoped via its id + base URL/client id through the refresh handshake), not the default — `--user ` checks/rotates the right account at the right instance. The PKCE provider's clientId resolver honours `handshake.clientId`. Hot path: - Restore parallel base-URL + token resolution; `proactiveRefresh` returns the token it resolved so unrefreshable/access-only accounts stay on a single store read (no redundant getApiToken). Dedup: - Extract `resolveAuth` (env→v2→legacy) shared by active/activeBundle and `writeThenDischargeLegacy` shared by set/setBundle. Tests: positive proactive-refresh path; public-client refresh form body (no client_secret); refresh-token-never-in-plaintext; auth.test back to rejects.toMatchObject. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/__tests__/api.test.ts | 16 ++++ src/__tests__/auth-command.test.ts | 9 +- src/__tests__/auth-provider.test.ts | 9 ++ src/__tests__/user-records.test.ts | 18 ++-- src/commands/auth.ts | 23 +++-- src/lib/api.ts | 27 ++++-- src/lib/auth-provider.ts | 135 ++++++++++++++-------------- src/lib/auth.ts | 43 +++++++-- src/lib/config.ts | 2 - src/lib/user-records.ts | 9 +- 10 files changed, 186 insertions(+), 105 deletions(-) diff --git a/src/__tests__/api.test.ts b/src/__tests__/api.test.ts index 4389d86..5dcd8a5 100644 --- a/src/__tests__/api.test.ts +++ b/src/__tests__/api.test.ts @@ -120,6 +120,22 @@ describe('apiRequest', () => { expect(retryHeaders.Authorization).toBe('Bearer rotated-token') }) + it('proactively refreshes a managed token and uses the rotated token (no extra store read)', async () => { + const { fetchWithRetry } = await import('../transport/fetch-with-retry.js') + const f = fetchWithRetry as ReturnType + f.mockResolvedValue({ ok: true, json: async () => ({ data: {} }) }) + authMocks.proactiveRefresh.mockResolvedValueOnce('rotated-proactive') + + const { apiRequest } = await import('../lib/api.js') + await apiRequest('documents.list') + + expect(authMocks.proactiveRefresh).toHaveBeenCalledTimes(1) + // The proactive token is reused — no fallback read of getApiToken. + expect(authMocks.getApiToken).not.toHaveBeenCalled() + const headers = f.mock.calls[0][0].options.headers as Record + expect(headers.Authorization).toBe('Bearer rotated-proactive') + }) + it('does not refresh when OUTLINE_API_TOKEN is set (unmanaged token)', async () => { vi.stubEnv('OUTLINE_API_TOKEN', 'env-tok') const { fetchWithRetry } = await import('../transport/fetch-with-retry.js') diff --git a/src/__tests__/auth-command.test.ts b/src/__tests__/auth-command.test.ts index 83d8a2f..35b17d7 100644 --- a/src/__tests__/auth-command.test.ts +++ b/src/__tests__/auth-command.test.ts @@ -8,6 +8,9 @@ vi.mock('../lib/auth.js', () => ({ getOAuthClientId: async () => undefined, getActiveTokenSource: async () => process.env.OUTLINE_API_TOKEN ? ('env' as const) : ('secure-store' as const), + // status resolves the live token for the selected account via this; echo + // the snapshot token back (no refresh in these command-surface tests). + refreshedTokenForStatus: async (_account: unknown, fallback: string) => fallback, })) vi.mock('../lib/api.js', () => ({ apiRequest: vi.fn() })) @@ -118,12 +121,12 @@ describe('auth status subcommand', () => { const program = await buildProgram() await program.parseAsync(['node', 'ol', 'auth', 'status']) - // status routes through the managed request path (no token override) - // so an expired-but-refreshable token is rotated before the check. + // status probes auth.info with the (account-scoped) live token resolved + // for the selected account. expect(apiRequest).toHaveBeenCalledWith( 'auth.info', {}, - { baseUrl: 'https://test.outline.com' }, + { token: 'env-token', baseUrl: 'https://test.outline.com' }, ) expect(logs.some((l) => l.includes('Authenticated'))).toBe(true) expect(logs.some((l) => l.includes('Team:') && l.includes('Analytics'))).toBe(true) diff --git a/src/__tests__/auth-provider.test.ts b/src/__tests__/auth-provider.test.ts index 6100e8b..863992c 100644 --- a/src/__tests__/auth-provider.test.ts +++ b/src/__tests__/auth-provider.test.ts @@ -200,6 +200,7 @@ describe('createOutlineAuthProvider', () => { default_user_id: 'user-uuid', }) const { fetchWithRetry } = await import('../transport/fetch-with-retry.js') + vi.mocked(fetchWithRetry).mockClear() // isolate this test's POST from earlier ones vi.mocked(fetchWithRetry).mockResolvedValue( Response.json({ access_token: 'tok-new', @@ -220,6 +221,14 @@ describe('createOutlineAuthProvider', () => { const called = vi.mocked(fetchWithRetry).mock.calls[0][0] const calledUrl = called.url instanceof Request ? called.url.url : String(called.url) expect(calledUrl).toBe('https://wiki.example.com/oauth/token') + + // Public-client form body: a regression to confidential-client refresh + // (sending a client_secret) must fail here. + const body = new URLSearchParams(called.options?.body as string) + expect(body.get('grant_type')).toBe('refresh_token') + expect(body.get('refresh_token')).toBe('r-old') + expect(body.get('client_id')).toBe('cid-xyz') + expect(body.has('client_secret')).toBe(false) }) }) diff --git a/src/__tests__/user-records.test.ts b/src/__tests__/user-records.test.ts index 2a589f3..01a61c8 100644 --- a/src/__tests__/user-records.test.ts +++ b/src/__tests__/user-records.test.ts @@ -98,10 +98,11 @@ describe('createOutlineUserRecordStore', () => { }) }) - it('round-trips the bundle metadata (expiry + refresh flag + refresh fallback)', async () => { - // Regression: these fields were dropped by the StoredUser mapping, so - // proactive refresh never knew the token's expiry and a keyring-down - // refresh token was lost. + it('round-trips expiry + refresh flag, but never persists the refresh token in plaintext', async () => { + // The expiry/flag metadata must round-trip (proactive refresh needs the + // expiry), but the refresh token is a long-lived secret and must stay + // in the secure store only — `fallbackRefreshToken` is dropped, never + // written to config, even when cli-core supplies it (keyring offline). const { createOutlineUserRecordStore } = await import('../lib/user-records.js') await createOutlineUserRecordStore().upsert({ @@ -114,14 +115,15 @@ describe('createOutlineUserRecordStore', () => { const stored = { ...ADA, - refresh_token: 'plain-refresh', access_token_expires_at: 1_700_000_000_000, refresh_token_expires_at: 1_800_000_000_000, has_refresh_token: true, } - expect(configMock.updateConfig).toHaveBeenCalledWith({ users: [stored] }) + const [[written]] = configMock.updateConfig.mock.calls + expect(written).toEqual({ users: [stored] }) + expect(written.users[0]).not.toHaveProperty('refresh_token') - // ...and back out through list() + // ...and back out through list() — still no refresh material. configMock.getConfig.mockResolvedValue({ users: [stored] }) const [record] = await createOutlineUserRecordStore().list() expect(record).toEqual({ @@ -129,8 +131,8 @@ describe('createOutlineUserRecordStore', () => { accessTokenExpiresAt: 1_700_000_000_000, refreshTokenExpiresAt: 1_800_000_000_000, hasRefreshToken: true, - fallbackRefreshToken: 'plain-refresh', }) + expect(record.fallbackRefreshToken).toBeUndefined() }) it('remove() drops the matching record', async () => { diff --git a/src/commands/auth.ts b/src/commands/auth.ts index 37824e3..9f327a7 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -16,6 +16,7 @@ import { type OutlineAccount, type OutlineTokenStore, } from '../lib/auth-provider.js' +import { refreshedTokenForStatus } from '../lib/auth.js' import { CliError } from '../lib/errors.js' const DEFAULT_OAUTH_CALLBACK_PORT = 54969 @@ -104,17 +105,21 @@ export function registerAuthCommand(program: Command): void { attachStatusCommand(auth, { store, description: 'Show current authentication state', - async fetchLive({ account }) { + async fetchLive({ account, token }) { try { + // Refresh the *selected* account before the check (scoped via + // its id + base URL/client id), then probe with the rotated + // token. Outline access tokens last ~an hour; without this + // `auth status` can't self-heal even though normal commands + // do. Passing the resolved token keeps the check tied to the + // requested account rather than the default. + const liveToken = await refreshedTokenForStatus(account, token) const [{ data: info }, source] = await Promise.all([ - // No token override → the managed request path, so an - // expired-but-refreshable token is rotated before the - // check rather than reported as expired. (Outline access - // tokens last ~an hour; without this, `auth status` can't - // self-heal even though normal commands do.) A `--user` - // targeting a non-default account resolves the default - // here — multi-account status is a separate concern. - apiRequest('auth.info', {}, { baseUrl: account.baseUrl }), + apiRequest( + 'auth.info', + {}, + { token: liveToken, baseUrl: account.baseUrl }, + ), getActiveTokenSource(), ]) statusData = { email: info.user.email, source } diff --git a/src/lib/api.ts b/src/lib/api.ts index 4afc274..675ee50 100644 --- a/src/lib/api.ts +++ b/src/lib/api.ts @@ -53,6 +53,21 @@ export type ApiRequestOverrides = { baseUrl?: string } +/** + * Resolve the token for a request. A caller-supplied override is used as-is. + * On the managed path, prefer the token `proactiveRefresh` resolved (rotated + * or current) so unrefreshable/access-only accounts stay on a single store + * read; only fall back to `getApiToken` when proactive refresh bows out. + */ +async function resolveRequestToken(managed: boolean, override?: string): Promise { + if (override) return override + if (managed) { + const refreshed = await proactiveRefresh() + if (refreshed) return refreshed + } + return getApiToken() +} + /** * Core API request function without spinner wrapping. */ @@ -65,11 +80,11 @@ async function rawApiRequest( // override or the `OUTLINE_API_TOKEN` env var is taken as-is. const managed = !overrides.token && !process.env[TOKEN_ENV_VAR]?.trim() - if (managed) await proactiveRefresh() - - const resolvedBaseUrl = overrides.baseUrl - ? overrides.baseUrl.replace(/\/$/, '') - : await getBaseUrl() + // Resolve the base URL and the (proactively refreshed) token in parallel. + const [resolvedBaseUrl, resolvedToken] = await Promise.all([ + overrides.baseUrl ? Promise.resolve(overrides.baseUrl.replace(/\/$/, '')) : getBaseUrl(), + resolveRequestToken(managed, overrides.token), + ]) const performRequest = (token: string) => fetchWithRetry({ @@ -84,7 +99,7 @@ async function rawApiRequest( }, }) - let res = await performRequest(overrides.token ?? (await getApiToken())) + let res = await performRequest(resolvedToken) // Reactive path: a 401 on a managed token triggers a forced rotation and // a single retry. `reactiveRefresh` throws `NoTokenError` when the refresh diff --git a/src/lib/auth-provider.ts b/src/lib/auth-provider.ts index 748252b..f994119 100644 --- a/src/lib/auth-provider.ts +++ b/src/lib/auth-provider.ts @@ -1,7 +1,6 @@ import { createInterface } from 'node:readline/promises' import { type AccountRef, - type ActiveBundleSnapshot, type AuthProvider, createKeyringTokenStore, createPkceProvider, @@ -88,13 +87,22 @@ export function createOutlineAuthProvider(): AuthProvider { return `${baseUrl}/oauth/authorize` }, tokenUrl: async ({ handshake }) => { + // `handshake.baseUrl` lets a caller scope refresh to a specific + // account's instance (status --user); otherwise the authorize-time + // value, then the default config. const base = baseUrl ?? (handshake.baseUrl as string | undefined) ?? (await getBaseUrl()) return `${base}/oauth/token` }, - // `resolveClientId` only prompts when there's no flag AND no stored id, - // so it's safe on the refresh path (the logged-in record carries one). - clientId: ({ flags }) => resolveClientId(flags), + // Prefer a handshake-scoped client id (account-aware refresh); else + // resolve from flag/config (only prompts when neither is set, so it's + // safe on the refresh path — the logged-in record carries one). + clientId: ({ handshake, flags }) => { + const fromHandshake = handshake.clientId + return typeof fromHandshake === 'string' && fromHandshake + ? fromHandshake + : resolveClientId(flags) + }, validate: async ({ token, handshake }) => { const base = baseUrl ?? (await getBaseUrl()) const { data } = await apiRequest( @@ -228,73 +236,68 @@ export function createOutlineTokenStore(): OutlineTokenStore { const result = await ensureMigrated() // memoised return result === null || !migrationIsConclusive(result) } - return { - async active(ref?: AccountRef) { - if (ref === undefined) { - const envToken = process.env[TOKEN_ENV_VAR]?.trim() - if (envToken) { - return { - token: envToken, - account: makeOutlineAccount({ - id: '', - label: '', - baseUrl: await getBaseUrl(), - }), - } - } + /** + * Shared env → v2 → legacy resolution for `active` / `activeBundle`. + * `fromV2` reads the matching cli-core method; `fromEnvOrLegacy` maps an + * env/legacy token into the caller's shape (env + legacy carry no refresh + * material, so the bundle form surfaces as access-only). + */ + async function resolveAuth( + ref: AccountRef | undefined, + fromV2: () => Promise, + fromEnvOrLegacy: (token: string, account: OutlineAccount) => T, + ): Promise { + if (ref === undefined) { + const envToken = process.env[TOKEN_ENV_VAR]?.trim() + if (envToken) { + return fromEnvOrLegacy( + envToken, + makeOutlineAccount({ id: '', label: '', baseUrl: await getBaseUrl() }), + ) } - await ensureMigrated() - const fromStore = await inner.active(ref) - if (fromStore) return fromStore + } + await ensureMigrated() + const fromStore = await fromV2() + if (fromStore) return fromStore + const legacy = await readLegacyTokenSnapshot() + if (legacy && (ref === undefined || matchOutlineAccount(legacy.account, ref))) { + return fromEnvOrLegacy(legacy.token, legacy.account) + } + return null + } - const legacy = await readLegacyTokenSnapshot() - if (legacy && (ref === undefined || matchOutlineAccount(legacy.account, ref))) { - return legacy - } - return null - }, - async activeBundle(ref?: AccountRef): Promise | null> { - // Env and legacy tokens carry no refresh material, so they surface - // as access-only bundles — the refresh helper then reports - // `AUTH_REFRESH_UNAVAILABLE`, which is the correct answer for them. - if (ref === undefined) { - const envToken = process.env[TOKEN_ENV_VAR]?.trim() - if (envToken) { - return { - account: makeOutlineAccount({ - id: '', - label: '', - baseUrl: await getBaseUrl(), - }), - bundle: { accessToken: envToken }, - } - } - } - await ensureMigrated() - const fromStore = await inner.activeBundle(ref) - if (fromStore) return fromStore + // `ensureMigrated` runs before the v2 write so the post-write discharge + // can't race a pending migration into re-grabbing the legacy token. The + // discharge is best-effort: a lingering `api_token` is dormant because + // reads hit v2 first. (`clear` needs the stricter propagating variant.) + async function writeThenDischargeLegacy(write: () => Promise): Promise { + await ensureMigrated() + await write() + if (await migrationIsInconclusive()) { + await dischargeLegacyState().catch(() => undefined) + } + } - const legacy = await readLegacyTokenSnapshot() - if (legacy && (ref === undefined || matchOutlineAccount(legacy.account, ref))) { - return { account: legacy.account, bundle: { accessToken: legacy.token } } - } - return null + return { + active(ref?: AccountRef) { + return resolveAuth( + ref, + () => inner.active(ref), + (token, account) => ({ token, account }), + ) }, - async set(account: OutlineAccount, token: string) { - await ensureMigrated() - await inner.set(account, token) - if (await migrationIsInconclusive()) { - // Best-effort: a lingering `api_token` is dormant because - // `active()` reads v2 first. - await dischargeLegacyState().catch(() => undefined) - } + activeBundle(ref?: AccountRef) { + return resolveAuth( + ref, + () => inner.activeBundle(ref), + (token, account) => ({ account, bundle: { accessToken: token } }), + ) }, - async setBundle(account: OutlineAccount, bundle: TokenBundle, options) { - await ensureMigrated() - await inner.setBundle(account, bundle, options) - if (await migrationIsInconclusive()) { - await dischargeLegacyState().catch(() => undefined) - } + set(account: OutlineAccount, token: string) { + return writeThenDischargeLegacy(() => inner.set(account, token)) + }, + setBundle(account: OutlineAccount, bundle: TokenBundle, options) { + return writeThenDischargeLegacy(() => inner.setBundle(account, bundle, options)) }, async clear(ref?: AccountRef) { await ensureMigrated() diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 65bff5a..e5d51bb 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -8,7 +8,7 @@ import { } from './auth-provider.js' import { getConfig, getConfigPath } from './config.js' import { CliError } from './errors.js' -import { DEFAULT_BASE_URL } from './outline-account.js' +import { DEFAULT_BASE_URL, type OutlineAccount } from './outline-account.js' import { getDefaultUserRecord } from './user-records.js' export { SecureStoreUnavailableError, getActiveTokenSource, TOKEN_ENV_VAR } @@ -50,19 +50,48 @@ function refreshLockPath(): string { } /** - * Best-effort proactive rotation before a request. Swallows every failure — - * the request and its 401 path are authoritative, so a transient or - * not-refreshable outcome here must not block an otherwise-valid token. + * Best-effort proactive rotation before a request. Returns the token to use + * (the rotated one, or the current one if no rotation was needed) so the + * caller doesn't re-read the store; `undefined` when the default account + * isn't refreshable (no refresh token / env / failure) — the caller then + * falls back to `getApiToken()` and the 401 path stays authoritative. */ -export async function proactiveRefresh(): Promise { +export async function proactiveRefresh(): Promise { try { - await refreshAccessToken({ + const { bundle } = await refreshAccessToken({ store: tokenStore(), provider: authProvider(), lockPath: refreshLockPath(), }) + return bundle.accessToken } catch { - // reactive 401 path owns the authoritative outcome + return undefined + } +} + +/** + * Refresh the *selected* account (not the default) for `auth status`, returning + * its current-or-rotated token. Scoped via the account's id + base URL/client + * id through the refresh handshake, so `--user ` checks and rotates the + * right account at the right instance. Env / legacy / record-less accounts + * aren't refreshable — the snapshot `fallback` token is returned as-is. + */ +export async function refreshedTokenForStatus( + account: OutlineAccount, + fallback: string, +): Promise { + if (process.env[TOKEN_ENV_VAR]?.trim() || !account.id) return fallback + try { + const { bundle } = await refreshAccessToken({ + store: tokenStore(), + provider: authProvider(), + lockPath: refreshLockPath(), + ref: account.id, + handshake: { baseUrl: account.baseUrl, clientId: account.oauthClientId }, + }) + return bundle.accessToken + } catch { + return fallback } } diff --git a/src/lib/config.ts b/src/lib/config.ts index 32467b5..27fa402 100644 --- a/src/lib/config.ts +++ b/src/lib/config.ts @@ -21,8 +21,6 @@ export type StoredUser = { team_name?: string /** Plaintext access-token fallback, present only when the keyring was unavailable at write time. */ token?: string - /** Plaintext refresh-token fallback, same lifecycle as `token`. */ - refresh_token?: string /** Access-token expiry (unix-epoch ms); drives proactive refresh. */ access_token_expires_at?: number /** Refresh-token expiry (unix-epoch ms), when the server returns one. */ diff --git a/src/lib/user-records.ts b/src/lib/user-records.ts index 7b114a0..82927ba 100644 --- a/src/lib/user-records.ts +++ b/src/lib/user-records.ts @@ -67,8 +67,6 @@ function toRecord(user: StoredUser): UserRecord { const record: UserRecord = { account } const token = user.token?.trim() if (token) record.fallbackToken = token - const refresh = user.refresh_token?.trim() - if (refresh) record.fallbackRefreshToken = refresh if (user.access_token_expires_at !== undefined) { record.accessTokenExpiresAt = user.access_token_expires_at } @@ -91,8 +89,11 @@ function fromRecord(record: UserRecord): StoredUser { if (record.account.teamName) next.team_name = record.account.teamName const token = record.fallbackToken?.trim() if (token) next.token = token - const refresh = record.fallbackRefreshToken?.trim() - if (refresh) next.refresh_token = refresh + // Deliberately NOT persisting `fallbackRefreshToken`: the refresh token is + // a long-lived credential and must stay in the secure store only. If the + // keyring is offline at write time, the refresh token isn't persisted, so + // that account fails closed (re-auth on next expiry) rather than leaving a + // long-lived secret in plaintext config. (Doist secrets-management standard.) if (record.accessTokenExpiresAt !== undefined) { next.access_token_expires_at = record.accessTokenExpiresAt }