[SecuritySolution] Fix behavior of pinnend events and comments on unsaved timelines #178212

janmonschke · 2024-03-07T12:53:09Z

This PR is the implementation of the results in the discussion around [ADR] 0001 - Saving of timeline-associated saved objects

Summary

As described in #178182, the removal of autosave on timeline resulted in a regression in which pinned events and comments on unsaved timelines are lost.

In this PR, we're re-introducing the previous behavior by saving the timeline when a comment is made / an event is pinned on an unsaved timeline. The timeline will then be auto-saved in a draft state instead of an active state that would persist it permanently.

Fixes #178182

Checklist

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2024-03-07T12:53:12Z

Pinging @elastic/security-threat-hunting-investigations (Team:Threat Hunting:Investigations)

PhilippeOberti · 2024-03-08T11:01:06Z

@janmonschke I haven't looked at the code itself yet, but have pulled down the branch and tested locally. While the fix seems to be working just fine, I'm not sure the approach is what I would have done.
It feels a bit weird to have the timeline magically saved when adding comments or notes. I think I would have maybe either:

disabled all the features we do not want to be done on an unsaved timeline (so for this PR it would just be disabling the Notes tab and the Add note button). This is the approach I took when I fix a similar issue where users could favorite an unsaved timeline, which makes no sense (see this PR)
instead of silently saving, maybe we could show the save modal? That way users wouldn't end up with tons of Untitled timeline timelines like I just did during testing 😆

janmonschke · 2024-03-12T15:28:06Z

@PhilippeOberti Agreed, wdyt about #178525 ?

janmonschke · 2024-04-02T13:29:05Z

Reopened this PR and updated/improved the tests.

x-pack/plugins/security_solution/public/timelines/store/middlewares/timeline_note.ts

…reated

janmonschke · 2024-04-04T19:30:42Z

@elasticmachine merge upstream

janmonschke · 2024-04-05T12:51:21Z

x-pack/test/api_integration/apis/security_solution/saved_objects/draft_timeline.ts

