From 45a9100fdec120ffd9ad9b652fc170f58ac6422f Mon Sep 17 00:00:00 2001 From: Brian Yencho Date: Mon, 8 Jan 2024 13:31:46 -0600 Subject: [PATCH] Set Settings defaults on login and clear them on logout (#537) --- .../auth/repository/AuthRepositoryImpl.kt | 8 +++ .../repository/di/AuthRepositoryModule.kt | 3 + .../platform/repository/SettingsRepository.kt | 11 ++++ .../repository/SettingsRepositoryImpl.kt | 21 +++++++ .../auth/repository/AuthRepositoryTest.kt | 13 +++- .../repository/SettingsRepositoryTest.kt | 63 +++++++++++++++++++ 6 files changed, 118 insertions(+), 1 deletion(-) 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 d61a512bea..cab7acc17c 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 @@ -33,6 +33,7 @@ import com.x8bit.bitwarden.data.auth.util.KdfParamsConstants.DEFAULT_PBKDF2_ITER import com.x8bit.bitwarden.data.auth.util.toSdkParams import com.x8bit.bitwarden.data.platform.manager.dispatcher.DispatcherManager import com.x8bit.bitwarden.data.platform.repository.EnvironmentRepository +import com.x8bit.bitwarden.data.platform.repository.SettingsRepository import com.x8bit.bitwarden.data.platform.repository.util.bufferedMutableSharedFlow import com.x8bit.bitwarden.data.platform.util.asFailure import com.x8bit.bitwarden.data.platform.util.flatMap @@ -62,6 +63,7 @@ class AuthRepositoryImpl constructor( private val authSdkSource: AuthSdkSource, private val authDiskSource: AuthDiskSource, private val environmentRepository: EnvironmentRepository, + private val settingsRepository: SettingsRepository, private val vaultRepository: VaultRepository, dispatcherManager: DispatcherManager, ) : AuthRepository { @@ -210,6 +212,9 @@ class AuthRepositoryImpl constructor( userId = userStateJson.activeUserId, privateKey = loginResponse.privateKey, ) + settingsRepository.setDefaultsIfNecessary( + userId = userStateJson.activeUserId, + ) vaultRepository.sync() specialCircumstance = null LoginResult.Success @@ -275,6 +280,9 @@ class AuthRepositoryImpl constructor( authDiskSource.userState = null } + // Clear settings + settingsRepository.clearData(userId) + // Delete all the vault data vaultRepository.deleteVaultData(userId) diff --git a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/di/AuthRepositoryModule.kt b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/di/AuthRepositoryModule.kt index 2214af28fa..10634359c6 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/di/AuthRepositoryModule.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/di/AuthRepositoryModule.kt @@ -9,6 +9,7 @@ import com.x8bit.bitwarden.data.auth.repository.AuthRepository import com.x8bit.bitwarden.data.auth.repository.AuthRepositoryImpl import com.x8bit.bitwarden.data.platform.manager.dispatcher.DispatcherManager import com.x8bit.bitwarden.data.platform.repository.EnvironmentRepository +import com.x8bit.bitwarden.data.platform.repository.SettingsRepository import com.x8bit.bitwarden.data.vault.repository.VaultRepository import dagger.Module import dagger.Provides @@ -33,6 +34,7 @@ object AuthRepositoryModule { authDiskSource: AuthDiskSource, dispatchers: DispatcherManager, environmentRepository: EnvironmentRepository, + settingsRepository: SettingsRepository, vaultRepository: VaultRepository, ): AuthRepository = AuthRepositoryImpl( accountsService = accountsService, @@ -42,6 +44,7 @@ object AuthRepositoryModule { haveIBeenPwnedService = haveIBeenPwnedService, dispatcherManager = dispatchers, environmentRepository = environmentRepository, + settingsRepository = settingsRepository, vaultRepository = vaultRepository, ) } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepository.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepository.kt index 5015af6e43..7e878c19d4 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepository.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepository.kt @@ -18,6 +18,17 @@ interface SettingsRepository { */ var vaultTimeoutAction: VaultTimeoutAction + /** + * Clears all the settings data for the given user. + */ + fun clearData(userId: String) + + /** + * Sets default values for various settings for the given [userId] if necessary. This is + * typically used when logging into a new account. + */ + fun setDefaultsIfNecessary(userId: String) + /** * Gets updates for the [VaultTimeout] associated with the given [userId]. */ diff --git a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryImpl.kt index 94f9709ff4..edeaaa6f98 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryImpl.kt @@ -51,6 +51,27 @@ class SettingsRepositoryImpl( ) } + override fun clearData(userId: String) { + settingsDiskSource.apply { + storeVaultTimeoutInMinutes( + userId = userId, + vaultTimeoutInMinutes = null, + ) + storeVaultTimeoutAction( + userId = userId, + vaultTimeoutAction = null, + ) + } + } + + override fun setDefaultsIfNecessary(userId: String) { + // Set Vault Settings defaults + if (!isVaultTimeoutActionSet(userId = userId)) { + storeVaultTimeout(userId, VaultTimeout.ThirtyMinutes) + storeVaultTimeoutAction(userId, VaultTimeoutAction.LOCK) + } + } + override fun getVaultTimeoutStateFlow(userId: String): StateFlow = settingsDiskSource .getVaultTimeoutInMinutesFlow(userId = userId) 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 1a85d4409a..88215f1dd6 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 @@ -42,6 +42,7 @@ import com.x8bit.bitwarden.data.auth.repository.util.toUserStateJson import com.x8bit.bitwarden.data.auth.util.toSdkParams import com.x8bit.bitwarden.data.platform.base.FakeDispatcherManager import com.x8bit.bitwarden.data.platform.manager.dispatcher.DispatcherManager +import com.x8bit.bitwarden.data.platform.repository.SettingsRepository import com.x8bit.bitwarden.data.platform.repository.model.Environment import com.x8bit.bitwarden.data.platform.repository.util.FakeEnvironmentRepository import com.x8bit.bitwarden.data.platform.util.asFailure @@ -89,6 +90,10 @@ class AuthRepositoryTest { .apply { environment = Environment.Us } + private val settingsRepository: SettingsRepository = mockk() { + every { clearData(any()) } just runs + every { setDefaultsIfNecessary(any()) } just runs + } private val authSdkSource = mockk { coEvery { hashPassword( @@ -123,6 +128,7 @@ class AuthRepositoryTest { authSdkSource = authSdkSource, authDiskSource = fakeAuthDiskSource, environmentRepository = fakeEnvironmentRepository, + settingsRepository = settingsRepository, vaultRepository = vaultRepository, dispatcherManager = dispatcherManager, ) @@ -498,6 +504,7 @@ class AuthRepositoryTest { fakeAuthDiskSource.userState, ) assertNull(repository.specialCircumstance) + verify { settingsRepository.setDefaultsIfNecessary(userId = USER_ID_1) } verify(exactly = 0) { vaultRepository.lockVaultIfNecessary(any()) } verify { vaultRepository.clearUnlockedData() } } @@ -579,6 +586,7 @@ class AuthRepositoryTest { fakeAuthDiskSource.userState, ) assertNull(repository.specialCircumstance) + verify { settingsRepository.setDefaultsIfNecessary(userId = USER_ID_1) } verify { vaultRepository.lockVaultIfNecessary(userId = USER_ID_2) } verify { vaultRepository.clearUnlockedData() } } @@ -949,7 +957,7 @@ class AuthRepositoryTest { @Suppress("MaxLineLength") @Test - fun `logout for single account should clear the access token and profile data`() = runTest { + fun `logout for single account should clear the access token and stored data`() = runTest { // First login: val successResponse = GET_TOKEN_RESPONSE_SUCCESS coEvery { @@ -1029,6 +1037,7 @@ class AuthRepositoryTest { userId = USER_ID_1, organizations = null, ) + verify { settingsRepository.clearData(userId = USER_ID_1) } verify { vaultRepository.deleteVaultData(userId = USER_ID_1) } verify { vaultRepository.clearUnlockedData() } verify { vaultRepository.lockVaultIfNecessary(userId = USER_ID_1) } @@ -1112,6 +1121,7 @@ class AuthRepositoryTest { userId = USER_ID_1, organizationKeys = null, ) + verify { settingsRepository.clearData(userId = USER_ID_1) } verify { vaultRepository.deleteVaultData(userId = USER_ID_1) } verify { vaultRepository.clearUnlockedData() } verify { vaultRepository.lockVaultIfNecessary(userId = USER_ID_1) } @@ -1163,6 +1173,7 @@ class AuthRepositoryTest { userId = USER_ID_2, organizationKeys = null, ) + verify { settingsRepository.clearData(userId = USER_ID_2) } verify { vaultRepository.deleteVaultData(userId = USER_ID_2) } verify(exactly = 0) { vaultRepository.clearUnlockedData() } verify { vaultRepository.lockVaultIfNecessary(userId = USER_ID_2) } diff --git a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryTest.kt index 4d3a18f75c..496f2b382d 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/platform/repository/SettingsRepositoryTest.kt @@ -11,6 +11,7 @@ import io.mockk.mockk import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Assertions.assertFalse +import org.junit.jupiter.api.Assertions.assertNull import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Test @@ -24,6 +25,68 @@ class SettingsRepositoryTest { dispatcherManager = FakeDispatcherManager(), ) + @Test + fun `clearData should clear all necessary data for the given user`() { + val userId = "userId" + + fakeSettingsDiskSource.apply { + storeVaultTimeoutInMinutes( + userId = userId, + vaultTimeoutInMinutes = 30, + ) + storeVaultTimeoutAction( + userId = userId, + vaultTimeoutAction = VaultTimeoutAction.LOCK, + ) + } + + settingsRepository.clearData(userId = userId) + + assertNull(fakeSettingsDiskSource.getVaultTimeoutInMinutes(userId = userId)) + assertNull(fakeSettingsDiskSource.getVaultTimeoutAction(userId = userId)) + } + + @Test + fun `setDefaultsIfNecessary should set default values for the given user if necessary`() { + val userId = "userId" + assertNull(fakeSettingsDiskSource.getVaultTimeoutInMinutes(userId = userId)) + assertNull(fakeSettingsDiskSource.getVaultTimeoutAction(userId = userId)) + + settingsRepository.setDefaultsIfNecessary(userId = userId) + + // Calling once sets values + assertEquals( + 30, + fakeSettingsDiskSource.getVaultTimeoutInMinutes(userId = userId), + ) + assertEquals( + VaultTimeoutAction.LOCK, + fakeSettingsDiskSource.getVaultTimeoutAction(userId = userId), + ) + + // Updating the Vault settings values and calling setDefaultsIfNecessary again has no effect + // on the currently stored values. + fakeSettingsDiskSource.apply { + storeVaultTimeoutInMinutes( + userId = userId, + vaultTimeoutInMinutes = 240, + ) + storeVaultTimeoutAction( + userId = userId, + vaultTimeoutAction = VaultTimeoutAction.LOGOUT, + ) + } + settingsRepository.setDefaultsIfNecessary(userId = userId) + assertEquals( + 240, + fakeSettingsDiskSource.getVaultTimeoutInMinutes(userId = userId), + ) + assertEquals( + VaultTimeoutAction.LOGOUT, + fakeSettingsDiskSource.getVaultTimeoutAction(userId = userId), + ) + } + @Test fun `vaultTimeout should pull from and update SettingsDiskSource for the current user`() { every { authDiskSource.userState?.activeUserId } returns null