Skip to content

Commit

Permalink
[Security Solutions] Critical bug to add network responses to error t…
Browse files Browse the repository at this point in the history
…oasters (#97945) (#98430)

## Summary

When we updated our codebase to use the newer bsearch/async search, we ended up using error messages which were not the same as we had before which would show the network errors in the full message when the "see full error button" was clicked.

This has been bad lately as users and quality assurance people have been posting screen shots that have very little information, blank error messages, and/or stack traces instead of network errors. This makes them think the code has issues when it is a configuration issue or a networking issue that is happening.

This PR does the following:
* Changes all the bsearch queries to use the use useAppToasts
* Modifies the useAppToasts to be able to transform bsearch with the kibana global notification
* Cleans up the useAppToasts some
* Deprecates the GlobalErrorToaster in favor of the useAppToasts
* Fixes and adds a few i18n missing strings found
* Removes most of the deprecated error dispatch toasters from detection_engine except for 1 place where it is not a hook.
 
Before screen shot of errors with no buttons and messages that were not pointing to network errors:
<img width="422" alt="Screen Shot 2021-04-21 at 4 24 45 PM" src="https://user-images.githubusercontent.com/1151048/115642119-9a942300-a2d7-11eb-955a-b51097a8d3ec.png">


After screen shot where you have a button and that button will show you the network error:
<img width="395" alt="Screen Shot 2021-04-21 at 3 26 12 PM" src="https://user-images.githubusercontent.com/1151048/115642216-c6170d80-a2d7-11eb-8c94-0a1c15186fab.png">

<img width="786" alt="Screen Shot 2021-04-21 at 3 26 21 PM" src="https://user-images.githubusercontent.com/1151048/115642222-ca432b00-a2d7-11eb-9ddd-4ed6c6270b94.png">

You can manually test this easily by making non ECS indexes to cause errors and then add them as a kibana index and use them in the data sourcer.

### Checklist

- [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios
  • Loading branch information
FrankHassanabad authored Apr 27, 2021
1 parent 27173be commit 4a35c1a
Show file tree
Hide file tree
Showing 67 changed files with 583 additions and 277 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,34 +16,58 @@ import * as i18n from './translations';
export * from './utils';
export * from './errors';

/**
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
*/
export interface AppToast extends Toast {
errors?: string[];
}

/**
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
*/
interface ToastState {
toasts: AppToast[];
}

/**
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
*/
const initialToasterState: ToastState = {
toasts: [],
};

/**
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
*/
export type ActionToaster =
| { type: 'addToaster'; toast: AppToast }
| { type: 'deleteToaster'; id: string }
| { type: 'toggleWaitToShowNextToast' };

/**
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
*/
export const StateToasterContext = createContext<[ToastState, Dispatch<ActionToaster>]>([
initialToasterState,
() => noop,
]);

/**
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
*/
export const useStateToaster = () => useContext(StateToasterContext);

/**
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
*/
interface ManageGlobalToasterProps {
children: React.ReactNode;
}

/**
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
*/
export const ManageGlobalToaster = ({ children }: ManageGlobalToasterProps) => {
const reducerToaster = (state: ToastState, action: ActionToaster) => {
switch (action.type) {
Expand All @@ -63,16 +87,25 @@ export const ManageGlobalToaster = ({ children }: ManageGlobalToasterProps) => {
);
};

/**
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
*/
const GlobalToasterListContainer = styled.div`
position: absolute;
right: 0;
bottom: 0;
`;

/**
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
*/
interface GlobalToasterProps {
toastLifeTimeMs?: number;
}

/**
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
*/
export const GlobalToaster = ({ toastLifeTimeMs = 5000 }: GlobalToasterProps) => {
const [{ toasts }, dispatch] = useStateToaster();
const [isShowing, setIsShowing] = useState(false);
Expand Down Expand Up @@ -108,6 +141,9 @@ export const GlobalToaster = ({ toastLifeTimeMs = 5000 }: GlobalToasterProps) =>
);
};

/**
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
*/
const formatToErrorToastIfNeeded = (
toast: AppToast,
toggle: (toast: AppToast) => void
Expand All @@ -129,8 +165,14 @@ const formatToErrorToastIfNeeded = (
return toast;
};

/**
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
*/
const ErrorToastContainer = styled.div`
text-align: right;
`;

/**
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
*/
ErrorToastContainer.displayName = 'ErrorToastContainer';
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { isAppError } from '../../utils/api';

/**
* Displays an error toast for the provided title and message
*
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
* @param errorTitle Title of error to display in toaster and modal
* @param errorMessages Message to display in error modal when clicked
* @param dispatchToaster provided by useStateToaster()
Expand All @@ -41,7 +41,7 @@ export const displayErrorToast = (

/**
* Displays a warning toast for the provided title and message
*
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
* @param title warning message to display in toaster and modal
* @param dispatchToaster provided by useStateToaster()
* @param id unique ID if necessary
Expand All @@ -65,7 +65,7 @@ export const displayWarningToast = (

/**
* Displays a success toast for the provided title and message
*
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
* @param title success message to display in toaster and modal
* @param dispatchToaster provided by useStateToaster()
*/
Expand All @@ -92,8 +92,16 @@ export type ErrorToToasterArgs = Partial<AppToast> & {
};

/**
* Displays an error toast with messages parsed from the error
* Displays an error toast with messages parsed from the error.
*
* This has shortcomings and bugs compared to using the use_app_toasts because it takes naive guesses at the
* underlying data structure and does not display much about the error. This is not compatible with bsearch (async search)
* and sometimes can display to the user blank messages.
*
* The use_app_toasts has more feature rich logic and uses the Kibana toaster system to figure out which type of
* error you have in a more robust way then this function does and supersedes this function.
*
* @deprecated Use x-pack/plugins/security_solution/public/common/hooks/use_app_toasts.ts instead
* @param title error message to display in toaster and modal
* @param error the error from which messages will be parsed
* @param dispatchToaster provided by useStateToaster()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,11 @@ const mockUseKibana = {

jest.mock('../../../../common/lib/kibana', () => ({
useKibana: jest.fn(),
useToasts: jest.fn().mockReturnValue({
addError: jest.fn(),
addSuccess: jest.fn(),
addWarning: jest.fn(),
}),
}));

describe('useTimelineLastEventTime', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
} from '../../../../../../../../src/plugins/data/common';
import * as i18n from './translations';
import { DocValueFields } from '../../../../../common/search_strategy';
import { useAppToasts } from '../../../hooks/use_app_toasts';

export interface UseTimelineLastEventTimeArgs {
lastSeen: string | null;
Expand All @@ -45,7 +46,7 @@ export const useTimelineLastEventTime = ({
indexNames,
details,
}: UseTimelineLastEventTimeProps): [boolean, UseTimelineLastEventTimeArgs] => {
const { data, notifications } = useKibana().services;
const { data } = useKibana().services;
const refetch = useRef<inputsModel.Refetch>(noop);
const abortCtrl = useRef(new AbortController());
const searchSubscription$ = useRef(new Subscription());
Expand All @@ -69,6 +70,7 @@ export const useTimelineLastEventTime = ({
refetch: refetch.current,
errorMessage: undefined,
});
const { addError, addWarning } = useAppToasts();

const timelineLastEventTimeSearch = useCallback(
(request: TimelineEventsLastEventTimeRequestOptions) => {
Expand Down Expand Up @@ -96,15 +98,13 @@ export const useTimelineLastEventTime = ({
}));
} else if (isErrorResponse(response)) {
setLoading(false);
// TODO: Make response error status clearer
notifications.toasts.addWarning(i18n.ERROR_LAST_EVENT_TIME);
addWarning(i18n.ERROR_LAST_EVENT_TIME);
}
},
error: (msg) => {
setLoading(false);
notifications.toasts.addDanger({
addError(msg, {
title: i18n.FAIL_LAST_EVENT_TIME,
text: msg.message,
});
setTimelineLastEventTimeResponse((prevResponse) => ({
...prevResponse,
Expand All @@ -118,7 +118,7 @@ export const useTimelineLastEventTime = ({
asyncSearch();
refetch.current = asyncSearch;
},
[data.search, notifications.toasts]
[data.search, addError, addWarning]
);

useEffect(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { isErrorResponse, isCompleteResponse } from '../../../../../../../src/pl
import { getInspectResponse } from '../../../helpers';
import { InspectResponse } from '../../../types';
import * as i18n from './translations';
import { useAppToasts } from '../../hooks/use_app_toasts';

export type Buckets = Array<{
key: string;
Expand Down Expand Up @@ -60,7 +61,7 @@ export const useMatrixHistogram = ({
UseMatrixHistogramArgs,
(to: string, from: string) => void
] => {
const { data, notifications } = useKibana().services;
const { data } = useKibana().services;
const refetch = useRef<inputsModel.Refetch>(noop);
const abortCtrl = useRef(new AbortController());
const searchSubscription$ = useRef(new Subscription());
Expand All @@ -83,6 +84,7 @@ export const useMatrixHistogram = ({
...(isPtrIncluded != null ? { isPtrIncluded } : {}),
...(!isEmpty(docValueFields) ? { docValueFields } : {}),
});
const { addError, addWarning } = useAppToasts();

const [matrixHistogramResponse, setMatrixHistogramResponse] = useState<UseMatrixHistogramArgs>({
data: [],
Expand Down Expand Up @@ -126,14 +128,13 @@ export const useMatrixHistogram = ({
searchSubscription$.current.unsubscribe();
} else if (isErrorResponse(response)) {
setLoading(false);
// TODO: Make response error status clearer
notifications.toasts.addWarning(i18n.ERROR_MATRIX_HISTOGRAM);
addWarning(i18n.ERROR_MATRIX_HISTOGRAM);
searchSubscription$.current.unsubscribe();
}
},
error: (msg) => {
setLoading(false);
notifications.toasts.addError(msg, {
addError(msg, {
title: errorMessage ?? i18n.FAIL_MATRIX_HISTOGRAM,
});
searchSubscription$.current.unsubscribe();
Expand All @@ -145,7 +146,7 @@ export const useMatrixHistogram = ({
asyncSearch();
refetch.current = asyncSearch;
},
[data.search, errorMessage, notifications.toasts]
[data.search, errorMessage, addError, addWarning]
);

useEffect(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import * as i18n from './translations';
import { SourcererScopeName } from '../../store/sourcerer/model';
import { sourcererActions, sourcererSelectors } from '../../store/sourcerer';
import { DocValueFields } from '../../../../common/search_strategy/common';
import { useAppToasts } from '../../hooks/use_app_toasts';

export { BrowserField, BrowserFields, DocValueFields };

Expand Down Expand Up @@ -125,7 +126,7 @@ export const useFetchIndex = (
indexNames: string[],
onlyCheckIfIndicesExist: boolean = false
): [boolean, FetchIndexReturn] => {
const { data, notifications } = useKibana().services;
const { data } = useKibana().services;
const abortCtrl = useRef(new AbortController());
const searchSubscription$ = useRef(new Subscription());
const previousIndexesName = useRef<string[]>([]);
Expand All @@ -138,6 +139,7 @@ export const useFetchIndex = (
indexExists: true,
indexPatterns: DEFAULT_INDEX_PATTERNS,
});
const { addError, addWarning } = useAppToasts();

const indexFieldsSearch = useCallback(
(iNames) => {
Expand Down Expand Up @@ -168,14 +170,13 @@ export const useFetchIndex = (
searchSubscription$.current.unsubscribe();
} else if (isErrorResponse(response)) {
setLoading(false);
notifications.toasts.addWarning(i18n.ERROR_BEAT_FIELDS);
addWarning(i18n.ERROR_BEAT_FIELDS);
searchSubscription$.current.unsubscribe();
}
},
error: (msg) => {
setLoading(false);
notifications.toasts.addDanger({
text: msg.message,
addError(msg, {
title: i18n.FAIL_BEAT_FIELDS,
});
searchSubscription$.current.unsubscribe();
Expand All @@ -186,7 +187,7 @@ export const useFetchIndex = (
abortCtrl.current.abort();
asyncSearch();
},
[data.search, notifications.toasts, onlyCheckIfIndicesExist]
[data.search, addError, addWarning, onlyCheckIfIndicesExist]
);

useEffect(() => {
Expand All @@ -203,7 +204,7 @@ export const useFetchIndex = (
};

export const useIndexFields = (sourcererScopeName: SourcererScopeName) => {
const { data, notifications } = useKibana().services;
const { data } = useKibana().services;
const abortCtrl = useRef(new AbortController());
const searchSubscription$ = useRef(new Subscription());
const dispatch = useDispatch();
Expand All @@ -215,6 +216,7 @@ export const useIndexFields = (sourcererScopeName: SourcererScopeName) => {
indexNames: string[];
previousIndexNames: string;
}>((state) => indexNamesSelectedSelector(state, sourcererScopeName));
const { addError, addWarning } = useAppToasts();

const setLoading = useCallback(
(loading: boolean) => {
Expand Down Expand Up @@ -257,14 +259,13 @@ export const useIndexFields = (sourcererScopeName: SourcererScopeName) => {
searchSubscription$.current.unsubscribe();
} else if (isErrorResponse(response)) {
setLoading(false);
notifications.toasts.addWarning(i18n.ERROR_BEAT_FIELDS);
addWarning(i18n.ERROR_BEAT_FIELDS);
searchSubscription$.current.unsubscribe();
}
},
error: (msg) => {
setLoading(false);
notifications.toasts.addDanger({
text: msg.message,
addError(msg, {
title: i18n.FAIL_BEAT_FIELDS,
});
searchSubscription$.current.unsubscribe();
Expand All @@ -275,7 +276,7 @@ export const useIndexFields = (sourcererScopeName: SourcererScopeName) => {
abortCtrl.current.abort();
asyncSearch();
},
[data.search, dispatch, notifications.toasts, setLoading, sourcererScopeName]
[data.search, dispatch, addError, addWarning, setLoading, sourcererScopeName]
);

useEffect(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ jest.mock('../../utils/route/use_route_spy', () => ({
useRouteSpy: () => [mockRouteSpy],
}));
jest.mock('../../lib/kibana', () => ({
useToasts: jest.fn().mockReturnValue({
addError: jest.fn(),
addSuccess: jest.fn(),
addWarning: jest.fn(),
}),
useKibana: jest.fn().mockReturnValue({
services: {
application: {
Expand Down
Loading

0 comments on commit 4a35c1a

Please sign in to comment.