+  const kibanaServer = getService('kibanaServer');
+  const supertest = getService('supertest');
+
+  describe('Draft timeline - Saved Objects', () => {


There were no FTR tests for the draft timeline route at all and parts of it were actually broken (see 633b8b6).

janmonschke · 2024-04-05T12:52:24Z

x-pack/plugins/security_solution/public/timelines/containers/pinned_event/api.ts

@@ -15,7 +15,7 @@ export const persistPinnedEvent = async ({
 }: {
  eventId: string;
  pinnedEventId?: string | null;
-  timelineId?: string | null;
+  timelineId: string;


The presence of timelineId is now enforced on the API.

janmonschke · 2024-04-05T12:53:50Z

x-pack/plugins/security_solution/common/api/timeline/model/api.ts

@@ -51,9 +51,6 @@ export const NoteRuntimeType = runtimeTypes.intersection([
    noteId: runtimeTypes.string,
    version: runtimeTypes.string,
  }),
-  runtimeTypes.partial({


timelineVersion is obsolete now.

janmonschke · 2024-04-05T12:54:35Z

x-pack/plugins/security_solution/common/api/timeline/pinned_events/pinned_events_route.ts

@@ -14,10 +14,10 @@ export const pinnedEventIds = unionWithNullType(runtimeTypes.array(runtimeTypes.
 export const persistPinnedEventSchema = runtimeTypes.intersection([
  runtimeTypes.type({
    eventId: runtimeTypes.string,
+    timelineId: runtimeTypes.string,


Same here, timelineId is not optional anymore

janmonschke · 2024-04-05T12:54:55Z

x-pack/plugins/security_solution/common/api/timeline/pinned_events/pinned_events_route.ts

@@ -51,9 +51,6 @@ export const PinnedEventRuntimeType = runtimeTypes.intersection([
    version: runtimeTypes.string,
  }),
  BarePinnedEventType,
-  runtimeTypes.partial({


And here as well, no more need for timelineVersion

janmonschke · 2024-04-05T12:56:28Z

x-pack/plugins/security_solution/public/timelines/components/timeline/body/index.test.tsx

@@ -366,13 +366,6 @@ describe('Body', () => {
          }).type,
        })
      );
-      expect(mockDispatch).toHaveBeenNthCalledWith(


The business logic of automatic pinning of events with a note has been moved to the redux middleware.

janmonschke · 2024-04-05T12:57:21Z

x-pack/plugins/security_solution/public/timelines/store/middlewares/timeline_note.ts

+
+        const currentTimeline = selectTimelineById(store.getState(), localTimelineId);
+
+        // Automatically pin an associated event if it's not pinned yet


The automatic pin logic moved here

janmonschke · 2024-04-05T12:58:17Z

x-pack/plugins/security_solution/server/lib/timeline/saved_object/notes/saved_object.test.ts

@@ -34,7 +34,7 @@ describe('saved_object', () => {
  });

  describe('Set create / update time correctly ', () => {
-    test('Creating a timeline', () => {
+    test('Creating a note', () => {


This was a referring to the wrong object in the entire file.

janmonschke · 2024-04-05T12:59:18Z

x-pack/plugins/security_solution/server/lib/timeline/saved_object/notes/saved_object.ts

-  const shallowCopyOfNote = { ...note };
-  let timelineVersion: string | undefined;
-
-  if (note.timelineId == null) {


This was the piece of code that would create a timeline when no timelineId was provided. Which is not needed anymore.

janmonschke · 2024-04-05T12:59:54Z

x-pack/plugins/security_solution/server/lib/timeline/saved_object/pinned_events/index.ts

@@ -49,8 +48,7 @@ export const deleteAllPinnedEventsOnTimeline = async (
  const savedObjectsClient = (await request.context.core).savedObjects.client;
  const options: SavedObjectsFindOptions = {
    type: pinnedEventSavedObjectType,
-    search: timelineId,
-    searchFields: ['timelineId'],
+    hasReference: { type: timelineSavedObjectType, id: timelineId },


Deleting pinned events was actually not working before. There are tests for this in the new draft timeline ftr tests.

kqualters-elastic

Desk tested this lightly and everything seemed to work as intended, 👍 lgtm

janmonschke · 2024-04-08T13:25:12Z

@elasticmachine merge upstream

kibana-ci · 2024-04-08T15:02:51Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: bab57d3

Failed CI Steps

FTR Configs #93

Test Failures

[job] [logs] FTR Configs #93 / serverless observability UI Serverless Observability Cases Cases persistable attachments lens visualization adds lens visualization to a new case

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	17.0MB	17.0MB	+697.0B

History

💚 Build #201388 succeeded 988ccfc
💔 Build #201329 failed 633b8b6
💔 Build #201310 failed 061bff2
💔 Build #201226 failed 606a747
💚 Build #200939 succeeded 6a2b7a8
💚 Build #200659 succeeded ba63ef2

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

cc @janmonschke

kibanamachine · 2024-04-08T15:07:17Z

💔 All backports failed

Status	Branch	Result
❌	8.13	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 178212

Questions ?

Please refer to the Backport tool documentation

kibanamachine · 2024-04-09T21:48:47Z

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add the label auto-backport or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 178212 locally

Fix behavior of pinnend events and comments on unsaved timelines

c315138

janmonschke added release_note:fix Team:Threat Hunting:Investigations Security Solution Investigations Team backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Mar 7, 2024

janmonschke self-assigned this Mar 7, 2024

janmonschke requested a review from a team as a code owner March 7, 2024 12:53

michaelolo24 added this to the 8.14 milestone Mar 8, 2024

janmonschke mentioned this pull request Mar 12, 2024

[Security Solution] Comments and pins are now not allowed on unsaved timelines #178525

Closed

1 task

janmonschke closed this Mar 12, 2024

janmonschke removed this from the 8.14 milestone Mar 12, 2024

janmonschke added 2 commits April 2, 2024 10:36

Merge branch 'main' into security/fix-note-on-unsaved-timeline-breaking

0cdb195

update tests and auto-pin functionality

a667c43

janmonschke reopened this Apr 2, 2024

janmonschke commented Apr 2, 2024

View reviewed changes

x-pack/plugins/security_solution/public/timelines/store/middlewares/timeline_note.ts Outdated Show resolved Hide resolved

janmonschke added 4 commits April 2, 2024 20:37

update tests

ba63ef2

automatically pin an event when a note is added to it

6a2b7a8

This file is not testing timelines, it's testing notes

0e5ccba

save unsaved timelines as drafts when an associated saved object is c…

975cb92

…reated

kibanamachine and others added 4 commits April 4, 2024 15:30

Merge branch 'main' into security/fix-note-on-unsaved-timeline-breaking

606a747

remove more occurrences of timelineVersion

28cc7f5

timelineId cannot be null anymore

9d5f581

fix ftr tests

061bff2

janmonschke requested a review from a team as a code owner April 5, 2024 06:41

janmonschke added 2 commits April 5, 2024 09:51

fix pinned_events ftr tests

51e2cf6

fix: pinned events were not deleted

633b8b6

janmonschke added 2 commits April 5, 2024 11:31

fix drft timelines test

a649d0c

add draft timeline ftr tests

988ccfc

janmonschke commented Apr 5, 2024

View reviewed changes

janmonschke mentioned this pull request Apr 5, 2024

[Epic][Security Solution][Investigations][Tech Debt] - Update timeline redux #124138

Open

kqualters-elastic approved these changes Apr 5, 2024

View reviewed changes

janmonschke enabled auto-merge (squash) April 8, 2024 13:25

Merge branch 'main' into security/fix-note-on-unsaved-timeline-breaking

bab57d3

janmonschke merged commit 8e9fa4a into elastic:main Apr 8, 2024
37 checks passed

kibanamachine added the v8.14.0 label Apr 8, 2024

kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 9, 2024

janmonschke removed backport missing Added to PRs automatically when the are determined to be missing a backport. backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Apr 11, 2024

kibanamachine added backport:skip This commit does not require backporting labels Apr 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SecuritySolution] Fix behavior of pinnend events and comments on unsaved timelines #178212

[SecuritySolution] Fix behavior of pinnend events and comments on unsaved timelines #178212

janmonschke commented Mar 7, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Mar 7, 2024

PhilippeOberti commented Mar 8, 2024

janmonschke commented Mar 12, 2024

janmonschke commented Apr 2, 2024

janmonschke commented Apr 4, 2024

janmonschke Apr 5, 2024

janmonschke Apr 5, 2024

janmonschke Apr 5, 2024

janmonschke Apr 5, 2024

janmonschke Apr 5, 2024

janmonschke Apr 5, 2024

janmonschke Apr 5, 2024

janmonschke Apr 5, 2024

janmonschke Apr 5, 2024

janmonschke Apr 5, 2024

kqualters-elastic left a comment

janmonschke commented Apr 8, 2024

kibana-ci commented Apr 8, 2024

kibanamachine commented Apr 8, 2024

kibanamachine commented Apr 9, 2024


		const currentTimeline = selectTimelineById(store.getState(), localTimelineId);

		// Automatically pin an associated event if it's not pinned yet

[SecuritySolution] Fix behavior of pinnend events and comments on unsaved timelines #178212

[SecuritySolution] Fix behavior of pinnend events and comments on unsaved timelines #178212

Conversation

janmonschke commented Mar 7, 2024 • edited by kibanamachine Loading

Summary

Checklist

elasticmachine commented Mar 7, 2024

PhilippeOberti commented Mar 8, 2024

janmonschke commented Mar 12, 2024

janmonschke commented Apr 2, 2024

janmonschke commented Apr 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kqualters-elastic left a comment

Choose a reason for hiding this comment

janmonschke commented Apr 8, 2024

kibana-ci commented Apr 8, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

History

kibanamachine commented Apr 8, 2024

💔 All backports failed

Manual backport

Questions ?

kibanamachine commented Apr 9, 2024

janmonschke commented Mar 7, 2024 •

edited by kibanamachine

Loading