[Cases] Validate attributes before create SOs #158590

cnasikas · 2023-05-26T16:48:03Z

Summary

This PR uses io-ts's decode utitlity function to validate the attributes before creating cases saved objects.

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2023-05-29T08:33:19Z

Pinging @elastic/response-ops (Team:ResponseOps)

elasticmachine · 2023-05-29T08:33:21Z

Pinging @elastic/response-ops-cases (Feature:Cases)

cnasikas · 2023-05-29T08:33:32Z

@elasticmachine merge upstream

cnasikas · 2023-05-29T08:35:03Z

x-pack/plugins/cases/common/api/cases/user_actions/comment.ts


 export const CommentUserActionRt = rt.strict({
  type: rt.literal(ActionTypes.comment),
  payload: CommentUserActionPayloadRt,
 });

+export const CommentUserActionWithoutIdsRt = rt.strict({


I needed this type so I can validate the attributes passed to the so client when you creating a user action. The attributes do not contain any ids as they are in the references array.

cnasikas · 2023-05-29T08:38:47Z

x-pack/plugins/cases/server/client/cases/mock.ts

@@ -242,24 +241,6 @@ export const basicParams = {
  ...entity,
 };

-export const mappings: ConnectorMappings = [


Moved on level up.

cnasikas · 2023-05-29T08:39:46Z

buildkite test this

jonathan-buttner

Great work, left a couple comments

jonathan-buttner · 2023-05-30T18:43:37Z

x-pack/plugins/cases/server/services/attachments/index.ts

@@ -167,9 +167,11 @@ export class AttachmentService {
    try {
      this.context.log.debug(`Attempting to POST a new comment`);

+      const decodedAttributes = decodeOrThrow(AttachmentTransformedAttributesRt)(attributes);


What do you think about using CommentAttributesRt etc for the io-ts schema instead? When I read this, my first thought is why are we checking that the attributes match the "transformed" schema. I could see that being confusing if you didn't know they were the same.

jonathan-buttner · 2023-05-30T18:49:11Z

x-pack/plugins/cases/server/services/cases/index.ts

@@ -583,7 +585,13 @@ export class CasesService {
  }: PatchCaseArgs): Promise<SavedObjectsUpdateResponse<CaseTransformedAttributes>> {
    try {
      this.log.debug(`Attempting to UPDATE case ${caseId}`);
-      const transformedAttributes = transformAttributesToESModel(updatedAttributes);
+
+      const PartialCaseTransformedAttributesRt = getPartialCaseTransformedAttributesRt();


Forgot to mention when we created getPartialCaseTransformedAttributesRt() but we could probably move it to a global within this file or static variable within the class since it won't change. That way we don't have to create it every time.

jonathan-buttner · 2023-05-30T18:53:58Z

x-pack/plugins/cases/server/services/user_actions/operations/create.test.ts

+        await expect(persister.createUserAction(getRequest())).resolves.not.toThrow();
+      });
+
+      it('throws if closure_type is omitted', async () => {


nit: throws if fields is omitted

jonathan-buttner · 2023-05-30T18:55:43Z

x-pack/plugins/cases/server/services/user_actions/operations/create.test.ts

+        ).resolves.not.toThrow();
+      });
+
+      it('throws if closure_type is omitted', async () => {


nit: throws if owner is omitted

jonathan-buttner · 2023-05-30T19:01:29Z

x-pack/plugins/cases/common/api/cases/comment/index.ts

  PersistableStateAttachmentRt,
 ]);

+export const CommentRequestRt = rt.union([...BasicCommentRequestRt.types, ExternalReferenceSORt]);


Does it work to do:

export const CommentRequestRt = rt.union([BasicCommentRequestRt, ExternalReferenceSORt]);

It seems so!

cnasikas · 2023-05-31T07:39:30Z

buildkite test this

cnasikas · 2023-06-06T09:01:41Z

@elasticmachine merge upstream

kibana-ci · 2023-06-06T10:12:41Z

💚 Build Succeeded

Buildkite Build
Commit: 5ec40bb

Metrics [docs]

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`cases`	175.6KB	175.8KB	+149.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`enterpriseSearch`	19	21	+2
`securitySolution`	413	417	+4
total			+6

Total ESLint disabled count

id	before	after	diff
`enterpriseSearch`	20	22	+2
`securitySolution`	497	501	+4
total			+6

History

💔 Build #131177 failed 50f24f5
💔 Build #131168 failed 50f24f5
💛 Build #130754 was flaky 784dc56
💔 Build #130751 failed 5bf2fde92885dd02e2a95374b7ec3af487ae3473
💚 Build #130715 succeeded 59f1232

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

cc @cnasikas

adcoelho

LGTM so far, I just want to take another look at the tests.

As discussed offline, I checked also other services in x-pack/plugins/cases/server/services/. The alerts service, in particular, felt like it could need a decode before calls to the SOClient. I took a look and it seemed fine because none of the methods there are exposed to the outside anyway, and none uses anything other than case or alert ids but perhaps you could double-check?

adcoelho · 2023-06-06T12:34:45Z

x-pack/plugins/cases/server/services/configure/index.test.ts

+          })
+        ).resolves.not.toThrow();
+      });
+


