diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManager.kt b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManager.kt index 71b825ade1e..d3d3e2f1acf 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManager.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManager.kt @@ -32,7 +32,8 @@ interface AuthRequestManager { fun getAuthRequestByIdFlow(requestId: String): Flow /** - * Get all auth request and emits updates over time. + * Get all auth requests and emits updates over time, including when a passwordless push for + * the active user indicates the list changed. */ fun getAuthRequestsWithUpdates(): Flow diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerImpl.kt b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerImpl.kt index 22855876f20..5e4f1bcbfdb 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerImpl.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerImpl.kt @@ -18,13 +18,18 @@ import com.x8bit.bitwarden.data.auth.manager.model.AuthRequestsResult import com.x8bit.bitwarden.data.auth.manager.model.AuthRequestsUpdatesResult import com.x8bit.bitwarden.data.auth.manager.model.CreateAuthRequestResult import com.x8bit.bitwarden.data.auth.manager.util.isSso +import com.x8bit.bitwarden.data.auth.manager.util.toAuthRequest import com.x8bit.bitwarden.data.auth.manager.util.toAuthRequestTypeJson import com.x8bit.bitwarden.data.platform.error.NoActiveUserException +import com.x8bit.bitwarden.data.platform.manager.PushManager import com.x8bit.bitwarden.data.vault.datasource.sdk.VaultSdkSource import kotlinx.coroutines.currentCoroutineContext import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.flow +import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.merge import kotlinx.coroutines.isActive import java.time.Clock import java.time.Instant @@ -38,7 +43,7 @@ private const val PASSWORDLESS_APPROVER_INTERVAL_MILLIS: Long = 5L * 60L * 1_000 /** * Default implementation of [AuthRequestManager]. */ -@Suppress("TooManyFunctions") +@Suppress("LongParameterList", "TooManyFunctions") @Singleton class AuthRequestManagerImpl( private val clock: Clock, @@ -47,23 +52,31 @@ class AuthRequestManagerImpl( private val authDiskSource: AuthDiskSource, private val authSdkSource: AuthSdkSource, private val vaultSdkSource: VaultSdkSource, + private val pushManager: PushManager, ) : AuthRequestManager { private val activeUserId: String? get() = authDiskSource.userState?.activeUserId - override fun getAuthRequestsWithUpdates(): Flow = flow { - while (currentCoroutineContext().isActive) { + override fun getAuthRequestsWithUpdates(): Flow = merge( + // Reads immediately, then on the polling interval. + flow { + while (currentCoroutineContext().isActive) { + emit(Unit) + delay(timeMillis = PASSWORDLESS_APPROVER_INTERVAL_MILLIS) + } + }, + pushManager + .passwordlessRequestFlow + .filter { it.userId == activeUserId } + .map { }, + ) + .map { when (val result = getAuthRequests()) { - is AuthRequestsResult.Error -> { - emit(AuthRequestsUpdatesResult.Error(error = result.error)) - } - + is AuthRequestsResult.Error -> AuthRequestsUpdatesResult.Error(error = result.error) is AuthRequestsResult.Success -> { - emit(AuthRequestsUpdatesResult.Update(authRequests = result.authRequests)) + AuthRequestsUpdatesResult.Update(authRequests = result.authRequests) } } - delay(timeMillis = PASSWORDLESS_APPROVER_INTERVAL_MILLIS) } - } @Suppress("LongMethod") override fun createAuthRequestWithUpdates( @@ -91,18 +104,11 @@ class AuthRequestManagerImpl( isSso = authRequestType.isSso, ) .map { request -> - AuthRequest( - id = request.id, + request.toAuthRequest( + fingerprint = authRequest.fingerprint, publicKey = request.publicKey, - platform = request.platform, - ipAddress = request.ipAddress, - key = request.key, - masterPasswordHash = request.masterPasswordHash, - creationDate = request.creationDate, responseDate = request.responseDate, - requestApproved = request.requestApproved ?: false, - originUrl = request.originUrl, - fingerprint = authRequest.fingerprint, + isRequestApproved = request.requestApproved ?: false, ) } .fold( @@ -183,20 +189,13 @@ class AuthRequestManagerImpl( isRequestApproved = false responseDate = clock.instant() } - AuthRequest( - id = request.id, - platform = request.platform, - ipAddress = request.ipAddress, - key = request.key, - masterPasswordHash = request.masterPasswordHash, - creationDate = request.creationDate, - originUrl = request.originUrl, - responseDate = responseDate, - requestApproved = isRequestApproved, - // The PublicKey and Fingerprint should be frozen in place to - // ensure no funny-business happens between multiple requests. - publicKey = initialAuthRequest.publicKey, + // The PublicKey and Fingerprint should be frozen in place to ensure no + // funny-business happens between multiple requests. + request.toAuthRequest( fingerprint = initialAuthRequest.fingerprint, + publicKey = initialAuthRequest.publicKey, + responseDate = responseDate, + isRequestApproved = isRequestApproved, ) } } @@ -258,23 +257,12 @@ class AuthRequestManagerImpl( authRequestsService .getAuthRequest(requestId) .mapCatching { response -> - getFingerprintPhrase(response.publicKey) - .getOrThrow() - .let { fingerprint -> - AuthRequest( - id = response.id, - publicKey = response.publicKey, - platform = response.platform, - ipAddress = response.ipAddress, - key = response.key, - masterPasswordHash = response.masterPasswordHash, - creationDate = response.creationDate, - responseDate = response.responseDate, - requestApproved = response.requestApproved ?: false, - originUrl = response.originUrl, - fingerprint = fingerprint, - ) - } + response.toAuthRequest( + fingerprint = getFingerprintPhrase(response.publicKey).getOrThrow(), + publicKey = response.publicKey, + responseDate = response.responseDate, + isRequestApproved = response.requestApproved ?: false, + ) } .fold( onFailure = { AuthRequestUpdatesResult.Error(error = it) }, @@ -288,18 +276,11 @@ class AuthRequestManagerImpl( .flatMap { request -> if (request.requestApproved == true) { getFingerprintPhrase(request.publicKey).map { fingerprint -> - AuthRequest( - id = request.id, + request.toAuthRequest( + fingerprint = fingerprint, publicKey = request.publicKey, - platform = request.platform, - ipAddress = request.ipAddress, - key = request.key, - masterPasswordHash = request.masterPasswordHash, - creationDate = request.creationDate, responseDate = request.responseDate, - requestApproved = true, - originUrl = request.originUrl, - fingerprint = fingerprint, + isRequestApproved = true, ) } } else { @@ -313,18 +294,11 @@ class AuthRequestManagerImpl( .map { response -> response.authRequests.mapNotNull { request -> getFingerprintPhrase(request.publicKey).getOrNull()?.let { fingerprint -> - AuthRequest( - id = request.id, + request.toAuthRequest( + fingerprint = fingerprint, publicKey = request.publicKey, - platform = request.platform, - ipAddress = request.ipAddress, - key = request.key, - masterPasswordHash = request.masterPasswordHash, - creationDate = request.creationDate, responseDate = request.responseDate, - requestApproved = request.requestApproved ?: false, - originUrl = request.originUrl, - fingerprint = fingerprint, + isRequestApproved = request.requestApproved ?: false, ) } } @@ -356,18 +330,11 @@ class AuthRequestManagerImpl( ) } .map { request -> - AuthRequest( - id = request.id, + request.toAuthRequest( + fingerprint = "", publicKey = request.publicKey, - platform = request.platform, - ipAddress = request.ipAddress, - key = request.key, - masterPasswordHash = request.masterPasswordHash, - creationDate = request.creationDate, responseDate = request.responseDate, - requestApproved = request.requestApproved ?: false, - originUrl = request.originUrl, - fingerprint = "", + isRequestApproved = request.requestApproved ?: false, ) } .fold( @@ -391,20 +358,13 @@ class AuthRequestManagerImpl( ?.let { pendingAuthRequest -> authRequestsService .getAuthRequest(pendingAuthRequest.requestId) - .map { + .map { request -> NewAuthRequestData( - authRequest = AuthRequest( - id = it.id, - publicKey = it.publicKey, - platform = it.platform, - ipAddress = it.ipAddress, - key = it.key, - masterPasswordHash = it.masterPasswordHash, - creationDate = it.creationDate, - responseDate = it.responseDate, - requestApproved = it.requestApproved ?: false, - originUrl = it.originUrl, + authRequest = request.toAuthRequest( fingerprint = pendingAuthRequest.requestFingerprint, + publicKey = request.publicKey, + responseDate = request.responseDate, + isRequestApproved = request.requestApproved ?: false, ), privateKey = pendingAuthRequest.requestPrivateKey, accessCode = pendingAuthRequest.requestAccessCode, @@ -456,18 +416,11 @@ class AuthRequestManagerImpl( } } .map { request -> - AuthRequest( - id = request.id, + request.toAuthRequest( + fingerprint = authRequestResponse.fingerprint, publicKey = request.publicKey, - platform = request.platform, - ipAddress = request.ipAddress, - key = request.key, - masterPasswordHash = request.masterPasswordHash, - creationDate = request.creationDate, responseDate = request.responseDate, - requestApproved = request.requestApproved ?: false, - originUrl = request.originUrl, - fingerprint = authRequestResponse.fingerprint, + isRequestApproved = request.requestApproved ?: false, ) } .map { diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/di/AuthManagerModule.kt b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/di/AuthManagerModule.kt index 5f8a2f4f44a..15ab027bfaa 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/di/AuthManagerModule.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/di/AuthManagerModule.kt @@ -73,6 +73,7 @@ object AuthManagerModule { authSdkSource: AuthSdkSource, vaultSdkSource: VaultSdkSource, authDiskSource: AuthDiskSource, + pushManager: PushManager, ): AuthRequestManager = AuthRequestManagerImpl( clock = clock, @@ -81,6 +82,7 @@ object AuthManagerModule { authSdkSource = authSdkSource, vaultSdkSource = vaultSdkSource, authDiskSource = authDiskSource, + pushManager = pushManager, ) @Provides diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestExtensions.kt b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestExtensions.kt new file mode 100644 index 00000000000..15127777c81 --- /dev/null +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestExtensions.kt @@ -0,0 +1,23 @@ +package com.x8bit.bitwarden.data.auth.manager.util + +import com.bitwarden.core.util.isOverFiveMinutesOld +import com.x8bit.bitwarden.data.auth.manager.model.AuthRequest +import java.time.Clock + +/** + * Whether this request may still be approved or declined + * and has not expired (it is under 5 minutes old). + */ +fun AuthRequest.isActionable(clock: Clock): Boolean = + !requestApproved && + responseDate == null && + !creationDate.isOverFiveMinutesOld(clock) + +/** + * Filters out [AuthRequest]s that match one of the following criteria: + * * The request has been approved. + * * The request has been declined (indicated by it not being approved & having a responseDate). + * * The request has expired (it is at least 5 minutes old). + */ +fun List.filterRespondedAndExpired(clock: Clock): List = + filter { it.isActionable(clock = clock) } diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestsResponseJsonExtensions.kt b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestsResponseJsonExtensions.kt new file mode 100644 index 00000000000..ca638ffc60b --- /dev/null +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestsResponseJsonExtensions.kt @@ -0,0 +1,30 @@ +package com.x8bit.bitwarden.data.auth.manager.util + +import com.bitwarden.network.model.AuthRequestsResponseJson +import com.x8bit.bitwarden.data.auth.manager.model.AuthRequest +import java.time.Instant + +/** + * Converts the given [AuthRequestsResponseJson.AuthRequest] to an [AuthRequest], given the + * [fingerprint] that the response itself does not carry. + * + * The [publicKey], [responseDate], and [isRequestApproved] are supplied by the caller. + */ +fun AuthRequestsResponseJson.AuthRequest.toAuthRequest( + fingerprint: String, + publicKey: String, + responseDate: Instant?, + isRequestApproved: Boolean, +): AuthRequest = AuthRequest( + id = this.id, + publicKey = publicKey, + platform = this.platform, + ipAddress = this.ipAddress, + key = this.key, + masterPasswordHash = this.masterPasswordHash, + creationDate = this.creationDate, + responseDate = responseDate, + requestApproved = isRequestApproved, + originUrl = this.originUrl, + fingerprint = fingerprint, +) diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesViewModel.kt b/app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesViewModel.kt index 034f63097cc..4b487ef90e0 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesViewModel.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesViewModel.kt @@ -10,7 +10,6 @@ import androidx.lifecycle.viewModelScope import com.bitwarden.core.data.manager.BuildInfoManager import com.bitwarden.core.data.util.toFormattedDateTimeStyle import com.bitwarden.core.util.isBuildVersionAtLeast -import com.bitwarden.core.util.isOverFiveMinutesOld import com.bitwarden.ui.platform.base.BackgroundEvent import com.bitwarden.ui.platform.base.BaseViewModel import com.bitwarden.ui.platform.components.snackbar.model.BitwardenSnackbarData @@ -18,6 +17,7 @@ import com.bitwarden.ui.platform.manager.snackbar.SnackbarRelayManager import com.bitwarden.ui.util.Text import com.x8bit.bitwarden.data.auth.manager.model.AuthRequest import com.x8bit.bitwarden.data.auth.manager.model.AuthRequestsUpdatesResult +import com.x8bit.bitwarden.data.auth.manager.util.filterRespondedAndExpired import com.x8bit.bitwarden.data.auth.repository.AuthRepository import com.x8bit.bitwarden.data.auth.repository.model.DeviceInfo import com.x8bit.bitwarden.data.auth.repository.model.GetDevicesResult @@ -63,15 +63,13 @@ class ManageDevicesViewModel @Inject constructor( isRefreshing = false, internalHideBottomSheet = false, isFdroid = buildInfoManager.isFdroid, - devicesLoaded = false, - authRequestsLoaded = false, ), ) { private var authJob: Job = Job().apply { complete() } + private var devicesJob: Job = Job().apply { complete() } init { updateAuthRequestList() - fetchAllDevices() settingsRepository .getPullToRefreshEnabledFlow() .map { ManageDevicesAction.Internal.PullToRefreshEnableReceive(it) } @@ -111,17 +109,8 @@ class ManageDevicesViewModel @Inject constructor( } private fun handleRefreshPull() { - val shouldRefetchDevices = !state.devicesLoaded - mutableStateFlow.update { - it.copy( - isRefreshing = true, - authRequestsLoaded = false, - ) - } + mutableStateFlow.update { it.copy(isRefreshing = true) } updateAuthRequestList() - if (shouldRefetchDevices) { - fetchAllDevices() - } } private fun handlePendingRequestRowClicked( @@ -174,7 +163,10 @@ class ManageDevicesViewModel @Inject constructor( } private fun fetchAllDevices() { - viewModelScope.launch { + // Canceled first so a slower earlier read cannot land after a newer one and render a + // stale list or dismiss the pull-to-refresh indicator early. + devicesJob.cancel() + devicesJob = viewModelScope.launch { sendAction( ManageDevicesAction.Internal.GetDevicesResultReceive( devicesResult = authRepository.getDevices(), @@ -193,16 +185,10 @@ class ManageDevicesViewModel @Inject constructor( is AuthRequestsUpdatesResult.Error -> emptyList() } - mutableStateFlow.update { - it.copy( - authRequests = filteredRequests.toImmutableList(), - authRequestsLoaded = true, - isRefreshing = if (state.devicesLoaded) false else it.isRefreshing, - ) - } - if (state.devicesLoaded) { - updateContentWithCurrentData() - } + mutableStateFlow.update { it.copy(authRequests = filteredRequests.toImmutableList()) } + // The device list is the only source that reports which device owns a pending request, so + // it is re-read before the new list can be rendered against it. + fetchAllDevices() } private fun handleGetDevicesResultReceived( @@ -211,7 +197,10 @@ class ManageDevicesViewModel @Inject constructor( val devicesResult = action.devicesResult as? GetDevicesResult.Success ?: run { mutableStateFlow.update { - it.copy(viewState = ManageDevicesState.ViewState.Error, isRefreshing = false) + it.copy( + viewState = ManageDevicesState.ViewState.Error, + isRefreshing = false, + ) } return } @@ -219,13 +208,10 @@ class ManageDevicesViewModel @Inject constructor( mutableStateFlow.update { it.copy( devices = devicesResult.devices.toImmutableList(), - devicesLoaded = true, - isRefreshing = if (state.authRequestsLoaded) false else it.isRefreshing, + isRefreshing = false, ) } - if (state.authRequestsLoaded) { - updateContentWithCurrentData() - } + updateContentWithCurrentData() } private fun updateContentWithCurrentData() { @@ -285,8 +271,6 @@ data class ManageDevicesState( val isRefreshing: Boolean, private val internalHideBottomSheet: Boolean, private val isFdroid: Boolean, - val devicesLoaded: Boolean, - val authRequestsLoaded: Boolean, ) : Parcelable { /** @@ -457,16 +441,3 @@ enum class DeviceSessionStatus { Pending, None, } - -/** - * Filters out [AuthRequest]s that match one of the following criteria: - * * The request has been approved. - * * The request has been declined (indicated by it not being approved & having a responseDate). - * * The request has expired (it is at least 5 minutes old). - */ -private fun List.filterRespondedAndExpired(clock: Clock) = - filterNot { request -> - request.requestApproved || - request.responseDate != null || - request.creationDate.isOverFiveMinutesOld(clock) - } diff --git a/app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModel.kt b/app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModel.kt index 3564e3dca41..cb35f281d91 100644 --- a/app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModel.kt +++ b/app/src/main/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModel.kt @@ -8,13 +8,13 @@ import androidx.lifecycle.viewModelScope import com.bitwarden.core.data.manager.BuildInfoManager import com.bitwarden.core.data.util.toFormattedDateTimeStyle import com.bitwarden.core.util.isBuildVersionAtLeast -import com.bitwarden.core.util.isOverFiveMinutesOld import com.bitwarden.ui.platform.base.BackgroundEvent import com.bitwarden.ui.platform.base.BaseViewModel import com.bitwarden.ui.platform.components.snackbar.model.BitwardenSnackbarData import com.bitwarden.ui.platform.manager.snackbar.SnackbarRelayManager import com.x8bit.bitwarden.data.auth.manager.model.AuthRequest import com.x8bit.bitwarden.data.auth.manager.model.AuthRequestsUpdatesResult +import com.x8bit.bitwarden.data.auth.manager.util.filterRespondedAndExpired import com.x8bit.bitwarden.data.auth.repository.AuthRepository import com.x8bit.bitwarden.data.platform.repository.SettingsRepository import com.x8bit.bitwarden.ui.platform.model.SnackbarRelay @@ -391,16 +391,3 @@ sealed class PendingRequestsAction { ) : Internal() } } - -/** - * Filters out [AuthRequest]s that match one of the following criteria: - * * The request has been approved. - * * The request has been declined (indicated by it not being approved & having a responseDate). - * * The request has expired (it is at least 5 minutes old). - */ -private fun List.filterRespondedAndExpired(clock: Clock) = - filterNot { request -> - request.requestApproved || - request.responseDate != null || - request.creationDate.isOverFiveMinutesOld(clock) - } diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerTest.kt index 259edb131bf..92e1dd90bd0 100644 --- a/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerTest.kt +++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/AuthRequestManagerTest.kt @@ -2,6 +2,7 @@ package com.x8bit.bitwarden.data.auth.manager import app.cash.turbine.test import com.bitwarden.core.AuthRequestResponse +import com.bitwarden.core.data.repository.util.bufferedMutableSharedFlow import com.bitwarden.core.data.util.asFailure import com.bitwarden.core.data.util.asSuccess import com.bitwarden.network.model.AuthRequestTypeJson @@ -22,9 +23,12 @@ import com.x8bit.bitwarden.data.auth.manager.model.AuthRequestUpdatesResult import com.x8bit.bitwarden.data.auth.manager.model.AuthRequestsResult import com.x8bit.bitwarden.data.auth.manager.model.AuthRequestsUpdatesResult import com.x8bit.bitwarden.data.auth.manager.model.CreateAuthRequestResult +import com.x8bit.bitwarden.data.platform.manager.PushManager +import com.x8bit.bitwarden.data.platform.manager.model.PasswordlessRequestData import com.x8bit.bitwarden.data.vault.datasource.sdk.VaultSdkSource import io.mockk.coEvery import io.mockk.coVerify +import io.mockk.every import io.mockk.mockk import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.advanceTimeBy @@ -52,6 +56,11 @@ class AuthRequestManagerTest { } returns "AsymmetricEncString".asSuccess() } private val fakeAuthDiskSource = FakeAuthDiskSource() + private val mutablePasswordlessRequestFlow = + bufferedMutableSharedFlow() + private val pushManager: PushManager = mockk { + every { passwordlessRequestFlow } returns mutablePasswordlessRequestFlow + } private val repository: AuthRequestManager = AuthRequestManagerImpl( clock = fixedClock, @@ -60,6 +69,7 @@ class AuthRequestManagerTest { authSdkSource = authSdkSource, vaultSdkSource = vaultSdkSource, authDiskSource = fakeAuthDiskSource, + pushManager = pushManager, ) @Suppress("MaxLineLength") @@ -1281,6 +1291,38 @@ class AuthRequestManagerTest { } assertEquals(expected, result) } + + @Test + fun `getAuthRequestsWithUpdates should re-read on a push and ignore a non-active user push`() = + runTest { + val expected = AuthRequestsUpdatesResult.Update(authRequests = listOf(AUTH_REQUEST)) + coEvery { authRequestsService.getAuthRequests() } returns AuthRequestsResponseJson( + authRequests = listOf(AUTH_REQUESTS_RESPONSE_JSON_AUTH_RESPONSE), + ) + .asSuccess() + coEvery { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } returns FINGER_PRINT.asSuccess() + fakeAuthDiskSource.userState = SINGLE_USER_STATE + + repository + .getAuthRequestsWithUpdates() + .test { + assertEquals(expected, awaitItem()) + + mutablePasswordlessRequestFlow.emit( + PASSWORDLESS_REQUEST_DATA.copy(userId = "otherUserId"), + ) + mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) + + // Only the active user's push produces a re-read, and it arrives without + // waiting for the polling interval. + assertEquals(expected, awaitItem()) + cancelAndIgnoreRemainingEvents() + } + + coVerify(exactly = 2) { authRequestsService.getAuthRequests() } + } } private const val EMAIL: String = "test@bitwarden.com" @@ -1363,3 +1405,8 @@ private val AUTH_REQUEST_RESPONSE: AuthRequestResponse = AuthRequestResponse( accessCode = "accessCode", fingerprint = "fingerprint", ) + +private val PASSWORDLESS_REQUEST_DATA: PasswordlessRequestData = PasswordlessRequestData( + loginRequestId = REQUEST_ID, + userId = USER_ID, +) diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestExtensionsTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestExtensionsTest.kt new file mode 100644 index 00000000000..1f78c191de5 --- /dev/null +++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestExtensionsTest.kt @@ -0,0 +1,87 @@ +package com.x8bit.bitwarden.data.auth.manager.util + +import com.x8bit.bitwarden.data.auth.manager.model.AuthRequest +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.Test +import java.time.Clock +import java.time.Instant +import java.time.ZoneOffset + +class AuthRequestExtensionsTest { + private val clock: Clock = Clock.fixed( + Instant.parse("2024-09-13T00:04:00Z"), + ZoneOffset.UTC, + ) + + @Test + fun `isActionable should return true when unanswered and under five minutes old`() { + assertTrue(AUTH_REQUEST.isActionable(clock = clock)) + } + + @Test + fun `isActionable should return false when the request has been approved`() { + assertFalse(AUTH_REQUEST.copy(requestApproved = true).isActionable(clock = clock)) + } + + @Test + fun `isActionable should return false when the request has a response date`() { + assertFalse( + AUTH_REQUEST + .copy(responseDate = Instant.parse("2024-09-13T00:03:00Z")) + .isActionable(clock = clock), + ) + } + + @Test + fun `isActionable should return false when the request is over five minutes old`() { + assertFalse( + AUTH_REQUEST + .copy(creationDate = Instant.parse("2024-09-12T23:58:00Z")) + .isActionable(clock = clock), + ) + } + + @Test + fun `filterRespondedAndExpired should retain only the actionable requests`() { + val actionable = AUTH_REQUEST.copy(id = "actionable") + val approved = AUTH_REQUEST.copy(id = "approved", requestApproved = true) + val declined = AUTH_REQUEST.copy( + id = "declined", + responseDate = Instant.parse("2024-09-13T00:03:00Z"), + ) + val expired = AUTH_REQUEST.copy( + id = "expired", + creationDate = Instant.parse("2024-09-12T23:58:00Z"), + ) + + assertEquals( + listOf(actionable), + listOf(actionable, approved, declined, expired) + .filterRespondedAndExpired(clock = clock), + ) + } + + @Test + fun `filterRespondedAndExpired should return an empty list when given an empty list`() { + assertEquals( + emptyList(), + emptyList().filterRespondedAndExpired(clock = clock), + ) + } +} + +private val AUTH_REQUEST: AuthRequest = AuthRequest( + id = "1", + publicKey = "publicKey", + platform = "Android", + ipAddress = "192.168.0.1", + key = "key", + masterPasswordHash = "verySecureHash", + creationDate = Instant.parse("2024-09-13T00:00:00Z"), + responseDate = null, + requestApproved = false, + originUrl = "www.bitwarden.com", + fingerprint = "fingerprint", +) diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestsResponseJsonExtensionsTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestsResponseJsonExtensionsTest.kt new file mode 100644 index 00000000000..aa0fc476610 --- /dev/null +++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestsResponseJsonExtensionsTest.kt @@ -0,0 +1,53 @@ +package com.x8bit.bitwarden.data.auth.manager.util + +import com.bitwarden.network.model.AuthRequestsResponseJson +import com.x8bit.bitwarden.data.auth.manager.model.AuthRequest +import org.junit.jupiter.api.Assertions.assertEquals +import org.junit.jupiter.api.Test +import java.time.Instant + +class AuthRequestsResponseJsonExtensionsTest { + @Test + fun `toAuthRequest should map each property and apply the given values`() { + val fingerprint = "fingerprint" + val responseDate = Instant.parse("2024-09-13T00:10:00Z") + + val result = AUTH_REQUEST_RESPONSE_JSON.toAuthRequest( + fingerprint = fingerprint, + publicKey = "givenPublicKey", + responseDate = responseDate, + isRequestApproved = false, + ) + + assertEquals( + AuthRequest( + id = "1", + publicKey = "givenPublicKey", + platform = "Android", + ipAddress = "192.168.0.1", + key = "key", + masterPasswordHash = "verySecureHash", + creationDate = Instant.parse("2024-09-13T00:00:00Z"), + responseDate = responseDate, + requestApproved = false, + originUrl = "www.bitwarden.com", + fingerprint = fingerprint, + ), + result, + ) + } +} + +private val AUTH_REQUEST_RESPONSE_JSON: AuthRequestsResponseJson.AuthRequest = + AuthRequestsResponseJson.AuthRequest( + id = "1", + publicKey = "publicKey", + platform = "Android", + ipAddress = "192.168.0.1", + key = "key", + masterPasswordHash = "verySecureHash", + creationDate = Instant.parse("2024-09-13T00:00:00Z"), + responseDate = Instant.parse("2024-09-13T00:05:00Z"), + requestApproved = true, + originUrl = "www.bitwarden.com", + ) diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesScreenTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesScreenTest.kt index 5ca806f7a01..26c9cbdd098 100644 --- a/app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesScreenTest.kt +++ b/app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesScreenTest.kt @@ -199,7 +199,9 @@ class ManageDevicesScreenTest : BitwardenComposeTest() { @Test fun `bottom sheet should not show when permission already granted`() { permissionsManager.checkPermissionResult = true - mutableStateFlow.value = DEFAULT_STATE.copy(devicesLoaded = true) + mutableStateFlow.value = DEFAULT_STATE.copy( + viewState = ManageDevicesState.ViewState.Content(items = emptyList()), + ) composeTestRule.onNodeWithText("Skip for now").assertDoesNotExist() } @@ -242,6 +244,4 @@ private val DEFAULT_STATE = ManageDevicesState( isRefreshing = false, internalHideBottomSheet = false, isFdroid = false, - devicesLoaded = false, - authRequestsLoaded = false, ) diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesViewModelTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesViewModelTest.kt index 43e7f245908..2cd53b36a10 100644 --- a/app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesViewModelTest.kt +++ b/app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesViewModelTest.kt @@ -83,19 +83,16 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { } @Test - fun `init should make necessary network calls`() { - createViewModel() - coVerify { - authRepository.getAuthRequestsWithUpdates() - authRepository.getDevices() - } - } - - @Test - fun `init should set devicesLoaded true after device fetch success`() { + fun `auth request update should trigger a device fetch`() { val viewModel = createViewModel() - // After init with unconfined dispatcher, devices coroutine runs immediately - assertEquals(true, viewModel.stateFlow.value.devicesLoaded) + coVerify(exactly = 0) { authRepository.getDevices() } + + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Update(authRequests = emptyList()), + ) + + coVerify(exactly = 1) { authRepository.getDevices() } + assertEquals(EMPTY_CONTENT_STATE, viewModel.stateFlow.value) } @Test @@ -116,42 +113,36 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { } @Test - fun `LifecycleResume should re-fetch auth requests only`() = runTest { + fun `LifecycleResume should re-subscribe to auth request updates`() { val viewModel = createViewModel() + viewModel.trySendAction(ManageDevicesAction.LifecycleResume) + // getAuthRequestsWithUpdates called twice: once on init, once on resume verify(exactly = 2) { authRepository.getAuthRequestsWithUpdates() } - coVerify(exactly = 1) { authRepository.getDevices() } } @Test - fun `RefreshPull when devices loaded should re-fetch auth requests only`() = runTest { - val viewModel = createViewModel() - viewModel.stateFlow.test { - skipItems(1) - - viewModel.trySendAction(ManageDevicesAction.RefreshPull) - - coVerify(exactly = 1) { authRepository.getDevices() } - verify(exactly = 2) { authRepository.getAuthRequestsWithUpdates() } - cancelAndIgnoreRemainingEvents() - } - } - - @Test - fun `RefreshPull when devices failed should re-fetch both devices and auth requests`() = + fun `RefreshPull should re-subscribe to auth request updates and clear isRefreshing`() = runTest { - coEvery { authRepository.getDevices() } returns GetDevicesResult.Error val viewModel = createViewModel() - viewModel.stateFlow.test { - skipItems(1) + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Update(authRequests = emptyList()), + ) - viewModel.trySendAction(ManageDevicesAction.RefreshPull) + viewModel.trySendAction(ManageDevicesAction.RefreshPull) + assertEquals( + EMPTY_CONTENT_STATE.copy(isRefreshing = true), + viewModel.stateFlow.value, + ) - coVerify(exactly = 2) { authRepository.getDevices() } - verify(exactly = 2) { authRepository.getAuthRequestsWithUpdates() } - cancelAndIgnoreRemainingEvents() - } + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Update(authRequests = emptyList()), + ) + + verify(exactly = 2) { authRepository.getAuthRequestsWithUpdates() } + coVerify(exactly = 2) { authRepository.getDevices() } + assertEquals(EMPTY_CONTENT_STATE, viewModel.stateFlow.value) } @Test @@ -171,32 +162,43 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { fun `when getDevices returns error should show error state`() { coEvery { authRepository.getDevices() } returns GetDevicesResult.Error val viewModel = createViewModel() + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Update(authRequests = emptyList()), + ) assertEquals( - ManageDevicesState( - authRequests = persistentListOf(), - devices = persistentListOf(), - viewState = ManageDevicesState.ViewState.Error, - isPullToRefreshSettingEnabled = false, - isRefreshing = false, - internalHideBottomSheet = false, - isFdroid = false, - devicesLoaded = false, - authRequestsLoaded = false, - ), + DEFAULT_STATE.copy(viewState = ManageDevicesState.ViewState.Error), viewModel.stateFlow.value, ) } @Test - fun `AuthRequestsResultReceive with error should use empty auth request list`() { + fun `when getDevices returns error after content should show error state`() { val viewModel = createViewModel() mutableAuthRequestsWithUpdatesFlow.tryEmit( - AuthRequestsUpdatesResult.Error(error = Throwable()), + AuthRequestsUpdatesResult.Update(authRequests = emptyList()), ) + assertEquals(EMPTY_CONTENT_STATE, viewModel.stateFlow.value) + + // A failed read means the rendered list is stale, so the error state is shown. + coEvery { authRepository.getDevices() } returns GetDevicesResult.Error + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Update(authRequests = emptyList()), + ) + + coVerify(exactly = 2) { authRepository.getDevices() } assertEquals( - emptyList(), - viewModel.stateFlow.value.authRequests, + DEFAULT_STATE.copy(viewState = ManageDevicesState.ViewState.Error), + viewModel.stateFlow.value, + ) + } + + @Test + fun `AuthRequestsResultReceive with error should still render with an empty request list`() { + val viewModel = createViewModel() + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Error(error = Throwable()), ) + assertEquals(EMPTY_CONTENT_STATE, viewModel.stateFlow.value) } @Test @@ -267,7 +269,7 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { ) viewModel.stateFlow.test { assertEquals( - ManageDevicesState( + DEFAULT_STATE.copy( authRequests = listOf(validAuthRequest).toImmutableList(), devices = listOf( otherDevice, @@ -311,18 +313,61 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { ), ), ), - isPullToRefreshSettingEnabled = false, - isRefreshing = false, - internalHideBottomSheet = false, - isFdroid = false, - devicesLoaded = true, - authRequestsLoaded = true, ), awaitItem(), ) } } + @Test + fun `push-driven auth request update should add a pending row for its device`() = runTest { + val pendingDevice = DEFAULT_DEVICE.copy( + id = "device-pending", + pendingAuthRequest = DevicePendingAuthRequest( + id = PASSWORDLESS_AUTH_REQUEST.id, + creationDate = fixedClock.instant(), + ), + ) + val viewModel = createViewModel() + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Update(authRequests = emptyList()), + ) + + // The device only reports its pending association once the request exists server-side. + coEvery { authRepository.getDevices() } returns GetDevicesResult.Success( + devices = listOf(pendingDevice), + ) + // A push makes the manager re-read the list, which surfaces here as another update. + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Update(authRequests = listOf(PASSWORDLESS_AUTH_REQUEST)), + ) + + viewModel.stateFlow.test { + assertEquals( + DEFAULT_STATE.copy( + authRequests = listOf(PASSWORDLESS_AUTH_REQUEST).toImmutableList(), + devices = listOf(pendingDevice).toImmutableList(), + viewState = ManageDevicesState.ViewState.Content( + items = listOf( + ManageDevicesState.ViewState.Content.DeviceItem( + id = pendingDevice.id, + name = pendingDevice.name, + typeName = pendingDevice.type.readableDeviceTypeName, + isTrusted = pendingDevice.isTrusted, + firstLoginDate = "Oct 27, 2023, 12:00:00 PM", + lastActivityLabel = pendingDevice.lastActivityDate + ?.toLastActivityLabel(clock = fixedClock), + status = DeviceSessionStatus.Pending, + fingerprintPhrase = PASSWORDLESS_AUTH_REQUEST.fingerprint, + ), + ), + ), + ), + awaitItem(), + ) + } + } + private fun createViewModel(state: ManageDevicesState? = null) = ManageDevicesViewModel( clock = fixedClock, authRepository = authRepository, @@ -333,6 +378,34 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { ) } +private val DEFAULT_STATE = ManageDevicesState( + authRequests = persistentListOf(), + devices = persistentListOf(), + viewState = ManageDevicesState.ViewState.Loading, + isPullToRefreshSettingEnabled = false, + isRefreshing = false, + internalHideBottomSheet = false, + isFdroid = false, +) + +private val EMPTY_CONTENT_STATE = DEFAULT_STATE.copy( + viewState = ManageDevicesState.ViewState.Content(items = emptyList()), +) + +private val PASSWORDLESS_AUTH_REQUEST = AuthRequest( + id = "auth-req-push", + publicKey = "publicKey", + platform = "Android", + ipAddress = "192.168.0.1", + key = null, + masterPasswordHash = null, + creationDate = Instant.parse("2023-10-27T12:00:00Z"), + responseDate = null, + requestApproved = false, + originUrl = "www.bitwarden.com", + fingerprint = "fingerprint-phrase", +) + private val DEFAULT_DEVICE = DeviceInfo( id = "device-current", name = "Test Device", diff --git a/app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModelTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModelTest.kt index d07b0219add..0bca0af7902 100644 --- a/app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModelTest.kt +++ b/app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/pendingrequests/PendingRequestsViewModelTest.kt @@ -202,6 +202,30 @@ class PendingRequestsViewModelTest : BaseViewModelTest() { assertEquals(expected, viewModel.stateFlow.value) } + @Test + fun `getPendingResults failure after a successful load should show the error state`() { + val viewModel = createViewModel( + state = DEFAULT_STATE.copy(viewState = PendingRequestsState.ViewState.Loading), + ) + mutableAuthRequestsWithUpdatesFlow.tryEmit( + value = AuthRequestsUpdatesResult.Update(authRequests = emptyList()), + ) + assertEquals( + DEFAULT_STATE.copy(viewState = PendingRequestsState.ViewState.Empty), + viewModel.stateFlow.value, + ) + + // A failed update means the rendered list is stale, so the error state is shown. + mutableAuthRequestsWithUpdatesFlow.tryEmit( + value = AuthRequestsUpdatesResult.Error(error = Throwable()), + ) + + assertEquals( + DEFAULT_STATE.copy(viewState = PendingRequestsState.ViewState.Error), + viewModel.stateFlow.value, + ) + } + @Test fun `on CloseClick should emit NavigateBack`() = runTest { val viewModel = createViewModel()