-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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][Tech debt][Epic] Redesign Security Solution hover/inline actions to make it extensible and reusable across the plugins. #144943
Comments
Pinging @elastic/security-threat-hunting (Team:Threat Hunting) |
We could assess the possibility of doing something similar to what the Kibana Dashboards plugin does with its chart actions, they are pretty similar to our hover actions: Kibana Dashboards actionsIn summary, the pattern is based on a registry of actions defined inside the plugin (code). The action registry is stateful, it needs Then, the different components where the actions need to be displayed get those registered actions, they are rendered inside charts, legends, menus... Depending on the trigger that the action has been associated with (example). What we could doWe could use a similar pattern in Security, starting by creating a new trigger (group of actions) for the "hoverActions", and registering all of them inside the security plugin (filter in, filter out, add to timeline, copy...). We will need the To render the hover actions we could create a new package. This component would need the Then we would be able to render this "hoverActions" packaged component in all the places inside (or outside) Security where we need to show hover actions, using a minimal set of props. I am sure I am missing things on the way and there might be other issues to consider, but that is a pattern that is currently working on the Dashboards side, so probably it's worth doing a proper investigation, and maybe a POC. |
I like this approach, and I agree with implementing a POC. The first step could be extracting It would be nice to define a clear set of requirements for the new architecture. Here are my 5 cents:
Questions: Should |
I've reviewed the approach with
Another thing, which I have in mind is ability to use all available Actions which were designed by other teams. I'm not sure yet how the approach with each action as a package will work, but it looks more extendable and reusable across plugins. |
Great initiative! I think threat_intelligence plugin will benefit from improvements made here. Btw small note about the naming - does it make sense to get rid of |
…able cell actions (#146779) ## Summary Enable XY chart legends and Table cell actions to render dynamically registered "cell value" actions, scoped for Lens embeddables only. Security Solution actions were created to be displayed in the Lens visualizations rendered. closes #145708 ### New Trigger A new _uiActions_ trigger `CELL_VALUE_TRIGGER` has been created to register these actions. It is used in both _TableChart_ cell actions and _XyCharts_ legend actions to create additional action options. #### Table cell actions Actions registered and added to the `CELL_VALUE_TRIGGER` will be appended to the cell actions of the table. The action compatibility is checked for each column. ![table_cell_actions](https://user-images.githubusercontent.com/17747913/205046554-d4ccf5c5-e49a-44e5-8567-4f39756f3fef.png) #### Chart legend actions The same for XyCharts legends. All actions registered and added to the `CELL_VALUE_TRIGGER` will be appended to the legend actions of the charts. The action compatibility is checked for each series accessor. ![chart_legend_actions](https://user-images.githubusercontent.com/17747913/205046313-cd50481c-1a06-4bf7-a8fd-b387a98857c1.png) #### Filter for & Filter out actions: The XY and Table charts have the "Filter for & Filter out" actions hardcoded in the components code. They manually check the value is filterable before showing the actions, but the panel items and cell actions are added explicitly for them in the components. This logic has not changed in this PR, the actions added dynamically using `CELL_VALUE_TRIGGER` are just appended after the "Filter for & Filter out" options. However, "Filter for / Filter out" actions could also be integrated into this pattern, we would only have to register these actions to the `CELL_VALUE_TRIGGER` with the proper `order` value, and then remove all hardcoded logic in the components. ### Security Solution actions The actions that we have registered from Security Solutions ("Add to timeline" and "Copy to clipboard") will be displayed only when the embeddable is rendered inside Security Solution, this is ensured via the `isCompatible` actions method. ![security_actions](https://user-images.githubusercontent.com/17747913/205046643-6ea7e849-b3d6-43f3-91c8-034050375623.png) ### Security Solution Filter In/Out bug There was a CSS rule present in a global Security Solution context, affecting all the `EuiDataGrid` cell actions popovers: https://github.com/elastic/kibana/blob/475e47ed993fba0ecd69caa211150f298e1eefe4/x-pack/plugins/security_solution/public/common/components/page/index.tsx#L124-L130 The goal of this rule was to save some vertical space, by hiding the two auto-generated Filter In/Out actions (generally the first two cell actions), so we could show the horizontal Filter In/Out custom buttons added manually to the popover top header. ![old_filter_in_out](https://user-images.githubusercontent.com/17747913/205293269-aa1e6409-a809-4328-8079-ef1e09c0750d.png) This CSS rule was causing a bug when rendering Table visualizations (they use `EuiDataGrid` as well), always hiding the two first cell actions in the popover. After discussing this topic with the team, and considering there is an ongoing task to unify the cell actions experience in Security and centralize the implementation (see: #144943), we decided to remove the problematic CSS rule and the custom horizontal Filter buttons, so the auto-generated Filter actions are displayed, this way all tables in Security (alerts, events, and also embedded table charts) will have a unified and consistent UX. ![new_filter_in_out](https://user-images.githubusercontent.com/17747913/205293120-3bf27a8a-90dd-47dd-a14e-d92570bff84f.png) If the cell actions popover grows too much in the future we could apply a "More actions ..." footer option strategy. ## Testing The new actions will be displayed on any embedded Lens object rendered inside Security. A good place to test that is Cases, since we can attach Lens visualizations in the comments. However, Cases explicitly disables all the trigger actions passing `disableTriggers` to the Lens Embeddables. https://github.com/elastic/kibana/blob/b80a23ee125cc4612b99e0b24e6ff2b55ebbdf55/x-pack/plugins/cases/public/components/markdown_editor/plugins/lens/processor.tsx#L52 The easiest way to test different chart types and tables is to remove/comment the `disableTriggers` prop in the Cases processor component, and then go to a Case and attach different Lens visualizations. All actions should appear in the legends and table cell hover actions. ### Checklist - [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) - [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 - [x] Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/)) Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Angela Chuang <[email protected]>
Epic: #144943 ## Summary Moving the existing CellActions implementation to a new home. The `kbn-cell-actions` package contains components and hooks that are going to be used by solutions to show data cell actions with a consistent UI across them. Security Solution is going to start using it by migrating all "hover-actions" to the unified implementation, but the usage is not restricted to it. Any plugin can register and attach its own actions to a trigger via uiActions, and use this package to render the CellActions components in a consistent way. The initial implementation was placed in the uiActions plugin itself due to a types constraints (https://github.com/elastic/kibana/tree/main/src/plugins/ui_actions/public/cell_actions), the constraint has been solved so we are creating the package for it as planned. This PR only moves that implementation to the new package, with small directory changes. The exported components are not being used anywhere currently, so the implementation may change during the migration phase. ### 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) - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 Co-authored-by: kibanamachine <[email protected]>
Epic: elastic#144943 ## Summary Moving the existing CellActions implementation to a new home. The `kbn-cell-actions` package contains components and hooks that are going to be used by solutions to show data cell actions with a consistent UI across them. Security Solution is going to start using it by migrating all "hover-actions" to the unified implementation, but the usage is not restricted to it. Any plugin can register and attach its own actions to a trigger via uiActions, and use this package to render the CellActions components in a consistent way. The initial implementation was placed in the uiActions plugin itself due to a types constraints (https://github.com/elastic/kibana/tree/main/src/plugins/ui_actions/public/cell_actions), the constraint has been solved so we are creating the package for it as planned. This PR only moves that implementation to the new package, with small directory changes. The exported components are not being used anywhere currently, so the implementation may change during the migration phase. ### 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) - [x] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 Co-authored-by: kibanamachine <[email protected]>
#148056) Epic: #144943 ## Summary Update explore pages to use the new `CellActions` component instead of `HoverActions `. ### What is included? * Update the user, host, and network page tables. <img width="1512" alt="Screenshot 2023-01-17 at 13 12 16" src="https://user-images.githubusercontent.com/1490444/212896520-f41e9026-cef0-4a37-8bd1-35784a87ca09.png"> <img width="1482" alt="Screenshot 2023-01-17 at 13 19 34" src="https://user-images.githubusercontent.com/1490444/212897411-cd3c3ef8-bca0-461b-a1ff-c7dd67159d1b.png"> * Fields rendered when clicking on "+{N} more" <img width="248" alt="Screenshot 2023-01-17 at 12 51 38" src="https://user-images.githubusercontent.com/1490444/212892255-2ecd7050-75f6-4883-b331-1fed527de53f.png"> ### What is NOT included? * Visualizations * Fields on details pages. They are also used by the Timeline and need to be draggable. * Timeline * Datagrid tables - Events and Alerts * Plugins ### Checklist Delete any items that are not applicable to this PR. - [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 - [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) Co-authored-by: kibanamachine <[email protected]>
…149120) Epic: #144943 ## Summary ### API changes It brings back the `aggregable` field property. It is necessary for show_top_n. Firstly I thought I could derive this information from the `field.type` but that isn't possible for all cases. ### New Action Creates a new trigger for flyouts that contain one extra action named `toggleColumn`. The action toggles a column in the table that opened the flyout. <img width="277" alt="Screenshot 2023-02-02 at 15 39 16" src="https://user-images.githubusercontent.com/1490444/216354526-13e4ea1c-c0a6-444e-9640-c1e66d4f917c.png"> ### Migration Migrate security solution inline actions (actions that show inlined and not inside a hover popover) to use CellActions. It includes: * Entity analytics page: Risk table * Event flyout: Highlighted events and summary overview * Event details Table * Event details enrichment summary <img width="1166" alt="Screenshot 2023-01-25 at 13 51 28" src="https://user-images.githubusercontent.com/1490444/214568144-3e9b2372-9882-4eff-8055-22a4ff52a7bd.png"> <img width="1783" alt="Screenshot 2023-01-25 at 13 52 00" src="https://user-images.githubusercontent.com/1490444/214568151-0a79e87d-5f87-438f-8365-46be7c43ac31.png"> <img width="905" alt="Screenshot 2023-02-02 at 15 35 07" src="https://user-images.githubusercontent.com/1490444/216353625-371d7cbc-94b7-4324-a177-4027917801f4.png"> <img width="894" alt="Screenshot 2023-02-02 at 15 35 21" src="https://user-images.githubusercontent.com/1490444/216353634-e9a02eba-e139-40b4-990b-12d547425fbd.png"> Todo: - [x] Check with Paul the best position for the new action ### Checklist Delete any items that are not applicable to this PR. - [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
Once this migrations is finished we'll be able to do a fix for #154714 |
…iscover (#160524) Original issue: #144943 ## Summary * Update CellActions value to be `Serializable`. * Update Default Actions and SecuritySolution Actions to allowlist the supported Kibana types. * Add an extra check to Action's `execute` to ensure the field value is compatible. ### How to test it? * Open Discover and create a saved search with many different field types * Go to Security Solutions dashboards * Create a new dashboard and import the saved search * Test the created dashboard inside Security Solutions ### Checklist - [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) - [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 --------- Co-authored-by: kibanamachine <[email protected]>
…CellActions (#161361) EPIC: #144943 ## Summary Update Events/alerts table to provide `CellActions` with a complete `FieldSpec`object from DataView ### Affected pages: * Alerts page * Security Dashboards * Rule preview * Host events * Users events ### How to test it Use CellActions on one of the affected pages. ### Checklist - [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 --------- Co-authored-by: kibanamachine <[email protected]>
The only missing task is this one, whose goal is solely standardizing a use case that is only used in a couple of places, and has already been solved using Since everything has already been migrated to cell-action inside Security, and the last task is just an enhancement, I am de-scoping it from the epic so we can close it. We can do that separately if more use cases appear. Kudos to @machadoum 👏 |
This is awesome! Thank you all for the effort! 🥳 🍾 |
The problem statement for this issue is about making Security Solution hover/inline actions components reusable and extensible and fit the specific consumer needs without impacting the rest of this actions usage in the other places. (Consumer is the component which display/manage the own set of needed hover actions like data tables, timeline, entity analytics page, etc.)
The goal of this epic is to provide the ability to create, register and use the actions with the ability to render them as the inline or hover UI mode.
We should provide enough flexibility to reuse the actions by any of the consumers.
Current Epic includes the next work items:
Phase 1 (Create
CellActions
package and use it inside SecuritySolutions)AddToTimeline
andCopyTo
actions using uiActions registry. [Security Solution][Lens] New trigger actions for chart legends and table cell actions #146779CellActions
[145663] Refactor explore pages to migrate HoverActions to CellActions #148056CellActions
. Existing issue [Security Solution] Replace HoverActions component and useHoverActions hook with the new CellActions for events viewer(alerts and events tables). #145666CellActions
implementation to a package. PR: [Security Solution] [CellActions] Move to a package #149057EuiDataGrid
column cellActions prop [Security Solution] integrate CellActions in Events/Alerts DataTables #149934createCellActionFactory
the action files are pretty small)Tech Debt
Rewrite actions to support multiple fields and use them on the investigation in timeline (.andFilters) Update Security Cell actions and default Cell actions to support multiple fields #159480CellActions
with a completeFieldSpec
object from DataView [Security Solutions] Update Events/alerts table to use FieldSpec for CellActions #161361Bugs
Phase 2 (Use
CellActions
package inside Discover)CellActions
[Security Solution] Use CellActions registry in Discover data grid #157201CellActions
to support Discover use case [SecuritySolutions] Update CellActions to support all types used by Discover #160524CellActions
type to value:Serializable
.field.type
in theisCompatible
of every action.execute
of every action.DiscoverGrid
and pass the value directly to CellActions.Existing architecture
There are a few concepts of the hover actions `security_solution` is operating with: cell actions of the TGrid, event details. The root of the functionality is located in the `timelines` plugin and exposed through the plugin public interface as `getHoverActions` hook. The diagram below shows the usage of the hook across plugins (security_solution, osquery, kubernetes_security, threat_inteligence):HoverActions
component anduseHoverActions
hook. Retrieving available actions fromtimelines.getHoverActions
by usage of the hookuseHoverActionsItems
:useHoverActions
hook is used byDraggableWraper
HoverActions
component is moved toActionCell
component:timelines.getHoverActions
is used directly by the component belongs toExpandedCellValueActionsComponent
. This tree of the components is ended by the hookuseRenderCellValue
(used for registering cell actions for the new alerts table inCases
) andDefaultCellRenderer
(used byStatefulEventsViewer
to path the available cell actions to theTGrid
)timelines.getHoverActions
is allocated bydefaultCellActions
Proposal diagram: https://whimsical.com/hoveractions-8t627KZTFtqK97rWDfgKxw
The text was updated successfully, but these errors were encountered: