[Security Solution] Use CellActions registry in Discover data grid #157201

semd · 2023-05-09T18:16:10Z

Summary

Enables Discover DataGrid to use registered cell actions instead of the default static actions.

New `cellActionsTriggerId` prop

This PR introduces a new cellActionsTriggerId optional prop in the DataGrid component:

kibana/src/plugins/discover/public/components/discover_grid/discover_grid.tsx

Lines 198 to 201 in 98c210f

    
             /** 
        
              * Optional triggerId to retrieve the column cell actions that will override the default ones 
        
              */ 
        
             cellActionsTriggerId?: string;

When this prop is defined, the component queries the trigger's registry to retrieve the cellActions attached to it, using the CellActions package' useDataGridColumnsCellActions hook. This hook returns the cellActions array ready to be passed for each column to the EuiDataGrid component.
When (non-empty) actions are found in the registry, they are used, replacing all of the default static Discover actions. Otherwise, the default cell actions are used.

This new prop also allows other instances of the Discover DataGrid to be configured with custom cell actions, which will probably be needed by Security Timeline integration with Discover.

New `SEARCH_EMBEDDABLE_CELL_ACTIONS_TRIGGER` Trigger

Along with the new cellActionsTriggerId prop the plugin also registers a new trigger for "saved search" embeddable:

kibana/src/plugins/discover/public/plugin.tsx

Line 387 in 055750c

uiActions.registerTrigger(SEARCH_EMBEDDABLE_CELL_ACTIONS_TRIGGER);

And it gets passed to the DataGrid component on the Embeddable creation:

kibana/src/plugins/discover/public/embeddable/saved_search_embeddable.tsx

Line 403 in 055750c

cellActionsTriggerId: SEARCH_EMBEDDABLE_CELL_ACTIONS_TRIGGER_ID,

Having this new trigger available allows solutions to attach custom actions to it, in order to be displayed in the saved search embeddables. Each action will be able to implement its isCompatible check to determine if they are going to be displayed in the embedded saved search DataGrid field, or not. If no compatible actions are found, DataGrid will render the default static actions.

ℹ️ In this implementation, the actions registered to this new "embeddable trigger" need to check if they are being rendered inside Security using the isCompatible function, to prevent them from being displayed in other solutions, resulting in a non-optimal architecture. This approach was needed since there's no plausible way to pass the cellActionsTriggerId property from the Dashboard Renderer used in Security, all the way down to the specific Discover "saved search" embeddable. However, the Dashboards team is planning to enable us to pass options to nested embeddables using a registry (#148933). When this new tool is available we will be able to delegate the trigger registering to Security and configure the "saved search" embeddables to use it. Therefore, the trigger will only be used by Security, so we won't have to worry about Security actions being rendered outside Security.

Videos

before:

regular_discover_actions.mov

after:

custom_discover_actions.mov

…fix'

…/semd/kibana into poc_discover_custom_cell_actions

elasticmachine · 2023-05-10T12:48:46Z

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

semd · 2023-05-11T09:27:50Z

@elasticmachine merge upstream

…_cell_actions

kertal

I think extending the cell actions of Discover data grid makes much sense 👍
We will have a closer look at it next week. Playing with it, I have an initial question. When I add something to a timeline, what does it mean in this cases? I've created a custom dashboard showing e-commerce orders, I've added some of the values to a timeline. Now those values are visible in the query builder, but that's it. Since the data view Alerts is selected, there are no entries visible in the table. I guess it's expected. But as a newbie user more familiar with Discover / Dashboards it's not 100% clear what add to "Add to timeline" means in this case

…_cell_actions

jughosta

Looking good! Left some questions to clarify the details.

src/plugins/discover/public/application/context/context_app_content.test.tsx

src/plugins/discover/public/application/services_provider.tsx

src/plugins/discover/public/components/discover_grid/discover_grid.tsx

semd · 2023-05-15T12:32:57Z

I think extending the cell actions of Discover data grid makes much sense 👍 We will have a closer look at it next week. Playing with it, I have an initial question. When I add something to a timeline, what does it mean in this cases? I've created a custom dashboard showing e-commerce orders, I've added some of the values to a timeline. Now those values are visible in the query builder, but that's it. Since the data view Alerts is selected, there are no entries visible in the table. I guess it's expected. But as a newbie user more familiar with Discover / Dashboards it's not 100% clear what add to "Add to timeline" means in this case

