From 7644d35ca2bb1203cacbb19cb25b57bb8a4c4142 Mon Sep 17 00:00:00 2001 From: Brian Yencho Date: Sun, 28 Jan 2024 21:30:39 -0600 Subject: [PATCH] Move validatePassword to AuthRepository and ensure errors are caught (#841) --- .../data/auth/repository/AuthRepository.kt | 6 + .../auth/repository/AuthRepositoryImpl.kt | 22 ++++ .../model/ValidatePasswordResult.kt | 19 ++++ .../platform/repository/SettingsRepository.kt | 5 - .../repository/SettingsRepositoryImpl.kt | 15 --- .../vault/datasource/sdk/VaultSdkSource.kt | 2 +- .../datasource/sdk/VaultSdkSourceImpl.kt | 3 +- .../exportvault/ExportVaultViewModel.kt | 48 +++++--- .../auth/repository/AuthRepositoryTest.kt | 105 ++++++++++++++++++ .../repository/SettingsRepositoryTest.kt | 34 ------ .../datasource/sdk/VaultSdkSourceTest.kt | 8 +- .../exportvault/ExportVaultViewModelTest.kt | 42 +++++-- 12 files changed, 227 insertions(+), 82 deletions(-) create mode 100644 app/src/main/java/com/x8bit/bitwarden/data/auth/repository/model/ValidatePasswordResult.kt diff --git a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepository.kt b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepository.kt index 37da6b5e82..6a60645754 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepository.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/AuthRepository.kt @@ -18,6 +18,7 @@ import com.x8bit.bitwarden.data.auth.repository.model.RegisterResult import com.x8bit.bitwarden.data.auth.repository.model.ResendEmailResult import com.x8bit.bitwarden.data.auth.repository.model.SwitchAccountResult import com.x8bit.bitwarden.data.auth.repository.model.UserState +import com.x8bit.bitwarden.data.auth.repository.model.ValidatePasswordResult import com.x8bit.bitwarden.data.auth.repository.util.CaptchaCallbackTokenResult import com.x8bit.bitwarden.data.auth.repository.util.SsoCallbackResult import com.x8bit.bitwarden.data.platform.datasource.network.authenticator.AuthenticatorProvider @@ -221,4 +222,9 @@ interface AuthRepository : AuthenticatorProvider { * Get the password strength for the given [email] and [password] combo. */ suspend fun getPasswordStrength(email: String, password: String): PasswordStrengthResult + + /** + * Validates the master password for the current logged in user. + */ + suspend fun validatePassword(password: String): ValidatePasswordResult } 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 462dc630a0..7808383539 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 @@ -43,6 +43,7 @@ import com.x8bit.bitwarden.data.auth.repository.model.ResendEmailResult import com.x8bit.bitwarden.data.auth.repository.model.SwitchAccountResult import com.x8bit.bitwarden.data.auth.repository.model.UserFingerprintResult import com.x8bit.bitwarden.data.auth.repository.model.UserState +import com.x8bit.bitwarden.data.auth.repository.model.ValidatePasswordResult import com.x8bit.bitwarden.data.auth.repository.model.VaultUnlockType import com.x8bit.bitwarden.data.auth.repository.util.CaptchaCallbackTokenResult import com.x8bit.bitwarden.data.auth.repository.util.SsoCallbackResult @@ -777,6 +778,27 @@ class AuthRepositoryImpl( }, ) + @Suppress("ReturnCount") + override suspend fun validatePassword(password: String): ValidatePasswordResult { + val userId = activeUserId ?: return ValidatePasswordResult.Error + val masterPasswordHash = authDiskSource.getMasterPasswordHash(userId = userId) + ?: return ValidatePasswordResult.Error + return vaultSdkSource + .validatePassword( + userId = userId, + password = password, + passwordHash = masterPasswordHash, + ) + .fold( + onSuccess = { + ValidatePasswordResult.Success(isValid = it) + }, + onFailure = { + ValidatePasswordResult.Error + }, + ) + } + private suspend fun getFingerprintPhrase( publicKey: String, ): UserFingerprintResult { diff --git a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/model/ValidatePasswordResult.kt b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/model/ValidatePasswordResult.kt new file mode 100644 index 0000000000..a36f605d00 --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/model/ValidatePasswordResult.kt @@ -0,0 +1,19 @@ +package com.x8bit.bitwarden.data.auth.repository.model + +/** + * Models result of determining if a password is valid. + */ +sealed class ValidatePasswordResult { + + /** + * The validity of the password was checked successfully and [isValid]. + */ + data class Success( + val isValid: Boolean, + ) : ValidatePasswordResult() + + /** + * There was an error determining if the validity of the password. + */ + data object Error : ValidatePasswordResult() +} 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 1140b5fc9a..a7db831cdd 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 @@ -197,9 +197,4 @@ interface SettingsRepository { * Clears any previously set unlock PIN for the current user. */ fun clearUnlockPin() - - /** - * Validate the master password. - */ - suspend fun validatePassword(password: String): Boolean } 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 b8d38d7181..14667d0664 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 @@ -430,21 +430,6 @@ class SettingsRepositoryImpl( } .launchIn(unconfinedScope) } - - override suspend fun validatePassword(password: String): Boolean = - activeUserId - ?.let { userId -> - authDiskSource - .getMasterPasswordHash(userId) - ?.let { passwordHash -> - vaultSdkSource.validatePassword( - userId = userId, - password = password, - passwordHash = passwordHash, - ) - } - } - ?: false } /** diff --git a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSource.kt b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSource.kt index 34ff8c3468..617f24aca3 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSource.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSource.kt @@ -311,5 +311,5 @@ interface VaultSdkSource { userId: String, password: String, passwordHash: String, - ): Boolean + ): Result } diff --git a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSourceImpl.kt b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSourceImpl.kt index 1ba22b806c..a2fd8e3f62 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSourceImpl.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSourceImpl.kt @@ -320,13 +320,14 @@ class VaultSdkSourceImpl( userId: String, password: String, passwordHash: String, - ): Boolean = + ): Result = runCatching { getClient(userId = userId) .auth() .validatePassword( password = password, passwordHash = passwordHash, ) + } private fun getClient( userId: String, diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/exportvault/ExportVaultViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/exportvault/ExportVaultViewModel.kt index 1565d9b63e..366d6a25a2 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/exportvault/ExportVaultViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/exportvault/ExportVaultViewModel.kt @@ -4,7 +4,8 @@ import android.os.Parcelable import androidx.lifecycle.SavedStateHandle import androidx.lifecycle.viewModelScope import com.x8bit.bitwarden.R -import com.x8bit.bitwarden.data.platform.repository.SettingsRepository +import com.x8bit.bitwarden.data.auth.repository.AuthRepository +import com.x8bit.bitwarden.data.auth.repository.model.ValidatePasswordResult import com.x8bit.bitwarden.ui.platform.base.BaseViewModel import com.x8bit.bitwarden.ui.platform.base.util.Text import com.x8bit.bitwarden.ui.platform.base.util.asText @@ -24,7 +25,7 @@ private const val KEY_STATE = "state" */ @HiltViewModel class ExportVaultViewModel @Inject constructor( - private val settingsRepository: SettingsRepository, + private val authRepository: AuthRepository, savedStateHandle: SavedStateHandle, ) : BaseViewModel( initialState = savedStateHandle[KEY_STATE] @@ -85,7 +86,7 @@ class ExportVaultViewModel @Inject constructor( viewModelScope.launch { sendAction( ExportVaultAction.Internal.ReceiveValidatePasswordResult( - isPasswordValid = settingsRepository.validatePassword( + result = authRepository.validatePassword( password = mutableStateFlow.value.passwordInput, ), ), @@ -124,19 +125,34 @@ class ExportVaultViewModel @Inject constructor( private fun handleReceiveValidatePasswordResult( action: ExportVaultAction.Internal.ReceiveValidatePasswordResult, ) { - // Display an error dialog if the password is invalid. - if (!action.isPasswordValid) { - mutableStateFlow.update { - it.copy( - dialogState = ExportVaultState.DialogState.Error( - title = null, - message = R.string.invalid_master_password.asText(), - ), - ) + when (action.result) { + ValidatePasswordResult.Error -> { + mutableStateFlow.update { + it.copy( + dialogState = ExportVaultState.DialogState.Error( + title = null, + message = R.string.generic_error_message.asText(), + ), + ) + } + } + + is ValidatePasswordResult.Success -> { + // Display an error dialog if the password is invalid. + if (!action.result.isValid) { + mutableStateFlow.update { + it.copy( + dialogState = ExportVaultState.DialogState.Error( + title = null, + message = R.string.invalid_master_password.asText(), + ), + ) + } + } else { + // TODO: BIT-1274, BIT-1275, and BIT-1276 + sendEvent(ExportVaultEvent.ShowToast("Not yet implemented".asText())) + } } - } else { - // TODO: BIT-1274, BIT-1275, and BIT-1276 - sendEvent(ExportVaultEvent.ShowToast("Not yet implemented".asText())) } } } @@ -226,7 +242,7 @@ sealed class ExportVaultAction { * Indicates that a validate password result has been received. */ data class ReceiveValidatePasswordResult( - val isPasswordValid: Boolean, + val result: ValidatePasswordResult, ) : Internal() } } 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 1dd5576547..4232ef908c 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 @@ -54,6 +54,7 @@ import com.x8bit.bitwarden.data.auth.repository.model.RegisterResult import com.x8bit.bitwarden.data.auth.repository.model.ResendEmailResult import com.x8bit.bitwarden.data.auth.repository.model.SwitchAccountResult import com.x8bit.bitwarden.data.auth.repository.model.UserOrganizations +import com.x8bit.bitwarden.data.auth.repository.model.ValidatePasswordResult import com.x8bit.bitwarden.data.auth.repository.model.VaultUnlockType import com.x8bit.bitwarden.data.auth.repository.util.CaptchaCallbackTokenResult import com.x8bit.bitwarden.data.auth.repository.util.SsoCallbackResult @@ -2668,6 +2669,110 @@ class AuthRepositoryTest { ) } + @Test + fun `validatePassword with no current user returns ValidatePasswordResult Error`() = runTest { + val userId = "userId" + val password = "password" + val passwordHash = "passwordHash" + fakeAuthDiskSource.userState = null + coEvery { + vaultSdkSource.validatePassword( + userId = userId, + password = password, + passwordHash = passwordHash, + ) + } returns true.asSuccess() + + val result = repository + .validatePassword( + password = password, + ) + + assertEquals( + ValidatePasswordResult.Error, + result, + ) + } + + @Suppress("MaxLineLength") + @Test + fun `validatePassword with no stored password hash returns ValidatePasswordResult Error`() = + runTest { + val userId = USER_ID_1 + val password = "password" + val passwordHash = "passwordHash" + fakeAuthDiskSource.userState = SINGLE_USER_STATE_1 + coEvery { + vaultSdkSource.validatePassword( + userId = userId, + password = password, + passwordHash = passwordHash, + ) + } returns true.asSuccess() + + val result = repository + .validatePassword( + password = password, + ) + + assertEquals( + ValidatePasswordResult.Error, + result, + ) + } + + @Test + fun `validatePassword with sdk failure returns a ValidatePasswordResult Error`() = runTest { + val userId = USER_ID_1 + val password = "password" + val passwordHash = "passwordHash" + fakeAuthDiskSource.userState = SINGLE_USER_STATE_1 + fakeAuthDiskSource.storeMasterPasswordHash(userId = userId, passwordHash = passwordHash) + coEvery { + vaultSdkSource.validatePassword( + userId = userId, + password = password, + passwordHash = passwordHash, + ) + } returns Throwable().asFailure() + + val result = repository + .validatePassword( + password = password, + ) + + assertEquals( + ValidatePasswordResult.Error, + result, + ) + } + + @Test + fun `validatePassword with sdk success returns a ValidatePasswordResult Success`() = runTest { + val userId = USER_ID_1 + val password = "password" + val passwordHash = "passwordHash" + fakeAuthDiskSource.userState = SINGLE_USER_STATE_1 + fakeAuthDiskSource.storeMasterPasswordHash(userId = userId, passwordHash = passwordHash) + coEvery { + vaultSdkSource.validatePassword( + userId = userId, + password = password, + passwordHash = passwordHash, + ) + } returns true.asSuccess() + + val result = repository + .validatePassword( + password = password, + ) + + assertEquals( + ValidatePasswordResult.Success(isValid = true), + result, + ) + } + companion object { private const val UNIQUE_APP_ID = "testUniqueAppId" private const val EMAIL = "test@bitwarden.com" 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 e13ef613c6..5357e7e84a 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 @@ -884,40 +884,6 @@ class SettingsRepositoryTest { assertEquals(false, fakeSettingsDiskSource.getScreenCaptureAllowed(userId)) } } - - @Test - fun `validatePassword returns the validate password result`() = runTest { - val userId = "userId" - val password = "password" - val passwordHash = "passwordHash" - fakeAuthDiskSource.userState = MOCK_USER_STATE - fakeAuthDiskSource.storeMasterPasswordHash(userId = userId, passwordHash = passwordHash) - coEvery { - vaultSdkSource.validatePassword( - userId = userId, - password = password, - passwordHash = passwordHash, - ) - } returns true - - val result = settingsRepository - .validatePassword( - password = password, - ) - assertTrue(result) - } - - @Test - fun `validatePassword returns false if there's no stored password hash`() = runTest { - fakeAuthDiskSource.userState = MOCK_USER_STATE - val password = "password" - - val result = settingsRepository - .validatePassword( - password = password, - ) - assertFalse(result) - } } private val MOCK_USER_STATE = diff --git a/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSourceTest.kt b/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSourceTest.kt index fbc47c16c3..98fd4bb33f 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSourceTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/VaultSdkSourceTest.kt @@ -39,7 +39,6 @@ import io.mockk.verify import kotlinx.coroutines.runBlocking import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.Assertions.assertEquals -import org.junit.jupiter.api.Assertions.assertTrue import org.junit.jupiter.api.Test @Suppress("LargeClass") @@ -742,7 +741,7 @@ class VaultSdkSourceTest { } @Test - fun `validatePassword should call SDK and return the expected Boolean`() = runTest { + fun `validatePassword should call SDK and a Result with correct data`() = runTest { val userId = "userId" val password = "password" val passwordHash = "passwordHash" @@ -758,6 +757,9 @@ class VaultSdkSourceTest { password = password, passwordHash = passwordHash, ) - assertTrue(result) + assertEquals( + true.asSuccess(), + result, + ) } } diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/exportvault/ExportVaultViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/exportvault/ExportVaultViewModelTest.kt index eefb8f8570..40383b7833 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/exportvault/ExportVaultViewModelTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/exportvault/ExportVaultViewModelTest.kt @@ -3,7 +3,8 @@ package com.x8bit.bitwarden.ui.platform.feature.settings.exportvault import androidx.lifecycle.SavedStateHandle import app.cash.turbine.test import com.x8bit.bitwarden.R -import com.x8bit.bitwarden.data.platform.repository.SettingsRepository +import com.x8bit.bitwarden.data.auth.repository.AuthRepository +import com.x8bit.bitwarden.data.auth.repository.model.ValidatePasswordResult import com.x8bit.bitwarden.ui.platform.base.BaseViewModelTest import com.x8bit.bitwarden.ui.platform.base.util.asText import com.x8bit.bitwarden.ui.platform.feature.settings.exportvault.model.ExportVaultFormat @@ -14,7 +15,7 @@ import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Test class ExportVaultViewModelTest : BaseViewModelTest() { - private val settingsRepository: SettingsRepository = mockk() + private val authRepository: AuthRepository = mockk() private val savedStateHandle = SavedStateHandle() @@ -42,10 +43,10 @@ class ExportVaultViewModelTest : BaseViewModelTest() { fun `ConfirmExportVaultClicked correct password should emit ShowToast`() = runTest { val password = "password" coEvery { - settingsRepository.validatePassword( + authRepository.validatePassword( password = password, ) - } returns true + } returns ValidatePasswordResult.Success(isValid = true) val viewModel = createViewModel() viewModel.eventFlow.test { @@ -88,10 +89,10 @@ class ExportVaultViewModelTest : BaseViewModelTest() { fun `ConfirmExportVaultClicked invalid password should show an error`() = runTest { val password = "password" coEvery { - settingsRepository.validatePassword( + authRepository.validatePassword( password = password, ) - } returns false + } returns ValidatePasswordResult.Success(isValid = false) val viewModel = createViewModel() viewModel.eventFlow.test { @@ -111,6 +112,33 @@ class ExportVaultViewModelTest : BaseViewModelTest() { } } + @Test + fun `ConfirmExportVaultClicked error checking password should show an error`() = runTest { + val password = "password" + coEvery { + authRepository.validatePassword( + password = password, + ) + } returns ValidatePasswordResult.Error + + val viewModel = createViewModel() + viewModel.eventFlow.test { + viewModel.trySendAction(ExportVaultAction.PasswordInputChanged(password)) + + viewModel.trySendAction(ExportVaultAction.ConfirmExportVaultClicked) + assertEquals( + DEFAULT_STATE.copy( + dialogState = ExportVaultState.DialogState.Error( + title = null, + message = R.string.generic_error_message.asText(), + ), + passwordInput = password, + ), + viewModel.stateFlow.value, + ) + } + } + @Test fun `ExportFormatOptionSelect should update the selected export format in the state`() = runTest { @@ -146,7 +174,7 @@ class ExportVaultViewModelTest : BaseViewModelTest() { private fun createViewModel(): ExportVaultViewModel = ExportVaultViewModel( - settingsRepository = settingsRepository, + authRepository = authRepository, savedStateHandle = savedStateHandle, ) }