Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Security Solution][Resolver] Resolver query panel load more #79160

Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@ import {
SafeResolverEvent,
} from '../../../common/endpoint/types';

export const relatedEventsPaginationSize = 25;

/**
* The data access layer for resolver. All communication with the Kibana server is done through this object. This object is provided to Resolver. In tests, a mock data access layer can be used instead.
*/
Expand Down Expand Up @@ -50,7 +52,7 @@ export function dataAccessLayerFactory(
after?: string
): Promise<ResolverPaginatedEvents> {
return context.services.http.post('/api/endpoint/resolver/events', {
query: { afterEvent: after },
query: { afterEvent: after, limit: relatedEventsPaginationSize },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this should be specified on the BE as the 'default' instead of specifying it here. That way we don't have two places defining this value.

body: JSON.stringify({
filter: `process.entity_id:"${entityID}" and event.category:"${category}"`,
}),
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
/*
* 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 { DataAccessLayer } from '../../types';
import { mockTreeWithOneNodeAndTwoPagesOfRelatedEvents } from '../../mocks/resolver_tree';
import { relatedEventsPaginationSize } from '../factory';
import {
ResolverRelatedEvents,
ResolverTree,
ResolverEntityIndex,
SafeResolverEvent,
} from '../../../../common/endpoint/types';
import * as eventModel from '../../../../common/endpoint/models/event';

interface Metadata {
/**
* The `_id` of the document being analyzed.
*/
databaseDocumentID: string;
/**
* A record of entityIDs to be used in tests assertions.
*/
entityIDs: {
/**
* The entityID of the node related to the document being analyzed.
*/
origin: 'origin';
};
}
export function oneNodeWithPaginatedEvents(): {
dataAccessLayer: DataAccessLayer;
metadata: Metadata;
} {
const metadata: Metadata = {
databaseDocumentID: '_id',
entityIDs: { origin: 'origin' },
};
const tree = mockTreeWithOneNodeAndTwoPagesOfRelatedEvents({
originID: metadata.entityIDs.origin,
});

return {
metadata,
dataAccessLayer: {
/**
* Fetch related events for an entity ID
*/
async relatedEvents(entityID: string): Promise<ResolverRelatedEvents> {
/**
* Respond with the mocked related events when the origin's related events are fetched.
**/
const events = entityID === metadata.entityIDs.origin ? tree.relatedEvents.events : [];

return {
entityID,
events,
nextEvent: null,
};
},

/**
* If called with an "after" cursor, return the 2nd page, else return the first.
*/
async eventsWithEntityIDAndCategory(
entityID: string,
category: string,
after?: string
): Promise<{ events: SafeResolverEvent[]; nextEvent: string | null }> {
let events: SafeResolverEvent[] = [];
const eventsOfCategory = tree.relatedEvents.events.filter(
(event) => event.event?.category === category
);
if (after === undefined) {
events = eventsOfCategory.slice(0, relatedEventsPaginationSize);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this mock should reference relatedEventsPaginationSize. I think you could redefine this here const relatedEventsPaginationSize = 25. I think you even use any arbitrary number you want. Is there some reason I'm missing?

} else {
events = eventsOfCategory.slice(relatedEventsPaginationSize);
}
return {
events,
nextEvent: typeof after === 'undefined' ? 'firstEventPage2' : null,
};
},

/**
* Any of the origin's related events by event.id
*/
async event(eventID: string): Promise<SafeResolverEvent | null> {
return (
tree.relatedEvents.events.find((event) => eventModel.eventID(event) === eventID) ?? null
);
},

/**
* Fetch a ResolverTree for a entityID
*/
async resolverTree(): Promise<ResolverTree> {
return tree;
},

/**
* Get entities matching a document.
*/
async entities(): Promise<ResolverEntityIndex> {
return [{ entity_id: metadata.entityIDs.origin }];
},
},
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,49 @@

import { mockEndpointEvent } from './endpoint_event';
import { ResolverTree, SafeResolverEvent } from '../../../common/endpoint/types';
import { relatedEventsPaginationSize } from '../data_access_layer/factory';
import * as eventModel from '../../../common/endpoint/models/event';

export function mockTreeWithOneNodeAndTwoPagesOfRelatedEvents({

This comment was marked as resolved.

originID,
}: {
originID: string;
}): ResolverTree {
const originEvent: SafeResolverEvent = mockEndpointEvent({
entityID: originID,
processName: 'c',
parentEntityID: undefined,
timestamp: 1600863932318,
});
const events = [];
const eventsToGenerate = relatedEventsPaginationSize + 5;
for (let i = 0; i < eventsToGenerate; i++) {
const newEvent = mockEndpointEvent({
entityID: originID,
eventID: 'test',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make the eventID something like test-${i} in case we want to interact with the individual events on a page-by-page basis in the test we can, and verify the right event shows up at the right time

eventType: 'access',
eventCategory: 'registry',
timestamp: 1600863932318,
});
events.push(newEvent);
}
return {
entityID: originID,
children: {
childNodes: [],
nextChild: null,
},
ancestry: {
nextAncestor: null,
ancestors: [],
},
lifecycle: [originEvent],
relatedEvents: { events, nextEvent: null },
relatedAlerts: { alerts: [], nextAlert: null },
stats: { events: { total: eventsToGenerate, byCategory: {} }, totalAlerts: 0 },
};
}

export function mockTreeWith2AncestorsAndNoChildren({
originID,
firstAncestorID,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,35 @@ interface AppRequestedResolverData {
readonly payload: TreeFetcherParameters;
}

interface AppRequestedRelatedEventData {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove this action?

readonly type: 'appRequestedRelatedEventData';

readonly payload: {};
}

interface UserRequestedAdditionalRelatedEvents {
readonly type: 'userRequestedAdditionalRelatedEvents';
readonly payload: {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove this payload?

}

interface ServerFailedToReturnNodeEventsInCategory {
readonly type: 'serverFailedToReturnNodeEventsInCategory';
readonly payload: {
/**
* The cursor, if any, that can be used to retrieve more events.
*/
cursor: string | null;
/**
* The nodeID that `events` are related to.
*/
nodeID: string;
/**
* The category that `events` have in common.
*/
eventCategory: string;
};
}

interface ServerFailedToReturnResolverData {
readonly type: 'serverFailedToReturnResolverData';
/**
Expand Down Expand Up @@ -101,4 +130,7 @@ export type DataAction =
| ServerReturnedRelatedEventData
| ServerReturnedNodeEventsInCategory
| AppRequestedResolverData
| AppRequestedRelatedEventData
| UserRequestedAdditionalRelatedEvents
| ServerFailedToReturnNodeEventsInCategory
| AppAbortedResolverDataRequest;
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export function updatedWith(
eventCategory: first.eventCategory,
events: [...first.events, ...second.events],
cursor: second.cursor,
lastCursorRequested: null,
};
} else {
return undefined;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ const initialState: DataState = {
relatedEvents: new Map(),
resolverComponentInstanceID: undefined,
};

/* eslint-disable complexity */
export const dataReducer: Reducer<DataState, ResolverAction> = (state = initialState, action) => {
if (action.type === 'appReceivedNewExternalProperties') {
const nextState: DataState = {
Expand Down Expand Up @@ -140,6 +140,9 @@ export const dataReducer: Reducer<DataState, ResolverAction> = (state = initialS
...state,
nodeEventsInCategory: updated,
};
if (next.nodeEventsInCategory) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove this?

next.nodeEventsInCategory.loading = false;
}
return next;
} else {
// this should never happen. This reducer ensures that any `nodeEventsInCategory` that are in state are relevant to the `panelViewAndParameters`.
Expand All @@ -151,12 +154,42 @@ export const dataReducer: Reducer<DataState, ResolverAction> = (state = initialS
...state,
nodeEventsInCategory: action.payload,
};
if (next.nodeEventsInCategory) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you also remove this?

next.nodeEventsInCategory.loading = false;
}
return next;
}
} else {
// the action is stale, ignore it
return state;
}
} else if (action.type === 'userRequestedAdditionalRelatedEvents') {
const nextState: DataState = {
...state,
nodeEventsInCategory: {
nodeID: '',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not following the logic here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're spreading in state.nodeEventsInCategory. You shouldn't need to add their nullish fields. What TS error are you getting?

eventCategory: '',
events: [],
cursor: null,
...state.nodeEventsInCategory,
loading: true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you remove loading here?

lastCursorRequested: state.nodeEventsInCategory?.cursor,
},
};
return nextState;
} else if (action.type === 'serverFailedToReturnNodeEventsInCategory') {
const nextState: DataState = {
...state,
nodeEventsInCategory: {
nodeID: '',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's fix this.

eventCategory: '',
events: [],
cursor: null,
...state.nodeEventsInCategory,
error: true,
},
};
return nextState;
} else if (action.type === 'appRequestedCurrentRelatedEventData') {
const nextState: DataState = {
...state,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
import { isGraphableProcess, isTerminatedProcess } from '../../models/process_event';
import * as indexedProcessTreeModel from '../../models/indexed_process_tree';
import * as eventModel from '../../../../common/endpoint/models/event';

import * as nodeEventsInCategoryModel from './node_events_in_category_model';
import {
ResolverTree,
ResolverNodeStats,
Expand Down Expand Up @@ -665,3 +665,74 @@ export const panelViewAndParameters = createSelector(
export const nodeEventsInCategory = (state: DataState) => {
return state.nodeEventsInCategory?.events ?? [];
};

export const userCanRequestMoreNodeEventsInCategory = createSelector(
(state: DataState) => state.nodeEventsInCategory,
panelViewAndParameters,
/* eslint-disable-next-line no-shadow */
function (nodeEventsInCategory, panelViewAndParameters) {
if (
nodeEventsInCategory !== undefined &&
nodeEventsInCategoryModel.isRelevantToPanelViewAndParameters(
nodeEventsInCategory,
panelViewAndParameters
)
) {
return nodeEventsInCategory.cursor !== null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should return false if lastCursorRequested === nodeEventsInCategory.cursor because in that case, a request is already in progress. You might be able to re-use isLoadingMoreNodeEventsInCategory here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is true, but the purpose of this selector is just to show/hide the button. When the button is visible and the request in progress, the EuiButton loading state disables any click events.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not following you.

Copy link
Contributor

@michaelolo24 michaelolo24 Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was just a misunderstanding? You should be able to just use isLoadingMoreNodeEventsInCategory Or are you saying this is to confirm it is not in an active loading state? Essentially nodeEventsInCategory.cursor !== nodeEventsInCategory.lastCursorRequested?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I now understand the logic. You want to show a button when the user can 'load more' or when the app is loading more. However I don't think the name of this selector matches its behavior.

If you'd rather not change your implementation, can you change the name of this selector?

Otherwise could you change the return to:

return nodeEventsInCategory.cursor !== null && nodeEventsInCategory.cursor !== lastCursorRequested

That would make the name of the selector match its return.

And then show the "Load More" / "Loading" button when:

userCanRequestMoreNodeEventsInCategory || isLoadingMoreNodeEventsInCategory

I think that design will be simpler to understand and modify in the future.

} else {
return false;
}
}
);

export const hadErrorLoadingNodeEventsInCategory = createSelector(
(state: DataState) => state.nodeEventsInCategory,
panelViewAndParameters,
/* eslint-disable-next-line no-shadow */
function (nodeEventsInCategory, panelViewAndParameters) {
if (
nodeEventsInCategory !== undefined &&
nodeEventsInCategoryModel.isRelevantToPanelViewAndParameters(
nodeEventsInCategory,
panelViewAndParameters
)
) {
return nodeEventsInCategory && nodeEventsInCategory.error === true;
} else {
return false;
}
}
);

export const isLoadingNodeEventsInCategory = createSelector(
(state: DataState) => state.nodeEventsInCategory,
panelViewAndParameters,
/* eslint-disable-next-line no-shadow */
function (nodeEventsInCategory, panelViewAndParameters) {
const { panelView } = panelViewAndParameters;
return panelView === 'nodeEventsInCategory' && nodeEventsInCategory === undefined;
}
);

export const isLoadingMoreNodeEventsInCategory = createSelector(
(state: DataState) => state.nodeEventsInCategory,
panelViewAndParameters,
/* eslint-disable-next-line no-shadow */
function (nodeEventsInCategory, panelViewAndParameters) {
if (
nodeEventsInCategory !== undefined &&
nodeEventsInCategoryModel.isRelevantToPanelViewAndParameters(
nodeEventsInCategory,
panelViewAndParameters
)
) {
return (
nodeEventsInCategory &&
nodeEventsInCategory.lastCursorRequested !== null &&
nodeEventsInCategory.cursor === nodeEventsInCategory.lastCursorRequested
);
} else {
return false;
}
}
);
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export const resolverMiddlewareFactory: MiddlewareFactory = (dataAccessLayer: Da
next(action);

resolverTreeFetcher();
relatedEventsFetcher();
relatedEventsFetcher(action);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you remove the action parameter and check for changes to isLoadingMoreNodeEventsInCategory and isLoadingNodeEventsInCategory instead? That way the reducer controls the logic of deciding when to request data. The middleware only has to respond to a change in state.

currentRelatedEventFetcher();
};
};
Expand Down
Loading