From 2b88f7b4347c7e38055720d18c5f2978c5b108cc Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Tue, 8 Oct 2019 11:02:29 +0200 Subject: [PATCH] Use `serverBasePath` provided by the core instead of the one provided via legacy shim. --- x-pack/legacy/plugins/security/index.js | 1 - .../server/authentication/providers/saml.test.ts | 6 +++--- .../server/authentication/providers/saml.ts | 9 ++++----- x-pack/plugins/security/server/plugin.ts | 1 - .../server/routes/authentication.test.ts | 6 ++---- .../security/server/routes/authentication.ts | 16 ++++++++++------ 6 files changed, 19 insertions(+), 20 deletions(-) diff --git a/x-pack/legacy/plugins/security/index.js b/x-pack/legacy/plugins/security/index.js index b396ef6cabe2f7..980af19cc8362b 100644 --- a/x-pack/legacy/plugins/security/index.js +++ b/x-pack/legacy/plugins/security/index.js @@ -138,7 +138,6 @@ export const security = (kibana) => new kibana.Plugin({ server.plugins.kibana.systemApi ), cspRules: createCSPRuleString(config.get('csp.rules')), - serverBasePath: config.get('server.basePath'), }); const plugin = this; diff --git a/x-pack/plugins/security/server/authentication/providers/saml.test.ts b/x-pack/plugins/security/server/authentication/providers/saml.test.ts index 732472230cfe9f..1fc49d5d344840 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.test.ts @@ -527,7 +527,7 @@ describe('SAMLAuthenticationProvider', () => { expect(authenticationResult.redirected()).toBe(true); expect(authenticationResult.redirectURL).toBe( - '/base-path/api/security/saml/capture-url-fragment' + '/mock-server-basepath/api/security/saml/capture-url-fragment' ); expect(authenticationResult.state).toEqual({ redirectURL: '/base-path/s/foo/some-path' }); }); @@ -746,7 +746,7 @@ describe('SAMLAuthenticationProvider', () => { expect(authenticationResult.redirected()).toBe(true); expect(authenticationResult.redirectURL).toBe( - '/base-path/api/security/saml/capture-url-fragment' + '/mock-server-basepath/api/security/saml/capture-url-fragment' ); expect(authenticationResult.state).toEqual({ redirectURL: '/base-path/s/foo/some-path' }); }); @@ -819,7 +819,7 @@ describe('SAMLAuthenticationProvider', () => { expect(authenticationResult.redirected()).toBe(true); expect(authenticationResult.redirectURL).toBe( - '/base-path/api/security/saml/capture-url-fragment' + '/mock-server-basepath/api/security/saml/capture-url-fragment' ); expect(authenticationResult.state).toEqual({ redirectURL: '/base-path/s/foo/some-path' }); }); diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index 87e1a11591a35a..712ddf3fb62109 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -571,10 +571,9 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { return this.authenticateViaHandshake(request, ''); } - // We should use `serverBasePath` here as soon as it's provided by the core. Using request scoped - // base path doesn't hurt, but isn't consistent with the rest of API routes. - return AuthenticationResult.redirectTo(`${basePath}/api/security/saml/capture-url-fragment`, { - state: { redirectURL }, - }); + return AuthenticationResult.redirectTo( + `${this.options.basePath.serverBasePath}/api/security/saml/capture-url-fragment`, + { state: { redirectURL } } + ); } } diff --git a/x-pack/plugins/security/server/plugin.ts b/x-pack/plugins/security/server/plugin.ts index dd076ffdc6c32f..18717f3e132b9f 100644 --- a/x-pack/plugins/security/server/plugin.ts +++ b/x-pack/plugins/security/server/plugin.ts @@ -27,7 +27,6 @@ export interface LegacyAPI { xpackInfo: Pick; isSystemAPIRequest: (request: KibanaRequest) => boolean; cspRules: string; - serverBasePath?: string; } /** diff --git a/x-pack/plugins/security/server/routes/authentication.test.ts b/x-pack/plugins/security/server/routes/authentication.test.ts index c7d35565a6fcb6..67fd86587a0ce5 100644 --- a/x-pack/plugins/security/server/routes/authentication.test.ts +++ b/x-pack/plugins/security/server/routes/authentication.test.ts @@ -31,8 +31,7 @@ describe('SAML authentication routes', () => { logger: loggingServiceMock.create().get(), config: { authc: { providers: ['saml'] } } as ConfigType, authc, - getLegacyAPI: () => - ({ cspRules: 'test-csp-rule', serverBasePath: '/test-base-path' } as LegacyAPI), + getLegacyAPI: () => ({ cspRules: 'test-csp-rule' } as LegacyAPI), }); }); @@ -44,8 +43,7 @@ describe('SAML authentication routes', () => { logger: loggingServiceMock.create().get(), config: { authc: { providers: ['basic'] } } as ConfigType, authc: authenticationMock.create(), - getLegacyAPI: () => - ({ cspRules: 'test-csp-rule', serverBasePath: '/test-base-path' } as LegacyAPI), + getLegacyAPI: () => ({ cspRules: 'test-csp-rule' } as LegacyAPI), }); const samlRoutePathPredicate = ([{ path }]: [{ path: string }, any]) => diff --git a/x-pack/plugins/security/server/routes/authentication.ts b/x-pack/plugins/security/server/routes/authentication.ts index 049cab811edff6..35a63e87339c42 100644 --- a/x-pack/plugins/security/server/routes/authentication.ts +++ b/x-pack/plugins/security/server/routes/authentication.ts @@ -17,7 +17,13 @@ export function defineAuthenticationRoutes(params: RouteDefinitionParams) { /** * Defines routes required for SAML authentication. */ -function defineSAMLRoutes({ router, logger, authc, getLegacyAPI }: RouteDefinitionParams) { +function defineSAMLRoutes({ + router, + logger, + authc, + getLegacyAPI, + basePath, +}: RouteDefinitionParams) { function createCustomResourceResponse(body: string, contentType: string) { return { body, @@ -37,7 +43,6 @@ function defineSAMLRoutes({ router, logger, authc, getLegacyAPI }: RouteDefiniti options: { authRequired: false }, }, (context, request, response) => { - const serverBasePath = getLegacyAPI().serverBasePath || ''; // We're also preventing `favicon.ico` request since it can cause new SAML handshake. return response.custom( createCustomResourceResponse( @@ -45,7 +50,7 @@ function defineSAMLRoutes({ router, logger, authc, getLegacyAPI }: RouteDefiniti Kibana SAML Login - + `, 'text/html' ) @@ -60,12 +65,11 @@ function defineSAMLRoutes({ router, logger, authc, getLegacyAPI }: RouteDefiniti options: { authRequired: false }, }, (context, request, response) => { - const serverBasePath = getLegacyAPI().serverBasePath || ''; return response.custom( createCustomResourceResponse( ` window.location.replace( - '${serverBasePath}/api/security/saml/start?redirectURLFragment=' + encodeURIComponent(window.location.hash) + '${basePath.serverBasePath}/api/security/saml/start?redirectURLFragment=' + encodeURIComponent(window.location.hash) ); `, 'text/javascript' @@ -120,7 +124,7 @@ function defineSAMLRoutes({ router, logger, authc, getLegacyAPI }: RouteDefiniti async (context, request, response) => { try { if (path === '/api/security/v1/saml') { - const serverBasePath = getLegacyAPI().serverBasePath || ''; + const serverBasePath = basePath.serverBasePath; logger.warn( `The "${serverBasePath}${path}" URL is deprecated and will stop working in the next major version, please use "${serverBasePath}/api/security/saml/callback" URL instead.`, { tags: ['deprecation'] }