From 9b215546d45c0f4efd5effcb2514d470512669a7 Mon Sep 17 00:00:00 2001 From: Xinrui Bai Date: Thu, 4 Jan 2024 15:02:35 -0800 Subject: [PATCH 1/5] fix Datasource testing connection don't validate endpoints with path #5656 Signed-off-by: Xinrui Bai --- .../public/components/utils.test.ts | 3 ++ .../public/components/utils.ts | 6 +++- .../datasource_form_validation.test.ts | 16 ++++++++++- .../data_source_management/public/mocks.ts | 28 +++++++++++++++++++ 4 files changed, 51 insertions(+), 2 deletions(-) diff --git a/src/plugins/data_source_management/public/components/utils.test.ts b/src/plugins/data_source_management/public/components/utils.test.ts index a94d5b2260e6..a113c114310e 100644 --- a/src/plugins/data_source_management/public/components/utils.test.ts +++ b/src/plugins/data_source_management/public/components/utils.test.ts @@ -212,6 +212,9 @@ describe('DataSourceManagement: Utils.ts', () => { expect(isValidUrl('')).toBeFalsy(); expect(isValidUrl('test')).toBeFalsy(); + /* False cases: path name scenario*/ + expect(isValidUrl('https://test.com/_somepath')).toBeFalsy(); + /* True cases */ expect(isValidUrl('https://test.com')).toBeTruthy(); expect(isValidUrl('http://test.com')).toBeTruthy(); diff --git a/src/plugins/data_source_management/public/components/utils.ts b/src/plugins/data_source_management/public/components/utils.ts index 5f2cfb2337ad..1ef80e3eb45c 100644 --- a/src/plugins/data_source_management/public/components/utils.ts +++ b/src/plugins/data_source_management/public/components/utils.ts @@ -103,7 +103,11 @@ export async function testConnection( export const isValidUrl = (endpoint: string) => { try { const url = new URL(endpoint); - return Boolean(url) && (url.protocol === 'http:' || url.protocol === 'https:'); + return ( + Boolean(url) && + (url.protocol === 'http:' || url.protocol === 'https:') && + (!url.pathname || url.pathname.length === 0 || url.pathname === '/') + ); } catch (e) { return false; } diff --git a/src/plugins/data_source_management/public/components/validation/datasource_form_validation.test.ts b/src/plugins/data_source_management/public/components/validation/datasource_form_validation.test.ts index 9a00d0f29c91..c93f935063ba 100644 --- a/src/plugins/data_source_management/public/components/validation/datasource_form_validation.test.ts +++ b/src/plugins/data_source_management/public/components/validation/datasource_form_validation.test.ts @@ -7,7 +7,11 @@ import { AuthType } from '../../types'; import { CreateDataSourceState } from '../create_data_source_wizard/components/create_form/create_data_source_form'; import { EditDataSourceState } from '../edit_data_source/components/edit_form/edit_data_source_form'; import { defaultValidation, performDataSourceFormValidation } from './datasource_form_validation'; -import { mockDataSourceAttributesWithAuth } from '../../mocks'; +import { + mockDataSourceAttributesWithAuth, + mockDataSourceAttributesWithValidEndpoint, + mockDataSourceAttributesWithInvalidEndpoint, +} from '../../mocks'; describe('DataSourceManagement: Form Validation', () => { describe('validate create/edit datasource', () => { @@ -49,6 +53,16 @@ describe('DataSourceManagement: Form Validation', () => { const result = performDataSourceFormValidation(form, [], ''); expect(result).toBe(false); }); + test('should fail validation when endpoint path is not empty', () => { + form.endpoint = mockDataSourceAttributesWithInvalidEndpoint.endpoint; + const result = performDataSourceFormValidation(form, [], ''); + expect(result).toBe(false); + }); + test('should NOT fail validation when endpoint path is empty', () => { + form.endpoint = mockDataSourceAttributesWithValidEndpoint.endpoint; + const result = performDataSourceFormValidation(form, [], ''); + expect(result).toBe(false); + }); test('should NOT fail validation on empty username/password when No Auth is selected', () => { form.auth.type = AuthType.NoAuth; form.title = 'test'; diff --git a/src/plugins/data_source_management/public/mocks.ts b/src/plugins/data_source_management/public/mocks.ts index c078247956e0..d3236daae8fb 100644 --- a/src/plugins/data_source_management/public/mocks.ts +++ b/src/plugins/data_source_management/public/mocks.ts @@ -131,6 +131,34 @@ export const getMappedDataSources = [ }, ]; +export const mockDataSourceAttributesWithValidEndpoint = { + id: 'test', + title: 'create-test-ds', + description: 'jest testing', + endpoint: 'https://test.com', + auth: { + type: AuthType.UsernamePasswordType, + credentials: { + username: 'test123', + password: 'test123', + }, + }, +}; + +export const mockDataSourceAttributesWithInvalidEndpoint = { + id: 'test', + title: 'create-test-ds', + description: 'jest testing', + endpoint: 'https://test.com/_somepath', + auth: { + type: AuthType.UsernamePasswordType, + credentials: { + username: 'test123', + password: 'test123', + }, + }, +}; + export const mockDataSourceAttributesWithAuth = { id: 'test', title: 'create-test-ds', From 95771680eaeceaeb23bbe4f5a39fcff492e9ace5 Mon Sep 17 00:00:00 2001 From: Xinrui Bai Date: Fri, 5 Jan 2024 15:12:38 -0800 Subject: [PATCH 2/5] add changelog Signed-off-by: Xinrui Bai --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 31d65e5cf6bd..2f154e36872c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - [BUG][Discover] Fix what is displayed in `selected fields` when removing columns from canvas [#5537](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5537) - [BUG][Discover] Fix advanced setting `discover:modifyColumnsOnSwitch` ([#5508](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5508)) - [Discover] Fix missing index pattern field from breaking Discover [#5626](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5626) +- [BUG][Multiple Datasource] fix Datasource testing connection don't validate endpoints with path [#5663](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5663) ### 🚞 Infrastructure From 092791be20e5cc6c44a0b6c18c42e701999475e3 Mon Sep 17 00:00:00 2001 From: Xinrui Bai Date: Mon, 8 Jan 2024 22:18:05 +0000 Subject: [PATCH 3/5] Update unit test cases Signed-off-by: Xinrui Bai --- .../components/validation/datasource_form_validation.test.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/plugins/data_source_management/public/components/validation/datasource_form_validation.test.ts b/src/plugins/data_source_management/public/components/validation/datasource_form_validation.test.ts index c93f935063ba..50854e7cc519 100644 --- a/src/plugins/data_source_management/public/components/validation/datasource_form_validation.test.ts +++ b/src/plugins/data_source_management/public/components/validation/datasource_form_validation.test.ts @@ -59,9 +59,11 @@ describe('DataSourceManagement: Form Validation', () => { expect(result).toBe(false); }); test('should NOT fail validation when endpoint path is empty', () => { + form.auth.type = AuthType.NoAuth; + form.title = 'test'; form.endpoint = mockDataSourceAttributesWithValidEndpoint.endpoint; const result = performDataSourceFormValidation(form, [], ''); - expect(result).toBe(false); + expect(result).toBe(true); }); test('should NOT fail validation on empty username/password when No Auth is selected', () => { form.auth.type = AuthType.NoAuth; From d650f3d63ffaf6c8ad3e8147b7f760a3bc1210bc Mon Sep 17 00:00:00 2001 From: Xinrui Bai Date: Mon, 22 Jan 2024 04:15:40 +0000 Subject: [PATCH 4/5] Pass datasource test connection only when opensearchclient's response status code is 200 Signed-off-by: Xinrui Bai --- CHANGELOG.md | 2 +- .../data_source_connection_validator.test.ts | 80 +++++++++++++++++++ .../data_source_connection_validator.ts | 17 +++- .../public/components/utils.test.ts | 3 - .../public/components/utils.ts | 6 +- .../datasource_form_validation.test.ts | 18 +---- .../data_source_management/public/mocks.ts | 28 ------- 7 files changed, 97 insertions(+), 57 deletions(-) create mode 100644 src/plugins/data_source/server/routes/data_source_connection_validator.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 41f4844d27cd..fac5e39539e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,7 +48,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - [BUG][Discover] Fix advanced setting `discover:modifyColumnsOnSwitch` ([#5508](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5508)) - [Discover] Fix missing index pattern field from breaking Discover [#5626](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5626) - [BUG] Remove duplicate sample data as id 90943e30-9a47-11e8-b64d-95841ca0b247 ([5668](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5668)) -- [BUG][Multiple Datasource] fix Datasource testing connection don't validate endpoints with path [#5663](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5663) +- [BUG][Multiple Datasource] Fix datasource testing connection unexpectedly passed with wrong endpoint [#5663](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/5663) ### 🚞 Infrastructure diff --git a/src/plugins/data_source/server/routes/data_source_connection_validator.test.ts b/src/plugins/data_source/server/routes/data_source_connection_validator.test.ts new file mode 100644 index 000000000000..cfbcf4489fe6 --- /dev/null +++ b/src/plugins/data_source/server/routes/data_source_connection_validator.test.ts @@ -0,0 +1,80 @@ +/* + * Copyright OpenSearch Contributors + * SPDX-License-Identifier: Apache-2.0 + */ + +import { opensearchServiceMock } from '../../../../core/server/mocks'; +import { DataSourceConnectionValidator } from './data_source_connection_validator'; +import { SigV4ServiceName } from '../../common/data_sources'; + +describe('DataSourceManagement: data_source_connection_validator.ts', () => { + describe('Test datasource connection without SigV4 auth', () => { + test('Success: opensearch client response code is 200', async () => { + const opensearchClient = opensearchServiceMock.createOpenSearchClient(); + opensearchClient.info.mockResolvedValue(opensearchServiceMock.createApiResponse()); + const dataSourceValidator = new DataSourceConnectionValidator(opensearchClient, {}); + const validateDataSourcesResponse = await dataSourceValidator.validate(); + expect(validateDataSourcesResponse.statusCode).toBe(200); + }); + test('failure: opensearch client response code is other than 200', async () => { + const statusCodeList = [100, 202, 300, 400, 500]; + statusCodeList.forEach(async function (code) { + try { + const opensearchClient = opensearchServiceMock.createOpenSearchClient(); + opensearchClient.info.mockResolvedValue( + opensearchServiceMock.createApiResponse({ + statusCode: code, + body: { Message: 'Your request is not correct.' }, + }) + ); + const dataSourceValidator = new DataSourceConnectionValidator(opensearchClient, {}); + await dataSourceValidator.validate(); + } catch (e) { + expect(e).toBeTruthy(); + expect(e.message).toContain('Your request is not correct.'); + } + }); + }); + }); + + describe('Test datasource connection for SigV4 auth', () => { + test('Success: opensearch client response code is 200', async () => { + const opensearchClient = opensearchServiceMock.createOpenSearchClient(); + opensearchClient.cat.indices.mockResolvedValue(opensearchServiceMock.createApiResponse()); + const dataSourceValidator = new DataSourceConnectionValidator(opensearchClient, { + auth: { + credentials: { + service: SigV4ServiceName.OpenSearchServerless, + }, + }, + }); + const validateDataSourcesResponse = await dataSourceValidator.validate(); + expect(validateDataSourcesResponse.statusCode).toBe(200); + }); + test('failure: opensearch client response code is other than 200', async () => { + const statusCodeList = [100, 202, 300, 400, 500]; + statusCodeList.forEach(async function (code) { + try { + const opensearchClient = opensearchServiceMock.createOpenSearchClient(); + opensearchClient.cat.indices.mockResolvedValue( + opensearchServiceMock.createApiResponse({ + statusCode: code, + body: { Message: 'Your request is not correct.' }, + }) + ); + const dataSourceValidator = new DataSourceConnectionValidator(opensearchClient, { + auth: { + credentials: { + service: SigV4ServiceName.OpenSearchServerless, + }, + }, + }); + await dataSourceValidator.validate(); + } catch (e) { + expect(e).toBeTruthy(); + expect(e.message).toContain('Your request is not correct.'); + } + }); + }); + }); +}); diff --git a/src/plugins/data_source/server/routes/data_source_connection_validator.ts b/src/plugins/data_source/server/routes/data_source_connection_validator.ts index f3233859e854..03481dd56e39 100644 --- a/src/plugins/data_source/server/routes/data_source_connection_validator.ts +++ b/src/plugins/data_source/server/routes/data_source_connection_validator.ts @@ -14,10 +14,21 @@ export class DataSourceConnectionValidator { async validate() { try { + let validationResponse; // Amazon OpenSearch Serverless does not support .info() API - if (this.dataSourceAttr.auth?.credentials?.service === SigV4ServiceName.OpenSearchServerless) - return await this.callDataCluster.cat.indices(); - return await this.callDataCluster.info(); + if ( + this.dataSourceAttr.auth?.credentials?.service === SigV4ServiceName.OpenSearchServerless + ) { + validationResponse = await this.callDataCluster.cat.indices(); + } else { + validationResponse = await this.callDataCluster.info(); + } + + if (validationResponse?.statusCode === 200) { + return validationResponse; + } + + throw new Error(JSON.stringify(validationResponse?.body)); } catch (e) { throw createDataSourceError(e); } diff --git a/src/plugins/data_source_management/public/components/utils.test.ts b/src/plugins/data_source_management/public/components/utils.test.ts index a113c114310e..a94d5b2260e6 100644 --- a/src/plugins/data_source_management/public/components/utils.test.ts +++ b/src/plugins/data_source_management/public/components/utils.test.ts @@ -212,9 +212,6 @@ describe('DataSourceManagement: Utils.ts', () => { expect(isValidUrl('')).toBeFalsy(); expect(isValidUrl('test')).toBeFalsy(); - /* False cases: path name scenario*/ - expect(isValidUrl('https://test.com/_somepath')).toBeFalsy(); - /* True cases */ expect(isValidUrl('https://test.com')).toBeTruthy(); expect(isValidUrl('http://test.com')).toBeTruthy(); diff --git a/src/plugins/data_source_management/public/components/utils.ts b/src/plugins/data_source_management/public/components/utils.ts index 1ef80e3eb45c..5f2cfb2337ad 100644 --- a/src/plugins/data_source_management/public/components/utils.ts +++ b/src/plugins/data_source_management/public/components/utils.ts @@ -103,11 +103,7 @@ export async function testConnection( export const isValidUrl = (endpoint: string) => { try { const url = new URL(endpoint); - return ( - Boolean(url) && - (url.protocol === 'http:' || url.protocol === 'https:') && - (!url.pathname || url.pathname.length === 0 || url.pathname === '/') - ); + return Boolean(url) && (url.protocol === 'http:' || url.protocol === 'https:'); } catch (e) { return false; } diff --git a/src/plugins/data_source_management/public/components/validation/datasource_form_validation.test.ts b/src/plugins/data_source_management/public/components/validation/datasource_form_validation.test.ts index 50854e7cc519..9a00d0f29c91 100644 --- a/src/plugins/data_source_management/public/components/validation/datasource_form_validation.test.ts +++ b/src/plugins/data_source_management/public/components/validation/datasource_form_validation.test.ts @@ -7,11 +7,7 @@ import { AuthType } from '../../types'; import { CreateDataSourceState } from '../create_data_source_wizard/components/create_form/create_data_source_form'; import { EditDataSourceState } from '../edit_data_source/components/edit_form/edit_data_source_form'; import { defaultValidation, performDataSourceFormValidation } from './datasource_form_validation'; -import { - mockDataSourceAttributesWithAuth, - mockDataSourceAttributesWithValidEndpoint, - mockDataSourceAttributesWithInvalidEndpoint, -} from '../../mocks'; +import { mockDataSourceAttributesWithAuth } from '../../mocks'; describe('DataSourceManagement: Form Validation', () => { describe('validate create/edit datasource', () => { @@ -53,18 +49,6 @@ describe('DataSourceManagement: Form Validation', () => { const result = performDataSourceFormValidation(form, [], ''); expect(result).toBe(false); }); - test('should fail validation when endpoint path is not empty', () => { - form.endpoint = mockDataSourceAttributesWithInvalidEndpoint.endpoint; - const result = performDataSourceFormValidation(form, [], ''); - expect(result).toBe(false); - }); - test('should NOT fail validation when endpoint path is empty', () => { - form.auth.type = AuthType.NoAuth; - form.title = 'test'; - form.endpoint = mockDataSourceAttributesWithValidEndpoint.endpoint; - const result = performDataSourceFormValidation(form, [], ''); - expect(result).toBe(true); - }); test('should NOT fail validation on empty username/password when No Auth is selected', () => { form.auth.type = AuthType.NoAuth; form.title = 'test'; diff --git a/src/plugins/data_source_management/public/mocks.ts b/src/plugins/data_source_management/public/mocks.ts index d3236daae8fb..c078247956e0 100644 --- a/src/plugins/data_source_management/public/mocks.ts +++ b/src/plugins/data_source_management/public/mocks.ts @@ -131,34 +131,6 @@ export const getMappedDataSources = [ }, ]; -export const mockDataSourceAttributesWithValidEndpoint = { - id: 'test', - title: 'create-test-ds', - description: 'jest testing', - endpoint: 'https://test.com', - auth: { - type: AuthType.UsernamePasswordType, - credentials: { - username: 'test123', - password: 'test123', - }, - }, -}; - -export const mockDataSourceAttributesWithInvalidEndpoint = { - id: 'test', - title: 'create-test-ds', - description: 'jest testing', - endpoint: 'https://test.com/_somepath', - auth: { - type: AuthType.UsernamePasswordType, - credentials: { - username: 'test123', - password: 'test123', - }, - }, -}; - export const mockDataSourceAttributesWithAuth = { id: 'test', title: 'create-test-ds', From 77b9ce6b69b1eccb52a47e66042bbd77586d59f8 Mon Sep 17 00:00:00 2001 From: Xinrui Bai Date: Mon, 22 Jan 2024 20:11:35 +0000 Subject: [PATCH 5/5] Update vaclidation logic to all validate on opensearch client response body Signed-off-by: Xinrui Bai --- .../data_source_connection_validator.test.ts | 62 +++++++++++++++++-- .../data_source_connection_validator.ts | 10 +-- 2 files changed, 63 insertions(+), 9 deletions(-) diff --git a/src/plugins/data_source/server/routes/data_source_connection_validator.test.ts b/src/plugins/data_source/server/routes/data_source_connection_validator.test.ts index cfbcf4489fe6..219888199016 100644 --- a/src/plugins/data_source/server/routes/data_source_connection_validator.test.ts +++ b/src/plugins/data_source/server/routes/data_source_connection_validator.test.ts @@ -9,13 +9,40 @@ import { SigV4ServiceName } from '../../common/data_sources'; describe('DataSourceManagement: data_source_connection_validator.ts', () => { describe('Test datasource connection without SigV4 auth', () => { - test('Success: opensearch client response code is 200', async () => { + test('Success: opensearch client response code is 200 and response body have cluster name', async () => { const opensearchClient = opensearchServiceMock.createOpenSearchClient(); - opensearchClient.info.mockResolvedValue(opensearchServiceMock.createApiResponse()); + opensearchClient.info.mockResolvedValue( + opensearchServiceMock.createApiResponse({ + statusCode: 200, + body: { + cluster_name: 'This is the cluster name', + }, + }) + ); const dataSourceValidator = new DataSourceConnectionValidator(opensearchClient, {}); const validateDataSourcesResponse = await dataSourceValidator.validate(); expect(validateDataSourcesResponse.statusCode).toBe(200); }); + + test('failure: opensearch client response code is 200 but response body not have cluster name', async () => { + try { + const opensearchClient = opensearchServiceMock.createOpenSearchClient(); + opensearchClient.info.mockResolvedValue( + opensearchServiceMock.createApiResponse({ + statusCode: 200, + body: { + Message: 'Response without cluster name.', + }, + }) + ); + const dataSourceValidator = new DataSourceConnectionValidator(opensearchClient, {}); + await dataSourceValidator.validate(); + } catch (e) { + expect(e).toBeTruthy(); + expect(e.message).toContain('Response without cluster name.'); + } + }); + test('failure: opensearch client response code is other than 200', async () => { const statusCodeList = [100, 202, 300, 400, 500]; statusCodeList.forEach(async function (code) { @@ -24,7 +51,9 @@ describe('DataSourceManagement: data_source_connection_validator.ts', () => { opensearchClient.info.mockResolvedValue( opensearchServiceMock.createApiResponse({ statusCode: code, - body: { Message: 'Your request is not correct.' }, + body: { + Message: 'Your request is not correct.', + }, }) ); const dataSourceValidator = new DataSourceConnectionValidator(opensearchClient, {}); @@ -38,7 +67,7 @@ describe('DataSourceManagement: data_source_connection_validator.ts', () => { }); describe('Test datasource connection for SigV4 auth', () => { - test('Success: opensearch client response code is 200', async () => { + test('Success: opensearch client response code is 200 and response body is not empty', async () => { const opensearchClient = opensearchServiceMock.createOpenSearchClient(); opensearchClient.cat.indices.mockResolvedValue(opensearchServiceMock.createApiResponse()); const dataSourceValidator = new DataSourceConnectionValidator(opensearchClient, { @@ -51,6 +80,27 @@ describe('DataSourceManagement: data_source_connection_validator.ts', () => { const validateDataSourcesResponse = await dataSourceValidator.validate(); expect(validateDataSourcesResponse.statusCode).toBe(200); }); + + test('failure: opensearch client response code is 200 and response body is empty', async () => { + try { + const opensearchClient = opensearchServiceMock.createOpenSearchClient(); + opensearchClient.cat.indices.mockResolvedValue(opensearchServiceMock.createApiResponse()); + const dataSourceValidator = new DataSourceConnectionValidator(opensearchClient, { + auth: { + statusCode: 200, + body: '', + credentials: { + service: SigV4ServiceName.OpenSearchServerless, + }, + }, + }); + const validateDataSourcesResponse = await dataSourceValidator.validate(); + expect(validateDataSourcesResponse.statusCode).toBe(200); + } catch (e) { + expect(e).toBeTruthy(); + } + }); + test('failure: opensearch client response code is other than 200', async () => { const statusCodeList = [100, 202, 300, 400, 500]; statusCodeList.forEach(async function (code) { @@ -59,7 +109,9 @@ describe('DataSourceManagement: data_source_connection_validator.ts', () => { opensearchClient.cat.indices.mockResolvedValue( opensearchServiceMock.createApiResponse({ statusCode: code, - body: { Message: 'Your request is not correct.' }, + body: { + Message: 'Your request is not correct.', + }, }) ); const dataSourceValidator = new DataSourceConnectionValidator(opensearchClient, { diff --git a/src/plugins/data_source/server/routes/data_source_connection_validator.ts b/src/plugins/data_source/server/routes/data_source_connection_validator.ts index 03481dd56e39..735d1429414c 100644 --- a/src/plugins/data_source/server/routes/data_source_connection_validator.ts +++ b/src/plugins/data_source/server/routes/data_source_connection_validator.ts @@ -20,12 +20,14 @@ export class DataSourceConnectionValidator { this.dataSourceAttr.auth?.credentials?.service === SigV4ServiceName.OpenSearchServerless ) { validationResponse = await this.callDataCluster.cat.indices(); + if (validationResponse?.statusCode === 200 && validationResponse?.body) { + return validationResponse; + } } else { validationResponse = await this.callDataCluster.info(); - } - - if (validationResponse?.statusCode === 200) { - return validationResponse; + if (validationResponse?.statusCode === 200 && validationResponse?.body?.cluster_name) { + return validationResponse; + } } throw new Error(JSON.stringify(validationResponse?.body));