From de0f8b2861768001e80f018042ddc23089b071df Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 26 Oct 2023 00:18:03 +0100 Subject: [PATCH 01/10] initial schema changes and testing Signed-off-by: Sam --- server/auth/types/openid/openid_auth.ts | 8 +++++++- server/index.ts | 1 + 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/server/auth/types/openid/openid_auth.ts b/server/auth/types/openid/openid_auth.ts index accabb7c1..55b1d2f72 100644 --- a/server/auth/types/openid/openid_auth.ts +++ b/server/auth/types/openid/openid_auth.ts @@ -92,6 +92,10 @@ export class OpenIdAuthentication extends AuthenticationType { public async init() { try { + console.log(Object.keys(this.config.openid.additionalProperties)); + console.log(this.config.openid?.additionalProperties.potato + "TTTTTT") + console.log(this.config.openid?.additionalProperties.test + "TTTTTT") + const response = await this.wreckClient.get(this.openIdConnectUrl); const payload = JSON.parse(response.payload as string); @@ -110,7 +114,6 @@ export class OpenIdAuthentication extends AuthenticationType { this.coreSetup, this.wreckClient ); - routes.setupRoutes(); } catch (error: any) { this.logger.error(error); // TODO: log more info @@ -180,6 +183,9 @@ export class OpenIdAuthentication extends AuthenticationType { } async getAdditionalAuthHeader(request: OpenSearchDashboardsRequest): Promise { + console.log(this.config.openid?.additionalProperties.potato + "TTTTTT") + console.log(this.config.openid?.additionalProperties.test + "TTTTTT") + Object.keys(this.config.openid.additionalProperties) return {}; } diff --git a/server/index.ts b/server/index.ts index b4384315a..128bc53f1 100644 --- a/server/index.ts +++ b/server/index.ts @@ -180,6 +180,7 @@ export const configSchema = schema.object({ verify_hostnames: schema.boolean({ defaultValue: true }), refresh_tokens: schema.boolean({ defaultValue: true }), trust_dynamic_headers: schema.boolean({ defaultValue: false }), + additionalProperties: schema.maybe(schema.any()), extra_storage: schema.object({ cookie_prefix: schema.string({ defaultValue: 'security_authentication_oidc', From 7b9050d2776d09fe23035d3bce85acfada2a76c8 Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 2 Nov 2023 02:53:31 +0000 Subject: [PATCH 02/10] adding parameters to query Signed-off-by: Sam --- server/auth/types/openid/openid_auth.ts | 7 ------- server/auth/types/openid/routes.ts | 16 ++++++++++++++++ server/index.ts | 2 +- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/server/auth/types/openid/openid_auth.ts b/server/auth/types/openid/openid_auth.ts index 55b1d2f72..42bfc7f77 100644 --- a/server/auth/types/openid/openid_auth.ts +++ b/server/auth/types/openid/openid_auth.ts @@ -92,10 +92,6 @@ export class OpenIdAuthentication extends AuthenticationType { public async init() { try { - console.log(Object.keys(this.config.openid.additionalProperties)); - console.log(this.config.openid?.additionalProperties.potato + "TTTTTT") - console.log(this.config.openid?.additionalProperties.test + "TTTTTT") - const response = await this.wreckClient.get(this.openIdConnectUrl); const payload = JSON.parse(response.payload as string); @@ -183,9 +179,6 @@ export class OpenIdAuthentication extends AuthenticationType { } async getAdditionalAuthHeader(request: OpenSearchDashboardsRequest): Promise { - console.log(this.config.openid?.additionalProperties.potato + "TTTTTT") - console.log(this.config.openid?.additionalProperties.test + "TTTTTT") - Object.keys(this.config.openid.additionalProperties) return {}; } diff --git a/server/auth/types/openid/routes.ts b/server/auth/types/openid/routes.ts index b598dd1a8..65fa51c1d 100644 --- a/server/auth/types/openid/routes.ts +++ b/server/auth/types/openid/routes.ts @@ -127,7 +127,15 @@ export class OpenIdAuthRoutes { state: nonce, scope: this.openIdAuthConfig.scope, }; + if (this.config.openid?.additional_properties) { + for (const [key, value] of Object.entries(this.config.openid?.additional_properties)) { + console.log(`${key}: ${value}`); + query[key] = value; + } + } const queryString = stringify(query); + console.log('query: ' + queryString); + const location = `${this.openIdAuthConfig.authorizationEndpoint}?${queryString}`; const cookie: SecuritySessionCookie = { oidc: { @@ -173,6 +181,14 @@ export class OpenIdAuthRoutes { client_id: clientId, client_secret: clientSecret, }; + if (this.config.openid?.additional_properties) { + for (const [key, value] of Object.entries(this.config.openid?.additional_properties)) { + console.log(`${key}: ${value}`); + query[key] = value; + } + } + const queryString = stringify(query); + console.log('query2: ' + queryString); try { const tokenResponse = await callTokenEndpoint( diff --git a/server/index.ts b/server/index.ts index 128bc53f1..611117918 100644 --- a/server/index.ts +++ b/server/index.ts @@ -180,7 +180,7 @@ export const configSchema = schema.object({ verify_hostnames: schema.boolean({ defaultValue: true }), refresh_tokens: schema.boolean({ defaultValue: true }), trust_dynamic_headers: schema.boolean({ defaultValue: false }), - additionalProperties: schema.maybe(schema.any()), + additional_properties: schema.maybe(schema.any({ defaultValue: null })), extra_storage: schema.object({ cookie_prefix: schema.string({ defaultValue: 'security_authentication_oidc', From 4140ce9baf8cbdcd27930803b8a281c4e12022e7 Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 3 Nov 2023 11:59:42 +0000 Subject: [PATCH 03/10] name refactor Signed-off-by: Sam --- server/auth/types/openid/routes.ts | 8 ++++---- server/index.ts | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/server/auth/types/openid/routes.ts b/server/auth/types/openid/routes.ts index 65fa51c1d..e6971cb02 100644 --- a/server/auth/types/openid/routes.ts +++ b/server/auth/types/openid/routes.ts @@ -127,8 +127,8 @@ export class OpenIdAuthRoutes { state: nonce, scope: this.openIdAuthConfig.scope, }; - if (this.config.openid?.additional_properties) { - for (const [key, value] of Object.entries(this.config.openid?.additional_properties)) { + if (this.config.openid?.additional_parameters) { + for (const [key, value] of Object.entries(this.config.openid?.additional_parameters)) { console.log(`${key}: ${value}`); query[key] = value; } @@ -181,8 +181,8 @@ export class OpenIdAuthRoutes { client_id: clientId, client_secret: clientSecret, }; - if (this.config.openid?.additional_properties) { - for (const [key, value] of Object.entries(this.config.openid?.additional_properties)) { + if (this.config.openid?.additional_parameters) { + for (const [key, value] of Object.entries(this.config.openid?.additional_parameters)) { console.log(`${key}: ${value}`); query[key] = value; } diff --git a/server/index.ts b/server/index.ts index 611117918..1b85d5867 100644 --- a/server/index.ts +++ b/server/index.ts @@ -180,7 +180,7 @@ export const configSchema = schema.object({ verify_hostnames: schema.boolean({ defaultValue: true }), refresh_tokens: schema.boolean({ defaultValue: true }), trust_dynamic_headers: schema.boolean({ defaultValue: false }), - additional_properties: schema.maybe(schema.any({ defaultValue: null })), + additional_parameters: schema.maybe(schema.any({ defaultValue: null })), extra_storage: schema.object({ cookie_prefix: schema.string({ defaultValue: 'security_authentication_oidc', From fc168d0415c94a75ec100f6866f879836a5ff4d8 Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 9 Nov 2023 14:41:44 +0000 Subject: [PATCH 04/10] Query Value replacement warning Signed-off-by: Sam --- server/auth/types/openid/routes.ts | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/server/auth/types/openid/routes.ts b/server/auth/types/openid/routes.ts index e6971cb02..ec5c56b6c 100644 --- a/server/auth/types/openid/routes.ts +++ b/server/auth/types/openid/routes.ts @@ -62,7 +62,7 @@ export class OpenIdAuthRoutes { private readonly openIdAuthConfig: OpenIdAuthConfig, private readonly securityClient: SecurityClient, private readonly core: CoreSetup, - private readonly wreckClient: typeof wreck + private readonly wreckClient: typeof wreck, ) {} private redirectToLogin( @@ -129,13 +129,15 @@ export class OpenIdAuthRoutes { }; if (this.config.openid?.additional_parameters) { for (const [key, value] of Object.entries(this.config.openid?.additional_parameters)) { - console.log(`${key}: ${value}`); + if (query[key] != null) { + context.security_plugin.logger.warn( + `OpenID config '${key}' is being overwritten to '${value}' by additional parameters` + ); + } query[key] = value; } } const queryString = stringify(query); - console.log('query: ' + queryString); - const location = `${this.openIdAuthConfig.authorizationEndpoint}?${queryString}`; const cookie: SecuritySessionCookie = { oidc: { @@ -183,13 +185,14 @@ export class OpenIdAuthRoutes { }; if (this.config.openid?.additional_parameters) { for (const [key, value] of Object.entries(this.config.openid?.additional_parameters)) { - console.log(`${key}: ${value}`); + if (query[key] != null) { + context.security_plugin.logger.warn( + `OpenID config '${key}' is being overwritten to '${value}' by additional parameters` + ); + } query[key] = value; } } - const queryString = stringify(query); - console.log('query2: ' + queryString); - try { const tokenResponse = await callTokenEndpoint( this.openIdAuthConfig.tokenEndpoint!, From 2f888bf7a733c4190ce043aa2583d7e60975a5f0 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 13 Nov 2023 13:15:24 +0000 Subject: [PATCH 05/10] Blocking Query Value replacement Signed-off-by: Sam --- server/auth/types/openid/routes.ts | 38 +++++++++++++----------------- 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/server/auth/types/openid/routes.ts b/server/auth/types/openid/routes.ts index ec5c56b6c..22f3684b2 100644 --- a/server/auth/types/openid/routes.ts +++ b/server/auth/types/openid/routes.ts @@ -62,7 +62,7 @@ export class OpenIdAuthRoutes { private readonly openIdAuthConfig: OpenIdAuthConfig, private readonly securityClient: SecurityClient, private readonly core: CoreSetup, - private readonly wreckClient: typeof wreck, + private readonly wreckClient: typeof wreck ) {} private redirectToLogin( @@ -127,16 +127,7 @@ export class OpenIdAuthRoutes { state: nonce, scope: this.openIdAuthConfig.scope, }; - if (this.config.openid?.additional_parameters) { - for (const [key, value] of Object.entries(this.config.openid?.additional_parameters)) { - if (query[key] != null) { - context.security_plugin.logger.warn( - `OpenID config '${key}' is being overwritten to '${value}' by additional parameters` - ); - } - query[key] = value; - } - } + this.includeAdditionalParameters(query, context); const queryString = stringify(query); const location = `${this.openIdAuthConfig.authorizationEndpoint}?${queryString}`; const cookie: SecuritySessionCookie = { @@ -183,16 +174,7 @@ export class OpenIdAuthRoutes { client_id: clientId, client_secret: clientSecret, }; - if (this.config.openid?.additional_parameters) { - for (const [key, value] of Object.entries(this.config.openid?.additional_parameters)) { - if (query[key] != null) { - context.security_plugin.logger.warn( - `OpenID config '${key}' is being overwritten to '${value}' by additional parameters` - ); - } - query[key] = value; - } - } + this.includeAdditionalParameters(query, context); try { const tokenResponse = await callTokenEndpoint( this.openIdAuthConfig.tokenEndpoint!, @@ -289,4 +271,18 @@ export class OpenIdAuthRoutes { } ); } + + private includeAdditionalParameters(query: any, context) { + if (this.config.openid?.additional_parameters) { + for (const [key, value] of Object.entries(this.config.openid?.additional_parameters)) { + if (query[key] == null) { + query[key] = value; + } else { + context.security_plugin.logger.warn( + `Additional parameter in OpenID config '${key}' was ignored as it would overwrite existing parameters` + ); + } + } + } + } } From 2835de13cbed57e73d4afb2ccb554e6d959d8c6d Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 14 Nov 2023 12:19:02 +0000 Subject: [PATCH 06/10] Just getting around 'unresolved import' lint error to see if tests fail Signed-off-by: Sam --- .eslintrc.js | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 2b5f80b11..ef5cdf316 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -15,8 +15,8 @@ const LICENSE_HEADER = ` */ ` -module.exports = { - root: true, +module.exports = { + root: true, extends: ['@elastic/eslint-config-kibana', 'plugin:@elastic/eui/recommended'], rules: { // "@osd/eslint/require-license-header": "off" @@ -44,7 +44,14 @@ module.exports = { }, ], "no-console": 0 - } - } + }, + + }, + { + files: ['./babel.config.js'], + rules: { + 'import/no-unresolved': 'warn', + }, + }, ], -}; \ No newline at end of file +}; From 2cdfc69e8befe9bc2bc030405c72773dcb46404a Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 14 Nov 2023 15:34:06 +0000 Subject: [PATCH 07/10] Moved function to helper class and included unit test Signed-off-by: Sam --- package.json | 2 +- server/auth/types/openid/helper.test.ts | 32 ++++++++++++++++++++++++- server/auth/types/openid/helper.ts | 14 +++++++++++ server/auth/types/openid/openid_auth.ts | 1 + server/auth/types/openid/routes.ts | 19 +++------------ 5 files changed, 50 insertions(+), 18 deletions(-) diff --git a/package.json b/package.json index 6ae13f2c1..e3993e5a5 100644 --- a/package.json +++ b/package.json @@ -39,4 +39,4 @@ "resolutions": { "selenium-webdriver": "4.10.0" } -} \ No newline at end of file +} diff --git a/server/auth/types/openid/helper.test.ts b/server/auth/types/openid/helper.test.ts index 3a125a2bf..edff05d1c 100644 --- a/server/auth/types/openid/helper.test.ts +++ b/server/auth/types/openid/helper.test.ts @@ -13,7 +13,13 @@ * permissions and limitations under the License. */ -import { composeLogoutUrl, getExpirationDate, getRootUrl, getNextUrl } from './helper'; +import { + composeLogoutUrl, + getExpirationDate, + getRootUrl, + getNextUrl, + includeAdditionalParameters, +} from './helper'; describe('test OIDC helper utility', () => { test('test compose logout url', () => { @@ -56,6 +62,30 @@ describe('test OIDC helper utility', () => { ); }); + test('test include additional parameters in query', () => { + const contextMock = { security_plugin: { logger: { warn: jest.fn() } } }; + + const query = { existingKey: 'value' }; + const constQueryModified = { existingKey: 'value', ping: 'pong', acr_values: '1' }; + const config = { + openid: { + enabled: false, + additional_parameters: { + ping: 'pong', + acr_values: '1', + existingKey: 'foobar', + }, + }, + }; + + includeAdditionalParameters(query, contextMock, config); + expect(query).toEqual(constQueryModified); + expect(contextMock.security_plugin.logger.warn).toHaveBeenCalledTimes(1); + expect(contextMock.security_plugin.logger.warn).toHaveBeenCalledWith( + "Additional parameter in OpenID config 'existingKey' was ignored as it would overwrite existing parameters" + ); + }); + test('test root url when trusted header unset', () => { const config = { openid: { diff --git a/server/auth/types/openid/helper.ts b/server/auth/types/openid/helper.ts index 9839175ca..dcea8c7a2 100644 --- a/server/auth/types/openid/helper.ts +++ b/server/auth/types/openid/helper.ts @@ -144,3 +144,17 @@ export function getExpirationDate(tokenResponse: TokenResponse | undefined) { return Date.now() + tokenResponse.expiresIn! * 1000; } } + +export function includeAdditionalParameters(query: any, context, config) { + if (config.openid?.additional_parameters) { + for (const [key, value] of Object.entries(config.openid?.additional_parameters)) { + if (query[key] == null) { + query[key] = value; + } else { + context.security_plugin.logger.warn( + `Additional parameter in OpenID config '${key}' was ignored as it would overwrite existing parameters` + ); + } + } + } +} diff --git a/server/auth/types/openid/openid_auth.ts b/server/auth/types/openid/openid_auth.ts index 42bfc7f77..accabb7c1 100644 --- a/server/auth/types/openid/openid_auth.ts +++ b/server/auth/types/openid/openid_auth.ts @@ -110,6 +110,7 @@ export class OpenIdAuthentication extends AuthenticationType { this.coreSetup, this.wreckClient ); + routes.setupRoutes(); } catch (error: any) { this.logger.error(error); // TODO: log more info diff --git a/server/auth/types/openid/routes.ts b/server/auth/types/openid/routes.ts index 22f3684b2..46c0d6c88 100644 --- a/server/auth/types/openid/routes.ts +++ b/server/auth/types/openid/routes.ts @@ -34,6 +34,7 @@ import { composeLogoutUrl, getNextUrl, getExpirationDate, + includeAdditionalParameters, } from './helper'; import { validateNextUrl } from '../../../utils/next_url'; import { @@ -127,7 +128,7 @@ export class OpenIdAuthRoutes { state: nonce, scope: this.openIdAuthConfig.scope, }; - this.includeAdditionalParameters(query, context); + includeAdditionalParameters(query, context, this.config); const queryString = stringify(query); const location = `${this.openIdAuthConfig.authorizationEndpoint}?${queryString}`; const cookie: SecuritySessionCookie = { @@ -174,7 +175,7 @@ export class OpenIdAuthRoutes { client_id: clientId, client_secret: clientSecret, }; - this.includeAdditionalParameters(query, context); + includeAdditionalParameters(query, context, this.config); try { const tokenResponse = await callTokenEndpoint( this.openIdAuthConfig.tokenEndpoint!, @@ -271,18 +272,4 @@ export class OpenIdAuthRoutes { } ); } - - private includeAdditionalParameters(query: any, context) { - if (this.config.openid?.additional_parameters) { - for (const [key, value] of Object.entries(this.config.openid?.additional_parameters)) { - if (query[key] == null) { - query[key] = value; - } else { - context.security_plugin.logger.warn( - `Additional parameter in OpenID config '${key}' was ignored as it would overwrite existing parameters` - ); - } - } - } - } } From 8f4c6a6c712415b318359ec7081d9c6033c8cfac Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 14 Nov 2023 16:24:46 +0000 Subject: [PATCH 08/10] babel import from main Signed-off-by: Sam --- .eslintrc.js | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index ef5cdf316..7b7a374be 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -44,14 +44,7 @@ module.exports = { }, ], "no-console": 0 - }, - - }, - { - files: ['./babel.config.js'], - rules: { - 'import/no-unresolved': 'warn', - }, - }, + } + } ], }; From d8d8d84943b6d0046098d6f833aac956f968b9cb Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 17 Nov 2023 11:28:53 +0000 Subject: [PATCH 09/10] additional unit test Signed-off-by: Sam --- server/auth/types/openid/helper.test.ts | 20 +++++++++++++++++++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/server/auth/types/openid/helper.test.ts b/server/auth/types/openid/helper.test.ts index edff05d1c..f5cce4af0 100644 --- a/server/auth/types/openid/helper.test.ts +++ b/server/auth/types/openid/helper.test.ts @@ -69,7 +69,8 @@ describe('test OIDC helper utility', () => { const constQueryModified = { existingKey: 'value', ping: 'pong', acr_values: '1' }; const config = { openid: { - enabled: false, + enabled: true, + client_secret: '', additional_parameters: { ping: 'pong', acr_values: '1', @@ -86,6 +87,23 @@ describe('test OIDC helper utility', () => { ); }); + test('test include additional parameters in query when no additional parameters specified', () => { + const contextMock = { security_plugin: { logger: { warn: jest.fn() } } }; + + const query = { existingKey: 'value' }; + const constQueryModified = { existingKey: 'value' }; + const config = { + openid: { + enabled: true, + client_secret: '', + }, + }; + + includeAdditionalParameters(query, contextMock, config); + expect(query).toEqual(constQueryModified); + expect(contextMock.security_plugin.logger.warn).toHaveBeenCalledTimes(0); + }); + test('test root url when trusted header unset', () => { const config = { openid: { From ae6cfeb5af19cb6d083a541469c35508c587fe6f Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 17 Nov 2023 11:35:21 +0000 Subject: [PATCH 10/10] reverting lint changes to eslintrc.js Signed-off-by: Sam --- .eslintrc.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.eslintrc.js b/.eslintrc.js index 7b7a374be..2b5f80b11 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -15,8 +15,8 @@ const LICENSE_HEADER = ` */ ` -module.exports = { - root: true, +module.exports = { + root: true, extends: ['@elastic/eslint-config-kibana', 'plugin:@elastic/eui/recommended'], rules: { // "@osd/eslint/require-license-header": "off" @@ -47,4 +47,4 @@ module.exports = { } } ], -}; +}; \ No newline at end of file