From 12ec2748b4297fd542dfeb773351b6c23679e007 Mon Sep 17 00:00:00 2001 From: Devin Binnie <52460000+devinbinnie@users.noreply.github.com> Date: Tue, 7 Mar 2023 08:50:39 -0500 Subject: [PATCH] Refactor webContentsEvents and popup listeners (#2587) --- src/main/views/webContentEvents.test.js | 49 +++++++---- src/main/views/webContentEvents.ts | 111 ++++++++++++++---------- 2 files changed, 96 insertions(+), 64 deletions(-) diff --git a/src/main/views/webContentEvents.test.js b/src/main/views/webContentEvents.test.js index 37c315c7..6b7461c6 100644 --- a/src/main/views/webContentEvents.test.js +++ b/src/main/views/webContentEvents.test.js @@ -3,10 +3,12 @@ 'use strict'; -import {shell} from 'electron'; +import {shell, BrowserWindow} from 'electron'; import urlUtils from 'common/utils/url'; +import ContextMenu from 'main/contextMenu'; + import * as WindowManager from '../windows/windowManager'; import allowProtocolDialog from '../allowProtocolDialog'; @@ -17,22 +19,19 @@ jest.mock('electron', () => ({ shell: { openExternal: jest.fn(), }, - BrowserWindow: jest.fn().mockImplementation(() => ({ - once: jest.fn(), - show: jest.fn(), - loadURL: jest.fn(), - webContents: { - setWindowOpenHandler: jest.fn(), - }, - })), + BrowserWindow: jest.fn(), session: {}, })); +jest.mock('main/contextMenu', () => jest.fn()); jest.mock('../allowProtocolDialog', () => ({})); jest.mock('../windows/windowManager', () => ({ getServerURLFromWebContentsId: jest.fn(), showMainWindow: jest.fn(), })); +jest.mock('../utils', () => ({ + composeUserAgent: jest.fn(), +})); jest.mock('common/config', () => ({ spellcheck: true, @@ -77,14 +76,14 @@ jest.mock('../allowProtocolDialog', () => ({ })); describe('main/views/webContentsEvents', () => { - const event = {preventDefault: jest.fn(), sender: {id: 1}}; + const event = {preventDefault: jest.fn()}; describe('willNavigate', () => { const webContentsEventManager = new WebContentsEventManager(); - const willNavigate = webContentsEventManager.generateWillNavigate(jest.fn()); + const willNavigate = webContentsEventManager.generateWillNavigate(1); beforeEach(() => { - WindowManager.getServerURLFromWebContentsId.mockImplementation(() => new URL('http://server-1.com')); + webContentsEventManager.getServerURLFromWebContentsId = jest.fn().mockImplementation(() => new URL('http://server-1.com')); }); afterEach(() => { @@ -144,10 +143,10 @@ describe('main/views/webContentsEvents', () => { describe('didStartNavigation', () => { const webContentsEventManager = new WebContentsEventManager(); - const didStartNavigation = webContentsEventManager.generateDidStartNavigation(jest.fn()); + const didStartNavigation = webContentsEventManager.generateDidStartNavigation(1); beforeEach(() => { - WindowManager.getServerURLFromWebContentsId.mockImplementation(() => new URL('http://server-1.com')); + webContentsEventManager.getServerURLFromWebContentsId = jest.fn().mockImplementation(() => new URL('http://server-1.com')); urlUtils.isTrustedURL.mockReturnValue(true); urlUtils.isInternalURL.mockImplementation((serverURL, parsedURL) => parsedURL.toString().startsWith(serverURL)); urlUtils.isCustomLoginURL.mockImplementation((parsedURL) => parsedURL.toString().startsWith('http://loginurl.com/login')); @@ -173,18 +172,32 @@ describe('main/views/webContentsEvents', () => { describe('newWindow', () => { const webContentsEventManager = new WebContentsEventManager(); - const newWindow = webContentsEventManager.generateNewWindowListener(jest.fn()); + const newWindow = webContentsEventManager.generateNewWindowListener(1, true); beforeEach(() => { urlUtils.isValidURI.mockReturnValue(true); - WindowManager.getServerURLFromWebContentsId.mockImplementation(() => new URL('http://server-1.com')); + webContentsEventManager.getServerURLFromWebContentsId = jest.fn().mockImplementation(() => new URL('http://server-1.com')); urlUtils.isTeamUrl.mockImplementation((serverURL, parsedURL) => parsedURL.toString().startsWith(`${serverURL}myteam`)); urlUtils.isAdminUrl.mockImplementation((serverURL, parsedURL) => parsedURL.toString().startsWith(`${serverURL}admin_console`)); urlUtils.isPluginUrl.mockImplementation((serverURL, parsedURL) => parsedURL.toString().startsWith(`${serverURL}myplugin`)); - urlUtils.isManagedResource.mockImplementation((serverURL, parsedURL) => parsedURL.toString().startsWith(`${serverURL}myplugin`)); + urlUtils.isManagedResource.mockImplementation((serverURL, parsedURL) => parsedURL.toString().startsWith(`${serverURL}trusted`)); + + BrowserWindow.mockImplementation(() => ({ + once: jest.fn(), + show: jest.fn(), + loadURL: jest.fn(), + webContents: { + on: jest.fn(), + setWindowOpenHandler: jest.fn(), + }, + })); + ContextMenu.mockImplementation(() => ({ + reload: jest.fn(), + })); }); afterEach(() => { + webContentsEventManager.popupWindow = undefined; jest.resetAllMocks(); }); it('should deny on bad URL', () => { @@ -237,7 +250,7 @@ describe('main/views/webContentsEvents', () => { }); it('should prevent from opening a new window if popup already exists', () => { - webContentsEventManager.popupWindow = {webContents: {getURL: () => 'http://server-1.com/myplugin/login'}}; + webContentsEventManager.popupWindow = {win: {webContents: {getURL: () => 'http://server-1.com/myplugin/login'}}}; expect(newWindow({url: 'http://server-1.com/myplugin/login'})).toStrictEqual({action: 'deny'}); }); diff --git a/src/main/views/webContentEvents.ts b/src/main/views/webContentEvents.ts index 61d0473b..99f86a3a 100644 --- a/src/main/views/webContentEvents.ts +++ b/src/main/views/webContentEvents.ts @@ -28,32 +28,36 @@ const scheme = protocols && protocols[0] && protocols[0].schemes && protocols[0] export class WebContentsEventManager { customLogins: Record; listeners: Record void>; - popupWindow?: BrowserWindow; + popupWindow?: {win: BrowserWindow; serverURL?: URL}; constructor() { this.customLogins = {}; this.listeners = {}; } - private isTrustedPopupWindow = (webContents: WebContents) => { - if (!webContents) { - return false; - } + private isTrustedPopupWindow = (webContentsId: number) => { if (!this.popupWindow) { return false; } - return BrowserWindow.fromWebContents(webContents) === this.popupWindow; + return webContentsId === this.popupWindow.win.webContents.id; } - generateWillNavigate = () => { - return (event: Event & {sender: WebContents}, url: string) => { - log.debug('webContentEvents.will-navigate', {webContentsId: event.sender.id, url}); + private getServerURLFromWebContentsId = (webContentsId: number) => { + if (this.popupWindow && webContentsId === this.popupWindow.win.webContents.id) { + return this.popupWindow.serverURL; + } + + return WindowManager.getServerURLFromWebContentsId(webContentsId); + } + + generateWillNavigate = (webContentsId: number) => { + return (event: Event, url: string) => { + log.debug('webContentEvents.will-navigate', {webContentsId, url}); - const contentID = event.sender.id; const parsedURL = urlUtils.parseURL(url)!; - const serverURL = WindowManager.getServerURLFromWebContentsId(event.sender.id); + const serverURL = this.getServerURLFromWebContentsId(webContentsId); - if (serverURL && (urlUtils.isTeamUrl(serverURL, parsedURL) || urlUtils.isAdminUrl(serverURL, parsedURL) || this.isTrustedPopupWindow(event.sender))) { + if (serverURL && (urlUtils.isTeamUrl(serverURL, parsedURL) || urlUtils.isAdminUrl(serverURL, parsedURL) || this.isTrustedPopupWindow(webContentsId))) { return; } @@ -67,7 +71,7 @@ export class WebContentsEventManager { if (parsedURL.protocol === 'mailto:') { return; } - if (this.customLogins[contentID]?.inProgress) { + if (this.customLogins[webContentsId]?.inProgress) { flushCookiesStore(session.defaultSession); return; } @@ -82,22 +86,21 @@ export class WebContentsEventManager { }; }; - generateDidStartNavigation = () => { - return (event: Event & {sender: WebContents}, url: string) => { - log.debug('webContentEvents.did-start-navigation', {webContentsId: event.sender.id, url}); + generateDidStartNavigation = (webContentsId: number) => { + return (event: Event, url: string) => { + log.debug('webContentEvents.did-start-navigation', {webContentsId, url}); - const contentID = event.sender.id; const parsedURL = urlUtils.parseURL(url)!; - const serverURL = WindowManager.getServerURLFromWebContentsId(event.sender.id); + const serverURL = this.getServerURLFromWebContentsId(webContentsId); if (!serverURL || !urlUtils.isTrustedURL(parsedURL, serverURL)) { return; } if (serverURL && urlUtils.isCustomLoginURL(parsedURL, serverURL)) { - this.customLogins[contentID].inProgress = true; - } else if (serverURL && this.customLogins[contentID].inProgress && urlUtils.isInternalURL(serverURL || new URL(''), parsedURL)) { - this.customLogins[contentID].inProgress = false; + this.customLogins[webContentsId].inProgress = true; + } else if (serverURL && this.customLogins[webContentsId].inProgress && urlUtils.isInternalURL(serverURL || new URL(''), parsedURL)) { + this.customLogins[webContentsId].inProgress = false; } }; }; @@ -135,7 +138,7 @@ export class WebContentsEventManager { return {action: 'deny'}; } - const serverURL = WindowManager.getServerURLFromWebContentsId(webContentsId); + const serverURL = this.getServerURLFromWebContentsId(webContentsId); if (!serverURL) { shell.openExternal(details.url); return {action: 'deny'}; @@ -170,24 +173,38 @@ export class WebContentsEventManager { log.info(`${details.url} is an admin console page, preventing to open a new window`); return {action: 'deny'}; } - if (this.popupWindow && this.popupWindow.webContents.getURL() === details.url) { + if (this.popupWindow && this.popupWindow.win.webContents.getURL() === details.url) { log.info(`Popup window already open at provided url: ${details.url}`); return {action: 'deny'}; } // TODO: move popups to its own and have more than one. if (urlUtils.isPluginUrl(serverURL, parsedURL) || urlUtils.isManagedResource(serverURL, parsedURL)) { - if (!this.popupWindow) { - this.popupWindow = new BrowserWindow({ - backgroundColor: '#fff', // prevents blurry text: https://electronjs.org/docs/faq#the-font-looks-blurry-what-is-this-and-what-can-i-do - //parent: WindowManager.getMainWindow(), - show: false, - center: true, - webPreferences: { - spellcheck: (typeof spellcheck === 'undefined' ? true : spellcheck), - }, + let popup: BrowserWindow; + if (this.popupWindow) { + this.popupWindow.win.once('ready-to-show', () => { + this.popupWindow?.win.show(); }); - this.popupWindow.webContents.on('will-redirect', (event, url) => { + popup = this.popupWindow.win; + } else { + this.popupWindow = { + win: new BrowserWindow({ + backgroundColor: '#fff', // prevents blurry text: https://electronjs.org/docs/faq#the-font-looks-blurry-what-is-this-and-what-can-i-do + //parent: WindowManager.getMainWindow(), + show: false, + center: true, + webPreferences: { + spellcheck: (typeof spellcheck === 'undefined' ? true : spellcheck), + }, + }), + serverURL, + }; + this.customLogins[this.popupWindow.win.webContents.id] = { + inProgress: false, + }; + + popup = this.popupWindow.win; + popup.webContents.on('will-redirect', (event, url) => { const parsedURL = urlUtils.parseURL(url); if (!parsedURL) { event.preventDefault(); @@ -198,23 +215,25 @@ export class WebContentsEventManager { event.preventDefault(); } }); - this.popupWindow.webContents.setWindowOpenHandler(this.denyNewWindow); - this.popupWindow.once('closed', () => { + popup.webContents.on('will-navigate', this.generateWillNavigate(popup.webContents.id)); + popup.webContents.on('did-start-navigation', this.generateDidStartNavigation(popup.webContents.id)); + popup.webContents.setWindowOpenHandler(this.denyNewWindow); + popup.once('closed', () => { this.popupWindow = undefined; }); - const contextMenu = new ContextMenu({}, this.popupWindow); + + const contextMenu = new ContextMenu({}, popup); contextMenu.reload(); } - const popupWindow = this.popupWindow; - popupWindow.once('ready-to-show', () => popupWindow.show()); + popup.once('ready-to-show', () => popup.show()); if (urlUtils.isManagedResource(serverURL, parsedURL)) { - popupWindow.loadURL(details.url); + popup.loadURL(details.url); } else { // currently changing the userAgent for popup windows to allow plugins to go through google's oAuth // should be removed once a proper oAuth2 implementation is setup. - popupWindow.loadURL(details.url, { + popup.loadURL(details.url, { userAgent: composeUserAgent(), }); } @@ -266,16 +285,16 @@ export class WebContentsEventManager { this.removeWebContentsListeners(contents.id); } - const willNavigate = this.generateWillNavigate(); - contents.on('will-navigate', willNavigate as (e: Event, u: string) => void); // TODO: Electron types don't include sender for some reason + const willNavigate = this.generateWillNavigate(contents.id); + contents.on('will-navigate', willNavigate); // handle custom login requests (oath, saml): // 1. are we navigating to a supported local custom login path from the `/login` page? // - indicate custom login is in progress // 2. are we finished with the custom login process? // - indicate custom login is NOT in progress - const didStartNavigation = this.generateDidStartNavigation(); - contents.on('did-start-navigation', didStartNavigation as (e: Event, u: string) => void); + const didStartNavigation = this.generateDidStartNavigation(contents.id); + contents.on('did-start-navigation', didStartNavigation); const spellcheck = Config.useSpellChecker; const newWindow = this.generateNewWindowListener(contents.id, spellcheck); @@ -285,8 +304,8 @@ export class WebContentsEventManager { const removeWebContentsListeners = () => { try { - contents.removeListener('will-navigate', willNavigate as (e: Event, u: string) => void); - contents.removeListener('did-start-navigation', didStartNavigation as (e: Event, u: string) => void); + contents.removeListener('will-navigate', willNavigate); + contents.removeListener('did-start-navigation', didStartNavigation); removeListeners?.(contents); } catch (e) { log.error(`Error while trying to detach listeners, this might be ok if the process crashed: ${e}`);