feat(auth): add resource indicator, refresh, and client caching to DCR provider#60
Conversation
…R provider Extend createDcrProvider so RFC 7591 DCR consumers can cover three cases the factory previously forced them to hand-roll a bespoke AuthProvider for: - RFC 8707 resource indicator: new `resource` option, threaded into the authorize URL and the authorization_code + refresh_token token requests so the AS issues a token whose audience targets the protected resource. - refreshToken: the DCR provider now implements it (previously only createPkceProvider did), honouring the handshake client auth (public None / client_secret_post / RFC-3986 Basic), the resource indicator, and a 10s timeout, mapping invalid_grant -> AUTH_REFRESH_EXPIRED else AUTH_REFRESH_TRANSIENT. - client caching: optional loadClient/saveClient hooks let a consumer reuse a registered client across logins (cache hit skips the registration round-trip and the oauth4webapi load); cli-core stays storage-agnostic. Factor the shared refresh-error mapping into oauth.ts (mapRefreshError) and reuse it from createPkceProvider, and add an optional additionalParameters field to buildPkceAuthorizeUrl. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
doistbot
left a comment
There was a problem hiding this comment.
Thanks Scott for expanding the DCR provider with resource indicators, refresh capabilities, and client caching 😇.
Few things worth tightening:
- Apply
additionalParametersbefore standard PKCE parameters so standard values aren't accidentally overwritten. - Ensure
loadClientvalidates the returned cache shape, and include theredirect_uriinDcrRegisteredClientso cached clients don't use mismatched callbacks on subsequent logins. - Provide a way for
refreshTokento reload the cached client itself, since the standard silent-refresh path doesn't persist the DCR handshake. - Update the
README.mdto reflect the new API and remove the outdated caching claim, and ensure tests cover async cache hooks and distinct auth-method precedence.
I also included a few optional follow-up notes in the details below.
Optional follow-up note (1)
- [P3] src/auth/providers/dcr.ts:349:
tokenUrlandresourceare resolved serially here. Both areOAuthLazyStrings, so if a consumer supplies async resolvers, every silent refresh now waits for them back-to-back. Resolve them withPromise.all(and likewise in the new authorize/exchangeCode resource lookups) to avoid adding avoidable latency on this path.
- buildPkceAuthorizeUrl: apply additionalParameters before the standard PKCE params so the latter always win (the loop previously ran after, enabling the clobbering the doc claimed to prevent). - Validate the loadClient cache shape (string clientId, supported tokenEndpointAuthMethod) so a cached client behaves like a freshly registered one instead of failing later or silently using the wrong auth method. - Resolve tokenUrl/authorizeUrl and the resource indicator concurrently (Promise.all) on the authorize, exchange, and refresh paths. - Document the loadClient redirect-URI binding (registration is bound to its redirect_uris; cache must be keyed on PrepareInput.redirectUri) and the DCR refresh handshake contract (clientId must be supplied on the refresh handshake, reconstructed from persisted account metadata). - Tests: make the refresh auth-method test prove handshake-over-config precedence (differing values), exercise async loadClient/saveClient, and cover malformed-cache rejection. - README: document resource indicators, refresh, and client caching; drop the stale "does not cache" claim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
amix
left a comment
There was a problem hiding this comment.
Looked through the code and reads well.
I think it’s critical that we test the refresh paths using comms-cli / todoist-cli to ensure everything works properly. There’s a lot of complexity in this flow, and plenty of places where things could break ... either in the implementation or on the backend.
Yup, just linking this PR to Comms CLI locally now to try it out. |
Add an optional `scope` to `ExchangeResult` carrying the raw `scope` from the token response (RFC 6749 §5.1). Populate it from the authorization_code and refresh_token grants in both createPkceProvider and createDcrProvider (and from postTokenEndpoint). Lets a provider's validateToken record the server-authoritative granted scope instead of re-deriving it from the request — important when the server narrows scope relative to what was asked for. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
## [0.25.0](v0.24.0...v0.25.0) (2026-06-05) ### Features * **auth:** add resource indicator, refresh, and client caching to DCR provider ([#60](#60)) ([0db0cf3](0db0cf3))
|
🎉 This PR is included in version 0.25.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Why
createDcrProvidercovered RFC 7591 dynamic client registration + PKCE code exchange, but a consumer that needs any of the following had to abandon the factory and hand-roll a fullAuthProvider(rawfetchDCR + PKCE + token exchange + refresh). This was discovered while reviewing comms-cli, whoseauth-provider.tsreimplements ~500 lines of OAuth that this factory already owns, purely to add three missing capabilities:resourceon the authorize URL and token requests. The factory had no hook for it.refreshTokenat all (onlycreatePkceProviderdid), so DCR consumers couldn't use cli-core's silent-refresh path.prepare()re-registered on every login; no way to reuse an issuedclient_id.What
resource?: OAuthLazyString— threaded into the authorize URL (new optionaladditionalParametersonbuildPkceAuthorizeUrl) and theauthorization_code+refresh_tokentoken request bodies (oauth4webapiadditionalParameters).refreshTokenon the DCR provider — honours the handshake's client auth (publicNone/client_secret_post/ RFC-3986 Basic), theresourceindicator, and a 10s timeout; mapsinvalid_grant → AUTH_REFRESH_EXPIRED, elseAUTH_REFRESH_TRANSIENT.loadClient/saveClienthooks — reuse a registered client across logins. A cache hit skips both the registration round-trip and theoauth4webapiload. cli-core stays storage-agnostic; the consumer owns persistence.Internal cleanup
oauth.tsasmapRefreshErrorand reused it fromcreatePkceProvider(−38 lines of duplicated mapping there).selectClientAuth/tokenRequestOptions/clientHandshakehelpers indcr.ts, shared byexchangeCodeand the newrefreshToken.Tests
9 new
dcr.test.tscases: resource on authorize URL + token body, refresh grant (+ resource), confidential-client refresh auth,invalid_grant → AUTH_REFRESH_EXPIRED, missing-clientId →AUTH_REFRESH_UNAVAILABLE,loadClientcache hit (no registration POST), andsaveClienton a cache miss. Full suite green, type-check + lint + format clean.🤖 Generated with Claude Code