[Security Solution] `FinalEdit`: Add fields that are common for all rule types #196642

nikitaindik · 2024-10-17T08:59:24Z

Partially addresses: #171520
Is a follow-up to: #196326

This PR enables editing of common fields in the new "Updates" tab of the rule upgrade flyout. The common fields are fields applicable to all rule types.

Summary

These fields are editable now:

building_block
description
false_positives
investigation_fields
max_signals
note
references
related_integrations
required_fields
risk_score
risk_score_mapping
rule_name_override
rule_schedule
setup
severity
severity_mapping
tags
threat
timeline_template
timestamp_override

Testing

Ensure the prebuiltRulesCustomizationEnabled feature flag is enabled.
To simulate the availability of prebuilt rule upgrades, downgrade a currently installed prebuilt rule using the PATCH api/detection_engine/rules API.
- Set version: 1 in the request body to downgrade it to version 1.
- Modify other rule fields in the request body as needed to test the changes.

…rride`

…lds-2

elasticmachine · 2024-11-08T15:14:14Z

Pinging @elastic/security-solution (Team: SecuritySolution)

elasticmachine · 2024-11-08T15:14:15Z

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

…lds-2

nikitaindik · 2024-11-09T10:26:41Z

@maximpn UPD

These two issues are not easy to fix due to the way the dropdown component is built. Let's discuss how we should approach this offline.

I've used the approach that we discussed offline and fixed these two as well in 66afe18

maximpn

@nikitaindik I had a look at the recent changes and left comments. I think we should make slight improvements before merging it back.

maximpn · 2024-11-11T09:01:09Z

...ty_solution/public/detection_engine/rule_creation_ui/components/autocomplete_field/index.tsx

+  const { selectedField, indexPattern } = useMemo(() => {
+    /* eslint-disable @typescript-eslint/no-shadow */
+    /* If the field is not found in the indices, we still want to show it in the dropdown */
+    const selectedField = foundField || { name: field.value, type: fieldType };


It's better to use a different name inside useMemo and remove eslint-disable.

maximpn · 2024-11-11T09:11:39Z

...ty_solution/public/detection_engine/rule_creation_ui/components/autocomplete_field/index.tsx

-    );
-    return newSelectedField;
-  }, [field.value, indices]);
+  const foundField = field.value && indices.fields.find(({ name }) => name === field.value);


This line should be inside useMemo()'s callback.

maximpn · 2024-11-11T09:54:04Z

...ty_solution/public/detection_engine/rule_creation_ui/components/autocomplete_field/index.tsx

+  const { selectedField, indexPattern } = useMemo(() => {
+    /* eslint-disable @typescript-eslint/no-shadow */
+    /* If the field is not found in the indices, we still want to show it in the dropdown */
+    const selectedField = foundField || { name: field.value, type: fieldType };
+    const indexPattern = foundField
+      ? indices
+      : { ...indices, fields: [...indices.fields, selectedField] };
+
+    return { selectedField, indexPattern };
+    /* eslint-enable @typescript-eslint/no-shadow */
+  }, [field.value, indices, foundField, fieldType]);


For better readability this logic could be moved to an "internal" hook named like useSelectedField() and placed under the component in the file.

Though it might be better to have this logic in FieldComponent.

…to `EsFieldSelectorField`

…lds-2

nikitaindik · 2024-11-11T12:47:45Z

@maximpn Thanks for taking another look Maxim and discussing it with me over Zoom. I've pushed a commit that addresses your feedback and also:

Renames AutocompleteField to EsFieldSelectorField
Renames FieldComponent to EsFieldSelector
Renames useField to useEsField

Please take a look.

nkhristinin

I tested locally, updated flow and create/edit rule. LGTM

Left some question about implementations

nkhristinin · 2024-11-11T11:47:16Z

...ic/detection_engine/rule_creation/components/required_fields/make_validate_required_field.ts

+) {
+  /*
+    Getting values for required fields via flattened fields instead of using `getFormData`.
+    This is because `getFormData` applies a serializer function to field values, which might update values.


How exactly serializer update values? Or, why it's a problem for our case?

Serializer is a function you can pass to the form library. It converts form data from an internal format to an output format when you click Submit.

To get the form data for validation, we normally use form.getFormData(). However, this method internally calls the serializer. In this case, we need the form data in its internal format, before it is serialized, because the returned value would be different if you pass a different serialzers.

In our case we have no serializer on the Rule Editing page, but we do have one in the flyout

nkhristinin · 2024-11-11T11:56:51Z

.../security_solution/public/detection_engine/rule_creation/components/required_fields/utils.ts

@@ -26,3 +28,13 @@ export function pickTypeForName({ name, type, typesByFieldName = {} }: PickTypeF
  */
  return typesAvailableForName[0] ?? type;
 }
+
+export function getFlattenedArrayFieldNames(


Not sure that commment, is enough. We rely on internal implmenatin, and if it chanes, we don't have ability to notice that our function don't work.

Ar least we should have a tests for his function, which use real form implmentaion

But can we maybe use getInternalArrayFieldPath function from the forms?

nkhristinin · 2024-11-11T12:07:30Z

...ion/public/detection_engine/rule_creation/components/required_fields/required_fields_row.tsx

@@ -44,14 +44,6 @@ export const RequiredFieldRow = ({
  const rowFieldConfig: FieldConfig<RequiredField | RequiredFieldInput, {}, RequiredFieldInput> =
    useMemo(
      () => ({
-        deserializer: (value) => {


Why we removing that?

It was needed to convert the type from RequiredField to RequiredFieldInput, but I've modified the component to use RequiredField, so this conversion is no longer needed.

nkhristinin · 2024-11-11T12:15:13Z

.../security_solution/public/detection_engine/rule_creation/components/required_fields/utils.ts

@@ -26,3 +28,13 @@ export function pickTypeForName({ name, type, typesByFieldName = {} }: PickTypeF
  */
  return typesAvailableForName[0] ?? type;
 }
+
+export function getFlattenedArrayFieldNames(


Ok, I see that you didn't add this code, it was already there.

Still if possible add tests, or reuse getInternalArrayFieldPath will be great

nkhristinin · 2024-11-11T12:20:57Z

...ty_solution/public/detection_engine/rule_creation_ui/components/autocomplete_field/index.tsx

-  const fieldTypeFilter = useMemo(() => [fieldType], [fieldType]);
+  const { selectedField, indexPattern } = useMemo(() => {
+    /* eslint-disable @typescript-eslint/no-shadow */
+    /* If the field is not found in the indices, we still want to show it in the dropdown */


Are we sure what we want to show it in dropdown?

Will be ther any error or message that field not in index?

Actually, I ended up removing this part and reverting to previous behaviour. It turned out we already handle such case in a nested component.

elasticmachine · 2024-11-11T14:22:21Z

💚 Build Succeeded

Buildkite Build
Commit: 892e1eb

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	6135	6163	+28

Async chunks

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

id	before	after	diff
`lists`	144.9KB	144.9KB	-15.0B
`securitySolution`	13.4MB	13.4MB	+13.7KB
total			+13.6KB

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
`@kbn/securitysolution-autocomplete`	1	0	-1

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`securitySolution`	553	554	+1

Total ESLint disabled count

id	before	after	diff
`securitySolution`	639	640	+1

History

💚 Build #249592 succeeded b4a4c29
💔 Build #249536 failed 6986e21
💔 Build #249511 failed db15ed8
💔 Build #247775 failed fcd9457

cc @nikitaindik

maximpn

@nikitaindik Thanks for addressing my comments 🙏

I tested the PR on the latest commit and editable components work as expected 👍

maximpn · 2024-11-11T16:05:04Z

@nkhristinin There is time constrain on Prebuilt Rule Customization readiness. Do you think there is something critical in you comments and should be addressed in this PR? I created a ticket for rule creation/editing forms improvements. Let's add any desired enhancements/improvements to that ticket.

nkhristinin · 2024-11-12T08:44:04Z

@maximpn I think we can add tests for this function later #196642 (comment)

Other comments, not blocking, more requesting reason for those changes

kibanamachine · 2024-11-12T10:04:30Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11794964800

…ule types (elastic#196642) **Partially addresses: elastic#171520 **Is a follow-up to: elastic#196326 This PR enables editing of common fields in the new "Updates" tab of the rule upgrade flyout. The common fields are fields applicable to all rule types. ## Summary These fields are editable now: - `building_block` - `description` - `false_positives` - `investigation_fields` - `max_signals` - `note` - `references` - `related_integrations` - `required_fields` - `risk_score` - `risk_score_mapping` - `rule_name_override` - `rule_schedule` - `setup` - `severity` - `severity_mapping` - `tags` - `threat` - `timeline_template` - `timestamp_override` <img width="2672" alt="Schermafbeelding 2024-10-16 om 17 32 06" src="https://github.com/user-attachments/assets/6dd615e2-6e84-4e1f-b674-f42d03f575e7"> ### Testing - Ensure the `prebuiltRulesCustomizationEnabled` feature flag is enabled. - To simulate the availability of prebuilt rule upgrades, downgrade a currently installed prebuilt rule using the `PATCH api/detection_engine/rules` API. - Set `version: 1` in the request body to downgrade it to version 1. - Modify other rule fields in the request body as needed to test the changes. (cherry picked from commit 3d3b32f)

kibanamachine · 2024-11-12T10:09:35Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…common for all rule types (#196642) (#199743) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution] `FinalEdit`: Add fields that are common for all rule types (#196642)](#196642)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Nikita Indik <[email protected]>

…ule types (elastic#196642) **Partially addresses: elastic#171520 **Is a follow-up to: elastic#196326 This PR enables editing of common fields in the new "Updates" tab of the rule upgrade flyout. The common fields are fields applicable to all rule types. ## Summary These fields are editable now: - `building_block` - `description` - `false_positives` - `investigation_fields` - `max_signals` - `note` - `references` - `related_integrations` - `required_fields` - `risk_score` - `risk_score_mapping` - `rule_name_override` - `rule_schedule` - `setup` - `severity` - `severity_mapping` - `tags` - `threat` - `timeline_template` - `timestamp_override` <img width="2672" alt="Schermafbeelding 2024-10-16 om 17 32 06" src="https://github.com/user-attachments/assets/6dd615e2-6e84-4e1f-b674-f42d03f575e7"> ### Testing - Ensure the `prebuiltRulesCustomizationEnabled` feature flag is enabled. - To simulate the availability of prebuilt rule upgrades, downgrade a currently installed prebuilt rule using the `PATCH api/detection_engine/rules` API. - Set `version: 1` in the request body to downgrade it to version 1. - Modify other rule fields in the request body as needed to test the changes.

nikitaindik self-assigned this Oct 17, 2024

nikitaindik added 7 commits October 17, 2024 11:02

Add tags

daee1bf

Add description

cb89547

Add references

c88c9c5

Add false_positives

34a549e

Add investigation_fields

e0ed2f2

Add building_block

c444bc1

Add max_signals

19ddb83

nikitaindik force-pushed the editable-common-fields-2 branch from cfacec3 to 19ddb83 Compare October 17, 2024 11:57

nikitaindik added 4 commits October 17, 2024 15:09

Add note

bbe7ddf

Add setup

c708ee1

Add related_integrations

70624d7

Add rule_name_override

70f6db1

nikitaindik mentioned this pull request Oct 18, 2024

[Security Solution] Rework Update flyout to display all field updates and build Three-Way-Diff field component #171520

Open

69 tasks

nikitaindik added 2 commits October 18, 2024 17:28

Add rule_schedule

33444ca

Add threat

97fe9b3

nikitaindik mentioned this pull request Oct 22, 2024

[Security Solution] FinalEdit: Add fields that are common for all rule types (PR 1) #196326

Closed

nikitaindik added 2 commits October 23, 2024 15:26

Add timeline_template

e0a5c83

Add timestamp_override

6377545

nikitaindik force-pushed the editable-common-fields-2 branch from 0f789a4 to 6377545 Compare October 23, 2024 13:27

nikitaindik added 4 commits October 24, 2024 10:51

Add required_fields

d371492

Add risk_score and risk_score_mapping

bca821a

Add severity, severity_override, risk_score and `risk_score_ove…

