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] Word-break long titles in related event… #75926

Merged
merged 20 commits into from
Aug 27, 2020
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import * as selectors from './selectors';
import { DataState } from '../../types';
import { DataAction } from './action';
import { ResolverChildNode, ResolverTree } from '../../../../common/endpoint/types';
import * as eventModel from '../../../../common/endpoint/models/event';

/**
* Test the data reducer and selector.
Expand Down Expand Up @@ -175,6 +176,25 @@ describe('Resolver Data Middleware', () => {
eventStatsForFirstChildNode.byCategory[categoryToOverCount] - 1
);
});
it('should return the correct related event detail metadata for a given related event', () => {
const relatedEventsByCategory = selectors.relatedEventsByCategory(store.getState());
const someRelatedEventForTheFirstChild = relatedEventsByCategory(firstChildNodeInTree.id)(
categoryToOverCount
)[0];
const relatedEventID = eventModel.eventId(someRelatedEventForTheFirstChild)!;
const relatedDisplayInfo = selectors.relatedEventDisplayInfoByEntityAndSelfID(
store.getState()
)(firstChildNodeInTree.id, relatedEventID);
const [, countOfSameType, , sectionData] = relatedDisplayInfo;
const hostEntries = sectionData.filter((section) => {
return section.sectionTitle === 'host';
})[0].entries;
// u+200b is the "Zero-width-space" character we put around "."s so they break/wrap nicely on display;
expect(hostEntries).toContainEqual({ title: 'os\u200b.platform', description: 'Windows' });
expect(countOfSameType).toBe(
eventStatsForFirstChildNode.byCategory[categoryToOverCount] - 1
);
});
it('should indicate the limit has been exceeded because the number of related events received for the category is less than what the stats count said it would be', () => {
const selectedRelatedInfo = selectors.relatedEventInfoByEntityId(store.getState());
const shouldShowLimit = selectedRelatedInfo(firstChildNodeInTree.id)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,14 @@ import {
ResolverNodeStats,
ResolverRelatedEvents,
SafeResolverEvent,
EndpointEvent,
LegacyEndpointEvent,
} from '../../../../common/endpoint/types';
import * as resolverTreeModel from '../../models/resolver_tree';
import * as isometricTaxiLayoutModel from '../../models/indexed_process_tree/isometric_taxi_layout';
import * as eventModel from '../../../../common/endpoint/models/event';
import * as vector2 from '../../models/vector2';
import { formatDate } from '../../view/panels/panel_content_utilities';

/**
* If there is currently a request.
Expand Down Expand Up @@ -173,6 +176,118 @@ export function relatedEventsByEntityId(data: DataState): Map<string, ResolverRe
return data.relatedEvents;
}

/**
* Seed word/non-word boundaries with zero-width spaces so they break/wrap nicely
*
* @param potentiallyLongString A string to seed with zero-width spaces so it wraps nicely
*/
function addWordBreakSpaces(potentiallyLongString: string): string {
// Seed boundaries beetween word and non-word characters with Zero-width spaces to allow
// for text to be wrapped when it overflows its container.
return potentiallyLongString.replace(/(\w\W)|(\W\w)/g, (boundary: string) => {
return `${boundary[0]}\u200b${boundary[1]}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@oatkiller I ended up pulling this into the selector here. Is this OK?

});
}

/**
* A helper function to turn objects into EuiDescriptionList entries.
* This reflects the strategy of more or less "dumping" metadata for related processes
* in description lists with little/no 'prettification'. This has the obvious drawback of
* data perhaps appearing inscrutable/daunting, but the benefit of presenting these fields
* to the user "as they occur" in ECS, which may help them with e.g. EQL queries.
*
* Given an object like: {a:{b: 1}, c: 'd'} it will yield title/description entries like so:
* {title: "a.b", description: "1"}, {title: "c", description: "d"}
*
* @param {object} obj The object to turn into `<dt><dd>` entries
*/
const objectToDescriptionListEntries = function* (
obj: object,
prefix = ''
): Generator<{ title: string; description: string }> {
const nextPrefix = prefix.length ? `${prefix}.` : '';
for (const [metaKey, metaValue] of Object.entries(obj)) {
if (typeof metaValue === 'number' || typeof metaValue === 'string') {
yield { title: addWordBreakSpaces(nextPrefix + metaKey), description: `${metaValue}` };
} else if (metaValue instanceof Array) {
yield {
title: addWordBreakSpaces(nextPrefix + metaKey),
description: metaValue
.filter((arrayEntry) => {
return typeof arrayEntry === 'number' || typeof arrayEntry === 'string';
})
.join(','),
};
} else if (typeof metaValue === 'object') {
yield* objectToDescriptionListEntries(metaValue, nextPrefix + metaKey);
}
}
};

type SectionData = Array<{
bkimmel marked this conversation as resolved.
Show resolved Hide resolved
sectionTitle: string;
entries: Array<{ title: string; description: string }>;
}>;

/**
* Returns a function that returns the information needed to display related event details based on
* the related event's entityID and its own ID.
*/
export const relatedEventDisplayInfoByEntityAndSelfID: (
state: DataState
) => (
entityId: string,
relatedEventId: string | number
) => [
EndpointEvent | LegacyEndpointEvent | undefined,
number,
string | undefined,
SectionData,
string
] = createSelector(relatedEventsByEntityId, function relatedEventDetails(
/* eslint-disable no-shadow */
relatedEventsByEntityId
/* eslint-enable no-shadow */
) {
return defaultMemoize((entityId: string, relatedEventId: string | number) => {
const relatedEventsForThisProcess = relatedEventsByEntityId.get(entityId);
if (!relatedEventsForThisProcess) {
return [undefined, 0, undefined, [], ''];
}
const specificEvent = relatedEventsForThisProcess.events.find(
(evt) => eventModel.eventId(evt) === relatedEventId
);
// For breadcrumbs:
const specificCategory = specificEvent && eventModel.primaryEventCategory(specificEvent);
const countOfCategory = relatedEventsForThisProcess.events.reduce((sumtotal, evt) => {
return eventModel.primaryEventCategory(evt) === specificCategory ? sumtotal + 1 : sumtotal;
}, 0);

// Assuming these details (agent, ecs, process) aren't as helpful, can revisit
const { agent, ecs, process, ...relevantData } = specificEvent as ResolverEvent & {
// Type this with various unknown keys so that ts will let us delete those keys
ecs: unknown;
process: unknown;
};

let displayDate = '';
const sectionData: SectionData = Object.entries(relevantData)
.map(([sectionTitle, val]) => {
if (sectionTitle === '@timestamp') {
displayDate = formatDate(val);
return { sectionTitle: '', entries: [] };
}
if (typeof val !== 'object') {
return { sectionTitle, entries: [{ title: sectionTitle, description: `${val}` }] };
}
return { sectionTitle, entries: [...objectToDescriptionListEntries(val)] };
})
.filter((v) => v.sectionTitle !== '' && v.entries.length);

return [specificEvent, countOfCategory, specificCategory, sectionData, displayDate];
});
});

/**
* Returns a function that returns a function (when supplied with an entity id for a node)
* that returns related events for a node that match an event.category (when supplied with the category)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,15 @@ export const relatedEventsByEntityId = composeSelectors(
dataSelectors.relatedEventsByEntityId
);

/**
* Returns a function that returns the information needed to display related event details based on
* the related event's entityID and its own ID.
*/
export const relatedEventDisplayInfoByEntityAndSelfId = composeSelectors(
dataStateSelector,
dataSelectors.relatedEventDisplayInfoByEntityAndSelfID
);

/**
* Returns a function that returns a function (when supplied with an entity id for a node)
* that returns related events for a node that match an event.category (when supplied with the category)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,58 +10,18 @@ import { EuiSpacer, EuiText, EuiDescriptionList, EuiTextColor, EuiTitle } from '
import styled from 'styled-components';
import { useSelector } from 'react-redux';
import { FormattedMessage } from 'react-intl';
import {
CrumbInfo,
formatDate,
StyledBreadcrumbs,
BoldCode,
StyledTime,
} from './panel_content_utilities';
import { CrumbInfo, StyledBreadcrumbs, BoldCode, StyledTime } from './panel_content_utilities';
import * as event from '../../../../common/endpoint/models/event';
import { ResolverEvent } from '../../../../common/endpoint/types';
import * as selectors from '../../store/selectors';
import { useResolverDispatch } from '../use_resolver_dispatch';
import { PanelContentError } from './panel_content_error';

/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ moved to selector

* A helper function to turn objects into EuiDescriptionList entries.
* This reflects the strategy of more or less "dumping" metadata for related processes
* in description lists with little/no 'prettification'. This has the obvious drawback of
* data perhaps appearing inscrutable/daunting, but the benefit of presenting these fields
* to the user "as they occur" in ECS, which may help them with e.g. EQL queries.
*
* Given an object like: {a:{b: 1}, c: 'd'} it will yield title/description entries like so:
* {title: "a.b", description: "1"}, {title: "c", description: "d"}
*
* @param {object} obj The object to turn into `<dt><dd>` entries
*/
const objectToDescriptionListEntries = function* (
obj: object,
prefix = ''
): Generator<{ title: string; description: string }> {
const nextPrefix = prefix.length ? `${prefix}.` : '';
for (const [metaKey, metaValue] of Object.entries(obj)) {
if (typeof metaValue === 'number' || typeof metaValue === 'string') {
yield { title: nextPrefix + metaKey, description: `${metaValue}` };
} else if (metaValue instanceof Array) {
yield {
title: nextPrefix + metaKey,
description: metaValue
.filter((arrayEntry) => {
return typeof arrayEntry === 'number' || typeof arrayEntry === 'string';
})
.join(','),
};
} else if (typeof metaValue === 'object') {
yield* objectToDescriptionListEntries(metaValue, nextPrefix + metaKey);
}
}
};

// Adding some styles to prevent horizontal scrollbars, per request from UX review
const StyledDescriptionList = memo(styled(EuiDescriptionList)`
&.euiDescriptionList.euiDescriptionList--column dt.euiDescriptionList__title.desc-title {
max-width: 8em;
overflow-wrap: break-word;
}
bkimmel marked this conversation as resolved.
Show resolved Hide resolved
&.euiDescriptionList.euiDescriptionList--column dd.euiDescriptionList__description {
max-width: calc(100% - 8.5em);
Expand Down Expand Up @@ -90,6 +50,39 @@ const TitleHr = memo(() => {
});
TitleHr.displayName = 'TitleHR';

// Indicates if the User Agent supports <wbr/>
const noWBRTagSupport = document.createElement('wbr') instanceof HTMLUnknownElement;
bkimmel marked this conversation as resolved.
Show resolved Hide resolved

/**
* Take description list entries and prepare them for display by
* replacing Zero-Width spaces with <wbr /> tags.
*
* @param entries {title: string, description: string}[]
*/
function entriesForDisplay(entries: Array<{ title: string; description: string }>) {
return noWBRTagSupport
? entries
: entries.map((entry) => {
return {
...entry,
title: (
<>
{entry.title.split('\u200b').map((titlePart, i) => {
return i ? (
<>
<wbr />
{titlePart}
</>
) : (
<>{titlePart}</>
);
})}
</>
),
};
});
}

/**
* This view presents a detailed view of all the available data for a related event, split and titled by the "section"
* it appears in the underlying ResolverEvent
Expand Down Expand Up @@ -138,60 +131,17 @@ export const RelatedEventDetail = memo(function RelatedEventDetail({
}
}, [relatedsReady, dispatch, processEntityId]);

const relatedEventsForThisProcess = useSelector(selectors.relatedEventsByEntityId).get(
processEntityId!
const [
relatedEventToShowDetailsFor,
countBySameCategory,
relatedEventCategory = naString,
sections,
formattedDate,
] = useSelector(selectors.relatedEventDisplayInfoByEntityAndSelfId)(
processEntityId,
relatedEventId
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ pulled a bunch of this junk into a selector

Copy link
Contributor

@michaelolo24 michaelolo24 Aug 26, 2020

Choose a reason for hiding this comment

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

I see the rationale of having view specific logic like this tied with the component rather than the data. Even though the it doesn't actually change any of the raw data in state, I think of selectors as specifically handling some kind of expensive/calculative logic rather than anything display related. Maybe a bit off here, but I consider this more fitting in the assets.tsx file. I don't think that name is particularly accurate anymore, but the concept of:
here are functions/components that, given some data, apply visual changes
is more representative. The name should be modified/changed when styles are cleaned up, but I think that may be a better compromise that will still allow this to be testable?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thinking out loud a bit, I think something a la a style-utils or something folder with separate file for visual components (cube assets etc...), text modifications, color modifications, and whatever else might make sense or not...probably something I missed here 😂 . Anyways, not really for this PR, but just thinking how we can begin to improve how we handle styles/visual changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think most of this stuff here does things that Robert has referred to in the past as "business logic" like counting things, filtering things out, selecting which category to display, etc. (and prefers that they be done in selectors) but I completely agree about thinking a little more deeply about how we organize visual components. I ended up bringing the "break-hinting" stuff back up to the front, b/c @kqualters-elastic made a good point that putting it in the selector just creates another place it can break (and Robert already had that GeneratedText thing done) but that might be a good first entry in that style-utils (until it finds a home in EUI or something later).

const [relatedEventToShowDetailsFor, countBySameCategory, relatedEventCategory] = useMemo(() => {
if (!relatedEventsForThisProcess) {
return [undefined, 0];
}
const specificEvent = relatedEventsForThisProcess.events.find(
(evt) => event.eventId(evt) === relatedEventId
);
// For breadcrumbs:
const specificCategory = specificEvent && event.primaryEventCategory(specificEvent);
const countOfCategory = relatedEventsForThisProcess.events.reduce((sumtotal, evt) => {
return event.primaryEventCategory(evt) === specificCategory ? sumtotal + 1 : sumtotal;
}, 0);
return [specificEvent, countOfCategory, specificCategory || naString];
}, [relatedEventsForThisProcess, naString, relatedEventId]);

const [sections, formattedDate] = useMemo(() => {
if (!relatedEventToShowDetailsFor) {
// This could happen if user relaods from URL param and requests an eventId that no longer exists
return [[], naString];
}
// Assuming these details (agent, ecs, process) aren't as helpful, can revisit
const {
agent,
ecs,
process,
...relevantData
} = relatedEventToShowDetailsFor as ResolverEvent & {
// Type this with various unknown keys so that ts will let us delete those keys
ecs: unknown;
process: unknown;
};
let displayDate = '';
const sectionData: Array<{
sectionTitle: string;
entries: Array<{ title: string; description: string }>;
}> = Object.entries(relevantData)
.map(([sectionTitle, val]) => {
if (sectionTitle === '@timestamp') {
displayDate = formatDate(val);
return { sectionTitle: '', entries: [] };
}
if (typeof val !== 'object') {
return { sectionTitle, entries: [{ title: sectionTitle, description: `${val}` }] };
}
return { sectionTitle, entries: [...objectToDescriptionListEntries(val)] };
})
.filter((v) => v.sectionTitle !== '' && v.entries.length);
return [sectionData, displayDate];
}, [relatedEventToShowDetailsFor, naString]);

const waitCrumbs = useMemo(() => {
return [
{
Expand Down Expand Up @@ -347,6 +297,12 @@ export const RelatedEventDetail = memo(function RelatedEventDetail({
</EuiText>
<EuiSpacer size="l" />
{sections.map(({ sectionTitle, entries }, index) => {
/**
* Replace Zero-Width Spaces with <wbr/> if the User Agent supports it.
* Both will hint breaking opportunities, but <wbr/> do not copy/paste
* so it's preferable to use them if the UA allows it.
*/
const displayEntries = entriesForDisplay(entries);
return (
<Fragment key={index}>
{index === 0 ? null : <EuiSpacer size="m" />}
Expand All @@ -364,7 +320,7 @@ export const RelatedEventDetail = memo(function RelatedEventDetail({
align="left"
titleProps={{ className: 'desc-title' }}
compressed
listItems={entries}
listItems={displayEntries}
/>
{index === sections.length - 1 ? null : <EuiSpacer size="m" />}
</Fragment>
Expand Down