@W-22701996: [Android] Improve error handling at token refresh #4
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,6 +105,12 @@ public class OAuth2 { | |
| public static final String LOGIN_HINT = "login_hint"; | ||
| private static final String REFRESH_TOKEN = "refresh_token"; // Grant Type Values | ||
|
|
||
| /** Token endpoint error: device/app permanently blocked by attestation. Triggers logout. */ | ||
| public static final String USER_BLOCKED_ERROR = "user_blocked"; | ||
|
|
||
| /** Token endpoint error: attestation could not be verified but may succeed on retry. Does not trigger logout. */ | ||
| public static final String USER_BLOCKED_RETRY_ERROR = "user_blocked_retry"; | ||
|
|
||
| /** | ||
| * OAuth 2.0 authorization endpoint request body parameter names: | ||
| * Salesforce App Attestation External Client App Attestation | ||
|
|
@@ -236,6 +242,7 @@ public enum LogoutReason { | |
| UNEXPECTED, // Unexpected error or crash | ||
| UNEXPECTED_RESPONSE, // Unexpected response from server | ||
| UNKNOWN, // Unknown | ||
|
Owner
Author
There was a problem hiding this comment. Requirement #3: |
||
| USER_BLOCKED, // Device/app blocked by server (e.g. failed attestation) | ||
| USER_LOGOUT, // User initiated logout | ||
| REFRESH_TOKEN_ROTATED; // Refresh token rotated | ||
|
|
||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,6 +26,12 @@ | |
| */ | ||
| package com.salesforce.androidsdk.rest; | ||
|
|
||
| import static com.salesforce.androidsdk.auth.OAuth2.LogoutReason.REFRESH_TOKEN_EXPIRED; | ||
| import static com.salesforce.androidsdk.auth.OAuth2.LogoutReason.USER_BLOCKED; | ||
| import static com.salesforce.androidsdk.auth.OAuth2.USER_BLOCKED_ERROR; | ||
| import static com.salesforce.androidsdk.auth.OAuth2.USER_BLOCKED_RETRY_ERROR; | ||
| import static com.salesforce.androidsdk.auth.OAuth2.refreshAuthToken; | ||
|
|
||
| import android.accounts.Account; | ||
| import android.accounts.AccountManager; | ||
| import android.accounts.NetworkErrorException; | ||
|
|
@@ -42,7 +48,10 @@ | |
| import com.salesforce.androidsdk.app.SalesforceSDKManager; | ||
| import com.salesforce.androidsdk.auth.AuthenticatorService; | ||
| import com.salesforce.androidsdk.auth.HttpAccess; | ||
| import com.salesforce.androidsdk.auth.OAuth2.LogoutReason; | ||
| import com.salesforce.androidsdk.auth.OAuth2.OAuthFailedException; | ||
| import com.salesforce.androidsdk.auth.OAuth2.TokenEndpointResponse; | ||
| import com.salesforce.androidsdk.auth.OAuth2.TokenErrorResponse; | ||
| import com.salesforce.androidsdk.rest.RestClient.ClientInfo; | ||
| import com.salesforce.androidsdk.util.SalesforceSDKLogger; | ||
|
|
||
|
|
@@ -60,6 +69,11 @@ public class ClientManager { | |
| public static final String ACCESS_TOKEN_REVOKE_INTENT = "access_token_revoked"; | ||
| public static final String ACCESS_TOKEN_REFRESH_INTENT = "access_token_refeshed"; | ||
| public static final String INSTANCE_URL_UPDATE_INTENT = "instance_url_updated"; | ||
| /** Intent extra: the {@code error} value from the token endpoint response (e.g. "user_blocked", "invalid_grant"). */ | ||
| public static final String EXTRA_TOKEN_ERROR = "token_error"; | ||
|
Owner
Author
|
||
|
|
||
| /** Intent extra: the {@code error_description} value from the token endpoint response. */ | ||
| public static final String EXTRA_TOKEN_ERROR_DESCRIPTION = "token_error_description"; | ||
| private static final String TAG = "ClientManager"; | ||
|
|
||
| private final AccountManager accountManager; | ||
|
|
@@ -397,17 +411,18 @@ public String getNewAuthToken() { | |
| String newAuthToken = null; | ||
| String newInstanceUrl = null; | ||
| boolean shouldUpdateCache = false; | ||
| Account[] accounts = null; | ||
| Account matchingAccount = null; | ||
|
|
||
| try { | ||
| // Only check for matching account inside synchronized thread that | ||
| // is actually getting the new auth token. | ||
| final UserAccountManager userAccountManager = SalesforceSDKManager.getInstance().getUserAccountManager(); | ||
| accounts = clientManager.getAccounts(); | ||
|
|
||
| if (refreshToken != null) { | ||
| for (Account account : accounts) { | ||
| final UserAccount user = userAccountManager.buildUserAccount(account); | ||
| if (user != null && refreshToken.equals(user.getRefreshToken())) { | ||
| matchingAccount = account; | ||
| break; | ||
|
|
@@ -423,41 +438,66 @@ public String getNewAuthToken() { | |
| // We found a matching account, so we'll attempt a refresh and should update the cache. | ||
| shouldUpdateCache = true; | ||
|
|
||
| /* | ||
| * Invalidate current auth token. After a prior | ||
| * user_blocked_retry the cached token is null because | ||
| * that path clears it without logging out. | ||
| * AccountManager.invalidateAuthToken is a no-op for | ||
| * null, but guarding here avoids a wasteful call whose | ||
| * frequency increases with retriable attestation | ||
| * errors. | ||
| */ | ||
| if (lastNewAuthToken != null) { | ||
| clientManager.invalidateToken(lastNewAuthToken); | ||
| } | ||
| final UserAccount userAccount = refreshStaleToken(matchingAccount); | ||
|
|
||
| newAuthToken = userAccount.getAuthToken(); | ||
| newInstanceUrl = userAccount.getInstanceServer(); | ||
|
|
||
| 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 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); | ||
| } catch (OAuthFailedException ofe) { | ||
| final TokenErrorResponse tokenError = ofe.getTokenErrorResponse(); | ||
| final String errorType = tokenError != null ? tokenError.error : null; | ||
| final String errorDesc = tokenError != null ? tokenError.errorDescription : null; | ||
|
|
||
| if (!USER_BLOCKED_RETRY_ERROR.equals(errorType)) { | ||
|
Owner
Author
There was a problem hiding this comment. Requirement #4: Requirement #3: |
||
| // Terminal error (user_blocked, invalid_grant, etc.) — logout. | ||
| if (clientManager.revokedTokenShouldLogout) { | ||
| if (Looper.myLooper() == null) { | ||
| Looper.prepare(); | ||
| } | ||
| final boolean showLoginPage = accounts.length == 1; | ||
| final LogoutReason reason = USER_BLOCKED_ERROR.equals(errorType) | ||
| ? USER_BLOCKED | ||
| : REFRESH_TOKEN_EXPIRED; | ||
| // Note: As of writing (2024) this call will never succeed because revoke API is an | ||
| // authenticated endpoint. However, there is no harm in attempting and the debug logs | ||
| // produced may help developers better understand the state of their app. | ||
| SalesforceSDKManager.getInstance() | ||
| .logout(matchingAccount, null, showLoginPage, reason); | ||
| } | ||
| } | ||
|
|
||
| // Broadcast revoke intent with error details for all OAuth failures. | ||
| 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); | ||
|
|
@@ -490,11 +530,11 @@ public long getLastRefreshTime() { | |
| @Override | ||
| public String getInstanceUrl() { return lastNewInstanceUrl; } | ||
|
|
||
| private UserAccount refreshStaleToken(Account account) throws NetworkErrorException, OAuthFailedException { | ||
| UserAccount originalUserAccount = UserAccountManager.getInstance().buildUserAccount(account); | ||
| final Map<String,String> addlParamsMap = originalUserAccount.getAdditionalOauthValues(); | ||
| try { | ||
| final TokenEndpointResponse tr = refreshAuthToken(HttpAccess.DEFAULT, | ||
| new URI(originalUserAccount.getLoginServer()), originalUserAccount.getClientIdForRefresh(), refreshToken, addlParamsMap); | ||
|
|
||
| UserAccount updatedUserAccount = UserAccountBuilder.getInstance() | ||
|
|
@@ -514,13 +554,9 @@ private UserAccount refreshStaleToken(Account account) throws NetworkErrorExcept | |
| } | ||
|
|
||
| return updatedUserAccount; | ||
| } catch (OAuthFailedException ofe) { | ||
|
Owner
Author
There was a problem hiding this comment. Requirement #1 (enabler): Previously this method caught |
||
| SalesforceSDKLogger.i(TAG, "Token endpoint error: (Error: " + ofe.getTokenErrorResponse().error + ", Status Code: " + ofe.getHttpStatusCode() + ")", ofe); | ||
| throw ofe; | ||
| } catch (Exception e) { | ||
| SalesforceSDKLogger.e(TAG, "Exception thrown while getting new auth token", e); | ||
| throw new NetworkErrorException(e); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,134 @@ | ||
| package com.salesforce.androidsdk.auth | ||
|
|
||
| import android.accounts.AbstractAccountAuthenticator | ||
| import android.accounts.Account | ||
| import android.accounts.AccountManager | ||
| import android.content.Context | ||
| import androidx.test.filters.SmallTest | ||
| import androidx.test.platform.app.InstrumentationRegistry | ||
| import com.salesforce.androidsdk.accounts.UserAccount | ||
| import com.salesforce.androidsdk.accounts.UserAccountManager | ||
| import com.salesforce.androidsdk.app.SalesforceSDKManager | ||
| import io.mockk.every | ||
| import io.mockk.mockk | ||
| import io.mockk.mockkObject | ||
| import io.mockk.mockkStatic | ||
| import io.mockk.unmockkAll | ||
| import okhttp3.Call | ||
| import okhttp3.MediaType.Companion.toMediaType | ||
| import okhttp3.OkHttpClient | ||
| import okhttp3.Response | ||
| import okhttp3.ResponseBody.Companion.toResponseBody | ||
| import org.junit.After | ||
| import org.junit.Assert.assertEquals | ||
| import org.junit.Assert.assertNotNull | ||
| import org.junit.Assert.assertNull | ||
| import org.junit.Before | ||
| import org.junit.Test | ||
|
|
||
| @SmallTest | ||
| class AuthenticatorServiceTest { | ||
|
|
||
| private lateinit var authenticator: AbstractAccountAuthenticator | ||
| private lateinit var mockUserAccountManager: UserAccountManager | ||
| private lateinit var mockAppContext: Context | ||
| private lateinit var mockAccount: Account | ||
| private lateinit var mockUser: UserAccount | ||
|
|
||
| @Before | ||
| fun setUp() { | ||
| val targetContext = InstrumentationRegistry.getInstrumentation().targetContext | ||
| mockAppContext = mockk(relaxed = true) { | ||
| every { packageName } returns "packageName" | ||
| every { filesDir } returns targetContext.filesDir | ||
| every { getSharedPreferences(any(), any()) } answers { | ||
| targetContext.getSharedPreferences(firstArg(), Context.MODE_PRIVATE) | ||
| } | ||
| } | ||
|
|
||
| mockUserAccountManager = mockk(relaxed = true) | ||
|
|
||
| mockkObject(SalesforceSDKManager) | ||
| val mockSDKManager = mockk<SalesforceSDKManager> { | ||
| every { userAccountManager } returns mockUserAccountManager | ||
| every { appContext } returns mockAppContext | ||
| every { deviceId } returns "test-device-id" | ||
| every { additionalOauthKeys } returns emptyList() | ||
| every { useHybridAuthentication } returns true | ||
| every { appAttestationClient } returns null | ||
| @Suppress("UNCHECKED_CAST") | ||
| every { loginActivityClass } returns Class.forName("com.salesforce.androidsdk.ui.LoginActivity") as Class<out android.app.Activity> | ||
| } | ||
| every { SalesforceSDKManager.getInstance() } returns mockSDKManager | ||
|
|
||
| mockkStatic(UserAccountManager::class) | ||
| every { UserAccountManager.getInstance() } returns mockUserAccountManager | ||
|
|
||
| mockkObject(HttpAccess.DEFAULT) | ||
|
|
||
| mockAccount = mockk(relaxed = true) | ||
| mockUser = mockk<UserAccount>(relaxed = true) { | ||
| every { loginServer } returns "https://login.salesforce.com" | ||
| every { refreshToken } returns "refresh-token" | ||
| every { clientIdForRefresh } returns "client-id" | ||
| } | ||
| every { mockUserAccountManager.buildUserAccount(mockAccount) } returns mockUser | ||
|
|
||
| // Instantiate the private Authenticator inner class via reflection. | ||
| val authenticatorClass = Class.forName("com.salesforce.androidsdk.auth.AuthenticatorService\$Authenticator") | ||
| val constructor = authenticatorClass.getDeclaredConstructor(Context::class.java) | ||
| constructor.isAccessible = true | ||
| authenticator = constructor.newInstance(targetContext) as AbstractAccountAuthenticator | ||
| } | ||
|
|
||
| @After | ||
| fun tearDown() { | ||
| unmockkAll() | ||
| } | ||
|
|
||
| private fun setupTokenErrorResponse(error: String, errorDescription: String) { | ||
| val errorBody = """ | ||
| {"error": "$error", "error_description": "$errorDescription"} | ||
| """.trimIndent().toResponseBody("application/json; charset=utf-8".toMediaType()) | ||
| every { HttpAccess.DEFAULT.okHttpClient } returns mockk<OkHttpClient> { | ||
| every { newCall(any()) } returns mockk<Call> { | ||
| every { execute() } returns mockk<Response>(relaxed = true) { | ||
| every { isSuccessful } returns false | ||
| every { code } returns 400 | ||
| every { body } returns errorBody | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| fun testGetAuthToken_userBlockedRetry_returnsErrorBundle() { | ||
| setupTokenErrorResponse("user_blocked_retry", "Attestation verification pending") | ||
|
|
||
| val result = authenticator.getAuthToken(null, mockAccount, "authTokenType", null) | ||
|
|
||
| assertEquals("user_blocked_retry", result.getString(AccountManager.KEY_ERROR_CODE)) | ||
| assertEquals("Attestation verification pending", result.getString(AccountManager.KEY_ERROR_MESSAGE)) | ||
| assertNull(result.getParcelable<android.content.Intent>(AccountManager.KEY_INTENT)) | ||
| } | ||
|
|
||
| @Test | ||
| fun testGetAuthToken_userBlocked_returnsLoginIntent() { | ||
| setupTokenErrorResponse("user_blocked", "Device failed integrity check") | ||
|
|
||
| val result = authenticator.getAuthToken(null, mockAccount, "authTokenType", null) | ||
|
|
||
| assertNotNull(result.getParcelable<android.content.Intent>(AccountManager.KEY_INTENT)) | ||
| assertNull(result.getString(AccountManager.KEY_ERROR_CODE)) | ||
| } | ||
|
|
||
| @Test | ||
| fun testGetAuthToken_invalidGrant_returnsLoginIntent() { | ||
| setupTokenErrorResponse("invalid_grant", "expired authorization code") | ||
|
|
||
| val result = authenticator.getAuthToken(null, mockAccount, "authTokenType", null) | ||
|
|
||
| assertNotNull(result.getParcelable<android.content.Intent>(AccountManager.KEY_INTENT)) | ||
| assertNull(result.getString(AccountManager.KEY_ERROR_CODE)) | ||
| } | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requirement #4: For the AuthenticatorService path,
user_blocked_retrymust NOT redirect to login. SinceisRefreshTokenInvalid()only checks HTTP status codes (400/401/403) anduser_blocked_retryreturns 400, we check the error type first. The combined condition ensures retriable errors return an error bundle while terminal errors still redirect to login.