[Cases] Do not check for version conflicts when adding/updating comme…

…nts to a case (elastic#173740)

## Summary

When we add or update a comment (attachment) to a case we update the
`updatedAt` and `updatedBy` attributes of the case. When we update the
case we pass the version of the current case to the SO client for
concurrency control. If the version of the current case is different
from the one we updating (someone else updated the case) the SO client
will throw an error. Although we always fetch the case before adding a
comment it may be possible in some weird race conditions for one Kibana
node to get the case and before updating another node to update the
case. This is extremely difficult to produce when users interact but it
may be possible (still rare) when we introduce the case action. This PR
does not do any concurrency control when updating the `updatedAt` and
`updatedBy` attributes of the case when adding/updating a comment.


### Checklist

Delete any items that are not applicable to this PR.

- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

### For maintainers

- [x] This was checked for breaking API changes and was [labeled
appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

x-pack/plugins/cases/server/common/models/case_with_comments.test.ts

@@ -6,11 +6,15 @@
  */
 
 import type { AlertAttachmentAttributes } from '../../../common/types/domain';
+import { AttachmentType } from '../../../common/types/domain';
 import type { SavedObject } from '@kbn/core-saved-objects-api-server';
 import { createCasesClientMockArgs } from '../../client/mocks';
 import { alertComment, comment, mockCaseComments, mockCases, multipleAlert } from '../../mocks';
 import { CaseCommentModel } from './case_with_comments';
-import { MAX_PERSISTABLE_STATE_AND_EXTERNAL_REFERENCES } from '../../../common/constants';
+import {
+  MAX_PERSISTABLE_STATE_AND_EXTERNAL_REFERENCES,
+  SECURITY_SOLUTION_OWNER,
+} from '../../../common/constants';
 import {
   commentExternalReference,
   commentFileExternalReference,
@@ -25,6 +29,7 @@ describe('CaseCommentModel', () => {
   clientArgs.services.caseService.getCase.mockResolvedValue(theCase);
   clientArgs.services.caseService.patchCase.mockResolvedValue(theCase);
   clientArgs.services.attachmentService.create.mockResolvedValue(mockCaseComments[0]);
+  clientArgs.services.attachmentService.update.mockResolvedValue(mockCaseComments[0]);
   clientArgs.services.attachmentService.bulkCreate.mockResolvedValue({
     saved_objects: mockCaseComments,
   });
@@ -274,6 +279,18 @@ describe('CaseCommentModel', () => {
       expect(clientArgs.services.attachmentService.create).not.toHaveBeenCalled();
     });
 
+    it('partial updates the case', async () => {
+      await model.createComment({
+        id: 'comment-1',
+        commentReq: comment,
+        createdDate,
+      });
+
+      const args = clientArgs.services.caseService.patchCase.mock.calls[0][0];
+
+      expect(args.version).toBeUndefined();
+    });
+
     describe('validation', () => {
       clientArgs.services.attachmentService.countPersistableStateAndExternalReferenceAttachments.mockResolvedValue(
         MAX_PERSISTABLE_STATE_AND_EXTERNAL_REFERENCES
@@ -579,6 +596,21 @@ describe('CaseCommentModel', () => {
       expect(multipleAlertsCall.attributes.index).toEqual(['test-index-3', 'test-index-5']);
     });
 
+    it('partial updates the case', async () => {
+      await model.bulkCreate({
+        attachments: [
+          {
+            id: 'comment-1',
+            ...comment,
+          },
+        ],
+      });
+
+      const args = clientArgs.services.caseService.patchCase.mock.calls[0][0];
+
+      expect(args.version).toBeUndefined();
+    });
+
     describe('validation', () => {
       clientArgs.services.attachmentService.countPersistableStateAndExternalReferenceAttachments.mockResolvedValue(
         MAX_PERSISTABLE_STATE_AND_EXTERNAL_REFERENCES
@@ -619,4 +651,24 @@ describe('CaseCommentModel', () => {
       });
     });
   });
+
+  describe('updateComment', () => {
+    it('partial updates the case', async () => {
+      await model.updateComment({
+        updateRequest: {
+          id: 'comment-id',
+          version: 'comment-version',
+          type: AttachmentType.user,
+          comment: 'my updated comment',
+          owner: SECURITY_SOLUTION_OWNER,
+        },
+        updatedAt: createdDate,
+        owner: SECURITY_SOLUTION_OWNER,
+      });
+
+      const args = clientArgs.services.caseService.patchCase.mock.calls[0][0];
+
+      expect(args.version).toBeUndefined();
+    });
+  });
 });

x-pack/plugins/cases/server/common/models/case_with_comments.ts

@@ -129,7 +129,7 @@ export class CaseCommentModel {
           },
           options,
         }),
-        this.updateCaseUserAndDateSkipRefresh(updatedAt),
+        this.partialUpdateCaseUserAndDateSkipRefresh(updatedAt),
       ]);
 
       await commentableCase.createUpdateCommentUserAction(comment, updateRequest, owner);
@@ -144,11 +144,11 @@ export class CaseCommentModel {
     }
   }
 
