From 74fd765529babdbc75a0b70a2951bfebaa89e4d3 Mon Sep 17 00:00:00 2001 From: Brandon Kobel Date: Mon, 25 Nov 2019 15:29:36 -0500 Subject: [PATCH] Using the server's basePath when building the SAML ACS (#51391) * Using the server's basePath when building the SAML ACS * Fixing getACS call --- .../authentication/providers/saml.test.ts | 48 ++++++++++++++----- .../server/authentication/providers/saml.ts | 12 ++--- 2 files changed, 42 insertions(+), 18 deletions(-) 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 10cdf198300427..320ee8776f370a 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.test.ts @@ -537,7 +537,11 @@ describe('SAMLAuthenticationProvider', () => { sinon.assert.calledWithExactly( mockOptions.client.callAsInternalUser, 'shield.samlPrepare', - { body: { acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml' } } + { + body: { + acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml', + }, + } ); expect(mockOptions.logger.warn).not.toHaveBeenCalled(); @@ -572,7 +576,11 @@ describe('SAMLAuthenticationProvider', () => { sinon.assert.calledWithExactly( mockOptions.client.callAsInternalUser, 'shield.samlPrepare', - { body: { acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml' } } + { + body: { + acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml', + }, + } ); expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1); @@ -610,7 +618,11 @@ describe('SAMLAuthenticationProvider', () => { sinon.assert.calledWithExactly( mockOptions.client.callAsInternalUser, 'shield.samlPrepare', - { body: { acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml' } } + { + body: { + acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml', + }, + } ); expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1); @@ -686,7 +698,11 @@ describe('SAMLAuthenticationProvider', () => { sinon.assert.calledWithExactly( mockOptions.client.callAsInternalUser, 'shield.samlPrepare', - { body: { acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml' } } + { + body: { + acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml', + }, + } ); expect(authenticationResult.failed()).toBe(true); @@ -752,7 +768,9 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.authenticate(request); sinon.assert.calledWithExactly(mockOptions.client.callAsInternalUser, 'shield.samlPrepare', { - body: { acs: `test-protocol://test-hostname:1234/base-path/api/security/v1/saml` }, + body: { + acs: `test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml`, + }, }); expect(authenticationResult.redirected()).toBe(true); @@ -773,7 +791,9 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.authenticate(request, null); sinon.assert.calledWithExactly(mockOptions.client.callAsInternalUser, 'shield.samlPrepare', { - body: { acs: `test-protocol://test-hostname:1234/base-path/api/security/v1/saml` }, + body: { + acs: `test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml`, + }, }); expect(authenticationResult.failed()).toBe(true); @@ -983,7 +1003,9 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.authenticate(request, state); sinon.assert.calledWithExactly(mockOptions.client.callAsInternalUser, 'shield.samlPrepare', { - body: { acs: `test-protocol://test-hostname:1234/base-path/api/security/v1/saml` }, + body: { + acs: `test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml`, + }, }); expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1); @@ -1053,7 +1075,9 @@ describe('SAMLAuthenticationProvider', () => { const authenticationResult = await provider.authenticate(request, state); sinon.assert.calledWithExactly(mockOptions.client.callAsInternalUser, 'shield.samlPrepare', { - body: { acs: `test-protocol://test-hostname:1234/base-path/api/security/v1/saml` }, + body: { + acs: `test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml`, + }, }); expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1); @@ -1184,7 +1208,7 @@ describe('SAMLAuthenticationProvider', () => { { body: { queryString: 'SAMLRequest=xxx%20yyy', - acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml', + acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml', }, } ); @@ -1287,7 +1311,7 @@ describe('SAMLAuthenticationProvider', () => { { body: { queryString: 'SAMLRequest=xxx%20yyy', - acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml', + acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml', }, } ); @@ -1312,7 +1336,7 @@ describe('SAMLAuthenticationProvider', () => { { body: { queryString: 'SAMLRequest=xxx%20yyy', - acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml', + acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml', }, } ); @@ -1364,7 +1388,7 @@ describe('SAMLAuthenticationProvider', () => { { body: { queryString: 'SAMLRequest=xxx%20yyy', - acs: 'test-protocol://test-hostname:1234/base-path/api/security/v1/saml', + acs: 'test-protocol://test-hostname:1234/mock-server-basepath/api/security/v1/saml', }, } ); diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index 30dfcc193b67d5..1bb4b39484e697 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -525,7 +525,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { try { // Prefer realm name if it's specified, otherwise fallback to ACS. - const preparePayload = this.realm ? { realm: this.realm } : { acs: this.getACS(request) }; + const preparePayload = this.realm ? { realm: this.realm } : { acs: this.getACS() }; // This operation should be performed on behalf of the user with a privilege that normal // user usually doesn't have `cluster:admin/xpack/security/saml/prepare`. @@ -572,7 +572,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { this.logger.debug('Single logout has been initiated by the Identity Provider.'); // Prefer realm name if it's specified, otherwise fallback to ACS. - const invalidatePayload = this.realm ? { realm: this.realm } : { acs: this.getACS(request) }; + const invalidatePayload = this.realm ? { realm: this.realm } : { acs: this.getACS() }; // This operation should be performed on behalf of the user with a privilege that normal // user usually doesn't have `cluster:admin/xpack/security/saml/invalidate`. @@ -592,10 +592,10 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { /** * Constructs and returns Kibana's Assertion consumer service URL. */ - private getACS(request: KibanaRequest) { - return `${this.options.getServerBaseURL()}${this.options.basePath.get( - request - )}/api/security/v1/saml`; + private getACS() { + return `${this.options.getServerBaseURL()}${ + this.options.basePath.serverBasePath + }/api/security/v1/saml`; } /**