From e6cb1fc357032927d46bb0d3d340a6d95507baae Mon Sep 17 00:00:00 2001 From: Brian Yencho Date: Mon, 15 Jan 2024 16:50:01 -0600 Subject: [PATCH] Add PIN entry dialog and confirmation UI (#625) --- .../accountsecurity/AccountSecurityScreen.kt | 117 +++++++++- .../AccountSecurityViewModel.kt | 50 ++++- .../accountsecurity/PinInputDialog.kt | 129 +++++++++++ .../AccountSecurityScreenTest.kt | 204 +++++++++++++++++- .../AccountSecurityViewModelTest.kt | 66 +++++- 5 files changed, 544 insertions(+), 22 deletions(-) create mode 100644 app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/PinInputDialog.kt diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreen.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreen.kt index 8845f81aa7..db2052f53a 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreen.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreen.kt @@ -182,17 +182,15 @@ fun AccountSecurityScreen( .fillMaxWidth() .padding(horizontal = 16.dp), ) - BitwardenWideSwitch( - label = stringResource(id = R.string.unlock_with_pin), - isChecked = state.isUnlockWithPinEnabled, - onCheckedChange = remember(viewModel) { - { viewModel.trySendAction(AccountSecurityAction.UnlockWithPinToggle(it)) } + UnlockWithPinRow( + isUnlockWithPinEnabled = state.isUnlockWithPinEnabled, + onUnlockWithPinToggleAction = remember(viewModel) { + { viewModel.trySendAction(it) } }, modifier = Modifier .fillMaxWidth() .padding(horizontal = 16.dp), ) - Spacer(Modifier.height(16.dp)) BitwardenListHeaderText( label = stringResource(id = R.string.session_timeout), @@ -289,6 +287,113 @@ fun AccountSecurityScreen( } } +@Suppress("LongMethod") +@Composable +private fun UnlockWithPinRow( + isUnlockWithPinEnabled: Boolean, + onUnlockWithPinToggleAction: (AccountSecurityAction.UnlockWithPinToggle) -> Unit, + modifier: Modifier = Modifier, +) { + var shouldShowPinInputDialog by rememberSaveable { mutableStateOf(false) } + var shouldShowPinConfirmationDialog by rememberSaveable { mutableStateOf(false) } + var pin by rememberSaveable { mutableStateOf("") } + BitwardenWideSwitch( + label = stringResource(id = R.string.unlock_with_pin), + isChecked = isUnlockWithPinEnabled, + onCheckedChange = { isChecked -> + if (isChecked) { + onUnlockWithPinToggleAction( + AccountSecurityAction.UnlockWithPinToggle.PendingEnabled, + ) + shouldShowPinInputDialog = true + } else { + onUnlockWithPinToggleAction( + AccountSecurityAction.UnlockWithPinToggle.Disabled, + ) + } + }, + modifier = modifier, + ) + + when { + shouldShowPinInputDialog -> { + PinInputDialog( + pin = pin, + onPinChange = { pin = it }, + onCancelClick = { + shouldShowPinInputDialog = false + onUnlockWithPinToggleAction( + AccountSecurityAction.UnlockWithPinToggle.Disabled, + ) + pin = "" + }, + onSubmitClick = { + if (pin.isNotEmpty()) { + shouldShowPinInputDialog = false + shouldShowPinConfirmationDialog = true + onUnlockWithPinToggleAction( + AccountSecurityAction.UnlockWithPinToggle.PendingEnabled, + ) + } else { + shouldShowPinInputDialog = false + onUnlockWithPinToggleAction( + AccountSecurityAction.UnlockWithPinToggle.Disabled, + ) + } + }, + onDismissRequest = { + shouldShowPinInputDialog = false + onUnlockWithPinToggleAction( + AccountSecurityAction.UnlockWithPinToggle.Disabled, + ) + pin = "" + }, + ) + } + + shouldShowPinConfirmationDialog -> { + BitwardenTwoButtonDialog( + title = stringResource(id = R.string.unlock_with_pin), + message = stringResource(id = R.string.pin_require_master_password_restart), + confirmButtonText = stringResource(id = R.string.yes), + dismissButtonText = stringResource(id = R.string.no), + onConfirmClick = { + shouldShowPinConfirmationDialog = false + onUnlockWithPinToggleAction( + AccountSecurityAction.UnlockWithPinToggle.Enabled( + pin = pin, + shouldRequireMasterPasswordOnRestart = true, + ), + ) + pin = "" + }, + onDismissClick = { + shouldShowPinConfirmationDialog = false + onUnlockWithPinToggleAction( + AccountSecurityAction.UnlockWithPinToggle.Enabled( + pin = pin, + shouldRequireMasterPasswordOnRestart = false, + ), + ) + pin = "" + }, + onDismissRequest = { + // Dismissing the dialog is the same as requiring a master password + // confirmation. + shouldShowPinConfirmationDialog = false + onUnlockWithPinToggleAction( + AccountSecurityAction.UnlockWithPinToggle.Enabled( + pin = pin, + shouldRequireMasterPasswordOnRestart = true, + ), + ) + pin = "" + }, + ) + } + } +} + @Suppress("LongMethod") @Composable private fun SessionTimeoutRow( diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModel.kt index 22d7bc918f..6e632322e1 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModel.kt @@ -187,9 +187,19 @@ class AccountSecurityViewModel @Inject constructor( } private fun handleUnlockWithPinToggle(action: AccountSecurityAction.UnlockWithPinToggle) { - // TODO BIT-974: Display alert - mutableStateFlow.update { it.copy(isUnlockWithPinEnabled = action.enabled) } - sendEvent(AccountSecurityEvent.ShowToast("Handle unlock with pin.".asText())) + mutableStateFlow.update { + it.copy(isUnlockWithPinEnabled = action.isUnlockWithPinEnabled) + } + + // TODO: Complete implementation (BIT-465) + when (action) { + AccountSecurityAction.UnlockWithPinToggle.PendingEnabled -> Unit + AccountSecurityAction.UnlockWithPinToggle.Disabled, + is AccountSecurityAction.UnlockWithPinToggle.Enabled, + -> { + sendEvent(AccountSecurityEvent.ShowToast("Handle unlock with pin.".asText())) + } + } } } @@ -357,7 +367,35 @@ sealed class AccountSecurityAction { /** * User toggled the unlock with pin switch. */ - data class UnlockWithPinToggle( - val enabled: Boolean, - ) : AccountSecurityAction() + sealed class UnlockWithPinToggle : AccountSecurityAction() { + /** + * Whether or not the action represents PIN unlocking being enabled. + */ + abstract val isUnlockWithPinEnabled: Boolean + + /** + * The toggle was disabled. + */ + data object Disabled : UnlockWithPinToggle() { + override val isUnlockWithPinEnabled: Boolean get() = false + } + + /** + * The toggle was enabled but the behavior is pending confirmation. + */ + data object PendingEnabled : UnlockWithPinToggle() { + override val isUnlockWithPinEnabled: Boolean get() = true + } + + /** + * The toggle was enabled and the user's [pin] and [shouldRequireMasterPasswordOnRestart] + * preference were confirmed. + */ + data class Enabled( + val pin: String, + val shouldRequireMasterPasswordOnRestart: Boolean, + ) : UnlockWithPinToggle() { + override val isUnlockWithPinEnabled: Boolean get() = true + } + } } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/PinInputDialog.kt b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/PinInputDialog.kt new file mode 100644 index 0000000000..1309a48476 --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/PinInputDialog.kt @@ -0,0 +1,129 @@ +package com.x8bit.bitwarden.ui.platform.feature.settings.accountsecurity + +import androidx.compose.foundation.background +import androidx.compose.foundation.layout.Box +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.height +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.requiredHeightIn +import androidx.compose.foundation.rememberScrollState +import androidx.compose.foundation.shape.RoundedCornerShape +import androidx.compose.foundation.verticalScroll +import androidx.compose.material3.MaterialTheme +import androidx.compose.material3.Text +import androidx.compose.runtime.Composable +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.platform.LocalConfiguration +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.text.input.KeyboardType +import androidx.compose.ui.unit.dp +import androidx.compose.ui.window.Dialog +import com.x8bit.bitwarden.R +import com.x8bit.bitwarden.ui.platform.components.BitwardenFilledButton +import com.x8bit.bitwarden.ui.platform.components.BitwardenTextButton +import com.x8bit.bitwarden.ui.platform.components.BitwardenTextField +import com.x8bit.bitwarden.ui.platform.components.util.maxDialogHeight + +/** + * A dialog for setting a user's PIN. + * + * @param pin The current value of the PIN. + * @param onPinChange A callback for internal changes to the PIN. + * @param onCancelClick A callback for when the "Cancel" button is clicked. + * @param onSubmitClick A callback for when the "Submit" button is clicked. + * @param onDismissRequest A callback for when the dialog is requesting to be dismissed. + */ +@Suppress("LongMethod") +@Composable +fun PinInputDialog( + pin: String, + onPinChange: (String) -> Unit, + onCancelClick: () -> Unit, + onSubmitClick: () -> Unit, + onDismissRequest: () -> Unit, +) { + Dialog( + onDismissRequest = onDismissRequest, + ) { + val configuration = LocalConfiguration.current + val scrollState = rememberScrollState() + Column( + modifier = Modifier + .requiredHeightIn( + max = configuration.maxDialogHeight, + ) + // This background is necessary for the dialog to not be transparent. + .background( + color = MaterialTheme.colorScheme.surfaceContainerHigh, + shape = RoundedCornerShape(28.dp), + ), + horizontalAlignment = Alignment.End, + ) { + Text( + modifier = Modifier + .padding(24.dp) + .fillMaxWidth(), + text = stringResource(id = R.string.enter_pin), + color = MaterialTheme.colorScheme.onSurface, + style = MaterialTheme.typography.headlineSmall, + ) + if (scrollState.canScrollBackward) { + Box( + modifier = Modifier + .fillMaxWidth() + .height(1.dp) + .background(MaterialTheme.colorScheme.outlineVariant), + ) + } + Column( + modifier = Modifier + .weight(1f, fill = false) + .verticalScroll(scrollState), + ) { + Text( + modifier = Modifier + .padding(24.dp) + .fillMaxWidth(), + text = stringResource(id = R.string.set_pin_description), + color = MaterialTheme.colorScheme.onSurfaceVariant, + style = MaterialTheme.typography.bodyMedium, + ) + + BitwardenTextField( + label = stringResource(id = R.string.pin), + value = pin, + onValueChange = onPinChange, + keyboardType = KeyboardType.Number, + modifier = Modifier + .padding(16.dp) + .fillMaxWidth(), + ) + } + if (scrollState.canScrollForward) { + Box( + modifier = Modifier + .fillMaxWidth() + .height(1.dp) + .background(MaterialTheme.colorScheme.outlineVariant), + ) + } + Row( + verticalAlignment = Alignment.CenterVertically, + modifier = Modifier.padding(24.dp), + ) { + BitwardenTextButton( + label = stringResource(id = R.string.cancel), + onClick = onCancelClick, + ) + + BitwardenFilledButton( + label = stringResource(id = R.string.submit), + onClick = onSubmitClick, + ) + } + } + } +} diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreenTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreenTest.kt index 71116a14c5..f7230cd659 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreenTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityScreenTest.kt @@ -15,6 +15,7 @@ import androidx.compose.ui.test.onNodeWithContentDescription import androidx.compose.ui.test.onNodeWithText import androidx.compose.ui.test.performClick import androidx.compose.ui.test.performScrollTo +import androidx.compose.ui.test.performTextInput import androidx.core.net.toUri import com.x8bit.bitwarden.data.platform.repository.model.VaultTimeout import com.x8bit.bitwarden.data.platform.repository.model.VaultTimeoutAction @@ -114,12 +115,211 @@ class AccountSecurityScreenTest : BaseComposeTest() { } @Test - fun `on unlock with pin toggle should send UnlockWithPinToggle`() { + fun `on unlock with pin toggle when enabled should send UnlockWithPinToggle Disabled`() { + mutableStateFlow.update { + it.copy(isUnlockWithPinEnabled = true) + } + composeTestRule .onNodeWithText("Unlock with PIN code") .performScrollTo() .performClick() - verify { viewModel.trySendAction(AccountSecurityAction.UnlockWithPinToggle(true)) } + + verify { viewModel.trySendAction(AccountSecurityAction.UnlockWithPinToggle.Disabled) } + } + + @Suppress("MaxLineLength") + @Test + fun `on unlock with pin toggle when disabled should show the PIN input dialog and send UnlockWithPinToggle PendingEnabled`() { + mutableStateFlow.update { + it.copy(isUnlockWithPinEnabled = false) + } + + composeTestRule.assertNoDialogExists() + + composeTestRule + .onNodeWithText("Unlock with PIN code") + .performScrollTo() + .performClick() + + composeTestRule + .onAllNodesWithText("Enter your PIN code.") + .filterToOne(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + composeTestRule + .onAllNodesWithText( + "Set your PIN code for unlocking Bitwarden. Your PIN settings will be reset if " + + "you ever fully log out of the application.", + ) + .filterToOne(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + composeTestRule + .onAllNodesWithText("PIN") + .filterToOne(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + composeTestRule + .onAllNodesWithText("Cancel") + .filterToOne(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + composeTestRule + .onAllNodesWithText("Submit") + .filterToOne(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + + verify { viewModel.trySendAction(AccountSecurityAction.UnlockWithPinToggle.PendingEnabled) } + } + + @Suppress("MaxLineLength") + @Test + fun `PIN input dialog Cancel click should clear the dialog and send UnlockWithPinToggle Disabled`() { + mutableStateFlow.update { + it.copy(isUnlockWithPinEnabled = false) + } + composeTestRule + .onNodeWithText("Unlock with PIN code") + .performScrollTo() + .performClick() + + composeTestRule + .onAllNodesWithText("Cancel") + .filterToOne(hasAnyAncestor(isDialog())) + .performClick() + + verify { viewModel.trySendAction(AccountSecurityAction.UnlockWithPinToggle.Disabled) } + composeTestRule.assertNoDialogExists() + } + + @Suppress("MaxLineLength") + @Test + fun `PIN input dialog Submit click with empty pin should clear the dialog and send UnlockWithPinToggle Disabled`() { + mutableStateFlow.update { + it.copy(isUnlockWithPinEnabled = false) + } + composeTestRule + .onNodeWithText("Unlock with PIN code") + .performScrollTo() + .performClick() + + composeTestRule + .onAllNodesWithText("Submit") + .filterToOne(hasAnyAncestor(isDialog())) + .performClick() + + verify { viewModel.trySendAction(AccountSecurityAction.UnlockWithPinToggle.Disabled) } + composeTestRule.assertNoDialogExists() + } + + @Suppress("MaxLineLength") + @Test + fun `PIN input dialog Submit click with non-empty pin should show a confirmation dialog and send UnlockWithPinToggle PendingEnabled`() { + mutableStateFlow.update { + it.copy(isUnlockWithPinEnabled = false) + } + composeTestRule + .onNodeWithText("Unlock with PIN code") + .performScrollTo() + .performClick() + + composeTestRule + .onAllNodesWithText("PIN") + .filterToOne(hasAnyAncestor(isDialog())) + .performTextInput("1234") + composeTestRule + .onAllNodesWithText("Submit") + .filterToOne(hasAnyAncestor(isDialog())) + .performClick() + + composeTestRule + .onAllNodesWithText("Unlock with PIN code") + .filterToOne(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + composeTestRule + .onAllNodesWithText( + "Do you want to require unlocking with your master password when the application " + + "is restarted?", + ) + .filterToOne(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + composeTestRule + .onAllNodesWithText("Yes") + .filterToOne(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + composeTestRule + .onAllNodesWithText("No") + .filterToOne(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + + verify { viewModel.trySendAction(AccountSecurityAction.UnlockWithPinToggle.PendingEnabled) } + } + + @Suppress("MaxLineLength") + @Test + fun `PIN confirmation dialog No click should send UnlockWithPinToggle Enabled and close the dialog`() { + mutableStateFlow.update { + it.copy(isUnlockWithPinEnabled = false) + } + composeTestRule + .onNodeWithText("Unlock with PIN code") + .performScrollTo() + .performClick() + composeTestRule + .onAllNodesWithText("PIN") + .filterToOne(hasAnyAncestor(isDialog())) + .performTextInput("1234") + composeTestRule + .onAllNodesWithText("Submit") + .filterToOne(hasAnyAncestor(isDialog())) + .performClick() + + composeTestRule + .onAllNodesWithText("No") + .filterToOne(hasAnyAncestor(isDialog())) + .performClick() + + verify { + viewModel.trySendAction( + AccountSecurityAction.UnlockWithPinToggle.Enabled( + pin = "1234", + shouldRequireMasterPasswordOnRestart = false, + ), + ) + } + composeTestRule.assertNoDialogExists() + } + + @Suppress("MaxLineLength") + @Test + fun `PIN confirmation dialog Yes click should send UnlockWithPinToggle Enabled and close the dialog`() { + mutableStateFlow.update { + it.copy(isUnlockWithPinEnabled = false) + } + composeTestRule + .onNodeWithText("Unlock with PIN code") + .performScrollTo() + .performClick() + composeTestRule + .onAllNodesWithText("PIN") + .filterToOne(hasAnyAncestor(isDialog())) + .performTextInput("1234") + composeTestRule + .onAllNodesWithText("Submit") + .filterToOne(hasAnyAncestor(isDialog())) + .performClick() + + composeTestRule + .onAllNodesWithText("Yes") + .filterToOne(hasAnyAncestor(isDialog())) + .performClick() + + verify { + viewModel.trySendAction( + AccountSecurityAction.UnlockWithPinToggle.Enabled( + pin = "1234", + shouldRequireMasterPasswordOnRestart = true, + ), + ) + } + composeTestRule.assertNoDialogExists() } @Test diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModelTest.kt index bd771c521c..3115b51e40 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModelTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/platform/feature/settings/accountsecurity/AccountSecurityViewModelTest.kt @@ -208,22 +208,71 @@ class AccountSecurityViewModelTest : BaseViewModelTest() { ) } + @Suppress("MaxLineLength") @Test - fun `on UnlockWithPinToggle should emit ShowToast`() = runTest { - val viewModel = createViewModel() - viewModel.eventFlow.test { - viewModel.trySendAction(AccountSecurityAction.UnlockWithPinToggle(true)) + fun `on UnlockWithPinToggle Disabled should set pin unlock to false and emit ShowToast`() = + runTest { + val initialState = DEFAULT_STATE.copy( + isUnlockWithPinEnabled = true, + ) + val viewModel = createViewModel(initialState = initialState) + viewModel.eventFlow.test { + viewModel.trySendAction( + AccountSecurityAction.UnlockWithPinToggle.Disabled, + ) + assertEquals( + AccountSecurityEvent.ShowToast("Handle unlock with pin.".asText()), + awaitItem(), + ) + } assertEquals( - AccountSecurityEvent.ShowToast("Handle unlock with pin.".asText()), - awaitItem(), + initialState.copy(isUnlockWithPinEnabled = false), + viewModel.stateFlow.value, ) } + + @Suppress("MaxLineLength") + @Test + fun `on UnlockWithPinToggle Enabled should set pin unlock to true`() { + val initialState = DEFAULT_STATE.copy( + isUnlockWithPinEnabled = false, + ) + val viewModel = createViewModel(initialState = initialState) + viewModel.trySendAction( + AccountSecurityAction.UnlockWithPinToggle.PendingEnabled, + ) assertEquals( - DEFAULT_STATE.copy(isUnlockWithPinEnabled = true), + initialState.copy(isUnlockWithPinEnabled = true), viewModel.stateFlow.value, ) } + @Suppress("MaxLineLength") + @Test + fun `on UnlockWithPinToggle Enabled should set pin unlock to true and emit ShowToast`() = + runTest { + val initialState = DEFAULT_STATE.copy( + isUnlockWithPinEnabled = false, + ) + val viewModel = createViewModel(initialState = initialState) + viewModel.eventFlow.test { + viewModel.trySendAction( + AccountSecurityAction.UnlockWithPinToggle.Enabled( + pin = "1234", + shouldRequireMasterPasswordOnRestart = true, + ), + ) + assertEquals( + AccountSecurityEvent.ShowToast("Handle unlock with pin.".asText()), + awaitItem(), + ) + } + assertEquals( + initialState.copy(isUnlockWithPinEnabled = true), + viewModel.stateFlow.value, + ) + } + @Test fun `on LogoutClick should show confirm log out dialog`() = runTest { val viewModel = createViewModel() @@ -253,11 +302,12 @@ class AccountSecurityViewModelTest : BaseViewModelTest() { } private fun createViewModel( + initialState: AccountSecurityState = DEFAULT_STATE, authRepository: AuthRepository = mockk(relaxed = true), vaultRepository: VaultRepository = mockk(relaxed = true), settingsRepository: SettingsRepository = mockk(relaxed = true), savedStateHandle: SavedStateHandle = SavedStateHandle().apply { - set("state", DEFAULT_STATE) + set("state", initialState) }, ): AccountSecurityViewModel = AccountSecurityViewModel( authRepository = authRepository,