[Security Solution] Persistent rules table state #145111

maximpn · 2022-11-14T13:41:09Z

Addresses: #140263

Summary

It implements rules table state persistence. Reopening or reloading of the page loads the same state as well as navigation from another page inside or outside the security plugin.

Screen.Recording.2022-11-15.at.18.13.17.mov

Details

The changes persist the rules table state in the url and the session storage to be able to recover after navigation from another page. The following parameters are persisted

search term, tags and elastic vs custom filters
sorting field and sorting direction
page number (only in the url) and page size

Page number is only persisted in the url. Avoiding persistence a page number in the session storage make the UX more predictable that users upon returning to the page expect the first page to be selected.

Selection state is not persisted since it can lead to URL overflow problem. According to the different resources for example this one the maximum length should not exceed 2KB though Chrome supports URLs up to 2MB.

Besides that this PR adds

unit tests which coved edge cases
basic Cypress tests

What is not done and will be addressed in the next PRs

persistence of the selected tab as a path segment in the url
comprehensive Cypress tests covering edge cases
getting rid of the back button < Rules on the rule details page

Edge cases

Since parameters in the URL and the session storage can be modified by users it can lead to unexpected results or security risks.
What have been tested and covered so far:

arbitrary modification of the serialized state which leads to deserialization error (default parameters are used in this case and the state is rewritten with the default parameters)
filter parameters modification (doesn't cause errors)
page number modification (isn't not handled in a special way, EuiBasicTable display either first of the last page if the value exceeds the limits)
page size (EuiBasicTable loads more records which isn't limited and can cause a performance issue, it has been addressed through validation)
sorting parameters modification (EuiBasicTable ignores wrong values)

The observation result is that EuiBasicTable gracefully handles the wrong values .

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2022-11-15T22:54:10Z

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

elasticmachine · 2022-11-15T22:54:11Z

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

xcrzx

I've briefly looked through the implementation and left some initial comments. I think my biggest concern currently is about the usage of local storage. Therefore, I propose discussing alternatives before proceeding with the implementation.

x-pack/plugins/security_solution/public/common/hooks/use_url_state.ts

x-pack/plugins/security_solution/common/constants.ts

...n_engine/rule_management_ui/components/rules_table/rules_table_filters/rule_search_field.tsx

...tion_engine/rule_management_ui/components/rules_table/rules_table/rules_table_saved_state.ts

...ne/rule_management_ui/components/rules_table/rules_table/use_sync_rules_table_saved_state.ts

...tion_engine/rule_management_ui/components/rules_table/rules_table/rules_table_saved_state.ts

maximpn · 2022-12-09T20:55:27Z

@elasticmachine merge upstream

maximpn · 2022-12-12T08:11:21Z

@elasticmachine merge upstream

kibana-ci · 2022-12-12T09:36:41Z

💚 Build Succeeded

Buildkite Build
Commit: 8de731b

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	3381	3387	+6

Async chunks

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

id	before	after	diff
`securitySolution`	10.1MB	10.1MB	+4.2KB

Page load bundle

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

id	before	after	diff
`securitySolution`	51.0KB	51.0KB	+79.0B

Unknown metric groups

ESLint disabled in files

id	before	after	diff
`osquery`	1	2	+1

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`fleet`	60	66	+6
`osquery`	109	115	+6
`securitySolution`	445	451	+6
total			+20

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`fleet`	69	75	+6
`osquery`	110	117	+7
`securitySolution`	521	527	+6
total			+21

History

💔 Build #93856 failed 33a822f
💔 Build #93827 failed 1094c24
💔 Build #93547 failed 88b09a5
💛 Build #93276 was flaky 059ced3
💛 Build #93095 was flaky 0ac50b0

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

cc @maximpn

**Addresses:** elastic#140263 ## Summary It implements rules table state persistence. Reopening or reloading of the page loads the same state as well as navigation from another page inside or outside the security plugin. https://user-images.githubusercontent.com/3775283/201983621-e57e8c64-d5fb-4111-92df-ebe912dada38.mov ## Details The changes persist the rules table state in the url and the session storage to be able to recover after navigation from another page. The following parameters are persisted - search term, tags and elastic vs custom filters - sorting field and sorting direction - page number **(only in the url)** and page size Page number is only persisted in the url. Avoiding persistence a page number in the session storage make the UX more predictable that users upon returning to the page expect the first page to be selected. Selection state is not persisted since it can lead to URL overflow problem. According to the different resources for example [this one](https://www.geeksforgeeks.org/maximum-length-of-a-url-in-different-browsers/) the maximum length should not exceed 2KB though Chrome supports URLs up to 2MB. Besides that this PR adds - unit tests which coved edge cases - basic Cypress tests What is not done and will be addressed in the next PRs - persistence of the selected tab as a path segment in the url - comprehensive Cypress tests covering edge cases - getting rid of the back button `< Rules` on the rule details page ## Edge cases Since parameters in the URL and the session storage can be modified by users it can lead to unexpected results or security risks. What have been tested and covered so far: - arbitrary modification of the serialized state which leads to deserialization error (default parameters are used in this case and the state is rewritten with the default parameters) - filter parameters modification (doesn't cause errors) - page number modification (isn't not handled in a special way, `EuiBasicTable` display either first of the last page if the value exceeds the limits) - page size (`EuiBasicTable` loads more records which isn't limited and can cause a performance issue, it **has been addressed** through validation) - sorting parameters modification (`EuiBasicTable` ignores wrong values) The observation result is that `EuiBasicTable` gracefully handles the wrong values . ### 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

…147218) Addresses: #140263 This PR was inspired by #146649 and while working on persistent rules table state #145111. ## Summary It includes improvements for url search parameters manipulation functionality. In particular `useGetInitialUrlParamValue` and `useReplaceUrlParams` hooks. The main idea is to isolate the encoding layer ([rison](https://github.com/Nanonid/rison)) as an implementation detail so `useGetInitialUrlParamValue` and `useReplaceUrlParams` consumers can just provide types they wish and the hooks serialize/desirealize the data into appropriate format under the hood. On top of that after `@kbn/rison` was added in #146649 it's possible to use this package directly to encode and decode parameters whenever necessary. The improvements include - store unserialized url parameters state in the redux store - encode and decode data by using functions from `@kbn/rison` directly - let `useReplaceUrlParams` accept an updater object ### 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

maximpn self-assigned this Nov 14, 2022

maximpn linked an issue Nov 14, 2022 that may be closed by this pull request

[Security Solution] Persistent Rules table state #140263

Closed

5 tasks

maximpn force-pushed the persistent-rules-table-state branch 5 times, most recently from 46606d8 to 3200603 Compare November 15, 2022 20:42

maximpn marked this pull request as ready for review November 15, 2022 22:54

maximpn requested review from a team as code owners November 15, 2022 22:54

maximpn requested a review from jpdjere November 15, 2022 22:54

maximpn force-pushed the persistent-rules-table-state branch from cd6b631 to eaf2380 Compare November 16, 2022 10:16

xcrzx self-requested a review November 16, 2022 10:17

xcrzx reviewed Nov 16, 2022

View reviewed changes

maximpn force-pushed the persistent-rules-table-state branch 5 times, most recently from fd0af5c to 1c6cf42 Compare November 17, 2022 12:35

maximpn added 15 commits December 9, 2022 10:35

do not sync page number with the session storage

af3cf31

fix a typing issue

ded775d

get rid of the duplicated hook dependencies

aaa238d

implement rules filter as enum

533d527

split rules table state into url and storage

f7a338b

fix wrong typing

f365776

rename sorting direction to sorting order

f84189a

get rid of as unknown as

a40b23f

use rules table context from the __mocks__ folder

b0d739b

add comments to @ts-expect-errors

b9b6f20

type rules table context state in unit tests

42ffc62

handle sessionStorage.get exceptions

ebd8e66

type sorting field as a union of literals

c2578e3

reuse shared types

c116b8d

fix typing

1094c24

maximpn force-pushed the persistent-rules-table-state branch from 88b09a5 to 1094c24 Compare December 9, 2022 09:36

maximpn mentioned this pull request Dec 9, 2022

[Security Solution] Global query string functionality improvements #147218

Merged

1 task

Merge branch 'main' into persistent-rules-table-state

33a822f

Merge branch 'main' into persistent-rules-table-state

8de731b

maximpn merged commit 2cf74a1 into elastic:main Dec 12, 2022

kibanamachine added the backport:skip This commit does not require backporting label Dec 12, 2022

banderror mentioned this pull request Feb 21, 2023

[DOCS] Mention that the Rules table can now be shared via the URL elastic/security-docs#3006

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution] Persistent rules table state #145111

[Security Solution] Persistent rules table state #145111

maximpn commented Nov 14, 2022 •

edited

elasticmachine commented Nov 15, 2022

elasticmachine commented Nov 15, 2022

xcrzx left a comment

maximpn commented Dec 9, 2022

maximpn commented Dec 12, 2022

kibana-ci commented Dec 12, 2022

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

[Security Solution] Persistent rules table state #145111

[Security Solution] Persistent rules table state #145111

Conversation

maximpn commented Nov 14, 2022 • edited

Summary

Details

Edge cases

Checklist

elasticmachine commented Nov 15, 2022

elasticmachine commented Nov 15, 2022

xcrzx left a comment

Choose a reason for hiding this comment

maximpn commented Dec 9, 2022

maximpn commented Dec 12, 2022

kibana-ci commented Dec 12, 2022

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

ESLint disabled in files

ESLint disabled line counts

Total ESLint disabled count

History

maximpn commented Nov 14, 2022 •

edited