From 7db844baf28ffae91e66d654ef5780b922ab891a Mon Sep 17 00:00:00 2001 From: Cesar Gonzalez Date: Fri, 8 Dec 2023 16:26:29 -0600 Subject: [PATCH] [PM-5048] Query params are not persisting in extension popout window (#7019) * [PM-5048] Query params are not persisting in extension popout window * [PM-5048] Reverting how we identify whether the user is popping out the current tab; * [PM-5048] Adding jest test to validate skipping parsing of the extension path * [PM-5048] Adding jest test to validate skipping parsing of the extension path * [PM-5048] Reworking implementation to ensure that popout window query params remain persistent * [PM-5048] Reworking implementation to ensure that appended query params do not remain persistent, but popout window query params do remain persistent. Fixing issues within vault item popouts and adding jest tests to verify those fixes * [PM-5048] Modifying how query params are set within vault popout windows * [PM-5048] Running prettier --- .../popup/browser-popup-utils.spec.ts | 23 ++++------ .../src/platform/popup/browser-popup-utils.ts | 22 +++------- .../popup/utils/vault-popout-window.spec.ts | 38 ++++++++++++++-- .../vault/popup/utils/vault-popout-window.ts | 43 +++++++++++++++---- 4 files changed, 82 insertions(+), 44 deletions(-) diff --git a/apps/browser/src/platform/popup/browser-popup-utils.spec.ts b/apps/browser/src/platform/popup/browser-popup-utils.spec.ts index 6db22a10ca..d640a3bc23 100644 --- a/apps/browser/src/platform/popup/browser-popup-utils.spec.ts +++ b/apps/browser/src/platform/popup/browser-popup-utils.spec.ts @@ -191,25 +191,18 @@ describe("BrowserPopupUtils", () => { }); }); - it("replaces any existing `uilocation=` query params within the passed extension url path to state the the uilocaiton is a popup", async () => { - const url = "popup/index.html#/tabs/vault?uilocation=sidebar"; + it("skips parsing the passed extension url path if the option to do that is set", () => { + const url = "popup/index.html?uilocation=popout#/tabs/vault"; jest.spyOn(BrowserPopupUtils as any, "isSingleActionPopoutOpen").mockResolvedValueOnce(false); + jest.spyOn(BrowserPopupUtils as any, "buildPopoutUrl"); - await BrowserPopupUtils.openPopout(url); + BrowserPopupUtils.openPopout(url); - expect(BrowserApi.createWindow).toHaveBeenCalledWith({ - type: "popup", - focused: true, - width: 380, - height: 630, - left: 85, - top: 190, - url: `chrome-extension://id/popup/index.html#/tabs/vault?uilocation=popout`, - }); + expect(BrowserPopupUtils["buildPopoutUrl"]).not.toHaveBeenCalled(); }); - it("appends the uilocation to the search params if an existing param is passed with the extension url path", async () => { - const url = "popup/index.html#/tabs/vault?existingParam=123"; + it("replaces any existing `uilocation=` query params within the passed extension url path to state the the uilocaiton is a popup", async () => { + const url = "popup/index.html?uilocation=sidebar#/tabs/vault"; jest.spyOn(BrowserPopupUtils as any, "isSingleActionPopoutOpen").mockResolvedValueOnce(false); await BrowserPopupUtils.openPopout(url); @@ -221,7 +214,7 @@ describe("BrowserPopupUtils", () => { height: 630, left: 85, top: 190, - url: `chrome-extension://id/${url}&uilocation=popout`, + url: `chrome-extension://id/popup/index.html?uilocation=popout#/tabs/vault`, }); }); diff --git a/apps/browser/src/platform/popup/browser-popup-utils.ts b/apps/browser/src/platform/popup/browser-popup-utils.ts index 3872c6b9ca..8cc060969d 100644 --- a/apps/browser/src/platform/popup/browser-popup-utils.ts +++ b/apps/browser/src/platform/popup/browser-popup-utils.ts @@ -127,9 +127,7 @@ class BrowserPopupUtils { top: senderWindow.top + offsetTop, ...defaultPopoutWindowOptions, ...windowOptions, - url: chrome.runtime.getURL( - BrowserPopupUtils.buildPopoutUrlPath(extensionUrlPath, singleActionKey), - ), + url: BrowserPopupUtils.buildPopoutUrl(extensionUrlPath, singleActionKey), }; if ( @@ -252,23 +250,15 @@ class BrowserPopupUtils { * @param extensionUrlPath - A relative path to the extension page. Example: "popup/index.html#/tabs/vault" * @param singleActionKey - The single action popout key used to identify the popout. */ - private static buildPopoutUrlPath(extensionUrlPath: string, singleActionKey: string) { - let formattedExtensionUrlPath = extensionUrlPath; - if (formattedExtensionUrlPath.includes("uilocation=")) { - formattedExtensionUrlPath = formattedExtensionUrlPath.replace( - /uilocation=[^&]*/g, - "uilocation=popout", - ); - } else { - formattedExtensionUrlPath += - (formattedExtensionUrlPath.includes("?") ? "&" : "?") + "uilocation=popout"; - } + private static buildPopoutUrl(extensionUrlPath: string, singleActionKey: string) { + const parsedUrl = new URL(chrome.runtime.getURL(extensionUrlPath)); + parsedUrl.searchParams.set("uilocation", "popout"); if (singleActionKey) { - formattedExtensionUrlPath += `&singleActionPopout=${singleActionKey}`; + parsedUrl.searchParams.set("singleActionPopout", singleActionKey); } - return formattedExtensionUrlPath; + return parsedUrl.toString(); } } diff --git a/apps/browser/src/vault/popup/utils/vault-popout-window.spec.ts b/apps/browser/src/vault/popup/utils/vault-popout-window.spec.ts index 883e5c8b9e..84b6101c1c 100644 --- a/apps/browser/src/vault/popup/utils/vault-popout-window.spec.ts +++ b/apps/browser/src/vault/popup/utils/vault-popout-window.spec.ts @@ -5,12 +5,14 @@ import { CipherType } from "@bitwarden/common/vault/enums"; import BrowserPopupUtils from "../../../platform/popup/browser-popup-utils"; import { + openViewVaultItemPopout, closeAddEditVaultItemPopout, closeFido2Popout, openAddEditVaultItemPopout, openFido2Popout, openVaultItemPasswordRepromptPopout, VaultPopoutType, + closeViewVaultItemPopout, } from "./vault-popout-window"; describe("VaultPopoutWindow", () => { @@ -25,6 +27,34 @@ describe("VaultPopoutWindow", () => { jest.clearAllMocks(); }); + describe("openViewVaultItemPopout", () => { + it("opens a popout window that contains a sender tab id query param reference", async () => { + const senderTab = { id: 1, windowId: 2 } as chrome.tabs.Tab; + + await openViewVaultItemPopout(senderTab, { + cipherId: "cipherId", + action: "action", + }); + + expect(openPopoutSpy).toHaveBeenCalledWith( + "popup/index.html#/view-cipher?cipherId=cipherId&senderTabId=1&action=action", + { + singleActionKey: `${VaultPopoutType.viewVaultItem}_cipherId`, + senderWindowId: 2, + forceCloseExistingWindows: undefined, + }, + ); + }); + }); + + describe("closeViewVaultItemPopout", () => { + it("closes the view vault item popout window", async () => { + await closeViewVaultItemPopout("cipherId"); + + expect(closeSingleActionPopoutSpy).toHaveBeenCalledWith(`cipherId`, 0); + }); + }); + describe("openVaultItemPasswordRepromptPopout", () => { it("opens a popout window that facilitates re-prompting for the password of a vault item", async () => { const senderTab = { windowId: 1 } as chrome.tabs.Tab; @@ -35,7 +65,7 @@ describe("VaultPopoutWindow", () => { }); expect(openPopoutSpy).toHaveBeenCalledWith( - "popup/index.html#/view-cipher?uilocation=popout&cipherId=cipherId&action=action", + "popup/index.html#/view-cipher?cipherId=cipherId&action=action", { singleActionKey: `${VaultPopoutType.viewVaultItem}_cipherId`, senderWindowId: 1, @@ -52,7 +82,7 @@ describe("VaultPopoutWindow", () => { ); expect(openPopoutSpy).toHaveBeenCalledWith( - "popup/index.html#/edit-cipher?uilocation=popout&uri=https://jest-testing-website.com", + "popup/index.html#/edit-cipher?uri=https://jest-testing-website.com", { singleActionKey: VaultPopoutType.addEditVaultItem, senderWindowId: 1, @@ -69,7 +99,7 @@ describe("VaultPopoutWindow", () => { ); expect(openPopoutSpy).toHaveBeenCalledWith( - `popup/index.html#/edit-cipher?uilocation=popout&type=${CipherType.Identity}&uri=https://jest-testing-website.com`, + `popup/index.html#/edit-cipher?type=${CipherType.Identity}&uri=https://jest-testing-website.com`, { singleActionKey: `${VaultPopoutType.addEditVaultItem}_${CipherType.Identity}`, senderWindowId: 1, @@ -86,7 +116,7 @@ describe("VaultPopoutWindow", () => { ); expect(openPopoutSpy).toHaveBeenCalledWith( - "popup/index.html#/edit-cipher?uilocation=popout&cipherId=cipherId&uri=https://jest-testing-website.com", + "popup/index.html#/edit-cipher?cipherId=cipherId&uri=https://jest-testing-website.com", { singleActionKey: `${VaultPopoutType.addEditVaultItem}_cipherId`, senderWindowId: 1, diff --git a/apps/browser/src/vault/popup/utils/vault-popout-window.ts b/apps/browser/src/vault/popup/utils/vault-popout-window.ts index 4316b6cd49..00cb4a71b8 100644 --- a/apps/browser/src/vault/popup/utils/vault-popout-window.ts +++ b/apps/browser/src/vault/popup/utils/vault-popout-window.ts @@ -9,6 +9,12 @@ const VaultPopoutType = { fido2Popout: "vault_Fido2Popout", } as const; +/** + * Opens a popout window that facilitates viewing a vault item. + * + * @param senderTab - The tab that sent the request. + * @param cipherOptions - The cipher id and action to perform. + */ async function openViewVaultItemPopout( senderTab: chrome.tabs.Tab, cipherOptions: { @@ -18,15 +24,22 @@ async function openViewVaultItemPopout( }, ) { const { cipherId, action, forceCloseExistingWindows } = cipherOptions; - let promptWindowPath = "popup/index.html#/view-cipher?uilocation=popout"; + let promptWindowPath = "popup/index.html#/view-cipher"; + let queryParamToken = "?"; + const formatQueryString = (key: string, value: string) => { + const queryString = `${queryParamToken}${key}=${value}`; + queryParamToken = "&"; + return queryString; + }; + if (cipherId) { - promptWindowPath += `&cipherId=${cipherId}`; + promptWindowPath += formatQueryString("cipherId", cipherId); } if (senderTab.id) { - promptWindowPath += `&senderTabId=${senderTab.id}`; + promptWindowPath += formatQueryString("senderTabId", String(senderTab.id)); } if (action) { - promptWindowPath += `&action=${action}`; + promptWindowPath += formatQueryString("action", action); } await BrowserPopupUtils.openPopout(promptWindowPath, { @@ -36,6 +49,12 @@ async function openViewVaultItemPopout( }); } +/** + * Closes the view vault item popout window. + * + * @param singleActionKey - The single action popout key used to identify the popout. + * @param delayClose - The amount of time to wait before closing the popout. Defaults to 0. + */ async function closeViewVaultItemPopout(singleActionKey: string, delayClose = 0) { await BrowserPopupUtils.closeSingleActionPopout(singleActionKey, delayClose); } @@ -73,19 +92,25 @@ async function openAddEditVaultItemPopout( ) { const { cipherId, cipherType } = cipherOptions; const { url, windowId } = senderTab; - let singleActionKey = VaultPopoutType.addEditVaultItem; - let addEditCipherUrl = "popup/index.html#/edit-cipher?uilocation=popout"; + let addEditCipherUrl = "popup/index.html#/edit-cipher"; + let queryParamToken = "?"; + const formatQueryString = (key: string, value: string) => { + const queryString = `${queryParamToken}${key}=${value}`; + queryParamToken = "&"; + return queryString; + }; + if (cipherId && !cipherType) { singleActionKey += `_${cipherId}`; - addEditCipherUrl += `&cipherId=${cipherId}`; + addEditCipherUrl += formatQueryString("cipherId", cipherId); } if (cipherType && !cipherId) { singleActionKey += `_${cipherType}`; - addEditCipherUrl += `&type=${cipherType}`; + addEditCipherUrl += formatQueryString("type", String(cipherType)); } if (senderTab.url) { - addEditCipherUrl += `&uri=${url}`; + addEditCipherUrl += formatQueryString("uri", url); } await BrowserPopupUtils.openPopout(addEditCipherUrl, {