Skip to content

Commit

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

## Summary

Original ticket describing the BUG:
elastic#197157

These changes fix two issues:
1. Updating an entry from Global to Private duplicates it. After
discussing with the team we decided that this is an expected behaviour
and we would add a modal dialog which warns users about it. See more
details here
elastic#197157 (comment)
2. Editing Private entry and switching the sharing option twice from
Private => Global => Private causes the issue where we would treat
selected entry as a new one and thus calling "create entry" instead of
"update".

### Steps to reproduce second issue:

* Edit private entry
* Update entry's name
* Switch sharing option to Global
* Switch sharing option back to Private
* Save the entry

**Current behaviour**: a new private entry is created
**Expected behaviour**: existing private entry is updated

### Screen recording of the fixed first issue


https://github.com/user-attachments/assets/e11e14bd-c557-401e-a23f-e01ac7aedf30

### Checklist

Delete any items that are not applicable to this PR.

- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
  • Loading branch information
@e40pud
e40pud authored Oct 25, 2024
1 parent de5ccde commit d17fc09
Show file tree
Hide file tree
Showing 5 changed files with 274 additions and 30 deletions.
21 changes: 17 additions & 4 deletions ...ssistant/impl/knowledge_base/knowledge_base_settings_management/document_entry_editor.tsx
View file
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
181 changes: 172 additions & 9 deletions ...n-elastic-assistant/impl/knowledge_base/knowledge_base_settings_management/index.test.tsx
View file
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 d17fc09

Please sign in to comment.