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][Detections] Adds UI for bulk applying timeline template #128691

Merged
merged 1 commit into from
Mar 29, 2022

Conversation

banderror
Copy link
Contributor

@banderror banderror commented Mar 28, 2022

Addresses: #93083, https://github.com/elastic/security-team/issues/2078 (internal)

Summary

This PR adds a UI for applying a timeline template to multiple rules in bulk.

  • A new bulk actions menu item to the Rule Management table.
  • A new form flyout for applying a timeline template.
  • Some glue code to connect them.

There are a few issues that I'd like to address in a follow-up PR after the FF:

  1. Resetting already applied templates to None doesn't work because of the way the patchRules function works. This is a known bug in this implementation. We will need to replace patchRules with something else for bulk editing actions.
  2. I need to add some test coverage.

Other notes:

  • I changed some copies to hopefully make it a little bit clearer. Let me know if you want to rephrase.

Screenshots

The template selector doesn't look good on a smaller screen:

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@banderror banderror requested a review from a team March 28, 2022 21:23
@banderror banderror self-assigned this Mar 28, 2022
@banderror banderror added Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team backport:skip This commit does not require backporting release_note:feature Makes this part of the condensed release notes Feature:Rule Management Security Solution Detection Rule Management v8.2.0 labels Mar 28, 2022
@banderror banderror marked this pull request as ready for review March 28, 2022 21:30
@banderror banderror requested a review from a team as a code owner March 28, 2022 21:30
@elasticmachine
Copy link
Contributor

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

@elasticmachine
Copy link
Contributor

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

@banderror banderror force-pushed the bulk-applying-timeline-template branch from 7e8aaee to 3c3b469 Compare March 28, 2022 23:38
@spong
Copy link
Member

spong commented Mar 29, 2022

This appears to be pre-existing, but the focus state of the Timeline Template selector component seems off in Dark Mode and doesn't match the combo box focus state from EUI here:

Within flyout:

Within Edit Rules (also notice how it re-open after selection....weird)

Also found this interesting click behavior where if you click the 'favorite star' when selecting the first component it can't be reselected until focus is lost 😅 . This component needs some 💙 it seems. No need to do any of that here, but maybe open an issue to revisit this component and throw it on the backlog?

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3000 3002 +2

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 +2.6KB

History

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

cc @banderror

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, code reviewed and tested locally -- LGTM! Couple interesting behaviors with the ole Timeline Template component itself (pre-existing), so may want to open an issue to track those, and did notice the behavior where you cannot reset timeline templates to None as mentioned in the description, but 👍 for addressing that in a follow-up.

Other than that testing went great and I was able to apply timeline templates to custom rules just as we can make changes to index patterns or tags. Great to see how little code it takes to start exposing these bulk actions! 🙌 🙂

@banderror banderror merged commit 17086ad into elastic:main Mar 29, 2022
@banderror banderror deleted the bulk-applying-timeline-template branch March 29, 2022 11:42
@banderror
Copy link
Contributor Author

banderror commented Mar 29, 2022

Couple interesting behaviors with the ole Timeline Template component itself (pre-existing), so may want to open an issue to track those

@spong Yup, here we go: #128740 🙂

Great to see how little code it takes to start exposing these bulk actions!

I agree, it was pretty easy. Many thanks to @vitaliidm for making it possible!

@yiyangliu9286
Copy link

Hi @banderror, thanks for the screenshots. I am wondering that can I get a url to actually try and test this?
Regarding to the content changes, I'm looping in @joepeeples to see if there needs to be any edits to adjust here.

@banderror
Copy link
Contributor Author

banderror commented Mar 30, 2022

@yiyangliu9286 At this point the best option for trying and testing it would be to pull from main and run Kibana locally.

Deployment to Cloud from CI won't work because the PR has been closed. I also checked the current 8.2.0 snapshot version on Staging Cloud and it seems to be out of date (way behind main) - I don't know what's the cadence of updating the snapshot builds, I thought it was 1 day, but the current snapshot seems staler than that.

Please ping me if you need any help with running it locally.

Next time I'll make sure to prepare a cloud deployment beforehand.

@yiyangliu9286
Copy link

@yiyangliu9286 At this point the best option for trying and testing it would be to pull from main and run Kibana locally.

Deployment to Cloud from CI won't work because the PR has been closed. I also checked the current 8.2.0 snapshot version on Staging Cloud and it seems to be out of date (way behind main) - I don't know what's the cadence of updating the snapshot builds, I thought it was 1 day, but the current snapshot seems staler than that.

Please ping me if you need any help with running it locally.

Next time I'll make sure to prepare a cloud deployment beforehand.

Thanks for the feedback @banderror. I am a little unfamiliar with the process to run Kibana locally (last time I run it took me 2 hours ish to do). I've also check the staging environment as this has not been updated yet. Next time let's make sure that we have the url or other ways to test it before we merged this branch, which would be ideal.

I have updated my design regarding to @joepeeples's feedback. I think we will need to update Timeline to be capitalized to align with docs that this is a feature. And also update the copy within the warning Callout, here are the lists of things to update:

  • Update copy in the bulk actions list, to be as "Apply Timeline template":

Screen Shot 2022-03-30 at 2 07 16 PM

  • Update warning Callout copy based on what @joepeeples suggested:
  • Capitalizing all the Timeline in the Flyout:

Screen Shot 2022-03-30 at 2 12 17 PM

@banderror I noticed that this PR has already merged, please let me know if we need to start another issue to address these changes, I'd be happy to do that. thanks!

@banderror
Copy link
Contributor Author

@yiyangliu9286 @joepeeples thanks so much for your feedback! I'm going to submit a new PR with the proposed changes and fixes and figure out how to deploy the PR to Cloud. I'll add you to the reviewers when it's ready.

banderror added a commit that referenced this pull request Apr 13, 2022
…emplate (#129491)

**Addresses:** #129294, #93083, elastic/security-team#2078 (internal)
**Related to:** #128691

## Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

- [x] Fix bulk resetting timeline template to **None**
- [x] Fix UI copies
- [ ] Add tests
kibanamachine pushed a commit that referenced this pull request Apr 13, 2022
…emplate (#129491)

**Addresses:** #129294, #93083, elastic/security-team#2078 (internal)
**Related to:** #128691

## Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

- [x] Fix bulk resetting timeline template to **None**
- [x] Fix UI copies
- [ ] Add tests

(cherry picked from commit 62c049b)
kibanamachine added a commit that referenced this pull request Apr 13, 2022
…emplate (#129491) (#130154)

**Addresses:** #129294, #93083, elastic/security-team#2078 (internal)
**Related to:** #128691

## Summary

Summarize your PR. If it involves visual changes include a screenshot or gif.

- [x] Fix bulk resetting timeline template to **None**
- [x] Fix UI copies
- [ ] Add tests

(cherry picked from commit 62c049b)

Co-authored-by: Georgii Gorbachev <georgii.gorbachev@elastic.co>
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:Rule Management Security Solution Detection Rule Management 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
6 participants