From 25ab96b31bafded491921471b3a82f8b4d591080 Mon Sep 17 00:00:00 2001 From: Andre Rosado Date: Tue, 18 Aug 2026 13:17:31 +0100 Subject: [PATCH 1/9] Added flow so push notifications update the manage devices screen --- .../data/auth/manager/AuthRequestManager.kt | 9 + .../auth/manager/AuthRequestManagerImpl.kt | 164 ++++++------------ .../data/auth/manager/di/AuthManagerModule.kt | 2 + .../AuthRequestsResponseJsonExtensions.kt | 24 +++ .../managedevices/ManageDevicesViewModel.kt | 70 ++++++++ .../auth/manager/AuthRequestManagerTest.kt | 148 ++++++++++++++++ .../AuthRequestsResponseJsonExtensionsTest.kt | 56 ++++++ .../ManageDevicesViewModelTest.kt | 83 +++++++++ 8 files changed, 447 insertions(+), 109 deletions(-) create mode 100644 app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestsResponseJsonExtensions.kt create mode 100644 app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestsResponseJsonExtensionsTest.kt 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..5cbe6eab2d9 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 @@ -36,6 +36,15 @@ interface AuthRequestManager { */ fun getAuthRequestsWithUpdates(): Flow + /** + * Get the [AuthRequest] for each incoming passwordless request for the active user, hydrated + * with the fingerprint required to approve it. + * + * Only requests that can still be acted upon are emitted; those already approved, declined, or + * expired are not. Requests that cannot be retrieved are omitted rather than emitted as errors. + */ + fun getPasswordlessAuthRequestFlow(): Flow + /** * Get an [AuthRequest] by its request ID. */ 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..07b287559a7 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 @@ -4,6 +4,7 @@ import com.bitwarden.core.AuthRequestResponse import com.bitwarden.core.data.util.asFailure import com.bitwarden.core.data.util.asSuccess import com.bitwarden.core.data.util.flatMap +import com.bitwarden.core.util.isOverFiveMinutesOld import com.bitwarden.network.model.AuthRequestTypeJson import com.bitwarden.network.service.AuthRequestsService import com.bitwarden.network.service.NewAuthRequestService @@ -18,14 +19,19 @@ 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.mapNotNull import kotlinx.coroutines.isActive +import timber.log.Timber import java.time.Clock import java.time.Instant import javax.inject.Singleton @@ -38,7 +44,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,6 +53,7 @@ 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 @@ -91,19 +98,7 @@ class AuthRequestManagerImpl( isSso = authRequestType.isSso, ) .map { request -> - AuthRequest( - id = request.id, - 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, - ) + request.toAuthRequest(fingerprint = authRequest.fingerprint) } .fold( onFailure = { emit(CreateAuthRequestResult.Error(error = it)) }, @@ -183,21 +178,15 @@ 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, + request // The PublicKey and Fingerprint should be frozen in place to // ensure no funny-business happens between multiple requests. - publicKey = initialAuthRequest.publicKey, - fingerprint = initialAuthRequest.fingerprint, - ) + .toAuthRequest(fingerprint = initialAuthRequest.fingerprint) + .copy( + publicKey = initialAuthRequest.publicKey, + responseDate = responseDate, + requestApproved = isRequestApproved, + ) } } .fold( @@ -258,23 +247,9 @@ 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(), + ) } .fold( onFailure = { AuthRequestUpdatesResult.Error(error = it) }, @@ -282,25 +257,34 @@ class AuthRequestManagerImpl( ) } + override fun getPasswordlessAuthRequestFlow(): Flow = pushManager + .passwordlessRequestFlow + // A push for a non-active user would otherwise be hydrated with the active user's token. + .filter { it.userId == activeUserId } + .mapNotNull { data -> + authRequestsService + .getAuthRequest(data.loginRequestId) + .mapCatching { response -> + response.toAuthRequest( + fingerprint = getFingerprintPhrase(response.publicKey).getOrThrow(), + ) + } + .fold( + onFailure = { + Timber.d(it, "Unable to hydrate the requested auth request.") + null + }, + onSuccess = { authRequest -> authRequest.takeIf { it.isActionable } }, + ) + } + override suspend fun getAuthRequestIfApproved(requestId: String): Result = authRequestsService .getAuthRequest(requestId) .flatMap { request -> if (request.requestApproved == true) { getFingerprintPhrase(request.publicKey).map { fingerprint -> - AuthRequest( - id = request.id, - 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, - ) + request.toAuthRequest(fingerprint = fingerprint) } } else { IllegalStateException("Request not approved.").asFailure() @@ -313,19 +297,7 @@ class AuthRequestManagerImpl( .map { response -> response.authRequests.mapNotNull { request -> getFingerprintPhrase(request.publicKey).getOrNull()?.let { fingerprint -> - AuthRequest( - id = request.id, - 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, - ) + request.toAuthRequest(fingerprint = fingerprint) } } } @@ -355,21 +327,7 @@ class AuthRequestManagerImpl( isApproved = isApproved, ) } - .map { request -> - AuthRequest( - id = request.id, - 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 = "", - ) - } + .map { request -> request.toAuthRequest(fingerprint = "") } .fold( onFailure = { AuthRequestResult.Error(error = it) }, onSuccess = { AuthRequestResult.Success(authRequest = it) }, @@ -393,17 +351,7 @@ class AuthRequestManagerImpl( .getAuthRequest(pendingAuthRequest.requestId) .map { 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 = it.toAuthRequest( fingerprint = pendingAuthRequest.requestFingerprint, ), privateKey = pendingAuthRequest.requestPrivateKey, @@ -456,19 +404,7 @@ class AuthRequestManagerImpl( } } .map { request -> - AuthRequest( - id = request.id, - 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, - ) + request.toAuthRequest(fingerprint = authRequestResponse.fingerprint) } .map { NewAuthRequestData( @@ -489,6 +425,16 @@ class AuthRequestManagerImpl( publicKey = publicKey, ) } + + /** + * Whether this request may still be approved or declined, meaning it has not already been + * approved, not been declined (indicated by it not being approved & having a responseDate), + * and has not expired (it is under 5 minutes old). + */ + private val AuthRequest.isActionable: Boolean + get() = !requestApproved && + responseDate == null && + !creationDate.isOverFiveMinutesOld(clock) } /** 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/AuthRequestsResponseJsonExtensions.kt b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestsResponseJsonExtensions.kt new file mode 100644 index 00000000000..c7373de4791 --- /dev/null +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestsResponseJsonExtensions.kt @@ -0,0 +1,24 @@ +package com.x8bit.bitwarden.data.auth.manager.util + +import com.bitwarden.network.model.AuthRequestsResponseJson +import com.x8bit.bitwarden.data.auth.manager.model.AuthRequest + +/** + * Converts the given [AuthRequestsResponseJson.AuthRequest] to an [AuthRequest], given the + * [fingerprint] that the response itself does not carry. + */ +fun AuthRequestsResponseJson.AuthRequest.toAuthRequest( + fingerprint: String, +): AuthRequest = AuthRequest( + id = id, + publicKey = publicKey, + platform = platform, + ipAddress = ipAddress, + key = key, + masterPasswordHash = masterPasswordHash, + creationDate = creationDate, + responseDate = responseDate, + requestApproved = requestApproved ?: false, + originUrl = 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..e3866ab85f7 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 @@ -72,6 +72,11 @@ class ManageDevicesViewModel @Inject constructor( init { updateAuthRequestList() fetchAllDevices() + authRepository + .getPasswordlessAuthRequestFlow() + .map { ManageDevicesAction.Internal.PasswordlessAuthRequestReceive(it) } + .onEach(::sendAction) + .launchIn(viewModelScope) settingsRepository .getPullToRefreshEnabledFlow() .map { ManageDevicesAction.Internal.PullToRefreshEnableReceive(it) } @@ -147,6 +152,14 @@ class ManageDevicesViewModel @Inject constructor( is ManageDevicesAction.Internal.AuthRequestsResultReceive -> { handleAuthRequestsResultReceived(action) } + + is ManageDevicesAction.Internal.PasswordlessAuthRequestReceive -> { + handlePasswordlessAuthRequestReceive(action) + } + + is ManageDevicesAction.Internal.PasswordlessAuthRequestDevicesReceive -> { + handlePasswordlessAuthRequestDevicesReceive(action) + } } } @@ -228,6 +241,47 @@ class ManageDevicesViewModel @Inject constructor( } } + private fun handlePasswordlessAuthRequestReceive( + action: ManageDevicesAction.Internal.PasswordlessAuthRequestReceive, + ) { + // The device list is the only source that reports which device owns a pending request, so + // it is re-read before the new request can be rendered against its device. + viewModelScope.launch { + sendAction( + ManageDevicesAction.Internal.PasswordlessAuthRequestDevicesReceive( + authRequest = action.authRequest, + devicesResult = authRepository.getDevices(), + ), + ) + } + } + + private fun handlePasswordlessAuthRequestDevicesReceive( + action: ManageDevicesAction.Internal.PasswordlessAuthRequestDevicesReceive, + ) { + // This refresh is not user-initiated, so a failure leaves the screen untouched rather than + // replacing it with an error; polling and pull-to-refresh reconcile it later. + val devices = (action.devicesResult as? GetDevicesResult.Success) + ?.devices + ?: return + + mutableStateFlow.update { currentState -> + currentState.copy( + // Replaces any earlier copy of this request so it cannot be listed twice. + authRequests = currentState + .authRequests + .filterNot { it.id == action.authRequest.id } + .plus(action.authRequest) + .toImmutableList(), + devices = devices.toImmutableList(), + devicesLoaded = true, + ) + } + if (state.authRequestsLoaded) { + updateContentWithCurrentData() + } + } + private fun updateContentWithCurrentData() { val authRequestMap = state.authRequests.associateBy { it.id } val items = state.devices @@ -446,6 +500,22 @@ sealed class ManageDevicesAction { data class AuthRequestsResultReceive( val authRequestsUpdatesResult: AuthRequestsUpdatesResult, ) : Internal() + + /** + * Indicates that an incoming passwordless request has been received. + */ + data class PasswordlessAuthRequestReceive( + val authRequest: AuthRequest, + ) : Internal() + + /** + * Indicates that the devices accompanying an incoming passwordless request have been + * received. + */ + data class PasswordlessAuthRequestDevicesReceive( + val authRequest: AuthRequest, + val devicesResult: GetDevicesResult, + ) : Internal() } } 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..7b888ba74bc 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,124 @@ class AuthRequestManagerTest { } assertEquals(expected, result) } + + @Test + fun `getPasswordlessAuthRequestFlow should emit hydrated pending request`() = runTest { + fakeAuthDiskSource.userState = SINGLE_USER_STATE + coEvery { + authRequestsService.getAuthRequest(REQUEST_ID) + } returns PENDING_AUTH_REQUEST_RESPONSE.asSuccess() + coEvery { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } returns FINGER_PRINT.asSuccess() + + repository.getPasswordlessAuthRequestFlow().test { + mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) + + assertEquals(PENDING_AUTH_REQUEST, awaitItem()) + } + } + + @Test + fun `getPasswordlessAuthRequestFlow should emit nothing for non-active user`() = runTest { + fakeAuthDiskSource.userState = SINGLE_USER_STATE + coEvery { + authRequestsService.getAuthRequest(REQUEST_ID) + } returns PENDING_AUTH_REQUEST_RESPONSE.asSuccess() + coEvery { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } returns FINGER_PRINT.asSuccess() + + repository.getPasswordlessAuthRequestFlow().test { + mutablePasswordlessRequestFlow.emit( + PASSWORDLESS_REQUEST_DATA.copy(userId = "otherUserId"), + ) + mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) + + // Only the active user's request arrives, proving the other was dropped. + assertEquals(PENDING_AUTH_REQUEST, awaitItem()) + } + + coVerify(exactly = 1) { authRequestsService.getAuthRequest(REQUEST_ID) } + } + + @Test + fun `getPasswordlessAuthRequestFlow should emit nothing on request failure`() = runTest { + fakeAuthDiskSource.userState = SINGLE_USER_STATE + coEvery { authRequestsService.getAuthRequest(REQUEST_ID) } returnsMany listOf( + Throwable("Fail").asFailure(), + PENDING_AUTH_REQUEST_RESPONSE.asSuccess(), + ) + coEvery { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } returns FINGER_PRINT.asSuccess() + + repository.getPasswordlessAuthRequestFlow().test { + mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) + mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) + + assertEquals(PENDING_AUTH_REQUEST, awaitItem()) + } + } + + @Test + fun `getPasswordlessAuthRequestFlow should emit nothing when approved`() = runTest { + fakeAuthDiskSource.userState = SINGLE_USER_STATE + coEvery { authRequestsService.getAuthRequest(REQUEST_ID) } returnsMany listOf( + PENDING_AUTH_REQUEST_RESPONSE.copy(requestApproved = true).asSuccess(), + PENDING_AUTH_REQUEST_RESPONSE.asSuccess(), + ) + coEvery { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } returns FINGER_PRINT.asSuccess() + + repository.getPasswordlessAuthRequestFlow().test { + mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) + mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) + + assertEquals(PENDING_AUTH_REQUEST, awaitItem()) + } + } + + @Test + fun `getPasswordlessAuthRequestFlow should emit nothing when declined`() = runTest { + fakeAuthDiskSource.userState = SINGLE_USER_STATE + coEvery { authRequestsService.getAuthRequest(REQUEST_ID) } returnsMany listOf( + PENDING_AUTH_REQUEST_RESPONSE.copy(responseDate = fixedClock.instant()).asSuccess(), + PENDING_AUTH_REQUEST_RESPONSE.asSuccess(), + ) + coEvery { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } returns FINGER_PRINT.asSuccess() + + repository.getPasswordlessAuthRequestFlow().test { + mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) + mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) + + assertEquals(PENDING_AUTH_REQUEST, awaitItem()) + } + } + + @Test + fun `getPasswordlessAuthRequestFlow should emit nothing when expired`() = runTest { + fakeAuthDiskSource.userState = SINGLE_USER_STATE + coEvery { authRequestsService.getAuthRequest(REQUEST_ID) } returnsMany listOf( + PENDING_AUTH_REQUEST_RESPONSE + .copy(creationDate = Instant.parse("2023-10-27T11:54:00Z")) + .asSuccess(), + PENDING_AUTH_REQUEST_RESPONSE.asSuccess(), + ) + coEvery { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } returns FINGER_PRINT.asSuccess() + + repository.getPasswordlessAuthRequestFlow().test { + mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) + mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) + + assertEquals(PENDING_AUTH_REQUEST, awaitItem()) + } + } } private const val EMAIL: String = "test@bitwarden.com" @@ -1363,3 +1491,23 @@ private val AUTH_REQUEST_RESPONSE: AuthRequestResponse = AuthRequestResponse( accessCode = "accessCode", fingerprint = "fingerprint", ) + +private val PASSWORDLESS_REQUEST_DATA: PasswordlessRequestData = PasswordlessRequestData( + loginRequestId = REQUEST_ID, + userId = USER_ID, +) + +/** + * An unanswered request created one minute before the [AuthRequestManagerTest] clock, making it + * neither responded to nor expired. + */ +private val PENDING_AUTH_REQUEST_RESPONSE: AuthRequestsResponseJson.AuthRequest = + AUTH_REQUESTS_RESPONSE_JSON_AUTH_RESPONSE.copy( + creationDate = Instant.parse("2023-10-27T11:59:00Z"), + requestApproved = false, + ) + +private val PENDING_AUTH_REQUEST: AuthRequest = AUTH_REQUEST.copy( + creationDate = Instant.parse("2023-10-27T11:59:00Z"), + requestApproved = false, +) 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..df2e57a42f1 --- /dev/null +++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestsResponseJsonExtensionsTest.kt @@ -0,0 +1,56 @@ +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 fingerprint`() { + val fingerprint = "fingerprint" + + val result = AUTH_REQUEST_RESPONSE_JSON.toAuthRequest(fingerprint = fingerprint) + + assertEquals( + 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", + fingerprint = fingerprint, + ), + result, + ) + } + + @Test + fun `toAuthRequest should map a null requestApproved to false`() { + val result = AUTH_REQUEST_RESPONSE_JSON + .copy(requestApproved = null) + .toAuthRequest(fingerprint = "fingerprint") + + assertEquals(false, result.requestApproved) + } +} + +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/ManageDevicesViewModelTest.kt b/app/src/test/kotlin/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/managedevices/ManageDevicesViewModelTest.kt index 43e7f245908..dc10023c07c 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 @@ -51,8 +51,10 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { ) private val mutableAuthRequestsWithUpdatesFlow = bufferedMutableSharedFlow() + private val mutablePasswordlessAuthRequestFlow = bufferedMutableSharedFlow() private val authRepository = mockk { every { getAuthRequestsWithUpdates() } returns mutableAuthRequestsWithUpdatesFlow + every { getPasswordlessAuthRequestFlow() } returns mutablePasswordlessAuthRequestFlow coEvery { getDevices() } returns GetDevicesResult.Success(emptyList()) } private val mutablePullToRefreshStateFlow = MutableStateFlow(false) @@ -323,6 +325,73 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { } } + @Test + fun `passwordless request 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), + ) + mutablePasswordlessAuthRequestFlow.tryEmit(PASSWORDLESS_AUTH_REQUEST) + + viewModel.stateFlow.test { + assertEquals( + ManageDevicesState( + 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, + ), + ), + ), + isPullToRefreshSettingEnabled = false, + isRefreshing = false, + internalHideBottomSheet = false, + isFdroid = false, + devicesLoaded = true, + authRequestsLoaded = true, + ), + awaitItem(), + ) + } + } + + @Test + fun `passwordless request should leave the state untouched when the device fetch fails`() = + runTest { + val viewModel = createViewModel() + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Update(authRequests = emptyList()), + ) + val expectedState = viewModel.stateFlow.value + + coEvery { authRepository.getDevices() } returns GetDevicesResult.Error + mutablePasswordlessAuthRequestFlow.tryEmit(PASSWORDLESS_AUTH_REQUEST) + + assertEquals(expectedState, viewModel.stateFlow.value) + } + private fun createViewModel(state: ManageDevicesState? = null) = ManageDevicesViewModel( clock = fixedClock, authRepository = authRepository, @@ -333,6 +402,20 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { ) } +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", From 5b943e1795151dc2cb49daf220b457f423e5871e Mon Sep 17 00:00:00 2001 From: Andre Rosado Date: Tue, 18 Aug 2026 18:02:21 +0100 Subject: [PATCH 2/9] Exported isActionable to an utils to avoid duplicate code --- .../auth/manager/AuthRequestManagerImpl.kt | 16 ++--- .../manager/util/AuthRequestExtensions.kt | 15 +++++ .../managedevices/ManageDevicesViewModel.kt | 8 +-- .../PendingRequestsViewModel.kt | 8 +-- .../manager/util/AuthRequestExtensionsTest.kt | 58 +++++++++++++++++++ 5 files changed, 81 insertions(+), 24 deletions(-) create mode 100644 app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestExtensions.kt create mode 100644 app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestExtensionsTest.kt 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 07b287559a7..cb2ba520457 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 @@ -4,7 +4,6 @@ import com.bitwarden.core.AuthRequestResponse import com.bitwarden.core.data.util.asFailure import com.bitwarden.core.data.util.asSuccess import com.bitwarden.core.data.util.flatMap -import com.bitwarden.core.util.isOverFiveMinutesOld import com.bitwarden.network.model.AuthRequestTypeJson import com.bitwarden.network.service.AuthRequestsService import com.bitwarden.network.service.NewAuthRequestService @@ -18,6 +17,7 @@ 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.auth.manager.util.isActionable 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 @@ -274,7 +274,9 @@ class AuthRequestManagerImpl( Timber.d(it, "Unable to hydrate the requested auth request.") null }, - onSuccess = { authRequest -> authRequest.takeIf { it.isActionable } }, + onSuccess = { authRequest -> + authRequest.takeIf { it.isActionable(clock = clock) } + }, ) } @@ -425,16 +427,6 @@ class AuthRequestManagerImpl( publicKey = publicKey, ) } - - /** - * Whether this request may still be approved or declined, meaning it has not already been - * approved, not been declined (indicated by it not being approved & having a responseDate), - * and has not expired (it is under 5 minutes old). - */ - private val AuthRequest.isActionable: Boolean - get() = !requestApproved && - responseDate == null && - !creationDate.isOverFiveMinutesOld(clock) } /** 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..49bb40c0cbe --- /dev/null +++ b/app/src/main/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestExtensions.kt @@ -0,0 +1,15 @@ +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, meaning it has not already been + * approved, not been declined (indicated by it not being approved & having a responseDate), and + * has not expired (it is under 5 minutes old). + */ +fun AuthRequest.isActionable(clock: Clock): Boolean = + !requestApproved && + responseDate == null && + !creationDate.isOverFiveMinutesOld(clock) 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 e3866ab85f7..a1e8825bad3 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.isActionable 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 @@ -535,8 +535,4 @@ enum class DeviceSessionStatus { * * 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) - } + filter { it.isActionable(clock = 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..d5019210b62 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.isActionable import com.x8bit.bitwarden.data.auth.repository.AuthRepository import com.x8bit.bitwarden.data.platform.repository.SettingsRepository import com.x8bit.bitwarden.ui.platform.model.SnackbarRelay @@ -399,8 +399,4 @@ sealed class PendingRequestsAction { * * 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) - } + filter { it.isActionable(clock = clock) } 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..0f3050ec958 --- /dev/null +++ b/app/src/test/kotlin/com/x8bit/bitwarden/data/auth/manager/util/AuthRequestExtensionsTest.kt @@ -0,0 +1,58 @@ +package com.x8bit.bitwarden.data.auth.manager.util + +import com.x8bit.bitwarden.data.auth.manager.model.AuthRequest +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), + ) + } +} + +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", +) From 35b427f7fb9375a4c55df995c7c1bf0ffcb51ab8 Mon Sep 17 00:00:00 2001 From: Andre Rosado Date: Wed, 19 Aug 2026 13:10:52 +0100 Subject: [PATCH 3/9] Address PR comments --- .../auth/manager/AuthRequestManagerImpl.kt | 67 ++++++++++++++----- .../manager/util/AuthRequestExtensions.kt | 5 +- .../AuthRequestsResponseJsonExtensions.kt | 22 +++--- .../managedevices/ManageDevicesViewModel.kt | 51 +++++--------- .../AuthRequestsResponseJsonExtensionsTest.kt | 25 +++---- 5 files changed, 96 insertions(+), 74 deletions(-) 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 cb2ba520457..5ca81f3dcb8 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 @@ -98,7 +98,12 @@ class AuthRequestManagerImpl( isSso = authRequestType.isSso, ) .map { request -> - request.toAuthRequest(fingerprint = authRequest.fingerprint) + request.toAuthRequest( + fingerprint = authRequest.fingerprint, + publicKey = request.publicKey, + responseDate = request.responseDate, + isRequestApproved = request.requestApproved ?: false, + ) } .fold( onFailure = { emit(CreateAuthRequestResult.Error(error = it)) }, @@ -178,15 +183,14 @@ class AuthRequestManagerImpl( isRequestApproved = false responseDate = clock.instant() } - request - // The PublicKey and Fingerprint should be frozen in place to - // ensure no funny-business happens between multiple requests. - .toAuthRequest(fingerprint = initialAuthRequest.fingerprint) - .copy( - publicKey = initialAuthRequest.publicKey, - responseDate = responseDate, - requestApproved = isRequestApproved, - ) + // 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, + ) } } .fold( @@ -249,6 +253,9 @@ class AuthRequestManagerImpl( .mapCatching { response -> response.toAuthRequest( fingerprint = getFingerprintPhrase(response.publicKey).getOrThrow(), + publicKey = response.publicKey, + responseDate = response.responseDate, + isRequestApproved = response.requestApproved ?: false, ) } .fold( @@ -267,6 +274,9 @@ class AuthRequestManagerImpl( .mapCatching { response -> response.toAuthRequest( fingerprint = getFingerprintPhrase(response.publicKey).getOrThrow(), + publicKey = response.publicKey, + responseDate = response.responseDate, + isRequestApproved = response.requestApproved ?: false, ) } .fold( @@ -286,7 +296,12 @@ class AuthRequestManagerImpl( .flatMap { request -> if (request.requestApproved == true) { getFingerprintPhrase(request.publicKey).map { fingerprint -> - request.toAuthRequest(fingerprint = fingerprint) + request.toAuthRequest( + fingerprint = fingerprint, + publicKey = request.publicKey, + responseDate = request.responseDate, + isRequestApproved = true, + ) } } else { IllegalStateException("Request not approved.").asFailure() @@ -299,7 +314,12 @@ class AuthRequestManagerImpl( .map { response -> response.authRequests.mapNotNull { request -> getFingerprintPhrase(request.publicKey).getOrNull()?.let { fingerprint -> - request.toAuthRequest(fingerprint = fingerprint) + request.toAuthRequest( + fingerprint = fingerprint, + publicKey = request.publicKey, + responseDate = request.responseDate, + isRequestApproved = request.requestApproved ?: false, + ) } } } @@ -329,7 +349,14 @@ class AuthRequestManagerImpl( isApproved = isApproved, ) } - .map { request -> request.toAuthRequest(fingerprint = "") } + .map { request -> + request.toAuthRequest( + fingerprint = "", + publicKey = request.publicKey, + responseDate = request.responseDate, + isRequestApproved = request.requestApproved ?: false, + ) + } .fold( onFailure = { AuthRequestResult.Error(error = it) }, onSuccess = { AuthRequestResult.Success(authRequest = it) }, @@ -351,10 +378,13 @@ class AuthRequestManagerImpl( ?.let { pendingAuthRequest -> authRequestsService .getAuthRequest(pendingAuthRequest.requestId) - .map { + .map { request -> NewAuthRequestData( - authRequest = it.toAuthRequest( + authRequest = request.toAuthRequest( fingerprint = pendingAuthRequest.requestFingerprint, + publicKey = request.publicKey, + responseDate = request.responseDate, + isRequestApproved = request.requestApproved ?: false, ), privateKey = pendingAuthRequest.requestPrivateKey, accessCode = pendingAuthRequest.requestAccessCode, @@ -406,7 +436,12 @@ class AuthRequestManagerImpl( } } .map { request -> - request.toAuthRequest(fingerprint = authRequestResponse.fingerprint) + request.toAuthRequest( + fingerprint = authRequestResponse.fingerprint, + publicKey = request.publicKey, + responseDate = request.responseDate, + isRequestApproved = request.requestApproved ?: false, + ) } .map { NewAuthRequestData( 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 index 49bb40c0cbe..7394980f216 100644 --- 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 @@ -5,9 +5,8 @@ import com.x8bit.bitwarden.data.auth.manager.model.AuthRequest import java.time.Clock /** - * Whether this request may still be approved or declined, meaning it has not already been - * approved, not been declined (indicated by it not being approved & having a responseDate), and - * has not expired (it is under 5 minutes old). + * 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 && 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 index c7373de4791..ca638ffc60b 100644 --- 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 @@ -2,23 +2,29 @@ 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 = id, + id = this.id, publicKey = publicKey, - platform = platform, - ipAddress = ipAddress, - key = key, - masterPasswordHash = masterPasswordHash, - creationDate = creationDate, + platform = this.platform, + ipAddress = this.ipAddress, + key = this.key, + masterPasswordHash = this.masterPasswordHash, + creationDate = this.creationDate, responseDate = responseDate, - requestApproved = requestApproved ?: false, - originUrl = originUrl, + 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 a1e8825bad3..79a69e06719 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 @@ -72,11 +72,7 @@ class ManageDevicesViewModel @Inject constructor( init { updateAuthRequestList() fetchAllDevices() - authRepository - .getPasswordlessAuthRequestFlow() - .map { ManageDevicesAction.Internal.PasswordlessAuthRequestReceive(it) } - .onEach(::sendAction) - .launchIn(viewModelScope) + observePasswordlessAuthRequests() settingsRepository .getPullToRefreshEnabledFlow() .map { ManageDevicesAction.Internal.PullToRefreshEnableReceive(it) } @@ -156,10 +152,6 @@ class ManageDevicesViewModel @Inject constructor( is ManageDevicesAction.Internal.PasswordlessAuthRequestReceive -> { handlePasswordlessAuthRequestReceive(action) } - - is ManageDevicesAction.Internal.PasswordlessAuthRequestDevicesReceive -> { - handlePasswordlessAuthRequestDevicesReceive(action) - } } } @@ -186,6 +178,21 @@ class ManageDevicesViewModel @Inject constructor( .launchIn(viewModelScope) } + private fun observePasswordlessAuthRequests() { + authRepository + .getPasswordlessAuthRequestFlow() + .map { authRequest -> + // The device list is the only source that reports which device owns a pending + // request, so it is re-read before the new request can be rendered against it. + ManageDevicesAction.Internal.PasswordlessAuthRequestReceive( + authRequest = authRequest, + devicesResult = authRepository.getDevices(), + ) + } + .onEach(::sendAction) + .launchIn(viewModelScope) + } + private fun fetchAllDevices() { viewModelScope.launch { sendAction( @@ -243,21 +250,6 @@ class ManageDevicesViewModel @Inject constructor( private fun handlePasswordlessAuthRequestReceive( action: ManageDevicesAction.Internal.PasswordlessAuthRequestReceive, - ) { - // The device list is the only source that reports which device owns a pending request, so - // it is re-read before the new request can be rendered against its device. - viewModelScope.launch { - sendAction( - ManageDevicesAction.Internal.PasswordlessAuthRequestDevicesReceive( - authRequest = action.authRequest, - devicesResult = authRepository.getDevices(), - ), - ) - } - } - - private fun handlePasswordlessAuthRequestDevicesReceive( - action: ManageDevicesAction.Internal.PasswordlessAuthRequestDevicesReceive, ) { // This refresh is not user-initiated, so a failure leaves the screen untouched rather than // replacing it with an error; polling and pull-to-refresh reconcile it later. @@ -502,18 +494,11 @@ sealed class ManageDevicesAction { ) : Internal() /** - * Indicates that an incoming passwordless request has been received. + * Indicates that an incoming passwordless request has been received, along with the + * devices it should be rendered against. */ data class PasswordlessAuthRequestReceive( val authRequest: AuthRequest, - ) : Internal() - - /** - * Indicates that the devices accompanying an incoming passwordless request have been - * received. - */ - data class PasswordlessAuthRequestDevicesReceive( - val authRequest: AuthRequest, val devicesResult: GetDevicesResult, ) : Internal() } 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 index df2e57a42f1..aa0fc476610 100644 --- 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 @@ -8,37 +8,34 @@ import java.time.Instant class AuthRequestsResponseJsonExtensionsTest { @Test - fun `toAuthRequest should map each property and apply the given fingerprint`() { + 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) + val result = AUTH_REQUEST_RESPONSE_JSON.toAuthRequest( + fingerprint = fingerprint, + publicKey = "givenPublicKey", + responseDate = responseDate, + isRequestApproved = false, + ) assertEquals( AuthRequest( id = "1", - publicKey = "publicKey", + publicKey = "givenPublicKey", 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, + responseDate = responseDate, + requestApproved = false, originUrl = "www.bitwarden.com", fingerprint = fingerprint, ), result, ) } - - @Test - fun `toAuthRequest should map a null requestApproved to false`() { - val result = AUTH_REQUEST_RESPONSE_JSON - .copy(requestApproved = null) - .toAuthRequest(fingerprint = "fingerprint") - - assertEquals(false, result.requestApproved) - } } private val AUTH_REQUEST_RESPONSE_JSON: AuthRequestsResponseJson.AuthRequest = From 807127380d585fd20b3f914d2bafa1a14065db57 Mon Sep 17 00:00:00 2001 From: Andre Rosado Date: Fri, 21 Aug 2026 17:35:41 +0100 Subject: [PATCH 4/9] add repeated functions into extension --- .../manager/util/AuthRequestExtensions.kt | 9 ++++++ .../managedevices/ManageDevicesViewModel.kt | 11 +------ .../PendingRequestsViewModel.kt | 11 +------ .../manager/util/AuthRequestExtensionsTest.kt | 29 +++++++++++++++++++ 4 files changed, 40 insertions(+), 20 deletions(-) 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 index 7394980f216..15127777c81 100644 --- 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 @@ -12,3 +12,12 @@ 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/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 79a69e06719..e7139402fd3 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 @@ -17,7 +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.isActionable +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 @@ -512,12 +512,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) = - filter { it.isActionable(clock = 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 d5019210b62..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 @@ -14,7 +14,7 @@ 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.isActionable +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,12 +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) = - filter { it.isActionable(clock = clock) } 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 index 0f3050ec958..1f78c191de5 100644 --- 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 @@ -1,6 +1,7 @@ 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 @@ -41,6 +42,34 @@ class AuthRequestExtensionsTest { .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( From 85ed1ce193d91d32301d6cce66b4f77b78c704fe Mon Sep 17 00:00:00 2001 From: Andre Rosado Date: Mon, 24 Aug 2026 17:19:40 +0100 Subject: [PATCH 5/9] Merge push notifications into the auth request updates flow --- .../data/auth/manager/AuthRequestManager.kt | 12 +- .../auth/manager/AuthRequestManagerImpl.kt | 56 ++---- .../managedevices/ManageDevicesViewModel.kt | 107 +++--------- .../PendingRequestsViewModel.kt | 14 +- .../auth/manager/AuthRequestManagerTest.kt | 135 +++----------- .../managedevices/ManageDevicesScreenTest.kt | 1 - .../ManageDevicesViewModelTest.kt | 164 ++++++++---------- .../PendingRequestsViewModelTest.kt | 22 ++- 8 files changed, 176 insertions(+), 335 deletions(-) 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 5cbe6eab2d9..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,19 +32,11 @@ 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 - /** - * Get the [AuthRequest] for each incoming passwordless request for the active user, hydrated - * with the fingerprint required to approve it. - * - * Only requests that can still be acted upon are emitted; those already approved, declined, or - * expired are not. Requests that cannot be retrieved are omitted rather than emitted as errors. - */ - fun getPasswordlessAuthRequestFlow(): Flow - /** * Get an [AuthRequest] by its request ID. */ 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 5ca81f3dcb8..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 @@ -17,7 +17,6 @@ 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.auth.manager.util.isActionable 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 @@ -29,9 +28,9 @@ import kotlinx.coroutines.delay import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.flow -import kotlinx.coroutines.flow.mapNotNull +import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.merge import kotlinx.coroutines.isActive -import timber.log.Timber import java.time.Clock import java.time.Instant import javax.inject.Singleton @@ -57,20 +56,27 @@ class AuthRequestManagerImpl( ) : 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( @@ -264,32 +270,6 @@ class AuthRequestManagerImpl( ) } - override fun getPasswordlessAuthRequestFlow(): Flow = pushManager - .passwordlessRequestFlow - // A push for a non-active user would otherwise be hydrated with the active user's token. - .filter { it.userId == activeUserId } - .mapNotNull { data -> - authRequestsService - .getAuthRequest(data.loginRequestId) - .mapCatching { response -> - response.toAuthRequest( - fingerprint = getFingerprintPhrase(response.publicKey).getOrThrow(), - publicKey = response.publicKey, - responseDate = response.responseDate, - isRequestApproved = response.requestApproved ?: false, - ) - } - .fold( - onFailure = { - Timber.d(it, "Unable to hydrate the requested auth request.") - null - }, - onSuccess = { authRequest -> - authRequest.takeIf { it.isActionable(clock = clock) } - }, - ) - } - override suspend fun getAuthRequestIfApproved(requestId: String): Result = authRequestsService .getAuthRequest(requestId) 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 e7139402fd3..d7de6e2089f 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 @@ -64,15 +64,13 @@ class ManageDevicesViewModel @Inject constructor( 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() - observePasswordlessAuthRequests() settingsRepository .getPullToRefreshEnabledFlow() .map { ManageDevicesAction.Internal.PullToRefreshEnableReceive(it) } @@ -112,17 +110,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( @@ -148,10 +137,6 @@ class ManageDevicesViewModel @Inject constructor( is ManageDevicesAction.Internal.AuthRequestsResultReceive -> { handleAuthRequestsResultReceived(action) } - - is ManageDevicesAction.Internal.PasswordlessAuthRequestReceive -> { - handlePasswordlessAuthRequestReceive(action) - } } } @@ -178,23 +163,11 @@ class ManageDevicesViewModel @Inject constructor( .launchIn(viewModelScope) } - private fun observePasswordlessAuthRequests() { - authRepository - .getPasswordlessAuthRequestFlow() - .map { authRequest -> - // The device list is the only source that reports which device owns a pending - // request, so it is re-read before the new request can be rendered against it. - ManageDevicesAction.Internal.PasswordlessAuthRequestReceive( - authRequest = authRequest, - devicesResult = authRepository.getDevices(), - ) - } - .onEach(::sendAction) - .launchIn(viewModelScope) - } - 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(), @@ -213,16 +186,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( @@ -231,7 +198,17 @@ class ManageDevicesViewModel @Inject constructor( val devicesResult = action.devicesResult as? GetDevicesResult.Success ?: run { mutableStateFlow.update { - it.copy(viewState = ManageDevicesState.ViewState.Error, isRefreshing = false) + it.copy( + // Once devices have rendered, a failed refresh leaves the existing content + // in place rather than replacing it with an error; the next update + // reconciles it. + viewState = if (it.devicesLoaded) { + it.viewState + } else { + ManageDevicesState.ViewState.Error + }, + isRefreshing = false, + ) } return } @@ -240,38 +217,10 @@ class ManageDevicesViewModel @Inject constructor( it.copy( devices = devicesResult.devices.toImmutableList(), devicesLoaded = true, - isRefreshing = if (state.authRequestsLoaded) false else it.isRefreshing, + isRefreshing = false, ) } - if (state.authRequestsLoaded) { - updateContentWithCurrentData() - } - } - - private fun handlePasswordlessAuthRequestReceive( - action: ManageDevicesAction.Internal.PasswordlessAuthRequestReceive, - ) { - // This refresh is not user-initiated, so a failure leaves the screen untouched rather than - // replacing it with an error; polling and pull-to-refresh reconcile it later. - val devices = (action.devicesResult as? GetDevicesResult.Success) - ?.devices - ?: return - - mutableStateFlow.update { currentState -> - currentState.copy( - // Replaces any earlier copy of this request so it cannot be listed twice. - authRequests = currentState - .authRequests - .filterNot { it.id == action.authRequest.id } - .plus(action.authRequest) - .toImmutableList(), - devices = devices.toImmutableList(), - devicesLoaded = true, - ) - } - if (state.authRequestsLoaded) { - updateContentWithCurrentData() - } + updateContentWithCurrentData() } private fun updateContentWithCurrentData() { @@ -332,7 +281,6 @@ data class ManageDevicesState( private val internalHideBottomSheet: Boolean, private val isFdroid: Boolean, val devicesLoaded: Boolean, - val authRequestsLoaded: Boolean, ) : Parcelable { /** @@ -492,15 +440,6 @@ sealed class ManageDevicesAction { data class AuthRequestsResultReceive( val authRequestsUpdatesResult: AuthRequestsUpdatesResult, ) : Internal() - - /** - * Indicates that an incoming passwordless request has been received, along with the - * devices it should be rendered against. - */ - data class PasswordlessAuthRequestReceive( - val authRequest: AuthRequest, - val devicesResult: GetDevicesResult, - ) : Internal() } } 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 cb35f281d91..cb6cb32c2b3 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 @@ -198,10 +198,16 @@ class PendingRequestsViewModel @Inject constructor( is AuthRequestsUpdatesResult.Error -> { mutableStateFlow.update { - it.copy( - authRequests = emptyList(), - viewState = PendingRequestsState.ViewState.Error, - ) + // Once something has rendered, a failed refresh leaves it in place rather + // than replacing it with an error; the next update reconciles it. + if (it.viewState is PendingRequestsState.ViewState.Loading) { + it.copy( + authRequests = emptyList(), + viewState = PendingRequestsState.ViewState.Error, + ) + } else { + it + } } } } 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 7b888ba74bc..6a3924bce25 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 @@ -1292,123 +1292,38 @@ class AuthRequestManagerTest { assertEquals(expected, result) } + @Suppress("MaxLineLength") @Test - fun `getPasswordlessAuthRequestFlow should emit hydrated pending request`() = runTest { - fakeAuthDiskSource.userState = SINGLE_USER_STATE - coEvery { - authRequestsService.getAuthRequest(REQUEST_ID) - } returns PENDING_AUTH_REQUEST_RESPONSE.asSuccess() - coEvery { - authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) - } returns FINGER_PRINT.asSuccess() - - repository.getPasswordlessAuthRequestFlow().test { - mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) - - assertEquals(PENDING_AUTH_REQUEST, awaitItem()) - } - } - - @Test - fun `getPasswordlessAuthRequestFlow should emit nothing for non-active user`() = runTest { - fakeAuthDiskSource.userState = SINGLE_USER_STATE - coEvery { - authRequestsService.getAuthRequest(REQUEST_ID) - } returns PENDING_AUTH_REQUEST_RESPONSE.asSuccess() - coEvery { - authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) - } returns FINGER_PRINT.asSuccess() - - repository.getPasswordlessAuthRequestFlow().test { - mutablePasswordlessRequestFlow.emit( - PASSWORDLESS_REQUEST_DATA.copy(userId = "otherUserId"), + 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), ) - mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) - - // Only the active user's request arrives, proving the other was dropped. - assertEquals(PENDING_AUTH_REQUEST, awaitItem()) - } - - coVerify(exactly = 1) { authRequestsService.getAuthRequest(REQUEST_ID) } - } - - @Test - fun `getPasswordlessAuthRequestFlow should emit nothing on request failure`() = runTest { - fakeAuthDiskSource.userState = SINGLE_USER_STATE - coEvery { authRequestsService.getAuthRequest(REQUEST_ID) } returnsMany listOf( - Throwable("Fail").asFailure(), - PENDING_AUTH_REQUEST_RESPONSE.asSuccess(), - ) - coEvery { - authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) - } returns FINGER_PRINT.asSuccess() - - repository.getPasswordlessAuthRequestFlow().test { - mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) - mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) - - assertEquals(PENDING_AUTH_REQUEST, awaitItem()) - } - } - - @Test - fun `getPasswordlessAuthRequestFlow should emit nothing when approved`() = runTest { - fakeAuthDiskSource.userState = SINGLE_USER_STATE - coEvery { authRequestsService.getAuthRequest(REQUEST_ID) } returnsMany listOf( - PENDING_AUTH_REQUEST_RESPONSE.copy(requestApproved = true).asSuccess(), - PENDING_AUTH_REQUEST_RESPONSE.asSuccess(), - ) - coEvery { - authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) - } returns FINGER_PRINT.asSuccess() - - repository.getPasswordlessAuthRequestFlow().test { - mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) - mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) - - assertEquals(PENDING_AUTH_REQUEST, awaitItem()) - } - } - - @Test - fun `getPasswordlessAuthRequestFlow should emit nothing when declined`() = runTest { - fakeAuthDiskSource.userState = SINGLE_USER_STATE - coEvery { authRequestsService.getAuthRequest(REQUEST_ID) } returnsMany listOf( - PENDING_AUTH_REQUEST_RESPONSE.copy(responseDate = fixedClock.instant()).asSuccess(), - PENDING_AUTH_REQUEST_RESPONSE.asSuccess(), - ) - coEvery { - authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) - } returns FINGER_PRINT.asSuccess() - - repository.getPasswordlessAuthRequestFlow().test { - mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) - mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) + .asSuccess() + coEvery { + authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) + } returns FINGER_PRINT.asSuccess() + fakeAuthDiskSource.userState = SINGLE_USER_STATE - assertEquals(PENDING_AUTH_REQUEST, awaitItem()) - } - } + repository + .getAuthRequestsWithUpdates() + .test { + assertEquals(expected, awaitItem()) - @Test - fun `getPasswordlessAuthRequestFlow should emit nothing when expired`() = runTest { - fakeAuthDiskSource.userState = SINGLE_USER_STATE - coEvery { authRequestsService.getAuthRequest(REQUEST_ID) } returnsMany listOf( - PENDING_AUTH_REQUEST_RESPONSE - .copy(creationDate = Instant.parse("2023-10-27T11:54:00Z")) - .asSuccess(), - PENDING_AUTH_REQUEST_RESPONSE.asSuccess(), - ) - coEvery { - authSdkSource.getUserFingerprint(email = EMAIL, publicKey = PUBLIC_KEY) - } returns FINGER_PRINT.asSuccess() + mutablePasswordlessRequestFlow.emit( + PASSWORDLESS_REQUEST_DATA.copy(userId = "otherUserId"), + ) + mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) - repository.getPasswordlessAuthRequestFlow().test { - mutablePasswordlessRequestFlow.emit(PASSWORDLESS_REQUEST_DATA) - 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() + } - assertEquals(PENDING_AUTH_REQUEST, awaitItem()) + coVerify(exactly = 2) { authRequestsService.getAuthRequests() } } - } } private const val EMAIL: String = "test@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..59450e19043 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 @@ -243,5 +243,4 @@ private val DEFAULT_STATE = ManageDevicesState( 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 dc10023c07c..6f6e157c53e 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 @@ -51,10 +51,8 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { ) private val mutableAuthRequestsWithUpdatesFlow = bufferedMutableSharedFlow() - private val mutablePasswordlessAuthRequestFlow = bufferedMutableSharedFlow() private val authRepository = mockk { every { getAuthRequestsWithUpdates() } returns mutableAuthRequestsWithUpdatesFlow - every { getPasswordlessAuthRequestFlow() } returns mutablePasswordlessAuthRequestFlow coEvery { getDevices() } returns GetDevicesResult.Success(emptyList()) } private val mutablePullToRefreshStateFlow = MutableStateFlow(false) @@ -85,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 @@ -118,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 @@ -170,35 +159,42 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { } @Test - fun `when getDevices returns error should show error state`() { + fun `when the first 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 a later getDevices returns error should leave the rendered content in place`() { val viewModel = createViewModel() mutableAuthRequestsWithUpdatesFlow.tryEmit( - AuthRequestsUpdatesResult.Error(error = Throwable()), + AuthRequestsUpdatesResult.Update(authRequests = emptyList()), ) - assertEquals( - emptyList(), - viewModel.stateFlow.value.authRequests, + assertEquals(EMPTY_CONTENT_STATE, viewModel.stateFlow.value) + + coEvery { authRepository.getDevices() } returns GetDevicesResult.Error + mutableAuthRequestsWithUpdatesFlow.tryEmit( + AuthRequestsUpdatesResult.Update(authRequests = emptyList()), ) + + coVerify(exactly = 2) { authRepository.getDevices() } + assertEquals(EMPTY_CONTENT_STATE, 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 @@ -269,13 +265,14 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { ) viewModel.stateFlow.test { assertEquals( - ManageDevicesState( + DEFAULT_STATE.copy( authRequests = listOf(validAuthRequest).toImmutableList(), devices = listOf( otherDevice, pendingDevice, currentDevice, ).toImmutableList(), + devicesLoaded = true, viewState = ManageDevicesState.ViewState.Content( items = listOf( ManageDevicesState.ViewState.Content.DeviceItem( @@ -313,12 +310,6 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { ), ), ), - isPullToRefreshSettingEnabled = false, - isRefreshing = false, - internalHideBottomSheet = false, - isFdroid = false, - devicesLoaded = true, - authRequestsLoaded = true, ), awaitItem(), ) @@ -326,7 +317,7 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { } @Test - fun `passwordless request should add a pending row for its device`() = runTest { + 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( @@ -343,13 +334,17 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { coEvery { authRepository.getDevices() } returns GetDevicesResult.Success( devices = listOf(pendingDevice), ) - mutablePasswordlessAuthRequestFlow.tryEmit(PASSWORDLESS_AUTH_REQUEST) + // 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( - ManageDevicesState( + DEFAULT_STATE.copy( authRequests = listOf(PASSWORDLESS_AUTH_REQUEST).toImmutableList(), devices = listOf(pendingDevice).toImmutableList(), + devicesLoaded = true, viewState = ManageDevicesState.ViewState.Content( items = listOf( ManageDevicesState.ViewState.Content.DeviceItem( @@ -365,33 +360,12 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { ), ), ), - isPullToRefreshSettingEnabled = false, - isRefreshing = false, - internalHideBottomSheet = false, - isFdroid = false, - devicesLoaded = true, - authRequestsLoaded = true, ), awaitItem(), ) } } - @Test - fun `passwordless request should leave the state untouched when the device fetch fails`() = - runTest { - val viewModel = createViewModel() - mutableAuthRequestsWithUpdatesFlow.tryEmit( - AuthRequestsUpdatesResult.Update(authRequests = emptyList()), - ) - val expectedState = viewModel.stateFlow.value - - coEvery { authRepository.getDevices() } returns GetDevicesResult.Error - mutablePasswordlessAuthRequestFlow.tryEmit(PASSWORDLESS_AUTH_REQUEST) - - assertEquals(expectedState, viewModel.stateFlow.value) - } - private fun createViewModel(state: ManageDevicesState? = null) = ManageDevicesViewModel( clock = fixedClock, authRepository = authRepository, @@ -402,6 +376,22 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { ) } +private val DEFAULT_STATE = ManageDevicesState( + authRequests = persistentListOf(), + devices = persistentListOf(), + viewState = ManageDevicesState.ViewState.Loading, + isPullToRefreshSettingEnabled = false, + isRefreshing = false, + internalHideBottomSheet = false, + isFdroid = false, + devicesLoaded = false, +) + +private val EMPTY_CONTENT_STATE = DEFAULT_STATE.copy( + viewState = ManageDevicesState.ViewState.Content(items = emptyList()), + devicesLoaded = true, +) + private val PASSWORDLESS_AUTH_REQUEST = AuthRequest( id = "auth-req-push", publicKey = "publicKey", 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..2391340e522 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 @@ -195,13 +195,33 @@ class PendingRequestsViewModelTest : BaseViewModelTest() { val expected = DEFAULT_STATE.copy( viewState = PendingRequestsState.ViewState.Error, ) - val viewModel = createViewModel() + val viewModel = createViewModel( + state = DEFAULT_STATE.copy(viewState = PendingRequestsState.ViewState.Loading), + ) mutableAuthRequestsWithUpdatesFlow.tryEmit( value = AuthRequestsUpdatesResult.Error(error = Throwable()), ) assertEquals(expected, viewModel.stateFlow.value) } + @Test + fun `getPendingResults failure after a successful load should leave the state in place`() { + val viewModel = createViewModel( + state = DEFAULT_STATE.copy(viewState = PendingRequestsState.ViewState.Loading), + ) + mutableAuthRequestsWithUpdatesFlow.tryEmit( + value = AuthRequestsUpdatesResult.Update(authRequests = emptyList()), + ) + val expected = DEFAULT_STATE.copy(viewState = PendingRequestsState.ViewState.Empty) + assertEquals(expected, viewModel.stateFlow.value) + + mutableAuthRequestsWithUpdatesFlow.tryEmit( + value = AuthRequestsUpdatesResult.Error(error = Throwable()), + ) + + assertEquals(expected, viewModel.stateFlow.value) + } + @Test fun `on CloseClick should emit NavigateBack`() = runTest { val viewModel = createViewModel() From f1f45da9883ad9a187db1ad3a79659c41abd0d92 Mon Sep 17 00:00:00 2001 From: Andre Rosado Date: Tue, 25 Aug 2026 13:11:46 +0100 Subject: [PATCH 6/9] Showing error when fetch fails --- .../PendingRequestsViewModel.kt | 14 ++++---------- .../auth/manager/AuthRequestManagerTest.kt | 15 --------------- .../PendingRequestsViewModelTest.kt | 18 +++++++++++------- 3 files changed, 15 insertions(+), 32 deletions(-) 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 cb6cb32c2b3..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 @@ -198,16 +198,10 @@ class PendingRequestsViewModel @Inject constructor( is AuthRequestsUpdatesResult.Error -> { mutableStateFlow.update { - // Once something has rendered, a failed refresh leaves it in place rather - // than replacing it with an error; the next update reconciles it. - if (it.viewState is PendingRequestsState.ViewState.Loading) { - it.copy( - authRequests = emptyList(), - viewState = PendingRequestsState.ViewState.Error, - ) - } else { - it - } + it.copy( + authRequests = emptyList(), + viewState = PendingRequestsState.ViewState.Error, + ) } } } 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 6a3924bce25..fc75eac513c 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 @@ -1411,18 +1411,3 @@ private val PASSWORDLESS_REQUEST_DATA: PasswordlessRequestData = PasswordlessReq loginRequestId = REQUEST_ID, userId = USER_ID, ) - -/** - * An unanswered request created one minute before the [AuthRequestManagerTest] clock, making it - * neither responded to nor expired. - */ -private val PENDING_AUTH_REQUEST_RESPONSE: AuthRequestsResponseJson.AuthRequest = - AUTH_REQUESTS_RESPONSE_JSON_AUTH_RESPONSE.copy( - creationDate = Instant.parse("2023-10-27T11:59:00Z"), - requestApproved = false, - ) - -private val PENDING_AUTH_REQUEST: AuthRequest = AUTH_REQUEST.copy( - creationDate = Instant.parse("2023-10-27T11:59:00Z"), - requestApproved = false, -) 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 2391340e522..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 @@ -195,9 +195,7 @@ class PendingRequestsViewModelTest : BaseViewModelTest() { val expected = DEFAULT_STATE.copy( viewState = PendingRequestsState.ViewState.Error, ) - val viewModel = createViewModel( - state = DEFAULT_STATE.copy(viewState = PendingRequestsState.ViewState.Loading), - ) + val viewModel = createViewModel() mutableAuthRequestsWithUpdatesFlow.tryEmit( value = AuthRequestsUpdatesResult.Error(error = Throwable()), ) @@ -205,21 +203,27 @@ class PendingRequestsViewModelTest : BaseViewModelTest() { } @Test - fun `getPendingResults failure after a successful load should leave the state in place`() { + 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()), ) - val expected = DEFAULT_STATE.copy(viewState = PendingRequestsState.ViewState.Empty) - assertEquals(expected, viewModel.stateFlow.value) + 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(expected, viewModel.stateFlow.value) + assertEquals( + DEFAULT_STATE.copy(viewState = PendingRequestsState.ViewState.Error), + viewModel.stateFlow.value, + ) } @Test From e8042cf5d46128b6efa13a34385525c9f875a098 Mon Sep 17 00:00:00 2001 From: Andre Rosado Date: Tue, 25 Aug 2026 17:23:52 +0100 Subject: [PATCH 7/9] reverted to display error when loading devices --- .../managedevices/ManageDevicesViewModel.kt | 12 +----------- .../managedevices/ManageDevicesScreenTest.kt | 3 +-- .../managedevices/ManageDevicesViewModelTest.kt | 14 +++++++------- 3 files changed, 9 insertions(+), 20 deletions(-) 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 d7de6e2089f..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 @@ -63,7 +63,6 @@ class ManageDevicesViewModel @Inject constructor( isRefreshing = false, internalHideBottomSheet = false, isFdroid = buildInfoManager.isFdroid, - devicesLoaded = false, ), ) { private var authJob: Job = Job().apply { complete() } @@ -199,14 +198,7 @@ class ManageDevicesViewModel @Inject constructor( ?: run { mutableStateFlow.update { it.copy( - // Once devices have rendered, a failed refresh leaves the existing content - // in place rather than replacing it with an error; the next update - // reconciles it. - viewState = if (it.devicesLoaded) { - it.viewState - } else { - ManageDevicesState.ViewState.Error - }, + viewState = ManageDevicesState.ViewState.Error, isRefreshing = false, ) } @@ -216,7 +208,6 @@ class ManageDevicesViewModel @Inject constructor( mutableStateFlow.update { it.copy( devices = devicesResult.devices.toImmutableList(), - devicesLoaded = true, isRefreshing = false, ) } @@ -280,7 +271,6 @@ data class ManageDevicesState( val isRefreshing: Boolean, private val internalHideBottomSheet: Boolean, private val isFdroid: Boolean, - val devicesLoaded: Boolean, ) : Parcelable { /** 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 59450e19043..65511e90d96 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,7 @@ 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 composeTestRule.onNodeWithText("Skip for now").assertDoesNotExist() } @@ -242,5 +242,4 @@ private val DEFAULT_STATE = ManageDevicesState( isRefreshing = false, internalHideBottomSheet = false, isFdroid = false, - devicesLoaded = 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 6f6e157c53e..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 @@ -159,7 +159,7 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { } @Test - fun `when the first getDevices returns error should show error state`() { + fun `when getDevices returns error should show error state`() { coEvery { authRepository.getDevices() } returns GetDevicesResult.Error val viewModel = createViewModel() mutableAuthRequestsWithUpdatesFlow.tryEmit( @@ -172,20 +172,24 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { } @Test - fun `when a later getDevices returns error should leave the rendered content in place`() { + fun `when getDevices returns error after content should show error state`() { val viewModel = createViewModel() mutableAuthRequestsWithUpdatesFlow.tryEmit( 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(EMPTY_CONTENT_STATE, viewModel.stateFlow.value) + assertEquals( + DEFAULT_STATE.copy(viewState = ManageDevicesState.ViewState.Error), + viewModel.stateFlow.value, + ) } @Test @@ -272,7 +276,6 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { pendingDevice, currentDevice, ).toImmutableList(), - devicesLoaded = true, viewState = ManageDevicesState.ViewState.Content( items = listOf( ManageDevicesState.ViewState.Content.DeviceItem( @@ -344,7 +347,6 @@ class ManageDevicesViewModelTest : BaseViewModelTest() { DEFAULT_STATE.copy( authRequests = listOf(PASSWORDLESS_AUTH_REQUEST).toImmutableList(), devices = listOf(pendingDevice).toImmutableList(), - devicesLoaded = true, viewState = ManageDevicesState.ViewState.Content( items = listOf( ManageDevicesState.ViewState.Content.DeviceItem( @@ -384,12 +386,10 @@ private val DEFAULT_STATE = ManageDevicesState( isRefreshing = false, internalHideBottomSheet = false, isFdroid = false, - devicesLoaded = false, ) private val EMPTY_CONTENT_STATE = DEFAULT_STATE.copy( viewState = ManageDevicesState.ViewState.Content(items = emptyList()), - devicesLoaded = true, ) private val PASSWORDLESS_AUTH_REQUEST = AuthRequest( From c378660b09bbbbc2429897fb3f4c15371ba363ba Mon Sep 17 00:00:00 2001 From: Andre Rosado Date: Wed, 26 Aug 2026 12:03:03 +0100 Subject: [PATCH 8/9] addressed ManageDevices screen test error --- .../accountsecurity/managedevices/ManageDevicesScreenTest.kt | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 65511e90d96..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 + mutableStateFlow.value = DEFAULT_STATE.copy( + viewState = ManageDevicesState.ViewState.Content(items = emptyList()), + ) composeTestRule.onNodeWithText("Skip for now").assertDoesNotExist() } From 2cdc315c74434175cd91d3ddd61488a83edf2903 Mon Sep 17 00:00:00 2001 From: Andre Rosado Date: Thu, 27 Aug 2026 18:21:53 +0100 Subject: [PATCH 9/9] removed unnecessary suppresion --- .../x8bit/bitwarden/data/auth/manager/AuthRequestManagerTest.kt | 1 - 1 file changed, 1 deletion(-) 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 fc75eac513c..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 @@ -1292,7 +1292,6 @@ class AuthRequestManagerTest { assertEquals(expected, result) } - @Suppress("MaxLineLength") @Test fun `getAuthRequestsWithUpdates should re-read on a push and ignore a non-active user push`() = runTest {