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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] Add validation error description on prebuilt rule editing #191832

Open
Tracked by #180407
, #174168
...
e40pud opened this issue Aug 30, 2024 · 8 comments
Open
Tracked by #180407
, #174168
...

[Security Solution] Add validation error description on prebuilt rule editing #191832

e40pud opened this issue Aug 30, 2024 · 8 comments
Assignees
@approksiu @ARWNightingale
Labels
8.18 candidate Feature:Rule Edit Security Solution Detection Rule Editing workflow needs design needs product Team:Detection Engine Security Solution Detection Engine Area Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc.

Comments

@e40pud
Copy link
Contributor

e40pud commented Aug 30, 2024

While working on #180407 and adding this PR #191487, @vitaliidm brought up the confusion with rule validation errors while editing prebuilt rules:

Right now, we show only a short message at the top of the page about the existing error on some tabs without any clarification about the source of the error. In the example below we show You have an invalid input in this tab: Definition:

Image

Since users still cannot edit prebuilt rules, they cannot navigate to the mentioned tabs and see the reason of the error. We believe this may confuse our users and not the best UX.

Possible solutions:

  1. We can show actual error description in the error panel
Sorry

You have an invalid input in this tab: Definition

Query bar: "Found 2 problems\nline 39:11: Unknown column [destination.ip]\nline 45:9: Unknown column [dns.question.name]"
  1. After these changes [Security Solution] Editing rules independently of source data (#180407) #191487 we show confirmation modal to the user where we could add error description. We decided to avoid it here, but maybe we could re-evaluate this.

Image

cc @approksiu @banderror @yctercero
@e40pud e40pud added discuss Feature:Rule Edit Security Solution Detection Rule Editing workflow needs product Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Team:Detection Engine Security Solution Detection Engine Area Team:Detections and Resp Security Detection Response Team labels Aug 30, 2024
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-engine (Team:Detection Engine)

@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@e40pud e40pud changed the title [Security Solution] Add validation error description on rule creation/editing [Security Solution] Add validation error description on prebuilt rule editing Aug 30, 2024
This was referenced Aug 30, 2024
Open
Merged
@banderror
Copy link
Contributor

banderror commented Sep 10, 2024 •
edited
Loading

@approksiu @e40pud /cc @peluja1012 @yctercero for resourcing

While working on #180407 and adding this PR #191487, @vitaliidm brought up the confusion with rule validation errors while editing prebuilt rules

I completely agree with @vitaliidm and I propose to address this issue in two stages:

  1. Stage 1.
    • We revert to the original implementation that @e40pud had. Specifically, we return back the validation errors to the modal window. An example of this modal is in the ticket description.
    • Additionally, on the Rule Editing page and only for prebuilt rules we: 1) hide the callout that says "You have an invalid input in this tab: ...", and 2) we don't show the modal if there are any data validation errors. We shouldn't show this modal and this callout until we release the prebuilt rule customization feature.
    • This would be worked by @e40pud and hopefully it will take just a few hours of work.
    • This is urgent.
  2. Stage 2.
    • We design a proper UI/UX for the modal and the callout. Or maybe use different UI patterns.
    • This would be worked by @ARWNightingale and then by an engineer from the Rule Management team.
    • This is not urgent and we can get back to stage 2 closer to the end of Milestone 3 development.

@e40pud
Copy link
Contributor Author

e40pud commented Sep 12, 2024

@banderror what would be an expected behaviour in case of editing a prebuilt rule which has a syntax error (or any other potential error which counts as a blocking error). Do we still want to show error callout "You have an invalid input in this tab: ..."?

@banderror
Copy link
Contributor

banderror commented Sep 12, 2024 •
edited
Loading

@e40pud Great point, I think it doesn't make sense to validate any fields that are under the disabled tabs on the Rule Editing page. The user won't be able to fix any validation errors there anyway. If we can't disable the validation for the disabled tabs itself, we shouldn't show any callouts or modals about the errors there. In other words, we should only validate the Actions tab. Again, this logic should only be applicable to prebuilt rules.

