Skip to content

feat(dpop): send dpop_jkt in /authorize for my-domain servers (W-23406836)#2965

Merged
wmathurin merged 8 commits into
forcedotcom:dpopfrom
wmathurin:dpop-phase7
Jul 16, 2026
Merged

feat(dpop): send dpop_jkt in /authorize for my-domain servers (W-23406836)#2965
wmathurin merged 8 commits into
forcedotcom:dpopfrom
wmathurin:dpop-phase7

Conversation

@wmathurin

@wmathurin wmathurin commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Core feature: `dpop_jkt` in `/authorize` (W-23406836)

  • Eagerly generates the DPoP EC key pair in `LoginViewModel.generateAuthorizationUrl()` when `useDPoP=true`, so the RFC 7638 JWK thumbprint is available before the `/authorize` URL is built
  • Adds `dpop_jkt=` to the `/authorize` request query params (my-domain servers only — pool servers `login.salesforce.com`, `test.salesforce.com`, `welcome.salesforce.com` do not support DPoP code binding)
  • Stores the generated `credentialsIdentifier` as `pendingCredentialsIdentifier` on the ViewModel so `doCodeExchange()` reuses the same key pair alias, preserving the binding from authorize → token exchange
  • Applies the same fix to `generateMigrationAuthorizationPath()`

Without this fix the server rejects with:
```
error=invalid_request&error_description=missing required dpop_jkt for code binding
```

Supporting refactors

  • Extracted `LoginServerManager.isPoolServer()` helper to centralise the pool-server check used by the `dpop_jkt` guard
  • Extracted `addDpopJktIfNeeded()` helper to remove duplication between the authorize and migration URL paths (previously `maybeAddDpopJkt`)
  • Cleared stale `dpop_jkt` and deleted the orphaned AndroidKeyStore key when re-entering a server, preventing a key-alias mismatch on subsequent logins
  • Fixed an accidental `_forceAdvancedAuthentication=false` regression introduced during refactoring

AuthFlowTester improvements

  • Added missing AndroidManifest intent filters for `ecajwtrtr`, `ecaopaquertr`, `ecajwtdpop`, and `ecajwtdpoprtr` redirect URI schemes — the four RTR/DPoP apps in `ui_test_config.json` had no filters, so OAuth callbacks would never be received
  • Added full `DPoPLoginTests` suite covering: basic DPoP login (hybrid and non-hybrid), RTR, multi-user unique tokens, migration, restart, and Login-for-Admins with DPoP
  • Fixed BW/DP/MU/RT flag assertions throughout `AuthFlowTest`: `forceAdvancedAuthentication` was not forwarded as `expectAdvancedAuth` in `loginAndValidate`, `adminLoginAndValidate`, `assertRevokeAndRefreshWorks`, `migrateAndValidate`, `restartAndValidateUser`, and `switchToUserAndValidateUser`
  • Removed stale nonce-change assertion: server issues DPoP nonces with a 24 h TTL, not one per `/token` response
  • Added `forceAdvancedAuthentication` parameter to `loginAndValidate` for tests that need the in-app WebView path
  • Ignored `testECAJwtDPoPRtr_Hybrid`: blocked by the same W-22512846 server limitation (hybrid grant rejected when RTR + JWT enabled); `testMigrate_ECAJwtDPoP_To_ECAJwtDPoPRtr` uses `useHybridAuthToken=false` as a workaround for the same reason

Test plan

  • All non-skipped tests in `DPoPLoginTests` pass locally on Android Studio
  • `generateAuthorizationUrl_WhenUseDPoP_AddsDpopJkt_ToUrl` — URL contains `dpop_jkt=` when `useDPoP=true`
  • `generateAuthorizationUrl_WhenNotUseDPoP_DoesNotAddDpopJkt_ToUrl` — no `dpop_jkt` when `useDPoP=false`
  • `doCodeExchange_WhenPendingCredentialsIdentifierSet_ReusesIt` — same identifier used in both authorize and code exchange
  • `doCodeExchange_WhenNoPendingCredentialsIdentifier_GeneratesNew` — fallback path unaffected
  • `./gradlew :libs:SalesforceSDK:compileDebugKotlin` — clean
  • `./gradlew :libs:test:SalesforceSDKTest:compileDebugAndroidTestKotlin` — clean

…6836)

When useDPoP=true and the login server is a custom/my-domain server,
generate credentialsIdentifier eagerly in generateAuthorizationUrl(),
load the EC key pair, compute the RFC 7638 JWK thumbprint, and add
dpop_jkt=<thumbprint> to the authorization URL parameters.

The identifier is stored as pendingCredentialsIdentifier so
doCodeExchange() reuses it, preserving the key pair binding from
authorize -> token exchange.

