Fix Refresh Token Rotation. (#2884)

* Fix Refresh Token Rotation.

* Update UserAccount.getRefreshToken to always return the latest values known to the SDK.  Add getRefreshTokenForPersistance to handle account updates and storage.

Author: brandonpage

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<String> 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<String> 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);

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.
@@ -488,6 +488,16 @@ 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
+		// 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));
+		}
+
 		return extras;
 	}
 

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.

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()) {

libs/test/SalesforceSDKTest/src/com/salesforce/androidsdk/accounts/UserAccountManagerTest.java

@@ -109,6 +109,89 @@ 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));
+    }
+
+    /*
+     * 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.
      */