diff --git a/CODEBASE.md b/CODEBASE.md index 19e7324..a24edc3 100644 --- a/CODEBASE.md +++ b/CODEBASE.md @@ -118,21 +118,24 @@ New subcommand? Copy a sibling in the target group, wire it in that group's `{ results, nextCursor }` lives in `pagination.ts`. - **`api/` siblings** — `filters.ts`, `workspaces.ts`, `notifications.ts`, `reminders.ts`, `stats.ts`, `user-settings.ts`, `uploads.ts` -- **`auth.ts`** — `getApiToken()`, `probeApiToken()`, `saveApiToken()`, - `clearApiToken()`, `NoTokenError`, `AuthProbeResult` +- **`auth.ts`** — read-side resolver: `resolveActiveUser`, `getApiToken`, + `probeApiToken`, `getAuthMetadata`, `listStoredUsers`, `NoTokenError`. All + write/clear paths go through `auth-store.ts`. - **`auth-flags.ts`** — `buildReloginCommand()` (rebuilds `td auth login` with `--read-only` / `--additional-scopes=...` preserved) - **`config.ts`** — `~/.config/todoist-cli/config.json` read/write, - `AuthMode`, `UpdateChannel`, `AUTH_FLAG_ORDER` -- **`secure-store.ts`** — `@napi-rs/keyring` wrapper (OS credential manager) -- **`auth-provider.ts`** — `createTodoistAuthProvider()`: `@doist/cli-core` - PKCE `AuthProvider` adapter with a Todoist-specific `validateToken` - (calls `getUser`, builds `auth_mode` / `auth_scope` / `auth_flags`) -- **`auth-store.ts`** — `createTodoistTokenStore()`: cli-core - `TokenStore` adapter over `auth.ts`'s multi-user primitives. - Also exports `toTodoistAccount` / `accountToUpsertInput` mappers (shared - account shape) and `getLastStorageResult()` for surfacing keyring-fallback - warnings after `set()`. + `stripLegacyAuthFields`, `AuthMode`, `UpdateChannel`, `AUTH_FLAG_ORDER`. +- **`auth-provider.ts`** — `createTodoistAuthProvider()`: cli-core PKCE + provider with Todoist `validateToken` (builds `auth_mode` / `auth_scope` / + `auth_flags` from `getUser`). +- **`auth-store.ts`** — `createTodoistTokenStore()` (cli-core + `createKeyringTokenStore` wired to the `UserRecordStore` adapter), + persisted identifier constants (`SERVICE_NAME`, `LEGACY_ACCOUNT`, + `accountForUser`), and `toTodoistAccount()` mapper. +- **`user-records.ts`** — `UserRecordStore` adapter over + the config file. REPLACE-not-merge `upsert`; `ensureV2` on every write. +- **`migrate-auth.ts`** — postinstall v1 → v2 migration; thin wrapper + around cli-core's `migrateLegacyAuth` with the Todoist callbacks. - **`auth-html.ts`** — branded HTML pages for the cli-core OAuth callback (`renderAuthSuccessPage` / `renderAuthErrorPage`) - **`oauth-scopes.ts`** — opt-in OAuth scope registry, `parseScopesOption`, @@ -206,22 +209,25 @@ All live in `src/lib/refs.ts`: ## Auth & token storage -Token lookup order (see `src/lib/auth.ts` — `getApiToken()` / `probeApiToken()`): - -1. `TODOIST_API_TOKEN` env var -2. `~/.config/todoist-cli/config.json` (`{ "api_token": "..." }`) — migrated - into secure-store on first read when present -3. OS credential manager via `src/lib/secure-store.ts` - -`td auth login` runs through `@doist/cli-core`'s OAuth runtime -(`attachLoginCommand` → `runOAuthFlow`). The Todoist-local pieces live in -`src/lib/auth-provider.ts` (PKCE provider + `validateToken`) and -`src/lib/auth-store.ts` (multi-user `TokenStore` adapter); the command is -attached in `src/commands/auth/login.ts`. cli-core wires the standard flags -(`--read-only`, `--callback-port`, `--json`, `--ndjson`) and binds the local -callback server on port `8765` with a small fallback range. Scopes are -opt-in: `--read-only` for a read-only token, -`--additional-scopes=app-management,backups` to broaden. +`@doist/cli-core/auth` owns the keyring, multi-user `TokenStore`, OAuth flow, +and the `login` / `logout` / `status` / `token view` registrars. todoist-cli +supplies a `UserRecordStore` adapter (`user-records.ts`) over +its config file plus a Todoist-specific `validateToken` (`auth-provider.ts`). + +Read path (`auth.ts` — `resolveActiveUser` / `getApiToken` / `probeApiToken`): +env `TODOIST_API_TOKEN` first, then a config-derived target user, then either +`StoredUser.api_token` (plaintext keyring-offline fallback) or cli-core's +`createSecureStore` for the keyring slot. + +Write/clear/list all route through `createTodoistTokenStore()`; commands never +write the config directly. `auth logout` and `auth token view` use +`withUserRefAware` (`commands/auth/store-wrap.ts`) to substitute the +global `--user ` that `index.ts` strips from argv before commander runs. + +v1 → v2 migration (`migrate-auth.ts` → cli-core's `migrateLegacyAuth`) runs +from `postinstall.ts`. Gate is `config.config_version === CONFIG_VERSION`, +which survives logout so a reinstall over a logged-out v2 install can't +re-migrate a stale legacy slot. ## Testing diff --git a/package-lock.json b/package-lock.json index 50cb09d..456a4da 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", "@doist/todoist-sdk": "10.1.5", "@napi-rs/keyring": "1.3.0", "@pnpm/tabtab": "0.5.4", @@ -136,9 +136,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", @@ -147,6 +147,9 @@ "engines": { "node": ">=20.18.1" }, + "optionalDependencies": { + "@napi-rs/keyring": "1.3.0" + }, "peerDependencies": { "commander": ">=14", "marked": ">=18", diff --git a/package.json b/package.json index 257cf5b..4e7e9e5 100644 --- a/package.json +++ b/package.json @@ -51,7 +51,7 @@ "CHANGELOG.md" ], "dependencies": { - "@doist/cli-core": "0.12.0", + "@doist/cli-core": "0.16.1", "@doist/todoist-sdk": "10.1.5", "@napi-rs/keyring": "1.3.0", "@pnpm/tabtab": "0.5.4", diff --git a/src/commands/auth/auth.test.ts b/src/commands/auth/auth.test.ts index 2028202..4954d58 100644 --- a/src/commands/auth/auth.test.ts +++ b/src/commands/auth/auth.test.ts @@ -1,13 +1,33 @@ import { Command } from 'commander' import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' -// Mock the auth module +// Mock the auth-store factory so token / logout commands can drive a stub. +const setMock = vi.fn() +const clearMock = vi.fn() +const activeMock = vi.fn<(ref?: string) => Promise>() +const listMock = vi.fn<() => Promise>().mockResolvedValue([]) +const lastStorageMock = vi.fn<() => unknown>() +const lastClearMock = vi.fn<() => unknown>() +vi.mock('../../lib/auth-store.js', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + createTodoistTokenStore: () => ({ + active: activeMock, + list: listMock, + setDefault: vi.fn(), + set: setMock, + clear: clearMock, + getLastStorageResult: lastStorageMock, + getLastClearResult: lastClearMock, + }), + } +}) + vi.mock('../../lib/auth.js', async (importOriginal) => { const actual = await importOriginal() return { ...actual, - upsertUser: vi.fn(), - clearApiToken: vi.fn(), getAuthMetadata: vi.fn(), listStoredUsers: vi.fn(), readConfig: vi.fn(), @@ -52,30 +72,17 @@ vi.mock('node:readline', () => ({ import { createInterface, type Interface } from 'node:readline' import { createApiForToken, getApi } from '../../lib/api/core.js' import type { TodoistAccount, TodoistTokenStore } from '../../lib/auth-store.js' -import { - NoTokenError, - TOKEN_ENV_VAR, - clearApiToken, - getAuthMetadata, - listStoredUsers, - readConfig, - resolveActiveUser, - upsertUser, -} from '../../lib/auth.js' +import { NoTokenError, getAuthMetadata, listStoredUsers, readConfig } from '../../lib/auth.js' import { resetGlobalArgs } from '../../lib/global-args.js' -import { UserNotFoundError } from '../../lib/users.js' import { createMockApi } from '../../test-support/mock-api.js' import { registerAuthCommand } from './index.js' import { attachTodoistStatusCommand } from './status.js' const mockCreateInterface = vi.mocked(createInterface) -const mockUpsertUser = vi.mocked(upsertUser) -const mockClearApiToken = vi.mocked(clearApiToken) const mockGetAuthMetadata = vi.mocked(getAuthMetadata) const mockListStoredUsers = vi.mocked(listStoredUsers) const mockReadConfig = vi.mocked(readConfig) -const mockResolveActiveUser = vi.mocked(resolveActiveUser) const mockGetApi = vi.mocked(getApi) const mockCreateApiForToken = vi.mocked(createApiForToken) @@ -117,51 +124,43 @@ describe('auth command', () => { }) describe('token subcommand', () => { + beforeEach(() => { + setMock.mockReset().mockResolvedValue(undefined) + lastStorageMock.mockReset().mockReturnValue({ storage: 'secure-store' }) + }) + it('successfully saves a token', async () => { const program = createProgram() const token = 'some_token_123456789' stubProbeApiForUser() - mockUpsertUser.mockResolvedValue({ storage: 'secure-store', replaced: false }) await program.parseAsync(['node', 'td', 'auth', 'token', token]) expect(mockCreateApiForToken).toHaveBeenCalledWith(token) - expect(mockUpsertUser).toHaveBeenCalledWith({ - id: TEST_USER.id, - email: TEST_USER.email, + expect(setMock).toHaveBeenCalledWith( + expect.objectContaining({ + id: TEST_USER.id, + email: TEST_USER.email, + label: TEST_USER.email, + auth_mode: 'unknown', + }), token, - authMode: 'unknown', - }) + ) expect(consoleSpy).toHaveBeenCalledWith('✓', `Saved token for ${TEST_USER.email}`) }) - it('handles upsertUser errors', async () => { - const program = createProgram() - const token = 'some_token_123456789' - - stubProbeApiForUser() - mockUpsertUser.mockRejectedValue(new Error('Permission denied')) - - await expect( - program.parseAsync(['node', 'td', 'auth', 'token', token]), - ).rejects.toThrow('Permission denied') - }) - it('trims whitespace from token', async () => { const program = createProgram() const tokenWithWhitespace = ' some_token_123456789 ' const expectedToken = 'some_token_123456789' stubProbeApiForUser() - mockUpsertUser.mockResolvedValue({ storage: 'secure-store', replaced: false }) await program.parseAsync(['node', 'td', 'auth', 'token', tokenWithWhitespace]) expect(mockCreateApiForToken).toHaveBeenCalledWith(expectedToken) - expect(mockUpsertUser).toHaveBeenCalledWith( - expect.objectContaining({ token: expectedToken }), - ) + expect(setMock).toHaveBeenCalledWith(expect.anything(), expectedToken) }) it('prompts interactively when no token argument given', async () => { @@ -175,15 +174,12 @@ describe('auth command', () => { } mockCreateInterface.mockReturnValue(mockRl as unknown as Interface) stubProbeApiForUser() - mockUpsertUser.mockResolvedValue({ storage: 'secure-store', replaced: false }) const writeSpy = vi.spyOn(process.stdout, 'write').mockImplementation(() => true) await program.parseAsync(['node', 'td', 'auth', 'token']) expect(mockRl.question).toHaveBeenCalled() - expect(mockUpsertUser).toHaveBeenCalledWith( - expect.objectContaining({ token: 'interactive_token_456' }), - ) + expect(setMock).toHaveBeenCalledWith(expect.anything(), 'interactive_token_456') writeSpy.mockRestore() }) @@ -201,30 +197,16 @@ describe('auth command', () => { await program.parseAsync(['node', 'td', 'auth', 'token']) - expect(mockUpsertUser).not.toHaveBeenCalled() + expect(setMock).not.toHaveBeenCalled() expect(errorSpy).toHaveBeenCalledWith('Error:', 'No token provided') writeSpy.mockRestore() }) - it('shows "Updated stored token for" when account already existed', async () => { - const program = createProgram() - stubProbeApiForUser() - mockUpsertUser.mockResolvedValue({ storage: 'secure-store', replaced: true }) - - await program.parseAsync(['node', 'td', 'auth', 'token', 'some_token_123456789']) - - expect(consoleSpy).toHaveBeenCalledWith( - '✓', - `Updated stored token for ${TEST_USER.email}`, - ) - }) - it('surfaces config-file fallback warning', async () => { const program = createProgram() stubProbeApiForUser() - mockUpsertUser.mockResolvedValue({ + lastStorageMock.mockReturnValue({ storage: 'config-file', - replaced: false, warning: 'system credential manager unavailable; token saved as plaintext in /tmp/test-config.json', }) @@ -444,124 +426,132 @@ describe('auth command', () => { 'system credential manager unavailable; token cleared from plaintext config.json', } - it('clears the API token', async () => { - const program = createProgram() - mockClearApiToken.mockResolvedValue({ storage: 'secure-store' }) - - await program.parseAsync(['node', 'td', 'auth', 'logout']) - - expect(mockClearApiToken).toHaveBeenCalled() - expect(consoleSpy).toHaveBeenCalledWith('✓ Logged out') + beforeEach(() => { + clearMock.mockReset().mockResolvedValue(undefined) + lastClearMock.mockReset().mockReturnValue({ storage: 'secure-store' }) + activeMock.mockReset() + listMock.mockReset().mockResolvedValue([]) }) - it('surfaces keyring-fallback warning to stderr in plain mode', async () => { + it('surfaces keyring-fallback warning to stderr', async () => { const program = createProgram() - mockClearApiToken.mockResolvedValue(WARNING_RESULT) + lastClearMock.mockReturnValue(WARNING_RESULT) await program.parseAsync(['node', 'td', 'auth', 'logout']) - expect(consoleSpy).toHaveBeenCalledWith('✓ Logged out') expect(errorSpy).toHaveBeenCalledWith('Warning:', WARNING_RESULT.warning) }) - it('routes warning to stderr and emits JSON envelope on stdout in --json mode', async () => { + it('threads --user from global args through to store.clear', async () => { + // `index.ts` strips `--user` from process.argv before commander + // runs; cli-core's `attachLogoutCommand` therefore can't see it. + // The Todoist wrap reads `getRequestedUserRef()` from global args + // and substitutes it when commander calls `store.clear(undefined)`. + // Existence is checked via `store.list()` (no token side effect), + // not `store.active()`. + listMock.mockResolvedValue([ + { + account: { id: '111', email: 'a@example.com', label: 'a@example.com' }, + isDefault: true, + }, + ]) + const program = createProgram() - mockClearApiToken.mockResolvedValue(WARNING_RESULT) + const originalArgv = process.argv + process.argv = ['node', 'td', '--user', 'a@example.com', 'auth', 'logout'] + resetGlobalArgs() + try { + await program.parseAsync(['node', 'td', 'auth', 'logout']) + expect(clearMock).toHaveBeenCalledWith('a@example.com') + } finally { + process.argv = originalArgv + resetGlobalArgs() + } + }) - await program.parseAsync(['node', 'td', 'auth', 'logout', '--json']) + it('throws UserNotFoundError when --user does not match any stored account', async () => { + // cli-core's `clear(ref)` is contractually a no-op on miss; without + // the wrap's pre-check, `logout --user mistake` would print + // "✓ Logged out" and exit 0. The wrap surfaces the typed miss. + listMock.mockResolvedValue([]) - const stdoutLines = consoleSpy.mock.calls.map((c: unknown[]) => String(c[0])) - expect(stdoutLines).toEqual([JSON.stringify({ ok: true }, null, 2)]) - // Plain "Stored token removed" confirmation must be suppressed under --json. - expect(stdoutLines.join('\n')).not.toContain('Stored token removed') - expect(errorSpy).toHaveBeenCalledWith('Warning:', WARNING_RESULT.warning) + const program = createProgram() + const originalArgv = process.argv + process.argv = ['node', 'td', '--user', 'missing@example.com', 'auth', 'logout'] + resetGlobalArgs() + try { + await expect( + program.parseAsync(['node', 'td', 'auth', 'logout']), + ).rejects.toHaveProperty('code', 'USER_NOT_FOUND') + expect(clearMock).not.toHaveBeenCalled() + } finally { + process.argv = originalArgv + resetGlobalArgs() + } }) - it('routes warning to stderr and keeps stdout silent in --ndjson mode', async () => { + it('emits the JSON envelope on stdout and routes the keyring warning to stderr under --json', async () => { + // `attachTodoistLogoutCommand`'s `onCleared` branches on + // `view.json || view.ndjson` to suppress the human "Stored token + // removed" confirmation; cli-core then prints `{ok: true}`. A + // regression that leaked human prose into stdout — or dropped the + // envelope — would corrupt machine-output consumers. const program = createProgram() - mockClearApiToken.mockResolvedValue(WARNING_RESULT) + lastClearMock.mockReturnValue(WARNING_RESULT) - await program.parseAsync(['node', 'td', 'auth', 'logout', '--ndjson']) + await program.parseAsync(['node', 'td', 'auth', 'logout', '--json']) - expect(consoleSpy).not.toHaveBeenCalled() + const stdout = consoleSpy.mock.calls.map((c: unknown[]) => String(c[0])).join('\n') + expect(stdout).toContain('"ok": true') + expect(stdout).not.toContain('Stored token removed') expect(errorSpy).toHaveBeenCalledWith('Warning:', WARNING_RESULT.warning) }) }) describe('token view subcommand', () => { - let originalEnvToken: string | undefined - - beforeEach(() => { - originalEnvToken = process.env[TOKEN_ENV_VAR] - delete process.env[TOKEN_ENV_VAR] - }) - - afterEach(() => { - if (originalEnvToken === undefined) { - delete process.env[TOKEN_ENV_VAR] - } else { - process.env[TOKEN_ENV_VAR] = originalEnvToken - } - }) - - it('prints the bare token to stdout', async () => { - const program = createProgram() - mockResolveActiveUser.mockResolvedValue({ - id: TEST_USER.id, - email: TEST_USER.email, - token: 'stored_token_abc123456', - authMode: 'read-write', - source: 'secure-store', - }) + // `attachTokenViewCommand`'s bare-token output, TOKEN_FROM_ENV + // refusal, and NOT_AUTHENTICATED on no-snapshot are all tested in + // cli-core. The wrap's `--user` injection is exercised here because + // it goes through `withUserRefAware.active`, which the logout tests + // don't cover (they only exercise `.clear`). - await program.parseAsync(['node', 'td', 'auth', 'token', 'view']) + it('routes --user from global args through the active() path', async () => { + const account = { id: '111', email: 'a@example.com', label: 'a@example.com' } + listMock.mockResolvedValue([{ account, isDefault: true }]) + activeMock.mockResolvedValue({ token: 'stored-token-1234567', account }) - expect(mockResolveActiveUser).toHaveBeenCalled() - expect(consoleSpy).toHaveBeenCalledTimes(1) - expect(consoleSpy).toHaveBeenCalledWith('stored_token_abc123456') - }) - - it('refuses when TODOIST_API_TOKEN is set', async () => { const program = createProgram() - process.env[TOKEN_ENV_VAR] = 'env_token_value' - - await expect( - program.parseAsync(['node', 'td', 'auth', 'token', 'view']), - ).rejects.toHaveProperty('code', 'TOKEN_FROM_ENV') - expect(mockResolveActiveUser).not.toHaveBeenCalled() - expect(consoleSpy).not.toHaveBeenCalled() + const stdoutWrite = vi.spyOn(process.stdout, 'write').mockImplementation(() => true) + const originalArgv = process.argv + process.argv = ['node', 'td', '--user', 'a@example.com', 'auth', 'token', 'view'] + resetGlobalArgs() + try { + await program.parseAsync(['node', 'td', 'auth', 'token', 'view']) + expect(activeMock).toHaveBeenCalledWith('a@example.com') + expect(stdoutWrite).toHaveBeenCalledWith('stored-token-1234567') + } finally { + process.argv = originalArgv + resetGlobalArgs() + stdoutWrite.mockRestore() + } }) - it('propagates NoTokenError when no users are stored', async () => { - const program = createProgram() - mockResolveActiveUser.mockRejectedValue(new NoTokenError()) + it('surfaces UserNotFoundError when --user does not match', async () => { + listMock.mockResolvedValue([]) - await expect( - program.parseAsync(['node', 'td', 'auth', 'token', 'view']), - ).rejects.toHaveProperty('code', 'NO_TOKEN') - expect(consoleSpy).not.toHaveBeenCalled() - }) - - it('propagates UserNotFoundError when --user ref does not match', async () => { const program = createProgram() - mockResolveActiveUser.mockRejectedValue(new UserNotFoundError('missing@example.com')) - - // `--user ` is parsed from process.argv by the global-args - // layer (not commander) and stripped before commander sees the - // argv. Stub process.argv to mirror production wiring so the - // test exercises the same code path as a real invocation. const originalArgv = process.argv - process.argv = ['node', 'td', 'auth', 'token', 'view', '--user', 'missing@example.com'] + process.argv = ['node', 'td', '--user', 'nobody@example.com', 'auth', 'token', 'view'] resetGlobalArgs() try { await expect( program.parseAsync(['node', 'td', 'auth', 'token', 'view']), ).rejects.toHaveProperty('code', 'USER_NOT_FOUND') + expect(activeMock).not.toHaveBeenCalled() } finally { process.argv = originalArgv resetGlobalArgs() } - expect(consoleSpy).not.toHaveBeenCalled() }) }) }) diff --git a/src/commands/auth/index.ts b/src/commands/auth/index.ts index d33402c..a8924b9 100644 --- a/src/commands/auth/index.ts +++ b/src/commands/auth/index.ts @@ -1,43 +1,45 @@ +import { attachTokenViewCommand } from '@doist/cli-core/auth' import type { Command } from 'commander' -import { createTodoistTokenStore } from '../../lib/auth-store.js' +import { createTodoistTokenStore, TOKEN_ENV_VAR } from '../../lib/auth-store.js' import { attachTodoistLoginCommand } from './login.js' import { attachTodoistLogoutCommand } from './logout.js' import { attachTodoistStatusCommand } from './status.js' -import { viewToken } from './token-view.js' +import { withUserRefAware } from './store-wrap.js' import { loginWithToken } from './token.js' export function registerAuthCommand(program: Command): void { const auth = program.command('auth').description('Manage authentication') - // Shared store instance: login stashes the post-`set` storage result for - // its success handler, logout reads the post-`clear` result for the same - // keyring-fallback warning surface. Status uses `active()` as the - // authenticated-snapshot gate. + // Two stores share storage but expose different reads: + // - `store` is the raw cli-core `TokenStore` — login uses it to `set()`, + // status uses `active()` (with env-token short-circuit) as the + // authenticated-snapshot gate. + // - `refAware` substitutes `getRequestedUserRef()` for the `--user + // ` that `index.ts` strips from argv before commander runs, and + // turns ref-misses into typed `UserNotFoundError`. Used by every + // cli-core registrar that needs the global `--user` flag (logout + + // token view). const store = createTodoistTokenStore() + const refAware = withUserRefAware(store) attachTodoistLoginCommand(auth, store) - attachTodoistLogoutCommand(auth, store) + attachTodoistLogoutCommand(auth, refAware) attachTodoistStatusCommand(auth, store) - // `token` is a hybrid: it accepts a positional `[token]` (save) and also - // exposes subcommands (`view`). Commander matches subcommand names before - // falling through to the parent action, so `td auth token view` always - // dispatches to the `view` subcommand — `view` is never treated as a - // literal token value. Real Todoist tokens are 40-char hex strings, so - // this disambiguation is safe in practice. - // - // `token view` stays hand-rolled (not migrated to `attachTokenViewCommand`) - // because it depends on the `--user ` selector + env precedence rules - // that the `TokenStore` adapter intentionally drops from `active()`. + // `token` is a hybrid: positional `[token]` saves, and the `view` + // subcommand prints. Commander matches the subcommand name before the + // parent action, so `td auth token view` always dispatches to the view + // path — Todoist tokens are 40-char hex so the disambiguation is safe. const tokenCmd = auth .command('token [token]') .description('Save API token for CLI authentication (or use a subcommand: `view`)') .action(loginWithToken) - tokenCmd - .command('view') - .description( + attachTokenViewCommand(tokenCmd, { + name: 'view', + store: refAware, + envVarName: TOKEN_ENV_VAR, + description: 'Print the stored API token for the active user (or --user ) to stdout for use in scripts', - ) - .action(viewToken) + }) } diff --git a/src/commands/auth/logout.ts b/src/commands/auth/logout.ts index e6c71a6..4627435 100644 --- a/src/commands/auth/logout.ts +++ b/src/commands/auth/logout.ts @@ -4,11 +4,11 @@ import type { TodoistAccount, TodoistTokenStore } from '../../lib/auth-store.js' import { logTokenStorageResult } from './helpers.js' /** - * Attach `td auth logout` via cli-core's generic `attachLogoutCommand`. The - * registrar emits the success line (`✓ Logged out` / `{ok:true}` / silent - * ndjson); `onCleared` only surfaces the keyring-fallback warning carried by - * `TokenStorageResult` — cli-core's `TokenStore.clear: void` contract can't - * expose it directly, so we stash it on the adapter (`getLastClearResult`). + * `td auth logout`. cli-core owns the success line + `--json` / `--ndjson` + * envelopes; the Todoist hook surfaces the keyring-fallback warning that + * cli-core's `TokenStore.clear: void` contract can't carry. The `--user + * ` injection lives on the wrapped store the caller passes in (see + * `withUserRefAware` in `store-wrap.ts`). */ export function attachTodoistLogoutCommand(auth: Command, store: TodoistTokenStore): Command { return attachLogoutCommand(auth, { diff --git a/src/commands/auth/store-wrap.ts b/src/commands/auth/store-wrap.ts new file mode 100644 index 0000000..daa04aa --- /dev/null +++ b/src/commands/auth/store-wrap.ts @@ -0,0 +1,45 @@ +import type { TodoistTokenStore } from '../../lib/auth-store.js' +import { getRequestedUserRef } from '../../lib/global-args.js' +import { matchUserRef, UserNotFoundError } from '../../lib/users.js' + +/** + * Substitute `getRequestedUserRef()` for missing `ref` arguments on + * `active` / `clear`. `index.ts` strips `--user` from argv before commander + * runs, so cli-core's registrars (`attachLogoutCommand`, + * `attachTokenViewCommand`) can't see the flag on their parsed args; this + * wrap puts the global selector back into play. + * + * Existence is checked via `store.list()` rather than `store.active()` — the + * latter loads the token and can throw `SecureStoreUnavailableError` when + * the keyring is offline, which would crash `td auth logout --user ` + * instead of letting it clear the broken credential. + * + * The wrap is built with `Object.assign(Object.create(store), …)` so any + * methods cli-core might later promote to a prototype still resolve via the + * prototype chain instead of being silently dropped by a spread. + */ +export function withUserRefAware(store: TodoistTokenStore): TodoistTokenStore { + async function requireExists(ref: string): Promise { + const records = await store.list() + if ( + !records.some(({ account }) => + matchUserRef({ id: account.id, email: account.email }, ref), + ) + ) { + throw new UserNotFoundError(ref) + } + } + + return Object.assign(Object.create(store) as TodoistTokenStore, { + active: async (ref?: string) => { + const targetRef = ref ?? getRequestedUserRef() + if (targetRef !== undefined) await requireExists(targetRef) + return store.active(targetRef) + }, + clear: async (ref?: string) => { + const targetRef = ref ?? getRequestedUserRef() + if (targetRef !== undefined) await requireExists(targetRef) + await store.clear(targetRef) + }, + }) +} diff --git a/src/commands/auth/token-view.ts b/src/commands/auth/token-view.ts deleted file mode 100644 index ecd2ecd..0000000 --- a/src/commands/auth/token-view.ts +++ /dev/null @@ -1,18 +0,0 @@ -import { resolveActiveUser, TOKEN_ENV_VAR } from '../../lib/auth.js' -import { CliError } from '../../lib/errors.js' - -export async function viewToken(): Promise { - if (process.env[TOKEN_ENV_VAR]) { - throw new CliError( - 'TOKEN_FROM_ENV', - `Refusing to print token: ${TOKEN_ENV_VAR} is set in the environment.`, - [ - `The token is already available in your environment as $${TOKEN_ENV_VAR}.`, - `Unset ${TOKEN_ENV_VAR} to print the stored token instead.`, - ], - ) - } - - const resolved = await resolveActiveUser() - console.log(resolved.token) -} diff --git a/src/commands/auth/token.ts b/src/commands/auth/token.ts index 35db759..5c88ced 100644 --- a/src/commands/auth/token.ts +++ b/src/commands/auth/token.ts @@ -1,6 +1,6 @@ import chalk from 'chalk' import { createApiForToken } from '../../lib/api/core.js' -import { upsertUser } from '../../lib/auth.js' +import { createTodoistTokenStore, toTodoistAccount } from '../../lib/auth-store.js' import { logTokenStorageResult, promptHiddenInput } from './helpers.js' export async function loginWithToken(token?: string): Promise { @@ -18,14 +18,15 @@ export async function loginWithToken(token?: string): Promise { const probeApi = createApiForToken(trimmed) const user = await probeApi.getUser() - const result = await upsertUser({ - id: user.id, - email: user.email, - token: trimmed, - authMode: 'unknown', - }) + const store = createTodoistTokenStore() + await store.set( + toTodoistAccount({ id: user.id, email: user.email, authMode: 'unknown' }), + trimmed, + ) - const verb = result.replaced ? 'Updated stored token for' : 'Saved token for' - console.log(chalk.green('✓'), `${verb} ${user.email}`) - logTokenStorageResult(result, 'Token stored securely in the system credential manager') + console.log(chalk.green('✓'), `Saved token for ${user.email}`) + const result = store.getLastStorageResult() + if (result) { + logTokenStorageResult(result, 'Token stored securely in the system credential manager') + } } diff --git a/src/commands/config/config.test.ts b/src/commands/config/config.test.ts index d4f2581..dc3ab40 100644 --- a/src/commands/config/config.test.ts +++ b/src/commands/config/config.test.ts @@ -18,10 +18,10 @@ vi.mock('../../lib/auth.js', async () => { vi.mock('chalk') +import { SecureStoreUnavailableError } from '@doist/cli-core/auth' import { listStoredUsers, NoTokenError, probeApiToken } from '../../lib/auth.js' import { type Config, readConfigStrict } from '../../lib/config.js' import { CliError } from '../../lib/errors.js' -import { SecureStoreUnavailableError } from '../../lib/secure-store.js' import { registerConfigCommand } from './index.js' const mockReadConfigStrict = vi.mocked(readConfigStrict) diff --git a/src/commands/config/view.ts b/src/commands/config/view.ts index fb23d43..f67e6e1 100644 --- a/src/commands/config/view.ts +++ b/src/commands/config/view.ts @@ -1,3 +1,4 @@ +import { SecureStoreUnavailableError } from '@doist/cli-core/auth' import chalk from 'chalk' import { type AuthMetadata, @@ -8,9 +9,10 @@ import { TOKEN_ENV_VAR, } from '../../lib/auth.js' import { type Config, getConfigPath, readConfigStrict } from '../../lib/config.js' -import { SECURE_STORE_DESCRIPTION, SecureStoreUnavailableError } from '../../lib/secure-store.js' import { getDefaultUserId, NoUserSelectedError } from '../../lib/users.js' +const SECURE_STORE_DESCRIPTION = 'system credential manager' + export interface ViewConfigOptions { json?: boolean showToken?: boolean diff --git a/src/commands/user/remove.ts b/src/commands/user/remove.ts index 87f6539..c3e1751 100644 --- a/src/commands/user/remove.ts +++ b/src/commands/user/remove.ts @@ -1,5 +1,6 @@ import chalk from 'chalk' -import { readConfig, removeUserById } from '../../lib/auth.js' +import { createTodoistTokenStore } from '../../lib/auth-store.js' +import { readConfig } from '../../lib/auth.js' import { CliError } from '../../lib/errors.js' import { isQuiet } from '../../lib/global-args.js' import { getDefaultUserId, requireUserByRef } from '../../lib/users.js' @@ -12,11 +13,16 @@ export async function removeUserCommand(ref: string | undefined): Promise ) } + // Resolve the ref against the on-disk config first so we can report + // "Removed " + the "cleared default" hint with the user's actual + // email — the store's clear path is keyed by id, not email, and would + // silently no-op on miss (printing the wrong reassurance). const config = await readConfig() const { user } = requireUserByRef(config, ref) const wasDefault = getDefaultUserId(config) === user.id - const result = await removeUserById(user.id) + const store = createTodoistTokenStore() + await store.clear(user.id) if (!isQuiet()) { console.log(chalk.green('✓'), `Removed ${user.email}`) @@ -27,9 +33,8 @@ export async function removeUserCommand(ref: string | undefined): Promise } } - // Always surface partial-cleanup warnings, even with --quiet — the user - // needs to know if the keyring or config wasn't fully cleared. - if (result.warning) { + const result = store.getLastClearResult() + if (result?.warning) { console.error(chalk.yellow('Warning:'), result.warning) } } diff --git a/src/commands/user/use.ts b/src/commands/user/use.ts index 99ff8b9..1089098 100644 --- a/src/commands/user/use.ts +++ b/src/commands/user/use.ts @@ -1,8 +1,7 @@ import chalk from 'chalk' -import { readConfig, setDefaultUserId } from '../../lib/auth.js' +import { createTodoistTokenStore } from '../../lib/auth-store.js' import { CliError } from '../../lib/errors.js' import { isQuiet } from '../../lib/global-args.js' -import { requireUserByRef } from '../../lib/users.js' export async function useUserCommand(ref: string | undefined): Promise { if (!ref) { @@ -12,11 +11,12 @@ export async function useUserCommand(ref: string | undefined): Promise { ) } - const config = await readConfig() - const { user } = requireUserByRef(config, ref) - await setDefaultUserId(user.id) + // `store.setDefault(ref)` throws `CliError('ACCOUNT_NOT_FOUND', …)` on miss, + // which the top-level error handler renders as the standard "not found" + // message — no pre-check needed. + await createTodoistTokenStore().setDefault(ref) if (!isQuiet()) { - console.log(chalk.green('✓'), `Default account set to ${user.email} (id:${user.id})`) + console.log(chalk.green('✓'), `Default account set to ${ref}`) } } diff --git a/src/commands/user/user.test.ts b/src/commands/user/user.test.ts index b6d4271..e1c57ef 100644 --- a/src/commands/user/user.test.ts +++ b/src/commands/user/user.test.ts @@ -8,27 +8,36 @@ vi.mock('../../lib/auth.js', async (importOriginal) => { ...actual, listStoredUsers: vi.fn(), readConfig: vi.fn(), - setDefaultUserId: vi.fn(), - removeUserById: vi.fn(), resolveActiveUser: vi.fn(), } }) +const setDefaultMock = vi.fn() +const clearMock = vi.fn() +const lastClearMock = vi.fn<() => unknown>() +vi.mock('../../lib/auth-store.js', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + createTodoistTokenStore: () => ({ + active: vi.fn(), + list: vi.fn().mockResolvedValue([]), + set: vi.fn(), + setDefault: setDefaultMock, + clear: clearMock, + getLastStorageResult: vi.fn(), + getLastClearResult: lastClearMock, + }), + } +}) + vi.mock('chalk') -import { - listStoredUsers, - readConfig, - removeUserById, - resolveActiveUser, - setDefaultUserId, -} from '../../lib/auth.js' +import { listStoredUsers, readConfig, resolveActiveUser } from '../../lib/auth.js' import { registerUserCommand } from './index.js' const mockListStoredUsers = vi.mocked(listStoredUsers) const mockReadConfig = vi.mocked(readConfig) -const mockSetDefaultUserId = vi.mocked(setDefaultUserId) -const mockRemoveUserById = vi.mocked(removeUserById) const mockResolveActiveUser = vi.mocked(resolveActiveUser) function createProgram() { @@ -135,6 +144,10 @@ describe('user command', () => { }) describe('use', () => { + beforeEach(() => { + setDefaultMock.mockReset().mockResolvedValue(undefined) + }) + it('sets the default user by id', async () => { mockReadConfig.mockResolvedValue({ config_version: 2, @@ -143,18 +156,22 @@ describe('user command', () => { await createProgram().parseAsync(['node', 'td', 'user', 'use', '111']) - expect(mockSetDefaultUserId).toHaveBeenCalledWith('111') + expect(setDefaultMock).toHaveBeenCalledWith('111') }) it('rejects an unknown ref', async () => { - mockReadConfig.mockResolvedValue({ - config_version: 2, - users: [{ id: '111', email: 'a@b.c' }], - }) + // `td user use` routes directly through `store.setDefault(ref)` + // now; cli-core's `KeyringTokenStore` throws + // `CliError('ACCOUNT_NOT_FOUND', …)` on miss — the test stub + // mirrors that contract so the command propagates correctly. + const { CliError } = await import('../../lib/errors.js') + setDefaultMock.mockRejectedValueOnce( + new CliError('ACCOUNT_NOT_FOUND', 'No stored account matches "nope".'), + ) await expect( createProgram().parseAsync(['node', 'td', 'user', 'use', 'nope']), - ).rejects.toHaveProperty('code', 'USER_NOT_FOUND') + ).rejects.toHaveProperty('code', 'ACCOUNT_NOT_FOUND') }) it('default subcommand is an alias of use', async () => { @@ -165,7 +182,7 @@ describe('user command', () => { await createProgram().parseAsync(['node', 'td', 'user', 'default', '111']) - expect(mockSetDefaultUserId).toHaveBeenCalledWith('111') + expect(setDefaultMock).toHaveBeenCalledWith('111') }) }) @@ -208,17 +225,21 @@ describe('user command', () => { }) describe('remove', () => { + beforeEach(() => { + clearMock.mockReset().mockResolvedValue(undefined) + lastClearMock.mockReset().mockReturnValue({ storage: 'secure-store' }) + }) + it('removes the user by id and clears default', async () => { mockReadConfig.mockResolvedValue({ config_version: 2, user: { defaultUser: '111' }, users: [{ id: '111', email: 'a@b.c' }], }) - mockRemoveUserById.mockResolvedValue({ storage: 'secure-store' }) await createProgram().parseAsync(['node', 'td', 'user', 'remove', '111']) - expect(mockRemoveUserById).toHaveBeenCalledWith('111') + expect(clearMock).toHaveBeenCalledWith('111') expect(consoleSpy.mock.calls.flat().join('\n')).toContain('Cleared default') }) @@ -231,7 +252,7 @@ describe('user command', () => { await expect( createProgram().parseAsync(['node', 'td', 'user', 'remove', 'nope']), ).rejects.toHaveProperty('code', 'USER_NOT_FOUND') - expect(mockRemoveUserById).not.toHaveBeenCalled() + expect(clearMock).not.toHaveBeenCalled() }) }) }) diff --git a/src/lib/auth-store.test.ts b/src/lib/auth-store.test.ts deleted file mode 100644 index 7ee2e0e..0000000 --- a/src/lib/auth-store.test.ts +++ /dev/null @@ -1,288 +0,0 @@ -import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' - -vi.mock('./auth.js', async (importOriginal) => { - const actual = await importOriginal() - return { - ...actual, - loadTokenForStoredUser: vi.fn(), - upsertUser: vi.fn(), - clearApiToken: vi.fn(), - setDefaultUserId: vi.fn(), - } -}) - -vi.mock('./config.js', async (importOriginal) => { - const actual = await importOriginal() - return { - ...actual, - readConfig: vi.fn(), - } -}) - -import { - accountToUpsertInput, - createTodoistTokenStore, - type TodoistAccount, - toTodoistAccount, -} from './auth-store.js' -import { - clearApiToken, - loadTokenForStoredUser, - NoTokenError, - setDefaultUserId, - TOKEN_ENV_VAR, - upsertUser, -} from './auth.js' -import { readConfig } from './config.js' -import { SecureStoreUnavailableError } from './secure-store.js' -import { UserNotFoundError } from './users.js' - -const mockReadConfig = vi.mocked(readConfig) -const mockLoadToken = vi.mocked(loadTokenForStoredUser) -const mockUpsertUser = vi.mocked(upsertUser) -const mockClearApiToken = vi.mocked(clearApiToken) -const mockSetDefaultUserId = vi.mocked(setDefaultUserId) - -const USER_A = { - id: '111', - email: 'a@example.com', - auth_mode: 'read-write' as const, - auth_scope: 'data:read_write,data:delete,project:delete', -} -const USER_B = { - id: '222', - email: 'b@example.com', - auth_mode: 'read-only' as const, - auth_scope: 'data:read', - auth_flags: ['read-only' as const], -} - -const ACCOUNT_A: TodoistAccount = { - id: USER_A.id, - email: USER_A.email, - label: USER_A.email, - auth_mode: USER_A.auth_mode, - auth_scope: USER_A.auth_scope, - auth_flags: undefined, -} - -const ACCOUNT_B: TodoistAccount = { - id: USER_B.id, - email: USER_B.email, - label: USER_B.email, - auth_mode: USER_B.auth_mode, - auth_scope: USER_B.auth_scope, - auth_flags: ['read-only'], -} - -describe('TodoistAccount mappers', () => { - it('round-trips through toTodoistAccount → accountToUpsertInput', () => { - const account = toTodoistAccount({ - id: USER_A.id, - email: USER_A.email, - authMode: USER_A.auth_mode, - authScope: USER_A.auth_scope, - }) - expect(account).toEqual(ACCOUNT_A) - expect(accountToUpsertInput(account, 'token_xyz123456')).toEqual({ - id: USER_A.id, - email: USER_A.email, - token: 'token_xyz123456', - authMode: USER_A.auth_mode, - authScope: USER_A.auth_scope, - authFlags: undefined, - }) - }) -}) - -describe('createTodoistTokenStore', () => { - let originalEnvToken: string | undefined - - beforeEach(() => { - vi.clearAllMocks() - originalEnvToken = process.env[TOKEN_ENV_VAR] - delete process.env[TOKEN_ENV_VAR] - }) - - afterEach(() => { - if (originalEnvToken === undefined) { - delete process.env[TOKEN_ENV_VAR] - } else { - process.env[TOKEN_ENV_VAR] = originalEnvToken - } - }) - - describe('active()', () => { - it('returns null when TODOIST_API_TOKEN is set (env tokens are not persisted)', async () => { - process.env[TOKEN_ENV_VAR] = 'env_token_value' - expect(await createTodoistTokenStore().active()).toBeNull() - expect(mockReadConfig).not.toHaveBeenCalled() - }) - - it('returns null when no users are stored', async () => { - mockReadConfig.mockResolvedValue({ users: [] }) - expect(await createTodoistTokenStore().active()).toBeNull() - }) - - it('returns the single stored user when no default is set', async () => { - mockReadConfig.mockResolvedValue({ users: [USER_A] }) - mockLoadToken.mockResolvedValue({ token: 'tok_a', source: 'secure-store' }) - - expect(await createTodoistTokenStore().active()).toEqual({ - token: 'tok_a', - account: ACCOUNT_A, - }) - }) - - it('prefers the default user when multiple are stored', async () => { - mockReadConfig.mockResolvedValue({ - users: [USER_A, USER_B], - user: { defaultUser: USER_B.id }, - }) - mockLoadToken.mockResolvedValue({ token: 'tok_b', source: 'secure-store' }) - - const result = await createTodoistTokenStore().active() - expect(mockLoadToken).toHaveBeenCalledWith(USER_B) - expect(result?.account.id).toBe(USER_B.id) - expect(result?.account.auth_flags).toEqual(['read-only']) - }) - - it('returns null on multi-user-no-default (no selection error leaks out)', async () => { - mockReadConfig.mockResolvedValue({ users: [USER_A, USER_B] }) - expect(await createTodoistTokenStore().active()).toBeNull() - expect(mockLoadToken).not.toHaveBeenCalled() - }) - - it.each([ - ['NoTokenError', new NoTokenError()], - ['SecureStoreUnavailableError', new SecureStoreUnavailableError('offline')], - ])( - 'propagates %s when token load fails for a matched user (does not collapse to null)', - async (_label, error) => { - // Pre cli-core 0.12.0 these were swallowed to null, but `null` - // is now reserved for "ref miss" — see TokenStore contract. - mockReadConfig.mockResolvedValue({ users: [USER_A] }) - mockLoadToken.mockRejectedValue(error) - await expect(createTodoistTokenStore().active()).rejects.toBe(error) - }, - ) - - describe('with --user ref', () => { - it('returns the matched account when ref hits a stored id', async () => { - mockReadConfig.mockResolvedValue({ users: [USER_A, USER_B] }) - mockLoadToken.mockResolvedValue({ token: 'tok_b', source: 'secure-store' }) - - const result = await createTodoistTokenStore().active(USER_B.id) - expect(mockLoadToken).toHaveBeenCalledWith(USER_B) - expect(result?.account.id).toBe(USER_B.id) - }) - - it('returns the matched account when ref hits a stored email', async () => { - mockReadConfig.mockResolvedValue({ users: [USER_A, USER_B] }) - mockLoadToken.mockResolvedValue({ token: 'tok_a', source: 'secure-store' }) - - const result = await createTodoistTokenStore().active(USER_A.email) - expect(mockLoadToken).toHaveBeenCalledWith(USER_A) - expect(result?.account.email).toBe(USER_A.email) - }) - - it('returns null when ref does not match any stored account', async () => { - mockReadConfig.mockResolvedValue({ users: [USER_A] }) - expect(await createTodoistTokenStore().active('nobody@example.com')).toBeNull() - expect(mockLoadToken).not.toHaveBeenCalled() - }) - - it('propagates keyring errors when ref matches but token load fails', async () => { - mockReadConfig.mockResolvedValue({ users: [USER_A, USER_B] }) - const error = new SecureStoreUnavailableError('offline') - mockLoadToken.mockRejectedValue(error) - await expect(createTodoistTokenStore().active(USER_B.id)).rejects.toBe(error) - }) - }) - }) - - describe('set()', () => { - it('persists via upsertUser and exposes the storage result + warning', async () => { - mockUpsertUser.mockResolvedValue({ - storage: 'config-file', - replaced: false, - warning: - 'system credential manager unavailable; token saved as plaintext in /tmp/c.json', - }) - - const store = createTodoistTokenStore() - await store.set(ACCOUNT_A, 'token_xyz123456') - - expect(mockUpsertUser).toHaveBeenCalledWith( - accountToUpsertInput(ACCOUNT_A, 'token_xyz123456'), - ) - expect(store.getLastStorageResult()).toEqual({ - storage: 'config-file', - warning: - 'system credential manager unavailable; token saved as plaintext in /tmp/c.json', - }) - }) - }) - - describe('clear()', () => { - it('delegates to clearApiToken with no ref', async () => { - mockClearApiToken.mockResolvedValue({ storage: 'secure-store' }) - await createTodoistTokenStore().clear() - expect(mockClearApiToken).toHaveBeenCalledWith({ ref: undefined }) - }) - - it('forwards a ref to clearApiToken', async () => { - mockClearApiToken.mockResolvedValue({ storage: 'secure-store' }) - await createTodoistTokenStore().clear(USER_B.email) - expect(mockClearApiToken).toHaveBeenCalledWith({ ref: USER_B.email }) - }) - }) - - describe('list()', () => { - it('returns an empty array when nothing is stored', async () => { - mockReadConfig.mockResolvedValue({ users: [] }) - expect(await createTodoistTokenStore().list()).toEqual([]) - }) - - it('marks the only stored user as the default (implicit single-user fallback)', async () => { - mockReadConfig.mockResolvedValue({ users: [USER_A] }) - expect(await createTodoistTokenStore().list()).toEqual([ - { account: ACCOUNT_A, isDefault: true }, - ]) - }) - - it('marks the explicit defaultUser when multiple are stored', async () => { - mockReadConfig.mockResolvedValue({ - users: [USER_A, USER_B], - user: { defaultUser: USER_B.id }, - }) - expect(await createTodoistTokenStore().list()).toEqual([ - { account: ACCOUNT_A, isDefault: false }, - { account: ACCOUNT_B, isDefault: true }, - ]) - }) - - it('marks none as default when multiple users are stored without an explicit default', async () => { - mockReadConfig.mockResolvedValue({ users: [USER_A, USER_B] }) - expect(await createTodoistTokenStore().list()).toEqual([ - { account: ACCOUNT_A, isDefault: false }, - { account: ACCOUNT_B, isDefault: false }, - ]) - }) - }) - - describe('setDefault()', () => { - it('delegates to setDefaultUserId', async () => { - await createTodoistTokenStore().setDefault(USER_B.email) - expect(mockSetDefaultUserId).toHaveBeenCalledWith(USER_B.email) - }) - - it('propagates UserNotFoundError when the ref does not match', async () => { - const error = new UserNotFoundError('nobody@example.com') - mockSetDefaultUserId.mockRejectedValueOnce(error) - await expect(createTodoistTokenStore().setDefault('nobody@example.com')).rejects.toBe( - error, - ) - }) - }) -}) diff --git a/src/lib/auth-store.ts b/src/lib/auth-store.ts index 66f5502..ca389e4 100644 --- a/src/lib/auth-store.ts +++ b/src/lib/auth-store.ts @@ -1,15 +1,25 @@ -import type { AccountRef, AuthAccount, TokenStore } from '@doist/cli-core/auth' import { - clearApiToken, - loadTokenForStoredUser, - setDefaultUserId, - type TokenStorageResult, - TOKEN_ENV_VAR, - upsertUser, - type UpsertUserInput, -} from './auth.js' -import { type AuthFlag, type AuthMode, readConfig, type StoredUser } from './config.js' -import { findUserByRef, getEffectiveDefaultUser, getStoredUsers } from './users.js' + type AccountRef, + type AuthAccount, + createKeyringTokenStore, + type KeyringTokenStore, +} from '@doist/cli-core/auth' +import { type AuthFlag, type AuthMode, getConfigPath } from './config.js' +import { createTodoistUserRecordStore } from './user-records.js' +import { matchUserRef } from './users.js' + +/** + * Persisted identifiers for the keyring/config ABI. Shared with the read-side + * resolver (`auth.ts`) and the postinstall migration (`migrate-auth.ts`) so + * a rename can't silently desynchronise the three paths that touch the OS + * credential manager. + */ +export const SERVICE_NAME = 'todoist-cli' +export const LEGACY_ACCOUNT = 'api-token' +export const TOKEN_ENV_VAR = 'TODOIST_API_TOKEN' +export function accountForUser(id: string): string { + return `user-${id}` +} /** * Account shape stored by todoist-cli. Extends cli-core's `AuthAccount` with @@ -33,9 +43,8 @@ export type TodoistAccountInput = { /** * Single source of truth for the `TodoistAccount` field layout. Used by the - * auth provider's `validateToken` (post-handshake) and the token-store - * `active()` adapter (post-read) so the persisted shape stays aligned with - * what `set()` later disassembles via `accountToUpsertInput`. + * auth provider's `validateToken` (post-handshake), the migration helper, + * and the `UserRecordStore` adapter's record → account mapping. */ export function toTodoistAccount(input: TodoistAccountInput): TodoistAccount { return { @@ -48,112 +57,42 @@ export function toTodoistAccount(input: TodoistAccountInput): TodoistAccount { } } -/** Inverse of `toTodoistAccount` — feeds the upsert path that owns persistence. */ -export function accountToUpsertInput(account: TodoistAccount, token: string): UpsertUserInput { - return { - id: account.id, - email: account.email, - token, - authMode: account.auth_mode, - authScope: account.auth_scope, - authFlags: account.auth_flags, - } -} - -function storedUserToAccount(user: StoredUser): TodoistAccount { - return toTodoistAccount({ - id: user.id, - email: user.email, - authMode: user.auth_mode ?? 'unknown', - authScope: user.auth_scope, - authFlags: user.auth_flags, - }) -} +export type TodoistTokenStore = KeyringTokenStore /** - * `TokenStore` adapter that bridges cli-core's auth runtime - * (which is single-user) to todoist's existing multi-user keyring + config - * store. `set()` is exercised by `runOAuthFlow` during login; `active()` and - * `clear()` are wired through for completeness so the public `TokenStore` - * contract is honoured — per-user reads are still driven from the - * todoist-local commands (logout, status, token, …). + * cli-core's keyring-backed `TokenStore`, wired to todoist-cli's + * `UserRecordStore` adapter. Two Todoist-specific overlays on top of the + * defaults: * - * The factory also returns `getLastStorageResult()` so the login command can - * surface keyring-fallback warnings (`upsertUser` reports them via the - * `TokenStorageResult` payload that `set()` would otherwise have to swallow, - * since cli-core's `TokenStore.set` signature is `void`). + * - `active()` short-circuits to `null` when `TODOIST_API_TOKEN` is set. + * The env var is the canonical override across the CLI; without this + * short-circuit, `td auth status` would render the stored account while + * `getAuthMetadata()` reports `source: 'env'` (wrong account, right + * diagnostic). + * - `accountForUser` / `matchAccount` are passed explicitly. `matchAccount` + * delegates to `matchUserRef` so the keyring-store path and the + * config-driven `findUserByRef` path share one matcher (case-insensitive + * email + trim). */ -export type TodoistTokenStore = TokenStore & { - getLastStorageResult(): TokenStorageResult | undefined - getLastClearResult(): TokenStorageResult | undefined -} - export function createTodoistTokenStore(): TodoistTokenStore { - let lastStorageResult: TokenStorageResult | undefined - let lastClearResult: TokenStorageResult | undefined - + const inner = createKeyringTokenStore({ + serviceName: SERVICE_NAME, + userRecords: createTodoistUserRecordStore(), + recordsLocation: getConfigPath(), + accountForUser, + matchAccount: (account: TodoistAccount, ref: AccountRef) => + matchUserRef({ id: account.id, email: account.email }, ref), + }) return { - /** - * Pure view of persisted state. Returns `null` when `TODOIST_API_TOKEN` - * is in play (env tokens don't represent a persisted account), when - * nothing is stored, or when `ref` does not match any stored account - * — cli-core's resolver layer translates the miss into a typed error. - * With `ref`, returns that specific stored account; without, returns - * the default-or-only account. - * - * Token-load failures (broken keyring, missing secret) propagate as - * typed errors once a user was matched — collapsing them to `null` - * would make a real account look like an unknown `ref` to cli-core. - */ - async active(ref?: AccountRef) { + active: async (ref) => { if (process.env[TOKEN_ENV_VAR]) return null - - const config = await readConfig() - const target = - ref !== undefined - ? (findUserByRef(config, ref)?.user ?? null) - : getEffectiveDefaultUser(config) - if (!target) return null - - const { token } = await loadTokenForStoredUser(target) - return { token, account: storedUserToAccount(target) } - }, - - async set(account, token) { - const { replaced: _replaced, ...result } = await upsertUser( - accountToUpsertInput(account, token), - ) - lastStorageResult = result - }, - - async clear(ref?: AccountRef) { - lastClearResult = await clearApiToken({ ref }) - }, - - async list() { - const config = await readConfig() - const users = getStoredUsers(config) - if (users.length === 0) return [] - const defaultId = getEffectiveDefaultUser(config)?.id - return users.map((user) => ({ - account: storedUserToAccount(user), - isDefault: user.id === defaultId, - })) - }, - - async setDefault(ref: AccountRef) { - // `setDefaultUserId` accepts any ref (id or email) and throws - // `UserNotFoundError` (a `CliError` with code `USER_NOT_FOUND`) - // when the ref does not match any stored account. - await setDefaultUserId(ref) - }, - - getLastStorageResult() { - return lastStorageResult - }, - - getLastClearResult() { - return lastClearResult + return inner.active(ref) }, + set: (account, token) => inner.set(account, token), + clear: (ref) => inner.clear(ref), + list: () => inner.list(), + setDefault: (ref) => inner.setDefault(ref), + getLastStorageResult: () => inner.getLastStorageResult(), + getLastClearResult: () => inner.getLastClearResult(), } } diff --git a/src/lib/auth.test.ts b/src/lib/auth.test.ts index a2e03b6..0928688 100644 --- a/src/lib/auth.test.ts +++ b/src/lib/auth.test.ts @@ -29,7 +29,6 @@ function entryFor(state: KeyringMockState, account: string): KeyringEntryState { describe('lib/auth', () => { let configContent: string | null - let configWriteError: Error | null let mkdirMock: ReturnType let readFileMock: ReturnType let unlinkMock: ReturnType @@ -43,7 +42,6 @@ describe('lib/auth', () => { vi.unstubAllEnvs() configContent = null - configWriteError = null keyring = { entries: new Map(), constructed: [], @@ -63,9 +61,6 @@ describe('lib/auth', () => { configContent = null }) writeFileMock = vi.fn().mockImplementation(async (_path: string, content: string) => { - if (configWriteError) { - throw configWriteError - } configContent = content }) @@ -96,38 +91,57 @@ describe('lib/auth', () => { } }) - vi.doMock('@napi-rs/keyring', () => ({ - AsyncEntry: class { - private account: string - constructor(service: string, account: string) { - this.account = account - keyring.constructed.push({ service, account }) - } - - async getPassword(): Promise { - if (keyring.getError) throw keyring.getError - const e = entryFor(keyring, this.account) - e.getCalls += 1 - return e.token - } - - async setPassword(password: string): Promise { - if (keyring.setError) throw keyring.setError - const e = entryFor(keyring, this.account) - e.token = password - e.setCalls.push(password) - } - - async deleteCredential(): Promise { - if (keyring.deleteError) throw keyring.deleteError - const e = entryFor(keyring, this.account) - const had = e.token !== null - e.token = null - e.deleteCalls += 1 - return had - } - }, - })) + // Mock cli-core's `createSecureStore` directly. Inlining cli-core via + // `server.deps.inline` is not sufficient on Linux + npm-linked cli-core: + // the dynamic `import('@napi-rs/keyring')` inside cli-core resolves + // against the symlink target and bypasses vitest's mock. Mocking + // `@doist/cli-core/auth` is the reliable boundary — it preserves the + // typed `SecureStoreUnavailableError` re-export so the actual class + // identity carries across the test/import boundary. + vi.doMock('@doist/cli-core/auth', async (importOriginal) => { + const actual = await importOriginal() + const { SecureStoreUnavailableError } = actual + return { + ...actual, + createSecureStore: ({ + serviceName, + account, + }: { + serviceName: string + account: string + }) => { + keyring.constructed.push({ service: serviceName, account }) + return { + async getSecret() { + if (keyring.getError) { + throw new SecureStoreUnavailableError(keyring.getError.message) + } + const e = entryFor(keyring, account) + e.getCalls += 1 + return e.token + }, + async setSecret(password: string) { + if (keyring.setError) { + throw new SecureStoreUnavailableError(keyring.setError.message) + } + const e = entryFor(keyring, account) + e.token = password + e.setCalls.push(password) + }, + async deleteSecret() { + if (keyring.deleteError) { + throw new SecureStoreUnavailableError(keyring.deleteError.message) + } + const e = entryFor(keyring, account) + const had = e.token !== null + e.token = null + e.deleteCalls += 1 + return had + }, + } + }, + } + }) errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}) }) @@ -156,153 +170,6 @@ describe('lib/auth', () => { expect(keyring.constructed).toEqual([]) }) - // --- upsertUser ----------------------------------------------------------- - - it('upsertUser stores token in per-user keyring slot and writes v2 config', async () => { - const { upsertUser } = await import('./auth.js') - - const result = await upsertUser({ - id: '12345', - email: 'scott@doist.com', - token: 'oauth-token-1234567', - authMode: 'read-write', - authScope: 'data:read_write', - }) - - expect(result).toEqual({ storage: 'secure-store', replaced: false }) - expect(entryFor(keyring, 'user-12345').token).toBe('oauth-token-1234567') - expect(readConfig()).toEqual({ - config_version: 2, - user: { defaultUser: '12345' }, - users: [ - { - id: '12345', - email: 'scott@doist.com', - auth_mode: 'read-write', - auth_scope: 'data:read_write', - }, - ], - }) - }) - - it('upsertUser does NOT overwrite an existing default when adding a second user', async () => { - setConfig({ - config_version: 2, - user: { defaultUser: '111' }, - users: [{ id: '111', email: 'first@example.com' }], - }) - - const { upsertUser } = await import('./auth.js') - - const result = await upsertUser({ - id: '222', - email: 'second@example.com', - token: 'second-token-1234567', - }) - - expect(result.replaced).toBe(false) - const config = readConfig() as Record - expect(config.user).toEqual({ defaultUser: '111' }) - expect((config.users as { id: string }[]).map((u) => u.id)).toEqual(['111', '222']) - }) - - it('upsertUser replaces existing record for same id', async () => { - setConfig({ - config_version: 2, - user: { defaultUser: '111' }, - users: [{ id: '111', email: 'old@example.com', auth_mode: 'read-only' }], - }) - - const { upsertUser } = await import('./auth.js') - - const result = await upsertUser({ - id: '111', - email: 'new@example.com', - token: 'new-token-1234567', - authMode: 'read-write', - }) - - expect(result.replaced).toBe(true) - expect((readConfig() as { users: unknown[] }).users).toEqual([ - { - id: '111', - email: 'new@example.com', - auth_mode: 'read-write', - }, - ]) - }) - - it('upsertUser sets the new account as default even when an orphaned defaultUser is in config', async () => { - // Pointer to a user that no longer exists (manual edit, mid-failed - // logout, etc.) should not block first-login from claiming default. - setConfig({ - config_version: 2, - user: { defaultUser: 'orphan-id' }, - users: [], - }) - - const { upsertUser } = await import('./auth.js') - await upsertUser({ - id: '111', - email: 'a@b.c', - token: 'first-token-1234567', - }) - - expect((readConfig() as { user: { defaultUser: string } }).user.defaultUser).toBe('111') - }) - - it('upsertUser rolls back the keyring write when the config write fails', async () => { - configWriteError = new Error('EACCES') - - const { upsertUser } = await import('./auth.js') - - await expect( - upsertUser({ id: '111', email: 'a@b.c', token: 'rollback-me-1234567' }), - ).rejects.toMatchObject({ code: 'CONFIG_WRITE_FAILED' }) - - // No leftover credential for an account that isn't actually stored. - expect(entryFor(keyring, 'user-111').token).toBeNull() - }) - - it('removeUserById fails hard when the config write fails — keyring is untouched', async () => { - setConfig({ - config_version: 2, - user: { defaultUser: '111' }, - users: [{ id: '111', email: 'a@b.c' }], - }) - entryFor(keyring, 'user-111').token = 'preserved' - configWriteError = new Error('EACCES') - - const { removeUserById } = await import('./auth.js') - - await expect(removeUserById('111')).rejects.toMatchObject({ - code: 'CONFIG_WRITE_FAILED', - }) - - // Account remains intact and resolvable on retry. - expect(entryFor(keyring, 'user-111').token).toBe('preserved') - }) - - it('upsertUser falls back to plaintext per-user config when keyring unavailable', async () => { - keyring.setError = new Error('Keychain unavailable') - - const { upsertUser } = await import('./auth.js') - - const result = await upsertUser({ - id: '12345', - email: 'a@b.c', - token: 'fallback-token-1234567', - }) - - expect(result).toEqual({ - storage: 'config-file', - replaced: false, - warning: `system credential manager unavailable; token saved as plaintext in ${TEST_CONFIG_PATH}`, - }) - const stored = (readConfig() as { users: { api_token?: string }[] }).users[0] - expect(stored.api_token).toBe('fallback-token-1234567') - }) - // --- resolveActiveUser ---------------------------------------------------- it('resolves single stored user implicitly', async () => { @@ -387,59 +254,7 @@ describe('lib/auth', () => { await expect(resolveActiveUser({ ref: 'nope' })).rejects.toBeInstanceOf(UserNotFoundError) }) - // --- legacy fallback ------------------------------------------------------ - - it('serves a legacy config token when no v2 users exist (graceful fallback)', async () => { - setConfig({ - api_token: 'legacy-token-1234567', - auth_mode: 'read-write', - }) - - const { resolveActiveUser } = await import('./auth.js') - - const resolved = await resolveActiveUser() - expect(resolved.id).toBe('legacy') - expect(resolved.token).toBe('legacy-token-1234567') - expect(resolved.authMode).toBe('read-write') - // does NOT auto-migrate at runtime — that's postinstall's job - expect(readConfig()).toEqual({ - api_token: 'legacy-token-1234567', - auth_mode: 'read-write', - }) - }) - - it('serves a legacy keyring token when no v2 users and no plaintext', async () => { - entryFor(keyring, 'api-token').token = 'legacy-secure-1234567' - - const { resolveActiveUser } = await import('./auth.js') - - const resolved = await resolveActiveUser() - expect(resolved.id).toBe('legacy') - expect(resolved.token).toBe('legacy-secure-1234567') - expect(resolved.source).toBe('secure-store') - }) - - it('treats v1 pendingSecureStoreClear as logged out', async () => { - setConfig({ pendingSecureStoreClear: true }) - entryFor(keyring, 'api-token').token = 'stale-1234567' - - const { resolveActiveUser, NoTokenError } = await import('./auth.js') - - await expect(resolveActiveUser()).rejects.toBeInstanceOf(NoTokenError) - }) - - it('does not reauth from legacy keyring when v2 users[] is explicitly empty', async () => { - // Logged-out v2 install. A leftover `api-token` keyring entry must - // NOT be picked up — that would silently sign the user back in. - setConfig({ config_version: 2, users: [] }) - entryFor(keyring, 'api-token').token = 'leftover-from-v1' - - const { resolveActiveUser, NoTokenError } = await import('./auth.js') - - await expect(resolveActiveUser()).rejects.toBeInstanceOf(NoTokenError) - // legacy slot wasn't even consulted - expect(entryFor(keyring, 'api-token').getCalls).toBe(0) - }) + // --- probe / metadata under keyring failure ------------------------------- it('probeApiToken surfaces SecureStoreUnavailableError instead of NoTokenError', async () => { // Stored v2 user, no plaintext fallback; keyring is offline. The @@ -452,7 +267,7 @@ describe('lib/auth', () => { keyring.getError = new Error('keychain locked') const { probeApiToken } = await import('./auth.js') - const { SecureStoreUnavailableError } = await import('./secure-store.js') + const { SecureStoreUnavailableError } = await import('@doist/cli-core/auth') await expect(probeApiToken()).rejects.toBeInstanceOf(SecureStoreUnavailableError) }) @@ -475,101 +290,10 @@ describe('lib/auth', () => { }) }) - // --- removeUserById / clearApiToken -------------------------------------- - - it('removeUserById deletes keyring slot, removes user record, and clears default if matched', async () => { - setConfig({ - config_version: 2, - user: { defaultUser: '111' }, - users: [ - { id: '111', email: 'a@b.c' }, - { id: '222', email: 'd@e.f' }, - ], - }) - entryFor(keyring, 'user-111').token = 't1' - entryFor(keyring, 'user-222').token = 't2' - - const { removeUserById } = await import('./auth.js') - - await expect(removeUserById('111')).resolves.toEqual({ storage: 'secure-store' }) - - expect(entryFor(keyring, 'user-111').token).toBeNull() - const config = readConfig() as Record - expect((config.users as { id: string }[]).map((u) => u.id)).toEqual(['222']) - expect(config.user).toBeUndefined() - }) - - it('clearApiToken errors when multiple users are stored without --user', async () => { - setConfig({ - config_version: 2, - users: [ - { id: '111', email: 'a@b.c' }, - { id: '222', email: 'd@e.f' }, - ], - }) - - const { clearApiToken } = await import('./auth.js') - const { NoUserSelectedError } = await import('./users.js') - - await expect(clearApiToken()).rejects.toBeInstanceOf(NoUserSelectedError) - }) - - it('clearApiToken targets the default user when one is set', async () => { - setConfig({ - config_version: 2, - user: { defaultUser: '222' }, - users: [ - { id: '111', email: 'a@b.c' }, - { id: '222', email: 'd@e.f' }, - ], - }) - entryFor(keyring, 'user-111').token = 't1' - entryFor(keyring, 'user-222').token = 't2' - - const { clearApiToken } = await import('./auth.js') - await clearApiToken() - - expect(entryFor(keyring, 'user-222').token).toBeNull() - expect((readConfig() as { users: { id: string }[] }).users).toEqual([ - { id: '111', email: 'a@b.c' }, - ]) - }) - - it('clearApiToken cleans up legacy state when no v2 users exist', async () => { - setConfig({ api_token: 'legacy-1234567' }) - - const { clearApiToken } = await import('./auth.js') - - await expect(clearApiToken()).resolves.toEqual({ storage: 'secure-store' }) - expect(readConfig()).toBeNull() - }) - - // --- listStoredUsers / setDefaultUserId ----------------------------------- - - it('setDefaultUserId only accepts a stored user', async () => { - setConfig({ - config_version: 2, - users: [{ id: '111', email: 'a@b.c' }], - }) - - const { setDefaultUserId } = await import('./auth.js') - const { UserNotFoundError } = await import('./users.js') - - await expect(setDefaultUserId('999')).rejects.toBeInstanceOf(UserNotFoundError) - await setDefaultUserId('111') - expect((readConfig() as { user?: { defaultUser?: string } }).user).toEqual({ - defaultUser: '111', - }) - }) - function setConfig(config: Record): void { configContent = `${JSON.stringify(config, null, 2)}\n` } - function readConfig(): Record | null { - return configContent ? (JSON.parse(configContent) as Record) : null - } - function createErrnoError(code: string): Error & { code: string } { const error = new Error(code) as Error & { code: string } error.code = code diff --git a/src/lib/auth.ts b/src/lib/auth.ts index 00d2a0b..1b2d305 100644 --- a/src/lib/auth.ts +++ b/src/lib/auth.ts @@ -1,10 +1,5 @@ -import { - accountForUser, - createSecureStore, - LEGACY_ACCOUNT_NAME, - SECURE_STORE_DESCRIPTION, - SecureStoreUnavailableError, -} from './secure-store.js' +import { createSecureStore, SecureStoreUnavailableError } from '@doist/cli-core/auth' +export type { TokenStorageLocation, TokenStorageResult } from '@doist/cli-core/auth' export { AUTH_FLAG_ORDER, CONFIG_VERSION, @@ -18,30 +13,13 @@ export { type UpdateChannel, } from './config.js' -import { - CONFIG_VERSION, - getConfigPath, - readConfig, - writeConfig, - type AuthFlag, - type AuthMode, - type Config, - type StoredUser, -} from './config.js' +import { accountForUser, SERVICE_NAME, TOKEN_ENV_VAR } from './auth-store.js' +import { type AuthFlag, type AuthMode, readConfig, type StoredUser } from './config.js' import { CliError } from './errors.js' import { getRequestedUserRef } from './global-args.js' -import { - findUserByRef, - getDefaultUser, - getStoredUsers, - NoUserSelectedError, - removeStoredUser, - setDefaultUser as setDefaultUserInConfig, - upsertStoredUser, - UserNotFoundError, -} from './users.js' +import { findUserByRef, getStoredUsers, NoUserSelectedError, UserNotFoundError } from './users.js' -export const TOKEN_ENV_VAR = 'TODOIST_API_TOKEN' +export { TOKEN_ENV_VAR } from './auth-store.js' export interface AuthMetadata { authMode: AuthMode @@ -62,15 +40,6 @@ export interface ResolvedUser { source: AuthMetadata['source'] } -export interface UpsertUserInput { - id: string - email: string - token: string - authMode?: AuthMode - authScope?: string - authFlags?: AuthFlag[] -} - export class NoTokenError extends CliError { constructor() { super( @@ -83,17 +52,6 @@ export class NoTokenError extends CliError { } } -export type TokenStorageLocation = 'secure-store' | 'config-file' - -export interface TokenStorageResult { - storage: TokenStorageLocation - warning?: string -} - -// --------------------------------------------------------------------------- -// Public API — used by api/core, uploads, stats, doctor, auth subcommands -// --------------------------------------------------------------------------- - /** * Resolve which stored user this invocation should act as, and load their * token. Honors `--user `, then `user.defaultUser`, then a single stored @@ -107,57 +65,21 @@ export interface TokenStorageResult { export async function resolveActiveUser(opts: { ref?: string } = {}): Promise { const envToken = process.env[TOKEN_ENV_VAR] if (envToken) { - return { - id: 'env', - email: '', - token: envToken, - authMode: 'unknown', - source: 'env', - } + return { id: 'env', email: '', token: envToken, authMode: 'unknown', source: 'env' } } const config = await readConfig() const users = getStoredUsers(config) - const requestedRef = opts.ref ?? getRequestedUserRef() + const ref = opts.ref ?? getRequestedUserRef() - // Gate the legacy fallback on the *absence* of `config.users` rather than - // an empty array. A v2 config that has been logged out (`users: []`) must - // not silently fall back to a stale `api-token` keyring entry — that - // would let a forgotten v1 credential reauthenticate the next command. - const isLegacyShape = !Array.isArray(config.users) if (users.length === 0) { - if (requestedRef) { - // Asked for a specific user, but the store is empty — same error - // as missing user, scoped to the request. - throw new UserNotFoundError(requestedRef) - } - if (isLegacyShape) { - return resolveLegacyToken(config) - } - throw new NoTokenError() + throw ref ? new UserNotFoundError(ref) : new NoTokenError() } - let target: StoredUser - if (requestedRef) { - const found = findUserByRef(config, requestedRef) - if (!found) throw new UserNotFoundError(requestedRef) - target = found.user - } else if (config.user?.defaultUser) { - const found = findUserByRef(config, config.user.defaultUser) - if (!found) { - // Default points at a missing user — treat like no default. - if (users.length === 1) { - target = users[0] - } else { - throw new NoUserSelectedError() - } - } else { - target = found.user - } - } else if (users.length === 1) { - target = users[0] - } else { - throw new NoUserSelectedError() + const target = ref ? (findUserByRef(config, ref)?.user ?? null) : pickDefault(config, users) + if (!target) { + // ref miss when records exist, or multi-user no-default. + throw ref ? new UserNotFoundError(ref) : new NoUserSelectedError() } const { token, source } = await loadTokenForStoredUser(target) @@ -172,38 +94,40 @@ export async function resolveActiveUser(opts: { ref?: string } = {}): Promise u.id === defaultId) + if (found) return found + // Default points at a missing user — fall through to single-user fallback. + } + return users.length === 1 ? users[0] : null +} + +/** Bearer-only shortcut. Most call sites (SDK, uploads, stats) only need this. */ export async function getApiToken(): Promise { - const resolved = await resolveActiveUser() - return resolved.token + return (await resolveActiveUser()).token } /** - * Like `resolveActiveUser` but returns whichever credentials are at hand - * without mutating storage. Useful for `td doctor` / `td config view` where - * we want to inspect (and report on) what would be used. + * Like `resolveActiveUser` but bundles the rendering metadata `td doctor` / + * `td config view` want. Lets `SecureStoreUnavailableError` propagate so the + * diagnostic surfaces can distinguish "missing token" from "broken keyring". */ export async function probeApiToken(): Promise<{ token: string; metadata: AuthMetadata }> { const resolved = await resolveActiveUser() - return { - token: resolved.token, - metadata: resolvedToMetadata(resolved), - } + return { token: resolved.token, metadata: resolvedToMetadata(resolved) } } export async function getAuthMetadata(): Promise { try { - const resolved = await resolveActiveUser() - return resolvedToMetadata(resolved) + return resolvedToMetadata(await resolveActiveUser()) } catch (error) { - // Metadata callers (e.g. `ensureWriteAllowed`, scope-error - // remediation) need a sensible default rather than a hard failure - // when credentials are missing or the keyring is offline. Diagnostic - // commands use `probeApiToken` for that — it intentionally lets - // `SecureStoreUnavailableError` propagate so it can be reported. + // Permission/scope callers need a sensible default rather than a hard + // failure when credentials are missing or the keyring is offline. if (error instanceof NoTokenError || error instanceof SecureStoreUnavailableError) { return { authMode: 'unknown', source: 'secure-store' } } @@ -211,327 +135,33 @@ export async function getAuthMetadata(): Promise { } } -/** - * Add or update a user record. Stores the token in the OS credential manager - * under `user-` when available, falls back to per-user plaintext in config. - * Sets `defaultUser` automatically if this is the first user being stored. - */ -export async function upsertUser( - input: UpsertUserInput, -): Promise { - if (!input.token || input.token.trim().length < 10) { - throw new CliError('INVALID_TOKEN', 'Invalid token: Token must be at least 10 characters') - } - if (!input.id) { - throw new CliError('INVALID_USER', 'Cannot store user record: missing id') - } - if (!input.email) { - throw new CliError('INVALID_USER', 'Cannot store user record: missing email') - } - - const trimmedToken = input.token.trim() - const config = await readConfig() - const previouslyExisted = getStoredUsers(config).some((u) => u.id === input.id) - // Always set the first user as the default — even if `config.user.defaultUser` - // points at a stale/orphaned id, that pointer would otherwise wedge multi-user - // resolution on subsequent logins. - const shouldSetDefault = getStoredUsers(config).length === 0 - - const baseRecord: StoredUser = { - id: input.id, - email: input.email, - auth_mode: input.authMode, - auth_scope: input.authScope, - auth_flags: input.authFlags, - } - - const secureStore = createSecureStore(accountForUser(input.id)) - let storedSecurely = false - try { - await secureStore.setSecret(trimmedToken) - storedSecurely = true - } catch (error) { - if (!(error instanceof SecureStoreUnavailableError)) throw error - } - - const userRecord: StoredUser = storedSecurely - ? baseRecord - : { ...baseRecord, api_token: trimmedToken } - - let next = ensureV2Shape(config) - next = upsertStoredUser(next, userRecord).config - if (shouldSetDefault) { - next = setDefaultUserInConfig(next, input.id) - } - next = stripLegacyAuthFields(next) - - try { - await writeConfig(next) - } catch (error) { - // Config write is the source of truth — without it, later commands - // can't resolve the user even though the keyring holds the secret. - // Roll the keyring back so we don't leak credentials for a non-stored - // account, then surface the failure. - if (storedSecurely) { - try { - await secureStore.deleteSecret() - } catch { - // best effort — the original error is what the user needs - } - } - const detail = error instanceof Error && error.message ? `: ${error.message}` : '' - throw new CliError( - 'CONFIG_WRITE_FAILED', - `Could not persist account record to ${getConfigPath()}${detail}`, - ['Check file permissions on ~/.config/todoist-cli/, then re-run the command'], - ) - } - - return { - storage: storedSecurely ? 'secure-store' : 'config-file', - warning: storedSecurely ? undefined : buildFallbackWarning('token saved as plaintext in'), - replaced: previouslyExisted, - } -} - -/** - * Remove the active user (resolved via `--user` or default). For multi-user - * installs without a default, callers must pass `--user ` to disambiguate. - */ -export async function clearApiToken(opts: { ref?: string } = {}): Promise { - const config = await readConfig() - const users = getStoredUsers(config) - const requestedRef = opts.ref ?? getRequestedUserRef() - - // No users stored yet — fall through to legacy logout only on a v1-shaped - // config (no `users` key at all). Empty `users: []` is an already-clean - // v2 install; treat it as a no-op rather than poking the legacy keyring. - // - // A `requestedRef` always wins over the legacy fallback: a caller asking - // for a specific account on a legacy install should see `USER_NOT_FOUND`, - // not have their legacy token silently wiped (matches `resolveActiveUser` - // above and prevents `td auth logout --user missing@x` from clearing the - // wrong credential). - if (users.length === 0) { - if (requestedRef) { - throw new UserNotFoundError(requestedRef) - } - if (!Array.isArray(config.users)) { - return clearLegacyToken(config) - } - throw new NoTokenError() - } - - let target: StoredUser - if (requestedRef) { - const found = findUserByRef(config, requestedRef) - if (!found) throw new UserNotFoundError(requestedRef) - target = found.user - } else { - const def = getDefaultUser(config) - if (def) { - target = def - } else if (users.length === 1) { - target = users[0] - } else { - throw new NoUserSelectedError() - } - } - - return removeUserById(target.id) -} - -/** - * Remove a specific user by id. Used by `td user remove` and as the underlying - * primitive for `clearApiToken`. - * - * Order matters: write the new config first (the source of truth) and only - * then delete the secret. If the config update fails the keyring is untouched, - * so the user remains fully functional and a retry will simply re-attempt - * the same operation. A keyring delete failure after a successful config - * update leaves an orphan secret that the keyring's own service can clean up - * later — the CLI no longer references it. - */ -export async function removeUserById(id: string): Promise { - const config = await readConfig() - const next = stripLegacyAuthFields(removeStoredUser(ensureV2Shape(config), id)) - - try { - await writeConfig(next) - } catch (error) { - const detail = error instanceof Error && error.message ? `: ${error.message}` : '' - throw new CliError('CONFIG_WRITE_FAILED', `Could not update ${getConfigPath()}${detail}`, [ - 'Check file permissions on ~/.config/todoist-cli/, then re-run the command', - ]) - } - - const secureStore = createSecureStore(accountForUser(id)) - try { - await secureStore.deleteSecret() - } catch (error) { - if (!(error instanceof SecureStoreUnavailableError)) throw error - return { - storage: 'config-file', - warning: buildFallbackWarning('local auth state cleared in'), - } - } - - return { storage: 'secure-store' } -} - -export async function setDefaultUserId(id: string): Promise { - const config = ensureV2Shape(await readConfig()) - const found = findUserByRef(config, id) - if (!found) throw new UserNotFoundError(id) - await writeConfig(stripLegacyAuthFields(setDefaultUserInConfig(config, found.user.id))) -} - export async function listStoredUsers(): Promise { - const config = await readConfig() - return getStoredUsers(config) + return getStoredUsers(await readConfig()) } -// --------------------------------------------------------------------------- -// Internal helpers -// --------------------------------------------------------------------------- - -export async function loadTokenForStoredUser( +async function loadTokenForStoredUser( user: StoredUser, ): Promise<{ token: string; source: 'secure-store' | 'config-file' }> { if (user.api_token?.trim()) { return { token: user.api_token.trim(), source: 'config-file' } } - const secureStore = createSecureStore(accountForUser(user.id)) - // Re-throw `SecureStoreUnavailableError` rather than collapsing it into - // `NoTokenError`. A stored v2 user with the keyring offline is *not* the - // same situation as no credentials at all — `td doctor` and `td config - // view` both have dedicated handling for the unavailable-store case and - // should report the keyring failure rather than misleadingly say the user - // has no saved credentials. - const stored = await secureStore.getSecret() - if (stored?.trim()) { - return { token: stored.trim(), source: 'secure-store' } - } - throw new NoTokenError() -} - -/** - * v1 fallback: when there are no v2 users in the config, see if a legacy - * single-user token is present (config plaintext or legacy `api-token` - * keyring entry). Returns it as a synthetic ResolvedUser so the CLI keeps - * working until postinstall (or `td auth login`) migrates the install. - */ -async function resolveLegacyToken(config: Config): Promise { - const legacyToken = typeof config.api_token === 'string' ? config.api_token.trim() : '' - if (legacyToken) { - return { - id: 'legacy', - email: '', - token: legacyToken, - authMode: config.auth_mode ?? 'unknown', - authScope: config.auth_scope, - authFlags: config.auth_flags, - source: 'config-file', - } - } - - if (config.pendingSecureStoreClear) { - // v1 logout state: nothing to restore. Surface as the same NoTokenError - // the v1 path used to throw. - throw new NoTokenError() - } - - const secureStore = createSecureStore(LEGACY_ACCOUNT_NAME) - try { - const stored = await secureStore.getSecret() - if (stored?.trim()) { - return { - id: 'legacy', - email: '', - token: stored.trim(), - authMode: config.auth_mode ?? 'unknown', - authScope: config.auth_scope, - authFlags: config.auth_flags, - source: 'secure-store', - } - } - } catch (error) { - if (!(error instanceof SecureStoreUnavailableError)) throw error - } - + // Let `SecureStoreUnavailableError` propagate — a stored v2 user with the + // keyring offline is not the same situation as no credentials at all. + const stored = await createSecureStore({ + serviceName: SERVICE_NAME, + account: accountForUser(user.id), + }).getSecret() + if (stored?.trim()) return { token: stored.trim(), source: 'secure-store' } throw new NoTokenError() } -async function clearLegacyToken(config: Config): Promise { - const secureStore = createSecureStore(LEGACY_ACCOUNT_NAME) - - try { - await secureStore.deleteSecret() - const cleaned = stripLegacyAuthFields(config) - try { - await writeConfig(cleaned) - } catch (error) { - return { - storage: 'secure-store', - warning: buildConfigCleanupWarning('Secure-store token was removed,', error), - } - } - return { storage: 'secure-store' } - } catch (error) { - if (!(error instanceof SecureStoreUnavailableError)) throw error - } - - const cleared: Config = { - ...stripLegacyAuthFields(config), - pendingSecureStoreClear: true, - } - try { - await writeConfig(cleared) - } catch { - // best-effort - } - return { - storage: 'config-file', - warning: buildFallbackWarning('local auth state cleared in'), - } -} - -function ensureV2Shape(config: Config): Config { - const next: Config = { ...config, config_version: CONFIG_VERSION } - if (!Array.isArray(next.users)) { - next.users = [] - } - return next -} - -function stripLegacyAuthFields(config: Config): Config { - const { - api_token: _t, - auth_mode: _m, - auth_scope: _s, - auth_flags: _f, - pendingSecureStoreClear: _p, - ...rest - } = config - return rest -} - function resolvedToMetadata(resolved: ResolvedUser): AuthMetadata { return { authMode: resolved.authMode, authScope: resolved.authScope, authFlags: resolved.authFlags, source: resolved.source, - userId: resolved.id === 'env' || resolved.id === 'legacy' ? undefined : resolved.id, + userId: resolved.id === 'env' ? undefined : resolved.id, email: resolved.email || undefined, } } - -function buildFallbackWarning(action: string): string { - return `${SECURE_STORE_DESCRIPTION} unavailable; ${action} ${getConfigPath()}` -} - -function buildConfigCleanupWarning(prefix: string, error: unknown): string { - const detail = error instanceof Error && error.message ? ` (${error.message})` : '' - return `${prefix} but could not update ${getConfigPath()}${detail}` -} diff --git a/src/lib/config.ts b/src/lib/config.ts index 51c1b9b..9113d53 100644 --- a/src/lib/config.ts +++ b/src/lib/config.ts @@ -160,6 +160,25 @@ export async function writeConfig(config: Config): Promise { await writeConfigCore(getConfigPath(), config, { deleteWhenEmpty: true }) } +/** + * Strip the v1 top-level auth fields. Called on every v2 config write so a + * stale `api_token` / `auth_mode` / `auth_flags` from a pre-migration config + * can't outlive a successful multi-user write. Single source of truth for + * the list — the keyring/migration paths and the v2-shape normalizer all + * delegate here. + */ +export function stripLegacyAuthFields(config: Config): Config { + const { + api_token: _t, + auth_mode: _m, + auth_scope: _s, + auth_flags: _f, + pendingSecureStoreClear: _p, + ...rest + } = config + return rest +} + // Keep this validator ad-hoc for now: it is only used by `td doctor`, and the // config schema is still small enough that adding a runtime validation // dependency would be heavier than the problem. If more config or payload diff --git a/src/lib/migrate-auth.test.ts b/src/lib/migrate-auth.test.ts index 7f26a1c..19f3cbf 100644 --- a/src/lib/migrate-auth.test.ts +++ b/src/lib/migrate-auth.test.ts @@ -1,35 +1,37 @@ +import type { MigrateAuthResult, MigrateLegacyAuthOptions } from '@doist/cli-core/auth' +/** + * Adapter-level tests for the local `migrate-auth.ts` wrapper. The actual + * migration mechanics (keyring read/write, default promotion, rollback) + * live in cli-core and are covered by its own `migrate.test.ts`. Here we + * verify only the todoist-specific glue we pass into cli-core: the legacy + * plaintext loader, the `/api/v1/user` identifier callback, and the + * legacy-config cleanup hook. + */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import type { TodoistAccount } from './auth-store.js' const TEST_HOME = '/tmp/todoist-cli-tests-migrate' - -interface KeyringEntryState { - token: string | null -} - -interface KeyringMockState { - entries: Map - available: boolean -} - -function entryFor(state: KeyringMockState, account: string): KeyringEntryState { - let e = state.entries.get(account) - if (!e) { - e = { token: null } - state.entries.set(account, e) - } - return e -} - -describe('migrateLegacyAuth', () => { +const TEST_CONFIG_PATH = `${TEST_HOME}/.config/todoist-cli/config.json` + +// Capture the options object the wrapper passes into cli-core. Each test +// re-imports the wrapper after mocks are installed so the captured value is +// scoped to one test. `coreResult` lets a test pre-load the return value the +// stubbed `migrateLegacyAuth` should resolve to — needed for the +// `toLocalResult` translation cases that don't have a real cli-core run to +// observe. +let capturedOptions: MigrateLegacyAuthOptions | undefined +let coreResult: MigrateAuthResult + +describe('migrateLegacyAuth (todoist-cli wrapper)', () => { let configContent: string | null - let keyring: KeyringMockState beforeEach(() => { vi.resetModules() vi.clearAllMocks() + capturedOptions = undefined + coreResult = { status: 'no-legacy-state' } configContent = null - keyring = { entries: new Map(), available: true } vi.doMock('node:os', () => ({ homedir: () => TEST_HOME })) vi.doMock('node:fs/promises', () => ({ @@ -51,27 +53,28 @@ describe('migrateLegacyAuth', () => { }), })) - vi.doMock('@napi-rs/keyring', () => ({ - AsyncEntry: class { - private account: string - constructor(_service: string, account: string) { - if (!keyring.available) throw new Error('keyring unavailable') - this.account = account - } - async getPassword(): Promise { - return entryFor(keyring, this.account).token - } - async setPassword(p: string): Promise { - entryFor(keyring, this.account).token = p - } - async deleteCredential(): Promise { - const e = entryFor(keyring, this.account) - const had = e.token !== null - e.token = null - return had - } - }, - })) + vi.doMock('@doist/cli-core', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + getConfigPath: () => TEST_CONFIG_PATH, + } + }) + + // Stub cli-core's migration so we observe what the wrapper passes in + // and can drive each consumer callback directly. + vi.doMock('@doist/cli-core/auth', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + migrateLegacyAuth: vi.fn( + async (options: MigrateLegacyAuthOptions) => { + capturedOptions = options + return coreResult + }, + ), + } + }) }) afterEach(() => { @@ -81,35 +84,54 @@ describe('migrateLegacyAuth', () => { function setConfig(config: unknown): void { configContent = `${JSON.stringify(config, null, 2)}\n` } + function readConfig(): Record | null { return configContent ? (JSON.parse(configContent) as Record) : null } - it('reports already-migrated when users array exists', async () => { - setConfig({ config_version: 2, users: [] }) + describe('hasMigrated / markMigrated (the one-way gate)', () => { + it('hasMigrated returns true when config_version is at CONFIG_VERSION', async () => { + setConfig({ config_version: 2, users: [] }) + const { migrateLegacyAuth } = await import('./migrate-auth.js') + await migrateLegacyAuth({ silent: true }) + await expect(capturedOptions?.hasMigrated()).resolves.toBe(true) + }) - const { migrateLegacyAuth } = await import('./migrate-auth.js') - const result = await migrateLegacyAuth({ silent: true, fetchImpl: failingFetch }) + it('hasMigrated returns false on a pre-v2 config (lets cli-core proceed)', async () => { + setConfig({ api_token: 'legacy-1234567' }) + const { migrateLegacyAuth } = await import('./migrate-auth.js') + await migrateLegacyAuth({ silent: true }) + await expect(capturedOptions?.hasMigrated()).resolves.toBe(false) + }) - expect(result.status).toBe('already-migrated') - expect(readConfig()).toEqual({ config_version: 2, users: [] }) + it('markMigrated stamps config_version without touching the legacy fields', async () => { + // Legacy strip is cleanupLegacyConfig's job — markMigrated only + // sets the durable gate so a later logout + reinstall can't + // re-migrate the (now stale) legacy token. + setConfig({ api_token: 'legacy-1234567', auth_mode: 'read-write' }) + const { migrateLegacyAuth } = await import('./migrate-auth.js') + await migrateLegacyAuth({ silent: true }) + await capturedOptions?.markMigrated() + expect(readConfig()).toEqual({ + api_token: 'legacy-1234567', + auth_mode: 'read-write', + config_version: 2, + }) + }) }) - it('reports no-legacy-state when config is empty and no keyring token', async () => { + it('loadLegacyPlaintextToken returns config.api_token when set', async () => { + setConfig({ api_token: ' legacy-token-1234567 ' }) const { migrateLegacyAuth } = await import('./migrate-auth.js') - const result = await migrateLegacyAuth({ silent: true, fetchImpl: failingFetch }) + await migrateLegacyAuth({ silent: true }) - expect(result.status).toBe('no-legacy-state') - expect(readConfig()).toBeNull() + await expect(capturedOptions?.loadLegacyPlaintextToken()).resolves.toBe( + 'legacy-token-1234567', + ) }) - it('migrates a v1 plaintext token via REST user fetch', async () => { - setConfig({ - api_token: 'legacy-1234567', - auth_mode: 'read-write', - auth_scope: 'data:read_write', - }) - + it('identifyAccount calls /api/v1/user with the bearer token and todoist headers', async () => { + setConfig({ auth_mode: 'read-write', auth_scope: 'data:read_write' }) const fetchImpl = vi.fn( async () => new Response(JSON.stringify({ id: '999', email: 'me@example.com' }), { @@ -118,87 +140,98 @@ describe('migrateLegacyAuth', () => { ) const { migrateLegacyAuth } = await import('./migrate-auth.js') - const result = await migrateLegacyAuth({ silent: true, fetchImpl }) + await migrateLegacyAuth({ silent: true, fetchImpl }) - expect(result).toMatchObject({ - status: 'migrated', - migratedUserId: '999', - migratedEmail: 'me@example.com', - }) - expect(entryFor(keyring, 'user-999').token).toBe('legacy-1234567') - expect(readConfig()).toEqual({ - config_version: 2, - user: { defaultUser: '999' }, - users: [ - { - id: '999', - email: 'me@example.com', - auth_mode: 'read-write', - auth_scope: 'data:read_write', - }, - ], + const account = await capturedOptions?.identifyAccount('legacy-token-1234567') + + expect(account).toEqual({ + id: '999', + email: 'me@example.com', + label: 'me@example.com', + auth_mode: 'read-write', + auth_scope: 'data:read_write', + auth_flags: undefined, }) expect(fetchImpl).toHaveBeenCalledWith( 'https://api.todoist.com/api/v1/user', expect.objectContaining({ headers: expect.objectContaining({ - authorization: 'Bearer legacy-1234567', + authorization: 'Bearer legacy-token-1234567', 'doist-platform': 'cli', - 'doist-version': expect.stringMatching(/^\d+\.\d+\.\d+$/), - 'request-id': expect.any(String), - 'session-id': expect.any(String), 'cli-command': 'postinstall:auth-migrate', }), }), ) }) - it('migrates a legacy keyring token (api-token account)', async () => { - entryFor(keyring, 'api-token').token = 'legacy-secure-1234567' - - const fetchImpl = vi.fn( - async () => new Response(JSON.stringify({ id: '42', email: 'k@e.y' }), { status: 200 }), - ) - + it('identifyAccount throws when the API request fails', async () => { + const fetchImpl = vi.fn(async () => new Response('boom', { status: 500 })) const { migrateLegacyAuth } = await import('./migrate-auth.js') - const result = await migrateLegacyAuth({ silent: true, fetchImpl }) + await migrateLegacyAuth({ silent: true, fetchImpl }) - expect(result.status).toBe('migrated') - expect(entryFor(keyring, 'user-42').token).toBe('legacy-secure-1234567') - // legacy slot should be cleared - expect(entryFor(keyring, 'api-token').token).toBeNull() + await expect(capturedOptions?.identifyAccount('any-token-1234567')).rejects.toThrow(/500/) }) - it('skips and leaves config untouched when fetch fails', async () => { - setConfig({ api_token: 'legacy-1234567' }) + describe('toLocalResult translation', () => { + it('migrated → migratedUserId + migratedEmail (the local result shape)', async () => { + coreResult = { + status: 'migrated', + account: { + id: '999', + email: 'me@example.com', + label: 'me@example.com', + auth_mode: 'read-write', + }, + } - const fetchImpl = vi.fn(async () => new Response('boom', { status: 500 })) + const { migrateLegacyAuth } = await import('./migrate-auth.js') + const result = await migrateLegacyAuth({ silent: true }) - const { migrateLegacyAuth } = await import('./migrate-auth.js') - const result = await migrateLegacyAuth({ silent: true, fetchImpl }) + expect(result).toEqual({ + status: 'migrated', + migratedUserId: '999', + migratedEmail: 'me@example.com', + }) + }) + + it('skipped → flattens cli-core reason + detail into one string', async () => { + coreResult = { + status: 'skipped', + reason: 'identify-failed', + detail: 'HTTP 500 boom', + } - expect(result.status).toBe('skipped') - expect(readConfig()).toEqual({ api_token: 'legacy-1234567' }) + const { migrateLegacyAuth } = await import('./migrate-auth.js') + const result = await migrateLegacyAuth({ silent: true }) + + expect(result).toEqual({ + status: 'skipped', + reason: 'identify-failed: HTTP 500 boom', + }) + }) }) - it('is idempotent — second run after a successful migration is a no-op', async () => { - setConfig({ api_token: 'legacy-1234567' }) - const fetchImpl = vi.fn( - async () => new Response(JSON.stringify({ id: '7', email: 'a@b.c' }), { status: 200 }), - ) + it('cleanupLegacyConfig strips top-level v1 fields', async () => { + setConfig({ + api_token: 'legacy-1234567', + auth_mode: 'read-write', + auth_scope: 'data:read_write', + auth_flags: ['read-only'], + pendingSecureStoreClear: true, + config_version: 2, + users: [{ id: '999', email: 'me@example.com' }], + user: { defaultUser: '999' }, + }) const { migrateLegacyAuth } = await import('./migrate-auth.js') + await migrateLegacyAuth({ silent: true }) - const first = await migrateLegacyAuth({ silent: true, fetchImpl }) - expect(first.status).toBe('migrated') + await capturedOptions?.cleanupLegacyConfig?.() - const second = await migrateLegacyAuth({ silent: true, fetchImpl }) - expect(second.status).toBe('already-migrated') - // fetch only called once - expect(fetchImpl).toHaveBeenCalledTimes(1) + expect(readConfig()).toEqual({ + config_version: 2, + users: [{ id: '999', email: 'me@example.com' }], + user: { defaultUser: '999' }, + }) }) }) - -const failingFetch: typeof fetch = async () => { - throw new Error('fetch should not be called') -} diff --git a/src/lib/migrate-auth.ts b/src/lib/migrate-auth.ts index 4822601..3f8f4f8 100644 --- a/src/lib/migrate-auth.ts +++ b/src/lib/migrate-auth.ts @@ -1,20 +1,32 @@ /** * One-time migration of v1 single-user auth state into the v2 multi-user - * shape. Invoked from `src/postinstall.ts` so existing installs upgrade - * transparently. Best-effort: any failure (offline, invalid token, keyring - * unavailable) leaves the v1 state untouched so the runtime fallback in - * `resolveActiveUser` can keep serving the legacy token until the next - * upgrade attempt or `td auth login`. + * shape. Delegates to `@doist/cli-core/auth`'s generic `migrateLegacyAuth`, + * supplying only the todoist-specific bits: the durable migration marker + * (`config_version === CONFIG_VERSION`), how to read the v1 plaintext token, + * how to identify the user behind a v1 token, and how to clean up the + * top-level legacy fields after a successful migration. + * + * Invoked from `src/postinstall.ts`. Best-effort: any failure leaves v1 state + * untouched so `resolveActiveUser`'s legacy fallback keeps serving the token + * until the next attempt. */ -import { type Config, CONFIG_VERSION, readConfig, type StoredUser, writeConfig } from './config.js' +import { migrateLegacyAuth as coreMigrateLegacyAuth } from '@doist/cli-core/auth' import { - accountForUser, - createSecureStore, - LEGACY_ACCOUNT_NAME, - SecureStoreUnavailableError, -} from './secure-store.js' + LEGACY_ACCOUNT, + SERVICE_NAME, + toTodoistAccount, + type TodoistAccount, +} from './auth-store.js' +import { + type Config, + CONFIG_VERSION, + readConfig, + stripLegacyAuthFields, + writeConfig, +} from './config.js' import { fetchTodoist } from './usage-tracking.js' +import { createTodoistUserRecordStore } from './user-records.js' export interface MigrateAuthResult { status: 'already-migrated' | 'no-legacy-state' | 'migrated' | 'skipped' @@ -30,124 +42,76 @@ interface MigrateOptions { fetchImpl?: typeof fetch } -interface TodoistUser { - id: string - email: string -} - const USER_ENDPOINT = 'https://api.todoist.com/api/v1/user' export async function migrateLegacyAuth(opts: MigrateOptions = {}): Promise { const fetchImpl = opts.fetchImpl ?? fetch - const config = await readConfig() - - // Already on v2 — the presence of `users` is the marker. Empty array still - // counts as migrated (clean slate after a logout). - if (Array.isArray(config.users)) { - return { status: 'already-migrated' } - } - - const legacyToken = await loadLegacyToken(config) - if (!legacyToken) { - // No usable token to migrate. Still write the v2 marker so the runtime - // resolver doesn't keep looking for one — but only if there's nothing - // there at all (untouched config file). - if ( - config.api_token === undefined && - config.auth_mode === undefined && - config.auth_scope === undefined && - config.auth_flags === undefined && - !config.pendingSecureStoreClear - ) { - return { status: 'no-legacy-state' } - } - - // Legacy state exists (e.g., pendingSecureStoreClear) but no token. - // Clean up to v2 shape. - try { - await writeConfig(toV2(stripLegacy(config), [])) - } catch (error) { - return skipped(opts, `failed to clean up legacy config (${describe(error)})`) - } - return { status: 'migrated' } - } - - // Identify the user behind the legacy token via a one-shot REST call — - // no SDK import to keep postinstall lightweight. - let user: TodoistUser - try { - user = await fetchUser(legacyToken, fetchImpl) - } catch (error) { - return skipped(opts, `could not identify Todoist user (${describe(error)})`) - } - const record: StoredUser = { - id: user.id, - email: user.email, - auth_mode: config.auth_mode, - auth_scope: config.auth_scope, - auth_flags: config.auth_flags, + // Cache the config across the cli-core callbacks: a successful migration + // hits `hasMigrated`, `loadLegacyPlaintextToken`, `identifyAccount`, and + // `cleanupLegacyConfig` back-to-back. Without the cache we'd re-read + + // re-parse the same file four times for one postinstall run. + let cached: Config | undefined + const read = async (): Promise => { + if (cached === undefined) cached = await readConfig() + return cached } - - // Move the token into the per-user keyring slot. - let storedSecurely = false - try { - const userStore = createSecureStore(accountForUser(user.id)) - await userStore.setSecret(legacyToken) - storedSecurely = true - } catch (error) { - if (!(error instanceof SecureStoreUnavailableError)) { - return skipped(opts, `failed to write user-scoped credential (${describe(error)})`) - } - } - - if (!storedSecurely) { - record.api_token = legacyToken - } - - const next = toV2(stripLegacy(config), [record], user.id) - - try { + const write = async (next: Config): Promise => { await writeConfig(next) - } catch (error) { - return skipped(opts, `failed to update config (${describe(error)})`) - } - - // Clean up the legacy keyring entry — best effort, never fatal. - try { - const legacyStore = createSecureStore(LEGACY_ACCOUNT_NAME) - await legacyStore.deleteSecret() - } catch { - // ignore - } - - if (!opts.silent) { - console.error(`todoist-cli: migrated existing token to multi-user store (${user.email}).`) - } + cached = next + } + + const result = await coreMigrateLegacyAuth({ + serviceName: SERVICE_NAME, + legacyAccount: LEGACY_ACCOUNT, + userRecords: createTodoistUserRecordStore(), + silent: opts.silent, + logPrefix: 'todoist-cli', + // `config_version === CONFIG_VERSION` is the durable, one-way gate. It + // survives logout (which clears `users[]` but leaves `config_version`), + // so a reinstall over a logged-out v2 install cannot re-migrate a + // stale legacy keyring entry. + hasMigrated: async () => (await read()).config_version === CONFIG_VERSION, + markMigrated: async () => { + const config = await read() + if (config.config_version === CONFIG_VERSION) return + await write({ ...config, config_version: CONFIG_VERSION }) + }, + loadLegacyPlaintextToken: async () => { + const config = await read() + return typeof config.api_token === 'string' && config.api_token.trim() + ? config.api_token.trim() + : null + }, + identifyAccount: async (token) => { + const config = await read() + const user = await fetchUser(token, fetchImpl) + return toTodoistAccount({ + id: user.id, + email: user.email, + authMode: config.auth_mode ?? 'unknown', + authScope: config.auth_scope, + authFlags: config.auth_flags, + }) + }, + cleanupLegacyConfig: async () => { + const config = await read() + await write(stripLegacyAuthFields(config)) + }, + }) - return { status: 'migrated', migratedUserId: user.id, migratedEmail: user.email } + return toLocalResult(result) } -async function loadLegacyToken(config: Config): Promise { - if (typeof config.api_token === 'string' && config.api_token.trim()) { - return config.api_token.trim() - } - try { - const legacyStore = createSecureStore(LEGACY_ACCOUNT_NAME) - const stored = await legacyStore.getSecret() - if (stored?.trim()) return stored.trim() - } catch (error) { - if (!(error instanceof SecureStoreUnavailableError)) throw error - } - return null +interface TodoistUser { + id: string + email: string } async function fetchUser(token: string, fetchImpl: typeof fetch): Promise { const response = await fetchTodoist( USER_ENDPOINT, - { - headers: { Authorization: `Bearer ${token}` }, - }, + { headers: { Authorization: `Bearer ${token}` } }, fetchImpl, 'postinstall:auth-migrate', ) @@ -164,33 +128,18 @@ async function fetchUser(token: string, fetchImpl: typeof fetch): Promise>>, +): MigrateAuthResult { + if (result.status === 'migrated') { + return { + status: 'migrated', + migratedUserId: result.account.id, + migratedEmail: result.account.email, + } } - return next -} - -function stripLegacy(config: Config): Config { - const { - api_token: _t, - auth_mode: _m, - auth_scope: _s, - auth_flags: _f, - pendingSecureStoreClear: _p, - ...rest - } = config - return rest -} - -function describe(error: unknown): string { - return error instanceof Error && error.message ? error.message : String(error) -} - -function skipped(opts: MigrateOptions, reason: string): MigrateAuthResult { - if (!opts.silent) { - console.error(`todoist-cli: skipped legacy auth migration — ${reason}.`) + if (result.status === 'skipped') { + return { status: 'skipped', reason: `${result.reason}: ${result.detail}` } } - return { status: 'skipped', reason } + return { status: result.status } } diff --git a/src/lib/secure-store.ts b/src/lib/secure-store.ts deleted file mode 100644 index 1d14058..0000000 --- a/src/lib/secure-store.ts +++ /dev/null @@ -1,84 +0,0 @@ -const SERVICE_NAME = 'todoist-cli' - -/** - * Legacy single-user account name. Read once during migration to v2 storage, - * then deleted. New code should always pass an explicit `user-` account. - */ -export const LEGACY_ACCOUNT_NAME = 'api-token' - -export const SECURE_STORE_DESCRIPTION = 'system credential manager' - -export class SecureStoreUnavailableError extends Error { - constructor(message = 'System credential storage is unavailable') { - super(message) - this.name = 'SecureStoreUnavailableError' - } -} - -export interface SecureStore { - getSecret(): Promise - setSecret(secret: string): Promise - deleteSecret(): Promise -} - -/** - * Build the keyring account name for a given Todoist user id. - */ -export function accountForUser(userId: string): string { - return `user-${userId}` -} - -/** - * Create a secure-store handle for a specific keyring account. Pass the result - * of `accountForUser(id)` for per-user tokens, or `LEGACY_ACCOUNT_NAME` when - * reading/cleaning up the v1 single-user entry. - */ -export function createSecureStore(account: string = LEGACY_ACCOUNT_NAME): SecureStore { - return { - async getSecret(): Promise { - const entry = await getEntry(account) - try { - return (await entry.getPassword()) ?? null - } catch (error) { - throw toUnavailableError(error) - } - }, - - async setSecret(secret: string): Promise { - const entry = await getEntry(account) - try { - await entry.setPassword(secret) - } catch (error) { - throw toUnavailableError(error) - } - }, - - async deleteSecret(): Promise { - const entry = await getEntry(account) - try { - return await entry.deleteCredential() - } catch (error) { - throw toUnavailableError(error) - } - }, - } -} - -async function getEntry(account: string): Promise { - try { - const { AsyncEntry } = await import('@napi-rs/keyring') - return new AsyncEntry(SERVICE_NAME, account) - } catch (error) { - throw toUnavailableError(error) - } -} - -function toUnavailableError(error: unknown): SecureStoreUnavailableError { - if (error instanceof SecureStoreUnavailableError) { - return error - } - - const message = - error instanceof Error ? error.message : 'System credential storage is unavailable' - return new SecureStoreUnavailableError(message) -} diff --git a/src/lib/user-records.test.ts b/src/lib/user-records.test.ts new file mode 100644 index 0000000..0f47507 --- /dev/null +++ b/src/lib/user-records.test.ts @@ -0,0 +1,147 @@ +/** + * Coverage for the `UserRecordStore` adapter — the on-disk + * config layout cli-core's keyring TokenStore is wired against. The migration + * helper (`migrate-auth.ts`) and every write-side command go through this + * adapter, so the REPLACE-not-merge contract, the legacy-fields strip, and + * the default-pointer cleanup on remove all need direct coverage. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const TEST_HOME = '/tmp/todoist-cli-user-records-tests' +const TEST_CONFIG_PATH = `${TEST_HOME}/.config/todoist-cli/config.json` + +describe('createTodoistUserRecordStore', () => { + let configContent: string | null + + beforeEach(() => { + vi.resetModules() + vi.clearAllMocks() + + configContent = null + + vi.doMock('node:os', () => ({ homedir: () => TEST_HOME })) + vi.doMock('node:fs/promises', () => ({ + chmod: vi.fn().mockResolvedValue(undefined), + mkdir: vi.fn().mockResolvedValue(undefined), + readFile: vi.fn().mockImplementation(async () => { + if (configContent === null) { + const err = new Error('ENOENT') as Error & { code: string } + err.code = 'ENOENT' + throw err + } + return configContent + }), + unlink: vi.fn().mockImplementation(async () => { + configContent = null + }), + writeFile: vi.fn().mockImplementation(async (_path: string, content: string) => { + configContent = content + }), + })) + + vi.doMock('@doist/cli-core', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, getConfigPath: () => TEST_CONFIG_PATH } + }) + }) + + afterEach(() => { + vi.restoreAllMocks() + }) + + function setConfig(config: unknown): void { + configContent = `${JSON.stringify(config, null, 2)}\n` + } + + function readPersisted(): Record | null { + return configContent ? (JSON.parse(configContent) as Record) : null + } + + const ACCOUNT_A = { + id: '111', + email: 'a@example.com', + label: 'a@example.com', + auth_mode: 'read-write' as const, + auth_scope: 'data:read_write', + } + + it('upsert REPLACES the StoredUser — a stale fallbackToken is dropped on a keyring-online write', async () => { + // Pre-existing record with a plaintext fallback (prior offline-fallback write). + setConfig({ + config_version: 2, + users: [{ id: '111', email: 'a@example.com', api_token: 'stale-plaintext-1234567' }], + }) + + const { createTodoistUserRecordStore } = await import('./user-records.js') + await createTodoistUserRecordStore().upsert({ account: ACCOUNT_A }) + + const persisted = readPersisted() as { users: { api_token?: string }[] } + expect(persisted.users[0]).toEqual({ + id: '111', + email: 'a@example.com', + auth_mode: 'read-write', + auth_scope: 'data:read_write', + }) + // `api_token` MUST be gone — leaving it behind would let the runtime + // preferentially serve the stale plaintext over the fresh keyring write. + expect(persisted.users[0].api_token).toBeUndefined() + }) + + it('upsert preserves fallbackToken when supplied (keyring-offline write)', async () => { + const { createTodoistUserRecordStore } = await import('./user-records.js') + await createTodoistUserRecordStore().upsert({ + account: ACCOUNT_A, + fallbackToken: 'plaintext-1234567', + }) + + const persisted = readPersisted() as { users: { api_token?: string }[] } + expect(persisted.users[0].api_token).toBe('plaintext-1234567') + }) + + it('every write strips top-level v1 legacy fields (ensureV2)', async () => { + // Pre-migration config with legacy top-level state still hanging around. + setConfig({ + api_token: 'legacy-1234567', + auth_mode: 'read-write', + auth_scope: 'data:read_write', + auth_flags: ['read-only'], + pendingSecureStoreClear: true, + }) + + const { createTodoistUserRecordStore } = await import('./user-records.js') + await createTodoistUserRecordStore().upsert({ account: ACCOUNT_A }) + + const persisted = readPersisted() as Record + expect(persisted).toEqual({ + config_version: 2, + users: [ + { + id: '111', + email: 'a@example.com', + auth_mode: 'read-write', + auth_scope: 'data:read_write', + }, + ], + }) + }) + + it('remove drops the matching record and clears the default pointer when it matched the removed user', async () => { + setConfig({ + config_version: 2, + user: { defaultUser: '111' }, + users: [ + { id: '111', email: 'a@example.com' }, + { id: '222', email: 'b@example.com' }, + ], + }) + + const { createTodoistUserRecordStore } = await import('./user-records.js') + await createTodoistUserRecordStore().remove('111') + + const persisted = readPersisted() as { users: { id: string }[]; user?: unknown } + expect(persisted.users.map((u) => u.id)).toEqual(['222']) + // Default pointed at the removed user — must be cleared (otherwise the + // resolver would keep targeting an orphan id). + expect(persisted.user).toBeUndefined() + }) +}) diff --git a/src/lib/user-records.ts b/src/lib/user-records.ts new file mode 100644 index 0000000..d3b0b59 --- /dev/null +++ b/src/lib/user-records.ts @@ -0,0 +1,99 @@ +import type { UserRecord, UserRecordStore } from '@doist/cli-core/auth' +import { toTodoistAccount, type TodoistAccount } from './auth-store.js' +import { + type Config, + CONFIG_VERSION, + readConfig, + type StoredUser, + stripLegacyAuthFields, + writeConfig, +} from './config.js' +import { clearDefaultUser, removeStoredUser, setDefaultUser, upsertStoredUser } from './users.js' + +function recordToStoredUser(record: UserRecord): StoredUser { + const stored: StoredUser = { + id: record.account.id, + email: record.account.email, + auth_mode: record.account.auth_mode, + auth_scope: record.account.auth_scope, + auth_flags: record.account.auth_flags, + } + if (record.fallbackToken !== undefined) { + stored.api_token = record.fallbackToken + } + return stored +} + +function storedUserToRecord(user: StoredUser): UserRecord { + const record: UserRecord = { + account: toTodoistAccount({ + id: user.id, + email: user.email, + authMode: user.auth_mode ?? 'unknown', + authScope: user.auth_scope, + authFlags: user.auth_flags, + }), + } + if (user.api_token !== undefined) { + record.fallbackToken = user.api_token + } + return record +} + +/** + * Normalize to the v2 on-disk shape on every write: stamp `config_version`, + * default `users` to `[]`, and strip top-level legacy v1 fields so a stale + * `api_token`/`auth_mode` can't outlive a successful multi-user write. + */ +function ensureV2(config: Config): Config { + const stripped = stripLegacyAuthFields(config) + return { + ...stripped, + config_version: CONFIG_VERSION, + users: Array.isArray(stripped.users) ? stripped.users : [], + } +} + +/** + * `UserRecordStore` backed by the existing JSON config file. + * + * REPLACE, do not merge: `upsert` rebuilds the `StoredUser` entirely from the + * incoming record so a stale `api_token` from a prior offline-fallback write + * cannot outlive a later keyring-online write (cli-core preferentially reads + * `fallbackToken` over the keyring). + * + * Array manipulation (upsert / remove / default-pointer maintenance) + * delegates to the same helpers `users.ts` exposes to the rest of the CLI + * (`upsertStoredUser`, `removeStoredUser`, `setDefaultUser`, + * `clearDefaultUser`) so the on-disk config layout has one set of mutators. + */ +export function createTodoistUserRecordStore(): UserRecordStore { + return { + async list() { + const config = await readConfig() + const users = Array.isArray(config.users) ? config.users : [] + return users.map(storedUserToRecord) + }, + + async upsert(record) { + const base = ensureV2(await readConfig()) + const next = upsertStoredUser(base, recordToStoredUser(record)).config + await writeConfig(next) + }, + + async remove(id) { + const base = ensureV2(await readConfig()) + await writeConfig(removeStoredUser(base, id)) + }, + + async getDefaultId() { + const config = await readConfig() + return config.user?.defaultUser ?? null + }, + + async setDefaultId(id) { + const base = ensureV2(await readConfig()) + await writeConfig(id === null ? clearDefaultUser(base) : setDefaultUser(base, id)) + }, + } +} diff --git a/src/lib/users.ts b/src/lib/users.ts index c2b27dc..ca7da79 100644 --- a/src/lib/users.ts +++ b/src/lib/users.ts @@ -1,19 +1,8 @@ import type { Config, StoredUser } from './config.js' import { CliError } from './errors.js' -/** - * Reference shape accepted by `--user` and `td user` subcommands: an exact - * Todoist user id, or a Todoist account email (case-insensitive). - */ -export type UserRef = string - -export interface FindUserResult { - user: StoredUser - index: number -} - export class UserNotFoundError extends CliError { - constructor(ref: UserRef) { + constructor(ref: string) { super( 'USER_NOT_FOUND', `No stored user matches "${ref}". Use \`td user list\` to see authenticated accounts.`, @@ -43,56 +32,48 @@ export function getStoredUsers(config: Config): StoredUser[] { return Array.isArray(config.users) ? config.users : [] } -export function findUserByRef(config: Config, ref: UserRef): FindUserResult | null { - const users = getStoredUsers(config) - const trimmed = ref.trim() - if (!trimmed) return null - - // Exact id match first (ids are numeric strings; case-sensitive) - const byId = users.findIndex((u) => u.id === trimmed) - if (byId !== -1) return { user: users[byId], index: byId } - - // Email match — case-insensitive - const lower = trimmed.toLowerCase() - const byEmail = users.findIndex((u) => u.email.toLowerCase() === lower) - if (byEmail !== -1) return { user: users[byEmail], index: byEmail } - - return null -} - -export function requireUserByRef(config: Config, ref: UserRef): FindUserResult { - const found = findUserByRef(config, ref) - if (!found) throw new UserNotFoundError(ref) - return found -} - export function getDefaultUserId(config: Config): string | undefined { return config.user?.defaultUser } -export function getDefaultUser(config: Config): StoredUser | null { - const id = getDefaultUserId(config) - if (!id) return null - return getStoredUsers(config).find((u) => u.id === id) ?? null +/** + * Single source of truth for `--user ` matching: exact id, or + * case-insensitive email. Both `findUserByRef` (config-driven path) and + * `auth-store.ts`'s cli-core `matchAccount` (keyring-store path) delegate + * here so the two routes can't drift. + */ +export function matchUserRef(user: { id: string; email: string }, ref: string): boolean { + const trimmed = ref.trim() + if (!trimmed) return false + if (user.id === trimmed) return true + return user.email.toLowerCase() === trimmed.toLowerCase() } /** - * The user that should be treated as the "default" when no `--user` ref is - * supplied: the explicitly-set default, or the sole stored user when only - * one exists. Returns `null` when no default can be inferred (empty store, - * or multiple users with no explicit default). + * Resolve a user ref against the on-disk config. Returns `null` on miss; + * the call sites that need to fail loudly wrap with `requireUserByRef`. */ -export function getEffectiveDefaultUser(config: Config): StoredUser | null { - const explicit = getDefaultUser(config) - if (explicit) return explicit +export function findUserByRef( + config: Config, + ref: string, +): { user: StoredUser; index: number } | null { const users = getStoredUsers(config) - return users.length === 1 ? users[0] : null + const idx = users.findIndex((u) => matchUserRef(u, ref)) + return idx !== -1 ? { user: users[idx], index: idx } : null } -/** - * Replace or append a user record. Returns a new config and whether the user - * was already present (so callers can show "replaced" vs "added" messages). - */ +export function requireUserByRef(config: Config, ref: string): { user: StoredUser; index: number } { + const found = findUserByRef(config, ref) + if (!found) throw new UserNotFoundError(ref) + return found +} + +// --------------------------------------------------------------------------- +// Pure mutators — driven by the `UserRecordStore` adapter in `user-records.ts`. +// Kept here (not inlined) so the on-disk config layout has one set of array + +// default-pointer manipulators across the codebase. +// --------------------------------------------------------------------------- + export function upsertStoredUser( config: Config, next: StoredUser, @@ -100,28 +81,26 @@ export function upsertStoredUser( const users = getStoredUsers(config).slice() const idx = users.findIndex((u) => u.id === next.id) const replaced = idx !== -1 - if (replaced) { - users[idx] = next - } else { - users.push(next) - } + if (replaced) users[idx] = next + else users.push(next) return { config: { ...config, users }, replaced } } export function removeStoredUser(config: Config, id: string): Config { const users = getStoredUsers(config).filter((u) => u.id !== id) const next: Config = { ...config, users } - if (next.user?.defaultUser === id) { - const { defaultUser: _, ...restUser } = next.user - next.user = Object.keys(restUser).length === 0 ? undefined : restUser - if (next.user === undefined) { - const { user: _u, ...rest } = next - return rest - } - } + if (next.user?.defaultUser === id) return clearDefaultUser(next) return next } export function setDefaultUser(config: Config, id: string): Config { return { ...config, user: { ...config.user, defaultUser: id } } } + +export function clearDefaultUser(config: Config): Config { + if (!config.user) return config + const { defaultUser: _d, ...restUser } = config.user + if (Object.keys(restUser).length > 0) return { ...config, user: restUser } + const { user: _u, ...rest } = config + return rest +}