Pool servers (login.salesforce.com, test.salesforce.com,
welcome.salesforce.com) are excluded — they don't support DPoP code
binding, matching the same guard already used in
fetchAuthenticationConfiguration() for browser login and app attestation.
@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown
3 Warnings
⚠️ libs/SalesforceSDK/src/com/salesforce/androidsdk/ui/LoginViewModel.kt#L323 - This method should only be accessed from tests or within private scope
⚠️ libs/SalesforceSDK/src/com/salesforce/androidsdk/ui/LoginViewModel.kt#L327 - This method should only be accessed from tests or within private scope
⚠️ libs/SalesforceSDK/src/com/salesforce/androidsdk/ui/LoginViewModel.kt#L331 - This method should only be accessed from tests or within private scope

Generated by 🚫 Danger

… stubs and server URLs (W-23406836)

- Rename helper to addDpopJktIfNeeded for clarity
- Fix DPoP unit tests: use my-domain URL (myorg.my.salesforce.com) instead of
  test.salesforce.com so dpop_jkt is not excluded by the pool-server guard
- Add missing useDPoP stub to two strict (relaxed=false) mock tests

@sfdctaka sfdctaka left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking observations on four edge cases in addDpopJktIfNeeded and doCodeExchange. All happy-path flows look correct; these only bite under specific re-entry / retry sequences. Flagging for awareness, not requesting changes.

Comment thread libs/SalesforceSDK/src/com/salesforce/androidsdk/ui/LoginViewModel.kt Outdated
…erver re-entry (W-23406836)

- On early return (pool server or DPoP disabled), remove any previous dpop_jkt from
  params, delete the orphaned AndroidKeyStore entry, and clear pendingCredentialsIdentifier
  so the two stay in sync
- On my-domain happy path, delete the previous orphaned key before generating a new one
  so repeated server-picker navigation doesn't accumulate unbounded keystore entries

@sfdctaka sfdctaka left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Comment on lines +736 to +738
val isMyDomainServer = server != PRODUCTION_LOGIN_URL
&& server != SANDBOX_LOGIN_URL
&& server != WELCOME_LOGIN_URL

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: I think we do this check in a couple places. Could be worth creating a static utility somewhere (if we don't already have one?).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done here: f9b9d5b

Consolidates the repeated three-way pool-server check
(PRODUCTION / SANDBOX / WELCOME) into a single static method on
LoginServerManager, and updates all three call sites:
- LoginViewModel.addDpopJktIfNeeded
- SalesforceSDKManager.fetchAndUpdateAuthConfigIfNeeded
- SalesforceDroidGapActivity.fetchAuthConfig
@github-actions

github-actions Bot commented Jul 16, 2026

Copy link
Copy Markdown
1 Warning
⚠️ libs/SalesforceHybrid/src/com/salesforce/androidsdk/phonegap/ui/SalesforceDroidGapActivity.kt#L168 - Field requires API level 33 (current min is 31): android.content.Context#RECEIVER_NOT_EXPORTED

Generated by 🚫 Danger

…W-23406836)

Decouples the login surface selection (Custom Tab vs in-app WebView) from
useWebServerFlow. Moves the new parameter after knownUserConfig so existing
positional call sites are unaffected.
@wmathurin

Copy link
Copy Markdown
Contributor Author

@brandonpage what do you think of 7d3b035 ? (It probably could have gone to a separate PR though).

@brandonpage

Copy link
Copy Markdown
Contributor

@brandonpage what do you think of 7d3b035 ? (It probably could have gone to a separate PR though).

Seems fine. I didn't want to over invest in opt-out WebView scenarios, but there is no harm in it.

… AuthFlowTester (W-23406836)

- AndroidManifest: add missing intent filters for ecajwtrtr, ecaopaquertr, ecajwtdpop, ecajwtdpoprtr
- AuthFlowTest: forward forceAdvancedAuthentication as expectAdvancedAuth in loginAndValidate/validateUser
- AuthFlowTest: add expectAdvancedAuth/isDpop/isMultiUser params to assertRevokeAndRefreshWorks, migrateAndValidate, restartAndValidateUser, switchToUserAndValidateUser
- AuthFlowTest: fix adminLoginAndValidate to forward useDPoP to validateUser
- AuthFlowTest: remove stale nonce-change assertion (server issues nonces with 24h TTL, not per-refresh)
- DPoPLoginTests: pass isDpop/isMultiUser at all call sites; ignore RTR+hybrid test (W-22512846); use useHybridAuthToken=false for DPoP→DPoPRtr migration
@wmathurin
wmathurin merged commit dd46b50 into forcedotcom:dpop Jul 16, 2026
6 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants