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

[Detection Engine][Rules] - Adds custom highlighted fields option #163235

Merged
merged 42 commits into from
Aug 16, 2023
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
42 commits
Select commit Hold shift + click to select a range
eb33083
trying to recreate changes for new custom highlighted fields
yctercero Aug 3, 2023
0b98a19
Merge branch 'main' of github.com:elastic/kibana into custom_highligh…
yctercero Aug 3, 2023
110eb99
got most of the code ported over
yctercero Aug 6, 2023
5a249b9
Merge branch 'main' of github.com:elastic/kibana into custom_highligh…
yctercero Aug 6, 2023
ccf1c24
adding back in tests and cleanup
yctercero Aug 6, 2023
cbbbbf9
continued cleanup
yctercero Aug 7, 2023
6f5ac58
cleanup and making by default additive
yctercero Aug 9, 2023
f416282
cleaning up types
yctercero Aug 10, 2023
a42910e
Merge branch 'main' of github.com:elastic/kibana into custom_highligh…
yctercero Aug 10, 2023
888c042
updating so now everywhere pulls latest custom highlighted fields
yctercero Aug 11, 2023
b8c3923
dont need to map it in alert since always accessing latest
yctercero Aug 11, 2023
b272000
continued cleanup
yctercero Aug 11, 2023
7b97501
continued cleanup
yctercero Aug 12, 2023
eda33ef
continued cleanup
yctercero Aug 12, 2023
5920bba
trying to fix tests
yctercero Aug 12, 2023
317873d
trying to fix tests
yctercero Aug 13, 2023
0363fb8
Merge branch 'main' of github.com:elastic/kibana into custom_highligh…
yctercero Aug 13, 2023
e0b6a90
trying to fix tests
yctercero Aug 13, 2023
bfda622
trying to fix tests
yctercero Aug 14, 2023
44aa7a5
Merge branch 'main' of github.com:elastic/kibana into custom_highligh…
yctercero Aug 14, 2023
013c437
cleanup, cleanup, everybody cleanup
yctercero Aug 14, 2023
7376b1b
addressing PR feedback
yctercero Aug 14, 2023
07d0e41
Merge branch 'main' of github.com:elastic/kibana into custom_highligh…
yctercero Aug 14, 2023
2fd6f51
updated jest test and linting issue
yctercero Aug 14, 2023
a7379e5
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Aug 14, 2023
df9d604
working on cypress tests and renaming field to investigation_fields
yctercero Aug 15, 2023
3f67bc5
Merge branch 'main' of github.com:elastic/kibana into custom_highligh…
yctercero Aug 15, 2023
ba11dba
Merge branch 'custom_highlighted_fields' of github.com:yctercero/kiba…
yctercero Aug 15, 2023
97d5adf
[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…
kibanamachine Aug 15, 2023
2188d0d
[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…
kibanamachine Aug 15, 2023
e0f8fc9
adding to older event details flyout after refactor
yctercero Aug 15, 2023
a416d1a
Merge branch 'custom_highlighted_fields' of github.com:yctercero/kiba…
yctercero Aug 15, 2023
5a225bf
Merge branch 'main' of github.com:elastic/kibana into custom_highligh…
yctercero Aug 15, 2023
49bdfc0
skipping flakey tests, linked to issue as required follow up for release
yctercero Aug 15, 2023
2d01586
some cleanup
yctercero Aug 15, 2023
294387d
Merge branch 'main' of github.com:elastic/kibana into custom_highligh…
yctercero Aug 15, 2023
9d6c1e3
Merge branch 'main' into custom_highlighted_fields
kibanamachine Aug 15, 2023
f62deca
Merge branch 'main' of github.com:elastic/kibana into custom_highligh…
yctercero Aug 16, 2023
e3b8078
trying to get ci to pass
yctercero Aug 16, 2023
983053e
Merge branch 'custom_highlighted_fields' of github.com:yctercero/kiba…
yctercero Aug 16, 2023
9cc5c9f
Merge branch 'main' into custom_highlighted_fields
kibanamachine Aug 16, 2023
d547c5e
Merge branch 'main' into custom_highlighted_fields
MadameSheema Aug 16, 2023
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 @@ -20,6 +20,7 @@ const getRulesSchemaMock = (anchorDate: string = ANCHOR_DATE) => ({
enabled: true,
false_positives: ['false positive 1', 'false positive 2'],
from: 'now-6m',
custom_highlighted_fields: ['custom.field1', 'custom.field2'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: not sure why we test the transformDataToNdjson function against a mock rule, considering the function is quite generic:

export const transformDataToNdjson = (data: unknown[]): string => {
  if (data.length !== 0) {
    const dataString = data.map((item) => JSON.stringify(item)).join('\n');
    return `${dataString}\n`;
  } else {
    return '';
  }
};

I think we should test it against generic objects 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Not going to touch in this PR though.

immutable: false,
name: 'Query with a rule id',
query: 'user.name: root or user.name: admin',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ export const RuleAuthorArray = t.array(t.string); // should be non-empty strings
export type RuleFalsePositiveArray = t.TypeOf<typeof RuleFalsePositiveArray>;
export const RuleFalsePositiveArray = t.array(t.string); // should be non-empty strings?

export type RuleCustomHighlightedFieldArray = t.TypeOf<typeof RuleCustomHighlightedFieldArray>;
export const RuleCustomHighlightedFieldArray = t.array(NonEmptyString);
Comment on lines +63 to +64
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we please add a JSDoc comment with an explanation what these fields mean and a link to the PR?


export type RuleReferenceArray = t.TypeOf<typeof RuleReferenceArray>;
export const RuleReferenceArray = t.array(t.string); // should be non-empty strings?

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1289,6 +1289,36 @@ describe('rules schema', () => {
expect(message.schema).toEqual({});
expect(getPaths(left(message.errors))).toEqual(['invalid keys "data_view_id"']);
});

test('You can optionally send in an array of custom_highlighted_fields', () => {
const payload: RuleCreateProps = {
...getCreateRulesSchemaMock(),
custom_highlighted_fields: ['field1', 'field2'],
};

const decoded = RuleCreateProps.decode(payload);
const checked = exactCheck(payload, decoded);
const message = pipe(checked, foldLeftRight);
expect(getPaths(left(message.errors))).toEqual([]);
expect(message.schema).toEqual(payload);
});

test('You cannot send in an array of custom_highlighted_fields that are numbers', () => {
const payload = {
...getCreateRulesSchemaMock(),
custom_highlighted_fields: [0, 1, 2],
};

const decoded = RuleCreateProps.decode(payload);
const checked = exactCheck(payload, decoded);
const message = pipe(checked, foldLeftRight);
expect(getPaths(left(message.errors))).toEqual([
'Invalid value "0" supplied to "custom_highlighted_fields"',
'Invalid value "1" supplied to "custom_highlighted_fields"',
'Invalid value "2" supplied to "custom_highlighted_fields"',
]);
expect(message.schema).toEqual({});
});
});

describe('response', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ const getResponseBaseParams = (anchorDate: string = ANCHOR_DATE): SharedResponse
timestamp_override: undefined,
timestamp_override_fallback_disabled: undefined,
namespace: undefined,
custom_highlighted_fields: undefined,
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 it shouldn't be possible to set it to undefined in the response (RuleResponse and other rule type-specific types). It should always be an array (empty or non-empty), which would improve DX for API users (including us on the FE side).

});

export const getRulesSchemaMock = (anchorDate: string = ANCHOR_DATE): QueryRule => ({
Expand All @@ -77,6 +78,7 @@ export const getRulesSchemaMock = (anchorDate: string = ANCHOR_DATE): QueryRule
saved_id: undefined,
response_actions: undefined,
alert_suppression: undefined,
custom_highlighted_fields: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

});

export const getSavedQuerySchemaMock = (anchorDate: string = ANCHOR_DATE): SavedQueryRule => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -232,4 +232,65 @@ describe('Rule response schema', () => {
expect(message.schema).toEqual({});
});
});

