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

janmonschke · 2024-03-12T15:27:05Z

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.

We're making the case, that creating comments and pinned events does not make on unsaved timelines does not make sense in the first place as it either leads to loss of data or it leads to sub-optimal solutions that involve saving timelines in the background (see #178212, especially this comment #178212 (comment)).

In this PR, we're disabling commenting and pinning for unsaved timelines.

Due to the nature of this change, a couple pieces of code have been identified to be not in use anymore and have also been deleted as part of this work (82bf54d & removal of timelineVersion on notes and pins).

This PR is best reviewed commit by commit.

Checklist

Delete any items that are not applicable to this PR.

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

…Select` prop

Requests without `timelineId` will now also fail on the server.

…red parameter

elasticmachine · 2024-03-12T15:27:09Z

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

andrew-goldstein

Consider, as an alternative to the disabled notes and pin buttons in the screenshot below:

re-enabling those buttons, and showing / focusing the save dialog for unsaved timelines:

andrew-goldstein · 2024-03-12T21:12:05Z

Consider, as an alternative to the disabled notes and pin buttons in the screenshot below:

re-enabling those buttons, and showing / focusing the save dialog for unsaved timelines:

@janmonschke please consider the following proposal, which adds detail to the above:

The Add note or Pin alert actions are always enabled. This preserves the current functionality, which is a call to action.

When (for example) a user clicks Add note for an Untitled timeline, the Save dialog appears, per the screenshot below:

In the screenshot above, an EuiCallOut reads:

Save timeline to continue adding a note

When the user enters a title and clicks Save, the Add note is expanded and focused, per the screenshot below:

…saved-timeline

janmonschke · 2024-03-22T07:23:49Z

@elasticmachine merge upstream

…saved-timeline

logeekal

Still doing Desk Testing. This is code review and most of the code looks good. 🚀 Thanks for additionaly cleanup as well.

But before I approve, I had some comments/questions for your review.

logeekal · 2024-03-22T06:00:54Z

...lugins/security_solution/public/common/components/header_actions/add_note_icon_item.test.tsx

+        </TestProviders>
+      );
+
+      expect(screen.getByTestId('timeline-notes-button-small')).toBeDisabled();


nit : I think it will make sense to test if tooltip appears as expected as well.

logeekal · 2024-03-22T06:04:35Z

.../plugins/security_solution/public/common/components/header_actions/pin_event_action.test.tsx

+        </TestProviders>
+      );
+
+      expect(screen.getByTestId('pin')).toBeDisabled();


same comment as above here about testing the tooltip.

logeekal · 2024-03-22T06:12:30Z

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

@@ -492,7 +496,7 @@ const TabsContentComponent: React.FC<BasicTimelineTab> = ({
            <StyledEuiTab
              data-test-subj={`timelineTabs-${TimelineTabs.securityAssistant}`}
              onClick={setSecurityAssistantAsActiveTab}
-              disabled={timelineType === TimelineType.template}
+              disabled={isTimelineTemplate}


thanks for cleaning this up

logeekal · 2024-03-22T06:31:03Z

.../plugins/security_solution/common/api/timeline/pinned_events/pinned_events_route_schema.yaml

@@ -1,7 +1,7 @@
 openapi: 3.0.0
 info:
  title: Elastic Security - Timeline - Pinned Event API (https://www.elastic.co/guide/en/security/current/_pin_an_event_to_an_existing_timeline.html)
-  version: 8.9.0
+  version: 8.14.0


I have a question unrelated to this PR.

Additionally, I see that you have updated version from 8.9.0 to 8.14.0 now. I wonder if users on our swagger open API spec page see the version as 8.9 even though they are 8.12 ? Do you know?

logeekal · 2024-03-22T07:14:03Z

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

@@ -18,24 +18,24 @@ export default function ({ getService }: FtrProviderContext) {
    after(() => kibanaServer.savedObjects.cleanStandardList());

    describe('create a note', () => {
-      it('should return a timelineId, timelineVersion, noteId and version', async () => {
+      it('should return a timelineId, noteId and version', async () => {


Could you also add a test to create with timelineID as null and make sure it returns appropriate message and status code?

logeekal · 2024-03-22T07:14:44Z

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

+          .send({
+            pinnedEventId: null,
+            eventId: 'bv4QSGsB9v5HJNSH-7fi',
+            timelineId: 'testTimelineId',


Same here. Could you also add a test with timelineId as null?

logeekal · 2024-03-22T07:25:45Z

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

+      noteId,
+      note,
+      overrideOwner,
+    });


One question regarding this. Why was exception clause removed. I see that neither parent not child of createNote is setting the appropriate status code. I can see 403 return code is wrong here but did you want to return default 500 status code when this kind of error occurs?

logeekal · 2024-03-22T07:48:20Z

...ecurity_solution/public/timelines/components/timeline/body/events/event_column_view.test.tsx

+    (useShallowEqualSelector as jest.Mock).mockReturnValue({
+      ...testTimeline,
+      timelineType: TimelineType.template,
+    });

    const wrapper = mount(<EventColumnView {...props} />, { wrappingComponent: TestProviders });

    expect(wrapper.find('[data-test-subj="add-note"]').prop('toolTip')).toEqual(


🧪 Should we check here that the button is disabled as well?

logeekal · 2024-03-22T07:49:41Z

...ecurity_solution/public/timelines/components/timeline/body/events/event_column_view.test.tsx

+    (useShallowEqualSelector as jest.Mock).mockReturnValue({
+      ...testTimeline,
+      timelineType: TimelineType.template,
+    });

    const wrapper = mount(<EventColumnView {...props} />, { wrappingComponent: TestProviders });


Additionally, I know it is not in the scope exactly, but you can see if this file is quick to change from enzyme to RTL. Because these incremental changes are the only to get these done.

logeekal

Desk Testing Completed. Almost all of it seems to be working fine. But there is one edge case that we should be cognizant about.

Let's say you have a Investigation guide in the rule. When you create timeline from the alert created from that rule, it automatically adds investigation guide as a note. So if user does not save that timeline, that note will be a ghost/orphan note in the backend.

My first thought was that note creation should fail, since there is not timeline Id but as you will see in the demo video, we are sending timelineId: timeline-1 so network request succeeds.

As much as it might be necessary to add Investigation Guide as the note, we should be able to clean that as well if an unsavedtimeline is discarded.

Alternatively, did we explore simply keeping the note/pinned Events in redux only till the timeline is saved?

Below is the video that demonstrate it.

note_creation_unsaved_timeline.mp4

kibana-ci · 2024-03-22T09:19:38Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: ec465f9

Failed CI Steps

Defend Workflows Cypress Tests on Serverless #1

Test Failures

[job] [logs] Defend Workflows Cypress Tests on Serverless #1 / User Roles for Security Complete PLI with Endpoint Complete addon for role: t3_analyst Response Actions access "before each" hook for "should have access to execute action: release" "before each" hook for "should have access to execute action: release"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	5036	5033	-3

Async chunks

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

id	before	after	diff
`securitySolution`	15.5MB	15.5MB	-13.3KB

History

💚 Build #197958 succeeded c4818ee
💛 Build #197295 was flaky 21f5708
💔 Build #197265 failed 6d90c80
💔 Build #197253 failed 0e221e8
💔 Build #197200 failed cf2fc96

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

cc @janmonschke

janmonschke · 2024-04-09T08:11:11Z

Closed in favor of an adjusted #178212

janmonschke added 6 commits March 12, 2024 11:50

disable commenting and pinning when the timeline isn't saved

f90040e

remove unused EventsSelect component and the associated `showEvents…

