W-19758940: Improve ClientManager for multi-user - #2973
Conversation
Generated by 🚫 Danger |
Generated by 🚫 Danger |
Generated by 🚫 Danger |
| } | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
This really should never have been an option. It contradicts our security posture and there is no iOS equivalent.
| @@ -206,13 +201,6 @@ | |||
| /** Additional Auth Values used for login. */ | |||
There was a problem hiding this comment.
This really should not have survived the 13.0 login modernization as I believe magic links are completely unused.
JohnsonEricAtSalesforce
left a comment
JohnsonEricAtSalesforce
left a comment
There was a problem hiding this comment.
Review summary
Reviewed the full diff (52 files, +3,148/−1,874) at HEAD 500c7bd8f, with a focus on the four areas this change touches that carry the most risk in a public SDK: OAuth token refresh/credential handling, public-API compatibility, multi-user account isolation, and localization. I also traced the concurrency logic at runtime and read the CI results from the underlying artifacts.
Verdict: Approve. This is a careful, well-structured redesign that delivers exactly what W-19758940 asks for — binding ClientManager and AccMgrAuthTokenProvider to a specific persisted user instead of implying identity from the current user at creation time. The behavior is thoroughly covered by new tests, and I found no correctness defects.
What I verified:
-
Public API (14.0 boundary). The removals from
ClientManager(old(Context, String, boolean)constructor,getRestClient,peekRestClient(Account/UserAccount),createNewAccount,getAccounts, etc.) are deliberate and complete — every non-test call site now correctly targets the relocatedSalesforceSDKManagermethods, andMobileSdk14ApiSurfaceTestguards the intended surface precisely. The token-snapshotAccMgrAuthTokenProviderconstructor is retained as@Deprecatedfor migration. Nullable returns (clientManager,getAccount(),peekRestClient()) are annotated and callers fail closed. -
Multi-user isolation.
logout(Account, …)now targets the exact persisted account with per-account dedup and guaranteed local credential removal, with push-unregister/token-revoke/DPoP-key cleanup as best-effort.isLoggingOutis now tracked per-account. The push path is a genuine fix: deregistration can no longer widen to all users (guarded at both enqueue and execution), and it uses a per-user unique WorkManager name so one user's logout can't replace another's pending work. The Hybrid activity now enforces a user-binding consistency check across its lifecycle so a REST callback returning after a user switch can't act on the wrong account. -
Refresh coordination (RTR). I traced the new per-
(userId:orgId)winner/loser election against each failure mode I could construct — lost-winner deadlock, a loser adopting a failed refresh, stale-token replay, RTR double-rotation, account removal mid-refresh, and a loser losing its own instance URL. Each is handled correctly, and each has a corresponding deterministic test inClientManagerMockTest(latch-based, noThread.sleep). -
Localization. The single removed string (
sf__jwt_authentication_error) has its only usage removed in the same change and was English-only, so nothing is orphaned.
Non-blocking notes:
- Release-notes / migration. Since this removes and relocates public API at the 14.0 boundary, it'd be worth calling out the
ClientManagerconstructor/method removals and theSalesforceSDKManagerrelocations explicitly in the 14.0 migration notes for external consumers. The removed user-facing string is also worth a mention. - CI —
ui-tests-pr. The redui-tests-prcheck is the known AuthFlowTester Custom Tab / Firebase Test Lab flake, not a regression from this PR: the failures are all in external Custom-Tab login-page / Compose-launch interaction (Username field not found in Custom Tab,No compose hierarchies found) across six unrelated test classes with 11 flaky retries, and the same job fails with the same signature on unrelated branches (e.g.dpop,fix-manage-space-clear-data) in the same window. All unit suites pass (SalesforceSDK 999/999, MobileSync 251/251, SalesforceHybrid 155/155). Recommend re-runningui-tests-pr.
This review was generated by an AI agent on behalf of @JohnsonEricAtSalesforce.
| if (user != null) { | ||
| ClientManager(appContext, user).peekRestClient()?.let { client -> | ||
| restClientCallback.authenticatedRestClient(client) | ||
| } ?: w(TAG, "Unable to create a REST client for the current user") |
There was a problem hiding this comment.
[High] Silent hang when a persisted user's peekRestClient() returns null
When a current user exists but peekRestClient() returns null (account being logged out, missing tokens, malformed record), the callback is never invoked and login is never started. The old ClientManager.getRestClient always either called the callback or kicked off LoginActivity.
The consequence is visible in SalesforceActivityDelegate — the client == null guard that triggered CORRUPT_STATE_MSDK logout is now dead code because RestClientCallback only ever receives a non-null RestClient. An app with a malformed-but-present account silently hangs at resume with no recovery path.
Suggestion: when peekRestClient() returns null for a present user, either start LoginActivity (same as the no-user path) or make the callback contract allow null so callers can trigger CORRUPT_STATE_MSDK logout.
There was a problem hiding this comment.
I am opting cleanup and logout. I think never returning a null client better fits the design/direction I have stated with the PR. Moreover, returning a null client does very little for the host app. Sure, they would get a chance to do something with the malformed user before logout but without a rest client they can't log the error or upload unsaved data so it is pointless as far as I can tell.
| SalesforceSDKManager.getInstance().getRestClient( | ||
| activity, | ||
| client -> { | ||
| if (client == null) { |
There was a problem hiding this comment.
[High] Dead null-check — CORRUPT_STATE_MSDK logout is now unreachable
RestClientCallback (the new fun interface) is only ever invoked with a non-null RestClient, so this branch can never execute. The CORRUPT_STATE_MSDK recovery path has been silently dropped — see the note on getRestClient above. This guard should either be removed (with a fix upstream) or the callback contract needs to allow null to preserve the path.
| }.onFailure { error -> | ||
| e(TAG, "Removing the persisted account failed", error) | ||
| }.getOrDefault(false) | ||
| if (!removed) { |
There was a problem hiding this comment.
[High] No cleanup when removeAccountExplicitly returns false
If platform account removal fails (SecurityException swallowed by runCatching, race with the system account manager, etc.) this early return skips cleanUp, clearWebViewCookiesAfterLogout, notifyLogoutComplete, and token revocation. finishLogout still runs in the outer finally so the account leaves loggingOutAccounts, but all SDK in-memory state is left dirty and the user appears stuck logged in.
purgeMalformedPersistedAccount wraps each step in its own runCatching and proceeds regardless — removeAccount should do the same rather than hard-stopping here.
| val client = clientManager?.peekRestClient() | ||
| if (client == null) { | ||
| SalesforceSDKLogger.e(TAG, "Unable to obtain the authenticated client while unlocking.") | ||
| return |
There was a problem hiding this comment.
[Medium] Activity not finished when client is null during biometric unlock
The old code flowed through clientManager.getRestClient(activity, callback) which always ended with activity.finish(). Now if peekRestClient() returns null this method logs an error and returns, leaving LoginActivity open indefinitely with the user stuck on the lock screen.
At minimum activity.finish() should be called on the null path.
| activity.finish() | ||
| val client = SalesforceSDKManager.getInstance().clientManager?.peekRestClient() | ||
| if (client == null) { | ||
| e(TAG, "Unable to obtain the authenticated client while unlocking.") |
There was a problem hiding this comment.
[Medium] Activity not finished when client is null during token-refresh unlock
Same issue as NativeLoginManager.onBiometricAuthenticationSucceeded: the old getRestClient path always ended with activity.finish(). This path returns early on a null client without finishing, leaving LoginActivity permanently open on the lock screen.
| @VisibleForTesting | ||
| ClientManager(@NonNull AccountManager accountManager, | ||
| @NonNull Account account, | ||
| @NonNull UserAccount user) { |
There was a problem hiding this comment.
[Low] Unused 'user' parameter in @VisibleForTesting constructor
The 'user' parameter is accepted but never read — this.account is set directly from the 'account' parameter. The omission is presumably intentional (to skip the buildAccount lookup in tests) but is silently surprising. Either remove the parameter so callers supply a pre-resolved Account directly, or add a brief comment explaining why it is ignored.
…anager-multi-user # Conflicts: # libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java
…anager-multi-user # Conflicts: # libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java # libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt
| return if (action == Register) { | ||
| TargetAccounts.Accounts(userAccountManager.authenticatedUsers.orEmpty()) | ||
| } else { | ||
| TargetAccounts.Fail |
There was a problem hiding this comment.
failure() here is permanent — WorkManager will not retry. The comment above says "safe because the SDK re-enqueues registration work on the next foreground," but that only covers registration. A deregistration that hits Fail is silently abandoned: the server-side MobilePushServiceDevice record persists until TTL or a future explicit deregister. Per the spec this is accepted best-effort, but the comment should say so.
Also: performRegistrationChange has a parallel skip path (restClient ?: return) in doWork() that falls through to return success() rather than failure(). Two can't-do-the-work cases, two different WorkManager outcomes. At minimum the null-client skip should log at warning level so silent no-op deregistrations are distinguishable from successful ones.
3 participants
Summary
ClientManagerto one persistedUserAccount, and keep retained managers and clients on that user after current-user switchesAccMgrAuthTokenProviderinherit manager identity, read live persisted credentials, and coordinate refresh per Salesforce user so RTR cannot redirect refresh or logout workPublic API impact
ClientManager(Context, String, boolean)withClientManager(Context, UserAccount)SalesforceSDKManager.clientManager,ClientManager.getAccount(), andClientManager.peekRestClient()nullable when no usable persisted account existsAccMgrAuthTokenProviderconstruction, including a routing-only instance URL overload; retain the token-snapshot overload as deprecated for 14.0 migrationSalesforceSDKManagerLoginViewModelstateValidation
ClientManagerTest: 16/16 passed after final fail-closed URL guardsWork item
W-19758940 — Improve ClientManager for Multi-User