From 711c098e9b2a8329c58c9674fc99de23842884d1 Mon Sep 17 00:00:00 2001 From: Gidi Meir Morris Date: Thu, 2 Apr 2020 19:42:57 +0100 Subject: [PATCH] replaces valdiation typing --- .../server/es/cluster_client_adapter.test.ts | 14 ++--- .../server/es/cluster_client_adapter.ts | 7 +-- .../event_log/server/event_log_client.test.ts | 52 +------------------ .../event_log/server/event_log_client.ts | 30 ++++------- .../server/lib/date_from_string.test.ts | 46 ++++++++++++++++ .../event_log/server/lib/date_from_string.ts | 46 ++++++++++++++++ .../server/lib/route_validator_by_type.ts | 25 +++++++++ .../plugins/event_log/server/routes/find.ts | 18 ++++--- .../event_log/public_api_integration.ts | 3 ++ 9 files changed, 154 insertions(+), 87 deletions(-) create mode 100644 x-pack/plugins/event_log/server/lib/date_from_string.test.ts create mode 100644 x-pack/plugins/event_log/server/lib/date_from_string.ts create mode 100644 x-pack/plugins/event_log/server/lib/route_validator_by_type.ts diff --git a/x-pack/plugins/event_log/server/es/cluster_client_adapter.test.ts b/x-pack/plugins/event_log/server/es/cluster_client_adapter.test.ts index da4b64ef61c4..884dbbb702f3 100644 --- a/x-pack/plugins/event_log/server/es/cluster_client_adapter.test.ts +++ b/x-pack/plugins/event_log/server/es/cluster_client_adapter.test.ts @@ -208,7 +208,7 @@ describe('queryEventsBySavedObject', () => { 'index-name', 'saved-object-type', 'saved-object-id', - { page: 10, per_page: 10, start: undefined, end: undefined } + { page: 10, per_page: 10 } ); expect(clusterClient.callAsInternalUser).toHaveBeenCalledWith('search', { index: 'index-name', @@ -240,7 +240,7 @@ describe('queryEventsBySavedObject', () => { const start = moment() .subtract(1, 'days') - .toISOString(); + .toDate(); await clusterClientAdapter.queryEventsBySavedObject( 'index-name', @@ -265,7 +265,7 @@ describe('queryEventsBySavedObject', () => { { range: { 'event.start': { - gte: start, + gte: start.toISOString(), }, }, }, @@ -285,10 +285,10 @@ describe('queryEventsBySavedObject', () => { const start = moment() .subtract(1, 'days') - .toISOString(); + .toDate(); const end = moment() .add(1, 'days') - .toISOString(); + .toDate(); await clusterClientAdapter.queryEventsBySavedObject( 'index-name', @@ -313,14 +313,14 @@ describe('queryEventsBySavedObject', () => { { range: { 'event.start': { - gte: start, + gte: start.toISOString(), }, }, }, { range: { 'event.end': { - lte: end, + lte: end.toISOString(), }, }, }, diff --git a/x-pack/plugins/event_log/server/es/cluster_client_adapter.ts b/x-pack/plugins/event_log/server/es/cluster_client_adapter.ts index 303c1c535021..0317186bd56f 100644 --- a/x-pack/plugins/event_log/server/es/cluster_client_adapter.ts +++ b/x-pack/plugins/event_log/server/es/cluster_client_adapter.ts @@ -114,7 +114,7 @@ export class ClusterClientAdapter { index: string, type: string, id: string, - { page, per_page: size, start, end }: Partial + { page, per_page: size, start, end }: FindOptionsType ): Promise { try { const { @@ -125,6 +125,7 @@ export class ClusterClientAdapter { ...(size && page ? { size, + // `page` count is a positive number, `from` is zero based index from: (page - 1) * size, } : {}), @@ -141,14 +142,14 @@ export class ClusterClientAdapter { start && { range: { 'event.start': { - gte: start, + gte: start.toISOString(), }, }, }, end && { range: { 'event.end': { - lte: end, + lte: end.toISOString(), }, }, }, diff --git a/x-pack/plugins/event_log/server/event_log_client.test.ts b/x-pack/plugins/event_log/server/event_log_client.test.ts index c6ce77f537f7..2e56446b8233 100644 --- a/x-pack/plugins/event_log/server/event_log_client.test.ts +++ b/x-pack/plugins/event_log/server/event_log_client.test.ts @@ -160,10 +160,10 @@ describe('EventLogStart', () => { const start = moment() .subtract(1, 'days') - .toISOString(); + .toDate(); const end = moment() .add(1, 'days') - .toISOString(); + .toDate(); expect( await eventLogClient.findEventsBySavedObject('saved-object-type', 'saved-object-id', { @@ -184,54 +184,6 @@ describe('EventLogStart', () => { } ); }); - - test('validates that the start date is valid', async () => { - const esContext = contextMock.create(); - const savedObjectsClient = savedObjectsClientMock.create(); - const eventLogClient = new EventLogClient({ - esContext, - savedObjectsClient, - }); - - savedObjectsClient.get.mockResolvedValueOnce({ - id: 'saved-object-id', - type: 'saved-object-type', - attributes: {}, - references: [], - }); - - esContext.esAdapter.queryEventsBySavedObject.mockResolvedValue([]); - - expect( - eventLogClient.findEventsBySavedObject('saved-object-type', 'saved-object-id', { - start: 'not a date string', - }) - ).rejects.toMatchInlineSnapshot(`[Error: [start]: Invalid Date]`); - }); - - test('validates that the end date is valid', async () => { - const esContext = contextMock.create(); - const savedObjectsClient = savedObjectsClientMock.create(); - const eventLogClient = new EventLogClient({ - esContext, - savedObjectsClient, - }); - - savedObjectsClient.get.mockResolvedValueOnce({ - id: 'saved-object-id', - type: 'saved-object-type', - attributes: {}, - references: [], - }); - - esContext.esAdapter.queryEventsBySavedObject.mockResolvedValue([]); - - expect( - eventLogClient.findEventsBySavedObject('saved-object-type', 'saved-object-id', { - end: 'not a date string', - }) - ).rejects.toMatchInlineSnapshot(`[Error: [end]: Invalid Date]`); - }); }); }); diff --git a/x-pack/plugins/event_log/server/event_log_client.ts b/x-pack/plugins/event_log/server/event_log_client.ts index c42e92686287..4012ee7a4c25 100644 --- a/x-pack/plugins/event_log/server/event_log_client.ts +++ b/x-pack/plugins/event_log/server/event_log_client.ts @@ -4,12 +4,14 @@ * you may not use this file except in compliance with the Elastic License. */ +import * as t from 'io-ts'; import { Observable } from 'rxjs'; import { ClusterClient, SavedObjectsClientContract } from 'src/core/server'; -import { schema, TypeOf } from '@kbn/config-schema'; +import { defaults } from 'lodash'; import { EsContext } from './es'; import { IEventLogClient, IEvent } from './types'; +import { DateFromString, PositiveNumberFromString } from './lib/date_from_string'; export type PluginClusterClient = Pick; export type AdminClusterClient$ = Observable; @@ -18,23 +20,13 @@ interface EventLogServiceCtorParams { savedObjectsClient: SavedObjectsClientContract; } -const optionalDateFieldSchema = schema.maybe( - schema.string({ - validate(value) { - if (isNaN(Date.parse(value))) { - return 'Invalid Date'; - } - }, - }) -); - -export const findOptionsSchema = schema.object({ - per_page: schema.number({ defaultValue: 10, min: 0 }), - page: schema.number({ defaultValue: 1, min: 1 }), - start: optionalDateFieldSchema, - end: optionalDateFieldSchema, +export const FindOptionsSchema = t.partial({ + per_page: PositiveNumberFromString, + page: PositiveNumberFromString, + start: DateFromString, + end: DateFromString, }); -export type FindOptionsType = TypeOf; +export type FindOptionsType = t.TypeOf; // note that clusterClient may be null, indicating we can't write to ES export class EventLogClient implements IEventLogClient { @@ -49,14 +41,14 @@ export class EventLogClient implements IEventLogClient { async findEventsBySavedObject( type: string, id: string, - options?: Partial + options: FindOptionsType = {} ): Promise { await this.savedObjectsClient.get(type, id); return (await this.esContext.esAdapter.queryEventsBySavedObject( this.esContext.esNames.alias, type, id, - findOptionsSchema.validate(options ?? {}) + defaults(options, { page: 1, per_page: 10 }) )) as IEvent[]; } } diff --git a/x-pack/plugins/event_log/server/lib/date_from_string.test.ts b/x-pack/plugins/event_log/server/lib/date_from_string.test.ts new file mode 100644 index 000000000000..be431d92789c --- /dev/null +++ b/x-pack/plugins/event_log/server/lib/date_from_string.test.ts @@ -0,0 +1,46 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { DateFromString, PositiveNumberFromString } from './date_from_string'; +import { right, isLeft } from 'fp-ts/lib/Either'; + +describe('DateFromString', () => { + test('validated and parses a string into a Date', () => { + const date = new Date(1973, 10, 30); + expect(DateFromString.decode(date.toISOString())).toEqual(right(date)); + }); + + test('validated and returns a failure for an actual Date', () => { + const date = new Date(1973, 10, 30); + expect(isLeft(DateFromString.decode(date))).toEqual(true); + }); + + test('validated and returns a failure for an invalid Date string', () => { + expect(isLeft(DateFromString.decode('1234-23-45'))).toEqual(true); + }); + + test('validated and returns a failure for a null value', () => { + expect(isLeft(DateFromString.decode(null))).toEqual(true); + }); +}); + +describe('PositiveNumberFromString', () => { + test('validated and parses a string into a positive number', () => { + expect(PositiveNumberFromString.decode('1')).toEqual(right(1)); + }); + + test('validated and returns a failure for an invalid number', () => { + expect(isLeft(PositiveNumberFromString.decode('a23'))).toEqual(true); + }); + + test('validated and returns a failure for a negative number', () => { + expect(isLeft(PositiveNumberFromString.decode('-45'))).toEqual(true); + }); + + test('validated and returns a failure for a null value', () => { + expect(isLeft(PositiveNumberFromString.decode(null))).toEqual(true); + }); +}); diff --git a/x-pack/plugins/event_log/server/lib/date_from_string.ts b/x-pack/plugins/event_log/server/lib/date_from_string.ts new file mode 100644 index 000000000000..f17ccf2f7d1a --- /dev/null +++ b/x-pack/plugins/event_log/server/lib/date_from_string.ts @@ -0,0 +1,46 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import * as t from 'io-ts'; +import { isNumber } from 'lodash'; +import { either } from 'fp-ts/lib/Either'; + +// represents a Date from an ISO string +export const DateFromString = new t.Type( + 'DateFromString', + // detect the type + (value): value is Date => value instanceof Date, + (valueToDecode, context) => + either.chain( + // validate this is a string + t.string.validate(valueToDecode, context), + // decode + value => { + const decoded = new Date(value); + return isNaN(decoded.getTime()) ? t.failure(valueToDecode, context) : t.success(decoded); + } + ), + valueToEncode => valueToEncode.toISOString() +); + +export const PositiveNumberFromString = new t.Type( + 'PositiveNumberFromString', + // detect the type + (value): value is number => isNumber(value), + (valueToDecode, context) => + either.chain( + // validate this is a string + t.string.validate(valueToDecode, context), + // decode + value => { + const decoded = parseInt(value, 10); + return isNaN(decoded) || decoded < 0 + ? t.failure(valueToDecode, context) + : t.success(decoded); + } + ), + valueToEncode => `${valueToEncode}` +); diff --git a/x-pack/plugins/event_log/server/lib/route_validator_by_type.ts b/x-pack/plugins/event_log/server/lib/route_validator_by_type.ts new file mode 100644 index 000000000000..a300de9a115a --- /dev/null +++ b/x-pack/plugins/event_log/server/lib/route_validator_by_type.ts @@ -0,0 +1,25 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +import * as t from 'io-ts'; +import { pipe } from 'fp-ts/lib/pipeable'; +import { fold } from 'fp-ts/lib/Either'; +import { RouteValidationResultFactory, RouteValidationFunction } from 'kibana/server'; +import { Type } from 'io-ts'; + +export const routeValidatorByType = >(type: T) => ( + value: any, + { ok, badRequest }: RouteValidationResultFactory +) => { + type TypeOf = t.TypeOf; + // const twemp = type.decode(value) + return pipe( + type.decode(value), + fold>>( + (errors: t.Errors) => badRequest(errors.map(e => `${e.message ?? e.value}`).join('\n')), + (val: TypeOf) => ok(val) + ) + ); +}; diff --git a/x-pack/plugins/event_log/server/routes/find.ts b/x-pack/plugins/event_log/server/routes/find.ts index cb170e50fb44..9d737e171a73 100644 --- a/x-pack/plugins/event_log/server/routes/find.ts +++ b/x-pack/plugins/event_log/server/routes/find.ts @@ -4,7 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ -import { schema, TypeOf } from '@kbn/config-schema'; +import * as t from 'io-ts'; import { IRouter, RequestHandlerContext, @@ -13,25 +13,27 @@ import { KibanaResponseFactory, } from 'kibana/server'; import { BASE_EVENT_LOG_API_PATH } from '../../common'; -import { findOptionsSchema, FindOptionsType } from '../event_log_client'; +import { FindOptionsSchema, FindOptionsType } from '../event_log_client'; +import { routeValidatorByType } from '../lib/route_validator_by_type'; -const paramSchema = schema.object({ - type: schema.string(), - id: schema.string(), +const ParamsSchema = t.type({ + type: t.string, + id: t.string, }); +type ParamsType = t.TypeOf; export const findRoute = (router: IRouter) => { router.get( { path: `${BASE_EVENT_LOG_API_PATH}/{type}/{id}/_find`, validate: { - params: paramSchema, - query: findOptionsSchema, + params: routeValidatorByType(ParamsSchema), + query: routeValidatorByType(FindOptionsSchema), }, }, router.handleLegacyErrors(async function( context: RequestHandlerContext, - req: KibanaRequest, FindOptionsType, any, any>, + req: KibanaRequest, res: KibanaResponseFactory ): Promise> { if (!context.eventLog) { diff --git a/x-pack/test/plugin_api_integration/test_suites/event_log/public_api_integration.ts b/x-pack/test/plugin_api_integration/test_suites/event_log/public_api_integration.ts index e7290777b2da..8a2c9082975d 100644 --- a/x-pack/test/plugin_api_integration/test_suites/event_log/public_api_integration.ts +++ b/x-pack/test/plugin_api_integration/test_suites/event_log/public_api_integration.ts @@ -48,6 +48,7 @@ export default function({ getService }: FtrProviderContext) { await logTestEvent(id, firstExpectedEvent); await Promise.all(expectedEvents.map(event => logTestEvent(id, event))); + log.debug(`Query with default pagination`); await retry.try(async () => { const { body: foundEvents } = await supertest .get(`/api/event_log/event_log_test/${id}/_find`) @@ -62,6 +63,7 @@ export default function({ getService }: FtrProviderContext) { 3 ); + log.debug(`Query with per_page pagination`); const { body: firstPage } = await supertest .get(`/api/event_log/event_log_test/${id}/_find?per_page=3`) .set('kbn-xsrf', 'foo') @@ -70,6 +72,7 @@ export default function({ getService }: FtrProviderContext) { expect(firstPage.length).to.be(3); assertEventsFromApiMatchCreatedEvents(firstPage, expectedFirstPage); + log.debug(`Query with all pagination params`); const { body: secondPage } = await supertest .get(`/api/event_log/event_log_test/${id}/_find?per_page=3&page=2`) .set('kbn-xsrf', 'foo')