Skip to content
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][Exceptions] - Updates exception hooks and viewer #73588

Merged
merged 24 commits into from
Jul 29, 2020

Conversation

yctercero
Copy link
Contributor

@yctercero yctercero commented Jul 29, 2020

Summary

This PR focuses on addressing issues around the pagination and functionality of rules with numerous (2+) exception lists.

  • Updated the use_exception_list.ts hook to make use of the new multi list find API
  • Updated the viewer to make use of the new multi list find API
    • Previously was doing a lot of the filtering and paging manually (and badly) in the UI, now the _find takes care of all that
  • Added logic for showing No results text if user filter/search returns no items
    • Previously would show the This rule has not exceptions text

Samples 🤤

Empty exception lists

Screen Shot 2020-07-28 at 7 06 24 PM

No search results found

Screen Shot 2020-07-28 at 6 37 22 PM

Successful search 🔍

Screen Shot 2020-07-28 at 6 37 38 PM

Checklist

? filterOptions.tags.map((t) => `${namespace}.attributes.tags:${t}`)
: []),
];
const filters: string = filterOptions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if listIds.length !== namespaceTypes.length? Maybe could use some extra validation around that...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 The backend has validation for it x-pack/plugins/lists/server/routes/find_exception_list_item_route.ts (line 50)

);

try {
if (ids.length > 0 && namespaces.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if they are not > 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just doesn't make the call here since there would be no lists to search.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated so if ids are not > 0 then it still calls onSuccess but with defaults.

const abortCtrl = new AbortController();
const { ids, namespaces } = lists
.filter((list) => {
if (showDetectionsListsOnly) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic of filter and reduce looks like is duplicated elsewhere. Just mentioning in case you want to make it into a utility, but am fine if it needs to be duplicated here and later on but would be prone to possible changes/bug fixes in one vs. the other later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to util and added test!

})
);
await waitForNextUpdate();
await waitForNextUpdate();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting there has to be two updates before the expect. I trust it, but that is interesting to me. Is there a comment of why two are needed we can add here? Otherwise I always will be like 🤔 when I read this or work with it later.

Optional change of a comment. Not required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So did a bit of digging. The first one is initialization. I added a note to them all.

Copy link
Contributor

@FrankHassanabad FrankHassanabad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read through it, a few optionals and I might not be the best person to trust giving a front end 👍 but I appreciate the extra conversations about your experience with the react reducer for my education of later when I make front end changes.

Everything looks clean enough and the demo of it looked great. Can't wait to see it in the product.

@yctercero yctercero marked this pull request as ready for review July 29, 2020 20:03
@yctercero yctercero requested review from a team as code owners July 29, 2020 20:03
@elasticmachine
Copy link
Contributor

Pinging @elastic/siem (Team:SIEM)

Copy link
Contributor

@peluja1012 peluja1012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pulled down the branch and did some testing on the feature. Looks to be functioning well. Thanks!

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
securitySolution 7.3MB +2.8KB 7.3MB

page load bundle size

id value diff baseline
lists 269.8KB -24.0B 269.8KB

History

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

@yctercero yctercero merged commit 0756dd3 into elastic:master Jul 29, 2020
yctercero added a commit to yctercero/kibana that referenced this pull request Jul 29, 2020
…lastic#73588)

## Summary

This PR focuses on addressing issues around the pagination and functionality of rules with numerous (2+) exception lists.

- Updated the `use_exception_list.ts` hook to make use of the new multi list find API
- Updated the viewer to make use of the new multi list find API
  - Previously was doing a lot of the filtering and paging manually (and badly) in the UI, now the _find takes care of all that
- Added logic for showing `No results` text if user filter/search returns no items
  - Previously would show the `This rule has not exceptions` text
yctercero added a commit to yctercero/kibana that referenced this pull request Jul 29, 2020
…lastic#73588)

## Summary

This PR focuses on addressing issues around the pagination and functionality of rules with numerous (2+) exception lists.

- Updated the `use_exception_list.ts` hook to make use of the new multi list find API
- Updated the viewer to make use of the new multi list find API
  - Previously was doing a lot of the filtering and paging manually (and badly) in the UI, now the _find takes care of all that
- Added logic for showing `No results` text if user filter/search returns no items
  - Previously would show the `This rule has not exceptions` text
yctercero added a commit that referenced this pull request Jul 30, 2020
…73588) (#73747)

## Summary

This PR focuses on addressing issues around the pagination and functionality of rules with numerous (2+) exception lists.

- Updated the `use_exception_list.ts` hook to make use of the new multi list find API
- Updated the viewer to make use of the new multi list find API
  - Previously was doing a lot of the filtering and paging manually (and badly) in the UI, now the _find takes care of all that
- Added logic for showing `No results` text if user filter/search returns no items
  - Previously would show the `This rule has not exceptions` text
yctercero added a commit that referenced this pull request Jul 30, 2020
…73588) (#73748)

## Summary

This PR focuses on addressing issues around the pagination and functionality of rules with numerous (2+) exception lists.

- Updated the `use_exception_list.ts` hook to make use of the new multi list find API
- Updated the viewer to make use of the new multi list find API
  - Previously was doing a lot of the filtering and paging manually (and badly) in the UI, now the _find takes care of all that
- Added logic for showing `No results` text if user filter/search returns no items
  - Previously would show the `This rule has not exceptions` text
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 30, 2020
* master:
  [Vega][Inspector] Request panel should show correct names for requests (elastic#73655)
  [Security Solution] Update filter (elastic#73350)
  TSVB Inaccurate Group By (elastic#73683)
  [Vega][Inspect panel] Write tutorials and reference (elastic#73262)
  [ML] Removing node info check for file data viz import (elastic#73717)
  check that pathname has been updated. ignore other parts (elastic#73689)
  [build] rewrite source as transpiled JS later in the process (elastic#73749)
  Fix Snapshot Restore /policies/indices API endpoint on Cloud (elastic#73734)
  skip flaky suite (elastic#69783) (elastic#70043)
  [Security Solution][Exceptions] - Updates exception hooks and viewer (elastic#73588)
  skip failing suite (elastic#58815)
  [Canvas][fatal bug] Fix props confusion in TextStylePicker (elastic#73732)
  [DOCS] Changes level offset of monitoring pages (elastic#73573)
  Added close button to toast notifications by migrating to different API that is more widely used in Kibana and Security solution in particular. (elastic#73662)
  [ML] Transforms/DFA: Change action button size back to 'xs'.
  [Metrics UI] Fix evaluating rate-aggregated alerts when there's no normalized value (elastic#73545)
  [Metrics UI] Fix formatting of values in inventory context.reason (elastic#73155)
  [maps] rename GisMap to MapContainer and convert to TS (elastic#73690)
  [APM] docs: remove watcher documentation  (elastic#73485)
@yctercero yctercero deleted the exceptions_ui_find branch October 14, 2020 11:59
@MindyRS MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021
@elasticmachine
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:enhancement Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:SIEM v7.9.0 v7.10.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants