BIT-2145: Create account dialog error (#1243)

2025-03-18 12:18:41 +03:00 · 2024-04-10 09:12:50 -06:00 · 2024-04-10 09:12:50 -06:00 · f01a67ea9c
commit f01a67ea9c
parent 0a63d85457
9 changed files with 263 additions and 30 deletions
--- 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

    /**
--- 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(
--- 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()
 }
--- 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) {
--- 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.
--- 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)
    }
--- 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")
--- 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
@ -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,
+    )
--- 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<AuthRepository> {
+            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"