feat(sdk): deprecate AuthProvider in favor of Interceptor pattern#899
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ConnectRPC interceptor-based authentication: new token and DPoP interceptors, an AuthConfig union and resolvers, wiring interceptors across Platform/TDF3 clients and RPC layers, README examples updated to show interceptor usage, and tests exercising both interceptor and legacy AuthProvider flows. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Interceptor as authTokenInterceptor
participant TokenProvider
participant Transport as RequestHandler
Client->>Interceptor: send RPC request
Interceptor->>TokenProvider: await token()
TokenProvider-->>Interceptor: token
Interceptor->>Interceptor: set Authorization: Bearer <token>
Interceptor->>Transport: next(req)
Transport-->>Client: response
sequenceDiagram
participant Client
participant DPoPInterceptor
participant TokenProvider
participant Crypto as CryptoService
participant DPoPUtils
participant Transport as RequestHandler
Client->>DPoPInterceptor: send RPC request
par resolve token and keys
DPoPInterceptor->>TokenProvider: await token()
DPoPInterceptor->>Crypto: await dpopKeys
end
TokenProvider-->>DPoPInterceptor: token
Crypto-->>DPoPInterceptor: keyPair
DPoPInterceptor->>DPoPUtils: generate DPoP proof (keys, method, url)
DPoPUtils-->>DPoPInterceptor: signed JWT
DPoPInterceptor->>DPoPInterceptor: set Authorization, DPoP, X-VirtruPubKey headers
DPoPInterceptor->>Transport: next(req)
Transport-->>Client: response
sequenceDiagram
participant OpenTDF
participant AuthConfig
participant Resolver as resolveInterceptors
participant PlatformClient
participant RPC
OpenTDF->>AuthConfig: provide { interceptors } or AuthProvider
AuthConfig->>Resolver: resolveInterceptors(auth)
Resolver-->>OpenTDF: interceptor[]
OpenTDF->>OpenTDF: store interceptors, set ready (if interceptors-only)
OpenTDF->>PlatformClient: construct with interceptors
PlatformClient->>RPC: intercepted request -> transport
RPC-->>PlatformClient: response
PlatformClient-->>OpenTDF: response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
eugenioenko
force-pushed
the
feat/interceptor-auth-pattern
branch
from
94b4d55 to
0e2ea76
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new authentication pattern using Connect RPC interceptors, offering a more flexible alternative to the legacy AuthProvider. It adds factory functions for bearer and DPoP-bound token interceptors, updates core clients (OpenTDF, PlatformClient, TDF3Client) to support this pattern, and provides a bridge for backward compatibility. Review feedback identifies a critical issue in lib/src/access.ts where RPC errors are masked by generic fallback error messages when an authProvider is absent. There is also a suggestion to broaden the TokenProvider type to allow synchronous string returns for better flexibility.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/policy/discovery.ts (1)
38-39:⚠️ Potential issue | 🟡 MinorStale JSDoc: parameter name changed from
authProvidertoauth.The documentation still references
authProviderbut the parameter is nowauth: AuthConfig. Same issue appears at lines 93-94, 154, and 194.📝 Suggested fix
* `@param` platformUrl The platform base URL. - * `@param` authProvider An auth provider for the request. + * `@param` auth Auth configuration (AuthProvider or { interceptors }) for the request.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/policy/discovery.ts` around lines 38 - 39, Summary: Stale JSDoc param name: replace authProvider with auth. Update every JSDoc `@param` tag that still says "authProvider" to use the actual parameter name "auth" and the correct type AuthConfig, e.g. change "@param authProvider ..." to "@param auth {AuthConfig} ...", keeping the original description; do this for all occurrences in this module (the functions that accept the auth parameter), run typecheck/lint to ensure JSDoc and signatures match, and adjust any related `@returns` or inline docs if they reference the old name.
🧹 Nitpick comments (2)
lib/src/opentdf.ts (1)
351-353: Dead code: the finalelsebranch is unreachable.Given the validation at lines 307-309, by the time we reach line 337:
- If
interceptorshas elements andauthProvideris absent → branch at 337- If
authProvideris present → branch at 341- If neither has valid auth → constructor already threw
The
elseat line 351 can never execute.🔧 Remove unreachable branch
if (interceptors && !authProvider) { // Interceptor path: no updateClientPublicKey needed. // DPoP key binding is handled by the interceptor itself. this.ready = Promise.resolve(); - } else if (authProvider) { + } else { // Legacy AuthProvider path: eagerly bind DPoP keys so PlatformClient // can make gRPC calls without waiting for a TDF operation first. this.ready = this.dpopEnabled ? this.dpopKeys.then((keys) => authProvider.updateClientPublicKey(keys)) : Promise.resolve(); // Prevent unhandled rejection if caller doesn't await ready. this.ready.catch((err) => { console.warn('OpenTDF: DPoP key binding failed during initialization:', err); }); - } else { - this.ready = Promise.resolve(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/opentdf.ts` around lines 351 - 353, The final unreachable else branch that sets this.ready = Promise.resolve() should be removed from the constructor: locate the constructor in opentdf.ts where this.ready is assigned (references to this.ready, interceptors, and authProvider) and delete the unreachable else block; keep the two valid branches that set this.ready when interceptors exist or when authProvider is provided, and ensure no other code expects the removed branch.lib/src/auth/interceptors.ts (1)
137-148: Consider more robust error detection for DPoP binding failures.The heuristic checking for
'public key'or'updateClientPublicKey'in error messages could miss variations or be triggered by unrelated errors. This is acceptable for backwards-compatibility guidance but is inherently fragile.💡 Alternative: Use error type/code if available
If the underlying auth providers throw typed errors (e.g., a custom
DPoPBindingError), checkinginstanceofwould be more reliable than string matching. However, if the existing providers only throw genericError, the current approach is pragmatic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/auth/interceptors.ts` around lines 137 - 148, Replace the fragile string-only checks in the catch block with a small robust predicate (e.g., isDPoPBindingError) and use it to detect DPoP binding failures: implement isDPoPBindingError(err) to first check typed indicators (err instanceof DPoPBindingError or err.name === 'DPoPBindingError' or err.code === 'DPoP_BINDING_FAILED'), then fall back to a conservative regex/test on the message (matching "public key" or "updateClientPublicKey") for backwards compatibility; call this predicate in the existing catch and keep the same thrown Error text when true, otherwise rethrow the original err.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@lib/src/policy/discovery.ts`:
- Around line 38-39: Summary: Stale JSDoc param name: replace authProvider with
auth. Update every JSDoc `@param` tag that still says "authProvider" to use the
actual parameter name "auth" and the correct type AuthConfig, e.g. change
"@param authProvider ..." to "@param auth {AuthConfig} ...", keeping the
original description; do this for all occurrences in this module (the functions
that accept the auth parameter), run typecheck/lint to ensure JSDoc and
signatures match, and adjust any related `@returns` or inline docs if they
reference the old name.
---
Nitpick comments:
In `@lib/src/auth/interceptors.ts`:
- Around line 137-148: Replace the fragile string-only checks in the catch block
with a small robust predicate (e.g., isDPoPBindingError) and use it to detect
DPoP binding failures: implement isDPoPBindingError(err) to first check typed
indicators (err instanceof DPoPBindingError or err.name === 'DPoPBindingError'
or err.code === 'DPoP_BINDING_FAILED'), then fall back to a conservative
regex/test on the message (matching "public key" or "updateClientPublicKey") for
backwards compatibility; call this predicate in the existing catch and keep the
same thrown Error text when true, otherwise rethrow the original err.
In `@lib/src/opentdf.ts`:
- Around line 351-353: The final unreachable else branch that sets this.ready =
Promise.resolve() should be removed from the constructor: locate the constructor
in opentdf.ts where this.ready is assigned (references to this.ready,
interceptors, and authProvider) and delete the unreachable else block; keep the
two valid branches that set this.ready when interceptors exist or when
authProvider is provided, and ensure no other code expects the removed branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da473a10-8b9f-49df-abb3-2bcb202eb352
📒 Files selected for processing (14)
lib/README.mdlib/src/access.tslib/src/access/access-rpc.tslib/src/auth/interceptors.tslib/src/index.tslib/src/opentdf.tslib/src/platform.tslib/src/policy/api.tslib/src/policy/discovery.tslib/tdf3/src/client/index.tslib/tdf3/src/tdf.tslib/tests/web/interceptors.test.tslib/tests/web/opentdf.test.tslib/tests/web/platform-rpc.test.ts
There was a problem hiding this comment.
Pull request overview
This PR migrates the SDK’s authentication model toward Connect RPC interceptors (with DPoP support) while keeping the legacy AuthProvider supported (deprecated). It threads the new AuthConfig shape through Platform/OpenTDF/TDF3 client layers, updates RPC/legacy fallbacks accordingly, and expands docs/tests to cover interceptor-first usage.
Changes:
- Introduces
authTokenInterceptor,authTokenDPoPInterceptor,authProviderInterceptor, plusAuthConfig+ resolver utilities. - Updates PlatformClient/OpenTDF/TDF3 client and access/policy layers to accept interceptor-based auth as the preferred path (AuthProvider remains supported but deprecated).
- Adds/updates web tests and README examples to validate the interceptor path.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/src/auth/interceptors.ts | New interceptor helpers + AuthConfig utilities; core of the interceptor auth feature. |
| lib/src/platform.ts | Switches legacy AuthProvider handling to the new authProviderInterceptor. |
| lib/src/opentdf.ts | Adds interceptor-first constructor options and readiness behavior. |
| lib/tdf3/src/client/index.ts | Adds interceptor support to TDF3 Client config and exposes a unified auth getter. |
| lib/tdf3/src/tdf.ts | Threads auth?: AuthConfig through encrypt/decrypt config and rewrap flow. |
| lib/src/access/access-rpc.ts | Updates RPC access helpers to accept AuthConfig and resolve interceptors. |
| lib/src/access.ts | Updates hybrid RPC/legacy fallback logic to use AuthConfig + resolveAuthConfig. |
| lib/src/policy/api.ts | Switches policy API functions to AuthConfig and resolves interceptors for PlatformClient. |
| lib/src/policy/discovery.ts | Same as above for discovery helpers. |
| lib/src/index.ts | Exports the new interceptor helpers/types from the public SDK entrypoint. |
| lib/tests/web/interceptors.test.ts | New test coverage for interceptor helpers and auth config utilities. |
| lib/tests/web/platform-rpc.test.ts | Adds PlatformClient integration tests for interceptor-only auth. |
| lib/tests/web/opentdf.test.ts | Adds OpenTDF constructor tests for interceptor-only auth. |
| lib/README.md | Updates docs to interceptor-first guidance with legacy AuthProvider section. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
If these changes look good, signoff on them with: If they aren't any good, please remove them with: |
X-Test Failure Report |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
lib/tests/web/interceptors.test.ts (1)
128-143: Assert the request passed intowithCreds(), not just the returned headers.This test would not catch regressions in the bridge input shape, which is the risky part of the legacy-compat adapter. Please capture the
HttpRequestargument and pin at leasturlandmethodhere so a lossy adapter cannot still pass the suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tests/web/interceptors.test.ts` around lines 128 - 143, Update the test that uses mockAuthProvider/withCreds and captureHeaders to also capture the HttpRequest argument passed into withCreds (the request parameter to the mock withCreds function) and assert at minimum its url and method properties alongside the existing header assertions; locate the mockAuthProvider.withCreds in the test, store the received HttpRequest (type HttpRequest) into a variable or spy when called by authProviderInterceptor, then add expect checks for request.url and request.method to ensure the bridge input shape is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/src/auth/interceptors.ts`:
- Around line 123-136: The code currently strips the request URL to pathOnly and
passes that to authProvider.withCreds, which loses origin/query info; update the
call in interceptors.ts to forward the full request URL (use req.url or
url.href) instead of pathOnly when calling authProvider.withCreds so the legacy
AuthProvider receives the entire HttpRequest URL for signature generation,
keeping the same method and headers handling around the withCreds call
(referencing authProvider.withCreds, req.url, url, and pathOnly to locate the
change).
In `@lib/src/opentdf.ts`:
- Around line 325-330: When constructing OpenTDF ensure you reuse any DPoP
keypair exposed by an auth interceptor before generating a new one: check for a
provided interceptor from authTokenDPoPInterceptor() (inspect
interceptor.dpopKeys) and if present assign this.dpopKeys =
interceptor.dpopKeys; otherwise fall back to
this.cryptoService.generateSigningKeyPair(); then pass this.dpopKeys into new
TDF3Client (dpopEnabled ? this.dpopKeys : undefined). Apply the same resolution
logic replacing the mirrored fallback in lib/tdf3/src/client/index.ts so both
places derive keys from interceptor.dpopKeys first.
---
Nitpick comments:
In `@lib/tests/web/interceptors.test.ts`:
- Around line 128-143: Update the test that uses mockAuthProvider/withCreds and
captureHeaders to also capture the HttpRequest argument passed into withCreds
(the request parameter to the mock withCreds function) and assert at minimum its
url and method properties alongside the existing header assertions; locate the
mockAuthProvider.withCreds in the test, store the received HttpRequest (type
HttpRequest) into a variable or spy when called by authProviderInterceptor, then
add expect checks for request.url and request.method to ensure the bridge input
shape is preserved.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 48289cf7-e755-4e48-a8bb-5aa5a0ba35bb
📒 Files selected for processing (6)
lib/src/access.tslib/src/auth/interceptors.tslib/src/opentdf.tslib/src/platform.tslib/tdf3/src/client/index.tslib/tests/web/interceptors.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/src/access.ts
eugenioenko
force-pushed
the
feat/interceptor-auth-pattern
branch
3 times, most recently
from
b9edbb9 to
70b18aa
Compare
X-Test Failure Report✅ js-v0.9.0 |
X-Test Failure Report |
X-Test Failure Report |
eugenioenko
commented
Mar 31, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/tests/web/interceptors.test.ts (1)
145-160: Minor: Test description doesn't match implementation.The test is named "wraps updateClientPublicKey errors" but actually tests error wrapping when
withCredsthrows. This is technically correct behavior (the implementation catches any error containing "public key"), but the test name is misleading.📝 Suggested rename
- it('wraps updateClientPublicKey errors with helpful message', async () => { + it('wraps withCreds errors mentioning public key with helpful message', async () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tests/web/interceptors.test.ts` around lines 145 - 160, Rename the test description to accurately reflect what it's asserting: change the "wraps updateClientPublicKey errors with helpful message" title to mention withCreds (e.g., "wraps withCreds errors with helpful message" or similar) so it matches the failingProvider implementation where updateClientPublicKey is a no-op and withCreds throws, and keep the rest of the test (authProviderInterceptor, failingProvider, captureHeaders) unchanged.lib/src/auth/interceptors.ts (1)
137-148: Consider: Fragile error detection via string matching.The error detection relies on checking if the message includes "public key" or "updateClientPublicKey". This could miss errors with different wording or match unrelated errors. However, since this is a backwards-compatibility bridge that will eventually be deprecated, this pragmatic approach is acceptable.
Based on learnings:
authProviderInterceptorintentionally passes onlyurl.pathname(not the full URL) toauthProvider.withCreds()to preserve backwards-compatibility with legacy AuthProvider implementations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/src/auth/interceptors.ts` around lines 137 - 148, The catch block in authProviderInterceptor currently detects DPoP/PK errors by fragile case-sensitive substring checks on msg (using includes('public key') or 'updateClientPublicKey'); update this handling to use a case-insensitive match (e.g., check msg.toLowerCase()) for those substrings to reduce false negatives, and add a clear comment near the catch explaining that authProviderInterceptor intentionally passes only url.pathname to authProvider.withCreds() to preserve backwards-compatibility with legacy AuthProvider implementations (so do not change the pathname behavior). Ensure you update the logic around the thrown Error message in the catch (the code that throws the 'PlatformClient: DPoP key binding is not complete...' error) to use the normalized/message check and keep the original error text included.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/src/auth/interceptors.ts`:
- Around line 137-148: The catch block in authProviderInterceptor currently
detects DPoP/PK errors by fragile case-sensitive substring checks on msg (using
includes('public key') or 'updateClientPublicKey'); update this handling to use
a case-insensitive match (e.g., check msg.toLowerCase()) for those substrings to
reduce false negatives, and add a clear comment near the catch explaining that
authProviderInterceptor intentionally passes only url.pathname to
authProvider.withCreds() to preserve backwards-compatibility with legacy
AuthProvider implementations (so do not change the pathname behavior). Ensure
you update the logic around the thrown Error message in the catch (the code that
throws the 'PlatformClient: DPoP key binding is not complete...' error) to use
the normalized/message check and keep the original error text included.
In `@lib/tests/web/interceptors.test.ts`:
- Around line 145-160: Rename the test description to accurately reflect what
it's asserting: change the "wraps updateClientPublicKey errors with helpful
message" title to mention withCreds (e.g., "wraps withCreds errors with helpful
message" or similar) so it matches the failingProvider implementation where
updateClientPublicKey is a no-op and withCreds throws, and keep the rest of the
test (authProviderInterceptor, failingProvider, captureHeaders) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dab6ee54-b1aa-4060-ab5d-d646526df503
📒 Files selected for processing (14)
lib/README.mdlib/src/access.tslib/src/access/access-rpc.tslib/src/auth/interceptors.tslib/src/index.tslib/src/opentdf.tslib/src/platform.tslib/src/policy/api.tslib/src/policy/discovery.tslib/tdf3/src/client/index.tslib/tdf3/src/tdf.tslib/tests/web/interceptors.test.tslib/tests/web/opentdf.test.tslib/tests/web/platform-rpc.test.ts
✅ Files skipped from review due to trivial changes (2)
- lib/src/index.ts
- lib/tests/web/opentdf.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/tdf3/src/tdf.ts
- lib/src/policy/discovery.ts
Unifies auth for both PlatformClient and OpenTDF on the Connect RPC
Interceptor pattern, eliminating the updateClientPublicKey footgun and
the dual-abstraction complexity before 1.0.
- Add authTokenInterceptor, authTokenDPoPInterceptor, authProviderInterceptor helpers
- Introduce AuthConfig union type (AuthProvider | { interceptors }) throughout
- OpenTDF and PlatformClient accept interceptors as the primary auth mechanism
- Thread interceptors through TDF3Client, tdf.ts, access, and policy layers
- AuthProvider remains fully supported as backwards-compatible (deprecated)
- Legacy fetch fallback preserved when AuthProvider is used
- Skip legacy fallback when no AuthProvider to propagate RPC errors directly
- Use full URI for DPoP htu claim, base64.encode for X-VirtruPubKey header
- Update README with interceptor-first examples
Discussion: https://github.com/orgs/opentdf/discussions/3167
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
eugenioenko
force-pushed
the
feat/interceptor-auth-pattern
branch
from
70b18aa to
4337118
Compare
X-Test Failure Report✅ go-main |
eugenioenko
commented
Mar 31, 2026
marythought
reviewed
Mar 31, 2026
Contributor
marythought
left a comment
marythought
left a comment
There was a problem hiding this comment.
Overall: This is a solid DX improvement. The interceptor pattern is genuinely simpler than AuthProvider for most users, kills the updateClientPublicKey / await client.ready footgun, and aligns with Connect RPC idioms. A few notes:
Docs follow-up
This PR marks AuthProvider as deprecated and the README now leads with interceptors as the recommended pattern. The published docs at opentdf.io (quickstart, discovery, tdf, authorization, policy, authentication, platform-client, troubleshooting) all use AuthProvider exclusively in JS examples — we should coordinate a docs update as a follow-up to this PR. When we do, I'd suggest leading with a "define once, pass everywhere" pattern:
import { authTokenInterceptor, OpenTDF, listAttributes, attributeExists } from '@opentdf/sdk';
// Define once
const auth = { interceptors: [authTokenInterceptor(() => getAccessToken())] };
// Reuse
const client = new OpenTDF({ ...auth, platformUrl });
const attrs = await listAttributes(platformUrl, auth);
const exists = await attributeExists(platformUrl, auth, 'https://opentdf.io/attr/department');Deprecation version
authProvider is marked @deprecated but without a version timeline. Adding something like @deprecated Since 0.x. Will be removed in 1.0. to the JSDoc would help developers plan their migration.
Dual auth + authProvider threading
After this PR, both auth: AuthConfig and authProvider: AuthProvider are passed side-by-side through multiple internal layers, and each layer has to decide which one to use. Concrete examples:
EncryptConfigurationandDecryptConfigurationintdf.tsboth carryauth?: AuthConfigandauthProvider?: AuthProvideras separate fieldsunwrapKeyintdf.tsaccepts both, then resolves withconst resolvedAuth: AuthConfig = (auth ?? authProvider) as AuthConfig— the type assertion papers over the ambiguityClientinclient/index.tsstores bothauthProviderandinterceptorsas separate fields, then has aget auth()getter that resolves which one to use- When calling downstream functions, both get passed:
auth: this.auth, authProvider: this.authProvider
This means every internal function has to reason about priority between two auth fields that represent the same concept, and the pattern will propagate into any new code that touches these layers.
Suggested cleanup (could be a follow-up): Resolve to AuthConfig once at the boundary — in the OpenTDF / Client constructor. Internally, thread only auth: AuthConfig through all layers. The authProvider field stays on the public-facing OpenTDFOptions and ClientConfig for backwards compat, but gets resolved to AuthConfig immediately in the constructor and never passed further. The one exception is the legacy fetch fallback in access.ts that genuinely needs AuthProvider.withCreds() — but resolveAuthConfig() already handles extracting that for exactly this case.
This would shrink the internal API surface, eliminate the type assertions, and make it impossible to accidentally use the wrong auth field in new code.
marythought
previously approved these changes
Mar 31, 2026
Contributor
marythought
left a comment
marythought
left a comment
There was a problem hiding this comment.
see notes for some followup ideas -- a great improvement!
…internally Remove dual auth/authProvider threading through internal layers. AuthConfig is now resolved once in the Client constructor from either authProvider or interceptors. Only `auth: AuthConfig` is passed through EncryptConfiguration, DecryptConfiguration, and unwrapKey. The authProvider field remains on the public-facing ClientConfig and OpenTDFOptions for backwards compatibility but is never passed further than the constructor. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Mark authProvider as deprecated since 0.14.0 across OpenTDFOptions, PlatformClientOptions, and ClientConfig. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/tests/web/opentdf.test.ts (1)
125-132: Test assertion is tangential to the test name.The test is titled "does not call updateClientPublicKey with interceptors" but the assertion checks
dpopEnabled. While the test logic is valid (noauthProvidermeans noupdateClientPublicKeyto call), consider either:
- Renaming to "interceptors-only mode keeps dpopEnabled true", or
- Adding a spy/tracking mechanism similar to the
trackingAuthProviderpattern used in lines 56-68 to explicitly verify the absence of a call.The current test still provides value by confirming the interceptor path completes without error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tests/web/opentdf.test.ts` around lines 125 - 132, The test name and assertion mismatch: it instantiates OpenTDF with interceptors via authTokenInterceptor and asserts client.dpopEnabled, but the title says "does not call updateClientPublicKey with interceptors"; either rename the test to reflect what is asserted (e.g., "interceptors-only mode keeps dpopEnabled true") or add an explicit spy like the existing trackingAuthProvider pattern to verify updateClientPublicKey was not called — locate the OpenTDF instantiation and client.ready usage in this test and either change the it(...) description to match the dpopEnabled assertion or inject a mock/tracker for updateClientPublicKey and assert it was never invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@lib/tests/web/opentdf.test.ts`:
- Around line 125-132: The test name and assertion mismatch: it instantiates
OpenTDF with interceptors via authTokenInterceptor and asserts
client.dpopEnabled, but the title says "does not call updateClientPublicKey with
interceptors"; either rename the test to reflect what is asserted (e.g.,
"interceptors-only mode keeps dpopEnabled true") or add an explicit spy like the
existing trackingAuthProvider pattern to verify updateClientPublicKey was not
called — locate the OpenTDF instantiation and client.ready usage in this test
and either change the it(...) description to match the dpopEnabled assertion or
inject a mock/tracker for updateClientPublicKey and assert it was never invoked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ddcabe31-94d9-4705-8444-ae8f263ca982
📒 Files selected for processing (4)
lib/src/opentdf.tslib/tdf3/src/client/index.tslib/tdf3/src/tdf.tslib/tests/web/opentdf.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- lib/tdf3/src/tdf.ts
- lib/tdf3/src/client/index.ts
X-Test Failure Report |
marythought
added a commit
to opentdf/docs
that referenced
this pull request
Apr 1, 2026
Updates all JavaScript/TypeScript code examples to use the new Connect RPC interceptor-based authentication introduced in opentdf/web-sdk#899, replacing the deprecated AuthProvider pattern. Key changes: - authTokenInterceptor as primary auth mechanism - "define once, pass everywhere" pattern for shared auth config - Remove await client.ready (no longer needed with interceptors) - AuthProvider examples moved to legacy sections - Standalone functions (listAttributes, etc.) use AuthConfig Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
4 tasks
marythought
added a commit
that referenced
this pull request
Apr 1, 2026
Adds convenience factories that return a TokenProvider for use with authTokenInterceptor, mirroring the existing AuthProviders pattern: - clientCredentialsTokenProvider: OAuth2 client credentials grant - refreshTokenProvider: refresh token exchange - externalJwtTokenProvider: RFC 8693 token exchange All providers cache tokens and auto-refresh when the JWT exp claim indicates expiration (with 30s buffer). This closes the ergonomics gap between the deprecated AuthProvider pattern and the new interceptor-based auth from #899. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
3 tasks
2 tasks
marythought
added a commit
that referenced
this pull request
Apr 2, 2026
Adds convenience factories that return a TokenProvider for use with authTokenInterceptor, mirroring the existing AuthProviders pattern: - clientCredentialsTokenProvider: OAuth2 client credentials grant - refreshTokenProvider: refresh token exchange - externalJwtTokenProvider: RFC 8693 token exchange All providers cache tokens and auto-refresh when the JWT exp claim indicates expiration (with 30s buffer). This closes the ergonomics gap between the deprecated AuthProvider pattern and the new interceptor-based auth from #899. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
marythought
added a commit
that referenced
this pull request
Apr 6, 2026
Adds convenience factories that return a TokenProvider for use with authTokenInterceptor, mirroring the existing AuthProviders pattern: - clientCredentialsTokenProvider: OAuth2 client credentials grant - refreshTokenProvider: refresh token exchange - externalJwtTokenProvider: RFC 8693 token exchange All providers cache tokens and auto-refresh when the JWT exp claim indicates expiration (with 30s buffer). This closes the ergonomics gap between the deprecated AuthProvider pattern and the new interceptor-based auth from #899. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mary Dickson <mary.dickson@virtru.com>
marythought
added a commit
that referenced
this pull request
Apr 6, 2026
…906) * feat(sdk): add TokenProvider factory functions for common OIDC flows Adds convenience factories that return a TokenProvider for use with authTokenInterceptor, mirroring the existing AuthProviders pattern: - clientCredentialsTokenProvider: OAuth2 client credentials grant - refreshTokenProvider: refresh token exchange - externalJwtTokenProvider: RFC 8693 token exchange All providers cache tokens and auto-refresh when the JWT exp claim indicates expiration (with 30s buffer). This closes the ergonomics gap between the deprecated AuthProvider pattern and the new interceptor-based auth from #899. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mary Dickson <mary.dickson@virtru.com> * fix(sdk): address review feedback on token providers - Add in-flight promise deduplication to all three providers to prevent concurrent token fetches and refresh token rotation races - Support expires_in from token response for cache TTL (opaque tokens) - Validate oidcOrigin is non-empty to prevent silent credential leaks - Add regression tests for concurrency, opaque token caching, and blank oidcOrigin rejection Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mary Dickson <mary.dickson@virtru.com> * chore(sdk): suppress no-unused-vars lint for mockFetch params The parameters are needed for sinon's type inference (tests access fetchFake.firstCall.args), so add an eslint-disable comment rather than removing them. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mary Dickson <mary.dickson@virtru.com> * chore(sdk): add error response handling tests for token providers Each provider now has a test verifying that a failed token fetch rejects the promise and clears the in-flight state, allowing a subsequent retry to succeed. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mary Dickson <mary.dickson@virtru.com> * 🤖 🎨 Autoformat Signed-off-by: Mary Dickson <mary.dickson@virtru.com> * chore(sdk): add server-side-only warning to clientCredentialsTokenProvider Client secrets must not be exposed in browser code. Adds JSDoc warnings to both the options type and the factory function per reviewer feedback. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mary Dickson <mary.dickson@virtru.com> * chore(sdk): add buffer refresh, fallback, and exact assertion tests - Test 30-second pre-expiry refresh buffer on clientCredentialsTokenProvider - Test externalJwtTokenProvider re-exchange fallback when no refresh_token - Use URLSearchParams for exact RFC 8693 form field assertions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mary Dickson <mary.dickson@virtru.com> * 🤖 🎨 Autoformat Signed-off-by: Mary Dickson <mary.dickson@virtru.com> --------- Signed-off-by: Mary Dickson <mary.dickson@virtru.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: marythought <10136229+marythought@users.noreply.github.com>
marythought
added a commit
to opentdf/docs
that referenced
this pull request
Apr 7, 2026
## Summary Updates all JavaScript/TypeScript code examples across SDK documentation to use the new Connect RPC interceptor-based authentication pattern with built-in token provider factories, replacing the deprecated `AuthProvider` approach and manual token fetching. **Dependencies:** opentdf/web-sdk#899 (interceptor pattern) and opentdf/web-sdk#906 (token provider factories) — both merged to main. ## What changed ### Auth pattern migration - Manual `getAccessToken()` implementations replaced with `clientCredentialsTokenProvider()`, `refreshTokenProvider()`, `externalJwtTokenProvider()` factory functions - `AuthProviders.clientSecretAuthProvider()` replaced with `authTokenInterceptor()` + token provider factories as the primary JS auth pattern - "Define once, pass everywhere" idiom: create an `auth` config object once, reuse for `OpenTDF`, `PlatformClient`, and standalone functions - Removed all `await client.ready` — no longer needed with interceptors - `AuthProvider` examples preserved in Legacy sections for backwards compatibility ### Browser-first messaging - Added shared admonition (`code_samples/js_auth_note.mdx`) across all SDK pages clarifying that `clientCredentialsTokenProvider` is for learning/server-side only — the JS SDK is designed for browser apps - Authentication page tip now leads with browser guidance, pointing to `refreshTokenProvider()` as the primary JS path - Client credentials warning strengthened: "Server-side only — not for browsers" - Quickstart frames Node.js as a convenience for learning, not the intended runtime - Certificate exchange (mTLS) section updated: browsers handle client certs at the OS level, no SDK config needed ### Developer experience improvements - Quickstart uses `attributeExists()` and `attributeValueExists()` discovery helpers instead of manual list+filter patterns - Subject condition set code collapsed from pyramid nesting to `[{` style with explanatory comments - `getMyToken()` in Custom Token Source section now has a concrete definition with OIDC library examples - DPoP section expanded with Go and Java tabs (was JS-only) - CreateTDF and LoadTDF parameter tables split into per-SDK tabs matching actual signatures - Troubleshooting "Getting Help" links to all 3 SDK repos - Platform client Go example shows service usage with authorization v2 ### Files modified **New:** - `code_samples/js_auth_note.mdx` — shared browser-use admonition partial **Docs (9 files):** - `docs/sdks/authentication.mdx` — all 4 auth patterns + DPoP tabs + mTLS fix + legacy in tabs - `docs/sdks/quickstart/javascript.mdx` — browser-first framing, discovery helpers, cleaner code - `docs/sdks/discovery.mdx` — shared admonition replaces old tip - `docs/sdks/tdf.mdx` — per-SDK parameter tables, shared admonition - `docs/sdks/authorization.mdx` — shared admonition - `docs/sdks/platform-client.mdx` — shared admonition, Go service usage example - `docs/sdks/policy.mdx` — shared admonition - `docs/sdks/troubleshooting.mdx` — shared admonition, SDK repo links - `docs/guides/authentication-guide.mdx` — decision guide references to factory functions **Code samples (7 files):** - All files in `code_samples/policy_code/` — inline interceptor + `clientCredentialsTokenProvider` pattern ## Follow-up Sidebar reordering, example placement, and JS/TS tab naming consistency tracked in #274. ## Test plan - [x] `npm run build` passes - [x] Surge preview renders admonitions correctly across all pages - [x] Code examples are copy-pasteable with correct imports - [x] Legacy AuthProvider section still present and accurate 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Updated SDK guides and examples to use a unified interceptor-based authentication approach * Added JavaScript-specific auth callouts clarifying client-credentials are server-only and browser apps should use OIDC/refresh-token flows * Revised quickstarts, samples, and troubleshooting with explicit token-provider guidance and token lifecycle notes * Added legacy deprecation guidance while noting backward compatibility remains intact <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Mary Dickson <mary.dickson@virtru.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Unifies auth for both PlatformClient and OpenTDF on the Connect RPC Interceptor pattern, eliminating the updateClientPublicKey footgun and the dual-abstraction complexity before 1.0.
Discussion: https://github.com/orgs/opentdf/discussions/3167
What we gain:
Summary by CodeRabbit
New Features
Documentation
Tests