e40pud added a commit to e40pud/kibana that referenced this issue Sep 12, 2024
@e40pud
[Security Solution] Add validation error description on prebuilt rule…
1aa5901 
… editing elastic#191832
@e40pud e40pud mentioned this issue Sep 12, 2024
Merged
1 task
e40pud added a commit that referenced this issue Sep 13, 2024
@e40pud
[Security Solution] Add validation error description on prebuilt rule…
c937e95 
… editing (#191832) (#192683)

## Summary

Partially addressed #191832

With these changes:
- We revert to the
#180407 (comment).
Specifically, we return back the validation errors to the modal window.
An example of this modal is in the ticket description.
- Additionally, on the Rule Editing page and **only for prebuilt rules**
we: 1) hide the callout that says "You have an invalid input in this
tab: ...", and 2) we don't show the modal if there are any data
validation errors. We shouldn't show this modal and this callout until
we release the prebuilt rule customization feature. 3) We will only
validate the Actions tab.
- Fix MKI flaky cypress tests introduced in
#191487
([1](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/b1f442af-db44-8029-a9fb-7e3d988303b3?branch=main),
[2](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/995655b6-ae70-86fd-b483-c65846cd8d66?branch=main),
[3](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/02318f5c-6ca1-8779-a5a4-60f52a55b344?branch=main)).
All three tests are failing due to missing
`[data-test-subj="eqlRuleType"]` element. After checking and comparing
my tests to other similar tests in the file, the only difference that
I've found was extra `login();` call. Thus removing those.

Here is the screen recording showing the new behaviour for prebuilt
rules. The has missing data source query validation error, though we do
not show it and allow user just to save the rule. Only Actions tab is
validated on rule save action.


https://github.com/user-attachments/assets/ce968f51-1a53-41b2-ad06-1b31dec085a6


### Checklist

Delete any items that are not applicable to this PR.

- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
* [Detection Engine -
Cypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6925)
(100 ESS & 100 Serverless)
* [Rule Management -
Cypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6926)
(100 ESS & 100 Serverless)
* [Prebuilt Rules -
Cypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6927)
(100 ESS & 100 Serverless)
@e40pud
Copy link
Contributor Author

e40pud commented Sep 13, 2024

@banderror the Stage 1 of the list above was done and merged in this PR.

kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Sep 13, 2024
@e40pud
[Security Solution] Add validation error description on prebuilt rule…
217c9b1 
… editing (elastic#191832) (elastic#192683)

## Summary

Partially addressed elastic#191832

With these changes:
- We revert to the
elastic#180407 (comment).
Specifically, we return back the validation errors to the modal window.
An example of this modal is in the ticket description.
- Additionally, on the Rule Editing page and **only for prebuilt rules**
we: 1) hide the callout that says "You have an invalid input in this
tab: ...", and 2) we don't show the modal if there are any data
validation errors. We shouldn't show this modal and this callout until
we release the prebuilt rule customization feature. 3) We will only
validate the Actions tab.
- Fix MKI flaky cypress tests introduced in
elastic#191487
([1](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/b1f442af-db44-8029-a9fb-7e3d988303b3?branch=main),
[2](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/995655b6-ae70-86fd-b483-c65846cd8d66?branch=main),
[3](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/02318f5c-6ca1-8779-a5a4-60f52a55b344?branch=main)).
All three tests are failing due to missing
`[data-test-subj="eqlRuleType"]` element. After checking and comparing
my tests to other similar tests in the file, the only difference that
I've found was extra `login();` call. Thus removing those.

Here is the screen recording showing the new behaviour for prebuilt
rules. The has missing data source query validation error, though we do
not show it and allow user just to save the rule. Only Actions tab is
validated on rule save action.

https://github.com/user-attachments/assets/ce968f51-1a53-41b2-ad06-1b31dec085a6

### Checklist

Delete any items that are not applicable to this PR.

- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
* [Detection Engine -
Cypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6925)
(100 ESS & 100 Serverless)
* [Rule Management -
Cypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6926)
(100 ESS & 100 Serverless)
* [Prebuilt Rules -
Cypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6927)
(100 ESS & 100 Serverless)

