feat(auth): add pluggable OAuth + token-storage runtime to /auth subpath#11
feat(auth): add pluggable OAuth + token-storage runtime to /auth subpath#11scottlovegrove wants to merge 4 commits into
Conversation
…uthCommand Adds a `./auth` subpath that exposes the full auth machinery the three Doist CLIs currently re-implement: PKCE primitives, a port-fallback callback server, three built-in `AuthProvider` factories (`createPkceProvider`, `createDcrProvider`, `createTokenPasteProvider`) plus the bare interface for bespoke methods, two `TokenStore` factories (`createConfigTokenStore` with a single-/multi-user shape switch and a CLI-supplied migration hook; `createKeyringTokenStore` lazy-loading `@napi-rs/keyring` with config fallback), a `runOAuthFlow` orchestrator, and `registerAuthCommand` mirroring the `registerUpdateCommand` registrar pattern. `@napi-rs/keyring` and `open` are declared as optional peer-deps so consumers that don't need them pay nothing. New `AUTH_*` error codes are folded into `CliErrorCode`. README updated with an `auth` row and usage block. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
doistbot
left a comment
doistbot
left a comment
There was a problem hiding this comment.
This PR introduces a robust, pluggable OAuth and token-storage runtime to the /auth subpath, complete with multi-user support and flexible provider architectures. The hybrid provider approach is a great design choice that cleanly covers our existing CLI needs while keeping future authentication flows decoupled from core releases. There are a few areas that need refinement, including resolving potential server hangs during the callback flow, addressing a security risk by reading tokens securely rather than through CLI arguments, optimizing redundant store mutations, and aligning test files with the repository's colocation rules.
Address 19 review comments from doistbot review on PR #11. P1: - callback-server: settle outcomePromise on stop() so AbortSignal-cancelled waitForCallback no longer hangs; call closeAllConnections() to flush browser keepalive sockets that would otherwise block CLI exit. - callback-server / register: validate --callback-port and the matching env override; reject non-integer / out-of-range values with AUTH_PORT_BIND_FAILED before they reach server.listen(). - commands/token: read pasted tokens from piped stdin instead of argv per Doist secrets-management standard. Drop the `login --token <value>` flag entirely and the positional `token set <value>`. `token set` now reads stdin (errors with hint if stdin is a TTY). P2: - TokenStore.set takes `{ setActive }` so config-backed multi-user stores collapse the upsert + activate into a single updateConfig cycle. - flow: merge prepareHandshake into the handshake passed to exchangeCode and validateToken so prepare-time state survives even when a third-party authorize() forgets to forward it. - status: --user bypasses the env override so an operator can inspect a specific stored account; the three independent store reads now run in Promise.all. - logout: skip the redundant store.list() call when --user isn't given. - LoginFlagSpec: drop the explicit `key` field — values land at the same Commander-derived camelCase property name as every other flag. - register: drop no-op `--user` from login; remove redundant mapLoginFlags (Commander already populates cmdOptions); CLI flag now wins over env override; extract addViewOptions for the repeated --json/--ndjson declarations. - handlers: extract shared emitView + persistPastedToken + readTokenFromStdin helpers so the four subcommand modules stop duplicating the json/ndjson/ human emitter and the accept/store/setActive sequence. - errors: prune AUTH_TOKEN_VALIDATION_FAILED, AUTH_STORE_UNAVAILABLE, AUTH_BROWSER_OPEN_FAILED — no callers in current code (per AGENTS.md no-dead-exports rule). Tests: - Split src/auth/commands/handlers.test.ts into login/logout/status/token test files per AGENTS.md colocation rule. - flow.test: drop the onAuthorizeUrl/hardcoded-port hack; capture the runtime redirectUri inside the wrapped provider's authorize() and drive the callback from openBrowser instead. - register.test: assert --additional-scopes value reaches resolveScopes via flags.additionalScopes (not just that Commander registered the flag); add CLI-flag-beats-env-override coverage; cover stdin-piped token set and TTY rejection. - callback-server.test: 404 test now starts waitForCallback first and asserts it stays pending; new tests for stop()-while-waiting and invalid preferredPort. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@doistbot /review |
doistbot
left a comment
doistbot
left a comment
There was a problem hiding this comment.
This PR introduces a robust, pluggable OAuth runtime and generic multi-user token storage system to the CLI core, offering flexible capabilities without locking consumers into a single workflow. The hybrid architecture thoughtfully centralizes complex orchestration while leaving adequate escape hatches for bespoke authentication methods. There are a few adjustments to make regarding input validation and execution order for port configuration, consolidating duplicated test setups, refining the handling of environment overrides and missing users, and ensuring secure documentation practices.
| // the OAuth provider's redirect lands on the same address family the | ||
| // server is listening on (avoids IPv6 ::1 vs IPv4 127.0.0.1 mismatches | ||
| // when `localhost` resolves to both). | ||
| const redirectUri = `http://${host}:${port}${path}` |
There was a problem hiding this comment.
[P2] If a consumer configures the bind host to be an IPv6 address (e.g. ::1), the resulting URL string http://::1:8080/callback will be invalid and cause downstream new URL() calls or browser launches to fail. Consider wrapping the host in brackets if it contains a colon: const hostForUrl = host.includes(':') ? '[' + host + ']' : host;
|
|
||
| loginCommand.action(async (cmdOptions: LoginCmdOptions) => { | ||
| // Precedence: explicit CLI flag > env override > registered preferred port. | ||
| const port = cmdOptions.callbackPort ?? portEnvOverride ?? options.callbackPort.preferred |
There was a problem hiding this comment.
[P2] The handler explicitly looks up cmdOptions.callbackPort. If a consumer overrides options.callbackPort.flagSpec with a different long flag name (e.g. --oauth-port <port>), Commander will map it to a different property (e.g. cmdOptions.oauthPort). This causes the CLI flag to be silently ignored here and fall back to the env var or preferred port, while also leaking the renamed flag into the flags bag via stripReservedFlags. Consider deriving the property key dynamically from portFlagSpec or enforcing that it retains callback-port as the long name.
| if (envOverridesActive) { | ||
| return [`${chalk.green('✓')} Using ${chalk.cyan(options.envTokenVar)} (env override)`] | ||
| } | ||
| if (!activeRecord) { |
There was a problem hiding this comment.
[P3] When an explicit --user <id> is provided but that account does not exist in the store, activeRecord will be nullish. The command will report 'Not signed in. Run login to authenticate.', which can be misleading since the operator might be signed in with other accounts. Consider handling the missing-user case explicitly (e.g. 'User not found'), similar to how runLogout handles it.
| ): void { | ||
| const envTokenVar = options.envTokenVar ?? `${options.appName.toUpperCase()}_API_TOKEN` | ||
| const portFlagSpec = options.callbackPort.flagSpec ?? '--callback-port <port>' | ||
| const portEnvOverride = resolveEnvPort(options.callbackPort.envVar) |
There was a problem hiding this comment.
[P2] resolveEnvPort() now runs during command registration, so a malformed callback-port env var breaks unrelated commands like status, logout, or token before Commander even parses the subcommand. Defer this lookup into the login action (or runLogin) so only the OAuth path validates the callback-port override.
| } | ||
|
|
||
| function parsePortFlag(value: string): number { | ||
| const parsed = Number.parseInt(value, 10) |
There was a problem hiding this comment.
[P2] Number.parseInt still accepts non-integer inputs like 123abc and 1.5, so both --callback-port and the env override can slip through as 123 / 1. Use a full-string integer check (Number(...) plus validation, or a /^\d+$/ guard) before the range test in both helpers.
| handle = await startCallbackServer(baseOptions()) | ||
| // Start the wait first so we can prove the 404 doesn't settle it. | ||
| let settled = false | ||
| const waiting = handle.waitForCallback(150).then( |
There was a problem hiding this comment.
[P2] This pending-state check depends on a 150ms real-time timeout that starts before the fetch(). On a busy CI runner, waitForCallback(150) can legitimately time out before the 404 round-trip completes, which makes settled flip to true even when /other behaves correctly. Please switch this to fake timers or another deterministic pending-assertion pattern so the test doesn't rely on a short wall-clock window.
| const { provider, getRedirect } = instrument({ prepare, exchangeCode, validateToken }) | ||
| const store = createConfigTokenStore<Account>({ configPath: path, multiUser: false }) | ||
|
|
||
| const openBrowser = vi.fn(async (url: string) => { |
There was a problem hiding this comment.
[P2] Every flow test injects openBrowser, so none of them exercise the new lazy-open / onAuthorizeUrl fallback path that this PR adds. A regression in loadDefaultOpener() or the catch/null branches of openOrFallback() would leave users without an authorize URL and this suite would still stay green. Please add a case that omits openBrowser and asserts the URL is surfaced when the opener is unavailable or throws.
| } | ||
| }) | ||
|
|
||
| it('--callback-port rejects non-integer / out-of-range values with AUTH_PORT_BIND_FAILED', async () => { |
There was a problem hiding this comment.
[P2] The validation coverage here only goes through Commander's --callback-port parser. It still doesn't cover the callbackPort.envVar path in resolveEnvPort(), which fails earlier during registerAuthCommand() and is a different code path. Add a case with TEST_CB_PORT=foo/70000 and assert build() (or registerAuthCommand()) rejects with AUTH_PORT_BIND_FAILED.
| }) | ||
|
|
||
| describe('runTokenSet', () => { | ||
| it('reads piped token, validates via provider, persists as active in one mutation', async () => { |
There was a problem hiding this comment.
[P2] This test title says it proves the new one-mutation behavior, but the assertion only checks final persisted state. That would still pass if runTokenSet() regressed to a two-step set() + setActive() write, because the end state is identical. To lock in the optimization, assert the interaction instead (for example with a fake store that records set(..., { setActive: true }) and whether setActive() was called), or count config writes in a lower-level store test.
| if (process.stdin.isTTY) { | ||
| throw new CliError('AUTH_INVALID_TOKEN', 'Token must be piped via stdin.', { | ||
| hints: [ | ||
| 'Pipe the token: `echo $YOUR_TOKEN | <cli> token set`', |
There was a problem hiding this comment.
[P2] The hint echo $YOUR_TOKEN | <cli> token set risks users substituting their literal token (e.g., echo my-secret | <cli> token set). This passes the secret as a command-line argument to echo, exposing it in their shell history and potentially the process list. This conflicts with the Secrets Management standard prohibiting secrets via command-line arguments (https://handbook.doist.com/doc/standard-secrets-management-wbKmIfrtgr). Consider suggesting a safer source that avoids the command line entirely, such as cat token.txt | <cli> token set or a clipboard tool like pbpaste | <cli> token set.
- Drop providers/token-paste.test.ts — the factory is a 5-line wrapper
around `validate`; covered indirectly via runTokenSet's smoke tests.
- Slim handler tests (login/logout/status/token.test.ts) to smoke level;
the Commander integration in register.test.ts already exercises the
same paths end-to-end.
- Parametrize store/config.test.ts via `describe.each([{ multiUser: true },
{ multiUser: false }])` so single- and multi-user variants of the
identical operations (set/get/active, delete/clear, backend) share one
block instead of two.
- Drop the migration "skips when target shape already present" case —
exercised implicitly by every other test that calls `set` first.
- Merge the three callback-server "rejects on bad callback" tests into a
single `it.each` over (state mismatch, error param, missing code).
272 → 260 tests across 25 → 24 files. No coverage loss; redundant
duplication removed.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This PR was 4500 LoC across the full pluggable abstraction (DCR, keyring, multi-user, four subcommand handlers). For the first cli-core auth release, ship only the surface a real CLI is about to consume — outline-cli is single-user, public-client PKCE, plain-config storage, and only needs the `login` flow extracted today. Defer to follow-up PRs: - `createDcrProvider` (twist-cli's dynamic client registration flow): not needed until twist migrates. The `AuthProvider` interface stays so twist can inline a DCR provider locally in the meantime. - `createKeyringTokenStore` + `@napi-rs/keyring` peer-dep: todoist + twist keep their existing keyring code locally for v1; the `TokenStore` interface is the contract once a keyring backend is worth sharing. - Multi-user store mode + `StoreMigration`: outline is single-user. Multi- user lands when twist or todoist migrates. The single-user store now has only `active` / `set` / `clear` / `backend`. - `createTokenPasteProvider` and `acceptPastedToken` on the provider: the one consumer (`token set`) is gone, so the optional method is too. - `runLogout` / `runStatus` / `runTokenSet` / `runTokenView` and their Commander wiring: short and CLI-specific in shape today; each CLI keeps its own implementations until a concrete migration proves them shared. Surface that remains: - `runOAuthFlow`, `startCallbackServer`, PKCE primitives — the genuinely reusable runtime. - `createPkceProvider` — public-client PKCE, covers outline + todoist. - `createConfigTokenStore` — single-user, plain-config. - `registerAuthCommand` — registers `login` only. - `AuthProvider` / `TokenStore` interfaces — the escape hatches for the deferred features. Net: 4568 → ~2200 LoC (this commit removes 2332 lines, adds 358). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Closing in favour of a fresh PR with squashed history — review-ping-pong commits made the diff hard to read. Re-opened as a single-commit PR. |
2 participants
Summary
@doist/cli-core/auth— full pluggable auth runtime: PKCE primitives, port-fallback callback server, three built-inAuthProviderfactories (createPkceProvider,createDcrProvider,createTokenPasteProvider), twoTokenStorefactories (createConfigTokenStore,createKeyringTokenStore),runOAuthFloworchestrator, andregisterAuthCommandmirroringregisterUpdateCommand.AuthProvider) is the escape hatch for bespoke methods (device code, magic link, username/password) — new auth flows do not require a cli-core release.TokenStore<TAccount extends AuthAccount>is generic over account shape with single-/multi-user backends; CLI-suppliedmigratehook keeps v1→v2 transitions out of core.@napi-rs/keyringandopendeclared as optional peer-deps; lazy-imported and silently fall back when unavailable.AUTH_*codes folded intoCliErrorCode.Approach decisions
users[]and twist'saccounts[](scottl/multi-userbranch) converge on the same shape, validated against twist's WIP branch before landing../authsubpath — registrar and runtime ship together; commander stays an optional peer-dep, only loaded whenregisterAuthCommandis referenced.Plan file: `~/.claude/plans/i-want-us-to-steady-pascal.md` (local).
Test plan
🤖 Generated with Claude Code