From 1fdfbac7b722c55510995d27642df52d096e170f Mon Sep 17 00:00:00 2001 From: David Perez Date: Fri, 19 Jul 2024 11:05:24 -0500 Subject: [PATCH] Add timeouts to operations that could hang (#3553) --- .../provider/AutofillCipherProviderImpl.kt | 23 +++- .../CipherMatchingManagerImpl.kt | 10 +- .../data/platform/util/FlowExtensions.kt | 22 ++++ .../processor/AutofillCipherProviderTest.kt | 73 +++++++++++- .../CipherMatchingManagerTest.kt | 35 +++++- .../data/platform/util/FlowExtensionsTest.kt | 112 ++++++++++++++++++ 6 files changed, 266 insertions(+), 9 deletions(-) create mode 100644 app/src/main/java/com/x8bit/bitwarden/data/platform/util/FlowExtensions.kt create mode 100644 app/src/test/java/com/x8bit/bitwarden/data/platform/util/FlowExtensionsTest.kt diff --git a/app/src/main/java/com/x8bit/bitwarden/data/autofill/provider/AutofillCipherProviderImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/autofill/provider/AutofillCipherProviderImpl.kt index 01265cb953..71c152575b 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/autofill/provider/AutofillCipherProviderImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/autofill/provider/AutofillCipherProviderImpl.kt @@ -6,11 +6,22 @@ import com.bitwarden.vault.CipherView import com.x8bit.bitwarden.data.auth.repository.AuthRepository import com.x8bit.bitwarden.data.autofill.model.AutofillCipher import com.x8bit.bitwarden.data.platform.manager.ciphermatching.CipherMatchingManager +import com.x8bit.bitwarden.data.platform.util.firstWithTimeoutOrNull import com.x8bit.bitwarden.data.platform.util.subtitle import com.x8bit.bitwarden.data.vault.repository.VaultRepository import com.x8bit.bitwarden.data.vault.repository.model.VaultUnlockData import com.x8bit.bitwarden.data.vault.repository.util.statusFor -import kotlinx.coroutines.flow.first + +/** + * The duration, in milliseconds, we should wait while waiting for the vault status to not be + * 'UNLOCKING' before proceeding. + */ +private const val VAULT_LOCKED_TIMEOUT_MS: Long = 500L + +/** + * The duration, in milliseconds, we should wait while retrieving ciphers before proceeding. + */ +private const val GET_CIPHERS_TIMEOUT_MS: Long = 5_000L /** * The default [AutofillCipherProvider] implementation. This service is used for getting current @@ -28,9 +39,11 @@ class AutofillCipherProviderImpl( // Wait for any unlocking actions to finish. This can be relevant on startup for Never lock // accounts. - vaultRepository.vaultUnlockDataStateFlow.first { - it.statusFor(userId) != VaultUnlockData.Status.UNLOCKING - } + vaultRepository + .vaultUnlockDataStateFlow + .firstWithTimeoutOrNull(timeMillis = VAULT_LOCKED_TIMEOUT_MS) { + it.statusFor(userId = userId) != VaultUnlockData.Status.UNLOCKING + } return !vaultRepository.isVaultUnlocked(userId = userId) } @@ -105,6 +118,6 @@ class AutofillCipherProviderImpl( vaultRepository .ciphersStateFlow .takeUnless { isVaultLocked() } - ?.first { it.data != null } + ?.firstWithTimeoutOrNull(timeMillis = GET_CIPHERS_TIMEOUT_MS) { it.data != null } ?.data } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/ciphermatching/CipherMatchingManagerImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/ciphermatching/CipherMatchingManagerImpl.kt index 6fc6e6215d..0bd367fa66 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/ciphermatching/CipherMatchingManagerImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/manager/ciphermatching/CipherMatchingManagerImpl.kt @@ -5,6 +5,7 @@ import com.bitwarden.vault.CipherView import com.bitwarden.vault.LoginUriView import com.bitwarden.vault.UriMatchType import com.x8bit.bitwarden.data.platform.repository.SettingsRepository +import com.x8bit.bitwarden.data.platform.util.firstWithTimeoutOrNull import com.x8bit.bitwarden.data.platform.util.getDomainOrNull import com.x8bit.bitwarden.data.platform.util.getHostWithPortOrNull import com.x8bit.bitwarden.data.platform.util.getWebHostFromAndroidUriOrNull @@ -13,7 +14,6 @@ import com.x8bit.bitwarden.data.platform.util.regexOrNull import com.x8bit.bitwarden.data.vault.repository.VaultRepository import com.x8bit.bitwarden.data.vault.repository.model.DomainsData import com.x8bit.bitwarden.ui.platform.feature.settings.autofill.util.toSdkUriMatchType -import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.mapNotNull import kotlin.text.RegexOption import kotlin.text.isNullOrBlank @@ -21,6 +21,11 @@ import kotlin.text.lowercase import kotlin.text.matches import kotlin.text.startsWith +/** + * The duration, in milliseconds, we should wait while retrieving domain data before we proceed. + */ +private const val GET_DOMAINS_TIMEOUT_MS: Long = 1_000L + /** * The default [CipherMatchingManager] implementation. This class is responsible for matching * ciphers based on special criteria. @@ -37,7 +42,8 @@ class CipherMatchingManagerImpl( val equivalentDomainsData = vaultRepository .domainsStateFlow .mapNotNull { it.data } - .first() + .firstWithTimeoutOrNull(timeMillis = GET_DOMAINS_TIMEOUT_MS) + ?: return emptyList() val isAndroidApp = matchUri.isAndroidApp() val defaultUriMatchType = settingsRepository.defaultUriMatchType.toSdkUriMatchType() diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/util/FlowExtensions.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/util/FlowExtensions.kt new file mode 100644 index 0000000000..95e0eae53a --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/util/FlowExtensions.kt @@ -0,0 +1,22 @@ +package com.x8bit.bitwarden.data.platform.util + +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.withTimeoutOrNull + +/** + * Returns the first element emitted by the [Flow] or `null` if the operation exceeds the given + * timeout of [timeMillis]. + */ +suspend fun Flow.firstWithTimeoutOrNull( + timeMillis: Long, +): T? = withTimeoutOrNull(timeMillis = timeMillis) { first() } + +/** + * Returns the first element emitted by the [Flow] matching the given [predicate] or `null` if the + * operation exceeds the given timeout of [timeMillis]. + */ +suspend fun Flow.firstWithTimeoutOrNull( + timeMillis: Long, + predicate: suspend (T) -> Boolean, +): T? = withTimeoutOrNull(timeMillis = timeMillis) { first(predicate) } diff --git a/app/src/test/java/com/x8bit/bitwarden/data/autofill/processor/AutofillCipherProviderTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/autofill/processor/AutofillCipherProviderTest.kt index 22cb56003c..e21601d561 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/autofill/processor/AutofillCipherProviderTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/autofill/processor/AutofillCipherProviderTest.kt @@ -21,6 +21,7 @@ import io.mockk.every import io.mockk.mockk import io.mockk.mockkStatic import io.mockk.unmockkStatic +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.async import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.test.runTest @@ -31,6 +32,7 @@ import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test +@OptIn(ExperimentalCoroutinesApi::class) class AutofillCipherProviderTest { private val cardView: CardView = mockk { every { cardholderName } returns CARD_CARDHOLDER_NAME @@ -139,7 +141,7 @@ class AutofillCipherProviderTest { autofillCipherProvider.isVaultLocked() } - testScheduler.advanceUntilIdle() + testScheduler.runCurrent() assertFalse(result.isCompleted) mutableVaultStateFlow.value = listOf( @@ -155,6 +157,53 @@ class AutofillCipherProviderTest { assertFalse(result.await()) } + @Suppress("MaxLineLength") + @Test + fun `isVaultLocked when there is an active user should wait for pending unlocking to finish and return the locked state for that user when it times out`() = + runTest { + every { authRepository.activeUserId } returns ACTIVE_USER_ID + mutableVaultStateFlow.value = listOf( + VaultUnlockData( + userId = ACTIVE_USER_ID, + status = VaultUnlockData.Status.UNLOCKING, + ), + ) + + val result = async { + autofillCipherProvider.isVaultLocked() + } + + testScheduler.runCurrent() + assertFalse(result.isCompleted) + + testScheduler.advanceTimeBy(delayTimeMillis = 1_000L) + testScheduler.runCurrent() + assertTrue(result.isCompleted) + assertTrue(result.await()) + } + + @Suppress("MaxLineLength") + @Test + fun `getCardAutofillCiphers when unlocked should return empty list when retrieving ciphers times out`() = + runTest { + coEvery { vaultRepository.isVaultUnlocked(ACTIVE_USER_ID) } returns true + mutableCiphersStateFlow.value = DataState.Loading + + // Test + val actual = async { + autofillCipherProvider.getCardAutofillCiphers() + } + + testScheduler.runCurrent() + assertFalse(actual.isCompleted) + testScheduler.advanceTimeBy(delayTimeMillis = 5_000L) + testScheduler.runCurrent() + + // Verify + assertTrue(actual.isCompleted) + assertEquals(emptyList(), actual.await()) + } + @Suppress("MaxLineLength") @Test fun `getCardAutofillCiphers when unlocked should return non-null, non-deleted, and non-reprompt card ciphers`() = @@ -205,6 +254,28 @@ class AutofillCipherProviderTest { assertEquals(emptyList(), actual) } + @Suppress("MaxLineLength") + @Test + fun `getLoginAutofillCiphers when unlocked should return empty list when retrieving ciphers times out`() = + runTest { + coEvery { vaultRepository.isVaultUnlocked(ACTIVE_USER_ID) } returns true + mutableCiphersStateFlow.value = DataState.Loading + + // Test + val actual = async { + autofillCipherProvider.getLoginAutofillCiphers(uri = URI) + } + + testScheduler.runCurrent() + assertFalse(actual.isCompleted) + testScheduler.advanceTimeBy(delayTimeMillis = 5_000L) + testScheduler.runCurrent() + + // Verify + assertTrue(actual.isCompleted) + assertEquals(emptyList(), actual.await()) + } + @Suppress("MaxLineLength") @Test fun `getLoginAutofillCiphers when unlocked should return matched, non-deleted, non-reprompt, login ciphers`() = diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/ciphermatching/CipherMatchingManagerTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/ciphermatching/CipherMatchingManagerTest.kt index c4b4190ac1..effb3b5f87 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/ciphermatching/CipherMatchingManagerTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/manager/ciphermatching/CipherMatchingManagerTest.kt @@ -17,13 +17,18 @@ import io.mockk.every import io.mockk.mockk import io.mockk.mockkStatic import io.mockk.unmockkStatic +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.async import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.AfterEach import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Assertions.assertFalse +import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test +@OptIn(ExperimentalCoroutinesApi::class) class CipherMatchingManagerTest { private lateinit var cipherMatchingManager: CipherMatchingManager @@ -32,8 +37,11 @@ class CipherMatchingManagerTest { private val settingsRepository: SettingsRepository = mockk { every { defaultUriMatchType } returns DEFAULT_URI_MATCH_TYPE } + private val mutableDomainsStateFlow = MutableStateFlow>( + value = DataState.Loaded(DOMAINS_DATA), + ) private val vaultRepository: VaultRepository = mockk { - every { domainsStateFlow } returns MutableStateFlow(DataState.Loaded(DOMAINS_DATA)) + every { domainsStateFlow } returns mutableDomainsStateFlow } // Setup test ciphers @@ -179,6 +187,31 @@ class CipherMatchingManagerTest { ) } + @Test + fun `filterCiphersForMatches should return an empty list when retrieving domains times out`() = + runTest { + // Setup + val uri = "google.com" + mutableDomainsStateFlow.value = DataState.Loading + + // Test + val actual = async { + cipherMatchingManager.filterCiphersForMatches( + ciphers = ciphers, + matchUri = uri, + ) + } + + testScheduler.runCurrent() + assertFalse(actual.isCompleted) + testScheduler.advanceTimeBy(delayTimeMillis = 1_000L) + testScheduler.runCurrent() + + // Verify + assertTrue(actual.isCompleted) + assertEquals(emptyList(), actual.await()) + } + @Suppress("MaxLineLength") @Test fun `filterCiphersForMatches should perform cipher matching when is android app and matching URI`() = diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/util/FlowExtensionsTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/util/FlowExtensionsTest.kt new file mode 100644 index 0000000000..b080e444fd --- /dev/null +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/util/FlowExtensionsTest.kt @@ -0,0 +1,112 @@ +package com.x8bit.bitwarden.data.platform.util + +import com.x8bit.bitwarden.data.platform.repository.util.bufferedMutableSharedFlow +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.async +import kotlinx.coroutines.test.runTest +import org.junit.jupiter.api.Assertions.assertFalse +import org.junit.jupiter.api.Assertions.assertNotNull +import org.junit.jupiter.api.Assertions.assertNull +import org.junit.jupiter.api.Assertions.assertTrue +import org.junit.jupiter.api.Test + +@OptIn(ExperimentalCoroutinesApi::class) +class FlowExtensionsTest { + + @Test + fun `firstWithTimeoutOrNull should return null when the defined timeout has be reached`() = + runTest { + val timeout = 1_000L + val mutableSharedFlow = bufferedMutableSharedFlow() + + val result = async { + mutableSharedFlow.firstWithTimeoutOrNull(timeMillis = timeout) + } + + testScheduler.runCurrent() + assertFalse(result.isCompleted) + testScheduler.advanceTimeBy(delayTimeMillis = timeout) + testScheduler.runCurrent() + + assertTrue(result.isCompleted) + assertNull(result.await()) + } + + @Suppress("MaxLineLength") + @Test + fun `firstWithTimeoutOrNull should return a value when the value is emitted before the defined timeout has be reached`() = + runTest { + val timeout = 1_000L + val mutableSharedFlow = bufferedMutableSharedFlow() + + val result = async { + mutableSharedFlow.firstWithTimeoutOrNull(timeMillis = timeout) + } + + testScheduler.runCurrent() + assertFalse(result.isCompleted) + testScheduler.advanceTimeBy(delayTimeMillis = timeout / 2) + testScheduler.runCurrent() + assertFalse(result.isCompleted) + + mutableSharedFlow.tryEmit(Unit) + testScheduler.runCurrent() + + assertTrue(result.isCompleted) + assertNotNull(result.await()) + } + + @Suppress("MaxLineLength") + @Test + fun `firstWithTimeoutOrNull with predicate should return null when the defined timeout has be reached`() = + runTest { + val timeout = 1_000L + val mutableSharedFlow = bufferedMutableSharedFlow() + + val result = async { + mutableSharedFlow.firstWithTimeoutOrNull(timeMillis = timeout) { it } + } + + testScheduler.runCurrent() + assertFalse(result.isCompleted) + testScheduler.advanceTimeBy(delayTimeMillis = timeout / 2) + testScheduler.runCurrent() + assertFalse(result.isCompleted) + + mutableSharedFlow.tryEmit(false) + testScheduler.advanceTimeBy(delayTimeMillis = timeout) + testScheduler.runCurrent() + + assertTrue(result.isCompleted) + assertNull(result.await()) + } + + @Suppress("MaxLineLength") + @Test + fun `firstWithTimeoutOrNull with predicate should return a value when the value is emitted before the defined timeout has be reached`() = + runTest { + val timeout = 1_000L + val mutableSharedFlow = bufferedMutableSharedFlow() + + val result = async { + mutableSharedFlow.firstWithTimeoutOrNull(timeMillis = timeout) { it } + } + + testScheduler.runCurrent() + assertFalse(result.isCompleted) + testScheduler.advanceTimeBy(delayTimeMillis = timeout / 2) + testScheduler.runCurrent() + assertFalse(result.isCompleted) + + mutableSharedFlow.tryEmit(false) + testScheduler.runCurrent() + + assertFalse(result.isCompleted) + + mutableSharedFlow.tryEmit(true) + testScheduler.runCurrent() + + assertTrue(result.isCompleted) + assertNotNull(result.await()) + } +}