Skip to content

feat(auth): adopt cli-core createDcrProvider for OAuth#243

Merged
scottlovegrove merged 3 commits into
mainfrom
scottl/dcr-provider
May 21, 2026
Merged

feat(auth): adopt cli-core createDcrProvider for OAuth#243
scottlovegrove merged 3 commits into
mainfrom
scottl/dcr-provider

Conversation

@scottlovegrove
Copy link
Copy Markdown
Collaborator

Summary

  • Replaces the bespoke ~170-LOC DCR AuthProvider (hand-rolled registration, PKCE authorize, HTTP Basic token exchange) with cli-core's createDcrProvider.
  • Bumps @doist/cli-core 0.16.1 → 0.20.0 and adds oauth4webapi@3.8.6 as a direct dependency (cli-core's optional peer dep, required by the DCR flow).
  • validate now derives authMode / authScope from the runOAuthFlow-folded handshake.readOnly instead of values stashed during a hand-rolled authorize — the scope set is a pure function of readOnly (see resolveScopes in login.ts).
  • Net −221 LOC in auth-provider.ts + tests.

Why client_secret_post (not the default client_secret_basic)

Twist client_ids contain _ (e.g. twd_…). oauth4webapi's client_secret_basic form-url-encodes the HTTP Basic credential per RFC 6749 §2.3.1, turning _ into %5F. Twist's token endpoint doesn't url-decode the credential, so it looks up twd%5F… and returns 404 "Integration with client_id not found". client_secret_post sends the credential in the request body via URLSearchParams, which preserves the underscore, so the lookup succeeds. (The old bespoke code worked because it sent the credential raw; Twist accepts raw.)

Test plan

  • npm run type-check — clean
  • npm test — 645 pass
  • npm run lint — clean
  • npm run build — clean
  • Live tw auth login against twist.com — full DCR registration + PKCE + token exchange + persist, verified against published cli-core 0.20.0

🤖 Generated with Claude Code

Replace the bespoke ~170-LOC DCR AuthProvider (hand-rolled registration,
PKCE authorize, HTTP Basic token exchange) with cli-core's createDcrProvider.
Bump @doist/cli-core to 0.20.0 and add oauth4webapi as a direct dependency
(cli-core's optional peer dep, required by the DCR flow).

Use client_secret_post rather than the default client_secret_basic: Twist
client_ids contain `_` (`twd_…`), and oauth4webapi form-url-encodes the Basic
credential per RFC 6749 §2.3.1 (`_` -> `%5F`), which Twist's token endpoint
does not url-decode -> "client_id not found". client_secret_post sends the
credential in the body (URLSearchParams), which preserves the underscore.

validate now derives authMode/authScope from the runOAuthFlow-folded
handshake.readOnly, since the scope set is a pure function of it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@scottlovegrove scottlovegrove self-assigned this May 21, 2026
@doistbot doistbot requested a review from rfgamaral May 21, 2026 16:57
Copy link
Copy Markdown
Member

@doistbot doistbot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This update successfully streamlines the OAuth flow by replacing the custom DCR provider with @doist/cli-core's implementation, significantly reducing boilerplate and improving maintainability. A few edge cases require attention before merging, particularly ensuring the token store's activeBundle() method mirrors the active() logic to prevent incorrect status reporting for environment tokens and pending migrations. Additional refinements include extracting the duplicated scope resolution logic into a shared helper to prevent drift, adding stricter validation for the untyped handshake.readOnly flag to protect local access guards, and preserving a dedicated unit test for the critical client_secret_post metadata override.

Share FeedbackReview Logs

Comment thread src/commands/auth/auth.test.ts
Comment thread src/lib/auth-provider.ts Outdated
Comment thread src/lib/auth-provider.ts Outdated
Comment thread src/lib/auth-provider.test.ts
…guard readOnly

- createTwistTokenStore now overrides activeBundle() with the same env-token +
  legacy-snapshot resolution as active() (shared resolveOverrideSnapshot), so
  cli-core's activeBundle-based auth commands (e.g. `tw auth status`) don't
  mis-report env-token / migration-pending users as signed out.
- Extract scopesForReadOnly() and reuse it in login.ts resolveScopes and the
  provider's validate, so requested and recorded scopes can't drift.
- validate fails closed (AUTH_FAILED) on a non-boolean handshake.readOnly
  instead of silently defaulting to read-write (which would relax the local
  write guard).
- Tests: assert client_secret_post is passed to createDcrProvider; cover
  activeBundle env-token + delegate paths and the readOnly guard.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread src/lib/auth-provider.ts Outdated
The old name implied it returned read-only scopes, but it takes a readOnly
boolean and returns either set. getScopes reads correctly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@scottlovegrove scottlovegrove added the 👀 Show PR PR must be reviewed before or after merging label May 21, 2026
@scottlovegrove scottlovegrove merged commit 670d6df into main May 21, 2026
5 checks passed
@scottlovegrove scottlovegrove deleted the scottl/dcr-provider branch May 21, 2026 17:27
doist-release-bot Bot added a commit that referenced this pull request May 21, 2026
## [2.42.0](v2.41.2...v2.42.0) (2026-05-21)

### Features

* **auth:** adopt cli-core createDcrProvider for OAuth ([#243](#243)) ([670d6df](670d6df))
@doist-release-bot
Copy link
Copy Markdown
Contributor

🎉 This PR is included in version 2.42.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link
Copy Markdown
Member

@rfgamaral rfgamaral left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

released 👀 Show PR PR must be reviewed before or after merging

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants