feat(W-22697744): DPoP proof JWT on API calls (Phase 3)#2949
feat(W-22697744): DPoP proof JWT on API calls (Phase 3)#2949wmathurin wants to merge 11 commits into
Conversation
…mentation reference
Analysis doc belongs with implementation spec/plan in the workspace repo, not in the Android repo — it references upcoming DPoP work rather than describing the existing codebase permanently.
- DPoPProofBuilder: add ath claim (SHA-256 of access token) when accessToken is non-null/non-empty; add nonce claim when nonce is non-null/non-empty - RestClient: add attachDPoPProofHeader helper that builds and attaches a DPoP proof on initial requests; OAuthRefreshInterceptor attaches proof on 401-retry path too; credentialsIdentifier threaded through constructor chain - ClientManager.peekRestClient: passes tokenType and credentialsIdentifier to RestClient - OAuth2.callIdentityService: new overload that attaches DPoP proof when token is DPoP-bound; TokenEndpointResponse carries credentialsIdentifier for downstream callers - AuthenticationUtilities: fetchUserIdentity passes tokenType and credentialsIdentifier to identity service call - DPoPProofBuilderTest: four new ath claim tests
JohnsonEricAtSalesforce
left a comment
JohnsonEricAtSalesforce
left a comment
There was a problem hiding this comment.
Phase 3 DPoP review. The core crypto is solid — ath is computed as base64url(SHA-256(access_token)) byte-for-byte identically to iOS, base64url flags match, and the null/empty guards are right. I verified iOS Phase 3 parity (DPoPRequestDecorator / PR 4060) and ran the DPoP instrumented suite locally (DPoPProofBuilderTest 11/11, DPoPKeyManagerTest 5/5, DPoPURLHelperTest 4/4). This also closes the Phase 2 "unused ath/nonce params" thread. 🎉
Requesting changes on one item — a redundant double proof-attach on the primary REST path (each request signs twice and discards the first). Plus a minor cross-platform consistency question on the identity-service fail-open behavior; not a blocker, just flagging for a conscious decision.
This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
| final String proof = DPoPProofBuilder.INSTANCE.buildProof("GET", htu, keyPair, null, authToken); | ||
| builder.header(DPOP, proof); | ||
| } catch (Exception e) { | ||
| SalesforceSDKLogger.e(TAG, "Failed to attach DPoP header for identity service, proceeding without it", e); |
There was a problem hiding this comment.
Small cross-platform consistency question on the identity-service path. If proof attachment throws here we log and proceed — but the request still carries Authorization: DPoP <token> with no DPoP proof header, which the server will reject, so the fallback can't really succeed.
iOS made the identity coordinator (SFIdentityCoordinator) fail-closed for exactly this in commit 97a62410a — it bails out and surfaces the error rather than sending an unusable request. Worth noting the resource-API path is fine as-is: both platforms are fail-open there (iOS SFRestRequest logs and proceeds too), so this is just about the identity call site matching iOS.
Not asserting a change — this touches auth/credential handling, so flagging for a conscious call: either match iOS and surface the failure on the identity path, or keep a uniform fail-open policy across all DPoP paths and drop a one-line comment noting it's intentional. What's your preference?
This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
- RestClient: remove redundant DPoP proof attach from buildRequest() — the interceptor's buildAuthenticatedRequest() is the sole attach site, avoiding a double EC P-256 signing per request - OAuth2.callIdentityService: make DPoP proof attachment fail-closed on the identity path (remove try/catch, let exceptions propagate) to match iOS SFIdentityCoordinator behaviour; a missing proof on a DPoP-bound identity request would be server-rejected anyway
credentialsIdentifier and tokenType were never stored in the Android AccountManager (via buildAuthBundle) nor read back (via buildUserAccount). This caused getCredentialsIdentifier() to return null at token refresh time, bypassing the DPoP proof attachment in makeTokenEndpointRequest and causing the server to reject the refresh with "app requires proof of possession". - Add KEY_CREDENTIALS_IDENTIFIER and KEY_TOKEN_TYPE constants to AuthenticatorService - Store both fields (encrypted) in buildAuthBundle - Read both fields back in buildUserAccount and pass to builder Also enable DPoP in AuthFlowTester application and add DPoP test server.
credentialsIdentifier and tokenType were previously omitted from buildAuthBundle/buildUserAccount. Add tests that would have caught it: - Add TEST_CREDENTIALS_IDENTIFIER and TEST_TOKEN_TYPE to createTestAccount() so the existing full round-trip test (testUserAccountToAccountToUserAccount) now exercises the DPoP fields through the complete serialization path. - test_givenDPoPAccount_whenCreateAndBuildUserAccount: explicit assertion that both fields survive createAccount → buildUserAccount. - test_givenDPoPAccount_whenUpdateAccount: same check for updateAccount path.
JohnsonEricAtSalesforce
left a comment
JohnsonEricAtSalesforce
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround, @wmathurin. Confirmed both items are addressed: the double proof-attach is gone (buildRequest() now defers entirely to the interceptor — 👍), and callIdentityService is fail-closed with the iOS 97a62410a reference. Nice catch on the AccountManager credentialsIdentifier/tokenType persistence gap too, and the round-trip regression tests for it pass locally.
One blocker before this is green: I ran the SalesforceSDK instrumented suite at this head and UserAccountTest.testConvertAccountToBundle and testConvertAccountToJSON both fail — the shared test-account builder now populates the two new fields, but the expected Bundle/JSON in those two tests weren't updated to match. Details inline. Everything else (DPoP suites + the new UserAccountManagerTest round-trips) passes 45/47.
This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
|
@wmathurin — a heads-up on CI coverage for the DPoP rollout, and a small ask. The phase PRs targeting the The config looks right: the If that's the cause, the fix is to add Worth doing now rather than at merge-back because this isn't the last phase — there are still three Android PRs queued to target As a concrete example of the debt: locally I found Phase 3 shipped with two red serialization tests ( This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce. |
createTestAccountJSON() and createTestAccountBundle() were missing credentialsIdentifier and tokenType, causing testConvertAccountToBundle and testConvertAccountToJSON to fail after createTestAccount() started populating those fields.
|
Thanks for flagging this — you're right about the root cause. Opened #2950 which adds |
|
@wmathurin — once the CI-enable change (2950) merges to Would appreciate the close/re-open once the CI PR is in, so we get a green (or informative) CI run here before this lands. Thanks! This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce. |
Generated by 🚫 Danger |
Same temporary exception as pr.yaml. Remove once DPoP merges back to dev.
Generated by 🚫 Danger |
- AuthenticatorServiceTest: stub isUseDPoP() on SalesforceSDKManager mock - LoginViewModelMockTest: add credentialsIdentifier param to all AuthenticationUtilities.onAuthFlowComplete mock/verify blocks; update LoginViewModel.onAuthFlowComplete spy mocks to 6-arg signature; update exchangeCode mocks to 8-arg signature (now passes SalesforceSDKManager and credentialsIdentifier) - ClientManagerMockTest: fix missing quotes around REFRESHED_ACCESS_TOKEN in refresh response JSON (caused JSON parse failure → null auth token)
Should be addressed by 223fdc3 |
makeTokenEndpointRequest calls isUseDPoP() when credentialsIdentifier is non-null. Relaxed UserAccount mocks return "" for getCredentialsIdentifier(), so the DPoP block is entered and isUseDPoP() is called on the non-relaxed SalesforceSDKManager mock — throwing MockKException and causing all token refresh calls to return null.
JohnsonEricAtSalesforce
left a comment
JohnsonEricAtSalesforce
left a comment
There was a problem hiding this comment.
Approving Phase 3. All three items from the earlier review are resolved and verified:
- P2.1 (double proof-attach) —
buildRequest()now defers entirely to the interceptor. 👍 - P2.2 (identity-service fail-open) —
callIdentityServiceis fail-closed with the iOS97a62410areference. - P2.3 (serialization tests) — the
credentialsIdentifier/tokenTypefixtures are updated, and the broader DPoP test-debt inLoginViewModelMockTest/AuthenticatorServiceTest/ClientManagerMockTestis fixed too.
The full SalesforceSDK unit suite is green at this head (0e1d6b8dd).
On the red ui-tests-pr job: I pulled the AuthFlowTester results and it's an environmental/harness flake, not a Phase-3 issue. 49 of the failures are the identical AssertionError: Timed out after 15000ms waiting for node: "user_creds_section" across every login-flow class (Beacon/ECA/CA/BootConfig/Admin), which is the harness's credentials section never rendering — the ath/proof changes don't touch that path. Confirmed it's PR-independent: this same user_creds_section timeout shows up on unrelated PRs in the same window (e.g. the W-23195021 RTR PR), and the ui-tests-pr job flips green/red across PRs regardless of content (it passed on other PRs around the same time). Recommend a re-run of the UI job to clear it — happy to help dig if it reproduces after a re-run.
This response was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
2 participants
Summary
DPoPProofBuilder:buildPayloadnow includes theathclaim (base64url(SHA-256(accessToken))) whenaccessTokenis non-null/non-empty, and thenonceclaim when non-null/non-empty (was silently ignored before)RestClient: newattachDPoPProofHeaderhelper attaches a DPoP proof on every outgoing API request whentokenType == "DPoP";OAuthRefreshInterceptor.buildAuthenticatedRequestalso callsattachDPoPProofIfNeededso the 401-retry path gets a fresh proof too;credentialsIdentifierthreaded through all constructor chainsClientManager.peekRestClient: now passesuserAccount.getTokenType()anduserAccount.getCredentialsIdentifier()toRestClientOAuth2.callIdentityService: new overload attaches DPoP proof (withath) for the identity endpoint when token is DPoP-bound;TokenEndpointResponsecarriescredentialsIdentifierfor downstream callers;makeTokenEndpointRequestsets it on the responseAuthenticationUtilities:fetchUserIdentitypassestokenTypeandcredentialsIdentifierfrom the token response to the identity service callDPoPProofBuilderTest: 4 newathclaim tests (present, null, empty, deterministic)Test plan
DPoPProofBuilderTest— all 11 tests pass (7 existing + 4 newathtests)DPoPKeyManagerTest— all 5 tests pass (unchanged)DPoPURLHelperTest— all 4 tests pass (unchanged)Authorization: DPoP <token>andDPoP: <proof>headers withathclaimDPoPheader,Authorization: Bearer <token>)