fix(sdk): correct disableDPoP flag and add eager DPoP key binding#883
fix(sdk): correct disableDPoP flag and add eager DPoP key binding#883marythought merged 10 commits intomainfrom
Conversation
`!!disableDPoP` coerces to boolean without negating, so `disableDPoP: true` was enabling DPoP. Change to `!disableDPoP`. Fixes #869 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical bug in the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
…ctor Verifies that disableDPoP correctly negates to dpopEnabled for true, false, and omitted cases. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request correctly fixes the logic for the disableDPoP flag in the OpenTDF constructor. However, the fix is incomplete because the resulting dpopEnabled property is not used to configure the underlying tdf3Client. As a result, the disableDPoP flag remains ineffective. I've added a comment with more details on this issue.
The TDF3Client defaults dpopEnabled to true when dpopKeys are present, so passing dpopKeys without dpopEnabled: false meant the flag was silently ignored. Now we forward dpopEnabled and only pass dpopKeys when DPoP is enabled. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Ensures the dpopEnabled flag is correctly forwarded so that disableDPoP actually prevents DPoP usage downstream. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
X-Test Failure Report |
X-Test Failure Report |
X-Test Failure Report |
PlatformClient shares an authProvider with OpenTDF but has no way to trigger DPoP key binding itself. Previously, keys were only bound lazily during the first encrypt/decrypt operation, causing gRPC calls to fail if PlatformClient was used before any TDF operation. Now OpenTDF exposes a `ready` promise that resolves once keys are bound. Users can `await client.ready` before creating a PlatformClient. Fixes #881 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request correctly inverts the disableDPoP flag logic and introduces an eager DPoP key binding mechanism via a ready promise. The changes are well-supported by new tests. However, I've identified a significant issue where separate DPoP keys could be generated for OpenTDF and TDF3Client instances, leading to inconsistent behavior. I've provided a suggestion to unify the key generation and ensure both instances use the same key.
X-Test Failure Report |
…pair When dpopKeys is not provided, both OpenTDF and TDF3Client were independently generating key pairs, resulting in mismatched DPoP keys. Move dpopKeys initialization before TDF3Client construction so both share the same key pair. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
X-Test Failure Report |
…double binding - Add .catch() to prevent unhandled promise rejection when caller doesn't await ready; the error still surfaces via TDF3Client - Add comment explaining the benign double updateClientPublicKey call - Add test for ready rejection propagation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
X-Test Failure Report |
Resolve conflict in platform-client.mdx and replace manual updateClientPublicKey workaround with the new `await client.ready` pattern from opentdf/web-sdk#883 across all JS/TS code samples. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- PlatformClient auth interceptor now catches DPoP-related errors and throws a clear message directing users to `await client.ready` - OpenTDF ready.catch now logs a console.warn instead of silently swallowing DPoP key binding failures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
X-Test Failure Report✅ java-main |
X-Test Failure Report |
Summary
disableDPoPoption being inverted in theOpenTDFconstructor —!!disableDPoPcoerced to boolean without negating, sodisableDPoP: truewas enabling DPoPdpopEnabledthrough toTDF3Clientso the flag actually takes effect downstreamreadypromise toOpenTDFthat eagerly binds DPoP keys to the auth provider at construction time, soPlatformClientcan make gRPC calls without waiting for a TDF operation firstBefore
After
Test plan
disableDPoP→dpopEnabledmapping (true, false, omitted)dpopEnabledpropagation totdf3Clientreadypromise with DPoP enabled and disabledFixes #869
Fixes #881
🤖 Generated with Claude Code