[Security Solution] Adds ability to revert prebuilt rules to their base version#223301
[Security Solution] Adds ability to revert prebuilt rules to their base version#223301maximpn merged 28 commits intoelastic:mainfrom
Conversation
|
Cloud deployments require a Github label, please add |
| <TabContentPadding> | ||
| <PerFieldRuleDiffTab | ||
| header={headerCallout} | ||
| ruleDiff={diff} |
There was a problem hiding this comment.
This is the reason I ended up returning the diff field in the GetPrebuiltRuleBaseVersionResponseBody type from the new /base_version endpoint. We can pass it directly to the existing <PerFieldRuleDiffTab /> component without any modification and have it work.
|
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
|
Pinging @elastic/security-solution (Team: SecuritySolution) |
|
Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management) |
| current_version: RuleResponse; | ||
|
|
||
| /** The resulting diff between the base and current versions of the rule */ | ||
| diff: PartialRuleDiff; |
There was a problem hiding this comment.
As an improvement you could return fieldsDiff: Partial<RuleFieldsDiff> and add the necessary fallback fields for example in x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management/components/rule_details/base_version_diff/base_version_flyout.tsx or in the upstream data fetching hook.
| } catch { | ||
| // Error is handled by the mutation's onError callback, so no need to do anything here | ||
| } finally { | ||
| closeFlyout(); |
There was a problem hiding this comment.
Having closeFlyout(); closes the flyout upon errors but it's also a suboptimal UX in case of concurrency control.
The flyout should be closed automatically only in the following cases
- successful prebuilt rule customizations reverting
- base version has disappeared after installing a new prebuilt rules package (the best approach is showing a warning message and disabling the
Revertbutton)
In case either revision or version or both don't match to the expected values users should see a warning message but the flyout shouldn't be closed automatically.
On top of that the diff may update after React query cache expires and it fetches the fresh base version. That base version may be a new one so the diff will be updated. The same happens when someone else edits the rule concurrently.
Implementation like in prebuilt rules upgrade workflow could be used as the reference https://github.com/elastic/kibana/blob/main/x-pack/solutions/security/plugins/security_solution/public/detection_engine/rule_management_ui/components/rules_table/upgrade_prebuilt_rules_table/use_prebuilt_rules_upgrade_state.ts#L74-L120.
There was a problem hiding this comment.
@dplumlee I tested the PR using multiple scenarios
- ✅ reverting prebuilt rule customizations
- ✅ exception lists, actions, non-customizable and runtime fields stay unchanged after customizations have been reverted
- ✅ reverting prebuilt rule customizations works after upgrading the rule
- ✅ "Revert rule" button isn't shown after successful reverting
- ✅ reverting under low-tier license where Prebuilt Rules Customiation is disabled
⚠️ concurrency control- flyout closes automatically after an attempt to revert the customizations when a new prebuilt rules package was installed concurrently
- flyout closes automatically after an attempt to revert the customizations when
revisiondoesn't match - flyout closes automatically after an attempt to revert the customizations when
versiondoesn't match - flyout diff updates happen silently, no notifications are shown
Besides that I left a few comments.
Let's fix the concurrency UI issues then this PR should be good to go.
|
@maximpn pushed up the changes, I think in the #207172 implementation follow-up, I might switch to a static viewing experience to simplify some of these concurrency edge cases. Loading in the initial data and then displaying that it's outdated and letting the user refresh the page themselves seems like a good way to handle it, similar to how its done in github's ui |
maximpn
left a comment
There was a problem hiding this comment.
@dplumlee Thanks for diligently addressing my comments 🙏
As we agreed some of the comments will be addressed in a follow up PR.
I've retested the PR and pushed a little change to auto-close the flyout upon base version disappearing. React Query retains stale data in case of failed requests. Besides that the functionality works fine.
⏳ Build in-progress
History
cc @dplumlee |
|
Starting backport for target branches: 8.19 https://github.com/elastic/kibana/actions/runs/15901009398 |
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
💚 All backports created successfully
Note: Successful backport PRs will be merged automatically after passing CI. Questions ?Please refer to the Backport tool documentation |
…heir base version (#223301) (#225666) # Backport This will backport the following commits from `main` to `8.19`: - [[Security Solution] Adds ability to revert prebuilt rules to their base version (#223301)](#223301) <!--- Backport version: 10.0.1 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sorenlouv/backport) <!--BACKPORT [{"author":{"name":"Davis Plumlee","email":"56367316+dplumlee@users.noreply.github.com"},"sourceCommit":{"committedDate":"2025-06-26T11:48:37Z","message":"[Security Solution] Adds ability to revert prebuilt rules to their base version (#223301)\n\n## Summary\n\nTicket: https://github.com/elastic/kibana/issues/215506\n\nAdds ability to revert prebuilt rules to their base version. \n\nImplements following endpoints:\n\n- `GET /internal/detection_engine/prebuilt_rules/base_version`\n- `POST /internal/detection_engine/prebuilt_rules/revert`\n\nAllows users to revert their customized prebuilt rules to the original Elastic versions. This also implements a rule diff flyout on the rule details page so users can see which fields are customized and would be changed on reversion.\n\n### Screenshots\n\n#### Rule base version flyout\n\n\n\n#### Disabled when cannot find base version\n\n\n\n### Checklist\n\nCheck the PR satisfies following conditions. \n\nReviewers should verify this PR satisfies this list as well.\n\n- [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n- [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials\n- [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","sha":"d532ff490aeb4f3b27f0cb733841f195b08696c2","branchLabelMapping":{"^v9.1.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:enhancement","Team:Detections and Resp","Team: SecuritySolution","Team:Detection Rule Management","Feature:Prebuilt Detection Rules","ci:cloud-deploy","ci:project-deploy-security","backport:version","v9.1.0","v8.19.0"],"title":"[Security Solution] Adds ability to revert prebuilt rules to their base version","number":223301,"url":"https://github.com/elastic/kibana/pull/223301","mergeCommit":{"message":"[Security Solution] Adds ability to revert prebuilt rules to their base version (#223301)\n\n## Summary\n\nTicket: https://github.com/elastic/kibana/issues/215506\n\nAdds ability to revert prebuilt rules to their base version. \n\nImplements following endpoints:\n\n- `GET /internal/detection_engine/prebuilt_rules/base_version`\n- `POST /internal/detection_engine/prebuilt_rules/revert`\n\nAllows users to revert their customized prebuilt rules to the original Elastic versions. This also implements a rule diff flyout on the rule details page so users can see which fields are customized and would be changed on reversion.\n\n### Screenshots\n\n#### Rule base version flyout\n\n\n\n#### Disabled when cannot find base version\n\n\n\n### Checklist\n\nCheck the PR satisfies following conditions. \n\nReviewers should verify this PR satisfies this list as well.\n\n- [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n- [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials\n- [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","sha":"d532ff490aeb4f3b27f0cb733841f195b08696c2"}},"sourceBranch":"main","suggestedTargetBranches":["8.19"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/223301","number":223301,"mergeCommit":{"message":"[Security Solution] Adds ability to revert prebuilt rules to their base version (#223301)\n\n## Summary\n\nTicket: https://github.com/elastic/kibana/issues/215506\n\nAdds ability to revert prebuilt rules to their base version. \n\nImplements following endpoints:\n\n- `GET /internal/detection_engine/prebuilt_rules/base_version`\n- `POST /internal/detection_engine/prebuilt_rules/revert`\n\nAllows users to revert their customized prebuilt rules to the original Elastic versions. This also implements a rule diff flyout on the rule details page so users can see which fields are customized and would be changed on reversion.\n\n### Screenshots\n\n#### Rule base version flyout\n\n\n\n#### Disabled when cannot find base version\n\n\n\n### Checklist\n\nCheck the PR satisfies following conditions. \n\nReviewers should verify this PR satisfies this list as well.\n\n- [x] Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n- [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials\n- [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","sha":"d532ff490aeb4f3b27f0cb733841f195b08696c2"}},{"branch":"8.19","label":"v8.19.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}] BACKPORT-->
|
Starting backport for target branches: 8.19, 9.1 https://github.com/elastic/kibana/actions/runs/16021022490 |
💔 All backports failed
Manual backportTo create the backport manually run: Questions ?Please refer to the Backport tool documentation |
Summary
Ticket: #215506
Adds ability to revert prebuilt rules to their base version.
Implements following endpoints:
GET /internal/detection_engine/prebuilt_rules/base_versionPOST /internal/detection_engine/prebuilt_rules/revertAllows users to revert their customized prebuilt rules to the original Elastic versions. This also implements a rule diff flyout on the rule details page so users can see which fields are customized and would be changed on reversion.
Screenshots
Rule base version flyout
Disabled when cannot find base version
Checklist
Check the PR satisfies following conditions.
Reviewers should verify this PR satisfies this list as well.