[SIEM] Fix and consolidate handling of error responses in the client #59438

rylnd · 2020-03-05T15:34:04Z

Summary

As a followup to #58292 and our consolidation to status_code in error responses on the backend, this is a similar consolidation for the frontend. It was driven by the realization that our custom error messages were being lost in our client's handling of these error responses, and was an attempt to fix it.

After some further investigation, the most prevalent pattern for error response handling is:

allow the API-fetching function to throw an error on failure
- This was part of the responsibility of throwIfNotOk, and the default behavior of the new HTTP client
catch the error wherever the fetching function is invoked
in the catch block, if appropriate, use errorToToaster to display an error toaster with a contextual title and messaging from the Error itself.

This chore performs the follow tasks:

Adds a new type KibanaApiError representing our current/future error response
Updates errorToToaster to properly transfer our custom error messages to the toaster message
Moves errorToToaster to a more central location (rather than deep inside of ml/)
Removes any re-throwing logic (e.g. throwIfNotOk) within our API functions
Ensures all existing invocations' errors are caught, and updates their documentation to reflect that they throw errors
Removes throwIfNotOk and our custom endpoint-specific Error classes as they were redundant with the above.
Cleans up a few errant API calls using legacy modules

This branch changes the following behavior:

Handles an uncaught error on an alternate code path in the RuleSwitch component (see inline comment below)
Displays an error toaster when fetching the newsfeed has an error.

I had the following notes as followup to the above work:

useQuerySignals needs a call to errorToToaster
updateSignalsStatus needs a call to errorToToaster (already had a TODO)

Checklist

Delete any items that are not applicable to this PR.

Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

Throwing a TypeError meant that our manual errors were indistinguishable from, say, trying to invoke a method on undefined. This adds a custom error, BadRequestError, that disambiguates that situation.

With Core's new HTTP client, an unsuccessful API call will raise an error containing the body of the response it received. In the case of SIEM endpoints, this will include a useful error message with potentially more specificity than e.g. 'Internal Server Error'. This adds a type predicate to check for such errors, and adds a handling case in our errorToToaster handler. If the error does not contain our SIEM-specific message, it will fall through as normal and the general error.message will be displayed in the toaster.

The new HTTP client raises an error on a 4xx or 5xx response, so there should not be a case where throwIfNotOk is actually going to throw an error. The established pattern on the frontend is to catch errors at the call site and handle them appropriately, so I'm mainly just verifying that these are caught where they're used, now.

These were living in ML since that's where they originated. However, we have need of it (and already use it) elsewhere. The basic pattern for error handling on the frontend is: 1) API call throws error 2) caller catches error and dispatches a toast throwIfNotOk is meant to convert the error into a useful message in 1). We currently use both errorToToaster and displayErrorToast to display that in a toaster in 2) Now that errorToToaster handles a few different types of errors, and throwIfNotOk is going to be bypassed due to the new client behavior of throwing on error, we're going to start consolidating on: 1) Api call throws error 2) caller catches error and passes it to errorToToaster

* Ensures that all callers of these methods properly catch errors * Updates error toasterification to use errorToToaster * Simplifies tests now that we mainly just invoke the http client and return the result. throwIfNotOk is not being used in the majority of cases, as the client raises an error and bypasses that call. The few cases this might break are where we return a 200 but have errors within the response. Whether throwIfNotOk handled this or not, I'll need a simpler helper to accomplish the same behavior.

These can be an array of errors OR rules; typing it as such forces downstream to deal with both. enableRules was being handled correctly with the bucketing helper, and TS has confirmed the rest are as well. This obviates the need to raise from our API calls, as bulk errors are recoverable and we want to both a) continue on with any successful rules and b) handle the errors as necessary. This is highly dependent on the caller and so we can't/shouldn't handle it here.

I'm not sure that we're ever using this non-dispatch version, but it was throwing a type error. Will bring it up in review.

These are unneeded as an error response will already throw an error to be handled at the call site.

This was left over as a result of elastic#56261

Again, not needed because the client already throws.

* Gets rid of throwIfNotOK usage * uses core http fetch

This served the same purpose as errorToToaster, but in a less robust way. All usages have been replaced, so now we say goodbye.

…redicate There was no functional difference between these two code paths, and removing these custom errors allowed us to delete a bunch of associated code as well..

