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 Assistant] Fix Knowledge Base API #211367

Merged
merged 55 commits into from
Feb 25, 2025

Conversation

patrykkopycinski
Copy link
Contributor

@patrykkopycinski patrykkopycinski commented Feb 16, 2025

Summary

Fixes bugs related to Security Assistant Knowledge Base API

@patrykkopycinski patrykkopycinski marked this pull request as ready for review February 17, 2025 10:54
@patrykkopycinski patrykkopycinski requested a review from a team as a code owner February 17, 2025 10:54
@@ -56,7 +56,26 @@ export class RequestContextFactory implements IRequestContextFactory {
const getSpaceId = (): string =>
startPlugins.spaces?.spacesService?.getSpaceId(request) || DEFAULT_NAMESPACE_STRING;

const getCurrentUser = () => coreContext.security.authc.getCurrentUser();
const getCurrentUser = async () => {
Copy link
Member

Choose a reason for hiding this comment

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

🚀

@e40pud
Copy link
Contributor

e40pud commented Feb 17, 2025

Changes related to user profile ID looks great and should work now!

I tested some other routes and noticed next issues:

1. KB entry by ID APIs:

I see that we report a few routes in our openapi files a few routes that are not available

Screenshot 2025-02-17 at 13 54 15
  • GET route does not exist at all in our code
  • DELETE route exists, but not registered: x-pack/solutions/security/plugins/elastic_assistant/server/routes/knowledge_base/entries/delete_route.ts
  • UPDATE route exists, but not registered: x-pack/solutions/security/plugins/elastic_assistant/server/routes/knowledge_base/entries/update_route.ts

2. Bulk actions - CREATE

I bumped into this error which happens once user created private entry and then tries to create a new one via bulk actions:

Screenshot 2025-02-17 at 14 06 21

After some investigation, I've noticed that we have an issue within our code:

const result = await kbDataClient?.findDocuments<EsKnowledgeBaseEntrySchema>({
              perPage: 100,
              page: 1,
              filter: `users:{ id: "${authenticatedUser?.profile_uid}" }`,
              fields: [],
            });
            if (result?.data != null && result.total > 0) {
              return assistantResponse.error({
                statusCode: 409,
                body: `Knowledge Base Entry id's: "${transformESSearchToKnowledgeBaseEntry(
                  result.data
                )
                  .map((c) => c.id)
                  .join(',')}" already exists`,
              });
            }

Here we check whether user has document entries and if there are some existing entries we throw an 409 error in this case and do no allow user to create new entries via bulk CREATE action.

3. Bulk actions - UPDATE

I still see the issue where the document entry disappears after the bulk update. It happens when during the update I set "kbResource": "string", "source": "string". Even though those values are probably invalid and should not be passed, I think we should return bad request response or correct those values before the update. Also, as an option we could remove those attributes from the list of allowed properties to be updated.

Screen.Recording.2025-02-17.at.14.25.05.-.1080.mov

@e40pud
Copy link
Contributor

e40pud commented Feb 19, 2025

@patrykkopycinski I checked APIs again and most routes work as expected!

Just two notes related to the "update entry by id" route:

  1. We require to pass entry id in the body even though it is part of the URL path parameters. I think it should not be mandatory within the body in this route.
Screenshot 2025-02-19 at 12 02 28
  1. Right now it is possible to update entry from global to private via direct API call (I guess this also applies to update via bulk actions). In this case the global entry will disappear and only private entry will exist. When we do the same via UI, we keep global entry and create a copy as a private entry. We show the modal saying Changing a knowledge base entry from global to private will create a private copy of the original global entry. Please delete the global entry if you would like to revoke the content for other users.. I was wondering if we should handle this case within API as well and create a copy entry or just allow users to update entry from global to private. I lean towards keeping current API behaviour, but just wanted to pointed it out and make sure it is expected. cc @jamesspi

@patrykkopycinski patrykkopycinski removed request for a team, rylnd and jkelas February 25, 2025 17:41
@patrykkopycinski
Copy link
Contributor Author

Sorry all for the ping, Github resolve conflicts 🤦

@patrykkopycinski patrykkopycinski changed the title Fix KB API [Security Assistant] Fix Knowledge Base API Feb 25, 2025
@patrykkopycinski patrykkopycinski enabled auto-merge (squash) February 25, 2025 17:42
@patrykkopycinski patrykkopycinski merged commit c822109 into elastic:main Feb 25, 2025
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.18, 9.0

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

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/elastic-assistant-common 429 434 +5

Async chunks

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

id before after diff
securitySolution 8.9MB 8.9MB +181.0B
Unknown metric groups

API count

id before after diff
@kbn/elastic-assistant-common 500 506 +6

ESLint disabled line counts

id before after diff
@kbn/test-suites-xpack 720 721 +1

Total ESLint disabled count

id before after diff
@kbn/test-suites-xpack 746 747 +1

History

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 25, 2025
## Summary

Fixes bugs related to Security Assistant Knowledge Base API

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Hannah Mudge <[email protected]>
Co-authored-by: Marta Bondyra <[email protected]>
Co-authored-by: Davis Plumlee <[email protected]>
Co-authored-by: Jatin Kathuria <[email protected]>
Co-authored-by: Chris Cowan <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Arturo Lidueña <[email protected]>
Co-authored-by: Jon <[email protected]>
Co-authored-by: Rodney Norris <[email protected]>
Co-authored-by: Elena Shostak <[email protected]>
Co-authored-by: Stratoula Kalafateli <[email protected]>
Co-authored-by: Irene Blanco <[email protected]>
Co-authored-by: Cauê Marcondes <[email protected]>
Co-authored-by: Carlos Crespo <[email protected]>
(cherry picked from commit c822109)
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 25, 2025
## Summary

Fixes bugs related to Security Assistant Knowledge Base API

---------

Co-authored-by: kibanamachine <[email protected]>
Co-authored-by: Hannah Mudge <[email protected]>
Co-authored-by: Marta Bondyra <[email protected]>
Co-authored-by: Davis Plumlee <[email protected]>
Co-authored-by: Jatin Kathuria <[email protected]>
Co-authored-by: Chris Cowan <[email protected]>
Co-authored-by: Elastic Machine <[email protected]>
Co-authored-by: Arturo Lidueña <[email protected]>
Co-authored-by: Jon <[email protected]>
Co-authored-by: Rodney Norris <[email protected]>
Co-authored-by: Elena Shostak <[email protected]>
Co-authored-by: Stratoula Kalafateli <[email protected]>
Co-authored-by: Irene Blanco <[email protected]>
Co-authored-by: Cauê Marcondes <[email protected]>
Co-authored-by: Carlos Crespo <[email protected]>
(cherry picked from commit c822109)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.18
9.0

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

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Feb 26, 2025
# Backport

This will backport the following commits from `main` to `9.0`:
- [[Security Assistant] Fix Knowledge Base API
(#211367)](#211367)

<!--- Backport version: 9.6.6 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT [{"author":{"name":"Patryk
Kopyciński","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-02-25T23:00:00Z","message":"[Security
Assistant] Fix Knowledge Base API (#211367)\n\n## Summary\n\nFixes bugs
related to Security Assistant Knowledge Base
API\n\n---------\n\nCo-authored-by: kibanamachine
<[email protected]>\nCo-authored-by:
Hannah Mudge <[email protected]>\nCo-authored-by: Marta
Bondyra <[email protected]>\nCo-authored-by:
Davis Plumlee
<[email protected]>\nCo-authored-by: Jatin
Kathuria <[email protected]>\nCo-authored-by: Chris Cowan
<[email protected]>\nCo-authored-by: Elastic Machine
<[email protected]>\nCo-authored-by: Arturo
Lidueña <[email protected]>\nCo-authored-by: Jon
<[email protected]>\nCo-authored-by: Rodney Norris
<[email protected]>\nCo-authored-by: Elena Shostak
<[email protected]>\nCo-authored-by:
Stratoula Kalafateli <[email protected]>\nCo-authored-by:
Irene Blanco <[email protected]>\nCo-authored-by: Cauê Marcondes
<[email protected]>\nCo-authored-by:
Carlos Crespo
<[email protected]>","sha":"c822109a492fe4dcf38ca5aa6d87b2a95bf075c4","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","v9.0.0","Feature:Security
Assistant","Team:Security Generative
AI","backport:version","v8.18.0","v9.1.0"],"title":"[Security Assistant]
Fix Knowledge Base
API","number":211367,"url":"https://github.com/elastic/kibana/pull/211367","mergeCommit":{"message":"[Security
Assistant] Fix Knowledge Base API (#211367)\n\n## Summary\n\nFixes bugs
related to Security Assistant Knowledge Base
API\n\n---------\n\nCo-authored-by: kibanamachine
<[email protected]>\nCo-authored-by:
Hannah Mudge <[email protected]>\nCo-authored-by: Marta
Bondyra <[email protected]>\nCo-authored-by:
Davis Plumlee
<[email protected]>\nCo-authored-by: Jatin
Kathuria <[email protected]>\nCo-authored-by: Chris Cowan
<[email protected]>\nCo-authored-by: Elastic Machine
<[email protected]>\nCo-authored-by: Arturo
Lidueña <[email protected]>\nCo-authored-by: Jon
<[email protected]>\nCo-authored-by: Rodney Norris
<[email protected]>\nCo-authored-by: Elena Shostak
<[email protected]>\nCo-authored-by:
Stratoula Kalafateli <[email protected]>\nCo-authored-by:
Irene Blanco <[email protected]>\nCo-authored-by: Cauê Marcondes
<[email protected]>\nCo-authored-by:
Carlos Crespo
<[email protected]>","sha":"c822109a492fe4dcf38ca5aa6d87b2a95bf075c4"}},"sourceBranch":"main","suggestedTargetBranches":["9.0","8.18"],"targetPullRequestStates":[{"branch":"9.0","label":"v9.0.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.18","label":"v8.18.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/211367","number":211367,"mergeCommit":{"message":"[Security
Assistant] Fix Knowledge Base API (#211367)\n\n## Summary\n\nFixes bugs
related to Security Assistant Knowledge Base
API\n\n---------\n\nCo-authored-by: kibanamachine
<[email protected]>\nCo-authored-by:
Hannah Mudge <[email protected]>\nCo-authored-by: Marta
Bondyra <[email protected]>\nCo-authored-by:
Davis Plumlee
<[email protected]>\nCo-authored-by: Jatin
Kathuria <[email protected]>\nCo-authored-by: Chris Cowan
<[email protected]>\nCo-authored-by: Elastic Machine
<[email protected]>\nCo-authored-by: Arturo
Lidueña <[email protected]>\nCo-authored-by: Jon
<[email protected]>\nCo-authored-by: Rodney Norris
<[email protected]>\nCo-authored-by: Elena Shostak
<[email protected]>\nCo-authored-by:
Stratoula Kalafateli <[email protected]>\nCo-authored-by:
Irene Blanco <[email protected]>\nCo-authored-by: Cauê Marcondes
<[email protected]>\nCo-authored-by:
Carlos Crespo
<[email protected]>","sha":"c822109a492fe4dcf38ca5aa6d87b2a95bf075c4"}}]}]
BACKPORT-->

Co-authored-by: Patryk Kopyciński <[email protected]>
kibanamachine added a commit that referenced this pull request Feb 26, 2025
# Backport

This will backport the following commits from `main` to `8.18`:
- [[Security Assistant] Fix Knowledge Base API
(#211367)](#211367)

<!--- Backport version: 9.6.6 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT [{"author":{"name":"Patryk
Kopyciński","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-02-25T23:00:00Z","message":"[Security
Assistant] Fix Knowledge Base API (#211367)\n\n## Summary\n\nFixes bugs
related to Security Assistant Knowledge Base
API\n\n---------\n\nCo-authored-by: kibanamachine
<[email protected]>\nCo-authored-by:
Hannah Mudge <[email protected]>\nCo-authored-by: Marta
Bondyra <[email protected]>\nCo-authored-by:
Davis Plumlee
<[email protected]>\nCo-authored-by: Jatin
Kathuria <[email protected]>\nCo-authored-by: Chris Cowan
<[email protected]>\nCo-authored-by: Elastic Machine
<[email protected]>\nCo-authored-by: Arturo
Lidueña <[email protected]>\nCo-authored-by: Jon
<[email protected]>\nCo-authored-by: Rodney Norris
<[email protected]>\nCo-authored-by: Elena Shostak
<[email protected]>\nCo-authored-by:
Stratoula Kalafateli <[email protected]>\nCo-authored-by:
Irene Blanco <[email protected]>\nCo-authored-by: Cauê Marcondes
<[email protected]>\nCo-authored-by:
Carlos Crespo
<[email protected]>","sha":"c822109a492fe4dcf38ca5aa6d87b2a95bf075c4","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","v9.0.0","Feature:Security
Assistant","Team:Security Generative
AI","backport:version","v8.18.0","v9.1.0"],"title":"[Security Assistant]
Fix Knowledge Base
API","number":211367,"url":"https://github.com/elastic/kibana/pull/211367","mergeCommit":{"message":"[Security
Assistant] Fix Knowledge Base API (#211367)\n\n## Summary\n\nFixes bugs
related to Security Assistant Knowledge Base
API\n\n---------\n\nCo-authored-by: kibanamachine
<[email protected]>\nCo-authored-by:
Hannah Mudge <[email protected]>\nCo-authored-by: Marta
Bondyra <[email protected]>\nCo-authored-by:
Davis Plumlee
<[email protected]>\nCo-authored-by: Jatin
Kathuria <[email protected]>\nCo-authored-by: Chris Cowan
<[email protected]>\nCo-authored-by: Elastic Machine
<[email protected]>\nCo-authored-by: Arturo
Lidueña <[email protected]>\nCo-authored-by: Jon
<[email protected]>\nCo-authored-by: Rodney Norris
<[email protected]>\nCo-authored-by: Elena Shostak
<[email protected]>\nCo-authored-by:
Stratoula Kalafateli <[email protected]>\nCo-authored-by:
Irene Blanco <[email protected]>\nCo-authored-by: Cauê Marcondes
<[email protected]>\nCo-authored-by:
Carlos Crespo
<[email protected]>","sha":"c822109a492fe4dcf38ca5aa6d87b2a95bf075c4"}},"sourceBranch":"main","suggestedTargetBranches":["9.0","8.18"],"targetPullRequestStates":[{"branch":"9.0","label":"v9.0.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"8.18","label":"v8.18.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/211367","number":211367,"mergeCommit":{"message":"[Security
Assistant] Fix Knowledge Base API (#211367)\n\n## Summary\n\nFixes bugs
related to Security Assistant Knowledge Base
API\n\n---------\n\nCo-authored-by: kibanamachine
<[email protected]>\nCo-authored-by:
Hannah Mudge <[email protected]>\nCo-authored-by: Marta
Bondyra <[email protected]>\nCo-authored-by:
Davis Plumlee
<[email protected]>\nCo-authored-by: Jatin
Kathuria <[email protected]>\nCo-authored-by: Chris Cowan
<[email protected]>\nCo-authored-by: Elastic Machine
<[email protected]>\nCo-authored-by: Arturo
Lidueña <[email protected]>\nCo-authored-by: Jon
<[email protected]>\nCo-authored-by: Rodney Norris
<[email protected]>\nCo-authored-by: Elena Shostak
<[email protected]>\nCo-authored-by:
Stratoula Kalafateli <[email protected]>\nCo-authored-by:
Irene Blanco <[email protected]>\nCo-authored-by: Cauê Marcondes
<[email protected]>\nCo-authored-by:
Carlos Crespo
<[email protected]>","sha":"c822109a492fe4dcf38ca5aa6d87b2a95bf075c4"}}]}]
BACKPORT-->

---------

Co-authored-by: Patryk Kopyciński <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels bug Fixes for quality problems that affect the customer experience Feature:Security Assistant Security Assistant release_note:skip Skip the PR/issue when compiling release notes Team:Security Generative AI Security Generative AI v8.18.0 v9.0.0 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.