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][Notes] Make MAX_UNASSOCIATED_NOTES an advanced Kibana setting #194947

Merged
Merged
Show file tree
Hide file tree
Changes from all 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 @@ -22,6 +22,10 @@ export const stackManagementSchema: MakeSchemaFrom<UsageStats> = {
_meta: { description: 'Non-default value of setting.' },
},
},
'securitySolution:maxUnassociatedNotes': {
type: 'integer',
_meta: { description: 'The maximum number of allowed unassociated notes' },
},
'securitySolution:defaultThreatIndex': {
type: 'keyword',
_meta: { description: 'Default value of the setting was changed.' },
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -183,5 +183,6 @@ export interface UsageStats {
'aiAssistant:preferredAIAssistantType': string;
'observability:profilingFetchTopNFunctionsFromStacktraces': boolean;
'securitySolution:excludedDataTiersForRuleExecution': string[];
'securitySolution:maxUnassociatedNotes': number;
'observability:searchExcludedDataTiers': string[];
}
8 changes: 7 additions & 1 deletion src/plugins/telemetry/schema/oss_plugins.json
Original file line number Diff line number Diff line change
Expand Up @@ -9888,6 +9888,12 @@
}
}
},
"securitySolution:maxUnassociatedNotes": {
"type": "integer",
"_meta": {
"description": "The maximum number of allowed unassociated notes"
}
},
"securitySolution:defaultThreatIndex": {
"type": "keyword",
"_meta": {
Expand Down Expand Up @@ -10050,7 +10056,7 @@
"description": "Non-default value of setting."
}
},
"securitySolution:enableVisualizationsInFlyout":{
"securitySolution:enableVisualizationsInFlyout": {
"type": "boolean",
"_meta": {
"description": "Non-default value of setting."
Expand Down
4 changes: 4 additions & 0 deletions x-pack/plugins/security_solution/common/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ export const SECURITY_TAG_NAME = 'Security Solution' as const;
export const SECURITY_TAG_DESCRIPTION = 'Security Solution auto-generated tag' as const;
export const DEFAULT_SPACE_ID = 'default' as const;
export const DEFAULT_RELATIVE_DATE_THRESHOLD = 24 as const;
export const DEFAULT_MAX_UNASSOCIATED_NOTES = 1000 as const;

// Document path where threat indicator fields are expected. Fields are used
// to enrich signals, and are copied to threat.enrichments.
Expand Down Expand Up @@ -200,6 +201,9 @@ export const ENABLE_ASSET_CRITICALITY_SETTING = 'securitySolution:enableAssetCri
export const EXCLUDED_DATA_TIERS_FOR_RULE_EXECUTION =
'securitySolution:excludedDataTiersForRuleExecution' as const;

/** This Kibana Advances setting allows users to define the maximum amount of unassociated notes (notes without a `timelineId`) */
export const MAX_UNASSOCIATED_NOTES = 'securitySolution:maxUnassociatedNotes' as const;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider renaming this to something as below because current name feels like it is the number of MAX_NOTES but not the setting name.

Suggested change
export const MAX_UNASSOCIATED_NOTES = 'securitySolution:maxUnassociatedNotes' as const;
export const MAX_UNASSOCIATED_NOTES_SETTING = 'securitySolution:maxUnassociatedNotes' as const;


/** This Kibana Advanced Setting allows users to enable/disable the Visualizations in Flyout feature */
export const ENABLE_VISUALIZATIONS_IN_FLYOUT_SETTING =
'securitySolution:enableVisualizationsInFlyout' as const;
Expand Down
53 changes: 53 additions & 0 deletions x-pack/plugins/security_solution/public/notes/api/api.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0; you may not use this file except in compliance with the Elastic License
* 2.0.
*/
import type { PersistNoteRouteResponse } from '../../../common/api/timeline';
import { KibanaServices } from '../../common/lib/kibana';
import * as api from './api';

jest.mock('../../common/lib/kibana', () => {
return {
KibanaServices: {
get: jest.fn(),
},
};
});

describe('Notes API client', () => {
beforeAll(() => {
jest.resetAllMocks();
});
describe('create note', () => {
it('should throw an error when a response code other than 200 is returned', async () => {
const errorResponse: PersistNoteRouteResponse = {
data: {
persistNote: {
code: 500,
message: 'Internal server error',
note: {
timelineId: '1',
noteId: '2',
version: '3',
},
},
},
};
(KibanaServices.get as jest.Mock).mockReturnValue({
http: {
patch: jest.fn().mockReturnValue(errorResponse),
},
});

expect(async () =>
api.createNote({
note: {
timelineId: '1',
},
})
).rejects.toThrow();
});
});
});
68 changes: 33 additions & 35 deletions x-pack/plugins/security_solution/public/notes/api/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,12 @@
* 2.0.
*/

import type { BareNote, Note } from '../../../common/api/timeline';
import type {
BareNote,
DeleteNoteResponse,
GetNotesResponse,
PersistNoteRouteResponse,
} from '../../../common/api/timeline';
import { KibanaServices } from '../../common/lib/kibana';
import { NOTE_URL } from '../../../common/constants';

Expand All @@ -16,16 +21,18 @@ import { NOTE_URL } from '../../../common/constants';
*/
export const createNote = async ({ note }: { note: BareNote }) => {
try {
const response = await KibanaServices.get().http.patch<{
data: { persistNote: { code: number; message: string; note: Note } };
}>(NOTE_URL, {
const response = await KibanaServices.get().http.patch<PersistNoteRouteResponse>(NOTE_URL, {
method: 'PATCH',
body: JSON.stringify({ note }),
version: '2023-10-31',
});
return response.data.persistNote.note;
const noteResponse = response.data.persistNote;
if (noteResponse.code !== 200) {
throw new Error(noteResponse.message);
}
return noteResponse.note;
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding unit tests for this case.

} catch (err) {
throw new Error(`Failed to stringify query: ${JSON.stringify(err)}`);
throw new Error(('message' in err && err.message) || 'Request failed');
}
};

Expand All @@ -44,56 +51,47 @@ export const fetchNotes = async ({
filter: string;
search: string;
}) => {
const response = await KibanaServices.get().http.get<{ totalCount: number; notes: Note[] }>(
NOTE_URL,
{
query: {
page,
perPage,
sortField,
sortOrder,
filter,
search,
},
version: '2023-10-31',
}
);
const response = await KibanaServices.get().http.get<GetNotesResponse>(NOTE_URL, {
query: {
page,
perPage,
sortField,
sortOrder,
filter,
search,
},
version: '2023-10-31',
});
return response;
};

/**
* Fetches all the notes for an array of document ids
*/
export const fetchNotesByDocumentIds = async (documentIds: string[]) => {
const response = await KibanaServices.get().http.get<{ notes: Note[]; totalCount: number }>(
NOTE_URL,
{
query: { documentIds },
version: '2023-10-31',
}
);
const response = await KibanaServices.get().http.get<GetNotesResponse>(NOTE_URL, {
query: { documentIds },
version: '2023-10-31',
});
return response;
};

/**
* Fetches all the notes for an array of saved object ids
*/
export const fetchNotesBySaveObjectIds = async (savedObjectIds: string[]) => {
const response = await KibanaServices.get().http.get<{ notes: Note[]; totalCount: number }>(
NOTE_URL,
{
query: { savedObjectIds },
version: '2023-10-31',
}
);
const response = await KibanaServices.get().http.get<GetNotesResponse>(NOTE_URL, {
query: { savedObjectIds },
version: '2023-10-31',
});
return response;
};

/**
* Deletes multiple notes
*/
export const deleteNotes = async (noteIds: string[]) => {
const response = await KibanaServices.get().http.delete<{ data: unknown }>(NOTE_URL, {
const response = await KibanaServices.get().http.delete<DeleteNoteResponse>(NOTE_URL, {
Copy link
Contributor

Choose a reason for hiding this comment

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

👌 on adding all the types.

body: JSON.stringify({ noteIds }),
version: '2023-10-31',
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,7 @@ describe('AddNote', () => {
});

it('should render error toast if create a note fails', () => {
const createNoteError = new Error('This error comes from the backend');
const store = createMockStore({
...mockGlobalState,
notes: {
Expand All @@ -112,7 +113,7 @@ describe('AddNote', () => {
},
error: {
...mockGlobalState.notes.error,
createNote: { type: 'http', status: 500 },
createNote: createNoteError,
},
},
});
Expand All @@ -123,9 +124,12 @@ describe('AddNote', () => {
</TestProviders>
);

expect(mockAddError).toHaveBeenCalledWith(null, {
title: CREATE_NOTE_ERROR,
});
expect(mockAddError).toHaveBeenCalledWith(
createNoteError,
expect.objectContaining({
title: CREATE_NOTE_ERROR,
})
);
});

it('should call onNodeAdd callback when it is available', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
ReqStatus,
selectCreateNoteError,
selectCreateNoteStatus,
userClosedCreateErrorToast,
} from '../store/notes.slice';
import { MarkdownEditor } from '../../common/components/markdown_editor';

Expand Down Expand Up @@ -101,14 +102,19 @@ export const AddNote = memo(
setEditorValue('');
}, [dispatch, editorValue, eventId, telemetry, timelineId, onNoteAdd]);

const resetError = useCallback(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

currently if the user tries to close the error toast using the cross icon in the top-right corner nothing happens. You're forced to click on the See the full error button that opens the modal then it clears

Screen.Recording.2024-10-10.at.4.35.21.PM.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

this is now fixed, thanks!

dispatch(userClosedCreateErrorToast());
}, [dispatch]);

// show a toast if the create note call fails
useEffect(() => {
if (createStatus === ReqStatus.Failed && createError) {
addErrorToast(null, {
addErrorToast(createError, {
title: CREATE_NOTE_ERROR,
});
resetError();
}
}, [addErrorToast, createError, createStatus]);
}, [addErrorToast, createError, createStatus, resetError]);

const buttonDisabled = useMemo(
() => disableButton || editorValue.trim().length === 0 || isMarkdownInvalid,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import {
fetchNotesBySavedObjectIds,
selectNotesBySavedObjectId,
selectSortedNotesBySavedObjectId,
userClosedCreateErrorToast,
} from './notes.slice';
import type { NotesState } from './notes.slice';
import { mockGlobalState } from '../../common/mock';
Expand Down Expand Up @@ -533,6 +534,25 @@ describe('notesSlice', () => {
});
});

describe('userClosedCreateErrorToast', () => {
it('should reset create note error', () => {
const action = { type: userClosedCreateErrorToast.type };

expect(
notesReducer(
{
...initalEmptyState,
error: {
...initalEmptyState.error,
createNote: new Error(),
},
},
action
).error.createNote
).toBe(null);
});
});

describe('userSelectedNotesForDeletion', () => {
it('should set correct id when user selects a note to delete', () => {
const action = { type: userSelectedNotesForDeletion.type, payload: '1' };
Expand Down
Loading