-
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
[Lists][Exceptions] - Updates exception list item comments structure #68864
Conversation
Pinging @elastic/siem (Team:SIEM) |
💚 Build SucceededTo update your PR or re-run it, just comment with: |
comments: { | ||
properties: { | ||
comment: { | ||
type: 'keyword', |
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 curious if we could change comment type to text
instead of keyword
so the full content of the comment is searchable?
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.
Good call. I noticed other ones like description
was set to keyword
. Wasn't sure if that was for a specific reason so I defaulted to keyword
. Will update!
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.
No worries I was just curious if it's something we might want. Open to hearing from others with their thoughts on this. Not a big deal either way I think.
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.
Per our chat, I'm planning on updating these in the upcoming PR that addresses entries
. I'll link to it once its up as a follow up.
expect(getPaths(left(message.errors))).toEqual(['invalid keys "extraKey"']); | ||
expect(message.schema).toEqual({}); | ||
}); | ||
}); |
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 test!
|
||
// TODO: Is it expected behavior for it not to auto-generate a uui or throw | ||
// error if item_id is not passed in? | ||
xtest('it should accept an undefined for "item_id" and auto generate a uuid', () => { |
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.
@FrankHassanabad I think I might have seen a to do related to this, but just wanted to double check the expected behavior 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.
Looks great! Thanks for the moving so fast on these and the tests!
…lastic#68864) ### Summary This is part of a series of upcoming changes to the exception list item structure. This PR focuses solely on updating exception_item.comment. The hope is to keep these PRs relatively small. - Updates exception_item.comment structure which was previously a string to exception_item.comments which is an array of { comment: string; created_by: string; created_at: string; } - Adds a few unit tests server side - Fixes some minor misspellings - Updates ExceptionViewer component in the UI to account for new structure
…cture (#68864) (#68907) * [Lists][Exceptions] - Updates exception list item comments structure (#68864) ### Summary This is part of a series of upcoming changes to the exception list item structure. This PR focuses solely on updating exception_item.comment. The hope is to keep these PRs relatively small. - Updates exception_item.comment structure which was previously a string to exception_item.comments which is an array of { comment: string; created_by: string; created_at: string; } - Adds a few unit tests server side - Fixes some minor misspellings - Updates ExceptionViewer component in the UI to account for new structure * Update API review file per CI instructions
…nt structure (#69532) ### Summary This PR is a follow up to #68864 . That PR used a partial to differentiate between new and existing comments, this meant that comments could be updated when they shouldn't. It was decided in our discussion of exception list schemas that comments should be append only. This PR assures that's the case, but also leaves it open to editing comments (via API). It checks to make sure that users can only update their own comments.
…nt structure (elastic#69532) ### Summary This PR is a follow up to elastic#68864 . That PR used a partial to differentiate between new and existing comments, this meant that comments could be updated when they shouldn't. It was decided in our discussion of exception list schemas that comments should be append only. This PR assures that's the case, but also leaves it open to editing comments (via API). It checks to make sure that users can only update their own comments.
…nt structure (#69532) (#70107) ### Summary This PR is a follow up to #68864 . That PR used a partial to differentiate between new and existing comments, this meant that comments could be updated when they shouldn't. It was decided in our discussion of exception list schemas that comments should be append only. This PR assures that's the case, but also leaves it open to editing comments (via API). It checks to make sure that users can only update their own comments.
Pinging @elastic/security-solution (Team: SecuritySolution) |
Summary
This is part of a series of upcoming changes to the exception list item structure. This PR focuses solely on updating
exception_item.comment
. The hope is to keep these PRs relatively small.exception_item.comment
structure which was previously astring
toexception_item.comments
which is an array of{ comment: string; created_by: string; created_at: string; }
ExceptionViewer
component in the UI to account for new structureSnapshots
As you can see, comments are now appearing on the UI as expected.
Testing
To turn on lists plugin - in kibana.dev.yml
Use the scripts in
x-pack/plugins/lists/server/scripts
to create some sample exception lists and items. You can use the following:./post_exception_list.sh ./exception_lists/new/exception_list_detection.json
./post_exception_list_item.sh ./exception_lists/new/exception_list_item_detection_auto_id.json
- this script auto generates the item_id so you can run it as many times as you like to create multiple items associated with the list generated in step 1./post_exception_list.sh
./post_exception_list_item.sh ./exception_lists/new/exception_list_item_auto_id.json
- this script auto generates the item_id so you can run it as many times as you like to create multiple items associated with the list generated in step 3./find_exception_lists.sh
to get the id of the two lists you createdExceptionsViewer
component inx-pack/plugins/security_solution/public/alerts/pages/detection_engine/rules/details/index.tsx
to something like the following:Navigate to the rules details page and click on the 'Exceptions' tab. Voila!
Checklist