Skip to content

Commit

Permalink
[8.x] [Security GenAI] When a "global" Knowledge Base entry…
Browse files Browse the repository at this point in the history
… is updated to "private", a duplicate "private" entry gets created and the global entry remains unchanged (#197157) (#197516) (#197919)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security GenAI] When a "global" Knowledge Base entry is
updated to "private", a duplicate "private" entry
gets created and the global entry remains unchanged (#197157)
(#197516)](#197516)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Ievgen
Sorokopud","email":"ievgen.sorokopud@elastic.co"},"sourceCommit":{"committedDate":"2024-10-25T22:45:32Z","message":"[Security
GenAI] When a \"global\" Knowledge Base entry is updated to \"private\",
a duplicate \"private\" entry gets created and the global entry remains
unchanged (#197157) (#197516)\n\n## Summary\r\n\r\nOriginal ticket
describing the
BUG:\r\nhttps://github.com//issues/197157\r\n\r\nThese
changes fix two issues:\r\n1. Updating an entry from Global to Private
duplicates it. After\r\ndiscussing with the team we decided that this is
an expected behaviour\r\nand we would add a modal dialog which warns
users about it. See more\r\ndetails
here\r\nhttps://github.com//issues/197157#issuecomment-2432592394\r\n2.
Editing Private entry and switching the sharing option twice
from\r\nPrivate => Global => Private causes the issue where we would
treat\r\nselected entry as a new one and thus calling \"create entry\"
instead of\r\n\"update\".\r\n\r\n### Steps to reproduce second
issue:\r\n\r\n* Edit private entry\r\n* Update entry's name\r\n* Switch
sharing option to Global\r\n* Switch sharing option back to Private\r\n*
Save the entry\r\n\r\n**Current behaviour**: a new private entry is
created\r\n**Expected behaviour**: existing private entry is
updated\r\n\r\n### Screen recording of the fixed first
issue\r\n\r\n\r\nhttps://github.com/user-attachments/assets/e11e14bd-c557-401e-a23f-e01ac7aedf30\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [ ] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"d17fc09034d69f15748ddb0a49eae78959401c5a","branchLabelMapping":{"^v9.0.0$":"main","^v8.17.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:
SecuritySolution","backport:prev-minor","Team:Security Generative
AI","v8.16.0"],"title":"[Security GenAI] When a \"global\" Knowledge
Base entry is updated to \"private\", a duplicate \"private\" entry gets
created and the global entry remains unchanged
(#197157)","number":197516,"url":"https://github.com/elastic/kibana/pull/197516","mergeCommit":{"message":"[Security
GenAI] When a \"global\" Knowledge Base entry is updated to \"private\",
a duplicate \"private\" entry gets created and the global entry remains
unchanged (#197157) (#197516)\n\n## Summary\r\n\r\nOriginal ticket
describing the
BUG:\r\nhttps://github.com//issues/197157\r\n\r\nThese
changes fix two issues:\r\n1. Updating an entry from Global to Private
duplicates it. After\r\ndiscussing with the team we decided that this is
an expected behaviour\r\nand we would add a modal dialog which warns
users about it. See more\r\ndetails
here\r\nhttps://github.com//issues/197157#issuecomment-2432592394\r\n2.
Editing Private entry and switching the sharing option twice
from\r\nPrivate => Global => Private causes the issue where we would
treat\r\nselected entry as a new one and thus calling \"create entry\"
instead of\r\n\"update\".\r\n\r\n### Steps to reproduce second
issue:\r\n\r\n* Edit private entry\r\n* Update entry's name\r\n* Switch
sharing option to Global\r\n* Switch sharing option back to Private\r\n*
Save the entry\r\n\r\n**Current behaviour**: a new private entry is
created\r\n**Expected behaviour**: existing private entry is
updated\r\n\r\n### Screen recording of the fixed first
issue\r\n\r\n\r\nhttps://github.com/user-attachments/assets/e11e14bd-c557-401e-a23f-e01ac7aedf30\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [ ] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"d17fc09034d69f15748ddb0a49eae78959401c5a"}},"sourceBranch":"main","suggestedTargetBranches":["8.16"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/197516","number":197516,"mergeCommit":{"message":"[Security
GenAI] When a \"global\" Knowledge Base entry is updated to \"private\",
a duplicate \"private\" entry gets created and the global entry remains
unchanged (#197157) (#197516)\n\n## Summary\r\n\r\nOriginal ticket
describing the
BUG:\r\nhttps://github.com//issues/197157\r\n\r\nThese
changes fix two issues:\r\n1. Updating an entry from Global to Private
duplicates it. After\r\ndiscussing with the team we decided that this is
an expected behaviour\r\nand we would add a modal dialog which warns
users about it. See more\r\ndetails
here\r\nhttps://github.com//issues/197157#issuecomment-2432592394\r\n2.
Editing Private entry and switching the sharing option twice
from\r\nPrivate => Global => Private causes the issue where we would
treat\r\nselected entry as a new one and thus calling \"create entry\"
instead of\r\n\"update\".\r\n\r\n### Steps to reproduce second
issue:\r\n\r\n* Edit private entry\r\n* Update entry's name\r\n* Switch
sharing option to Global\r\n* Switch sharing option back to Private\r\n*
Save the entry\r\n\r\n**Current behaviour**: a new private entry is
created\r\n**Expected behaviour**: existing private entry is
updated\r\n\r\n### Screen recording of the fixed first
issue\r\n\r\n\r\nhttps://github.com/user-attachments/assets/e11e14bd-c557-401e-a23f-e01ac7aedf30\r\n\r\n###
Checklist\r\n\r\nDelete any items that are not applicable to this
PR.\r\n\r\n- [ ] [Unit or
functional\r\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\r\nwere
updated or added to match the most common
scenarios","sha":"d17fc09034d69f15748ddb0a49eae78959401c5a"}},{"branch":"8.16","label":"v8.16.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Ievgen Sorokopud <ievgen.sorokopud@elastic.co>
  • Loading branch information
kibanamachine and e40pud authored Oct 26, 2024
1 parent fc831f1 commit f11c125
Show file tree
Hide file tree
Showing 5 changed files with 274 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,18 +14,28 @@ import {
EuiIcon,
EuiText,
} from '@elastic/eui';
import React, { useCallback } from 'react';
import React, { useCallback, useMemo } from 'react';
import { DocumentEntry } from '@kbn/elastic-assistant-common';
import * as i18n from './translations';
import { isGlobalEntry } from './helpers';

interface Props {
entry?: DocumentEntry;
originalEntry?: DocumentEntry;
setEntry: React.Dispatch<React.SetStateAction<Partial<DocumentEntry>>>;
hasManageGlobalKnowledgeBase: boolean;
}

export const DocumentEntryEditor: React.FC<Props> = React.memo(
({ entry, setEntry, hasManageGlobalKnowledgeBase }) => {
({ entry, setEntry, hasManageGlobalKnowledgeBase, originalEntry }) => {
const privateUsers = useMemo(() => {
const originalUsers = originalEntry?.users;
if (originalEntry && !isGlobalEntry(originalEntry)) {
return originalUsers;
}
return undefined;
}, [originalEntry]);

// Name
const setName = useCallback(
(e: React.ChangeEvent<HTMLInputElement>) =>
Expand All @@ -38,12 +48,13 @@ export const DocumentEntryEditor: React.FC<Props> = React.memo(
(value: string) =>
setEntry((prevEntry) => ({
...prevEntry,
users: value === i18n.SHARING_GLOBAL_OPTION_LABEL ? [] : undefined,
users: value === i18n.SHARING_GLOBAL_OPTION_LABEL ? [] : privateUsers,
})),
[setEntry]
[privateUsers, setEntry]
);
const sharingOptions = [
{
'data-test-subj': 'sharing-private-option',
value: i18n.SHARING_PRIVATE_OPTION_LABEL,
inputDisplay: (
<EuiText size={'s'}>
Expand All @@ -57,6 +68,7 @@ export const DocumentEntryEditor: React.FC<Props> = React.memo(
),
},
{
'data-test-subj': 'sharing-global-option',
value: i18n.SHARING_GLOBAL_OPTION_LABEL,
inputDisplay: (
<EuiText size={'s'}>
Expand Down Expand Up @@ -111,6 +123,7 @@ export const DocumentEntryEditor: React.FC<Props> = React.memo(
fullWidth
>
<EuiSuperSelect
data-test-subj="sharing-select"
options={sharingOptions}
valueOfSelected={selectedSharingOption}
onChange={setSharingOptions}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ const mockContext = {
selectedSettingsTab: null,
assistantAvailability: {
isAssistantEnabled: true,
hasManageGlobalKnowledgeBase: true,
},
};
jest.mock('../../assistant_context');
Expand Down Expand Up @@ -64,23 +65,67 @@ const wrapper = (props: { children: React.ReactNode }) => (
</I18nProvider>
);
describe('KnowledgeBaseSettingsManagement', () => {
const mockCreateEntry = jest.fn();
const mockUpdateEntry = jest.fn();
const mockDeleteEntry = jest.fn();
const mockData = [
{ id: '1', name: 'Test Entry 1', type: 'document', kbResource: 'user', users: [{ id: 'hi' }] },
{
id: '1',
createdAt: '2024-10-21T18:54:14.773Z',
createdBy: 'u_user_id_1',
updatedAt: '2024-10-23T17:33:15.933Z',
updatedBy: 'u_user_id_1',
users: [{ name: 'Test User 1' }],
name: 'Test Entry 1',
namespace: 'default',
type: 'document',
kbResource: 'user',
source: 'user',
text: 'Very nice text',
},
{
id: '2',
createdAt: '2024-10-25T09:55:56.596Z',
createdBy: 'u_user_id_2',
updatedAt: '2024-10-25T09:55:56.596Z',
updatedBy: 'u_user_id_2',
users: [],
name: 'Test Entry 2',
namespace: 'default',
type: 'index',
kbResource: 'global',
users: [],
index: 'missing-index',
index: 'index-1',
field: 'semantic_field1',
description: 'Test description',
queryDescription: 'Test query instruction',
},
{
id: '3',
createdAt: '2024-10-25T09:55:56.596Z',
createdBy: 'u_user_id_1',
updatedAt: '2024-10-25T09:55:56.596Z',
updatedBy: 'u_user_id_1',
users: [{ name: 'Test User 1' }],
name: 'Test Entry 3',
namespace: 'default',
type: 'index',
kbResource: 'private',
users: [{ id: 'fake-user' }],
index: 'index-2',
field: 'semantic_field2',
description: 'Test description',
queryDescription: 'Test query instruction',
},
{
id: '4',
createdAt: '2024-10-21T18:54:14.773Z',
createdBy: 'u_user_id_3',
updatedAt: '2024-10-23T17:33:15.933Z',
updatedBy: 'u_user_id_3',
users: [],
name: 'Test Entry 4',
namespace: 'default',
type: 'document',
kbResource: 'user',
source: 'user',
text: 'Very nice text',
},
];

Expand Down Expand Up @@ -114,15 +159,15 @@ describe('KnowledgeBaseSettingsManagement', () => {
closeFlyout: jest.fn(),
});
(useCreateKnowledgeBaseEntry as jest.Mock).mockReturnValue({
mutateAsync: jest.fn(),
mutateAsync: mockCreateEntry,
isLoading: false,
});
(useUpdateKnowledgeBaseEntries as jest.Mock).mockReturnValue({
mutateAsync: jest.fn(),
mutateAsync: mockUpdateEntry,
isLoading: false,
});
(useDeleteKnowledgeBaseEntries as jest.Mock).mockReturnValue({
mutateAsync: jest.fn(),
mutateAsync: mockDeleteEntry,
isLoading: false,
});
});
Expand Down Expand Up @@ -258,6 +303,124 @@ describe('KnowledgeBaseSettingsManagement', () => {
expect(screen.queryByTestId('delete-entry-confirmation')).not.toBeInTheDocument();
});

it('does not create a duplicate document entry when switching sharing option twice', async () => {
(useFlyoutModalVisibility as jest.Mock).mockReturnValue({
isFlyoutOpen: true,
openFlyout: jest.fn(),
closeFlyout: jest.fn(),
});
render(<KnowledgeBaseSettingsManagement dataViews={mockDataViews} />, {
wrapper,
});

await waitFor(() => {
fireEvent.click(screen.getAllByTestId('edit-button')[0]);
});
expect(screen.getByTestId('flyout')).toBeVisible();

await waitFor(() => {
expect(screen.getByText('Edit document entry')).toBeInTheDocument();
});

const updatedName = 'New Entry Name';
await waitFor(() => {
const nameInput = screen.getByTestId('entryNameInput');
fireEvent.change(nameInput, { target: { value: updatedName } });
});

await waitFor(() => {
fireEvent.click(screen.getByTestId('sharing-select'));
fireEvent.click(screen.getByTestId('sharing-global-option'));
fireEvent.click(screen.getByTestId('sharing-select'));
fireEvent.click(screen.getByTestId('sharing-private-option'));
fireEvent.click(screen.getByTestId('save-button'));
});

await waitFor(() => {
expect(mockUpdateEntry).toHaveBeenCalledTimes(1);
});
expect(mockCreateEntry).toHaveBeenCalledTimes(0);
expect(mockUpdateEntry).toHaveBeenCalledWith([{ ...mockData[0], name: updatedName }]);
});

it('does not create a duplicate index entry when switching sharing option twice', async () => {
(useFlyoutModalVisibility as jest.Mock).mockReturnValue({
isFlyoutOpen: true,
openFlyout: jest.fn(),
closeFlyout: jest.fn(),
});
render(<KnowledgeBaseSettingsManagement dataViews={mockDataViews} />, {
wrapper,
});

await waitFor(() => {
fireEvent.click(screen.getAllByTestId('edit-button')[2]);
});
expect(screen.getByTestId('flyout')).toBeVisible();

await waitFor(() => {
expect(screen.getByText('Edit index entry')).toBeInTheDocument();
});

const updatedName = 'New Entry Name';
await waitFor(() => {
const nameInput = screen.getByTestId('entry-name');
fireEvent.change(nameInput, { target: { value: updatedName } });
});

await waitFor(() => {
fireEvent.click(screen.getByTestId('sharing-select'));
fireEvent.click(screen.getByTestId('sharing-global-option'));
fireEvent.click(screen.getByTestId('sharing-select'));
fireEvent.click(screen.getByTestId('sharing-private-option'));
fireEvent.click(screen.getByTestId('save-button'));
});

await waitFor(() => {
expect(mockUpdateEntry).toHaveBeenCalledTimes(1);
});
expect(mockCreateEntry).toHaveBeenCalledTimes(0);
expect(mockUpdateEntry).toHaveBeenCalledWith([{ ...mockData[2], name: updatedName }]);
});

it('shows duplicate entry modal when making global to private entry update', async () => {
(useFlyoutModalVisibility as jest.Mock).mockReturnValue({
isFlyoutOpen: true,
openFlyout: jest.fn(),
closeFlyout: jest.fn(),
});
render(<KnowledgeBaseSettingsManagement dataViews={mockDataViews} />, {
wrapper,
});

await waitFor(() => {
fireEvent.click(screen.getAllByTestId('edit-button')[3]);
});
expect(screen.getByTestId('flyout')).toBeVisible();

await waitFor(() => {
expect(screen.getByText('Edit document entry')).toBeInTheDocument();
});

await waitFor(() => {
fireEvent.click(screen.getByTestId('sharing-select'));
fireEvent.click(screen.getByTestId('sharing-private-option'));
fireEvent.click(screen.getByTestId('save-button'));
});

expect(screen.getByTestId('create-duplicate-entry-modal')).toBeInTheDocument();
await waitFor(() => {
fireEvent.click(screen.getByTestId('confirmModalConfirmButton'));
});
expect(screen.queryByTestId('create-duplicate-entry-modal')).not.toBeInTheDocument();

await waitFor(() => {
expect(mockCreateEntry).toHaveBeenCalledTimes(1);
});
expect(mockUpdateEntry).toHaveBeenCalledTimes(0);
expect(mockCreateEntry).toHaveBeenCalledWith({ ...mockData[3], users: undefined });
});

it('shows warning icon for index entries with missing indices', async () => {
render(<KnowledgeBaseSettingsManagement dataViews={mockDataViews} />, {
wrapper,
Expand Down
Loading

0 comments on commit f11c125

Please sign in to comment.