From 83b8a0b22faa1f9a87ae546609592b52d3e05492 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Mon, 2 Dec 2024 22:58:52 -0600 Subject: [PATCH 01/17] remove todo --- libs/vault/src/cipher-form/cipher-form-container.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/libs/vault/src/cipher-form/cipher-form-container.ts b/libs/vault/src/cipher-form/cipher-form-container.ts index 8010cf260df..6cee58d2448 100644 --- a/libs/vault/src/cipher-form/cipher-form-container.ts +++ b/libs/vault/src/cipher-form/cipher-form-container.ts @@ -12,7 +12,6 @@ import { SshKeySectionComponent } from "./components/sshkey-section/sshkey-secti /** * The complete form for a cipher. Includes all the sub-forms from their respective section components. - * TODO: Add additional form sections as they are implemented. */ export type CipherForm = { itemDetails?: ItemDetailsSectionComponent["itemDetailsForm"]; From 69753d2a83327baf5c9f9e8e73a330844135adba Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Mon, 2 Dec 2024 23:00:54 -0600 Subject: [PATCH 02/17] Retrieve cache cipher for add-edit form --- .../src/cipher-form/cipher-form-container.ts | 5 +++ .../components/cipher-form.component.ts | 42 +++++++++++++++++++ .../default-cipher-form-cache.service.ts | 32 ++++++++++++++ 3 files changed, 79 insertions(+) create mode 100644 libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts diff --git a/libs/vault/src/cipher-form/cipher-form-container.ts b/libs/vault/src/cipher-form/cipher-form-container.ts index 6cee58d2448..55411a1cd4e 100644 --- a/libs/vault/src/cipher-form/cipher-form-container.ts +++ b/libs/vault/src/cipher-form/cipher-form-container.ts @@ -54,4 +54,9 @@ export abstract class CipherFormContainer { * @param updateFn - A function that takes the current cipherView and returns the updated cipherView */ abstract patchCipher(updateFn: (current: CipherView) => CipherView): void; + + /** + * Returns initial values for the input properties of properties + */ + abstract getInitialCipherView(): CipherView | null; } diff --git a/libs/vault/src/cipher-form/components/cipher-form.component.ts b/libs/vault/src/cipher-form/components/cipher-form.component.ts index d1bbbef0910..8933a451313 100644 --- a/libs/vault/src/cipher-form/components/cipher-form.component.ts +++ b/libs/vault/src/cipher-form/components/cipher-form.component.ts @@ -36,6 +36,7 @@ import { import { CipherFormConfig } from "../abstractions/cipher-form-config.service"; import { CipherFormService } from "../abstractions/cipher-form.service"; import { CipherForm, CipherFormContainer } from "../cipher-form-container"; +import { CipherFormCacheService } from "../services/default-cipher-form-cache.service"; import { AdditionalOptionsSectionComponent } from "./additional-options/additional-options-section.component"; import { CardDetailsSectionComponent } from "./card-details-section/card-details-section.component"; @@ -53,6 +54,9 @@ import { SshKeySectionComponent } from "./sshkey-section/sshkey-section.componen provide: CipherFormContainer, useExisting: forwardRef(() => CipherFormComponent), }, + { + provide: CipherFormCacheService, + }, ], imports: [ AsyncActionsModule, @@ -162,6 +166,21 @@ export class CipherFormComponent implements AfterViewInit, OnInit, OnChanges, Ci */ patchCipher(updateFn: (current: CipherView) => CipherView): void { this.updatedCipherView = updateFn(this.updatedCipherView); + // Cache the updated cipher + this.cipherFormCacheService.cacheCipherView(this.updatedCipherView); + } + + /** + * Return initial values for given keys of a cipher + */ + getInitialCipherView(): CipherView { + const cachedCipherView = this.cipherFormCacheService.getCachedCipherView(); + + if (cachedCipherView) { + return cachedCipherView; + } + + return this.updatedCipherView; } /** @@ -218,16 +237,39 @@ export class CipherFormComponent implements AfterViewInit, OnInit, OnChanges, Ci } } + this.setInitialCipherFromCache(); + this.loading = false; this.formReadySubject.next(); } + /** + * Updates `updatedCipherView` based on the value from the cache. + */ + setInitialCipherFromCache() { + const cachedCipher = this.cipherFormCacheService.getCachedCipherView(); + if (cachedCipher === null) { + return; + } + + // Use the cached cipher when it matches the cipher being edited + if (this.updatedCipherView.id === cachedCipher.id) { + this.updatedCipherView = cachedCipher; + } + + // `id` is null when a cipher is being added + if (this.updatedCipherView.id === null) { + this.updatedCipherView = cachedCipher; + } + } + constructor( private formBuilder: FormBuilder, private addEditFormService: CipherFormService, private toastService: ToastService, private i18nService: I18nService, private changeDetectorRef: ChangeDetectorRef, + private cipherFormCacheService: CipherFormCacheService, ) {} /** diff --git a/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts new file mode 100644 index 00000000000..2d84e7f282e --- /dev/null +++ b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts @@ -0,0 +1,32 @@ +import { inject, Injectable } from "@angular/core"; + +import { ViewCacheService } from "@bitwarden/angular/platform/abstractions/view-cache.service"; +import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; + +const POPUP_CIPHER_CACHE_KEY = "popup-cipher-cache"; + +@Injectable() +export class CipherFormCacheService { + private popupViewCacheService: ViewCacheService = inject(ViewCacheService); + + private cipherCache = this.popupViewCacheService.signal({ + key: POPUP_CIPHER_CACHE_KEY, + initialValue: null, + deserializer: (obj) => (obj ? CipherView.fromJSON(obj) : null), + }); + + /** + * Update the cache with the new CipherView. + */ + cacheCipherView(cipherView: CipherView): void { + // Create a new shallow reference to force the signal to update + this.cipherCache.set({ ...cipherView } as CipherView); + } + + /** + * Returns the cached CipherView when available. + */ + getCachedCipherView(): CipherView | null { + return this.cipherCache(); + } +} From 7823c00abbf7896bdf3a919538917e08aba735ad Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 3 Dec 2024 13:21:32 -0600 Subject: [PATCH 03/17] user prefilled cipher for add-edit form --- .../src/cipher-form/cipher-form-container.ts | 2 +- ...ditional-options-section.component.spec.ts | 11 ++- .../additional-options-section.component.ts | 9 ++- .../autofill-options.component.spec.ts | 22 ++++-- .../autofill-options.component.ts | 5 +- .../card-details-section.component.spec.ts | 10 +-- .../card-details-section.component.ts | 10 ++- .../custom-fields.component.spec.ts | 8 +- .../custom-fields/custom-fields.component.ts | 6 +- .../components/identity/identity.component.ts | 11 ++- .../item-details-section.component.spec.ts | 75 ++++++------------- .../item-details-section.component.ts | 33 ++++---- .../login-details-section.component.spec.ts | 45 ++++++----- .../login-details-section.component.ts | 12 +-- 14 files changed, 135 insertions(+), 124 deletions(-) diff --git a/libs/vault/src/cipher-form/cipher-form-container.ts b/libs/vault/src/cipher-form/cipher-form-container.ts index 55411a1cd4e..cc439e37a13 100644 --- a/libs/vault/src/cipher-form/cipher-form-container.ts +++ b/libs/vault/src/cipher-form/cipher-form-container.ts @@ -56,7 +56,7 @@ export abstract class CipherFormContainer { abstract patchCipher(updateFn: (current: CipherView) => CipherView): void; /** - * Returns initial values for the input properties of properties + * Returns initial values for the CipherView, either from the config or the cached cipher */ abstract getInitialCipherView(): CipherView | null; } diff --git a/libs/vault/src/cipher-form/components/additional-options/additional-options-section.component.spec.ts b/libs/vault/src/cipher-form/components/additional-options/additional-options-section.component.spec.ts index 15784f1ca06..705c170933a 100644 --- a/libs/vault/src/cipher-form/components/additional-options/additional-options-section.component.spec.ts +++ b/libs/vault/src/cipher-form/components/additional-options/additional-options-section.component.spec.ts @@ -27,9 +27,12 @@ describe("AdditionalOptionsSectionComponent", () => { let passwordRepromptService: MockProxy; let passwordRepromptEnabled$: BehaviorSubject; + const getInitialCipherView = jest.fn(() => null); + beforeEach(async () => { - cipherFormProvider = mock(); + getInitialCipherView.mockClear(); + cipherFormProvider = mock({ getInitialCipherView }); passwordRepromptService = mock(); passwordRepromptEnabled$ = new BehaviorSubject(true); passwordRepromptService.enabled$ = passwordRepromptEnabled$; @@ -94,11 +97,11 @@ describe("AdditionalOptionsSectionComponent", () => { expect(component.additionalOptionsForm.disabled).toBe(true); }); - it("initializes 'additionalOptionsForm' with original cipher view values", () => { - (cipherFormProvider.originalCipherView as any) = { + it("initializes 'additionalOptionsForm' from `getInitialCipherValue`", () => { + getInitialCipherView.mockReturnValueOnce({ notes: "original notes", reprompt: 1, - } as CipherView; + } as CipherView); component.ngOnInit(); diff --git a/libs/vault/src/cipher-form/components/additional-options/additional-options-section.component.ts b/libs/vault/src/cipher-form/components/additional-options/additional-options-section.component.ts index 4d402b98e38..c3e7381c269 100644 --- a/libs/vault/src/cipher-form/components/additional-options/additional-options-section.component.ts +++ b/libs/vault/src/cipher-form/components/additional-options/additional-options-section.component.ts @@ -75,11 +75,12 @@ export class AdditionalOptionsSectionComponent implements OnInit { } ngOnInit() { - if (this.cipherFormContainer.originalCipherView) { + const prefillCipher = this.cipherFormContainer.getInitialCipherView(); + + if (prefillCipher) { this.additionalOptionsForm.patchValue({ - notes: this.cipherFormContainer.originalCipherView.notes, - reprompt: - this.cipherFormContainer.originalCipherView.reprompt === CipherRepromptType.Password, + notes: prefillCipher.notes, + reprompt: prefillCipher.reprompt === CipherRepromptType.Password, }); } diff --git a/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.spec.ts b/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.spec.ts index f1e5af2868a..ea756d17c40 100644 --- a/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.spec.ts +++ b/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.spec.ts @@ -25,9 +25,11 @@ describe("AutofillOptionsComponent", () => { let domainSettingsService: MockProxy; let autofillSettingsService: MockProxy; let platformUtilsService: MockProxy; + const getInitialCipherView = jest.fn(() => null); beforeEach(async () => { - cipherFormContainer = mock(); + getInitialCipherView.mockClear(); + cipherFormContainer = mock({ getInitialCipherView }); liveAnnouncer = mock(); platformUtilsService = mock(); domainSettingsService = mock(); @@ -107,12 +109,14 @@ describe("AutofillOptionsComponent", () => { existingLogin.uri = "https://example.com"; existingLogin.match = UriMatchStrategy.Exact; - (cipherFormContainer.originalCipherView as CipherView) = new CipherView(); - cipherFormContainer.originalCipherView.login = { + const cipher = new CipherView(); + cipher.login = { autofillOnPageLoad: true, uris: [existingLogin], } as LoginView; + getInitialCipherView.mockReturnValueOnce(cipher); + fixture.detectChanges(); expect(component.autofillOptionsForm.value.uris).toEqual([ @@ -138,12 +142,14 @@ describe("AutofillOptionsComponent", () => { existingLogin.uri = "https://example.com"; existingLogin.match = UriMatchStrategy.Exact; - (cipherFormContainer.originalCipherView as CipherView) = new CipherView(); - cipherFormContainer.originalCipherView.login = { + const cipher = new CipherView(); + cipher.login = { autofillOnPageLoad: true, uris: [existingLogin], } as LoginView; + getInitialCipherView.mockReturnValueOnce(cipher); + fixture.detectChanges(); expect(component.autofillOptionsForm.value.uris).toEqual([ @@ -159,12 +165,14 @@ describe("AutofillOptionsComponent", () => { existingLogin.uri = "https://example.com"; existingLogin.match = UriMatchStrategy.Exact; - (cipherFormContainer.originalCipherView as CipherView) = new CipherView(); - cipherFormContainer.originalCipherView.login = { + const cipher = new CipherView(); + cipher.login = { autofillOnPageLoad: true, uris: [existingLogin], } as LoginView; + getInitialCipherView.mockReturnValueOnce(cipher); + fixture.detectChanges(); expect(component.autofillOptionsForm.value.uris).toEqual([ diff --git a/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.ts b/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.ts index 40f7eeecf07..127e8de62f5 100644 --- a/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.ts +++ b/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.ts @@ -128,8 +128,9 @@ export class AutofillOptionsComponent implements OnInit { } ngOnInit() { - if (this.cipherFormContainer.originalCipherView?.login) { - this.initFromExistingCipher(this.cipherFormContainer.originalCipherView.login); + const prefillCipher = this.cipherFormContainer.getInitialCipherView(); + if (prefillCipher) { + this.initFromExistingCipher(prefillCipher.login); } else { this.initNewCipher(); } diff --git a/libs/vault/src/cipher-form/components/card-details-section/card-details-section.component.spec.ts b/libs/vault/src/cipher-form/components/card-details-section/card-details-section.component.spec.ts index e9581859b2e..39a59192985 100644 --- a/libs/vault/src/cipher-form/components/card-details-section/card-details-section.component.spec.ts +++ b/libs/vault/src/cipher-form/components/card-details-section/card-details-section.component.spec.ts @@ -20,8 +20,10 @@ describe("CardDetailsSectionComponent", () => { let registerChildFormSpy: jest.SpyInstance; let patchCipherSpy: jest.SpyInstance; + const getInitialCipherView = jest.fn(() => null); + beforeEach(async () => { - cipherFormProvider = mock(); + cipherFormProvider = mock({ getInitialCipherView }); registerChildFormSpy = jest.spyOn(cipherFormProvider, "registerChildForm"); patchCipherSpy = jest.spyOn(cipherFormProvider, "patchCipher"); @@ -94,7 +96,7 @@ describe("CardDetailsSectionComponent", () => { expect(component.cardDetailsForm.disabled).toBe(true); }); - it("initializes `cardDetailsForm` with current values", () => { + it("initializes `cardDetailsForm` from `getInitialCipherValue`", () => { const cardholderName = "Ron Burgundy"; const number = "4242 4242 4242 4242"; const code = "619"; @@ -105,9 +107,7 @@ describe("CardDetailsSectionComponent", () => { cardView.code = code; cardView.brand = "Visa"; - component.originalCipherView = { - card: cardView, - } as CipherView; + getInitialCipherView.mockReturnValueOnce({ card: cardView }); component.ngOnInit(); diff --git a/libs/vault/src/cipher-form/components/card-details-section/card-details-section.component.ts b/libs/vault/src/cipher-form/components/card-details-section/card-details-section.component.ts index bc4ff608805..b01566da0b6 100644 --- a/libs/vault/src/cipher-form/components/card-details-section/card-details-section.component.ts +++ b/libs/vault/src/cipher-form/components/card-details-section/card-details-section.component.ts @@ -134,8 +134,10 @@ export class CardDetailsSectionComponent implements OnInit { } ngOnInit() { - if (this.originalCipherView?.card) { - this.setInitialValues(); + const prefillCipher = this.cipherFormContainer.getInitialCipherView(); + + if (prefillCipher) { + this.setInitialValues(prefillCipher); } if (this.disabled) { @@ -170,8 +172,8 @@ export class CardDetailsSectionComponent implements OnInit { } /** Set form initial form values from the current cipher */ - private setInitialValues() { - const { cardholderName, number, brand, expMonth, expYear, code } = this.originalCipherView.card; + private setInitialValues(cipherView: CipherView) { + const { cardholderName, number, brand, expMonth, expYear, code } = cipherView.card; this.cardDetailsForm.setValue({ cardholderName: cardholderName, diff --git a/libs/vault/src/cipher-form/components/custom-fields/custom-fields.component.spec.ts b/libs/vault/src/cipher-form/components/custom-fields/custom-fields.component.spec.ts index 0c0fa1b4184..320cb82b00e 100644 --- a/libs/vault/src/cipher-form/components/custom-fields/custom-fields.component.spec.ts +++ b/libs/vault/src/cipher-form/components/custom-fields/custom-fields.component.spec.ts @@ -59,7 +59,13 @@ describe("CustomFieldsComponent", () => { }, { provide: CipherFormContainer, - useValue: { patchCipher, originalCipherView, registerChildForm: jest.fn(), config }, + useValue: { + patchCipher, + originalCipherView, + registerChildForm: jest.fn(), + config, + getInitialCipherView: jest.fn(() => originalCipherView), + }, }, { provide: LiveAnnouncer, diff --git a/libs/vault/src/cipher-form/components/custom-fields/custom-fields.component.ts b/libs/vault/src/cipher-form/components/custom-fields/custom-fields.component.ts index 1aeb9e0da08..c3e425b7879 100644 --- a/libs/vault/src/cipher-form/components/custom-fields/custom-fields.component.ts +++ b/libs/vault/src/cipher-form/components/custom-fields/custom-fields.component.ts @@ -146,8 +146,10 @@ export class CustomFieldsComponent implements OnInit, AfterViewInit { value: id, })); - // Populate the form with the existing fields - this.cipherFormContainer.originalCipherView?.fields?.forEach((field) => { + const prefillCipher = this.cipherFormContainer.getInitialCipherView(); + + // When available, populate the form with the existing fields + prefillCipher.fields?.forEach((field) => { let value: string | boolean = field.value; if (field.type === FieldType.Boolean) { diff --git a/libs/vault/src/cipher-form/components/identity/identity.component.ts b/libs/vault/src/cipher-form/components/identity/identity.component.ts index d8f938f4ae7..a54adfcf841 100644 --- a/libs/vault/src/cipher-form/components/identity/identity.component.ts +++ b/libs/vault/src/cipher-form/components/identity/identity.component.ts @@ -111,8 +111,10 @@ export class IdentitySectionComponent implements OnInit { this.identityForm.disable(); } - if (this.originalCipherView && this.originalCipherView.id) { - this.populateFormData(); + const prefillCipher = this.cipherFormContainer.getInitialCipherView(); + + if (prefillCipher) { + this.populateFormData(prefillCipher); } else { this.identityForm.patchValue({ username: this.cipherFormContainer.config.initialValues?.username || "", @@ -120,8 +122,9 @@ export class IdentitySectionComponent implements OnInit { } } - populateFormData() { - const { identity } = this.originalCipherView; + populateFormData(cipherView: CipherView) { + const { identity } = cipherView; + this.identityForm.setValue({ title: identity.title, firstName: identity.firstName, 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 93229bda6c3..f9457de9e5f 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 @@ -9,7 +9,6 @@ import { CollectionView } from "@bitwarden/admin-console/common"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; import { AccountService } from "@bitwarden/common/auth/abstractions/account.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; -import { CollectionId, OrganizationId } from "@bitwarden/common/types/guid"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { SelectComponent } from "@bitwarden/components"; @@ -25,9 +24,11 @@ describe("ItemDetailsSectionComponent", () => { let i18nService: MockProxy; const activeAccount$ = new BehaviorSubject<{ email: string }>({ email: "test@example.com" }); + const getInitialCipherView = jest.fn(() => null); beforeEach(async () => { - cipherFormProvider = mock(); + getInitialCipherView.mockClear(); + cipherFormProvider = mock({ getInitialCipherView }); i18nService = mock(); await TestBed.configureTestingModule({ @@ -94,13 +95,14 @@ describe("ItemDetailsSectionComponent", () => { canEditItems: (_org) => true, } as CollectionView, ]; - component.originalCipherView = { + + getInitialCipherView.mockReturnValueOnce({ name: "cipher1", organizationId: "org1", folderId: "folder1", collectionIds: ["col1"], favorite: true, - } as CipherView; + }); await component.ngOnInit(); tick(); @@ -117,53 +119,6 @@ describe("ItemDetailsSectionComponent", () => { expect(updatedCipher.favorite).toBe(true); })); - it("should prioritize initialValues when editing an existing cipher ", fakeAsync(async () => { - component.config.allowPersonalOwnership = true; - component.config.organizations = [{ id: "org1" } as Organization]; - component.config.collections = [ - { - id: "col1", - name: "Collection 1", - organizationId: "org1", - canEditItems: (_org) => false, - } as CollectionView, - { - id: "col2", - name: "Collection 2", - organizationId: "org1", - canEditItems: (_org) => true, - } as CollectionView, - ]; - component.originalCipherView = { - name: "cipher1", - organizationId: "org1", - folderId: "folder1", - collectionIds: ["col1"], - favorite: true, - } as CipherView; - - component.config.initialValues = { - name: "new-name", - folderId: "new-folder", - organizationId: "bad-org" as OrganizationId, // Should not be set in edit mode - collectionIds: ["col2" as CollectionId], - }; - - await component.ngOnInit(); - tick(); - - expect(cipherFormProvider.patchCipher).toHaveBeenCalled(); - const patchFn = cipherFormProvider.patchCipher.mock.lastCall[0]; - - const updatedCipher = patchFn(new CipherView()); - - expect(updatedCipher.name).toBe("new-name"); - expect(updatedCipher.organizationId).toBe("org1"); - expect(updatedCipher.folderId).toBe("new-folder"); - expect(updatedCipher.collectionIds).toEqual(["col2"]); - expect(updatedCipher.favorite).toBe(true); - })); - it("should disable organizationId control if ownership change is not allowed", async () => { component.config.allowPersonalOwnership = false; component.config.organizations = [{ id: "org1" } as Organization]; @@ -294,7 +249,7 @@ describe("ItemDetailsSectionComponent", () => { it("should append '- Clone' to the title if in clone mode", async () => { component.config.mode = "clone"; component.config.allowPersonalOwnership = true; - component.originalCipherView = { + const cipher = { name: "cipher1", organizationId: null, folderId: null, @@ -302,6 +257,8 @@ describe("ItemDetailsSectionComponent", () => { favorite: false, } as CipherView; + getInitialCipherView.mockReturnValueOnce(cipher); + i18nService.t.calledWith("clone").mockReturnValue("Clone"); await component.ngOnInit(); @@ -373,13 +330,13 @@ describe("ItemDetailsSectionComponent", () => { it("should set collectionIds to originalCipher collections on first load", async () => { component.config.mode = "clone"; - component.originalCipherView = { + getInitialCipherView.mockReturnValueOnce({ name: "cipher1", organizationId: "org1", folderId: "folder1", collectionIds: ["col1", "col2"], favorite: true, - } as CipherView; + }); component.config.organizations = [{ id: "org1" } as Organization]; component.config.collections = [ { @@ -440,6 +397,13 @@ describe("ItemDetailsSectionComponent", () => { it("should show readonly hint if readonly collections are present", async () => { component.config.mode = "edit"; + getInitialCipherView.mockReturnValueOnce({ + name: "cipher1", + organizationId: "org1", + folderId: "folder1", + collectionIds: ["col1", "col2", "col3"], + favorite: true, + }); component.originalCipherView = { name: "cipher1", organizationId: "org1", @@ -550,6 +514,9 @@ describe("ItemDetailsSectionComponent", () => { collectionIds: ["col1", "col2", "col3"], favorite: true, } as CipherView; + + getInitialCipherView.mockReturnValue(component.originalCipherView); + component.config.organizations = [{ id: "org1" } as Organization]; }); 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 ea82aa0cae4..110466f824f 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 @@ -181,8 +181,10 @@ export class ItemDetailsSectionComponent implements OnInit { throw new Error("No organizations available for ownership."); } - if (this.originalCipherView) { - await this.initFromExistingCipher(); + const prefillCipher = this.cipherFormContainer.getInitialCipherView(); + + if (prefillCipher) { + await this.initFromExistingCipher(prefillCipher); } else { this.itemDetailsForm.setValue({ name: this.initialValues?.name || "", @@ -208,30 +210,33 @@ export class ItemDetailsSectionComponent implements OnInit { .subscribe(); } - private async initFromExistingCipher() { + private async initFromExistingCipher(prefillCipher: CipherView) { + const { name, folderId } = prefillCipher; + this.itemDetailsForm.setValue({ - name: this.initialValues?.name ?? this.originalCipherView.name, - organizationId: this.originalCipherView.organizationId, // We do not allow changing ownership of an existing cipher. - folderId: this.initialValues?.folderId ?? this.originalCipherView.folderId, - collectionIds: [], - favorite: this.originalCipherView.favorite, + name: name ? name : (this.initialValues.name ?? ""), + organizationId: prefillCipher.organizationId, // We do not allow changing ownership of an existing cipher. + folderId: folderId ? folderId : (this.initialValues?.folderId ?? null), + collectionIds: prefillCipher.collectionIds, + favorite: prefillCipher.favorite, }); // Configure form for clone mode. if (this.config.mode === "clone") { this.itemDetailsForm.controls.name.setValue( - this.originalCipherView.name + " - " + this.i18nService.t("clone"), + prefillCipher.name + " - " + this.i18nService.t("clone"), ); - if (!this.allowPersonalOwnership && this.originalCipherView.organizationId == null) { + if (!this.allowPersonalOwnership && prefillCipher.organizationId == null) { this.itemDetailsForm.controls.organizationId.setValue(this.defaultOwner); } } - await this.updateCollectionOptions( - this.initialValues?.collectionIds ?? - (this.originalCipherView.collectionIds as CollectionId[]), - ); + const prefillCollections = prefillCipher.collectionIds?.length + ? (prefillCipher.collectionIds as CollectionId[]) + : (this.initialValues?.collectionIds ?? []); + + await this.updateCollectionOptions(prefillCollections); if (this.partialEdit) { this.itemDetailsForm.disable(); diff --git a/libs/vault/src/cipher-form/components/login-details-section/login-details-section.component.spec.ts b/libs/vault/src/cipher-form/components/login-details-section/login-details-section.component.spec.ts index 232a4b2d27b..8e339eb9150 100644 --- a/libs/vault/src/cipher-form/components/login-details-section/login-details-section.component.spec.ts +++ b/libs/vault/src/cipher-form/components/login-details-section/login-details-section.component.spec.ts @@ -43,9 +43,11 @@ describe("LoginDetailsSectionComponent", () => { let configService: MockProxy; const collect = jest.fn().mockResolvedValue(null); + const getInitialCipherView = jest.fn(() => null); beforeEach(async () => { - cipherFormContainer = mock(); + getInitialCipherView.mockClear(); + cipherFormContainer = mock({ getInitialCipherView }); generationService = mock(); auditService = mock(); @@ -120,18 +122,18 @@ describe("LoginDetailsSectionComponent", () => { }); it("initializes 'loginDetailsForm' with original cipher view values", async () => { - (cipherFormContainer.originalCipherView as CipherView) = { + getInitialCipherView.mockReturnValueOnce({ viewPassword: true, login: { password: "original-password", username: "original-username", totp: "original-totp", - } as LoginView, - } as CipherView; + }, + }); - await component.ngOnInit(); + component.ngOnInit(); - expect(component.loginDetailsForm.value).toEqual({ + expect(component.loginDetailsForm.getRawValue()).toEqual({ username: "original-username", password: "original-password", totp: "original-totp", @@ -139,22 +141,23 @@ describe("LoginDetailsSectionComponent", () => { }); it("initializes 'loginDetailsForm' with initialValues that override any original cipher view values", async () => { - (cipherFormContainer.originalCipherView as CipherView) = { + getInitialCipherView.mockReturnValueOnce({ viewPassword: true, login: { password: "original-password", username: "original-username", totp: "original-totp", - } as LoginView, - } as CipherView; + }, + }); + cipherFormContainer.config.initialValues = { username: "new-username", password: "new-password", }; - await component.ngOnInit(); + component.ngOnInit(); - expect(component.loginDetailsForm.value).toEqual({ + expect(component.loginDetailsForm.getRawValue()).toEqual({ username: "new-username", password: "new-password", totp: "original-totp", @@ -163,12 +166,12 @@ describe("LoginDetailsSectionComponent", () => { describe("viewHiddenFields", () => { beforeEach(() => { - (cipherFormContainer.originalCipherView as CipherView) = { + getInitialCipherView.mockReturnValue({ viewPassword: false, login: { password: "original-password", - } as LoginView, - } as CipherView; + }, + }); }); it("returns value of originalCipher.viewPassword", () => { @@ -249,6 +252,10 @@ describe("LoginDetailsSectionComponent", () => { }); describe("password", () => { + beforeEach(() => { + getInitialCipherView.mockReturnValue(null); + }); + const getGeneratePasswordBtn = () => fixture.nativeElement.querySelector("button[data-testid='generate-password-button']"); @@ -516,11 +523,11 @@ describe("LoginDetailsSectionComponent", () => { fixture.nativeElement.querySelector("input[data-testid='passkey-field']"); beforeEach(() => { - (cipherFormContainer.originalCipherView as CipherView) = { + getInitialCipherView.mockReturnValue({ login: Object.assign(new LoginView(), { fido2Credentials: [{ creationDate: passkeyDate } as Fido2CredentialView], }), - } as CipherView; + }); fixture = TestBed.createComponent(LoginDetailsSectionComponent); component = fixture.componentInstance; @@ -563,7 +570,11 @@ describe("LoginDetailsSectionComponent", () => { }); it("hides the passkey field when missing a passkey", () => { - (cipherFormContainer.originalCipherView as CipherView).login.fido2Credentials = []; + getInitialCipherView.mockReturnValueOnce({ + login: Object.assign(new LoginView(), { + fido2Credentials: [], + }), + }); fixture.detectChanges(); diff --git a/libs/vault/src/cipher-form/components/login-details-section/login-details-section.component.ts b/libs/vault/src/cipher-form/components/login-details-section/login-details-section.component.ts index 691b05be2b4..42dea8e7091 100644 --- a/libs/vault/src/cipher-form/components/login-details-section/login-details-section.component.ts +++ b/libs/vault/src/cipher-form/components/login-details-section/login-details-section.component.ts @@ -133,11 +133,13 @@ export class LoginDetailsSectionComponent implements OnInit { }); } - async ngOnInit() { - if (this.cipherFormContainer.originalCipherView?.login) { - this.initFromExistingCipher(this.cipherFormContainer.originalCipherView.login); + ngOnInit() { + const prefillCipher = this.cipherFormContainer.getInitialCipherView(); + + if (prefillCipher) { + this.initFromExistingCipher(prefillCipher.login); } else { - await this.initNewCipher(); + this.initNewCipher(); } if (this.cipherFormContainer.config.mode === "partial-edit") { @@ -160,7 +162,7 @@ export class LoginDetailsSectionComponent implements OnInit { } } - private async initNewCipher() { + private initNewCipher() { this.loginDetailsForm.patchValue({ username: this.initialValues?.username || "", password: this.initialValues?.password || "", From feba423717a2287176783a8e1edc10aa5a35b0b5 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 3 Dec 2024 13:46:02 -0600 Subject: [PATCH 04/17] add listener for clearing view cache --- .../platform/services/popup-view-cache-background.service.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/apps/browser/src/platform/services/popup-view-cache-background.service.ts b/apps/browser/src/platform/services/popup-view-cache-background.service.ts index 35c55633c0c..546c6e63912 100644 --- a/apps/browser/src/platform/services/popup-view-cache-background.service.ts +++ b/apps/browser/src/platform/services/popup-view-cache-background.service.ts @@ -58,6 +58,11 @@ export class PopupViewCacheBackgroundService { ) .subscribe(); + this.messageListener + .messages$(ClEAR_VIEW_CACHE_COMMAND) + .pipe(concatMap(() => this.popupViewCacheState.update(() => null))) + .subscribe(); + merge( // on tab changed, excluding extension tabs fromChromeEvent(chrome.tabs.onActivated).pipe( From fd38a8c57b4174a259fcc5b68c4f6613abd39de8 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 3 Dec 2024 13:46:25 -0600 Subject: [PATCH 05/17] clear local cache when clearing global state --- .../src/platform/popup/view-cache/popup-view-cache.service.ts | 1 + .../src/platform/popup/view-cache/popup-view-cache.spec.ts | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/apps/browser/src/platform/popup/view-cache/popup-view-cache.service.ts b/apps/browser/src/platform/popup/view-cache/popup-view-cache.service.ts index 819a2aa3e46..b0867bb2ece 100644 --- a/apps/browser/src/platform/popup/view-cache/popup-view-cache.service.ts +++ b/apps/browser/src/platform/popup/view-cache/popup-view-cache.service.ts @@ -131,6 +131,7 @@ export class PopupViewCacheService implements ViewCacheService { } private clearState() { + this._cache = {}; // clear local cache this.messageSender.send(ClEAR_VIEW_CACHE_COMMAND, {}); } } diff --git a/apps/browser/src/platform/popup/view-cache/popup-view-cache.spec.ts b/apps/browser/src/platform/popup/view-cache/popup-view-cache.spec.ts index fbe94bece8c..72cfd39cd62 100644 --- a/apps/browser/src/platform/popup/view-cache/popup-view-cache.spec.ts +++ b/apps/browser/src/platform/popup/view-cache/popup-view-cache.spec.ts @@ -196,13 +196,15 @@ describe("popup view cache", () => { }); it("should clear on 2nd navigation", async () => { - await initServiceWithState({}); + await initServiceWithState({ temp: "state" }); await router.navigate(["a"]); expect(messageSenderMock.send).toHaveBeenCalledTimes(0); + expect(service["_cache"]).toEqual({ temp: "state" }); await router.navigate(["b"]); expect(messageSenderMock.send).toHaveBeenCalledWith(ClEAR_VIEW_CACHE_COMMAND, {}); + expect(service["_cache"]).toEqual({}); }); it("should ignore cached values when feature flag is off", async () => { From fed19919104b8962a496d8fac0c5e2e8c7046274 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 3 Dec 2024 14:46:43 -0600 Subject: [PATCH 06/17] track initial value of cache for down stream logic that should only occur on non-cached values --- .../src/cipher-form/cipher-form-container.ts | 3 ++ .../components/cipher-form.component.ts | 5 +++ .../item-details-section.component.spec.ts | 35 +++++++++++++++++-- .../item-details-section.component.ts | 10 ++++-- .../default-cipher-form-cache.service.ts | 12 +++++++ 5 files changed, 59 insertions(+), 6 deletions(-) diff --git a/libs/vault/src/cipher-form/cipher-form-container.ts b/libs/vault/src/cipher-form/cipher-form-container.ts index cc439e37a13..f7e735f10f4 100644 --- a/libs/vault/src/cipher-form/cipher-form-container.ts +++ b/libs/vault/src/cipher-form/cipher-form-container.ts @@ -59,4 +59,7 @@ export abstract class CipherFormContainer { * Returns initial values for the CipherView, either from the config or the cached cipher */ abstract getInitialCipherView(): CipherView | null; + + /** Returns true when the `CipherFormContainer` was initialized with a cached cipher available. */ + abstract initializedWithCachedCipher(): boolean; } diff --git a/libs/vault/src/cipher-form/components/cipher-form.component.ts b/libs/vault/src/cipher-form/components/cipher-form.component.ts index 8933a451313..d3d3b9fbed4 100644 --- a/libs/vault/src/cipher-form/components/cipher-form.component.ts +++ b/libs/vault/src/cipher-form/components/cipher-form.component.ts @@ -183,6 +183,11 @@ export class CipherFormComponent implements AfterViewInit, OnInit, OnChanges, Ci return this.updatedCipherView; } + /** */ + initializedWithCachedCipher(): boolean { + return this.cipherFormCacheService.initializedWithValue; + } + /** * We need to re-initialize the form when the config is updated. */ 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 f9457de9e5f..2e130bf875d 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 @@ -25,10 +25,16 @@ describe("ItemDetailsSectionComponent", () => { const activeAccount$ = new BehaviorSubject<{ email: string }>({ email: "test@example.com" }); const getInitialCipherView = jest.fn(() => null); + const initializedWithCachedCipher = jest.fn(() => false); beforeEach(async () => { getInitialCipherView.mockClear(); - cipherFormProvider = mock({ getInitialCipherView }); + initializedWithCachedCipher.mockClear(); + + cipherFormProvider = mock({ + getInitialCipherView, + initializedWithCachedCipher, + }); i18nService = mock(); await TestBed.configureTestingModule({ @@ -246,8 +252,11 @@ describe("ItemDetailsSectionComponent", () => { }); describe("cloneMode", () => { - it("should append '- Clone' to the title if in clone mode", async () => { + beforeEach(() => { component.config.mode = "clone"; + }); + + it("should append '- Clone' to the title if in clone mode", async () => { component.config.allowPersonalOwnership = true; const cipher = { name: "cipher1", @@ -266,8 +275,28 @@ describe("ItemDetailsSectionComponent", () => { expect(component.itemDetailsForm.controls.name.value).toBe("cipher1 - Clone"); }); + it("does not append clone when the cipher was populated from the cache", async () => { + component.config.allowPersonalOwnership = true; + const cipher = { + name: "from cache cipher", + organizationId: null, + folderId: null, + collectionIds: null, + favorite: false, + } as CipherView; + + getInitialCipherView.mockReturnValueOnce(cipher); + + initializedWithCachedCipher.mockReturnValueOnce(true); + + i18nService.t.calledWith("clone").mockReturnValue("Clone"); + + await component.ngOnInit(); + + expect(component.itemDetailsForm.controls.name.value).toBe("from cache cipher"); + }); + it("should select the first organization if personal ownership is not allowed", async () => { - component.config.mode = "clone"; component.config.allowPersonalOwnership = false; component.config.organizations = [ { id: "org1" } as Organization, 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 110466f824f..c278ab4d347 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 @@ -221,11 +221,15 @@ export class ItemDetailsSectionComponent implements OnInit { favorite: prefillCipher.favorite, }); + const initializedWithCachedCipher = this.cipherFormContainer.initializedWithCachedCipher(); + // Configure form for clone mode. if (this.config.mode === "clone") { - this.itemDetailsForm.controls.name.setValue( - prefillCipher.name + " - " + this.i18nService.t("clone"), - ); + if (!initializedWithCachedCipher) { + this.itemDetailsForm.controls.name.setValue( + prefillCipher.name + " - " + this.i18nService.t("clone"), + ); + } if (!this.allowPersonalOwnership && prefillCipher.organizationId == null) { this.itemDetailsForm.controls.organizationId.setValue(this.defaultOwner); diff --git a/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts index 2d84e7f282e..71dac4cf40e 100644 --- a/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts +++ b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts @@ -9,12 +9,24 @@ const POPUP_CIPHER_CACHE_KEY = "popup-cipher-cache"; export class CipherFormCacheService { private popupViewCacheService: ViewCacheService = inject(ViewCacheService); + /** + * When true the `CipherFormCacheService` a cipher was stored in cache when the service was initialized. + * Otherwise false, when the cache was empty. + * + * This is helpful to know the initial state of the cache as it can be populated quickly after initialization. + */ + initializedWithValue: boolean; + private cipherCache = this.popupViewCacheService.signal({ key: POPUP_CIPHER_CACHE_KEY, initialValue: null, deserializer: (obj) => (obj ? CipherView.fromJSON(obj) : null), }); + constructor() { + this.initializedWithValue = !!this.cipherCache(); + } + /** * Update the cache with the new CipherView. */ From d85f7f4d98de5caecbcbbba512a652d034ef9fe9 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 3 Dec 2024 15:35:50 -0600 Subject: [PATCH 07/17] add feature flag for edit form persistence --- libs/common/src/enums/feature-flag.enum.ts | 2 ++ .../components/cipher-form.component.ts | 2 ++ .../default-cipher-form-cache.service.ts | 27 +++++++++++++++++++ 3 files changed, 31 insertions(+) diff --git a/libs/common/src/enums/feature-flag.enum.ts b/libs/common/src/enums/feature-flag.enum.ts index 6b8af3ef78d..c290865f4c7 100644 --- a/libs/common/src/enums/feature-flag.enum.ts +++ b/libs/common/src/enums/feature-flag.enum.ts @@ -40,6 +40,7 @@ export enum FeatureFlag { NewDeviceVerificationPermanentDismiss = "new-device-permanent-dismiss", DisableFreeFamiliesSponsorship = "PM-12274-disable-free-families-sponsorship", PM11360RemoveProviderExportPermission = "pm-11360-remove-provider-export-permission", + PM9111ExtensionPersistAddEditForm = "pm-9111-extension-persist-add-edit-form", } export type AllowedFeatureFlagTypes = boolean | number | string; @@ -90,6 +91,7 @@ export const DefaultFeatureFlagValue = { [FeatureFlag.NewDeviceVerificationPermanentDismiss]: FALSE, [FeatureFlag.DisableFreeFamiliesSponsorship]: FALSE, [FeatureFlag.PM11360RemoveProviderExportPermission]: FALSE, + [FeatureFlag.PM9111ExtensionPersistAddEditForm]: FALSE, } satisfies Record; export type DefaultFeatureFlagValueType = typeof DefaultFeatureFlagValue; diff --git a/libs/vault/src/cipher-form/components/cipher-form.component.ts b/libs/vault/src/cipher-form/components/cipher-form.component.ts index d3d3b9fbed4..12bea976b0a 100644 --- a/libs/vault/src/cipher-form/components/cipher-form.component.ts +++ b/libs/vault/src/cipher-form/components/cipher-form.component.ts @@ -209,6 +209,8 @@ export class CipherFormComponent implements AfterViewInit, OnInit, OnChanges, Ci // Force change detection so that all child components are destroyed and re-created this.changeDetectorRef.detectChanges(); + await this.cipherFormCacheService.init(); + this.updatedCipherView = new CipherView(); this.originalCipherView = null; this.cipherForm = this.formBuilder.group({}); diff --git a/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts index 71dac4cf40e..a1d00d316ae 100644 --- a/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts +++ b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts @@ -1,6 +1,8 @@ import { inject, Injectable } from "@angular/core"; import { ViewCacheService } from "@bitwarden/angular/platform/abstractions/view-cache.service"; +import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; const POPUP_CIPHER_CACHE_KEY = "popup-cipher-cache"; @@ -8,6 +10,10 @@ const POPUP_CIPHER_CACHE_KEY = "popup-cipher-cache"; @Injectable() export class CipherFormCacheService { private popupViewCacheService: ViewCacheService = inject(ViewCacheService); + private configService: ConfigService = inject(ConfigService); + + /** True when the `PM9111ExtensionPersistAddEditForm` flag is enabled */ + private featureEnabled: boolean; /** * When true the `CipherFormCacheService` a cipher was stored in cache when the service was initialized. @@ -27,10 +33,27 @@ export class CipherFormCacheService { this.initializedWithValue = !!this.cipherCache(); } + /** + * Must be called once before interacting with the cached cipher, otherwise methods will be noop. + */ + async init() { + this.featureEnabled = await this.configService.getFeatureFlag( + FeatureFlag.PM9111ExtensionPersistAddEditForm, + ); + + if (!this.featureEnabled) { + this.initializedWithValue = false; + } + } + /** * Update the cache with the new CipherView. */ cacheCipherView(cipherView: CipherView): void { + if (!this.featureEnabled) { + return; + } + // Create a new shallow reference to force the signal to update this.cipherCache.set({ ...cipherView } as CipherView); } @@ -39,6 +62,10 @@ export class CipherFormCacheService { * Returns the cached CipherView when available. */ getCachedCipherView(): CipherView | null { + if (!this.featureEnabled) { + return null; + } + return this.cipherCache(); } } From 388b1f2f2c19d9e37c0ae692a6d264586cc1c144 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 3 Dec 2024 19:41:28 -0600 Subject: [PATCH 08/17] add tests for cipher form cache service --- .../default-cipher-form-cache.service.spec.ts | 97 +++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 libs/vault/src/cipher-form/services/default-cipher-form-cache.service.spec.ts diff --git a/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.spec.ts b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.spec.ts new file mode 100644 index 00000000000..5f8e44d03c3 --- /dev/null +++ b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.spec.ts @@ -0,0 +1,97 @@ +import { signal } from "@angular/core"; +import { TestBed } from "@angular/core/testing"; + +import { ViewCacheService } from "@bitwarden/angular/platform/abstractions/view-cache.service"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; +import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; + +import { CipherFormCacheService } from "./default-cipher-form-cache.service"; + +describe("CipherFormCacheService", () => { + let service: CipherFormCacheService; + let testBed: TestBed; + const cacheSignal = signal(null); + const getCacheSignal = jest.fn().mockReturnValue(cacheSignal); + const getFeatureFlag = jest.fn().mockResolvedValue(false); + const cacheSetMock = jest.spyOn(cacheSignal, "set"); + + beforeEach(() => { + getCacheSignal.mockClear(); + getFeatureFlag.mockClear(); + cacheSetMock.mockClear(); + + testBed = TestBed.configureTestingModule({ + providers: [ + { provide: ViewCacheService, useValue: { signal: getCacheSignal } }, + { provide: ConfigService, useValue: { getFeatureFlag } }, + CipherFormCacheService, + ], + }); + }); + + describe("feature enabled", () => { + beforeEach(async () => { + getFeatureFlag.mockResolvedValue(true); + }); + + it("`getCachedCipherView` returns the cipher", async () => { + cacheSignal.set({ id: "cipher-4" }); + service = testBed.inject(CipherFormCacheService); + await service.init(); + + expect(service.getCachedCipherView()).toEqual({ id: "cipher-4" }); + }); + + it("updates the signal value", async () => { + service = testBed.inject(CipherFormCacheService); + await service.init(); + + service.cacheCipherView({ id: "cipher-5" } as CipherView); + + expect(cacheSignal.set).toHaveBeenCalledWith({ id: "cipher-5" }); + }); + + describe("initializedWithValue", () => { + it("sets `initializedWithValue` to true when there is a cached cipher", async () => { + cacheSignal.set({ id: "cipher-3" }); + service = testBed.inject(CipherFormCacheService); + await service.init(); + + expect(service.initializedWithValue).toBe(true); + }); + + it("sets `initializedWithValue` to false when there is not a cached cipher", async () => { + cacheSignal.set(null); + service = testBed.inject(CipherFormCacheService); + await service.init(); + + expect(service.initializedWithValue).toBe(false); + }); + }); + }); + + describe("featured disabled", () => { + beforeEach(async () => { + cacheSignal.set({ id: "cipher-1" }); + getFeatureFlag.mockResolvedValue(false); + cacheSetMock.mockClear(); + + service = testBed.inject(CipherFormCacheService); + await service.init(); + }); + + it("sets `initializedWithValue` to false", () => { + expect(service.initializedWithValue).toBe(false); + }); + + it("`getCachedCipherView` returns null", () => { + expect(service.getCachedCipherView()).toBeNull(); + }); + + it("does not update the signal value", () => { + service.cacheCipherView({ id: "cipher-2" } as CipherView); + + expect(cacheSignal.set).not.toHaveBeenCalled(); + }); + }); +}); From 3f6374fb756a11e40c0adc47259553eb78e4c13b Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 3 Dec 2024 21:05:11 -0600 Subject: [PATCH 09/17] fix optional initialValues --- .../components/item-details/item-details-section.component.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 c278ab4d347..ef5da526c02 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 @@ -214,7 +214,7 @@ export class ItemDetailsSectionComponent implements OnInit { const { name, folderId } = prefillCipher; this.itemDetailsForm.setValue({ - name: name ? name : (this.initialValues.name ?? ""), + name: name ? name : (this.initialValues?.name ?? ""), organizationId: prefillCipher.organizationId, // We do not allow changing ownership of an existing cipher. folderId: folderId ? folderId : (this.initialValues?.folderId ?? null), collectionIds: prefillCipher.collectionIds, From 7b1255919d5d416643d968adf6d63fb7dd24ed82 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 3 Dec 2024 21:05:31 -0600 Subject: [PATCH 10/17] add services to cipher form storybook --- .../src/cipher-form/cipher-form.stories.ts | 24 ++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/libs/vault/src/cipher-form/cipher-form.stories.ts b/libs/vault/src/cipher-form/cipher-form.stories.ts index e48cf384c2c..a86b7979e16 100644 --- a/libs/vault/src/cipher-form/cipher-form.stories.ts +++ b/libs/vault/src/cipher-form/cipher-form.stories.ts @@ -1,4 +1,4 @@ -import { importProvidersFrom } from "@angular/core"; +import { importProvidersFrom, signal } from "@angular/core"; import { action } from "@storybook/addon-actions"; import { applicationConfig, @@ -10,6 +10,7 @@ import { import { BehaviorSubject } from "rxjs"; import { CollectionView } from "@bitwarden/admin-console/common"; +import { ViewCacheService } from "@bitwarden/angular/platform/abstractions/view-cache.service"; import { AuditService } from "@bitwarden/common/abstractions/audit.service"; import { EventCollectionService } from "@bitwarden/common/abstractions/event/event-collection.service"; import { Organization } from "@bitwarden/common/admin-console/models/domain/organization"; @@ -18,6 +19,7 @@ import { AutofillSettingsServiceAbstraction } from "@bitwarden/common/autofill/s import { DomainSettingsService } from "@bitwarden/common/autofill/services/domain-settings.service"; import { ClientType } from "@bitwarden/common/enums"; import { UriMatchStrategy } from "@bitwarden/common/models/domain/domain-service"; +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; import { CipherType } from "@bitwarden/common/vault/enums"; import { Cipher } from "@bitwarden/common/vault/models/domain/cipher"; @@ -38,6 +40,7 @@ import { CipherFormService } from "./abstractions/cipher-form.service"; import { TotpCaptureService } from "./abstractions/totp-capture.service"; import { CipherFormModule } from "./cipher-form.module"; import { CipherFormComponent } from "./components/cipher-form.component"; +import { CipherFormCacheService } from "./services/default-cipher-form-cache.service"; const defaultConfig: CipherFormConfig = { mode: "add", @@ -190,6 +193,25 @@ export default { activeAccount$: new BehaviorSubject({ email: "test@example.com" }), }, }, + { + provide: CipherFormCacheService, + useValue: { + getCachedCipherView: (): null => null, + initializedWithValue: false, + }, + }, + { + provide: ViewCacheService, + useValue: { + signal: () => signal(null), + }, + }, + { + provide: ConfigService, + useValue: { + getFeatureFlag: () => Promise.resolve(false), + }, + }, ], }), componentWrapperDecorator( From 00266fd96717d866c45260579f20e04b05aae7a3 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Thu, 12 Dec 2024 14:48:40 -0600 Subject: [PATCH 11/17] fix strict types --- .../services/default-cipher-form-cache.service.spec.ts | 8 ++++---- .../services/default-cipher-form-cache.service.ts | 4 ++-- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.spec.ts b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.spec.ts index 5f8e44d03c3..6236e2d3dac 100644 --- a/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.spec.ts +++ b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.spec.ts @@ -10,7 +10,7 @@ import { CipherFormCacheService } from "./default-cipher-form-cache.service"; describe("CipherFormCacheService", () => { let service: CipherFormCacheService; let testBed: TestBed; - const cacheSignal = signal(null); + const cacheSignal = signal(null); const getCacheSignal = jest.fn().mockReturnValue(cacheSignal); const getFeatureFlag = jest.fn().mockResolvedValue(false); const cacheSetMock = jest.spyOn(cacheSignal, "set"); @@ -35,7 +35,7 @@ describe("CipherFormCacheService", () => { }); it("`getCachedCipherView` returns the cipher", async () => { - cacheSignal.set({ id: "cipher-4" }); + cacheSignal.set({ id: "cipher-4" } as CipherView); service = testBed.inject(CipherFormCacheService); await service.init(); @@ -53,7 +53,7 @@ describe("CipherFormCacheService", () => { describe("initializedWithValue", () => { it("sets `initializedWithValue` to true when there is a cached cipher", async () => { - cacheSignal.set({ id: "cipher-3" }); + cacheSignal.set({ id: "cipher-3" } as CipherView); service = testBed.inject(CipherFormCacheService); await service.init(); @@ -72,7 +72,7 @@ describe("CipherFormCacheService", () => { describe("featured disabled", () => { beforeEach(async () => { - cacheSignal.set({ id: "cipher-1" }); + cacheSignal.set({ id: "cipher-1" } as CipherView); getFeatureFlag.mockResolvedValue(false); cacheSetMock.mockClear(); diff --git a/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts index a1d00d316ae..c539c425bb6 100644 --- a/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts +++ b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts @@ -13,7 +13,7 @@ export class CipherFormCacheService { private configService: ConfigService = inject(ConfigService); /** True when the `PM9111ExtensionPersistAddEditForm` flag is enabled */ - private featureEnabled: boolean; + private featureEnabled: boolean = false; /** * When true the `CipherFormCacheService` a cipher was stored in cache when the service was initialized. @@ -26,7 +26,7 @@ export class CipherFormCacheService { private cipherCache = this.popupViewCacheService.signal({ key: POPUP_CIPHER_CACHE_KEY, initialValue: null, - deserializer: (obj) => (obj ? CipherView.fromJSON(obj) : null), + deserializer: CipherView.fromJSON, }); constructor() { From cb5d689cc19be94df11d146ecca23aee7456bd00 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Thu, 12 Dec 2024 14:48:59 -0600 Subject: [PATCH 12/17] rename variables to be platform agnostic --- .../services/default-cipher-form-cache.service.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts index c539c425bb6..bbb4e5049ea 100644 --- a/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts +++ b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts @@ -5,11 +5,11 @@ import { FeatureFlag } from "@bitwarden/common/enums/feature-flag.enum"; import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; -const POPUP_CIPHER_CACHE_KEY = "popup-cipher-cache"; +const CIPHER_FORM_CACHE_KEY = "cipher-form-cache"; @Injectable() export class CipherFormCacheService { - private popupViewCacheService: ViewCacheService = inject(ViewCacheService); + private viewCacheService: ViewCacheService = inject(ViewCacheService); private configService: ConfigService = inject(ConfigService); /** True when the `PM9111ExtensionPersistAddEditForm` flag is enabled */ @@ -23,8 +23,8 @@ export class CipherFormCacheService { */ initializedWithValue: boolean; - private cipherCache = this.popupViewCacheService.signal({ - key: POPUP_CIPHER_CACHE_KEY, + private cipherCache = this.viewCacheService.signal({ + key: CIPHER_FORM_CACHE_KEY, initialValue: null, deserializer: CipherView.fromJSON, }); From 7ef3a7b00c484bd537f97b8884e1b62518e1775a Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Thu, 12 Dec 2024 15:48:56 -0600 Subject: [PATCH 13/17] use deconstructed collectionIds variable to avoid them be overwritten --- .../item-details/item-details-section.component.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 f2db45483d8..94f6c97650c 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 @@ -213,13 +213,13 @@ export class ItemDetailsSectionComponent implements OnInit { } private async initFromExistingCipher(prefillCipher: CipherView) { - const { name, folderId } = prefillCipher; + const { name, folderId, collectionIds } = prefillCipher; this.itemDetailsForm.setValue({ name: name ? name : (this.initialValues?.name ?? ""), organizationId: prefillCipher.organizationId, // We do not allow changing ownership of an existing cipher. folderId: folderId ? folderId : (this.initialValues?.folderId ?? null), - collectionIds: prefillCipher.collectionIds, + collectionIds: [], favorite: prefillCipher.favorite, }); @@ -238,8 +238,8 @@ export class ItemDetailsSectionComponent implements OnInit { } } - const prefillCollections = prefillCipher.collectionIds?.length - ? (prefillCipher.collectionIds as CollectionId[]) + const prefillCollections = collectionIds?.length + ? (collectionIds as CollectionId[]) : (this.initialValues?.collectionIds ?? []); await this.updateCollectionOptions(prefillCollections); From 77c0c19f40a7365e5337bf487bda905e1ec295dd Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Thu, 12 Dec 2024 19:21:48 -0600 Subject: [PATCH 14/17] use the originalCipherView for initial values --- libs/vault/src/cipher-form/components/cipher-form.component.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libs/vault/src/cipher-form/components/cipher-form.component.ts b/libs/vault/src/cipher-form/components/cipher-form.component.ts index 9760d89a550..a4f8f8f61e0 100644 --- a/libs/vault/src/cipher-form/components/cipher-form.component.ts +++ b/libs/vault/src/cipher-form/components/cipher-form.component.ts @@ -182,7 +182,7 @@ export class CipherFormComponent implements AfterViewInit, OnInit, OnChanges, Ci return cachedCipherView; } - return this.updatedCipherView; + return this.originalCipherView; } /** */ From 5c0a399e5e47dd37ef5423c55642321bf168d7d8 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Thu, 12 Dec 2024 20:12:27 -0600 Subject: [PATCH 15/17] add comment about signal equality --- .../cipher-form/services/default-cipher-form-cache.service.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts index bbb4e5049ea..268b2db306b 100644 --- a/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts +++ b/libs/vault/src/cipher-form/services/default-cipher-form-cache.service.ts @@ -55,6 +55,8 @@ export class CipherFormCacheService { } // Create a new shallow reference to force the signal to update + // By default, signals use `Object.is` to determine equality + // Docs: https://angular.dev/guide/signals#signal-equality-functions this.cipherCache.set({ ...cipherView } as CipherView); } From af9dc2e4bdf1fd5d5d3447de89807b319129e894 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 14 Jan 2025 14:55:51 -0600 Subject: [PATCH 16/17] prevent events from being emitted when adding uris to the existing form - This stops other values from being overwrote in the initialization process --- .../autofill-options.component.ts | 27 ++++++++++++++----- 1 file changed, 21 insertions(+), 6 deletions(-) diff --git a/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.ts b/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.ts index 297d58a6d42..bb8a73e4e7b 100644 --- a/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.ts +++ b/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.ts @@ -143,11 +143,19 @@ export class AutofillOptionsComponent implements OnInit { } private initFromExistingCipher(existingLogin: LoginView) { + // The `uris` control is a FormArray which needs to dynamically + // add controls to the form. Doing this will trigger the `valueChanges` observable on the form + // and overwrite the `autofillOnPageLoad` value before it is set in the following `patchValue` call. + // Pass `false` to `addUri` to stop events from emitting when adding the URIs. existingLogin.uris?.forEach((uri) => { - this.addUri({ - uri: uri.uri, - matchDetection: uri.match, - }); + this.addUri( + { + uri: uri.uri, + matchDetection: uri.match, + }, + false, + false, + ); }); this.autofillOptionsForm.patchValue({ autofillOnPageLoad: existingLogin.autofillOnPageLoad, @@ -198,9 +206,16 @@ export class AutofillOptionsComponent implements OnInit { * Adds a new URI input to the form. * @param uriFieldValue The initial value for the new URI input. * @param focusNewInput If true, the new URI input will be focused after being added. + * @param emitEvent When false, prevents the `valueChanges` & `statusChanges` observables from firing. */ - addUri(uriFieldValue: UriField = { uri: null, matchDetection: null }, focusNewInput = false) { - this.autofillOptionsForm.controls.uris.push(this.formBuilder.control(uriFieldValue)); + addUri( + uriFieldValue: UriField = { uri: null, matchDetection: null }, + focusNewInput = false, + emitEvent = true, + ) { + this.autofillOptionsForm.controls.uris.push(this.formBuilder.control(uriFieldValue), { + emitEvent, + }); if (focusNewInput) { this.focusOnNewInput$.next(); From b8b8c2cdbe8786c883bb50b062ee3a5674c1ad32 Mon Sep 17 00:00:00 2001 From: Nick Krantz Date: Tue, 14 Jan 2025 14:59:15 -0600 Subject: [PATCH 17/17] add check for cached cipher when adding initial uris --- .../autofill-options/autofill-options.component.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.ts b/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.ts index bb8a73e4e7b..c3b2a0fb9f9 100644 --- a/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.ts +++ b/libs/vault/src/cipher-form/components/autofill-options/autofill-options.component.ts @@ -161,7 +161,11 @@ export class AutofillOptionsComponent implements OnInit { autofillOnPageLoad: existingLogin.autofillOnPageLoad, }); - if (this.cipherFormContainer.config.initialValues?.loginUri) { + // Only add the initial value when the cipher was not initialized from a cached state + if ( + this.cipherFormContainer.config.initialValues?.loginUri && + !this.cipherFormContainer.initializedWithCachedCipher() + ) { // Avoid adding the same uri again if it already exists if ( existingLogin.uris?.findIndex(