Skip to content
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 @@ -4,6 +4,7 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { set } from '@elastic/safer-lodash-set';
import {
SignalSourceHit,
SignalSearchResponse,
Expand Down Expand Up @@ -189,9 +190,25 @@ export const sampleDocNoSortId = (
sort: [],
});

export const sampleDocSeverity = (
severity?: Array<string | number | null> | string | number | null
): SignalSourceHit => ({
export const sampleDocSeverity = (severity?: unknown, fieldName?: string): SignalSourceHit => {
const doc = {
_index: 'myFakeSignalIndex',
_type: 'doc',
_score: 100,
_version: 1,
_id: sampleIdGuid,
_source: {
someKey: 'someValue',
'@timestamp': '2020-04-20T21:27:45+0000',
},
sort: [],
};

set(doc._source, fieldName ?? 'event.severity', severity);
return doc;
};

export const sampleDocRiskScore = (riskScore?: unknown): SignalSourceHit => ({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: we're starting to add a lot of indirection to these tests, and I question the utility of such a method over something like:

const testEventWithDescriptiveName = {
  ...buildSpecificEventType(),
  [fieldName]: overriddenValue,
};

For context: my perception has been that we typically prefer repetitive, inline test logic as opposed to shared helpers, with the exceptions of (normalized) data generation and ad hoc test helpers within the test file.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Discussed with @rylnd. We talked about creating test data factories and all that patterns in general, agreed on a few ideas, but no action required for this PR.

_index: 'myFakeSignalIndex',
_type: 'doc',
_score: 100,
Expand All @@ -201,7 +218,7 @@ export const sampleDocSeverity = (
someKey: 'someValue',
'@timestamp': '2020-04-20T21:27:45+0000',
event: {
severity: severity ?? 100,
risk: riskScore,
},
},
sort: [],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,23 +4,218 @@
* you may not use this file except in compliance with the Elastic License.
*/

import { sampleDocNoSortId } from '../__mocks__/es_results';
import { buildRiskScoreFromMapping } from './build_risk_score_from_mapping';
import {
RiskScore,
RiskScoreMappingOrUndefined,
} from '../../../../../common/detection_engine/schemas/common/schemas';
import { sampleDocRiskScore } from '../__mocks__/es_results';
import {
buildRiskScoreFromMapping,
BuildRiskScoreFromMappingReturn,
} from './build_risk_score_from_mapping';

describe('buildRiskScoreFromMapping', () => {
beforeEach(() => {
jest.clearAllMocks();
});

test('risk score defaults to provided if mapping is incomplete', () => {
const riskScore = buildRiskScoreFromMapping({
eventSource: sampleDocNoSortId()._source,
riskScore: 57,
riskScoreMapping: undefined,
describe('base cases: when mapping is undefined', () => {
test('returns the provided default score', () => {
testIt({
fieldValue: 42,
scoreDefault: 57,
scoreMapping: undefined,
expected: scoreOf(57),
});
});
});

describe('base cases: when mapping to a field of type number', () => {
test(`returns that number if it's integer and within the range [0;100]`, () => {
testIt({
fieldValue: 42,
scoreDefault: 57,
scoreMapping: mappingToSingleField(),
expected: overriddenScoreOf(42),
});
});

test(`returns that number if it's float and within the range [0;100]`, () => {
testIt({
fieldValue: 3.14,
scoreDefault: 57,
scoreMapping: mappingToSingleField(),
expected: overriddenScoreOf(3.14),
});
});

test(`returns default score if the number is < 0`, () => {
testIt({
fieldValue: -0.0000000000001,
scoreDefault: 57,
scoreMapping: mappingToSingleField(),
expected: scoreOf(57),
});
});

test(`returns default score if the number is > 100`, () => {
testIt({
fieldValue: 100.0000000000001,
scoreDefault: 57,
scoreMapping: mappingToSingleField(),
expected: scoreOf(57),
});
});
});

describe('base cases: when mapping to a field of type string', () => {
test(`returns the number casted from string if it's integer and within the range [0;100]`, () => {
testIt({
fieldValue: '42',
scoreDefault: 57,
scoreMapping: mappingToSingleField(),
expected: overriddenScoreOf(42),
});
});

test(`returns the number casted from string if it's float and within the range [0;100]`, () => {
testIt({
fieldValue: '3.14',
scoreDefault: 57,
scoreMapping: mappingToSingleField(),
expected: overriddenScoreOf(3.14),
});
});

test(`returns default score if the "number" is < 0`, () => {
testIt({
fieldValue: '-1',
scoreDefault: 57,
scoreMapping: mappingToSingleField(),
expected: scoreOf(57),
});
});

test(`returns default score if the "number" is > 100`, () => {
testIt({
fieldValue: '101',
scoreDefault: 57,
scoreMapping: mappingToSingleField(),
expected: scoreOf(57),
});
});
});

expect(riskScore).toEqual({ riskScore: 57, riskScoreMeta: {} });
describe('base cases: when mapping to an array of numbers or strings', () => {
test(`returns that number if it's a single element and it's within the range [0;100]`, () => {
testIt({
fieldValue: [3.14],
scoreDefault: 57,
scoreMapping: mappingToSingleField(),
expected: overriddenScoreOf(3.14),
});
});

test(`returns the max number of those that are within the range [0;100]`, () => {
testIt({
fieldValue: [42, -42, 17, 87, 87.5, '86.5', 110, 66],
scoreDefault: 57,
scoreMapping: mappingToSingleField(),
expected: overriddenScoreOf(87.5),
});
});

test(`supports casting strings to numbers`, () => {
testIt({
fieldValue: [-1, 1, '3', '1.5', '3.14', 2],
scoreDefault: 57,
scoreMapping: mappingToSingleField(),
expected: overriddenScoreOf(3.14),
});
});
});

// TODO: Enhance...
describe('edge cases: when mapping to a single junk value', () => {
describe('ignores it and returns the default score', () => {
const cases = [
undefined,
null,
NaN,
Infinity,
-Infinity,
Number.MAX_VALUE,
-Number.MAX_VALUE,
-Number.MIN_VALUE,
'string',
[],
{},
new Date(),
];

test.each(cases)('%p', (value) => {
testIt({
fieldValue: value,
scoreDefault: 57,
scoreMapping: mappingToSingleField(),
expected: scoreOf(57),
});
});
});
});

describe('edge cases: when mapping to an array of junk values', () => {
describe('ignores junk, extracts valid numbers and returns the max number within the range [0;100]', () => {
type Case = [unknown[], number];
const cases: Case[] = [
[[undefined, null, 1.5, 1, -Infinity], 1.5],
[['42', NaN, '44', '43', 42, {}], 44],
[[Infinity, '101', 100, 99, Number.MIN_VALUE], 100],
[[Number.MIN_VALUE, -0], Number.MIN_VALUE],
];

test.each(cases)('%p', (value, expectedScore) => {
testIt({
fieldValue: value,
scoreDefault: 57,
scoreMapping: mappingToSingleField(),
expected: overriddenScoreOf(expectedScore),
});
});
});
});
});

interface TestCase {
fieldValue: unknown;
scoreDefault: RiskScore;
scoreMapping: RiskScoreMappingOrUndefined;
expected: BuildRiskScoreFromMappingReturn;
}

function testIt({ fieldValue, scoreDefault, scoreMapping, expected }: TestCase) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: we tend to prefer arrow functions

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I would personally do away with the testIt function as I find it makes the tests a bit too convoluted, but that's totally subjective.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

On functions, arrows vs fn expressions vs fn declarations. Normally, unless there's a different coding standard/guideline, to vertically structure the code I prefer using the "newspaper metaphor" coined by Uncle Bob in his Clean Code book.

Newspaper Metaphor. The source file should be organized like a newspaper article, with the highest level summary at the top, and more and more details further down. Functions called from the top function come directly below it, and so on down to the lowest level and most detailed functions at the bottom. This is a good way to organize the source code, even though IDE:s make the location of functions less important, since it is so easy to navigate in and out of them.

It's a top-down approach, from general things to details, and it's possible with function declarations. With arrows and fn expressions it's possible to use the opposite bottow-up approach, where you first have details, helper functions etc, and at the bottom of the file there's let's say the main function.

Normally I use arrows in all other cases: inline calls, closures to an outer this, just a bunch of small independent functions in a file (e.g. selectors), etc.

Of course I wouldn't go against our guidelines, whether they are documented or not, and against the typical style of the codebase. For the sake of consistency.

Hopefully I managed to explain my motivation. Let me know what you think. Although the styleguide uses fn declarations, it's a general one. If this style looks too foreign for the security_solution plugin's codebase, I'll change it to the one based on arrows.

Copy link
Copy Markdown
Contributor Author

@banderror banderror Nov 24, 2020

Choose a reason for hiding this comment

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

On the testIt helper. Yea, it definitely has pros and cons. It reduces duplication and makes the test suite shorter, but abstracts away some details. One can perceive it as either good or bad thing in terms of readability and easiness to understand the tests. I personally find that as soon as you got what testIt does, it's easier and faster to read all the tests. On the other hand, it can be just a bit harder to start reading the tests once you opened the file.

If you don't have tests written in such a way and/or don't like it (yes, even subjectively), I'll refactor. But maybe there are people who are ok with that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Any opinions, clarifications, comments or just votes are super-welcome. This helps me a lot with learning the codebase faster.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

"Newspaper metaphor" with fn declarations?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

testIt helper?

const result = buildRiskScoreFromMapping({
eventSource: sampleDocRiskScore(fieldValue)._source,
riskScore: scoreDefault,
riskScoreMapping: scoreMapping,
});

expect(result).toEqual(expected);
}

function mappingToSingleField() {
return [{ field: 'event.risk', operator: 'equals' as const, value: '', risk_score: undefined }];
}

function scoreOf(value: number) {
return {
riskScore: value,
riskScoreMeta: {},
};
}

function overriddenScoreOf(value: number) {
return {
riskScore: value,
riskScoreMeta: { riskScoreOverridden: true },
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,35 +11,78 @@ import {
} from '../../../../../common/detection_engine/schemas/common/schemas';
import { SignalSource } from '../types';

interface BuildRiskScoreFromMappingProps {
export interface BuildRiskScoreFromMappingProps {
eventSource: SignalSource;
riskScore: RiskScore;
riskScoreMapping: RiskScoreMappingOrUndefined;
}

interface BuildRiskScoreFromMappingReturn {
export interface BuildRiskScoreFromMappingReturn {
riskScore: RiskScore;
riskScoreMeta: Meta; // TODO: Stricter types
}

/**
* Calculates the final risk score for a detection alert based on:
* - source event object that can potentially contain fields representing risk score
* - the default score specified by the user
* - (optional) score mapping specified by the user ("map this field to the score")
*
* NOTE: Current MVP support is for mapping from a single field.
*/
export const buildRiskScoreFromMapping = ({
eventSource,
riskScore,
riskScoreMapping,
}: BuildRiskScoreFromMappingProps): BuildRiskScoreFromMappingReturn => {
// MVP support is for mapping from a single field
if (riskScoreMapping != null && riskScoreMapping.length > 0) {
const mappedField = riskScoreMapping[0].field;
// TODO: Expand by verifying fieldType from index via doc._index
const mappedValue = get(mappedField, eventSource);
if (
typeof mappedValue === 'number' &&
Number.isSafeInteger(mappedValue) &&
mappedValue >= 0 &&
mappedValue <= 100
) {
return { riskScore: mappedValue, riskScoreMeta: { riskScoreOverridden: true } };
if (!riskScoreMapping || !riskScoreMapping.length) {
return defaultScore(riskScore);
}

// TODO: Expand by verifying fieldType from index via doc._index
const eventField = riskScoreMapping[0].field;
const eventValue = get(eventField, eventSource);
const eventValues = Array.isArray(eventValue) ? eventValue : [eventValue];

const validNumbers = eventValues.map(toValidNumberOrMinusOne).filter((n) => n > -1);

if (validNumbers.length > 0) {
const maxNumber = getMaxOf(validNumbers);
return overriddenScore(maxNumber);
}

return defaultScore(riskScore);
};

function toValidNumberOrMinusOne(value: unknown): number {
if (typeof value === 'number' && isValidNumber(value)) {
return value;
}

if (typeof value === 'string') {
const num = Number(value);
if (isValidNumber(num)) {
return num;
}
}

return -1;
}

function isValidNumber(value: number): boolean {
return Number.isFinite(value) && value >= 0 && value <= 100;
}

function getMaxOf(array: number[]) {
// NOTE: It's safer to use reduce rather than Math.max(...array). The latter won't handle large input.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Huh, TIL!

// https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Math/max
return array.reduce((a, b) => Math.max(a, b));
}

function defaultScore(riskScore: RiskScore): BuildRiskScoreFromMappingReturn {
return { riskScore, riskScoreMeta: {} };
};
}

function overriddenScore(riskScore: RiskScore): BuildRiskScoreFromMappingReturn {
return { riskScore, riskScoreMeta: { riskScoreOverridden: true } };
}
Loading