[Security Solution] Allow users to save rule query with non critical validation errors #202544

maximpn · 2024-12-02T17:06:01Z

Addresses: #171520

Summary

This PR adds functionality to allow users save EQL and ES|QL queries in Prebuilt Rule Customization workflow by displaying a confirmation modal with non critical validation errors (a.k.a warnings). It also refactors confirmation modal usage in rule creation/editing forms for better reusability.

Screenshots

Screen.Recording.2024-12-03.at.14.01.57.mov

Screen.Recording.2024-12-03.at.14.05.29.mov

elasticmachine · 2024-12-03T13:14:09Z

Pinging @elastic/security-detections-response (Team:Detections and Resp)

elasticmachine · 2024-12-03T13:14:10Z

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

elasticmachine · 2024-12-03T13:14:12Z

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

dplumlee

LGTM @maximpn, left a couple minor comments but couldn't find any implementation bugs

Tested with a prebuilt ESQL and EQL rule in the

create rule form
edit rule form
upgrade flyout

as well as a custom EQL and ESQL rule in the create/edit forms.

Something I noticed (and maybe it was just because I was down a rabbit hole for testing) but it might be nice to have something differentiating the different validation types (blocking vs non-blocking). Right now they're identically presented but one allows saving and one doesn't unless the user knows the different types just by the EQL error message. I'm not sure if we'd want to label them as "warnings" instead of errors like we do for some of the other fields (e.g. required_fields) but either way probably not in the scope of this PR

dplumlee · 2024-12-04T18:33:19Z

x-pack/plugins/security_solution/public/common/hooks/use_form_with_warn/use_form_with_warn.ts

+interface SubmitHandlerWithWarnExtras {
+  errors: ValidationError[];
+  warnings: ValidationError[];
+}


nit: this same interface is defined multiple places in this PR, could use a common one

dplumlee · 2024-12-04T19:00:56Z

x-pack/plugins/security_solution/public/detection_engine/rule_creation_ui/pages/form.test.ts

Are there any plans to update and replace these tests?

maximpn · 2024-12-04T21:02:21Z

Something I noticed (and maybe it was just because I was down a rabbit hole for testing) but it might be nice to have something differentiating the different validation types (blocking vs non-blocking). Right now they're identically presented but one allows saving and one doesn't unless the user knows the different types just by the EQL error message. I'm not sure if we'd want to label them as "warnings" instead of errors like we do for some of the other fields (e.g. required_fields) but either way probably not in the scope of this PR

@dplumlee,

Thanks for review 🙏 Earlier I created #201095 to discuss the approach. Warnings look like better UX. And we still can can show a confirmation modal before saving. Anyway it's a longer story than Milestone 3. But it's a good improvement to be included in for Milestone 4.

elasticmachine · 2024-12-05T12:20:08Z

💚 Build Succeeded

Buildkite Build
Commit: 4fbb346

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	6308	6314	+6

Async chunks

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

id	before	after	diff
`securitySolution`	14.6MB	14.6MB	-274.0B

History

💛 Build #257130 was flaky 754c361
💚 Build #256482 succeeded 2045273
💔 Build #256438 failed 743a712

cc @maximpn

maximpn · 2024-12-05T13:12:30Z

Are there any plans to update and replace these tests?

@dplumlee

Tests in public/detection_engine/rule_creation_ui/pages/form.test.ts didn't test the form behavior. It was just a suite of primitive unit tests. To have real coverage we should set up UI integration tests for rule creation/editing forms. My preliminary research has shown that it might take significant amount of time. Additionally it'd be nice addressing technical debt impacting rule creation/editing forms we introduced while working on Prebuil Rule Customization workflow. We should get back to that after finishing Prebuil Rule Customization feature. I'll create necessary tickets for that. There is one I created to improve Form lib #203097.

rylnd

I tested the upgrade/resolution path with a few rules and everything looks as presented in your videos 👍 .

I had similar feedback to @dplumlee in that I think the UI distinction between warnings/errors could be more obvious, but I also agree that it's a relatively minor issue and can be addressed later.

I would love to see discussion on #203097; it would be great to have some/all of this functionality moved upstream into the form lib itself, but this is a great intermediate step 👍 .

I had a few nits about naming, but overall this LGTM.

rylnd · 2024-12-05T18:32:49Z

x-pack/plugins/security_solution/public/common/hooks/use_form_with_warn/form_hook_with_warn.ts

+
+import type { FormHook, FormData, ValidationError } from '../../../shared_imports';
+
+export interface FormHookWithWarn<T extends FormData = FormData, I extends FormData = T>


I think this naming is a little ambiguous. Why not use the full word for clarity?

Suggested change

export interface FormHookWithWarn<T extends FormData = FormData, I extends FormData = T>

export interface FormHookWithWarnings<T extends FormData = FormData, I extends FormData = T>

rylnd · 2024-12-05T18:36:34Z

x-pack/plugins/security_solution/public/common/hooks/use_form_with_warn/use_form_with_warn.ts

