From e41e68170038698e6e92b5d021db5b0a13b0d4f2 Mon Sep 17 00:00:00 2001 From: Brian Yencho Date: Tue, 28 Nov 2023 13:28:23 -0600 Subject: [PATCH] Lock vault and clear data when logging out (#288) --- .../auth/repository/AuthRepositoryImpl.kt | 7 ++ .../data/vault/repository/VaultRepository.kt | 5 ++ .../vault/repository/VaultRepositoryImpl.kt | 14 ++++ .../auth/repository/AuthRepositoryTest.kt | 9 +++ .../vault/repository/VaultRepositoryTest.kt | 77 +++++++++++++++++++ 5 files changed, 112 insertions(+) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt index 9e10786e5c..4cd08a7979 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryImpl.kt @@ -202,6 +202,7 @@ class AuthRepositoryImpl constructor( override fun logout(userId: String) { val currentUserState = authDiskSource.userState ?: return + val wasActiveUser = userId == activeUserId // Remove the active user from the accounts map val updatedAccounts = currentUserState @@ -228,6 +229,12 @@ class AuthRepositoryImpl constructor( // Update the user information and log out authDiskSource.userState = null } + + // Lock the vault for the logged out user + vaultRepository.lockVaultIfNecessary(userId) + + // Clear the current vault data if the logged out user was the active one. + if (wasActiveUser) vaultRepository.clearUnlockedData() } @Suppress("ReturnCount", "LongMethod") 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 eea967f962..5ac1dcab07 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 @@ -52,6 +52,11 @@ interface VaultRepository { */ fun getVaultFolderStateFlow(folderId: String): StateFlow> + /** + * Locks the vault for the user with the given [userId] if necessary. + */ + fun lockVaultIfNecessary(userId: String) + /** * Attempt to unlock the vault and sync the vault data for the currently active user. */ 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 a01a7e9560..c779416a37 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 @@ -155,6 +155,10 @@ class VaultRepositoryImpl constructor( initialValue = DataState.Loading, ) + override fun lockVaultIfNecessary(userId: String) { + setVaultToLocked(userId = userId) + } + @Suppress("ReturnCount") override suspend fun unlockVaultAndSyncForCurrentUser( masterPassword: String, @@ -233,6 +237,16 @@ class VaultRepositoryImpl constructor( } } + // TODO: This is temporary. Eventually this needs to be based on the presence of various + // user keys but this will likely require SDK updates to support this (BIT-1190). + private fun setVaultToLocked(userId: String) { + vaultMutableStateFlow.update { + it.copy( + unlockedVaultUserIds = it.unlockedVaultUserIds - userId, + ) + } + } + private fun storeUserKeyAndPrivateKey( userKey: String?, privateKey: String?, diff --git a/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt index 507abc6389..8c8e4ff218 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/auth/repository/AuthRepositoryTest.kt @@ -50,6 +50,7 @@ import io.mockk.mockk import io.mockk.mockkStatic import io.mockk.runs import io.mockk.unmockkStatic +import io.mockk.verify import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.AfterEach @@ -69,6 +70,8 @@ class AuthRepositoryTest { private val mutableVaultStateFlow = MutableStateFlow(VAULT_STATE) private val vaultRepository: VaultRepository = mockk() { every { vaultStateFlow } returns mutableVaultStateFlow + every { lockVaultIfNecessary(any()) } just runs + every { clearUnlockedData() } just runs } private val fakeAuthDiskSource = FakeAuthDiskSource() private val fakeEnvironmentRepository = @@ -846,6 +849,8 @@ class AuthRepositoryTest { userId = USER_ID_1, userKey = null, ) + verify { vaultRepository.clearUnlockedData() } + verify { vaultRepository.lockVaultIfNecessary(userId = USER_ID_1) } } } @@ -907,6 +912,8 @@ class AuthRepositoryTest { userId = USER_ID_1, userKey = null, ) + verify { vaultRepository.clearUnlockedData() } + verify { vaultRepository.lockVaultIfNecessary(userId = USER_ID_1) } } } @@ -937,6 +944,8 @@ class AuthRepositoryTest { userId = USER_ID_2, userKey = null, ) + verify(exactly = 0) { vaultRepository.clearUnlockedData() } + verify { vaultRepository.lockVaultIfNecessary(userId = USER_ID_2) } } } 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 2c23192702..cb1f9f0447 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 @@ -463,6 +463,29 @@ class VaultRepositoryTest { } } + @Test + fun `lockVaultIfNecessary should lock the given account if it is currently unlocked`() = + runTest { + val userId = "userId" + verifyUnlockedVault(userId = userId) + + assertEquals( + VaultState( + unlockedVaultUserIds = setOf(userId), + ), + vaultRepository.vaultStateFlow.value, + ) + + vaultRepository.lockVaultIfNecessary(userId = userId) + + assertEquals( + VaultState( + unlockedVaultUserIds = emptySet(), + ), + vaultRepository.vaultStateFlow.value, + ) + } + @Suppress("MaxLineLength") @Test fun `unlockVaultAndSyncForCurrentUser with unlockVault Success should sync and return Success`() = @@ -1357,6 +1380,60 @@ class VaultRepositoryTest { vaultSdkSource.decryptSendList(listOf(createMockSdkSend(1))) } } + + //region Helper functions + + /** + * Helper to ensures that the vault for the user with the given [userId] is unlocked. + */ + private suspend fun verifyUnlockedVault(userId: String) { + val kdf = MOCK_PROFILE.toSdkParams() + val email = MOCK_PROFILE.email + val masterPassword = "drowssap" + val userKey = "12345" + val privateKey = "54321" + val organizationalKeys = emptyMap() + coEvery { + vaultSdkSource.initializeCrypto( + request = InitUserCryptoRequest( + kdfParams = kdf, + email = email, + privateKey = privateKey, + method = InitUserCryptoMethod.Password( + password = masterPassword, + userKey = userKey, + ), + ), + ) + } returns InitializeCryptoResult.Success.asSuccess() + + val result = vaultRepository.unlockVault( + userId = userId, + masterPassword = masterPassword, + kdf = kdf, + email = email, + userKey = userKey, + privateKey = privateKey, + organizationalKeys = organizationalKeys, + ) + + assertEquals(VaultUnlockResult.Success, result) + coVerify(exactly = 1) { + vaultSdkSource.initializeCrypto( + request = InitUserCryptoRequest( + kdfParams = kdf, + email = email, + privateKey = privateKey, + method = InitUserCryptoMethod.Password( + password = masterPassword, + userKey = userKey, + ), + ), + ) + } + } + + //endregion Helper functions } private val MOCK_PROFILE = AccountJson.Profile(