From aaab2af195a70b993cbf8263a74b7ff1b8d1fe81 Mon Sep 17 00:00:00 2001 From: Timothy Sullivan Date: Fri, 14 May 2021 16:21:31 -0700 Subject: [PATCH 1/2] move require() to screenshotMode plugin --- .../common/get_set_browser_screenshot_mode.ts | 4 ++-- src/plugins/screenshot_mode/server/index.ts | 2 ++ src/plugins/screenshot_mode/server/plugin.ts | 15 ++++++++++++- .../chromium/driver/chromium_driver.ts | 21 +++++++++++-------- .../browsers/chromium/driver_factory/index.ts | 12 +++++++---- .../server/browsers/chromium/index.ts | 6 +++--- .../reporting/server/browsers/index.ts | 13 ++++-------- x-pack/plugins/reporting/server/core.ts | 7 +++++++ .../server/lib/screenshots/observable.test.ts | 20 +++++++++++------- x-pack/plugins/reporting/server/plugin.ts | 6 +++--- .../create_mock_browserdriverfactory.ts | 6 ++++-- x-pack/plugins/reporting/server/types.ts | 2 ++ 12 files changed, 73 insertions(+), 41 deletions(-) diff --git a/src/plugins/screenshot_mode/common/get_set_browser_screenshot_mode.ts b/src/plugins/screenshot_mode/common/get_set_browser_screenshot_mode.ts index acab69caf12274..3a0e0a425c7c6a 100644 --- a/src/plugins/screenshot_mode/common/get_set_browser_screenshot_mode.ts +++ b/src/plugins/screenshot_mode/common/get_set_browser_screenshot_mode.ts @@ -15,9 +15,9 @@ export const KBN_SCREENSHOT_MODE_ENABLED_KEY = '__KBN_SCREENSHOT_MODE_ENABLED_KE * localStorage. The ability to set a value in localStorage enables more convenient development and testing * in functionality that needs to detect screenshot mode. */ -export const getScreenshotMode = (): unknown => { +export const getScreenshotMode = (): boolean => { return ( - ((window as unknown) as Record)[KBN_SCREENSHOT_MODE_ENABLED_KEY] || + Boolean(((window as unknown) as Record)[KBN_SCREENSHOT_MODE_ENABLED_KEY]) || window.localStorage.getItem(KBN_SCREENSHOT_MODE_ENABLED_KEY) === 'true' ); }; diff --git a/src/plugins/screenshot_mode/server/index.ts b/src/plugins/screenshot_mode/server/index.ts index 22363af34a8843..76fe97b95f63c2 100644 --- a/src/plugins/screenshot_mode/server/index.ts +++ b/src/plugins/screenshot_mode/server/index.ts @@ -12,6 +12,8 @@ export { setScreenshotModeEnabled, KBN_SCREENSHOT_MODE_HEADER } from '../common' export { ScreenshotModeRequestHandlerContext } from './types'; +export { ScreenshotModePluginSetup } from './plugin'; + export function plugin() { return new ScreenshotModePlugin(); } diff --git a/src/plugins/screenshot_mode/server/plugin.ts b/src/plugins/screenshot_mode/server/plugin.ts index f526af8426918d..c93be5c9bfb4a7 100644 --- a/src/plugins/screenshot_mode/server/plugin.ts +++ b/src/plugins/screenshot_mode/server/plugin.ts @@ -10,7 +10,11 @@ import { Plugin, CoreSetup } from '../../../core/server'; import { KBN_SCREENSHOT_MODE_HEADER } from '../common'; import { ScreenshotModeRequestHandlerContext } from './types'; -export class ScreenshotModePlugin implements Plugin { +export interface ScreenshotModePluginSetup { + setScreenshotModeEnabled: () => void; +} + +export class ScreenshotModePlugin implements Plugin { public setup(core: CoreSetup) { core.http.registerRouteHandlerContext( 'screenshotMode', @@ -22,6 +26,15 @@ export class ScreenshotModePlugin implements Plugin { }; } ); + + // We use "require" here to ensure the import does not have external references due to code bundling that + // commonly happens during transpiling which would be missing in the environment puppeteer creates. + // eslint-disable-next-line @typescript-eslint/no-var-requires + const { setScreenshotModeEnabled } = require('../'); + + return { + setScreenshotModeEnabled, + }; } public start() {} diff --git a/x-pack/plugins/reporting/server/browsers/chromium/driver/chromium_driver.ts b/x-pack/plugins/reporting/server/browsers/chromium/driver/chromium_driver.ts index be8ee6382f20a4..0ef995f72a5ef9 100644 --- a/x-pack/plugins/reporting/server/browsers/chromium/driver/chromium_driver.ts +++ b/x-pack/plugins/reporting/server/browsers/chromium/driver/chromium_driver.ts @@ -10,20 +10,15 @@ import { map, truncate } from 'lodash'; import open from 'opn'; import puppeteer, { ElementHandle, EvaluateFn, SerializableOrJSHandle } from 'puppeteer'; import { parse as parseUrl } from 'url'; -import { KBN_SCREENSHOT_MODE_HEADER } from '../../../../../../../src/plugins/screenshot_mode/server'; import { getDisallowedOutgoingUrlError } from '../'; +import { ReportingCore } from '../../..'; +import { KBN_SCREENSHOT_MODE_HEADER } from '../../../../../../../src/plugins/screenshot_mode/server'; import { ConditionalHeaders, ConditionalHeadersConditions } from '../../../export_types/common'; import { LevelLogger } from '../../../lib'; import { ViewZoomWidthHeight } from '../../../lib/layouts/layout'; import { ElementPosition } from '../../../lib/screenshots'; import { allowRequest, NetworkPolicy } from '../../network_policy'; -// We use "require" here to ensure the import does not have external references due to code bundling that -// commonly happens during transpiling which would be missing in the environment puppeteer creates. -// eslint-disable-next-line @typescript-eslint/no-var-requires -const setScreenshotModeEnabled = require('../../../../../../../src/plugins/screenshot_mode/server') - .setScreenshotModeEnabled; - export interface ChromiumDriverOptions { inspect: boolean; networkPolicy: NetworkPolicy; @@ -66,8 +61,14 @@ export class HeadlessChromiumDriver { private listenersAttached = false; private interceptedCount = 0; + private core: ReportingCore; - constructor(page: puppeteer.Page, { inspect, networkPolicy }: ChromiumDriverOptions) { + constructor( + core: ReportingCore, + page: puppeteer.Page, + { inspect, networkPolicy }: ChromiumDriverOptions + ) { + this.core = core; this.page = page; this.inspect = inspect; this.networkPolicy = networkPolicy; @@ -105,7 +106,9 @@ export class HeadlessChromiumDriver { // Reset intercepted request count this.interceptedCount = 0; - await this.page.evaluateOnNewDocument(setScreenshotModeEnabled); + const enableScreenshotMode = this.core.enableScreenshotMode(); + + await this.page.evaluateOnNewDocument(enableScreenshotMode); await this.page.setRequestInterception(true); this.registerListeners(conditionalHeaders, logger); diff --git a/x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts b/x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts index 5fe2050ddb6f13..2005541b81ead5 100644 --- a/x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts +++ b/x-pack/plugins/reporting/server/browsers/chromium/driver_factory/index.ts @@ -15,6 +15,7 @@ import * as Rx from 'rxjs'; import { InnerSubscriber } from 'rxjs/internal/InnerSubscriber'; import { ignoreElements, map, mergeMap, tap } from 'rxjs/operators'; import { getChromiumDisconnectedError } from '../'; +import { ReportingCore } from '../../..'; import { BROWSER_TYPE } from '../../../../common/constants'; import { durationToNumber } from '../../../../common/schema_utils'; import { CaptureConfig } from '../../../../server/types'; @@ -32,11 +33,14 @@ export class HeadlessChromiumDriverFactory { private browserConfig: BrowserConfig; private userDataDir: string; private getChromiumArgs: (viewport: ViewportConfig) => string[]; + private core: ReportingCore; - constructor(binaryPath: string, captureConfig: CaptureConfig, logger: LevelLogger) { + constructor(core: ReportingCore, binaryPath: string, logger: LevelLogger) { + this.core = core; this.binaryPath = binaryPath; - this.captureConfig = captureConfig; - this.browserConfig = captureConfig.browser.chromium; + const config = core.getConfig(); + this.captureConfig = config.get('capture'); + this.browserConfig = this.captureConfig.browser.chromium; if (this.browserConfig.disableSandbox) { logger.warning(`Enabling the Chromium sandbox provides an additional layer of protection.`); @@ -138,7 +142,7 @@ export class HeadlessChromiumDriverFactory { this.getProcessLogger(browser, logger).subscribe(); // HeadlessChromiumDriver: object to "drive" a browser page - const driver = new HeadlessChromiumDriver(page, { + const driver = new HeadlessChromiumDriver(this.core, page, { inspect: !!this.browserConfig.inspect, networkPolicy: this.captureConfig.networkPolicy, }); diff --git a/x-pack/plugins/reporting/server/browsers/chromium/index.ts b/x-pack/plugins/reporting/server/browsers/chromium/index.ts index 0d5639254b8168..e0d043f821ab48 100644 --- a/x-pack/plugins/reporting/server/browsers/chromium/index.ts +++ b/x-pack/plugins/reporting/server/browsers/chromium/index.ts @@ -7,15 +7,15 @@ import { i18n } from '@kbn/i18n'; import { BrowserDownload } from '../'; -import { CaptureConfig } from '../../../server/types'; +import { ReportingCore } from '../../../server'; import { LevelLogger } from '../../lib'; import { HeadlessChromiumDriverFactory } from './driver_factory'; import { ChromiumArchivePaths } from './paths'; export const chromium: BrowserDownload = { paths: new ChromiumArchivePaths(), - createDriverFactory: (binaryPath: string, captureConfig: CaptureConfig, logger: LevelLogger) => - new HeadlessChromiumDriverFactory(binaryPath, captureConfig, logger), + createDriverFactory: (core: ReportingCore, binaryPath: string, logger: LevelLogger) => + new HeadlessChromiumDriverFactory(core, binaryPath, logger), }; export const getChromiumDisconnectedError = () => diff --git a/x-pack/plugins/reporting/server/browsers/index.ts b/x-pack/plugins/reporting/server/browsers/index.ts index df95b69d9d2544..c47514960bb09f 100644 --- a/x-pack/plugins/reporting/server/browsers/index.ts +++ b/x-pack/plugins/reporting/server/browsers/index.ts @@ -6,9 +6,8 @@ */ import { first } from 'rxjs/operators'; -import { ReportingConfig } from '../'; +import { ReportingCore } from '../'; import { LevelLogger } from '../lib'; -import { CaptureConfig } from '../types'; import { chromium, ChromiumArchivePaths } from './chromium'; import { HeadlessChromiumDriverFactory } from './chromium/driver_factory'; import { installBrowser } from './install'; @@ -18,8 +17,8 @@ export { HeadlessChromiumDriver } from './chromium/driver'; export { HeadlessChromiumDriverFactory } from './chromium/driver_factory'; type CreateDriverFactory = ( + core: ReportingCore, binaryPath: string, - captureConfig: CaptureConfig, logger: LevelLogger ) => HeadlessChromiumDriverFactory; @@ -28,12 +27,8 @@ export interface BrowserDownload { paths: ChromiumArchivePaths; } -export const initializeBrowserDriverFactory = async ( - config: ReportingConfig, - logger: LevelLogger -) => { +export const initializeBrowserDriverFactory = async (core: ReportingCore, logger: LevelLogger) => { const { binaryPath$ } = installBrowser(logger); const binaryPath = await binaryPath$.pipe(first()).toPromise(); - const captureConfig = config.get('capture'); - return chromium.createDriverFactory(binaryPath, captureConfig, logger); + return chromium.createDriverFactory(core, binaryPath, logger); }; diff --git a/x-pack/plugins/reporting/server/core.ts b/x-pack/plugins/reporting/server/core.ts index 62cab5a8fef199..e44e20fcef5524 100644 --- a/x-pack/plugins/reporting/server/core.ts +++ b/x-pack/plugins/reporting/server/core.ts @@ -8,6 +8,7 @@ import Hapi from '@hapi/hapi'; import * as Rx from 'rxjs'; import { first, map, take } from 'rxjs/operators'; +import { ScreenshotModePluginSetup } from 'src/plugins/screenshot_mode/server'; import { BasePath, IClusterClient, @@ -41,6 +42,7 @@ export interface ReportingInternalSetup { security?: SecurityPluginSetup; spaces?: SpacesPluginSetup; taskManager: TaskManagerSetupContract; + screenshotMode: ScreenshotModePluginSetup; logger: LevelLogger; } @@ -237,6 +239,11 @@ export class ReportingCore { return screenshotsObservableFactory(config.get('capture'), browserDriverFactory); } + public enableScreenshotMode() { + const { screenshotMode } = this.getPluginSetupDeps(); + return screenshotMode.setScreenshotModeEnabled; + } + /* * Gives synchronous access to the setupDeps */ diff --git a/x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts b/x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts index a10f1f7a3788da..dd8aadb49a5ba3 100644 --- a/x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts +++ b/x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts @@ -19,6 +19,7 @@ jest.mock('puppeteer', () => ({ import moment from 'moment'; import * as Rx from 'rxjs'; +import { ReportingCore } from '../..'; import { HeadlessChromiumDriver } from '../../browsers'; import { ConditionalHeaders } from '../../export_types/common'; import { @@ -27,6 +28,7 @@ import { createMockConfigSchema, createMockLayoutInstance, createMockLevelLogger, + createMockReportingCore, } from '../../test_helpers'; import { ElementsPositionAndAttribute } from './'; import * as contexts from './constants'; @@ -37,7 +39,7 @@ import { screenshotsObservableFactory } from './observable'; */ const logger = createMockLevelLogger(); -const reportingConfig = { +const mockSchema = createMockConfigSchema({ capture: { loadDelay: moment.duration(2, 's'), timeouts: { @@ -46,12 +48,13 @@ const reportingConfig = { renderComplete: moment.duration(10, 's'), }, }, -}; -const mockSchema = createMockConfigSchema(reportingConfig); +}); const mockConfig = createMockConfig(mockSchema); const captureConfig = mockConfig.get('capture'); const mockLayout = createMockLayoutInstance(captureConfig); +let core: ReportingCore; + /* * Tests */ @@ -59,7 +62,8 @@ describe('Screenshot Observable Pipeline', () => { let mockBrowserDriverFactory: any; beforeEach(async () => { - mockBrowserDriverFactory = await createMockBrowserDriverFactory(logger, {}); + core = await createMockReportingCore(mockSchema); + mockBrowserDriverFactory = await createMockBrowserDriverFactory(core, logger, {}); }); it('pipelines a single url into screenshot and timeRange', async () => { @@ -118,7 +122,7 @@ describe('Screenshot Observable Pipeline', () => { const mockOpen = jest.fn(); // mocks - mockBrowserDriverFactory = await createMockBrowserDriverFactory(logger, { + mockBrowserDriverFactory = await createMockBrowserDriverFactory(core, logger, { screenshot: mockScreenshot, open: mockOpen, }); @@ -218,7 +222,7 @@ describe('Screenshot Observable Pipeline', () => { }); // mocks - mockBrowserDriverFactory = await createMockBrowserDriverFactory(logger, { + mockBrowserDriverFactory = await createMockBrowserDriverFactory(core, logger, { waitForSelector: mockWaitForSelector, }); @@ -312,7 +316,7 @@ describe('Screenshot Observable Pipeline', () => { return Rx.never().toPromise(); }); - mockBrowserDriverFactory = await createMockBrowserDriverFactory(logger, { + mockBrowserDriverFactory = await createMockBrowserDriverFactory(core, logger, { getCreatePage: mockGetCreatePage, waitForSelector: mockWaitForSelector, }); @@ -345,7 +349,7 @@ describe('Screenshot Observable Pipeline', () => { return Promise.resolve(); } }); - mockBrowserDriverFactory = await createMockBrowserDriverFactory(logger, { + mockBrowserDriverFactory = await createMockBrowserDriverFactory(core, logger, { evaluate: mockBrowserEvaluate, }); mockLayout.getViewport = () => null; diff --git a/x-pack/plugins/reporting/server/plugin.ts b/x-pack/plugins/reporting/server/plugin.ts index 26a9be2b15c3f7..fc52e10dd0cf94 100644 --- a/x-pack/plugins/reporting/server/plugin.ts +++ b/x-pack/plugins/reporting/server/plugin.ts @@ -48,12 +48,13 @@ export class ReportingPlugin registerUiSettings(core); const { http } = core; - const { features, licensing, security, spaces, taskManager } = plugins; + const { screenshotMode, features, licensing, security, spaces, taskManager } = plugins; const router = http.createRouter(); const basePath = http.basePath; reportingCore.pluginSetup({ + screenshotMode, features, licensing, basePath, @@ -91,9 +92,8 @@ export class ReportingPlugin // async background start (async () => { await reportingCore.pluginSetsUp(); - const config = reportingCore.getConfig(); - const browserDriverFactory = await initializeBrowserDriverFactory(config, this.logger); + const browserDriverFactory = await initializeBrowserDriverFactory(reportingCore, this.logger); const store = new ReportingStore(reportingCore, this.logger); await reportingCore.pluginStart({ diff --git a/x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts b/x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts index 3446160c0d7f59..7dd7c246e9a048 100644 --- a/x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts +++ b/x-pack/plugins/reporting/server/test_helpers/create_mock_browserdriverfactory.ts @@ -8,6 +8,7 @@ import moment from 'moment'; import { Page } from 'puppeteer'; import * as Rx from 'rxjs'; +import { ReportingCore } from '..'; import { chromium, HeadlessChromiumDriver, HeadlessChromiumDriverFactory } from '../browsers'; import { LevelLogger } from '../lib'; import { ElementsPositionAndAttribute } from '../lib/screenshots'; @@ -96,6 +97,7 @@ const defaultOpts: CreateMockBrowserDriverFactoryOpts = { }; export const createMockBrowserDriverFactory = async ( + core: ReportingCore, logger: LevelLogger, opts: Partial = {} ): Promise => { @@ -122,9 +124,9 @@ export const createMockBrowserDriverFactory = async ( }; const binaryPath = '/usr/local/share/common/secure/super_awesome_binary'; - const mockBrowserDriverFactory = chromium.createDriverFactory(binaryPath, captureConfig, logger); + const mockBrowserDriverFactory = chromium.createDriverFactory(core, binaryPath, logger); const mockPage = ({ setViewport: () => {} } as unknown) as Page; - const mockBrowserDriver = new HeadlessChromiumDriver(mockPage, { + const mockBrowserDriver = new HeadlessChromiumDriver(core, mockPage, { inspect: true, networkPolicy: captureConfig.networkPolicy, }); diff --git a/x-pack/plugins/reporting/server/types.ts b/x-pack/plugins/reporting/server/types.ts index 757d1a68075a8d..7df1dce597d56b 100644 --- a/x-pack/plugins/reporting/server/types.ts +++ b/x-pack/plugins/reporting/server/types.ts @@ -8,6 +8,7 @@ import type { IRouter, KibanaRequest, RequestHandlerContext } from 'src/core/server'; // eslint-disable-next-line @kbn/eslint/no-restricted-paths import { DataPluginStart } from 'src/plugins/data/server/plugin'; +import { ScreenshotModePluginSetup } from 'src/plugins/screenshot_mode/server'; import { UsageCollectionSetup } from 'src/plugins/usage_collection/server'; import { PluginSetupContract as FeaturesPluginSetup } from '../../features/server'; import { LicensingPluginSetup } from '../../licensing/server'; @@ -32,6 +33,7 @@ export interface ReportingSetupDeps { spaces?: SpacesPluginSetup; taskManager: TaskManagerSetupContract; usageCollection?: UsageCollectionSetup; + screenshotMode: ScreenshotModePluginSetup; } export interface ReportingStartDeps { From 7dc5c0fd7b45bf863c11816c4203eecb52cbff54 Mon Sep 17 00:00:00 2001 From: Tim Sullivan Date: Fri, 14 May 2021 16:44:13 -0700 Subject: [PATCH 2/2] Update chromium_driver.ts --- .../reporting/server/browsers/chromium/driver/chromium_driver.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugins/reporting/server/browsers/chromium/driver/chromium_driver.ts b/x-pack/plugins/reporting/server/browsers/chromium/driver/chromium_driver.ts index 0ef995f72a5ef9..5e85124ad42f98 100644 --- a/x-pack/plugins/reporting/server/browsers/chromium/driver/chromium_driver.ts +++ b/x-pack/plugins/reporting/server/browsers/chromium/driver/chromium_driver.ts @@ -107,7 +107,6 @@ export class HeadlessChromiumDriver { this.interceptedCount = 0; const enableScreenshotMode = this.core.enableScreenshotMode(); - await this.page.evaluateOnNewDocument(enableScreenshotMode); await this.page.setRequestInterception(true);