-
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][Endpoint] Update list api summary endpoint to use filter #123476
[Security Solution][Endpoint] Update list api summary endpoint to use filter #123476
Conversation
5c0458e
to
b9df82b
Compare
c4b4519
to
d806b35
Compare
d806b35
to
35d1ef9
Compare
Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt) |
// Do not add up the items by OS if host isolation exception | ||
// As each host exception entry applies to all OSs |
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 commenting the "why" here 👍
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.
😄
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 looking so good! Thanks for adding this filter parameter.
I think we are missing the changes on the Trusted Apps card and on the Host isolation Exceptions card. In those places we can use the new summary api instead of the list one.
page: 1, | ||
per_page: 0, | ||
saved_objects: [], | ||
total: 7, |
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.
Not relevant but I think this total number should be 6 according the counters above
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.
@dasansol92 so this was because the API was returning +1 as total. I went looking for why that was and @paul-tavares helped me figure that out. We made the changes in bdc0c97 to fix that. Also updated the test mock to have the correct total.
x-pack/plugins/lists/server/services/exception_lists/get_exception_list_summary.ts
Outdated
Show resolved
Hide resolved
...ration/endpoint_package_custom_extension/components/fleet_integration_event_filters_card.tsx
Outdated
Show resolved
Hide resolved
review changes
review changes
x-pack/plugins/lists/server/services/exception_lists/get_exception_list_summary.ts
Outdated
Show resolved
Hide resolved
@elasticmachine merge upstream |
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.
Just did a code review, didn't pull down to check but code LGTM.
++ what Frank suggested about integration tests. Could you add an integration test that uses this new filter in https://github.com/elastic/kibana/blob/main/x-pack/test/lists_api_integration/security_and_spaces/tests/summary_exception_lists.ts ?
review suggestions
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 all the tests 👍
LGTM. Make sure you get 👍 's from Detections team and/or security solution platform team
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 the tests!
@elasticmachine merge upstream |
💚 Build SucceededMetrics [docs]Async chunks
History
To update your PR or re-run it, just comment with: cc @ashokaditya |
@ashokaditya I was checking this and I think my comment was missed
Did you created a separated issue for it? |
Oops my bad! I'll update those two in a new PR now. Thanks for looking into this and following up. |
* Update exception list summary api doc fixes elastic/security-team/issues/2513 ref elastic/kibana/pull/123476 * Update api-summary-exception-container.asciidoc * Update docs/detections/api/exceptions/api-summary-exception-container.asciidoc Co-authored-by: Janeen Mikell-Straughn <[email protected]> Co-authored-by: Janeen Mikell-Straughn <[email protected]>
…ation Exception cards to use exception list summary API (#123900) * don't use deprecated class ref /pull/123476 * update TA card to use summary api with filter ref #123476 (review) * update fleet hie card to use summary api ref #123476 (review) * update param types review changes Co-authored-by: Kibana Machine <[email protected]>
…ation Exception cards to use exception list summary API (elastic#123900) * don't use deprecated class ref elastic/pull/123476 * update TA card to use summary api with filter ref elastic#123476 (review) * update fleet hie card to use summary api ref elastic#123476 (review) * update param types review changes Co-authored-by: Kibana Machine <[email protected]>
Summary
Updates the summary endpoint of list API to allow kql filters in requests.
policy detail card:
integration host isolation exception card total
Checklist
Delete any items that are not applicable to this PR.