-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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.
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
[Security Solution] Prebuilt rule installation / upgrade flyout #163304
Conversation
23dd003
to
d639806
Compare
7bf4be5
to
8ee5ffd
Compare
Pinging @elastic/security-solution (Team: SecuritySolution) |
Pinging @elastic/security-detections-response (Team:Detections and Resp) |
Kibana crashes when switching to a rule without the "Investigation Guide" section: Screen.Recording.2023-08-14.at.11.37.15.mov |
@nikitaindik, could you please fix the paddings and spacings so that they match the original design more closely? |
@nikitaindik On the rule update page, we display the currently installed version of a rule in the update flyout, not the updated one. Is this intended? It seems the flyout should show the forthcoming version with all updates instead of the existing rule state. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't test locally, I just reviewed 2 files owned by @elastic/security-detection-engine
Left some comments, which can be addressed after FF
x-pack/plugins/security_solution/common/detection_engine/diffable_rule_to_rule_response.ts
Show resolved
Hide resolved
x-pack/plugins/security_solution/common/detection_engine/diffable_rule_to_rule_response.ts
Show resolved
Hide resolved
x-pack/plugins/security_solution/common/api/detection_engine/model/rule_schema/rule_schemas.ts
Show resolved
Hide resolved
@nikitaindik Could you rebase the PR? It's getting too far behind |
* (e.g. when the rule is already being installed or when the table is being refetched) | ||
* | ||
**/ | ||
isFlyoutInstallButtonDisabled: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Data contexts should not be aware of any UI elements that are consuming them. We should choose a name for this prop that's both neutral and generic, so it can be used in components beyond just the button inside a flyout.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ Maybe we could call it canPreviewedRuleBeInstalled
or something like that.
@@ -77,6 +88,8 @@ export interface AddPrebuiltRulesTableActions { | |||
installSelectedRules: () => void; | |||
setFilterOptions: Dispatch<SetStateAction<AddPrebuiltRulesTableFilterOptions>>; | |||
selectRules: (rules: RuleInstallationInfoForReview[]) => void; | |||
openFlyoutForRuleId: (ruleId: RuleSignatureId) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here: openFlyoutForRuleId
-> openRulePreview
@@ -118,6 +119,8 @@ export const UpgradePrebuiltRulesTable = React.memo(() => { | |||
data-test-subj="rules-upgrades-table" | |||
columns={rulesColumns} | |||
/> | |||
|
|||
<UpgradePrebuiltRulesFlyout /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's move this component to the context provider. The table should not be aware of the flyout.
@@ -95,6 +97,8 @@ export const AddPrebuiltRulesTable = React.memo(() => { | |||
data-test-subj="add-prebuilt-rules-table" | |||
columns={rulesColumns} | |||
/> | |||
|
|||
<AddPrebuiltRulesFlyout /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same: Let's move this component to the context provider. The table should not be aware of the flyout.
@@ -77,6 +88,8 @@ export interface AddPrebuiltRulesTableActions { | |||
installSelectedRules: () => void; | |||
setFilterOptions: Dispatch<SetStateAction<AddPrebuiltRulesTableFilterOptions>>; | |||
selectRules: (rules: RuleInstallationInfoForReview[]) => void; | |||
openFlyoutForRuleId: (ruleId: RuleSignatureId) => void; | |||
closeFlyout: () => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to make properties like flyoutRule
and closeFlyout
internal for this context. Components outside don't need to know anything about how a rule is displayed. We can encapsulate this knowledge inside the context and expose only a generic method, like openRulePreview(ruleId)
.
actionButtonLabel: string; | ||
isActionButtonDisabled: boolean; | ||
onActionButtonClick: (ruleId: string) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed on a zoom call, let's replace these props with a more generic action: React.ReactNode
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to reuse the extracted components on the rule details page as well.
export const BUILDING_BLOCK_FIELD_LABEL = i18n.translate( | ||
'xpack.securitySolution.detectionEngine.ruleDetails.buildingBlockFieldLabel', | ||
{ | ||
defaultMessage: 'Building block', | ||
} | ||
); | ||
|
||
export const BUILDING_BLOCK_FIELD_DESCRIPTION = i18n.translate( | ||
'xpack.securitySolution.detectionEngine.ruleDetails.buildingBlockFieldDescription', | ||
{ | ||
defaultMessage: 'All generated alerts will be marked as "building block" alerts', | ||
} | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we reuse the existing translations for all these fields?
invariant(ruleToShowInFlyout, `Rule with id ${ruleId} not found`); | ||
if (ruleToShowInFlyout) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ruleToShowInFlyout
cannot be undefined
after checking the invariant on the previous line.
} | ||
`; | ||
|
||
const StyledEuiTabbedContent = styled(EuiTabbedContent)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a full copy of StyledEuiTabbedContent
from components/event_details/event_details.tsx
. Please reuse components whenever possible instead of copying them.
💛 Build succeeded, but was flaky
Failed CI StepsTest Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @nikitaindik |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your effort, Nikita 👍 Our users are really going to appreciate this new feature 🎉
I've gone through the PR locally, and the new workflow works smoothly. Let's proceed with the merge. Any remaining feedback isn't urgent and can be addressed in a follow-up PR. Could you summarize the remaining tasks somewhere, please?
Also, I'd love to dive deeper into the context usage and discuss potential improvements.
…tic#163304) **Addresses:** elastic#162334 ## Summary This PR adds a flyout for viewing a prebuilt rule before installing or updating it. The flyout can be opened by clicking on a rule title within "Add Elastic Rules" page and within "Rule Updates" tab of the Rule Managament table. I plan to add tests and do minor visual tweaks after the FF. <img width="1269" alt="Screenshot 2023-08-14 at 03 59 30" src="https://github.com/elastic/kibana/assets/15949146/c8200ff8-fbe2-445a-a03e-3545ea77f750"> An additional goal of these changes was to create lightweight reusable components for rule details sections ("About", "Definition", "Schedule") and for rule properties, so that these can later be reused in other flyouts within the Security Solution, on MITRE ATT&CK™ overview page and potentially on the Rule Details page. These reusable section components are basically copy-pasted components from the Rule Details page that were refactored to remove the dependence from the form schema, ### Checklist Delete any items that are not applicable to this PR. - [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/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [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 - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @nikitaindik, I'm a bit late with this review, but still wanted to share some suggestions I had after going through the diff.
I also tested how the flyout works in both tables, I think it looks great! One thing I noticed is that it feels like it takes more than 100ms to render it for the first time (when it's closed), so maybe we could also look into improving the rendering performance.
Thank you for your efforts!
export const useRuleDetailsFlyout = ( | ||
rules: DiffableRule[] | ||
): RuleDetailsFlyoutState & RuleDetailsFlyoutActions => { | ||
const [flyoutRule, setFlyoutRule] = React.useState<RuleInstallationInfoForReview | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hook, given it knows about RuleInstallationInfoForReview
, either should not be here in the rule_management
subdomain, or should be somehow rewritten to remove this dependency. I lean towards just moving it closer to the rule installation table's React context.
Same for the upgrade table's context. But I think there we'd need a slightly modified version of this function (see my next comment about using ruleId: RuleSignatureId
).
const openFlyoutForRuleId = useCallback( | ||
(ruleId: RuleSignatureId) => { | ||
const ruleToShowInFlyout = rules.find((rule) => rule.rule_id === ruleId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the only way to find rules that are not installed yet - they only have this RuleSignatureId
. For the rule upgrade table, I think a better way would be finding rules by their RuleObjectId
. Signature ids sometimes can be duplicated in multiple rules.
Also I think a hook for the upgrade table shouldn't depend on the RuleInstallationInfoForReview
type. There's a RuleUpgradeInfoForReview
type which is used in UpgradePrebuiltRulesTableContextProvider
.
return null; | ||
} | ||
|
||
const ruleResponse: Partial<RuleResponse> = diffableRuleToRuleResponse(flyoutRule); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be memoized? The conversion could be expensive in terms of rendering time -- if it's tens of milliseconds it would be perceivable by users.
I know we're going to change the two API endpoints to return RuleResponse
, so no conversion will be needed, but still wanted to pay attention to this.
* (e.g. when the rule is already being installed or when the table is being refetched) | ||
* | ||
**/ | ||
isFlyoutInstallButtonDisabled: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ Maybe we could call it canPreviewedRuleBeInstalled
or something like that.
) **Addresses:** #162334 ## Summary This PR adds a flyout for viewing a prebuilt rule before installing or updating it. The flyout can be opened by clicking on a rule title within "Add Elastic Rules" page and within "Rule Updates" tab of the Rule Managament table. I plan to add tests and do minor visual tweaks after the FF. <img width="1269" alt="Screenshot 2023-08-14 at 03 59 30" src="https://github.com/elastic/kibana/assets/15949146/c8200ff8-fbe2-445a-a03e-3545ea77f750"> An additional goal of these changes was to create lightweight reusable components for rule details sections ("About", "Definition", "Schedule") and for rule properties, so that these can later be reused in other flyouts within the Security Solution, on MITRE ATT&CK™ overview page and potentially on the Rule Details page. These reusable section components are basically copy-pasted components from the Rule Details page that were refactored to remove the dependence from the form schema, ### Checklist Delete any items that are not applicable to this PR. - [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/packages/kbn-i18n/README.md) - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [ ] [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 - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)
…ovements (#164179) **Addresses: #162334 **Base PR: #163304 <img width="1177" alt="Screenshot 2023-08-24 at 04 09 07" src="https://github.com/elastic/kibana/assets/15949146/73ac6726-69d4-4c46-bb16-da704a02aba5"> ## Summary This is a follow-up refactoring and bugfix PR to improve the prebuilt rules flyout. Base PR: #163304 #### Changes - [x] Tweak UI so that it matches the design more closely. [Design](https://www.figma.com/file/gLHm8LpTtSkAUQHrkG3RHU/%5B8.7%5D-%5BRules%5D-Rule-Immutability%2FCustomization?type=design&node-id=3563-612771&mode=design&t=yqZ6LI0vAjbir9xc-0) (external). - [x] Rewrite preview installation and upgrade API endpoints to respond with `RuleResponse` instead of `DiffableRule` - [x] Revert some changes introduced by this [PR](#163304) - [x] Revert exports in `x-pack/plugins/security_solution/common/api/detection_engine/model/rule_schema/rule_schemas.ts` - [x] Delete `x-pack/plugins/security_solution/common/detection_engine/diffable_rule_to_rule_response.ts` - [x] Make the data contexts unaware of any UI elements that are consuming them - [x] Move rendering of specialized flyout components into to the context provider so that the table is unaware of the flyout. - [x] Make "flyoutRule" and "closeFlyout" internal to the context. Components outside don't need to know anything about how a rule is displayed. We can encapsulate this knowledge inside the context and expose only a generic method, like openRulePreview(ruleId) - [x] Remove unnecessary checks after using "invariant" - [x] Make sure query, timeline template and all the other fields are shown in the flyout. Compare each rule in a flyout with the Rule Details to ensure that all fields are in place. - [x] Remove the enable / disable switch machine learning job UI switch element - [x] Add custom highlighted fields to the flyout ([comment](#163235 (comment))) ### Checklist Delete any items that are not applicable to this PR. - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials. [Docs ticket](elastic/security-docs#3798) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)
…ovements (elastic#164179) **Addresses: elastic#162334 **Base PR: elastic#163304 <img width="1177" alt="Screenshot 2023-08-24 at 04 09 07" src="https://github.com/elastic/kibana/assets/15949146/73ac6726-69d4-4c46-bb16-da704a02aba5"> ## Summary This is a follow-up refactoring and bugfix PR to improve the prebuilt rules flyout. Base PR: elastic#163304 #### Changes - [x] Tweak UI so that it matches the design more closely. [Design](https://www.figma.com/file/gLHm8LpTtSkAUQHrkG3RHU/%5B8.7%5D-%5BRules%5D-Rule-Immutability%2FCustomization?type=design&node-id=3563-612771&mode=design&t=yqZ6LI0vAjbir9xc-0) (external). - [x] Rewrite preview installation and upgrade API endpoints to respond with `RuleResponse` instead of `DiffableRule` - [x] Revert some changes introduced by this [PR](elastic#163304) - [x] Revert exports in `x-pack/plugins/security_solution/common/api/detection_engine/model/rule_schema/rule_schemas.ts` - [x] Delete `x-pack/plugins/security_solution/common/detection_engine/diffable_rule_to_rule_response.ts` - [x] Make the data contexts unaware of any UI elements that are consuming them - [x] Move rendering of specialized flyout components into to the context provider so that the table is unaware of the flyout. - [x] Make "flyoutRule" and "closeFlyout" internal to the context. Components outside don't need to know anything about how a rule is displayed. We can encapsulate this knowledge inside the context and expose only a generic method, like openRulePreview(ruleId) - [x] Remove unnecessary checks after using "invariant" - [x] Make sure query, timeline template and all the other fields are shown in the flyout. Compare each rule in a flyout with the Rule Details to ensure that all fields are in place. - [x] Remove the enable / disable switch machine learning job UI switch element - [x] Add custom highlighted fields to the flyout ([comment](elastic#163235 (comment))) ### Checklist Delete any items that are not applicable to this PR. - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials. [Docs ticket](elastic/security-docs#3798) - [x] Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US)) - [x] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server)) - [x] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers) (cherry picked from commit c115f5d) # Conflicts: # x-pack/plugins/security_solution/server/lib/detection_engine/prebuilt_rules/api/review_rule_installation/review_rule_installation_route.ts
…ut improvements (#164179) (#164897) # Backport This will backport the following commits from `main` to `8.10`: - [[Security Solution] Prebuilt rules installation / upgrade flyout improvements (#164179)](#164179) <!--- Backport version: 8.9.8 --> ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport) <!--BACKPORT [{"author":{"name":"Nikita Indik","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-08-25T19:47:13Z","message":"[Security Solution] Prebuilt rules installation / upgrade flyout improvements (#164179)\n\n**Addresses: https://github.com/elastic/kibana/issues/162334**\r\n**Base PR: https://github.com/elastic/kibana/pull/163304**\r\n\r\n<img width=\"1177\" alt=\"Screenshot 2023-08-24 at 04 09 07\"\r\nsrc=\"https://github.com/elastic/kibana/assets/15949146/73ac6726-69d4-4c46-bb16-da704a02aba5\">\r\n\r\n## Summary\r\n\r\nThis is a follow-up refactoring and bugfix PR to improve the prebuilt\r\nrules flyout. Base PR: #163304\r\n\r\n#### Changes\r\n- [x] Tweak UI so that it matches the design more closely.\r\n[Design](https://www.figma.com/file/gLHm8LpTtSkAUQHrkG3RHU/%5B8.7%5D-%5BRules%5D-Rule-Immutability%2FCustomization?type=design&node-id=3563-612771&mode=design&t=yqZ6LI0vAjbir9xc-0)\r\n(external).\r\n- [x] Rewrite preview installation and upgrade API endpoints to respond\r\nwith `RuleResponse` instead of `DiffableRule`\r\n- [x] Revert some changes introduced by this\r\n[PR](https://github.com/elastic/kibana/pull/163304)\r\n- [x] Revert exports in\r\n`x-pack/plugins/security_solution/common/api/detection_engine/model/rule_schema/rule_schemas.ts`\r\n- [x] Delete\r\n`x-pack/plugins/security_solution/common/detection_engine/diffable_rule_to_rule_response.ts`\r\n- [x] Make the data contexts unaware of any UI elements that are\r\nconsuming them\r\n- [x] Move rendering of specialized flyout components into to the\r\ncontext provider so that the table is unaware of the flyout.\r\n- [x] Make \"flyoutRule\" and \"closeFlyout\" internal to the context.\r\nComponents outside don't need to know anything about how a rule is\r\ndisplayed. We can encapsulate this knowledge inside the context and\r\nexpose only a generic method, like openRulePreview(ruleId)\r\n - [x] Remove unnecessary checks after using \"invariant\"\r\n- [x] Make sure query, timeline template and all the other fields are\r\nshown in the flyout. Compare each rule in a flyout with the Rule Details\r\nto ensure that all fields are in place.\r\n- [x] Remove the enable / disable switch machine learning job UI switch\r\nelement\r\n- [x] Add custom highlighted fields to the flyout\r\n([comment](https://github.com/elastic/kibana/pull/163235#discussion_r1293821203))\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials. [Docs\r\nticket](https://github.com/elastic/security-docs/issues/3798)\r\n- [x] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [x] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)","sha":"c115f5d3d6f580b195e823c9e948f7b1daf8fddc","branchLabelMapping":{"^v8.11.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Detections and Resp","Team: SecuritySolution","Team:Detection Rule Management","Feature:Prebuilt Detection Rules","v8.10.0","v8.11.0"],"number":164179,"url":"https://github.com/elastic/kibana/pull/164179","mergeCommit":{"message":"[Security Solution] Prebuilt rules installation / upgrade flyout improvements (#164179)\n\n**Addresses: https://github.com/elastic/kibana/issues/162334**\r\n**Base PR: https://github.com/elastic/kibana/pull/163304**\r\n\r\n<img width=\"1177\" alt=\"Screenshot 2023-08-24 at 04 09 07\"\r\nsrc=\"https://github.com/elastic/kibana/assets/15949146/73ac6726-69d4-4c46-bb16-da704a02aba5\">\r\n\r\n## Summary\r\n\r\nThis is a follow-up refactoring and bugfix PR to improve the prebuilt\r\nrules flyout. Base PR: #163304\r\n\r\n#### Changes\r\n- [x] Tweak UI so that it matches the design more closely.\r\n[Design](https://www.figma.com/file/gLHm8LpTtSkAUQHrkG3RHU/%5B8.7%5D-%5BRules%5D-Rule-Immutability%2FCustomization?type=design&node-id=3563-612771&mode=design&t=yqZ6LI0vAjbir9xc-0)\r\n(external).\r\n- [x] Rewrite preview installation and upgrade API endpoints to respond\r\nwith `RuleResponse` instead of `DiffableRule`\r\n- [x] Revert some changes introduced by this\r\n[PR](https://github.com/elastic/kibana/pull/163304)\r\n- [x] Revert exports in\r\n`x-pack/plugins/security_solution/common/api/detection_engine/model/rule_schema/rule_schemas.ts`\r\n- [x] Delete\r\n`x-pack/plugins/security_solution/common/detection_engine/diffable_rule_to_rule_response.ts`\r\n- [x] Make the data contexts unaware of any UI elements that are\r\nconsuming them\r\n- [x] Move rendering of specialized flyout components into to the\r\ncontext provider so that the table is unaware of the flyout.\r\n- [x] Make \"flyoutRule\" and \"closeFlyout\" internal to the context.\r\nComponents outside don't need to know anything about how a rule is\r\ndisplayed. We can encapsulate this knowledge inside the context and\r\nexpose only a generic method, like openRulePreview(ruleId)\r\n - [x] Remove unnecessary checks after using \"invariant\"\r\n- [x] Make sure query, timeline template and all the other fields are\r\nshown in the flyout. Compare each rule in a flyout with the Rule Details\r\nto ensure that all fields are in place.\r\n- [x] Remove the enable / disable switch machine learning job UI switch\r\nelement\r\n- [x] Add custom highlighted fields to the flyout\r\n([comment](https://github.com/elastic/kibana/pull/163235#discussion_r1293821203))\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials. [Docs\r\nticket](https://github.com/elastic/security-docs/issues/3798)\r\n- [x] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [x] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)","sha":"c115f5d3d6f580b195e823c9e948f7b1daf8fddc"}},"sourceBranch":"main","suggestedTargetBranches":["8.10"],"targetPullRequestStates":[{"branch":"8.10","label":"v8.10.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.11.0","labelRegex":"^v8.11.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/164179","number":164179,"mergeCommit":{"message":"[Security Solution] Prebuilt rules installation / upgrade flyout improvements (#164179)\n\n**Addresses: https://github.com/elastic/kibana/issues/162334**\r\n**Base PR: https://github.com/elastic/kibana/pull/163304**\r\n\r\n<img width=\"1177\" alt=\"Screenshot 2023-08-24 at 04 09 07\"\r\nsrc=\"https://github.com/elastic/kibana/assets/15949146/73ac6726-69d4-4c46-bb16-da704a02aba5\">\r\n\r\n## Summary\r\n\r\nThis is a follow-up refactoring and bugfix PR to improve the prebuilt\r\nrules flyout. Base PR: #163304\r\n\r\n#### Changes\r\n- [x] Tweak UI so that it matches the design more closely.\r\n[Design](https://www.figma.com/file/gLHm8LpTtSkAUQHrkG3RHU/%5B8.7%5D-%5BRules%5D-Rule-Immutability%2FCustomization?type=design&node-id=3563-612771&mode=design&t=yqZ6LI0vAjbir9xc-0)\r\n(external).\r\n- [x] Rewrite preview installation and upgrade API endpoints to respond\r\nwith `RuleResponse` instead of `DiffableRule`\r\n- [x] Revert some changes introduced by this\r\n[PR](https://github.com/elastic/kibana/pull/163304)\r\n- [x] Revert exports in\r\n`x-pack/plugins/security_solution/common/api/detection_engine/model/rule_schema/rule_schemas.ts`\r\n- [x] Delete\r\n`x-pack/plugins/security_solution/common/detection_engine/diffable_rule_to_rule_response.ts`\r\n- [x] Make the data contexts unaware of any UI elements that are\r\nconsuming them\r\n- [x] Move rendering of specialized flyout components into to the\r\ncontext provider so that the table is unaware of the flyout.\r\n- [x] Make \"flyoutRule\" and \"closeFlyout\" internal to the context.\r\nComponents outside don't need to know anything about how a rule is\r\ndisplayed. We can encapsulate this knowledge inside the context and\r\nexpose only a generic method, like openRulePreview(ruleId)\r\n - [x] Remove unnecessary checks after using \"invariant\"\r\n- [x] Make sure query, timeline template and all the other fields are\r\nshown in the flyout. Compare each rule in a flyout with the Rule Details\r\nto ensure that all fields are in place.\r\n- [x] Remove the enable / disable switch machine learning job UI switch\r\nelement\r\n- [x] Add custom highlighted fields to the flyout\r\n([comment](https://github.com/elastic/kibana/pull/163235#discussion_r1293821203))\r\n\r\n\r\n### Checklist\r\n\r\nDelete any items that are not applicable to this PR.\r\n\r\n- [ ]\r\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\r\nwas added for features that require explanation or tutorials. [Docs\r\nticket](https://github.com/elastic/security-docs/issues/3798)\r\n- [x] Any UI touched in this PR does not create any new axe failures\r\n(run axe in browser:\r\n[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),\r\n[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))\r\n- [x] This renders correctly on smaller devices using a responsive\r\nlayout. (You can test this [in your\r\nbrowser](https://www.browserstack.com/guide/responsive-testing-on-local-server))\r\n- [x] This was checked for [cross-browser\r\ncompatibility](https://www.elastic.co/support/matrix#matrix_browsers)","sha":"c115f5d3d6f580b195e823c9e948f7b1daf8fddc"}}]}] BACKPORT--> Co-authored-by: Nikita Indik <[email protected]> Co-authored-by: Patryk Kopyciński <[email protected]>
Addresses: #162334
Summary
This PR adds a flyout for viewing a prebuilt rule before installing or updating it. The flyout can be opened by clicking on a rule title within "Add Elastic Rules" page and within "Rule Updates" tab of the Rule Managament table.
I plan to add tests and do minor visual tweaks after the FF.
An additional goal of these changes was to create lightweight reusable components for rule details sections ("About", "Definition", "Schedule") and for rule properties, so that these can later be reused in other flyouts within the Security Solution, on MITRE ATT&CK™ overview page and potentially on the Rule Details page.
These reusable section components are basically copy-pasted components from the Rule Details page that were refactored to remove the dependence from the form schema,
Checklist
Delete any items that are not applicable to this PR.