[SIEM] move away from Joi for importing/exporting timeline

[angorayc]

Summary

1. This PR is to remove the schemas created with Joi and reuse the existing io-ts schemas
2. After migrating to io-ts, this case should work as well
   timelines_export_noFilterQuery.ndjson.txt
   (To verify this, please download the attachment and rename it with .ndjson)
3. The validation error is updated accordingly due to the migration:

e.g:

Request body takes:

{
  ids: string[];
}

but we send { id: 'someId' } instead

Before the PR, the error message was

'"id" is not allowed'

After the PR, the error message becomes

'Invalid value undefined supplied to : { ids: Array<string> }/ids: Array<string>'

Checklist

Delete any items that are not applicable to this PR.

• [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
• [ ] Documentation was added for features that require explanation or tutorials
• 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, including a check against IE11

For maintainers

• [ ] This was checked for breaking API changes and was labeled appropriately

[elasticmachine]

Pinging @elastic/siem (Team:SIEM)

[angorayc]

.allow(null)

[rylnd]

@angorayc I was imagining that once Joi was gone, this buildRouteValidation function would be modified to take an io-ts schema and perform whatever decoding/validation needs to happen.

It looks like we're currently loosely typing the request with @kbn/config-schema in the validation step, and then refining that further within the route handler with io-ts; is it possible to simply use io-ts in the validation step, similar to what buildRouteValidation was doing? If we can we should, as that removes the possibility of interacting with these "loose" types in the handler and losing type safety there (and removes the double validation).

[FrankHassanabad]

@rylnd , @angorayc Here is my draft PR where I am doing what @rylnd is suggesting for detection engine:

Example of where I validate incoming using io-ts to search for:

validate: {
        body: buildRouteValidationIoTS<CreateListsSchema>(createListsSchema),
      },

My function which performs an io-ts validation similar to the one above it that @rylnd is using:
https://github.com/elastic/kibana/pull/62552/files#diff-8b8c8b8c0404c58c669ab9551d6f2e8bL238-L239

export const buildRouteValidationIoTS = <T>(schema: t.Mixed): RouteValidationFunction<T> => (
  payload: object,
  { ok, badRequest }
) => {
  const decoded = schema.decode(payload);
  const checked = exactCheck(payload, decoded);
  const left = (errors: t.Errors): RouteValidationFunctionReturn<T> => {
    return badRequest(formatErrors(errors).join(','));
  };
  const right = (output: T): RouteValidationFunctionReturn<T> => {
    return ok(output);
  };
  return pipe(checked, fold(left, right));
};

This is very draft so again, refactoring/changing things afterwards is good but if it helps I can begin pulling in some of these utilities as soon as this the beginning of this week @rylnd to start helping reducing our usage of schemas and establishing better patterns.

It might be best I begin pulling in my pieces from draft mode in smaller PR's Monday and getting those reviewed. Then we can migrate any other things like this after 7.7.0 is released as the time is running out quickly?

I don't want to hold anything up though. Refactoring can happen after this PR too and my pieces.

[angorayc]

That's beautiful @FrankHassanabad, @rylnd I've been trying to figure this out for a while! If I'm going to reuse it in timeline's route once it's merged, would you recommend importing from this file or there's better way of how we could reuse it? As currently I use lots of stuff from detection engine etc., not sure if we could have an utils for all the routes? Or is it possible if we implement this part in middleware so we can just provide the validation schemas in each route without implementing buildRouteValidationIoTS ?

[FrankHassanabad]

Yeah it's tricky at the moment but we should be promoting things "up" as you are suggesting and out of detection engine as they see more re-use. In your PR's you can move things around as you see fit or if we have some duplication we can deconflict as well.

It's tricky as we are all working in the same areas but getting code in first and working is probably easier and then refactors second once people move to other areas maybe?

I am fine with either approach and I like your ideas. The middle ware idea is a good one but I haven't written those before with Hapi or the Kibana platform but am open to any and all ideas if you want to give it a try.

[angorayc]

Thanks @FrankHassanabad , at the same time I found some code useful for validation,
https://elastic.slack.com/archives/CCJQY1NS0/p1584130424322000
It matches what I thought and fixes the incorrect error message I had, what do you think?

[rylnd]

Note: if you need to override the output type above (A), you'll have to do something like:

buildRouteValidation<t.Any, OverridingResponseType>(schema)

It feels like there should be a way to support overriding the type with a single generic argument, but I hit the limits of my typescript knowledge trying to make that work.

[angorayc]

Thanks @rylnd , I've applied the suggested type and it works well. I agree that we should warning user if their input got dropped, and given that @FrankHassanabad has implemented the solution nicely, I'd like to park here and creating an issue to keep track of this.
https://github.com/elastic/siem-team/issues/615

[angorayc]

I found in x-pack/plugins/case/common/api/runtime_types.ts Line. 37
Seems it's doing similar thing as exact check, which means when validation error occurs, it return the original object.

e.g.:
I updated the buildValidation:

export const buildRouteValidation = <T extends rt.Mixed, A = rt.TypeOf<T>>(
  schema: T
): RouteValidationFunction<A> => (
  inputValue: unknown,
  validationResult: RouteValidationResultFactory
) =>
  pipe(
    excess(schema).decode(inputValue),
    fold<rt.Errors, A, RequestValidationResult<A>>(
      (errors: rt.Errors) => validationResult.badRequest(failure(errors).join('\n')),
      (validatedInput: A) => validationResult.ok(validatedInput)
    )
  );

Use case:
I sent this as request body:
{ id: 'someId' }
but it's expecting
{ id: string[] }

The error message was
"Invalid value undefined supplied to : { ids: Array }/ids: Array"
It becomes
Received: "Invalid value {"id":"someId"} supplied to : { ids: Array }, excess properties: ["id"]"

Given that io-ts by default drops the redundant keys and provides the error messages not easy to read, I can see why we have patches for that.

[rylnd]

Ok, we can address this as part of #63525.

[rylnd]

@angorayc it looks like this PR is still a WIP so these are just comments, but I think we can make this even slicker than it is right now!

[rylnd]

I think we've got some redundant code and types that should be cleaned up, but we're so close! Thank you for adding/updating tests and for taking on establishment of this validation pattern 🙏

[rylnd]

I think this comment (and the function that follows it) are now moved elsewhere, and can be deleted

[rylnd]

@angorayc I think the createRulesStreamFromNdJson definition below is dead code, but we can clean it up in a subsequent PR if you'd like.

[angorayc]

My bad, I shouldn't have put createRulesStreamFromNdJson into server/utils as this is only consumed by import_rules_route, I'll remove the one in server/utils and keep this one here.

Regarding to the file create_rules_stream_from_ndjson.ts in server/utils, I renamed it to create_stream_from_ndjson.ts.

[rylnd]

😅 that's quite the error message ... harder to parse, but more info than before!

[angorayc]

I see how you mean, I found some example in case as well, please find this comment #62125 (comment)

[rylnd]

LGTM. I think we still have some dead code but up to you whether you want to clean that up here or subsequently. Thanks for the feedback changes!

[rylnd]

@angorayc I think the createRulesStreamFromNdJson definition below is dead code, but we can clean it up in a subsequent PR if you'd like.

[rylnd]

Ok, we can address this as part of #63525.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 2689f3b

History

• 💔 Build #41075 failed ddf4409
• 💚 Build #41008 succeeded 4f45e17
• 💚 Build #40805 succeeded 81b9859
• 💚 Build #40681 succeeded 4655aea
• 💚 Build #40608 succeeded f8b3b03

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

[elasticmachine]

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