-  private async updateCaseUserAndDateSkipRefresh(date: string) {
-    return this.updateCaseUserAndDate(date, false);
+  private async partialUpdateCaseUserAndDateSkipRefresh(date: string) {
+    return this.partialUpdateCaseUserAndDate(date, false);
   }
 
-  private async updateCaseUserAndDate(
+  private async partialUpdateCaseUserAndDate(
     date: string,
     refresh: RefreshSetting
   ): Promise<CaseCommentModel> {
@@ -160,7 +160,6 @@ export class CaseCommentModel {
           updated_at: date,
           updated_by: { ...this.params.user },
         },
-        version: this.caseInfo.version,
         refresh,
       });
 
@@ -242,7 +241,7 @@ export class CaseCommentModel {
           id,
           refresh: false,
         }),
-        this.updateCaseUserAndDateSkipRefresh(createdDate),
+        this.partialUpdateCaseUserAndDateSkipRefresh(createdDate),
       ]);
 
       await Promise.all([
@@ -502,7 +501,7 @@ export class CaseCommentModel {
           }),
           refresh: false,
         }),
-        this.updateCaseUserAndDateSkipRefresh(new Date().toISOString()),
+        this.partialUpdateCaseUserAndDateSkipRefresh(new Date().toISOString()),
       ]);
 
       const savedObjectsWithoutErrors = newlyCreatedAttachments.saved_objects.filter(

x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/patch_comment.ts

@@ -12,6 +12,7 @@ import {
   AlertAttachmentAttributes,
   UserCommentAttachmentAttributes,
   AttachmentType,
+  CaseStatuses,
 } from '@kbn/cases-plugin/common/types/domain';
 import { FtrProviderContext } from '../../../../common/ftr_provider_context';
 
@@ -34,6 +35,7 @@ import {
   updateComment,
   superUserSpace1Auth,
   removeServerGeneratedPropertiesFromSavedObject,
+  updateCase,
 } from '../../../../common/lib/api';
 import {
   globalRead,
@@ -549,6 +551,48 @@ export default ({ getService }: FtrProviderContext): void => {
       }
     });
 
+    describe('partial updates', () => {
+      it('should not result to a version conflict (409) when updating a comment to an updated case', async () => {
+        const postedCase = await createCase(supertest, postCaseReq);
+        const caseWithComments = await createComment({
+          supertest,
+          caseId: postedCase.id,
+          params: postCommentUserReq,
+          expectedHttpCode: 200,
+        });
+
+        /**
+         * Updating the status of the case will
+         * change the version of the case
+         */
+        await updateCase({
+          supertest,
+          params: {
+            cases: [
+              {
+                id: caseWithComments.id,
+                version: caseWithComments.version,
+                status: CaseStatuses['in-progress'],
+              },
+            ],
+          },
+        });
+
+        await updateComment({
+          supertest,
+          caseId: postedCase.id,
+          req: {
+            id: caseWithComments.comments![0].id,
+            version: caseWithComments.comments![0].version,
+            comment: 'my new comment',
+            type: AttachmentType.user,
+            owner: 'securitySolutionFixture',
+          },
+          expectedHttpCode: 200,
+        });
+      });
+    });
+
     describe('rbac', () => {
       const supertestWithoutAuth = getService('supertestWithoutAuth');
 

x-pack/test/cases_api_integration/security_and_spaces/tests/common/comments/post_comment.ts

@@ -1123,6 +1123,36 @@ export default ({ getService }: FtrProviderContext): void => {
       });
     });
 
+    describe('partial updates', () => {
+      it('should not result to a version conflict (409) when adding a comment to an updated case', async () => {
+        const postedCase = await createCase(supertest, postCaseReq);
+
+        /**
+         * Updating the status of the case will
+         * change the version of the case
+         */
+        await updateCase({
+          supertest,
+          params: {
+            cases: [
+              {
+                id: postedCase.id,
+                version: postedCase.version,
+                status: CaseStatuses['in-progress'],
+              },
+            ],
+          },
+        });
+
+        await createComment({
+          supertest,
+          caseId: postedCase.id,
+          params: postCommentUserReq,
+          expectedHttpCode: 200,
+        });
+      });
+    });
+
     describe('rbac', () => {
       afterEach(async () => {
         await deleteAllCaseItems(es);

x-pack/test/cases_api_integration/security_and_spaces/tests/common/internal/bulk_create_attachments.ts

@@ -1512,6 +1512,36 @@ export default ({ getService }: FtrProviderContext): void => {
       });
     });
 
+    describe('partial updates', () => {
+      it('should not result to a version conflict (409) when adding comments to an updated case', async () => {
+        const postedCase = await createCase(supertest, postCaseReq);
+
+        /**
+         * Updating the status of the case will
+         * change the version of the case
+         */
+        await updateCase({
+          supertest,
+          params: {
+            cases: [
+              {
+                id: postedCase.id,
+                version: postedCase.version,
+                status: CaseStatuses['in-progress'],
+              },
+            ],
+          },
+        });
+
+        await bulkCreateAttachments({
+          supertest,
+          caseId: postedCase.id,
+          params: [postCommentUserReq],
+          expectedHttpCode: 200,
+        });
+      });
+    });
+
     describe('rbac', () => {
       afterEach(async () => {
         await deleteAllCaseItems(es);