mirrors/bitwarden-android
1
0
Fork 0
mirror of https://github.com/bitwarden/android.git synced 2024-10-31 07:05:35 +03:00
Code Issues Projects Releases Packages Wiki Activity

[PM-12922] Disable delete if user can't manage collection

Browse source 
Disables the delete button for items in collections where the user does not have "manage" permission.

This change ensures that users cannot delete items from collections they are not authorized to manage. It updates the UI to reflect the user's permissions and prevents accidental or unauthorized deletions.
This commit is contained in:
Patrick Honkonen 2024-10-23 13:47:55 -04:00
parent eaa7923d1f
commit da59d056fc
No known key found for this signature in database
GPG key ID: B63AF42A5531C877
15 changed files with 502 additions and 14 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditScreen.kt
View file

@ -314,7 +314,7 @@ fun VaultAddEditScreen(
text = stringResource(id = R.string.delete),
onClick = { pendingDeleteCipher = true },
)
.takeUnless { state.isAddItemMode },
.takeUnless { state.isAddItemMode || !state.canDelete },
),
)
},

26
app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt
View file

@ -149,7 +149,9 @@ class VaultAddEditViewModel @Inject constructor(
attestationOptions = fido2AttestationOptions,
isIndividualVaultDisabled = isIndividualVaultDisabled,
)
?: totpData?.toDefaultAddTypeContent(isIndividualVaultDisabled)
?: totpData?.toDefaultAddTypeContent(
isIndividualVaultDisabled = isIndividualVaultDisabled,
)
?: VaultAddEditState.ViewState.Content(
common = VaultAddEditState.ViewState.Content.Common(),
isIndividualVaultDisabled = isIndividualVaultDisabled,
@ -1619,6 +1621,16 @@ class VaultAddEditViewModel @Inject constructor(
currentAccount = userData?.activeAccount,
vaultAddEditType = vaultAddEditType,
) { currentAccount, cipherView ->
val canDelete = vaultData.collectionViewList
.none {
val itemIsInCollection = cipherView
?.collectionIds
?.contains(it.id) == true
val canManageCollection = it.manage
itemIsInCollection && !canManageCollection
}
// Derive the view state from the current Cipher for Edit mode
// or use the current state for Add
(cipherView
@ -1628,6 +1640,7 @@ class VaultAddEditViewModel @Inject constructor(
totpData = totpData,
resourceManager = resourceManager,
clock = clock,
canDelete = canDelete,
)
?: viewState)
.appendFolderAndOwnerData(
@ -2056,6 +2069,15 @@ data class VaultAddEditState(
*/
val isCloneMode: Boolean get() = vaultAddEditType is VaultAddEditType.CloneItem
/**
* Helper to determine if the UI should allow deletion of this item.
*/
val canDelete: Boolean
get() = (viewState as? ViewState.Content)
?.common
?.canDelete
?: false
/**
* Enum representing the main type options for the vault, such as LOGIN, CARD, etc.
*
@ -2114,6 +2136,7 @@ data class VaultAddEditState(
* @property availableFolders The list of folders that this item could be added too.
* @property selectedOwnerId The ID of the owner associated with the item.
* @property availableOwners A list of available owners.
* @property canDelete Indicates whether the current user can delete the item.
*/
@Parcelize
data class Common(
@ -2129,6 +2152,7 @@ data class VaultAddEditState(
val availableFolders: List<Folder> = emptyList(),
val selectedOwnerId: String? = null,
val availableOwners: List<Owner> = emptyList(),
val canDelete: Boolean = true,
) : Parcelable {
/**

4
app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/util/CipherViewExtensions.kt
View file

@ -35,13 +35,14 @@ private const val PASSKEY_CREATION_TIME_PATTERN: String = "hh:mm a"
/**
* Transforms [CipherView] into [VaultAddEditState.ViewState].
*/
@Suppress("LongMethod")
@Suppress("LongMethod", "LongParameterList")
fun CipherView.toViewState(
isClone: Boolean,
isIndividualVaultDisabled: Boolean,
totpData: TotpData?,
resourceManager: ResourceManager,
clock: Clock,
canDelete: Boolean,
): VaultAddEditState.ViewState =
VaultAddEditState.ViewState.Content(
type = when (type) {
@ -107,6 +108,7 @@ fun CipherView.toViewState(
notes = this.notes.orEmpty(),
availableOwners = emptyList(),
customFieldData = this.fields.orEmpty().map { it.toCustomField() },
canDelete = canDelete,
),
isIndividualVaultDisabled = isIndividualVaultDisabled,
)

3
app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemScreen.kt
View file

@ -226,7 +226,8 @@ fun VaultItemScreen(
)
}
},
),
)
.takeIf { state.canDelete },
),
)
},

