In-place upgrade to DPoP + per-call DPoP intent on token migration

[sfdctaka]

What

Adds a public UserAccountManager.upgradeToDPoP(userAccount, onSuccess, onFailure) API that binds an existing Bearer (non-DPoP) session's refresh token to DPoP in place — reusing the user's own consumer key, redirect URI, and scopes, so no re-consent is required. This is the production path for migrating users to DPoP proactively, while the server still accepts the Bearer session, ahead of an ECA flipping to DPoP-enforced.

Supporting changes:

• migrateRefreshToken(...) gains a per-call useDPoP: Boolean? — true binds the migration to DPoP, false migrates unbound, null (default) defers to the global flag. The /authorize dpop_jkt gate (LoginViewModel.addDpopJktIfNeeded) now honors this per-call intent (threaded through TokenMigrationActivity → LoginViewModel.dpopOverride) without changing normal-login gating, which still reads the global flag.
• redirectUri is now persisted on UserAccount (round-trips through JSON, Bundle, and UserAccountManager, mirroring clientId/scope) so an in-place upgrade uses the exact redirect URI captured at login.
• DP (FEATURE_DPOP) user-agent marker is now registered on the token-migration completion path (which bypasses LoginActivity.onAuthFlowSuccess).
• AuthFlowTester sample app: "Upgrade to DPoP" affordance in the migration sheet; new androidTest coverage; LoginViewModel unit tests for the per-call gate.

The global SalesforceSDKManager.useDPoP flag remains the default DPoP posture for new logins only; the migration API is an explicit action on an existing session and is deliberately flag-independent.

Cross-platform note — mirrors iOS

This change mirrors the iOS DPoP flag/gate design. On iOS the DPoP choice is expressed per migration call and the /authorize binding is driven by that per-call intent rather than the process-wide flag; this PR brings Android to the same behavior (per-call useDPoP, flag-independent migration, flag still governs new logins). Two spots specifically match iOS to reach parity:

• redirectUri persisted on the account — iOS reads credentials.redirectUri, a value persisted on the credential at login and never re-resolved. Android previously had no per-user redirect URI, so it's added here to match.
• DP feature marker on migration — iOS writes the DPoP marker from its single auth-completion funnel, so migration gets it for free. Android split completion between interactive login and the migration path, so the marker write is added to the migration branch to match.

The iOS siblings ship as a separate PR in the iOS repo (parallel work).

Changes beyond the original spec/plan (added to make on-device tests pass)

The original story was scoped as a thin wrapper + tests. On-device runs surfaced three issues that required additional changes not in the initial spec/plan; recording them here for review:

1. Per-call param is Boolean? = null, not Boolean = false. A false default silently broke existing migrateRefreshToken callers: the extra was always sent, so an omitted arg forced the /authorize gate to false instead of falling back to the global flag — regressing the pre-existing DPoP-enforced migrate tests (migrated unbound → server rejected → account removed). Fixed by making the param tri-state and only sending the extra when non-null, so absent ⇒ defer to the global flag (verbatim prior behavior).

2. Persist redirectUri on UserAccount. The upgrade initially re-resolved the redirect URI per login-server, but that resolver is temporal — a successful login clears the debug override, so by upgrade time it fell back to the boot-config default and produced a redirect_uri_mismatch (consumer key from one app + redirect URI from another), stalling the migration WebView. Fixed by persisting the redirect URI on the account (as iOS does), with the resolver kept only as a null fallback for accounts persisted before this change.

3. Register the DP marker on the migration path. The migrated session's user agent lacked DP because that marker was only ever written by LoginActivity.onAuthFlowSuccess, which migration bypasses. Added a per-account tokenType == "DPoP" register/unregister in the migration completion branch (matching iOS).

Localization

New user-facing strings added to AuthFlowTester/src/main/res/values/strings.xml (sample app only): upgrade_to_dpop_title, upgrade_to_dpop_description, upgrade_to_dpop_button, change_connected_app_title, and a retitled migrate_app_title.

Review flags

• Touches the OAuth /authorize DPoP gate, credential storage/handling, a new public UserAccountManager API, and login-UI-adjacent sample-app code — requires maintainer sign-off. The migrateRefreshToken param and UserAccount.redirectUri are additive/backward-compatible.

Testing

• :libs:SalesforceSDK:assembleDebug + AuthFlowTester compileDebugAndroidTestKotlin green.
• LoginViewModel unit tests cover the per-call gate (flag off + intent on ⇒ dpop_jkt present; normal login unchanged).
• On-device: testUpgrade_NonDPoP_InPlace_ToDPoP and testLogin_DPoP_ECA_Without_DPoP_Fails pass (require a provisioned ui_test_config.json).

