From b4b5429ecbe7b15ede9fcb7d0e0437f95991073e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=E2=89=A1ZRS?= <12814349+LZRS@users.noreply.github.com> Date: Wed, 27 Sep 2023 09:47:34 +0300 Subject: [PATCH] Fix crash for uncaught exceptions when refresh token request fails (#2772) * Fix crash for uncaught exceptions on refreshing token * Update android/quest/src/main/java/org/smartregister/fhircore/quest/ui/login/AccountAuthenticator.kt --------- Co-authored-by: Peter Lubell-Doughtie --- .../data/remote/shared/TokenAuthenticator.kt | 67 ++++++++++--------- .../engine/auth/TokenAuthenticatorTest.kt | 16 +++-- .../quest/ui/login/AccountAuthenticator.kt | 9 +-- .../ui/login/AccountAuthenticatorTest.kt | 9 ++- 4 files changed, 55 insertions(+), 46 deletions(-) diff --git a/android/engine/src/main/java/org/smartregister/fhircore/engine/data/remote/shared/TokenAuthenticator.kt b/android/engine/src/main/java/org/smartregister/fhircore/engine/data/remote/shared/TokenAuthenticator.kt index 19f355ea2e..08593a82a7 100644 --- a/android/engine/src/main/java/org/smartregister/fhircore/engine/data/remote/shared/TokenAuthenticator.kt +++ b/android/engine/src/main/java/org/smartregister/fhircore/engine/data/remote/shared/TokenAuthenticator.kt @@ -67,40 +67,43 @@ constructor( private var isLoginPageRendered = false fun getAccessToken(): String { - val account = findAccount() - return if (account != null) { - val accessToken = accountManager.peekAuthToken(account, AUTH_TOKEN_TYPE) ?: "" - if (!isTokenActive(accessToken)) { - accountManager.run { - invalidateAuthToken(account.type, accessToken) - try { - getAuthToken( - account, - AUTH_TOKEN_TYPE, - bundleOf(), - true, - handleAccountManagerFutureCallback(account), - Handler(Looper.getMainLooper()) { message: Message -> - Timber.e(message.toString()) - true - }, - ) - } catch (operationCanceledException: OperationCanceledException) { - Timber.e(operationCanceledException) - } catch (ioException: IOException) { - Timber.e(ioException) - } catch (authenticatorException: AuthenticatorException) { - Timber.e(authenticatorException) - // TODO: Should we cancel the sync job to avoid retries when offline? - } - } - } else { - isLoginPageRendered = false + val account = findAccount() ?: return "" + val accessToken = accountManager.peekAuthToken(account, AUTH_TOKEN_TYPE) ?: "" + if (isTokenActive(accessToken)) { + isLoginPageRendered = false + return accessToken + } + + accountManager.invalidateAuthToken(account.type, accessToken) + val authTokenBundleFuture = + with(accountManager) { + getAuthToken( + account, + AUTH_TOKEN_TYPE, + bundleOf(), + true, + handleAccountManagerFutureCallback(account), + Handler(Looper.getMainLooper()) { message: Message -> + Timber.e(message.toString()) + true + }, + ) } - accessToken - } else { - "" + + try { + val authTokenBundle = authTokenBundleFuture.result + if (authTokenBundle.containsKey(AccountManager.KEY_AUTHTOKEN)) { + return authTokenBundle.getString(AccountManager.KEY_AUTHTOKEN)!! // refreshed token + } + } catch (operationCanceledException: OperationCanceledException) { + Timber.e(operationCanceledException) + } catch (ioException: IOException) { + Timber.e(ioException) + } catch (authenticatorException: AuthenticatorException) { + Timber.e(authenticatorException) + // TODO: Should we cancel the sync job to avoid retries when offline? } + return accessToken } private fun AccountManager.handleAccountManagerFutureCallback(account: Account?) = diff --git a/android/engine/src/test/java/org/smartregister/fhircore/engine/auth/TokenAuthenticatorTest.kt b/android/engine/src/test/java/org/smartregister/fhircore/engine/auth/TokenAuthenticatorTest.kt index d99f3fdb28..56397cb32e 100644 --- a/android/engine/src/test/java/org/smartregister/fhircore/engine/auth/TokenAuthenticatorTest.kt +++ b/android/engine/src/test/java/org/smartregister/fhircore/engine/auth/TokenAuthenticatorTest.kt @@ -18,9 +18,11 @@ package org.smartregister.fhircore.engine.auth import android.accounts.Account import android.accounts.AccountManager +import android.accounts.AccountManagerFuture import android.accounts.AuthenticatorException import android.accounts.OperationCanceledException import android.os.Bundle +import androidx.core.os.bundleOf import androidx.test.core.app.ApplicationProvider import dagger.hilt.android.testing.HiltAndroidRule import dagger.hilt.android.testing.HiltAndroidTest @@ -121,13 +123,15 @@ class TokenAuthenticatorTest : RobolectricTest() { fun testGetAccessTokenShouldInvalidateExpiredToken() { val account = Account(sampleUsername, PROVIDER) val accessToken = "gibberishaccesstoken" + val accountManagerFuture = mockk>() every { tokenAuthenticator.findAccount() } returns account every { tokenAuthenticator.isTokenActive(any()) } returns false every { accountManager.peekAuthToken(account, AUTH_TOKEN_TYPE) } returns accessToken every { accountManager.invalidateAuthToken(account.type, accessToken) } just runs + every { accountManagerFuture.result } returns bundleOf() every { accountManager.getAuthToken(account, AUTH_TOKEN_TYPE, any(), true, any(), any()) - } returns mockk() + } returns accountManagerFuture tokenAuthenticator.getAccessToken() @@ -146,22 +150,26 @@ class TokenAuthenticatorTest : RobolectricTest() { @Test fun testGetAccessTokenShouldCatchOperationCanceledAndIOAndAuthenticatorExceptions() { val account = Account(sampleUsername, PROVIDER) + val accountManagerFutureBundle = mockk>() every { tokenAuthenticator.findAccount() } returns account every { tokenAuthenticator.isTokenActive(any()) } returns false val accessToken = "gibberishaccesstoken" every { accountManager.peekAuthToken(account, AUTH_TOKEN_TYPE) } returns accessToken every { accountManager.invalidateAuthToken(account.type, accessToken) } just runs + every { accountManagerFutureBundle.result } throws OperationCanceledException() every { accountManager.getAuthToken(account, AUTH_TOKEN_TYPE, any(), true, any(), any()) - } throws OperationCanceledException() + } returns accountManagerFutureBundle Assert.assertEquals(accessToken, tokenAuthenticator.getAccessToken()) + every { accountManagerFutureBundle.result } throws IOException() every { accountManager.getAuthToken(account, AUTH_TOKEN_TYPE, any(), true, any(), any()) - } throws IOException() + } returns accountManagerFutureBundle Assert.assertEquals(accessToken, tokenAuthenticator.getAccessToken()) + every { accountManagerFutureBundle.result } throws AuthenticatorException() every { accountManager.getAuthToken(account, AUTH_TOKEN_TYPE, any(), true, any(), any()) - } throws AuthenticatorException() + } returns accountManagerFutureBundle Assert.assertEquals(accessToken, tokenAuthenticator.getAccessToken()) } diff --git a/android/quest/src/main/java/org/smartregister/fhircore/quest/ui/login/AccountAuthenticator.kt b/android/quest/src/main/java/org/smartregister/fhircore/quest/ui/login/AccountAuthenticator.kt index 3f98aef7bf..2fed16fd1e 100644 --- a/android/quest/src/main/java/org/smartregister/fhircore/quest/ui/login/AccountAuthenticator.kt +++ b/android/quest/src/main/java/org/smartregister/fhircore/quest/ui/login/AccountAuthenticator.kt @@ -25,12 +25,10 @@ import android.content.Intent import android.os.Bundle import androidx.core.os.bundleOf import dagger.hilt.android.qualifiers.ApplicationContext -import java.net.UnknownHostException import java.util.Locale import javax.inject.Inject import org.smartregister.fhircore.engine.data.remote.shared.TokenAuthenticator import org.smartregister.fhircore.engine.data.remote.shared.TokenAuthenticator.Companion.AUTH_TOKEN_TYPE -import retrofit2.HttpException import timber.log.Timber class AccountAuthenticator @@ -82,11 +80,8 @@ constructor( tokenAuthenticator.refreshToken(refreshToken) } catch (ex: Exception) { Timber.e(ex) - when (ex) { - is HttpException, - is UnknownHostException, -> "" - else -> throw ex - } + // any form of exception will unset token to empty, thereby forcing re-login + "" } } } diff --git a/android/quest/src/test/java/org/smartregister/fhircore/quest/ui/login/AccountAuthenticatorTest.kt b/android/quest/src/test/java/org/smartregister/fhircore/quest/ui/login/AccountAuthenticatorTest.kt index 4a8d2976be..c5f8b60b80 100644 --- a/android/quest/src/test/java/org/smartregister/fhircore/quest/ui/login/AccountAuthenticatorTest.kt +++ b/android/quest/src/test/java/org/smartregister/fhircore/quest/ui/login/AccountAuthenticatorTest.kt @@ -179,8 +179,8 @@ class AccountAuthenticatorTest : RobolectricTest() { Assert.assertFalse(authTokenBundle.containsKey(AccountManager.KEY_AUTHTOKEN)) } - @Test(expected = RuntimeException::class) - fun testGetBundleWithoutAuthInfoWhenCaughtUnknownHost() { + @Test + fun testGetBundleWithoutAuthInfoWhenRefreshTokenFailsWithRuntimeException() { every { tokenAuthenticator.isTokenActive(any()) } returns false val account = spyk(Account("newAccName", "newAccType")) every { accountManager.peekAuthToken(account, authTokenType) } returns "" @@ -189,7 +189,10 @@ class AccountAuthenticatorTest : RobolectricTest() { every { accountManager.getPassword(account) } returns refreshToken every { tokenAuthenticator.refreshToken(refreshToken) } throws RuntimeException() - accountAuthenticator.getAuthToken(null, account, authTokenType, bundleOf()) + accountAuthenticator.getAuthToken(null, account, authTokenType, bundleOf()).also { + Assert.assertNotNull(it) + Assert.assertFalse(it.containsKey(AccountManager.KEY_AUTHTOKEN)) + } } @Test