From 962962a73175207b12a74ab75aa72beb7dfa0015 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=E2=89=A1ZRS?= <12814349+LZRS@users.noreply.github.com> Date: Fri, 8 Sep 2023 17:38:49 +0300 Subject: [PATCH] Fix crash occurring when authentication fails after initial log-in --- .../engine/auth/AccountAuthenticator.kt | 7 +-- .../data/remote/shared/TokenAuthenticator.kt | 63 ++++++++++--------- .../fhircore/engine/di/CoreModule.kt | 3 + .../fhircore/engine/sync/SyncBroadcaster.kt | 31 ++++++--- .../engine/auth/AccountAuthenticatorTest.kt | 7 ++- .../engine/auth/TokenAuthenticatorTest.kt | 1 + .../engine/sync/SyncBroadcasterTest.kt | 52 ++++++++++++++- .../userprofile/UserProfileViewModelTest.kt | 5 ++ 8 files changed, 120 insertions(+), 49 deletions(-) diff --git a/android/engine/src/main/java/org/smartregister/fhircore/engine/auth/AccountAuthenticator.kt b/android/engine/src/main/java/org/smartregister/fhircore/engine/auth/AccountAuthenticator.kt index 32ebc64576..a9f39dd35c 100644 --- a/android/engine/src/main/java/org/smartregister/fhircore/engine/auth/AccountAuthenticator.kt +++ b/android/engine/src/main/java/org/smartregister/fhircore/engine/auth/AccountAuthenticator.kt @@ -28,14 +28,12 @@ import android.os.Handler import android.os.Looper 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 org.smartregister.fhircore.engine.ui.login.LoginActivity import org.smartregister.fhircore.engine.util.SecureSharedPreference -import retrofit2.HttpException import timber.log.Timber class AccountAuthenticator @@ -88,10 +86,7 @@ constructor( tokenAuthenticator.refreshToken(refreshToken) } catch (ex: Exception) { Timber.e(ex) - when (ex) { - is HttpException, is UnknownHostException -> "" - else -> throw ex - } + "" // Set to EMPTY, so as to redirect to log in screen, and try again } } } 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 be746a28f1..2600130faa 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 @@ -66,38 +66,42 @@ constructor( private var isLoginPageRendered = false override 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) + 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 authResultBundle = + try { + val authResultFuture = + accountManager.getAuthToken( + account, + AUTH_TOKEN_TYPE, + bundleOf(), + true, + accountManager.handleAccountManagerFutureCallback(account), + Handler(Looper.getMainLooper()) { message: Message -> + Timber.e(message.toString()) + true + } + ) + authResultFuture.result + } catch (exception: Exception) { + when (exception) { + is OperationCanceledException, is AuthenticatorException, is IOException -> { // TODO: Should we cancel the sync job to avoid retries when offline? + Timber.e(exception) + bundleOf(AccountManager.KEY_AUTHTOKEN to accessToken) } + else -> bundleOf() } - } else { - isLoginPageRendered = false } - accessToken - } else "" + return if (authResultBundle.containsKey(AccountManager.KEY_AUTHTOKEN)) + authResultBundle.getString(AccountManager.KEY_AUTHTOKEN)!! + else "" } private fun AccountManager.handleAccountManagerFutureCallback(account: Account?) = @@ -248,8 +252,7 @@ constructor( } } - fun sessionActive(): Boolean = - findAccount()?.let { isTokenActive(accountManager.peekAuthToken(it, AUTH_TOKEN_TYPE)) } ?: false + fun sessionActive(): Boolean = isTokenActive(getAccessToken()) fun invalidateSession(onSessionInvalidated: () -> Unit) { findAccount()?.let { account -> diff --git a/android/engine/src/main/java/org/smartregister/fhircore/engine/di/CoreModule.kt b/android/engine/src/main/java/org/smartregister/fhircore/engine/di/CoreModule.kt index e43f00e006..1e7a50b407 100644 --- a/android/engine/src/main/java/org/smartregister/fhircore/engine/di/CoreModule.kt +++ b/android/engine/src/main/java/org/smartregister/fhircore/engine/di/CoreModule.kt @@ -34,6 +34,7 @@ import org.smartregister.fhircore.engine.configuration.ConfigurationRegistry import org.smartregister.fhircore.engine.configuration.app.ConfigService import org.smartregister.fhircore.engine.data.local.DefaultRepository import org.smartregister.fhircore.engine.data.local.register.dao.HivRegisterDao +import org.smartregister.fhircore.engine.data.remote.shared.TokenAuthenticator import org.smartregister.fhircore.engine.domain.repository.PatientDao import org.smartregister.fhircore.engine.sync.SyncBroadcaster import org.smartregister.fhircore.engine.trace.PerformanceReporter @@ -51,6 +52,7 @@ class CoreModule { configService: ConfigService, fhirEngine: FhirEngine, tracer: PerformanceReporter, + tokenAuthenticator: TokenAuthenticator, sharedPreferencesHelper: SharedPreferencesHelper ) = SyncBroadcaster( @@ -59,6 +61,7 @@ class CoreModule { fhirEngine = fhirEngine, appContext = context, tracer = tracer, + tokenAuthenticator = tokenAuthenticator, sharedPreferencesHelper = sharedPreferencesHelper ) diff --git a/android/engine/src/main/java/org/smartregister/fhircore/engine/sync/SyncBroadcaster.kt b/android/engine/src/main/java/org/smartregister/fhircore/engine/sync/SyncBroadcaster.kt index 4d5ff648b9..c166a97e6d 100644 --- a/android/engine/src/main/java/org/smartregister/fhircore/engine/sync/SyncBroadcaster.kt +++ b/android/engine/src/main/java/org/smartregister/fhircore/engine/sync/SyncBroadcaster.kt @@ -42,6 +42,7 @@ import org.hl7.fhir.r4.model.ResourceType import org.smartregister.fhircore.engine.R import org.smartregister.fhircore.engine.configuration.ConfigurationRegistry import org.smartregister.fhircore.engine.configuration.app.ConfigService +import org.smartregister.fhircore.engine.data.remote.shared.TokenAuthenticator import org.smartregister.fhircore.engine.trace.PerformanceReporter import org.smartregister.fhircore.engine.util.DefaultDispatcherProvider import org.smartregister.fhircore.engine.util.DispatcherProvider @@ -64,6 +65,7 @@ constructor( val dispatcherProvider: DispatcherProvider = DefaultDispatcherProvider(), val tracer: PerformanceReporter, val sharedPreferencesHelper: SharedPreferencesHelper, + val tokenAuthenticator: TokenAuthenticator, @ApplicationContext val appContext: Context ) { /** @@ -112,17 +114,26 @@ constructor( fun runSync(networkState: (Context) -> Boolean = { NetworkState(it).invoke() }) { Timber.i("Running one-time sync...") CoroutineScope(dispatcherProvider.io()).launch { - networkState(appContext).apply { - if (this) { - Sync.oneTimeSync(appContext) - getWorkerInfo().collect { sharedSyncStatus.emit(it) } - } else { - val message = appContext.getString(R.string.unable_to_sync) - val resourceSyncException = - listOf(ResourceSyncException(ResourceType.Flag, java.lang.Exception(message))) - sharedSyncStatus.emit(SyncJobStatus.Failed(resourceSyncException)) - } + val isConnected = networkState(appContext) + if (!isConnected) { + val message = appContext.getString(R.string.unable_to_sync) + val resourceSyncException = + listOf(ResourceSyncException(ResourceType.Flag, java.lang.Exception(message))) + sharedSyncStatus.emit(SyncJobStatus.Failed(resourceSyncException)) + return@launch } + val isAuthenticated = tokenAuthenticator.sessionActive() + if (!isAuthenticated) { + val authFailResourceSyncException = + ResourceSyncException( + ResourceType.Flag, + Exception(appContext.getString(R.string.sync_authentication_error)) + ) + sharedSyncStatus.emit(SyncJobStatus.Failed(listOf(authFailResourceSyncException))) + return@launch + } + Sync.oneTimeSync(appContext) + getWorkerInfo().collect { sharedSyncStatus.emit(it) } } } diff --git a/android/engine/src/test/java/org/smartregister/fhircore/engine/auth/AccountAuthenticatorTest.kt b/android/engine/src/test/java/org/smartregister/fhircore/engine/auth/AccountAuthenticatorTest.kt index 1e9c2b4996..6a6d8e9ddd 100644 --- a/android/engine/src/test/java/org/smartregister/fhircore/engine/auth/AccountAuthenticatorTest.kt +++ b/android/engine/src/test/java/org/smartregister/fhircore/engine/auth/AccountAuthenticatorTest.kt @@ -323,7 +323,6 @@ class AccountAuthenticatorTest : RobolectricTest() { Assert.assertFalse(authTokenBundle.containsKey(KEY_AUTHTOKEN)) } - @Test(expected = RuntimeException::class) fun testGetBundleWithoutAuthInfoWhenCaughtUnknownHost() { every { tokenAuthenticator.isTokenActive(any()) } returns false val account = spyk(Account("newAccName", "newAccType")) @@ -333,7 +332,11 @@ class AccountAuthenticatorTest : RobolectricTest() { every { accountManager.getPassword(account) } returns refreshToken every { tokenAuthenticator.refreshToken(refreshToken) } throws RuntimeException() - accountAuthenticator.getAuthToken(null, account, authTokenType, bundleOf()) + val authTokenBundle = + accountAuthenticator.getAuthToken(null, account, authTokenType, bundleOf()) + Assert.assertNotNull(authTokenBundle) + Assert.assertFalse(authTokenBundle.containsKey(KEY_AUTHTOKEN)) + Assert.assertTrue(authTokenBundle.containsKey(KEY_INTENT)) // contains intent to re-login } @Test 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 c7c1651fc9..f2037ccd90 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 @@ -462,6 +462,7 @@ class TokenAuthenticatorTest : RobolectricTest() { every { accountManager.peekAuthToken(account, TokenAuthenticator.AUTH_TOKEN_TYPE) } returns token every { tokenAuthenticator.isTokenActive(any()) } returns false + every { accountManager.invalidateAuthToken(any(), any()) } just runs Assert.assertFalse(tokenAuthenticator.sessionActive()) } diff --git a/android/engine/src/test/java/org/smartregister/fhircore/engine/sync/SyncBroadcasterTest.kt b/android/engine/src/test/java/org/smartregister/fhircore/engine/sync/SyncBroadcasterTest.kt index afeb697172..90affa5242 100644 --- a/android/engine/src/test/java/org/smartregister/fhircore/engine/sync/SyncBroadcasterTest.kt +++ b/android/engine/src/test/java/org/smartregister/fhircore/engine/sync/SyncBroadcasterTest.kt @@ -51,8 +51,10 @@ import org.junit.After import org.junit.Assert import org.junit.Before import org.junit.Test +import org.smartregister.fhircore.engine.R import org.smartregister.fhircore.engine.app.AppConfigService import org.smartregister.fhircore.engine.app.fakes.Faker +import org.smartregister.fhircore.engine.data.remote.shared.TokenAuthenticator import org.smartregister.fhircore.engine.robolectric.RobolectricTest import org.smartregister.fhircore.engine.rule.CoroutineTestRule import org.smartregister.fhircore.engine.trace.FakePerformanceReporter @@ -65,6 +67,7 @@ class SyncBroadcasterTest : RobolectricTest() { private lateinit var syncBroadcaster: SyncBroadcaster private lateinit var workManager: WorkManager + private lateinit var tokenAuthenticator: TokenAuthenticator private val sharedSyncStatus: MutableSharedFlow = MutableSharedFlow() private val sharedPreferencesHelper: SharedPreferencesHelper = mockk() private lateinit var tracer: PerformanceReporter @@ -75,6 +78,9 @@ class SyncBroadcasterTest : RobolectricTest() { mockkStatic(WorkManager::class) workManager = mockk() tracer = mockk() + tokenAuthenticator = mockk() + + every { tokenAuthenticator.sessionActive() } returns true val mutableLiveData = MutableLiveData(listOf()) every { WorkManager.getInstance(appContext) } returns workManager @@ -103,6 +109,7 @@ class SyncBroadcasterTest : RobolectricTest() { sharedSyncStatus = sharedSyncStatus, dispatcherProvider = CoroutineTestRule().testDispatcherProvider, tracer = tracer, + tokenAuthenticator = tokenAuthenticator, sharedPreferencesHelper = sharedPreferencesHelper, appContext = mockk(relaxed = true) ) @@ -201,6 +208,7 @@ class SyncBroadcasterTest : RobolectricTest() { dispatcherProvider = CoroutineTestRule().testDispatcherProvider, appContext = context, tracer = FakePerformanceReporter(), + tokenAuthenticator = tokenAuthenticator, sharedPreferencesHelper = sharedPreferencesHelper ) val collectedSyncStatusList = mutableListOf() @@ -209,7 +217,49 @@ class SyncBroadcasterTest : RobolectricTest() { sharedSyncStatus.toList(collectedSyncStatusList) } syncBroadcaster.runSync { false } - Assert.assertTrue(collectedSyncStatusList.first() is SyncJobStatus.Failed) + val syncStatus = collectedSyncStatusList.first() + Assert.assertTrue(syncStatus is SyncJobStatus.Failed) + syncStatus as SyncJobStatus.Failed + Assert.assertEquals( + context.getString(R.string.unable_to_sync), + syncStatus.exceptions.first().exception.message + ) + job.cancel() + } + + @Test + fun shouldEmitSyncFailedWhenNetworkAndTokenSessionIsNotActive() = runTest { + val configurationRegistry = Faker.buildTestConfigurationRegistry() + val context = ApplicationProvider.getApplicationContext() + val configService = AppConfigService(context = context) + val sharedSyncStatus: MutableSharedFlow = MutableSharedFlow() + val tokenAuthenticatorAlt = mockk() + every { tokenAuthenticatorAlt.sessionActive() } returns false + syncBroadcaster = + SyncBroadcaster( + configurationRegistry, + configService, + fhirEngine = mockk(), + sharedSyncStatus, + dispatcherProvider = CoroutineTestRule().testDispatcherProvider, + appContext = context, + tracer = FakePerformanceReporter(), + tokenAuthenticator = tokenAuthenticatorAlt, + sharedPreferencesHelper = sharedPreferencesHelper + ) + val collectedSyncStatusList = mutableListOf() + val job = + launch(UnconfinedTestDispatcher(testScheduler)) { + sharedSyncStatus.toList(collectedSyncStatusList) + } + syncBroadcaster.runSync { true } + val syncStatus = collectedSyncStatusList.first() + Assert.assertTrue(syncStatus is SyncJobStatus.Failed) + syncStatus as SyncJobStatus.Failed + Assert.assertEquals( + context.getString(R.string.sync_authentication_error), + syncStatus.exceptions.first().exception.message + ) job.cancel() } } diff --git a/android/engine/src/test/java/org/smartregister/fhircore/engine/ui/userprofile/UserProfileViewModelTest.kt b/android/engine/src/test/java/org/smartregister/fhircore/engine/ui/userprofile/UserProfileViewModelTest.kt index f196ed8ee9..0b1348a70a 100644 --- a/android/engine/src/test/java/org/smartregister/fhircore/engine/ui/userprofile/UserProfileViewModelTest.kt +++ b/android/engine/src/test/java/org/smartregister/fhircore/engine/ui/userprofile/UserProfileViewModelTest.kt @@ -43,6 +43,7 @@ import org.smartregister.fhircore.engine.auth.AccountAuthenticator import org.smartregister.fhircore.engine.configuration.app.ConfigService import org.smartregister.fhircore.engine.data.remote.fhir.resource.FhirResourceDataSource import org.smartregister.fhircore.engine.data.remote.fhir.resource.FhirResourceService +import org.smartregister.fhircore.engine.data.remote.shared.TokenAuthenticator import org.smartregister.fhircore.engine.domain.model.Language import org.smartregister.fhircore.engine.robolectric.RobolectricTest import org.smartregister.fhircore.engine.rule.CoroutineTestRule @@ -63,6 +64,7 @@ class UserProfileViewModelTest : RobolectricTest() { lateinit var sharedPreferencesHelper: SharedPreferencesHelper @BindValue var configurationRegistry = Faker.buildTestConfigurationRegistry() + var tokenAuthenticator: TokenAuthenticator = mockk() private lateinit var configService: ConfigService @@ -81,6 +83,8 @@ class UserProfileViewModelTest : RobolectricTest() { configService = AppConfigService(context = context) fhirResourceDataSource = spyk(FhirResourceDataSource(resourceService)) + every { tokenAuthenticator.sessionActive() } returns true + syncBroadcaster = SyncBroadcaster( configurationRegistry, @@ -90,6 +94,7 @@ class UserProfileViewModelTest : RobolectricTest() { dispatcherProvider = CoroutineTestRule().testDispatcherProvider, appContext = context, tracer = mockk(), + tokenAuthenticator = tokenAuthenticator, sharedPreferencesHelper = sharedPreferencesHelper ) userProfileViewModel =