82bf54d

…Select` prop

don't create a new timeline when pinning an event on an unsaved timeline

a3c62df

Requests without `timelineId` will now also fail on the server.

Remove the now unused timelineVersion property

3379204

use request validation instead of manual validation

99d4722

remove timelineVersion from note endpoint and make timelineId a requi…

a0d41bb

…red parameter

janmonschke added release_note:enhancement 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 12, 2024

janmonschke requested review from a team as code owners March 12, 2024 15:27

janmonschke self-assigned this Mar 12, 2024

janmonschke mentioned this pull request Mar 12, 2024

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

Merged

1 task

janmonschke added 2 commits March 12, 2024 16:44

update router versions to reflect changes

2000e45

make sure body messages are surfaced

cf2fc96

janmonschke added release_note:fix and removed release_note:enhancement labels Mar 12, 2024

janmonschke added 3 commits March 12, 2024 19:15

fix some broken tests

d2f5b99

revert router versions

0e221e8

fix some type errors

6d90c80

andrew-goldstein reviewed Mar 12, 2024

View reviewed changes

add version headers were needed

21f5708

janmonschke added this to the 8.14 milestone Mar 18, 2024

Merge branch 'main' into security/dont-allow-pinning-commenting-on-un…

c4818ee

…saved-timeline

Merge branch 'main' into security/dont-allow-pinning-commenting-on-un…

ec465f9

…saved-timeline

logeekal reviewed Mar 22, 2024

View reviewed changes

logeekal mentioned this pull request Apr 3, 2024

[Threat Hunting][ADR] 0001 - Saving of timeline-associated saved objects #179830

Merged

janmonschke closed this Apr 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

janmonschke commented Mar 12, 2024 •

edited

Loading

elasticmachine commented Mar 12, 2024

andrew-goldstein left a comment

andrew-goldstein commented Mar 12, 2024

janmonschke commented Mar 22, 2024

logeekal left a comment

logeekal Mar 22, 2024

logeekal Mar 22, 2024

logeekal Mar 22, 2024

logeekal Mar 22, 2024

logeekal Mar 22, 2024

logeekal Mar 22, 2024

logeekal Mar 22, 2024

logeekal Mar 22, 2024

logeekal Mar 22, 2024

logeekal left a comment

kibana-ci commented Mar 22, 2024

janmonschke commented Apr 9, 2024

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

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

Conversation

janmonschke commented Mar 12, 2024 • edited Loading

Summary

Checklist

elasticmachine commented Mar 12, 2024

andrew-goldstein left a comment

Choose a reason for hiding this comment

andrew-goldstein commented Mar 12, 2024

janmonschke commented Mar 22, 2024

logeekal left a 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

Choose a reason for hiding this comment

logeekal left a comment

Choose a reason for hiding this comment

kibana-ci commented Mar 22, 2024

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Module Count

Async chunks

History

janmonschke commented Apr 9, 2024

janmonschke commented Mar 12, 2024 •

edited

Loading