feat(dpop): attach DPoP proof to token request and handle nonce retry#2511
feat(dpop): attach DPoP proof to token request and handle nonce retry#2511abhip2565 wants to merge 2 commits into
Conversation
Wire the wallet side of DPoP (RFC 9449) per the DPoP ADR. The VCIClient library builds and signs the token-endpoint proof and delivers it on tokenRequest.dpopProof; the wallet copies it into the DPoP header on the token POST. On an authorization server use_dpop_nonce challenge the wallet asks the library for a fresh proof bound to the nonce and retries once. - Extract the token request into shared/openId4VCI/sendTokenRequest with DPoP header support and the use_dpop_nonce retry. - Add VciClient.generateTokenDPoPProof and expose it through the Android and iOS native bridges; forward tokenRequest.dpopProof to JS. - Unit tests for the DPoP header, nonce retry, and error handling. Signed-off-by: abhip2565 <paul.apaul.abhishek.ap@gmail.com>
|
Warning Review limit reached
Next review available in: 57 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughAdds DPoP Token Request Flow
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shared/openId4VCI/sendTokenRequest.test.ts (1)
83-133: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a nonce-path test for proof-generation failures.
These cases only cover the branch where
generateTokenDPoPProof()succeeds. That call is the new native-bridge dependency in this flow, so a rejection there would currently bypass this suite entirely.Suggested test
+ it('surfaces proof generation failures during a use_dpop_nonce retry', async () => { + (global.fetch as jest.Mock).mockResolvedValueOnce( + fakeResponse({ + ok: false, + status: 400, + body: '{"error":"use_dpop_nonce"}', + nonce: 'server-nonce', + }), + ); + mockGenerateTokenDPoPProof.mockRejectedValueOnce(new Error('bridge failed')); + + await expect( + sendTokenRequest({...baseRequest, dpopProof: 'proof-a'}), + ).rejects.toThrow('bridge failed'); + expect((global.fetch as jest.Mock).mock.calls).toHaveLength(1); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/openId4VCI/sendTokenRequest.test.ts` around lines 83 - 133, The nonce retry coverage in sendTokenRequest.test.ts only verifies the success path after a use_dpop_nonce challenge, so add a test for when generateTokenDPoPProof() fails while handling that nonce challenge. Extend the sendTokenRequest retry branch tests around sendTokenRequest and mockGenerateTokenDPoPProof to reject on the retry attempt, and assert the request fails with the expected issuer error handling instead of proceeding to a second fetch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@shared/openId4VCI/sendTokenRequest.ts`:
- Around line 58-63: `toVciClientError()` is dropping the existing error fields
that `VciClient.ts` still expects. Update the VCI error mapping in
`sendTokenRequest()` so the returned `VciClientErrorResponse` preserves the
original `code` and `message` contract while also supporting `issuerErrorCode`
and `issuerErrorMessage`. Make the change in the `toVciClientError` helper and
ensure `VciClient` continues to receive the same thrown error shape it already
handles.
---
Nitpick comments:
In `@shared/openId4VCI/sendTokenRequest.test.ts`:
- Around line 83-133: The nonce retry coverage in sendTokenRequest.test.ts only
verifies the success path after a use_dpop_nonce challenge, so add a test for
when generateTokenDPoPProof() fails while handling that nonce challenge. Extend
the sendTokenRequest retry branch tests around sendTokenRequest and
mockGenerateTokenDPoPProof to reject on the retry attempt, and assert the
request fails with the expected issuer error handling instead of proceeding to a
second fetch.
🪄 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: 2b6ea7af-2cf6-4d80-8635-81ccce9fed50
📒 Files selected for processing (8)
android/app/src/main/java/io/mosip/residentapp/InjiVciClientModule.javaandroid/app/src/main/java/io/mosip/residentapp/VCIClientBridge.ktios/RNVCIClientModule.mios/RNVCIClientModule.swiftmachines/Issuers/IssuersService.tsshared/openId4VCI/sendTokenRequest.test.tsshared/openId4VCI/sendTokenRequest.tsshared/vciClient/VciClient.ts
| function toVciClientError(errorText: string): VciClientErrorResponse { | ||
| const parsedError = parseError(errorText); | ||
| return { | ||
| issuerErrorCode: parsedError.error ?? 'UNKNOWN_ERROR', | ||
| issuerErrorMessage: parsedError.error_description, | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep code and message in the thrown error contract.
toVciClientError() now returns only issuerErrorCode / issuerErrorMessage, but shared/vciClient/VciClient.ts Lines 182-187 still treat VciClientErrorResponse as carrying code and message too. That changes the error shape for sendTokenRequest() callers and breaks the PR’s “preserve existing error shape” goal.
Suggested fix
function toVciClientError(errorText: string): VciClientErrorResponse {
const parsedError = parseError(errorText);
+ const code = parsedError.error ?? 'UNKNOWN_ERROR';
+ const message =
+ parsedError.error_description ?? 'An unknown error occurred';
+
return {
- issuerErrorCode: parsedError.error ?? 'UNKNOWN_ERROR',
- issuerErrorMessage: parsedError.error_description,
+ code,
+ message,
+ issuerErrorCode: code,
+ issuerErrorMessage: parsedError.error_description,
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function toVciClientError(errorText: string): VciClientErrorResponse { | |
| const parsedError = parseError(errorText); | |
| return { | |
| issuerErrorCode: parsedError.error ?? 'UNKNOWN_ERROR', | |
| issuerErrorMessage: parsedError.error_description, | |
| }; | |
| function toVciClientError(errorText: string): VciClientErrorResponse { | |
| const parsedError = parseError(errorText); | |
| const code = parsedError.error ?? 'UNKNOWN_ERROR'; | |
| const message = | |
| parsedError.error_description ?? 'An unknown error occurred'; | |
| return { | |
| code, | |
| message, | |
| issuerErrorCode: code, | |
| issuerErrorMessage: parsedError.error_description, | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@shared/openId4VCI/sendTokenRequest.ts` around lines 58 - 63,
`toVciClientError()` is dropping the existing error fields that `VciClient.ts`
still expects. Update the VCI error mapping in `sendTokenRequest()` so the
returned `VciClientErrorResponse` preserves the original `code` and `message`
contract while also supporting `issuerErrorCode` and `issuerErrorMessage`. Make
the change in the `toVciClientError` helper and ensure `VciClient` continues to
receive the same thrown error shape it already handles.
Signed-off-by: abhip2565 <paul.apaul.abhishek.ap@gmail.com>
abhip2565
force-pushed
the
feat/dpop-token-request
branch
2 times, most recently
from
e1f75bb to
3c99857
Compare
1 participant
Wires the wallet side of DPoP (RFC 9449) per the DPoP ADR.
The VCIClient library owns DPoP end-to-end: it builds and signs the token-endpoint proof and delivers it on
tokenRequest.dpopProof. The wallet's only responsibilities are to copy that proof into theDPoPheader on the token POST and, on an authorization serveruse_dpop_noncechallenge, ask the library for a fresh proof bound to the supplied nonce and retry once.Changes:
shared/openId4VCI/sendTokenRequest.tswith DPoP header support and theuse_dpop_nonceretry, preserving the existingVciClientErrorResponseerror shape.VciClient.generateTokenDPoPProof(dpopNonce)and expose it through the Android (InjiVciClientModule/VCIClientBridge) and iOS (RNVCIClientModule) native bridges; forwardtokenRequest.dpopProofto JS.Note
The native bridge changes consume
TokenRequest.dpopProofandVCIClient.generateTokenDPoPProof, which are introduced by the DPoP changes ininji-vci-client(inji/inji-vci-client#145) andinji-vci-client-ios-swift(inji/inji-vci-client-ios-swift#107). The native build needs those published and the dependency bumped; the JS layer (and its tests) are self-contained and gracefully no-op whendpopProofis absent.Summary by CodeRabbit
New Features
Bug Fixes
Tests