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] Add the ability to filter by index pattern to the rules management table #128245

Merged
merged 1 commit into from Mar 29, 2022

Conversation

xcrzx
Copy link
Contributor

@xcrzx xcrzx commented Mar 22, 2022

Partially addresses: #125596, https://github.com/elastic/security-team/issues/1972
Resolves: #126166

Summary

Added the ability to sort detection rules by index pattern and MITRE ATT&CK tactics or techniques.

1. Filter by index pattern

Screenshot 2022-03-24 at 11 52 29

2. Filter by MITRE ATT&CK tactic or technique name

Screenshot 2022-03-24 at 11 42 51

3. Filter by MITRE ATT&CK tactic or technique id

Screenshot 2022-03-24 at 11 46 44

Other changes

In the in-memory implementation, rules are not filtered on the client-side anymore. Instead, we fetch filtered rules from the server and only sort and paginate them in memory. This change was necessary to get rid of discrepancies between the two implementations. See #126166 for more detail.

@xcrzx xcrzx added backport:skip This commit does not require backporting Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. release_note:feature Makes this part of the condensed release notes Feature:Alerting/RulesManagement Issues related to the Rules Management UX Team:Detection Rule Management Security Detection Rule Management Team v8.2.0 labels Mar 22, 2022
@xcrzx xcrzx self-assigned this Mar 22, 2022
@xcrzx xcrzx force-pushed the index-pattern-filter branch 7 times, most recently from 0581c6a to 8ef2d28 Compare March 24, 2022 13:18
@xcrzx xcrzx marked this pull request as ready for review March 24, 2022 13:44
@xcrzx xcrzx requested a review from a team as a code owner March 24, 2022 13:44
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@xcrzx
Copy link
Contributor Author

xcrzx commented Mar 24, 2022

@yiyangliu9286 As a follow-up to our discussion, could you please review the changes to the rules table header and filters?

Screenshot 2022-03-24 at 17 50 50

  • I removed the "All rules" title
  • Extended the search bar to the full width
  • Moved the "last updated" label to the right side

I kept the refresh button as it was before as there's no ready-to-use UI component at the moment (or I am missing something 🤔). So I would propose implementing it separately and updating all our tables at once.

Also, I kept the Custom/Elastic rules toggle in its place as there are not too many other filters, so enough space. But please let me know if you think we should move that toggle.

@yiyangliu9286
Copy link

@yiyangliu9286 As a follow-up to our discussion, could you please review the changes to the rules table header and filters?

Screenshot 2022-03-24 at 17 50 50
  • I removed the "All rules" title
  • Extended the search bar to the full width
  • Moved the "last updated" label to the right side

I kept the refresh button as it was before as there's no ready-to-use UI component at the moment (or I am missing something 🤔). So I would propose implementing it separately and updating all our tables at once.

Also, I kept the Custom/Elastic rules toggle in its place as there are not too many other filters, so enough space. But please let me know if you think we should move that toggle.

Hi @xcrzx! Thanks so much for this quick implementation updates, this looks awesome!

I kept the refresh button as it was before as there's no ready-to-use UI component at the moment (or I am missing something 🤔). So I would propose implementing it separately and updating all our tables at once.

I agree that we can keep the current table actions as is because the new table utility bar component haven't been made to an EUI component yet (as far as I know of). One small question, do you think if it makes sense to implement the "Auto refresh" and the "Refresh" button as part of this release's changes, since the EuiAutoRefreshButton component is ready to use (i know that in our case, we need some customization to the existing component). Or we should make this as an UX/UI enhancement as part of the 8.3 cycle? I am happy for either decision based on your time and capacity.

Screen Shot 2022-03-24 at 11 46 15 AM

Screen Shot 2022-03-24 at 11 46 25 AM

@yiyangliu9286 yiyangliu9286 self-requested a review March 24, 2022 17:14
@banderror
Copy link
Contributor

One small question, do you think if it makes sense to implement the "Auto refresh" and the "Refresh" button as part of this release's changes, since the EuiAutoRefreshButton component is ready to use (i know that in our case, we need some customization to the existing component). Or we should make this as an UX/UI enhancement as part of the 8.3 cycle?

Let's work on it separately and ship in a different PR to keep our PRs single-purpose.

@yiyangliu9286 It would be super helpful if we had a ticket describing the design changes that we'd like to apply to the Rule Management and Monitoring tables. While it's clear how we could move filters from one place on the table to another, I'd be willing to sync on the new table refresh button in more detail: its behavior and UI states.

@banderror
Copy link
Contributor

I kept the refresh button as it was before as there's no ready-to-use UI component at the moment (or I am missing something 🤔).

@xcrzx If <EuiAutoRefresh> is not available in Kibana, it could be that EUI folks have merged it to the EUI repo, but haven't upgraded EUI in Kibana yet. The next upgrade is planned to be done in 8.3 (#128313).

@xcrzx
Copy link
Contributor Author

xcrzx commented Mar 25, 2022

@xcrzx If <EuiAutoRefresh> is not available in Kibana, it could be that EUI folks have merged it to the EUI repo, but haven't upgraded EUI in Kibana yet. The next upgrade is planned to be done in 8.3 (#128313).