Known pre-existing failure (not caused by this PR)

DPoPLoginTests#testLoginForAdmin_DPoP does not currently pass — it fails to reach the login top bar (sf__more_options_button) during the admin Custom-Tab hand-off. This is unrelated to these changes (reproduces on the base branch) and will be triaged separately.

[github-actions]

	5 Warnings
⚠️	libs/SalesforceSDK/src/com/salesforce/androidsdk/accounts/UserAccountManager.java#L113 - Do not place Android context classes in static fields (static reference to UserAccountManager which has field context pointing to Context); this is a memory leak
⚠️	libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/AuthenticatorService.java#L56 - Do not place Android context classes in static fields (static reference to Authenticator which has field context pointing to Context); this is a memory leak
⚠️	libs/SalesforceSDK/src/com/salesforce/androidsdk/ui/LoginViewModel.kt#L348 - This method should only be accessed from tests or within private scope
⚠️	libs/SalesforceSDK/src/com/salesforce/androidsdk/ui/LoginViewModel.kt#L352 - This method should only be accessed from tests or within private scope
⚠️	libs/SalesforceSDK/src/com/salesforce/androidsdk/ui/LoginViewModel.kt#L356 - This method should only be accessed from tests or within private scope

Generated by 🚫 Danger

[JohnsonEricAtSalesforce]

Thanks for this — the design is solid and the cross-platform parity with the iOS twin (4133) is clear: the tri-state per-call dpopOverride, the in-place upgradeToDPoP convenience, persisting redirectUri on the account, and mirroring the DP feature marker onto the migrated session all line up with the iOS approach. A few points before this is ready to merge — two are blocking.

1. Backward compatibility — migrateRefreshToken signature (blocking).
migrateRefreshToken is a released public API (it shipped in 13.2.0). This PR inserts the new useDPoP: Boolean? = null parameter between appConfig and onMigrationSuccess. Any existing caller that passes the two callbacks positionally — migrateRefreshToken(user, cfg, { … }, { … }) — will now bind its success lambda to useDPoP and fail to compile.

The iOS twin (4133) handled this by keeping migrateRefreshToken:newAppConfig:success:failure: intact and adding a new overload migrateRefreshToken:useDPoP:success:failure:, with the old one delegating to the new with useDPoP: nil. Could we mirror that on Android — either append useDPoP after the two callbacks (keeping = null), or add a separate overload — so existing source keeps compiling? 14.0 is a major so a break is technically permitted, but this is easy to keep compatible, and either way it should be called out in the 14.0 migration notes / release notes.

2. Unit test failures in this PR's blast radius (blocking).
The unit-tests-pr check is red. I reproduced the failures locally on an api36 emulator (running the affected classes directly), and there are two deterministic, PR-specific clusters. Neither test file was updated in this PR, and none of these appear in the current dev nightly baseline (whose SalesforceSDK failures are all network tests — RestClientTest, OAuth2Test.testRefreshAuthToken, HttpAccessTest):

• TokenMigrationActivityTest.onCreate_withClientException_callsErrorCallbackAndFinishes — ClassCastException: androidx.lifecycle.ViewModel_1_Proxy cannot be cast to com.salesforce.androidsdk.ui.LoginViewModel at TokenMigrationActivity.kt:92, reached from the new dpopOverride access in onCreate (line 130). Locally this doesn't just fail the one test — it crashes the instrumentation process ("Test run failed to complete… Process crashed"), which takes down the rest of the shard with it. Worth checking whether reading viewModel earlier in onCreate (to set dpopOverride) changed the initialization ordering the test's ViewModel stub relied on.
• LoginViewModelMockTest.onAuthFlowComplete_CallsAuthenticationUtilitiesSuccessfully, …_WithCorrectParameters, …_UsesEmptyString_WhenSelectedServerIsNull, onAuthFlowComplete_WithTokenMigration_PassesCorrectParameters — all four NPE in UserAccount.hashCode (null userId) via MockK's EqMatcher. These fail deterministically even when the class is run in isolation. The mock/verify blocks enumerate onAuthFlowComplete's parameters explicitly; the PR added redirectUri to that function, so the DSLs need the new parameter (and a non-null userId on the mock account).