(cherry picked from commit c937e95)
kibanamachine added a commit that referenced this issue Sep 13, 2024
@kibanamachine @e40pud
[8.x] [Security Solution] Add validation error description on prebuil…
0457580 
…t rule editing (#191832) (#192683) (#192819)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution] Add validation error description on prebuilt rule
editing (#191832)
(#192683)](#192683)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Ievgen
Sorokopud","email":"[email protected]"},"sourceCommit":{"committedDate":"2024-09-13T08:37:39Z","message":"[Security
Solution] Add validation error description on prebuilt rule editing
(#191832) (#192683)\n\n## Summary\r\n\r\nPartially addressed
https://github.com/elastic/kibana/issues/191832\r\n\r\nWith these
changes:\r\n- We revert to
the\r\nhttps://github.com//issues/180407#issuecomment-2312891214.\r\nSpecifically,
we return back the validation errors to the modal window.\r\nAn example
of this modal is in the ticket description.\r\n- Additionally, on the
Rule Editing page and **only for prebuilt rules**\r\nwe: 1) hide the
callout that says \"You have an invalid input in this\r\ntab: ...\", and
2) we don't show the modal if there are any data\r\nvalidation errors.
We shouldn't show this modal and this callout until\r\nwe release the
prebuilt rule customization feature. 3) We will only\r\nvalidate the
Actions tab.\r\n- Fix MKI flaky cypress tests introduced
in\r\nhttps://github.com//pull/191487\r\n([1](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/b1f442af-db44-8029-a9fb-7e3d988303b3?branch=main),\r\n[2](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/995655b6-ae70-86fd-b483-c65846cd8d66?branch=main),\r\n[3](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/02318f5c-6ca1-8779-a5a4-60f52a55b344?branch=main)).\r\nAll
three tests are failing due to
missing\r\n`[data-test-subj=\"eqlRuleType\"]` element. After checking
and comparing\r\nmy tests to other similar tests in the file, the only
difference that\r\nI've found was extra `login();` call. Thus removing
those.\r\n\r\nHere is the screen recording showing the new behaviour for
prebuilt\r\nrules. The has missing data source query validation error,
though we do\r\nnot show it and allow user just to save the rule. Only
Actions tab is\r\nvalidated on rule save
action.\r\n\r\n\r\nhttps://github.com/user-attachments/assets/ce968f51-1a53-41b2-ad06-1b31dec085a6\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n* [Detection Engine
-\r\nCypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6925)\r\n(100
ESS & 100 Serverless)\r\n* [Rule Management
-\r\nCypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6926)\r\n(100
ESS & 100 Serverless)\r\n* [Prebuilt Rules
-\r\nCypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6927)\r\n(100
ESS & 100
Serverless)","sha":"c937e95e3137821b510fa480ee28f0cf3afb85ad","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:
SecuritySolution","ci:cloud-deploy","Team:Detection
Engine","ci:project-deploy-security","v8.16.0"],"title":"[Security
Solution] Add validation error description on prebuilt rule editing
(#191832)","number":192683,"url":"https://github.com/elastic/kibana/pull/192683","mergeCommit":{"message":"[Security
Solution] Add validation error description on prebuilt rule editing
(#191832) (#192683)\n\n## Summary\r\n\r\nPartially addressed
https://github.com/elastic/kibana/issues/191832\r\n\r\nWith these
changes:\r\n- We revert to
the\r\nhttps://github.com//issues/180407#issuecomment-2312891214.\r\nSpecifically,
we return back the validation errors to the modal window.\r\nAn example
of this modal is in the ticket description.\r\n- Additionally, on the
Rule Editing page and **only for prebuilt rules**\r\nwe: 1) hide the
callout that says \"You have an invalid input in this\r\ntab: ...\", and
2) we don't show the modal if there are any data\r\nvalidation errors.
We shouldn't show this modal and this callout until\r\nwe release the
prebuilt rule customization feature. 3) We will only\r\nvalidate the
Actions tab.\r\n- Fix MKI flaky cypress tests introduced
in\r\nhttps://github.com//pull/191487\r\n([1](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/b1f442af-db44-8029-a9fb-7e3d988303b3?branch=main),\r\n[2](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/995655b6-ae70-86fd-b483-c65846cd8d66?branch=main),\r\n[3](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/02318f5c-6ca1-8779-a5a4-60f52a55b344?branch=main)).\r\nAll
three tests are failing due to
missing\r\n`[data-test-subj=\"eqlRuleType\"]` element. After checking
and comparing\r\nmy tests to other similar tests in the file, the only
difference that\r\nI've found was extra `login();` call. Thus removing
those.\r\n\r\nHere is the screen recording showing the new behaviour for
prebuilt\r\nrules. The has missing data source query validation error,
though we do\r\nnot show it and allow user just to save the rule. Only
Actions tab is\r\nvalidated on rule save
action.\r\n\r\n\r\nhttps://github.com/user-attachments/assets/ce968f51-1a53-41b2-ad06-1b31dec085a6\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n* [Detection Engine
-\r\nCypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6925)\r\n(100
ESS & 100 Serverless)\r\n* [Rule Management
-\r\nCypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6926)\r\n(100
ESS & 100 Serverless)\r\n* [Prebuilt Rules
-\r\nCypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6927)\r\n(100
ESS & 100
Serverless)","sha":"c937e95e3137821b510fa480ee28f0cf3afb85ad"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/192683","number":192683,"mergeCommit":{"message":"[Security
Solution] Add validation error description on prebuilt rule editing
(#191832) (#192683)\n\n## Summary\r\n\r\nPartially addressed
https://github.com/elastic/kibana/issues/191832\r\n\r\nWith these
changes:\r\n- We revert to
the\r\nhttps://github.com//issues/180407#issuecomment-2312891214.\r\nSpecifically,
we return back the validation errors to the modal window.\r\nAn example
of this modal is in the ticket description.\r\n- Additionally, on the
Rule Editing page and **only for prebuilt rules**\r\nwe: 1) hide the
callout that says \"You have an invalid input in this\r\ntab: ...\", and
2) we don't show the modal if there are any data\r\nvalidation errors.
We shouldn't show this modal and this callout until\r\nwe release the
prebuilt rule customization feature. 3) We will only\r\nvalidate the
Actions tab.\r\n- Fix MKI flaky cypress tests introduced
in\r\nhttps://github.com//pull/191487\r\n([1](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/b1f442af-db44-8029-a9fb-7e3d988303b3?branch=main),\r\n[2](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/995655b6-ae70-86fd-b483-c65846cd8d66?branch=main),\r\n[3](https://buildkite.com/organizations/elastic/analytics/suites/serverless-mki-cypress-detection-engine/tests/02318f5c-6ca1-8779-a5a4-60f52a55b344?branch=main)).\r\nAll
three tests are failing due to
missing\r\n`[data-test-subj=\"eqlRuleType\"]` element. After checking
and comparing\r\nmy tests to other similar tests in the file, the only
difference that\r\nI've found was extra `login();` call. Thus removing
those.\r\n\r\nHere is the screen recording showing the new behaviour for
prebuilt\r\nrules. The has missing data source query validation error,
though we do\r\nnot show it and allow user just to save the rule. Only
Actions tab is\r\nvalidated on rule save
action.\r\n\r\n\r\nhttps://github.com/user-attachments/assets/ce968f51-1a53-41b2-ad06-1b31dec085a6\r\n\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [ ] [Flaky
Test\r\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\r\nused on any tests changed\r\n* [Detection Engine
-\r\nCypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6925)\r\n(100
ESS & 100 Serverless)\r\n* [Rule Management
-\r\nCypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6926)\r\n(100
ESS & 100 Serverless)\r\n* [Prebuilt Rules
-\r\nCypress](https://buildkite.com/elastic/kibana-flaky-test-suite-runner/builds/6927)\r\n(100
ESS & 100
Serverless)","sha":"c937e95e3137821b510fa480ee28f0cf3afb85ad"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Ievgen Sorokopud <[email protected]>
@banderror banderror mentioned this issue Sep 13, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@approksiu approksiu

@ARWNightingale ARWNightingale

Labels
8.18 candidate Feature:Rule Edit Security Solution Detection Rule Editing workflow needs design needs product Team:Detection Engine Security Solution Detection Engine Area Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@e40pud @banderror @elasticmachine @approksiu @ARWNightingale