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

[Reporting] Convert Export Type Definitions to Typescript #51643

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
5531c06
simplify ts
tsullivan Nov 22, 2019
d409bf0
fix generate_png + get_full_url
tsullivan Nov 22, 2019
87fb7a0
fix pdf execute job
tsullivan Nov 22, 2019
4d8f03e
fix pdf create job
tsullivan Nov 22, 2019
74caad1
fix decrypt job headers
tsullivan Nov 22, 2019
c4aff15
fix generate pdf / generate png
tsullivan Nov 22, 2019
dea6767
remove log
tsullivan Nov 22, 2019
9729aea
export consts
tsullivan Nov 22, 2019
0ac0879
move export type registration to ts
tsullivan Nov 23, 2019
e5b9fd4
more export type registration to ts
tsullivan Nov 23, 2019
aabd4da
ts generics
tsullivan Nov 23, 2019
08dbaa0
remove console.log
tsullivan Nov 25, 2019
1fb9462
use generics
tsullivan Nov 25, 2019
1db2b7e
fix ts
tsullivan Nov 25, 2019
9ffff82
fix ts
tsullivan Nov 25, 2019
1d300b1
fix ts
tsullivan Nov 26, 2019
c173316
fix ts
tsullivan Nov 26, 2019
cd22792
Multi-type handling readability fix
tsullivan Nov 26, 2019
32bf3fd
Support createJob's jobParams
tsullivan Nov 26, 2019
7267351
i18n fixes
tsullivan Nov 26, 2019
86a8711
track down mysterious field
tsullivan Nov 26, 2019
e18cb07
revisit ts-ignores
tsullivan Nov 26, 2019
0107d9d
remove an any type in get_conditional_headers
tsullivan Nov 26, 2019
e4eaa02
ts fixes
tsullivan Nov 26, 2019
52b9741
typed export treatment for csv_from_savedobject#executeJob
tsullivan Nov 26, 2019
00870d6
refactor helper function plain bonkers signature
tsullivan Nov 27, 2019
f69b5b3
Merge branch 'master' into reporting/ts-for-export-type-registration
tsullivan Dec 2, 2019
55f7071
i18n merge fix
tsullivan Dec 2, 2019
df29171
add error handling test
tsullivan Dec 2, 2019
9ca9f79
todo
tsullivan Dec 2, 2019
81f54a8
fix .headers type def
tsullivan Dec 2, 2019
2bc9907
Reduce number of loc change
tsullivan Dec 2, 2019
6172995
Merge branch 'master' into reporting/ts-for-export-type-registration
tsullivan Dec 3, 2019
5ce0bb7
remove unused params from generic signatures
tsullivan Dec 3, 2019
4048408
Remove as/any
tsullivan Dec 3, 2019
ab9fe91
hoist out GenericWorkerFn for naming
tsullivan Dec 3, 2019
171f7ed
remove unnecessary fields from JobDocPayloadPanelCsv
tsullivan Dec 3, 2019
60fbee9
Introduce user defined type guard
tsullivan Dec 3, 2019
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 @@ -4,10 +4,10 @@
* 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';
import { Logger } from '../../../types';
import { decryptJobHeaders } from './decrypt_job_headers';

