From b5bcc3b0cc0782ac1a587e629c97a800352e0bff Mon Sep 17 00:00:00 2001 From: Brandon Page Date: Tue, 12 May 2026 18:49:22 -0700 Subject: [PATCH 1/2] Fix Refresh Token Rotation. --- .../accounts/UserAccountManager.java | 9 ++ .../androidsdk/rest/ClientManager.java | 9 +- .../accounts/UserAccountManagerTest.java | 42 ++++++ .../androidsdk/rest/ClientManagerMockTest.kt | 142 ++++++++++++++++++ 4 files changed, 201 insertions(+), 1 deletion(-) diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/accounts/UserAccountManager.java b/libs/SalesforceSDK/src/com/salesforce/androidsdk/accounts/UserAccountManager.java index e56d3e24f5..650a68183b 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/accounts/UserAccountManager.java +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/accounts/UserAccountManager.java @@ -488,6 +488,15 @@ public Bundle updateAccount(Account account, UserAccount userAccount) { accountManager.setUserData(account, key, extras.getString(key)); } + // The refresh token is stored as the Account's password (see createAccount), not as user data, + // so buildAuthBundle does not include it. Persist it explicitly here so that server-side + // Refresh Token Rotation is correctly reflected in storage. + final String refreshToken = userAccount.getRefreshToken(); + if (refreshToken != null) { + final String encryptionKey = SalesforceSDKManager.getEncryptionKey(); + accountManager.setPassword(account, SalesforceSDKManager.encrypt(refreshToken, encryptionKey)); + } + return extras; } diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java index 23fec11f92..84a23e7972 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/rest/ClientManager.java @@ -354,7 +354,8 @@ public static class AccMgrAuthTokenProvider implements RestClient.AuthTokenProvi private final Object lock = new Object(); private final ClientManager clientManager; private String lastNewAuthToken; - private final String refreshToken; + // Mutable to support server-side Refresh Token Rotation (RTR). + private String refreshToken; private String lastNewInstanceUrl; private long lastRefreshTime = -1 /* never refreshed */; @@ -506,6 +507,12 @@ private UserAccount refreshStaleToken(Account account) throws NetworkErrorExcept updatedUserAccount.downloadProfilePhoto(); UserAccountManager.getInstance().clearCachedCurrentUser(); + // Handle server-side Refresh Token Rotation: if the response contained a new refresh token, + // update this provider's cached copy. + if (tr.refreshToken != null && !tr.refreshToken.equals(refreshToken)) { + refreshToken = tr.refreshToken; + } + return updatedUserAccount; } catch (OAuth2.OAuthFailedException ofe) { if (ofe.isRefreshTokenInvalid()) { diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/accounts/UserAccountManagerTest.java b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/accounts/UserAccountManagerTest.java index d8e08d11ef..e4edfb38ae 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/accounts/UserAccountManagerTest.java +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/accounts/UserAccountManagerTest.java @@ -109,6 +109,48 @@ public void testUserAccountToAccountToUserAccount() { checkSameUserAccount(userAccount, restoredUserAccount); } + /* + * Server-side Refresh Token Rotation (RTR) regression test. + * + * The refresh token is persisted as the Account's password and is read back via accountManager.getPassword(). + * updateAccount must therefore persist a rotated refresh token via setPassword. + */ + @Test + public void testUpdateAccountPersistsRotatedRefreshToken() { + final UserAccount original = UserAccountTest.createTestAccount(); + userAccMgr.createAccount(original); + final Account account = userAccMgr.getCurrentAccount(); + Assert.assertEquals( + "Initial refresh token should round-trip through AccountManager", + UserAccountTest.TEST_REFRESH_TOKEN, + userAccMgr.buildUserAccount(account).getRefreshToken()); + + // Simulate a server-side refresh token rotation by building a + // UserAccount with a new refresh token value and updating. + final String rotatedRefreshToken = "rotated_refresh_token"; + final UserAccount rotated = UserAccountBuilder.getInstance() + .populateFromUserAccount(original) + .refreshToken(rotatedRefreshToken) + .build(); + userAccMgr.updateAccount(account, rotated); + + // The persisted refresh token must reflect the rotated value. + final UserAccount reloaded = userAccMgr.buildUserAccount(account); + Assert.assertEquals( + "Rotated refresh token should be persisted by updateAccount", + rotatedRefreshToken, + reloaded.getRefreshToken()); + + // Encryption (AES-GCM with a random IV) is non-deterministic, so + // compare against the decrypted password rather than re-encrypting + // the expected value. + final String encryptionKey = SalesforceSDKManager.getEncryptionKey(); + Assert.assertEquals( + "Rotated refresh token should be used as the account password.", + rotatedRefreshToken, + SalesforceSDKManager.decrypt(accMgr.getPassword(account), encryptionKey)); + } + /** * Test to get all authenticated users. */ 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 98cd985528..c2583c2544 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt @@ -36,6 +36,7 @@ import org.junit.Test private const val OLD_ACCESS_TOKEN = "old-token" private const val REFRESHED_ACCESS_TOKEN = "refreshed-auth-token" private const val REFRESH_TOKEN = "refresh-token" +private const val ROTATED_REFRESH_TOKEN = "rotated-refresh-token" @SmallTest class ClientManagerMockTest { @@ -346,6 +347,147 @@ class ClientManagerMockTest { Assert.assertEquals(ClientManager.ACCESS_TOKEN_REVOKE_INTENT, broadcastIntentSlot.captured.action) } + /* + Server-side Refresh Token Rotation (RTR): when the token endpoint returns + a rotated refresh_token, the provider must update its cached refresh + token so subsequent calls don't reuse the now-invalidated previous one. + */ + @Test + fun testGetNewAuthToken_RefreshTokenRotation_UpdatesCachedRefreshToken() { + val responseBody = """ + { + "access_token": "$REFRESHED_ACCESS_TOKEN", + "refresh_token": "$ROTATED_REFRESH_TOKEN", + "instance_url": "https://login.salesforce.com", + "id": "https://login.salesforce.com/id/orgId/userId", + "token_type": "Bearer", + "issued_at": "1234567890", + "signature": "mock-signature" + } + """.trimIndent().toResponseBody("application/json; charset=utf-8".toMediaType()) + val rotatedResponse = mockk(relaxed = true) { + every { isSuccessful } returns true + every { close() } just runs + every { body } returns responseBody + } + every { HttpAccess.DEFAULT.okHttpClient } returns mockk { + every { newCall(any()) } returns mockk { + every { execute() } returns rotatedResponse + } + } + + val userSlot = slot() + val mockAccount = mockk(relaxed = true) + val mockUser = mockk(relaxed = true) { + every { authToken } returns OLD_ACCESS_TOKEN + every { refreshToken } returns REFRESH_TOKEN + every { loginServer } returns "https://login.salesforce.com" + } + val mockClientManager = mockk(relaxed = true) { + every { accounts } returns arrayOf(mockAccount) + } + every { mockUserAccountManager.currentUser } returns mockUser + every { mockUserAccountManager.buildUserAccount(mockAccount) } returns mockUser + every { mockUserAccountManager.updateAccount(mockAccount, any()) } returns mockk() + + val authTokenProvider = ClientManager.AccMgrAuthTokenProvider( + mockClientManager, + "https://login.salesforce.com", + OLD_ACCESS_TOKEN, + REFRESH_TOKEN, + ) + + // First refresh: server rotates the refresh token. + Assert.assertEquals(REFRESHED_ACCESS_TOKEN, authTokenProvider.getNewAuthToken()) + + // The persisted account should be updated with the rotated refresh token... + verify(exactly = 1) { + mockUserAccountManager.updateAccount(mockAccount, capture(userSlot)) + } + Assert.assertEquals(ROTATED_REFRESH_TOKEN, userSlot.captured.refreshToken) + // ...and so should the provider's in-memory cache, so that subsequent + // refreshes (and getRefreshToken consumers) use the rotated token. + Assert.assertEquals(ROTATED_REFRESH_TOKEN, authTokenProvider.refreshToken) + } + + /* + Server-side Refresh Token Rotation (RTR): after a refresh that rotates + the refresh token, the provider's cached refresh token must reflect + the new value so that a subsequent refresh sends the current token + and the per-account lookup matches the rotated value persisted to + the account. + */ + @Test + fun testGetNewAuthToken_RefreshTokenRotation_SubsequentRefreshSucceeds() { + val firstRotated = ROTATED_REFRESH_TOKEN + val secondRotated = "rotated-refresh-token-2" + + fun rotationResponse(rt: String): Response { + val responseBody = """ + { + "access_token": "$REFRESHED_ACCESS_TOKEN", + "refresh_token": "$rt", + "instance_url": "https://login.salesforce.com", + "id": "https://login.salesforce.com/id/orgId/userId", + "token_type": "Bearer", + "issued_at": "1234567890", + "signature": "mock-signature" + } + """.trimIndent().toResponseBody("application/json; charset=utf-8".toMediaType()) + return mockk(relaxed = true) { + every { isSuccessful } returns true + every { close() } just runs + every { body } returns responseBody + } + } + + // Return a different rotated refresh token on each refresh. + every { HttpAccess.DEFAULT.okHttpClient } returns mockk { + every { newCall(any()) } returnsMany listOf( + mockk { every { execute() } returns rotationResponse(firstRotated) }, + mockk { every { execute() } returns rotationResponse(secondRotated) }, + ) + } + + val mockAccount = mockk(relaxed = true) + // The persisted account's refresh token follows whatever updateAccount + // was last called with (i.e., the most recent rotated value). + var persistedRefreshToken = REFRESH_TOKEN + val mockUser = mockk(relaxed = true) { + every { authToken } returns OLD_ACCESS_TOKEN + every { refreshToken } answers { persistedRefreshToken } + every { loginServer } returns "https://login.salesforce.com" + } + val mockClientManager = mockk(relaxed = true) { + every { accounts } returns arrayOf(mockAccount) + } + every { mockUserAccountManager.currentUser } returns mockUser + every { mockUserAccountManager.buildUserAccount(mockAccount) } returns mockUser + every { mockUserAccountManager.updateAccount(mockAccount, any()) } answers { + persistedRefreshToken = secondArg().refreshToken + mockk() + } + + val authTokenProvider = ClientManager.AccMgrAuthTokenProvider( + mockClientManager, + "https://login.salesforce.com", + OLD_ACCESS_TOKEN, + REFRESH_TOKEN, + ) + + // First refresh succeeds, rotates to firstRotated. + Assert.assertEquals(REFRESHED_ACCESS_TOKEN, authTokenProvider.getNewAuthToken()) + Assert.assertEquals(firstRotated, authTokenProvider.refreshToken) + Assert.assertEquals(firstRotated, persistedRefreshToken) + + // Second refresh, ensure each rotation is stored. + Assert.assertEquals(REFRESHED_ACCESS_TOKEN, authTokenProvider.getNewAuthToken()) + Assert.assertEquals(secondRotated, authTokenProvider.refreshToken) + verify(exactly = 0) { + mockSDKManager.logout(any(), any(), any(), any()) + } + } + /* Non-current user tests the scenario of attempting to make a network call as the previous user on user account switch, but From 9558d9c72492e5af457b54bb67b3673f6d4c78bb Mon Sep 17 00:00:00 2001 From: Brandon Page Date: Thu, 14 May 2026 15:52:56 -0700 Subject: [PATCH 2/2] Update UserAccount.getRefreshToken to always return the latest values known to the SDK. Add getRefreshTokenForPersistance to handle account updates and storage. --- .../androidsdk/accounts/UserAccount.java | 46 ++++++++++++++++++- .../accounts/UserAccountManager.java | 7 +-- .../auth/AuthenticationUtilities.kt | 14 ++++-- .../accounts/UserAccountManagerTest.java | 41 +++++++++++++++++ .../androidsdk/rest/ClientManagerMockTest.kt | 2 +- 5 files changed, 100 insertions(+), 10 deletions(-) diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/accounts/UserAccount.java b/libs/SalesforceSDK/src/com/salesforce/androidsdk/accounts/UserAccount.java index 06865a205a..aadad63d11 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/accounts/UserAccount.java +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/accounts/UserAccount.java @@ -26,6 +26,8 @@ */ package com.salesforce.androidsdk.accounts; +import android.accounts.Account; +import android.accounts.AccountManager; import android.app.DownloadManager; import android.content.Context; import android.content.pm.PackageManager; @@ -372,6 +374,46 @@ public String getAuthToken() { * @return Refresh token. */ public String getRefreshToken() { + if (accountName != null) { + try { + final AccountManager accMrg = AccountManager.get(SalesforceSDKManager.getInstance().getAppContext()); + final String accountType = SalesforceSDKManager.getInstance().getAccountType(); + for (Account account : accMrg.getAccountsByType(accountType)) { + if (accountName.equals(account.name)) { + final String encryptedPassword = accMrg.getPassword(account); + if (encryptedPassword != null) { + final String newestRefreshToken = SalesforceSDKManager.decrypt( + encryptedPassword, SalesforceSDKManager.getEncryptionKey()); + if (newestRefreshToken != null) { + return newestRefreshToken; + } + } + break; + } + } + } catch (Exception e) { + SalesforceSDKLogger.w(TAG, + "Failed to read latest refresh token from AccountManager; using in-memory value", + e); + } + } + return refreshToken; + } + + /** + * Returns the in-memory refresh token snapshot, without consulting + * {@link AccountManager}. + * + * Intended for persistence call sites that are about to write a refresh + * token to storage (account creation, update, token migration, duplicate + * user reconciliation). Using {@link #getRefreshToken()} at those sites + * would read back the value currently persisted in {@link AccountManager} + * — i.e. the value we are about to overwrite — instead of the value the + * caller built this {@code UserAccount} with. + * + * @return The refresh token field as set at construction time. + */ + public String getRefreshTokenForPersistence() { return refreshToken; } @@ -948,7 +990,7 @@ JSONObject toJson(List additionalOauthKeys) { JSONObject object = new JSONObject(); try { object.put(AUTH_TOKEN, authToken); - object.put(REFRESH_TOKEN, refreshToken); + object.put(REFRESH_TOKEN, getRefreshToken()); object.put(LOGIN_SERVER, loginServer); object.put(ID_URL, idUrl); object.put(INSTANCE_SERVER, instanceServer); @@ -1007,7 +1049,7 @@ public JSONObject toJson() { Bundle toBundle(List additionalOauthKeys) { Bundle object = new Bundle(); object.putString(AUTH_TOKEN, authToken); - object.putString(REFRESH_TOKEN, refreshToken); + object.putString(REFRESH_TOKEN, getRefreshToken()); object.putString(LOGIN_SERVER, loginServer); object.putString(ID_URL, idUrl); object.putString(INSTANCE_SERVER, instanceServer); diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/accounts/UserAccountManager.java b/libs/SalesforceSDK/src/com/salesforce/androidsdk/accounts/UserAccountManager.java index 650a68183b..1f45e2c9a1 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/accounts/UserAccountManager.java +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/accounts/UserAccountManager.java @@ -442,7 +442,7 @@ public Bundle createAccount(UserAccount userAccount) { final Bundle extras = buildAuthBundle(userAccount); Account acc = new Account(userAccount.getAccountName(), accountType); - String password = SalesforceSDKManager.encrypt(userAccount.getRefreshToken(), encryptionKey); + String password = SalesforceSDKManager.encrypt(userAccount.getRefreshTokenForPersistence(), encryptionKey); boolean success = accountManager.addAccountExplicitly(acc, password, new Bundle()); // Add account will fail if the account already exists, so update refresh token. @@ -490,8 +490,9 @@ public Bundle updateAccount(Account account, UserAccount userAccount) { // The refresh token is stored as the Account's password (see createAccount), not as user data, // so buildAuthBundle does not include it. Persist it explicitly here so that server-side - // Refresh Token Rotation is correctly reflected in storage. - final String refreshToken = userAccount.getRefreshToken(); + // Use the in-memory snapshot rather than getRefreshToken(), which now performs a live lookup + // against AccountManager and would return the updated value we may be about to write. + final String refreshToken = userAccount.getRefreshTokenForPersistence(); if (refreshToken != null) { final String encryptionKey = SalesforceSDKManager.getEncryptionKey(); accountManager.setPassword(account, SalesforceSDKManager.encrypt(refreshToken, encryptionKey)); diff --git a/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/AuthenticationUtilities.kt b/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/AuthenticationUtilities.kt index f2cddb6625..6a37a66585 100644 --- a/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/AuthenticationUtilities.kt +++ b/libs/SalesforceSDK/src/com/salesforce/androidsdk/auth/AuthenticationUtilities.kt @@ -465,8 +465,13 @@ internal fun handleDuplicateUserAccount( clearCaches() userAccountManager.clearCachedCurrentUser() - // Revoke existing refresh token - if (account.refreshToken != duplicateUserAccount.refreshToken) { + // Revoke existing refresh token. Compare the new account's in-memory + // snapshot (the value just issued by the server) against the duplicate + // account's snapshot (the value currently persisted). Using + // getRefreshToken() here would do a live AccountManager lookup for + // both UserAccounts and return the same persisted value, suppressing + // the revocation and leaking the prior refresh token. + if (account.refreshTokenForPersistence != duplicateUserAccount.refreshTokenForPersistence) { runCatching { URI(duplicateUserAccount.instanceServer) }.onFailure { throwable -> @@ -481,7 +486,7 @@ internal fun handleDuplicateUserAccount( revokeRefreshToken( HttpAccess.DEFAULT, uri, - duplicateUserAccount.refreshToken, + duplicateUserAccount.refreshTokenForPersistence, OAuth2.LogoutReason.REFRESH_TOKEN_ROTATED, ) } @@ -515,7 +520,8 @@ private fun UserAccountManager.persistAccount( acctManager: AccountManager = AccountManager.get(SalesforceSDKManager.getInstance().appContext), ) { val account = Account(userAccount.accountName, accountType) - val password = SalesforceSDKManager.encrypt(userAccount.refreshToken, encryptionKey) + // Encrypt the in-memory snapshot rather than getRefreshToken(), which performs a lookup. + val password = SalesforceSDKManager.encrypt(userAccount.refreshTokenForPersistence, encryptionKey) val created = acctManager.addAccountExplicitly(account, password, /* userdata = */ Bundle()) // addAccountExplicitly fails if the account already exists, so update the refresh token. diff --git a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/accounts/UserAccountManagerTest.java b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/accounts/UserAccountManagerTest.java index e4edfb38ae..30b5b6425a 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/accounts/UserAccountManagerTest.java +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/accounts/UserAccountManagerTest.java @@ -151,6 +151,47 @@ public void testUpdateAccountPersistsRotatedRefreshToken() { SalesforceSDKManager.decrypt(accMgr.getPassword(account), encryptionKey)); } + /* + * Server-side Refresh Token Rotation (RTR) regression test. + * + * A UserAccount snapshot built from the persisted Account must reflect a + * subsequent rotation of the refresh token via updateAccount, even when + * the snapshot itself was built before the rotation. This guards the + * live-lookup behavior of UserAccount.getRefreshToken(). + */ + @Test + public void testGetRefreshTokenReflectsLatestPersistedValue() { + final UserAccount original = UserAccountTest.createTestAccount(); + userAccMgr.createAccount(original); + final Account account = userAccMgr.getCurrentAccount(); + + // Snapshot taken before rotation. + final UserAccount staleUser = userAccMgr.buildUserAccount(account); + Assert.assertEquals( + "Initial refresh token should be observed via the snapshot", + UserAccountTest.TEST_REFRESH_TOKEN, + staleUser.getRefreshToken()); + + // Rotate via updateAccount. + final String rotatedRefreshToken = "rotated_refresh_token"; + final UserAccount rotated = UserAccountBuilder.getInstance() + .populateFromUserAccount(original) + .refreshToken(rotatedRefreshToken) + .build(); + userAccMgr.updateAccount(account, rotated); + + Assert.assertEquals( + "Initial refresh token should be observed via the snapshot", + UserAccountTest.TEST_REFRESH_TOKEN, + staleUser.getRefreshTokenForPersistence()); + // The previously-built snapshot should now observe the rotated value + // through its live lookup against AccountManager. + Assert.assertEquals( + "Snapshot built before rotation should reflect the latest persisted refresh token", + rotatedRefreshToken, + staleUser.getRefreshToken()); + } + /** * Test to get all authenticated users. */ 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 c2583c2544..95984be653 100644 --- a/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt +++ b/libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/rest/ClientManagerMockTest.kt @@ -404,7 +404,7 @@ class ClientManagerMockTest { verify(exactly = 1) { mockUserAccountManager.updateAccount(mockAccount, capture(userSlot)) } - Assert.assertEquals(ROTATED_REFRESH_TOKEN, userSlot.captured.refreshToken) + Assert.assertEquals(ROTATED_REFRESH_TOKEN, userSlot.captured.refreshTokenForPersistence) // ...and so should the provider's in-memory cache, so that subsequent // refreshes (and getRefreshToken consumers) use the rotated token. Assert.assertEquals(ROTATED_REFRESH_TOKEN, authTokenProvider.refreshToken)