@W-24269726 fix: Reject incomplete Android DPoP credentials

[wmathurin]

Description

Fails closed when an Android credential declares a DPoP token type without the non-blank credentials identifier required to locate its proof key.

• Rejects incomplete persisted DPoP accounts in ClientManager, allowing the existing corrupt-account recovery flow to remove the unusable account.
• Rejects invalid public request decoration before adding authorization headers.
• Rejects invalid RestClient decoration before OkHttp sends network traffic.
• Preserves case-insensitive DPoP matching, valid DPoP proof generation, and Bearer behavior.
• Does not fall back to Bearer and does not log credential material.

Spec: https://git.soma.salesforce.com/SalesforceMobileSDK/SalesforceMobileSDK-Workspace/pull/111

Resolves

W-24269726

Testing

Tests in the PR

• Manual Tests
• Unit Tests
• Integration Tests

Manual Tests

After merging the latest upstream/dev:

• The repository-wide assembleDebug assembleAndroidTest build passed (718 tasks; 626 executed and 92 up-to-date).
• 89 tests passed on a Pixel 9 Pro XL API 36 emulator across DPoPKeyManagerTest, DPoPRequestDecoratorTest, RestClientDPoPGateTests, ClientManagerTest, and SalesforceSDKManagerClientManagerTest.
• Verified null, empty, whitespace-only, and mixed-case DPoP cases.
• Verified request interception fails before Chain.proceed.
• Verified the SDK corrupt-account path removes the invalid persisted account.
• Verified valid DPoP and Bearer request behavior remains unchanged.

[JohnsonEricAtSalesforce]

The ClientManager.validateUser fix is solid: an account persisted with tokenType == "DPoP" and no credentialsIdentifier now correctly fails validateUser and falls into the existing corrupt-account recovery path — I traced getRestClient's null-client branch through logout(..., showLoginPage = true, reason = CORRUPT_STATE_MSDK) to notifyLogoutComplete to startLoginPage(), and the user ends up signed out and re-prompted to log in rather than stuck half-authenticated. That's the scenario the linked work item describes, and this closes it without falling back to Bearer, matching the PR description.

The other two enforcement points are where I'd like a second look before this merges. applyAuthHeaders and attachProof in DPoPRequestDecorator.kt now call check(...), which throws an unchecked IllegalStateException on an incomplete credential. That's fine when the call happens to run on a thread with a handler ready to catch it, but both functions run inside RestClient.OAuthRefreshInterceptor.intercept, an OkHttp Interceptor. On the async send path (RestClient.sendAsync, which per its own doc comment is the API meant for the UI thread), OkHttp's dispatcher catches the throwable, delivers it to onFailure, and then rethrows it on its own dispatcher thread (confirmed against the actual OkHttp 5.3.2 source pinned by this project — RealCall.AsyncCall.run()'s catch-all block rethrows anything that isn't InterruptedException, after already signaling the callback). I grepped the SDK for a custom Thread.UncaughtExceptionHandler and found none, so that rethrow is very likely an app crash rather than a caught, recoverable error — a different failure mode than the corrupt-account recovery flow the rest of this PR relies on.

I initially assumed this could only be reached via already-corrupted persisted storage, which ClientManager.validateUser would already have screened out before a RestClient is ever handed out. That doesn't hold up, though the exact mechanism is more specific than "migration never invalidates the cache" — migration success does clear RestClient's static lookup map (it re-authenticates the same org/user ID, and since UserAccount.equals() compares only those two fields, handleDuplicateUserAccount treats it as a duplicate login and calls RestClient.clearCaches()). The gap is for a caller that's already holding a RestClient object by direct reference from before the migration — clearing the lookup map doesn't reach into an object another class is still holding. MobileSync's SyncManager does exactly this: it caches a RestClient for the lifetime of its singleton (SyncManager.INSTANCES), and that cache is only invalidated by SyncManager.reset(userAccount), which is called solely from MobileSyncSDKManager.cleanUp() — i.e. on logout/account removal, never on a DPoP migration. Inside that held-over OAuthRefreshInterceptor, tokenType IS mutated on every subsequent 401-triggered refresh (setAuthToken(newAuthToken, newTokenType) picks up the now-"DPoP" value from the live persisted account), but credentialsIdentifier has exactly one assignment site — the constructor — and is never reassigned. So the same actively-used, non-corrupt interceptor object can silently drift into "DPoP tokenType + null identifier" purely as a side effect of a normal token refresh following a supported upgradeToDPoP/downgradeFromDPoP call. Before this PR that combination silently sent a malformed request (the bug the work item reports); after this PR the same combination throws uncaught from inside an interceptor on a code path with no exception handler installed.

I don't think this needs to block the corrupt-persisted-account fix, which is correct and narrower than the enforcement added in the decorator — but I'd like to understand whether the crash risk on the live-migration path is intentional (i.e., "an app that doesn't discard/refetch its RestClient after migration is misusing the API, so failing loudly is fine") or whether it's an unintended side effect of reusing the same check()-based helper across both the already-corrupt case and the just-migrated case.

