Skip to content

Commit

Permalink
[PM-13360] Respect manage permission to assign collections
Browse files Browse the repository at this point in the history 
This commit prevents users from assigning items to collections if the item is already in a read-only collection where the user does not have "manage" permission.

This change ensures that users with limited permissions cannot modify items in a way that violates the collection's access controls.
  • Loading branch information
@SaintPatrck
SaintPatrck committed Oct 30, 2024
1 parent 9850607 commit 76954d4
Show file tree
Hide file tree
Showing 15 changed files with 563 additions and 43 deletions.
8 changes: 6 additions & 2 deletions app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditScreen.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -235,7 +235,7 @@ fun VaultAddEditScreen(
},
)

if (pendingDeleteCipher) {
if (pendingDeleteCipher && state.canDelete) {
BitwardenTwoButtonDialog(
title = stringResource(id = R.string.delete),
message = stringResource(id = R.string.do_you_really_want_to_soft_delete_cipher),
Expand Down Expand Up @@ -309,7 +309,11 @@ fun VaultAddEditScreen(
}
},
)
.takeUnless { state.isAddItemMode || !state.isCipherInCollection },
.takeUnless {
state.isAddItemMode ||
(!state.isCipherInCollection ||
!state.canAssociateToCollections)
},
OverflowMenuItemData(
text = stringResource(id = R.string.delete),
onClick = { pendingDeleteCipher = true },
Expand Down
27 changes: 24 additions & 3 deletions app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModel.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1621,16 +1621,29 @@ class VaultAddEditViewModel @Inject constructor(
currentAccount = userData?.activeAccount,
vaultAddEditType = vaultAddEditType,
) { currentAccount, cipherView ->

// Deletion is not allowed when the item is in a collection that the user
// does not have "manage" permission for.
val canDelete = vaultData.collectionViewList
.none {
val itemIsInCollection = cipherView
val isItemInCollection = cipherView
?.collectionIds
?.contains(it.id) == true

val canManageCollection = it.manage
isItemInCollection && !it.manage
}

// Assigning to a collection is not allowed when the item is in a collection
// that the user does not have "manage" and "edit" permission for.
val canAssignToCollections = vaultData.collectionViewList
.none {
val isItemInCollection = cipherView
?.collectionIds
?.contains(it.id) == true

itemIsInCollection && !canManageCollection
isItemInCollection && (!it.manage || it.readOnly)
}

// Derive the view state from the current Cipher for Edit mode
// or use the current state for Add
(cipherView
Expand All @@ -1641,6 +1654,7 @@ class VaultAddEditViewModel @Inject constructor(
resourceManager = resourceManager,
clock = clock,
canDelete = canDelete,
canAssignToCollections = canAssignToCollections,
)
?: viewState)
.appendFolderAndOwnerData(
Expand Down Expand Up @@ -2078,6 +2092,12 @@ data class VaultAddEditState(
?.canDelete
?: false

val canAssociateToCollections: Boolean
get() = (viewState as? ViewState.Content)
?.common
?.canAssignToCollections
?: false

/**
* Enum representing the main type options for the vault, such as LOGIN, CARD, etc.
*
Expand Down Expand Up @@ -2153,6 +2173,7 @@ data class VaultAddEditState(
val selectedOwnerId: String? = null,
val availableOwners: List<Owner> = emptyList(),
val canDelete: Boolean = true,
val canAssignToCollections: Boolean = true,
) : Parcelable {

/**
Expand Down
2 changes: 2 additions & 0 deletions app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/addedit/util/CipherViewExtensions.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ fun CipherView.toViewState(
resourceManager: ResourceManager,
clock: Clock,
canDelete: Boolean,
canAssignToCollections: Boolean,
): VaultAddEditState.ViewState =
VaultAddEditState.ViewState.Content(
type = when (type) {
Expand Down Expand Up @@ -109,6 +110,7 @@ fun CipherView.toViewState(
availableOwners = emptyList(),
customFieldData = this.fields.orEmpty().map { it.toCustomField() },
canDelete = canDelete,
canAssignToCollections = canAssignToCollections,
),
isIndividualVaultDisabled = isIndividualVaultDisabled,
)
Expand Down
7 changes: 5 additions & 2 deletions app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemScreen.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ fun VaultItemScreen(
{ viewModel.trySendAction(VaultItemAction.Common.CloseClick) }
},
actions = {
if (state.isCipherDeleted) {
if (state.isCipherDeleted && state.canDelete) {
BitwardenTextButton(
label = stringResource(id = R.string.restore),
onClick = remember(viewModel) {
Expand Down Expand Up @@ -216,7 +216,10 @@ fun VaultItemScreen(
}
},
)
.takeIf { state.isCipherInCollection },
.takeIf {
state.isCipherInCollection &&
state.canAssignToCollections
},
OverflowMenuItemData(
text = stringResource(id = R.string.delete),
onClick = remember(viewModel) {
Expand Down
27 changes: 25 additions & 2 deletions app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/item/VaultItemViewModel.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,16 +111,27 @@ class VaultItemViewModel @Inject constructor(
?.collectionIds
?.contains(it.id) == true

val canManageCollection = it.manage
itemIsInCollection && !it.manage
}
?: true

itemIsInCollection && !canManageCollection
// Assigning to a collection is not allowed when the item is in a collection
// that the user does not have "manage" and "edit" permission for.
val canAssignToCollections = collectionsState.data
?.none {
val itemIsInCollection = cipherViewState.data
?.collectionIds
?.contains(it.id) == true

itemIsInCollection && !it.manage && it.readOnly
}
?: true

VaultItemStateData(
cipher = cipherViewState.data,
totpCodeItemData = totpCodeData,
canDelete = canDelete,
canAssociateToCollections = canAssignToCollections,
)
},
)
Expand Down Expand Up @@ -917,6 +928,7 @@ class VaultItemViewModel @Inject constructor(
hasMasterPassword = account.hasMasterPassword,
totpCodeItemData = this.data?.totpCodeItemData,
canDelete = this.data?.canDelete == true,
canAssignToCollections = this.data?.canAssociateToCollections == true,
)
?: VaultItemState.ViewState.Error(message = errorText)

Expand Down Expand Up @@ -1163,6 +1175,12 @@ data class VaultItemState(
?.common
?.canDelete == true

val canAssignToCollections: Boolean
get() = viewState.asContentOrNull()
?.common
?.canAssignToCollections
?: false

/**
* The text to display on the deletion confirmation dialog.
*/
Expand Down Expand Up @@ -1214,6 +1232,10 @@ data class VaultItemState(
* @property requiresCloneConfirmation Indicates user confirmation is required when
* cloning a cipher.
* @property currentCipher The cipher that is currently being viewed (nullable).
* @property attachments A list of attachments associated with the cipher.
* @property canDelete Indicates if the cipher can be deleted.
* @property canAssignToCollections Indicates if the cipher can be assigned to
* collections.
*/
@Parcelize
data class Common(
Expand All @@ -1227,6 +1249,7 @@ data class VaultItemState(
val currentCipher: CipherView? = null,
val attachments: List<AttachmentItem>?,
val canDelete: Boolean,
val canAssignToCollections: Boolean,
) : Parcelable {

/**
Expand Down
3 changes: 3 additions & 0 deletions app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/item/model/VaultItemStateData.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@ import com.bitwarden.vault.CipherView
*
* @property cipher The cipher view for the item.
* @property totpCodeItemData The data for the totp code.
* @property canDelete Whether the item can be deleted.
* @property canAssociateToCollections Whether the item can be associated to a collection.
*/
data class VaultItemStateData(
val cipher: CipherView?,
val totpCodeItemData: TotpCodeItemData?,
val canDelete: Boolean,
val canAssociateToCollections: Boolean,
)
2 changes: 2 additions & 0 deletions app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/item/util/CipherViewExtensions.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ fun CipherView.toViewState(
totpCodeItemData: TotpCodeItemData?,
clock: Clock = Clock.systemDefaultZone(),
canDelete: Boolean,
canAssignToCollections: Boolean,
): VaultItemState.ViewState =
VaultItemState.ViewState.Content(
common = VaultItemState.ViewState.Content.Common(
Expand Down Expand Up @@ -81,6 +82,7 @@ fun CipherView.toViewState(
}
.orEmpty(),
canDelete = canDelete,
canAssignToCollections = canAssignToCollections,
),
type = when (type) {
CipherType.LOGIN -> {
Expand Down
3 changes: 2 additions & 1 deletion app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/CollectionViewUtil.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import com.bitwarden.vault.CollectionView
fun createMockCollectionView(
number: Int,
name: String? = null,
readOnly: Boolean = false,
manage: Boolean = true,
): CollectionView =
CollectionView(
Expand All @@ -16,6 +17,6 @@ fun createMockCollectionView(
hidePasswords = false,
name = name ?: "mockName-$number",
externalId = "mockExternalId-$number",
readOnly = false,
readOnly = readOnly,
manage = manage,
)
52 changes: 52 additions & 0 deletions app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditScreenTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import androidx.compose.ui.test.assertIsOn
import androidx.compose.ui.test.assertTextContains
import androidx.compose.ui.test.assertTextEquals
import androidx.compose.ui.test.click
import androidx.compose.ui.test.filter
import androidx.compose.ui.test.filterToOne
import androidx.compose.ui.test.hasAnyAncestor
import androidx.compose.ui.test.hasContentDescription
Expand Down Expand Up @@ -3023,6 +3024,57 @@ class VaultAddEditScreenTest : BaseComposeTest() {
.assertIsDisplayed()
}

@Test
fun `Menu Collections should display correctly according to state`() {
mutableStateFlow.update {
it.copy(
vaultAddEditType = VaultAddEditType.EditItem(vaultItemId = "mockId-1"),
viewState = VaultAddEditState.ViewState.Content(
common = VaultAddEditState.ViewState.Content.Common(
originalCipher = createMockCipherView(1),
),
type = VaultAddEditState.ViewState.Content.ItemType.SecureNotes,
isIndividualVaultDisabled = false,
),
)
}
// Confirm overflow is closed on initial load
composeTestRule
.onAllNodesWithText("Collections")
.filter(hasAnyAncestor(isPopup()))
.assertCountEquals(0)

// Open the overflow menu
composeTestRule
.onNodeWithContentDescription("More")
.performClick()

// Confirm Collections option is present
composeTestRule
.onAllNodesWithText("Collections")
.filterToOne(hasAnyAncestor(isPopup()))
.assertIsDisplayed()

// Confirm Collections option is not present when canAssignToCollections is false
mutableStateFlow.update {
it.copy(
vaultAddEditType = VaultAddEditType.EditItem(vaultItemId = "mockId-1"),
viewState = VaultAddEditState.ViewState.Content(
common = VaultAddEditState.ViewState.Content.Common(
originalCipher = createMockCipherView(1),
canAssignToCollections = false,
),
type = VaultAddEditState.ViewState.Content.ItemType.SecureNotes,
isIndividualVaultDisabled = false,
),
)
}
composeTestRule
.onAllNodesWithText("Collections")
.filter(hasAnyAncestor(isPopup()))
.assertCountEquals(0)
}

@Test
fun `Menu should display correct items when cipher is not in a collection`() {
mutableStateFlow.update {
Expand Down
Loading

0 comments on commit 76954d4

Please sign in to comment.