From b4f481863533f68418924b9e8df05e2127a0c9bb Mon Sep 17 00:00:00 2001 From: Shane Melton Date: Tue, 14 May 2024 08:24:51 -0700 Subject: [PATCH] [PM-7883] Fix Collection Dialog Component (#9088) * [PM-7883] Cleanup/refactor collection-dialog.component - Add new limitNestedCollections option - Remove redundant calls to collectionService and collectionAdminService - Adjust deleted parent logic to account for users that cannot ViewAllCollections * [PM-7883] Ensure collection management setting is considered when limiting nested collections in the org vault --- .../collection-dialog.component.ts | 69 +++++++++---------- .../vault/individual-vault/vault.component.ts | 9 ++- .../app/vault/org-vault/vault.component.ts | 6 ++ 3 files changed, 46 insertions(+), 38 deletions(-) diff --git a/apps/web/src/app/vault/components/collection-dialog/collection-dialog.component.ts b/apps/web/src/app/vault/components/collection-dialog/collection-dialog.component.ts index a0d7617ce83..f386665186f 100644 --- a/apps/web/src/app/vault/components/collection-dialog/collection-dialog.component.ts +++ b/apps/web/src/app/vault/components/collection-dialog/collection-dialog.component.ts @@ -3,7 +3,6 @@ import { ChangeDetectorRef, Component, Inject, OnDestroy, OnInit } from "@angula import { AbstractControl, FormBuilder, Validators } from "@angular/forms"; import { combineLatest, - from, map, Observable, of, @@ -23,7 +22,6 @@ import { ConfigService } from "@bitwarden/common/platform/abstractions/config/co import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { Utils } from "@bitwarden/common/platform/misc/utils"; -import { CollectionService } from "@bitwarden/common/vault/abstractions/collection.service"; import { CollectionResponse } from "@bitwarden/common/vault/models/response/collection.response"; import { CollectionView } from "@bitwarden/common/vault/models/view/collection.view"; import { BitValidators, DialogService } from "@bitwarden/components"; @@ -56,7 +54,10 @@ export interface CollectionDialogParams { initialTab?: CollectionDialogTabType; parentCollectionId?: string; showOrgSelector?: boolean; - collectionIds?: string[]; + /** + * Flag to limit the nested collections to only those the user has explicit CanManage access too. + */ + limitNestedCollections?: boolean; readonly?: boolean; } @@ -85,7 +86,7 @@ export class CollectionDialogComponent implements OnInit, OnDestroy { protected tabIndex: CollectionDialogTabType; protected loading = true; protected organization?: Organization; - protected collection?: CollectionView; + protected collection?: CollectionAdminView; protected nestOptions: CollectionView[] = []; protected accessItems: AccessItemView[] = []; protected deletedParentName: string | undefined; @@ -107,7 +108,6 @@ export class CollectionDialogComponent implements OnInit, OnDestroy { private organizationService: OrganizationService, private groupService: GroupService, private collectionAdminService: CollectionAdminService, - private collectionService: CollectionService, private i18nService: I18nService, private platformUtilsService: PlatformUtilsService, private organizationUserService: OrganizationUserService, @@ -124,7 +124,7 @@ export class CollectionDialogComponent implements OnInit, OnDestroy { this.showOrgSelector = true; this.formGroup.controls.selectedOrg.valueChanges .pipe(takeUntil(this.destroy$)) - .subscribe((id) => this.loadOrg(id, this.params.collectionIds)); + .subscribe((id) => this.loadOrg(id)); this.organizations$ = this.organizationService.organizations$.pipe( first(), map((orgs) => @@ -138,11 +138,11 @@ export class CollectionDialogComponent implements OnInit, OnDestroy { } else { // Opened from the org vault this.formGroup.patchValue({ selectedOrg: this.params.organizationId }); - await this.loadOrg(this.params.organizationId, this.params.collectionIds); + await this.loadOrg(this.params.organizationId); } } - async loadOrg(orgId: string, collectionIds: string[]) { + async loadOrg(orgId: string) { const organization$ = this.organizationService .get$(orgId) .pipe(shareReplay({ refCount: true, bufferSize: 1 })); @@ -158,28 +158,14 @@ export class CollectionDialogComponent implements OnInit, OnDestroy { combineLatest({ organization: organization$, collections: this.collectionAdminService.getAll(orgId), - collectionDetails: this.params.collectionId - ? from(this.collectionAdminService.get(orgId, this.params.collectionId)) - : of(null), groups: groups$, // Collection(s) needed to map readonlypermission for (potential) access selector disabled state users: this.organizationUserService.getAllUsers(orgId, { includeCollections: true }), - collection: this.params.collectionId - ? this.collectionService.get(this.params.collectionId) - : of(null), flexibleCollectionsV1: this.flexibleCollectionsV1Enabled$, }) .pipe(takeUntil(this.formGroup.controls.selectedOrg.valueChanges), takeUntil(this.destroy$)) .subscribe( - ({ - organization, - collections, - collectionDetails, - groups, - users, - collection, - flexibleCollectionsV1, - }) => { + ({ organization, collections: allCollections, groups, users, flexibleCollectionsV1 }) => { this.organization = organization; this.accessItems = [].concat( groups.map((group) => mapGroupToAccessItemView(group, this.collectionId)), @@ -189,37 +175,48 @@ export class CollectionDialogComponent implements OnInit, OnDestroy { // Force change detection to update the access selector's items this.changeDetectorRef.detectChanges(); - if (collectionIds) { - collections = collections.filter((c) => collectionIds.includes(c.id)); - } + this.nestOptions = this.params.limitNestedCollections + ? allCollections.filter((c) => c.manage) + : allCollections; if (this.params.collectionId) { - this.collection = collections.find((c) => c.id === this.collectionId); - this.nestOptions = collections.filter((c) => c.id !== this.collectionId); + this.collection = allCollections.find((c) => c.id === this.collectionId); + // Ensure we don't allow nesting the current collection within itself + this.nestOptions = this.nestOptions.filter((c) => c.id !== this.collectionId); if (!this.collection) { throw new Error("Could not find collection to edit."); } - const { name, parent } = parseName(this.collection); - if (parent !== undefined && !this.nestOptions.find((c) => c.name === parent)) { - this.deletedParentName = parent; + // Parse the name to find its parent name + const { name, parent: parentName } = parseName(this.collection); + + // Determine if the user can see/select the parent collection + if (parentName !== undefined) { + if ( + this.organization.canViewAllCollections && + !allCollections.find((c) => c.name === parentName) + ) { + // The user can view all collections, but the parent was not found -> assume it has been deleted + this.deletedParentName = parentName; + } else if (!this.nestOptions.find((c) => c.name === parentName)) { + // We cannot find the current parent collection in our list of options, so add a placeholder + this.nestOptions.unshift({ name: parentName } as CollectionView); + } } - const accessSelections = mapToAccessSelections(collectionDetails); + const accessSelections = mapToAccessSelections(this.collection); this.formGroup.patchValue({ name, externalId: this.collection.externalId, - parent, + parent: parentName, access: accessSelections, }); - this.collection.manage = collection?.manage ?? false; // Get manage flag from sync data collection this.showDeleteButton = !this.dialogReadonly && this.collection.canDelete(organization, flexibleCollectionsV1); } else { - this.nestOptions = collections; - const parent = collections.find((c) => c.id === this.params.parentCollectionId); + const parent = this.nestOptions.find((c) => c.id === this.params.parentCollectionId); const currentOrgUserId = users.data.find( (u) => u.userId === this.organization?.userId, )?.id; diff --git a/apps/web/src/app/vault/individual-vault/vault.component.ts b/apps/web/src/app/vault/individual-vault/vault.component.ts index 49565bdceef..f203fee0632 100644 --- a/apps/web/src/app/vault/individual-vault/vault.component.ts +++ b/apps/web/src/app/vault/individual-vault/vault.component.ts @@ -650,7 +650,7 @@ export class VaultComponent implements OnInit, OnDestroy { .sort(Utils.getSortFunction(this.i18nService, "name"))[0].id, parentCollectionId: this.filter.collectionId, showOrgSelector: true, - collectionIds: this.allCollections.map((c) => c.id), + limitNestedCollections: true, }, }); const result = await lastValueFrom(dialog.closed); @@ -666,7 +666,12 @@ export class VaultComponent implements OnInit, OnDestroy { async editCollection(c: CollectionView, tab: CollectionDialogTabType): Promise { const dialog = openCollectionDialog(this.dialogService, { - data: { collectionId: c?.id, organizationId: c.organizationId, initialTab: tab }, + data: { + collectionId: c?.id, + organizationId: c.organizationId, + initialTab: tab, + limitNestedCollections: true, + }, }); const result = await lastValueFrom(dialog.closed); 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 64823c97548..0b16c1c78f3 100644 --- a/apps/web/src/app/vault/org-vault/vault.component.ts +++ b/apps/web/src/app/vault/org-vault/vault.component.ts @@ -1175,6 +1175,9 @@ export class VaultComponent implements OnInit, OnDestroy { data: { organizationId: this.organization?.id, parentCollectionId: this.selectedCollection?.node.id, + limitNestedCollections: !this.organization.canEditAnyCollection( + this.flexibleCollectionsV1Enabled, + ), }, }); @@ -1198,6 +1201,9 @@ export class VaultComponent implements OnInit, OnDestroy { organizationId: this.organization?.id, initialTab: tab, readonly: readonly, + limitNestedCollections: !this.organization.canEditAnyCollection( + this.flexibleCollectionsV1Enabled, + ), }, });