[SIEM] move away from Joi for importing/exporting timeline #62125
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,31 +7,29 @@ | |
| import { set as _set } from 'lodash/fp'; | ||
| import { IRouter } from '../../../../../../../../src/core/server'; | ||
| import { LegacyServices } from '../../../types'; | ||
|
|
||
| import { transformError, buildSiemResponse } from '../../detection_engine/routes/utils'; | ||
| import { TIMELINE_EXPORT_URL } from '../../../../common/constants'; | ||
|
|
||
| import { getExportTimelineByObjectIds } from './utils/export_timelines'; | ||
| import { | ||
| exportTimelinesQuerySchema, | ||
| exportTimelinesRequestBodySchema, | ||
| buildRouteValidation, | ||
| } from './schemas/export_timelines_schema'; | ||
| import { ExportTimelinesQuery, ExportTimelinesRequestBody } from '../types'; | ||
|
|
||
| export const exportTimelinesRoute = (router: IRouter, config: LegacyServices['config']) => { | ||
| router.post( | ||
| { | ||
| path: TIMELINE_EXPORT_URL, | ||
| validate: { | ||
|
There was a problem hiding this comment. @angorayc I was imagining that once Joi was gone, this It looks like we're currently loosely typing the request with There was a problem hiding this comment. @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: 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. There was a problem hiding this comment. 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 There was a problem hiding this comment. 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. There was a problem hiding this comment. Thanks @FrankHassanabad , at the same time I found some code useful for validation,
This comment was marked as resolved.
Sorry, something went wrong. There was a problem hiding this comment. Note: if you need to override the output type above ( 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. There was a problem hiding this comment. 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. There was a problem hiding this comment. I found in x-pack/plugins/case/common/api/runtime_types.ts Line. 37 e.g.: Use case:
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. There was a problem hiding this comment. Ok, we can address this as part of #63525. |
||
| query: buildRouteValidation<ExportTimelinesQuery, ExportTimelinesQuery, unknown>( | ||
| exportTimelinesQuerySchema | ||
| ), | ||
| body: buildRouteValidation<ExportTimelinesRequestBody, ExportTimelinesRequestBody, unknown>( | ||
| exportTimelinesRequestBodySchema | ||
| ), | ||
| }, | ||
| options: { | ||
| tags: ['access:siem'], | ||
|
|
@@ -42,6 +40,7 @@ export const exportTimelinesRoute = (router: IRouter, config: LegacyServices['co | |
| const siemResponse = buildSiemResponse(response); | ||
| const savedObjectsClient = context.core.savedObjects.client; | ||
| const exportSizeLimit = config().get<number>('savedObjects.maxImportExportSize'); | ||
|
|
||
| if (request.body?.ids != null && request.body.ids.length > exportSizeLimit) { | ||
| return siemResponse.error({ | ||
| statusCode: 400, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -334,7 +334,10 @@ describe('import timelines', () => { | |
| const result = server.validate(request); | ||
|
|
||
| expect(result.badRequest).toHaveBeenCalledWith( | ||
| [ | ||
| 'Invalid value undefined supplied to : { file: (ReadableRt & { hapi: { filename: string } }) }/file: (ReadableRt & { hapi: { filename: string } })/0: ReadableRt', | ||
|
There was a problem hiding this comment. 😅 that's quite the error message ... harder to parse, but more info than before! There was a problem hiding this comment. I see how you mean, I found some example in case as well, please find this comment #62125 (comment) |
||
| 'Invalid value undefined supplied to : { file: (ReadableRt & { hapi: { filename: string } }) }/file: (ReadableRt & { hapi: { filename: string } })/1: { hapi: { filename: string } }', | ||
| ].join('\n') | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
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 think this comment (and the function that follows it) are now moved elsewhere, and can be deleted
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.
@angorayc I think the
createRulesStreamFromNdJsondefinition below is dead code, but we can clean it up in a subsequent PR if you'd like.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.
My bad, I shouldn't have put
createRulesStreamFromNdJsoninto server/utils as this is only consumed byimport_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.