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.

Sign up for GitHub

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

[Security Solution][Exceptions] Handle promise rejections when building artifacts #73831

Merged
merged 3 commits into from
Jul 30, 2020
Merged

[Security Solution][Exceptions] Handle promise rejections when building artifacts #73831

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
61d243a
Handle promise rejections when building artifacts
madirey Jul 30, 2020
957f344
Merge branch 'master' of github.com:elastic/kibana into unhandled-pro…
madirey Jul 30, 2020
bdcfdc7
Merge branch 'master' into unhandled-promise-rejection
elasticmachine Jul 30, 2020
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
21 changes: 17 additions & 4 deletions ...ity_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.mock.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { savedObjectsClientMock, loggingSystemMock } from 'src/core/server/mocks
import { Logger } from 'src/core/server';
import { PackageConfigServiceInterface } from '../../../../../../ingest_manager/server';
import { createPackageConfigServiceMock } from '../../../../../../ingest_manager/server/mocks';
import { ExceptionListClient } from '../../../../../../lists/server';
import { listMock } from '../../../../../../lists/server/mocks';
import LRU from 'lru-cache';
import { getArtifactClientMock } from '../artifact_client.mock';
Expand All @@ -23,22 +24,29 @@ import {

export enum ManifestManagerMockType {
InitialSystemState,
ListClientPromiseRejection,
NormalFlow,
}

export const getManifestManagerMock = (opts?: {
mockType?: ManifestManagerMockType;
cache?: LRU<string, Buffer>;
exceptionListClient?: ExceptionListClient;
packageConfigService?: jest.Mocked<PackageConfigServiceInterface>;
savedObjectsClient?: ReturnType<typeof savedObjectsClientMock.create>;
}): ManifestManager => {
let cache = new LRU<string, Buffer>({ max: 10, maxAge: 1000 * 60 * 60 });
if (opts?.cache !== undefined) {
if (opts?.cache != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ think you want !== instead of !=

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bkimmel Maybe I misunderstood, but I thought the prevailing recommendation was that != null is preferable to !== undefined because it catches both null and undefined...

Copy link
Contributor

Choose a reason for hiding this comment

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

It does, but coercively. == and it's != cousin hurt to read. That fancy new nullish coalescer ?? could come in handy, since it's made to handle undefined/null like if(opts?.cache ?? false) maybe,

cache = opts.cache;
}

let exceptionListClient = listMock.getExceptionListClient();
if (opts?.exceptionListClient != null) {
exceptionListClient = opts.exceptionListClient;
}

let packageConfigService = createPackageConfigServiceMock();
if (opts?.packageConfigService !== undefined) {
if (opts?.packageConfigService != null) {
packageConfigService = opts.packageConfigService;
}
packageConfigService.list = jest.fn().mockResolvedValue({
Expand All @@ -51,7 +59,7 @@ export const getManifestManagerMock = (opts?: {
});

let savedObjectsClient = savedObjectsClientMock.create();
if (opts?.savedObjectsClient !== undefined) {
if (opts?.savedObjectsClient != null) {
savedObjectsClient = opts.savedObjectsClient;
}

Expand All @@ -61,6 +69,11 @@ export const getManifestManagerMock = (opts?: {
switch (mockType) {
case ManifestManagerMockType.InitialSystemState:
return getEmptyMockArtifacts();
case ManifestManagerMockType.ListClientPromiseRejection:
exceptionListClient.findExceptionListItem = jest
.fn()
.mockRejectedValue(new Error('unexpected thing happened'));
return super.buildExceptionListArtifacts('v1');
case ManifestManagerMockType.NormalFlow:
return getMockArtifactsWithDiff();
}
Expand All @@ -85,7 +98,7 @@ export const getManifestManagerMock = (opts?: {
artifactClient: getArtifactClientMock(savedObjectsClient),
cache,
packageConfigService,
exceptionListClient: listMock.getExceptionListClient(),
exceptionListClient,
logger: loggingSystemMock.create().get() as jest.Mocked<Logger>,
savedObjectsClient,
});
Expand Down
11 changes: 10 additions & 1 deletion ...ity_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { savedObjectsClientMock } from 'src/core/server/mocks';
import { createPackageConfigServiceMock } from '../../../../../../ingest_manager/server/mocks';
import { ArtifactConstants, ManifestConstants, isCompleteArtifact } from '../../../lib/artifacts';

import { getManifestManagerMock } from './manifest_manager.mock';
import { getManifestManagerMock, ManifestManagerMockType } from './manifest_manager.mock';
import LRU from 'lru-cache';

describe('manifest_manager', () => {
Expand Down Expand Up @@ -204,5 +204,14 @@ describe('manifest_manager', () => {
oldArtifactId
);
});

test('ManifestManager handles promise rejections when building artifacts', async () => {
// This test won't fail on an unhandled promise rejection, but it will cause
// an UnhandledPromiseRejectionWarning to be printed.
const manifestManager = getManifestManagerMock({
mockType: ManifestManagerMockType.ListClientPromiseRejection,
});
await expect(manifestManager.buildNewManifest()).rejects.toThrow();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this test passes without the changes manifest_manager.ts https://github.com/elastic/kibana/pull/73831/files#diff-21a709e8a87929d93a7efdb95905d024R85-R95

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We zoomed. It passes either way, but an UnhandledPromiseRejectionWarning is printed when you run it against the pre-change code, and it's absent now. Doesn't appear to be a way to force jest to behave differently...

Copy link
Contributor

Choose a reason for hiding this comment

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

@madirey explained to me, as posted in the comment, the test will pass without those changes, but the changes prevent the rejection warning from appearing when running the tests. The rejection does get handled further down the line when building the manifest.

});
});
11 changes: 5 additions & 6 deletions ...security_solution/server/endpoint/services/artifacts/manifest_manager/manifest_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,18 +82,17 @@ export class ManifestManager {
protected async buildExceptionListArtifacts(
artifactSchemaVersion?: string
): Promise<InternalArtifactCompleteSchema[]> {
return ArtifactConstants.SUPPORTED_OPERATING_SYSTEMS.reduce<
Promise<InternalArtifactCompleteSchema[]>
>(async (acc, os) => {
const artifacts: InternalArtifactCompleteSchema[] = [];
for (const os of ArtifactConstants.SUPPORTED_OPERATING_SYSTEMS) {
const exceptionList = await getFullEndpointExceptionList(
this.exceptionListClient,
os,
artifactSchemaVersion ?? 'v1'
);
const artifacts = await acc;
const artifact = await buildArtifact(exceptionList, os, artifactSchemaVersion ?? 'v1');
return Promise.resolve([...artifacts, artifact]);
}, Promise.resolve([]));
artifacts.push(artifact);
}
return artifacts;
}

/**
Expand Down