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.

Sign up for GitHub

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

Jump to bottom

[Security Solution] Exceptions TTL Follow-up #151952

Merged
merged 8 commits into from Mar 3, 2023
Merged

[Security Solution] Exceptions TTL Follow-up #151952

merged 8 commits into from Mar 3, 2023

Conversation

dplumlee
Copy link
Contributor

@dplumlee dplumlee commented Feb 22, 2023 •
edited by kibanamachine

Summary

Addresses a number of bugs that were found in the original Exceptions TTL PR:

Also strips out the UI elements for endpoint exception TTL - to be reimplemented in a later PR

PUT update route

Adds another update method to the exceptions list client as our existing method uses a PATCH style update wherein any undefined values don't get unset, they just remain unchanged. For the expire_time field, we couldn't use this style, and so this PR switches the update exception list item route to utilize the new method which should act identical in all cases from the users perspective except fixing the existing bug listed above where expire_time fields are unable to be unset.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

@dplumlee dplumlee added bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Detection Alerts Security Detection Alerts Area Team v8.7.0 labels Feb 22, 2023
@dplumlee dplumlee self-assigned this Feb 22, 2023
@dplumlee dplumlee marked this pull request as ready for review February 23, 2023 19:52
@dplumlee dplumlee requested a review from a team as a code owner February 23, 2023 19:52
@dplumlee
Copy link
Contributor Author

@elasticmachine merge upstream

@dplumlee dplumlee requested a review from a team as a code owner February 28, 2023 20:07
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
lists 93 94 +1

Async chunks

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

id before after diff
lists 152.8KB 152.8KB -30.0B
securitySolution 15.7MB 15.7MB +1.2KB
total +1.1KB
Unknown metric groups

API count

id before after diff
lists 208 210 +2

ESLint disabled line counts

id before after diff
securitySolution 428 430 +2

Total ESLint disabled count

id before after diff
securitySolution 506 508 +2

History

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

cc @dplumlee

banderror
banderror approved these changes Mar 1, 2023
View reviewed changes
Copy link
Contributor

@banderror banderror left a comment

Choose a reason for hiding this comment

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

The removed TODO comment LGTM 👍 🙂

@dhurley14 dhurley14 self-requested a review March 1, 2023 17:25
@peluja1012 peluja1012 mentioned this pull request Mar 1, 2023
Closed
dhurley14
dhurley14 approved these changes Mar 3, 2023
View reviewed changes
Copy link
Contributor

@dhurley14 dhurley14 left a comment

Choose a reason for hiding this comment

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

I think this looks great! thanks for the bug fixes!

yctercero
yctercero approved these changes Mar 3, 2023
View reviewed changes
Copy link
Contributor

@yctercero yctercero left a comment •
edited

Choose a reason for hiding this comment

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

Code LGTM. Pulled down and tested:

  • creating non endpoint exception item with ttl
  • waiting to see it expire and it switch over to only showing under expired filter in rule details exceptions tab
  • edited ttl
  • removed ttl from item
  • created endpoint exception and made sure it wasn't showing there
  • created trusted apps and made sure it wasn't showing there
  • made sure I could not create an endpoint exception item with TTL via API
  • made sure I could not modify created_by during update

It would be great to add cypress tests for this flow.

