From 80d021b691118ea46a92f1c5cd65be70472b3aca Mon Sep 17 00:00:00 2001 From: Brian Yencho Date: Thu, 1 Feb 2024 09:29:27 -0600 Subject: [PATCH] BIT-1682: Add more master password reprompts to Item Listing screen (#938) --- .../itemlisting/VaultItemListingContent.kt | 38 ++++-- .../itemlisting/VaultItemListingViewModel.kt | 56 ++++++++- .../handlers/VaultItemListingHandlers.kt | 7 +- .../util/VaultItemListingDataExtensions.kt | 4 +- .../itemlisting/VaultItemListingScreenTest.kt | 108 +++++++++++++++++- .../VaultItemListingViewModelTest.kt | 43 ++++++- .../VaultItemListingDataExtensionsTest.kt | 11 +- .../util/VaultItemListingDataUtil.kt | 6 + 8 files changed, 237 insertions(+), 36 deletions(-) diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingContent.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingContent.kt index 7804fc1b8f..e9e922fcd3 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingContent.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingContent.kt @@ -36,7 +36,7 @@ fun VaultItemListingContent( state: VaultItemListingState.ViewState.Content, policyDisablesSend: Boolean, vaultItemClick: (id: String) -> Unit, - masterPasswordRepromptSubmit: (id: String, password: String) -> Unit, + masterPasswordRepromptSubmit: (password: String, data: MasterPasswordRepromptData) -> Unit, onOverflowItemClick: (action: ListingItemOverflowAction) -> Unit, modifier: Modifier = Modifier, ) { @@ -76,20 +76,18 @@ fun VaultItemListingContent( -> Unit } - var masterPasswordRepromptCipherId by remember { mutableStateOf(null) } - if (masterPasswordRepromptCipherId != null) { + var masterPasswordRepromptData by remember { mutableStateOf(null) } + masterPasswordRepromptData?.let { data -> BitwardenMasterPasswordDialog( - onConfirmClick = { - val cipherId = masterPasswordRepromptCipherId - ?: return@BitwardenMasterPasswordDialog - masterPasswordRepromptCipherId = null + onConfirmClick = { password -> + masterPasswordRepromptData = null masterPasswordRepromptSubmit( - cipherId, - it, + password, + data, ) }, onDismissRequest = { - masterPasswordRepromptCipherId = null + masterPasswordRepromptData = null }, ) } @@ -123,8 +121,11 @@ fun VaultItemListingContent( label = it.title, supportingLabel = it.subtitle, onClick = { - if (it.shouldShowMasterPasswordReprompt) { - masterPasswordRepromptCipherId = it.id + if (it.isAutofill && it.shouldShowMasterPasswordReprompt) { + masterPasswordRepromptData = + MasterPasswordRepromptData.Autofill( + cipherId = it.id, + ) } else { vaultItemClick(it.id) } @@ -144,6 +145,19 @@ fun VaultItemListingContent( showConfirmationDialog = option } + is ListingItemOverflowAction.VaultAction -> { + if (option.requiresPasswordReprompt && + it.shouldShowMasterPasswordReprompt + ) { + masterPasswordRepromptData = + MasterPasswordRepromptData.OverflowItem( + action = option, + ) + } else { + onOverflowItemClick(option) + } + } + else -> onOverflowItemClick(option) } }, diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingViewModel.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingViewModel.kt index 08bdbd2997..6eecf35316 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingViewModel.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingViewModel.kt @@ -260,7 +260,7 @@ class VaultItemListingViewModel @Inject constructor( val result = authRepository.validatePassword(action.password) sendAction( VaultItemListingsAction.Internal.ValidatePasswordResultReceive( - cipherId = action.cipherId, + masterPasswordRepromptData = action.masterPasswordRepromptData, result = result, ), ) @@ -573,10 +573,28 @@ class VaultItemListingViewModel @Inject constructor( } return } + handleMasterPasswordRepromptData( + data = action.masterPasswordRepromptData, + ) + } + } + } + + private fun handleMasterPasswordRepromptData( + data: MasterPasswordRepromptData, + ) { + when (data) { + is MasterPasswordRepromptData.Autofill -> { // Complete the autofill selection flow - val cipherView = getCipherViewOrNull(cipherId = action.cipherId) ?: return + val cipherView = getCipherViewOrNull(cipherId = data.cipherId) ?: return autofillSelectionManager.emitAutofillSelection(cipherView = cipherView) } + + is MasterPasswordRepromptData.OverflowItem -> { + handleOverflowOptionClick( + VaultItemListingsAction.OverflowOptionClick(data.action), + ) + } } } //endregion VaultItemListing Handlers @@ -848,6 +866,9 @@ data class VaultItemListingState( * @property subtitle subtitle of the item (nullable). * @property iconData data for the icon to be displayed (nullable). * @property overflowOptions list of options for the item's overflow menu. + * @property isAutofill whether or not this screen is part of an autofill flow. + * @property shouldShowMasterPasswordReprompt whether or not a master password reprompt is + * required for various secure actions. */ data class DisplayItem( val id: String, @@ -856,6 +877,7 @@ data class VaultItemListingState( val iconData: IconData, val extraIconList: List, val overflowOptions: List, + val isAutofill: Boolean, val shouldShowMasterPasswordReprompt: Boolean, ) @@ -1122,12 +1144,12 @@ sealed class VaultItemListingsAction { data class ItemClick(val id: String) : VaultItemListingsAction() /** - * A master password prompt was encountered when trying to access the cipher with the given - * [cipherId] and the given [password] was submitted. + * A master password prompt was encountered when trying to perform a senstive action described + * by the given [masterPasswordRepromptData] and the given [password] was submitted. */ data class MasterPasswordRepromptSubmit( - val cipherId: String, val password: String, + val masterPasswordRepromptData: MasterPasswordRepromptData, ) : VaultItemListingsAction() /** @@ -1181,7 +1203,7 @@ sealed class VaultItemListingsAction { * Indicates that a result for verifying the user's master password has been received. */ data class ValidatePasswordResultReceive( - val cipherId: String, + val masterPasswordRepromptData: MasterPasswordRepromptData, val result: ValidatePasswordResult, ) : Internal() @@ -1193,3 +1215,25 @@ sealed class VaultItemListingsAction { ) : Internal() } } + +/** + * Data tracking the type of request that triggered a master password reprompt. + */ +sealed class MasterPasswordRepromptData : Parcelable { + + /** + * Autofill was selected. + */ + @Parcelize + data class Autofill( + val cipherId: String, + ) : MasterPasswordRepromptData() + + /** + * A cipher overflow menu item action was selected. + */ + @Parcelize + data class OverflowItem( + val action: ListingItemOverflowAction.VaultAction, + ) : MasterPasswordRepromptData() +} diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/handlers/VaultItemListingHandlers.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/handlers/VaultItemListingHandlers.kt index a1ee863a3f..df0ecbf807 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/handlers/VaultItemListingHandlers.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/handlers/VaultItemListingHandlers.kt @@ -1,6 +1,7 @@ package com.x8bit.bitwarden.ui.vault.feature.itemlisting.handlers import com.x8bit.bitwarden.ui.platform.components.model.AccountSummary +import com.x8bit.bitwarden.ui.vault.feature.itemlisting.MasterPasswordRepromptData import com.x8bit.bitwarden.ui.vault.feature.itemlisting.VaultItemListingViewModel import com.x8bit.bitwarden.ui.vault.feature.itemlisting.VaultItemListingsAction import com.x8bit.bitwarden.ui.vault.feature.itemlisting.model.ListingItemOverflowAction @@ -17,7 +18,7 @@ data class VaultItemListingHandlers( val searchIconClick: () -> Unit, val addVaultItemClick: () -> Unit, val itemClick: (id: String) -> Unit, - val masterPasswordRepromptSubmit: (id: String, password: String) -> Unit, + val masterPasswordRepromptSubmit: (password: String, MasterPasswordRepromptData) -> Unit, val refreshClick: () -> Unit, val syncClick: () -> Unit, val lockClick: () -> Unit, @@ -49,11 +50,11 @@ data class VaultItemListingHandlers( viewModel.trySendAction(VaultItemListingsAction.AddVaultItemClick) }, itemClick = { viewModel.trySendAction(VaultItemListingsAction.ItemClick(it)) }, - masterPasswordRepromptSubmit = { cipherId, password -> + masterPasswordRepromptSubmit = { password, data -> viewModel.trySendAction( VaultItemListingsAction.MasterPasswordRepromptSubmit( - cipherId = cipherId, password = password, + masterPasswordRepromptData = data, ), ) }, diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensions.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensions.kt index 6e86781f53..1f8bb004f4 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensions.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensions.kt @@ -218,7 +218,8 @@ private fun CipherView.toDisplayItem( ), extraIconList = toLabelIcons(), overflowOptions = toOverflowActions(), - shouldShowMasterPasswordReprompt = isAutofill && reprompt == CipherRepromptType.PASSWORD, + isAutofill = isAutofill, + shouldShowMasterPasswordReprompt = reprompt == CipherRepromptType.PASSWORD, ) private fun CipherView.toIconData( @@ -255,6 +256,7 @@ private fun SendView.toDisplayItem( ), extraIconList = toLabelIcons(clock = clock), overflowOptions = toOverflowActions(baseWebSendUrl = baseWebSendUrl), + isAutofill = false, shouldShowMasterPasswordReprompt = false, ) diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingScreenTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingScreenTest.kt index 6e9c79ae3a..e442239b81 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingScreenTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingScreenTest.kt @@ -35,6 +35,7 @@ import com.x8bit.bitwarden.ui.platform.feature.search.model.SearchType import com.x8bit.bitwarden.ui.platform.manager.intent.IntentManager import com.x8bit.bitwarden.ui.util.assertLockOrLogoutDialogIsDisplayed import com.x8bit.bitwarden.ui.util.assertLogoutConfirmationDialogIsDisplayed +import com.x8bit.bitwarden.ui.util.assertMasterPasswordDialogDisplayed import com.x8bit.bitwarden.ui.util.assertNoDialogExists import com.x8bit.bitwarden.ui.util.assertSwitcherIsDisplayed import com.x8bit.bitwarden.ui.util.assertSwitcherIsNotDisplayed @@ -639,7 +640,10 @@ class VaultItemListingScreenTest : BaseComposeTest() { viewState = VaultItemListingState.ViewState.Content( displayItemList = listOf( createDisplayItem(number = 1) - .copy(shouldShowMasterPasswordReprompt = false), + .copy( + isAutofill = false, + shouldShowMasterPasswordReprompt = false, + ), ), ), ) @@ -656,13 +660,16 @@ class VaultItemListingScreenTest : BaseComposeTest() { @Suppress("MaxLineLength") @Test - fun `clicking on a display item when master password reprompt is required should show the master password dialog`() { + fun `clicking on a display item when master password reprompt is required for autofill should show the master password dialog`() { mutableStateFlow.update { it.copy( viewState = VaultItemListingState.ViewState.Content( displayItemList = listOf( createDisplayItem(number = 1) - .copy(shouldShowMasterPasswordReprompt = true), + .copy( + isAutofill = true, + shouldShowMasterPasswordReprompt = true, + ), ), ), ) @@ -709,7 +716,10 @@ class VaultItemListingScreenTest : BaseComposeTest() { viewState = VaultItemListingState.ViewState.Content( displayItemList = listOf( createDisplayItem(number = 1) - .copy(shouldShowMasterPasswordReprompt = true), + .copy( + isAutofill = true, + shouldShowMasterPasswordReprompt = true, + ), ), ), ) @@ -735,7 +745,10 @@ class VaultItemListingScreenTest : BaseComposeTest() { viewState = VaultItemListingState.ViewState.Content( displayItemList = listOf( createDisplayItem(number = 1) - .copy(shouldShowMasterPasswordReprompt = true), + .copy( + isAutofill = true, + shouldShowMasterPasswordReprompt = true, + ), ), ), ) @@ -757,8 +770,10 @@ class VaultItemListingScreenTest : BaseComposeTest() { verify { viewModel.trySendAction( VaultItemListingsAction.MasterPasswordRepromptSubmit( - cipherId = "mockId-1", password = "password", + masterPasswordRepromptData = MasterPasswordRepromptData.Autofill( + cipherId = "mockId-1", + ), ), ) } @@ -861,6 +876,72 @@ class VaultItemListingScreenTest : BaseComposeTest() { } } + @Test + fun `on cipher item overflow option click should emit the appropriate action`() { + mutableStateFlow.update { + it.copy( + itemListingType = VaultItemListingState.ItemListingType.Vault.Login, + viewState = VaultItemListingState.ViewState.Content( + displayItemList = listOf(createCipherDisplayItem(number = 1)), + ), + ) + } + + composeTestRule.assertNoDialogExists() + + composeTestRule + .onNodeWithContentDescription("Options") + .assertIsDisplayed() + .performClick() + composeTestRule + .onNodeWithText("Edit") + .assert(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + .performClick() + verify(exactly = 1) { + viewModel.trySendAction( + VaultItemListingsAction.OverflowOptionClick( + action = ListingItemOverflowAction.VaultAction.EditClick( + cipherId = "mockId-1", + ), + ), + ) + } + + composeTestRule.assertNoDialogExists() + } + + @Suppress("MaxLineLength") + @Test + fun `on cipher item overflow option click when reprompt is required should show the master password dialog`() { + mutableStateFlow.update { + it.copy( + itemListingType = VaultItemListingState.ItemListingType.Vault.Login, + viewState = VaultItemListingState.ViewState.Content( + displayItemList = listOf( + createCipherDisplayItem(number = 1) + .copy(shouldShowMasterPasswordReprompt = true), + ), + ), + ) + } + + composeTestRule.assertNoDialogExists() + + composeTestRule + .onNodeWithContentDescription("Options") + .assertIsDisplayed() + .performClick() + composeTestRule + .onNodeWithText("Edit") + .assert(hasAnyAncestor(isDialog())) + .assertIsDisplayed() + .performClick() + verify(exactly = 0) { viewModel.trySendAction(any()) } + + composeTestRule.assertMasterPasswordDialogDisplayed() + } + @Test fun `send item overflow item button should update according to state`() { mutableStateFlow.update { @@ -1166,5 +1247,20 @@ private fun createDisplayItem(number: Int): VaultItemListingState.DisplayItem = ListingItemOverflowAction.SendAction.RemovePasswordClick(sendId = "mockId-$number"), ListingItemOverflowAction.SendAction.DeleteClick(sendId = "mockId-$number"), ), + isAutofill = false, + shouldShowMasterPasswordReprompt = false, + ) + +private fun createCipherDisplayItem(number: Int): VaultItemListingState.DisplayItem = + VaultItemListingState.DisplayItem( + id = "mockId-$number", + title = "mockTitle-$number", + subtitle = "mockSubtitle-$number", + iconData = IconData.Local(R.drawable.ic_vault), + extraIconList = emptyList(), + overflowOptions = listOf( + ListingItemOverflowAction.VaultAction.EditClick(cipherId = "mockId-$number"), + ), + isAutofill = false, shouldShowMasterPasswordReprompt = false, ) diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingViewModelTest.kt index e7e1f4cbb4..4a59ace32d 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingViewModelTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/VaultItemListingViewModelTest.kt @@ -317,8 +317,10 @@ class VaultItemListingViewModelTest : BaseViewModelTest() { viewModel.trySendAction( VaultItemListingsAction.MasterPasswordRepromptSubmit( - cipherId = cipherId, password = password, + masterPasswordRepromptData = MasterPasswordRepromptData.Autofill( + cipherId = cipherId, + ), ), ) @@ -368,8 +370,10 @@ class VaultItemListingViewModelTest : BaseViewModelTest() { viewModel.trySendAction( VaultItemListingsAction.MasterPasswordRepromptSubmit( - cipherId = cipherId, password = password, + masterPasswordRepromptData = MasterPasswordRepromptData.Autofill( + cipherId = cipherId, + ), ), ) @@ -386,7 +390,7 @@ class VaultItemListingViewModelTest : BaseViewModelTest() { @Suppress("MaxLineLength") @Test - fun `MasterPasswordRepromptSubmit for a request Success with a valid password should post to the AutofillSelectionManager`() = + fun `MasterPasswordRepromptSubmit for a request Success with a valid password for autofill should post to the AutofillSelectionManager`() = runTest { setupMockUri() val cipherView = createMockCipherView(number = 1) @@ -408,8 +412,10 @@ class VaultItemListingViewModelTest : BaseViewModelTest() { autofillSelectionManager.autofillSelectionFlow.test { viewModel.trySendAction( VaultItemListingsAction.MasterPasswordRepromptSubmit( - cipherId = cipherId, password = password, + masterPasswordRepromptData = MasterPasswordRepromptData.Autofill( + cipherId = cipherId, + ), ), ) assertEquals( @@ -419,6 +425,33 @@ class VaultItemListingViewModelTest : BaseViewModelTest() { } } + @Suppress("MaxLineLength") + @Test + fun `MasterPasswordRepromptSubmit for a request Success with a valid password for overflow actions should process the action`() = + runTest { + val cipherId = "cipherId-1234" + val password = "password" + val viewModel = createVaultItemListingViewModel() + coEvery { + authRepository.validatePassword(password = password) + } returns ValidatePasswordResult.Success(isValid = true) + + viewModel.eventFlow.test { + viewModel.trySendAction( + VaultItemListingsAction.MasterPasswordRepromptSubmit( + password = password, + masterPasswordRepromptData = MasterPasswordRepromptData.OverflowItem( + action = ListingItemOverflowAction.VaultAction.EditClick( + cipherId = cipherId, + ), + ), + ), + ) + // An Edit action navigates to the Edit screen + assertEquals(VaultItemListingEvent.NavigateToEditCipher(cipherId), awaitItem()) + } + } + @Test fun `AddVaultItemClick for vault item should emit NavigateToAddVaultItem`() = runTest { val viewModel = createVaultItemListingViewModel() @@ -843,7 +876,7 @@ class VaultItemListingViewModelTest : BaseViewModelTest() { createVaultItemListingState( viewState = VaultItemListingState.ViewState.Content( displayItemList = listOf( - createMockDisplayItemForCipher(number = 1), + createMockDisplayItemForCipher(number = 1).copy(isAutofill = true), ), ), ) diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensionsTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensionsTest.kt index b1398932e9..425cf34cc0 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensionsTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataExtensionsTest.kt @@ -358,7 +358,8 @@ class VaultItemListingDataExtensionsTest { number = 1, cipherType = CipherType.LOGIN, subtitle = null, - ), + ) + .copy(shouldShowMasterPasswordReprompt = true), createMockDisplayItemForCipher( number = 2, cipherType = CipherType.CARD, @@ -421,12 +422,16 @@ class VaultItemListingDataExtensionsTest { cipherType = CipherType.LOGIN, subtitle = null, ) - .copy(shouldShowMasterPasswordReprompt = true), + .copy( + isAutofill = true, + shouldShowMasterPasswordReprompt = true, + ), createMockDisplayItemForCipher( number = 2, cipherType = CipherType.CARD, subtitle = null, - ), + ) + .copy(isAutofill = true), ), ), result, diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataUtil.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataUtil.kt index 3d607fbc8a..e642a1d096 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataUtil.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/itemlisting/util/VaultItemListingDataUtil.kt @@ -53,6 +53,7 @@ fun createMockDisplayItemForCipher( url = "www.mockuri$number.com", ), ), + isAutofill = false, shouldShowMasterPasswordReprompt = false, ) } @@ -80,6 +81,7 @@ fun createMockDisplayItemForCipher( notes = "mockNotes-$number", ), ), + isAutofill = false, shouldShowMasterPasswordReprompt = false, ) } @@ -110,6 +112,7 @@ fun createMockDisplayItemForCipher( securityCode = "mockCode-$number", ), ), + isAutofill = false, shouldShowMasterPasswordReprompt = false, ) } @@ -134,6 +137,7 @@ fun createMockDisplayItemForCipher( ListingItemOverflowAction.VaultAction.ViewClick(cipherId = "mockId-$number"), ListingItemOverflowAction.VaultAction.EditClick(cipherId = "mockId-$number"), ), + isAutofill = false, shouldShowMasterPasswordReprompt = false, ) } @@ -175,6 +179,7 @@ fun createMockDisplayItemForSend( ListingItemOverflowAction.SendAction.RemovePasswordClick(sendId = "mockId-$number"), ListingItemOverflowAction.SendAction.DeleteClick(sendId = "mockId-$number"), ), + isAutofill = false, shouldShowMasterPasswordReprompt = false, ) } @@ -206,6 +211,7 @@ fun createMockDisplayItemForSend( ListingItemOverflowAction.SendAction.RemovePasswordClick(sendId = "mockId-$number"), ListingItemOverflowAction.SendAction.DeleteClick(sendId = "mockId-$number"), ), + isAutofill = false, shouldShowMasterPasswordReprompt = false, ) }