From c4ee2fdae2e6f3d3adbe6569d8739337344b0dd1 Mon Sep 17 00:00:00 2001 From: Nick Krantz <125900171+nick-livefront@users.noreply.github.com> Date: Mon, 29 Sep 2025 13:06:36 -0500 Subject: [PATCH] [PM-25982] Assign to Collections - My Items (#16591) * update cipher form to exclude my items collections * handle default collections for assign to collections and bulk * account for every returning true for empty arrays --- .../item-details-section.component.spec.ts | 41 +++++++- .../item-details-section.component.ts | 18 ++++ .../assign-collections.component.spec.ts | 96 ++++++++++++++++++- .../assign-collections.component.ts | 52 +++++++++- 4 files changed, 199 insertions(+), 8 deletions(-) diff --git a/libs/vault/src/cipher-form/components/item-details/item-details-section.component.spec.ts b/libs/vault/src/cipher-form/components/item-details/item-details-section.component.spec.ts index c41e58f679e..4da299ed039 100644 --- a/libs/vault/src/cipher-form/components/item-details/item-details-section.component.spec.ts +++ b/libs/vault/src/cipher-form/components/item-details/item-details-section.component.spec.ts @@ -7,7 +7,7 @@ import { BehaviorSubject, of } from "rxjs"; // This import has been flagged as unallowed for this class. It may be involved in a circular dependency loop. // eslint-disable-next-line no-restricted-imports -import { CollectionTypes, CollectionView } from "@bitwarden/admin-console/common"; +import { CollectionType, CollectionTypes, CollectionView } from "@bitwarden/admin-console/common"; import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { Policy } from "@bitwarden/common/admin-console/models/domain/policy"; @@ -33,6 +33,7 @@ const createMockCollection = ( organizationId: string, readOnly = false, canEdit = true, + type: CollectionType = CollectionTypes.DefaultUserCollection, ): CollectionView => { const cv = new CollectionView({ name, @@ -41,7 +42,7 @@ const createMockCollection = ( }); cv.readOnly = readOnly; cv.manage = true; - cv.type = CollectionTypes.DefaultUserCollection; + cv.type = type; cv.externalId = ""; cv.hidePasswords = false; cv.assigned = true; @@ -519,6 +520,42 @@ describe("ItemDetailsSectionComponent", () => { expect(component["collectionOptions"].map((c) => c.id)).toEqual(["col1", "col2", "col3"]); }); + + it("should exclude default collections when the cipher is only assigned to shared collections", async () => { + component.config.admin = false; + component.config.organizationDataOwnershipDisabled = true; + component.config.organizations = [{ id: "org1" } as Organization]; + component.config.collections = new Array(4) + .fill(null) + .map((_, i) => i + 1) + .map( + (i) => + createMockCollection( + `col${i}`, + `Collection ${i}`, + "org1", + false, + false, + i < 4 ? CollectionTypes.SharedCollection : CollectionTypes.DefaultUserCollection, + ) as CollectionView, + ); + component.originalCipherView = { + name: "cipher1", + organizationId: "org1", + folderId: "folder1", + collectionIds: ["col2", "col3"], + favorite: true, + } as CipherView; + fixture.detectChanges(); + await fixture.whenStable(); + + component.itemDetailsForm.controls.organizationId.setValue("org1"); + + fixture.detectChanges(); + await fixture.whenStable(); + + expect(component["collectionOptions"].map((c) => c.id)).toEqual(["col1", "col2", "col3"]); + }); }); describe("readonlyCollections", () => { diff --git a/libs/vault/src/cipher-form/components/item-details/item-details-section.component.ts b/libs/vault/src/cipher-form/components/item-details/item-details-section.component.ts index 6af2fa19e26..ced6c809724 100644 --- a/libs/vault/src/cipher-form/components/item-details/item-details-section.component.ts +++ b/libs/vault/src/cipher-form/components/item-details/item-details-section.component.ts @@ -406,6 +406,17 @@ export class ItemDetailsSectionComponent implements OnInit { this.showCollectionsControl = true; } + /** + * Determine if the the cipher is only assigned to shared collections. + * i.e. The cipher is not assigned to a default collections. + * Note: `.every` will return true for an empty array + */ + const cipherIsOnlyInOrgCollections = + (this.originalCipherView?.collectionIds ?? []).length > 0 && + this.originalCipherView.collectionIds.every( + (cId) => + this.collections.find((c) => c.id === cId)?.type === CollectionTypes.SharedCollection, + ); this.collectionOptions = this.collections .filter((c) => { // The collection belongs to the organization @@ -423,10 +434,17 @@ export class ItemDetailsSectionComponent implements OnInit { return true; } + // When the cipher is only assigned to shared collections, do not allow a user to + // move it back to a default collection. Exclude the default collection from the list. + if (cipherIsOnlyInOrgCollections && c.type === CollectionTypes.DefaultUserCollection) { + return false; + } + // Non-admins can only select assigned collections that are not read only. (Non-AC) return c.assigned && !c.readOnly; }) .sort((a, b) => { + // Show default collection first const aIsDefaultCollection = a.type === CollectionTypes.DefaultUserCollection ? -1 : 0; const bIsDefaultCollection = b.type === CollectionTypes.DefaultUserCollection ? -1 : 0; return aIsDefaultCollection - bIsDefaultCollection; diff --git a/libs/vault/src/components/assign-collections.component.spec.ts b/libs/vault/src/components/assign-collections.component.spec.ts index e54bada30ba..414613e67d8 100644 --- a/libs/vault/src/components/assign-collections.component.spec.ts +++ b/libs/vault/src/components/assign-collections.component.spec.ts @@ -5,7 +5,11 @@ import { of } from "rxjs"; // This import has been flagged as unallowed for this class. It may be involved in a circular dependency loop. // eslint-disable-next-line no-restricted-imports -import { CollectionService, CollectionView } from "@bitwarden/admin-console/common"; +import { + CollectionService, + CollectionTypes, + CollectionView, +} from "@bitwarden/admin-console/common"; import { OrganizationService } from "@bitwarden/common/admin-console/abstractions/organization/organization.service.abstraction"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; @@ -34,7 +38,6 @@ describe("AssignCollectionsComponent", () => { organizationId: "org-id" as OrganizationId, name: "Editable Collection", }); - editCollection.readOnly = false; editCollection.manage = true; @@ -52,6 +55,24 @@ describe("AssignCollectionsComponent", () => { }); readOnlyCollection2.readOnly = true; + const sharedCollection = new CollectionView({ + id: "shared-collection-id" as CollectionId, + organizationId: "org-id" as OrganizationId, + name: "Shared Collection", + }); + sharedCollection.readOnly = false; + sharedCollection.assigned = true; + sharedCollection.type = CollectionTypes.SharedCollection; + + const defaultCollection = new CollectionView({ + id: "default-collection-id" as CollectionId, + organizationId: "org-id" as OrganizationId, + name: "Default Collection", + }); + defaultCollection.readOnly = false; + defaultCollection.manage = true; + defaultCollection.type = CollectionTypes.DefaultUserCollection; + const params = { organizationId: "org-id" as OrganizationId, ciphers: [ @@ -116,4 +137,75 @@ describe("AssignCollectionsComponent", () => { ]); }); }); + + describe("default collections", () => { + const cipher1 = new CipherView(); + cipher1.id = "cipher-id-1"; + cipher1.collectionIds = [editCollection.id, sharedCollection.id]; + cipher1.edit = true; + + const cipher2 = new CipherView(); + cipher2.id = "cipher-id-2"; + cipher2.collectionIds = [defaultCollection.id]; + cipher2.edit = true; + + const cipher3 = new CipherView(); + cipher3.id = "cipher-id-3"; + cipher3.collectionIds = [defaultCollection.id]; + cipher3.edit = true; + + const cipher4 = new CipherView(); + cipher4.id = "cipher-id-4"; + cipher4.collectionIds = []; + cipher4.edit = true; + + it('does not show the "Default Collection" if any cipher is in a shared collection', async () => { + component.params = { + ...component.params, + ciphers: [cipher1, cipher2], + availableCollections: [editCollection, sharedCollection, defaultCollection], + }; + + await component.ngOnInit(); + fixture.detectChanges(); + + expect(component["availableCollections"].map((c) => c.id)).toEqual([ + editCollection.id, + sharedCollection.id, + ]); + }); + + it('shows the "Default Collection" if no ciphers are in a shared collection', async () => { + component.params = { + ...component.params, + ciphers: [cipher2, cipher3], + availableCollections: [editCollection, sharedCollection, defaultCollection], + }; + + await component.ngOnInit(); + fixture.detectChanges(); + + expect(component["availableCollections"].map((c) => c.id)).toEqual([ + editCollection.id, + sharedCollection.id, + defaultCollection.id, + ]); + }); + + it('shows the "Default Collection" for singular cipher', async () => { + component.params = { + ...component.params, + ciphers: [cipher4], + availableCollections: [readOnlyCollection1, sharedCollection, defaultCollection], + }; + + await component.ngOnInit(); + fixture.detectChanges(); + + expect(component["availableCollections"].map((c) => c.id)).toEqual([ + sharedCollection.id, + defaultCollection.id, + ]); + }); + }); }); diff --git a/libs/vault/src/components/assign-collections.component.ts b/libs/vault/src/components/assign-collections.component.ts index b2bd6e31ee5..453ba93f380 100644 --- a/libs/vault/src/components/assign-collections.component.ts +++ b/libs/vault/src/components/assign-collections.component.ts @@ -26,7 +26,11 @@ import { // This import has been flagged as unallowed for this class. It may be involved in a circular dependency loop. // eslint-disable-next-line no-restricted-imports -import { CollectionService, CollectionView } from "@bitwarden/admin-console/common"; +import { + CollectionService, + CollectionTypes, + CollectionView, +} from "@bitwarden/admin-console/common"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { getOrganizationById, @@ -311,9 +315,19 @@ export class AssignCollectionsComponent implements OnInit, OnDestroy, AfterViewI await this.setReadOnlyCollectionNames(); + const canAccessDefaultCollection = this.canAccessDefaultCollection( + this.params.availableCollections, + ); + this.availableCollections = this.params.availableCollections .filter((collection) => { - return collection.canEditItems(org); + if (canAccessDefaultCollection) { + return collection.canEditItems(org); + } + + return ( + collection.canEditItems(org) && collection.type !== CollectionTypes.DefaultUserCollection + ); }) .map((c) => ({ icon: "bwi-collection-shared", @@ -447,8 +461,16 @@ export class AssignCollectionsComponent implements OnInit, OnDestroy, AfterViewI const org = organizations.find((o) => o.id === orgId); this.orgName = org.name; - return collections.filter((c) => { - return c.organizationId === orgId && !c.readOnly; + const orgCollections = collections.filter((c) => c.organizationId === orgId); + + const canAccessDefaultCollection = this.canAccessDefaultCollection(collections); + + return orgCollections.filter((c) => { + if (canAccessDefaultCollection) { + return !c.readOnly; + } + + return !c.readOnly && c.type !== CollectionTypes.DefaultUserCollection; }); }), shareReplay({ refCount: true, bufferSize: 1 }), @@ -536,4 +558,26 @@ export class AssignCollectionsComponent implements OnInit, OnDestroy, AfterViewI }) .map((c) => c.name); } + + /** + * Determines if the ciphers to be assigned can be assigned to the Default Collection. + * When false, the Default Collections should be excluded from the list of available collections. + */ + private canAccessDefaultCollection(collections: CollectionView[]): boolean { + const collectionsObject = Object.fromEntries(collections.map((c) => [c.id, c])); + + const allCiphersUnassignedOrInDefault = this.params.ciphers.every( + (cipher) => + !cipher.collectionIds.length || + cipher.collectionIds.some( + (cId) => collectionsObject[cId]?.type === CollectionTypes.DefaultUserCollection, + ), + ); + + // When all ciphers are either: + // - unassigned + // - already in a Default Collection + // then the Default Collection can be shown. + return allCiphersUnassignedOrInDefault; + } }