What happens if no attribute is passed?

js-jankisalvi

LGTM, thanks for adding tests to verify error scenarios. 👍

cnasikas added 7 commits May 26, 2023 17:00

Decode requests in the case service

ba39701

Decode requests in the configure service

81478fd

Merge branch 'main' into decode_cases_services_requests

6b733a6

Decode requests in the mapping service

8bbad6e

Decode requests in the user action service

c836ff7

Merge branch 'main' into decode_cases_services_requests

ffed615

Decode requests in the attachments service

d2a3481

cnasikas added release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) Feature:Cases Cases feature v8.9.0 labels May 26, 2023

cnasikas self-assigned this May 26, 2023

cnasikas added 3 commits May 26, 2023 19:52

Fix test

cbd81e7

Fix types to not include ids added in the references array

6a6c347

Fix types

59f1232

cnasikas marked this pull request as ready for review May 29, 2023 08:33

cnasikas requested a review from a team as a code owner May 29, 2023 08:33

Merge branch 'main' into decode_cases_services_requests

84d6747

cnasikas commented May 29, 2023

View reviewed changes

Improve types

784dc56

cnasikas force-pushed the decode_cases_services_requests branch from 5bf2fde to 784dc56 Compare May 29, 2023 08:37

cnasikas commented May 29, 2023

View reviewed changes

jonathan-buttner reviewed May 30, 2023

View reviewed changes

cnasikas added 2 commits May 31, 2023 09:57

PR feedback

d66e247

Merge branch 'main' into decode_cases_services_requests

50f24f5

cnasikas mentioned this pull request Jun 6, 2023

[Cases][Meta] Road to versioned HTTP APIs #153726

Open

30 tasks

Merge branch 'main' into decode_cases_services_requests

5ec40bb

adcoelho reviewed Jun 6, 2023

View reviewed changes

adcoelho approved these changes Jun 6, 2023

View reviewed changes

js-jankisalvi approved these changes Jun 6, 2023

View reviewed changes

cnasikas merged commit b5adaec into elastic:main Jun 7, 2023

cnasikas deleted the decode_cases_services_requests branch June 7, 2023 15:58

kibanamachine added the backport:skip This commit does not require backporting label Jun 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cases] Validate attributes before create SOs #158590

[Cases] Validate attributes before create SOs #158590

cnasikas commented May 26, 2023 •

edited

Loading

elasticmachine commented May 29, 2023

elasticmachine commented May 29, 2023

cnasikas commented May 29, 2023

cnasikas May 29, 2023

cnasikas May 29, 2023

cnasikas commented May 29, 2023

jonathan-buttner left a comment

jonathan-buttner May 30, 2023

jonathan-buttner May 30, 2023

jonathan-buttner May 30, 2023

jonathan-buttner May 30, 2023

jonathan-buttner May 30, 2023

cnasikas May 31, 2023

cnasikas commented May 31, 2023

cnasikas commented Jun 6, 2023

kibana-ci commented Jun 6, 2023

ESLint disabled line counts

Total ESLint disabled count

adcoelho left a comment

adcoelho Jun 6, 2023

js-jankisalvi left a comment

[Cases] Validate attributes before create SOs #158590

[Cases] Validate attributes before create SOs #158590

Conversation

cnasikas commented May 26, 2023 • edited Loading

Summary

Checklist

For maintainers

elasticmachine commented May 29, 2023

elasticmachine commented May 29, 2023

cnasikas commented May 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnasikas commented May 29, 2023

jonathan-buttner left a 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

cnasikas commented May 31, 2023

cnasikas commented Jun 6, 2023

kibana-ci commented Jun 6, 2023

💚 Build Succeeded

Metrics [docs]

Page load bundle

ESLint disabled line counts

Total ESLint disabled count

History

adcoelho left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

js-jankisalvi left a comment

Choose a reason for hiding this comment

cnasikas commented May 26, 2023 •

edited

Loading