Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

move require() to screenshotMode plugin #3

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, unknown>)[KBN_SCREENSHOT_MODE_ENABLED_KEY] ||
Boolean(((window as unknown) as Record<string, unknown>)[KBN_SCREENSHOT_MODE_ENABLED_KEY]) ||
Copy link
Owner

@jloleysens jloleysens May 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to update this function to return a boolean!

However, I'd prefer this code to be explicit about the value it expects to find at KBN_SCREENSHOT_MODE_ENABLED_KEY since this forms part of the contract it seems unintentional to accept objects etc for this value. I'll update on my PR with my take!

window.localStorage.getItem(KBN_SCREENSHOT_MODE_ENABLED_KEY) === 'true'
);
};
Expand Down
2 changes: 2 additions & 0 deletions src/plugins/screenshot_mode/server/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
15 changes: 14 additions & 1 deletion src/plugins/screenshot_mode/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<ScreenshotModePluginSetup> {
public setup(core: CoreSetup) {
core.http.registerRouteHandlerContext<ScreenshotModeRequestHandlerContext, 'screenshotMode'>(
'screenshotMode',
Expand All @@ -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() {}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Comment on lines 13 to +15
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style question: I see the imports with files further away (like ReportingCore) are below the import for getDisallowedOutgoingUrlError. Typically I place imports that are further away higher up in import section. Is there a specific order that you prefer or was this not intentional?

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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -105,7 +106,8 @@ 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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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.`);
Expand Down Expand Up @@ -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,
});
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugins/reporting/server/browsers/chromium/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 = () =>
Expand Down
13 changes: 4 additions & 9 deletions x-pack/plugins/reporting/server/browsers/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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;

Expand All @@ -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);
};
7 changes: 7 additions & 0 deletions x-pack/plugins/reporting/server/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -41,6 +42,7 @@ export interface ReportingInternalSetup {
security?: SecurityPluginSetup;
spaces?: SpacesPluginSetup;
taskManager: TaskManagerSetupContract;
screenshotMode: ScreenshotModePluginSetup;
logger: LevelLogger;
}

Expand Down Expand Up @@ -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
*/
Expand Down
20 changes: 12 additions & 8 deletions x-pack/plugins/reporting/server/lib/screenshots/observable.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -27,6 +28,7 @@ import {
createMockConfigSchema,
createMockLayoutInstance,
createMockLevelLogger,
createMockReportingCore,
} from '../../test_helpers';
import { ElementsPositionAndAttribute } from './';
import * as contexts from './constants';
Expand All @@ -37,7 +39,7 @@ import { screenshotsObservableFactory } from './observable';
*/
const logger = createMockLevelLogger();

const reportingConfig = {
const mockSchema = createMockConfigSchema({
capture: {
loadDelay: moment.duration(2, 's'),
timeouts: {
Expand All @@ -46,20 +48,22 @@ 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
*/
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 () => {
Expand Down Expand Up @@ -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,
});
Expand Down Expand Up @@ -218,7 +222,7 @@ describe('Screenshot Observable Pipeline', () => {
});

// mocks
mockBrowserDriverFactory = await createMockBrowserDriverFactory(logger, {
mockBrowserDriverFactory = await createMockBrowserDriverFactory(core, logger, {
waitForSelector: mockWaitForSelector,
});

Expand Down Expand Up @@ -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,
});
Expand Down Expand Up @@ -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;
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugins/reporting/server/plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<ReportingRequestHandlerContext>();
const basePath = http.basePath;

reportingCore.pluginSetup({
screenshotMode,
features,
licensing,
basePath,
Expand Down Expand Up @@ -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({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -96,6 +97,7 @@ const defaultOpts: CreateMockBrowserDriverFactoryOpts = {
};

export const createMockBrowserDriverFactory = async (
core: ReportingCore,
logger: LevelLogger,
opts: Partial<CreateMockBrowserDriverFactoryOpts> = {}
): Promise<HeadlessChromiumDriverFactory> => {
Expand All @@ -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,
});
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/reporting/server/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -32,6 +33,7 @@ export interface ReportingSetupDeps {
spaces?: SpacesPluginSetup;
taskManager: TaskManagerSetupContract;
usageCollection?: UsageCollectionSetup;
screenshotMode: ScreenshotModePluginSetup;
}

export interface ReportingStartDeps {
Expand Down