[Security Solution][Notes] - fix incorrect get_notes api for documentIds and savedObjectIds query parameters and adding api integration tests #196225

PhilippeOberti · 2024-10-14T23:56:00Z

Summary

While working on adding api integration tests for this PR, I realized that the current logic in the getNotes endpoint is not always correct. Here are the issues:

when retrieving notes for a list of documentIds by passing one or multiple documentIds query parameters (used for example when the UI wants to retrieve all the notes for all the alerts in the alerts table), we were actually searching for the values of the documentIds in all the fields, instead of just in the eventId field. That means that if a user had used the document id value in the note itself (the text part) this value would be returned. The new logic only looks at the eventId attributes to return only the necessary notes. I did not do and performance comparison, but I would assume that looking at a specific field vs all the fields would be much more efficient
the same mistake was done on the savedObjectIds query parameter, where we were searching for the values passed within all the fields of the notes objects, instead of just looking for the references to other saved object ids

This PR fixes the 2 issues above and adds a lot of api integration tests to check the above behavior as well as testing other query parameters (like sortField, sortOrder, perPage, page and search).

Checklist

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

elasticmachine · 2024-10-14T23:56:03Z

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

janmonschke · 2024-10-15T06:47:41Z

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

-            const filter = queryParams?.filter;
-            const options = {
+
+            // searching for all the notes associated with a specific for saved object id


Suggested change

// searching for all the notes associated with a specific for saved object id

// searching for all the notes associated with a specific saved object id

fixed in this commit

janmonschke · 2024-10-15T06:50:49Z

...i_integration/test_suites/investigation/saved_objects/trial_license_complete_tier/helpers.ts

+    .patch(NOTE_URL)
+    .set('kbn-xsrf', 'true')
+    .send({
+      note: {


💅 You could use the actual request body types here and in the cases below, so that it will be easier to refactor these tests once we are changing the request body types.

good point, fixed here

janmonschke · 2024-10-15T06:51:57Z

...api_integration/test_suites/investigation/saved_objects/trial_license_complete_tier/notes.ts

+      // TODO should add more tests for the filter query parameter (I don't know how it's supposed to work)
+
+      // TODO should add more tests for the MAX_UNASSOCIATED_NOTES advanced settings values
+    });


Do you still want to do this before this PR is merged?

no, there are some unknowns on my side, I just added these todos when we revisit the api, probably around 9.0

janmonschke

Nice catch! I left some small comments but feel free to ignore them or tackle them after merging this PR.

…Ids and savedObjectIds query parameters and adding api integration tests

PhilippeOberti · 2024-10-15T15:20:25Z

Nice catch! I left some small comments but feel free to ignore them or tackle them after merging this PR.

thanks! I actually made some changes like wrapping the creation of the notes by Promise.all like you suggested in my other PR. A couple of tests purposefully do not use that though, because I want to ensure the order in which the notes are ingested (to test the pagination).

janmonschke · 2024-10-15T16:34:43Z

...i_integration/test_suites/investigation/saved_objects/trial_license_complete_tier/helpers.ts

-      },
+        updatedBy: note.user || 'elastic',
+        note: note.text,
+      } as BareNote,


This should probably be PersistNoteRouteRequestBody

that makes sense, if that's ok I made the change in this other PR (here) that way it saves some CI time as this PR is nearly done... ?

kibanamachine · 2024-10-15T17:09:41Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11350967812

elasticmachine · 2024-10-15T17:09:52Z

💚 Build Succeeded

Buildkite Build
Commit: fcd19df

Metrics [docs]

✅ unchanged

History

💚 Build #242390 succeeded 6b89eb7a8401a8ebf9f812a9d379433a27231f25

