diff --git a/apps/browser/src/_locales/en/messages.json b/apps/browser/src/_locales/en/messages.json index 09ea964823c..91f64a6c4eb 100644 --- a/apps/browser/src/_locales/en/messages.json +++ b/apps/browser/src/_locales/en/messages.json @@ -3052,29 +3052,9 @@ "message": "Send saved", "description": "'Send' is a noun and the name of a feature called 'Bitwarden Send'. It should not be translated." }, - "sendFilePopoutDialogText": { - "message": "Pop out extension?", - "description": "'Send' is a noun and the name of a feature called 'Bitwarden Send'. It should not be translated." - }, - "sendFilePopoutDialogDesc": { - "message": "To create a file Send, you need to pop out the extension to a new window.", - "description": "'Send' is a noun and the name of a feature called 'Bitwarden Send'. It should not be translated." - }, - "sendLinuxChromiumFileWarning": { - "message": "In order to choose a file, open the extension in the sidebar (if possible) or pop out to a new window by clicking this banner." - }, - "sendFirefoxFileWarning": { - "message": "In order to choose a file using Firefox, open the extension in the sidebar or pop out to a new window by clicking this banner." - }, - "sendSafariFileWarning": { - "message": "In order to choose a file using Safari, pop out to a new window by clicking this banner." - }, "popOut": { "message": "Pop out" }, - "sendFileCalloutHeader": { - "message": "Before you start" - }, "expirationDateIsInvalid": { "message": "The expiration date provided is not valid." }, diff --git a/apps/browser/src/popup/app-routing.module.ts b/apps/browser/src/popup/app-routing.module.ts index 12e1288e806..69b55aa7eb2 100644 --- a/apps/browser/src/popup/app-routing.module.ts +++ b/apps/browser/src/popup/app-routing.module.ts @@ -68,7 +68,7 @@ import { popupRouterCacheGuard } from "../platform/popup/view-cache/popup-router import { RouteCacheOptions } from "../platform/services/popup-view-cache-background.service"; import { CredentialGeneratorHistoryComponent } from "../tools/popup/generator/credential-generator-history.component"; import { CredentialGeneratorComponent } from "../tools/popup/generator/credential-generator.component"; -import { firefoxPopoutGuard } from "../tools/popup/guards/firefox-popout.guard"; +import { filePickerPopoutGuard } from "../tools/popup/guards/file-picker-popout.guard"; import { SendAddEditComponent as SendAddEditV2Component } from "../tools/popup/send-v2/add-edit/send-add-edit.component"; import { SendCreatedComponent } from "../tools/popup/send-v2/send-created/send-created.component"; import { SendV2Component } from "../tools/popup/send-v2/send-v2.component"; @@ -245,7 +245,7 @@ const routes: Routes = [ { path: "attachments", component: AttachmentsV2Component, - canActivate: [authGuard], + canActivate: [authGuard, filePickerPopoutGuard()], data: { elevation: 4 } satisfies RouteDataProperties, }, { @@ -263,7 +263,7 @@ const routes: Routes = [ { path: "import", component: ImportBrowserV2Component, - canActivate: [authGuard, firefoxPopoutGuard()], + canActivate: [authGuard, filePickerPopoutGuard()], data: { elevation: 1 } satisfies RouteDataProperties, }, { @@ -341,13 +341,13 @@ const routes: Routes = [ { path: "add-send", component: SendAddEditV2Component, - canActivate: [authGuard, firefoxPopoutGuard()], + canActivate: [authGuard, filePickerPopoutGuard()], data: { elevation: 1 } satisfies RouteDataProperties, }, { path: "edit-send", component: SendAddEditV2Component, - canActivate: [authGuard, firefoxPopoutGuard()], + canActivate: [authGuard, filePickerPopoutGuard()], data: { elevation: 1 } satisfies RouteDataProperties, }, { diff --git a/apps/browser/src/popup/app.module.ts b/apps/browser/src/popup/app.module.ts index d178cee2fc3..4ed79dd144d 100644 --- a/apps/browser/src/popup/app.module.ts +++ b/apps/browser/src/popup/app.module.ts @@ -33,7 +33,6 @@ import { PopupFooterComponent } from "../platform/popup/layout/popup-footer.comp import { PopupHeaderComponent } from "../platform/popup/layout/popup-header.component"; import { PopupPageComponent } from "../platform/popup/layout/popup-page.component"; import { PopupTabNavigationComponent } from "../platform/popup/layout/popup-tab-navigation.component"; -import { FilePopoutCalloutComponent } from "../tools/popup/components/file-popout-callout.component"; import { AppRoutingModule } from "./app-routing.module"; import { AppComponent } from "./app.component"; @@ -67,7 +66,6 @@ import "../platform/popup/locales"; ScrollingModule, ServicesModule, DialogModule, - FilePopoutCalloutComponent, AvatarModule, AccountComponent, ButtonModule, diff --git a/apps/browser/src/popup/services/services.module.ts b/apps/browser/src/popup/services/services.module.ts index bb89eff1147..777de5f0d82 100644 --- a/apps/browser/src/popup/services/services.module.ts +++ b/apps/browser/src/popup/services/services.module.ts @@ -204,7 +204,6 @@ import { isNotificationsSupported, } from "../../platform/system-notifications/browser-system-notification.service"; import { fromChromeRuntimeMessaging } from "../../platform/utils/from-chrome-runtime-messaging"; -import { FilePopoutUtilsService } from "../../tools/popup/services/file-popout-utils.service"; import { Fido2UserVerificationService } from "../../vault/services/fido2-user-verification.service"; import { ExtensionAnonLayoutWrapperDataService } from "../components/extension-anon-layout-wrapper/extension-anon-layout-wrapper-data.service"; @@ -475,13 +474,6 @@ const safeProviders: SafeProvider[] = [ }, deps: [PlatformUtilsService], }), - safeProvider({ - provide: FilePopoutUtilsService, - useFactory: (platformUtilsService: PlatformUtilsService) => { - return new FilePopoutUtilsService(platformUtilsService); - }, - deps: [PlatformUtilsService], - }), safeProvider({ provide: DerivedStateProvider, useClass: InlineDerivedStateProvider, diff --git a/apps/browser/src/tools/popup/components/file-popout-callout.component.html b/apps/browser/src/tools/popup/components/file-popout-callout.component.html deleted file mode 100644 index 0df77dc4367..00000000000 --- a/apps/browser/src/tools/popup/components/file-popout-callout.component.html +++ /dev/null @@ -1,11 +0,0 @@ - -
{{ "sendLinuxChromiumFileWarning" | i18n }}
-
{{ "sendFirefoxFileWarning" | i18n }}
-
{{ "sendSafariFileWarning" | i18n }}
- diff --git a/apps/browser/src/tools/popup/components/file-popout-callout.component.ts b/apps/browser/src/tools/popup/components/file-popout-callout.component.ts deleted file mode 100644 index 33044b79351..00000000000 --- a/apps/browser/src/tools/popup/components/file-popout-callout.component.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { CommonModule } from "@angular/common"; -import { Component, OnInit } from "@angular/core"; - -import { JslibModule } from "@bitwarden/angular/jslib.module"; -import { CalloutModule } from "@bitwarden/components"; - -import BrowserPopupUtils from "../../../platform/browser/browser-popup-utils"; -import { FilePopoutUtilsService } from "../services/file-popout-utils.service"; - -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection -@Component({ - selector: "tools-file-popout-callout", - templateUrl: "file-popout-callout.component.html", - imports: [CommonModule, JslibModule, CalloutModule], -}) -export class FilePopoutCalloutComponent implements OnInit { - protected showFilePopoutMessage: boolean = false; - protected showFirefoxFileWarning: boolean = false; - protected showSafariFileWarning: boolean = false; - protected showChromiumFileWarning: boolean = false; - - constructor(private filePopoutUtilsService: FilePopoutUtilsService) {} - - ngOnInit() { - this.showFilePopoutMessage = this.filePopoutUtilsService.showFilePopoutMessage(window); - this.showFirefoxFileWarning = this.filePopoutUtilsService.showFirefoxFileWarning(window); - this.showSafariFileWarning = this.filePopoutUtilsService.showSafariFileWarning(window); - this.showChromiumFileWarning = this.filePopoutUtilsService.showChromiumFileWarning(window); - } - - popOutWindow() { - // FIXME: Verify that this floating promise is intentional. If it is, add an explanatory comment and ensure there is proper error handling. - // eslint-disable-next-line @typescript-eslint/no-floating-promises - BrowserPopupUtils.openCurrentPagePopout(window); - } -} diff --git a/apps/browser/src/tools/popup/guards/file-picker-popout.guard.spec.ts b/apps/browser/src/tools/popup/guards/file-picker-popout.guard.spec.ts new file mode 100644 index 00000000000..14fa300c3c1 --- /dev/null +++ b/apps/browser/src/tools/popup/guards/file-picker-popout.guard.spec.ts @@ -0,0 +1,380 @@ +import { TestBed } from "@angular/core/testing"; +import { ActivatedRouteSnapshot, RouterStateSnapshot } from "@angular/router"; + +import { DeviceType } from "@bitwarden/common/enums"; + +import { BrowserApi } from "../../../platform/browser/browser-api"; +import BrowserPopupUtils from "../../../platform/browser/browser-popup-utils"; +import { BrowserPlatformUtilsService } from "../../../platform/services/platform-utils/browser-platform-utils.service"; + +import { filePickerPopoutGuard } from "./file-picker-popout.guard"; + +describe("filePickerPopoutGuard", () => { + let getDeviceSpy: jest.SpyInstance; + let inPopoutSpy: jest.SpyInstance; + let inSidebarSpy: jest.SpyInstance; + let openPopoutSpy: jest.SpyInstance; + let closePopupSpy: jest.SpyInstance; + + const mockRoute = {} as ActivatedRouteSnapshot; + const mockState: RouterStateSnapshot = { + url: "/add-send?type=1", + } as RouterStateSnapshot; + + beforeEach(() => { + getDeviceSpy = jest.spyOn(BrowserPlatformUtilsService, "getDevice"); + inPopoutSpy = jest.spyOn(BrowserPopupUtils, "inPopout"); + inSidebarSpy = jest.spyOn(BrowserPopupUtils, "inSidebar"); + openPopoutSpy = jest.spyOn(BrowserPopupUtils, "openPopout").mockImplementation(); + closePopupSpy = jest.spyOn(BrowserApi, "closePopup").mockImplementation(); + + TestBed.configureTestingModule({}); + }); + + afterEach(() => { + jest.clearAllMocks(); + }); + + describe("Firefox browser", () => { + beforeEach(() => { + getDeviceSpy.mockReturnValue(DeviceType.FirefoxExtension); + inPopoutSpy.mockReturnValue(false); + inSidebarSpy.mockReturnValue(false); + }); + + it("should open popout and block navigation when not in popout or sidebar", async () => { + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(getDeviceSpy).toHaveBeenCalledWith(window); + expect(inPopoutSpy).toHaveBeenCalledWith(window); + expect(inSidebarSpy).toHaveBeenCalledWith(window); + expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/add-send?type=1"); + expect(closePopupSpy).toHaveBeenCalledWith(window); + expect(result).toBe(false); + }); + + it("should allow navigation when already in popout", async () => { + inPopoutSpy.mockReturnValue(true); + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).not.toHaveBeenCalled(); + expect(closePopupSpy).not.toHaveBeenCalled(); + expect(result).toBe(true); + }); + + it("should allow navigation when already in sidebar", async () => { + inSidebarSpy.mockReturnValue(true); + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).not.toHaveBeenCalled(); + expect(closePopupSpy).not.toHaveBeenCalled(); + expect(result).toBe(true); + }); + }); + + describe("Safari browser", () => { + beforeEach(() => { + getDeviceSpy.mockReturnValue(DeviceType.SafariExtension); + inPopoutSpy.mockReturnValue(false); + inSidebarSpy.mockReturnValue(false); + }); + + it("should open popout and block navigation when not in popout", async () => { + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(getDeviceSpy).toHaveBeenCalledWith(window); + expect(inPopoutSpy).toHaveBeenCalledWith(window); + expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/add-send?type=1"); + expect(closePopupSpy).toHaveBeenCalledWith(window); + expect(result).toBe(false); + }); + + it("should allow navigation when already in popout", async () => { + inPopoutSpy.mockReturnValue(true); + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).not.toHaveBeenCalled(); + expect(closePopupSpy).not.toHaveBeenCalled(); + expect(result).toBe(true); + }); + + it("should not allow sidebar bypass (Safari doesn't support sidebar)", async () => { + inSidebarSpy.mockReturnValue(true); + inPopoutSpy.mockReturnValue(false); + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + // Safari requires popout, sidebar is not sufficient + expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/add-send?type=1"); + expect(closePopupSpy).toHaveBeenCalledWith(window); + expect(result).toBe(false); + }); + }); + + describe("Chromium browsers on Linux", () => { + beforeEach(() => { + inPopoutSpy.mockReturnValue(false); + inSidebarSpy.mockReturnValue(false); + Object.defineProperty(window, "navigator", { + value: { + userAgent: "Mozilla/5.0 (X11; Linux x86_64)", + appVersion: "5.0 (X11; Linux x86_64)", + }, + configurable: true, + writable: true, + }); + }); + + it.each([ + { deviceType: DeviceType.ChromeExtension, name: "Chrome" }, + { deviceType: DeviceType.EdgeExtension, name: "Edge" }, + { deviceType: DeviceType.OperaExtension, name: "Opera" }, + { deviceType: DeviceType.VivaldiExtension, name: "Vivaldi" }, + ])( + "should open popout and block navigation for $name on Linux when not in popout or sidebar", + async ({ deviceType }) => { + getDeviceSpy.mockReturnValue(deviceType); + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/add-send?type=1"); + expect(closePopupSpy).toHaveBeenCalledWith(window); + expect(result).toBe(false); + }, + ); + + it("should allow navigation when in popout", async () => { + getDeviceSpy.mockReturnValue(DeviceType.ChromeExtension); + inPopoutSpy.mockReturnValue(true); + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).not.toHaveBeenCalled(); + expect(closePopupSpy).not.toHaveBeenCalled(); + expect(result).toBe(true); + }); + + it("should allow navigation when in sidebar", async () => { + getDeviceSpy.mockReturnValue(DeviceType.ChromeExtension); + inSidebarSpy.mockReturnValue(true); + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).not.toHaveBeenCalled(); + expect(closePopupSpy).not.toHaveBeenCalled(); + expect(result).toBe(true); + }); + }); + + describe("Chromium browsers on Mac", () => { + beforeEach(() => { + inPopoutSpy.mockReturnValue(false); + inSidebarSpy.mockReturnValue(false); + Object.defineProperty(window, "navigator", { + value: { + userAgent: "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7)", + appVersion: "5.0 (Macintosh; Intel Mac OS X 10_15_7)", + }, + configurable: true, + writable: true, + }); + }); + + it("should open popout and block navigation for Chrome on Mac when not in popout or sidebar", async () => { + getDeviceSpy.mockReturnValue(DeviceType.ChromeExtension); + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/add-send?type=1"); + expect(closePopupSpy).toHaveBeenCalledWith(window); + expect(result).toBe(false); + }); + + it("should not require popout for Edge on Mac (not Chrome)", async () => { + getDeviceSpy.mockReturnValue(DeviceType.EdgeExtension); + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + // Edge/Opera/Vivaldi on Mac don't match the Chrome-specific Mac check + expect(openPopoutSpy).not.toHaveBeenCalled(); + expect(closePopupSpy).not.toHaveBeenCalled(); + expect(result).toBe(true); + }); + + it("should allow navigation when in popout", async () => { + getDeviceSpy.mockReturnValue(DeviceType.ChromeExtension); + inPopoutSpy.mockReturnValue(true); + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).not.toHaveBeenCalled(); + expect(closePopupSpy).not.toHaveBeenCalled(); + expect(result).toBe(true); + }); + + it("should allow navigation when in sidebar", async () => { + getDeviceSpy.mockReturnValue(DeviceType.ChromeExtension); + inSidebarSpy.mockReturnValue(true); + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).not.toHaveBeenCalled(); + expect(closePopupSpy).not.toHaveBeenCalled(); + expect(result).toBe(true); + }); + }); + + describe("Chromium browsers on Windows", () => { + beforeEach(() => { + getDeviceSpy.mockReturnValue(DeviceType.ChromeExtension); + inPopoutSpy.mockReturnValue(false); + inSidebarSpy.mockReturnValue(false); + Object.defineProperty(window, "navigator", { + value: { + userAgent: "Mozilla/5.0 (Windows NT 10.0; Win64; x64)", + appVersion: "5.0 (Windows NT 10.0; Win64; x64)", + }, + configurable: true, + writable: true, + }); + }); + + it("should allow navigation without popout on Windows", async () => { + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(getDeviceSpy).toHaveBeenCalledWith(window); + expect(openPopoutSpy).not.toHaveBeenCalled(); + expect(closePopupSpy).not.toHaveBeenCalled(); + expect(result).toBe(true); + }); + }); + + describe("file picker routes", () => { + beforeEach(() => { + getDeviceSpy.mockReturnValue(DeviceType.FirefoxExtension); + inPopoutSpy.mockReturnValue(false); + inSidebarSpy.mockReturnValue(false); + }); + + it("should open popout for /import route", async () => { + const importState: RouterStateSnapshot = { + url: "/import", + } as RouterStateSnapshot; + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, importState)); + + expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/import"); + expect(closePopupSpy).toHaveBeenCalledWith(window); + expect(result).toBe(false); + }); + + it("should open popout for /add-send route", async () => { + const addSendState: RouterStateSnapshot = { + url: "/add-send", + } as RouterStateSnapshot; + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, addSendState)); + + expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/add-send"); + expect(closePopupSpy).toHaveBeenCalledWith(window); + expect(result).toBe(false); + }); + + it("should open popout for /edit-send route", async () => { + const editSendState: RouterStateSnapshot = { + url: "/edit-send", + } as RouterStateSnapshot; + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, editSendState)); + + expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/edit-send"); + expect(closePopupSpy).toHaveBeenCalledWith(window); + expect(result).toBe(false); + }); + + it("should open popout for /attachments route", async () => { + const attachmentsState: RouterStateSnapshot = { + url: "/attachments?cipherId=123", + } as RouterStateSnapshot; + + const guard = filePickerPopoutGuard(); + const result = await TestBed.runInInjectionContext(() => guard(mockRoute, attachmentsState)); + + expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/attachments?cipherId=123"); + expect(closePopupSpy).toHaveBeenCalledWith(window); + expect(result).toBe(false); + }); + + it("should preserve query parameters on file picker routes", async () => { + const editSendWithParams: RouterStateSnapshot = { + url: "/edit-send?sendId=123&type=1", + } as RouterStateSnapshot; + + const guard = filePickerPopoutGuard(); + await TestBed.runInInjectionContext(() => guard(mockRoute, editSendWithParams)); + + expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/edit-send?sendId=123&type=1"); + expect(closePopupSpy).toHaveBeenCalledWith(window); + }); + }); + + describe("url handling", () => { + beforeEach(() => { + getDeviceSpy.mockReturnValue(DeviceType.FirefoxExtension); + inPopoutSpy.mockReturnValue(false); + inSidebarSpy.mockReturnValue(false); + }); + + it("should preserve query parameters in the popout url", async () => { + const stateWithQuery: RouterStateSnapshot = { + url: "/import?foo=bar&baz=qux", + } as RouterStateSnapshot; + + const guard = filePickerPopoutGuard(); + await TestBed.runInInjectionContext(() => guard(mockRoute, stateWithQuery)); + + expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/import?foo=bar&baz=qux"); + expect(closePopupSpy).toHaveBeenCalledWith(window); + }); + + it("should handle urls without query parameters", async () => { + const stateWithoutQuery: RouterStateSnapshot = { + url: "/simple-path", + } as RouterStateSnapshot; + + const guard = filePickerPopoutGuard(); + await TestBed.runInInjectionContext(() => guard(mockRoute, stateWithoutQuery)); + + expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/simple-path"); + expect(closePopupSpy).toHaveBeenCalledWith(window); + }); + + it("should not add autoClosePopout parameter to the url", async () => { + const guard = filePickerPopoutGuard(); + await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); + + expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/add-send?type=1"); + expect(openPopoutSpy).not.toHaveBeenCalledWith(expect.stringContaining("autoClosePopout")); + }); + }); +}); diff --git a/apps/browser/src/tools/popup/guards/file-picker-popout.guard.ts b/apps/browser/src/tools/popup/guards/file-picker-popout.guard.ts new file mode 100644 index 00000000000..e321910153c --- /dev/null +++ b/apps/browser/src/tools/popup/guards/file-picker-popout.guard.ts @@ -0,0 +1,78 @@ +import { ActivatedRouteSnapshot, CanActivateFn, RouterStateSnapshot } from "@angular/router"; + +import { BrowserApi } from "@bitwarden/browser/platform/browser/browser-api"; +import BrowserPopupUtils from "@bitwarden/browser/platform/browser/browser-popup-utils"; +import { BrowserPlatformUtilsService } from "@bitwarden/browser/platform/services/platform-utils/browser-platform-utils.service"; +import { DeviceType } from "@bitwarden/common/enums"; + +/** + * Composite guard that handles file picker popout requirements for all browsers. + * Forces a popout window when file pickers could be exposed on browsers that require it. + * + * Browser-specific requirements: + * - Firefox: Requires sidebar OR popout (crashes with file picker in popup: https://bugzilla.mozilla.org/show_bug.cgi?id=1292701) + * - Safari: Requires popout only + * - Chromium on Linux/Mac: Requires sidebar OR popout + * - Chromium on Windows: No special requirement + * + * @returns CanActivateFn that opens popout and blocks navigation when file picker access is needed + */ +export function filePickerPopoutGuard(): CanActivateFn { + return async (_route: ActivatedRouteSnapshot, state: RouterStateSnapshot) => { + // Check if browser is one that needs popout for file pickers + const deviceType = BrowserPlatformUtilsService.getDevice(window); + + // Check current context + const inPopout = BrowserPopupUtils.inPopout(window); + const inSidebar = BrowserPopupUtils.inSidebar(window); + + let needsPopout = false; + + // Firefox: needs sidebar OR popout to avoid crash with file picker + if (deviceType === DeviceType.FirefoxExtension && !inPopout && !inSidebar) { + needsPopout = true; + } + + // Safari: needs popout only (sidebar not available) + if (deviceType === DeviceType.SafariExtension && !inPopout) { + needsPopout = true; + } + + // Chromium on Linux: needs sidebar OR popout for file picker access + // All Chromium-based browsers (Chrome, Edge, Opera, Vivaldi) on Linux + const isChromiumBased = [ + DeviceType.ChromeExtension, + DeviceType.EdgeExtension, + DeviceType.OperaExtension, + DeviceType.VivaldiExtension, + ].includes(deviceType); + + const isLinux = window?.navigator?.userAgent?.indexOf("Linux") !== -1; + + if (isChromiumBased && isLinux && !inPopout && !inSidebar) { + needsPopout = true; + } + + // Chrome (specifically) on Mac: needs sidebar OR popout for file picker access + const isMac = + deviceType === DeviceType.ChromeExtension && + window?.navigator?.appVersion.includes("Mac OS X"); + + if (isMac && !inPopout && !inSidebar) { + needsPopout = true; + } + + // Open popout if needed + if (needsPopout) { + // Don't add autoClosePopout for file picker scenarios - user should manually close + await BrowserPopupUtils.openPopout(`popup/index.html#${state.url}`); + + // Close the original popup window + BrowserApi.closePopup(window); + + return false; // Block navigation - popout will reload + } + + return true; // Allow navigation + }; +} diff --git a/apps/browser/src/tools/popup/guards/firefox-popout.guard.spec.ts b/apps/browser/src/tools/popup/guards/firefox-popout.guard.spec.ts deleted file mode 100644 index fcc7af6441f..00000000000 --- a/apps/browser/src/tools/popup/guards/firefox-popout.guard.spec.ts +++ /dev/null @@ -1,208 +0,0 @@ -import { TestBed } from "@angular/core/testing"; -import { ActivatedRouteSnapshot, RouterStateSnapshot } from "@angular/router"; - -import { DeviceType } from "@bitwarden/common/enums"; - -import { BrowserApi } from "../../../platform/browser/browser-api"; -import BrowserPopupUtils from "../../../platform/browser/browser-popup-utils"; -import { BrowserPlatformUtilsService } from "../../../platform/services/platform-utils/browser-platform-utils.service"; - -import { firefoxPopoutGuard } from "./firefox-popout.guard"; - -describe("firefoxPopoutGuard", () => { - let getDeviceSpy: jest.SpyInstance; - let inPopoutSpy: jest.SpyInstance; - let inSidebarSpy: jest.SpyInstance; - let openPopoutSpy: jest.SpyInstance; - let closePopupSpy: jest.SpyInstance; - - const mockRoute = {} as ActivatedRouteSnapshot; - const mockState: RouterStateSnapshot = { - url: "/import?param=value", - } as RouterStateSnapshot; - - beforeEach(() => { - getDeviceSpy = jest.spyOn(BrowserPlatformUtilsService, "getDevice"); - inPopoutSpy = jest.spyOn(BrowserPopupUtils, "inPopout"); - inSidebarSpy = jest.spyOn(BrowserPopupUtils, "inSidebar"); - openPopoutSpy = jest.spyOn(BrowserPopupUtils, "openPopout").mockImplementation(); - closePopupSpy = jest.spyOn(BrowserApi, "closePopup").mockImplementation(); - - TestBed.configureTestingModule({}); - }); - - afterEach(() => { - jest.clearAllMocks(); - }); - - describe("when browser is Firefox", () => { - beforeEach(() => { - getDeviceSpy.mockReturnValue(DeviceType.FirefoxExtension); - inPopoutSpy.mockReturnValue(false); - inSidebarSpy.mockReturnValue(false); - }); - - it("should open popout and block navigation when not already in popout or sidebar", async () => { - const guard = firefoxPopoutGuard(); - const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); - - expect(getDeviceSpy).toHaveBeenCalledWith(window); - expect(inPopoutSpy).toHaveBeenCalledWith(window); - expect(inSidebarSpy).toHaveBeenCalledWith(window); - expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/import?param=value"); - expect(closePopupSpy).toHaveBeenCalledWith(window); - expect(result).toBe(false); - }); - - it("should not add autoClosePopout parameter to the url", async () => { - const guard = firefoxPopoutGuard(); - await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); - - expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/import?param=value"); - expect(openPopoutSpy).not.toHaveBeenCalledWith(expect.stringContaining("autoClosePopout")); - }); - - it("should allow navigation when already in popout", async () => { - inPopoutSpy.mockReturnValue(true); - - const guard = firefoxPopoutGuard(); - const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); - - expect(openPopoutSpy).not.toHaveBeenCalled(); - expect(closePopupSpy).not.toHaveBeenCalled(); - expect(result).toBe(true); - }); - - it("should allow navigation when already in sidebar", async () => { - inSidebarSpy.mockReturnValue(true); - - const guard = firefoxPopoutGuard(); - const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); - - expect(openPopoutSpy).not.toHaveBeenCalled(); - expect(closePopupSpy).not.toHaveBeenCalled(); - expect(result).toBe(true); - }); - }); - - describe("when browser is not Firefox", () => { - beforeEach(() => { - inPopoutSpy.mockReturnValue(false); - inSidebarSpy.mockReturnValue(false); - }); - - it.each([ - { deviceType: DeviceType.ChromeExtension, name: "ChromeExtension" }, - { deviceType: DeviceType.EdgeExtension, name: "EdgeExtension" }, - { deviceType: DeviceType.OperaExtension, name: "OperaExtension" }, - { deviceType: DeviceType.SafariExtension, name: "SafariExtension" }, - { deviceType: DeviceType.VivaldiExtension, name: "VivaldiExtension" }, - ])( - "should allow navigation without opening popout when device is $name", - async ({ deviceType }) => { - getDeviceSpy.mockReturnValue(deviceType); - - const guard = firefoxPopoutGuard(); - const result = await TestBed.runInInjectionContext(() => guard(mockRoute, mockState)); - - expect(getDeviceSpy).toHaveBeenCalledWith(window); - expect(openPopoutSpy).not.toHaveBeenCalled(); - expect(closePopupSpy).not.toHaveBeenCalled(); - expect(result).toBe(true); - }, - ); - }); - - describe("file picker routes", () => { - beforeEach(() => { - getDeviceSpy.mockReturnValue(DeviceType.FirefoxExtension); - inPopoutSpy.mockReturnValue(false); - inSidebarSpy.mockReturnValue(false); - }); - - it("should open popout for /import route", async () => { - const importState: RouterStateSnapshot = { - url: "/import", - } as RouterStateSnapshot; - - const guard = firefoxPopoutGuard(); - const result = await TestBed.runInInjectionContext(() => guard(mockRoute, importState)); - - expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/import"); - expect(closePopupSpy).toHaveBeenCalledWith(window); - expect(result).toBe(false); - }); - - it("should open popout for /add-send route", async () => { - const addSendState: RouterStateSnapshot = { - url: "/add-send", - } as RouterStateSnapshot; - - const guard = firefoxPopoutGuard(); - const result = await TestBed.runInInjectionContext(() => guard(mockRoute, addSendState)); - - expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/add-send"); - expect(closePopupSpy).toHaveBeenCalledWith(window); - expect(result).toBe(false); - }); - - it("should open popout for /edit-send route", async () => { - const editSendState: RouterStateSnapshot = { - url: "/edit-send", - } as RouterStateSnapshot; - - const guard = firefoxPopoutGuard(); - const result = await TestBed.runInInjectionContext(() => guard(mockRoute, editSendState)); - - expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/edit-send"); - expect(closePopupSpy).toHaveBeenCalledWith(window); - expect(result).toBe(false); - }); - - it("should preserve query parameters on file picker routes", async () => { - const editSendWithParams: RouterStateSnapshot = { - url: "/edit-send?sendId=123&mode=edit", - } as RouterStateSnapshot; - - const guard = firefoxPopoutGuard(); - await TestBed.runInInjectionContext(() => guard(mockRoute, editSendWithParams)); - - expect(openPopoutSpy).toHaveBeenCalledWith( - "popup/index.html#/edit-send?sendId=123&mode=edit", - ); - expect(closePopupSpy).toHaveBeenCalledWith(window); - }); - }); - - describe("url handling", () => { - beforeEach(() => { - getDeviceSpy.mockReturnValue(DeviceType.FirefoxExtension); - inPopoutSpy.mockReturnValue(false); - inSidebarSpy.mockReturnValue(false); - }); - - it("should preserve query parameters in the popout url", async () => { - const stateWithQuery: RouterStateSnapshot = { - url: "/import?foo=bar&baz=qux", - } as RouterStateSnapshot; - - const guard = firefoxPopoutGuard(); - await TestBed.runInInjectionContext(() => guard(mockRoute, stateWithQuery)); - - expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/import?foo=bar&baz=qux"); - expect(closePopupSpy).toHaveBeenCalledWith(window); - }); - - it("should handle urls without query parameters", async () => { - const stateWithoutQuery: RouterStateSnapshot = { - url: "/simple-path", - } as RouterStateSnapshot; - - const guard = firefoxPopoutGuard(); - await TestBed.runInInjectionContext(() => guard(mockRoute, stateWithoutQuery)); - - expect(openPopoutSpy).toHaveBeenCalledWith("popup/index.html#/simple-path"); - expect(closePopupSpy).toHaveBeenCalledWith(window); - }); - }); -}); diff --git a/apps/browser/src/tools/popup/guards/firefox-popout.guard.ts b/apps/browser/src/tools/popup/guards/firefox-popout.guard.ts deleted file mode 100644 index 821f1b7a5bc..00000000000 --- a/apps/browser/src/tools/popup/guards/firefox-popout.guard.ts +++ /dev/null @@ -1,38 +0,0 @@ -import { ActivatedRouteSnapshot, CanActivateFn, RouterStateSnapshot } from "@angular/router"; - -import { BrowserApi } from "@bitwarden/browser/platform/browser/browser-api"; -import BrowserPopupUtils from "@bitwarden/browser/platform/browser/browser-popup-utils"; -import { BrowserPlatformUtilsService } from "@bitwarden/browser/platform/services/platform-utils/browser-platform-utils.service"; -import { DeviceType } from "@bitwarden/common/enums"; - -/** - * Guard that forces a popout window on Firefox browser when a file picker could be exposed. - * Necessary to avoid a crash: https://bugzilla.mozilla.org/show_bug.cgi?id=1292701 - * Also disallows the user from closing a popout and re-opening the view exposing the file picker. - * - * @returns CanActivateFn that opens popout and blocks navigation on Firefox - */ -export function firefoxPopoutGuard(): CanActivateFn { - return async (_route: ActivatedRouteSnapshot, state: RouterStateSnapshot) => { - // Check if browser is Firefox using the platform utils service - const deviceType = BrowserPlatformUtilsService.getDevice(window); - const isFirefox = deviceType === DeviceType.FirefoxExtension; - - // Check if already in popout/sidebar - const inPopout = BrowserPopupUtils.inPopout(window); - const inSidebar = BrowserPopupUtils.inSidebar(window); - - // Open popout if on Firefox and not already in popout/sidebar - if (isFirefox && !inPopout && !inSidebar) { - // Don't add autoClosePopout for file picker scenarios - user should manually close - await BrowserPopupUtils.openPopout(`popup/index.html#${state.url}`); - - // Close the original popup window - BrowserApi.closePopup(window); - - return false; // Block navigation - popout will reload - } - - return true; // Allow navigation - }; -} diff --git a/apps/browser/src/tools/popup/send-v2/add-edit/send-add-edit.component.html b/apps/browser/src/tools/popup/send-v2/add-edit/send-add-edit.component.html index a72847a5bf2..2d588a9ee78 100644 --- a/apps/browser/src/tools/popup/send-v2/add-edit/send-add-edit.component.html +++ b/apps/browser/src/tools/popup/send-v2/add-edit/send-add-edit.component.html @@ -10,8 +10,6 @@ > - - - - - diff --git a/apps/browser/src/tools/popup/send-v2/send-file-popout-dialog/send-file-popout-dialog.component.ts b/apps/browser/src/tools/popup/send-v2/send-file-popout-dialog/send-file-popout-dialog.component.ts deleted file mode 100644 index 23fa744995a..00000000000 --- a/apps/browser/src/tools/popup/send-v2/send-file-popout-dialog/send-file-popout-dialog.component.ts +++ /dev/null @@ -1,26 +0,0 @@ -import { CommonModule } from "@angular/common"; -import { Component } from "@angular/core"; - -import { JslibModule } from "@bitwarden/angular/jslib.module"; -import { ButtonModule, DialogModule, DialogService, TypographyModule } from "@bitwarden/components"; - -import BrowserPopupUtils from "../../../../platform/browser/browser-popup-utils"; - -// FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush -// eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection -@Component({ - selector: "send-file-popout-dialog", - templateUrl: "./send-file-popout-dialog.component.html", - imports: [JslibModule, CommonModule, DialogModule, ButtonModule, TypographyModule], -}) -export class SendFilePopoutDialogComponent { - constructor(private dialogService: DialogService) {} - - async popOutWindow() { - await BrowserPopupUtils.openCurrentPagePopout(window); - } - - close() { - this.dialogService.closeAll(); - } -} diff --git a/apps/browser/src/tools/popup/services/file-popout-utils.service.ts b/apps/browser/src/tools/popup/services/file-popout-utils.service.ts deleted file mode 100644 index 9a04d4b8f23..00000000000 --- a/apps/browser/src/tools/popup/services/file-popout-utils.service.ts +++ /dev/null @@ -1,71 +0,0 @@ -import { Injectable } from "@angular/core"; - -import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service"; - -import BrowserPopupUtils from "../../../platform/browser/browser-popup-utils"; - -/** - * Service for determining whether to display file popout callout messages. - */ -@Injectable() -export class FilePopoutUtilsService { - /** - * Creates an instance of FilePopoutUtilsService. - */ - constructor(private platformUtilsService: PlatformUtilsService) {} - - /** - * Determines whether to show any file popout callout message in the current browser. - * @param win - The window context in which the check should be performed. - * @returns True if a file popout callout message should be displayed; otherwise, false. - */ - showFilePopoutMessage(win: Window): boolean { - return ( - this.showFirefoxFileWarning(win) || - this.showSafariFileWarning(win) || - this.showChromiumFileWarning(win) - ); - } - - /** - * Determines whether to show a file popout callout message for the Firefox browser - * @param win - The window context in which the check should be performed. - * @returns True if the extension is not in a sidebar or popout; otherwise, false. - */ - showFirefoxFileWarning(win: Window): boolean { - return ( - this.platformUtilsService.isFirefox() && - !(BrowserPopupUtils.inSidebar(win) || BrowserPopupUtils.inPopout(win)) - ); - } - - /** - * Determines whether to show a file popout message for the Safari browser - * @param win - The window context in which the check should be performed. - * @returns True if the extension is not in a popout; otherwise, false. - */ - showSafariFileWarning(win: Window): boolean { - return this.platformUtilsService.isSafari() && !BrowserPopupUtils.inPopout(win); - } - - /** - * Determines whether to show a file popout callout message for Chromium-based browsers in Linux and Mac OS X - * @param win - The window context in which the check should be performed. - * @returns True if the extension is not in a sidebar or popout; otherwise, false. - */ - showChromiumFileWarning(win: Window): boolean { - return ( - (this.isLinux(win) || this.isUnsupportedMac(win)) && - !this.platformUtilsService.isFirefox() && - !(BrowserPopupUtils.inSidebar(win) || BrowserPopupUtils.inPopout(win)) - ); - } - - private isLinux(win: Window): boolean { - return win?.navigator?.userAgent.indexOf("Linux") !== -1; - } - - private isUnsupportedMac(win: Window): boolean { - return this.platformUtilsService.isChrome() && win?.navigator?.appVersion.includes("Mac OS X"); - } -} diff --git a/apps/browser/src/vault/popup/components/vault-v2/attachments/open-attachments/open-attachments.component.html b/apps/browser/src/vault/popup/components/vault-v2/attachments/open-attachments/open-attachments.component.html index 0fbe1c55b0a..67f9f75e24c 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/attachments/open-attachments/open-attachments.component.html +++ b/apps/browser/src/vault/popup/components/vault-v2/attachments/open-attachments/open-attachments.component.html @@ -10,8 +10,7 @@ - - + diff --git a/apps/browser/src/vault/popup/components/vault-v2/attachments/open-attachments/open-attachments.component.spec.ts b/apps/browser/src/vault/popup/components/vault-v2/attachments/open-attachments/open-attachments.component.spec.ts index 459b328c44e..d99e99ed1af 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/attachments/open-attachments/open-attachments.component.spec.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/attachments/open-attachments/open-attachments.component.spec.ts @@ -19,9 +19,6 @@ import { CipherView } from "@bitwarden/common/vault/models/view/cipher.view"; import { ToastService } from "@bitwarden/components"; import { CipherFormContainer } from "@bitwarden/vault"; -import BrowserPopupUtils from "../../../../../../platform/browser/browser-popup-utils"; -import { FilePopoutUtilsService } from "../../../../../../tools/popup/services/file-popout-utils.service"; - import { OpenAttachmentsComponent } from "./open-attachments.component"; describe("OpenAttachmentsComponent", () => { @@ -30,9 +27,6 @@ describe("OpenAttachmentsComponent", () => { let router: Router; const showToast = jest.fn(); const hasPremiumFromAnySource$ = new BehaviorSubject(true); - const openCurrentPagePopout = jest - .spyOn(BrowserPopupUtils, "openCurrentPagePopout") - .mockResolvedValue(null); const cipherView = { id: "5555-444-3333", type: CipherType.Login, @@ -54,7 +48,6 @@ describe("OpenAttachmentsComponent", () => { const getCipher = jest.fn().mockResolvedValue(cipherDomain); const organizations$ = jest.fn().mockReturnValue(of([org])); - const showFilePopoutMessage = jest.fn().mockReturnValue(false); const mockUserId = Utils.newGuid() as UserId; const accountService = { @@ -68,11 +61,9 @@ describe("OpenAttachmentsComponent", () => { const formStatusChange$ = new BehaviorSubject<"enabled" | "disabled">("enabled"); beforeEach(async () => { - openCurrentPagePopout.mockClear(); getCipher.mockClear(); showToast.mockClear(); organizations$.mockClear(); - showFilePopoutMessage.mockClear(); hasPremiumFromAnySource$.next(true); formStatusChange$.next("enabled"); @@ -101,10 +92,6 @@ describe("OpenAttachmentsComponent", () => { provide: OrganizationService, useValue: { organizations$ }, }, - { - provide: FilePopoutUtilsService, - useValue: { showFilePopoutMessage }, - }, { provide: AccountService, useValue: accountService, @@ -128,8 +115,7 @@ describe("OpenAttachmentsComponent", () => { fixture.detectChanges(); }); - it("opens attachments in new popout", async () => { - showFilePopoutMessage.mockReturnValue(true); + it("navigates to attachments route", async () => { component.canAccessAttachments = true; await component.ngOnInit(); @@ -138,20 +124,6 @@ describe("OpenAttachmentsComponent", () => { expect(router.navigate).toHaveBeenCalledWith(["/attachments"], { queryParams: { cipherId: "5555-444-3333" }, }); - expect(openCurrentPagePopout).toHaveBeenCalledWith(window); - }); - - it("opens attachments in same window", async () => { - showFilePopoutMessage.mockReturnValue(false); - component.canAccessAttachments = true; - await component.ngOnInit(); - - await component.openAttachments(); - - expect(openCurrentPagePopout).not.toHaveBeenCalled(); - expect(router.navigate).toHaveBeenCalledWith(["/attachments"], { - queryParams: { cipherId: "5555-444-3333" }, - }); }); it("routes the user to the premium page when they cannot access premium features", async () => { diff --git a/apps/browser/src/vault/popup/components/vault-v2/attachments/open-attachments/open-attachments.component.ts b/apps/browser/src/vault/popup/components/vault-v2/attachments/open-attachments/open-attachments.component.ts index a267e7999ab..1a1f767ca8c 100644 --- a/apps/browser/src/vault/popup/components/vault-v2/attachments/open-attachments/open-attachments.component.ts +++ b/apps/browser/src/vault/popup/components/vault-v2/attachments/open-attachments/open-attachments.component.ts @@ -23,9 +23,6 @@ import { PremiumUpgradePromptService } from "@bitwarden/common/vault/abstraction import { BadgeModule, ItemModule, ToastService, TypographyModule } from "@bitwarden/components"; import { CipherFormContainer } from "@bitwarden/vault"; -import BrowserPopupUtils from "../../../../../../platform/browser/browser-popup-utils"; -import { FilePopoutUtilsService } from "../../../../../../tools/popup/services/file-popout-utils.service"; - // FIXME(https://bitwarden.atlassian.net/browse/CL-764): Migrate to OnPush // eslint-disable-next-line @angular-eslint/prefer-on-push-component-change-detection @Component({ @@ -46,9 +43,6 @@ export class OpenAttachmentsComponent implements OnInit { // eslint-disable-next-line @angular-eslint/prefer-signals @Input({ required: true }) cipherId: CipherId; - /** True when the attachments window should be opened in a popout */ - openAttachmentsInPopout: boolean; - /** True when the user has access to premium or h */ canAccessAttachments: boolean; @@ -65,7 +59,6 @@ export class OpenAttachmentsComponent implements OnInit { private organizationService: OrganizationService, private toastService: ToastService, private i18nService: I18nService, - private filePopoutUtilsService: FilePopoutUtilsService, private accountService: AccountService, private cipherFormContainer: CipherFormContainer, private premiumUpgradeService: PremiumUpgradePromptService, @@ -87,8 +80,6 @@ export class OpenAttachmentsComponent implements OnInit { } async ngOnInit(): Promise { - this.openAttachmentsInPopout = this.filePopoutUtilsService.showFilePopoutMessage(window); - if (!this.cipherId) { return; } @@ -131,12 +122,5 @@ export class OpenAttachmentsComponent implements OnInit { } await this.router.navigate(["/attachments"], { queryParams: { cipherId: this.cipherId } }); - - // Open the attachments page in a popout - // This is done after the router navigation to ensure that the navigation - // is included in the `PopupRouterCacheService` history - if (this.openAttachmentsInPopout) { - await BrowserPopupUtils.openCurrentPagePopout(window); - } } }