From ef4ea183e9493037e7db65fb67327ffa538f9674 Mon Sep 17 00:00:00 2001 From: Nick Krantz <125900171+nick-livefront@users.noreply.github.com> Date: Tue, 20 Aug 2024 19:12:10 -0500 Subject: [PATCH] [PM-10974] Edit/Add Cipher - back handling (#10620) * update back logic for add/edit component to account for how the user arrived at the add/edit screen * add tests for handle back * update comment for cipher saved navigation --- .../add-edit/add-edit-v2.component.spec.ts | 192 ++++++++++++++++++ .../add-edit/add-edit-v2.component.ts | 24 ++- 2 files changed, 206 insertions(+), 10 deletions(-) create mode 100644 apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.spec.ts diff --git a/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.spec.ts new file mode 100644 index 0000000000..95288f6b41 --- /dev/null +++ b/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.spec.ts @@ -0,0 +1,192 @@ +import { ComponentFixture, fakeAsync, TestBed, tick } from "@angular/core/testing"; +import { ActivatedRoute, Router } from "@angular/router"; +import { mock } from "jest-mock-extended"; +import { BehaviorSubject } from "rxjs"; + +import { ConfigService } from "@bitwarden/common/platform/abstractions/config/config.service"; +import { I18nService } from "@bitwarden/common/platform/abstractions/i18n.service"; +import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; +import { Cipher } from "@bitwarden/common/vault/models/domain/cipher"; +import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; +import { CipherFormConfig, CipherFormConfigService, CipherFormMode } from "@bitwarden/vault"; + +import { BrowserFido2UserInterfaceSession } from "../../../../../autofill/fido2/services/browser-fido2-user-interface.service"; +import BrowserPopupUtils from "../../../../../platform/popup/browser-popup-utils"; +import { PopupRouterCacheService } from "../../../../../platform/popup/view-cache/popup-router-cache.service"; +import { PopupCloseWarningService } from "../../../../../popup/services/popup-close-warning.service"; + +import { AddEditV2Component } from "./add-edit-v2.component"; + +// 'qrcode-parser' is used by `BrowserTotpCaptureService` but is an es6 module that jest can't compile. +// Mock the entire module here to prevent jest from throwing an error. I wasn't able to find a way to mock the +// `BrowserTotpCaptureService` where jest would not load the file in the first place. +jest.mock("qrcode-parser", () => {}); + +describe("AddEditV2Component", () => { + let component: AddEditV2Component; + let fixture: ComponentFixture; + + const buildConfigResponse = { originalCipher: {} } as CipherFormConfig; + const buildConfig = jest.fn((mode: CipherFormMode) => + Promise.resolve({ mode, ...buildConfigResponse }), + ); + const queryParams$ = new BehaviorSubject({}); + const disable = jest.fn(); + const navigate = jest.fn(); + const back = jest.fn().mockResolvedValue(null); + + beforeEach(async () => { + buildConfig.mockClear(); + disable.mockClear(); + navigate.mockClear(); + back.mockClear(); + + await TestBed.configureTestingModule({ + imports: [AddEditV2Component], + providers: [ + { provide: PlatformUtilsService, useValue: mock() }, + { provide: ConfigService, useValue: mock() }, + { provide: PopupRouterCacheService, useValue: { back } }, + { provide: PopupCloseWarningService, useValue: { disable } }, + { provide: Router, useValue: { navigate } }, + { provide: ActivatedRoute, useValue: { queryParams: queryParams$ } }, + { provide: I18nService, useValue: { t: (key: string) => key } }, + ], + }) + .overrideProvider(CipherFormConfigService, { + useValue: { + buildConfig, + }, + }) + .compileComponents(); + + fixture = TestBed.createComponent(AddEditV2Component); + component = fixture.componentInstance; + fixture.detectChanges(); + }); + + describe("query params", () => { + describe("mode", () => { + it("sets mode to `add` when no `cipherId` is provided", fakeAsync(() => { + queryParams$.next({}); + + tick(); + + expect(buildConfig.mock.lastCall[0]).toBe("add"); + expect(component.config.mode).toBe("add"); + })); + + it("sets mode to `edit` when `params.clone` is not provided", fakeAsync(() => { + queryParams$.next({ cipherId: "222-333-444-5555", clone: "true" }); + + tick(); + + expect(buildConfig.mock.lastCall[0]).toBe("clone"); + expect(component.config.mode).toBe("clone"); + })); + + it("sets mode to `edit` when `params.clone` is not provided", fakeAsync(() => { + buildConfigResponse.originalCipher = { edit: true } as Cipher; + queryParams$.next({ cipherId: "222-333-444-5555" }); + + tick(); + + expect(buildConfig.mock.lastCall[0]).toBe("edit"); + expect(component.config.mode).toBe("edit"); + })); + + it("sets mode to `partial-edit` when `config.originalCipher.edit` is false", fakeAsync(() => { + buildConfigResponse.originalCipher = { edit: false } as Cipher; + queryParams$.next({ cipherId: "222-333-444-5555" }); + + tick(); + + expect(buildConfig.mock.lastCall[0]).toBe("edit"); + expect(component.config.mode).toBe("partial-edit"); + })); + }); + }); + + describe("onCipherSaved", () => { + it("disables warning when in popout", async () => { + jest.spyOn(BrowserPopupUtils, "inPopout").mockReturnValueOnce(true); + + await component.onCipherSaved({ id: "123-456-789" } as CipherView); + + expect(disable).toHaveBeenCalled(); + }); + + it("calls `confirmNewCredentialResponse` when in fido2 popout", async () => { + // @ts-expect-error - `inFido2PopoutWindow` is a private getter, mock the response here + // for the test rather than setting up the dependencies. + jest.spyOn(component, "inFido2PopoutWindow", "get").mockReturnValueOnce(true); + + await component.onCipherSaved({ id: "123-456-789" } as CipherView); + + expect(BrowserPopupUtils.inPopout).toHaveBeenCalled(); + expect(navigate).not.toHaveBeenCalled(); + }); + + it("closes single action popout", async () => { + jest.spyOn(BrowserPopupUtils, "inSingleActionPopout").mockReturnValueOnce(true); + jest.spyOn(BrowserPopupUtils, "closeSingleActionPopout").mockResolvedValue(); + + await component.onCipherSaved({ id: "123-456-789" } as CipherView); + + expect(BrowserPopupUtils.closeSingleActionPopout).toHaveBeenCalled(); + expect(navigate).not.toHaveBeenCalled(); + }); + + it("navigates to view-cipher for new ciphers", async () => { + component.config.mode = "add"; + + await component.onCipherSaved({ id: "123-456-789" } as CipherView); + + expect(navigate).toHaveBeenCalledWith(["/view-cipher"], { + replaceUrl: true, + queryParams: { cipherId: "123-456-789" }, + }); + expect(back).not.toHaveBeenCalled(); + }); + + it("navigates to view-cipher for edit ciphers", async () => { + component.config.mode = "edit"; + + await component.onCipherSaved({ id: "123-456-789" } as CipherView); + + expect(navigate).not.toHaveBeenCalled(); + expect(back).toHaveBeenCalled(); + }); + }); + + describe("handleBackButton", () => { + it("disables warning and aborts fido2 popout", async () => { + // @ts-expect-error - `inFido2PopoutWindow` is a private getter, mock the response here + // for the test rather than setting up the dependencies. + jest.spyOn(component, "inFido2PopoutWindow", "get").mockReturnValueOnce(true); + jest.spyOn(BrowserFido2UserInterfaceSession, "abortPopout"); + + await component.handleBackButton(); + + expect(disable).toHaveBeenCalled(); + expect(BrowserFido2UserInterfaceSession.abortPopout).toHaveBeenCalled(); + expect(back).not.toHaveBeenCalled(); + }); + + it("closes single action popout", async () => { + jest.spyOn(BrowserPopupUtils, "inSingleActionPopout").mockReturnValueOnce(true); + jest.spyOn(BrowserPopupUtils, "closeSingleActionPopout").mockResolvedValue(); + + await component.handleBackButton(); + + expect(BrowserPopupUtils.closeSingleActionPopout).toHaveBeenCalled(); + expect(back).not.toHaveBeenCalled(); + }); + + it("navigates the user backwards", async () => { + await component.handleBackButton(); + + expect(back).toHaveBeenCalled(); + }); + }); +}); diff --git a/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.ts b/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.ts index 6cf2ba0e7d..b830ae7504 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/add-edit/add-edit-v2.component.ts @@ -151,10 +151,10 @@ export class AddEditV2Component implements OnInit { constructor( private route: ActivatedRoute, - private popupRouterCacheService: PopupRouterCacheService, private i18nService: I18nService, private addEditFormConfigService: CipherFormConfigService, private popupCloseWarningService: PopupCloseWarningService, + private popupRouterCacheService: PopupRouterCacheService, private router: Router, ) { this.subscribeToParams(); @@ -183,11 +183,7 @@ export class AddEditV2Component implements OnInit { }; /** - * Navigates to previous view or view-cipher path - * depending on the history length. - * - * This can happen when history is lost due to the extension being - * forced into a popout window. + * Handle back button */ async handleBackButton() { if (this.inFido2PopoutWindow) { @@ -223,10 +219,18 @@ export class AddEditV2Component implements OnInit { return; } - await this.router.navigate(["/view-cipher"], { - replaceUrl: true, - queryParams: { cipherId: cipher.id }, - }); + // When the cipher is in edit / partial edit, the previous page was the view-cipher page. + // In the case of creating a new cipher, the user should go view-cipher page but we need to also + // remove it from the history stack. This avoids the user having to click back twice on the + // view-cipher page. + if (this.config.mode === "edit" || this.config.mode === "partial-edit") { + await this.popupRouterCacheService.back(); + } else { + await this.router.navigate(["/view-cipher"], { + replaceUrl: true, + queryParams: { cipherId: cipher.id }, + }); + } } subscribeToParams(): void {