x-pack/plugins/lists/server/services/exception_lists/exception_list_client.ts
@@ -579,6 +578,11 @@ export class ExceptionListClient {

/**
* Update an existing exception list item
*
* NOTE: This method will PATCH the targeted exception list item, not fully overwrite it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we update the exception docs? This may already be in the works, but this is what we have right now I think - https://www.elastic.co/guide/en/security/current/exceptions-api-update-item.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm probably worth updating them since the behavior will be different, I'll reach out to docs team

@dplumlee
Copy link
Contributor Author

dplumlee commented Mar 3, 2023

@yctercero Agreed, have cypress tests coming for this and a few other workflows in this PR

@dplumlee dplumlee merged commit 203fa3a into elastic:main Mar 3, 2023
@dplumlee dplumlee added the backport:prev-minor Backport to the previous minor version (i.e. one version back from main) label Mar 3, 2023
@kibanamachine kibanamachine mentioned this pull request Mar 4, 2023
Merged
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.7

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 4, 2023
@dplumlee
[Security Solution] Exceptions TTL Follow-up (elastic#151952)
4282398 
(cherry picked from commit 203fa3a)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 4, 2023
@dplumlee
[Security Solution] Exceptions TTL Follow-up (elastic#151952)
98d9d15 
(cherry picked from commit 203fa3a)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
✅ 8.7

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

@dplumlee dplumlee deleted the expire-time-follow-up branch March 4, 2023 00:04
kibanamachine added a commit that referenced this pull request Mar 4, 2023
@kibanamachine @dplumlee
[8.7] [Security Solution] Exceptions TTL Follow-up (#151952) (#152645)
729dcc0 
# Backport

This will backport the following commits from `main` to `8.7`:
- [[Security Solution] Exceptions TTL Follow-up
(#151952)](#151952)

<!--- Backport version: 8.9.7 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Davis
Plumlee","email":"56367316+dplumlee@users.noreply.github.com"},"sourceCommit":{"committedDate":"2023-03-03T23:56:39Z","message":"[Security
Solution] Exceptions TTL Follow-up
(#151952)","sha":"203fa3a9552bdcef37cf62473af5792cc8d23a79","branchLabelMapping":{"^v8.8.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","Team:Detection
Alerts","v8.7.0","v8.8.0"],"number":151952,"url":"#151952
Solution] Exceptions TTL Follow-up
(#151952)","sha":"203fa3a9552bdcef37cf62473af5792cc8d23a79"}},"sourceBranch":"main","suggestedTargetBranches":["8.7"],"targetPullRequestStates":[{"branch":"8.7","label":"v8.7.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.8.0","labelRegex":"^v8.8.0$","isSourceBranch":true,"state":"MERGED","url":"#151952
Solution] Exceptions TTL Follow-up
(#151952)","sha":"203fa3a9552bdcef37cf62473af5792cc8d23a79"}}]}]
BACKPORT-->

---------

Co-authored-by: Davis Plumlee <56367316+dplumlee@users.noreply.github.com>
Co-authored-by: Davis Plumlee <davis.plumlee@elastic.co>
@dplumlee dplumlee mentioned this pull request Mar 5, 2023
Merged
4 tasks
dplumlee added a commit that referenced this pull request Mar 5, 2023
@dplumlee
[Security Solution] Fixes breaking type in main (#152654)
05f306c 
Addresses #152653

Fixes a merge conflict that somehow made its way into main between these
two PRs ([1](#152301),
[2](#151952))
sloanelybutsurely pushed a commit to sloanelybutsurely/kibana that referenced this pull request Mar 8, 2023
@dplumlee
[Security Solution] Exceptions TTL Follow-up (elastic#151952)
35584d9
sloanelybutsurely pushed a commit to sloanelybutsurely/kibana that referenced this pull request Mar 8, 2023
@dplumlee
[Security Solution] Fixes breaking type in main (elastic#152654)
0baac57 
Addresses elastic#152653

Fixes a merge conflict that somehow made its way into main between these
two PRs ([1](elastic#152301),
[2](elastic#151952))
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
@dplumlee @bmorelli25
[Security Solution] Exceptions TTL Follow-up (elastic#151952)
1761a40
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
@dplumlee @bmorelli25
[Security Solution] Fixes breaking type in main (elastic#152654)
e5419d2 
Addresses elastic#152653

Fixes a merge conflict that somehow made its way into main between these
two PRs ([1](elastic#152301),
[2](elastic#151952))
@nastasha-solomon nastasha-solomon mentioned this pull request Mar 21, 2023
Closed
15 tasks
nkhristinin pushed a commit that referenced this pull request Mar 22, 2023
@dplumlee @nkhristinin
[Security Solution] Exceptions TTL Follow-up (#151952)
0d17b05
nkhristinin pushed a commit that referenced this pull request Mar 22, 2023
@dplumlee @nkhristinin
[Security Solution] Fixes breaking type in main (#152654)
6893603 
Addresses #152653

Fixes a merge conflict that somehow made its way into main between these
two PRs ([1](#152301),
[2](#151952))
@nastasha-solomon nastasha-solomon mentioned this pull request Mar 22, 2023
Closed
15 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marshallmain marshallmain marshallmain left review comments

@dhurley14 dhurley14 dhurley14 approved these changes

@banderror banderror banderror approved these changes

@yctercero yctercero yctercero approved these changes

Assignees

@dplumlee dplumlee

Labels
backport:prev-minor Backport to the previous minor version (i.e. one version back from main) bug Fixes for quality problems that affect the customer experience release_note:skip Skip the PR/issue when compiling release notes Team:Detection Alerts Security Detection Alerts Area Team v8.7.0 v8.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@dplumlee @kibana-ci @kibanamachine @dhurley14 @banderror @yctercero @marshallmain