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

Fix reporting tests spaces #7

Merged
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 @@ -109,13 +109,35 @@ describe('CSV Execute Job', function () {
mockServer.config().get.withArgs('xpack.reporting.csv.scroll').returns({});
});

describe('savedObjects', function () {
it('calls getScopedSavedObjectsClient with request containing decrypted headers', async function () {
describe('calls getScopedSavedObjectsClient with request', function () {
it('containing decrypted headers', async function () {
const executeJob = executeJobFactory(mockServer);
await executeJob({ headers: encryptedHeaders, fields: [], searchRequest: { index: null, body: null } }, cancellationToken);
expect(mockServer.savedObjects.getScopedSavedObjectsClient.calledOnce).to.be(true);
expect(mockServer.savedObjects.getScopedSavedObjectsClient.firstCall.args[0].headers).to.be.eql(headers);
});

it(`containing getBasePath() returning server's basePath if the job doesn't have one`, async function () {
const serverBasePath = '/foo-server/basePath/';
mockServer.config().get.withArgs('server.basePath').returns(serverBasePath);
const executeJob = executeJobFactory(mockServer);
await executeJob({ headers: encryptedHeaders, fields: [], searchRequest: { index: null, body: null } }, cancellationToken);
expect(mockServer.savedObjects.getScopedSavedObjectsClient.calledOnce).to.be(true);
expect(mockServer.savedObjects.getScopedSavedObjectsClient.firstCall.args[0].getBasePath()).to.be.eql(serverBasePath);
});

it(`containing getBasePath() returning job's basePath if the job has one`, async function () {
const serverBasePath = '/foo-server/basePath/';
mockServer.config().get.withArgs('server.basePath').returns(serverBasePath);
const executeJob = executeJobFactory(mockServer);
const jobBasePath = 'foo-job/basePath/';
await executeJob(
{ headers: encryptedHeaders, fields: [], searchRequest: { index: null, body: null }, basePath: jobBasePath },
cancellationToken
);
expect(mockServer.savedObjects.getScopedSavedObjectsClient.calledOnce).to.be(true);
expect(mockServer.savedObjects.getScopedSavedObjectsClient.firstCall.args[0].getBasePath()).to.be.eql(jobBasePath);
});
});

describe('uiSettings', function () {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ function createJobFn(server) {
return {
headers: serializedEncryptedHeaders,
indexPatternSavedObject: indexPatternSavedObject,
basePath: request.getBasePath(),
...jobParams
};
};
Expand Down
15 changes: 13 additions & 2 deletions x-pack/plugins/reporting/export_types/csv/server/execute_job.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,15 @@ function executeJobFn(server) {
const serverBasePath = config.get('server.basePath');

return async function executeJob(job, cancellationToken) {
const { searchRequest, fields, indexPatternSavedObject, metaFields, conflictedTypesFields, headers: serializedEncryptedHeaders } = job;
const {
searchRequest,
fields,
indexPatternSavedObject,
metaFields,
conflictedTypesFields,
headers: serializedEncryptedHeaders,
basePath
} = job;

let decryptedHeaders;
try {
Expand All @@ -32,7 +40,10 @@ function executeJobFn(server) {

const fakeRequest = {
headers: decryptedHeaders,
getBasePath: () => serverBasePath,
// This is used by the spaces SavedObjectClientWrapper to determine the existing space.
// We use the basePath from the saved job, which we'll have post spaces being implemented;
// or we use the server base path, which uses the default space
getBasePath: () => basePath || serverBasePath,
};

const callEndpoint = (endpoint, clientParams = {}, options = {}) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ function createJobFn(server) {
relativeUrls,
browserTimezone,
layout
}, headers) {
}, headers, request) {
const serializedEncryptedHeaders = await crypto.encrypt(headers);

return {
Expand All @@ -28,6 +28,7 @@ function createJobFn(server) {
headers: serializedEncryptedHeaders,
browserTimezone,
layout,
basePath: request.getBasePath(),
forceNow: new Date().toISOString(),
};
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +10,28 @@ import { getAbsoluteUrlFactory } from './get_absolute_url';
export function compatibilityShimFactory(server) {
const getAbsoluteUrl = getAbsoluteUrlFactory(server);

const getSavedObjectAbsoluteUrl = (savedObj) => {
if (savedObj.urlHash) {
return getAbsoluteUrl({ hash: savedObj.urlHash });
const getSavedObjectAbsoluteUrl = (job, savedObject) => {
if (savedObject.urlHash) {
return getAbsoluteUrl({ hash: savedObject.urlHash });
}

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

if (savedObj.url.startsWith(getAbsoluteUrl())) {
return savedObj.url;
if (savedObject.url.startsWith(getAbsoluteUrl())) {
return savedObject.url;
}

throw new Error(`Unable to generate report for url ${savedObj.url}, it's not a Kibana URL`);
throw new Error(`Unable to generate report for url ${savedObject.url}, it's not a Kibana URL`);
};

return function (executeJob) {
return async function (job, cancellationToken) {
const urls = job.objects.map(getSavedObjectAbsoluteUrl);
const urls = job.objects.map(savedObject => getSavedObjectAbsoluteUrl(job, savedObject));

return await executeJob({ ...job, urls }, cancellationToken);
};
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ test(`it generates the absolute url if a urlHash is provided`, async () => {
expect(mockCreateJob.mock.calls[0][0].urls[0]).toBe('http://localhost:5601/app/kibana#visualize');
});

test(`it generates the absolute url if a relativeUrl is provided`, async () => {
test(`it generates the absolute url using server's basePath if a relativeUrl is provided`, async () => {
const mockCreateJob = jest.fn();
const compatibilityShim = compatibilityShimFactory(createMockServer());

Expand All @@ -64,7 +64,17 @@ test(`it generates the absolute url if a relativeUrl is provided`, async () => {
expect(mockCreateJob.mock.calls[0][0].urls[0]).toBe('http://localhost:5601/app/kibana#/visualize?');
});

test(`it generates the absolute url if a relativeUrl with querystring is provided`, async () => {
test(`it generates the absolute url using job's basePath if a relativeUrl is provided`, async () => {
const mockCreateJob = jest.fn();
const compatibilityShim = compatibilityShimFactory(createMockServer());

const relativeUrl = '/app/kibana#/visualize?';
await compatibilityShim(mockCreateJob)({ basePath: '/s/marketing', objects: [ { relativeUrl } ] });
expect(mockCreateJob.mock.calls.length).toBe(1);
expect(mockCreateJob.mock.calls[0][0].urls[0]).toBe('http://localhost:5601/s/marketing/app/kibana#/visualize?');
});

test(`it generates the absolute url using server's basePath if a relativeUrl with querystring is provided`, async () => {
const mockCreateJob = jest.fn();
const compatibilityShim = compatibilityShimFactory(createMockServer());

Expand All @@ -74,6 +84,16 @@ test(`it generates the absolute url if a relativeUrl with querystring is provide
expect(mockCreateJob.mock.calls[0][0].urls[0]).toBe('http://localhost:5601/app/kibana?_t=123456789#/visualize?_g=()');
});

test(`it generates the absolute url using job's basePath if a relativeUrl with querystring is provided`, async () => {
const mockCreateJob = jest.fn();
const compatibilityShim = compatibilityShimFactory(createMockServer());

const relativeUrl = '/app/kibana?_t=123456789#/visualize?_g=()';
await compatibilityShim(mockCreateJob)({ basePath: '/s/marketing', objects: [ { relativeUrl } ] });
expect(mockCreateJob.mock.calls.length).toBe(1);
expect(mockCreateJob.mock.calls[0][0].urls[0]).toBe('http://localhost:5601/s/marketing/app/kibana?_t=123456789#/visualize?_g=()');
});

test(`it passes the provided browserTimezone through`, async () => {
const mockCreateJob = jest.fn();
const compatibilityShim = compatibilityShimFactory(createMockServer());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ function getAbsoluteUrlFn(server) {
const config = server.config();

return function getAbsoluteUrl({
basePath = config.get('server.basePath'),
hash,
path = '/app/kibana',
search
Expand All @@ -19,7 +20,7 @@ function getAbsoluteUrlFn(server) {
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: config.get('server.basePath') + path,
pathname: basePath + path,
hash: hash,
search
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,14 @@ test(`uses the provided hash with queryString`, () => {
expect(absoluteUrl).toBe(`http://something:8080/tst/app/kibana#${hash}`);
});

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

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

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

Expand All @@ -109,3 +117,5 @@ test(`uses the search`, () => {
const absoluteUrl = getAbsoluteUrl({ search });
expect(absoluteUrl).toBe(`http://something:8080/tst/app/kibana?${search}`);
});


Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,10 @@ function executeJobFn(server) {
const getCustomLogo = async ({ job, filteredHeaders }) => {
const fakeRequest = {
headers: filteredHeaders,
getBasePath: () => serverBasePath
// This is used by the spaces SavedObjectClientWrapper to determine the existing space.
// We use the basePath from the saved job, which we'll have post spaces being implemented;
// or we use the server base path, which uses the default space
getBasePath: () => job.basePath || serverBasePath
};

const savedObjects = server.savedObjects;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ beforeEach(() => {
'xpack.reporting.kibanaServer.protocol': 'http',
'xpack.reporting.kibanaServer.hostname': 'localhost',
'xpack.reporting.kibanaServer.port': 5601,
'server.basePath': ''
'server.basePath': '/sbp'
}[key];
});

Expand Down Expand Up @@ -106,6 +106,37 @@ test(`omits blacklisted headers`, async () => {
expect(generatePdfObservable).toBeCalledWith(undefined, [], undefined, permittedHeaders, undefined, undefined);
});

test('uses basePath from job when creating saved object service', async () => {
const encryptedHeaders = await encryptHeaders({});

const logo = 'custom-logo';
mockServer.uiSettingsServiceFactory().get.mockReturnValue(logo);

const generatePdfObservable = generatePdfObservableFactory();
generatePdfObservable.mockReturnValue(Rx.of(Buffer.from('')));

const executeJob = executeJobFactory(mockServer);
const jobBasePath = '/sbp/s/marketing';
await executeJob({ objects: [], headers: encryptedHeaders, basePath: jobBasePath }, cancellationToken);

expect(mockServer.savedObjects.getScopedSavedObjectsClient.mock.calls[0][0].getBasePath()).toBe(jobBasePath);
});

test(`uses basePath from server if job doesn't have a basePath when creating saved object service`, async () => {
const encryptedHeaders = await encryptHeaders({});

const logo = 'custom-logo';
mockServer.uiSettingsServiceFactory().get.mockReturnValue(logo);

const generatePdfObservable = generatePdfObservableFactory();
generatePdfObservable.mockReturnValue(Rx.of(Buffer.from('')));

const executeJob = executeJobFactory(mockServer);
await executeJob({ objects: [], headers: encryptedHeaders }, cancellationToken);

expect(mockServer.savedObjects.getScopedSavedObjectsClient.mock.calls[0][0].getBasePath()).toBe('/sbp');
});

test(`gets logo from uiSettings`, async () => {
const encryptedHeaders = await encryptHeaders({});

Expand Down Expand Up @@ -145,9 +176,9 @@ test(`adds forceNow to hash's query, if it exists`, async () => {
const executeJob = executeJobFactory(mockServer);
const forceNow = '2000-01-01T00:00:00.000Z';

await executeJob({ objects: [{ relativeUrl: 'app/kibana#/something' }], forceNow, headers: encryptedHeaders }, cancellationToken);
await executeJob({ objects: [{ relativeUrl: '/app/kibana#/something' }], forceNow, headers: encryptedHeaders }, cancellationToken);

expect(generatePdfObservable).toBeCalledWith(undefined, ['http://localhost:5601/app/kibana#/something?forceNow=2000-01-01T00%3A00%3A00.000Z'], undefined, {}, undefined, undefined);
expect(generatePdfObservable).toBeCalledWith(undefined, ['http://localhost:5601/sbp/app/kibana#/something?forceNow=2000-01-01T00%3A00%3A00.000Z'], undefined, {}, undefined, undefined);
});

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

await executeJob({
objects: [{ relativeUrl: 'app/kibana#/something?_g=something' }],
objects: [{ relativeUrl: '/app/kibana#/something?_g=something' }],
Copy link
Owner

Choose a reason for hiding this comment

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

For my own education: what is the reason for this change? Is this something that existing/stored jobs will have a problem with?

Copy link
Author

Choose a reason for hiding this comment

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

tldr; I exacerbated a problem with the old tests that existed if you changed the tests to use server.basePath other than ''.

The old tests weren't using the server.basePath so the URL parsing just happened to be working with the relativeUrl that didn't start with a /, even though the relativeUrls that we create via the API required them to start with a slash or else we weren't building the URL properly.

forceNow,
headers: encryptedHeaders
}, cancellationToken);

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

test(`doesn't append forceNow query to url, if it doesn't exists`, async () => {
Expand All @@ -176,9 +207,9 @@ test(`doesn't append forceNow query to url, if it doesn't exists`, async () => {

const executeJob = executeJobFactory(mockServer);

await executeJob({ objects: [{ relativeUrl: 'app/kibana#/something' }], headers: encryptedHeaders }, cancellationToken);
await executeJob({ objects: [{ relativeUrl: '/app/kibana#/something' }], headers: encryptedHeaders }, cancellationToken);

expect(generatePdfObservable).toBeCalledWith(undefined, ['http://localhost:5601/app/kibana#/something'], undefined, {}, undefined, undefined);
expect(generatePdfObservable).toBeCalledWith(undefined, ['http://localhost:5601/sbp/app/kibana#/something'], undefined, {}, undefined, undefined);
});

test(`returns content_type of application/pdf`, async () => {
Expand Down