diff --git a/libs/vault/src/cipher-form/cipher-form-container.ts b/libs/vault/src/cipher-form/cipher-form-container.ts index eb21f3561d..9e93472cf5 100644 --- a/libs/vault/src/cipher-form/cipher-form-container.ts +++ b/libs/vault/src/cipher-form/cipher-form-container.ts @@ -46,5 +46,9 @@ export abstract class CipherFormContainer { group: Exclude, ): void; - abstract patchCipher(cipher: Partial): void; + /** + * Method to update the cipherView with the new values. This method should be called by the child form components + * @param updateFn - A function that takes the current cipherView and returns the updated cipherView + */ + abstract patchCipher(updateFn: (current: CipherView) => CipherView): void; } 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 d488fc9db9..15784f1ca0 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 @@ -4,6 +4,7 @@ import { mock, MockProxy } from "jest-mock-extended"; import { BehaviorSubject } from "rxjs"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { CipherRepromptType } from "@bitwarden/common/vault/enums"; import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { PasswordRepromptService } from "../../../services/password-reprompt.service"; @@ -73,10 +74,16 @@ describe("AdditionalOptionsSectionComponent", () => { reprompt: true, }); - expect(cipherFormProvider.patchCipher).toHaveBeenCalledWith({ - notes: "new notes", - reprompt: 1, - }); + const expectedCipher = new CipherView(); + expectedCipher.notes = "new notes"; + expectedCipher.reprompt = CipherRepromptType.Password; + + expect(cipherFormProvider.patchCipher).toHaveBeenCalled(); + const patchFn = cipherFormProvider.patchCipher.mock.lastCall[0]; + + const updated = patchFn(new CipherView()); + + expect(updated).toEqual(expectedCipher); }); it("disables 'additionalOptionsForm' when in partial-edit mode", () => { 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 6c061e1eea..4d402b98e3 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 @@ -66,9 +66,10 @@ export class AdditionalOptionsSectionComponent implements OnInit { this.cipherFormContainer.registerChildForm("additionalOptions", this.additionalOptionsForm); this.additionalOptionsForm.valueChanges.pipe(takeUntilDestroyed()).subscribe((value) => { - this.cipherFormContainer.patchCipher({ - notes: value.notes, - reprompt: value.reprompt ? CipherRepromptType.Password : CipherRepromptType.None, + this.cipherFormContainer.patchCipher((cipher) => { + cipher.notes = value.notes; + cipher.reprompt = value.reprompt ? CipherRepromptType.Password : CipherRepromptType.None; + return cipher; }); }); } 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 bcc86a3d2e..196be144fd 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 @@ -62,9 +62,11 @@ describe("CardDetailsSectionComponent", () => { cardView.number = "4242 4242 4242 4242"; cardView.brand = "Visa"; - expect(patchCipherSpy).toHaveBeenCalledWith({ - card: cardView, - }); + expect(patchCipherSpy).toHaveBeenCalled(); + const patchFn = patchCipherSpy.mock.lastCall[0]; + + const updateCipher = patchFn(new CipherView()); + expect(updateCipher.card).toEqual(cardView); }); it("it converts the year integer to a string", () => { @@ -75,9 +77,11 @@ describe("CardDetailsSectionComponent", () => { const cardView = new CardView(); cardView.expYear = "2022"; - expect(patchCipherSpy).toHaveBeenCalledWith({ - card: cardView, - }); + expect(patchCipherSpy).toHaveBeenCalled(); + const patchFn = patchCipherSpy.mock.lastCall[0]; + + const updatedCipher = patchFn(new CipherView()); + expect(updatedCipher.card).toEqual(cardView); }); it('disables `cardDetailsForm` when "disabled" is true', () => { 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 ecbddc3655..a80954a044 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 @@ -90,9 +90,6 @@ export class CardDetailsSectionComponent implements OnInit { { name: "12 - " + this.i18nService.t("december"), value: "12" }, ]; - /** Local CardView, either created empty or set to the existing card instance */ - private cardView: CardView; - constructor( private cipherFormContainer: CipherFormContainer, private formBuilder: FormBuilder, @@ -103,21 +100,21 @@ export class CardDetailsSectionComponent implements OnInit { this.cardDetailsForm.valueChanges .pipe(takeUntilDestroyed()) .subscribe(({ cardholderName, number, brand, expMonth, expYear, code }) => { - // The input[type="number"] is returning a number, convert it to a string - // An empty field returns null, avoid casting `"null"` to a string - const expirationYear = expYear !== null ? `${expYear}` : null; + this.cipherFormContainer.patchCipher((cipher) => { + // The input[type="number"] is returning a number, convert it to a string + // An empty field returns null, avoid casting `"null"` to a string + const expirationYear = expYear !== null ? `${expYear}` : null; - const patchedCard = Object.assign(this.cardView, { - cardholderName, - number, - brand, - expMonth, - expYear: expirationYear, - code, - }); + Object.assign(cipher.card, { + cardholderName, + number, + brand, + expMonth, + expYear: expirationYear, + code, + }); - this.cipherFormContainer.patchCipher({ - card: patchedCard, + return cipher; }); }); @@ -133,9 +130,6 @@ export class CardDetailsSectionComponent implements OnInit { } ngOnInit() { - // If the original cipher has a card, use it. Otherwise, create a new card instance - this.cardView = this.originalCipherView?.card ?? new CardView(); - if (this.originalCipherView?.card) { this.setInitialValues(); } 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 0f107b247e..b666e6833b 100644 --- a/libs/vault/src/cipher-form/components/cipher-form.component.ts +++ b/libs/vault/src/cipher-form/components/cipher-form.component.ts @@ -143,12 +143,11 @@ export class CipherFormComponent implements AfterViewInit, OnInit, OnChanges, Ci } /** - * Patches the updated cipher with the provided partial cipher. Used by child components to update the cipher - * as their form values change. - * @param cipher + * Method to update the cipherView with the new values. This method should be called by the child form components + * @param updateFn - A function that takes the current cipherView and returns the updated cipherView */ - patchCipher(cipher: Partial): void { - this.updatedCipherView = Object.assign(this.updatedCipherView, cipher); + patchCipher(updateFn: (current: CipherView) => CipherView): void { + this.updatedCipherView = updateFn(this.updatedCipherView); } /** 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 9ae7f10e8f..036a59672e 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 @@ -270,7 +270,11 @@ describe("CustomFieldsComponent", () => { fieldView.value = "new text value"; fieldView.type = FieldType.Text; - expect(patchCipher).toHaveBeenCalledWith({ fields: [fieldView] }); + expect(patchCipher).toHaveBeenCalled(); + const patchFn = patchCipher.mock.lastCall[0]; + + const updatedCipher = patchFn(new CipherView()); + expect(updatedCipher.fields).toEqual([fieldView]); }); it("updates the label", () => { @@ -281,7 +285,11 @@ describe("CustomFieldsComponent", () => { fieldView.value = "text value"; fieldView.type = FieldType.Text; - expect(patchCipher).toHaveBeenCalledWith({ fields: [fieldView] }); + expect(patchCipher).toHaveBeenCalled(); + const patchFn = patchCipher.mock.lastCall[0]; + + const updatedCipher = patchFn(new CipherView()); + expect(updatedCipher.fields).toEqual([fieldView]); }); }); @@ -295,7 +303,11 @@ describe("CustomFieldsComponent", () => { it("removes the field", () => { component.removeField(0); - expect(patchCipher).toHaveBeenCalledWith({ fields: [] }); + expect(patchCipher).toHaveBeenCalled(); + const patchFn = patchCipher.mock.lastCall[0]; + + const updatedCipher = patchFn(new CipherView()); + expect(updatedCipher.fields).toEqual([]); }); }); @@ -325,9 +337,12 @@ describe("CustomFieldsComponent", () => { // Move second field to first component.drop({ previousIndex: 0, currentIndex: 1 } as CdkDragDrop); - const latestCallParams = patchCipher.mock.lastCall[0]; + expect(patchCipher).toHaveBeenCalled(); + const patchFn = patchCipher.mock.lastCall[0]; - expect(latestCallParams.fields.map((f: FieldView) => f.name)).toEqual([ + const updatedCipher = patchFn(new CipherView()); + + expect(updatedCipher.fields.map((f: FieldView) => f.name)).toEqual([ "hidden label", "text label", "boolean label", @@ -342,9 +357,12 @@ describe("CustomFieldsComponent", () => { preventDefault: jest.fn(), }); - const latestCallParams = patchCipher.mock.lastCall[0]; + expect(patchCipher).toHaveBeenCalled(); + const patchFn = patchCipher.mock.lastCall[0]; - expect(latestCallParams.fields.map((f: FieldView) => f.name)).toEqual([ + const updatedCipher = patchFn(new CipherView()); + + expect(updatedCipher.fields.map((f: FieldView) => f.name)).toEqual([ "text label", "hidden label", "linked label", @@ -356,9 +374,12 @@ describe("CustomFieldsComponent", () => { // Move 2nd item (hidden label) up to 1st toggleItems[1].triggerEventHandler("keydown", { key: "ArrowUp", preventDefault: jest.fn() }); - const latestCallParams = patchCipher.mock.lastCall[0]; + expect(patchCipher).toHaveBeenCalled(); + const patchFn = patchCipher.mock.lastCall[0]; - expect(latestCallParams.fields.map((f: FieldView) => f.name)).toEqual([ + const updatedCipher = patchFn(new CipherView()); + + expect(updatedCipher.fields.map((f: FieldView) => f.name)).toEqual([ "hidden label", "text label", "boolean label", 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 44464fa324..e2aa118b88 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 @@ -8,11 +8,11 @@ import { DestroyRef, ElementRef, EventEmitter, + inject, OnInit, Output, QueryList, ViewChildren, - inject, } from "@angular/core"; import { takeUntilDestroyed } from "@angular/core/rxjs-interop"; import { FormArray, FormBuilder, FormsModule, ReactiveFormsModule } from "@angular/forms"; @@ -26,16 +26,16 @@ import { FieldView } from "@bitwarden/common/vault/models/view/field.view"; import { IdentityView } from "@bitwarden/common/vault/models/view/identity.view"; import { LoginView } from "@bitwarden/common/vault/models/view/login.view"; import { + CardComponent, + CheckboxModule, DialogService, + FormFieldModule, + IconButtonModule, + LinkModule, SectionComponent, SectionHeaderComponent, - FormFieldModule, - TypographyModule, - CardComponent, - IconButtonModule, - CheckboxModule, SelectModule, - LinkModule, + TypographyModule, } from "@bitwarden/components"; import { CipherFormContainer } from "../../cipher-form-container"; @@ -344,8 +344,9 @@ export class CustomFieldsComponent implements OnInit, AfterViewInit { this.numberOfFieldsChange.emit(newFields.length); - this.cipherFormContainer.patchCipher({ - fields: newFields, + this.cipherFormContainer.patchCipher((cipher) => { + cipher.fields = newFields; + return cipher; }); } } 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 bc5823dcd1..9e84f8ea6c 100644 --- a/libs/vault/src/cipher-form/components/identity/identity.component.ts +++ b/libs/vault/src/cipher-form/components/identity/identity.component.ts @@ -9,11 +9,11 @@ import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { IdentityView } from "@bitwarden/common/vault/models/view/identity.view"; import { ButtonModule, - SectionComponent, - SectionHeaderComponent, CardComponent, FormFieldModule, IconButtonModule, + SectionComponent, + SectionHeaderComponent, SelectModule, TypographyModule, } from "@bitwarden/components"; @@ -98,8 +98,9 @@ export class IdentitySectionComponent implements OnInit { data.postalCode = value.postalCode; data.country = value.country; - this.cipherFormContainer.patchCipher({ - identity: data, + this.cipherFormContainer.patchCipher((cipher) => { + cipher.identity = data; + return cipher; }); }); } 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 8ec42f807a..a0a1b4e83f 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 @@ -62,13 +62,17 @@ describe("ItemDetailsSectionComponent", () => { component.config.organizations = [{ id: "org1" } as Organization]; await component.ngOnInit(); tick(); - expect(cipherFormProvider.patchCipher).toHaveBeenLastCalledWith({ - name: "", - organizationId: null, - folderId: null, - collectionIds: [], - favorite: false, - }); + + expect(cipherFormProvider.patchCipher).toHaveBeenCalled(); + const patchFn = cipherFormProvider.patchCipher.mock.lastCall[0]; + + const updatedCipher = patchFn(new CipherView()); + + expect(updatedCipher.name).toBe(""); + expect(updatedCipher.organizationId).toBeNull(); + expect(updatedCipher.folderId).toBeNull(); + expect(updatedCipher.collectionIds).toEqual([]); + expect(updatedCipher.favorite).toBe(false); })); it("should initialize form with values from originalCipher if provided", fakeAsync(async () => { @@ -88,13 +92,16 @@ describe("ItemDetailsSectionComponent", () => { await component.ngOnInit(); tick(); - expect(cipherFormProvider.patchCipher).toHaveBeenLastCalledWith({ - name: "cipher1", - organizationId: "org1", - folderId: "folder1", - collectionIds: ["col1"], - favorite: true, - }); + expect(cipherFormProvider.patchCipher).toHaveBeenCalled(); + const patchFn = cipherFormProvider.patchCipher.mock.lastCall[0]; + + const updatedCipher = patchFn(new CipherView()); + + expect(updatedCipher.name).toBe("cipher1"); + expect(updatedCipher.organizationId).toBe("org1"); + expect(updatedCipher.folderId).toBe("folder1"); + expect(updatedCipher.collectionIds).toEqual(["col1"]); + expect(updatedCipher.favorite).toBe(true); })); it("should disable organizationId control if ownership change is not allowed", async () => { @@ -294,11 +301,12 @@ describe("ItemDetailsSectionComponent", () => { fixture.detectChanges(); await fixture.whenStable(); - expect(cipherFormProvider.patchCipher).toHaveBeenLastCalledWith( - expect.objectContaining({ - collectionIds: ["col1", "col2"], - }), - ); + expect(cipherFormProvider.patchCipher).toHaveBeenCalled(); + const patchFn = cipherFormProvider.patchCipher.mock.lastCall[0]; + + const updatedCipher = patchFn(new CipherView()); + + expect(updatedCipher.collectionIds).toEqual(["col1", "col2"]); }); it("should automatically select the first collection if only one is available", async () => { 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 bb0300cb8f..75cb160c92 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 @@ -110,12 +110,15 @@ export class ItemDetailsSectionComponent implements OnInit { map(() => this.itemDetailsForm.getRawValue()), ) .subscribe((value) => { - this.cipherFormContainer.patchCipher({ - name: value.name, - organizationId: value.organizationId, - folderId: value.folderId, - collectionIds: value.collectionIds?.map((c) => c.id) || [], - favorite: value.favorite, + this.cipherFormContainer.patchCipher((cipher) => { + Object.assign(cipher, { + name: value.name, + organizationId: value.organizationId, + folderId: value.folderId, + collectionIds: value.collectionIds?.map((c) => c.id) || [], + favorite: value.favorite, + } as CipherView); + return cipher; }); }); } @@ -212,7 +215,6 @@ export class ItemDetailsSectionComponent implements OnInit { this.itemDetailsForm.controls.favorite.enable(); this.itemDetailsForm.controls.folderId.enable(); } else if (this.config.mode === "edit") { - // this.readOnlyCollections = this.collections .filter( (c) => c.readOnly && this.originalCipherView.collectionIds.includes(c.id as CollectionId), 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 610c451c95..0fe4b128d3 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 @@ -70,13 +70,14 @@ describe("LoginDetailsSectionComponent", () => { totp: "123456", }); - expect(cipherFormContainer.patchCipher).toHaveBeenLastCalledWith({ - login: expect.objectContaining({ - username: "new-username", - password: "secret-password", - totp: "123456", - }), - }); + expect(cipherFormContainer.patchCipher).toHaveBeenCalled(); + const patchFn = cipherFormContainer.patchCipher.mock.lastCall[0]; + + const updatedCipher = patchFn(new CipherView()); + + expect(updatedCipher.login.username).toBe("new-username"); + expect(updatedCipher.login.password).toBe("secret-password"); + expect(updatedCipher.login.totp).toBe("123456"); }); it("disables 'loginDetailsForm' when in partial-edit mode", async () => { @@ -154,12 +155,13 @@ describe("LoginDetailsSectionComponent", () => { username: "new-username", }); - expect(cipherFormContainer.patchCipher).toHaveBeenLastCalledWith({ - login: expect.objectContaining({ - username: "new-username", - password: "original-password", - }), - }); + expect(cipherFormContainer.patchCipher).toHaveBeenCalled(); + const patchFn = cipherFormContainer.patchCipher.mock.lastCall[0]; + + const updatedCipher = patchFn(new CipherView()); + + expect(updatedCipher.login.username).toBe("new-username"); + expect(updatedCipher.login.password).toBe("original-password"); }); }); @@ -493,11 +495,13 @@ describe("LoginDetailsSectionComponent", () => { tick(); - expect(cipherFormContainer.patchCipher).toHaveBeenLastCalledWith({ - login: expect.objectContaining({ - fido2Credentials: null, - }), - }); + expect(cipherFormContainer.patchCipher).toHaveBeenCalled(); + const patchFn = cipherFormContainer.patchCipher.mock.lastCall[0]; + + const updatedCipher = patchFn(new CipherView()); + + expect(updatedCipher.login.fido2Credentials).toBeNull(); + expect(component.hasPasskey).toBe(false); })); }); }); 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 e88e1c0a5f..61a51fadaa 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 @@ -7,6 +7,7 @@ import { map } from "rxjs"; import { JslibModule } from "@bitwarden/angular/jslib.module"; import { AuditService } from "@bitwarden/common/abstractions/audit.service"; import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { Fido2CredentialView } from "@bitwarden/common/vault/models/view/fido2-credential.view"; import { LoginView } from "@bitwarden/common/vault/models/view/login.view"; import { AsyncActionsModule, @@ -58,16 +59,21 @@ export class LoginDetailsSectionComponent implements OnInit { private datePipe = inject(DatePipe); - private loginView: LoginView; + /** + * A local reference to the Fido2 credentials for an existing login being edited. + * These cannot be created in the form and thus have no form control. + * @private + */ + private existingFido2Credentials?: Fido2CredentialView[]; get hasPasskey(): boolean { - return this.loginView?.hasFido2Credentials; + return this.existingFido2Credentials != null && this.existingFido2Credentials.length > 0; } get fido2CredentialCreationDateValue(): string { const dateCreated = this.i18nService.t("dateCreated"); const creationDate = this.datePipe.transform( - this.loginView?.fido2Credentials?.[0]?.creationDate, + this.existingFido2Credentials?.[0]?.creationDate, "short", ); return `${dateCreated} ${creationDate}`; @@ -98,20 +104,19 @@ export class LoginDetailsSectionComponent implements OnInit { map(() => this.loginDetailsForm.getRawValue()), ) .subscribe((value) => { - Object.assign(this.loginView, { - username: value.username, - password: value.password, - totp: value.totp, - } as LoginView); + this.cipherFormContainer.patchCipher((cipher) => { + Object.assign(cipher.login, { + username: value.username, + password: value.password, + totp: value.totp, + } as LoginView); - this.cipherFormContainer.patchCipher({ - login: this.loginView, + return cipher; }); }); } async ngOnInit() { - this.loginView = new LoginView(); if (this.cipherFormContainer.originalCipherView?.login) { this.initFromExistingCipher(this.cipherFormContainer.originalCipherView.login); } else { @@ -124,15 +129,14 @@ export class LoginDetailsSectionComponent implements OnInit { } private initFromExistingCipher(existingLogin: LoginView) { - // Note: this.loginView will still contain references to the existing login's Uri and Fido2Credential arrays. - // We may need to deep clone these in the future. - Object.assign(this.loginView, existingLogin); this.loginDetailsForm.patchValue({ - username: this.loginView.username, - password: this.loginView.password, - totp: this.loginView.totp, + username: existingLogin.username, + password: existingLogin.password, + totp: existingLogin.totp, }); + this.existingFido2Credentials = existingLogin.fido2Credentials; + if (!this.viewHiddenFields) { this.loginDetailsForm.controls.password.disable(); this.loginDetailsForm.controls.totp.disable(); @@ -170,9 +174,10 @@ export class LoginDetailsSectionComponent implements OnInit { removePasskey = async () => { // Fido2Credentials do not have a form control, so update directly - this.loginView.fido2Credentials = null; - this.cipherFormContainer.patchCipher({ - login: this.loginView, + this.existingFido2Credentials = null; + this.cipherFormContainer.patchCipher((cipher) => { + cipher.login.fido2Credentials = null; + return cipher; }); };