31
app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemViewModel.kt
View file

@ -82,7 +82,8 @@ class VaultItemViewModel @Inject constructor(
vaultRepository.getVaultItemStateFlow(state.vaultItemId),
authRepository.userStateFlow,
vaultRepository.getAuthCodeFlow(state.vaultItemId),
) { cipherViewState, userState, authCodeState ->
vaultRepository.collectionsStateFlow,
) { cipherViewState, userState, authCodeState, collectionsState ->
val totpCodeData = authCodeState.data?.let {
TotpCodeItemData(
periodSeconds = it.periodSeconds,
@ -96,14 +97,30 @@ class VaultItemViewModel @Inject constructor(
vaultDataState = combineDataStates(
cipherViewState,
authCodeState,
) { _, _ ->
collectionsState,
) { _, _, _ ->
// We are only combining the DataStates to know the overall state,
// we map it to the appropriate value below.
}
.mapNullable {
// Deletion is not allowed when the item is in a collection that the user
// does not have "manage" permission for.
val canDelete = collectionsState.data
?.none {
val itemIsInCollection = cipherViewState.data
?.collectionIds
?.contains(it.id) == true
val canManageCollection = it.manage
itemIsInCollection && !canManageCollection
}
?: true
VaultItemStateData(
cipher = cipherViewState.data,
totpCodeItemData = totpCodeData,
canDelete = canDelete,
)
},
)
@ -899,6 +916,7 @@ class VaultItemViewModel @Inject constructor(
isPremiumUser = account.isPremium,
hasMasterPassword = account.hasMasterPassword,
totpCodeItemData = this.data?.totpCodeItemData,
canDelete = this.data?.canDelete == true,
)
?: VaultItemState.ViewState.Error(message = errorText)
@ -1137,6 +1155,14 @@ data class VaultItemState(
?.isNotEmpty()
?: false
/**
* Whether or not the cipher can be deleted.
*/
val canDelete: Boolean
get() = viewState.asContentOrNull()
?.common
?.canDelete == true
/**
* The text to display on the deletion confirmation dialog.
*/
@ -1200,6 +1226,7 @@ data class VaultItemState(
@IgnoredOnParcel
val currentCipher: CipherView? = null,
val attachments: List<AttachmentItem>?,
val canDelete: Boolean,
) : Parcelable {
/**

1
app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/item/model/VaultItemStateData.kt
View file

@ -11,4 +11,5 @@ import com.bitwarden.vault.CipherView
data class VaultItemStateData(
val cipher: CipherView?,
val totpCodeItemData: TotpCodeItemData?,
val canDelete: Boolean,
)

4
app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/item/util/CipherViewExtensions.kt
View file

@ -32,13 +32,14 @@ private const val FIDO2_CREDENTIAL_CREATION_TIME_PATTERN: String = "h:mm a"
/**
* Transforms [VaultData] into [VaultState.ViewState].
*/
@Suppress("CyclomaticComplexMethod", "LongMethod")
@Suppress("CyclomaticComplexMethod", "LongMethod", "LongParameterList")
fun CipherView.toViewState(
previousState: VaultItemState.ViewState.Content?,
isPremiumUser: Boolean,
hasMasterPassword: Boolean,
totpCodeItemData: TotpCodeItemData?,
clock: Clock = Clock.systemDefaultZone(),
canDelete: Boolean,
): VaultItemState.ViewState =
VaultItemState.ViewState.Content(
common = VaultItemState.ViewState.Content.Common(
@ -79,6 +80,7 @@ fun CipherView.toViewState(
}
}
.orEmpty(),
canDelete = canDelete,
),
type = when (type) {
CipherType.LOGIN -> {

8
app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/CollectionViewUtil.kt
View file

@ -5,7 +5,11 @@ import com.bitwarden.vault.CollectionView
/**
* Create a mock [CollectionView] with a given [number].
*/
fun createMockCollectionView(number: Int, name: String? = null): CollectionView =
fun createMockCollectionView(
number: Int,
name: String? = null,
manage: Boolean = true,
): CollectionView =
CollectionView(
id = "mockId-$number",
organizationId = "mockOrganizationId-$number",
@ -13,5 +17,5 @@ fun createMockCollectionView(number: Int, name: String? = null): CollectionView
name = name ?: "mockName-$number",
externalId = "mockExternalId-$number",
readOnly = false,
manage = true,
manage = manage,
)

140
app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt
View file

@ -45,6 +45,7 @@ import com.x8bit.bitwarden.data.vault.datasource.network.model.OrganizationType
import com.x8bit.bitwarden.data.vault.datasource.network.model.PolicyTypeJson
import com.x8bit.bitwarden.data.vault.datasource.network.model.SyncResponseJson
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createMockCipherView
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createMockCollectionView
import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createMockSdkFido2CredentialList
import com.x8bit.bitwarden.data.vault.repository.VaultRepository
import com.x8bit.bitwarden.data.vault.repository.model.CreateCipherResult
@ -1195,6 +1196,136 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
}
}
@Suppress("MaxLineLength")
@Test
fun `in edit mode, canDelete should be false when cipher is in a collection the user cannot manage`() =
runTest {
val cipherView = createMockCipherView(1)
val vaultAddEditType = VaultAddEditType.EditItem(DEFAULT_EDIT_ITEM_ID)
val stateWithName = createVaultAddItemState(
vaultAddEditType = vaultAddEditType,
commonContentViewState = createCommonContentViewState(
name = "mockName-1",
originalCipher = cipherView,
customFieldData = listOf(
VaultAddEditState.Custom.HiddenField(
itemId = "testId",
name = "mockName-1",
value = "mockValue-1",
),
),
notes = "mockNotes-1",
canDelete = false,
),
)
every {
cipherView.toViewState(
isClone = false,
isIndividualVaultDisabled = false,
totpData = null,
resourceManager = resourceManager,
clock = fixedClock,
canDelete = false,
)
} returns stateWithName.viewState
mutableVaultDataFlow.value = DataState.Loaded(
data = createVaultData(
cipherView = cipherView,
collectionViewList = listOf(
createMockCollectionView(
number = 1,
manage = false,
),
),
),
)
createAddVaultItemViewModel(
createSavedStateHandleWithState(
state = stateWithName,
vaultAddEditType = vaultAddEditType,
),
)
verify {
cipherView.toViewState(
isClone = false,
isIndividualVaultDisabled = false,
totpData = null,
resourceManager = resourceManager,
clock = fixedClock,
canDelete = false,
)
}
}
@Suppress("MaxLineLength")
@Test
fun `in edit mode, canDelete should be true when cipher is in a collection the user can manage`() =
runTest {
val cipherView = createMockCipherView(1)
val vaultAddEditType = VaultAddEditType.EditItem(DEFAULT_EDIT_ITEM_ID)
val stateWithName = createVaultAddItemState(
vaultAddEditType = vaultAddEditType,
commonContentViewState = createCommonContentViewState(
name = "mockName-1",
originalCipher = cipherView,
customFieldData = listOf(
VaultAddEditState.Custom.HiddenField(
itemId = "testId",
name = "mockName-1",
value = "mockValue-1",
),
),
notes = "mockNotes-1",
canDelete = true,
),
)
every {
cipherView.toViewState(
isClone = false,
isIndividualVaultDisabled = false,
totpData = null,
resourceManager = resourceManager,
clock = fixedClock,
canDelete = true,
)
} returns stateWithName.viewState
mutableVaultDataFlow.value = DataState.Loaded(
data = createVaultData(
cipherView = cipherView,
collectionViewList = listOf(
createMockCollectionView(
number = 1,
manage = true,
),
),
),
)
createAddVaultItemViewModel(
createSavedStateHandleWithState(
state = stateWithName,
vaultAddEditType = vaultAddEditType,
),
)
verify {
cipherView.toViewState(
isClone = false,
isIndividualVaultDisabled = false,
totpData = null,
resourceManager = resourceManager,
clock = fixedClock,
canDelete = true,
)
}
}
@Test
fun `in edit mode, updateCipher success should ShowToast and NavigateBack`() =
runTest {
@ -1310,6 +1441,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
totpData = null,
resourceManager = resourceManager,
clock = fixedClock,
canDelete = true,
)
} returns stateWithName.viewState
mutableVaultDataFlow.value = DataState.Loaded(
@ -1341,6 +1473,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
totpData = null,
resourceManager = resourceManager,
clock = fixedClock,
canDelete = true,
)
vaultRepository.updateCipher(DEFAULT_EDIT_ITEM_ID, any())
}
@ -1375,6 +1508,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
totpData = null,
resourceManager = resourceManager,
clock = fixedClock,
canDelete = true,
)
} returns stateWithName.viewState
coEvery {
@ -1437,6 +1571,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
totpData = null,
resourceManager = resourceManager,
clock = fixedClock,
canDelete = true,
)
} returns stateWithName.viewState
coEvery {
@ -1502,6 +1637,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
totpData = null,
resourceManager = resourceManager,
clock = fixedClock,
canDelete = true,
)
} returns stateWithName.viewState
mutableVaultDataFlow.value = DataState.Loaded(
@ -1558,6 +1694,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
totpData = null,
resourceManager = resourceManager,
clock = fixedClock,
canDelete = true,
)
} returns stateWithName.viewState
every { fido2CredentialManager.isUserVerified } returns true
@ -1619,6 +1756,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
totpData = null,
resourceManager = resourceManager,
clock = fixedClock,
canDelete = true,
)
} returns stateWithName.viewState
every { fido2CredentialManager.isUserVerified } returns false
@ -4026,6 +4164,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
),
availableOwners: List<VaultAddEditState.Owner> = createOwnerList(),
selectedOwnerId: String? = null,
canDelete: Boolean = true,
): VaultAddEditState.ViewState.Content.Common =
VaultAddEditState.ViewState.Content.Common(
name = name,
@ -4038,6 +4177,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() {
originalCipher = originalCipher,
availableFolders = availableFolders,
availableOwners = availableOwners,
canDelete = canDelete,
)
@Suppress("LongParameterList")

7
app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/util/CipherViewExtensionsTest.kt
View file

@ -75,6 +75,7 @@ class CipherViewExtensionsTest {
totpData = null,
resourceManager = resourceManager,
clock = FIXED_CLOCK,
canDelete = true,
)
assertEquals(
@ -121,6 +122,7 @@ class CipherViewExtensionsTest {
totpData = null,
resourceManager = resourceManager,
clock = FIXED_CLOCK,
canDelete = true,
)
assertEquals(
@ -172,6 +174,7 @@ class CipherViewExtensionsTest {
totpData = null,
resourceManager = resourceManager,
clock = FIXED_CLOCK,
canDelete = true,
)
assertEquals(
@ -232,6 +235,7 @@ class CipherViewExtensionsTest {
totpData = mockk { every { uri } returns totp },
resourceManager = resourceManager,
clock = FIXED_CLOCK,
canDelete = true,
)
assertEquals(
@ -289,6 +293,7 @@ class CipherViewExtensionsTest {
totpData = null,
resourceManager = resourceManager,
clock = FIXED_CLOCK,
canDelete = true,
)
assertEquals(
@ -324,6 +329,7 @@ class CipherViewExtensionsTest {
totpData = null,
resourceManager = resourceManager,
clock = FIXED_CLOCK,
canDelete = true,
)
assertEquals(
@ -368,6 +374,7 @@ class CipherViewExtensionsTest {
totpData = null,
resourceManager = resourceManager,
clock = FIXED_CLOCK,
canDelete = true,
)
assertEquals(

4
app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/util/TotpDataExtensionsTest.kt
View file

@ -42,7 +42,9 @@ class TotpDataExtensionsTest {
isIndividualVaultDisabled = false,
type = VaultAddEditState.ViewState.Content.ItemType.Login(totp = uri),
),
totpData.toDefaultAddTypeContent(isIndividualVaultDisabled = false),
totpData.toDefaultAddTypeContent(
isIndividualVaultDisabled = false,
),
)
}
}

28
app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemScreenTest.kt
View file

@ -780,22 +780,39 @@ class VaultItemScreenTest : BaseComposeTest() {
}
@Test
fun `menu Delete option click should be displayed`() {
// Confirm dropdown version of item is absent
fun `menu Delete option should be displayed based on state`() {
// Confirm overflow is closed on initial load
composeTestRule
.onAllNodesWithText("Delete")
.filter(hasAnyAncestor(isPopup()))
.assertCountEquals(0)
// Open the overflow menu
composeTestRule
.onNodeWithContentDescription("More")
.performClick()
// Click on the delete item in the dropdown
// Confirm Delete option is present
mutableStateFlow.update { it.copy(viewState = DEFAULT_LOGIN_VIEW_STATE) }
composeTestRule
.onAllNodesWithText("Delete")
.filterToOne(hasAnyAncestor(isPopup()))
.assertIsDisplayed()
// Confirm Delete option is not present when canDelete is false
mutableStateFlow.update {
it.copy(
viewState = DEFAULT_LOGIN_VIEW_STATE
.copy(
common = DEFAULT_COMMON
.copy(canDelete = false),
),
)
}
composeTestRule
.onAllNodesWithText("Delete")
.filter(hasAnyAncestor(isPopup()))
.assertCountEquals(0)
}
@Test
@ -1166,6 +1183,7 @@ class VaultItemScreenTest : BaseComposeTest() {
@Test
fun `Menu should display correct items when cipher is not in a collection`() {
mutableStateFlow.update { it.copy(viewState = DEFAULT_LOGIN_VIEW_STATE) }
composeTestRule
.onNodeWithContentDescription("More")
.performClick()
@ -2350,6 +2368,7 @@ private val DEFAULT_COMMON: VaultItemState.ViewState.Content.Common =
title = "test.mp4",
),
),
canDelete = true,
)
private val DEFAULT_PASSKEY = R.string.created_xy.asText(
@ -2431,6 +2450,7 @@ private val EMPTY_COMMON: VaultItemState.ViewState.Content.Common =
requiresReprompt = true,
requiresCloneConfirmation = false,
attachments = emptyList(),
canDelete = true,
)
private val EMPTY_LOGIN_TYPE: VaultItemState.ViewState.Content.ItemType.Login =

243
app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemViewModelTest.kt
View file

File diff suppressed because it is too large Load diff

13
app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/item/util/CipherViewExtensionsTest.kt
View file

@ -45,6 +45,7 @@ class CipherViewExtensionsTest {
totpCode = "testCode",
),
clock = fixedClock,
canDelete = true,
)
assertEquals(
@ -80,6 +81,7 @@ class CipherViewExtensionsTest {
totpCode = "testCode",
),
clock = fixedClock,
canDelete = true,
)
assertEquals(
@ -108,6 +110,7 @@ class CipherViewExtensionsTest {
totpCode = "testCode",
),
clock = fixedClock,
canDelete = true,
)
assertEquals(
@ -136,6 +139,7 @@ class CipherViewExtensionsTest {
totpCode = "testCode",
),
clock = fixedClock,
canDelete = true,
)
assertEquals(
@ -170,6 +174,7 @@ class CipherViewExtensionsTest {
totpCode = "testCode",
),
clock = fixedClock,
canDelete = true,
)
assertEquals(
@ -194,6 +199,7 @@ class CipherViewExtensionsTest {
hasMasterPassword = true,
totpCodeItemData = null,
clock = fixedClock,
canDelete = true,
)
assertEquals(
@ -216,6 +222,7 @@ class CipherViewExtensionsTest {
hasMasterPassword = true,
totpCodeItemData = null,
clock = fixedClock,
canDelete = true,
)
assertEquals(
@ -237,6 +244,7 @@ class CipherViewExtensionsTest {
hasMasterPassword = true,
totpCodeItemData = null,
clock = fixedClock,
canDelete = true,
)
assertEquals(
@ -268,6 +276,7 @@ class CipherViewExtensionsTest {
hasMasterPassword = true,
totpCodeItemData = null,
clock = fixedClock,
canDelete = true,
)
assertEquals(
@ -304,6 +313,7 @@ class CipherViewExtensionsTest {
hasMasterPassword = true,
totpCodeItemData = null,
clock = fixedClock,
canDelete = true,
)
assertEquals(
@ -342,6 +352,7 @@ class CipherViewExtensionsTest {
hasMasterPassword = true,
totpCodeItemData = null,
clock = fixedClock,
canDelete = true,
)
assertEquals(
@ -364,6 +375,7 @@ class CipherViewExtensionsTest {
hasMasterPassword = true,
totpCodeItemData = null,
clock = fixedClock,
canDelete = true,
)
val expectedState = VaultItemState.ViewState.Content(
@ -384,6 +396,7 @@ class CipherViewExtensionsTest {
hasMasterPassword = true,
totpCodeItemData = null,
clock = fixedClock,
canDelete = true,
)
assertEquals(
VaultItemState.ViewState.Content(

2
app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/item/util/VaultItemTestUtil.kt
View file

@ -170,6 +170,7 @@ fun createCommonContent(
requiresReprompt = true,
requiresCloneConfirmation = false,
attachments = emptyList(),
canDelete = true,
)
} else {
VaultItemState.ViewState.Content.Common(
@ -213,6 +214,7 @@ fun createCommonContent(
title = "test.mp4",
),
),
canDelete = true,
)
}