Skip to content

Commit

Permalink
[Security Solution] Keep original note creator (elastic#74203) (elast…
Browse files Browse the repository at this point in the history 
…ic#74293)

* keep original note creator

* unit test

* fix types

* typo

* wording

Co-authored-by: Angela Chuang <6295984+angorayc@users.noreply.github.com>
  • Loading branch information
@XavierM @angorayc
XavierM and angorayc authored Aug 4, 2020
1 parent ff72546 commit 8ca6677
Show file tree
Hide file tree
Showing 7 changed files with 171 additions and 25 deletions.
14 changes: 10 additions & 4 deletions x-pack/plugins/security_solution/server/graphql/note/resolvers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,16 @@ export const createNoteResolvers = (
return true;
},
async persistNote(root, args, { req }) {
return libs.note.persistNote(req, args.noteId || null, args.version || null, {
...args.note,
timelineId: args.note.timelineId || null,
});
return libs.note.persistNote(
req,
args.noteId || null,
args.version || null,
{
...args.note,
timelineId: args.note.timelineId || null,
},
true
);
},
},
});
12 changes: 7 additions & 5 deletions x-pack/plugins/security_solution/server/lib/note/saved_object.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ export interface Note {
request: FrameworkRequest,
noteId: string | null,
version: string | null,
note: SavedNote
note: SavedNote,
overrideOwner: boolean
) => Promise<ResponseNote>;
convertSavedObjectToSavedNote: (
savedObject: unknown,
Expand Down Expand Up @@ -136,7 +137,8 @@ export const persistNote = async (
request: FrameworkRequest,
noteId: string | null,
version: string | null,
note: SavedNote
note: SavedNote,
overrideOwner: boolean = true
): Promise<ResponseNote> => {
try {
const savedObjectsClient = request.context.core.savedObjects.client;
Expand All @@ -163,14 +165,14 @@ export const persistNote = async (
note: convertSavedObjectToSavedNote(
await savedObjectsClient.create(
noteSavedObjectType,
pickSavedNote(noteId, note, request.user)
overrideOwner ? pickSavedNote(noteId, note, request.user) : note
),
timelineVersionSavedObject != null ? timelineVersionSavedObject : undefined
),
};
}

// Update new note
// Update existing note

const existingNote = await getSavedNote(request, noteId);
return {
Expand All @@ -180,7 +182,7 @@ export const persistNote = async (
await savedObjectsClient.update(
noteSavedObjectType,
noteId,
pickSavedNote(noteId, note, request.user),
overrideOwner ? pickSavedNote(noteId, note, request.user) : note,
{
version: existingNote.version || undefined,
}
Expand Down
3 changes: 2 additions & 1 deletion x-pack/plugins/security_solution/server/lib/timeline/routes/create_timelines_route.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
TimelineStatusActions,
} from './utils/common';
import { createTimelines } from './utils/create_timelines';
import { DEFAULT_ERROR } from './utils/failure_cases';

export const createTimelinesRoute = (
router: IRouter,
Expand Down Expand Up @@ -85,7 +86,7 @@ export const createTimelinesRoute = (
return siemResponse.error(
compareTimelinesStatus.checkIsFailureCases(TimelineStatusActions.create) || {
statusCode: 405,
body: 'update timeline error',
body: DEFAULT_ERROR,
}
);
}
Expand Down
98 changes: 97 additions & 1 deletion x-pack/plugins/security_solution/server/lib/timeline/routes/import_timelines_route.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ describe('import timelines', () => {
let mockPersistTimeline: jest.Mock;
let mockPersistPinnedEventOnTimeline: jest.Mock;
let mockPersistNote: jest.Mock;
let mockGetNote: jest.Mock;
let mockGetTupleDuplicateErrorsAndUniqueTimeline: jest.Mock;

beforeEach(() => {
Expand All @@ -69,6 +70,7 @@ describe('import timelines', () => {
mockPersistTimeline = jest.fn();
mockPersistPinnedEventOnTimeline = jest.fn();
mockPersistNote = jest.fn();
mockGetNote = jest.fn();
mockGetTupleDuplicateErrorsAndUniqueTimeline = jest.fn();

jest.doMock('../create_timelines_stream_from_ndjson', () => {
Expand Down Expand Up @@ -113,6 +115,37 @@ describe('import timelines', () => {
jest.doMock('../../note/saved_object', () => {
return {
persistNote: mockPersistNote,
getNote: mockGetNote
.mockResolvedValueOnce({
noteId: 'd2649d40-6bc5-11ea-86f0-5db0048c6086',
version: 'WzExNjQsMV0=',
eventId: undefined,
note: 'original note',
created: '1584830796960',
createdBy: 'original author A',
updated: '1584830796960',
updatedBy: 'original author A',
})
.mockResolvedValueOnce({
noteId: '73ac2370-6bc2-11ea-a90b-f5341fb7a189',
version: 'WzExMjgsMV0=',
eventId: 'ZaAi8nAB5OldxqFfdhke',
note: 'original event note',
created: '1584830796960',
createdBy: 'original author B',
updated: '1584830796960',
updatedBy: 'original author B',
})
.mockResolvedValue({
noteId: 'f7b71620-6bc2-11ea-a0b6-33c7b2a78885',
version: 'WzExMzUsMV0=',
eventId: 'ZaAi8nAB5OldxqFfdhke',
note: 'event note2',
created: '1584830796960',
createdBy: 'angela',
updated: '1584830796960',
updatedBy: 'angela',
}),
};
});

Expand Down Expand Up @@ -213,6 +246,14 @@ describe('import timelines', () => {
);
});

test('should Check if note exists', async () => {
const mockRequest = getImportTimelinesRequest();
await server.inject(mockRequest, context);
expect(mockGetNote.mock.calls[0][1]).toEqual(
mockUniqueParsedObjects[0].globalNotes[0].noteId
);
});

test('should Create notes', async () => {
const mockRequest = getImportTimelinesRequest();
await server.inject(mockRequest, context);
Expand All @@ -237,20 +278,67 @@ describe('import timelines', () => {
expect(mockPersistNote.mock.calls[0][2]).toEqual(mockCreatedTimeline.version);
});

test('should provide new notes when Creating notes for a timeline', async () => {
test('should provide new notes with original author info when Creating notes for a timeline', async () => {
const mockRequest = getImportTimelinesRequest();
await server.inject(mockRequest, context);
expect(mockPersistNote.mock.calls[0][3]).toEqual({
eventId: undefined,
note: 'original note',
created: '1584830796960',
createdBy: 'original author A',
updated: '1584830796960',
updatedBy: 'original author A',
timelineId: mockCreatedTimeline.savedObjectId,
});
expect(mockPersistNote.mock.calls[1][3]).toEqual({
eventId: mockUniqueParsedObjects[0].eventNotes[0].eventId,
note: 'original event note',
created: '1584830796960',
createdBy: 'original author B',
updated: '1584830796960',
updatedBy: 'original author B',
timelineId: mockCreatedTimeline.savedObjectId,
});
expect(mockPersistNote.mock.calls[2][3]).toEqual({
eventId: mockUniqueParsedObjects[0].eventNotes[1].eventId,
note: 'event note2',
created: '1584830796960',
createdBy: 'angela',
updated: '1584830796960',
updatedBy: 'angela',
timelineId: mockCreatedTimeline.savedObjectId,
});
});

test('should keep current author if note does not exist when Creating notes for a timeline', async () => {
mockGetNote.mockReset();
mockGetNote.mockRejectedValue(new Error());

const mockRequest = getImportTimelinesRequest();
await server.inject(mockRequest, context);
expect(mockPersistNote.mock.calls[0][3]).toEqual({
created: mockUniqueParsedObjects[0].globalNotes[0].created,
createdBy: mockUniqueParsedObjects[0].globalNotes[0].createdBy,
updated: mockUniqueParsedObjects[0].globalNotes[0].updated,
updatedBy: mockUniqueParsedObjects[0].globalNotes[0].updatedBy,
eventId: undefined,
note: mockUniqueParsedObjects[0].globalNotes[0].note,
timelineId: mockCreatedTimeline.savedObjectId,
});
expect(mockPersistNote.mock.calls[1][3]).toEqual({
created: mockUniqueParsedObjects[0].eventNotes[0].created,
createdBy: mockUniqueParsedObjects[0].eventNotes[0].createdBy,
updated: mockUniqueParsedObjects[0].eventNotes[0].updated,
updatedBy: mockUniqueParsedObjects[0].eventNotes[0].updatedBy,
eventId: mockUniqueParsedObjects[0].eventNotes[0].eventId,
note: mockUniqueParsedObjects[0].eventNotes[0].note,
timelineId: mockCreatedTimeline.savedObjectId,
});
expect(mockPersistNote.mock.calls[2][3]).toEqual({
created: mockUniqueParsedObjects[0].eventNotes[1].created,
createdBy: mockUniqueParsedObjects[0].eventNotes[1].createdBy,
updated: mockUniqueParsedObjects[0].eventNotes[1].updated,
updatedBy: mockUniqueParsedObjects[0].eventNotes[1].updatedBy,
eventId: mockUniqueParsedObjects[0].eventNotes[1].eventId,
note: mockUniqueParsedObjects[0].eventNotes[1].note,
timelineId: mockCreatedTimeline.savedObjectId,
Expand Down Expand Up @@ -573,6 +661,10 @@ describe('import timeline templates', () => {
expect(mockPersistNote.mock.calls[0][3]).toEqual({
eventId: undefined,
note: mockUniqueParsedTemplateTimelineObjects[0].globalNotes[0].note,
created: mockUniqueParsedTemplateTimelineObjects[0].globalNotes[0].created,
createdBy: mockUniqueParsedTemplateTimelineObjects[0].globalNotes[0].createdBy,
updated: mockUniqueParsedTemplateTimelineObjects[0].globalNotes[0].updated,
updatedBy: mockUniqueParsedTemplateTimelineObjects[0].globalNotes[0].updatedBy,
timelineId: mockCreatedTemplateTimeline.savedObjectId,
});
});
Expand Down Expand Up @@ -721,6 +813,10 @@ describe('import timeline templates', () => {
expect(mockPersistNote.mock.calls[0][3]).toEqual({
eventId: undefined,
note: mockUniqueParsedTemplateTimelineObjects[0].globalNotes[0].note,
created: mockUniqueParsedTemplateTimelineObjects[0].globalNotes[0].created,
createdBy: mockUniqueParsedTemplateTimelineObjects[0].globalNotes[0].createdBy,
updated: mockUniqueParsedTemplateTimelineObjects[0].globalNotes[0].updated,
updatedBy: mockUniqueParsedTemplateTimelineObjects[0].globalNotes[0].updatedBy,
timelineId: mockCreatedTemplateTimeline.savedObjectId,
});
});
Expand Down
60 changes: 49 additions & 11 deletions x-pack/plugins/security_solution/server/lib/timeline/routes/utils/create_timelines.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,26 +45,56 @@ export const savePinnedEvents = (
)
);

export const saveNotes = (
const getNewNote = async (
frameworkRequest: FrameworkRequest,
note: NoteResult,
timelineSavedObjectId: string,
timelineVersion?: string | null,
existingNoteIds?: string[],
newNotes?: NoteResult[]
) => {
return Promise.all(
newNotes?.map((note) => {
const newNote: SavedNote = {
overrideOwner: boolean
): Promise<SavedNote> => {
let savedNote = note;
try {
savedNote = await noteLib.getNote(frameworkRequest, note.noteId);
// eslint-disable-next-line no-empty
} catch (e) {}
return overrideOwner
? {
eventId: note.eventId,
note: note.note,
timelineId: timelineSavedObjectId,
}
: {
eventId: savedNote.eventId,
note: savedNote.note,
created: savedNote.created,
createdBy: savedNote.createdBy,
updated: savedNote.updated,
updatedBy: savedNote.updatedBy,
timelineId: timelineSavedObjectId,
};
};

export const saveNotes = async (
frameworkRequest: FrameworkRequest,
timelineSavedObjectId: string,
timelineVersion?: string | null,
existingNoteIds?: string[],
newNotes?: NoteResult[],
overrideOwner: boolean = true
) => {
return Promise.all(
newNotes?.map(async (note) => {
const newNote = await getNewNote(
frameworkRequest,
note,
timelineSavedObjectId,
overrideOwner
);
return noteLib.persistNote(
frameworkRequest,
existingNoteIds?.find((nId) => nId === note.noteId) ?? null,
overrideOwner ? existingNoteIds?.find((nId) => nId === note.noteId) ?? null : null,
timelineVersion ?? null,
newNote
newNote,
overrideOwner
);
}) ?? []
);
Expand All @@ -75,12 +105,18 @@ interface CreateTimelineProps {
timeline: SavedTimeline;
timelineSavedObjectId?: string | null;
timelineVersion?: string | null;
overrideNotesOwner?: boolean;
pinnedEventIds?: string[] | null;
notes?: NoteResult[];
existingNoteIds?: string[];
isImmutable?: boolean;
}

/** allow overrideNotesOwner means overriding by current username,
* disallow overrideNotesOwner means keep the original username.
* overrideNotesOwner = false only happens when import timeline templates,
* as we want to keep the original creator for notes
**/
export const createTimelines = async ({
frameworkRequest,
timeline,
Expand All @@ -90,6 +126,7 @@ export const createTimelines = async ({
notes = [],
existingNoteIds = [],
isImmutable,
overrideNotesOwner = true,
}: CreateTimelineProps): Promise<ResponseTimeline> => {
const responseTimeline = await saveTimelines(
frameworkRequest,
Expand Down Expand Up @@ -119,7 +156,8 @@ export const createTimelines = async ({
timelineSavedObjectId ?? newTimelineSavedObjectId,
newTimelineVersion,
existingNoteIds,
notes
notes,
overrideNotesOwner
),
];
}
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/security_solution/server/lib/timeline/routes/utils/failure_cases.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export const NOT_ALLOW_UPDATE_TIMELINE_TYPE_ERROR_MESSAGE =
'You cannot convert a Timeline template to a timeline, or a timeline to a Timeline template.';
export const UPDAT_TIMELINE_VIA_IMPORT_NOT_ALLOWED_ERROR_MESSAGE =
'You cannot update a timeline via imports. Use the UI to modify existing timelines.';
export const DEFAULT_ERROR = `Something has gone wrong. We didn't handle something properly. To help us fix this, please upload your file to https://discuss.elastic.co/c/security/siem.`;

const isUpdatingStatus = (
isHandlingTemplateTimeline: boolean,
Expand Down
Loading

0 comments on commit 8ca6677

Please sign in to comment.