-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Hide endpoint event filters list in detections tab #102644
[Security Solution][Endpoint] Hide endpoint event filters list in detections tab #102644
Conversation
Pinging @elastic/security-onboarding-and-lifecycle-mgt (Team:Onboarding and Lifecycle Mgt) |
@elasticmachine merge upstream |
…st_in_detections_tab
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.
Approving, but left a question that may result in some refactoring (in either this or a subsequent PR)
@@ -43,6 +44,7 @@ export const useExceptionLists = ({ | |||
namespaceTypes, | |||
notifications, | |||
showTrustedApps = false, | |||
showEventFilters = false, |
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.
I'm not sure what use case outside of the Event Filters/Trusted Apps pages need this hook to pull in Trusted apps and/or event filters. I searched for showTrustedApps
and its not being used today.
Proposal/suggestion:
Is there a way to remove these props and instead have this hook only pull in what it need to show (exceptions)? Feels like that would be the better long term solution. else, we'll likely have to continue to revisit this area of the system every time we add new types of exceptions.
(this might be more of a question for the @elastic/security-detections-response 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.
Yeah! I'm agree that would be better to filter by what we want instead that filter what we want to skip (event filters list and trusted apps list in this case).
I'm not sure why this was done in this way. Is it supposed to have more cain of exceptions lists than the current ones? Is this something user can create and then, yes, we should just filter trusted apps and event filter lists in order to avoid loosing data?
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.
yeah, I'm not sure either, but we can take that on to another (potential refactor) issue.
}); | ||
|
||
test('it properly formats when no filters passed and "showTrustedApps" is true', () => { | ||
const filter = getFilters({}, ['single'], true); | ||
const filter = getFilters({}, ['single'], true, false); |
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 API is getting difficult to read, once we have something such as true, false
on the end mixed with an object and an array. Maintainers and others will have a hard time knowing which true and false belongs to what without reading the source function.
Could we maybe just use an object so that we have named input variables?
Then this API would look something like this and be easier to read in the code base that the second false indicates that it is a showEventFilters
vs showTrustedApps
:
Usually when people get to 2-3 parameters it is easier to read named parameters than guessing what each true, false, etc...
means.
getFilters({
filters: {},
namespaceTypes: ['single'],
showTrustedApps: true,
showEventFilters: false,
})
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.
Left one comment, but don't want to hold this up. Feel free to do what you need to on this one. I will leave a LGTM for ya' since Paul is looking at this closely.
💚 Build SucceededMetrics [docs]Module Count
Async chunks
History
To update your PR or re-run it, just comment with: cc @dasansol92 |
…ections tab (elastic#102644) * Add event filters filter on exception list to hide it in UI * Fixes unit test and added more tests for showEventFilters * fixes test adding showEventFilters test cases * Pass params as js object instead of individual variables Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
💚 Backport successful
This backport PR will be merged automatically after passing CI. |
…ections tab (#102644) (#103097) * Add event filters filter on exception list to hide it in UI * Fixes unit test and added more tests for showEventFilters * fixes test adding showEventFilters test cases * Pass params as js object instead of individual variables Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: David Sánchez <davidsansol92@gmail.com>
Hi @kevinlog, We tested this ticket on the latest 7.14.0-BC3 build and found that the issue is now fixed. There is no entry for event filters under the exception list. Please find below the testing details. Build details:
Hence, marking it as "Validated". Thanks! |
Summary
Hides endpoint event filters from the exception lists in detections tab:
Checklist
For maintainers