From 1cd50a52b62f4ccf542d6e1576c2657e15c0dee3 Mon Sep 17 00:00:00 2001 From: Wolfgang Mathurin Date: Tue, 22 Sep 2026 15:18:57 -0700 Subject: [PATCH 1/4] @W-24269724: Clean up Android refresh state on logout --- .../androidsdk/app/SalesforceSDKManager.kt | 3 ++ .../androidsdk/rest/ClientManager.java | 31 ++++++++++++++ .../androidsdk/rest/ClientManagerInternal.kt | 5 +++ .../SalesforceSDKManagerClientManagerTest.kt | 25 +++++++++++ .../androidsdk/rest/ClientManagerMockTest.kt | 35 ++++++++++++++++ .../androidsdk/rest/RefreshStateTestAccess.kt | 42 +++++++++++++++++++ 6 files changed, 141 insertions(+) create mode 100644 libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RefreshStateTestAccess.kt diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/app/SalesforceSDKManager.kt b/libs/SalesforceSDK/src/com/salesforce/androidsdk/app/SalesforceSDKManager.kt index 300d5e0e62..41cc1b94dd 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/app/SalesforceSDKManager.kt +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/app/SalesforceSDKManager.kt @@ -146,6 +146,7 @@ import com.salesforce.androidsdk.rest.ClientManager import com.salesforce.androidsdk.rest.NotificationsActionsResponseBody import com.salesforce.androidsdk.rest.NotificationsApiClient import com.salesforce.androidsdk.rest.RestClient +import com.salesforce.androidsdk.rest.clearRefreshStateForUser import com.salesforce.androidsdk.rest.peekRestClientWithResolvedUser import com.salesforce.androidsdk.security.BiometricAuthenticationManager import com.salesforce.androidsdk.security.SalesforceKeyGenerator @@ -1049,6 +1050,8 @@ open class SalesforceSDKManager protected constructor( cachedClientManager = null userAccount?.let { userAccountResolved -> + clearRefreshStateForUser(userAccountResolved) + /* * Drops this user's persisted feature markers so a later login * as the same identity starts from an empty set rather than diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java index dd6f5eac82..1c9fee67c5 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java @@ -308,6 +308,27 @@ static void resetRefreshStateForTest() { REFRESH_STATES.clear(); } + /** + * Removes refresh coordination data for an account whose SDK session is ending. + * Threads that already hold the removed state can finish safely, while future refreshes + * for the same identity start with a fresh coordination state. + */ + static void clearRefreshState(UserAccount user) { + if (user != null) { + REFRESH_STATES.remove(refreshStateKeyFor(user)); + } + } + + @VisibleForTesting + static void createRefreshStateForTest(UserAccount user) { + REFRESH_STATES.put(refreshStateKeyFor(user), new RefreshState()); + } + + @VisibleForTesting + static boolean hasRefreshStateForTest(UserAccount user) { + return REFRESH_STATES.containsKey(refreshStateKeyFor(user)); + } + /** Bounded safety-net so a loser never parks forever if a winner is somehow lost. */ private static final long LOSER_WAIT_TIMEOUT_MILLIS = 30_000L; @@ -424,6 +445,16 @@ public String getNewAuthToken() { // broadcasting. final RefreshState state = REFRESH_STATES.computeIfAbsent( refreshStateKey, k -> new RefreshState()); + + // Account cleanup can race between the validation above and computeIfAbsent. Recheck + // after insertion so a refresh that lost its account cannot recreate an entry after + // logout removed it. Compare-removal avoids deleting a replacement state installed by + // a later session. + if (clientManager.getValidatedUser(/* requireRefreshFields = */ true) == null) { + REFRESH_STATES.remove(refreshStateKey, state); + return null; + } + synchronized (state.lock) { if (state.refreshing) { // Snapshot the publish generation BEFORE waiting. We adopt on a generation diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManagerInternal.kt b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManagerInternal.kt index b659e22b9c..f173c7903d 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManagerInternal.kt +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManagerInternal.kt @@ -41,3 +41,8 @@ import com.salesforce.androidsdk.accounts.UserAccount internal fun ClientManager.peekRestClientWithResolvedUser( user: UserAccount, ): RestClient? = peekRestClient(user) + +/** Removes process-local refresh coordination data when an account session ends. */ +internal fun clearRefreshStateForUser(user: UserAccount) { + ClientManager.AccMgrAuthTokenProvider.clearRefreshState(user) +} diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/app/SalesforceSDKManagerClientManagerTest.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/app/SalesforceSDKManagerClientManagerTest.kt index 35d244d339..30f467fb94 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/app/SalesforceSDKManagerClientManagerTest.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/app/SalesforceSDKManagerClientManagerTest.kt @@ -37,6 +37,7 @@ import com.salesforce.androidsdk.accounts.UserAccountBuilder import com.salesforce.androidsdk.auth.AuthenticatorService import com.salesforce.androidsdk.auth.HttpAccess import com.salesforce.androidsdk.rest.ClientManager +import com.salesforce.androidsdk.rest.RefreshStateTestAccess import com.salesforce.androidsdk.rest.RestClient import com.salesforce.androidsdk.ui.LoginActivity import io.mockk.CapturingSlot @@ -282,6 +283,30 @@ class SalesforceSDKManagerClientManagerTest { assertManagerBoundTo(freshManager, user) } + @Test + fun logout_removesOnlyDepartingAccountsRefreshState() { + val userA = persistUser("refresh-state-a") + val accountA = requireNotNull(userAccountManager.buildAccount(userA)) + val userB = persistUser("refresh-state-b") + RefreshStateTestAccess.create(userA) + RefreshStateTestAccess.create(userB) + + try { + sdkManager.logout(accountA, null, false) + + assertFalse( + "Logout must remove refresh coordination state for the departing account", + RefreshStateTestAccess.contains(userA), + ) + assertTrue( + "Logout must preserve refresh coordination state for other accounts", + RefreshStateTestAccess.contains(userB), + ) + } finally { + RefreshStateTestAccess.clear(userB) + } + } + @Test fun clientManager_afterLogoutAndReloginAsSameIdentity_startsWithClearedFeatureMarkers() { /* diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt index d41471e756..af7a8552ca 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt @@ -976,6 +976,41 @@ class ClientManagerMockTest { verify(exactly = 0) { mockSDKManager.logout(any(), any(), any(), any()) } } + @Test + fun testClearRefreshState_RemovesOnlyMatchingAccount() { + val userA = testUser(userId = "user-a", orgId = "org-a") + val userB = testUser(userId = "user-b", orgId = "org-b") + ClientManager.AccMgrAuthTokenProvider.createRefreshStateForTest(userA) + ClientManager.AccMgrAuthTokenProvider.createRefreshStateForTest(userB) + + ClientManager.AccMgrAuthTokenProvider.clearRefreshState(userA) + ClientManager.AccMgrAuthTokenProvider.clearRefreshState(userA) + + assertFalse( + ClientManager.AccMgrAuthTokenProvider.hasRefreshStateForTest(userA), + ) + assertTrue( + ClientManager.AccMgrAuthTokenProvider.hasRefreshStateForTest(userB), + ) + } + + @Test + fun testGetNewAuthToken_AccountRemovedBeforeStateElection_DoesNotRecreateState() { + val account = mockk(relaxed = true) + val user = testUser(userId = "removed-user", orgId = "removed-org") + val validationCalls = AtomicInteger(0) + val manager = mockk(relaxed = true) { + every { getAccount() } returns account + every { getValidatedUser(any()) } answers { + if (validationCalls.getAndIncrement() < 2) user else null + } + } + val provider = ClientManager.AccMgrAuthTokenProvider(manager) + + assertNull(provider.getNewAuthToken()) + assertFalse(ClientManager.AccMgrAuthTokenProvider.hasRefreshStateForTest(user)) + } + @Test fun testGetNewAuthToken_AccountRemovedDuringSuccessfulRefresh_DiscardsResponse() { assertInFlightRemovalSuppressesSideEffects(successResponse(ROTATED_REFRESH_TOKEN)) diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RefreshStateTestAccess.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RefreshStateTestAccess.kt new file mode 100644 index 0000000000..8e5602b706 --- /dev/null +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/RefreshStateTestAccess.kt @@ -0,0 +1,42 @@ +/* + * Copyright (c) 2026-present, salesforce.com, inc. + * All rights reserved. + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * - Redistributions of source code must retain the above copyright notice, + * this list of conditions and the following disclaimer. + * - Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * - Neither the name of salesforce.com, inc. nor the names of its contributors + * may be used to endorse or promote products derived from this software + * without specific prior written permission. + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE + * POSSIBILITY OF SUCH DAMAGE. + */ +package com.salesforce.androidsdk.rest + +import com.salesforce.androidsdk.accounts.UserAccount + +/** Test-only access to package-private refresh coordination state helpers. */ +internal object RefreshStateTestAccess { + fun create(user: UserAccount) { + ClientManager.AccMgrAuthTokenProvider.createRefreshStateForTest(user) + } + + fun clear(user: UserAccount) { + ClientManager.AccMgrAuthTokenProvider.clearRefreshState(user) + } + + fun contains(user: UserAccount): Boolean = + ClientManager.AccMgrAuthTokenProvider.hasRefreshStateForTest(user) +} From c48ba9e80f8c60232ab7ee88acf8ffb1cee9c0b2 Mon Sep 17 00:00:00 2001 From: Wolfgang Mathurin Date: Tue, 22 Sep 2026 20:06:19 -0700 Subject: [PATCH 2/4] @W-24269724: Preserve shared refresh coordination on revalidation --- .../salesforce/androidsdk/rest/ClientManager.java | 9 +++++---- .../androidsdk/rest/ClientManagerMockTest.kt | 14 +++++++++++--- 2 files changed, 16 insertions(+), 7 deletions(-) diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java index 1c9fee67c5..7ffa83b0f8 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java @@ -447,11 +447,12 @@ public String getNewAuthToken() { refreshStateKey, k -> new RefreshState()); // Account cleanup can race between the validation above and computeIfAbsent. Recheck - // after insertion so a refresh that lost its account cannot recreate an entry after - // logout removed it. Compare-removal avoids deleting a replacement state installed by - // a later session. + // after acquiring the shared state so a refresh that lost its account fails closed + // before making a token request. Do not unlink the state here: another provider for a + // newly restored session may already be using the same object. Normal account cleanup + // owns map removal; this rare race may retain one bounded stale entry until the next + // cleanup rather than splitting refresh coordination across two live states. if (clientManager.getValidatedUser(/* requireRefreshFields = */ true) == null) { - REFRESH_STATES.remove(refreshStateKey, state); return null; } diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt index af7a8552ca..1649231b4f 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt @@ -995,7 +995,7 @@ class ClientManagerMockTest { } @Test - fun testGetNewAuthToken_AccountRemovedBeforeStateElection_DoesNotRecreateState() { + fun testGetNewAuthToken_AccountRemovedBeforeRefresh_FailsClosedWithoutUnlinkingSharedState() { val account = mockk(relaxed = true) val user = testUser(userId = "removed-user", orgId = "removed-org") val validationCalls = AtomicInteger(0) @@ -1007,8 +1007,16 @@ class ClientManagerMockTest { } val provider = ClientManager.AccMgrAuthTokenProvider(manager) - assertNull(provider.getNewAuthToken()) - assertFalse(ClientManager.AccMgrAuthTokenProvider.hasRefreshStateForTest(user)) + try { + assertNull(provider.getNewAuthToken()) + assertTrue( + "A failed revalidation must not unlink state another provider may already share", + ClientManager.AccMgrAuthTokenProvider.hasRefreshStateForTest(user), + ) + verify(exactly = 0) { mockOkHttpClient.newCall(any()) } + } finally { + ClientManager.AccMgrAuthTokenProvider.clearRefreshState(user) + } } @Test From e4d67cdb9d39e12a6a52485e1db47c152949f43e Mon Sep 17 00:00:00 2001 From: Wolfgang Mathurin Date: Tue, 22 Sep 2026 21:27:06 -0700 Subject: [PATCH 3/4] @W-24269724: Defer active refresh-state cleanup --- docs/auth/token-lifecycle.md | 11 ++ .../androidsdk/rest/ClientManager.java | 133 ++++++++++++++---- .../androidsdk/rest/ClientManagerMockTest.kt | 103 ++++++++++++++ 3 files changed, 221 insertions(+), 26 deletions(-) diff --git a/docs/auth/token-lifecycle.md b/docs/auth/token-lifecycle.md index 715b2e9e99..9214d1f04c 100644 --- a/docs/auth/token-lifecycle.md +++ b/docs/auth/token-lifecycle.md @@ -135,6 +135,8 @@ static final ConcurrentHashMap REFRESH_STATES class RefreshState { final Object lock // coordination primitive + int activeCallers // callers holding a lifecycle lease + boolean cleanupRequested // logout has scrubbed and retired this state boolean refreshing // true while winner is in-flight long publishGeneration // incremented only on successful publish String newAuthToken // last successfully refreshed token @@ -145,6 +147,13 @@ class RefreshState { } ``` +Each caller acquires a lifecycle lease before using the state and releases it on every exit +path. Logout immediately marks the state for cleanup and scrubs its published access token, +refresh token, instance URL, and token type. An idle state is removed immediately. If callers are +still active, the scrubbed state remains mapped until the final lease is released; new callers +that encounter it fail closed. This prevents a quick same-identity login from creating a second +coordinator while an old-session refresh is still in flight. + ### Flow ``` @@ -273,6 +282,8 @@ therefore share credential state but do not perform network I/O while holding th ### Logout `SalesforceSDKManager.removeAccount()` calls: +- `AccMgrAuthTokenProvider.clearRefreshState(user)` — scrubs published refresh results and removes + the per-account coordinator immediately or after its last active lifecycle lease is released - `DPoPKeyManager.deleteKeyPair(alias)` — evicts the process-local handle and attempts to destroy the EC keypair from the Android Keystore - `DPoPNonceCache.clear(credentialsIdentifier)` — evicts cached nonces for this session diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java index 7ffa83b0f8..4761ca7ffc 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java @@ -286,6 +286,10 @@ private static final class RefreshState { // object (rather than synchronizing on the RefreshState reference itself) makes the // intent explicit and avoids the "synchronization on local variable" inspection. final Object lock = new Object(); + // Lifecycle leases prevent logout cleanup from unlinking this state while a caller + // can still use it. All lifecycle fields are guarded by lock. + int activeCallers; + boolean cleanupRequested; boolean refreshing; // Incremented once per successful publish. Waiting losers adopt only when this edge // advances, so a failed refresh cannot be mistaken for a usable result. @@ -309,13 +313,67 @@ static void resetRefreshStateForTest() { } /** - * Removes refresh coordination data for an account whose SDK session is ending. - * Threads that already hold the removed state can finish safely, while future refreshes - * for the same identity start with a fresh coordination state. + * Clears refresh coordination data for an account whose SDK session is ending. Published + * credentials are scrubbed immediately. An idle state is removed immediately; a leased + * state remains mapped until its last caller exits so a same-identity login cannot create + * a second refresh coordinator while an old-session refresh is still in flight. */ static void clearRefreshState(UserAccount user) { - if (user != null) { - REFRESH_STATES.remove(refreshStateKeyFor(user)); + if (user == null) { + return; + } + final String refreshStateKey = refreshStateKeyFor(user); + final RefreshState state = REFRESH_STATES.get(refreshStateKey); + if (state == null) { + return; + } + synchronized (state.lock) { + if (REFRESH_STATES.get(refreshStateKey) != state) { + return; + } + state.cleanupRequested = true; + state.newAuthToken = null; + state.newInstanceUrl = null; + state.rotatedRefreshToken = null; + state.newTokenType = null; + state.lastRefreshTime = -1; + state.lock.notifyAll(); + if (state.activeCallers == 0) { + REFRESH_STATES.remove(refreshStateKey, state); + } + } + } + + /** + * Acquires a lifecycle lease on the current per-account state. A state undergoing cleanup + * deliberately rejects new callers so they fail closed until every old-session caller has + * drained and the state can be removed without splitting coordination. + */ + @Nullable + private static RefreshState acquireRefreshState(String refreshStateKey) { + while (true) { + final RefreshState state = REFRESH_STATES.computeIfAbsent( + refreshStateKey, k -> new RefreshState()); + synchronized (state.lock) { + if (state.cleanupRequested) { + return null; + } + if (REFRESH_STATES.get(refreshStateKey) != state) { + continue; + } + state.activeCallers++; + return state; + } + } + } + + /** Releases a lifecycle lease and completes deferred cleanup for the last caller. */ + private static void releaseRefreshState(String refreshStateKey, RefreshState state) { + synchronized (state.lock) { + state.activeCallers--; + if (state.cleanupRequested && state.activeCallers == 0) { + REFRESH_STATES.remove(refreshStateKey, state); + } } } @@ -443,20 +501,32 @@ public String getNewAuthToken() { // Losers wait (looping on the condition to absorb spurious/lost wakeups) for the // winner's published result and adopt it without re-attempting, logging out, or // broadcasting. - final RefreshState state = REFRESH_STATES.computeIfAbsent( - refreshStateKey, k -> new RefreshState()); - - // Account cleanup can race between the validation above and computeIfAbsent. Recheck - // after acquiring the shared state so a refresh that lost its account fails closed - // before making a token request. Do not unlink the state here: another provider for a - // newly restored session may already be using the same object. Normal account cleanup - // owns map removal; this rare race may retain one bounded stale entry until the next - // cleanup rather than splitting refresh coordination across two live states. + final RefreshState state = acquireRefreshState(refreshStateKey); + if (state == null) { + return null; + } + try { + return refreshWithState(state, matchingAccount); + } finally { + releaseRefreshState(refreshStateKey, state); + } + } + + private String refreshWithState(RefreshState state, Account matchingAccount) { + + // Account cleanup can race between the validation above and lifecycle-lease + // acquisition. Recheck after acquiring the shared state so a refresh that lost its + // account fails closed before making a token request. Do not unlink the state here: + // another provider for a newly restored session may already be using the same object. + // Normal account cleanup owns map removal. if (clientManager.getValidatedUser(/* requireRefreshFields = */ true) == null) { return null; } synchronized (state.lock) { + if (state.cleanupRequested) { + return null; + } if (state.refreshing) { // Snapshot the publish generation BEFORE waiting. We adopt on a generation // change (an edge), not on refreshing becoming false (a level). If a later @@ -469,19 +539,23 @@ public String getNewAuthToken() { // Loop until a result is published or the in-flight refresh ends without // one. The generation guard absorbs spurious and lost wakeups; the deadline // prevents a lost winner from parking this caller forever. - while (state.refreshing && state.publishGeneration == startGeneration) { + while (!state.cleanupRequested + && state.refreshing + && state.publishGeneration == startGeneration) { final long timeRemaining = deadline - System.currentTimeMillis(); if (timeRemaining <= 0) { break; } state.lock.wait(timeRemaining); } - published = state.publishGeneration != startGeneration; + published = !state.cleanupRequested + && state.publishGeneration != startGeneration; } catch (InterruptedException e) { SalesforceSDKLogger.w(TAG, "Interrupted while waiting for in-flight token refresh", e); Thread.currentThread().interrupt(); - if (state.publishGeneration != startGeneration + if (!state.cleanupRequested + && state.publishGeneration != startGeneration && tryAdoptWinnerResult(state)) { return state.newAuthToken; } @@ -669,20 +743,18 @@ && tryAdoptWinnerResult(state)) { } catch (Exception e) { SalesforceSDKLogger.w(TAG, "Exception thrown while getting auth token", e); } finally { - // Keep the attempted result in this provider, but only successful refresh or - // storage-adoption paths count as a completed refresh. - lastNewAuthToken = newAuthToken; - lastNewInstanceUrl = newInstanceUrl; - lastTokenType = newTokenType; - if (newAuthToken != null) { - lastRefreshTime = System.currentTimeMillis(); - } // Publish the result to the per-account state and wake any waiting losers. // This is the SINGLE publish path and ALWAYS runs on every winner exit path so // losers never wait forever and never wake without a definitive result. synchronized (state.lock) { state.refreshing = false; - if (newAuthToken != null) { + if (state.cleanupRequested) { + // Logout won the state-lock race. Do not republish credentials into the + // scrubbed state or return an old-session token to the caller. + newAuthToken = null; + newInstanceUrl = null; + newTokenType = null; + } else if (newAuthToken != null) { state.newAuthToken = newAuthToken; state.newInstanceUrl = newInstanceUrl; state.rotatedRefreshToken = this.refreshToken; @@ -701,6 +773,15 @@ && tryAdoptWinnerResult(state)) { // indefinitely stale retained value. state.lock.notifyAll(); } + // Keep the attempted result in this provider, but only successful refresh or + // storage-adoption paths count as a completed refresh. This runs after the cleanup + // check above so an old-session result cannot survive cleanup in the provider. + lastNewAuthToken = newAuthToken; + lastNewInstanceUrl = newInstanceUrl; + lastTokenType = newTokenType; + if (newAuthToken != null) { + lastRefreshTime = System.currentTimeMillis(); + } } return newAuthToken; } diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt index 1649231b4f..4feae8813f 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt @@ -994,6 +994,109 @@ class ClientManagerMockTest { ) } + @Test + fun testClearRefreshState_DefersRemovalUntilInFlightCallerDrains() { + val userId = "same-user" + val orgId = "same-org" + val oldRefreshToken = "old-session-refresh-token" + val newRefreshToken = "new-session-refresh-token" + val oldFixture = boundFixture( + refreshToken = oldRefreshToken, + userId = userId, + orgId = orgId, + ) + val oldUser = oldFixture.liveUser.get()!! + val oldRequestStarted = CountDownLatch(1) + val releaseOldRequest = CountDownLatch(1) + val tokenEndpointCalls = AtomicInteger(0) + val activeRequests = AtomicInteger(0) + val maxConcurrentRequests = AtomicInteger(0) + val postedRefreshTokens = arrayOfNulls(3) + + every { HttpAccess.DEFAULT.okHttpClient } returns mockk { + every { newCall(any()) } answers { + val request = firstArg() + mockk { + every { execute() } answers { + val callIndex = tokenEndpointCalls.getAndIncrement() + postedRefreshTokens[callIndex] = postedRefreshToken(request) + val inFlight = activeRequests.incrementAndGet() + maxConcurrentRequests.set( + maxOf(maxConcurrentRequests.get(), inFlight), + ) + try { + if (callIndex == 0) { + oldRequestStarted.countDown() + releaseOldRequest.await(5, TimeUnit.SECONDS) + successResponse( + "old-session-rotated-refresh-token", + accessToken = "old-session-refreshed-access-token", + userId = userId, + orgId = orgId, + ) + } else { + successResponse( + "new-session-rotated-refresh-token", + accessToken = "new-session-refreshed-access-token", + userId = userId, + orgId = orgId, + ) + } + } finally { + activeRequests.decrementAndGet() + } + } + } + } + } + + val oldProvider = ClientManager.AccMgrAuthTokenProvider(oldFixture.manager) + val oldResult = AtomicReference() + val oldThread = Thread { oldResult.set(oldProvider.getNewAuthToken()) } + oldThread.start() + assertTrue(oldRequestStarted.await(5, TimeUnit.SECONDS)) + + oldFixture.liveUser.set(null) + ClientManager.AccMgrAuthTokenProvider.clearRefreshState(oldUser) + assertTrue( + "An active state must remain mapped until its lifecycle lease is released", + ClientManager.AccMgrAuthTokenProvider.hasRefreshStateForTest(oldUser), + ) + + val newFixture = boundFixture( + refreshToken = newRefreshToken, + userId = userId, + orgId = orgId, + ) + val newProvider = ClientManager.AccMgrAuthTokenProvider(newFixture.manager) + val blockedReloginResult = AtomicReference() + val blockedReloginThread = Thread { + blockedReloginResult.set(newProvider.getNewAuthToken()) + } + blockedReloginThread.start() + blockedReloginThread.join(TimeUnit.SECONDS.toMillis(5)) + + assertFalse("Relogin refresh thread did not finish", blockedReloginThread.isAlive) + assertNull(blockedReloginResult.get()) + assertEquals(1, tokenEndpointCalls.get()) + + releaseOldRequest.countDown() + oldThread.join(TimeUnit.SECONDS.toMillis(5)) + + assertFalse("Old-session refresh thread did not finish", oldThread.isAlive) + assertNull(oldResult.get()) + assertFalse( + "The last lifecycle lease must complete deferred state removal", + ClientManager.AccMgrAuthTokenProvider.hasRefreshStateForTest(oldUser), + ) + + assertEquals("new-session-refreshed-access-token", newProvider.getNewAuthToken()) + assertEquals(2, tokenEndpointCalls.get()) + assertEquals(1, maxConcurrentRequests.get()) + assertEquals(oldRefreshToken, postedRefreshTokens[0]) + assertEquals(newRefreshToken, postedRefreshTokens[1]) + } + @Test fun testGetNewAuthToken_AccountRemovedBeforeRefresh_FailsClosedWithoutUnlinkingSharedState() { val account = mockk(relaxed = true) From 548432095285da7b3d64d1a1f7448a76ea87cae2 Mon Sep 17 00:00:00 2001 From: Wolfgang Mathurin Date: Wed, 23 Sep 2026 12:59:01 -0700 Subject: [PATCH 4/4] @W-24269724: Fence retired refresh side effects --- docs/auth/token-lifecycle.md | 12 +- .../androidsdk/rest/ClientManager.java | 200 ++++++++++-------- .../androidsdk/rest/ClientManagerMockTest.kt | 83 +++++++- 3 files changed, 199 insertions(+), 96 deletions(-) diff --git a/docs/auth/token-lifecycle.md b/docs/auth/token-lifecycle.md index 123b9d7f43..2e8dcf450d 100644 --- a/docs/auth/token-lifecycle.md +++ b/docs/auth/token-lifecycle.md @@ -174,6 +174,12 @@ If callers are still active, the scrubbed state remains mapped until the final l new callers that encounter it fail closed. This prevents a quick same-identity login from creating a second coordinator while an old-session refresh is still in flight. +The same state lock also fences every post-response side effect. After the token endpoint returns, +the old winner checks `cleanupRequested` while holding `state.lock` before persisting credentials, +logging out, registering RTR, or broadcasting. If logout retired the state while the request was +in flight, the response is discarded even when a quick relogin has recreated the same backing +Android Account. + ### Flow ``` @@ -195,7 +201,11 @@ refreshStaleToken() → POST /token (DPoP proof + nonce) → 400 use_dpop_nonce → cache nonce → retry → 200: new access_token, rotated refresh_token -broadcast ACCESS_TOKEN_REFRESH_INTENT +synchronized(state.lock) + cleanupRequested == false + → persist refreshed credentials + → register RTR and broadcast ACCESS_TOKEN_REFRESH_INTENT +lock released synchronized(state.lock) state.refreshing = false diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java index 76c0ab6521..62e14aec9a 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java @@ -658,39 +658,49 @@ && tryAdoptWinnerResult(state)) { final UserAccount userAccount = refreshStaleToken( matchingAccount, requestUser, - requestUser.getRefreshTokenForPersistence() + requestUser.getRefreshTokenForPersistence(), + state ); if (userAccount == null) { return null; } - newAuthToken = userAccount.getAuthToken(); - newInstanceUrl = userAccount.getInstanceServer(); - newTokenType = userAccount.getTokenType(); - newUiSid = userAccount.getUiSid(); - - if (clientManager.getValidatedUser( - /* requireRefreshFields = */ false) == null) { - newAuthToken = null; - newInstanceUrl = null; - newTokenType = null; - newUiSid = null; - return null; - } + // Serialize post-response side effects with logout cleanup. If cleanup acquired + // this lock first, the old session must not publish or broadcast into a newly + // recreated Android Account for the same Salesforce identity. + synchronized (state.lock) { + if (state.cleanupRequested) { + return null; + } + newAuthToken = userAccount.getAuthToken(); + newInstanceUrl = userAccount.getInstanceServer(); + newTokenType = userAccount.getTokenType(); + newUiSid = userAccount.getUiSid(); + + if (clientManager.getValidatedUser( + /* requireRefreshFields = */ false) == null) { + newAuthToken = null; + newInstanceUrl = null; + newTokenType = null; + newUiSid = null; + return null; + } - Intent broadcastIntent; - if (newInstanceUrl != null && !newInstanceUrl.equalsIgnoreCase(lastNewInstanceUrl)) { + Intent broadcastIntent; + if (newInstanceUrl != null + && !newInstanceUrl.equalsIgnoreCase(lastNewInstanceUrl)) { - // Broadcasts an intent that the instance server has changed (implicitly token refreshed too). - broadcastIntent = new Intent(INSTANCE_URL_UPDATE_INTENT); - } else { + // Broadcasts an intent that the instance server has changed (implicitly token refreshed too). + broadcastIntent = new Intent(INSTANCE_URL_UPDATE_INTENT); + } else { - // Broadcasts an intent that the access token has been refreshed. - broadcastIntent = new Intent(ACCESS_TOKEN_REFRESH_INTENT); - EventBuilderHelper.createAndStoreEvent("tokenRefresh", null, TAG, null); + // Broadcasts an intent that the access token has been refreshed. + broadcastIntent = new Intent(ACCESS_TOKEN_REFRESH_INTENT); + EventBuilderHelper.createAndStoreEvent("tokenRefresh", null, TAG, null); + } + broadcastIntent.setPackage(SalesforceSDKManager.getInstance().getAppContext().getPackageName()); + SalesforceSDKManager.getInstance().getAppContext().sendBroadcast(broadcastIntent); } - broadcastIntent.setPackage(SalesforceSDKManager.getInstance().getAppContext().getPackageName()); - SalesforceSDKManager.getInstance().getAppContext().sendBroadcast(broadcastIntent); } catch (OAuthFailedException | MalformedTokenException e) { /* * OAuthFailedException: token endpoint returned @@ -717,40 +727,44 @@ && tryAdoptWinnerResult(state)) { errorCode = OAuthErrorCode.UNKNOWN; } - // Account removal or malformed persisted data suppresses every local side effect. - if (clientManager.getValidatedUser( - /* requireRefreshFields = */ false) == null) { - return null; - } + // Serialize terminal-error side effects with cleanup for the same reason as the + // success path above: an old-session response must not log out or notify a newly + // recreated same-identity account. + synchronized (state.lock) { + if (state.cleanupRequested || clientManager.getValidatedUser( + /* requireRefreshFields = */ false) == null) { + return null; + } - final boolean terminal = !(e instanceof OAuthFailedException) - || errorCode != OAuthErrorCode.APP_ATTESTATION_FAILED_RETRY; + final boolean terminal = !(e instanceof OAuthFailedException) + || errorCode != OAuthErrorCode.APP_ATTESTATION_FAILED_RETRY; - if (terminal) { - // Terminal error (app_attest_failed, invalid_grant, malformed token, etc.) — logout. - if (Looper.myLooper() == null) { - Looper.prepare(); + if (terminal) { + // Terminal error (app_attest_failed, invalid_grant, malformed token, etc.) — logout. + if (Looper.myLooper() == null) { + Looper.prepare(); + } + final boolean showLoginPage = clientManager.getBoundAccountCount() == 1; + final LogoutReason reason = errorCode == OAuthErrorCode.APP_ATTESTATION_FAILED + ? CLIENT_BLOCKED + : REFRESH_TOKEN_EXPIRED; + // The refresh token may already be unusable, but logout still performs + // best-effort remote cleanup before removing the exact local account. + SalesforceSDKManager.getInstance() + .logout(matchingAccount, null, showLoginPage, reason); } - final boolean showLoginPage = clientManager.getBoundAccountCount() == 1; - final LogoutReason reason = errorCode == OAuthErrorCode.APP_ATTESTATION_FAILED - ? CLIENT_BLOCKED - : REFRESH_TOKEN_EXPIRED; - // The refresh token may already be unusable, but logout still performs - // best-effort remote cleanup before removing the exact local account. - SalesforceSDKManager.getInstance() - .logout(matchingAccount, null, showLoginPage, reason); - } - // Broadcast revoke intent with error details when available. - final Intent broadcastIntent = new Intent(ACCESS_TOKEN_REVOKE_INTENT); - if (errorType != null) { - broadcastIntent.putExtra(EXTRA_TOKEN_ERROR, errorType); - } - if (errorDesc != null) { - broadcastIntent.putExtra(EXTRA_TOKEN_ERROR_DESCRIPTION, errorDesc); + // Broadcast revoke intent with error details when available. + final Intent broadcastIntent = new Intent(ACCESS_TOKEN_REVOKE_INTENT); + if (errorType != null) { + broadcastIntent.putExtra(EXTRA_TOKEN_ERROR, errorType); + } + if (errorDesc != null) { + broadcastIntent.putExtra(EXTRA_TOKEN_ERROR_DESCRIPTION, errorDesc); + } + broadcastIntent.setPackage(SalesforceSDKManager.getInstance().getAppContext().getPackageName()); + SalesforceSDKManager.getInstance().getAppContext().sendBroadcast(broadcastIntent); } - broadcastIntent.setPackage(SalesforceSDKManager.getInstance().getAppContext().getPackageName()); - SalesforceSDKManager.getInstance().getAppContext().sendBroadcast(broadcastIntent); } catch (Exception e) { SalesforceSDKLogger.w(TAG, "Exception thrown while getting auth token", e); } finally { @@ -878,7 +892,8 @@ public String getUiSid() { private UserAccount refreshStaleToken( Account account, UserAccount originalUserAccount, - String currentRefreshToken + String currentRefreshToken, + RefreshState state ) throws NetworkErrorException, OAuthFailedException, MalformedTokenException { final Map addlParamsMap = originalUserAccount.getAdditionalOauthValues(); try { @@ -900,46 +915,51 @@ private UserAccount refreshStaleToken( .populateFromTokenEndpointResponse(tr) .build(); - // Confirm that the account still exists and can be rebuilt immediately before and - // after persistence. Token-generation comparisons are handled separately. - if (clientManager.getValidatedUser( - /* requireRefreshFields = */ false) == null) { - return null; - } - - /* - * Detect server-side Refresh Token Rotation: the response - * carried a refresh token that differs from this provider's - * cached copy. Stamp the ISO-8601 rotation time on the account - * BEFORE the primary persist below so the timestamp is written - * by the authoritative updateAccount call, not as a side - * effect of feature-flag registration. - */ - boolean refreshTokenRotated = tr.refreshToken != null && !tr.refreshToken.equals(refreshToken); - if (refreshTokenRotated) { - updatedUserAccount.setLastTokenRotationTime(Instant.now().toString()); - } - - UserAccountManager.getInstance().updateAccount(account, updatedUserAccount); - if (clientManager.getValidatedUser( - /* requireRefreshFields = */ false) == null) { - return null; - } - updatedUserAccount.downloadProfilePhoto(); - UserAccountManager.getInstance().clearCachedCurrentUser(); + synchronized (state.lock) { + // Logout can remove and then recreate the same backing Android Account while + // this network request is in flight. Gate every post-response side effect on + // the cleanup marker under the same lock used by clearRefreshState. + if (state.cleanupRequested || clientManager.getValidatedUser( + /* requireRefreshFields = */ false) == null) { + return null; + } - if (refreshTokenRotated) { /* - * Update this provider's cached copy and surface RTR as a - * per-user feature flag. The rotation timestamp is already - * persisted (above), so RTR-Active state here is - * independent of the timestamp's durability. + * Detect server-side Refresh Token Rotation: the response + * carried a refresh token that differs from this provider's + * cached copy. Stamp the ISO-8601 rotation time on the account + * BEFORE the primary persist below so the timestamp is written + * by the authoritative updateAccount call, not as a side + * effect of feature-flag registration. */ - refreshToken = tr.refreshToken; - SalesforceSDKManager.getInstance().registerUsedAppFeature(Features.FEATURE_RTR, updatedUserAccount); - } + boolean refreshTokenRotated = tr.refreshToken != null + && !tr.refreshToken.equals(refreshToken); + if (refreshTokenRotated) { + updatedUserAccount.setLastTokenRotationTime(Instant.now().toString()); + } - return updatedUserAccount; + UserAccountManager.getInstance().updateAccount(account, updatedUserAccount); + if (state.cleanupRequested || clientManager.getValidatedUser( + /* requireRefreshFields = */ false) == null) { + return null; + } + updatedUserAccount.downloadProfilePhoto(); + UserAccountManager.getInstance().clearCachedCurrentUser(); + + if (refreshTokenRotated) { + /* + * Update this provider's cached copy and surface RTR as a + * per-user feature flag. The rotation timestamp is already + * persisted (above), so RTR-Active state here is + * independent of the timestamp's durability. + */ + refreshToken = tr.refreshToken; + SalesforceSDKManager.getInstance().registerUsedAppFeature( + Features.FEATURE_RTR, updatedUserAccount); + } + + return updatedUserAccount; + } } catch (OAuthFailedException ofe) { SalesforceSDKLogger.i(TAG, "Token endpoint error: (Error: " + ofe.getTokenErrorResponse().error + ", Status Code: " + ofe.getHttpStatusCode() + ")", ofe); throw ofe; diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt index 0917235440..4175adfe61 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt @@ -1065,12 +1065,15 @@ class ClientManagerMockTest { ClientManager.AccMgrAuthTokenProvider.hasRefreshStateForTest(oldUser), ) - val newFixture = boundFixture( + // Recreate the same identity on the same backing Android Account while the old token POST + // is still in flight. This is the relogin shape that must not receive old-session effects. + oldFixture.liveUser.set(testUser( + authToken = "new-session-access-token", refreshToken = newRefreshToken, userId = userId, orgId = orgId, - ) - val newProvider = ClientManager.AccMgrAuthTokenProvider(newFixture.manager) + )) + val newProvider = ClientManager.AccMgrAuthTokenProvider(oldFixture.manager) val blockedReloginResult = AtomicReference() val blockedReloginThread = Thread { blockedReloginResult.set(newProvider.getNewAuthToken()) @@ -1091,6 +1094,13 @@ class ClientManagerMockTest { "Logout must scrub an in-flight old-session UI SID from the provider", oldProvider.uiSid, ) + assertEquals("new-session-access-token", oldFixture.liveUser.get()?.authToken) + assertEquals(newRefreshToken, oldFixture.liveUser.get()?.refreshTokenForPersistence) + verify(exactly = 0) { + mockUserAccountManager.updateAccount(oldFixture.account, any()) + mockSDKManager.logout(any(), any(), any(), any()) + mockAppContext.sendBroadcast(any()) + } assertFalse( "The last lifecycle lease must complete deferred state removal", ClientManager.AccMgrAuthTokenProvider.hasRefreshStateForTest(oldUser), @@ -1101,6 +1111,67 @@ class ClientManagerMockTest { assertEquals(1, maxConcurrentRequests.get()) assertEquals(oldRefreshToken, postedRefreshTokens[0]) assertEquals(newRefreshToken, postedRefreshTokens[1]) + verify(exactly = 1) { mockUserAccountManager.updateAccount(oldFixture.account, any()) } + } + + @Test + fun testClearRefreshState_InFlightInvalidGrant_DoesNotLogOutReloggedSameAccount() { + val userId = "same-user-invalid-grant" + val orgId = "same-org-invalid-grant" + val fixture = boundFixture( + refreshToken = "old-session-refresh-token", + userId = userId, + orgId = orgId, + ) + val oldUser = fixture.liveUser.get()!! + val requestStarted = CountDownLatch(1) + val releaseResponse = CountDownLatch(1) + every { HttpAccess.DEFAULT.okHttpClient } returns mockk { + every { newCall(any()) } returns mockk { + every { execute() } answers { + requestStarted.countDown() + releaseResponse.await(5, TimeUnit.SECONDS) + invalidGrantResponse() + } + } + } + + val result = AtomicReference() + val failure = AtomicReference() + val refreshThread = Thread { + try { + result.set( + ClientManager.AccMgrAuthTokenProvider(fixture.manager).getNewAuthToken() + ) + } catch (throwable: Throwable) { + failure.set(throwable) + } + } + refreshThread.start() + try { + assertTrue(requestStarted.await(5, TimeUnit.SECONDS)) + fixture.liveUser.set(null) + ClientManager.AccMgrAuthTokenProvider.clearRefreshState(oldUser) + fixture.liveUser.set(testUser( + authToken = "new-session-access-token", + refreshToken = "new-session-refresh-token", + userId = userId, + orgId = orgId, + )) + } finally { + releaseResponse.countDown() + } + refreshThread.join(TimeUnit.SECONDS.toMillis(5)) + + assertFalse("Old-session refresh thread did not finish", refreshThread.isAlive) + assertNull(failure.get()) + assertNull(result.get()) + assertEquals("new-session-access-token", fixture.liveUser.get()?.authToken) + verify(exactly = 0) { + mockUserAccountManager.updateAccount(any(), any()) + mockSDKManager.logout(any(), any(), any(), any()) + mockAppContext.sendBroadcast(any()) + } } @Test @@ -1782,12 +1853,14 @@ class ClientManagerMockTest { ?.let { URLDecoder.decode(it, "UTF-8") } } - /** Deterministically blocks until [count] threads are parked in WAITING/TIMED_WAITING. */ + /** Deterministically blocks until [count] threads are parked on a wait or state monitor. */ private fun awaitThreadsParked(threads: List, count: Int) { val deadline = System.currentTimeMillis() + TimeUnit.SECONDS.toMillis(5) while (System.currentTimeMillis() < deadline) { val parked = threads.count { - it.state == Thread.State.WAITING || it.state == Thread.State.TIMED_WAITING + it.state == Thread.State.BLOCKED + || it.state == Thread.State.WAITING + || it.state == Thread.State.TIMED_WAITING } if (parked >= count) return Thread.sleep(50)