From 01ab047d9c014445a60aa30750e8678dea70bc29 Mon Sep 17 00:00:00 2001 From: Patrick Honkonen <1883101+SaintPatrck@users.noreply.github.com> Date: Wed, 2 Oct 2024 13:50:18 -0400 Subject: [PATCH] [PM-13074] Explicitly sync FIDO2 credentials (#4012) --- .../sdk/model/Fido2CredentialStoreImpl.kt | 15 +- .../data/vault/repository/VaultRepository.kt | 7 + .../vault/repository/VaultRepositoryImpl.kt | 159 ++++++++++-------- .../repository/model/SyncVaultDataResult.kt | 18 ++ .../vault/repository/VaultRepositoryTest.kt | 90 ++++++++-- 5 files changed, 207 insertions(+), 82 deletions(-) create mode 100644 app/src/main/java/com/x8bit/bitwarden/data/vault/repository/model/SyncVaultDataResult.kt diff --git a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/Fido2CredentialStoreImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/Fido2CredentialStoreImpl.kt index ad0fa3f7f..a119f58bb 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/Fido2CredentialStoreImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/Fido2CredentialStoreImpl.kt @@ -9,6 +9,7 @@ import com.x8bit.bitwarden.data.autofill.util.isActiveWithFido2Credentials import com.x8bit.bitwarden.data.platform.annotation.OmitFromCoverage import com.x8bit.bitwarden.data.vault.datasource.sdk.VaultSdkSource import com.x8bit.bitwarden.data.vault.repository.VaultRepository +import com.x8bit.bitwarden.data.vault.repository.model.SyncVaultDataResult /** * Primary implementation of [Fido2CredentialStore]. @@ -24,7 +25,12 @@ class Fido2CredentialStoreImpl( * Return all active ciphers that contain FIDO 2 credentials. */ override suspend fun allCredentials(): List { - vaultRepository.sync() + val syncResult = vaultRepository.syncFido2Credentials() + if (syncResult is SyncVaultDataResult.Error) { + syncResult.throwable + ?.let { throw it } + ?: throw IllegalStateException("Sync failed.") + } return vaultRepository.ciphersStateFlow.value.data ?.filter { it.isActiveWithFido2Credentials } ?: emptyList() @@ -40,7 +46,12 @@ class Fido2CredentialStoreImpl( override suspend fun findCredentials(ids: List?, ripId: String): List { val userId = getActiveUserIdOrThrow() - vaultRepository.sync() + val syncResult = vaultRepository.syncFido2Credentials() + if (syncResult is SyncVaultDataResult.Error) { + syncResult.throwable + ?.let { throw it } + ?: throw IllegalStateException("Sync failed.") + } val ciphersWithFido2Credentials = vaultRepository.ciphersStateFlow.value.data ?.filter { it.isActiveWithFido2Credentials } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/VaultRepository.kt b/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/VaultRepository.kt index 63f4cd187..16f02bd2c 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/VaultRepository.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/VaultRepository.kt @@ -24,6 +24,7 @@ import com.x8bit.bitwarden.data.vault.repository.model.ExportVaultDataResult import com.x8bit.bitwarden.data.vault.repository.model.GenerateTotpResult import com.x8bit.bitwarden.data.vault.repository.model.RemovePasswordSendResult import com.x8bit.bitwarden.data.vault.repository.model.SendData +import com.x8bit.bitwarden.data.vault.repository.model.SyncVaultDataResult import com.x8bit.bitwarden.data.vault.repository.model.TotpCodeResult import com.x8bit.bitwarden.data.vault.repository.model.UpdateFolderResult import com.x8bit.bitwarden.data.vault.repository.model.UpdateSendResult @@ -116,6 +117,12 @@ interface VaultRepository : CipherManager, VaultLockManager { */ fun syncIfNecessary() + /** + * Syncs the user's FIDO 2 credentials. This is an explicit request to sync and is not dependent + * on whether the last sync time was sufficiently in the past. + */ + suspend fun syncFido2Credentials(): SyncVaultDataResult + /** * Flow that represents the data for a specific vault item as found by ID. This may emit `null` * if the item cannot be found. diff --git a/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryImpl.kt index 9f3ce2754..c4ebeccbb 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryImpl.kt @@ -65,6 +65,7 @@ import com.x8bit.bitwarden.data.vault.repository.model.ExportVaultDataResult import com.x8bit.bitwarden.data.vault.repository.model.GenerateTotpResult import com.x8bit.bitwarden.data.vault.repository.model.RemovePasswordSendResult import com.x8bit.bitwarden.data.vault.repository.model.SendData +import com.x8bit.bitwarden.data.vault.repository.model.SyncVaultDataResult import com.x8bit.bitwarden.data.vault.repository.model.TotpCodeResult import com.x8bit.bitwarden.data.vault.repository.model.UpdateFolderResult import com.x8bit.bitwarden.data.vault.repository.model.UpdateSendResult @@ -83,9 +84,11 @@ import com.x8bit.bitwarden.data.vault.repository.util.toEncryptedSdkSend import com.x8bit.bitwarden.data.vault.repository.util.toEncryptedSdkSendList import com.x8bit.bitwarden.ui.vault.feature.vault.model.VaultFilterType import com.x8bit.bitwarden.ui.vault.feature.vault.util.toFilteredList +import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job +import kotlinx.coroutines.async import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.SharingStarted @@ -312,7 +315,6 @@ class VaultRepositoryImpl( } } - @Suppress("LongMethod") override fun sync() { val userId = activeUserId ?: return if (!syncJob.isCompleted) return @@ -321,74 +323,7 @@ class VaultRepositoryImpl( mutableFoldersStateFlow.updateToPendingOrLoading() mutableCollectionsStateFlow.updateToPendingOrLoading() mutableSendDataStateFlow.updateToPendingOrLoading() - syncJob = ioScope.launch { - val lastSyncInstant = settingsDiskSource - .getLastSyncTime(userId = userId) - ?.toEpochMilli() - ?: 0 - - syncService - .getAccountRevisionDateMillis() - .fold( - onSuccess = { serverRevisionDate -> - if (serverRevisionDate < lastSyncInstant) { - // We can skip the actual sync call if there is no new data - vaultDiskSource.resyncVaultData(userId) - settingsDiskSource.storeLastSyncTime( - userId = userId, - lastSyncTime = clock.instant(), - ) - return@launch - } - }, - onFailure = { - updateVaultStateFlowsToError(it) - return@launch - }, - ) - - syncService - .sync() - .fold( - onSuccess = { syncResponse -> - val localSecurityStamp = - authDiskSource.userState?.activeAccount?.profile?.stamp - val serverSecurityStamp = syncResponse.profile.securityStamp - - // Log the user out if the stamps do not match - localSecurityStamp?.let { - if (serverSecurityStamp != localSecurityStamp) { - userLogoutManager.softLogout(userId = userId, isExpired = true) - return@launch - } - } - - // Update user information with additional information from sync response - authDiskSource.userState = authDiskSource - .userState - ?.toUpdatedUserStateJson( - syncResponse = syncResponse, - ) - - unlockVaultForOrganizationsIfNecessary(syncResponse = syncResponse) - storeProfileData(syncResponse = syncResponse) - // Treat absent network policies as known empty data to - // distinguish between unknown null data. - authDiskSource.storePolicies( - userId = userId, - policies = syncResponse.policies.orEmpty(), - ) - settingsDiskSource.storeLastSyncTime( - userId = userId, - lastSyncTime = clock.instant(), - ) - vaultDiskSource.replaceVaultData(userId = userId, vault = syncResponse) - }, - onFailure = { throwable -> - updateVaultStateFlowsToError(throwable) - }, - ) - } + syncJob = ioScope.launch { syncInternal(userId) } } @Suppress("MagicNumber") @@ -405,6 +340,20 @@ class VaultRepositoryImpl( } } + override suspend fun syncFido2Credentials(): SyncVaultDataResult { + val userId = activeUserId + ?: return SyncVaultDataResult.Error(throwable = null) + syncJob = ioScope + .async { syncInternal(userId) } + .also { + return try { + it.await() + } catch (e: CancellationException) { + SyncVaultDataResult.Error(throwable = e) + } + } + } + override fun getVaultItemStateFlow(itemId: String): StateFlow> = vaultDataStateFlow .map { dataState -> @@ -1355,6 +1304,78 @@ class VaultRepositoryImpl( .onSuccess { vaultDiskSource.saveFolder(userId, it) } } //endregion Push Notification helpers + + @Suppress("LongMethod") + private suspend fun syncInternal(userId: String): SyncVaultDataResult { + val lastSyncInstant = settingsDiskSource + .getLastSyncTime(userId = userId) + ?.toEpochMilli() + ?: 0 + + syncService + .getAccountRevisionDateMillis() + .fold( + onSuccess = { serverRevisionDate -> + if (serverRevisionDate < lastSyncInstant) { + // We can skip the actual sync call if there is no new data + vaultDiskSource.resyncVaultData(userId) + settingsDiskSource.storeLastSyncTime( + userId = userId, + lastSyncTime = clock.instant(), + ) + return SyncVaultDataResult.Success + } + }, + onFailure = { + updateVaultStateFlowsToError(it) + return SyncVaultDataResult.Error(it) + }, + ) + + syncService + .sync() + .fold( + onSuccess = { syncResponse -> + val localSecurityStamp = + authDiskSource.userState?.activeAccount?.profile?.stamp + val serverSecurityStamp = syncResponse.profile.securityStamp + + // Log the user out if the stamps do not match + localSecurityStamp?.let { + if (serverSecurityStamp != localSecurityStamp) { + userLogoutManager.softLogout(userId = userId, isExpired = true) + return SyncVaultDataResult.Error(throwable = null) + } + } + + // Update user information with additional information from sync response + authDiskSource.userState = authDiskSource + .userState + ?.toUpdatedUserStateJson( + syncResponse = syncResponse, + ) + + unlockVaultForOrganizationsIfNecessary(syncResponse = syncResponse) + storeProfileData(syncResponse = syncResponse) + // Treat absent network policies as known empty data to + // distinguish between unknown null data. + authDiskSource.storePolicies( + userId = userId, + policies = syncResponse.policies.orEmpty(), + ) + settingsDiskSource.storeLastSyncTime( + userId = userId, + lastSyncTime = clock.instant(), + ) + vaultDiskSource.replaceVaultData(userId = userId, vault = syncResponse) + return SyncVaultDataResult.Success + }, + onFailure = { throwable -> + updateVaultStateFlowsToError(throwable) + return SyncVaultDataResult.Error(throwable) + }, + ) + } } private fun Throwable.toNetworkOrErrorState(data: T?): DataState = diff --git a/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/model/SyncVaultDataResult.kt b/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/model/SyncVaultDataResult.kt new file mode 100644 index 000000000..cbccb7db7 --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/repository/model/SyncVaultDataResult.kt @@ -0,0 +1,18 @@ +package com.x8bit.bitwarden.data.vault.repository.model + +/** + * Represents the result of a sync operation. + */ +sealed class SyncVaultDataResult { + /** + * Indicates a successful sync operation. + */ + data object Success : SyncVaultDataResult() + + /** + * Indicates a failed sync operation. + * + * @property throwable The exception that caused the failure, if any. + */ + data class Error(val throwable: Throwable?) : SyncVaultDataResult() +} diff --git a/app/src/test/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryTest.kt index f321f6f40..fce5988df 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/vault/repository/VaultRepositoryTest.kt @@ -86,6 +86,7 @@ import com.x8bit.bitwarden.data.vault.repository.model.ExportVaultDataResult import com.x8bit.bitwarden.data.vault.repository.model.GenerateTotpResult import com.x8bit.bitwarden.data.vault.repository.model.RemovePasswordSendResult import com.x8bit.bitwarden.data.vault.repository.model.SendData +import com.x8bit.bitwarden.data.vault.repository.model.SyncVaultDataResult import com.x8bit.bitwarden.data.vault.repository.model.UpdateFolderResult import com.x8bit.bitwarden.data.vault.repository.model.UpdateSendResult import com.x8bit.bitwarden.data.vault.repository.model.VaultData @@ -1072,20 +1073,24 @@ class VaultRepositoryTest { } @Test - fun `sync when the last sync time is more recent than the revision date should not sync `() { - val userId = "mockId-1" - fakeAuthDiskSource.userState = MOCK_USER_STATE - every { - settingsDiskSource.getLastSyncTime(userId = userId) - } returns clock.instant().plus(2, ChronoUnit.MINUTES) + fun `sync when the last sync time is more recent than the revision date should not sync `() = + runTest { + val userId = "mockId-1" + fakeAuthDiskSource.userState = MOCK_USER_STATE + every { + settingsDiskSource.getLastSyncTime(userId = userId) + } returns clock.instant().plus(2, ChronoUnit.MINUTES) - vaultRepository.sync() + vaultRepository.sync() - verify(exactly = 1) { - settingsDiskSource.storeLastSyncTime(userId = userId, lastSyncTime = clock.instant()) + verify(exactly = 1) { + settingsDiskSource.storeLastSyncTime( + userId = userId, + lastSyncTime = clock.instant(), + ) + } + coVerify(exactly = 0) { syncService.sync() } } - coVerify(exactly = 0) { syncService.sync() } - } @Test fun `lockVaultForCurrentUser should delegate to the VaultLockManager`() { @@ -4329,6 +4334,69 @@ class VaultRepositoryTest { } } + @Test + fun `syncFido2Credentials should return result`() = runTest { + fakeAuthDiskSource.userState = MOCK_USER_STATE + val userId = "mockId-1" + val mockSyncResponse = createMockSyncResponse(number = 1) + coEvery { + syncService.sync() + } returns mockSyncResponse.asSuccess() + coEvery { + vaultSdkSource.initializeOrganizationCrypto( + userId = userId, + request = InitOrgCryptoRequest( + organizationKeys = createMockOrganizationKeys(1), + ), + ) + } returns InitializeCryptoResult.Success.asSuccess() + coEvery { + vaultDiskSource.replaceVaultData( + userId = MOCK_USER_STATE.activeUserId, + vault = mockSyncResponse, + ) + } just runs + + every { + settingsDiskSource.storeLastSyncTime( + MOCK_USER_STATE.activeUserId, + clock.instant(), + ) + } just runs + + val syncResult = vaultRepository.syncFido2Credentials() + assertEquals(SyncVaultDataResult.Success, syncResult) + } + + @Test + fun `syncFido2Credentials should return error when getAccountRevisionDateMillis fails`() = + runTest { + fakeAuthDiskSource.userState = MOCK_USER_STATE + val throwable = Throwable() + coEvery { + syncService.getAccountRevisionDateMillis() + } returns throwable.asFailure() + val syncResult = vaultRepository.syncFido2Credentials() + assertEquals( + SyncVaultDataResult.Error(throwable = throwable), + syncResult, + ) + } + + @Test + fun `syncFido2Credentials should return error when sync fails`() = runTest { + fakeAuthDiskSource.userState = MOCK_USER_STATE + val throwable = Throwable() + coEvery { + syncService.sync() + } returns throwable.asFailure() + val syncResult = vaultRepository.syncFido2Credentials() + assertEquals( + SyncVaultDataResult.Error(throwable = throwable), + syncResult, + ) + } + //region Helper functions /**