From 0b950080ca25b6fe08e1893d3f671ffea4f1323e Mon Sep 17 00:00:00 2001 From: Shane Melton Date: Wed, 22 May 2024 11:58:04 -0700 Subject: [PATCH] [PM-8004] [AC-2603] [AC-2616] [AC-2621] [AC-2622] Unmanaged collection fixes (#9301) * [AC-2603] Add unmanaged property to CollectionAdminView and response models * [AC-2603] Cleanup CollectionViews - Remove getters that have been replaced with Unmanaged property - Remove AddAccess that is also being replaced - Add canEditUnmanagedCollections() helper to organization * [AC-2603] Replace old AddAccess logic with Unmanaged flag * [AC-2603] Fix failing test * [AC-2603] Ensure Add Access badge/toggle only shows when V1 flag is enabled * [AC-2603] Undo change to canEditUserAccess and canEditGroupAccess Custom users should not get access to an unmanaged collection with only Manage Groups and Manage User permissions. That is still reserved for admin/owners and EditAnyCollection custom users. --- .../vault-collection-row.component.html | 7 +- .../vault-collection-row.component.ts | 22 +++++ .../vault-items/vault-items.component.ts | 47 ----------- .../vault/core/collection-admin.service.ts | 1 + .../vault/core/views/collection-admin.view.ts | 40 +++------ .../app/vault/org-vault/vault.component.ts | 82 ++++++------------- .../src/app/vault/utils/collection-utils.ts | 1 + .../models/domain/organization.ts | 5 ++ .../vault/models/domain/collection.spec.ts | 1 - .../models/response/collection.response.ts | 2 + .../src/vault/models/view/collection.view.ts | 1 - 11 files changed, 66 insertions(+), 143 deletions(-) diff --git a/apps/web/src/app/vault/components/vault-items/vault-collection-row.component.html b/apps/web/src/app/vault/components/vault-items/vault-collection-row.component.html index 48db50e202..d8a98101ef 100644 --- a/apps/web/src/app/vault/components/vault-items/vault-collection-row.component.html +++ b/apps/web/src/app/vault/components/vault-items/vault-collection-row.component.html @@ -30,12 +30,7 @@ > {{ collection.name }}
- {{ "addAccess" | i18n }} + {{ "addAccess" | i18n }}
diff --git a/apps/web/src/app/vault/components/vault-items/vault-collection-row.component.ts b/apps/web/src/app/vault/components/vault-items/vault-collection-row.component.ts index 7dcb463548..67bbf7b380 100644 --- a/apps/web/src/app/vault/components/vault-items/vault-collection-row.component.ts +++ b/apps/web/src/app/vault/components/vault-items/vault-collection-row.component.ts @@ -56,6 +56,28 @@ export class VaultCollectionRowComponent { return this.organizations.find((o) => o.id === this.collection.organizationId); } + get showAddAccess() { + if (!this.flexibleCollectionsV1Enabled) { + return false; + } + + if (this.collection.id == Unassigned) { + return false; + } + + // Only show AddAccess when viewing the Org vault (implied by CollectionAdminView) + if (this.collection instanceof CollectionAdminView) { + // Only show AddAccess if unmanaged and allowAdminAccessToAllCollectionItems is disabled + return ( + !this.organization.allowAdminAccessToAllCollectionItems && + this.collection.unmanaged && + this.organization.canEditUnmanagedCollections() + ); + } + + return false; + } + get permissionText() { if ( this.collection.id == Unassigned && diff --git a/apps/web/src/app/vault/components/vault-items/vault-items.component.ts b/apps/web/src/app/vault/components/vault-items/vault-items.component.ts index 6443d8de3d..baca403f18 100644 --- a/apps/web/src/app/vault/components/vault-items/vault-items.component.ts +++ b/apps/web/src/app/vault/components/vault-items/vault-items.component.ts @@ -1,7 +1,6 @@ import { SelectionModel } from "@angular/cdk/collections"; import { Component, EventEmitter, Input, Output } from "@angular/core"; -import { OrganizationUserType } from "@bitwarden/common/admin-console/enums"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { CollectionView } from "@bitwarden/common/vault/models/view/collection.view"; @@ -106,27 +105,6 @@ export class VaultItemsComponent { const organization = this.allOrganizations.find((o) => o.id === collection.organizationId); - if (this.flexibleCollectionsV1Enabled) { - //Custom user without edit access should not see the Edit option unless that user has "Can Manage" access to a collection - if ( - !collection.manage && - organization?.type === OrganizationUserType.Custom && - !organization?.permissions.editAnyCollection - ) { - return false; - } - //Owner/Admin and Custom Users with Edit can see Edit and Access of Orphaned Collections - if ( - collection.addAccess && - collection.id !== Unassigned && - ((organization?.type === OrganizationUserType.Custom && - organization?.permissions.editAnyCollection) || - organization.isAdmin || - organization.isOwner) - ) { - return true; - } - } return collection.canEdit(organization, this.flexibleCollectionsV1Enabled); } @@ -138,31 +116,6 @@ export class VaultItemsComponent { const organization = this.allOrganizations.find((o) => o.id === collection.organizationId); - if (this.flexibleCollectionsV1Enabled) { - //Custom user with only edit access should not see the Delete button for orphaned collections - if ( - collection.addAccess && - organization?.type === OrganizationUserType.Custom && - !organization?.permissions.deleteAnyCollection && - organization?.permissions.editAnyCollection - ) { - return false; - } - - // Owner/Admin with no access to a collection will not see Delete - if ( - !collection.assigned && - !collection.addAccess && - (organization.isAdmin || organization.isOwner) && - !( - organization?.type === OrganizationUserType.Custom && - organization?.permissions.deleteAnyCollection - ) - ) { - return false; - } - } - return collection.canDelete(organization, this.flexibleCollectionsV1Enabled); } diff --git a/apps/web/src/app/vault/core/collection-admin.service.ts b/apps/web/src/app/vault/core/collection-admin.service.ts index 7f78ab214a..b6ddd452a1 100644 --- a/apps/web/src/app/vault/core/collection-admin.service.ts +++ b/apps/web/src/app/vault/core/collection-admin.service.ts @@ -127,6 +127,7 @@ export class CollectionAdminService { view.readOnly = c.readOnly; view.hidePasswords = c.hidePasswords; view.manage = c.manage; + view.unmanaged = c.unmanaged; } return view; diff --git a/apps/web/src/app/vault/core/views/collection-admin.view.ts b/apps/web/src/app/vault/core/views/collection-admin.view.ts index f8695ba217..6e842023d3 100644 --- a/apps/web/src/app/vault/core/views/collection-admin.view.ts +++ b/apps/web/src/app/vault/core/views/collection-admin.view.ts @@ -1,4 +1,3 @@ -import { OrganizationUserUserDetailsResponse } from "@bitwarden/common/admin-console/abstractions/organization-user/responses"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { CollectionAccessDetailsResponse } from "@bitwarden/common/src/vault/models/response/collection.response"; import { CollectionView } from "@bitwarden/common/vault/models/view/collection.view"; @@ -9,7 +8,12 @@ import { Unassigned } from "../../individual-vault/vault-filter/shared/models/ro export class CollectionAdminView extends CollectionView { groups: CollectionAccessSelectionView[] = []; users: CollectionAccessSelectionView[] = []; - addAccess: boolean; + + /** + * Flag indicating the collection has no active user or group assigned to it with CanManage permissions + * In this case, the collection can be managed by admins/owners or custom users with appropriate permissions + */ + unmanaged: boolean; /** * Flag indicating the user has been explicitly assigned to this Collection @@ -34,39 +38,13 @@ export class CollectionAdminView extends CollectionView { this.assigned = response.assigned; } - groupsCanManage() { - if (this.groups.length === 0) { - return this.groups; - } - - const returnedGroups = this.groups.filter((group) => { - if (group.manage) { - return group; - } - }); - return returnedGroups; - } - - usersCanManage(revokedUsers: OrganizationUserUserDetailsResponse[]) { - if (this.users.length === 0) { - return this.users; - } - - const returnedUsers = this.users.filter((user) => { - const isRevoked = revokedUsers.some((revoked) => revoked.id === user.id); - if (user.manage && !isRevoked) { - return user; - } - }); - return returnedUsers; - } - /** * Returns true if the user can edit a collection (including user and group access) from the Admin Console. */ override canEdit(org: Organization, flexibleCollectionsV1Enabled: boolean): boolean { return ( org?.canEditAnyCollection(flexibleCollectionsV1Enabled) || + (flexibleCollectionsV1Enabled && this.unmanaged && org?.canEditUnmanagedCollections()) || super.canEdit(org, flexibleCollectionsV1Enabled) ); } @@ -125,6 +103,10 @@ export class CollectionAdminView extends CollectionView { return this.manage || org?.isAdmin || org?.permissions.editAnyCollection; } + /** + * True if this collection represents the pseudo "Unassigned" collection + * This is different from the "unmanaged" flag, which indicates that no users or groups have access to the collection + */ get isUnassignedCollection() { return this.id === Unassigned; } diff --git a/apps/web/src/app/vault/org-vault/vault.component.ts b/apps/web/src/app/vault/org-vault/vault.component.ts index 0247b89bfd..514cb8150d 100644 --- a/apps/web/src/app/vault/org-vault/vault.component.ts +++ b/apps/web/src/app/vault/org-vault/vault.component.ts @@ -38,7 +38,6 @@ import { SearchService } from "@bitwarden/common/abstractions/search.service"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { OrganizationUserService } from "@bitwarden/common/admin-console/abstractions/organization-user/organization-user.service"; import { OrganizationUserUserDetailsResponse } from "@bitwarden/common/admin-console/abstractions/organization-user/responses"; -import { OrganizationUserType } from "@bitwarden/common/admin-console/enums"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { EventType } from "@bitwarden/common/enums"; import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; @@ -433,13 +432,13 @@ export class VaultComponent implements OnInit, OnDestroy { this.showAddAccessToggle = false; let collectionsToReturn = []; if (filter.collectionId === undefined || filter.collectionId === All) { - collectionsToReturn = await this.addAccessCollectionsMap(collections); + collectionsToReturn = collections.map((c) => c.node); } else { const selectedCollection = ServiceUtils.getTreeNodeObjectFromList( collections, filter.collectionId, ); - collectionsToReturn = await this.addAccessCollectionsMap(selectedCollection?.children); + collectionsToReturn = selectedCollection?.children.map((c) => c.node) ?? []; } if (await this.searchService.isSearchable(searchText)) { @@ -451,8 +450,15 @@ export class VaultComponent implements OnInit, OnDestroy { ); } + // Add access toggle is only shown if allowAdminAccessToAllCollectionItems is false and there are unmanaged collections the user can edit + this.showAddAccessToggle = + this.flexibleCollectionsV1Enabled && + !this.organization.allowAdminAccessToAllCollectionItems && + this.organization.canEditUnmanagedCollections() && + collectionsToReturn.some((c) => c.unmanaged); + if (addAccessStatus === 1 && this.showAddAccessToggle) { - collectionsToReturn = collectionsToReturn.filter((c: any) => c.addAccess); + collectionsToReturn = collectionsToReturn.filter((c) => c.unmanaged); } return collectionsToReturn; }), @@ -663,57 +669,7 @@ export class VaultComponent implements OnInit, OnDestroy { ); } - // Update the list of collections to see if any collection is orphaned - // and will receive the addAccess badge / be filterable by the user - async addAccessCollectionsMap(collections: TreeNode[]) { - let mappedCollections; - const { type, allowAdminAccessToAllCollectionItems, permissions } = this.organization; - - const canEditCiphersCheck = - this._flexibleCollectionsV1FlagEnabled && - !this.organization.canEditAllCiphers( - this._flexibleCollectionsV1FlagEnabled, - this.restrictProviderAccessEnabled, - ); - - // This custom type check will show addAccess badge for - // Custom users with canEdit access AND owner/admin manage access setting is OFF - const customUserCheck = - this._flexibleCollectionsV1FlagEnabled && - !allowAdminAccessToAllCollectionItems && - type === OrganizationUserType.Custom && - permissions.editAnyCollection; - - // If Custom user has Delete Only access they will not see Add Access toggle - const customUserOnlyDelete = - this.flexibleCollectionsV1Enabled && - type === OrganizationUserType.Custom && - permissions.deleteAnyCollection && - !permissions.editAnyCollection; - - if (!customUserOnlyDelete && (canEditCiphersCheck || customUserCheck)) { - mappedCollections = collections.map((c: TreeNode) => { - const groupsCanManage = c.node.groupsCanManage(); - const usersCanManage = c.node.usersCanManage(this.orgRevokedUsers); - if ( - groupsCanManage.length === 0 && - usersCanManage.length === 0 && - c.node.id !== Unassigned - ) { - c.node.addAccess = true; - this.showAddAccessToggle = true; - } else { - c.node.addAccess = false; - } - return c.node; - }); - } else { - mappedCollections = collections.map((c: TreeNode) => c.node); - } - return mappedCollections; - } - - addAccessToggle(e: any) { + addAccessToggle(e: AddAccessStatusType) { this.addAccessStatus$.next(e); } @@ -758,9 +714,17 @@ export class VaultComponent implements OnInit, OnDestroy { } else if (event.type === "copyField") { await this.copy(event.item, event.field); } else if (event.type === "editCollection") { - await this.editCollection(event.item, CollectionDialogTabType.Info, event.readonly); + await this.editCollection( + event.item as CollectionAdminView, + CollectionDialogTabType.Info, + event.readonly, + ); } else if (event.type === "viewCollectionAccess") { - await this.editCollection(event.item, CollectionDialogTabType.Access, event.readonly); + await this.editCollection( + event.item as CollectionAdminView, + CollectionDialogTabType.Access, + event.readonly, + ); } else if (event.type === "bulkEditCollectionAccess") { await this.bulkEditCollectionAccess(event.items, this.organization); } else if (event.type === "assignToCollections") { @@ -1273,7 +1237,7 @@ export class VaultComponent implements OnInit, OnDestroy { } async editCollection( - c: CollectionView, + c: CollectionAdminView, tab: CollectionDialogTabType, readonly: boolean, ): Promise { @@ -1283,7 +1247,7 @@ export class VaultComponent implements OnInit, OnDestroy { organizationId: this.organization?.id, initialTab: tab, readonly: readonly, - isAddAccessCollection: c.addAccess, + isAddAccessCollection: c.unmanaged, limitNestedCollections: !this.organization.canEditAnyCollection( this.flexibleCollectionsV1Enabled, ), diff --git a/apps/web/src/app/vault/utils/collection-utils.ts b/apps/web/src/app/vault/utils/collection-utils.ts index ce620498bc..cb995fd8ff 100644 --- a/apps/web/src/app/vault/utils/collection-utils.ts +++ b/apps/web/src/app/vault/utils/collection-utils.ts @@ -44,6 +44,7 @@ function cloneCollection( cloned.groups = [...collection.groups]; cloned.users = [...collection.users]; cloned.assigned = collection.assigned; + cloned.unmanaged = collection.unmanaged; } else { cloned = new CollectionView(); } diff --git a/libs/common/src/admin-console/models/domain/organization.ts b/libs/common/src/admin-console/models/domain/organization.ts index 752b792844..a4cadcd392 100644 --- a/libs/common/src/admin-console/models/domain/organization.ts +++ b/libs/common/src/admin-console/models/domain/organization.ts @@ -203,6 +203,11 @@ export class Organization { ); } + canEditUnmanagedCollections() { + // Any admin or custom user with editAnyCollection permission can edit unmanaged collections + return this.isAdmin || this.permissions.editAnyCollection; + } + canEditUnassignedCiphers(restrictProviderAccessFlagEnabled: boolean) { if (this.isProviderUser) { return !restrictProviderAccessFlagEnabled; diff --git a/libs/common/src/vault/models/domain/collection.spec.ts b/libs/common/src/vault/models/domain/collection.spec.ts index 9ec1d34ff7..ecbb1df083 100644 --- a/libs/common/src/vault/models/domain/collection.spec.ts +++ b/libs/common/src/vault/models/domain/collection.spec.ts @@ -64,7 +64,6 @@ describe("Collection", () => { const view = await collection.decrypt(key); expect(view).toEqual({ - addAccess: false, externalId: "extId", hidePasswords: false, id: "id", diff --git a/libs/common/src/vault/models/response/collection.response.ts b/libs/common/src/vault/models/response/collection.response.ts index ac4781df71..19dbe2acf2 100644 --- a/libs/common/src/vault/models/response/collection.response.ts +++ b/libs/common/src/vault/models/response/collection.response.ts @@ -42,10 +42,12 @@ export class CollectionDetailsResponse extends CollectionResponse { export class CollectionAccessDetailsResponse extends CollectionDetailsResponse { groups: SelectionReadOnlyResponse[] = []; users: SelectionReadOnlyResponse[] = []; + unmanaged: boolean; constructor(response: any) { super(response); this.assigned = this.getResponseProperty("Assigned") || false; + this.unmanaged = this.getResponseProperty("Unmanaged") || false; const groups = this.getResponseProperty("Groups"); if (groups != null) { diff --git a/libs/common/src/vault/models/view/collection.view.ts b/libs/common/src/vault/models/view/collection.view.ts index f5f9cb60b7..3a147f67cb 100644 --- a/libs/common/src/vault/models/view/collection.view.ts +++ b/libs/common/src/vault/models/view/collection.view.ts @@ -17,7 +17,6 @@ export class CollectionView implements View, ITreeNodeObject { readOnly: boolean = null; hidePasswords: boolean = null; manage: boolean = null; - addAccess: boolean = false; assigned: boolean = null; constructor(c?: Collection | CollectionAccessDetailsResponse) {