Skip to content

Commit

Permalink
design feedback to avoid handling the entire config getter
Browse files Browse the repository at this point in the history
  • Loading branch information
tsullivan committed Mar 21, 2020
1 parent 29e99c3 commit ad8269c
Show file tree
Hide file tree
Showing 38 changed files with 101 additions and 110 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -5,37 +5,25 @@
*/

import { cryptoFactory } from '../../../server/lib/crypto';
import { ReportingConfig } from '../../../server/types';
import { Logger } from '../../../types';
import { decryptJobHeaders } from './decrypt_job_headers';

const getMockConfig = (mockGet: jest.Mock<any, any>) => ({
get: mockGet,
kbnConfig: { get: mockGet },
});
const mockConfigGet = jest.fn().mockImplementation((key: string) => {
if (key === 'encryptionKey') {
return 'testencryptionkey';
}
});
const mockConfig = getMockConfig(mockConfigGet);

const encryptHeaders = async (config: ReportingConfig, headers: Record<string, string>) => {
const crypto = cryptoFactory(config);
const encryptHeaders = async (encryptionKey: string, headers: Record<string, string>) => {
const crypto = cryptoFactory(encryptionKey);
return await crypto.encrypt(headers);
};

describe('headers', () => {
test(`fails if it can't decrypt headers`, async () => {
const getDecryptedHeaders = () =>
decryptJobHeaders({
encryptionKey: 'abcsecretsauce',
job: {
headers: 'Q53+9A+zf+Xe+ceR/uB/aR/Sw/8e+M+qR+WiG+8z+EY+mo+HiU/zQL+Xn',
},
logger: ({
error: jest.fn(),
} as unknown) as Logger,
config: mockConfig,
});
await expect(getDecryptedHeaders()).rejects.toMatchInlineSnapshot(
`[Error: Failed to decrypt report job data. Please ensure that xpack.reporting.encryptionKey is set and re-generate this report. Error: Invalid IV length]`
Expand All @@ -48,15 +36,15 @@ describe('headers', () => {
baz: 'quix',
};

const encryptedHeaders = await encryptHeaders(mockConfig, headers);
const encryptedHeaders = await encryptHeaders('abcsecretsauce', headers);
const decryptedHeaders = await decryptJobHeaders({
encryptionKey: 'abcsecretsauce',
job: {
title: 'cool-job-bro',
type: 'csv',
headers: encryptedHeaders,
},
logger: {} as Logger,
config: mockConfig,
});
expect(decryptedHeaders).toEqual(headers);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

import { i18n } from '@kbn/i18n';
import { cryptoFactory } from '../../../server/lib/crypto';
import { ReportingConfig } from '../../../server/types';
import { CryptoFactory, Logger } from '../../../types';

interface HasEncryptedHeaders {
Expand All @@ -18,15 +17,15 @@ export const decryptJobHeaders = async <
JobParamsType,
JobDocPayloadType extends HasEncryptedHeaders
>({
config,
encryptionKey,
job,
logger,
}: {
config: ReportingConfig;
encryptionKey?: string;
job: JobDocPayloadType;
logger: Logger;
}): Promise<Record<string, string>> => {
const crypto: CryptoFactory = cryptoFactory(config);
const crypto: CryptoFactory = cryptoFactory(encryptionKey);
try {
const decryptedHeaders: Record<string, string> = await crypto.decrypt(job.headers);
return decryptedHeaders;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { ReportingConfig } from '../../../server/types';
import { CaptureConfig } from '../../../server/types';
import { LayoutTypes } from '../constants';
import { Layout, LayoutParams } from './layout';
import { PreserveLayout } from './preserve_layout';
import { PrintLayout } from './print_layout';

export function createLayout(config: ReportingConfig, layoutParams?: LayoutParams): Layout {
export function createLayout(captureConfig: CaptureConfig, layoutParams?: LayoutParams): Layout {
if (layoutParams && layoutParams.id === LayoutTypes.PRESERVE_LAYOUT) {
return new PreserveLayout(layoutParams.dimensions);
}

// this is the default because some jobs won't have anything specified
return new PrintLayout(config);
return new PrintLayout(captureConfig);
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import path from 'path';
import { EvaluateFn, SerializableOrJSHandle } from 'puppeteer';
import { HeadlessChromiumDriver } from '../../../server/browsers';
import { LevelLogger } from '../../../server/lib';
import { ReportingConfig } from '../../../server/types';
import { ReportingConfigType } from '../../../server/core';
import { LayoutTypes } from '../constants';
import { getDefaultLayoutSelectors, Layout, LayoutSelectorDictionary, Size } from './layout';
import { CaptureConfig } from './types';
Expand All @@ -21,9 +21,9 @@ export class PrintLayout extends Layout {
public readonly groupCount = 2;
private captureConfig: CaptureConfig;

constructor(config: ReportingConfig) {
constructor(captureConfig: ReportingConfigType['capture']) {
super(LayoutTypes.PRINT);
this.captureConfig = config.get('capture');
this.captureConfig = captureConfig;
}

public getCssOverridesPath() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
import { i18n } from '@kbn/i18n';
import { HeadlessChromiumDriver as HeadlessBrowser } from '../../../../server/browsers';
import { LevelLogger } from '../../../../server/lib';
import { CaptureConfig } from '../../../../server/types';
import { LayoutInstance } from '../../layouts/layout';
import { CONTEXT_GETNUMBEROFITEMS, CONTEXT_READMETADATA } from './constants';
import { CaptureConfig } from './types';

export const getNumberOfItems = async (
captureConfig: CaptureConfig,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,12 @@ jest.mock('../../../../server/browsers/chromium/puppeteer', () => ({
}));

import * as Rx from 'rxjs';
import { get } from 'lodash';
// eslint-disable-next-line @kbn/eslint/no-restricted-paths
import { loggingServiceMock } from '../../../../../../../../src/core/server/mocks';
import { LevelLogger } from '../../../../server/lib';
import { createMockBrowserDriverFactory, createMockLayoutInstance } from '../../../../test_helpers';
import { ConditionalHeaders, HeadlessChromiumDriver } from '../../../../types';
import { ReportingConfig } from '../../../../server/types';
import { CaptureConfig } from '../../../../server/types';
import { screenshotsObservableFactory } from './observable';
import { ElementsPositionAndAttribute } from './types';

Expand All @@ -32,10 +31,7 @@ import { ElementsPositionAndAttribute } from './types';
const mockLogger = jest.fn(loggingServiceMock.create);
const logger = new LevelLogger(mockLogger());

const mockConfig = {
get: (key: string) => get({ capture: { timeouts: { openUrl: 13 } } }, key, null),
kbnConfig: { get: jest.fn() },
} as ReportingConfig;
const mockConfig = { timeouts: { openUrl: 13 } } as CaptureConfig;
const mockLayout = createMockLayoutInstance(mockConfig);

/*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import * as Rx from 'rxjs';
import { catchError, concatMap, first, mergeMap, take, takeUntil, toArray } from 'rxjs/operators';
import { ReportingConfig } from '../../../../server';
import { CaptureConfig } from '../../../../server/types';
import { HeadlessChromiumDriverFactory } from '../../../../types';
import { getElementPositionAndAttributes } from './get_element_position_data';
import { getNumberOfItems } from './get_number_of_items';
Expand All @@ -19,11 +19,9 @@ import { waitForRenderComplete } from './wait_for_render';
import { waitForVisualizations } from './wait_for_visualizations';

export function screenshotsObservableFactory(
config: ReportingConfig,
captureConfig: CaptureConfig,
browserDriverFactory: HeadlessChromiumDriverFactory
) {
const captureConfig = config.get('capture');

return function screenshotsObservable({
logger,
urls,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
import { i18n } from '@kbn/i18n';
import { HeadlessChromiumDriver as HeadlessBrowser } from '../../../../server/browsers';
import { LevelLogger } from '../../../../server/lib';
import { CaptureConfig } from '../../../../server/types';
import { ConditionalHeaders } from '../../../../types';
import { PAGELOAD_SELECTOR } from '../../constants';
import { CaptureConfig } from './types';

export const openUrl = async (
captureConfig: CaptureConfig,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,9 @@
*/

import { LevelLogger } from '../../../../server/lib';
import { ReportingConfigType } from '../../../../server/types';
import { ConditionalHeaders, ElementPosition } from '../../../../types';
import { LayoutInstance } from '../../layouts/layout';

export type CaptureConfig = ReportingConfigType['capture'];

export interface ScreenshotObservableOpts {
logger: LevelLogger;
urls: string[];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
import { i18n } from '@kbn/i18n';
import { HeadlessChromiumDriver as HeadlessBrowser } from '../../../../server/browsers';
import { LevelLogger } from '../../../../server/lib';
import { CaptureConfig } from '../../../../server/types';
import { LayoutInstance } from '../../layouts/layout';
import { CONTEXT_WAITFORRENDER } from './constants';
import { CaptureConfig } from './types';

export const waitForRenderComplete = async (
captureConfig: CaptureConfig,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
import { i18n } from '@kbn/i18n';
import { HeadlessChromiumDriver as HeadlessBrowser } from '../../../../server/browsers';
import { LevelLogger } from '../../../../server/lib';
import { CaptureConfig } from '../../../../server/types';
import { LayoutInstance } from '../../layouts/layout';
import { CONTEXT_WAITFORELEMENTSTOBEINDOM } from './constants';
import { CaptureConfig } from './types';

type SelectorArgs = Record<string, string>;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export const createJobFactory: CreateJobFactory<ESQueueCreateJobFn<
JobParamsDiscoverCsv
>> = async function createJobFactoryFn(reporting: ReportingCore) {
const config = await reporting.getConfig();
const crypto = cryptoFactory(config);
const crypto = cryptoFactory(config.get('encryptionKey'));

return async function createJob(
jobParams: JobParamsDiscoverCsv,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export const executeJobFactory: ExecuteJobFactory<ESQueueWorkerExecuteFn<
reporting.getConfig(),
reporting.getElasticsearchService(),
]);
const crypto = cryptoFactory(config);
const crypto = cryptoFactory(config.get('encryptionKey'));
const logger = parentLogger.clone([CSV_JOB_TYPE, 'execute-job']);
const serverBasePath = config.kbnConfig.get('server', 'basePath');

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,10 @@
import expect from '@kbn/expect';
import sinon from 'sinon';
import { CancellationToken } from '../../../../common/cancellation_token';
import { ReportingConfigType } from '../../../../server/types';
import { ScrollConfig } from '../../../../server/types';
import { Logger } from '../../../../types';
import { createHitIterator } from './hit_iterator';

type ScrollConfig = ReportingConfigType['csv']['scroll'];

const mockLogger = {
error: new Function(),
debug: new Function(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import { i18n } from '@kbn/i18n';
import { SearchParams, SearchResponse } from 'elasticsearch';
import { ReportingConfigType } from '../../../../server/types';
import { ScrollConfig } from '../../../../server/types';
import { CancellationToken, Logger } from '../../../../types';

async function parseResponse(request: SearchResponse<any>) {
Expand Down Expand Up @@ -37,7 +37,7 @@ async function parseResponse(request: SearchResponse<any>) {

export function createHitIterator(logger: Logger) {
return async function* hitIterator(
scrollSettings: ReportingConfigType['csv']['scroll'],
scrollSettings: ScrollConfig,
callEndpoint: Function,
searchRequest: SearchParams,
cancellationToken: CancellationToken
Expand Down
4 changes: 2 additions & 2 deletions x-pack/legacy/plugins/reporting/export_types/csv/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*/

import { CancellationToken } from '../../common/cancellation_token';
import { ReportingConfigType } from '../../server/types';
import { ScrollConfig } from '../../server/types';
import { JobDocPayload, JobParamPostPayload } from '../../types';

interface DocValueField {
Expand Down Expand Up @@ -107,7 +107,7 @@ export interface GenerateCsvParams {
quoteValues: boolean;
timezone: string | null;
maxSizeBytes: number;
scroll: ReportingConfigType['csv']['scroll'];
scroll: ScrollConfig;
checkForFormulas?: boolean;
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ export const createJobFactory: CreateJobFactory<ImmediateCreateJobFn<
JobParamsPanelCsv
>> = async function createJobFactoryFn(reporting: ReportingCore, parentLogger: Logger) {
const config = await reporting.getConfig();
const crypto = cryptoFactory(config);
const crypto = cryptoFactory(config.get('encryptionKey'));
const logger = parentLogger.clone([CSV_FROM_SAVEDOBJECT_JOB_TYPE, 'create-job']);

return async function createJob(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ export const executeJobFactory: ExecuteJobFactory<ImmediateExecuteFn<
JobParamsPanelCsv
>> = async function executeJobFactoryFn(reporting: ReportingCore, parentLogger: Logger) {
const config = await reporting.getConfig();
const crypto = cryptoFactory(config);
const crypto = cryptoFactory(config.get('encryptionKey'));
const logger = parentLogger.clone([CSV_FROM_SAVEDOBJECT_JOB_TYPE, 'execute-job']);
const generateCsv = await createGenerateCsv(reporting, parentLogger);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export const createJobFactory: CreateJobFactory<ESQueueCreateJobFn<
JobParamsPNG
>> = async function createJobFactoryFn(reporting: ReportingCore) {
const config = await reporting.getConfig();
const crypto = cryptoFactory(config);
const crypto = cryptoFactory(config.get('encryptionKey'));

return async function createJob(
{ objectType, title, relativeUrl, browserTimezone, layout }: JobParamsPNG,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,10 @@ const mockLoggerFactory = {
})),
};
const getMockLogger = () => new LevelLogger(mockLoggerFactory);
const encryptHeaders = async (config, headers) => {
const crypto = cryptoFactory(config);

const mockEncryptionKey = 'abcabcsecuresecret';
const encryptHeaders = async headers => {
const crypto = cryptoFactory(mockEncryptionKey);
return await crypto.encrypt(headers);
};

Expand All @@ -40,7 +42,7 @@ beforeEach(async () => {
'server.basePath': '/sbp',
};
const reportingConfig = {
encryptionKey: 'testencryptionkey',
encryptionKey: mockEncryptionKey,
'kibanaServer.hostname': 'localhost',
'kibanaServer.port': 5601,
'kibanaServer.protocol': 'http',
Expand Down Expand Up @@ -69,7 +71,7 @@ beforeEach(async () => {
afterEach(() => generatePngObservableFactory.mockReset());

test(`passes browserTimezone to generatePng`, async () => {
const encryptedHeaders = await encryptHeaders(mockReportingConfig, {});
const encryptedHeaders = await encryptHeaders({});

const generatePngObservable = generatePngObservableFactory();
generatePngObservable.mockReturnValue(Rx.of(Buffer.from('')));
Expand All @@ -93,7 +95,7 @@ test(`passes browserTimezone to generatePng`, async () => {

test(`returns content_type of application/png`, async () => {
const executeJob = await executeJobFactory(mockReporting, getMockLogger());
const encryptedHeaders = await encryptHeaders(mockReportingConfig, {});
const encryptedHeaders = await encryptHeaders({});

const generatePngObservable = generatePngObservableFactory();
generatePngObservable.mockReturnValue(Rx.of(Buffer.from('')));
Expand All @@ -113,7 +115,7 @@ test(`returns content of generatePng getBuffer base64 encoded`, async () => {
generatePngObservable.mockReturnValue(Rx.of({ buffer: Buffer.from(testContent) }));

const executeJob = await executeJobFactory(mockReporting, getMockLogger());
const encryptedHeaders = await encryptHeaders(mockReportingConfig, {});
const encryptedHeaders = await encryptHeaders({});
const { content } = await executeJob(
'pngJobId',
{ relativeUrl: '/app/kibana#/something', timeRange: {}, headers: encryptedHeaders },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,14 +25,17 @@ export const executeJobFactory: QueuedPngExecutorFactory = async function execut
parentLogger: Logger
) {
const config = await reporting.getConfig();
const captureConfig = config.get('capture');
const encryptionKey = config.get('encryptionKey');

const browserDriverFactory = await reporting.getBrowserDriverFactory();
const generatePngObservable = generatePngObservableFactory(config, browserDriverFactory);
const generatePngObservable = generatePngObservableFactory(captureConfig, browserDriverFactory);
const logger = parentLogger.clone([PNG_JOB_TYPE, 'execute']);

return function executeJob(jobId: string, job: JobDocPayloadPNG, cancellationToken: any) {
const jobLogger = logger.clone([jobId]);
const process$: Rx.Observable<JobDocOutput> = Rx.of(1).pipe(
mergeMap(() => decryptJobHeaders({ config, job, logger })),
mergeMap(() => decryptJobHeaders({ encryptionKey, job, logger })),
map(decryptedHeaders => omitBlacklistedHeaders({ job, decryptedHeaders })),
map(filteredHeaders => getConditionalHeaders({ config, job, filteredHeaders })),
mergeMap(conditionalHeaders => {
Expand Down
Loading

0 comments on commit ad8269c

Please sign in to comment.