From 59546c7f233767fb5304feda69f15a707768e427 Mon Sep 17 00:00:00 2001 From: Madison Caldwell Date: Mon, 3 Feb 2020 21:37:14 -0500 Subject: [PATCH] Address comments, clean up -- Round 1 --- .../endpoint/store/alerts/action.ts | 2 - .../endpoint/store/alerts/middleware.ts | 22 +------ .../endpoint/store/alerts/types.ts | 1 - .../endpoint/view/alerts/index.tsx | 8 ++- .../endpoint/server/routes/alerts.test.ts | 11 +--- .../plugins/endpoint/server/routes/alerts.ts | 63 +++++++------------ .../endpoint/alert_query_builders.test.ts | 2 + .../services/endpoint/alert_query_builders.ts | 32 +++++----- 8 files changed, 53 insertions(+), 88 deletions(-) diff --git a/x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/action.ts b/x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/action.ts index e1e67477498b41..7f8ac5a40d1e20 100644 --- a/x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/action.ts +++ b/x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/action.ts @@ -4,10 +4,8 @@ * you may not use this file except in compliance with the Elastic License. */ -// import { Immutable } from '../../../../../common/types'; import { AlertListData } from './types'; -// type ServerReturnedAlertsData = Immutable<{ interface ServerReturnedAlertsData { type: 'serverReturnedAlertsData'; payload: AlertListData; diff --git a/x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/middleware.ts b/x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/middleware.ts index 267b05e2d1cc82..aede95ceb3759d 100644 --- a/x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/middleware.ts +++ b/x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/middleware.ts @@ -4,35 +4,19 @@ * you may not use this file except in compliance with the Elastic License. */ -// import { AlertData, ImmutableArray } from '../../../../../common/types'; import qs from 'querystring'; +import { HttpFetchQuery } from 'src/core/public'; import { AppAction } from '../action'; import { MiddlewareFactory } from '../../types'; export const alertMiddlewareFactory: MiddlewareFactory = coreStart => { - interface Params { - paging_properties: object[]; - } - - const params: Params = { - paging_properties: [], - }; - const qp = qs.parse(window.location.search.slice(1)); - if ('page_size' in qp && typeof qp.page_size === 'string') { - const pageSize: number = parseInt(qp.page_size, 10); - params.paging_properties.push({ page_size: pageSize }); - } - if ('page_index' in qp && typeof qp.page_index === 'string') { - const pageIndex: number = parseInt(qp.page_index, 10); - params.paging_properties.push({ page_index: pageIndex }); - } return api => next => async (action: AppAction) => { next(action); if (action.type === 'userNavigatedToPage' && action.payload === 'alertsPage') { - const response = await coreStart.http.post('/api/endpoint/alerts', { - body: JSON.stringify(params), + const response = await coreStart.http.get('/api/endpoint/alerts', { + query: qp as HttpFetchQuery, }); api.dispatch({ type: 'serverReturnedAlertsData', payload: response }); } diff --git a/x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/types.ts b/x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/types.ts index 4f9366d0e868bb..b3ecc4a3bebe76 100644 --- a/x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/types.ts +++ b/x-pack/plugins/endpoint/public/applications/endpoint/store/alerts/types.ts @@ -6,7 +6,6 @@ import { AlertData } from '../../../../../common/types'; -// FIXME: temporary until server defined `interface` is moved to a module we can reference export interface AlertListData { alerts: AlertData[]; request_page_size: number; diff --git a/x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/index.tsx b/x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/index.tsx index 6708a3545dfbe3..8c32426dcc8681 100644 --- a/x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/index.tsx +++ b/x-pack/plugins/endpoint/public/applications/endpoint/view/alerts/index.tsx @@ -8,6 +8,7 @@ import { memo, useState, useMemo } from 'react'; import React from 'react'; import { EuiDataGrid } from '@elastic/eui'; import { useSelector } from 'react-redux'; +import { i18n } from '@kbn/i18n'; import * as selectors from '../../store/selectors'; import { usePageId } from '../use_page_id'; @@ -40,7 +41,12 @@ export const AlertIndex = memo(() => { const row = json[rowIndex]; if (columnId === 'alert_type') { - return 'Malicious File'; + return i18n.translate( + 'xpack.endpoint.application.endpoint.alerts.alertType.maliciousFileDescription', + { + defaultMessage: 'Malicious File', + } + ); } else if (columnId === 'event_type') { return row.event.action; } else if (columnId === 'os') { diff --git a/x-pack/plugins/endpoint/server/routes/alerts.test.ts b/x-pack/plugins/endpoint/server/routes/alerts.test.ts index 039dce1d3f2cba..f99c29dc8d56fb 100644 --- a/x-pack/plugins/endpoint/server/routes/alerts.test.ts +++ b/x-pack/plugins/endpoint/server/routes/alerts.test.ts @@ -113,14 +113,8 @@ describe('test alerts route', () => { const mockRequest = httpServerMock.createKibanaRequest({ method: 'post', body: { - paging_properties: [ - { - page_size: 20, - }, - { - page_index: 2, - }, - ], + page_size: 20, + page_index: 2, }, }); mockScopedClient.callAsCurrentUser.mockImplementationOnce(() => @@ -188,4 +182,5 @@ describe('test alerts route', () => { expect(alertResultList.request_page_index).toEqual(40); expect(alertResultList.request_page_size).toEqual(20); }); + // TODO: add tests for track_total_hits }); diff --git a/x-pack/plugins/endpoint/server/routes/alerts.ts b/x-pack/plugins/endpoint/server/routes/alerts.ts index c0af56418c623c..685a1902a1e20d 100644 --- a/x-pack/plugins/endpoint/server/routes/alerts.ts +++ b/x-pack/plugins/endpoint/server/routes/alerts.ts @@ -16,14 +16,14 @@ import { EndpointAppContext } from '../types'; const ALERTS_ROUTE = '/api/endpoint/alerts'; export function registerAlertRoutes(router: IRouter, endpointAppContext: EndpointAppContext) { - const alertsHandler: RequestHandler = async (ctx, req, res) => { + const alertsHandler: RequestHandler = async (ctx, req, res) => { try { const queryParams = await kibanaRequestToAlertListQuery(req, endpointAppContext); const response = (await ctx.core.elasticsearch.dataClient.callAsCurrentUser( 'search', queryParams )) as SearchResponse; - return res.ok({ body: mapToAlertResultList(queryParams, response) }); + return res.ok({ body: mapToAlertResultList(endpointAppContext, queryParams, response) }); } catch (err) { return res.internalError({ body: err }); } @@ -32,7 +32,12 @@ export function registerAlertRoutes(router: IRouter, endpointAppContext: Endpoin router.get( { path: ALERTS_ROUTE, - validate: {}, + validate: { + query: schema.object({ + page_size: schema.number({ defaultValue: 10, min: 1, max: 10000 }), + page_index: schema.number({ defaultValue: 0, min: 0 }), + }), + }, options: { authRequired: true }, }, alertsHandler @@ -42,24 +47,10 @@ export function registerAlertRoutes(router: IRouter, endpointAppContext: Endpoin { path: ALERTS_ROUTE, validate: { - body: schema.nullable( - schema.object({ - paging_properties: schema.arrayOf( - // FIXME: This structure is weird, but it will change when the following bug is fixed: - // https://github.com/elastic/kibana/issues/54843 - schema.oneOf([ - // the number of results to return for this request per page - schema.object({ - page_size: schema.number({ defaultValue: 10, min: 1, max: 10000 }), - }), - // the index of the page to return - schema.object({ - page_index: schema.number({ defaultValue: 0, min: 0 }), - }), - ]) - ), - }) - ), + body: schema.object({ + page_size: schema.number({ defaultValue: 10, min: 1, max: 10000 }), + page_index: schema.number({ defaultValue: 0, min: 0 }), + }), }, options: { authRequired: true }, }, @@ -68,6 +59,7 @@ export function registerAlertRoutes(router: IRouter, endpointAppContext: Endpoin } function mapToAlertResultList( + endpointAppContext: EndpointAppContext, queryParams: Record, searchResponse: SearchResponse ): AlertResultList { @@ -79,8 +71,7 @@ function mapToAlertResultList( let totalNumberOfAlerts: number = 0; let totalIsLowerBound: boolean = false; - // HACK: because of mismatch in elasticsearch type versions - // https://www.elastic.co/guide/en/elasticsearch/reference/current/breaking-changes-7.0.html#hits-total-now-object-search-response + // We handle 2 separate schemas for the response below, due to: https://github.com/elastic/kibana/issues/56694 if (typeof searchResponse?.hits?.total === 'object') { const total: Total = searchResponse?.hits?.total as Total; totalNumberOfAlerts = total?.value || 0; @@ -90,22 +81,16 @@ function mapToAlertResultList( } if (totalIsLowerBound) { - // TODO: handle this + // This shouldn't happen, as we always try to fetch enough hits to satisfy the current request and the next page. + endpointAppContext.logFactory + .get('endpoint') + .warn('WARNING: Total hits not counted accurately. Pagination numbers may be inaccurate.'); } - if (searchResponse.hits.hits.length > 0) { - return { - request_page_size: queryParams.size, - request_page_index: queryParams.from, - alerts: searchResponse.hits.hits.map(entry => entry._source), - total: totalNumberOfAlerts, - }; - } else { - return { - request_page_size: queryParams.size, - request_page_index: queryParams.from, - total: totalNumberOfAlerts, - alerts: [], - }; - } + return { + request_page_size: queryParams.size, + request_page_index: queryParams.from, + alerts: searchResponse?.hits?.hits?.map(entry => entry._source), + total: totalNumberOfAlerts, + }; } diff --git a/x-pack/plugins/endpoint/server/services/endpoint/alert_query_builders.test.ts b/x-pack/plugins/endpoint/server/services/endpoint/alert_query_builders.test.ts index b28b3682b6ec75..b74e895d2d4fba 100644 --- a/x-pack/plugins/endpoint/server/services/endpoint/alert_query_builders.test.ts +++ b/x-pack/plugins/endpoint/server/services/endpoint/alert_query_builders.test.ts @@ -29,11 +29,13 @@ describe('test query builder', () => { }, }, ], + track_total_hits: 10000, }, from: 0, size: 10, index: 'my-index', } as Record); }); + // TODO: add tests for calculating track_total_hits }); }); diff --git a/x-pack/plugins/endpoint/server/services/endpoint/alert_query_builders.ts b/x-pack/plugins/endpoint/server/services/endpoint/alert_query_builders.ts index 5cd283aacfa25b..190b6bdd3d088b 100644 --- a/x-pack/plugins/endpoint/server/services/endpoint/alert_query_builders.ts +++ b/x-pack/plugins/endpoint/server/services/endpoint/alert_query_builders.ts @@ -3,7 +3,6 @@ * or more contributor license agreements. Licensed under the Elastic License; * you may not use this file except in compliance with the Elastic License. */ -import qs from 'querystring'; import { KibanaRequest } from 'kibana/server'; import { EndpointAppConstants } from '../../../common/types'; import { EndpointAppContext } from '../../types'; @@ -13,8 +12,16 @@ export const kibanaRequestToAlertListQuery = async ( endpointAppContext: EndpointAppContext ): Promise> => { const pagingProperties = await getPagingProperties(request, endpointAppContext); + + // Calculate minimum total hits set to indicate there's a next page + const DEFAULT_TOTAL_HITS = 10000; + + const fromIdx = pagingProperties.pageIndex * pagingProperties.pageSize; + const totalHitsMin = Math.max(fromIdx + pagingProperties.pageSize * 2, DEFAULT_TOTAL_HITS); + return { body: { + track_total_hits: totalHitsMin, query: { match_all: {}, }, @@ -26,7 +33,7 @@ export const kibanaRequestToAlertListQuery = async ( }, ], }, - from: pagingProperties.pageIndex * pagingProperties.pageSize, + from: fromIdx, size: pagingProperties.pageSize, index: EndpointAppConstants.ALERT_INDEX_NAME, }; @@ -37,25 +44,14 @@ async function getPagingProperties( endpointAppContext: EndpointAppContext ) { const config = await endpointAppContext.config(); - let pagingProperties: { page_size?: number; page_index?: number } = {}; + const pagingProperties: { page_size?: number; page_index?: number } = {}; if (request?.route?.method === 'get') { - if (typeof request?.url?.query === 'string') { - const qp = qs.parse(request.url.query); - pagingProperties.page_size = Number(qp.page_size); - pagingProperties.page_index = Number(qp.page_index); - } else if (request?.url?.query) { - pagingProperties = request.url.query; - } + pagingProperties.page_index = request.query?.page_index; + pagingProperties.page_size = request.query?.page_size; } else { - if (request?.body?.paging_properties) { - for (const property of request.body.paging_properties) { - Object.assign( - pagingProperties, - ...Object.keys(property).map(key => ({ [key]: property[key] })) - ); - } - } + pagingProperties.page_index = request.body?.page_index; + pagingProperties.page_size = request.body?.page_size; } return {