Hi @kertal
Yes you are right, the scenario you described is the expected behavior. However, these actions will only be visible inside Security, where the dashboards are expected to contain Security-related data. The "add to timeline" action is meant to make it easy for Security analysts to pivot to Timeline investigations on an specific value, probably from the alerts index.
It does not make much sense to have e-commerce orders in a Security dashboards, nevertheless, it is also possible to change the Data View in the Timeline, so we are not limited to render alerts only.

kertal · 2023-05-15T13:38:00Z

Hi @kertal Yes you are right, the scenario you described is the expected behavior. However, these actions will only be visible inside Security, where the dashboards are expected to contain Security-related data. The "add to timeline" action is meant to make it easy for Security analysts to pivot to Timeline investigations on an specific value, probably from the alerts index. It does not make much sense to have e-commerce orders in a Security dashboards, nevertheless, it is also possible to change the Data View in the Timeline, so we are not limited to render alerts only.

@semd Thx, this makes sense. Quick follow up, what's the quickest way to generate test data for the security related data?
@lukasolson could you also have a look, this PR has some overlapping with your work to extend the doc viewer , many thx

semd · 2023-05-15T18:30:18Z

@kertal What I usually do to generate some alerts data locally is executing the test generator with:

cd x-pack/plugins/security_solution
yarn test:generate -s resolver --gen 3 --pr 100 --pt 100

And then create a new rule in the UI (/app/security/rules/create) that takes everything * using the default index patterns, to generate the alerts.

kertal · 2023-05-16T07:39:47Z

@kertal What I usually do to generate some alerts data locally is executing the test generator with:
cd x-pack/plugins/security_solution
yarn test:generate -s resolver --gen 3 --pr 100 --pt 100
And then create a new rule in the UI (/app/security/rules/create) that takes everything * using the default index patterns, to generate the alerts.

@semd thx, now it makes much more sense for me, and looking at it, I think we should align styling and wording, here's a screen to compare

…fix'

machadoum · 2023-06-20T13:32:05Z

Hey team! As Sergi mentioned, I am taking it over from here.

First of all, thank you for helping us improve CellActions ❤️

I pushed code with KBN types allowlist. We are only going to display CellActions for the types: date, ip, string, and number. Could you please take a look 5e4ac31?

@davismcphee I tried to reproduce the response field bug step-by-step, but it works for me. I suspect that a previous change fixed it. Could you try to reproduce it one more time with the current changes?

Jun-20-2023.15-23-26.mp4

jughosta

Hi @machadoum,
Thanks for continuing the work and planning on extending cell actions package with more types!

jughosta · 2023-06-20T13:42:39Z

src/plugins/discover/public/components/discover_grid/discover_grid.tsx