let mockServer: any;
beforeEach(() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

It looked weird to me that this isn't in a describe

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is an anti-pattern from what I recall.

Expand All @@ -24,17 +24,16 @@ describe('headers', () => {
await expect(
decryptJobHeaders({
job: {
title: 'cool-job-bro',
type: 'csv',
jobParams: {
savedObjectId: 'abc-123',
isImmediate: false,
savedObjectType: 'search',
},
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,
server: mockServer,
})
).rejects.toBeDefined();
).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]`
);
});

test(`passes back decrypted headers that were passed in`, async () => {
Expand All @@ -48,13 +47,9 @@ describe('headers', () => {
job: {
title: 'cool-job-bro',
type: 'csv',
jobParams: {
savedObjectId: 'abc-123',
isImmediate: false,
savedObjectType: 'search',
},
headers: encryptedHeaders,
},
logger: {} as Logger,
server: mockServer,
});
expect(decryptedHeaders).toEqual(headers);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,48 @@
* 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 { i18n } from '@kbn/i18n';
import { cryptoFactory } from '../../../server/lib/crypto';
import { CryptoFactory, JobDocPayload, ServerFacade } from '../../../types';
import { CryptoFactory, ServerFacade, Logger } from '../../../types';

interface HasEncryptedHeaders {
headers?: string;
}

export const decryptJobHeaders = async ({
// TODO merge functionality with CSV execute job
export const decryptJobHeaders = async <
JobParamsType,
JobDocPayloadType extends HasEncryptedHeaders
>({
job,
server,
logger,
}: {
job: JobDocPayload;
job: JobDocPayloadType;
server: ServerFacade;
}) => {
logger: Logger;
}): Promise<{
job: JobDocPayloadType;
server: ServerFacade;
decryptedHeaders: Record<string, string>;
}> => {
const crypto: CryptoFactory = cryptoFactory(server);
const decryptedHeaders: string = await crypto.decrypt(job.headers);
return { job, decryptedHeaders, server };
try {
const decryptedHeaders: Record<string, string> = await crypto.decrypt(job.headers);
Copy link
Contributor

Choose a reason for hiding this comment

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

I realise this isn't new in this code, but is this path on an api that's open to the world?
The reason I ask is that it could potentially be a DOS attack vector as cryptio.decrypt can be a very heavy operation on large inputs.
Perhaps worth limiting the length of the string as part of the validation?

Copy link
Member Author

Choose a reason for hiding this comment

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

The execute job functions are only called in the server background, when a reporting job is claimed. Then the job parameters, which include encrypted headers, get passed to the execute job function.

This is a great consideration though which hasn't come up before. I know we'd really like to move away from encrypting the header from the generation request and use some kind of token for authentication, but how/when we'll do that isn't yet clear.

return { job, decryptedHeaders, server };
} catch (err) {
logger.error(err);
tsullivan marked this conversation as resolved.
Show resolved Hide resolved

throw new Error(
i18n.translate(
'xpack.reporting.exportTypes.common.failedToDecryptReportJobDataErrorMessage',
{
defaultMessage:
'Failed to decrypt report job data. Please ensure that {encryptionKey} is set and re-generate this report. {err}',
values: { encryptionKey: 'xpack.reporting.encryptionKey', err: err.toString() },
}
)
);
}
};
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ describe('conditions', () => {
};

const { conditionalHeaders } = await getConditionalHeaders({
job: {} as JobDocPayload,
job: {} as JobDocPayload<any>,
filteredHeaders: permittedHeaders,
server: mockServer,
});
Expand All @@ -45,7 +45,7 @@ describe('conditions', () => {
};

const { conditionalHeaders } = await getConditionalHeaders({
job: {} as JobDocPayload,
job: {} as JobDocPayload<any>,
filteredHeaders: permittedHeaders,
server: mockServer,
});
Expand All @@ -66,7 +66,7 @@ describe('conditions', () => {
};

const { conditionalHeaders } = await getConditionalHeaders({
job: {} as JobDocPayload,
job: {} as JobDocPayload<any>,
filteredHeaders: permittedHeaders,
server: mockServer,
});
Expand All @@ -83,7 +83,7 @@ describe('conditions', () => {
};

const { conditionalHeaders } = await getConditionalHeaders({
job: {} as JobDocPayload,
job: {} as JobDocPayload<any>,
filteredHeaders: permittedHeaders,
server: mockServer,
});
Expand All @@ -98,7 +98,7 @@ describe('conditions', () => {
};

const { conditionalHeaders } = await getConditionalHeaders({
job: {} as JobDocPayload,
job: {} as JobDocPayload<any>,
filteredHeaders: permittedHeaders,
server: mockServer,
});
Expand All @@ -121,7 +121,7 @@ describe('conditions', () => {
};

const { conditionalHeaders } = await getConditionalHeaders({
job: {} as JobDocPayload,
job: {} as JobDocPayload<any>,
filteredHeaders: permittedHeaders,
server: mockServer,
});
Expand All @@ -138,7 +138,7 @@ describe('conditions', () => {
};

const { conditionalHeaders } = await getConditionalHeaders({
job: {} as JobDocPayload,
job: {} as JobDocPayload<any>,
filteredHeaders: permittedHeaders,
server: mockServer,
});
Expand All @@ -154,7 +154,7 @@ test('uses basePath from job when creating saved object service', async () => {
};

const { conditionalHeaders } = await getConditionalHeaders({
job: {} as JobDocPayload,
job: {} as JobDocPayload<any>,
filteredHeaders: permittedHeaders,
server: mockServer,
});
Expand All @@ -181,7 +181,7 @@ test(`uses basePath from server if job doesn't have a basePath when creating sav
};

const { conditionalHeaders } = await getConditionalHeaders({
job: {} as JobDocPayload,
job: {} as JobDocPayload<any>,
filteredHeaders: permittedHeaders,
server: mockServer,
});
Expand All @@ -204,7 +204,7 @@ describe('config formatting', () => {
test(`lowercases server.host`, async () => {
mockServer = createMockServer({ settings: { 'server.host': 'COOL-HOSTNAME' } });
const { conditionalHeaders } = await getConditionalHeaders({
job: {} as JobDocPayload,
job: {} as JobDocPayload<any>,
filteredHeaders: {},
server: mockServer,
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,14 @@
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
import { ConditionalHeaders, JobDocPayload, ServerFacade } from '../../../types';
import { ConditionalHeaders, ServerFacade } from '../../../types';

export const getConditionalHeaders = ({
export const getConditionalHeaders = <JobDocPayloadType>({
job,
filteredHeaders,
server,
}: {
job: JobDocPayload;
job: JobDocPayloadType;
filteredHeaders: Record<string, string>;
server: ServerFacade;
}) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,17 @@
*/

import { createMockServer } from '../../../test_helpers/create_mock_server';
import { ServerFacade } from '../../../types';
import { JobDocPayloadPNG } from '../../png/types';
import { JobDocPayloadPDF } from '../../printable_pdf/types';
import { getFullUrls } from './get_full_urls';

interface FullUrlsOpts {
job: JobDocPayloadPNG & JobDocPayloadPDF;
server: ServerFacade;
conditionalHeaders: any;
}

let mockServer: any;
beforeEach(() => {
mockServer = createMockServer('');
Expand All @@ -17,9 +24,9 @@ beforeEach(() => {
test(`fails if no URL is passed`, async () => {
await expect(
getFullUrls({
job: {} as JobDocPayloadPNG,
job: {},
server: mockServer,
})
} as FullUrlsOpts)
).rejects.toMatchInlineSnapshot(
`[Error: No valid URL fields found in Job Params! Expected \`job.relativeUrl\` or \`job.objects[{ relativeUrl }]\`]`
);
Expand All @@ -33,9 +40,9 @@ test(`fails if URLs are file-protocols for PNGs`, async () => {
job: {
relativeUrl,
forceNow,
} as JobDocPayloadPNG,
},
server: mockServer,
})
} as FullUrlsOpts)
).rejects.toMatchInlineSnapshot(
`[Error: Found invalid URL(s), all URLs must be relative: ${relativeUrl}]`
);
Expand All @@ -50,9 +57,9 @@ test(`fails if URLs are absolute for PNGs`, async () => {
job: {
relativeUrl,
forceNow,
} as JobDocPayloadPNG,
},
server: mockServer,
})
} as FullUrlsOpts)
).rejects.toMatchInlineSnapshot(
`[Error: Found invalid URL(s), all URLs must be relative: ${relativeUrl}]`
);
Expand All @@ -70,9 +77,9 @@ test(`fails if URLs are file-protocols for PDF`, async () => {
},
],
forceNow,
} as JobDocPayloadPDF,
},
server: mockServer,
})
} as FullUrlsOpts)
).rejects.toMatchInlineSnapshot(
`[Error: Found invalid URL(s), all URLs must be relative: ${relativeUrl}]`
);
Expand All @@ -91,9 +98,9 @@ test(`fails if URLs are absolute for PDF`, async () => {
},
],
forceNow,
} as JobDocPayloadPDF,
},
server: mockServer,
})
} as FullUrlsOpts)
).rejects.toMatchInlineSnapshot(
`[Error: Found invalid URL(s), all URLs must be relative: ${relativeUrl}]`
);
Expand All @@ -118,9 +125,9 @@ test(`fails if any URLs are absolute or file's for PDF`, async () => {
job: {
objects,
forceNow,
} as JobDocPayloadPDF,
},
server: mockServer,
})
} as FullUrlsOpts)
).rejects.toMatchInlineSnapshot(
`[Error: Found invalid URL(s), all URLs must be relative: ${objects[1].relativeUrl} ${objects[2].relativeUrl}]`
);
Expand All @@ -131,9 +138,9 @@ test(`fails if URL does not route to a visualization`, async () => {
getFullUrls({
job: {
relativeUrl: '/app/phoney',
} as JobDocPayloadPNG,
},
server: mockServer,
})
} as FullUrlsOpts)
).rejects.toMatchInlineSnapshot(
`[Error: No valid hash in the URL! A hash is expected for the application to route to the intended visualization.]`
);
Expand All @@ -145,9 +152,9 @@ test(`adds forceNow to hash's query, if it exists`, async () => {
job: {
relativeUrl: '/app/kibana#/something',
forceNow,
} as JobDocPayloadPNG,
},
server: mockServer,
});
} as FullUrlsOpts);