These were mainly related to my swapping any remaining fetch calls with the core router as good kibana denizens should :salute:

elasticmachine · 2020-03-05T15:34:08Z

Pinging @elastic/siem (Team:SIEM)

rylnd · 2020-03-05T15:39:35Z

x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/components/rule_switch/index.tsx

@@ -62,13 +64,29 @@ export const RuleSwitchComponent = ({
        await enableRulesAction([id], event.target.checked!, dispatch, dispatchToaster);


This is the normal path that catches/toasters the error properly. the else clause below is handling a case where we don't have dispatch. @XavierM is the dispatchless branch for portability, or do we have a specific use case for it right now?

After discussing with @XavierM, it looks like the else path here is used on the rule details page. We should consolidate these, but they're both currently used.

This is enough to get our tests to pass. We can't use the core mocks for now since there are circular dependencies there, which breaks our build.

dhurley14

LGTM. Detection engine changes look good. Did some testing locally and everything seemed good to go.

When possible. Also adds missing error-throwing documentation where necessary.

x-pack/legacy/plugins/siem/public/components/news_feed/index.tsx

This reverts commit 6421322.

x-pack/legacy/plugins/siem/public/components/news_feed/helpers.ts

x-pack/legacy/plugins/siem/public/components/toasters/errors.ts

x-pack/legacy/plugins/siem/public/components/toasters/utils.ts

FrankHassanabad · 2020-03-09T23:06:02Z

x-pack/legacy/plugins/siem/public/components/toasters/utils.ts

+export const displaySuccessToast = (
+  title: string,
+  dispatchToaster: React.Dispatch<ActionToaster>
+): void => {


👍 on typing the return types. Really appreciate it.

FrankHassanabad · 2020-03-09T23:16:25Z

x-pack/legacy/plugins/siem/public/containers/detection_engine/rules/api.test.ts

+      slice: jest.fn(),
+      stream: jest.fn(),
+      text: jest.fn(),
+    } as Blob;


This as Blob looks redundant since it is already declared above?

I used this and it was ok:

const blob: Blob = { size: 89, type: 'json', slice: jest.fn(), };

FrankHassanabad · 2020-03-09T23:30:27Z

x-pack/legacy/plugins/siem/public/containers/detection_engine/signals/api.test.ts

+      await fetchQuerySignals({ query: mockSignalsQuery, signal: abortCtrl.signal });
+      expect(fetchMock).toHaveBeenCalledWith('/api/detection_engine/signals/search', {
+        body:
+          '{"aggs":{"signalsByGrouping":{"terms":{"field":"signal.rule.risk_score","missing":"All others","order":{"_count":"desc"},"size":10},"aggs":{"signals":{"date_histogram":{"field":"@timestamp","fixed_interval":"81000000ms","min_doc_count":0,"extended_bounds":{"min":1579644343954,"max":1582236343955}}}}}},"query":{"bool":{"filter":[{"bool":{"must":[],"filter":[{"match_all":{}}],"should":[],"must_not":[]}},{"range":{"@timestamp":{"gte":1579644343954,"lte":1582236343955}}}]}}}',


These would be better as:

body: JSON.stringify({ aggs: ... }

Rather than string dumps for future maintainers.

Very true; these mainly came over from my cherry-pick of X's branch, and I simplified them since they're no longer dealing with the try/catch logic. I think we'd find that this test is pretty much the same as the one below it, and that this argument is in fact { body: JSON.stringify(mockSignalsQuery) } 😉

x-pack/legacy/plugins/siem/public/pages/detection_engine/rules/components/rule_switch/index.tsx

x-pack/legacy/plugins/siem/public/utils/api/index.ts

x-pack/legacy/plugins/siem/server/lib/detection_engine/routes/utils.ts

FrankHassanabad

Thanks for all the work on this.

Appreciate the BadRequest vs. TypeError.

Uses has to properly inspect our errorish object. Turns out we have a 'message' property, not an 'error' property.

We need a message (via new Error), a body.message, and a body.status_code to satisfy isApiError.

rylnd · 2020-03-10T03:04:50Z

@elasticmachine merge upstream

kibanamachine · 2020-03-10T04:40:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: e28734d

History

💔 Build #31953 failed 6ecc237
💚 Build #31890 succeeded 9e36715
💚 Build #31598 succeeded 9ca2c16
💚 Build #31225 succeeded f3477df
💚 Build #31209 succeeded 063d60a

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

…59438) (#59757) * Convert our manual throwing of TypeError to a custom Error Throwing a TypeError meant that our manual errors were indistinguishable from, say, trying to invoke a method on undefined. This adds a custom error, BadRequestError, that disambiguates that situation. * Present API Error messages to the user With Core's new HTTP client, an unsuccessful API call will raise an error containing the body of the response it received. In the case of SIEM endpoints, this will include a useful error message with potentially more specificity than e.g. 'Internal Server Error'. This adds a type predicate to check for such errors, and adds a handling case in our errorToToaster handler. If the error does not contain our SIEM-specific message, it will fall through as normal and the general error.message will be displayed in the toaster. * Remove unnecessary use of throwIfNotOk in our client API calls The new HTTP client raises an error on a 4xx or 5xx response, so there should not be a case where throwIfNotOk is actually going to throw an error. The established pattern on the frontend is to catch errors at the call site and handle them appropriately, so I'm mainly just verifying that these are caught where they're used, now. * Move errorToToaster and ToasterError to general location These were living in ML since that's where they originated. However, we have need of it (and already use it) elsewhere. The basic pattern for error handling on the frontend is: 1) API call throws error 2) caller catches error and dispatches a toast throwIfNotOk is meant to convert the error into a useful message in 1). We currently use both errorToToaster and displayErrorToast to display that in a toaster in 2) Now that errorToToaster handles a few different types of errors, and throwIfNotOk is going to be bypassed due to the new client behavior of throwing on error, we're going to start consolidating on: 1) Api call throws error 2) caller catches error and passes it to errorToToaster * Refactor Rules API functions to not use throwIfNotOk * Ensures that all callers of these methods properly catch errors * Updates error toasterification to use errorToToaster * Simplifies tests now that we mainly just invoke the http client and return the result. throwIfNotOk is not being used in the majority of cases, as the client raises an error and bypasses that call. The few cases this might break are where we return a 200 but have errors within the response. Whether throwIfNotOk handled this or not, I'll need a simpler helper to accomplish the same behavior. * Define a type for our BulkRule responses These can be an array of errors OR rules; typing it as such forces downstream to deal with both. enableRules was being handled correctly with the bucketing helper, and TS has confirmed the rest are as well. This obviates the need to raise from our API calls, as bulk errors are recoverable and we want to both a) continue on with any successful rules and b) handle the errors as necessary. This is highly dependent on the caller and so we can't/shouldn't handle it here. * Address case where bulk rules errors were not handled I'm not sure that we're ever using this non-dispatch version, but it was throwing a type error. Will bring it up in review. * Remove more throwIfNotOk uses from API calls These are unneeded as an error response will already throw an error to be handled at the call site. * Display an error toaster on newsfeed fetch failure * Remove dead code This was left over as a result of #56261 * Remove throwIfNotOk from case API calls Again, not needed because the client already throws. * Update use_get_tags for NP * Gets rid of throwIfNotOK usage * uses core http fetch * Remove throwIfNotOk from signals API * Remove throwIfNotOk This served the same purpose as errorToToaster, but in a less robust way. All usages have been replaced, so now we say goodbye. * Remove custom errors in favor of KibanaApiError and isApiError type predicate There was no functional difference between these two code paths, and removing these custom errors allowed us to delete a bunch of associated code as well.. * Fix test failures These were mainly related to my swapping any remaining fetch calls with the core router as good kibana denizens should :salute: * Replace use of core mocks with our simpler local ones This is enough to get our tests to pass. We can't use the core mocks for now since there are circular dependencies there, which breaks our build. * add signal api unit tests * privilege unit test api * Add unit tests on the signals container * Refactor signals API tests to use core mocks * Simplifies our mocking verbosity by leveraging core mocks * Simplifies test setup by isolating a reference to our fetch mock * Abstracts response structure to pure helper functions The try/catch tests had some false positives in that nothing would be asserted if the code did not throw an error. These proved to be masking a gap in coverage for our get/create signal index requests, which do not leverage `throwIfNotOk` but instead rely on the fetch to throw an error; once that behavior is verified we can update those tests to have our fetchMock throw errors, and we should be all set. * Simplify signals API tests now that the subjects do less We no longer re-throw errors, or parse the response, we just return the result of the client call. Simple! * Simplify API functions to use implict returns When possible. Also adds missing error-throwing documentation where necessary. * Revert "Display an error toaster on newsfeed fetch failure" This reverts commit 6421322. * Error property is readonly * Pull uuid generation into default argument value * Fix type predicate isApiError Uses has to properly inspect our errorish object. Turns out we have a 'message' property, not an 'error' property. * Fix test setup following modification of type predicate We need a message (via new Error), a body.message, and a body.status_code to satisfy isApiError. Co-authored-by: Xavier Mouligneau <[email protected]> Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Xavier Mouligneau <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

* master: Add a retry to dashboard test for a sometimes slow async operation (elastic#59600) [Endpoint] Sample data generator for endpoint app (elastic#58936) [Vis Editor] Refactoring metrics axes (elastic#59135) [DOCS] Changed Discover app to Discover (elastic#59769) [Metrics Alerts] Add support for search query and groupBy on alerts (elastic#59388) Enhancement - EUICodeEditor for Visualize JSON (elastic#58679) [ML] Transforms: Data grid fixes. (elastic#59538) [SIEM] Fix and consolidate handling of error responses in the client (elastic#59438) [Maps] convert tooltip classes to typescript (elastic#59589) [ML] Functional tests - re-activate date_nanos test (elastic#59649) Move canvas to use NP Expressions service (elastic#58387) Update misc dependencies (elastic#59542) [Unit Testing] Configure react-testing-library queries to use Kibana's data-test-subj instead of default data-testid (elastic#59445) [Console] Remove unused code (elastic#59554) [Logs / Metrics UI] Link handling / stop page reloads (elastic#58478) Add SavedObject management section registration in core (elastic#59291)

rylnd added 16 commits March 5, 2020 08:21

Convert our manual throwing of TypeError to a custom Error

a540e68

Throwing a TypeError meant that our manual errors were indistinguishable from, say, trying to invoke a method on undefined. This adds a custom error, BadRequestError, that disambiguates that situation.

Address case where bulk rules errors were not handled

15edf2d

I'm not sure that we're ever using this non-dispatch version, but it was throwing a type error. Will bring it up in review.

Remove more throwIfNotOk uses from API calls

797b607

These are unneeded as an error response will already throw an error to be handled at the call site.

Display an error toaster on newsfeed fetch failure

6421322

Remove dead code

cf80333

This was left over as a result of elastic#56261

Remove throwIfNotOk from case API calls

69b90f8

Again, not needed because the client already throws.

Update use_get_tags for NP

36c05f6

* Gets rid of throwIfNotOK usage * uses core http fetch

Remove throwIfNotOk from signals API

924508f

Remove throwIfNotOk

dcd8edf

This served the same purpose as errorToToaster, but in a less robust way. All usages have been replaced, so now we say goodbye.

Remove custom errors in favor of KibanaApiError and isApiError type p…

d83a21c

…redicate There was no functional difference between these two code paths, and removing these custom errors allowed us to delete a bunch of associated code as well..

Fix test failures

fd0e273

These were mainly related to my swapping any remaining fetch calls with the core router as good kibana denizens should :salute:

rylnd added bug Fixes for quality problems that affect the customer experience Team:SIEM v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.7.0 labels Mar 5, 2020

rylnd requested review from XavierM, FrankHassanabad and andrew-goldstein March 5, 2020 15:34

rylnd self-assigned this Mar 5, 2020

rylnd commented Mar 5, 2020

View reviewed changes

rylnd and others added 3 commits March 5, 2020 12:39

Replace use of core mocks with our simpler local ones

5f7467a

This is enough to get our tests to pass. We can't use the core mocks for now since there are circular dependencies there, which breaks our build.

add signal api unit tests

71acc43

privilege unit test api

4caf9ad

dhurley14 approved these changes Mar 9, 2020

View reviewed changes

rylnd added 2 commits March 9, 2020 14:35

Merge branch 'master' into fix_siem_errors

ba40135

Simplify API functions to use implict returns

9e36715

When possible. Also adds missing error-throwing documentation where necessary.

andrew-goldstein reviewed Mar 9, 2020

View reviewed changes

x-pack/legacy/plugins/siem/public/components/news_feed/index.tsx Outdated Show resolved Hide resolved

Revert "Display an error toaster on newsfeed fetch failure"

177490e

This reverts commit 6421322.