Could you get the unit suite green and update these tests as part of the PR? (I'd start with the TokenMigrationActivity crash, since a process crash can mask or destabilize other tests in the same run — see the note below.)

Note on NativeLoginManagerTest (not blocking on its own). CI also flags NativeLoginManagerTest.testShouldShowBackButtonBioAuth and testBiometricAuthenticationUsername, but I could not reproduce these as clean, PR-caused failures: testShouldShowBackButtonBioAuth passes for me every way I ran it, and testBiometricAuthenticationUsername passes in isolation and only fails when run in a batch after the crashing test — i.e. it looks like collateral from the process crash / shared static state, not a defect this PR introduced (the file NativeLoginManager.kt isn't touched here). Once the TokenMigrationActivity crash is fixed, I'd expect these to go green on their own — worth re-confirming after.

3. UI check — please sanity-check one migration test.
The ui-tests-pr failures (testLoginForAdmin_DPoP, LoginForAdminTests.testLoginForAdmin_WebServerFlowEnabled, LoginWithRestartTests.testAdvancedAuth_WithRestart) match the known LoginForAdmin / advanced-auth Custom-Tab harness-flake signature we've seen on the base, so I'm not treating those as blocking. One I'd like you to confirm is genuinely the flake and not a real regression: DPoPLoginTests.testMigrate_ECAJwtDPoP_AddMoreScopes, since it exercises the migration path this PR changes.

Non-blocking / optional

• upgradeToDPoP: the KDoc note that onSuccess/onFailure may be invoked off the main thread is a nice touch. Small thought — the synchronous null-check failure calls onFailure on the caller's thread while the resolution/migration failures call it on Default; a one-line mention that the very first null-check callback is synchronous (vs. the rest async) could save a caller a surprise, but it's already covered well enough.
• The dpopOverride reset in generateMigrationAuthorizationPath is correct, and because the view model is activity-scoped (by viewModels) the "shared view model" leak it guards against can't actually reach a normal login — the comment slightly overstates the risk, but the defensive reset is fine to keep.

This review was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.

[sfdctaka]

@JohnsonEricAtSalesforce thanks for the review — all three points are addressed in 8ea8c9e.

1. migrateRefreshToken backward compatibility. Rather than reorder the parameter, I kept the released 4-param signature exactly as shipped and added a separate overload that carries useDPoP:

// unchanged, released signature — defers to the global useDPoP flag
fun UserAccountManager.migrateRefreshToken(
    userAccount: UserAccount? = ...,
    appConfig: OAuthConfig,
    onMigrationSuccess: (UserAccount) -> Unit,
    onMigrationError: (String, String?, Throwable?) -> Unit,
)

// new overload — explicit per-call DPoP intent
fun UserAccountManager.migrateRefreshToken(
    userAccount: UserAccount? = ...,
    appConfig: OAuthConfig,
    useDPoP: Boolean?,
    onMigrationSuccess: (UserAccount) -> Unit,
    onMigrationError: (String, String?, Throwable?) -> Unit,
)

useDPoP has no default on the overload, so there's no resolution ambiguity, and existing positional callers migrateRefreshToken(user, cfg, {…}, {…}) continue to bind to the original. This is both source- and binary-compatible, and it matches the iOS approach (kept the original selector, added a new one) rather than mutating a released public API.

2. TokenMigrationActivity crash. Moved the viewModel.dpopOverride = … assignment below the early-return error paths, so a failing restClientFactory no longer forces lazy ViewModel init before the error is handled. Verified the RestClient-build error path no longer crashes.

2b. Mock NPEs. Added the new redirectUri = any() argument to all onAuthFlowComplete coEvery/coVerify blocks in LoginViewModelMockTest.

3. testMigrate_ECAJwtDPoP_AddMoreScopes. Confirmed it's the known flake, not a regression — passed on a clean local run.

Local verification on API 36 emulator:

• :libs:SalesforceSDK:compileDebugKotlin + assembleAndroidTest — clean
• LoginViewModelMockTest, TokenMigrationActivityTest, UserAccountManagerMigrateTokenTest — 36 tests, 0 failed
• DPoPLoginTests#testMigrate_ECAJwtDPoP_AddMoreScopes — 1 passed

[JohnsonEricAtSalesforce]

Re-reviewed on the latest push (8ea8c9ec) — all three items from the previous review are resolved and verified. Thanks for the quick turnaround.

1. Backward compatibility — migrateRefreshToken signature. Fixed by splitting into two overloads: the original 4-parameter signature is preserved exactly (userAccount, appConfig, onMigrationSuccess, onMigrationError) and now delegates to a new overload with useDPoP = null; the new overload carries the required useDPoP: Boolean? ahead of the callbacks. Existing positional callers keep compiling. This mirrors the iOS twin's (4133) overload approach as discussed.

2. Unit test failures. Both clusters are fixed and I re-ran them locally on an api36 emulator to confirm:

• TokenMigrationActivityTest.onCreate_withClientException_callsErrorCallbackAndFinishes — the dpopOverride assignment now happens after the early-return error paths, so the error path no longer forces ViewModel initialization. The class runs clean (8/8) with no process crash.
• The four LoginViewModelMockTest.onAuthFlowComplete_* cases — the mock-DSL updates (redirectUri = any() in the verify/coEvery blocks) resolve the failures. Class runs clean (21/21).
• As a side effect, NativeLoginManagerTest also runs clean (21/21) now that the process-crash collateral is gone — confirms those two CI-flagged cases were never PR-caused.

CI's unit-tests-pr check is green on this head.

3. UI check — migration test. Confirmed: DPoPLoginTests.testMigrate_ECAJwtDPoP_AddMoreScopes passes on this head. The ui-tests-pr check still shows red, but the only failures are the known Custom-Tab harness flake ("Username field not found in Custom Tab" at ChromeCustomTabPageObject.setUsername) on three unrelated tests — not a regression from this PR.

No new blocking issues found. Approving.

This review was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.

[sfdctaka]

@wmathurin this was the wrong-user revocation concern from the iOS PR (#4133) — I checked whether it applies here, and the Android path is structurally different, so the bug does not reproduce:

• No currentUser snapshot in the migration path. The only currentUser references this PR adds are default parameter values (userAccount: UserAccount? = getInstance().currentUser), i.e. "use current user if the caller passes none," evaluated at call time. Nothing carries a currentUser snapshot through migration. TokenMigrationActivity resolves the target explicitly via getUserFromOrgAndUserId(orgId, userId) from the intent extras and builds that user's RestClient. There are zero revoke calls in TokenMigrationActivity/LoginViewModel.
• The one old-token revocation is keyed on the matched account, not the current user. It lives in handleDuplicateUserAccount (on the onAuthFlowComplete success path) and revokes duplicateUserAccount.refreshTokenForPersistence, where the duplicate is found via existingUsers.contains(account). UserAccount.equals keys on userId+orgId, so it always revokes the upgraded user's own prior token regardless of who is current.

So upgrading background user A while B is current revokes A's old token, which is correct — no code change needed.

That said, your regression ask is a good guard, so I added one in 0de11e997 (UserAccountManagerMigrateTokenTest#migrateRefreshToken_withNonCurrentUser_targetsPassedUserNotCurrentUser): with B as current user, migrate non-current user A and assert the migration intent carries A's org/user id — not B's. That pins the "operates on the passed-in user, not currentUser" contract at the layer where the iOS bug originated, so a future refactor can't silently reintroduce it. Full class passes locally (8/8) on an API 36 emulator.

[wmathurin]

Same feedback as iOS. Looking at loginAndExpectFailure (which existed before this PR), should we assert for a specific error message (in web view or alert) - if it is possible ?

[sfdctaka]

Good call — and it turns out Android differs from iOS here, so I was able to do what we couldn't on the iOS side.

On iOS the enforced-ECA rejection surfaces through ASWebAuthenticationSession as a QuickLook document preview of a short non-HTML body — there's no error=/error_description text on screen to assert against, so absence-of-main-page is the strongest reliable signal there.

On Android the rejection renders as a real OAuth error page inside the Custom Tab, and UiAutomator can read that text across the process boundary. I confirmed the enforced server returns error=invalid_request&error_description=missing required dpop_jkt for code binding, so I've already gone ahead and added a specific assertion for it.

Changes (latest commit):

• New ChromeCustomTabPageObject.isShowingDPoPBindingError() that matches the rendered dpop_jkt token — I match the distinctive, server-stable substring rather than the full phrase, which is URL-encoded in the page text and could be reworded server-side.
• Opt-in expectDPoPBindingError flag on loginAndExpectFailure (default false), wired only into testLogin_DPoP_ECA_Without_DPoP_Fails. The invalid-consumer-key / invalid-scope negative tests produce different errors, so they keep the generic baseline.

Verified on-device against the live enforced ECA: passes with the real matcher, and I ran a negative control (matcher pointed at an impossible string) to confirm the assertion actually fails when the text is absent — i.e. it's not a vacuous assert() no-op.