If the latter, there's already an established idiom in this exact codebase for exactly this situation: RestClient.RefreshTokenRevokedException, ClientManager.MalformedTokenException, and HttpAccess.NoNetworkException all extend IOException rather than being unchecked. That's not incidental — OkHttp's RealCall.AsyncCall.run() special-cases IOException: it's caught and delivered to onFailure without being rethrown, while any other Throwable (including IllegalStateException) falls through to the branch that rethrows onto the dispatcher thread. Making requireCompleteDPoPCredentials throw an IOException subclass instead of using check() would fail closed exactly as intended — the request never leaves the device — while staying inside OkHttp's normal error-delivery path instead of escaping it.

Two smaller notes, neither blocking:

• The three enforcement points now use two different fail-closed mechanisms — a null return in ClientManager.validateUser versus a thrown IllegalStateException in DPoPRequestDecorator. That's a real behavioral difference for a caller (one is "silently gone," the other can crash), not just a style choice — worth a line in the PR description if it's deliberate.
• The new tests in RestClientDPoPGateTests.kt construct a fresh OAuthRefreshInterceptor directly with the incomplete credential already in place; none of them exercise the cache-hit/stale-interceptor path described above (an interceptor built complete, then the account migrated out from under it while the interceptor itself is reused). If the live-migration path above is meant to be supported, that scenario seems worth a regression test; if it's meant to be unsupported usage, it'd still be good to assert the crash explicitly somewhere rather than leave it implicit.

No iOS twin expected; this is Android-specific persisted-account/migration behavior.

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

[wmathurin]

Addressed the review in ea75f47. Incomplete DPoP credentials now throw an IOException subtype instead of IllegalStateException, preserving fail-closed behavior while allowing OkHttp async calls to report the failure through normal onFailure handling rather than rethrowing on the dispatcher thread. Added a regression test for the retained-client migration path: a cached Bearer interceptor refreshes to DPoP without an identifier, makes no replay request, and surfaces IOException. Existing direct decorator/interceptor assertions now verify IOException. Validation: full :libs:SalesforceSDK:build plus assembleAndroidTest passed, and 43 focused emulator tests passed with 0 failures.

[JohnsonEricAtSalesforce]

Verified the fix in ea75f47fc against the actual diff, not just the commit message.

requireCompleteDPoPCredentials no longer uses check() — it throws a new private IncompleteDPoPCredentialsException : IOException(...), which is exactly the fix suggested in the prior round. I re-checked both call sites that matter for this: RestClient.OAuthRefreshInterceptor.intercept (declares throws IOException) and OAuth2.callIdentityService (also throws IOException), so the exception propagates through OkHttp's normal onFailure delivery on the async path instead of being rethrown on the dispatcher thread. DPoPRequestDecorator.applyAuthHeaders — the public entry point, not just the internal attachProof helper — goes through the same private helper, so both surfaces fail closed the same way.

The new test, RestClientDPoPGateTests.test_givenCachedBearerInterceptorMigratesToIncompleteDPoP_whenReplay_thenReportsIoFailure, exercises the specific scenario from the prior review: an interceptor constructed with tokenType = "Bearer" whose authTokenProvider.getTokenType() returns "DPoP" on refresh while credentialsIdentifier stays null — the same tokenType-mutates/credentialsIdentifier-doesn't asymmetry that let a live, non-corrupt interceptor drift into an invalid state after a supported migration. It asserts IOException (not a crash), asserts chain.proceed() is called exactly once (no request leaks through on the failed refresh), and asserts the one attempted request still carried the original Bearer scheme — confirming the interceptor object itself was reused across the simulated migration rather than rebuilt fresh. The two pre-existing direct-construction tests, and the separate DPoPRequestDecoratorTest suite covering applyAuthHeaders, were also updated from IllegalStateException to IOException — so both the interceptor path and the public decorator API are covered consistently.

I grepped for any code that catches IllegalStateException around auth/REST/DPoP paths that might have relied on the old unchecked type — found nothing relevant (the only hits are unrelated: PushMessaging.kt, an identity-service error branch in AuthenticationUtilities.kt, and sample-app UI test helpers). Nothing regresses from the type change.

One non-blocking observation: IncompleteDPoPCredentialsException is private, unlike this codebase's other IOException subclasses (RefreshTokenRevokedException, MalformedTokenException), which are public/package-private specifically so callers like SyncTask can catch the specific subtype and react differently. As written, a caller can only observe "some IOException occurred," not "credentials were incomplete" specifically. That may be intentional (this is meant to look like any other request failure to a consumer), but worth a one-line confirmation in case a caller further up ever wants to distinguish it.

This resolves the crash-risk concern from the previous round. Approving.

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

[brandonpage]

One small observation: IncompleteDPoPCredentialsException is currently defined as private inside DPoPRequestDecorator.kt.

Because it's private, external consumer applications or other internal SDK orchestrators/synchronization engines (such as SyncTask) won't be able to catch this specific exception class. They will only see a generic IOException, meaning they cannot easily differentiate between a local corrupt/incomplete credential state and a standard network issue (like a transient socket timeout) to trigger proactive recovery/telemetry.

If there is any plan or possibility that callers would want to programmatically detect and recover from this state, it might be worth exposing it as internal or package-private (similar to RefreshTokenRevokedException or MalformedTokenException).