From 35e8cecdcf9bff793af03983ea9ec2d66ef0bd9a Mon Sep 17 00:00:00 2001 From: Patrick Honkonen <1883101+SaintPatrck@users.noreply.github.com> Date: Fri, 20 Dec 2024 12:33:09 -0500 Subject: [PATCH] [PM-15970] Allow assigning collections if user has correct permissions (#4461) --- .../feature/util/CollectionViewExtensions.kt | 49 ++++++++++++---- .../util/model/CollectionPermission.kt | 12 ++++ .../sdk/model/CollectionViewUtil.kt | 53 ++++++++++++++++- .../addedit/VaultAddEditViewModelTest.kt | 58 +++++++------------ .../util/CollectionViewExtensionsTest.kt | 58 ++++++++++++++----- 5 files changed, 169 insertions(+), 61 deletions(-) create mode 100644 app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/util/model/CollectionPermission.kt diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/util/CollectionViewExtensions.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/util/CollectionViewExtensions.kt index b75b186bed..eec2a72a92 100644 --- a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/util/CollectionViewExtensions.kt +++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/util/CollectionViewExtensions.kt @@ -1,6 +1,7 @@ package com.x8bit.bitwarden.ui.vault.feature.util import com.bitwarden.vault.CollectionView +import com.x8bit.bitwarden.ui.vault.feature.util.model.CollectionPermission private const val COLLECTION_DIVIDER: String = "/" @@ -110,15 +111,43 @@ fun List?.hasDeletePermissionInAtLeastOneCollection( * Checks if the user has permission to assign an item to a collection. * * 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. + * have "manage" permission for and is also in a collection they cannot view the passwords in. + * + * E.g., If an item is in A collection with "view except passwords" or "edit except passwords" + * permission and in another with "manage" permission, the user **cannot** assign the item to other + * collections. Conversely, if an item is in a collection with "manage" permission and another with + * "view" or "edit" permission, the user **can** assign the item to other collections. */ -fun List?.canAssignToCollections(currentCollectionIds: List?) = - this - ?.none { - val itemIsInCollection = currentCollectionIds - ?.contains(it.id) - ?: false +fun List?.canAssignToCollections(currentCollectionIds: List?): Boolean { + if (this.isNullOrEmpty()) return true + if (currentCollectionIds.isNullOrEmpty()) return true - itemIsInCollection && (!it.manage || it.readOnly) - } - ?: true + // Verify user can MANAGE at least one collection the item is in. + return this + .any { + currentCollectionIds.contains(it.id) && + it.permission == CollectionPermission.MANAGE + } && + + // Verify user does not have "edit except password" or "view except passwords" + // permission in any collection the item is not in. + this + .none { + currentCollectionIds.contains(it.id) && + (it.permission == CollectionPermission.EDIT_EXCEPT_PASSWORD || + it.permission == CollectionPermission.VIEW_EXCEPT_PASSWORDS) + } +} + +/** + * Determines the user's permission level for a given [CollectionView]. + */ +val CollectionView.permission: CollectionPermission + get() = when { + manage -> CollectionPermission.MANAGE + readOnly && hidePasswords -> CollectionPermission.VIEW_EXCEPT_PASSWORDS + readOnly -> CollectionPermission.VIEW + !readOnly && hidePasswords -> CollectionPermission.EDIT_EXCEPT_PASSWORD + // !readOnly is the only other possible condition, which resolves to EDIT permission + else -> CollectionPermission.EDIT + } diff --git a/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/util/model/CollectionPermission.kt b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/util/model/CollectionPermission.kt new file mode 100644 index 0000000000..5b802c7859 --- /dev/null +++ b/app/src/main/java/com/x8bit/bitwarden/ui/vault/feature/util/model/CollectionPermission.kt @@ -0,0 +1,12 @@ +package com.x8bit.bitwarden.ui.vault.feature.util.model + +/** + * Represents the permission levels a user can be assigned to a collection. + */ +enum class CollectionPermission { + VIEW, + VIEW_EXCEPT_PASSWORDS, + EDIT, + EDIT_EXCEPT_PASSWORD, + MANAGE, +} diff --git a/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/CollectionViewUtil.kt b/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/CollectionViewUtil.kt index 9e3813745a..3131d8c328 100644 --- a/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/CollectionViewUtil.kt +++ b/app/src/test/java/com/x8bit/bitwarden/data/vault/datasource/sdk/model/CollectionViewUtil.kt @@ -10,13 +10,64 @@ fun createMockCollectionView( name: String? = null, readOnly: Boolean = false, manage: Boolean = true, + hidePasswords: Boolean = false, ): CollectionView = CollectionView( id = "mockId-$number", organizationId = "mockOrganizationId-$number", - hidePasswords = false, + hidePasswords = hidePasswords, name = name ?: "mockName-$number", externalId = "mockExternalId-$number", readOnly = readOnly, manage = manage, ) + +/** + * Create a [CollectionView] configured to reflect MANAGE permission. + */ +fun createManageCollectionView(number: Int) = createMockCollectionView( + number = number, + manage = true, + readOnly = false, + hidePasswords = false, +) + +/** + * Create a [CollectionView] configured to reflect EDIT permission. + */ +fun createEditCollectionView(number: Int) = createMockCollectionView( + number = number, + manage = false, + readOnly = false, + hidePasswords = false, +) + +/** + * Create a [CollectionView] configured to reflect EDIT_EXCEPT_PASSWORDS permission. + */ +fun createEditExceptPasswordsCollectionView(number: Int) = createMockCollectionView( + number = number, + manage = false, + readOnly = false, + hidePasswords = true, +) + +/** + * Create a [CollectionView] configured to reflect VIEW permission. + */ +fun createViewCollectionView(number: Int) = createMockCollectionView( + number = number, + manage = false, + readOnly = true, + hidePasswords = false, +) + +/** + * Create a [CollectionView] configured to reflect VIEW_EXCEPT_PASSWORDS permission. + */ +fun createViewExceptPasswordsCollectionView(number: Int) = createMockCollectionView( + number = number, + manage = false, + readOnly = true, + hidePasswords = true, +) diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt index 5471f69d93..112ede659c 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/addedit/VaultAddEditViewModelTest.kt @@ -43,9 +43,13 @@ import com.x8bit.bitwarden.data.tools.generator.repository.util.FakeGeneratorRep 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.createEditCollectionView +import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createEditExceptPasswordsCollectionView +import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createManageCollectionView 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.datasource.sdk.model.createViewCollectionView +import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createViewExceptPasswordsCollectionView import com.x8bit.bitwarden.data.vault.repository.VaultRepository import com.x8bit.bitwarden.data.vault.repository.model.CreateCipherResult import com.x8bit.bitwarden.data.vault.repository.model.DeleteCipherResult @@ -1182,7 +1186,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { resourceManager = resourceManager, clock = fixedClock, canDelete = false, - canAssignToCollections = true, + canAssignToCollections = false, ) } returns stateWithName.viewState @@ -1190,10 +1194,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { data = createVaultData( cipherView = cipherView, collectionViewList = listOf( - createMockCollectionView( - number = 1, - manage = false, - ), + createEditCollectionView(number = 1), ), ), ) @@ -1223,6 +1224,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { fun `in edit mode, canDelete should be true when cipher is in a collection the user can manage`() = runTest { val cipherView = createMockCipherView(1) + .copy(collectionIds = listOf("mockId-1", "mockId-2")) val vaultAddEditType = VaultAddEditType.EditItem(DEFAULT_EDIT_ITEM_ID) val stateWithName = createVaultAddItemState( vaultAddEditType = vaultAddEditType, @@ -1258,16 +1260,8 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { data = createVaultData( cipherView = cipherView, collectionViewList = listOf( - createMockCollectionView( - number = 1, - manage = true, - readOnly = false, - ), - createMockCollectionView( - number = 2, - manage = false, - readOnly = true, - ), + createManageCollectionView(number = 1), + createViewCollectionView(number = 2), ), ), ) @@ -1294,7 +1288,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { @Suppress("MaxLineLength") @Test - fun `in edit mode, canAssociateToCollections should be false when cipher is in a collection the user cannot manage or edit`() = + fun `in edit mode, canAssociateToCollections should be false when cipher is in a collection with view permission`() = runTest { val cipherView = createMockCipherView(1) val vaultAddEditType = VaultAddEditType.EditItem(DEFAULT_EDIT_ITEM_ID) @@ -1332,11 +1326,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { data = createVaultData( cipherView = cipherView, collectionViewList = listOf( - createMockCollectionView( - number = 1, - readOnly = true, - manage = false, - ), + createViewCollectionView(number = 1), ), ), ) @@ -1363,9 +1353,10 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { @Suppress("MaxLineLength") @Test - fun `in edit mode, canAssociateToCollections should be false when cipher is in a collection the user cannot manage but can edit`() = + fun `in edit mode, canAssociateToCollections should be false when cipher is in a collection with manage permission and a collection with edit, except password permission`() = runTest { val cipherView = createMockCipherView(1) + .copy(collectionIds = listOf("mockId-1", "mockId-2")) val vaultAddEditType = VaultAddEditType.EditItem(DEFAULT_EDIT_ITEM_ID) val stateWithName = createVaultAddItemState( vaultAddEditType = vaultAddEditType, @@ -1392,7 +1383,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { totpData = null, resourceManager = resourceManager, clock = fixedClock, - canDelete = false, + canDelete = true, canAssignToCollections = false, ) } returns stateWithName.viewState @@ -1401,11 +1392,8 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { data = createVaultData( cipherView = cipherView, collectionViewList = listOf( - createMockCollectionView( - number = 1, - manage = false, - readOnly = false, - ), + createManageCollectionView(number = 1), + createEditExceptPasswordsCollectionView(number = 2), ), ), ) @@ -1424,7 +1412,7 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { totpData = null, resourceManager = resourceManager, clock = fixedClock, - canDelete = false, + canDelete = true, canAssignToCollections = false, ) } @@ -1432,9 +1420,10 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { @Suppress("MaxLineLength") @Test - fun `in edit mode, canAssociateToCollections should be false when cipher is in a collection the user can manage but cannot edit`() = + fun `in edit mode, canAssociateToCollections should be false when cipher is in a collection with manage permission and a collection with view, except password permission`() = runTest { val cipherView = createMockCipherView(1) + .copy(collectionIds = listOf("mockId-1", "mockId-2")) val vaultAddEditType = VaultAddEditType.EditItem(DEFAULT_EDIT_ITEM_ID) val stateWithName = createVaultAddItemState( vaultAddEditType = vaultAddEditType, @@ -1470,11 +1459,8 @@ class VaultAddEditViewModelTest : BaseViewModelTest() { data = createVaultData( cipherView = cipherView, collectionViewList = listOf( - createMockCollectionView( - number = 1, - manage = true, - readOnly = true, - ), + createManageCollectionView(number = 1), + createViewExceptPasswordsCollectionView(number = 2), ), ), ) diff --git a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/util/CollectionViewExtensionsTest.kt b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/util/CollectionViewExtensionsTest.kt index 50538d7bdd..2c57c1ea58 100644 --- a/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/util/CollectionViewExtensionsTest.kt +++ b/app/src/test/java/com/x8bit/bitwarden/ui/vault/feature/util/CollectionViewExtensionsTest.kt @@ -1,7 +1,13 @@ package com.x8bit.bitwarden.ui.vault.feature.util import com.bitwarden.vault.CollectionView +import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createEditCollectionView +import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createEditExceptPasswordsCollectionView +import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createManageCollectionView import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createMockCollectionView +import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createViewCollectionView +import com.x8bit.bitwarden.data.vault.datasource.sdk.model.createViewExceptPasswordsCollectionView +import com.x8bit.bitwarden.ui.vault.feature.util.model.CollectionPermission import org.junit.jupiter.api.Assertions.assertEquals import org.junit.jupiter.api.Assertions.assertFalse import org.junit.jupiter.api.Assertions.assertTrue @@ -73,11 +79,14 @@ class CollectionViewExtensionsTest { @Test fun `hasDeletePermissionInAtLeastOneCollection should return true if the user has manage permission in at least one collection`() { val collectionList: List = listOf( - createMockCollectionView(number = 1, manage = true), - createMockCollectionView(number = 2, manage = false), + createManageCollectionView(number = 1), + createEditCollectionView(number = 2), + createEditExceptPasswordsCollectionView(number = 3), + createViewCollectionView(number = 4), + createViewExceptPasswordsCollectionView(number = 5), ) - val collectionIds = listOf("mockId-1", "mockId-2") + val collectionIds = collectionList.mapNotNull { it.id } assertTrue(collectionList.hasDeletePermissionInAtLeastOneCollection(collectionIds)) } @@ -86,10 +95,12 @@ class CollectionViewExtensionsTest { @Test fun `hasDeletePermissionInAtLeastOneCollection should return false if the user does not have manage permission in at least one collection`() { val collectionList: List = listOf( - createMockCollectionView(number = 1, manage = false), - createMockCollectionView(number = 2, manage = false), + createEditCollectionView(number = 1), + createEditExceptPasswordsCollectionView(number = 2), + createViewCollectionView(number = 3), + createViewExceptPasswordsCollectionView(number = 4), ) - val collectionIds = listOf("mockId-1", "mockId-2") + val collectionIds = collectionList.mapNotNull { it.id } assertFalse(collectionList.hasDeletePermissionInAtLeastOneCollection(collectionIds)) } @@ -104,8 +115,8 @@ class CollectionViewExtensionsTest { @Test fun `hasDeletePermissionInAtLeastOneCollection should return true if the collectionIds list is null`() { val collectionList: List = listOf( - createMockCollectionView(number = 1, manage = true), - createMockCollectionView(number = 2, manage = false), + createManageCollectionView(number = 1), + createEditCollectionView(number = 2), ) assertTrue(collectionList.hasDeletePermissionInAtLeastOneCollection(null)) } @@ -135,20 +146,24 @@ class CollectionViewExtensionsTest { @Test fun `canAssociateToCollections should return true if the user has edit and manage permission`() { val collectionList: List = listOf( - createMockCollectionView(number = 1, manage = true, readOnly = false), + createEditCollectionView(number = 1), + createManageCollectionView(number = 2), ) - val collectionIds = listOf("mockId-1", "mockId-2") + val collectionIds = collectionList.mapNotNull { it.id } assertTrue(collectionList.canAssignToCollections(collectionIds)) } @Suppress("MaxLineLength") @Test - fun `canAssociateToCollections should return false if the user does not have manage or edit permission in at least one collection`() { + fun `canAssociateToCollections should return false if the user has except password permission at least one collection`() { val collectionList: List = listOf( - createMockCollectionView(number = 1, manage = false, readOnly = true), - createMockCollectionView(number = 2, manage = false, readOnly = true), + createEditExceptPasswordsCollectionView(number = 1), + createViewCollectionView(number = 2), + createViewExceptPasswordsCollectionView(number = 3), + createManageCollectionView(number = 4), + createEditCollectionView(number = 5), ) - val collectionIds = listOf("mockId-1", "mockId-2") + val collectionIds = collectionList.mapNotNull { it.id } assertFalse(collectionList.canAssignToCollections(collectionIds)) } @@ -166,4 +181,19 @@ class CollectionViewExtensionsTest { ) assertTrue(collectionList.canAssignToCollections(null)) } + + @Test + fun `permission should return correct value`() { + assertEquals(CollectionPermission.MANAGE, createManageCollectionView(number = 1).permission) + assertEquals(CollectionPermission.EDIT, createEditCollectionView(number = 1).permission) + assertEquals( + CollectionPermission.EDIT_EXCEPT_PASSWORD, + createEditExceptPasswordsCollectionView(number = 1).permission, + ) + assertEquals(CollectionPermission.VIEW, createViewCollectionView(number = 1).permission) + assertEquals( + CollectionPermission.VIEW_EXCEPT_PASSWORDS, + createViewExceptPasswordsCollectionView(number = 1).permission, + ) + } }