From 8a8660e05526c1272e51cd7594c4d1ff9fd8ef29 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sun, 17 May 2026 16:14:44 +0100 Subject: [PATCH 1/3] feat(auth): store API tokens in the OS keyring via @doist/cli-core MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces outline-cli's bespoke plaintext-config token storage with cli-core's `createKeyringTokenStore` + `createSecureStore` (keytar via `@napi-rs/keyring`). Mirrors the twist-cli architecture so the auth surface stays consistent across Doist CLIs. WSL / headless Linux is handled by cli-core natively: when the OS keyring is unreachable, the token is parked on the user record as a `fallbackToken` plaintext field, login still succeeds, and a stderr warning surfaces via `logTokenStorageResult`. A one-time, lazy migration moves the v1 `api_token` plaintext slot into the v2 `users[]` record on first store access. Migration is memoised per-process and gated by a `config_version` marker so it can never re-run. Failures (offline `identifyAccount`, no keyring) fall back to the legacy snapshot until the next attempt — users are never locked out mid-flight. Co-Authored-By: Claude Opus 4.7 (1M context) --- package-lock.json | 231 +++++++++++++++- package.json | 2 +- src/__tests__/_fixtures/auth.ts | 64 +++++ src/__tests__/auth-command.test.ts | 69 ++--- src/__tests__/auth-provider.test.ts | 402 +++++++++++++++++----------- src/__tests__/auth.test.ts | 60 ++++- src/__tests__/migrate-auth.test.ts | 82 ++++++ src/commands/auth.ts | 73 ++++- src/lib/auth-constants.ts | 10 + src/lib/auth-provider.ts | 273 ++++++++++++------- src/lib/auth.ts | 88 +++--- src/lib/config.ts | 22 ++ src/lib/migrate-auth.ts | 96 +++++++ src/lib/outline-account.ts | 37 +++ src/lib/user-records.ts | 86 ++++++ 15 files changed, 1227 insertions(+), 368 deletions(-) create mode 100644 src/__tests__/_fixtures/auth.ts create mode 100644 src/__tests__/migrate-auth.test.ts create mode 100644 src/lib/auth-constants.ts create mode 100644 src/lib/migrate-auth.ts create mode 100644 src/lib/outline-account.ts create mode 100644 src/lib/user-records.ts diff --git a/package-lock.json b/package-lock.json index 06d0718..c633551 100644 --- a/package-lock.json +++ b/package-lock.json @@ -10,7 +10,7 @@ "hasInstallScript": true, "license": "MIT", "dependencies": { - "@doist/cli-core": "0.12.0", + "@doist/cli-core": "0.16.1", "chalk": "5.6.2", "commander": "14.0.2", "marked": "18.0.3", @@ -132,9 +132,9 @@ } }, "node_modules/@doist/cli-core": { - "version": "0.12.0", - "resolved": "https://registry.npmjs.org/@doist/cli-core/-/cli-core-0.12.0.tgz", - "integrity": "sha512-6dk9mL+HMmkfijcO9t+AtXO+2vxV6Gj5thD91sExNJfInnmQzFhMSYOJ/8u/k5gzpD9wyVAxL+q+bzt8s9eeQw==", + "version": "0.16.1", + "resolved": "https://registry.npmjs.org/@doist/cli-core/-/cli-core-0.16.1.tgz", + "integrity": "sha512-OoHWYM8Rv+eoPhVR1BUMwFIejZGk5Wyxheva4Cp9x3zTh4RURE+OIJ03G95oBegbXCA2O9LRtZZ9Ww0D6vU1dQ==", "license": "MIT", "dependencies": { "chalk": "5.6.2", @@ -143,6 +143,9 @@ "engines": { "node": ">=20.18.1" }, + "optionalDependencies": { + "@napi-rs/keyring": "1.3.0" + }, "peerDependencies": { "commander": ">=14", "marked": ">=18", @@ -639,6 +642,226 @@ "integrity": "sha512-dXn3FZhPv0US+7dtJsIi2R+c7qWYiReoEh5zUntWCf4oSpMNib8FDhSoed6m3QyZdx5hK7iLFkYk3rNxwt8vTA==", "license": "MIT" }, + "node_modules/@napi-rs/keyring": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring/-/keyring-1.3.0.tgz", + "integrity": "sha512-WrOw/bcXm0f9qHkumlT1QlArXSTWqaY9sunsDpOk+yCCorCKMxvWT/a3xko4EYHVdeZoh00yI2TydXn6eyICDA==", + "license": "MIT", + "optional": true, + "engines": { + "node": ">= 10" + }, + "funding": { + "type": "github", + "url": "https://github.com/sponsors/Brooooooklyn" + }, + "optionalDependencies": { + "@napi-rs/keyring-darwin-arm64": "1.3.0", + "@napi-rs/keyring-darwin-x64": "1.3.0", + "@napi-rs/keyring-freebsd-x64": "1.3.0", + "@napi-rs/keyring-linux-arm-gnueabihf": "1.3.0", + "@napi-rs/keyring-linux-arm64-gnu": "1.3.0", + "@napi-rs/keyring-linux-arm64-musl": "1.3.0", + "@napi-rs/keyring-linux-riscv64-gnu": "1.3.0", + "@napi-rs/keyring-linux-x64-gnu": "1.3.0", + "@napi-rs/keyring-linux-x64-musl": "1.3.0", + "@napi-rs/keyring-win32-arm64-msvc": "1.3.0", + "@napi-rs/keyring-win32-ia32-msvc": "1.3.0", + "@napi-rs/keyring-win32-x64-msvc": "1.3.0" + } + }, + "node_modules/@napi-rs/keyring-darwin-arm64": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-darwin-arm64/-/keyring-darwin-arm64-1.3.0.tgz", + "integrity": "sha512-pl76hJvdYUBn6I24bXiOBMA9nbDapo3I5B+f3OorjDU4dUMSypXeKbOVehJe8fhgTiH24flMyTS3aAIy43xegQ==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-darwin-x64": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-darwin-x64/-/keyring-darwin-x64-1.3.0.tgz", + "integrity": "sha512-YcJtEV5LA3cvA4z3BurgxH5IhTsW1JfIvcAAcqcecwk06Si9F9NqkxbZVIfDwQ8oRHgaBmT3zZJnLAotCrVahw==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "darwin" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-freebsd-x64": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-freebsd-x64/-/keyring-freebsd-x64-1.3.0.tgz", + "integrity": "sha512-vlLf31TGhfRAaxLDBhg8b89ss0HHD/lyNmL5F3UjSaz5CUXElsJmKYq9fqA/B+cZKUEUcLHHGhF0I/CqcFdaVw==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "freebsd" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-arm-gnueabihf": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-arm-gnueabihf/-/keyring-linux-arm-gnueabihf-1.3.0.tgz", + "integrity": "sha512-KiWdMMu/Inz/bHHIAGrnF7r54FZDYXuHO6UFF/rhIrshUsxbMG1Rl9lEymNtqqsVo927G0VYcb02FzWQ3iBQRQ==", + "cpu": [ + "arm" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-arm64-gnu": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-arm64-gnu/-/keyring-linux-arm64-gnu-1.3.0.tgz", + "integrity": "sha512-eyKGpY40lm9Jvs1aD294XRH4y7+TlJM0YVAryZeXA6TX0mb4gMkxVXwSQv7MCwgah7raeUd0dKUb4BPAYIgcMg==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-arm64-musl": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-arm64-musl/-/keyring-linux-arm64-musl-1.3.0.tgz", + "integrity": "sha512-iIK6JWHXAJqDrEyLY3TmswwloVyt2vj+04TZnew+uSJ9gnDO8EwRbp3/iw3LpWaXiDO7VomGO6y8I0Id8uBZSw==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-riscv64-gnu": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-riscv64-gnu/-/keyring-linux-riscv64-gnu-1.3.0.tgz", + "integrity": "sha512-/PGqrwn6EwgtK6vccASSXJRfOSP4vN1F4ASsIQ+7MdrK6hNvAJ1FZPrIuD5gGGdxezo3F++To2Wq7DbuGIeuNQ==", + "cpu": [ + "riscv64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-x64-gnu": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-x64-gnu/-/keyring-linux-x64-gnu-1.3.0.tgz", + "integrity": "sha512-2PDK1WKWTu9lBGq9VvNEkSlQD3O7YwVpmnyN2M3cy4v7NJ/8gDMd9GXv3G+FVXN13uhp4gnnPBS+ScefmEeD2A==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-linux-x64-musl": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-linux-x64-musl/-/keyring-linux-x64-musl-1.3.0.tgz", + "integrity": "sha512-oJ2HkX8YUo46QBkn0pG+HuIKQNqr523q6vBobCn+P95s4C4K6/kLBqHY/1bg5J4ap31DzsznhnFKcfBNBsjCnw==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "linux" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-win32-arm64-msvc": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-win32-arm64-msvc/-/keyring-win32-arm64-msvc-1.3.0.tgz", + "integrity": "sha512-tOd3c/uAaeoE4ycVlmAdSvygz0Zt3zdca6Y7gokBeIbaRDWpjDIUOpU3MvML59XAaqyuKGsVVu0F/DZb1lHPmw==", + "cpu": [ + "arm64" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-win32-ia32-msvc": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-win32-ia32-msvc/-/keyring-win32-ia32-msvc-1.3.0.tgz", + "integrity": "sha512-sPSqeAFZMGqP1R++M2JTza7GQJJ/TpCo6JU6Vcd4jnebvOaEDs9b7eipakU1PJdSvhpC2yXMCNRk9gXfrhuwHQ==", + "cpu": [ + "ia32" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, + "node_modules/@napi-rs/keyring-win32-x64-msvc": { + "version": "1.3.0", + "resolved": "https://registry.npmjs.org/@napi-rs/keyring-win32-x64-msvc/-/keyring-win32-x64-msvc-1.3.0.tgz", + "integrity": "sha512-4DnCWXwDc0HRKwyRlG5y0VhKZW2tNRQfKKfyj6IX/KWfDNyq9hn4n+GL1auyDcOO/v8PwnhmYo2+rOOqCkvvOg==", + "cpu": [ + "x64" + ], + "license": "MIT", + "optional": true, + "os": [ + "win32" + ], + "engines": { + "node": ">= 10" + } + }, "node_modules/@napi-rs/wasm-runtime": { "version": "1.1.4", "resolved": "https://registry.npmjs.org/@napi-rs/wasm-runtime/-/wasm-runtime-1.1.4.tgz", diff --git a/package.json b/package.json index 3df1cb5..b5f0cb9 100644 --- a/package.json +++ b/package.json @@ -48,7 +48,7 @@ "node": ">=20.18.1" }, "dependencies": { - "@doist/cli-core": "0.12.0", + "@doist/cli-core": "0.16.1", "chalk": "5.6.2", "commander": "14.0.2", "marked": "18.0.3", diff --git a/src/__tests__/_fixtures/auth.ts b/src/__tests__/_fixtures/auth.ts new file mode 100644 index 0000000..6fee15b --- /dev/null +++ b/src/__tests__/_fixtures/auth.ts @@ -0,0 +1,64 @@ +import type { MigrateAuthResult } from '@doist/cli-core/auth' +import type { OutlineAccount } from '../../lib/outline-account.js' + +/** Canonical persisted `OutlineAccount` used across auth tests. */ +export const STORED_ACCOUNT: OutlineAccount = { + id: 'user-uuid', + label: 'Ada', + baseUrl: 'https://wiki.example.com', + oauthClientId: 'cid-xyz', + teamName: 'Analytics', +} + +/** v1 plaintext config snapshot that round-trips to `STORED_ACCOUNT`. */ +export const LEGACY_CONFIG = { + api_token: 'tk_legacy_plaintext', + base_url: 'https://wiki.example.com', + oauth_client_id: 'cid-xyz', + auth_user_id: 'user-uuid', + auth_user_name: 'Ada', + auth_team_name: 'Analytics', +} as const + +/** `updateConfig` payload that wipes every v1 auth key. */ +export const LEGACY_CLEAR_PAYLOAD = { + api_token: undefined, + base_url: undefined, + oauth_client_id: undefined, + auth_user_id: undefined, + auth_user_name: undefined, + auth_team_name: undefined, +} as const + +/** Outline `auth.info` response body. Richer than `STORED_ACCOUNT` (carries email). */ +export const AUTH_INFO = { + user: { id: 'user-uuid', name: 'Ada Lovelace', email: 'ada@example.com' }, + team: { name: 'Analytics', subdomain: 'analytics' }, +} as const + +/** Stand-in for a cli-core `migrateLegacyAuth` skip result. */ +export const SKIPPED_RESULT: MigrateAuthResult = { + status: 'skipped', + reason: 'identify-failed', + detail: 'offline', +} + +/** Build a `Response`-shaped object whose `json()` resolves to `body`. */ +export function okResponse(body: unknown): Response { + return { + ok: true, + status: 200, + statusText: 'OK', + json: async () => body, + } as Response +} + +/** Build an error `Response` with the given status (default 500). */ +export function errResponse(status: number, statusText = 'Error', body?: unknown): Response { + return { + ok: false, + status, + statusText, + json: async () => body ?? {}, + } as Response +} diff --git a/src/__tests__/auth-command.test.ts b/src/__tests__/auth-command.test.ts index 1b2016f..349a68d 100644 --- a/src/__tests__/auth-command.test.ts +++ b/src/__tests__/auth-command.test.ts @@ -1,12 +1,13 @@ import { Command } from 'commander' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { AUTH_INFO } from './_fixtures/auth.js' vi.mock('../lib/auth.js', () => ({ getApiToken: async () => 'test-token', getBaseUrl: async () => 'https://test.outline.com', getOAuthClientId: async () => undefined, - getTokenSource: async () => 'config' as const, - clearConfig: vi.fn(), + getActiveTokenSource: async () => + process.env.OUTLINE_API_TOKEN ? ('env' as const) : ('secure-store' as const), })) vi.mock('../lib/api.js', () => ({ apiRequest: vi.fn() })) @@ -27,6 +28,19 @@ vi.mock('@doist/cli-core/auth', async () => ({ attachLoginCommand: vi.fn(), })) +/** + * Replace `console.log` with a recorder. Tests read `logs` to assert on + * stdout-bound output. Lines are joined with spaces, matching how chalk's + * styled fragments arrive at the spy. + */ +function captureLogs(): { logs: string[] } { + const logs: string[] = [] + vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { + logs.push(args.join(' ')) + }) + return { logs } +} + async function captureAttachOptions() { const { attachLoginCommand } = await import('@doist/cli-core/auth') const login = new Command('login') @@ -59,10 +73,7 @@ afterEach(() => { describe('registerAuthCommand', () => { it('wires --base-url / --client-id, env-driven port, and prints success only in human output mode', async () => { process.env.OUTLINE_OAUTH_CALLBACK_PORT = '7000' - const logs: string[] = [] - vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { - logs.push(args.join(' ')) - }) + const { logs } = captureLogs() const { options, login } = await captureAttachOptions() @@ -93,11 +104,6 @@ describe('registerAuthCommand', () => { }) describe('auth status subcommand', () => { - const AUTH_INFO = { - user: { id: 'user-uuid', name: 'Ada Lovelace', email: 'ada@example.com' }, - team: { name: 'Analytics', subdomain: 'analytics' }, - } - async function importApiMock() { const { apiRequest } = await import('../lib/api.js') return vi.mocked(apiRequest) @@ -105,10 +111,7 @@ describe('auth status subcommand', () => { it('renders the human status from the env-token snapshot path', async () => { process.env.OUTLINE_API_TOKEN = 'env-token' - const logs: string[] = [] - vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { - logs.push(args.join(' ')) - }) + const { logs } = captureLogs() const apiRequest = await importApiMock() apiRequest.mockResolvedValue({ data: AUTH_INFO }) @@ -130,10 +133,7 @@ describe('auth status subcommand', () => { it('emits a PII-free JSON envelope under --json', async () => { process.env.OUTLINE_API_TOKEN = 'env-token' - const logs: string[] = [] - vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { - logs.push(args.join(' ')) - }) + const { logs } = captureLogs() const apiRequest = await importApiMock() apiRequest.mockResolvedValue({ data: AUTH_INFO }) @@ -154,10 +154,7 @@ describe('auth status subcommand', () => { it('emits a single newline-free NDJSON line under --ndjson', async () => { process.env.OUTLINE_API_TOKEN = 'env-token' - const logs: string[] = [] - vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { - logs.push(args.join(' ')) - }) + const { logs } = captureLogs() const apiRequest = await importApiMock() apiRequest.mockResolvedValue({ data: AUTH_INFO }) @@ -194,25 +191,17 @@ describe('auth status subcommand', () => { }) describe('auth logout subcommand', () => { - it('clears the token and prints the registrar success line', async () => { - const logs: string[] = [] - vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { - logs.push(args.join(' ')) - }) - const { clearConfig } = await import('../lib/auth.js') + it('prints the registrar success line in human mode', async () => { + const { logs } = captureLogs() const program = await buildProgram() await program.parseAsync(['node', 'ol', 'auth', 'logout']) - expect(clearConfig).toHaveBeenCalledTimes(1) expect(logs).toContain('✓ Logged out') }) it('emits {"ok": true} under --json and skips the human success line', async () => { - const logs: string[] = [] - vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { - logs.push(args.join(' ')) - }) + const { logs } = captureLogs() const program = await buildProgram() await program.parseAsync(['node', 'ol', 'auth', 'logout', '--json']) @@ -220,16 +209,4 @@ describe('auth logout subcommand', () => { expect(logs).toHaveLength(1) expect(JSON.parse(logs[0])).toEqual({ ok: true }) }) - - it('stays silent on stdout under --ndjson', async () => { - const logs: string[] = [] - vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { - logs.push(args.join(' ')) - }) - - const program = await buildProgram() - await program.parseAsync(['node', 'ol', 'auth', 'logout', '--ndjson']) - - expect(logs).toEqual([]) - }) }) diff --git a/src/__tests__/auth-provider.test.ts b/src/__tests__/auth-provider.test.ts index 46b8578..59d7ea0 100644 --- a/src/__tests__/auth-provider.test.ts +++ b/src/__tests__/auth-provider.test.ts @@ -1,31 +1,77 @@ -import { existsSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' -import { tmpdir } from 'node:os' -import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' - -const TEST_XDG = join(tmpdir(), `outline-cli-test-${process.pid}-auth-provider`) -const TEST_CONFIG_DIR = join(TEST_XDG, 'outline-cli') -const TEST_CONFIG_PATH = join(TEST_CONFIG_DIR, 'config.json') +import { + LEGACY_CLEAR_PAYLOAD, + LEGACY_CONFIG, + okResponse, + SKIPPED_RESULT, + STORED_ACCOUNT, +} from './_fixtures/auth.js' vi.mock('../transport/fetch-with-retry.js', () => ({ fetchWithRetry: vi.fn() })) vi.mock('../lib/api.js', () => ({ apiRequest: vi.fn() })) -beforeEach(() => { - process.env.XDG_CONFIG_HOME = TEST_XDG - mkdirSync(TEST_CONFIG_DIR, { recursive: true }) - delete process.env.OUTLINE_API_TOKEN - delete process.env.OUTLINE_URL - delete process.env.OUTLINE_OAUTH_CLIENT_ID - vi.resetModules() - vi.clearAllMocks() +const migrateMocks = vi.hoisted(() => ({ + runMigrateLegacyAuth: vi.fn(), +})) + +vi.mock('../lib/migrate-auth.js', () => migrateMocks) + +const keyringMocks = vi.hoisted(() => ({ + createKeyringTokenStore: vi.fn(), + inner: { + active: vi.fn(), + set: vi.fn(), + clear: vi.fn(), + list: vi.fn(), + setDefault: vi.fn(), + getLastStorageResult: vi.fn(), + getLastClearResult: vi.fn(), + }, +})) + +vi.mock('@doist/cli-core/auth', async (importOriginal) => { + const actual = await importOriginal() + keyringMocks.createKeyringTokenStore.mockImplementation(() => keyringMocks.inner) + return { + ...actual, + createKeyringTokenStore: keyringMocks.createKeyringTokenStore, + } }) -afterEach(() => { - if (existsSync(TEST_XDG)) rmSync(TEST_XDG, { recursive: true }) - delete process.env.XDG_CONFIG_HOME +const configMocks = vi.hoisted(() => ({ + getConfig: vi.fn(), + updateConfig: vi.fn(), +})) + +vi.mock('../lib/config.js', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + getConfigPath: () => '/home/user/.config/outline-cli/config.json', + getConfig: configMocks.getConfig, + updateConfig: configMocks.updateConfig, + } }) -describe('OutlineAuthProvider', () => { +const TOKEN_ENV_VAR = 'OUTLINE_API_TOKEN' + +/** Reset the module-level migration memo for each test by re-importing. */ +async function loadCreateOutlineTokenStore(): Promise< + typeof import('../lib/auth-provider.js').createOutlineTokenStore +> { + vi.resetModules() + const mod = await import('../lib/auth-provider.js') + return mod.createOutlineTokenStore +} + +describe('createOutlineAuthProvider', () => { + beforeEach(() => { + delete process.env.OUTLINE_URL + delete process.env.OUTLINE_OAUTH_CLIENT_ID + configMocks.getConfig.mockReset().mockResolvedValue({}) + configMocks.updateConfig.mockReset().mockResolvedValue(undefined) + }) + it('authorize builds an outline /oauth/authorize URL with PKCE params from flags', async () => { const { createOutlineAuthProvider } = await import('../lib/auth-provider.js') const result = await createOutlineAuthProvider().authorize({ @@ -58,10 +104,7 @@ describe('OutlineAuthProvider', () => { it('exchangeCode posts via fetchWithRetry and surfaces provider errors', async () => { const { fetchWithRetry } = await import('../transport/fetch-with-retry.js') - vi.mocked(fetchWithRetry).mockResolvedValueOnce({ - ok: true, - json: async () => ({ access_token: 'tok-abc' }), - } as Response) + vi.mocked(fetchWithRetry).mockResolvedValueOnce(okResponse({ access_token: 'tok-abc' })) const { createOutlineAuthProvider } = await import('../lib/auth-provider.js') const provider = createOutlineAuthProvider() @@ -135,151 +178,212 @@ describe('OutlineAuthProvider', () => { }) }) -describe('OutlineTokenStore', () => { - const sampleAccount = { - id: 'user-uuid', - label: 'Ada', - baseUrl: 'https://wiki.example.com', - oauthClientId: 'cid-xyz', - teamName: 'Analytics', - } - - /** Config-file shape that round-trips to `sampleAccount`. */ - const sampleConfig = { - api_token: 'tok', - base_url: sampleAccount.baseUrl, - oauth_client_id: sampleAccount.oauthClientId, - auth_user_id: sampleAccount.id, - auth_user_name: sampleAccount.label, - auth_team_name: sampleAccount.teamName, - } +describe('createOutlineTokenStore', () => { + beforeEach(() => { + delete process.env[TOKEN_ENV_VAR] + delete process.env.OUTLINE_URL + keyringMocks.createKeyringTokenStore.mockClear() + keyringMocks.inner.active.mockReset().mockResolvedValue(null) + keyringMocks.inner.set.mockReset().mockResolvedValue(undefined) + keyringMocks.inner.clear.mockReset().mockResolvedValue(undefined) + keyringMocks.inner.list.mockReset().mockResolvedValue([]) + keyringMocks.inner.setDefault.mockReset().mockResolvedValue(undefined) + migrateMocks.runMigrateLegacyAuth + .mockReset() + .mockResolvedValue({ status: 'no-legacy-state' }) + configMocks.getConfig.mockReset().mockResolvedValue({}) + configMocks.updateConfig.mockReset().mockResolvedValue(undefined) + }) - it('round-trips token + account through the config file', async () => { - const { createOutlineTokenStore } = await import('../lib/auth-provider.js') - const store = createOutlineTokenStore() - await store.set(sampleAccount, 'tok-persisted') - const got = await store.active() - expect(got).toEqual({ token: 'tok-persisted', account: sampleAccount }) + afterEach(() => { + vi.unstubAllEnvs() }) - it('active returns null when the saved token predates the persisted-identity fields', async () => { - writeFileSync(TEST_CONFIG_PATH, JSON.stringify({ api_token: 'legacy-tok' })) - const { createOutlineTokenStore } = await import('../lib/auth-provider.js') - await expect(createOutlineTokenStore().active()).resolves.toBeNull() + it('passes outline-cli wiring to cli-core: serviceName, records location, and the id-or-label matcher', async () => { + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + createOutlineTokenStore() + + const options = keyringMocks.createKeyringTokenStore.mock.calls[0][0] + expect(options.serviceName).toBe('outline-cli') + expect(options.recordsLocation).toBe('/home/user/.config/outline-cli/config.json') + const { matchOutlineAccount } = await import('../lib/auth-provider.js') + expect(options.matchAccount).toBe(matchOutlineAccount) }) - it('clear strips every auth field but preserves unrelated config keys', async () => { - writeFileSync( - TEST_CONFIG_PATH, - JSON.stringify({ - api_token: 'tok', - base_url: 'https://x', - oauth_client_id: 'c', - auth_user_id: 'u', - auth_user_name: 'l', - auth_team_name: 't', - update_channel: 'pre-release', - }), - ) - const { createOutlineTokenStore } = await import('../lib/auth-provider.js') - await createOutlineTokenStore().clear() - const after = JSON.parse(readFileSync(TEST_CONFIG_PATH, 'utf8')) - expect(after).toEqual({ update_channel: 'pre-release' }) + it('active() env-token short-circuit: returns env token, honours OUTLINE_URL, bypasses migration + inner.active', async () => { + vi.stubEnv(TOKEN_ENV_VAR, 'env_token_value') + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + + const defaultBase = await createOutlineTokenStore().active() + expect(defaultBase?.token).toBe('env_token_value') + expect(defaultBase?.account.id).toBe('') + expect(defaultBase?.account.baseUrl).toBe('https://app.getoutline.com') + expect(keyringMocks.inner.active).not.toHaveBeenCalled() + expect(migrateMocks.runMigrateLegacyAuth).not.toHaveBeenCalled() + + vi.stubEnv('OUTLINE_URL', 'https://custom.example.com/') + const customBase = await createOutlineTokenStore().active() + expect(customBase?.account.baseUrl).toBe('https://custom.example.com') }) - describe('ref-aware lookups', () => { - it('active(ref) returns the snapshot when the id ref matches', async () => { - writeFileSync(TEST_CONFIG_PATH, JSON.stringify(sampleConfig)) - const { createOutlineTokenStore } = await import('../lib/auth-provider.js') - expect(await createOutlineTokenStore().active(sampleAccount.id)).toEqual({ - token: sampleConfig.api_token, - account: sampleAccount, - }) + it('active() ignores OUTLINE_API_TOKEN when an explicit ref targets a stored account', async () => { + vi.stubEnv(TOKEN_ENV_VAR, 'env_token_value') + keyringMocks.inner.active.mockResolvedValue({ + token: 'tk_stored', + account: STORED_ACCOUNT, }) + const createOutlineTokenStore = await loadCreateOutlineTokenStore() - it('active(ref) matches the stored label case-insensitively', async () => { - writeFileSync(TEST_CONFIG_PATH, JSON.stringify(sampleConfig)) - const { createOutlineTokenStore } = await import('../lib/auth-provider.js') - expect(await createOutlineTokenStore().active('ADA')).toEqual({ - token: sampleConfig.api_token, - account: sampleAccount, - }) - }) + await createOutlineTokenStore().active('user-uuid') - it('active(ref) throws ACCOUNT_NOT_FOUND on mismatch', async () => { - writeFileSync(TEST_CONFIG_PATH, JSON.stringify(sampleConfig)) - const { createOutlineTokenStore } = await import('../lib/auth-provider.js') - await expect(createOutlineTokenStore().active('other')).rejects.toMatchObject({ - code: 'ACCOUNT_NOT_FOUND', - }) - }) + expect(keyringMocks.inner.active).toHaveBeenCalledWith('user-uuid') + }) - it('active(ref) throws ACCOUNT_NOT_FOUND when no token is stored', async () => { - const { createOutlineTokenStore } = await import('../lib/auth-provider.js') - await expect(createOutlineTokenStore().active(sampleAccount.id)).rejects.toMatchObject({ - code: 'ACCOUNT_NOT_FOUND', - }) - }) + it('runs runMigrateLegacyAuth on the first store access and memoises across subsequent calls', async () => { + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + const store = createOutlineTokenStore() - it('clear(ref) clears the config when the ref matches', async () => { - writeFileSync( - TEST_CONFIG_PATH, - JSON.stringify({ ...sampleConfig, update_channel: 'stable' }), - ) - const { createOutlineTokenStore } = await import('../lib/auth-provider.js') - await createOutlineTokenStore().clear(sampleAccount.id) - const after = JSON.parse(readFileSync(TEST_CONFIG_PATH, 'utf8')) - expect(after).toEqual({ update_channel: 'stable' }) - }) + await store.active('user-uuid') + await store.list() + await store.clear('user-uuid') + await store.set(STORED_ACCOUNT, 'tk') + await store.setDefault('user-uuid') - it('clear(ref) throws ACCOUNT_NOT_FOUND on mismatch and does not touch storage', async () => { - writeFileSync(TEST_CONFIG_PATH, JSON.stringify(sampleConfig)) - const { createOutlineTokenStore } = await import('../lib/auth-provider.js') - await expect(createOutlineTokenStore().clear('other')).rejects.toMatchObject({ - code: 'ACCOUNT_NOT_FOUND', - }) - const after = JSON.parse(readFileSync(TEST_CONFIG_PATH, 'utf8')) - expect(after).toEqual(sampleConfig) - }) + expect(migrateMocks.runMigrateLegacyAuth).toHaveBeenCalledTimes(1) + expect(migrateMocks.runMigrateLegacyAuth).toHaveBeenCalledWith({ silent: true }) + }) - it('clear(ref) throws ACCOUNT_NOT_FOUND when no token is stored at all', async () => { - // Guards against a regression to the old silent no-op when the - // store is empty — `attachLogoutCommand` would otherwise emit - // ✓ Logged out for a ref that never had any backing account. - const { createOutlineTokenStore } = await import('../lib/auth-provider.js') - await expect(createOutlineTokenStore().clear(sampleAccount.id)).rejects.toMatchObject({ - code: 'ACCOUNT_NOT_FOUND', - }) - expect(existsSync(TEST_CONFIG_PATH)).toBe(false) - }) + it('falls back to the legacy plaintext snapshot when migration is skipped', async () => { + migrateMocks.runMigrateLegacyAuth.mockResolvedValue(SKIPPED_RESULT) + configMocks.getConfig.mockResolvedValue(LEGACY_CONFIG) + const createOutlineTokenStore = await loadCreateOutlineTokenStore() - it('list() returns the stored account flagged as default', async () => { - writeFileSync(TEST_CONFIG_PATH, JSON.stringify(sampleConfig)) - const { createOutlineTokenStore } = await import('../lib/auth-provider.js') - expect(await createOutlineTokenStore().list()).toEqual([ - { account: sampleAccount, isDefault: true }, - ]) - }) + const snapshot = await createOutlineTokenStore().active() - it('list() returns an empty array when no token is stored', async () => { - const { createOutlineTokenStore } = await import('../lib/auth-provider.js') - expect(await createOutlineTokenStore().list()).toEqual([]) + expect(snapshot).toEqual({ + token: LEGACY_CONFIG.api_token, + account: STORED_ACCOUNT, }) + expect(keyringMocks.inner.active).not.toHaveBeenCalled() + }) - it('setDefault(ref) resolves silently when the ref matches', async () => { - writeFileSync(TEST_CONFIG_PATH, JSON.stringify(sampleConfig)) - const { createOutlineTokenStore } = await import('../lib/auth-provider.js') - await expect( - createOutlineTokenStore().setDefault(sampleAccount.id), - ).resolves.toBeUndefined() - }) + it('delegates to the v2 store when migration is conclusive (no-legacy-state) — no legacy read attempt', async () => { + migrateMocks.runMigrateLegacyAuth.mockResolvedValue({ status: 'no-legacy-state' }) + keyringMocks.inner.active.mockResolvedValue({ token: 'tk_v2', account: STORED_ACCOUNT }) + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + + const snapshot = await createOutlineTokenStore().active() - it('setDefault(ref) throws ACCOUNT_NOT_FOUND when the ref does not match', async () => { - writeFileSync(TEST_CONFIG_PATH, JSON.stringify(sampleConfig)) - const { createOutlineTokenStore } = await import('../lib/auth-provider.js') - await expect(createOutlineTokenStore().setDefault('other')).rejects.toMatchObject({ - code: 'ACCOUNT_NOT_FOUND', - }) + expect(snapshot).toEqual({ token: 'tk_v2', account: STORED_ACCOUNT }) + expect(configMocks.getConfig).not.toHaveBeenCalled() + }) + + it('falls back to legacy when runMigrateLegacyAuth rejects (catch branch of ensureMigrated)', async () => { + migrateMocks.runMigrateLegacyAuth.mockRejectedValue(new Error('boom')) + configMocks.getConfig.mockResolvedValue(LEGACY_CONFIG) + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + + const snapshot = await createOutlineTokenStore().active() + + expect(snapshot?.token).toBe(LEGACY_CONFIG.api_token) + expect(snapshot?.account.id).toBe('user-uuid') + expect(keyringMocks.inner.active).not.toHaveBeenCalled() + }) + + it('legacy snapshot synthesises empty id/label when the v1 config never carried persisted identity fields', async () => { + migrateMocks.runMigrateLegacyAuth.mockResolvedValue(SKIPPED_RESULT) + configMocks.getConfig.mockResolvedValue({ api_token: 'tk_old' }) + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + + const snapshot = await createOutlineTokenStore().active() + + expect(snapshot?.token).toBe('tk_old') + expect(snapshot?.account.id).toBe('') + expect(snapshot?.account.label).toBe('') + expect(snapshot?.account.baseUrl).toBe('https://app.getoutline.com') + }) + + it('active(ref) returns the legacy snapshot when ref matches, falls through to v2 when it does not', async () => { + migrateMocks.runMigrateLegacyAuth.mockResolvedValue(SKIPPED_RESULT) + configMocks.getConfig.mockResolvedValue(LEGACY_CONFIG) + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + const store = createOutlineTokenStore() + + const matched = await store.active('user-uuid') + expect(matched?.token).toBe(LEGACY_CONFIG.api_token) + + const mismatched = await store.active('other-user') + expect(mismatched).toBeNull() + expect(keyringMocks.inner.active).toHaveBeenCalledWith('other-user') + }) + + it('set() / clear() discharge legacy state on disk when migration is inconclusive', async () => { + migrateMocks.runMigrateLegacyAuth.mockResolvedValue(SKIPPED_RESULT) + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + const store = createOutlineTokenStore() + + await store.set(STORED_ACCOUNT, 'tk_new') + await store.clear('user-uuid') + + expect(configMocks.updateConfig).toHaveBeenCalledWith(LEGACY_CLEAR_PAYLOAD) + expect(keyringMocks.inner.set).toHaveBeenCalledWith(STORED_ACCOUNT, 'tk_new') + expect(keyringMocks.inner.clear).toHaveBeenCalledWith('user-uuid') + }) + + it('set() / clear() do NOT touch legacy state when migration is conclusive', async () => { + migrateMocks.runMigrateLegacyAuth.mockResolvedValue({ status: 'no-legacy-state' }) + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + const store = createOutlineTokenStore() + + await store.set(STORED_ACCOUNT, 'tk_new') + await store.clear('user-uuid') + + expect(configMocks.updateConfig).not.toHaveBeenCalled() + }) +}) + +describe('matchOutlineAccount', () => { + it('matches the UUID exactly and the label case-insensitively', async () => { + const { matchOutlineAccount } = await import('../lib/auth-provider.js') + expect(matchOutlineAccount(STORED_ACCOUNT, 'user-uuid')).toBe(true) + expect(matchOutlineAccount(STORED_ACCOUNT, 'ADA')).toBe(true) + expect(matchOutlineAccount(STORED_ACCOUNT, 'ada')).toBe(true) + expect(matchOutlineAccount(STORED_ACCOUNT, 'other-user')).toBe(false) + // Case-sensitive on the UUID — would never collide with a label. + expect(matchOutlineAccount(STORED_ACCOUNT, 'USER-UUID')).toBe(false) + }) +}) + +describe('getActiveTokenSource', () => { + beforeEach(() => { + delete process.env[TOKEN_ENV_VAR] + configMocks.getConfig.mockReset() + }) + + afterEach(() => { + vi.unstubAllEnvs() + }) + + it('reports the storage location of the active token across env / v1 plaintext / v2 record / empty', async () => { + const { getActiveTokenSource } = await import('../lib/auth-provider.js') + + vi.stubEnv(TOKEN_ENV_VAR, 'tk') + configMocks.getConfig.mockResolvedValue({}) + await expect(getActiveTokenSource()).resolves.toBe('env') + vi.unstubAllEnvs() + + configMocks.getConfig.mockResolvedValue({ api_token: 'tk' }) + await expect(getActiveTokenSource()).resolves.toBe('config-file') + + configMocks.getConfig.mockResolvedValue({ + users: [{ id: 'u', name: 'Ada', token: 'plaintext' }], }) + await expect(getActiveTokenSource()).resolves.toBe('config-file') + + configMocks.getConfig.mockResolvedValue({ users: [{ id: 'u', name: 'Ada' }] }) + await expect(getActiveTokenSource()).resolves.toBe('secure-store') + + configMocks.getConfig.mockResolvedValue({}) + await expect(getActiveTokenSource()).resolves.toBe('secure-store') }) }) diff --git a/src/__tests__/auth.test.ts b/src/__tests__/auth.test.ts index a87beb0..6a2aa1a 100644 --- a/src/__tests__/auth.test.ts +++ b/src/__tests__/auth.test.ts @@ -3,10 +3,20 @@ import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -const TEST_XDG = join(tmpdir(), `outline-cli-test-${process.pid}`) +const TEST_XDG = join(tmpdir(), `outline-cli-test-${process.pid}-auth`) const TEST_CONFIG_DIR = join(TEST_XDG, 'outline-cli') const TEST_CONFIG_PATH = join(TEST_CONFIG_DIR, 'config.json') +// Force `identifyAccount` to fail so migration returns `skipped` and the +// runtime falls back to the legacy plaintext snapshot — the v1 token in +// these fixtures has no live API behind it. cli-core's secure-store is left +// real because the env-token tests don't hit the keyring at all. +vi.mock('../transport/fetch-with-retry.js', () => ({ + fetchWithRetry: vi.fn(async () => { + throw new Error('offline (mocked)') + }), +})) + describe('auth', () => { beforeEach(() => { process.env.XDG_CONFIG_HOME = TEST_XDG @@ -30,7 +40,7 @@ describe('auth', () => { await expect(getApiToken()).resolves.toBe('env-token') }) - it('getApiToken reads from config file', async () => { + it('getApiToken falls back to the legacy plaintext config token', async () => { writeFileSync(TEST_CONFIG_PATH, JSON.stringify({ api_token: 'file-token' })) const { getApiToken } = await import('../lib/auth.js') await expect(getApiToken()).resolves.toBe('file-token') @@ -58,18 +68,29 @@ describe('auth', () => { await expect(getBaseUrl()).resolves.toBe('https://app.getoutline.com') }) - it('clearConfig removes the saved token', async () => { + it('getBaseUrl reads from the default user record (v2)', async () => { writeFileSync( TEST_CONFIG_PATH, JSON.stringify({ - api_token: 'test-token', - base_url: 'https://wiki.test.com', - oauth_client_id: 'client-id', + config_version: 2, + users: [ + { + id: 'u', + name: 'Ada', + base_url: 'https://wiki.example.com', + token: 'tok', + }, + ], }), ) - const { clearConfig, getApiToken } = await import('../lib/auth.js') - await clearConfig() - await expect(getApiToken()).rejects.toThrow() + const { getBaseUrl } = await import('../lib/auth.js') + await expect(getBaseUrl()).resolves.toBe('https://wiki.example.com') + }) + + it('getBaseUrl reads from the legacy v1 base_url slot', async () => { + writeFileSync(TEST_CONFIG_PATH, JSON.stringify({ base_url: 'https://legacy.example.com' })) + const { getBaseUrl } = await import('../lib/auth.js') + await expect(getBaseUrl()).resolves.toBe('https://legacy.example.com') }) it('getOAuthClientId returns env var first', async () => { @@ -78,9 +99,28 @@ describe('auth', () => { await expect(getOAuthClientId()).resolves.toBe('env-client-id') }) - it('getOAuthClientId reads from config file', async () => { + it('getOAuthClientId reads from the legacy v1 config slot', async () => { writeFileSync(TEST_CONFIG_PATH, JSON.stringify({ oauth_client_id: 'file-client-id' })) const { getOAuthClientId } = await import('../lib/auth.js') await expect(getOAuthClientId()).resolves.toBe('file-client-id') }) + + it('getOAuthClientId reads from the default user record (v2)', async () => { + writeFileSync( + TEST_CONFIG_PATH, + JSON.stringify({ + config_version: 2, + users: [ + { + id: 'u', + name: 'Ada', + oauth_client_id: 'cid-record', + token: 'tok', + }, + ], + }), + ) + const { getOAuthClientId } = await import('../lib/auth.js') + await expect(getOAuthClientId()).resolves.toBe('cid-record') + }) }) diff --git a/src/__tests__/migrate-auth.test.ts b/src/__tests__/migrate-auth.test.ts new file mode 100644 index 0000000..ccccc4e --- /dev/null +++ b/src/__tests__/migrate-auth.test.ts @@ -0,0 +1,82 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { errResponse, okResponse } from './_fixtures/auth.js' + +const fetchMock = vi.hoisted(() => ({ fetchWithRetry: vi.fn() })) +vi.mock('../transport/fetch-with-retry.js', () => fetchMock) + +const cliCoreMock = vi.hoisted(() => ({ migrateLegacyAuth: vi.fn() })) +vi.mock('@doist/cli-core/auth', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, migrateLegacyAuth: cliCoreMock.migrateLegacyAuth } +}) + +const configMock = vi.hoisted(() => ({ + getConfig: vi.fn(), + updateConfig: vi.fn(), +})) +vi.mock('../lib/config.js', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, getConfig: configMock.getConfig, updateConfig: configMock.updateConfig } +}) + +/** Run the wrapper once, return the options handed to cli-core. */ +async function captureCliCoreOptions() { + cliCoreMock.migrateLegacyAuth.mockResolvedValue({ status: 'no-legacy-state' }) + const { runMigrateLegacyAuth } = await import('../lib/migrate-auth.js') + await runMigrateLegacyAuth() + return cliCoreMock.migrateLegacyAuth.mock.calls[0][0] +} + +describe('runMigrateLegacyAuth', () => { + beforeEach(() => { + cliCoreMock.migrateLegacyAuth.mockReset() + fetchMock.fetchWithRetry.mockReset() + configMock.getConfig.mockReset().mockResolvedValue({}) + configMock.updateConfig.mockReset().mockResolvedValue(undefined) + }) + + afterEach(() => { + vi.resetModules() + }) + + it('hands cli-core the outline-cli wiring (service, legacy slot, records store, log prefix)', async () => { + const opts = await captureCliCoreOptions() + expect(opts.serviceName).toBe('outline-cli') + expect(opts.legacyAccount).toBe('api-token') + expect(opts.logPrefix).toBe('outline-cli') + expect(opts.silent).toBe(true) + expect(typeof opts.userRecords.list).toBe('function') + }) + + it('identifyAccount POSTs auth.info to the configured base URL and builds an OutlineAccount', async () => { + const opts = await captureCliCoreOptions() + configMock.getConfig.mockResolvedValue({ + base_url: 'https://wiki.example.com/', + oauth_client_id: 'cid-xyz', + }) + fetchMock.fetchWithRetry.mockResolvedValue( + okResponse({ + data: { user: { id: 'user-uuid', name: 'Ada' }, team: { name: 'Analytics' } }, + }), + ) + + const account = await opts.identifyAccount('tk_v1') + + const args = fetchMock.fetchWithRetry.mock.calls[0][0] + expect(args.url).toBe('https://wiki.example.com/api/auth.info') + expect(args.options.headers.Authorization).toBe('Bearer tk_v1') + expect(account).toEqual({ + id: 'user-uuid', + label: 'Ada', + baseUrl: 'https://wiki.example.com', + oauthClientId: 'cid-xyz', + teamName: 'Analytics', + }) + }) + + it('identifyAccount throws when auth.info responds with !ok (triggers identify-failed)', async () => { + const opts = await captureCliCoreOptions() + fetchMock.fetchWithRetry.mockResolvedValue(errResponse(401, 'Unauthorized')) + await expect(opts.identifyAccount('tk')).rejects.toThrow('auth.info failed: 401') + }) +}) diff --git a/src/commands/auth.ts b/src/commands/auth.ts index 8e40c0d..09b0a5a 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -1,4 +1,9 @@ -import { attachLoginCommand, attachLogoutCommand, attachStatusCommand } from '@doist/cli-core/auth' +import { + attachLoginCommand, + attachLogoutCommand, + attachStatusCommand, + type TokenStorageResult, +} from '@doist/cli-core/auth' import chalk from 'chalk' import type { Command } from 'commander' import { apiRequest } from '../lib/api.js' @@ -7,7 +12,9 @@ import { type AuthInfoResponse, createOutlineAuthProvider, createOutlineTokenStore, + getActiveTokenSource, type OutlineAccount, + type OutlineTokenStore, } from '../lib/auth-provider.js' import { CliError } from '../lib/errors.js' @@ -15,7 +22,7 @@ const DEFAULT_OAUTH_CALLBACK_PORT = 54969 type StatusData = { email: string - source: 'env' | 'config' + source: 'env' | 'secure-store' | 'config-file' } function resolvePreferredCallbackPort(): number { @@ -28,11 +35,30 @@ function resolvePreferredCallbackPort(): number { return parsed } +/** + * Surface a `TokenStorageResult` from a save/clear: the human-readable + * confirmation goes to stdout, any keyring-fallback warning goes to stderr. + * Pass `isMachineOutput: true` to suppress the stdout confirmation in + * `--json` / `--ndjson` mode while still routing the warning to stderr. + */ +function logTokenStorageResult( + result: TokenStorageResult, + secureStoreMessage: string, + isMachineOutput = false, +): void { + if (!isMachineOutput && result.storage === 'secure-store') { + console.log(chalk.dim(secureStoreMessage)) + } + if (result.warning) { + console.error(chalk.yellow('Warning:'), result.warning) + } +} + export function registerAuthCommand(program: Command): void { const auth = program.command('auth').description('Manage authentication') const provider = createOutlineAuthProvider() - const store = createOutlineTokenStore() + const store: OutlineTokenStore = createOutlineTokenStore() attachLoginCommand(auth, { provider, @@ -42,8 +68,18 @@ export function registerAuthCommand(program: Command): void { renderSuccess, renderError, onSuccess({ view, account }) { - if (view.json || view.ndjson) return - console.log(chalk.green(`Authenticated to ${account.teamName} as ${account.label}`)) + const isMachineOutput = view.json || view.ndjson + if (!isMachineOutput) { + console.log(chalk.green(`Authenticated to ${account.teamName} as ${account.label}`)) + } + const result = store.getLastStorageResult() + if (result) { + logTokenStorageResult( + result, + 'Token stored securely in the system credential manager', + isMachineOutput, + ) + } }, }) .description('Authenticate with an Outline instance via OAuth') @@ -66,15 +102,15 @@ export function registerAuthCommand(program: Command): void { description: 'Show current authentication state', async fetchLive({ token, account }) { try { - const { data: info } = await apiRequest( - 'auth.info', - {}, - { token, baseUrl: account.baseUrl }, - ) - statusData = { - email: info.user.email, - source: process.env.OUTLINE_API_TOKEN ? 'env' : 'config', - } + const [{ data: info }, source] = await Promise.all([ + apiRequest( + 'auth.info', + {}, + { token, baseUrl: account.baseUrl }, + ), + getActiveTokenSource(), + ]) + statusData = { email: info.user.email, source } return { ...account, id: info.user.id, @@ -118,5 +154,14 @@ export function registerAuthCommand(program: Command): void { attachLogoutCommand(auth, { store, description: 'Clear saved authentication', + onCleared({ view }) { + const result = store.getLastClearResult() + if (!result) return + logTokenStorageResult( + result, + 'Stored token removed from the system credential manager', + view.json || view.ndjson, + ) + }, }) } diff --git a/src/lib/auth-constants.ts b/src/lib/auth-constants.ts new file mode 100644 index 0000000..02e0aa7 --- /dev/null +++ b/src/lib/auth-constants.ts @@ -0,0 +1,10 @@ +/** OS keyring `service` identifier for every outline-cli secret. */ +export const SECURE_STORE_SERVICE = 'outline-cli' + +/** + * Legacy single-user keyring slot. Outline never wrote a token to the + * keyring before this migration, so there is no real legacy slot to read — + * `migrateLegacyAuth` still expects the option, and its best-effort delete + * after migration is a harmless no-op on an empty entry. + */ +export const LEGACY_KEYRING_ACCOUNT = 'api-token' diff --git a/src/lib/auth-provider.ts b/src/lib/auth-provider.ts index 4254e40..a78bd31 100644 --- a/src/lib/auth-provider.ts +++ b/src/lib/auth-provider.ts @@ -1,32 +1,32 @@ import { createInterface } from 'node:readline/promises' import { type AccountRef, - type AuthAccount, type AuthProvider, + createKeyringTokenStore, deriveChallenge, generateVerifier, - type TokenStore, + type KeyringTokenStore, + type MigrateAuthResult, } from '@doist/cli-core/auth' import { fetchWithRetry } from '../transport/fetch-with-retry.js' import { apiRequest } from './api.js' -import { clearConfig, getBaseUrl, getOAuthClientId } from './auth.js' -import { type Config, getConfig, updateConfig } from './config.js' -import { CliError } from './errors.js' +import { SECURE_STORE_SERVICE } from './auth-constants.js' +import { getBaseUrl, getOAuthClientId } from './auth.js' +import { getConfig, getConfigPath, updateConfig } from './config.js' +import { runMigrateLegacyAuth } from './migrate-auth.js' +import { makeOutlineAccount, type OutlineAccount } from './outline-account.js' +import { createOutlineUserRecordStore, getDefaultUserRecord } from './user-records.js' -const DEFAULT_BASE_URL = 'https://app.getoutline.com' +export type { OutlineAccount } from './outline-account.js' + +const TOKEN_ENV_VAR = 'OUTLINE_API_TOKEN' export type AuthInfoResponse = { user: { id: string; name: string; email: string } team: { name: string; subdomain: string } } -export type OutlineAccount = AuthAccount & { - id: string - label: string - baseUrl: string - oauthClientId: string - teamName?: string -} +export type OutlineTokenStore = KeyringTokenStore type OutlineHandshake = Record & { baseUrl: string @@ -150,115 +150,190 @@ export function createOutlineAuthProvider(): AuthProvider { baseUrl: hs.baseUrl, }, ) - return { + return makeOutlineAccount({ id: data.user.id, label: data.user.name, baseUrl: hs.baseUrl, oauthClientId: hs.clientId, teamName: data.team.name, - } + }) }, } } -export function createOutlineTokenStore(): TokenStore { - /** - * Derive a snapshot from an already-loaded config. Pure, so ref-aware - * callers can validate without a second config read. - */ - function deriveSnapshot( - config: Partial, - ): { token: string; account: OutlineAccount } | null { - if (!config.api_token) return null - const id = config.auth_user_id - const label = config.auth_user_name - if (!id || !label) { - // Stored token predates this adapter (env var, pre-upgrade - // config). No persisted identity to round-trip. - return null - } - return { - token: config.api_token, - account: { - id, - label, - baseUrl: config.base_url ?? DEFAULT_BASE_URL, - oauthClientId: config.oauth_client_id ?? '', - teamName: config.auth_team_name, - }, - } - } +/** + * Accepts the Outline user UUID or display name. Id matches are + * case-sensitive (UUIDs are canonical); label matches are + * case-insensitive so users can pass the name they see in `auth status`. + */ +export function matchOutlineAccount(account: OutlineAccount, ref: AccountRef): boolean { + if (account.id === ref) return true + return account.label.toLowerCase() === ref.toLowerCase() +} + +/** True when the v2 store is the authoritative source. */ +function migrationIsConclusive(result: MigrateAuthResult): boolean { + return ( + result.status === 'migrated' || + result.status === 'already-migrated' || + result.status === 'no-legacy-state' + ) +} - /** - * Match the stored account against `--user `. Outline accounts use - * UUID ids and a display name — id matches are case-sensitive (UUIDs - * are canonical), label matches are case-insensitive so users can pass - * the name they see in `auth status` regardless of casing. - */ - function matchesRef(account: OutlineAccount, ref: AccountRef): boolean { - if (account.id === ref) return true - return account.label.toLowerCase() === ref.toLowerCase() +/** + * Synthesise a snapshot from v1 plaintext state still on disk. Fallback for + * when migration can't complete (offline `identifyAccount`, WSL + * `legacy-keyring-unreachable`). Returns `null` when no legacy token is + * present. The synthesised account uses whatever identity fields the v1 + * config has — for pre-#71 configs the id/label may be empty, but the + * runtime never renders those (status's `fetchLive` re-derives from the + * API), so empty placeholders are safe. + */ +async function readLegacyTokenSnapshot(): Promise<{ + token: string + account: OutlineAccount +} | null> { + const config = await getConfig() + const token = config.api_token?.trim() || null + if (!token) return null + return { + token, + account: makeOutlineAccount({ + id: config.auth_user_id ?? '', + label: config.auth_user_name ?? '', + baseUrl: config.base_url, + oauthClientId: config.oauth_client_id, + teamName: config.auth_team_name, + }), } +} + +/** + * Best-effort discharge of v1 plaintext state. Runs before a write/clear + * when migration is inconclusive so v2 writes aren't shadowed by a stale + * legacy token. Failures are swallowed — the marker is what gates + * re-migration, not the absence of these fields. + */ +async function dischargeLegacyState(): Promise { + await updateConfig({ + api_token: undefined, + base_url: undefined, + oauth_client_id: undefined, + auth_user_id: undefined, + auth_user_name: undefined, + auth_team_name: undefined, + }).catch(() => undefined) +} - function refMismatch(ref: AccountRef): CliError { - return new CliError('ACCOUNT_NOT_FOUND', `No stored account matches "${ref}".`) +/** + * Memoised one-shot migration trigger. Resolves with `null` on rejection so + * the CLI never fails to start because of a migration error — the legacy + * snapshot fallback handles that case. Tests reset the memo with + * `vi.resetModules()` + a dynamic re-import. + */ +let migrationPromise: Promise | null> | undefined +function ensureMigrated(): Promise | null> { + if (!migrationPromise) { + migrationPromise = runMigrateLegacyAuth({ silent: true }).catch(() => null) } + return migrationPromise +} - return { +/** + * True when the v2 store is empty but a legacy v1 token snapshot is still + * the only thing keeping the CLI authenticated — typically because + * `migrateLegacyAuth` couldn't reach the Outline API to identify the + * account (`MigrateSkipReason: 'identify-failed'`) or the OS keyring is + * unreachable (`'legacy-keyring-unreachable'`, although outline has no + * v1 keyring slot to read from). Useful for downstream diagnostics. + */ +export async function isLegacyAuthActive(): Promise { + const result = await ensureMigrated() + if (result !== null && migrationIsConclusive(result)) return false + const legacy = await readLegacyTokenSnapshot() + return legacy !== null +} + +/** + * `OUTLINE_API_TOKEN` short-circuits `active()` only when no explicit ref + * is supplied — cli-core's `KeyringTokenStore` doesn't know about the env + * var, and an explicit ref means the caller targets a specific stored + * account. + * + * `ensureMigrated()` runs on every stored-state op so the lazy migration + * fires on first command. When migration isn't conclusive: + * - `active()` falls back to the legacy snapshot, honouring `ref` so it + * can't resolve to a different account than the caller asked for. + * - `set()` / `clear()` discharge legacy state on disk first so v2 writes + * aren't shadowed by a stale v1 token on the next read. + */ +export function createOutlineTokenStore(): OutlineTokenStore { + const inner = createKeyringTokenStore({ + serviceName: SECURE_STORE_SERVICE, + userRecords: createOutlineUserRecordStore(), + recordsLocation: getConfigPath(), + matchAccount: matchOutlineAccount, + }) + async function maybeDischargeLegacy(): Promise { + const result = await ensureMigrated() + if (result === null || !migrationIsConclusive(result)) { + await dischargeLegacyState() + } + } + return Object.assign(Object.create(inner) as OutlineTokenStore, { async active(ref?: AccountRef) { - // Env token wins per the `getApiToken` cascade. Surface it as a - // snapshot with placeholder identity fields — `status`'s - // `fetchLive` re-derives the canonical account from the API, so - // the empty id/label here are never rendered. Returning a - // snapshot here is what makes `attachStatusCommand` / - // `attachLogoutCommand` work for `OUTLINE_API_TOKEN`-only users. - const envToken = process.env.OUTLINE_API_TOKEN?.trim() - if (envToken) { - const account: OutlineAccount = { - id: '', - label: '', - baseUrl: await getBaseUrl(), - oauthClientId: '', + if (ref === undefined) { + const envToken = process.env[TOKEN_ENV_VAR]?.trim() + if (envToken) { + return { + token: envToken, + account: makeOutlineAccount({ + id: '', + label: '', + baseUrl: await getBaseUrl(), + }), + } } - if (ref !== undefined && !matchesRef(account, ref)) throw refMismatch(ref) - return { token: envToken, account } } - const snapshot = deriveSnapshot(await getConfig()) - if (ref === undefined) return snapshot - if (!snapshot || !matchesRef(snapshot.account, ref)) throw refMismatch(ref) - return snapshot + const result = await ensureMigrated() + if (result === null || !migrationIsConclusive(result)) { + const legacy = await readLegacyTokenSnapshot() + if (legacy && (ref === undefined || matchOutlineAccount(legacy.account, ref))) { + return legacy + } + } + return inner.active(ref) }, - async set(account, token) { - await updateConfig({ - api_token: token, - base_url: account.baseUrl, - oauth_client_id: account.oauthClientId, - auth_user_id: account.id, - auth_user_name: account.label, - auth_team_name: account.teamName, - }) + async set(account: OutlineAccount, token: string) { + await maybeDischargeLegacy() + return inner.set(account, token) }, async clear(ref?: AccountRef) { - // With `ref`, validate before touching storage so a mismatch is - // an `ACCOUNT_NOT_FOUND` error rather than a silent success — - // `attachLogoutCommand` treats any non-throwing `clear()` as - // success. Load the config once and hand it to `clearConfig` - // so ref-based logout stays at one read + one write. - const config = await getConfig() - if (ref !== undefined) { - const snapshot = deriveSnapshot(config) - if (!snapshot || !matchesRef(snapshot.account, ref)) throw refMismatch(ref) - } - await clearConfig(config) + await maybeDischargeLegacy() + return inner.clear(ref) }, async list() { - const snapshot = deriveSnapshot(await getConfig()) - return snapshot ? [{ account: snapshot.account, isDefault: true }] : [] + await ensureMigrated() + return inner.list() }, async setDefault(ref: AccountRef) { - const snapshot = deriveSnapshot(await getConfig()) - if (!snapshot || !matchesRef(snapshot.account, ref)) throw refMismatch(ref) - // Single-user store — already the default once `ref` matches. + await ensureMigrated() + return inner.setDefault(ref) }, - } + }) +} + +/** + * Where the currently-active token lives. Returns `'config-file'` whenever + * a plaintext token is on disk — the v2 `fallbackToken` field or the v1 + * `api_token` slot — so diagnostics report the security-relevant state + * accurately. + */ +export async function getActiveTokenSource(): Promise<'env' | 'secure-store' | 'config-file'> { + if (process.env[TOKEN_ENV_VAR]?.trim()) return 'env' + const config = await getConfig() + if (config.api_token?.trim()) return 'config-file' + const record = getDefaultUserRecord(config) + if (!record) return 'secure-store' + return record.fallbackToken ? 'config-file' : 'secure-store' } diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 1077fab..0fe1f3f 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -1,67 +1,65 @@ -import { type Config, getConfig, setConfig } from './config.js' +import { SecureStoreUnavailableError } from '@doist/cli-core/auth' +import { createOutlineTokenStore, getActiveTokenSource } from './auth-provider.js' +import { getConfig } from './config.js' +import { CliError } from './errors.js' +import { getDefaultUserRecord } from './user-records.js' -const DEFAULT_BASE_URL = 'https://app.getoutline.com' +export { SecureStoreUnavailableError, getActiveTokenSource } -export async function getApiToken(): Promise { - const envToken = process.env.OUTLINE_API_TOKEN - if (envToken) return envToken +export const TOKEN_ENV_VAR = 'OUTLINE_API_TOKEN' - const config = await getConfig() - if (config.api_token) return config.api_token +const DEFAULT_BASE_URL = 'https://app.getoutline.com' - throw new Error('No API token found. Set OUTLINE_API_TOKEN env var or run: ol auth login') +export class NoTokenError extends CliError { + constructor() { + super( + 'NO_TOKEN', + `No API token found. Set ${TOKEN_ENV_VAR} env var or run: ol auth login`, + ['Set OUTLINE_API_TOKEN or run: ol auth login'], + 'info', + ) + this.name = 'NoTokenError' + } } +/** + * Read the active token. The keyring-backed store wraps env-var precedence + * internally and falls back to the legacy plaintext snapshot when migration + * hasn't completed. + */ +export async function getApiToken(): Promise { + const snapshot = await createOutlineTokenStore().active() + if (!snapshot || !snapshot.token) throw new NoTokenError() + return snapshot.token +} + +/** + * Base URL cascade: env var → default user record (v2) → legacy + * `base_url` config (v1) → built-in default. The record takes priority + * over the legacy slot so post-migration logins keep defaulting to the + * same Outline instance. + */ export async function getBaseUrl(): Promise { const envUrl = process.env.OUTLINE_URL if (envUrl) return envUrl.replace(/\/$/, '') const config = await getConfig() + const record = getDefaultUserRecord(config) + if (record?.account.baseUrl) return record.account.baseUrl.replace(/\/$/, '') if (config.base_url) return config.base_url.replace(/\/$/, '') - return DEFAULT_BASE_URL } +/** + * OAuth client id cascade: env var → default user record (v2) → legacy + * `oauth_client_id` config (v1) → undefined (caller prompts). + */ export async function getOAuthClientId(): Promise { const envClientId = process.env.OUTLINE_OAUTH_CLIENT_ID if (envClientId) return envClientId const config = await getConfig() + const record = getDefaultUserRecord(config) + if (record?.account.oauthClientId) return record.account.oauthClientId return config.oauth_client_id } - -export async function getTokenSource(): Promise<'env' | 'config' | null> { - if (process.env.OUTLINE_API_TOKEN) return 'env' - const config = await getConfig() - if (config.api_token) return 'config' - return null -} - -/** - * Clear the auth-related keys without deleting the file. The config is now - * shared with non-auth settings (notably `update_channel`); a blanket unlink - * would silently reset the user's update-channel preference too. - * - * Pass `existing` to skip the read when the caller has already loaded the - * config (e.g. ref-validated logout) — keeps that flow at one read + one - * write instead of two reads. - */ -export async function clearConfig(existing?: Partial): Promise { - const config = existing ?? (await getConfig()) - const { - api_token, - base_url, - oauth_client_id, - auth_user_id, - auth_user_name, - auth_team_name, - ...rest - } = config - void api_token - void base_url - void oauth_client_id - void auth_user_id - void auth_user_name - void auth_team_name - await setConfig(rest as Config) -} diff --git a/src/lib/config.ts b/src/lib/config.ts index baf7dda..46ce074 100644 --- a/src/lib/config.ts +++ b/src/lib/config.ts @@ -8,7 +8,29 @@ import { const APP_NAME = 'outline-cli' +/** + * One row of the `users[]` array. `id` is the Outline user UUID. `token` is + * a plaintext fallback persisted only when the OS keyring is unavailable at + * write time (WSL, headless Linux, missing native binary). + */ +export type StoredUser = { + id: string + name: string + base_url?: string + oauth_client_id?: string + team_name?: string + token?: string +} + export type Config = CoreConfig & { + config_version?: number + users?: StoredUser[] + default_user_id?: string + + /** + * Legacy v1 single-user fields. Read by the migration helper; removed + * from disk by `cleanupLegacyConfig` after the v2 record write succeeds. + */ api_token?: string base_url?: string oauth_client_id?: string diff --git a/src/lib/migrate-auth.ts b/src/lib/migrate-auth.ts new file mode 100644 index 0000000..44399b8 --- /dev/null +++ b/src/lib/migrate-auth.ts @@ -0,0 +1,96 @@ +import { type MigrateAuthResult, migrateLegacyAuth } from '@doist/cli-core/auth' +import { fetchWithRetry } from '../transport/fetch-with-retry.js' +import { LEGACY_KEYRING_ACCOUNT, SECURE_STORE_SERVICE } from './auth-constants.js' +import { getConfig, updateConfig } from './config.js' +import { makeOutlineAccount, type OutlineAccount } from './outline-account.js' +import { createOutlineUserRecordStore } from './user-records.js' + +/** + * Pinned to this migration's target schema. Decoupled from any future + * `config_version` bump so this helper doesn't re-run for users already + * on v2 or beyond. + */ +const V2_SCHEMA_VERSION = 2 + +const DEFAULT_BASE_URL = 'https://app.getoutline.com' + +type AuthInfoResponse = { + data: { + user: { id: string; name: string } + team: { name: string } + } +} + +/** + * Direct `auth.info` call used only by the migration path. We bypass + * `apiRequest` (and its spinner + `getApiToken`/`getBaseUrl` cascade) so + * this module stays out of the runtime auth/token-store import graph and + * so the silent migration doesn't render a spinner during postinstall-style + * invocations. + */ +async function identifyOutlineAccount(token: string): Promise { + const config = await getConfig() + const baseUrl = (config.base_url ?? DEFAULT_BASE_URL).replace(/\/$/, '') + const res = await fetchWithRetry({ + url: `${baseUrl}/api/auth.info`, + options: { + method: 'POST', + headers: { + 'Content-Type': 'application/json', + Authorization: `Bearer ${token}`, + }, + body: JSON.stringify({}), + }, + }) + if (!res.ok) { + throw new Error(`auth.info failed: ${res.status} ${res.statusText}`) + } + const json = (await res.json()) as AuthInfoResponse + return makeOutlineAccount({ + id: json.data.user.id, + label: json.data.user.name, + baseUrl, + oauthClientId: config.oauth_client_id ?? '', + teamName: json.data.team.name, + }) +} + +/** + * One-time migration of v1 plaintext auth state into the v2 `users[]` + * shape. Called lazily by `createOutlineTokenStore` on first store access. + * Idempotent via the `config_version` marker — once set to `>= 2`, this + * helper short-circuits with `already-migrated`. + */ +export async function runMigrateLegacyAuth( + options: { silent: boolean } = { silent: true }, +): Promise> { + return migrateLegacyAuth({ + serviceName: SECURE_STORE_SERVICE, + legacyAccount: LEGACY_KEYRING_ACCOUNT, + userRecords: createOutlineUserRecordStore(), + hasMigrated: async () => { + const config = await getConfig() + return (config.config_version ?? 0) >= V2_SCHEMA_VERSION + }, + markMigrated: async () => { + await updateConfig({ config_version: V2_SCHEMA_VERSION }) + }, + loadLegacyPlaintextToken: async () => { + const config = await getConfig() + return config.api_token?.trim() || null + }, + identifyAccount: identifyOutlineAccount, + cleanupLegacyConfig: async () => { + await updateConfig({ + api_token: undefined, + base_url: undefined, + oauth_client_id: undefined, + auth_user_id: undefined, + auth_user_name: undefined, + auth_team_name: undefined, + }) + }, + silent: options.silent, + logPrefix: 'outline-cli', + }) +} diff --git a/src/lib/outline-account.ts b/src/lib/outline-account.ts new file mode 100644 index 0000000..6590cb7 --- /dev/null +++ b/src/lib/outline-account.ts @@ -0,0 +1,37 @@ +import type { AuthAccount } from '@doist/cli-core/auth' + +/** + * Narrow account shape persisted by the keyring-backed token store. + * + * `id` is the Outline user UUID; `label` is the user's display name. + * `baseUrl` and `oauthClientId` ride along on the account because Outline + * is self-hostable — every persisted token is tied to the instance it was + * issued for, not to a global default. `teamName` is rendered by + * `ol auth status`; it's the only field we cache for display. + */ +export type OutlineAccount = AuthAccount & { + id: string + label: string + baseUrl: string + oauthClientId: string + teamName?: string +} + +const DEFAULT_BASE_URL = 'https://app.getoutline.com' + +/** Canonical `OutlineAccount` factory. Applies the `baseUrl` default. */ +export function makeOutlineAccount(input: { + id: string + label: string + baseUrl?: string + oauthClientId?: string + teamName?: string +}): OutlineAccount { + return { + id: input.id, + label: input.label, + baseUrl: input.baseUrl ?? DEFAULT_BASE_URL, + oauthClientId: input.oauthClientId ?? '', + teamName: input.teamName, + } +} diff --git a/src/lib/user-records.ts b/src/lib/user-records.ts new file mode 100644 index 0000000..4e7f7c9 --- /dev/null +++ b/src/lib/user-records.ts @@ -0,0 +1,86 @@ +import type { UserRecord, UserRecordStore } from '@doist/cli-core/auth' +import { type Config, getConfig, type StoredUser, updateConfig } from './config.js' +import { makeOutlineAccount, type OutlineAccount } from './outline-account.js' + +/** + * `UserRecordStore` adapter over `config.users[]`. `StoredUser.token` is + * surfaced as `fallbackToken` — cli-core's name for the plaintext copy + * persisted only when the keyring is unreachable. + */ +export function createOutlineUserRecordStore(): UserRecordStore { + return { + async list() { + const config = await getConfig() + return (config.users ?? []).map(toRecord) + }, + async upsert(record) { + const config = await getConfig() + const existing = config.users ?? [] + const next = fromRecord(record) + const index = existing.findIndex((u) => u.id === record.account.id) + const users = + index >= 0 + ? [...existing.slice(0, index), next, ...existing.slice(index + 1)] + : [...existing, next] + await updateConfig({ users }) + }, + async remove(id) { + const config = await getConfig() + const existing = config.users ?? [] + const index = existing.findIndex((u) => u.id === id) + if (index < 0) return + const users = [...existing.slice(0, index), ...existing.slice(index + 1)] + const updates: { users: StoredUser[]; default_user_id?: undefined } = { users } + if (config.default_user_id === id) updates.default_user_id = undefined + await updateConfig(updates) + }, + async getDefaultId() { + const config = await getConfig() + return config.default_user_id ?? null + }, + async setDefaultId(id) { + await updateConfig({ default_user_id: id ?? undefined }) + }, + } +} + +/** + * Resolve the default-or-first `UserRecord` from an already-loaded config. + * Returns `null` when no users are stored. + */ +export function getDefaultUserRecord(config: Partial): UserRecord | null { + const users = config.users ?? [] + if (users.length === 0) return null + const defaultId = config.default_user_id + const user = (defaultId && users.find((u) => u.id === defaultId)) || users[0] + return toRecord(user) +} + +function toRecord(user: StoredUser): UserRecord { + const account = makeOutlineAccount({ + id: user.id, + label: user.name, + baseUrl: user.base_url, + oauthClientId: user.oauth_client_id, + teamName: user.team_name, + }) + const trimmed = user.token?.trim() + const record: UserRecord = { account } + if (trimmed) record.fallbackToken = trimmed + 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() + const next: StoredUser = { + id: record.account.id, + name: record.account.label, + } + 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 + return next +} From 102968fefabbe1c16ed04a816a3b3481ac33a947 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sun, 17 May 2026 16:35:16 +0100 Subject: [PATCH 2/3] fix(auth): address PR #73 review (atomicity, dedupe, coverage) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 — correctness bugs - `active()` now tries the v2 store first and only falls back to the legacy snapshot when the v2 store is empty. A stale `api_token` can no longer shadow a successful v2 write even if the best-effort legacy cleanup hasn't run. - `set()` / `clear()` now discharge legacy state AFTER the v2 op succeeds. Previously a failing keyring write would erase the user's only working credentials before the v2 attempt. - Add direct unit tests for `createOutlineUserRecordStore` mutation methods (`upsert` / `remove` / `setDefaultId`) and `getDefaultUserRecord`. Previously zero coverage because tests mocked cli-core's keyring layer. P2 — should-fix - Drop `Object.create(inner) as OutlineTokenStore` prototype-swap + forced cast. Return an explicit delegating object that forwards `getLastStorageResult` / `getLastClearResult` by call. - Fix `getActiveTokenSource` to mirror `active()`s resolution order — v2 record check now runs before the v1 plaintext slot check, so a stale `api_token` surviving a `cleanupLegacyConfig` failure no longer makes `auth status` lie about the source. - Dedupe `TOKEN_ENV_VAR` (now in auth-constants.ts, imported by both auth.ts and auth-provider.ts). - Dedupe `DEFAULT_BASE_URL` (single source in outline-account.ts). - Dedupe legacy-clear payload — shared `LEGACY_CLEAR_PAYLOAD` constant in auth-constants.ts, used by both `migrate-auth.cleanupLegacyConfig` and `auth-provider.dischargeLegacyState`. - Hoist `createOutlineTokenStore()` to a module-level singleton in auth.ts so the request hot path doesn't rebuild keyring + user-record adapters per API POST. - Add 10s timeout to migration `auth.info` call so a stalled connection can't hang the CLI indefinitely. - Restore the `auth logout --ndjson` silent-stdout test. The storage-result confirmation now flowing through `logTokenStorageResult` makes this invariant more important, not less. P3 — nits - Use `${TOKEN_ENV_VAR}` template in NoTokenError hint. - Drop forced `as AuthInfoResponse` cast in migrate-auth. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/__tests__/auth-command.test.ts | 14 +++ src/__tests__/auth-provider.test.ts | 39 ++++-- src/__tests__/user-records.test.ts | 180 ++++++++++++++++++++++++++++ src/lib/auth-constants.ts | 19 +++ src/lib/auth-provider.ts | 90 ++++++++------ src/lib/auth.ts | 29 +++-- src/lib/migrate-auth.ts | 27 +++-- src/lib/outline-account.ts | 2 +- 8 files changed, 334 insertions(+), 66 deletions(-) create mode 100644 src/__tests__/user-records.test.ts diff --git a/src/__tests__/auth-command.test.ts b/src/__tests__/auth-command.test.ts index 349a68d..48f72d8 100644 --- a/src/__tests__/auth-command.test.ts +++ b/src/__tests__/auth-command.test.ts @@ -209,4 +209,18 @@ describe('auth logout subcommand', () => { expect(logs).toHaveLength(1) expect(JSON.parse(logs[0])).toEqual({ ok: true }) }) + + it('stays silent on stdout under --ndjson (no human storage-result line leaks)', async () => { + // Critical: logout now surfaces a storage-result confirmation via + // `logTokenStorageResult` on success. NDJSON consumers expect a + // clean stdout — any human-readable line here would corrupt the + // stream. Guards the `isMachineOutput` branch in + // `logTokenStorageResult`. + const { logs } = captureLogs() + + const program = await buildProgram() + await program.parseAsync(['node', 'ol', 'auth', 'logout', '--ndjson']) + + expect(logs).toEqual([]) + }) }) diff --git a/src/__tests__/auth-provider.test.ts b/src/__tests__/auth-provider.test.ts index 59d7ea0..84c20a5 100644 --- a/src/__tests__/auth-provider.test.ts +++ b/src/__tests__/auth-provider.test.ts @@ -253,7 +253,7 @@ describe('createOutlineTokenStore', () => { expect(migrateMocks.runMigrateLegacyAuth).toHaveBeenCalledWith({ silent: true }) }) - it('falls back to the legacy plaintext snapshot when migration is skipped', async () => { + it('falls back to the legacy plaintext snapshot only when the v2 store is empty', async () => { migrateMocks.runMigrateLegacyAuth.mockResolvedValue(SKIPPED_RESULT) configMocks.getConfig.mockResolvedValue(LEGACY_CONFIG) const createOutlineTokenStore = await loadCreateOutlineTokenStore() @@ -264,7 +264,9 @@ describe('createOutlineTokenStore', () => { token: LEGACY_CONFIG.api_token, account: STORED_ACCOUNT, }) - expect(keyringMocks.inner.active).not.toHaveBeenCalled() + // v2 consulted first (returned null per the beforeEach default), + // then the legacy snapshot served the answer. + expect(keyringMocks.inner.active).toHaveBeenCalledTimes(1) }) it('delegates to the v2 store when migration is conclusive (no-legacy-state) — no legacy read attempt', async () => { @@ -287,7 +289,6 @@ describe('createOutlineTokenStore', () => { expect(snapshot?.token).toBe(LEGACY_CONFIG.api_token) expect(snapshot?.account.id).toBe('user-uuid') - expect(keyringMocks.inner.active).not.toHaveBeenCalled() }) it('legacy snapshot synthesises empty id/label when the v1 config never carried persisted identity fields', async () => { @@ -340,6 +341,20 @@ describe('createOutlineTokenStore', () => { expect(configMocks.updateConfig).not.toHaveBeenCalled() }) + + it('set() does NOT discharge legacy state when the v2 write fails (atomicity)', async () => { + // Regression test for the pre-fix order where legacy fields were + // erased before the v2 write — a failing keyring call would leave + // the user with no recoverable credentials. + migrateMocks.runMigrateLegacyAuth.mockResolvedValue(SKIPPED_RESULT) + keyringMocks.inner.set.mockRejectedValue(new Error('keyring boom')) + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + + await expect(createOutlineTokenStore().set(STORED_ACCOUNT, 'tk_new')).rejects.toThrow( + 'keyring boom', + ) + expect(configMocks.updateConfig).not.toHaveBeenCalled() + }) }) describe('matchOutlineAccount', () => { @@ -364,7 +379,7 @@ describe('getActiveTokenSource', () => { vi.unstubAllEnvs() }) - it('reports the storage location of the active token across env / v1 plaintext / v2 record / empty', async () => { + it('reports the storage location of the active token, mirroring active()s resolution order', async () => { const { getActiveTokenSource } = await import('../lib/auth-provider.js') vi.stubEnv(TOKEN_ENV_VAR, 'tk') @@ -372,9 +387,6 @@ describe('getActiveTokenSource', () => { await expect(getActiveTokenSource()).resolves.toBe('env') vi.unstubAllEnvs() - configMocks.getConfig.mockResolvedValue({ api_token: 'tk' }) - await expect(getActiveTokenSource()).resolves.toBe('config-file') - configMocks.getConfig.mockResolvedValue({ users: [{ id: 'u', name: 'Ada', token: 'plaintext' }], }) @@ -383,6 +395,19 @@ describe('getActiveTokenSource', () => { configMocks.getConfig.mockResolvedValue({ users: [{ id: 'u', name: 'Ada' }] }) await expect(getActiveTokenSource()).resolves.toBe('secure-store') + // v2 record (even without fallbackToken) wins over a lingering v1 + // plaintext slot — `active()` ignores `api_token` once a record + // exists, so this classifier must too. Regression guard for the + // pre-fix order where the v1 check ran first. + configMocks.getConfig.mockResolvedValue({ + api_token: 'stale-v1', + users: [{ id: 'u', name: 'Ada' }], + }) + await expect(getActiveTokenSource()).resolves.toBe('secure-store') + + configMocks.getConfig.mockResolvedValue({ api_token: 'tk' }) + await expect(getActiveTokenSource()).resolves.toBe('config-file') + configMocks.getConfig.mockResolvedValue({}) await expect(getActiveTokenSource()).resolves.toBe('secure-store') }) diff --git a/src/__tests__/user-records.test.ts b/src/__tests__/user-records.test.ts new file mode 100644 index 0000000..acce45d --- /dev/null +++ b/src/__tests__/user-records.test.ts @@ -0,0 +1,180 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { STORED_ACCOUNT } from './_fixtures/auth.js' + +const configMock = vi.hoisted(() => ({ + getConfig: vi.fn(), + updateConfig: vi.fn(), +})) + +vi.mock('../lib/config.js', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, getConfig: configMock.getConfig, updateConfig: configMock.updateConfig } +}) + +const ADA = { + id: 'user-uuid', + name: 'Ada', + base_url: 'https://wiki.example.com', + oauth_client_id: 'cid-xyz', + team_name: 'Analytics', +} as const + +const GRACE = { + id: 'grace-uuid', + name: 'Grace', + base_url: 'https://other.example.com', +} as const + +describe('createOutlineUserRecordStore', () => { + beforeEach(() => { + configMock.getConfig.mockReset().mockResolvedValue({}) + configMock.updateConfig.mockReset().mockResolvedValue(undefined) + }) + + it('list() maps StoredUser → UserRecord, surfacing token as fallbackToken when present', async () => { + configMock.getConfig.mockResolvedValue({ + users: [ADA, { ...GRACE, token: ' plaintext-token ' }], + }) + const { createOutlineUserRecordStore } = await import('../lib/user-records.js') + + const records = await createOutlineUserRecordStore().list() + + expect(records).toHaveLength(2) + expect(records[0]).toEqual({ account: STORED_ACCOUNT }) + expect(records[0].fallbackToken).toBeUndefined() + expect(records[1].account.id).toBe('grace-uuid') + expect(records[1].fallbackToken).toBe('plaintext-token') + }) + + it('upsert() appends a brand-new record', async () => { + configMock.getConfig.mockResolvedValue({ users: [ADA] }) + const { createOutlineUserRecordStore } = await import('../lib/user-records.js') + + await createOutlineUserRecordStore().upsert({ + account: { + id: 'grace-uuid', + label: 'Grace', + baseUrl: 'https://other.example.com', + oauthClientId: '', + }, + }) + + expect(configMock.updateConfig).toHaveBeenCalledWith({ + users: [ + ADA, + { id: 'grace-uuid', name: 'Grace', base_url: 'https://other.example.com' }, + ], + }) + }) + + it('upsert() REPLACES an existing record in-place (preserves order, drops absent fallbackToken)', async () => { + // Critical: an absent `fallbackToken` on the new record must + // erase any prior plaintext slot. Cli-core's contract says + // `upsert` is replace-not-merge so a stale fallback can't + // shadow a fresh keyring-backed write. + configMock.getConfig.mockResolvedValue({ + users: [{ ...ADA, token: 'stale-plaintext' }, GRACE], + }) + const { createOutlineUserRecordStore } = await import('../lib/user-records.js') + + await createOutlineUserRecordStore().upsert({ account: STORED_ACCOUNT }) + + expect(configMock.updateConfig).toHaveBeenCalledWith({ + users: [ADA, GRACE], + }) + }) + + it('upsert() persists fallbackToken back to the StoredUser.token slot', async () => { + configMock.getConfig.mockResolvedValue({ users: [] }) + const { createOutlineUserRecordStore } = await import('../lib/user-records.js') + + await createOutlineUserRecordStore().upsert({ + account: STORED_ACCOUNT, + fallbackToken: 'wsl-plaintext', + }) + + expect(configMock.updateConfig).toHaveBeenCalledWith({ + users: [{ ...ADA, token: 'wsl-plaintext' }], + }) + }) + + it('remove() drops the matching record', async () => { + configMock.getConfig.mockResolvedValue({ users: [ADA, GRACE] }) + const { createOutlineUserRecordStore } = await import('../lib/user-records.js') + + await createOutlineUserRecordStore().remove('user-uuid') + + expect(configMock.updateConfig).toHaveBeenCalledWith({ users: [GRACE] }) + }) + + it('remove() also clears default_user_id when it matched the removed record', async () => { + configMock.getConfig.mockResolvedValue({ + users: [ADA, GRACE], + default_user_id: 'user-uuid', + }) + const { createOutlineUserRecordStore } = await import('../lib/user-records.js') + + await createOutlineUserRecordStore().remove('user-uuid') + + expect(configMock.updateConfig).toHaveBeenCalledWith({ + users: [GRACE], + default_user_id: undefined, + }) + }) + + it('remove() is a no-op when the id is unknown (does not touch config)', async () => { + configMock.getConfig.mockResolvedValue({ users: [ADA] }) + const { createOutlineUserRecordStore } = await import('../lib/user-records.js') + + await createOutlineUserRecordStore().remove('nobody') + + expect(configMock.updateConfig).not.toHaveBeenCalled() + }) + + it('getDefaultId / setDefaultId round-trip via the default_user_id config field', async () => { + const { createOutlineUserRecordStore } = await import('../lib/user-records.js') + const store = createOutlineUserRecordStore() + + configMock.getConfig.mockResolvedValueOnce({}) + await expect(store.getDefaultId()).resolves.toBeNull() + + configMock.getConfig.mockResolvedValueOnce({ default_user_id: 'user-uuid' }) + await expect(store.getDefaultId()).resolves.toBe('user-uuid') + + await store.setDefaultId('grace-uuid') + expect(configMock.updateConfig).toHaveBeenCalledWith({ default_user_id: 'grace-uuid' }) + + await store.setDefaultId(null) + expect(configMock.updateConfig).toHaveBeenLastCalledWith({ default_user_id: undefined }) + }) +}) + +describe('getDefaultUserRecord', () => { + it('returns null when no users are stored', async () => { + const { getDefaultUserRecord } = await import('../lib/user-records.js') + expect(getDefaultUserRecord({})).toBeNull() + expect(getDefaultUserRecord({ users: [] })).toBeNull() + }) + + it('returns the pinned default when default_user_id matches', async () => { + const { getDefaultUserRecord } = await import('../lib/user-records.js') + const record = getDefaultUserRecord({ + users: [ADA, GRACE], + default_user_id: 'grace-uuid', + }) + expect(record?.account.id).toBe('grace-uuid') + }) + + it('falls back to the first record when default_user_id is absent or stale', async () => { + const { getDefaultUserRecord } = await import('../lib/user-records.js') + + const noDefault = getDefaultUserRecord({ users: [ADA, GRACE] }) + expect(noDefault?.account.id).toBe('user-uuid') + + const staleDefault = getDefaultUserRecord({ + users: [ADA, GRACE], + default_user_id: 'nobody', + }) + expect(staleDefault?.account.id).toBe('user-uuid') + }) +}) diff --git a/src/lib/auth-constants.ts b/src/lib/auth-constants.ts index 02e0aa7..360a950 100644 --- a/src/lib/auth-constants.ts +++ b/src/lib/auth-constants.ts @@ -1,6 +1,11 @@ +import type { Config } from './config.js' + /** OS keyring `service` identifier for every outline-cli secret. */ export const SECURE_STORE_SERVICE = 'outline-cli' +/** Env var that short-circuits the token store with a manually-supplied token. */ +export const TOKEN_ENV_VAR = 'OUTLINE_API_TOKEN' + /** * Legacy single-user keyring slot. Outline never wrote a token to the * keyring before this migration, so there is no real legacy slot to read — @@ -8,3 +13,17 @@ export const SECURE_STORE_SERVICE = 'outline-cli' * after migration is a harmless no-op on an empty entry. */ export const LEGACY_KEYRING_ACCOUNT = 'api-token' + +/** + * `updateConfig` payload that wipes every v1 auth key. Used by both the + * one-shot migration `cleanupLegacyConfig` and the runtime + * `dischargeLegacyState` so both paths stay in lockstep. + */ +export const LEGACY_CLEAR_PAYLOAD: Partial = { + api_token: undefined, + base_url: undefined, + oauth_client_id: undefined, + auth_user_id: undefined, + auth_user_name: undefined, + auth_team_name: undefined, +} diff --git a/src/lib/auth-provider.ts b/src/lib/auth-provider.ts index a78bd31..4ce0c73 100644 --- a/src/lib/auth-provider.ts +++ b/src/lib/auth-provider.ts @@ -10,7 +10,7 @@ import { } from '@doist/cli-core/auth' import { fetchWithRetry } from '../transport/fetch-with-retry.js' import { apiRequest } from './api.js' -import { SECURE_STORE_SERVICE } from './auth-constants.js' +import { LEGACY_CLEAR_PAYLOAD, SECURE_STORE_SERVICE, TOKEN_ENV_VAR } from './auth-constants.js' import { getBaseUrl, getOAuthClientId } from './auth.js' import { getConfig, getConfigPath, updateConfig } from './config.js' import { runMigrateLegacyAuth } from './migrate-auth.js' @@ -19,8 +19,6 @@ import { createOutlineUserRecordStore, getDefaultUserRecord } from './user-recor export type { OutlineAccount } from './outline-account.js' -const TOKEN_ENV_VAR = 'OUTLINE_API_TOKEN' - export type AuthInfoResponse = { user: { id: string; name: string; email: string } team: { name: string; subdomain: string } @@ -209,20 +207,17 @@ async function readLegacyTokenSnapshot(): Promise<{ } /** - * Best-effort discharge of v1 plaintext state. Runs before a write/clear - * when migration is inconclusive so v2 writes aren't shadowed by a stale - * legacy token. Failures are swallowed — the marker is what gates - * re-migration, not the absence of these fields. + * Discharge v1 plaintext state. Runs **after** a successful write/clear + * (never before) so a v2-op failure doesn't strand the user with no + * recoverable credentials, and we can prefer the v2 store on the next + * read regardless of whether this best-effort cleanup ran. + * + * Failures are swallowed because `active()` now reads the v2 store first + * and only falls back to the legacy snapshot when the v2 store is empty — + * a lingering `api_token` field can no longer shadow a fresh v2 write. */ async function dischargeLegacyState(): Promise { - await updateConfig({ - api_token: undefined, - base_url: undefined, - oauth_client_id: undefined, - auth_user_id: undefined, - auth_user_name: undefined, - auth_team_name: undefined, - }).catch(() => undefined) + await updateConfig(LEGACY_CLEAR_PAYLOAD).catch(() => undefined) } /** @@ -261,11 +256,15 @@ export async function isLegacyAuthActive(): Promise { * account. * * `ensureMigrated()` runs on every stored-state op so the lazy migration - * fires on first command. When migration isn't conclusive: - * - `active()` falls back to the legacy snapshot, honouring `ref` so it - * can't resolve to a different account than the caller asked for. - * - `set()` / `clear()` discharge legacy state on disk first so v2 writes - * aren't shadowed by a stale v1 token on the next read. + * fires on first command. `active()` always prefers the v2 store and only + * falls back to the legacy snapshot when the v2 store is empty *and* + * migration is inconclusive — that ordering means a stale v1 token can + * never shadow a successful v2 write, even if the best-effort legacy + * cleanup has not yet run. + * + * `set()` / `clear()` discharge legacy state **after** the v2 op succeeds + * (not before) so a failure in the v2 op doesn't strand the user with no + * recoverable credentials. */ export function createOutlineTokenStore(): OutlineTokenStore { const inner = createKeyringTokenStore({ @@ -280,7 +279,7 @@ export function createOutlineTokenStore(): OutlineTokenStore { await dischargeLegacyState() } } - return Object.assign(Object.create(inner) as OutlineTokenStore, { + return { async active(ref?: AccountRef) { if (ref === undefined) { const envToken = process.env[TOKEN_ENV_VAR]?.trim() @@ -295,22 +294,27 @@ export function createOutlineTokenStore(): OutlineTokenStore { } } } - const result = await ensureMigrated() - if (result === null || !migrationIsConclusive(result)) { - const legacy = await readLegacyTokenSnapshot() - if (legacy && (ref === undefined || matchOutlineAccount(legacy.account, ref))) { - return legacy - } + await ensureMigrated() + const fromStore = await inner.active(ref) + if (fromStore) return fromStore + + // v2 store empty — try the legacy snapshot. We don't gate this + // on migration status: even on conclusive results the snapshot + // is just `null` (cleanupLegacyConfig already ran), so the + // extra read is cheap and the branch is the same. + const legacy = await readLegacyTokenSnapshot() + if (legacy && (ref === undefined || matchOutlineAccount(legacy.account, ref))) { + return legacy } - return inner.active(ref) + return null }, async set(account: OutlineAccount, token: string) { + await inner.set(account, token) await maybeDischargeLegacy() - return inner.set(account, token) }, async clear(ref?: AccountRef) { + await inner.clear(ref) await maybeDischargeLegacy() - return inner.clear(ref) }, async list() { await ensureMigrated() @@ -320,20 +324,30 @@ export function createOutlineTokenStore(): OutlineTokenStore { await ensureMigrated() return inner.setDefault(ref) }, - }) + getLastStorageResult: () => inner.getLastStorageResult(), + getLastClearResult: () => inner.getLastClearResult(), + } } /** - * Where the currently-active token lives. Returns `'config-file'` whenever - * a plaintext token is on disk — the v2 `fallbackToken` field or the v1 - * `api_token` slot — so diagnostics report the security-relevant state - * accurately. + * Where the currently-active token lives. Mirrors the resolution order in + * `active()` so the answer can never contradict the token the runtime is + * actually using: + * + * 1. env var → `'env'` + * 2. v2 user record present → `fallbackToken` ? `'config-file'` : `'secure-store'` + * 3. v1 plaintext `api_token` slot → `'config-file'` (legacy snapshot) + * 4. nothing on disk → `'secure-store'` (neutral default for fresh CLI) + * + * Critically the v2 record check runs **before** the v1 slot check — if a + * stale `api_token` survives a best-effort `cleanupLegacyConfig` failure, + * `active()` ignores it (v2 record wins), and so do we. */ export async function getActiveTokenSource(): Promise<'env' | 'secure-store' | 'config-file'> { if (process.env[TOKEN_ENV_VAR]?.trim()) return 'env' const config = await getConfig() - if (config.api_token?.trim()) return 'config-file' const record = getDefaultUserRecord(config) - if (!record) return 'secure-store' - return record.fallbackToken ? 'config-file' : 'secure-store' + if (record) return record.fallbackToken ? 'config-file' : 'secure-store' + if (config.api_token?.trim()) return 'config-file' + return 'secure-store' } diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 0fe1f3f..81ec94e 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -1,34 +1,47 @@ import { SecureStoreUnavailableError } from '@doist/cli-core/auth' -import { createOutlineTokenStore, getActiveTokenSource } from './auth-provider.js' +import { TOKEN_ENV_VAR } from './auth-constants.js' +import { + createOutlineTokenStore, + getActiveTokenSource, + type OutlineTokenStore, +} from './auth-provider.js' import { getConfig } from './config.js' import { CliError } from './errors.js' +import { DEFAULT_BASE_URL } from './outline-account.js' import { getDefaultUserRecord } from './user-records.js' -export { SecureStoreUnavailableError, getActiveTokenSource } - -export const TOKEN_ENV_VAR = 'OUTLINE_API_TOKEN' - -const DEFAULT_BASE_URL = 'https://app.getoutline.com' +export { SecureStoreUnavailableError, getActiveTokenSource, TOKEN_ENV_VAR } export class NoTokenError extends CliError { constructor() { super( 'NO_TOKEN', `No API token found. Set ${TOKEN_ENV_VAR} env var or run: ol auth login`, - ['Set OUTLINE_API_TOKEN or run: ol auth login'], + [`Set ${TOKEN_ENV_VAR} or run: ol auth login`], 'info', ) this.name = 'NoTokenError' } } +/** + * Module-level token-store singleton. Built lazily on first call; reused + * across every `apiRequest` so the request hot path doesn't reconstruct + * the keyring + user-record adapters per POST. + */ +let storeSingleton: OutlineTokenStore | undefined +function tokenStore(): OutlineTokenStore { + if (!storeSingleton) storeSingleton = createOutlineTokenStore() + return storeSingleton +} + /** * Read the active token. The keyring-backed store wraps env-var precedence * internally and falls back to the legacy plaintext snapshot when migration * hasn't completed. */ export async function getApiToken(): Promise { - const snapshot = await createOutlineTokenStore().active() + const snapshot = await tokenStore().active() if (!snapshot || !snapshot.token) throw new NoTokenError() return snapshot.token } diff --git a/src/lib/migrate-auth.ts b/src/lib/migrate-auth.ts index 44399b8..f59e14e 100644 --- a/src/lib/migrate-auth.ts +++ b/src/lib/migrate-auth.ts @@ -1,8 +1,12 @@ import { type MigrateAuthResult, migrateLegacyAuth } from '@doist/cli-core/auth' import { fetchWithRetry } from '../transport/fetch-with-retry.js' -import { LEGACY_KEYRING_ACCOUNT, SECURE_STORE_SERVICE } from './auth-constants.js' +import { + LEGACY_CLEAR_PAYLOAD, + LEGACY_KEYRING_ACCOUNT, + SECURE_STORE_SERVICE, +} from './auth-constants.js' import { getConfig, updateConfig } from './config.js' -import { makeOutlineAccount, type OutlineAccount } from './outline-account.js' +import { DEFAULT_BASE_URL, makeOutlineAccount, type OutlineAccount } from './outline-account.js' import { createOutlineUserRecordStore } from './user-records.js' /** @@ -12,7 +16,12 @@ import { createOutlineUserRecordStore } from './user-records.js' */ const V2_SCHEMA_VERSION = 2 -const DEFAULT_BASE_URL = 'https://app.getoutline.com' +/** + * Cap migration's `auth.info` lookup so a stalled connection can't hang + * the CLI indefinitely on first command. cli-core treats the resulting + * timeout as `identify-failed`, falling back to the legacy snapshot. + */ +const IDENTIFY_TIMEOUT_MS = 10_000 type AuthInfoResponse = { data: { @@ -40,12 +49,13 @@ async function identifyOutlineAccount(token: string): Promise { Authorization: `Bearer ${token}`, }, body: JSON.stringify({}), + timeout: IDENTIFY_TIMEOUT_MS, }, }) if (!res.ok) { throw new Error(`auth.info failed: ${res.status} ${res.statusText}`) } - const json = (await res.json()) as AuthInfoResponse + const json: AuthInfoResponse = await res.json() return makeOutlineAccount({ id: json.data.user.id, label: json.data.user.name, @@ -81,14 +91,7 @@ export async function runMigrateLegacyAuth( }, identifyAccount: identifyOutlineAccount, cleanupLegacyConfig: async () => { - await updateConfig({ - api_token: undefined, - base_url: undefined, - oauth_client_id: undefined, - auth_user_id: undefined, - auth_user_name: undefined, - auth_team_name: undefined, - }) + await updateConfig(LEGACY_CLEAR_PAYLOAD) }, silent: options.silent, logPrefix: 'outline-cli', diff --git a/src/lib/outline-account.ts b/src/lib/outline-account.ts index 6590cb7..6e46cc9 100644 --- a/src/lib/outline-account.ts +++ b/src/lib/outline-account.ts @@ -17,7 +17,7 @@ export type OutlineAccount = AuthAccount & { teamName?: string } -const DEFAULT_BASE_URL = 'https://app.getoutline.com' +export const DEFAULT_BASE_URL = 'https://app.getoutline.com' /** Canonical `OutlineAccount` factory. Applies the `baseUrl` default. */ export function makeOutlineAccount(input: { From 76ba02e14cccedd89dd75836ebe3c49329e99fb9 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sun, 17 May 2026 16:58:24 +0100 Subject: [PATCH 3/3] fix(auth): address PR #73 second review (race, logout atomicity, dedupe) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit P1 — correctness - `auth-provider.ts`: `ensureMigrated()` now runs BEFORE every mutating v2 op (`set` / `clear`), and legacy discharge runs after the v2 op succeeds. Eliminates the race where a stale `api_token` survived `inner.set/clear` long enough for the post-op migration to grab it (which would either duplicate the freshly-logged-in account or revive auth right after logout). - `auth-provider.ts`: `clear()` now propagates the legacy-discharge failure instead of swallowing it. With v2 empty after a successful `inner.clear`, a silently-failed cleanup would let `active()` fall back to the surviving `api_token` — `ol auth logout` reporting success while leaving the user authenticated. Logout now fails loudly. `set()` retains best-effort swallow (v2 wins in `active()`). P2 — should-fix - `migrate-auth.hasMigrated` now also accepts `users.length > 0` as a "v2" signal, so fresh installs that login directly (no v1 marker ever written) don't get re-migrated on every launch. - `auth.getApiToken` short-circuits `OUTLINE_API_TOKEN` directly, skipping the token store's `active()` (which would call `getBaseUrl()` to synthesise an account just for the snapshot — redundant since `apiRequest` already resolves the base URL). - `_fixtures/auth.okResponse` / `errResponse` use the native `Response.json()` static helper instead of casting an object literal. - `__tests__/auth.test.ts` mocks `runMigrateLegacyAuth` directly instead of stubbing transitive `fetchWithRetry` failures. - Added regression tests: `clear()` atomicity (no discharge on v2 failure), `clear()` failure propagation, `set()` migration ordering, and `args.options.timeout === 10_000` in the migrate-auth test. - `logTokenStorageResult` exported from `commands/auth.ts` and tested directly — covers the secure-store stdout confirmation, the machine-mode suppression, and the warning-routes-to-stderr branch. Deferred (with rationale on PR thread) - P2.3 `getActiveTokenSource` precedence dedupe: real dedupe requires either extra hot-path disk I/O or augmenting cli-core's `TokenStore` contract. The drift is guarded by the existing regression test. - P2.5 import cycle: `auth.ts` ↔ `auth-provider.ts` is function-level safe today; extracting a new shared module just to break the cycle costs more LOC than the theoretical ESM risk warrants. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/__tests__/_fixtures/auth.ts | 18 ++----- src/__tests__/auth-command.test.ts | 48 +++++++++++++++++ src/__tests__/auth-provider.test.ts | 42 +++++++++++++++ src/__tests__/auth.test.ts | 16 +++--- src/__tests__/migrate-auth.test.ts | 4 ++ src/commands/auth.ts | 6 ++- src/lib/auth-provider.ts | 80 ++++++++++++++--------------- src/lib/auth.ts | 12 +++-- src/lib/migrate-auth.ts | 9 +++- 9 files changed, 166 insertions(+), 69 deletions(-) diff --git a/src/__tests__/_fixtures/auth.ts b/src/__tests__/_fixtures/auth.ts index 6fee15b..dc868ce 100644 --- a/src/__tests__/_fixtures/auth.ts +++ b/src/__tests__/_fixtures/auth.ts @@ -43,22 +43,12 @@ export const SKIPPED_RESULT: MigrateAuthResult = { detail: 'offline', } -/** Build a `Response`-shaped object whose `json()` resolves to `body`. */ +/** 200 `Response` with `body` as JSON, using the native static helper. */ export function okResponse(body: unknown): Response { - return { - ok: true, - status: 200, - statusText: 'OK', - json: async () => body, - } as Response + return Response.json(body) } -/** Build an error `Response` with the given status (default 500). */ +/** Error `Response` with the given status (default 500). */ export function errResponse(status: number, statusText = 'Error', body?: unknown): Response { - return { - ok: false, - status, - statusText, - json: async () => body ?? {}, - } as Response + return Response.json(body ?? {}, { status, statusText }) } diff --git a/src/__tests__/auth-command.test.ts b/src/__tests__/auth-command.test.ts index 48f72d8..9c87d11 100644 --- a/src/__tests__/auth-command.test.ts +++ b/src/__tests__/auth-command.test.ts @@ -224,3 +224,51 @@ describe('auth logout subcommand', () => { expect(logs).toEqual([]) }) }) + +describe('logTokenStorageResult', () => { + function captureStreams() { + const logs: string[] = [] + const errs: string[] = [] + vi.spyOn(console, 'log').mockImplementation((...a: unknown[]) => { + logs.push(a.join(' ')) + }) + vi.spyOn(console, 'error').mockImplementation((...a: unknown[]) => { + errs.push(a.join(' ')) + }) + return { logs, errs } + } + + it('prints the secure-store confirmation to stdout in human mode', async () => { + const { logs, errs } = captureStreams() + const { logTokenStorageResult } = await import('../commands/auth.js') + + logTokenStorageResult({ storage: 'secure-store' }, 'Token stored securely', false) + + expect(logs.some((l) => l.includes('Token stored securely'))).toBe(true) + expect(errs).toEqual([]) + }) + + it('suppresses the stdout confirmation in machine-output mode', async () => { + const { logs } = captureStreams() + const { logTokenStorageResult } = await import('../commands/auth.js') + + logTokenStorageResult({ storage: 'secure-store' }, 'Token stored securely', true) + + expect(logs).toEqual([]) + }) + + it('routes the keyring-fallback warning to stderr (in both human and machine modes)', async () => { + const { logs, errs } = captureStreams() + const { logTokenStorageResult } = await import('../commands/auth.js') + + logTokenStorageResult( + { storage: 'config-file', warning: 'system credential manager unavailable' }, + 'Token stored securely', + true, + ) + + // No stdout in machine mode, but warning still reaches operator on stderr. + expect(logs).toEqual([]) + expect(errs.some((e) => e.includes('system credential manager unavailable'))).toBe(true) + }) +}) diff --git a/src/__tests__/auth-provider.test.ts b/src/__tests__/auth-provider.test.ts index 84c20a5..2f666f4 100644 --- a/src/__tests__/auth-provider.test.ts +++ b/src/__tests__/auth-provider.test.ts @@ -355,6 +355,48 @@ describe('createOutlineTokenStore', () => { ) expect(configMocks.updateConfig).not.toHaveBeenCalled() }) + + it('clear() does NOT discharge legacy state when the v2 clear fails (atomicity)', async () => { + migrateMocks.runMigrateLegacyAuth.mockResolvedValue(SKIPPED_RESULT) + keyringMocks.inner.clear.mockRejectedValue(new Error('keyring boom')) + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + + await expect(createOutlineTokenStore().clear('user-uuid')).rejects.toThrow('keyring boom') + expect(configMocks.updateConfig).not.toHaveBeenCalled() + }) + + it('clear() PROPAGATES updateConfig failures (logout must be atomic, not silently partial)', async () => { + // If the v2 clear succeeded but the legacy discharge silently + // failed, the next `active()` call would fall back to the + // surviving plaintext `api_token` — i.e. "logout" leaves the + // user authenticated. Logout has to fail loudly instead. + migrateMocks.runMigrateLegacyAuth.mockResolvedValue(SKIPPED_RESULT) + configMocks.updateConfig.mockRejectedValue(new Error('disk full')) + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + + await expect(createOutlineTokenStore().clear('user-uuid')).rejects.toThrow('disk full') + expect(keyringMocks.inner.clear).toHaveBeenCalledWith('user-uuid') + }) + + it('set() runs migration BEFORE the v2 write (no post-write race that re-grabs the legacy token)', async () => { + // Regression test for the race where ensureMigrated only fired + // after inner.set/clear: migration would then see the stale + // api_token still on disk and migrate it on top of the fresh + // login, either duplicating accounts or reviving auth after logout. + const callOrder: string[] = [] + migrateMocks.runMigrateLegacyAuth.mockImplementation(async () => { + callOrder.push('migrate') + return { status: 'no-legacy-state' } + }) + keyringMocks.inner.set.mockImplementation(async () => { + callOrder.push('set') + }) + const createOutlineTokenStore = await loadCreateOutlineTokenStore() + + await createOutlineTokenStore().set(STORED_ACCOUNT, 'tk_new') + + expect(callOrder).toEqual(['migrate', 'set']) + }) }) describe('matchOutlineAccount', () => { diff --git a/src/__tests__/auth.test.ts b/src/__tests__/auth.test.ts index 6a2aa1a..c4dea1d 100644 --- a/src/__tests__/auth.test.ts +++ b/src/__tests__/auth.test.ts @@ -2,19 +2,19 @@ import { existsSync, mkdirSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { SKIPPED_RESULT } from './_fixtures/auth.js' const TEST_XDG = join(tmpdir(), `outline-cli-test-${process.pid}-auth`) const TEST_CONFIG_DIR = join(TEST_XDG, 'outline-cli') const TEST_CONFIG_PATH = join(TEST_CONFIG_DIR, 'config.json') -// Force `identifyAccount` to fail so migration returns `skipped` and the -// runtime falls back to the legacy plaintext snapshot — the v1 token in -// these fixtures has no live API behind it. cli-core's secure-store is left -// real because the env-token tests don't hit the keyring at all. -vi.mock('../transport/fetch-with-retry.js', () => ({ - fetchWithRetry: vi.fn(async () => { - throw new Error('offline (mocked)') - }), +// Force a `skipped` migration so the runtime falls back to the legacy +// plaintext snapshot — the v1 token in these fixtures has no live API +// behind it. Mocking `runMigrateLegacyAuth` directly is more robust than +// stubbing transitive network deps: tests don't have to know how +// migration internally decides to skip. +vi.mock('../lib/migrate-auth.js', () => ({ + runMigrateLegacyAuth: vi.fn(async () => SKIPPED_RESULT), })) describe('auth', () => { diff --git a/src/__tests__/migrate-auth.test.ts b/src/__tests__/migrate-auth.test.ts index ccccc4e..39e525d 100644 --- a/src/__tests__/migrate-auth.test.ts +++ b/src/__tests__/migrate-auth.test.ts @@ -65,6 +65,10 @@ describe('runMigrateLegacyAuth', () => { const args = fetchMock.fetchWithRetry.mock.calls[0][0] expect(args.url).toBe('https://wiki.example.com/api/auth.info') expect(args.options.headers.Authorization).toBe('Bearer tk_v1') + // Critical: timeout must be set or a stalled connection during + // lazy migration can hang every CLI invocation. Guards the + // `IDENTIFY_TIMEOUT_MS` constant in migrate-auth.ts. + expect(args.options.timeout).toBe(10_000) expect(account).toEqual({ id: 'user-uuid', label: 'Ada', diff --git a/src/commands/auth.ts b/src/commands/auth.ts index 09b0a5a..a2dc509 100644 --- a/src/commands/auth.ts +++ b/src/commands/auth.ts @@ -40,8 +40,12 @@ function resolvePreferredCallbackPort(): number { * confirmation goes to stdout, any keyring-fallback warning goes to stderr. * Pass `isMachineOutput: true` to suppress the stdout confirmation in * `--json` / `--ndjson` mode while still routing the warning to stderr. + * + * Exported for direct unit testing — the alternative (driving this via + * mocked cli-core login/logout hooks) would require stubbing the entire + * store contract just to assert two console calls. */ -function logTokenStorageResult( +export function logTokenStorageResult( result: TokenStorageResult, secureStoreMessage: string, isMachineOutput = false, diff --git a/src/lib/auth-provider.ts b/src/lib/auth-provider.ts index 4ce0c73..24045ad 100644 --- a/src/lib/auth-provider.ts +++ b/src/lib/auth-provider.ts @@ -207,17 +207,16 @@ async function readLegacyTokenSnapshot(): Promise<{ } /** - * Discharge v1 plaintext state. Runs **after** a successful write/clear - * (never before) so a v2-op failure doesn't strand the user with no - * recoverable credentials, and we can prefer the v2 store on the next - * read regardless of whether this best-effort cleanup ran. - * - * Failures are swallowed because `active()` now reads the v2 store first - * and only falls back to the legacy snapshot when the v2 store is empty — - * a lingering `api_token` field can no longer shadow a fresh v2 write. + * Discharge v1 plaintext state. Runs **after** a successful v2 write/clear + * — never before — so a v2-op failure doesn't strand the user with no + * recoverable credentials. Caller decides whether to swallow or propagate + * the `updateConfig` failure: + * - `set()` swallows (v2 record will win in `active()` regardless). + * - `clear()` propagates (v2 is empty, so a stale legacy token would + * shadow the logout via the fallback). */ async function dischargeLegacyState(): Promise { - await updateConfig(LEGACY_CLEAR_PAYLOAD).catch(() => undefined) + await updateConfig(LEGACY_CLEAR_PAYLOAD) } /** @@ -255,16 +254,13 @@ export async function isLegacyAuthActive(): Promise { * var, and an explicit ref means the caller targets a specific stored * account. * - * `ensureMigrated()` runs on every stored-state op so the lazy migration - * fires on first command. `active()` always prefers the v2 store and only - * falls back to the legacy snapshot when the v2 store is empty *and* - * migration is inconclusive — that ordering means a stale v1 token can - * never shadow a successful v2 write, even if the best-effort legacy - * cleanup has not yet run. - * - * `set()` / `clear()` discharge legacy state **after** the v2 op succeeds - * (not before) so a failure in the v2 op doesn't strand the user with no - * recoverable credentials. + * `ensureMigrated()` runs **before** every mutating v2 op so the post-op + * legacy discharge can't race a still-pending migration into re-grabbing + * the legacy `api_token` we just consumed. `set()` / `clear()` then + * discharge legacy state **after** the v2 op succeeds. `set()` swallows + * the cleanup failure (v2 wins in `active()` regardless); `clear()` + * propagates it so a failed logout fails loudly instead of leaving the + * user authenticated via the legacy fallback. */ export function createOutlineTokenStore(): OutlineTokenStore { const inner = createKeyringTokenStore({ @@ -273,11 +269,9 @@ export function createOutlineTokenStore(): OutlineTokenStore { recordsLocation: getConfigPath(), matchAccount: matchOutlineAccount, }) - async function maybeDischargeLegacy(): Promise { - const result = await ensureMigrated() - if (result === null || !migrationIsConclusive(result)) { - await dischargeLegacyState() - } + async function migrationIsInconclusive(): Promise { + const result = await ensureMigrated() // memoised + return result === null || !migrationIsConclusive(result) } return { async active(ref?: AccountRef) { @@ -298,10 +292,6 @@ export function createOutlineTokenStore(): OutlineTokenStore { const fromStore = await inner.active(ref) if (fromStore) return fromStore - // v2 store empty — try the legacy snapshot. We don't gate this - // on migration status: even on conclusive results the snapshot - // is just `null` (cleanupLegacyConfig already ran), so the - // extra read is cheap and the branch is the same. const legacy = await readLegacyTokenSnapshot() if (legacy && (ref === undefined || matchOutlineAccount(legacy.account, ref))) { return legacy @@ -309,12 +299,22 @@ export function createOutlineTokenStore(): OutlineTokenStore { return null }, async set(account: OutlineAccount, token: string) { + await ensureMigrated() await inner.set(account, token) - await maybeDischargeLegacy() + if (await migrationIsInconclusive()) { + // Best-effort: a lingering `api_token` is dormant because + // `active()` reads v2 first. + await dischargeLegacyState().catch(() => undefined) + } }, async clear(ref?: AccountRef) { + await ensureMigrated() await inner.clear(ref) - await maybeDischargeLegacy() + if (await migrationIsInconclusive()) { + // Must succeed: v2 is now empty, so a surviving legacy + // token would shadow the logout via the fallback. + await dischargeLegacyState() + } }, async list() { await ensureMigrated() @@ -330,18 +330,16 @@ export function createOutlineTokenStore(): OutlineTokenStore { } /** - * Where the currently-active token lives. Mirrors the resolution order in - * `active()` so the answer can never contradict the token the runtime is - * actually using: - * - * 1. env var → `'env'` - * 2. v2 user record present → `fallbackToken` ? `'config-file'` : `'secure-store'` - * 3. v1 plaintext `api_token` slot → `'config-file'` (legacy snapshot) - * 4. nothing on disk → `'secure-store'` (neutral default for fresh CLI) + * Where the currently-active token lives. Mirrors `active()`'s resolution + * order — env → v2 record → legacy plaintext — so the answer can never + * contradict the token the runtime is actually using. * - * Critically the v2 record check runs **before** the v1 slot check — if a - * stale `api_token` survives a best-effort `cleanupLegacyConfig` failure, - * `active()` ignores it (v2 record wins), and so do we. + * The precedence cascade is intentionally duplicated with `active()`: + * the only true dedupe options either (a) add an extra config read on + * every `apiRequest` call (regressing the request hot path) or (b) + * augment cli-core's `TokenStore` contract. The drift the duplication + * invites is guarded by the `getActiveTokenSource` regression test that + * asserts v2-record presence wins over a lingering v1 `api_token`. */ export async function getActiveTokenSource(): Promise<'env' | 'secure-store' | 'config-file'> { if (process.env[TOKEN_ENV_VAR]?.trim()) return 'env' diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 81ec94e..6a40df0 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -36,13 +36,17 @@ function tokenStore(): OutlineTokenStore { } /** - * Read the active token. The keyring-backed store wraps env-var precedence - * internally and falls back to the legacy plaintext snapshot when migration - * hasn't completed. + * Read the active token. Hot path: when `OUTLINE_API_TOKEN` is set we + * return it directly without consulting the token store, since + * `apiRequest` already resolves the base URL separately — going through + * `store.active()` here would trigger a redundant `getBaseUrl()` lookup + * per request just to synthesise an account we don't need. */ export async function getApiToken(): Promise { + const envToken = process.env[TOKEN_ENV_VAR]?.trim() + if (envToken) return envToken const snapshot = await tokenStore().active() - if (!snapshot || !snapshot.token) throw new NoTokenError() + if (!snapshot?.token) throw new NoTokenError() return snapshot.token } diff --git a/src/lib/migrate-auth.ts b/src/lib/migrate-auth.ts index f59e14e..485c46d 100644 --- a/src/lib/migrate-auth.ts +++ b/src/lib/migrate-auth.ts @@ -80,7 +80,14 @@ export async function runMigrateLegacyAuth( userRecords: createOutlineUserRecordStore(), hasMigrated: async () => { const config = await getConfig() - return (config.config_version ?? 0) >= V2_SCHEMA_VERSION + // Two signals of "already on v2": the explicit marker, or + // the structural presence of `users[]` (a fresh install that + // logged in directly via cli-core writes records without ever + // setting `config_version`, so the marker alone would + // miscount that case as "needs migration"). + if ((config.config_version ?? 0) >= V2_SCHEMA_VERSION) return true + if ((config.users ?? []).length > 0) return true + return false }, markMigrated: async () => { await updateConfig({ config_version: V2_SCHEMA_VERSION })