…Ids and savedObjectIds query parameters and adding api integration tests (elastic#196225) (cherry picked from commit 0764261)

kibanamachine · 2024-10-15T17:14:21Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…cumentIds and savedObjectIds query parameters and adding api integration tests (#196225) (#196394) # Backport This will backport the following commits from `main` to `8.x`: - [[Security Solution][Notes] - fix incorrect get_notes api for documentIds and savedObjectIds query parameters and adding api integration tests (#196225)](#196225)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Philippe Oberti <[email protected]>

PhilippeOberti added backport release_note:skip Skip the PR/issue when compiling release notes v9.0.0 Team:Threat Hunting:Investigations Security Solution Investigations Team v8.16.0 labels Oct 14, 2024

PhilippeOberti requested review from a team as code owners October 14, 2024 23:56

PhilippeOberti mentioned this pull request Oct 15, 2024

[Security Solution][Notes] - allow filtering by note association #195501

Merged

2 tasks

janmonschke reviewed Oct 15, 2024

View reviewed changes

janmonschke approved these changes Oct 15, 2024

View reviewed changes

PhilippeOberti added 2 commits October 15, 2024 09:25

[Security Solution][Notes] - fix incorrect get_notes api for document…

0ff0bc1

…Ids and savedObjectIds query parameters and adding api integration tests

PR comments

fcd19df

PhilippeOberti force-pushed the notes-api-tests branch from 6b89eb7 to fcd19df Compare October 15, 2024 15:17

janmonschke reviewed Oct 15, 2024

View reviewed changes

PhilippeOberti merged commit 0764261 into elastic:main Oct 15, 2024
42 checks passed

PhilippeOberti deleted the notes-api-tests branch October 15, 2024 17:09

kibanamachine mentioned this pull request Oct 15, 2024

[8.x] [Security Solution][Notes] - fix incorrect get_notes api for documentIds and savedObjectIds query parameters and adding api integration tests (#196225) #196394

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Notes] - fix incorrect get_notes api for documentIds and savedObjectIds query parameters and adding api integration tests #196225

[Security Solution][Notes] - fix incorrect get_notes api for documentIds and savedObjectIds query parameters and adding api integration tests #196225

PhilippeOberti commented Oct 14, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Oct 14, 2024

janmonschke Oct 15, 2024

PhilippeOberti Oct 15, 2024

janmonschke Oct 15, 2024

PhilippeOberti Oct 15, 2024

janmonschke Oct 15, 2024

PhilippeOberti Oct 15, 2024

janmonschke left a comment

PhilippeOberti commented Oct 15, 2024

janmonschke Oct 15, 2024

PhilippeOberti Oct 15, 2024 •

edited

Loading

kibanamachine commented Oct 15, 2024

elasticmachine commented Oct 15, 2024

kibanamachine commented Oct 15, 2024

	// searching for all the notes associated with a specific for saved object id
	// searching for all the notes associated with a specific saved object id

[Security Solution][Notes] - fix incorrect get_notes api for documentIds and savedObjectIds query parameters and adding api integration tests #196225

[Security Solution][Notes] - fix incorrect get_notes api for documentIds and savedObjectIds query parameters and adding api integration tests #196225

Conversation

PhilippeOberti commented Oct 14, 2024 • edited by kibanamachine Loading

Summary

Checklist

elasticmachine commented Oct 14, 2024

janmonschke Oct 15, 2024

Choose a reason for hiding this comment

PhilippeOberti Oct 15, 2024

Choose a reason for hiding this comment

janmonschke Oct 15, 2024

Choose a reason for hiding this comment

PhilippeOberti Oct 15, 2024

Choose a reason for hiding this comment

janmonschke Oct 15, 2024

Choose a reason for hiding this comment

PhilippeOberti Oct 15, 2024

Choose a reason for hiding this comment

janmonschke left a comment

Choose a reason for hiding this comment

PhilippeOberti commented Oct 15, 2024

janmonschke Oct 15, 2024

Choose a reason for hiding this comment

PhilippeOberti Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

kibanamachine commented Oct 15, 2024

elasticmachine commented Oct 15, 2024

💚 Build Succeeded

Metrics [docs]

History

kibanamachine commented Oct 15, 2024

💚 All backports created successfully

Questions ?

PhilippeOberti commented Oct 14, 2024 •

edited by kibanamachine

Loading

PhilippeOberti Oct 15, 2024 •

edited

Loading