Thanks, @banderror! I couldn't find it in the docs for some reason. It looks like we can use it but need some exploration on how we wire it up with other components. Let's do that separately, as there's not much time left before FF.

@xcrzx xcrzx force-pushed the index-pattern-filter branch 3 times, most recently from 5d600d1 to a9c358d Compare March 28, 2022 14:10
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Test Failures

  • [job] [logs] Security Solution Tests / value lists user with restricted access role "before each" hook for "Does not allow a t1 analyst user to upload a value list"

Metrics [docs]

Async chunks

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

id before after diff
securitySolution 4.8MB 4.8MB -252.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
securitySolution 441 440 -1

Total ESLint disabled count

id before after diff
securitySolution 510 509 -1

History

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

cc @xcrzx

Comment on lines +93 to +96
// Check that the rules table shows at least one row
cy.get(RULES_TABLE).find(RULES_ROW).should('have.length.gte', 1);
// Check that the rules table doesn't show the rule from the first page
cy.get(RULES_TABLE).should('not.contain', ruleNameFirstPage);
Copy link
Member

Choose a reason for hiding this comment

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

cc @MadameSheema -- not sure if you're still tracking cypress changes, but here's a few more. Let me know if you'd like to stop receiving these pings! 🙂

@@ -408,7 +401,7 @@ export const SEARCH_RULES = i18n.translate(
export const SEARCH_PLACEHOLDER = i18n.translate(
'xpack.securitySolution.detectionEngine.rules.allRules.searchPlaceholder',
{
defaultMessage: 'e.g. rule name',
defaultMessage: 'Search by rule name, index pattern, or MITRE ATT&CK tactic or technique',
Copy link
Member

Choose a reason for hiding this comment

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

May want the as we're adding it elsewhere in the UI:

Suggested change
defaultMessage: 'Search by rule name, index pattern, or MITRE ATT&CK tactic or technique',
defaultMessage: 'Search by rule name, index pattern, or MITRE ATT&CK tactic or technique',

Comment on lines +12 to +19
const SEARCHABLE_RULE_PARAMS = [
'alert.attributes.name',
'alert.attributes.params.index',
'alert.attributes.params.threat.tactic.id',
'alert.attributes.params.threat.tactic.name',
'alert.attributes.params.threat.technique.id',
'alert.attributes.params.threat.technique.name',
];
Copy link
Member

Choose a reason for hiding this comment

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

Was subtechnique not added as a searchable param? You know if there was any particular reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks, @spong! I'll add subtechnique to the searchable params as well.

Comment on lines +34 to +38
const filters: string[] = [];

if (showCustomRules) {
filters.push(`alert.attributes.tags: "${INTERNAL_IMMUTABLE_KEY}:false"`);
}
Copy link
Member

Choose a reason for hiding this comment

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

This function is far more readable -- thank you @xcrzx!


if (tags.length > 0) {
filters.push(
`alert.attributes.tags:(${tags.map((tag) => `"${escapeKuery(tag)}"`).join(' AND ')})`
Copy link
Member

Choose a reason for hiding this comment

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

++ for bringing in the common escapeKuery 👍

{
refetchInterval,
enabled: isInMemorySorting,
keepPreviousData: true, // Use this option so that the state doesn't jump between "success" and "loading" on page change
Copy link
Member

Choose a reason for hiding this comment

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

Thank-you for the comment here 👍

@@ -408,7 +401,7 @@ export const SEARCH_RULES = i18n.translate(
export const SEARCH_PLACEHOLDER = i18n.translate(
'xpack.securitySolution.detectionEngine.rules.allRules.searchPlaceholder',
{
defaultMessage: 'e.g. rule name',
defaultMessage: 'Search by rule name, index pattern, or MITRE ATT&CK tactic or technique',
Copy link
Member

Choose a reason for hiding this comment

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

Also noticed a small UX item here: if a user copies the whole tactic/technique from Rule Details, ie. Defense Evasion (TA0005) and pastes that in they won't get any results since id/name are independent fields -- they would need to search Defense Evasion or TA0005. Not too much we could do here outside updating the placeholder text on the search bar to include an example maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I agree 👍 Providing an example would make this UI a bit friendlier to our users.

Copy link
Member

@spong spong left a comment

Choose a reason for hiding this comment

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

Checked out, tested locally, and performed code review. LGTM @xcrzx! 👍

Couple small nits, and a potential copy change that may need to be updated, but everything else looks and tested great! Thanks for the additional cleanup you've provided along the way as well! 🙂

@xcrzx xcrzx merged commit eb51ea6 into elastic:main Mar 29, 2022
@xcrzx
Copy link
Contributor Author

xcrzx commented Mar 29, 2022

Thanks for your review, @spong! I'm going to add a tour showing the new capabilities in the next PR, so I will also include changes to copy and filter by subtechnique there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Alerting/RulesManagement Issues related to the Rules Management UX release_note:feature Makes this part of the condensed release notes Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. v8.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Security Solution] Bulk actions in in-memory mode affect the wrong number of rules
6 participants