From 369c1edaf782fc2799362c79e16cef48aae7b178 Mon Sep 17 00:00:00 2001 From: Jordan Aasen <166539328+jaasen-livefront@users.noreply.github.com> Date: Wed, 2 Jul 2025 08:54:42 -0700 Subject: [PATCH] [PM-22376] - [Vault] [Clients] Update cipher form component to default to My Items collections (#15356) * fix tests * remove unused code * fix storybook * fix storybook * cleanup * move observable to function. update tests * fix type error * move call to getDefaultCollectionId * fix test --- .../src/cipher-form/cipher-form.stories.ts | 8 +++ .../item-details-section.component.spec.ts | 53 ++++++++++++-- .../item-details-section.component.ts | 71 ++++++++++++++++--- 3 files changed, 115 insertions(+), 17 deletions(-) diff --git a/libs/vault/src/cipher-form/cipher-form.stories.ts b/libs/vault/src/cipher-form/cipher-form.stories.ts index f46eb457e30..25494350dc3 100644 --- a/libs/vault/src/cipher-form/cipher-form.stories.ts +++ b/libs/vault/src/cipher-form/cipher-form.stories.ts @@ -19,6 +19,7 @@ import { ViewCacheService } from "@bitwarden/angular/platform/view-cache"; import { NudgeStatus, NudgesService } from "@bitwarden/angular/vault"; import { AuditService } from "@bitwarden/common/abstractions/audit.service"; import { EventCollectionService } from "@bitwarden/common/abstractions/event/event-collection.service"; +import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { AutofillSettingsServiceAbstraction } from "@bitwarden/common/autofill/services/autofill-settings.service"; @@ -243,6 +244,7 @@ export default { provide: ConfigService, useValue: { getFeatureFlag: () => Promise.resolve(false), + getFeatureFlag$: () => new BehaviorSubject(false), }, }, { @@ -253,6 +255,12 @@ export default { }, }, }, + { + provide: PolicyService, + useValue: { + policiesByType$: new BehaviorSubject([]), + }, + }, ], }), componentWrapperDecorator( 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 12fba0c7409..99c377a0873 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 @@ -3,18 +3,24 @@ import { ComponentFixture, fakeAsync, TestBed, tick } from "@angular/core/testin import { ReactiveFormsModule } from "@angular/forms"; import { By } from "@angular/platform-browser"; import { mock, MockProxy } from "jest-mock-extended"; -import { BehaviorSubject } from "rxjs"; +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 { 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"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { SelectComponent } from "@bitwarden/components"; -import { CipherFormConfig } from "../../abstractions/cipher-form-config.service"; +import { + CipherFormConfig, + OptionalInitialValues, +} from "../../abstractions/cipher-form-config.service"; import { CipherFormContainer } from "../../cipher-form-container"; import { ItemDetailsSectionComponent } from "./item-details-section.component"; @@ -48,6 +54,8 @@ describe("ItemDetailsSectionComponent", () => { let fixture: ComponentFixture; let cipherFormProvider: MockProxy; let i18nService: MockProxy; + let mockConfigService: MockProxy; + let mockPolicyService: MockProxy; const activeAccount$ = new BehaviorSubject<{ email: string }>({ email: "test@example.com" }); const getInitialCipherView = jest.fn(() => null); @@ -66,12 +74,19 @@ describe("ItemDetailsSectionComponent", () => { compare: (a: string, b: string) => a.localeCompare(b), } as Intl.Collator; + mockConfigService = mock(); + mockConfigService.getFeatureFlag$.mockReturnValue(of(true)); + mockPolicyService = mock(); + mockPolicyService.policiesByType$.mockReturnValue(of([])); + await TestBed.configureTestingModule({ imports: [ItemDetailsSectionComponent, CommonModule, ReactiveFormsModule], providers: [ { provide: CipherFormContainer, useValue: cipherFormProvider }, { provide: I18nService, useValue: i18nService }, { provide: AccountService, useValue: { activeAccount$ } }, + { provide: ConfigService, useValue: mockConfigService }, + { provide: PolicyService, useValue: mockPolicyService }, ], }).compileComponents(); @@ -369,7 +384,7 @@ describe("ItemDetailsSectionComponent", () => { expect(collectionSelect).toBeNull(); }); - it("should enable/show collection control when an organization is selected", async () => { + it("should enable/show collection control when an organization is selected", fakeAsync(() => { component.config.organizationDataOwnershipDisabled = true; component.config.organizations = [{ id: "org1" } as Organization]; component.config.collections = [ @@ -378,12 +393,12 @@ describe("ItemDetailsSectionComponent", () => { ]; fixture.detectChanges(); - await fixture.whenStable(); + tick(); component.itemDetailsForm.controls.organizationId.setValue("org1"); + tick(); fixture.detectChanges(); - await fixture.whenStable(); const collectionSelect = fixture.nativeElement.querySelector( "bit-multi-select[formcontrolname='collectionIds']", @@ -391,7 +406,7 @@ describe("ItemDetailsSectionComponent", () => { expect(component.itemDetailsForm.controls.collectionIds.enabled).toBe(true); expect(collectionSelect).not.toBeNull(); - }); + })); it("should set collectionIds to originalCipher collections on first load", async () => { component.config.mode = "clone"; @@ -488,6 +503,9 @@ describe("ItemDetailsSectionComponent", () => { component.itemDetailsForm.controls.organizationId.setValue("org1"); + fixture.detectChanges(); + await fixture.whenStable(); + expect(component["collectionOptions"].map((c) => c.id)).toEqual(["col1", "col2", "col3"]); }); }); @@ -548,4 +566,27 @@ describe("ItemDetailsSectionComponent", () => { expect(label).toBe("org1"); }); }); + + describe("getDefaultCollectionId", () => { + it("returns matching default when flag & policy match", async () => { + const def = createMockCollection("def1", "Def", "orgA"); + component.config.collections = [def] as CollectionView[]; + component.config.initialValues = { collectionIds: [] } as OptionalInitialValues; + mockConfigService.getFeatureFlag.mockResolvedValue(true); + mockPolicyService.policiesByType$.mockReturnValue(of([{ organizationId: "orgA" } as Policy])); + + const id = await (component as any).getDefaultCollectionId("orgA"); + expect(id).toEqual("def1"); + }); + + it("returns undefined when no default found", async () => { + component.config.collections = [createMockCollection("c1", "C1", "orgB")] as CollectionView[]; + component.config.initialValues = { collectionIds: [] } as OptionalInitialValues; + mockConfigService.getFeatureFlag.mockResolvedValue(true); + mockPolicyService.policiesByType$.mockReturnValue(of([{ organizationId: "orgA" } as Policy])); + + const result = await (component as any).getDefaultCollectionId("orgA"); + expect(result).toBeUndefined(); + }); + }); }); 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 1d30edf27e0..1064980050f 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 @@ -4,15 +4,19 @@ import { CommonModule } from "@angular/common"; import { Component, DestroyRef, Input, OnInit } from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; import { FormBuilder, FormControl, ReactiveFormsModule, Validators } from "@angular/forms"; -import { concatMap, map } from "rxjs"; +import { concatMap, firstValueFrom, map } 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 { CollectionView } from "@bitwarden/admin-console/common"; +import { CollectionTypes, CollectionView } from "@bitwarden/admin-console/common"; import { JslibModule } from "@bitwarden/angular/jslib.module"; -import { OrganizationUserType } from "@bitwarden/common/admin-console/enums"; +import { PolicyService } from "@bitwarden/common/admin-console/abstractions/policy/policy.service.abstraction"; +import { OrganizationUserType, PolicyType } from "@bitwarden/common/admin-console/enums"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; +import { getUserId } from "@bitwarden/common/auth/services/account.service"; +import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; import { Utils } from "@bitwarden/common/platform/misc/utils"; import { CollectionId, OrganizationId } from "@bitwarden/common/types/guid"; @@ -124,6 +128,8 @@ export class ItemDetailsSectionComponent implements OnInit { private i18nService: I18nService, private destroyRef: DestroyRef, private accountService: AccountService, + private configService: ConfigService, + private policyService: PolicyService, ) { this.cipherFormContainer.registerChildForm("itemDetails", this.itemDetailsForm); this.itemDetailsForm.valueChanges @@ -200,30 +206,61 @@ export class ItemDetailsSectionComponent implements OnInit { if (prefillCipher) { await this.initFromExistingCipher(prefillCipher); } else { + const orgId = this.initialValues?.organizationId; this.itemDetailsForm.setValue({ name: this.initialValues?.name || "", - organizationId: this.initialValues?.organizationId || this.defaultOwner, + organizationId: orgId || this.defaultOwner, folderId: this.initialValues?.folderId || null, collectionIds: [], favorite: false, }); - await this.updateCollectionOptions(this.initialValues?.collectionIds || []); + await this.updateCollectionOptions(this.initialValues?.collectionIds); } - if (!this.allowOwnershipChange) { this.itemDetailsForm.controls.organizationId.disable(); } - this.itemDetailsForm.controls.organizationId.valueChanges .pipe( takeUntilDestroyed(this.destroyRef), - concatMap(async () => { - await this.updateCollectionOptions(); - }), + concatMap(async () => await this.updateCollectionOptions()), ) .subscribe(); } + /** + * Gets the default collection IDs for the selected organization. + * Returns null if any of the following apply: + * - the feature flag is disabled + * - no org is currently selected + * - the selected org doesn't have the "no private data policy" enabled + */ + private async getDefaultCollectionId(orgId?: OrganizationId) { + if (!orgId) { + return; + } + const isFeatureEnabled = await this.configService.getFeatureFlag( + FeatureFlag.CreateDefaultLocation, + ); + if (!isFeatureEnabled) { + return; + } + const userId = await firstValueFrom(this.accountService.activeAccount$.pipe(getUserId)); + const selectedOrgHasPolicyEnabled = ( + await firstValueFrom( + this.policyService.policiesByType$(PolicyType.OrganizationDataOwnership, userId), + ) + ).find((p) => p.organizationId); + if (!selectedOrgHasPolicyEnabled) { + return; + } + const defaultUserCollection = this.collections.find( + (c) => c.organizationId === orgId && c.type === CollectionTypes.DefaultUserCollection, + ); + // If the user was added after the policy was enabled as they will not have any private data + // and will not have a default collection. + return defaultUserCollection?.id; + } + private async initFromExistingCipher(prefillCipher: CipherView) { const { name, folderId, collectionIds } = prefillCipher; @@ -332,6 +369,11 @@ export class ItemDetailsSectionComponent implements OnInit { // Non-admins can only select assigned collections that are not read only. (Non-AC) return c.assigned && !c.readOnly; }) + .sort((a, b) => { + const aIsDefaultCollection = a.type === CollectionTypes.DefaultUserCollection ? -1 : 0; + const bIsDefaultCollection = b.type === CollectionTypes.DefaultUserCollection ? -1 : 0; + return aIsDefaultCollection - bIsDefaultCollection; + }) .map((c) => ({ id: c.id, name: c.name, @@ -349,10 +391,17 @@ export class ItemDetailsSectionComponent implements OnInit { return; } - if (startingSelection.length > 0) { + if (startingSelection.filter(Boolean).length > 0) { collectionsControl.setValue( this.collectionOptions.filter((c) => startingSelection.includes(c.id as CollectionId)), ); + } else { + const defaultCollectionId = await this.getDefaultCollectionId(orgId); + if (defaultCollectionId) { + collectionsControl.setValue( + this.collectionOptions.filter((c) => c.id === defaultCollectionId), + ); + } } } }