From dd6da6463b0ebc1751757e9ada5fc9804dc549a8 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Tue, 20 Oct 2020 13:36:13 -0700 Subject: [PATCH 1/8] Add fullEngineAccessChecked logic --- .../credentials/credentials_logic.test.ts | 52 +++++++++++++++++++ .../credentials/credentials_logic.ts | 6 ++- 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts index 8eb0f7582516d..16b420a82fe3d 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts @@ -13,6 +13,7 @@ jest.mock('../../../shared/http', () => ({ HttpLogic: { values: { http: { get: jest.fn(), delete: jest.fn() } } }, })); import { HttpLogic } from '../../../shared/http'; + jest.mock('../../../shared/flash_messages', () => ({ FlashMessagesLogic: { actions: { clearFlashMessages: jest.fn() } }, setSuccessMessage: jest.fn(), @@ -24,6 +25,13 @@ import { flashAPIErrors, } from '../../../shared/flash_messages'; +jest.mock('../../app_logic', () => ({ + AppLogic: { + selectors: { myRole: jest.fn(() => ({})) }, + }, +})); +import { AppLogic } from '../../app_logic'; + describe('CredentialsLogic', () => { const DEFAULT_VALUES = { activeApiToken: { @@ -1174,6 +1182,50 @@ describe('CredentialsLogic', () => { }); describe('selectors', () => { + describe('fullEngineAccessChecked', () => { + it('should be true if active token is set to access all engines and the user can access all engines', () => { + (AppLogic.selectors.myRole as jest.Mock).mockReturnValueOnce({ + canAccessAllEngines: true, + }); + mount({ + activeApiToken: { + ...DEFAULT_VALUES.activeApiToken, + access_all_engines: true, + }, + }); + + expect(CredentialsLogic.values.fullEngineAccessChecked).toEqual(true); + }); + + it('should be false if the token is not set to access all engines', () => { + (AppLogic.selectors.myRole as jest.Mock).mockReturnValueOnce({ + canAccessAllEngines: true, + }); + mount({ + activeApiToken: { + ...DEFAULT_VALUES.activeApiToken, + access_all_engines: false, + }, + }); + + expect(CredentialsLogic.values.fullEngineAccessChecked).toEqual(false); + }); + + it('should be false if the user cannot acess all engines', () => { + (AppLogic.selectors.myRole as jest.Mock).mockReturnValueOnce({ + canAccessAllEngines: false, + }); + mount({ + activeApiToken: { + ...DEFAULT_VALUES.activeApiToken, + access_all_engines: true, + }, + }); + + expect(CredentialsLogic.values.fullEngineAccessChecked).toEqual(false); + }); + }); + describe('activeApiTokenExists', () => { it('should be false if the token has no id', () => { mount({ diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts index 40966d64212f6..8144fc5f93228 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts @@ -15,6 +15,7 @@ import { setSuccessMessage, flashAPIErrors, } from '../../../shared/flash_messages'; +import { AppLogic } from '../../app_logic'; import { IMeta } from '../../../../../common/types'; import { IEngine } from '../../types'; @@ -204,7 +205,10 @@ export const CredentialsLogic = kea< ], }), selectors: ({ selectors }) => ({ - // TODO fullEngineAccessChecked from ent-search + fullEngineAccessChecked: [ + () => [AppLogic.selectors.myRole, selectors.activeApiToken], + (myRole, activeApiToken) => myRole.canAccessAllEngines && !!activeApiToken.access_all_engines, + ], dataLoading: [ () => [selectors.isCredentialsDetailsComplete, selectors.isCredentialsDataComplete], (isCredentialsDetailsComplete, isCredentialsDataComplete) => { From de1c47a8699707e2de1475e14c4178bce3771ac5 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Wed, 21 Oct 2020 12:24:31 -0700 Subject: [PATCH 2/8] Add onEngineSelect logic --- .../credentials/credentials_logic.test.ts | 30 +++++++++++++++++++ .../credentials/credentials_logic.ts | 10 ++++++- 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts index 16b420a82fe3d..70efd317254f0 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts @@ -1179,6 +1179,36 @@ describe('CredentialsLogic', () => { } }); }); + + describe('onEngineSelect', () => { + it('calls addEngineName if the engine is not selected', () => { + mount({ + activeApiToken: { + ...DEFAULT_VALUES.activeApiToken, + engines: [], + }, + }); + jest.spyOn(CredentialsLogic.actions, 'addEngineName'); + + CredentialsLogic.actions.onEngineSelect('engine1'); + expect(CredentialsLogic.actions.addEngineName).toHaveBeenCalledWith('engine1'); + expect(CredentialsLogic.values.activeApiToken.engines).toEqual(['engine1']); + }); + + it('calls removeEngineName if the engine is already selected', () => { + mount({ + activeApiToken: { + ...DEFAULT_VALUES.activeApiToken, + engines: ['engine1', 'engine2'], + }, + }); + jest.spyOn(CredentialsLogic.actions, 'removeEngineName'); + + CredentialsLogic.actions.onEngineSelect('engine1'); + expect(CredentialsLogic.actions.removeEngineName).toHaveBeenCalledWith('engine1'); + expect(CredentialsLogic.values.activeApiToken.engines).toEqual(['engine2']); + }); + }); }); describe('selectors', () => { diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts index 8144fc5f93228..ee262f4d030cd 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts @@ -50,6 +50,7 @@ interface ICredentialsLogicActions { fetchCredentials(page?: number): number; fetchDetails(): { value: boolean }; deleteApiKey(tokenName: string): string; + onEngineSelect(engineName: string): string; } interface ICredentialsLogicValues { @@ -93,6 +94,7 @@ export const CredentialsLogic = kea< fetchCredentials: (page) => page, fetchDetails: true, deleteApiKey: (tokenName) => tokenName, + onEngineSelect: (engineName) => engineName, }), reducers: () => ({ apiTokens: [ @@ -260,6 +262,12 @@ export const CredentialsLogic = kea< } }, // TODO onApiTokenChange from ent-search - // TODO onEngineSelect from ent-search + onEngineSelect: (engineName: string) => { + if (values.activeApiToken?.engines?.includes(engineName)) { + actions.removeEngineName(engineName); + } else { + actions.addEngineName(engineName); + } + }, }), }); From 9fc03609617e36b3ca4ad9d8f2d45abee3de93d8 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Wed, 21 Oct 2020 12:26:10 -0700 Subject: [PATCH 3/8] [Refactor] DRY out/simplify http mocks Note: import reorder is required in for mocks to work correctly --- .../credentials/credentials_logic.test.ts | 33 +++++++++---------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts index 70efd317254f0..00dc3670c2ca5 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts @@ -6,13 +6,11 @@ import { resetContext } from 'kea'; -import { CredentialsLogic } from './credentials_logic'; -import { ApiTokenTypes } from './constants'; - +import { mockHttpValues } from '../../../__mocks__'; jest.mock('../../../shared/http', () => ({ - HttpLogic: { values: { http: { get: jest.fn(), delete: jest.fn() } } }, + HttpLogic: { values: mockHttpValues }, })); -import { HttpLogic } from '../../../shared/http'; +const { http } = mockHttpValues; jest.mock('../../../shared/flash_messages', () => ({ FlashMessagesLogic: { actions: { clearFlashMessages: jest.fn() } }, @@ -32,6 +30,9 @@ jest.mock('../../app_logic', () => ({ })); import { AppLogic } from '../../app_logic'; +import { ApiTokenTypes } from './constants'; +import { CredentialsLogic } from './credentials_logic'; + describe('CredentialsLogic', () => { const DEFAULT_VALUES = { activeApiToken: { @@ -1089,10 +1090,10 @@ describe('CredentialsLogic', () => { mount(); jest.spyOn(CredentialsLogic.actions, 'setCredentialsData').mockImplementationOnce(() => {}); const promise = Promise.resolve({ meta, results }); - (HttpLogic.values.http.get as jest.Mock).mockReturnValue(promise); + http.get.mockReturnValue(promise); CredentialsLogic.actions.fetchCredentials(2); - expect(HttpLogic.values.http.get).toHaveBeenCalledWith('/api/app_search/credentials', { + expect(http.get).toHaveBeenCalledWith('/api/app_search/credentials', { query: { 'page[current]': 2, }, @@ -1104,7 +1105,7 @@ describe('CredentialsLogic', () => { it('handles errors', async () => { mount(); const promise = Promise.reject('An error occured'); - (HttpLogic.values.http.get as jest.Mock).mockReturnValue(promise); + http.get.mockReturnValue(promise); CredentialsLogic.actions.fetchCredentials(); try { @@ -1122,12 +1123,10 @@ describe('CredentialsLogic', () => { .spyOn(CredentialsLogic.actions, 'setCredentialsDetails') .mockImplementationOnce(() => {}); const promise = Promise.resolve(credentialsDetails); - (HttpLogic.values.http.get as jest.Mock).mockReturnValue(promise); + http.get.mockReturnValue(promise); CredentialsLogic.actions.fetchDetails(); - expect(HttpLogic.values.http.get).toHaveBeenCalledWith( - '/api/app_search/credentials/details' - ); + expect(http.get).toHaveBeenCalledWith('/api/app_search/credentials/details'); await promise; expect(CredentialsLogic.actions.setCredentialsDetails).toHaveBeenCalledWith( credentialsDetails @@ -1137,7 +1136,7 @@ describe('CredentialsLogic', () => { it('handles errors', async () => { mount(); const promise = Promise.reject('An error occured'); - (HttpLogic.values.http.get as jest.Mock).mockReturnValue(promise); + http.get.mockReturnValue(promise); CredentialsLogic.actions.fetchDetails(); try { @@ -1155,12 +1154,10 @@ describe('CredentialsLogic', () => { mount(); jest.spyOn(CredentialsLogic.actions, 'onApiKeyDelete').mockImplementationOnce(() => {}); const promise = Promise.resolve(); - (HttpLogic.values.http.delete as jest.Mock).mockReturnValue(promise); + http.delete.mockReturnValue(promise); CredentialsLogic.actions.deleteApiKey(tokenName); - expect(HttpLogic.values.http.delete).toHaveBeenCalledWith( - `/api/app_search/credentials/${tokenName}` - ); + expect(http.delete).toHaveBeenCalledWith(`/api/app_search/credentials/${tokenName}`); await promise; expect(CredentialsLogic.actions.onApiKeyDelete).toHaveBeenCalledWith(tokenName); expect(setSuccessMessage).toHaveBeenCalled(); @@ -1169,7 +1166,7 @@ describe('CredentialsLogic', () => { it('handles errors', async () => { mount(); const promise = Promise.reject('An error occured'); - (HttpLogic.values.http.delete as jest.Mock).mockReturnValue(promise); + http.delete.mockReturnValue(promise); CredentialsLogic.actions.deleteApiKey(tokenName); try { From be27c543dc6c5b4be0f15daa9b66d49d1b2880b9 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Wed, 21 Oct 2020 12:27:30 -0700 Subject: [PATCH 4/8] Add onApiTokenChange logic --- .../credentials/credentials_logic.test.ts | 65 +++++++++++++++++++ .../credentials/credentials_logic.ts | 47 +++++++++++++- 2 files changed, 110 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts index 00dc3670c2ca5..9fcd1ce2e9b47 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts @@ -26,6 +26,7 @@ import { jest.mock('../../app_logic', () => ({ AppLogic: { selectors: { myRole: jest.fn(() => ({})) }, + values: { myRole: jest.fn(() => ({})) }, }, })); import { AppLogic } from '../../app_logic'; @@ -1177,6 +1178,70 @@ describe('CredentialsLogic', () => { }); }); + describe('onApiTokenChange', () => { + it('calls a POST API endpoint that creates a new token', async () => { + const createdToken = { + name: 'new-key', + type: ApiTokenTypes.Admin, + }; + mount({ + activeApiToken: createdToken, + }); + jest.spyOn(CredentialsLogic.actions, 'onApiTokenCreateSuccess'); + const promise = Promise.resolve(createdToken); + http.post.mockReturnValue(promise); + + CredentialsLogic.actions.onApiTokenChange(); + expect(http.post).toHaveBeenCalledWith('/api/app_search/credentials', { + body: JSON.stringify(createdToken), + }); + await promise; + expect(CredentialsLogic.actions.onApiTokenCreateSuccess).toHaveBeenCalledWith(createdToken); + expect(setSuccessMessage).toHaveBeenCalled(); + }); + + it('calls a PUT endpoint that updates existing API tokens', async () => { + const updatedToken = { + name: 'test-key', + type: ApiTokenTypes.Private, + read: true, + write: false, + access_all_engines: false, + engines: ['engine1'], + }; + mount({ + activeApiToken: { + ...updatedToken, + id: 'some-id', + }, + }); + jest.spyOn(CredentialsLogic.actions, 'onApiTokenUpdateSuccess'); + const promise = Promise.resolve(updatedToken); + http.put.mockReturnValue(promise); + + CredentialsLogic.actions.onApiTokenChange(); + expect(http.put).toHaveBeenCalledWith('/api/app_search/credentials/test-key', { + body: JSON.stringify(updatedToken), + }); + await promise; + expect(CredentialsLogic.actions.onApiTokenUpdateSuccess).toHaveBeenCalledWith(updatedToken); + expect(setSuccessMessage).toHaveBeenCalled(); + }); + + it('handles errors', async () => { + mount(); + const promise = Promise.reject('An error occured'); + http.post.mockReturnValue(promise); + + CredentialsLogic.actions.onApiTokenChange(); + try { + await promise; + } catch { + expect(flashAPIErrors).toHaveBeenCalledWith('An error occured'); + } + }); + }); + describe('onEngineSelect', () => { it('calls addEngineName if the engine is not selected', () => { mount({ diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts index ee262f4d030cd..a21fc57b870e7 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts @@ -7,7 +7,7 @@ import { kea, MakeLogicType } from 'kea'; import { formatApiName } from '../../utils/format_api_name'; -import { ApiTokenTypes, DELETE_MESSAGE } from './constants'; +import { ApiTokenTypes, CREATE_MESSAGE, UPDATE_MESSAGE, DELETE_MESSAGE } from './constants'; import { HttpLogic } from '../../../shared/http'; import { @@ -50,6 +50,7 @@ interface ICredentialsLogicActions { fetchCredentials(page?: number): number; fetchDetails(): { value: boolean }; deleteApiKey(tokenName: string): string; + onApiTokenChange(): void; onEngineSelect(engineName: string): string; } @@ -94,6 +95,7 @@ export const CredentialsLogic = kea< fetchCredentials: (page) => page, fetchDetails: true, deleteApiKey: (tokenName) => tokenName, + onApiTokenChange: () => null, onEngineSelect: (engineName) => engineName, }), reducers: () => ({ @@ -261,7 +263,48 @@ export const CredentialsLogic = kea< flashAPIErrors(e); } }, - // TODO onApiTokenChange from ent-search + onApiTokenChange: async () => { + const { myRole } = AppLogic.values; + const { + id, + name, + engines, + type, + read, + write, + access_all_engines: accessAllEngines, + } = values.activeApiToken; + + const data: IApiToken = { + name, + type, + }; + if (type === ApiTokenTypes.Private) { + data.read = read; + data.write = write; + } + if (type !== ApiTokenTypes.Admin) { + data.access_all_engines = !!(accessAllEngines && myRole.canAccessAllEngines); + data.engines = engines; + } + + try { + const { http } = HttpLogic.values; + const body = JSON.stringify(data); + + if (id) { + const response = await http.put(`/api/app_search/credentials/${name}`, { body }); + actions.onApiTokenUpdateSuccess(response); + setSuccessMessage(UPDATE_MESSAGE); + } else { + const response = await http.post('/api/app_search/credentials', { body }); + actions.onApiTokenCreateSuccess(response); + setSuccessMessage(CREATE_MESSAGE); + } + } catch (e) { + flashAPIErrors(e); + } + }, onEngineSelect: (engineName: string) => { if (values.activeApiToken?.engines?.includes(engineName)) { actions.removeEngineName(engineName); From 5842451bc41910e7ce6a9de86ce72a69e891054a Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Wed, 21 Oct 2020 12:27:54 -0700 Subject: [PATCH 5/8] Update flyout footer to use onApiTokenChange --- .../components/credentials/credentials_flyout/footer.test.tsx | 3 ++- .../components/credentials/credentials_flyout/footer.tsx | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_flyout/footer.test.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_flyout/footer.test.tsx index 1ec3e4756c5c4..c31546472b036 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_flyout/footer.test.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_flyout/footer.test.tsx @@ -18,6 +18,7 @@ describe('CredentialsFlyoutFooter', () => { }; const actions = { hideCredentialsForm: jest.fn(), + onApiTokenChange: jest.fn(), }; beforeEach(() => { @@ -59,6 +60,6 @@ describe('CredentialsFlyoutFooter', () => { const button = wrapper.find('[data-test-subj="APIKeyActionButton"]'); button.simulate('click'); - // TODO: Expect onApiTokenChange to have been called + expect(actions.onApiTokenChange).toHaveBeenCalled(); }); }); diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_flyout/footer.tsx b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_flyout/footer.tsx index 7564560eade95..e59a75a578ba4 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_flyout/footer.tsx +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_flyout/footer.tsx @@ -18,7 +18,7 @@ import { i18n } from '@kbn/i18n'; import { CredentialsLogic } from '../credentials_logic'; export const CredentialsFlyoutFooter: React.FC = () => { - const { hideCredentialsForm } = useActions(CredentialsLogic); + const { hideCredentialsForm, onApiTokenChange } = useActions(CredentialsLogic); const { activeApiTokenExists } = useValues(CredentialsLogic); return ( @@ -33,7 +33,7 @@ export const CredentialsFlyoutFooter: React.FC = () => { window.alert('submit')} + onClick={onApiTokenChange} fill={true} color="secondary" iconType="check" From de5d6cc1ea8bb21c5a1b7f526231a5608103b28c Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Wed, 21 Oct 2020 12:30:59 -0700 Subject: [PATCH 6/8] Add new POST/PUT server routes + some opinionated comments --- .../routes/app_search/credentials.test.ts | 226 ++++++++++++++++++ .../server/routes/app_search/credentials.ts | 53 ++++ 2 files changed, 279 insertions(+) diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/credentials.test.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/credentials.test.ts index 6b5f4a05b3aa6..357b49de93412 100644 --- a/x-pack/plugins/enterprise_search/server/routes/app_search/credentials.test.ts +++ b/x-pack/plugins/enterprise_search/server/routes/app_search/credentials.test.ts @@ -41,6 +41,115 @@ describe('credentials routes', () => { }); }); + describe('POST /api/app_search/credentials', () => { + let mockRouter: MockRouter; + + beforeEach(() => { + jest.clearAllMocks(); + mockRouter = new MockRouter({ method: 'post', payload: 'body' }); + + registerCredentialsRoutes({ + ...mockDependencies, + router: mockRouter.router, + }); + }); + + it('creates a request handler', () => { + expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({ + path: '/as/credentials/collection', + }); + }); + + describe('validates', () => { + describe('admin keys', () => { + it('correctly', () => { + const request = { + body: { + name: 'admin-key', + type: 'admin', + }, + }; + mockRouter.shouldValidate(request); + }); + + it('throws on unnecessary properties', () => { + const request = { + body: { + name: 'admin-key', + type: 'admin', + read: true, + access_all_engines: true, + }, + }; + mockRouter.shouldThrow(request); + }); + }); + + describe('private keys', () => { + it('correctly', () => { + const request = { + body: { + name: 'private-key', + type: 'private', + read: true, + write: false, + access_all_engines: false, + engines: ['engine1', 'engine2'], + }, + }; + mockRouter.shouldValidate(request); + }); + + it('throws on missing keys', () => { + const request = { + body: { + name: 'private-key', + type: 'private', + }, + }; + mockRouter.shouldThrow(request); + }); + }); + + describe('search keys', () => { + it('correctly', () => { + const request = { + body: { + name: 'search-key', + type: 'search', + access_all_engines: true, + }, + }; + mockRouter.shouldValidate(request); + }); + + it('throws on missing keys', () => { + const request = { + body: { + name: 'search-key', + type: 'search', + }, + }; + mockRouter.shouldThrow(request); + }); + + it('throws on extra keys', () => { + const request = { + body: { + name: 'search-key', + type: 'search', + read: true, + write: false, + access_all_engines: false, + engines: ['engine1', 'engine2'], + }, + }; + mockRouter.shouldThrow(request); + }); + }); + }); + }); + describe('GET /api/app_search/credentials/details', () => { let mockRouter: MockRouter; @@ -61,6 +170,123 @@ describe('credentials routes', () => { }); }); + describe('PUT /api/app_search/credentials/{name}', () => { + let mockRouter: MockRouter; + + beforeEach(() => { + jest.clearAllMocks(); + mockRouter = new MockRouter({ method: 'put', payload: 'body' }); + + registerCredentialsRoutes({ + ...mockDependencies, + router: mockRouter.router, + }); + }); + + it('creates a request to enterprise search', () => { + const mockRequest = { + params: { + name: 'abc123', + }, + }; + + mockRouter.callRoute(mockRequest); + + expect(mockRequestHandler.createRequest).toHaveBeenCalledWith({ + path: '/as/credentials/abc123', + }); + }); + + describe('validates', () => { + describe('admin keys', () => { + it('correctly', () => { + const request = { + body: { + name: 'admin-key', + type: 'admin', + }, + }; + mockRouter.shouldValidate(request); + }); + + it('throws on unnecessary properties', () => { + const request = { + body: { + name: 'admin-key', + type: 'admin', + read: true, + access_all_engines: true, + }, + }; + mockRouter.shouldThrow(request); + }); + }); + + describe('private keys', () => { + it('correctly', () => { + const request = { + body: { + name: 'private-key', + type: 'private', + read: true, + write: false, + access_all_engines: false, + engines: ['engine1', 'engine2'], + }, + }; + mockRouter.shouldValidate(request); + }); + + it('throws on missing keys', () => { + const request = { + body: { + name: 'private-key', + type: 'private', + }, + }; + mockRouter.shouldThrow(request); + }); + }); + + describe('search keys', () => { + it('correctly', () => { + const request = { + body: { + name: 'search-key', + type: 'search', + access_all_engines: true, + }, + }; + mockRouter.shouldValidate(request); + }); + + it('throws on missing keys', () => { + const request = { + body: { + name: 'search-key', + type: 'search', + }, + }; + mockRouter.shouldThrow(request); + }); + + it('throws on extra keys', () => { + const request = { + body: { + name: 'search-key', + type: 'search', + read: true, + write: false, + access_all_engines: false, + engines: ['engine1', 'engine2'], + }, + }; + mockRouter.shouldThrow(request); + }); + }); + }); + }); + describe('DELETE /api/app_search/credentials/{name}', () => { let mockRouter: MockRouter; diff --git a/x-pack/plugins/enterprise_search/server/routes/app_search/credentials.ts b/x-pack/plugins/enterprise_search/server/routes/app_search/credentials.ts index 0f2c1133192c5..85d213c82dd05 100644 --- a/x-pack/plugins/enterprise_search/server/routes/app_search/credentials.ts +++ b/x-pack/plugins/enterprise_search/server/routes/app_search/credentials.ts @@ -8,10 +8,32 @@ import { schema } from '@kbn/config-schema'; import { IRouteDependencies } from '../../plugin'; +const tokenSchema = schema.oneOf([ + schema.object({ + name: schema.string(), + type: schema.literal('admin'), + }), + schema.object({ + name: schema.string(), + type: schema.literal('private'), + read: schema.boolean(), + write: schema.boolean(), + access_all_engines: schema.boolean(), + engines: schema.maybe(schema.arrayOf(schema.string())), + }), + schema.object({ + name: schema.string(), + type: schema.literal('search'), + access_all_engines: schema.boolean(), + engines: schema.maybe(schema.arrayOf(schema.string())), + }), +]); + export function registerCredentialsRoutes({ router, enterpriseSearchRequestHandler, }: IRouteDependencies) { + // Credentials API router.get( { path: '/api/app_search/credentials', @@ -25,6 +47,19 @@ export function registerCredentialsRoutes({ path: '/as/credentials/collection', }) ); + router.post( + { + path: '/api/app_search/credentials', + validate: { + body: tokenSchema, + }, + }, + enterpriseSearchRequestHandler.createRequest({ + path: '/as/credentials/collection', + }) + ); + + // TODO: It would be great to remove this someday router.get( { path: '/api/app_search/credentials/details', @@ -34,6 +69,24 @@ export function registerCredentialsRoutes({ path: '/as/credentials/details', }) ); + + // Single credential API + router.put( + { + path: '/api/app_search/credentials/{name}', + validate: { + params: schema.object({ + name: schema.string(), + }), + body: tokenSchema, + }, + }, + async (context, request, response) => { + return enterpriseSearchRequestHandler.createRequest({ + path: `/as/credentials/${request.params.name}`, + })(context, request, response); + } + ); router.delete( { path: '/api/app_search/credentials/{name}', From b34d7300de4e66791bcdd73f9d1cdacb65c041d5 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 22 Oct 2020 13:03:51 -0700 Subject: [PATCH 7/8] [PR feedback] tests copy, extra data tests --- .../credentials/credentials_logic.test.ts | 46 ++++++++++++++++++- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts index 9fcd1ce2e9b47..4702c8b0c6883 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts @@ -1179,7 +1179,7 @@ describe('CredentialsLogic', () => { }); describe('onApiTokenChange', () => { - it('calls a POST API endpoint that creates a new token', async () => { + it('calls a POST API endpoint that creates a new token if the active token does not exist yet', async () => { const createdToken = { name: 'new-key', type: ApiTokenTypes.Admin, @@ -1200,7 +1200,7 @@ describe('CredentialsLogic', () => { expect(setSuccessMessage).toHaveBeenCalled(); }); - it('calls a PUT endpoint that updates existing API tokens', async () => { + it('calls a PUT endpoint that updates the active token if it already exists', async () => { const updatedToken = { name: 'test-key', type: ApiTokenTypes.Private, @@ -1240,6 +1240,48 @@ describe('CredentialsLogic', () => { expect(flashAPIErrors).toHaveBeenCalledWith('An error occured'); } }); + + describe('token type data', () => { + it('does not send extra read/write/engine access data for admin tokens', () => { + const correctAdminToken = { + name: 'bogus-admin', + type: ApiTokenTypes.Admin, + }; + const extraData = { + read: true, + write: true, + access_all_engines: true, + }; + mount({ activeApiToken: { ...correctAdminToken, ...extraData } }); + + CredentialsLogic.actions.onApiTokenChange(); + expect(http.post).toHaveBeenCalledWith('/api/app_search/credentials', { + body: JSON.stringify(correctAdminToken), + }); + }); + + it('does not send extra read/write access data for search tokens', () => { + const correctSearchToken = { + name: 'bogus-search', + type: ApiTokenTypes.Search, + access_all_engines: false, + engines: ['some-engine'], + }; + const extraData = { + read: true, + write: false, + }; + mount({ activeApiToken: { ...correctSearchToken, ...extraData } }); + + CredentialsLogic.actions.onApiTokenChange(); + expect(http.post).toHaveBeenCalledWith('/api/app_search/credentials', { + body: JSON.stringify(correctSearchToken), + }); + }); + + // Private tokens send all data per the PUT test above. + // If that ever changes, we should capture that in another test here. + }); }); describe('onEngineSelect', () => { From 2b200a0520f44c40967e139ea45eb4f4433418f8 Mon Sep 17 00:00:00 2001 From: Constance Chen Date: Thu, 22 Oct 2020 13:04:31 -0700 Subject: [PATCH 8/8] [PR feedback] Reuse fullEngineAccessChecked, fix fullEngineAccessChecked being undefined vs a bool --- .../credentials/credentials_logic.test.ts | 1 + .../components/credentials/credentials_logic.ts | 16 ++++------------ 2 files changed, 5 insertions(+), 12 deletions(-) diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts index 4702c8b0c6883..de79862b540ba 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.test.ts @@ -54,6 +54,7 @@ describe('CredentialsLogic', () => { meta: {}, nameInputBlurred: false, shouldShowCredentialsForm: false, + fullEngineAccessChecked: false, }; const mount = (defaults?: object) => { diff --git a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts index a21fc57b870e7..30b5fabc4d0c4 100644 --- a/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts +++ b/x-pack/plugins/enterprise_search/public/applications/app_search/components/credentials/credentials_logic.ts @@ -211,7 +211,8 @@ export const CredentialsLogic = kea< selectors: ({ selectors }) => ({ fullEngineAccessChecked: [ () => [AppLogic.selectors.myRole, selectors.activeApiToken], - (myRole, activeApiToken) => myRole.canAccessAllEngines && !!activeApiToken.access_all_engines, + (myRole, activeApiToken) => + !!(myRole.canAccessAllEngines && activeApiToken.access_all_engines), ], dataLoading: [ () => [selectors.isCredentialsDetailsComplete, selectors.isCredentialsDataComplete], @@ -264,16 +265,7 @@ export const CredentialsLogic = kea< } }, onApiTokenChange: async () => { - const { myRole } = AppLogic.values; - const { - id, - name, - engines, - type, - read, - write, - access_all_engines: accessAllEngines, - } = values.activeApiToken; + const { id, name, engines, type, read, write } = values.activeApiToken; const data: IApiToken = { name, @@ -284,7 +276,7 @@ export const CredentialsLogic = kea< data.write = write; } if (type !== ApiTokenTypes.Admin) { - data.access_all_engines = !!(accessAllEngines && myRole.canAccessAllEngines); + data.access_all_engines = values.fullEngineAccessChecked; data.engines = engines; }