+    () =>
+      cellActionsTriggerId && !isPlainRecord
+        ? visibleColumns.map((columnName) => {
+            const field = dataView.getFieldByName(columnName)?.spec;


Can be simplified. Also mock changes might be unnecessary.

Suggested change

const field = dataView.getFieldByName(columnName)?.spec;

const field = dataView.getFieldByName(columnName);

jughosta · 2023-06-20T13:46:01Z

src/plugins/discover/public/components/discover_grid/discover_grid.tsx

+            if (
+              !field?.type ||
+              !SUPPORTED_CELL_ACTIONS_TYPES.includes(field.type as KBN_FIELD_TYPES)
+            ) {


This logic could be probably extracted into a function inside @kbn/cell-actions package: for example, isSupportedByCellActions(field).

Would be great to cover it with tests.

src/plugins/discover/public/components/discover_grid/discover_grid_cell_actions.tsx

jughosta

Unblocking the PR 👍

davismcphee

@machadoum Thanks for the updates, the allowlist change looks good to me! Overall this PR seems just about ready to merge, except I'm still encountering the issue with adding saved searches containing a flattened field to a Dashboard.

Here's a quick video of what I'm doing from start to finish to reproduce the issue: https://github.com/elastic/kibana/assets/25592674/5904ba82-f015-4263-98f3-e02da83ff696

@davismcphee I tried to reproduce the response field bug step-by-step, but it works for me. I suspect that a previous change fixed it. Could you try to reproduce it one more time with the current changes?

I'm now thinking this might just be a misunderstanding on my part and maybe this is expected. I notice in your video there's a search bar above the dashboard, but I'm not seeing one when I view my dashboard, which probably explains why clicking the filter actions don't do anything for me:

This one is less of a concern for me anyway since it doesn't have an impact outside of Security, so I'd be happy to merge with the current behaviour if it's working as expected for your team. The only thing left I'd like to make sure we figure out before I approve is the blank saved search panel since it impacts any dashboard configured this way.

machadoum · 2023-06-21T12:08:43Z

Hey @davismcphee!
Thank you for the video. It was very helpful. I managed to reproduce the bug. It happened when the data view had no time field. I noticed that before this PR, there was an extra validation for it, so I added it back here. Good catch!

[edited] ~~I don't know why you can't see the search bar on the dashboards page. Do you get any errors on the console?~~ You are right, the filter action adds the field to the search bar. If you can't see the search bar you can't see the effect.

[edited2] The search bar doesn't show up when you don't have security solution indices. That explains the mystery. But it is a weird scenario, I will double-check with the team.

davismcphee

I tested with the latest changes and the blank panel issue is fixed! Also thanks for adding those tests to help prevent it in the future. I'm glad you were able to get to the bottom of the missing search bar, I'll leave it to you to decide if that's something you need to address in this PR or not. Thanks for all the work on this, LGTM 👍

…cell_actions

machadoum · 2023-06-23T11:46:49Z

@elasticmachine merge upstream

…cell_actions

kibana-ci · 2023-06-23T14:15:42Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 74767dd

Failed CI Steps

Test Failures

[job] [logs] Security Solution Tests #7 / Shared exception lists - read only "before each" hook for "Displays missing privileges primary callout"
[job] [logs] FTR Configs #27 / Synthetics API Tests PrivateLocationAddMonitor handles is_tls_enabled false

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`discover`	559	589	+30
`securitySolution`	4212	4216	+4
total			+34

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/cell-actions`	43	44	+1
`discover`	67	68	+1
total			+2

Async chunks

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

id	before	after	diff
`discover`	513.7KB	527.3KB	+13.6KB
`securitySolution`	11.0MB	11.0MB	-8.8KB
total			+4.9KB

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`discover`	28.9KB	29.3KB	+419.0B
`securitySolution`	52.0KB	52.1KB	+138.0B
total			+557.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/cell-actions`	57	62	+5
`discover`	87	88	+1
total			+6

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	13	15	+2
`securitySolution`	416	420	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	14	16	+2
`securitySolution`	497	501	+4
total			+6

History

💔 Build #137593 failed 680ba4d
💔 Build #137564 failed af336de3c91a28a22e5c629d83febd757d6274fb
💔 Build #137348 failed b144dfa
💔 Build #137347 failed b144dfa
💛 Build #136894 was flaky 6eb4a2a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @machadoum @semd

…ctions (#160095) ## Summary Add support for `Boolean` and `Number` types to CellActions and update Security Actions accordingly. It also fixes the copy-to-clipboard action for fields of the type number (`process.parent.pid`). issue: #159298 - [x] Remove discover fields value casting if it gets merged after the [discover PR](#157201) ### How to test it? The quickest way is to find an alert field that is a boolean or number on the alerts page and check if security solution actions still work. But all boolean fields that I tested are actually strings. 🤷 Alternatively, you could render the `<SecurityCellActions />` with fake data (boolean and number). ### 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: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>

initial working version

d30d248

semd self-assigned this May 9, 2023

kibanamachine and others added 3 commits May 9, 2023 18:25

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

6190243

…fix'

tests

77713cb

Merge branch 'poc_discover_custom_cell_actions' of https://github.com…

98c210f

…/semd/kibana into poc_discover_custom_cell_actions

semd changed the title ~~[Security Solution][PoC] Use CellActions registry in Discover~~ [Security Solution] Use CellActions registry in Discover data grid May 10, 2023

conflicts solved

e476940

semd added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team Team:Threat Hunting:Explore v8.9.0 labels May 10, 2023

new test cases

443bb40

semd marked this pull request as ready for review May 10, 2023 12:48

semd requested review from a team as code owners May 10, 2023 12:48

kibanamachine and others added 4 commits May 11, 2023 05:27

Merge branch 'main' into poc_discover_custom_cell_actions

5630fc1

Merge remote-tracking branch 'upstream/main' into poc_discover_custom…

f9cb2f0

…_cell_actions

fix weird dependencies problem

055750c

solve conflicts

fd5f05b

kertal reviewed May 12, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into poc_discover_custom…

0c14b0d

…_cell_actions

jughosta reviewed May 15, 2023

View reviewed changes

relocate cellActions context provider

cd1bee1

add cellActions provider to context_app

fcb24f6

[CI] Auto-commit changed files from 'node scripts/lint_ts_projects --…

9d0ab82

…fix'

machadoum mentioned this pull request Jun 20, 2023

[Security Solution][Tech debt][Epic] Redesign Security Solution hover/inline actions to make it extensible and reusable across the plugins. #144943

Closed

29 tasks

Fix unit test

02de751

jughosta reviewed Jun 20, 2023

View reviewed changes

jughosta self-requested a review June 20, 2023 19:23

jughosta approved these changes Jun 20, 2023

View reviewed changes

jughosta requested a review from davismcphee June 20, 2023 19:27

davismcphee requested changes Jun 21, 2023

View reviewed changes

machadoum added 2 commits June 21, 2023 13:04

Fix blank disciver grid when data view has no timestamp

23fec20

Please code reviewers

6eb4a2a

machadoum mentioned this pull request Jun 21, 2023

[SecuritySolutions] Add support for Number and Boolean types to CellActions #160095

Merged

2 tasks

davismcphee approved these changes Jun 22, 2023

View reviewed changes

machadoum added v8.10.0 and removed v8.9.0 labels Jun 22, 2023

Merge remote-tracking branch 'elastic/main' into poc_discover_custom_…

b144dfa

…cell_actions

machadoum enabled auto-merge (squash) June 22, 2023 14:47

machadoum disabled auto-merge June 22, 2023 15:00

Fix merge conflicts

680ba4d

machadoum force-pushed the poc_discover_custom_cell_actions branch from af336de to 680ba4d Compare June 23, 2023 09:14

machadoum enabled auto-merge (squash) June 23, 2023 09:15

Merge remote-tracking branch 'elastic/main' into poc_discover_custom_…

74767dd

…cell_actions

machadoum force-pushed the poc_discover_custom_cell_actions branch from 8743a60 to 74767dd Compare June 23, 2023 12:50

machadoum merged commit f4159c4 into elastic:main Jun 23, 2023

kibanamachine added the backport:skip This commit does not require backporting label Jun 23, 2023

kertal mentioned this pull request Jun 20, 2024

[OneDiscover][Extension] DataTable Cell Actions #186576

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Use CellActions registry in Discover data grid #157201

[Security Solution] Use CellActions registry in Discover data grid #157201

semd commented May 9, 2023 •

edited

Loading

elasticmachine commented May 10, 2023

semd commented May 11, 2023

kertal left a comment

jughosta left a comment

semd commented May 15, 2023

kertal commented May 15, 2023

semd commented May 15, 2023

kertal commented May 16, 2023

machadoum commented Jun 20, 2023

jughosta left a comment

jughosta Jun 20, 2023

jughosta Jun 20, 2023

jughosta Jun 20, 2023

jughosta left a comment

davismcphee left a comment •

edited

Loading

machadoum commented Jun 21, 2023 •

edited

Loading

davismcphee left a comment

machadoum commented Jun 23, 2023

kibana-ci commented Jun 23, 2023

API count

ESLint disabled line counts

Total ESLint disabled count

	/**
	* Optional triggerId to retrieve the column cell actions that will override the default ones
	*/
	cellActionsTriggerId?: string;

	const field = dataView.getFieldByName(columnName)?.spec;
	const field = dataView.getFieldByName(columnName);

[Security Solution] Use CellActions registry in Discover data grid #157201

[Security Solution] Use CellActions registry in Discover data grid #157201

Conversation

semd commented May 9, 2023 • edited Loading

Summary

New cellActionsTriggerId prop

New SEARCH_EMBEDDABLE_CELL_ACTIONS_TRIGGER Trigger

Videos

elasticmachine commented May 10, 2023

semd commented May 11, 2023

kertal left a comment

Choose a reason for hiding this comment

jughosta left a comment

Choose a reason for hiding this comment

semd commented May 15, 2023

kertal commented May 15, 2023

semd commented May 15, 2023

kertal commented May 16, 2023

machadoum commented Jun 20, 2023

jughosta left a comment

Choose a reason for hiding this comment

jughosta Jun 20, 2023

Choose a reason for hiding this comment

jughosta Jun 20, 2023

Choose a reason for hiding this comment

jughosta Jun 20, 2023

Choose a reason for hiding this comment

jughosta left a comment

Choose a reason for hiding this comment

davismcphee left a comment • edited Loading

Choose a reason for hiding this comment

machadoum commented Jun 21, 2023 • edited Loading

davismcphee left a comment

Choose a reason for hiding this comment

machadoum commented Jun 23, 2023

kibana-ci commented Jun 23, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Public APIs missing comments

Async chunks

Page load bundle

API count

ESLint disabled line counts

Total ESLint disabled count

History

semd commented May 9, 2023 •

edited

Loading

New `cellActionsTriggerId` prop

New `SEARCH_EMBEDDABLE_CELL_ACTIONS_TRIGGER` Trigger

davismcphee left a comment •

edited

Loading

machadoum commented Jun 21, 2023 •

edited

Loading