Skip to content

Commit 4f008a6

Browse files
rylndkibanamachine
andauthored
[Detection Engine] Fix infinite-looping bug related to bootstrapping lists resources (#241052)
# Summary Addresses #229018. ## Problem Statement When the users visits the alerting page, we check to see whether the lists indices exist, and if not, and they have the appropriate privileges, we try to create it on behalf of the user. However, if the user has enough privileges to _ask_ about the lists indices status, but has insufficient privileges to _create_ the lists indices, the frontend will enter an infinite loop. With sufficient privileges, this loop is exited when the frontend creates the lists indices. ## Explanation The looping is due to a) the fact that the "read list index" API responds with a 404 if either of the indices is not present, and b) how 404s are handled in the frontend. By default, a 404 will cause the http client to throw an error. We use useQuery to cache this query, but because the request throws an error the request is never cached, and thus on the next hook render the cycle renews. ## Solution By manually catching and parsing the 404 API response, we can both a) ensure proper caching and b) present more information to the consumer of that API. For backwards compatibility reasons, we cannot update the semantics of the API itself. Practically, since the response to a "list index not found" response is to call the (generic) "create list index" API, this additional information is not strictly necessary, the additional information could e.g. be used to provide a more helpful error message on the client in the future. ## Further Improvements In these cases where the lists indices do not exist, and the user does not have permission to create them, we do not currently inform the user of this fact. We could improve the user experience by providing a more helpful error message in this scenario. # How to review 1. Create a user/role in kibana with _almost_ all of the [necessary privileges to create the list indices](https://www.elastic.co/docs/solutions/security/detect-and-alert/detections-requirements#security-detections-requirements-custom-role-privileges), save for the "manage" cluster privilege (which is easily overlooked, due to its being a single word in a column on that table): ```json almost-lists-manager: cluster: [] indices: - names: - ".alerts-security.alerts-default" - ".internal.alerts-security.alerts-default-*" - ".siem-signals-default" - ".lists-default" - ".items-default" privileges: ["manage", "read", "write", "view_index_metadata"] field_security: grant: ["*"] except: [] allow_restricted_indices: false - names: - ".preview.alerts-security.alerts-default" - ".internal.preview.alerts-security.alerts-default-*" privileges: ["read"] field_security: grant: ["*"] except: [] allow_restricted_indices: false applications: - application: "kibana-.kibana" privileges: ["feature_indexPatterns.all", "feature_siemV4.all", "feature_actions.all", "feature_savedObjectsManagement.all"] resources: ["space:default"] run_as: [] metadata: {} transient_metadata: enabled: true description: "" ``` 1. Navigate to the alerts page, and confirm that the page loads successfully, and does not loop. 1. (optional) Confirm that navigating to the alerts page on `main` reproduces the infinite loop. 1. (optional) Confirm that adding the "manage" cluster privilege results in the lists indices being created successfully. ### Checklist - [ ] [Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials - [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 - [x] This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The `release_note:breaking` label should be applied in these situations. - [ ] [Flaky Test Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was used on any tests changed - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) - [x] Review the [backport guidelines](https://docs.google.com/document/d/1VyN5k91e5OVumlc0Gb9RPa3h1ewuPE705nRtioPiTvY/edit?usp=sharing) and apply applicable `backport:*` labels. --------- Co-authored-by: kibanamachine <[email protected]>
1 parent 95796dc commit 4f008a6

File tree

5 files changed

+320
-20
lines changed

5 files changed

+320
-20
lines changed
Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
import type { ListItemIndexExistSchema } from '@kbn/securitysolution-io-ts-list-types';
9+
10+
export const getListIndexExistSchemaMock = (
11+
overrides: Partial<ListItemIndexExistSchema> = {}
12+
): ListItemIndexExistSchema => ({
13+
list_index: true,
14+
list_item_index: true,
15+
...overrides,
16+
});
Lines changed: 252 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,252 @@
1+
/*
2+
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
3+
* or more contributor license agreements. Licensed under the Elastic License
4+
* 2.0; you may not use this file except in compliance with the Elastic License
5+
* 2.0.
6+
*/
7+
8+
import React from 'react';
9+
import { QueryClient, QueryClientProvider } from '@kbn/react-query';
10+
import { waitFor, renderHook } from '@testing-library/react';
11+
12+
import type { SecurityAppError } from '@kbn/securitysolution-t-grid';
13+
import { httpServiceMock } from '@kbn/core-http-browser-mocks';
14+
import * as Api from '@kbn/securitysolution-list-api';
15+
import { getListIndexExistSchemaMock } from '../mocks/response/read_list_index_schema.mock';
16+
import { useReadListIndex } from '.';
17+
18+
jest.mock('@kbn/securitysolution-list-api');
19+
20+
const customQueryProviderWrapper: React.FC<React.PropsWithChildren<{}>> = ({ children }) => {
21+
const mockLogger = { error: jest.fn(), log: jest.fn(), warn: jest.fn() };
22+
const queryClient = new QueryClient({ logger: mockLogger });
23+
return <QueryClientProvider client={queryClient}>{children}</QueryClientProvider>;
24+
};
25+
26+
describe('useReadListIndex', () => {
27+
let httpMock: ReturnType<typeof httpServiceMock.createStartContract>;
28+
29+
beforeEach(() => {
30+
httpMock = httpServiceMock.createStartContract();
31+
});
32+
33+
describe('when both indices exist', () => {
34+
beforeEach(() => {
35+
(Api.readListIndex as jest.Mock).mockResolvedValue(getListIndexExistSchemaMock());
36+
});
37+
38+
it('returns a response indicating both indices exist', async () => {
39+
const { result } = renderHook(
40+
() =>
41+
useReadListIndex({
42+
http: httpMock,
43+
isEnabled: true,
44+
}),
45+
{ wrapper: customQueryProviderWrapper }
46+
);
47+
48+
await waitFor(() => {
49+
expect(result.current).toEqual(
50+
expect.objectContaining({
51+
loading: false,
52+
result: { list_index: true, list_item_index: true },
53+
error: null,
54+
})
55+
);
56+
});
57+
});
58+
});
59+
60+
describe('error conditions', () => {
61+
let mockOnError: jest.Mock;
62+
63+
beforeEach(() => {
64+
mockOnError = jest.fn();
65+
});
66+
67+
it('does not call onError for an expected 404 response', async () => {
68+
const notFoundError: SecurityAppError = {
69+
message: 'Error',
70+
name: 'Error',
71+
body: {
72+
message: 'data stream .lists-default does not exist',
73+
status_code: 404,
74+
},
75+
};
76+
(Api.readListIndex as jest.Mock).mockRejectedValue(notFoundError);
77+
78+
const { result } = renderHook(
79+
() =>
80+
useReadListIndex({
81+
http: httpMock,
82+
isEnabled: true,
83+
onError: mockOnError,
84+
}),
85+
{ wrapper: customQueryProviderWrapper }
86+
);
87+
88+
await waitFor(() => {
89+
expect(result.current).toEqual(
90+
expect.objectContaining({
91+
loading: false,
92+
error: null,
93+
})
94+
);
95+
});
96+
97+
expect(mockOnError).not.toHaveBeenCalled();
98+
});
99+
100+
it('returns the appropriate response when neither list index exists', async () => {
101+
const neitherFoundError: SecurityAppError = {
102+
message: 'Error',
103+
name: 'Error',
104+
body: {
105+
message: 'data stream .lists-default and data stream .items-default does not exist',
106+
status_code: 404,
107+
},
108+
};
109+
(Api.readListIndex as jest.Mock).mockRejectedValue(neitherFoundError);
110+
111+
const { result } = renderHook(
112+
() =>
113+
useReadListIndex({
114+
http: httpMock,
115+
isEnabled: true,
116+
}),
117+
{ wrapper: customQueryProviderWrapper }
118+
);
119+
120+
await waitFor(() => {
121+
expect(result.current).toEqual(
122+
expect.objectContaining({
123+
loading: false,
124+
result: { list_index: false, list_item_index: false },
125+
error: null,
126+
})
127+
);
128+
});
129+
});
130+
131+
it('returns the appropriate response when only the lists index exists', async () => {
132+
const neitherFoundError: SecurityAppError = {
133+
message: 'Error',
134+
name: 'Error',
135+
body: {
136+
message: 'data stream .items-default does not exist',
137+
status_code: 404,
138+
},
139+
};
140+
(Api.readListIndex as jest.Mock).mockRejectedValue(neitherFoundError);
141+
142+
const { result } = renderHook(
143+
() =>
144+
useReadListIndex({
145+
http: httpMock,
146+
isEnabled: true,
147+
}),
148+
{ wrapper: customQueryProviderWrapper }
149+
);
150+
151+
await waitFor(() => {
152+
expect(result.current).toEqual(
153+
expect.objectContaining({
154+
loading: false,
155+
result: { list_index: true, list_item_index: false },
156+
error: null,
157+
})
158+
);
159+
});
160+
});
161+
162+
it('returns the appropriate response when only the items index exists', async () => {
163+
const neitherFoundError: SecurityAppError = {
164+
message: 'Error',
165+
name: 'Error',
166+
body: {
167+
message: 'data stream .lists-default does not exist',
168+
status_code: 404,
169+
},
170+
};
171+
(Api.readListIndex as jest.Mock).mockRejectedValue(neitherFoundError);
172+
173+
const { result } = renderHook(
174+
() =>
175+
useReadListIndex({
176+
http: httpMock,
177+
isEnabled: true,
178+
}),
179+
{ wrapper: customQueryProviderWrapper }
180+
);
181+
182+
await waitFor(() => {
183+
expect(result.current).toEqual(
184+
expect.objectContaining({
185+
loading: false,
186+
result: { list_index: false, list_item_index: true },
187+
error: null,
188+
})
189+
);
190+
});
191+
});
192+
193+
it('calls onError with and returns a generic error', async () => {
194+
const genericError = new Error('Generic error');
195+
(Api.readListIndex as jest.Mock).mockRejectedValue(genericError);
196+
197+
const { result } = renderHook(
198+
() =>
199+
useReadListIndex({
200+
http: httpMock,
201+
isEnabled: true,
202+
onError: mockOnError,
203+
}),
204+
{ wrapper: customQueryProviderWrapper }
205+
);
206+
207+
await waitFor(() => {
208+
expect(result.current).toEqual(
209+
expect.objectContaining({
210+
loading: false,
211+
result: undefined,
212+
error: genericError,
213+
})
214+
);
215+
});
216+
expect(mockOnError).toHaveBeenCalledWith(genericError);
217+
});
218+
219+
it('calls onError with and returns a 404 error that does not mention an index not being found', async () => {
220+
const genericNotFoundError: SecurityAppError = {
221+
message: 'Error',
222+
name: 'Error',
223+
body: {
224+
message: 'Not found',
225+
status_code: 404,
226+
},
227+
};
228+
(Api.readListIndex as jest.Mock).mockRejectedValue(genericNotFoundError);
229+
230+
const { result } = renderHook(
231+
() =>
232+
useReadListIndex({
233+
http: httpMock,
234+
isEnabled: true,
235+
onError: mockOnError,
236+
}),
237+
{ wrapper: customQueryProviderWrapper }
238+
);
239+
240+
await waitFor(() => {
241+
expect(result.current).toEqual(
242+
expect.objectContaining({
243+
loading: false,
244+
result: undefined,
245+
error: genericNotFoundError,
246+
})
247+
);
248+
});
249+
expect(mockOnError).toHaveBeenCalledWith(genericNotFoundError);
250+
});
251+
});
252+
});

x-pack/solutions/security/packages/kbn-securitysolution-list-hooks/src/use_read_list_index/index.ts

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,11 @@ import { useQuery } from '@kbn/react-query';
1010
import type { ApiParams } from '@kbn/securitysolution-list-api';
1111
import { readListIndex } from '@kbn/securitysolution-list-api';
1212
import { withOptionalSignal } from '@kbn/securitysolution-hook-utils';
13+
import {
14+
type SecurityAppError,
15+
isSecurityAppError,
16+
isNotFoundError,
17+
} from '@kbn/securitysolution-t-grid';
1318

1419
import { READ_INDEX_QUERY_KEY } from '../constants';
1520

@@ -31,7 +36,14 @@ export const useReadListIndex = ({
3136
return null;
3237
}
3338

34-
return readListIndexWithOptionalSignal({ http, signal });
39+
try {
40+
return await readListIndexWithOptionalSignal({ http, signal });
41+
} catch (err) {
42+
if (isIndexNotCreatedError(err)) {
43+
return parseReadIndexResultFrom404Error(err);
44+
}
45+
return Promise.reject(err);
46+
}
3547
},
3648
{
3749
onError,
@@ -48,3 +60,38 @@ export const useReadListIndex = ({
4860
error: query.error,
4961
};
5062
};
63+
64+
const errorMentionsListsIndex = (error: SecurityAppError): boolean =>
65+
error.body.message.includes('.lists-');
66+
const errorMentionsItemsIndex = (error: SecurityAppError): boolean =>
67+
error.body.message.includes('.items-');
68+
69+
/**
70+
* Determines whether an error response from the `readListIndex`
71+
* API call indicates that the index is not yet created.
72+
*/
73+
export const isIndexNotCreatedError = (err: unknown): err is SecurityAppError => {
74+
return (
75+
isSecurityAppError(err) &&
76+
isNotFoundError(err) &&
77+
(errorMentionsListsIndex(err) || errorMentionsItemsIndex(err))
78+
);
79+
};
80+
81+
/**
82+
* Parses the result of a 404 error from the read list index API into an actionable response
83+
*
84+
* This endpoint returns a 404 if either of the underlying indices do
85+
* not exist. By default, this causes the http client to throw an error,
86+
* but as this is a valid result from the endpoint, we want to catch that error
87+
* and present a result that is actionable to the consumer.
88+
* @param error The 404 IndexNotCreated error returned from the read list index API
89+
*/
90+
export const parseReadIndexResultFrom404Error = (
91+
error: SecurityAppError
92+
): Awaited<ReturnType<typeof readListIndexWithOptionalSignal>> => {
93+
return {
94+
list_index: !errorMentionsListsIndex(error),
95+
list_item_index: !errorMentionsItemsIndex(error),
96+
};
97+
};

x-pack/solutions/security/packages/kbn-securitysolution-list-hooks/tsconfig.json

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
},
1010
"include": [
1111
"**/*.ts",
12+
"**/*.tsx",
1213
],
1314
"kbn_references": [
1415
"@kbn/securitysolution-hook-utils",
@@ -19,6 +20,7 @@
1920
"@kbn/core-http-browser-mocks",
2021
"@kbn/core-http-browser",
2122
"@kbn/react-query",
23+
"@kbn/securitysolution-t-grid",
2224
],
2325
"exclude": [
2426
"target/**/*",

0 commit comments

Comments
 (0)