[Security Solution][Exceptions] - Update exception item comments to include id #73129

yctercero · 2020-07-23T19:47:24Z

Summary

This PR is somewhat of an intermediary step. Comments on exception list items are denormalized. We initially decided that we would not add uuid to comments, but found that it is in fact necessary. This is intermediary in the sense that what we ideally want to have is a dedicated comments CRUD route.

Also just note that I added a callout for when a version conflict occurs (ie: exception item was updated by someone else while a user is editing the same item).

With this PR users are able to:

Create comments when creating exception list items
Add new comments on exception item update

Users will currently be blocked from:

Deleting comments
Updating comments
Updating exception item if version conflict is found

Conflict callout

TO DO

Probably will need to update the error message? @marrasherrier @MikePaquette
Create route for comments

Checklist

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios
This was checked for keyboard-only and screenreader accessibility
This renders correctly on smaller devices using a responsive layout. (You can test this in your browser
This was checked for cross-browser compatibility

For maintainers

This was checked for breaking API changes and was labeled appropriately
- Yes, it is breaking, API is not yet in prod

yctercero · 2020-07-23T19:49:01Z

x-pack/plugins/lists/common/schemas/request/create_endpoint_list_item_schema.test.ts

@@ -65,7 +65,7 @@ describe('create_endpoint_list_item_schema', () => {
    expect(message.schema).toEqual({});
  });

-  test('it should not validate a "list_id" since it does not required one', () => {
+  test('it should fail validation when supplied a "list_id" since it does not required one', () => {


@rylnd you'd mentioned the wording not validate was a bit misleading, so began updating the wording. Does this seem better?

yctercero · 2020-07-23T19:49:38Z

x-pack/plugins/lists/common/schemas/types/comment.mock.ts


-import { Comments, CommentsArray } from './comments';


Made these singular, was really confusing when it was plural but referred to a single comment schema.

yctercero · 2020-07-23T19:50:50Z

x-pack/plugins/lists/common/schemas/types/comment.ts

-      created_at: t.string, // TODO: Make this into an ISO Date string check,
-      created_by: t.string,
+      comment: NonEmptyString,
+      created_at,


Moved to use the same ones that are in common, so when we update those to be ISO Date types, these are good to go as well.

yctercero · 2020-07-23T19:55:05Z

x-pack/plugins/lists/server/services/exception_lists/utils.ts

-  } else if (existingComment.created_at !== comment.created_at) {
-    throw new ErrorWithStatusCode('Unable to update comment', 403);
-  } else if (comment.comment.trim().length === 0) {
-    throw new ErrorWithStatusCode('Empty comments not allowed', 403);


Now delegated to schema validation.

yctercero · 2020-07-23T19:55:27Z

x-pack/plugins/lists/server/services/exception_lists/utils.ts

-  if (comments != null) {
-    return comments.map((c: CreateComments) => {
-      if (c.comment.trim().length === 0) {
-        throw new ErrorWithStatusCode('Empty comments not allowed', 403);


Now delegated to schema validation.

yctercero · 2020-07-23T19:56:26Z

x-pack/plugins/security_solution/public/common/components/exceptions/builder/index.tsx

@@ -377,7 +377,7 @@ export const ExceptionBuilder = ({
          )}
          <EuiFlexItem grow={1}>
            <BuilderButtonOptions
-              isOrDisabled={disableOr}
+              isOrDisabled={isOrDisabled ? isOrDisabled : disableOr}


If builder consumer specifies that OR should be disabled no matter what, makes sure it does so and is not overwritten by disableOr logic.

yctercero · 2020-07-23T19:57:59Z

...plugins/security_solution/public/common/components/exceptions/edit_exception_modal/index.tsx

-      addError(error, { title: i18n.EDIT_EXCEPTION_ERROR });
-      onCancel();
+      if (error.message.includes('Conflict')) {
+        setHasVersionConflict(true);


If there's a conflict error, am showing a EuiCallout and not closing modal. Let's say a user spent 20 minutes writing a comment, for example, if we just close it, they lose it. This gives the user the chance to copy anything they need to.

yctercero · 2020-07-23T19:59:07Z

...plugins/security_solution/public/common/components/exceptions/edit_exception_modal/index.tsx

@@ -172,11 +178,16 @@ export const EditExceptionModal = memo(function EditExceptionModal({
  );

  const enrichExceptionItems = useCallback(() => {
+    const [exceptionItemToEdit] = exceptionItemsToAdd;


Workaround as OR is disabled for now, so only need to update single item.

yctercero · 2020-07-23T19:59:43Z

...plugins/security_solution/public/common/components/exceptions/edit_exception_modal/index.tsx

@@ -258,6 +269,14 @@ export const EditExceptionModal = memo(function EditExceptionModal({
          </>
        )}

+        {hasVersionConflict && (


@marrasherrier @dontcallmesherryli Any feedback on where this should be shown?

elasticmachine · 2020-07-23T20:04:37Z

Pinging @elastic/siem (Team:SIEM)

marrasherrier · 2020-07-23T20:34:10Z

would it be possible to have the error message outside the modal, like in the bottom right corner? if not, that error message placement looks ok to me. perhaps we could also disable everything besides the cancel button, so users can't ignore the message

...plugins/security_solution/public/common/components/exceptions/edit_exception_modal/index.tsx

peluja1012 · 2020-07-24T01:32:34Z

x-pack/plugins/lists/server/services/exception_lists/utils.ts

-}): Comments => {
-  if (comment.created_by !== user) {
+}): Comment => {
+  if (incomingComment.id == null) {


I'm seeing some issues when trying to edit an exception that has been changed at the same time by the same user (in another browser) or by another user. The steps I'm following are:

Create an exception with one comment in one browser (Firefox)

In another browser (Chrome) click "Edit" on that recently added exception but don't edit it and don't click submit.

Back in the Firefox window edit the exception by adding a new comment

In the Chrome window add a new comment and click submit

Before this change, in the master branch, I get a 403 on PUT /api/exception_lists/items error with the following message:

When trying to update a comment, "created_at" and "created_by" must be present

After this change, in this branch, I get a 401 unathorized error on PUT /api/exception_lists/items and get logged out of Kibana. It looks like this is the line that's throwing the error

kibana/x-pack/plugins/lists/server/services/exception_lists/utils.ts

Line 335 in 4be6378

throw new ErrorWithStatusCode(`Cannot delete comment id: ${matchingCommentByIndex.id}`, 401);

Thanks so much for catching this Pedro. I updated the API to currently be append only. I was doing validations and transformations prior to the version check. I removed much of this and am now just checking that the incoming comments are append only. Still working to have full CRUD in an upcoming release.

peluja1012 · 2020-07-24T02:12:32Z

x-pack/plugins/lists/server/services/exception_lists/utils.ts

+    // cannot delete, only append at the moment
+    // TODO: Add new comments endpoint to fully support CRUD
+    if (!idMatches && index < existingComments.length) {
+      throw new ErrorWithStatusCode(`Cannot delete comment id: ${matchingCommentByIndex.id}`, 401);


I think we're overusing the 401 error code in this file. This seems to be causing unexpected side effects like the user getting logged out of Kibana. We should consider using a different error code like 400 for these types of errors. @FrankHassanabad @madirey thoughts?

Yes, these probably should be 400's. 401 should only be used on failure to authenticate, 403 for attempts to access a resource that the authenticated user does not have permission to access.

Thanks @peluja1012 and @madirey ! I simplified validations and made comments append only. Full CRUD is a wip for now.

x-pack/plugins/lists/server/services/exception_lists/utils.ts

...s/security_solution/public/common/components/exceptions/edit_exception_modal/translations.ts

peluja1012

Thanks for helping us fix these issues with the Edit flow! I left a couple of comments related to issues when editing exceptions concurrently with exception comments.

peluja1012

Pulled updated branch and verified that the issues that I was experiencing with editing an exception with existing comments are no longer there. Thanks for the changes, @yctercero!

kibanamachine · 2020-07-27T22:14:19Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3cfa536

Build metrics

async chunks size

id	value	diff	baseline
securitySolution	7.3MB	+2.7KB	7.3MB

page load bundle size

id	value	diff	baseline
lists	269.4KB	+441.0B	269.0KB
securitySolution	870.7KB	+98.0B	870.6KB
total	-	+539.0B	-

Saved Objects .kibana field count

id	value	diff	baseline
exception-list	38	+1	37
exception-list-agnostic	38	+1	37
total	-	+2	-

History

💔 Build #64048 failed 4be6378

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

…nclude id (elastic#73129) ## Summary This PR is somewhat of an intermediary step. Comments on exception list items are denormalized. We initially decided that we would not add `uuid` to comments, but found that it is in fact necessary. This is intermediary in the sense that what we ideally want to have is a dedicated `comments` CRUD route. Also just note that I added a callout for when a version conflict occurs (ie: exception item was updated by someone else while a user is editing the same item). With this PR users are able to: - Create comments when creating exception list items - Add new comments on exception item update Users will currently be blocked from: - Deleting comments - Updating comments - Updating exception item if version conflict is found

…nclude id (#73129) (#73374) ## Summary This PR is somewhat of an intermediary step. Comments on exception list items are denormalized. We initially decided that we would not add `uuid` to comments, but found that it is in fact necessary. This is intermediary in the sense that what we ideally want to have is a dedicated `comments` CRUD route. Also just note that I added a callout for when a version conflict occurs (ie: exception item was updated by someone else while a user is editing the same item). With this PR users are able to: - Create comments when creating exception list items - Add new comments on exception item update Users will currently be blocked from: - Deleting comments - Updating comments - Updating exception item if version conflict is found

…nclude id (#73129) (#73375) ## Summary This PR is somewhat of an intermediary step. Comments on exception list items are denormalized. We initially decided that we would not add `uuid` to comments, but found that it is in fact necessary. This is intermediary in the sense that what we ideally want to have is a dedicated `comments` CRUD route. Also just note that I added a callout for when a version conflict occurs (ie: exception item was updated by someone else while a user is editing the same item). With this PR users are able to: - Create comments when creating exception list items - Add new comments on exception item update Users will currently be blocked from: - Deleting comments - Updating comments - Updating exception item if version conflict is found

madirey · 2020-07-28T02:28:50Z

I'm late to the party, but LGTM! Thanks for the updates!

* master: (44 commits) [Search] add server logs (elastic#72454) [SIEM][Timelines] Updates timeline template callout text (elastic#73334) Fix App status flaky test (elastic#72853) [Functional Tests] Increase the timeout when locating the tableview] (elastic#73243) Use "Apply_filter_trigger" in dashboard drilldown (elastic#71468) fix dashboard index pattern race condition (elastic#72899) [Functional Tests] Increase waitTime for timelion to fetch the results (elastic#73255) [Functional Tests] Fix flakiness on TSVB chart on switching index patterns test (elastic#73238) updates cypress to v4.11.0 (elastic#73327) [Metrics UI] Saved views bugs (elastic#72518) [Ingest Manager] Convert select agent config step to use combo box (elastic#73172) Exclude `version` from package config attributes that are copied, add safeguard to package config bulk create (elastic#73128) [Security Solution][ML] Updates siem group name to security (elastic#73218) [Security Solution] Show proper icon for termination status of all processes (elastic#73235) [Security Solution][Resolver] Show origin node details in panel on load (elastic#73313) [Security solution] Threat hunting test coverage improvements (elastic#73276) [Security Solution][Exceptions] - Update exception item comments to include id (elastic#73129) [Enterprise Search] Error state UI tweaks to account for current Cloud SSO behavior (elastic#73324) [dev/build/docker_generator] convert to typescript (elastic#73339) [APM] Fix focus map link on service map (elastic#73338) ...

elasticmachine · 2021-09-23T14:29:17Z

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

yctercero added 2 commits July 23, 2020 15:23

updated comments to include id

a7767a9

Merge branch 'master' of github.com:yctercero/kibana into comments_again

259f511

yctercero commented Jul 23, 2020

View reviewed changes

cleanup

4be6378

yctercero self-assigned this Jul 23, 2020

yctercero added release_note:enhancement v7.10.0 v7.9.0 v8.0.0 Team:SIEM labels Jul 23, 2020

yctercero marked this pull request as ready for review July 23, 2020 20:04

yctercero requested review from a team as code owners July 23, 2020 20:04

peluja1012 reviewed Jul 23, 2020

View reviewed changes

...plugins/security_solution/public/common/components/exceptions/edit_exception_modal/index.tsx Show resolved Hide resolved

peluja1012 reviewed Jul 23, 2020

View reviewed changes

...plugins/security_solution/public/common/components/exceptions/edit_exception_modal/index.tsx Outdated Show resolved Hide resolved

peluja1012 reviewed Jul 24, 2020

View reviewed changes

x-pack/plugins/lists/server/services/exception_lists/utils.ts Outdated Show resolved Hide resolved

peluja1012 reviewed Jul 24, 2020

View reviewed changes

...s/security_solution/public/common/components/exceptions/edit_exception_modal/translations.ts Outdated Show resolved Hide resolved

peluja1012 requested changes Jul 24, 2020

View reviewed changes

yctercero added 3 commits July 27, 2020 10:04

Merge branch 'master' of github.com:yctercero/kibana into comments_again

72f5e0a

fixed validation issues for comments, made append only

4095c23

Merge branch 'master' of github.com:yctercero/kibana into comments_again

3cfa536

peluja1012 approved these changes Jul 27, 2020

View reviewed changes

yctercero merged commit 94ed783 into elastic:master Jul 27, 2020

yctercero mentioned this pull request Jul 27, 2020

[7.x] [Security Solution][Exceptions] - Update exception item comments to include id (#73129) #73374

Merged

yctercero mentioned this pull request Jul 27, 2020

[7.9] [Security Solution][Exceptions] - Update exception item comments to include id (#73129) #73375

Merged

yctercero deleted the comments_again branch October 14, 2020 12:00

MindyRS added the Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. label Sep 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Exceptions] - Update exception item comments to include id #73129

[Security Solution][Exceptions] - Update exception item comments to include id #73129

yctercero commented Jul 23, 2020 •

edited

Loading

yctercero Jul 23, 2020

yctercero Jul 23, 2020

yctercero Jul 23, 2020

yctercero Jul 23, 2020

yctercero Jul 23, 2020

yctercero Jul 23, 2020

yctercero Jul 23, 2020

yctercero Jul 23, 2020

yctercero Jul 23, 2020

elasticmachine commented Jul 23, 2020

marrasherrier commented Jul 23, 2020 •

edited

Loading

peluja1012 Jul 24, 2020 •

edited

Loading

yctercero Jul 27, 2020

peluja1012 Jul 24, 2020 •

edited

Loading

madirey Jul 24, 2020 •

edited

Loading

yctercero Jul 27, 2020

peluja1012 left a comment

peluja1012 left a comment

kibanamachine commented Jul 27, 2020

madirey commented Jul 28, 2020

elasticmachine commented Sep 23, 2021

[Security Solution][Exceptions] - Update exception item comments to include id #73129

[Security Solution][Exceptions] - Update exception item comments to include id #73129

Conversation

yctercero commented Jul 23, 2020 • edited Loading

Summary

Conflict callout

TO DO

Checklist

For maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jul 23, 2020

marrasherrier commented Jul 23, 2020 • edited Loading

peluja1012 Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peluja1012 Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

madirey Jul 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peluja1012 left a comment

Choose a reason for hiding this comment

peluja1012 left a comment

Choose a reason for hiding this comment

kibanamachine commented Jul 27, 2020

💚 Build Succeeded

Build metrics

async chunks size

page load bundle size

Saved Objects .kibana field count

History

madirey commented Jul 28, 2020

elasticmachine commented Sep 23, 2021

yctercero commented Jul 23, 2020 •

edited

Loading

marrasherrier commented Jul 23, 2020 •

edited

Loading

peluja1012 Jul 24, 2020 •

edited

Loading

peluja1012 Jul 24, 2020 •

edited

Loading

madirey Jul 24, 2020 •

edited

Loading