feat(auth): add keyring-backed multi-account TokenStore by scottlovegrove · Pull Request #24 · Doist/cli-core

scottlovegrove · 2026-05-16T09:55:03Z

Summary

Ship a reusable secure-token implementation so todoist-cli and twist-cli can drop their duplicate secure-store.ts wrappers and converge on the same multi-account model.
Wraps @napi-rs/keyring for cross-platform secret storage (Keychain / Credential Manager / libsecret) and falls back to a plaintext token on the consumer's user record when the keyring is unreachable (WSL without D-Bus, headless Linux, containers, CI).
Exposed under @doist/cli-core/auth and the root barrel.

What's new under `src/auth/keyring/`

createSecureStore({ serviceName, account }) + SecureStoreUnavailableError — keyring primitive. Dynamic import of @napi-rs/keyring so a missing native binary surfaces as SecureStoreUnavailableError rather than crashing module load.
createKeyringTokenStore({ serviceName, userRecords, accountForUser?, matchAccount? }) — full multi-account TokenStore (including list / setDefault and --user <ref> matching from feat(auth): multi-user TokenStore contract #23) that composes the primitive with a consumer-supplied UserRecordStore port over their config file. Exposes getLastStorageResult() / getLastClearResult() so login/logout commands can surface keyring-fallback warnings on stderr.
migrateLegacyAuth({ … }) — generic v1→v2 helper for postinstall hooks. Reads the legacy keyring slot first, then the consumer's plaintext slot; identifies the user via a consumer-supplied callback; writes the v2 record + sets default; best-effort cleans up the legacy slot.

Cross-platform behaviour

Platform	Backend (via `@napi-rs/keyring`)	Fallback
macOS	Keychain	`fallbackToken` on user record
Windows	Credential Manager	`fallbackToken` on user record
Linux (desktop)	libsecret / Secret Service	`fallbackToken` on user record
WSL / headless Linux / containers / CI	typically unavailable	`fallbackToken` on user record + visible warning

@napi-rs/keyring is declared as an optionalDependency so install survives architectures without a prebuilt binary.

API contract changes

None. TokenStore is unchanged from main; this just ships a default implementation of it. Consumers stay free to roll their own.

Test plan

npm run type-check, npm run check, npm test — green locally (336 tests).
Manual smoke: npm link into a todoist-cli branch, swap auth-store.ts to createKeyringTokenStore, run td auth login on Linux desktop (libsecret available) and in a WSL session without D-Bus to confirm the fallback path emits the warning + writes fallbackToken to the config record.
Manual smoke on macOS Keychain + Windows Credential Manager before publishing to npm.
Follow-up PRs in todoist-cli and twist-cli to migrate to the new factory and delete their duplicate secure-store.ts / migrate-auth.ts.

🤖 Generated with Claude Code

Ship a reusable secure-token implementation so todoist-cli and twist-cli can drop their duplicate `secure-store.ts` wrappers and converge on the same multi-account model. Wraps `@napi-rs/keyring` for cross-platform secret storage (Keychain / Credential Manager / libsecret), and falls back to a plaintext token on the consumer's user record when the keyring is unreachable (WSL without D-Bus, headless Linux, containers, CI). New under `src/auth/keyring/`: - `createSecureStore({ serviceName, account })` + `SecureStoreUnavailableError` — keyring primitive. - `createKeyringTokenStore({ serviceName, userRecords, … })` — full multi-account `TokenStore` (including `list` / `setDefault` and `--user <ref>` matching) that composes the primitive with a consumer-supplied `UserRecordStore` port over their config file. - `migrateLegacyAuth({ … })` — generic v1→v2 helper for postinstall hooks. `@napi-rs/keyring` is declared as an optional dependency and dynamic-imported so a missing native binary surfaces as `SecureStoreUnavailableError` and triggers the plaintext fallback instead of crashing module load. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

doistbot

This PR introduces a reusable, cross-platform multi-account TokenStore backed by @napi-rs/keyring, providing an excellent path to unify the auth model across our CLIs. The implementation lays a solid foundation for secure token management with a thoughtful plaintext fallback for headless and continuous integration environments. There are a few areas to refine regarding state management, including addressing edge cases in fallback cleanup and error masking, optimizing repeated file I/O during default resolution, preventing potential PII leaks in migration logs, and expanding test coverage for offline scenarios.

_{Share Feedback • Review Logs}

P2: - `clear()` always attempts the keyring delete, even when the record carries a `fallbackToken`, so an orphan secret from an earlier online write can't survive. - `active()` throws `AUTH_STORE_READ_FAILED` when a matching record exists but the keyring can't be read, instead of returning `null` (which would otherwise translate to a misleading `ACCOUNT_NOT_FOUND` on `--user <ref>`). - `set()` makes the default-user promotion best-effort so a `setDefaultId` failure doesn't undo a successful credential write via `AUTH_STORE_WRITE_FAILED`. - `active()` and `clear()` resolve the default record from a single `list`+`getDefaultId` snapshot (was 2–3 sequential reads). - `DEFAULT_ACCOUNT_FOR_USER` lives in `secure-store.ts` and is shared by `createKeyringTokenStore` and `migrateLegacyAuth` so a future rename can't park tokens in a slot the runtime no longer reads. - `UserRecordStore.upsert` is now documented as replace-not-merge so a consumer's merge impl can't strand a stale `fallbackToken` on top of a later keyring-backed write. - `migrateLegacyAuth` logs only `account.id` (was `label`, which is typically an email — PII on stderr). - Multi-account tests use a keyed `createSecureStore` mock so per-user slot routing is actually exercised; offline-keyring migration case added. - `vi.doUnmock` moved into `afterEach` so the mock is torn down even when an assertion fails earlier in the block. P3: - `migrateLegacyAuth` uses the shared `getErrorMessage` util. - `setDefault` and `requireSnapshotForRef` go through a shared `accountNotFoundError(ref)` helper. - README clarifies that `@napi-rs/keyring` arrives transitively via cli-core's own `optionalDependencies`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

scottlovegrove · 2026-05-16T11:34:28Z

@doistbot /review

doistbot

This PR introduces a robust, keyring-backed multi-account TokenStore for CLI core, wrapping @napi-rs/keyring with smart fallbacks for headless environments and providing a solid migration path from legacy auth. The implementation effectively centralizes our cross-platform secure storage, though there are a few opportunities to refine error handling, privacy, and internal consistency. A few adjustments are needed to ensure the logout flow reliably cleans up records when the keyring is offline, align default account resolution across commands, extract duplicated test and storage logic, and ensure error logging avoids echoing potentially sensitive user data.

_{Share Feedback • Review Logs}

P2: - `clear()` wraps `setDefaultId(null)` in best-effort try/catch so a default-pointer write failure can't skip the keyring delete that follows and leave an unreachable orphan secret. - `attachLogoutCommand` catches `AUTH_STORE_READ_FAILED` from the snapshot pre-flight and proceeds with `clear(ref)` — `logout --user <ref>` can now clear a secure-only record even when the keyring is offline. Other typed errors (notably `ACCOUNT_NOT_FOUND`) still propagate. - `list()` resolves the implicit single-record default via `resolveDefault(snapshot)` so its `isDefault` markers match `active()`'s behaviour (single record with no pinned default → `isDefault: true`). - `readSnapshot()` skips `getDefaultId()` when an explicit ref is supplied, halving the config-file reads on the `--user <ref>` hot path. - `UserRecordStore.getById` removed: unused after the snapshot refactor; the README example and consumer surface are simpler. - `writeRecordWithKeyringFallback` in `record-write.ts` is the single source of truth for the keyring-write → fallback → upsert → rollback choreography shared by `createKeyringTokenStore.set` and `migrateLegacyAuth`. - `migrateLegacyAuth` stderr emits a fixed phrase keyed off a `SkipReason` enum so consumer-supplied error text (which may contain emails, paths, or auth diagnostics) can't leak into logs. The raw detail is still attached to `MigrateAuthResult.reason` for in-process callers. - `src/test-support/keyring-mocks.ts` is the shared `buildKeyringMap` / `buildSingleSlot` / `buildUserRecords` used by both keyring test files (excluded from build by `tsconfig.build.json`). - New tests cover: `clear()` keyring delete when `setDefaultId(null)` throws, `set()` survives `setDefaultId` failure, `active()` keyring read failure surfaces `AUTH_STORE_READ_FAILED`, `list()` matches `active()`'s implicit default, attacher-level end-to-end coverage for `AUTH_STORE_READ_FAILED` propagation in status/logout/token-view, and a non-silent migrate log that asserts only `account.id` (no `label`/email) and a generic skip phrase. P3: - `UserRecord.fallbackToken` doc now matches the runtime read order (`fallbackToken` first, then keyring). - `clear()` hoists the fallback `TokenStorageResult` to a single `const`, removing the duplicate object literal across the try/catch. - `DEFAULT_ACCOUNT_FOR_USER` no longer exported from the keyring barrel (it's already accessible via direct import from `secure-store`; dead surface from `src/auth/index.ts`). - Already-migrated, list/setDefault, and "no records" tests drop the unnecessary `createSecureStore` mock and assert that the keyring is never reached. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Trims 11 tests that asserted trivial empty-state pass-throughs, duplicated code paths via different inputs, or locked in implementation details rather than behaviour: - secure-store.test.ts: drop the `?? null` empty-slot test and the set/delete variants of the error-wrapping test (all three methods share the same wrap helper, so the get test covers them). - token-store.test.ts: drop the "no record" trio (`active`/`clear`/`list` empty cases), the implementation-detail test asserting `getDefaultId` is skipped on the `--user` path, the label-matching test (already exercised via `setDefault('b')`), and the trivial `clear(ref)` miss no-op. - migrate.test.ts: drop the upsert-rollback test — the rollback now lives in `writeRecordWithKeyringFallback` and is covered by the equivalent token-store test. - token-view.test.ts: drop the `AUTH_STORE_READ_FAILED` propagation test — status.test.ts covers the same code path (both go through `requireSnapshotForRef` with no local catch). Net: 346 → 336 tests passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

scottlovegrove · 2026-05-16T12:20:29Z

Closing in favour of a 4-PR stack that splits this ~1900 LoC change into independently reviewable pieces:

feat(auth): add createSecureStore keyring primitive (1/4) #25 — keyring primitive (createSecureStore)
feat(auth): tolerate AUTH_STORE_READ_FAILED in logout --user (2/4) #26 — AUTH_STORE_READ_FAILED + logout --user tolerance
feat(auth): add createKeyringTokenStore multi-account TokenStore (3/4) #27 — multi-account createKeyringTokenStore + UserRecordStore port + shared write helper
feat(auth): add migrateLegacyAuth helper (4/4) #28 — migrateLegacyAuth v1→v2 postinstall helper

All 14 + 15 review comments from the original two passes are already addressed in the stack.

scottlovegrove self-assigned this May 16, 2026

doistbot reviewed May 16, 2026

View reviewed changes

scottlovegrove and others added 2 commits May 16, 2026 12:53

scottlovegrove closed this May 16, 2026

scottlovegrove deleted the feat/keyring-token-store branch May 16, 2026 16:11

Conversation

scottlovegrove commented May 16, 2026

Summary

What's new under src/auth/keyring/

Cross-platform behaviour

API contract changes

Test plan

Uh oh!

doistbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scottlovegrove commented May 16, 2026

Uh oh!

doistbot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

scottlovegrove commented May 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

What's new under `src/auth/keyring/`