describe('custom_highlighted_fields', () => {
test('it should validate rule with empty array for "custom_highlighted_fields"', () => {
const payload = getRulesSchemaMock();
payload.custom_highlighted_fields = [];

const decoded = RuleResponse.decode(payload);
const checked = exactCheck(payload, decoded);
const message = pipe(checked, foldLeftRight);
const expected = { ...getRulesSchemaMock(), custom_highlighted_fields: [] };

expect(getPaths(left(message.errors))).toEqual([]);
expect(message.schema).toEqual(expected);
});

test('it should validate rule with "custom_highlighted_fields"', () => {
const payload = getRulesSchemaMock();
payload.custom_highlighted_fields = ['foo', 'bar'];

const decoded = RuleResponse.decode(payload);
const checked = exactCheck(payload, decoded);
const message = pipe(checked, foldLeftRight);
const expected = { ...getRulesSchemaMock(), custom_highlighted_fields: ['foo', 'bar'] };

expect(getPaths(left(message.errors))).toEqual([]);
expect(message.schema).toEqual(expected);
});

test('it should validate undefined for "custom_highlighted_fields"', () => {
const payload: RuleResponse = {
...getRulesSchemaMock(),
custom_highlighted_fields: undefined,
};

const decoded = RuleResponse.decode(payload);
const checked = exactCheck(payload, decoded);
const message = pipe(checked, foldLeftRight);
const expected = { ...getRulesSchemaMock(), custom_highlighted_fields: undefined };

expect(getPaths(left(message.errors))).toEqual([]);
expect(message.schema).toEqual(expected);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here, validation shouldn't pass


test('it should NOT validate a string for "custom_highlighted_fields"', () => {
const payload: Omit<RuleResponse, 'custom_highlighted_fields'> & {
custom_highlighted_fields: string;
} = {
...getRulesSchemaMock(),
custom_highlighted_fields: 'foo',
};

const decoded = RuleResponse.decode(payload);
const checked = exactCheck(payload, decoded);
const message = pipe(checked, foldLeftRight);

expect(getPaths(left(message.errors))).toEqual([
'Invalid value "foo" supplied to "custom_highlighted_fields"',
]);
expect(message.schema).toEqual({});
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import {
RelatedIntegrationArray,
RequiredFieldArray,
RuleAuthorArray,
RuleCustomHighlightedFieldArray,
RuleDescription,
RuleFalsePositiveArray,
RuleFilterArray,
Expand Down Expand Up @@ -116,6 +117,7 @@ export const baseSchema = buildRuleSchemas({
output_index: AlertsIndex,
namespace: AlertsIndexNamespace,
meta: RuleMetadata,
custom_highlighted_fields: RuleCustomHighlightedFieldArray,
Copy link
Contributor

Choose a reason for hiding this comment

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

Posting for visibility here. As discussed over zoom with @yctercero, I think this field should be:

  • Defaultable instead of optional, meaning it should always be defined in RuleResponse and other types of rules we return from the API (QueryRule, ThresholdRule, etc).
  • Wrapped in an object to make space for other parameters we might want to add for this feature in the future. This would also make it more compatible with the DiffableRule interface and the rule diff/upgrade algorithm.
  • Renamed to something more UI-neutral and generic, e.g. "investigation fields".

So in a rule, it would be something like rule.investigation_fields.field_names.

We should consider writing a migration to add a "null" investigation_fields object to existing rules. Pseudocode:

rule.investigation_fields = {
  field_names: [],
};

// Throttle
throttle: RuleActionThrottle,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ describe('Auto populate exception with Alert data', () => {
cy.task('esArchiverResetKibana');
cy.task('esArchiverLoad', 'endpoint');
login();
createRule(getEndpointRule());
createRule({ ...getEndpointRule(), custom_highlighted_fields: ['message'] });
visitWithoutDateRange(DETECTIONS_RULE_MANAGEMENT_URL);
goToRuleDetails();
waitForAlertsToPopulate();
Expand All @@ -67,6 +67,7 @@ describe('Auto populate exception with Alert data', () => {
addExceptionFromFirstAlert();

const highlightedFieldsBasedOnAlertDoc = [
'message',
'host.name',
'agent.id',
'user.name',
Expand Down
2 changes: 2 additions & 0 deletions x-pack/plugins/security_solution/cypress/objects/rule.ts
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,7 @@ export const expectedExportedRule = (ruleResponse: Cypress.Response<RuleResponse
immutable,
related_integrations: relatedIntegrations,
setup,
custom_highlighted_fields: customHighlightedFields,
} = ruleResponse.body;

let query: string | undefined;
Expand All @@ -558,6 +559,7 @@ export const expectedExportedRule = (ruleResponse: Cypress.Response<RuleResponse
severity,
note,
output_index: '',
custom_highlighted_fields: customHighlightedFields,
author,
false_positives: falsePositives,
from,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,15 @@ function getFieldsByRuleType(ruleType?: string): EventSummaryField[] {
}
}

/**
* Gets the fields to display based on custom rules and configuration
* @param customs The list of custom-defined fields to display
* @returns The list of custom-defined fields to display
*/
function getHighlightedFieldsOverride(customs: string[]): EventSummaryField[] {
return customs.map((field) => ({ id: field }));
}

/**
This function is exported because it is used in the Exception Component to
populate the conditions with the Highlighted Fields. Additionally, the new
Expand All @@ -229,12 +238,15 @@ export function getEventFieldsToDisplay({
eventCategories,
eventCode,
eventRuleType,
highlightedFieldsOverride,
}: {
eventCategories: EventCategories;
eventCode?: string;
eventRuleType?: string;
highlightedFieldsOverride: string[];
}): EventSummaryField[] {
const fields = [
...getHighlightedFieldsOverride(highlightedFieldsOverride),
...alwaysDisplayedFields,
...getFieldsByCategory(eventCategories),
...getFieldsByEventCode(eventCode, eventCategories),
Expand Down Expand Up @@ -281,11 +293,13 @@ export const getSummaryRows = ({
eventId,
isDraggable = false,
isReadOnly = false,
customHighlightedFields,
}: {
data: TimelineEventsDetailsItem[];
browserFields: BrowserFields;
scopeId: string;
eventId: string;
customHighlightedFields?: string[];
isDraggable?: boolean;
isReadOnly?: boolean;
}) => {
Expand All @@ -306,6 +320,7 @@ export const getSummaryRows = ({
eventCategories,
eventCode,
eventRuleType,
highlightedFieldsOverride: customHighlightedFields ?? [],
});

return data != null
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,7 @@ describe('helpers', () => {
severity_mapping: [],
tags: ['tag1', 'tag2'],
threat: getThreatMock(),
custom_highlighted_fields: ['foo', 'bar'],
};

expect(result).toEqual(expected);
Expand Down Expand Up @@ -635,6 +636,7 @@ describe('helpers', () => {
severity_mapping: [],
tags: ['tag1', 'tag2'],
threat: getThreatMock(),
custom_highlighted_fields: ['foo', 'bar'],
};

expect(result).toEqual(expected);
Expand All @@ -659,6 +661,7 @@ describe('helpers', () => {
severity_mapping: [],
tags: ['tag1', 'tag2'],
threat: getThreatMock(),
custom_highlighted_fields: ['foo', 'bar'],
};

expect(result).toEqual(expected);
Expand Down Expand Up @@ -702,6 +705,7 @@ describe('helpers', () => {
severity_mapping: [],
tags: ['tag1', 'tag2'],
threat: getThreatMock(),
custom_highlighted_fields: ['foo', 'bar'],
};

expect(result).toEqual(expected);
Expand Down Expand Up @@ -754,6 +758,7 @@ describe('helpers', () => {
],
},
],
custom_highlighted_fields: ['foo', 'bar'],
};

expect(result).toEqual(expected);
Expand Down Expand Up @@ -782,6 +787,95 @@ describe('helpers', () => {
threat: getThreatMock(),
timestamp_override: 'event.ingest',
timestamp_override_fallback_disabled: true,
custom_highlighted_fields: ['foo', 'bar'],
};

expect(result).toEqual(expected);
});

test('returns formatted object if custom_highlighted_fields is empty array', () => {
const mockStepData: AboutStepRule = {
...mockData,
customHighlightedFields: [],
};
const result = formatAboutStepData(mockStepData);
const expected: AboutStepRuleJson = {
author: ['Elastic'],
description: '24/7',
false_positives: ['test'],
license: 'Elastic License',
name: 'Query with rule-id',
note: '# this is some markdown documentation',
references: ['www.test.co'],
risk_score: 21,
risk_score_mapping: [],
severity: 'low',
severity_mapping: [],
tags: ['tag1', 'tag2'],
rule_name_override: undefined,
threat_indicator_path: undefined,
timestamp_override: undefined,
timestamp_override_fallback_disabled: undefined,
threat: getThreatMock(),
custom_highlighted_fields: [],
};

expect(result).toEqual(expected);
});

test('returns formatted object with custom_highlighted_fields', () => {
const mockStepData: AboutStepRule = {
...mockData,
customHighlightedFields: ['foo', 'bar'],
};
const result = formatAboutStepData(mockStepData);
const expected: AboutStepRuleJson = {
author: ['Elastic'],
description: '24/7',
false_positives: ['test'],
license: 'Elastic License',
name: 'Query with rule-id',
note: '# this is some markdown documentation',
references: ['www.test.co'],
risk_score: 21,
risk_score_mapping: [],
severity: 'low',
severity_mapping: [],
tags: ['tag1', 'tag2'],
threat: getThreatMock(),
custom_highlighted_fields: ['foo', 'bar'],
threat_indicator_path: undefined,
timestamp_override: undefined,
timestamp_override_fallback_disabled: undefined,
};

expect(result).toEqual(expected);
});

test('returns formatted object if custom_highlighted_fields includes empty string', () => {
const mockStepData: AboutStepRule = {
...mockData,
customHighlightedFields: [' '],
};
const result = formatAboutStepData(mockStepData);
const expected: AboutStepRuleJson = {
author: ['Elastic'],
description: '24/7',
false_positives: ['test'],
license: 'Elastic License',
name: 'Query with rule-id',
note: '# this is some markdown documentation',
references: ['www.test.co'],
risk_score: 21,
risk_score_mapping: [],
severity: 'low',
severity_mapping: [],
tags: ['tag1', 'tag2'],
threat: getThreatMock(),
custom_highlighted_fields: [],
threat_indicator_path: undefined,
timestamp_override: undefined,
timestamp_override_fallback_disabled: undefined,
};

expect(result).toEqual(expected);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -485,6 +485,7 @@ export const formatAboutStepData = (
const {
author,
falsePositives,
customHighlightedFields,
references,
riskScore,
severity,
Expand Down Expand Up @@ -524,6 +525,7 @@ export const formatAboutStepData = (
: {}),
false_positives: falsePositives.filter((item) => !isEmpty(item)),
references: references.filter((item) => !isEmpty(item)),
custom_highlighted_fields: customHighlightedFields.filter((item) => !isEmpty(item.trim())),
risk_score: riskScore.value,
risk_score_mapping: riskScore.isMappingChecked
? riskScore.mapping.filter((m) => m.field != null && m.field !== '')
Expand Down
Loading