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

Combine common pdf png functions #25152

Merged
12 changes: 12 additions & 0 deletions x-pack/plugins/reporting/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,18 @@ export const WHITELISTED_JOB_CONTENT_TYPES = [
'image/png',
];

export const KBN_SCREENSHOT_HEADER_BLACKLIST = [
'accept-encoding',
'content-length',
'content-type',
'host',
'referer',
// `Transfer-Encoding` is hop-by-hop header that is meaningful
// only for a single transport-level connection, and shouldn't
// be stored by caches or forwarded by proxies.
'transfer-encoding',
];

export const UI_SETTINGS_CUSTOM_PDF_LOGO = 'xpackReporting:customPdfLogo';

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

import { createMockServer } from '../../../test_helpers/create_mock_server';
import { addForceNowQuerystring } from './index';

let mockServer: any;
beforeEach(() => {
mockServer = createMockServer('');
});

test(`fails if no URL is passed`, async () => {
await expect(
addForceNowQuerystring({
job: {},
server: mockServer,
})
).rejects.toBeDefined();
});

test(`adds forceNow to hash's query, if it exists`, async () => {
const forceNow = '2000-01-01T00:00:00.000Z';
const { urls } = await addForceNowQuerystring({
job: { relativeUrl: '/app/kibana#/something', forceNow },
server: mockServer,
});

expect(urls[0]).toEqual(
'http://localhost:5601/sbp/app/kibana#/something?forceNow=2000-01-01T00%3A00%3A00.000Z'
);
});

test(`appends forceNow to hash's query, if it exists`, async () => {
const forceNow = '2000-01-01T00:00:00.000Z';

const { urls } = await addForceNowQuerystring({
job: {
relativeUrl: '/app/kibana#/something?_g=something',
forceNow,
},
server: mockServer,
});

expect(urls[0]).toEqual(
'http://localhost:5601/sbp/app/kibana#/something?_g=something&forceNow=2000-01-01T00%3A00%3A00.000Z'
);
});