expect(urls[0]).toEqual(
'http://localhost:5601/sbp/app/kibana#/something?forceNow=2000-01-01T00%3A00%3A00.000Z'
Expand All @@ -161,9 +168,9 @@ test(`appends forceNow to hash's query, if it exists`, async () => {
job: {
relativeUrl: '/app/kibana#/something?_g=something',
forceNow,
} as JobDocPayloadPNG,
},
server: mockServer,
});
} as FullUrlsOpts);

expect(urls[0]).toEqual(
'http://localhost:5601/sbp/app/kibana#/something?_g=something&forceNow=2000-01-01T00%3A00%3A00.000Z'
Expand All @@ -174,9 +181,9 @@ test(`doesn't append forceNow query to url, if it doesn't exists`, async () => {
const { urls } = await getFullUrls({
job: {
relativeUrl: '/app/kibana#/something',
} as JobDocPayloadPNG,
},
server: mockServer,
});
} as FullUrlsOpts);

expect(urls[0]).toEqual('http://localhost:5601/sbp/app/kibana#/something');
});
Expand All @@ -192,9 +199,9 @@ test(`adds forceNow to each of multiple urls`, async () => {
{ relativeUrl: '/app/kibana#/something_ddd' },
],
forceNow,
} as JobDocPayloadPDF,
},
server: mockServer,
});
} as FullUrlsOpts);

expect(urls).toEqual([
'http://localhost:5601/sbp/app/kibana#/something_aaa?forceNow=2000-01-01T00%3A00%3A00.000Z',
Expand Down
Loading