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 6690465d32..62baf8e46b 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 @@ -212,12 +212,14 @@ interface AuthRepository : AuthenticatorProvider, AuthRequestManager { /** * Attempt to register a new account with the given parameters. */ + @Suppress("LongParameterList") suspend fun register( email: String, masterPassword: String, masterPasswordHint: String?, captchaToken: String?, shouldCheckDataBreaches: Boolean, + isMasterPasswordStrong: Boolean, ): RegisterResult /** 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 30e2c9f6a6..072fdd7073 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 @@ -590,16 +590,24 @@ class AuthRepositoryImpl( masterPasswordHint: String?, captchaToken: String?, shouldCheckDataBreaches: Boolean, + isMasterPasswordStrong: Boolean, ): RegisterResult { if (shouldCheckDataBreaches) { haveIBeenPwnedService .hasPasswordBeenBreached(password = masterPassword) .onSuccess { foundDataBreaches -> if (foundDataBreaches) { - return RegisterResult.DataBreachFound + return if (isMasterPasswordStrong) { + RegisterResult.DataBreachFound + } else { + RegisterResult.DataBreachAndWeakPassword + } } } } + if (!isMasterPasswordStrong) { + return RegisterResult.WeakPassword + } val kdf = Kdf.Pbkdf2(iterations = DEFAULT_PBKDF2_ITERATIONS.toUInt()) return authSdkSource .makeRegisterKeys( diff --git a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/model/RegisterResult.kt b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/model/RegisterResult.kt index 71a2babbbb..5182e5d289 100644 --- a/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/model/RegisterResult.kt +++ b/app/src/main/java/com/x8bit/bitwarden/data/auth/repository/model/RegisterResult.kt @@ -29,4 +29,14 @@ sealed class RegisterResult { * Password hash was found in a data breach. */ data object DataBreachFound : RegisterResult() + + /** + * Password hash was found to be weak. + */ + data object WeakPassword : RegisterResult() + + /** + * Password hash was found in a data breach and found to be weak. + */ + data object DataBreachAndWeakPassword : RegisterResult() } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountScreen.kt index c73d0bee58..53ab26081f 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountScreen.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountScreen.kt @@ -137,10 +137,10 @@ fun CreateAccountScreen( ) } - CreateAccountDialog.HaveIBeenPwned -> { + is CreateAccountDialog.HaveIBeenPwned -> { BitwardenTwoButtonDialog( - title = stringResource(id = R.string.weak_and_exposed_master_password), - message = haveIBeenPwnedMessage(), + title = dialog.title(), + message = dialog.message(), confirmButtonText = stringResource(id = R.string.yes), dismissButtonText = stringResource(id = R.string.no), onConfirmClick = remember(viewModel) { diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountViewModel.kt index 92eb3c690c..1af6680d1a 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountViewModel.kt @@ -154,12 +154,14 @@ class CreateAccountViewModel @Inject constructor( is CaptchaCallbackTokenResult.Success -> { submitRegisterAccountRequest( shouldCheckForDataBreaches = false, + shouldIgnorePasswordStrength = true, captchaToken = result.token, ) } } } + @Suppress("LongMethod", "MaxLineLength") private fun handleReceiveRegisterAccountResult( action: CreateAccountAction.Internal.ReceiveRegisterResult, ) { @@ -199,7 +201,34 @@ class CreateAccountViewModel @Inject constructor( RegisterResult.DataBreachFound -> { mutableStateFlow.update { - it.copy(dialog = CreateAccountDialog.HaveIBeenPwned) + it.copy( + dialog = CreateAccountDialog.HaveIBeenPwned( + title = R.string.exposed_master_password.asText(), + message = R.string.password_found_in_a_data_breach_alert_description.asText(), + ), + ) + } + } + + RegisterResult.DataBreachAndWeakPassword -> { + mutableStateFlow.update { + it.copy( + dialog = CreateAccountDialog.HaveIBeenPwned( + title = R.string.weak_and_exposed_master_password.asText(), + message = R.string.weak_password_identified_and_found_in_a_data_breach_alert_description.asText(), + ), + ) + } + } + + RegisterResult.WeakPassword -> { + mutableStateFlow.update { + it.copy( + dialog = CreateAccountDialog.HaveIBeenPwned( + title = R.string.weak_master_password.asText(), + message = R.string.weak_password_identified_use_a_strong_password_to_protect_your_account.asText(), + ), + ) } } } @@ -308,17 +337,23 @@ class CreateAccountViewModel @Inject constructor( else -> { submitRegisterAccountRequest( shouldCheckForDataBreaches = state.isCheckDataBreachesToggled, + shouldIgnorePasswordStrength = false, captchaToken = null, ) } } private fun handleContinueWithBreachedPasswordClick() { - submitRegisterAccountRequest(shouldCheckForDataBreaches = false, captchaToken = null) + submitRegisterAccountRequest( + shouldCheckForDataBreaches = false, + shouldIgnorePasswordStrength = true, + captchaToken = null, + ) } private fun submitRegisterAccountRequest( shouldCheckForDataBreaches: Boolean, + shouldIgnorePasswordStrength: Boolean, captchaToken: String?, ) { mutableStateFlow.update { @@ -327,6 +362,8 @@ class CreateAccountViewModel @Inject constructor( viewModelScope.launch { val result = authRepository.register( shouldCheckDataBreaches = shouldCheckForDataBreaches, + isMasterPasswordStrong = shouldIgnorePasswordStrength || + state.isMasterPasswordStrong, email = state.emailInput, masterPassword = state.passwordInput, masterPasswordHint = state.passwordHintInput.ifBlank { null }, @@ -367,6 +404,22 @@ data class CreateAccountState( R.string.your_master_password_cannot_be_recovered_if_you_forget_it_x_characters_minimum .asText(MIN_PASSWORD_LENGTH), ) + + /** + * Whether or not the provided master password is considered strong. + */ + val isMasterPasswordStrong: Boolean + get() = when (passwordStrengthState) { + PasswordStrengthState.NONE, + PasswordStrengthState.WEAK_1, + PasswordStrengthState.WEAK_2, + PasswordStrengthState.WEAK_3, + -> false + + PasswordStrengthState.GOOD, + PasswordStrengthState.STRONG, + -> true + } } /** @@ -381,9 +434,15 @@ sealed class CreateAccountDialog : Parcelable { /** * Confirm the user wants to continue with potentially breached password. + * + * @param title The title for the HaveIBeenPwned dialog. + * @param message The message for the HaveIBeenPwned dialog. */ @Parcelize - data object HaveIBeenPwned : CreateAccountDialog() + data class HaveIBeenPwned( + val title: Text, + val message: Text, + ) : CreateAccountDialog() /** * General error dialog with an OK button. 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 73f0fc1aad..4f1ac5047a 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 @@ -2910,25 +2910,64 @@ class AuthRepositoryTest { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = true, + isMasterPasswordStrong = true, ) assertEquals(RegisterResult.Success(CAPTCHA_KEY), result) } @Test - fun `register check data breaches found should return DataBreachFound`() = runTest { - coEvery { - haveIBeenPwnedService.hasPasswordBeenBreached(PASSWORD) - } returns true.asSuccess() + fun `register check data breaches found and strong password should return DataBreachFound`() = + runTest { + coEvery { + haveIBeenPwnedService.hasPasswordBeenBreached(PASSWORD) + } returns true.asSuccess() - val result = repository.register( - email = EMAIL, - masterPassword = PASSWORD, - masterPasswordHint = null, - captchaToken = null, - shouldCheckDataBreaches = true, - ) - assertEquals(RegisterResult.DataBreachFound, result) - } + val result = repository.register( + email = EMAIL, + masterPassword = PASSWORD, + masterPasswordHint = null, + captchaToken = null, + shouldCheckDataBreaches = true, + isMasterPasswordStrong = true, + ) + assertEquals(RegisterResult.DataBreachFound, result) + } + + @Test + fun `register check data breaches and weak password should return DataBreachAndWeakPassword`() = + runTest { + coEvery { + haveIBeenPwnedService.hasPasswordBeenBreached(PASSWORD) + } returns true.asSuccess() + + val result = repository.register( + email = EMAIL, + masterPassword = PASSWORD, + masterPasswordHint = null, + captchaToken = null, + shouldCheckDataBreaches = true, + isMasterPasswordStrong = false, + ) + assertEquals(RegisterResult.DataBreachAndWeakPassword, result) + } + + @Test + fun `register check no data breaches found with weak password should return WeakPassword`() = + runTest { + coEvery { + haveIBeenPwnedService.hasPasswordBeenBreached(PASSWORD) + } returns false.asSuccess() + + val result = repository.register( + email = EMAIL, + masterPassword = PASSWORD, + masterPasswordHint = null, + captchaToken = null, + shouldCheckDataBreaches = true, + isMasterPasswordStrong = false, + ) + assertEquals(RegisterResult.WeakPassword, result) + } @Test fun `register check data breaches Success should return Success`() = runTest { @@ -2959,6 +2998,7 @@ class AuthRepositoryTest { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = true, + isMasterPasswordStrong = true, ) assertEquals(RegisterResult.Success(CAPTCHA_KEY), result) coVerify { haveIBeenPwnedService.hasPasswordBeenBreached(PASSWORD) } @@ -2991,6 +3031,7 @@ class AuthRepositoryTest { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = false, + isMasterPasswordStrong = true, ) assertEquals(RegisterResult.Success(CAPTCHA_KEY), result) } @@ -3031,6 +3072,7 @@ class AuthRepositoryTest { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = false, + isMasterPasswordStrong = true, ) assertEquals(RegisterResult.Error(errorMessage = null), result) } @@ -3071,6 +3113,7 @@ class AuthRepositoryTest { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = false, + isMasterPasswordStrong = true, ) assertEquals(RegisterResult.CaptchaRequired(captchaId = CAPTCHA_KEY), result) } @@ -3102,6 +3145,7 @@ class AuthRepositoryTest { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = false, + isMasterPasswordStrong = true, ) assertEquals(RegisterResult.Error(errorMessage = null), result) } @@ -3133,6 +3177,7 @@ class AuthRepositoryTest { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = false, + isMasterPasswordStrong = true, ) assertEquals(RegisterResult.Error(errorMessage = "message"), result) } @@ -3169,6 +3214,7 @@ class AuthRepositoryTest { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = false, + isMasterPasswordStrong = true, ) assertEquals(RegisterResult.Error(errorMessage = "expected"), result) } @@ -3200,6 +3246,7 @@ class AuthRepositoryTest { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = false, + isMasterPasswordStrong = true, ) assertEquals(RegisterResult.Error(errorMessage = "message"), result) } diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountScreenTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountScreenTest.kt index 665bf5e3c6..8d9560fc7e 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountScreenTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountScreenTest.kt @@ -198,7 +198,7 @@ class CreateAccountScreenTest : BaseComposeTest() { @Test fun `clicking No on the HIBP dialog should send ErrorDialogDismiss action`() { mutableStateFlow.update { - it.copy(dialog = CreateAccountDialog.HaveIBeenPwned) + it.copy(dialog = createHaveIBeenPwned()) } composeTestRule .onAllNodesWithText("No") @@ -210,7 +210,7 @@ class CreateAccountScreenTest : BaseComposeTest() { @Test fun `clicking Yes on the HIBP dialog should send ContinueWithBreachedPasswordClick action`() { mutableStateFlow.update { - it.copy(dialog = CreateAccountDialog.HaveIBeenPwned) + it.copy(dialog = createHaveIBeenPwned()) } composeTestRule .onAllNodesWithText("Yes") diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountTestUtil.kt b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountTestUtil.kt new file mode 100644 index 0000000000..0e1d8d88b1 --- /dev/null +++ b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountTestUtil.kt @@ -0,0 +1,18 @@ +package com.x8bit.bitwarden.ui.auth.feature.createaccount + +import com.x8bit.bitwarden.R +import com.x8bit.bitwarden.ui.platform.base.util.Text +import com.x8bit.bitwarden.ui.platform.base.util.asText + +/** + * Creates a mock [CreateAccountDialog.HaveIBeenPwned]. + */ +fun createHaveIBeenPwned( + title: Text = R.string.weak_and_exposed_master_password.asText(), + message: Text = R.string.weak_password_identified_and_found_in_a_data_breach_alert_description + .asText(), +): CreateAccountDialog.HaveIBeenPwned = + CreateAccountDialog.HaveIBeenPwned( + title = title, + message = message, + ) diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountViewModelTest.kt index 80e5a4afd8..bf09b1ae8c 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountViewModelTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/auth/feature/createaccount/CreateAccountViewModelTest.kt @@ -30,6 +30,7 @@ import io.mockk.every import io.mockk.mockk import io.mockk.mockkStatic import io.mockk.unmockkStatic +import kotlinx.coroutines.flow.emptyFlow import kotlinx.coroutines.flow.flowOf import kotlinx.coroutines.test.runTest import org.junit.jupiter.api.AfterEach @@ -37,6 +38,7 @@ import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.BeforeEach import org.junit.jupiter.api.Test +@Suppress("LargeClass") class CreateAccountViewModelTest : BaseViewModelTest() { /** @@ -232,6 +234,7 @@ class CreateAccountViewModelTest : BaseViewModelTest() { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = false, + isMasterPasswordStrong = true, ) } returns RegisterResult.Success(captchaToken = "mock_token") } @@ -271,6 +274,7 @@ class CreateAccountViewModelTest : BaseViewModelTest() { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = false, + isMasterPasswordStrong = true, ) } returns RegisterResult.Error(errorMessage = "mock_error") } @@ -314,6 +318,7 @@ class CreateAccountViewModelTest : BaseViewModelTest() { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = false, + isMasterPasswordStrong = true, ) } returns RegisterResult.CaptchaRequired(captchaId = "mock_captcha_id") } @@ -345,6 +350,7 @@ class CreateAccountViewModelTest : BaseViewModelTest() { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = false, + isMasterPasswordStrong = true, ) } returns RegisterResult.Success(captchaToken = "mock_captcha_token") } @@ -375,6 +381,7 @@ class CreateAccountViewModelTest : BaseViewModelTest() { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = false, + isMasterPasswordStrong = true, ) } returns RegisterResult.Error(null) } @@ -390,6 +397,7 @@ class CreateAccountViewModelTest : BaseViewModelTest() { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = false, + isMasterPasswordStrong = true, ) } } @@ -397,7 +405,7 @@ class CreateAccountViewModelTest : BaseViewModelTest() { @Test fun `SubmitClick register returns ShowDataBreaches should show HaveIBeenPwned dialog`() = runTest { - val repo = mockk { + mockAuthRepository.apply { every { captchaTokenResultFlow } returns flowOf() coEvery { register( @@ -406,20 +414,92 @@ class CreateAccountViewModelTest : BaseViewModelTest() { masterPasswordHint = null, captchaToken = null, shouldCheckDataBreaches = true, + isMasterPasswordStrong = true, ) } returns RegisterResult.DataBreachFound } - val viewModel = CreateAccountViewModel( - savedStateHandle = validInputHandle, - authRepository = repo, + val initialState = VALID_INPUT_STATE.copy( + isCheckDataBreachesToggled = true, ) - viewModel.trySendAction(CreateAccountAction.CheckDataBreachesToggle(true)) + val viewModel = createCreateAccountViewModel(createAccountState = initialState) viewModel.trySendAction(CreateAccountAction.SubmitClick) viewModel.stateFlow.test { assertEquals( - VALID_INPUT_STATE.copy( - isCheckDataBreachesToggled = true, - dialog = CreateAccountDialog.HaveIBeenPwned, + initialState.copy( + dialog = createHaveIBeenPwned( + title = R.string.exposed_master_password.asText(), + message = R.string.password_found_in_a_data_breach_alert_description + .asText(), + ), + ), + awaitItem(), + ) + } + } + + @Test + @Suppress("MaxLineLength") + fun `SubmitClick register returns DataBreachAndWeakPassword should show HaveIBeenPwned dialog`() = + runTest { + mockAuthRepository.apply { + every { captchaTokenResultFlow } returns emptyFlow() + coEvery { + register( + email = EMAIL, + masterPassword = PASSWORD, + masterPasswordHint = null, + captchaToken = null, + shouldCheckDataBreaches = true, + isMasterPasswordStrong = false, + ) + } returns RegisterResult.DataBreachAndWeakPassword + } + val initialState = VALID_INPUT_STATE.copy( + passwordStrengthState = PasswordStrengthState.WEAK_1, + isCheckDataBreachesToggled = true, + ) + + val viewModel = createCreateAccountViewModel(createAccountState = initialState) + viewModel.trySendAction(CreateAccountAction.SubmitClick) + viewModel.stateFlow.test { + assertEquals( + initialState.copy(dialog = createHaveIBeenPwned()), + awaitItem(), + ) + } + } + + @Test + @Suppress("MaxLineLength") + fun `SubmitClick register returns WeakPassword should show HaveIBeenPwned dialog`() = + runTest { + mockAuthRepository.apply { + every { captchaTokenResultFlow } returns flowOf() + coEvery { + register( + email = EMAIL, + masterPassword = PASSWORD, + masterPasswordHint = null, + captchaToken = null, + shouldCheckDataBreaches = true, + isMasterPasswordStrong = false, + ) + } returns RegisterResult.WeakPassword + } + val initialState = VALID_INPUT_STATE + .copy( + passwordStrengthState = PasswordStrengthState.WEAK_1, + isCheckDataBreachesToggled = true, + ) + val viewModel = createCreateAccountViewModel(createAccountState = initialState) + viewModel.trySendAction(CreateAccountAction.SubmitClick) + viewModel.stateFlow.test { + assertEquals( + initialState.copy( + dialog = createHaveIBeenPwned( + title = R.string.weak_master_password.asText(), + message = R.string.weak_password_identified_use_a_strong_password_to_protect_your_account.asText(), + ), ), awaitItem(), ) @@ -604,6 +684,15 @@ class CreateAccountViewModelTest : BaseViewModelTest() { } } + private fun createCreateAccountViewModel( + createAccountState: CreateAccountState? = null, + authRepository: AuthRepository = mockAuthRepository, + ): CreateAccountViewModel = + CreateAccountViewModel( + savedStateHandle = SavedStateHandle(mapOf("state" to createAccountState)), + authRepository = authRepository, + ) + companion object { private const val PASSWORD = "longenoughtpassword" private const val EMAIL = "test@test.com"