test(`doesn't append forceNow query to url, if it doesn't exists`, async () => {
const { urls } = await addForceNowQuerystring({
job: {
relativeUrl: '/app/kibana#/something',
},
server: mockServer,
});

expect(urls[0]).toEqual('http://localhost:5601/sbp/app/kibana#/something');
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
// @ts-ignore
import url from 'url';
import { ConditionalHeaders, KbnServer, ReportingJob } from '../../../types';
import { getAbsoluteUrlFactory } from './get_absolute_url';

function getSavedObjectAbsoluteUrl(job: ReportingJob, relativeUrl: string, server: KbnServer) {
const getAbsoluteUrl: any = getAbsoluteUrlFactory(server);

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it common to handle the "happy path" in an if statement and throw at the bottom? I've typically seen error-path's be captured in an if block and the "happy path" follows below it. Similar to line 34 below. Example:

  if (!relativeUrl) {
    throw new Error(`Unable to generate report. Url is not defined.`);
  }
  const { pathname: path, hash, search } = url.parse(relativeUrl);
  return getAbsoluteUrl({ basePath: job.basePath, path, hash, search });

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored this function from an existing compatabilityShim function and it had the happy path first so I just followed what it had. Since I already have a check for this before calling the function I am going to remove the check in the function. So is the consensus that the error path should be first followed by the happy-path? Just so in the future I can be consistent

Copy link
Member

Choose a reason for hiding this comment

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

Since I already have a check for this before calling the function I am going to remove the check in the function

Thanks! That helps simplify

So is the consensus that the error path should be first followed by the happy-path?

Yes, that is preferred, so the error path can exit early when appropriate

const { pathname: path, hash, search } = url.parse(relativeUrl);
return getAbsoluteUrl({ basePath: job.basePath, path, hash, search });
}

export const addForceNowQuerystring = async ({
job,
conditionalHeaders,
logo,
server,
}: {
job: ReportingJob;
conditionalHeaders?: ConditionalHeaders;
logo?: any;
server: KbnServer;
Copy link
Member

Choose a reason for hiding this comment

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

Looks like urls missing from this definition

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above

}) => {
// if no URLS then its from PNG which should only have one so put it in the array and process as PDF does
if (!job.urls) {
if (!job.relativeUrl) {
throw new Error(`Unable to generate report. Url is not defined.`);
}
Copy link
Member

Choose a reason for hiding this comment

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

not sure if this block is needed since it looks like it happens on line 14 and the same error thrown on 19

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to leave these here and take the ones in the function on line 14 and 19 out since if I remove it here and try and reference job.relativeUrl to pass it along to the function it will throw an error.

job.urls = [getSavedObjectAbsoluteUrl(job, job.relativeUrl, server)];
}

const urls = job.urls.map(jobUrl => {
if (!job.forceNow) {
return jobUrl;
}

const parsed: any = url.parse(jobUrl, true);
const hash: any = url.parse(parsed.hash.replace(/^#/, ''), true);

const transformedHash = url.format({
pathname: hash.pathname,
query: {
...hash.query,
forceNow: job.forceNow,
},
});

return url.format({
...parsed,
hash: transformedHash,
});
});

return { job, conditionalHeaders, logo, urls, server };
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/

// @ts-ignore
import { cryptoFactory } from '../../../server/lib/crypto';
import { createMockServer } from '../../../test_helpers/create_mock_server';
import { decryptJobHeaders } from './index';

let mockServer: any;
beforeEach(() => {
mockServer = createMockServer('');
});

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

describe('headers', () => {
test(`fails if it can't decrypt headers`, async () => {
await expect(
decryptJobHeaders({
job: { relativeUrl: '/app/kibana#/something', timeRange: {} },
server: mockServer,
})
).rejects.toBeDefined();
});

test(`passes back decrypted headers that were passed in`, async () => {
const headers = {
foo: 'bar',
baz: 'quix',
};

const encryptedHeaders = await encryptHeaders(headers);
const { decryptedHeaders } = await decryptJobHeaders({
job: { relativeUrl: '/app/kibana#/something', headers: encryptedHeaders },
server: mockServer,
});
expect(decryptedHeaders).toEqual(headers);
});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
// @ts-ignore
import { cryptoFactory } from '../../../server/lib/crypto';
import { CryptoFactory, KbnServer, ReportingJob } from '../../../types';

export const decryptJobHeaders = async ({
job,
server,
}: {
job: ReportingJob;
server: KbnServer;
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't the definition of the return object include decryptedHeaders ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are the typescript definitions for the input parameters. decryptedHeaders is defined in the function and then returned along with job and server.

Copy link
Member

Choose a reason for hiding this comment

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

you are right, sorry for my confusion

}) => {
const crypto: CryptoFactory = cryptoFactory(server);
const decryptedHeaders: string = await crypto.decrypt(job.headers);
return { job, decryptedHeaders, server };
};
Original file line number Diff line number Diff line change
Expand Up @@ -4,118 +4,89 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { memoize } from 'lodash';
import { createMockServer } from '../../../test_helpers/create_mock_server';
import { getAbsoluteUrlFactory } from './get_absolute_url';

const createMockServer = ({ settings = {} } = {}) => {
const mockServer = {
expose: () => {},
config: memoize(() => {
return {
get: jest.fn()
};
}),
info: {
protocol: 'http'
}
};

const defaultSettings = {
'server.host': 'something',
'server.port': 8080,
'server.basePath': '/tst',
'xpack.reporting.kibanaServer': {}
};
mockServer.config().get.mockImplementation(key => {
return key in settings ? settings[key] : defaultSettings[key];
});

return mockServer;
};

test(`by default it builds url using information from server.info.protocol and the server.config`, () => {
const mockServer = createMockServer();

const mockServer = createMockServer('');
const getAbsoluteUrl = getAbsoluteUrlFactory(mockServer);
const absoluteUrl = getAbsoluteUrl();
expect(absoluteUrl).toBe(`http://something:8080/tst/app/kibana`);
expect(absoluteUrl).toBe(`http://localhost:5601/sbp/app/kibana`);
});

test(`uses kibanaServer.protocol if specified`, () => {
const settings = {
'xpack.reporting.kibanaServer.protocol': 'https'
'xpack.reporting.kibanaServer.protocol': 'https',
};
const mockServer = createMockServer({ settings });

const getAbsoluteUrl = getAbsoluteUrlFactory(mockServer);
const absoluteUrl = getAbsoluteUrl();
expect(absoluteUrl).toBe(`https://something:8080/tst/app/kibana`);
expect(absoluteUrl).toBe(`https://localhost:5601/sbp/app/kibana`);
});

test(`uses kibanaServer.hostname if specified`, () => {
const settings = {
'xpack.reporting.kibanaServer.hostname': 'something-else'
'xpack.reporting.kibanaServer.hostname': 'something-else',
};
const mockServer = createMockServer({ settings });

const getAbsoluteUrl = getAbsoluteUrlFactory(mockServer);
const absoluteUrl = getAbsoluteUrl();
expect(absoluteUrl).toBe(`http://something-else:8080/tst/app/kibana`);
expect(absoluteUrl).toBe(`http://something-else:5601/sbp/app/kibana`);
});

test(`uses kibanaServer.port if specified`, () => {
const settings = {
'xpack.reporting.kibanaServer.port': 8008
'xpack.reporting.kibanaServer.port': 8008,
};
const mockServer = createMockServer({ settings });

const getAbsoluteUrl = getAbsoluteUrlFactory(mockServer);
const absoluteUrl = getAbsoluteUrl();
expect(absoluteUrl).toBe(`http://something:8008/tst/app/kibana`);
expect(absoluteUrl).toBe(`http://localhost:8008/sbp/app/kibana`);
});

test(`uses the provided hash`, () => {
const mockServer = createMockServer();
const mockServer = createMockServer('');

const getAbsoluteUrl = getAbsoluteUrlFactory(mockServer);
const hash = '/hash';
const absoluteUrl = getAbsoluteUrl({ hash });
expect(absoluteUrl).toBe(`http://something:8080/tst/app/kibana#${hash}`);
expect(absoluteUrl).toBe(`http://localhost:5601/sbp/app/kibana#${hash}`);
});

test(`uses the provided hash with queryString`, () => {
const mockServer = createMockServer();
const mockServer = createMockServer('');

const getAbsoluteUrl = getAbsoluteUrlFactory(mockServer);
const hash = '/hash?querystring';
const absoluteUrl = getAbsoluteUrl({ hash });
expect(absoluteUrl).toBe(`http://something:8080/tst/app/kibana#${hash}`);
expect(absoluteUrl).toBe(`http://localhost:5601/sbp/app/kibana#${hash}`);
});

test(`uses the provided basePath`, () => {
const mockServer = createMockServer();
const mockServer = createMockServer('');

const getAbsoluteUrl = getAbsoluteUrlFactory(mockServer);
const absoluteUrl = getAbsoluteUrl({ basePath: '/s/marketing' });
expect(absoluteUrl).toBe(`http://something:8080/s/marketing/app/kibana`);
expect(absoluteUrl).toBe(`http://localhost:5601/s/marketing/app/kibana`);
});

test(`uses the path`, () => {
const mockServer = createMockServer();
const mockServer = createMockServer('');

const getAbsoluteUrl = getAbsoluteUrlFactory(mockServer);
const path = '/app/canvas';
const absoluteUrl = getAbsoluteUrl({ path });
expect(absoluteUrl).toBe(`http://something:8080/tst${path}`);
expect(absoluteUrl).toBe(`http://localhost:5601/sbp${path}`);
});

test(`uses the search`, () => {
const mockServer = createMockServer();
const mockServer = createMockServer('');

const getAbsoluteUrl = getAbsoluteUrlFactory(mockServer);
const search = '_t=123456789';
const absoluteUrl = getAbsoluteUrl({ search });
expect(absoluteUrl).toBe(`http://something:8080/tst/app/kibana?${search}`);
expect(absoluteUrl).toBe(`http://localhost:5601/sbp/app/kibana?${search}`);
});


Original file line number Diff line number Diff line change
Expand Up @@ -5,24 +5,26 @@
*/

import url from 'url';
// @ts-ignore
import { oncePerServer } from '../../../server/lib/once_per_server';
import { ConfigObject, KbnServer } from '../../../types';

function getAbsoluteUrlFn(server) {
const config = server.config();
function getAbsoluteUrlFn(server: KbnServer) {
const config: ConfigObject = server.config();

return function getAbsoluteUrl({
basePath = config.get('server.basePath'),
hash,
hash = '',
path = '/app/kibana',
search
search = '',
} = {}) {
return url.format({
protocol: config.get('xpack.reporting.kibanaServer.protocol') || server.info.protocol,
hostname: config.get('xpack.reporting.kibanaServer.hostname') || config.get('server.host'),
port: config.get('xpack.reporting.kibanaServer.port') || config.get('server.port'),
pathname: basePath + path,
hash: hash,
search
hash,
search,
});
};
}
Expand Down
Loading