+ * `isBlocking`. Validators marked as `isBlocking` will produce non blocking validation errors
+ * a.k.a. warnings.
+ *
+ * The problem with the supported approach is lack of flexibility and necessary API like one for getting


@maximpn have you discussed these limitations (and #203097) with @sebelga? This looks like good feedback for the form-lib team.

I shared the ticket with Sébastien. Will see how it could be tackled.

My goal was to move incrementally and the first step was existing functionality refactoring to reuse it in Prebuilt Rules Customization workflow. I'd be happy to reiterate on that when I get a chance.

rylnd · 2024-12-05T18:47:10Z

x-pack/plugins/security_solution/public/common/hooks/use_form_with_warn/use_form_with_warn.ts

+ * `message` and `code` fields from a validator function. Attempts to reuse `__isBlocking__` internal
+ * field lead to inconsistent behavior.
+ *
+ * `useFormWithWarn` implements warnings (non blocking errors) on top of `FormHook` using validation


This is an excellent explanation for this hook, thank you for writing this up!

rylnd · 2024-12-05T18:54:19Z

...n_engine/rule_creation/components/esql_query_edit/validators/esql_query_validator_factory.ts

@@ -19,15 +19,10 @@ import * as i18n from './translations';

 interface EsqlQueryValidatorFactoryParams {
  queryClient: QueryClient;
-  /**
-   * This is a temporal fix to unlock prebuilt rule customization workflow


rylnd · 2024-12-05T18:56:51Z

...ecurity_solution/public/detection_engine/rule_creation/constants/non_blocking_error_codes.ts

+  }
+);
+
+export const NON_BLOCKING_ERROR_CODES = [


Nit: we now use both "non-blocking error" and "warning" interchangeably in this domain. I think we should strive to consolidate to a single term, and "warning" makes the most sense as we're diverging from the "blocking" concept of the form lib.

kibanamachine · 2024-12-06T12:06:56Z

Starting backport for target branches: 8.x

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

…validation errors (elastic#202544) **Addresses:** elastic#171520 ## Summary This PR adds functionality to allow users save EQL and ES|QL queries in Prebuilt Rule Customization workflow by displaying a confirmation modal with non critical validation errors (a.k.a warnings). It also refactors confirmation modal usage in rule creation/editing forms for better reusability. ## Screenshots <img width="1909" alt="Screenshot 2024-12-03 at 14 01 36" src="https://github.com/user-attachments/assets/c0054fc1-b52b-400f-80d7-af42391f4e18"> https://github.com/user-attachments/assets/2a20fcfe-ffc0-4547-8621-7ac6873c8dc9 https://github.com/user-attachments/assets/50b5cf5a-ea3f-4c22-a443-b5d4056a92c8 <img width="2541" alt="Screenshot 2024-12-03 at 14 06 29" src="https://github.com/user-attachments/assets/dde3fd60-6c69-4f8e-a65a-837b2319e4ac"> <img width="2552" alt="Screenshot 2024-12-03 at 14 06 51" src="https://github.com/user-attachments/assets/220817a6-991f-4361-88d2-ee3a47a36ad6"> <img width="2555" alt="Screenshot 2024-12-03 at 14 07 52" src="https://github.com/user-attachments/assets/c46fc49c-9ce1-4472-bdea-f9507aa62ece"> <img width="2553" alt="Screenshot 2024-12-03 at 14 08 18" src="https://github.com/user-attachments/assets/92388c56-8644-4c54-8727-b9a73b3497d1"> (cherry picked from commit b0c7a8c)

kibanamachine · 2024-12-06T12:11:53Z

💚 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

…tical validation errors (#202544) (#203233) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution] Allow users to save rule query with non critical validation errors (#202544)](#202544)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Maxim Palenov <maxim.palenov@elastic.co>

…validation errors (elastic#202544) **Addresses:** elastic#171520 ## Summary This PR adds functionality to allow users save EQL and ES|QL queries in Prebuilt Rule Customization workflow by displaying a confirmation modal with non critical validation errors (a.k.a warnings). It also refactors confirmation modal usage in rule creation/editing forms for better reusability. ## Screenshots <img width="1909" alt="Screenshot 2024-12-03 at 14 01 36" src="https://github.com/user-attachments/assets/c0054fc1-b52b-400f-80d7-af42391f4e18"> https://github.com/user-attachments/assets/2a20fcfe-ffc0-4547-8621-7ac6873c8dc9 https://github.com/user-attachments/assets/50b5cf5a-ea3f-4c22-a443-b5d4056a92c8 <img width="2541" alt="Screenshot 2024-12-03 at 14 06 29" src="https://github.com/user-attachments/assets/dde3fd60-6c69-4f8e-a65a-837b2319e4ac"> <img width="2552" alt="Screenshot 2024-12-03 at 14 06 51" src="https://github.com/user-attachments/assets/220817a6-991f-4361-88d2-ee3a47a36ad6"> <img width="2555" alt="Screenshot 2024-12-03 at 14 07 52" src="https://github.com/user-attachments/assets/c46fc49c-9ce1-4472-bdea-f9507aa62ece"> <img width="2553" alt="Screenshot 2024-12-03 at 14 08 18" src="https://github.com/user-attachments/assets/92388c56-8644-4c54-8727-b9a73b3497d1">

maximpn self-assigned this Dec 2, 2024

maximpn marked this pull request as ready for review December 3, 2024 13:14

maximpn requested review from a team as code owners December 3, 2024 13:14

maximpn requested review from dhurley14 and dplumlee December 3, 2024 13:14

maximpn force-pushed the implement-non-blocking-query-validation branch from 743a712 to 8203aa2 Compare December 3, 2024 13:46

dplumlee approved these changes Dec 4, 2024

View reviewed changes

maximpn changed the title ~~[Security Solution] Allow users save rule query with non critical validation errors~~ [Security Solution] Allow users to save rule query with non critical validation errors Dec 4, 2024

maximpn force-pushed the implement-non-blocking-query-validation branch from 2045273 to 754c361 Compare December 4, 2024 21:28

maximpn mentioned this pull request Dec 5, 2024

[Security Solution] Extend form lib with warnings #203097

Open

rylnd approved these changes Dec 5, 2024

View reviewed changes

maximpn added 3 commits December 6, 2024 10:14

implement non blocking validation errors as warnings

8edca70

prepend warnings with a field name

aedded6

remove unused translation keys

a368e58

maximpn added 7 commits December 6, 2024 10:14

add useFormWithWarn description

d818e15

reuse validation errors type

1f81a17

rename modal constant

ac99156

specify clear ownership for use_form_with_warn

1aba3a5

add explicit isAsync state for async validators

957040b

add unit tests

779d55e

improve description comment

cefdd61

maximpn removed the request for review from dhurley14 December 6, 2024 09:17

maximpn added 2 commits December 6, 2024 10:22

rename FormWithWarm to FormWithWarnings

b5e7635

rename non blocking error codes to warning codes

c3e5c91

maximpn force-pushed the implement-non-blocking-query-validation branch from fa0b2ba to c3e5c91 Compare December 6, 2024 09:31

maximpn enabled auto-merge (squash) December 6, 2024 09:31

maximpn merged commit b0c7a8c into elastic:main Dec 6, 2024
8 checks passed

maximpn deleted the implement-non-blocking-query-validation branch December 6, 2024 12:10

kibanamachine mentioned this pull request Dec 6, 2024

[8.x] [Security Solution] Allow users to save rule query with non critical validation errors (#202544) #203233

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Allow users to save rule query with non critical validation errors #202544

[Security Solution] Allow users to save rule query with non critical validation errors #202544

maximpn commented Dec 2, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Dec 3, 2024

elasticmachine commented Dec 3, 2024

elasticmachine commented Dec 3, 2024

dplumlee left a comment •

edited

Loading

dplumlee Dec 4, 2024

dplumlee Dec 4, 2024

maximpn commented Dec 4, 2024

elasticmachine commented Dec 5, 2024

maximpn commented Dec 5, 2024

rylnd left a comment

rylnd Dec 5, 2024

rylnd Dec 5, 2024

maximpn Dec 6, 2024 •

edited

Loading

rylnd Dec 5, 2024

rylnd Dec 5, 2024

rylnd Dec 5, 2024

kibanamachine commented Dec 6, 2024

kibanamachine commented Dec 6, 2024


		import type { FormHook, FormData, ValidationError } from '../../../shared_imports';

		export interface FormHookWithWarn<T extends FormData = FormData, I extends FormData = T>

	export interface FormHookWithWarn<T extends FormData = FormData, I extends FormData = T>
	export interface FormHookWithWarnings<T extends FormData = FormData, I extends FormData = T>

[Security Solution] Allow users to save rule query with non critical validation errors #202544

[Security Solution] Allow users to save rule query with non critical validation errors #202544

Conversation

maximpn commented Dec 2, 2024 • edited by kibanamachine Loading

Summary

Screenshots

elasticmachine commented Dec 3, 2024

elasticmachine commented Dec 3, 2024

elasticmachine commented Dec 3, 2024

dplumlee left a comment • edited Loading

Choose a reason for hiding this comment

dplumlee Dec 4, 2024

Choose a reason for hiding this comment

dplumlee Dec 4, 2024

Choose a reason for hiding this comment

maximpn commented Dec 4, 2024

elasticmachine commented Dec 5, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

maximpn commented Dec 5, 2024

rylnd left a comment

Choose a reason for hiding this comment

rylnd Dec 5, 2024

Choose a reason for hiding this comment

rylnd Dec 5, 2024

Choose a reason for hiding this comment

maximpn Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

rylnd Dec 5, 2024

Choose a reason for hiding this comment

rylnd Dec 5, 2024

Choose a reason for hiding this comment

rylnd Dec 5, 2024

Choose a reason for hiding this comment

kibanamachine commented Dec 6, 2024

kibanamachine commented Dec 6, 2024

💚 All backports created successfully

Questions ?

maximpn commented Dec 2, 2024 •

edited by kibanamachine

Loading

dplumlee left a comment •

edited

Loading

maximpn Dec 6, 2024 •

edited

Loading