cd9521f

…rride`

Merge remote-tracking branch 'upstream/main' into editable-common-fie…

4702272

…lds-2

nikitaindik removed the request for review from dplumlee November 8, 2024 15:14

nikitaindik added 2 commits November 9, 2024 11:23

Make AutocompleteField display values that are missing from indices

66afe18

Merge remote-tracking branch 'upstream/main' into editable-common-fie…

a8b424f

…lds-2

nikitaindik requested a review from maximpn November 10, 2024 15:39

maximpn reviewed Nov 11, 2024

View reviewed changes

nikitaindik added 2 commits November 11, 2024 13:35

Simplify logic in AutocompleteField and rename AutocompleteField …

e276eb6

…to `EsFieldSelectorField`

Merge remote-tracking branch 'upstream/main' into editable-common-fie…

892e1eb

…lds-2

nikitaindik requested a review from maximpn November 11, 2024 12:47

nkhristinin reviewed Nov 11, 2024

View reviewed changes

maximpn approved these changes Nov 11, 2024

View reviewed changes

nkhristinin self-requested a review November 12, 2024 08:44

nkhristinin approved these changes Nov 12, 2024

View reviewed changes

maximpn mentioned this pull request Nov 12, 2024

[Security Solution] Rule creation/editing forms improvements #199662

Open

4 tasks

maximpn merged commit 3d3b32f into elastic:main Nov 12, 2024
43 checks passed

kibanamachine added the v9.0.0 label Nov 12, 2024

kibanamachine mentioned this pull request Nov 12, 2024

[8.x] [Security Solution] `FinalEdit`: Add fields that are common for all rule types (#196642) #199743

Merged

kibanamachine added the v8.17.0 label Nov 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] `FinalEdit`: Add fields that are common for all rule types #196642

[Security Solution] `FinalEdit`: Add fields that are common for all rule types #196642

nikitaindik commented Oct 17, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Nov 8, 2024

elasticmachine commented Nov 8, 2024

nikitaindik commented Nov 9, 2024

maximpn left a comment

maximpn Nov 11, 2024

maximpn Nov 11, 2024

maximpn Nov 11, 2024

nikitaindik commented Nov 11, 2024

nkhristinin left a comment

nkhristinin Nov 11, 2024 •

edited

Loading

nikitaindik Nov 12, 2024

nikitaindik Nov 12, 2024

nkhristinin Nov 11, 2024

nkhristinin Nov 11, 2024

nikitaindik Nov 12, 2024

nkhristinin Nov 11, 2024

nkhristinin Nov 11, 2024

nikitaindik Nov 12, 2024

elasticmachine commented Nov 11, 2024

ESLint disabled line counts

Total ESLint disabled count

maximpn left a comment

maximpn commented Nov 11, 2024 •

edited

Loading

nkhristinin commented Nov 12, 2024

kibanamachine commented Nov 12, 2024

kibanamachine commented Nov 12, 2024

[Security Solution] FinalEdit: Add fields that are common for all rule types #196642

[Security Solution] FinalEdit: Add fields that are common for all rule types #196642

Conversation

nikitaindik commented Oct 17, 2024 • edited by kibanamachine Loading

Summary

Testing

elasticmachine commented Nov 8, 2024

elasticmachine commented Nov 8, 2024

nikitaindik commented Nov 9, 2024

maximpn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikitaindik commented Nov 11, 2024

nkhristinin left a comment

Choose a reason for hiding this comment

nkhristinin Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Nov 11, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Public APIs missing exports

ESLint disabled line counts

Total ESLint disabled count

History

maximpn left a comment

Choose a reason for hiding this comment

maximpn commented Nov 11, 2024 • edited Loading

nkhristinin commented Nov 12, 2024

kibanamachine commented Nov 12, 2024

kibanamachine commented Nov 12, 2024

💚 All backports created successfully

Questions ?

[Security Solution] `FinalEdit`: Add fields that are common for all rule types #196642

[Security Solution] `FinalEdit`: Add fields that are common for all rule types #196642

nikitaindik commented Oct 17, 2024 •

edited by kibanamachine

Loading

nkhristinin Nov 11, 2024 •

edited

Loading

maximpn commented Nov 11, 2024 •

edited

Loading