[Security Solution][Detections] Fix severity and risk score overrides when field mapping exists but the mapped fields do not #87004

banderror · 2020-12-29T18:24:56Z

Addresses: #85951 and a similar bug in Risk score override section.

Summary

Check the bug description and steps to reproduce.

When a field does not exist, but specified in severity or risk score override of an existing rule, and the user opens this rule for editing, we should show all the mapped fields, even if they don't exist. Maybe the rule has been activated (e.g. a built-in rule) but the data hasn't been indexed yet. Or maybe the data existed before, but was deleted for any reason.

The components we use to display the mapping contained strict logic for making sure that any value we display in a combobox must correspond to a real field or field value, existing right now in the indices specified in the rule. In order to be able to fix the bug, I removed this logic for "initial state" (first render), but left for the case when the user starts typing something in the combobox. In other words:

we now allow showing non-existing fields in those overrides, if they were specified before
we still disallow selecting any non-existing fields if the user starts typing it in the combobox

This should fix the filed bug.

Screen recording

Video

Checklist

~~[ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~[ ] Documentation was added for features that require explanation or tutorials~~
Unit or functional tests were updated or added to match the most common scenarios
Any UI touched in this PR is usable by keyboard only (learn more about keyboard accessibility)
~~[ ] Any UI touched in this PR does not create any new axe failures (run axe in browser: FF, Chrome)~~
~~[ ] This renders correctly on smaller devices using a responsive layout. (You can test this in your browser)~~
~~[ ] This was checked for cross-browser compatibility~~

For maintainers

This was checked for breaking API changes and was labeled appropriately

banderror · 2020-12-29T19:25:32Z

@MikePaquette @peluja1012 @spong As a future enhancement, do you think there's any value in letting the user type any field names in the overrides, even if the fields don't exist (yet)? So our combobox values can be used to assist users but not to restrict them, especially for cases when the user wants to create a rule before indexing any data?

…e sections

kibanamachine · 2021-01-05T18:06:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: ad7a2a0

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	8.5MB	8.5MB	+1.1KB

Distributable file count

id	before	after	diff
`default`	47252	48012	+760

History

💚 Build #96695 succeeded fbe87afad0ba376f4cce5959a29ef8bce0c7cf9a
💔 Build #96625 failed c2141402487f9e93852850c64d95c03198d1e016
💔 Build #96371 failed e1f7532fcb4aab7134fff6af9276820dd4ef3453

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

rylnd

Verified this locally; thank you for the helpful repro steps! I had a few nits but nothing to block this!

rylnd · 2021-01-05T20:51:05Z

x-pack/plugins/security_solution/cypress/screens/create_new_rule.ts

@@ -101,7 +101,11 @@ export const REFERENCE_URLS_INPUT =

 export const REFRESH_BUTTON = '[data-test-subj="refreshButton"]';

-export const RISK_INPUT = '.euiRangeInput';
+export const DEFAULT_RISK_SCORE_INPUT =


Is it possible to omit the .eui* from these selectors? The data-test-subjs are much preferred, but these selectors will still be brittle to EUI changes as they are now.

Yeah, I don't like this too... Unfortunately, not possible w/o submitting a PR to EUI.

<EuiRange data-test-subj="detectionEngineStepAboutRuleRiskScore-defaultRiskRange"/>

This guy renders two inputs under the hood (first one is fancy slider, second one is the normal input) and passes this data-test-subj to both of them. So to select one of them we can use either its order or its classnames.

rylnd · 2021-01-05T21:13:43Z

x-pack/plugins/security_solution/public/common/components/autocomplete/field.tsx

+  selectedFields: IFieldType[],
+  fieldTypeFilter: string[]
+): IFieldType[] => {
+  const map = new Map<string, IFieldType>();


Nit: maybe this could be fieldsByName instead of map?

Great naming suggestions, thank you! Sometimes I skip making names descriptive if the scope is small 🙂 Here I think it makes sense, I'll submit a quick PR.

rylnd · 2021-01-05T21:46:55Z

x-pack/plugins/security_solution/public/common/components/autocomplete/field.tsx

+  existingFields.forEach((f) => map.set(f.name, f));
+  selectedFields.forEach((f) => map.set(f.name, f));
+
+  const array = Array.from(map.values());


similar nit:

Suggested change

const array = Array.from(map.values());

const uniqueFields = Array.from(map.values());

… when field mapping exists but the mapped fields do not (elastic#87004) * Fix Source field combobox in Severity override and Risk score override sections * Clean up * Fix unit and Cypress tests

… when field mapping exists but the mapped fields do not (#87004) (#87412) * Fix Source field combobox in Severity override and Risk score override sections * Clean up * Fix unit and Cypress tests Co-authored-by: Georgii Gorbachev <[email protected]>

… when field mapping exists but the mapped fields do not (#87004) (#87411) * Fix Source field combobox in Severity override and Risk score override sections * Clean up * Fix unit and Cypress tests Co-authored-by: Georgii Gorbachev <[email protected]>

…87516) ## Summary This is a follow-up PR addressing some of the comments in: - #86908 - #87004

…lastic#87516) ## Summary This is a follow-up PR addressing some of the comments in: - elastic#86908 - elastic#87004

…87516) (#87549) ## Summary This is a follow-up PR addressing some of the comments in: - #86908 - #87004

…87516) (#87550) ## Summary This is a follow-up PR addressing some of the comments in: - #86908 - #87004

…nd marks other duplicated code (#105374) ## Summary * Removes dead duplicated code from `security_solution` and `lists` * Adds notes and TODO's where we still have duplicated logic * Adds notes where I saw that the original deviated from the copy from modifications in one file but not the other. * DOES NOT fix the bugs existing in one copy but not the other. That should be done when the copied chunks are collapsed into a package. Instead see this issue where I marked those areas: #105378 See these two files where things have deviated from our duplications as an example: [security_solution/public/common/components/autocomplete/field.tsx](https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/public/common/components/autocomplete/field.tsx ) [lists/public/exceptions/components/autocomplete/field.tsx](https://github.com/elastic/kibana/blob/master/x-pack/plugins/lists/public/exceptions/components/autocomplete/field.tsx) Ref PR where fixes are applied to one of the files but not the other (could be other PR's in addition to this one): #87004 ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…nd marks other duplicated code (elastic#105374) ## Summary * Removes dead duplicated code from `security_solution` and `lists` * Adds notes and TODO's where we still have duplicated logic * Adds notes where I saw that the original deviated from the copy from modifications in one file but not the other. * DOES NOT fix the bugs existing in one copy but not the other. That should be done when the copied chunks are collapsed into a package. Instead see this issue where I marked those areas: elastic#105378 See these two files where things have deviated from our duplications as an example: [security_solution/public/common/components/autocomplete/field.tsx](https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/public/common/components/autocomplete/field.tsx ) [lists/public/exceptions/components/autocomplete/field.tsx](https://github.com/elastic/kibana/blob/master/x-pack/plugins/lists/public/exceptions/components/autocomplete/field.tsx) Ref PR where fixes are applied to one of the files but not the other (could be other PR's in addition to this one): elastic#87004 ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…nd marks other duplicated code (#105374) (#105524) ## Summary * Removes dead duplicated code from `security_solution` and `lists` * Adds notes and TODO's where we still have duplicated logic * Adds notes where I saw that the original deviated from the copy from modifications in one file but not the other. * DOES NOT fix the bugs existing in one copy but not the other. That should be done when the copied chunks are collapsed into a package. Instead see this issue where I marked those areas: #105378 See these two files where things have deviated from our duplications as an example: [security_solution/public/common/components/autocomplete/field.tsx](https://github.com/elastic/kibana/blob/master/x-pack/plugins/security_solution/public/common/components/autocomplete/field.tsx ) [lists/public/exceptions/components/autocomplete/field.tsx](https://github.com/elastic/kibana/blob/master/x-pack/plugins/lists/public/exceptions/components/autocomplete/field.tsx) Ref PR where fixes are applied to one of the files but not the other (could be other PR's in addition to this one): #87004 ### Checklist Delete any items that are not applicable to this PR. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios Co-authored-by: Frank Hassanabad <[email protected]>

…e and moves duplicate code between lists and security_solution there (#105382) ## Summary Creates an autocomplete package from `lists` and removes duplicate code between `lists` and `security_solutions` * Consolidates different PR's where we were changing different parts of autocomplete in different ways. * Existing Cypress tests should cover any mistakes hopefully Manual Testing: * Ensure this bug does not crop up again #87004 * Make sure that the exception list autocomplete looks alright ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…e and moves duplicate code between lists and security_solution there (elastic#105382) ## Summary Creates an autocomplete package from `lists` and removes duplicate code between `lists` and `security_solutions` * Consolidates different PR's where we were changing different parts of autocomplete in different ways. * Existing Cypress tests should cover any mistakes hopefully Manual Testing: * Ensure this bug does not crop up again elastic#87004 * Make sure that the exception list autocomplete looks alright ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios

…e and moves duplicate code between lists and security_solution there (elastic#105382) ## Summary Creates an autocomplete package from `lists` and removes duplicate code between `lists` and `security_solutions` * Consolidates different PR's where we were changing different parts of autocomplete in different ways. * Existing Cypress tests should cover any mistakes hopefully Manual Testing: * Ensure this bug does not crop up again elastic#87004 * Make sure that the exception list autocomplete looks alright ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios # Conflicts: # x-pack/plugins/translations/translations/ja-JP.json

…e and moves duplicate code between lists and security_solution there (#105382) (#106612) ## Summary Creates an autocomplete package from `lists` and removes duplicate code between `lists` and `security_solutions` * Consolidates different PR's where we were changing different parts of autocomplete in different ways. * Existing Cypress tests should cover any mistakes hopefully Manual Testing: * Ensure this bug does not crop up again #87004 * Make sure that the exception list autocomplete looks alright ### Checklist - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios # Conflicts: # x-pack/plugins/translations/translations/ja-JP.json

banderror self-assigned this Dec 29, 2020

banderror marked this pull request as ready for review December 29, 2020 19:23

banderror requested review from a team as code owners December 29, 2020 19:23

banderror requested review from yctercero and a team December 29, 2020 19:23

banderror force-pushed the fix-severity-overrides-in-duplicated-rules branch 2 times, most recently from c214140 to fbe87af Compare January 4, 2021 17:07

banderror added 3 commits January 5, 2021 16:49

Fix Source field combobox in Severity override and Risk score overrid…

83f9821

…e sections

Clean up

709ca98

Fix unit and Cypress tests

ad7a2a0

banderror force-pushed the fix-severity-overrides-in-duplicated-rules branch from fbe87af to ad7a2a0 Compare January 5, 2021 15:50

rylnd approved these changes Jan 5, 2021

View reviewed changes

rylnd merged commit a1f949b into elastic:master Jan 5, 2021

rylnd mentioned this pull request Jan 5, 2021

[7.x] [Security Solution][Detections] Fix severity and risk score overrides when field mapping exists but the mapped fields do not (#87004) #87411

Merged

rylnd mentioned this pull request Jan 5, 2021

[7.11] [Security Solution][Detections] Fix severity and risk score overrides when field mapping exists but the mapped fields do not (#87004) #87412

Merged

banderror deleted the fix-severity-overrides-in-duplicated-rules branch January 6, 2021 14:26

banderror mentioned this pull request Jan 6, 2021

[Security Solution] The detection rules have defined source values for severity overridden but there is no Source field selected for the same. #85951

Closed

banderror added a commit to banderror/kibana that referenced this pull request Jan 6, 2021

Cleanup for elastic#87004

48d778d

banderror mentioned this pull request Jan 6, 2021

[Security Solution][Detections] Follow up cleanup after two bugfixes #87516

Merged

1 task

banderror added a commit that referenced this pull request Jan 6, 2021

[Security Solution][Detections] Follow up cleanup after two bugfixes (#…

e50ad38

…87516) ## Summary This is a follow-up PR addressing some of the comments in: - #86908 - #87004

banderror added a commit to banderror/kibana that referenced this pull request Jan 6, 2021

[Security Solution][Detections] Follow up cleanup after two bugfixes (e…

b0e84f4

…lastic#87516) ## Summary This is a follow-up PR addressing some of the comments in: - elastic#86908 - elastic#87004

banderror added a commit to banderror/kibana that referenced this pull request Jan 6, 2021

[Security Solution][Detections] Follow up cleanup after two bugfixes (e…

f8d82e5

…lastic#87516) ## Summary This is a follow-up PR addressing some of the comments in: - elastic#86908 - elastic#87004

banderror added a commit that referenced this pull request Jan 7, 2021

[Security Solution][Detections] Follow up cleanup after two bugfixes (#…

8976bd1

…87516) (#87549) ## Summary This is a follow-up PR addressing some of the comments in: - #86908 - #87004

banderror added a commit that referenced this pull request Jan 7, 2021

[Security Solution][Detections] Follow up cleanup after two bugfixes (#…

eb3973b

…87516) (#87550) ## Summary This is a follow-up PR addressing some of the comments in: - #86908 - #87004

This was referenced Jul 12, 2021

[Security Solutions][Detection Engine] Removes dead duplicated code and marks other duplicated code #105374

Merged

[Security Solution][Detection Engine] Pull out duplicated code from lists and security_solution into a package #105378

Closed

FrankHassanabad mentioned this pull request Jul 20, 2021

[Security Solutions][Detection Engine] Creates an autocomplete package and moves duplicate code between lists and security_solution there #105382

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detections] Fix severity and risk score overrides when field mapping exists but the mapped fields do not #87004

[Security Solution][Detections] Fix severity and risk score overrides when field mapping exists but the mapped fields do not #87004

banderror commented Dec 29, 2020 •

edited

Loading

banderror commented Dec 29, 2020

kibanamachine commented Jan 5, 2021

rylnd left a comment

rylnd Jan 5, 2021

banderror Jan 6, 2021

rylnd Jan 5, 2021

banderror Jan 6, 2021

rylnd Jan 5, 2021

	const array = Array.from(map.values());
	const uniqueFields = Array.from(map.values());

[Security Solution][Detections] Fix severity and risk score overrides when field mapping exists but the mapped fields do not #87004

[Security Solution][Detections] Fix severity and risk score overrides when field mapping exists but the mapped fields do not #87004

Conversation

banderror commented Dec 29, 2020 • edited Loading

Summary

Screen recording

Checklist

For maintainers

banderror commented Dec 29, 2020

kibanamachine commented Jan 5, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Distributable file count

History

rylnd left a comment

Choose a reason for hiding this comment

rylnd Jan 5, 2021

Choose a reason for hiding this comment

banderror Jan 6, 2021

Choose a reason for hiding this comment

rylnd Jan 5, 2021

Choose a reason for hiding this comment

banderror Jan 6, 2021

Choose a reason for hiding this comment

rylnd Jan 5, 2021

Choose a reason for hiding this comment

banderror commented Dec 29, 2020 •

edited

Loading