From f5bdc104f6f5b7384eec129e9f149327134cdc18 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Tue, 23 Jun 2020 13:32:07 +0200 Subject: [PATCH 1/2] Redirect to Logged Out UI on SAML Logout Response. Prefer Login Selector UI to Logged Out UI whenever possible. --- .../authentication/authenticator.test.ts | 27 ++++++++++ .../server/authentication/authenticator.ts | 5 ++ .../authentication/providers/base.mock.ts | 8 +-- .../server/authentication/providers/base.ts | 3 ++ .../authentication/providers/basic.test.ts | 8 +-- .../authentication/providers/kerberos.test.ts | 4 +- .../authentication/providers/kerberos.ts | 4 +- .../authentication/providers/oidc.test.ts | 10 ++-- .../server/authentication/providers/oidc.ts | 4 +- .../authentication/providers/pki.test.ts | 4 +- .../server/authentication/providers/pki.ts | 4 +- .../authentication/providers/saml.test.ts | 54 ++++++++++--------- .../server/authentication/providers/saml.ts | 46 +++++++++++----- .../authentication/providers/token.test.ts | 13 ++--- .../server/routes/authentication/common.ts | 2 +- 15 files changed, 124 insertions(+), 72 deletions(-) diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts index 60d0521a2947e..81bc61f72aca9 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.test.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.test.ts @@ -112,6 +112,33 @@ describe('Authenticator', () => { ).toThrowError('Provider name "__http__" is reserved.'); }); + it('properly set `loggedOut` URL.', () => { + const basicAuthenticationProviderMock = jest.requireMock('./providers/basic') + .BasicAuthenticationProvider; + + basicAuthenticationProviderMock.mockClear(); + new Authenticator(getMockOptions()); + expect(basicAuthenticationProviderMock).toHaveBeenCalledWith( + expect.objectContaining({ + urls: { + loggedOut: '/mock-server-basepath/security/logged_out', + }, + }), + expect.anything() + ); + + basicAuthenticationProviderMock.mockClear(); + new Authenticator(getMockOptions({ selector: { enabled: true } })); + expect(basicAuthenticationProviderMock).toHaveBeenCalledWith( + expect.objectContaining({ + urls: { + loggedOut: `/mock-server-basepath/login?msg=LOGGED_OUT`, + }, + }), + expect.anything() + ); + }); + describe('HTTP authentication provider', () => { beforeEach(() => { jest diff --git a/x-pack/plugins/security/server/authentication/authenticator.ts b/x-pack/plugins/security/server/authentication/authenticator.ts index ac5c2a72b9667..70f4063878aa8 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.ts @@ -242,6 +242,11 @@ export class Authenticator { client: this.options.clusterClient, logger: this.options.loggers.get('tokens'), }), + urls: { + loggedOut: options.config.authc.selector.enabled + ? `${options.basePath.serverBasePath}/login?msg=LOGGED_OUT` + : `${options.basePath.serverBasePath}/security/logged_out`, + }, }; this.providers = new Map( diff --git a/x-pack/plugins/security/server/authentication/providers/base.mock.ts b/x-pack/plugins/security/server/authentication/providers/base.mock.ts index 1dcd2885f66dc..283f43fe977b1 100644 --- a/x-pack/plugins/security/server/authentication/providers/base.mock.ts +++ b/x-pack/plugins/security/server/authentication/providers/base.mock.ts @@ -15,14 +15,14 @@ export type MockAuthenticationProviderOptions = ReturnType< >; export function mockAuthenticationProviderOptions(options?: { name: string }) { - const basePath = httpServiceMock.createSetupContract().basePath; - basePath.get.mockReturnValue('/base-path'); - return { client: elasticsearchServiceMock.createClusterClient(), logger: loggingServiceMock.create().get(), - basePath, + basePath: httpServiceMock.createBasePath(), tokens: { refresh: jest.fn(), invalidate: jest.fn() }, name: options?.name ?? 'basic1', + urls: { + loggedOut: '/mock-server-basepath/security/logged_out', + }, }; } diff --git a/x-pack/plugins/security/server/authentication/providers/base.ts b/x-pack/plugins/security/server/authentication/providers/base.ts index d2d2e82951a3e..32ea41802d31b 100644 --- a/x-pack/plugins/security/server/authentication/providers/base.ts +++ b/x-pack/plugins/security/server/authentication/providers/base.ts @@ -26,6 +26,9 @@ export interface AuthenticationProviderOptions { client: IClusterClient; logger: Logger; tokens: PublicMethodsOf; + urls: { + loggedOut: string; + }; } /** diff --git a/x-pack/plugins/security/server/authentication/providers/basic.test.ts b/x-pack/plugins/security/server/authentication/providers/basic.test.ts index 97ca4e46d3eb5..ee6a12e36df05 100644 --- a/x-pack/plugins/security/server/authentication/providers/basic.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/basic.test.ts @@ -107,7 +107,7 @@ describe('BasicAuthenticationProvider', () => { ) ).resolves.toEqual( AuthenticationResult.redirectTo( - '/base-path/login?next=%2Fbase-path%2Fs%2Ffoo%2Fsome-path%20%23%20that%20needs%20to%20be%20encoded' + '/mock-server-basepath/login?next=%2Fmock-server-basepath%2Fs%2Ffoo%2Fsome-path%20%23%20that%20needs%20to%20be%20encoded' ) ); }); @@ -186,7 +186,7 @@ describe('BasicAuthenticationProvider', () => { it('always redirects to the login page.', async () => { await expect(provider.logout(httpServerMock.createKibanaRequest(), {})).resolves.toEqual( - DeauthenticationResult.redirectTo('/base-path/login?msg=LOGGED_OUT') + DeauthenticationResult.redirectTo('/mock-server-basepath/login?msg=LOGGED_OUT') ); }); @@ -199,7 +199,9 @@ describe('BasicAuthenticationProvider', () => { {} ) ).resolves.toEqual( - DeauthenticationResult.redirectTo('/base-path/login?next=%2Fapp%2Fml&msg=SESSION_EXPIRED') + DeauthenticationResult.redirectTo( + '/mock-server-basepath/login?next=%2Fapp%2Fml&msg=SESSION_EXPIRED' + ) ); }); }); diff --git a/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts b/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts index ca80761ee140c..ebf1341127e5f 100644 --- a/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/kerberos.test.ts @@ -518,7 +518,7 @@ describe('KerberosAuthenticationProvider', () => { expect(mockOptions.tokens.invalidate).toHaveBeenCalledWith(tokenPair); }); - it('redirects to `/logged_out` page if tokens are invalidated successfully.', async () => { + it('redirects to `loggedOut` URL if tokens are invalidated successfully.', async () => { const request = httpServerMock.createKibanaRequest(); const tokenPair = { accessToken: 'some-valid-token', @@ -528,7 +528,7 @@ describe('KerberosAuthenticationProvider', () => { mockOptions.tokens.invalidate.mockResolvedValue(undefined); await expect(provider.logout(request, tokenPair)).resolves.toEqual( - DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out') + DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut) ); expect(mockOptions.tokens.invalidate).toHaveBeenCalledTimes(1); diff --git a/x-pack/plugins/security/server/authentication/providers/kerberos.ts b/x-pack/plugins/security/server/authentication/providers/kerberos.ts index 2540c21210bd5..66a0ce22d3d19 100644 --- a/x-pack/plugins/security/server/authentication/providers/kerberos.ts +++ b/x-pack/plugins/security/server/authentication/providers/kerberos.ts @@ -114,9 +114,7 @@ export class KerberosAuthenticationProvider extends BaseAuthenticationProvider { return DeauthenticationResult.failed(err); } - return DeauthenticationResult.redirectTo( - `${this.options.basePath.serverBasePath}/security/logged_out` - ); + return DeauthenticationResult.redirectTo(this.options.urls.loggedOut); } /** diff --git a/x-pack/plugins/security/server/authentication/providers/oidc.test.ts b/x-pack/plugins/security/server/authentication/providers/oidc.test.ts index 2d42d90ab60b8..74344e8ae3eda 100644 --- a/x-pack/plugins/security/server/authentication/providers/oidc.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/oidc.test.ts @@ -353,7 +353,7 @@ describe('OIDCAuthenticationProvider', () => { state: { state: 'statevalue', nonce: 'noncevalue', - nextURL: '/base-path/s/foo/some-path', + nextURL: '/mock-server-basepath/s/foo/some-path', realm: 'oidc1', }, } @@ -575,7 +575,7 @@ describe('OIDCAuthenticationProvider', () => { state: { state: 'statevalue', nonce: 'noncevalue', - nextURL: '/base-path/s/foo/some-path', + nextURL: '/mock-server-basepath/s/foo/some-path', realm: 'oidc1', }, } @@ -702,7 +702,7 @@ describe('OIDCAuthenticationProvider', () => { }); }); - it('redirects to /logged_out if `redirect` field in OpenID Connect logout response is null.', async () => { + it('redirects to `loggedOut` URL if `redirect` field in OpenID Connect logout response is null.', async () => { const request = httpServerMock.createKibanaRequest(); const accessToken = 'x-oidc-token'; const refreshToken = 'x-oidc-refresh-token'; @@ -711,9 +711,7 @@ describe('OIDCAuthenticationProvider', () => { await expect( provider.logout(request, { accessToken, refreshToken, realm: 'oidc1' }) - ).resolves.toEqual( - DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out') - ); + ).resolves.toEqual(DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut)); expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1); expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.oidcLogout', { diff --git a/x-pack/plugins/security/server/authentication/providers/oidc.ts b/x-pack/plugins/security/server/authentication/providers/oidc.ts index f8e6ac0f9b5d0..ac7374401f99a 100644 --- a/x-pack/plugins/security/server/authentication/providers/oidc.ts +++ b/x-pack/plugins/security/server/authentication/providers/oidc.ts @@ -433,9 +433,7 @@ export class OIDCAuthenticationProvider extends BaseAuthenticationProvider { return DeauthenticationResult.redirectTo(redirect); } - return DeauthenticationResult.redirectTo( - `${this.options.basePath.serverBasePath}/security/logged_out` - ); + return DeauthenticationResult.redirectTo(this.options.urls.loggedOut); } catch (err) { this.logger.debug(`Failed to deauthenticate user: ${err.message}`); return DeauthenticationResult.failed(err); diff --git a/x-pack/plugins/security/server/authentication/providers/pki.test.ts b/x-pack/plugins/security/server/authentication/providers/pki.test.ts index 28db64edd9e32..a1279c9b9ca7f 100644 --- a/x-pack/plugins/security/server/authentication/providers/pki.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/pki.test.ts @@ -547,14 +547,14 @@ describe('PKIAuthenticationProvider', () => { expect(mockOptions.tokens.invalidate).toHaveBeenCalledWith({ accessToken: 'foo' }); }); - it('redirects to `/logged_out` page if access token is invalidated successfully.', async () => { + it('redirects to `loggedOut` URL if access token is invalidated successfully.', async () => { const request = httpServerMock.createKibanaRequest(); const state = { accessToken: 'foo', peerCertificateFingerprint256: '2A:7A:C2:DD' }; mockOptions.tokens.invalidate.mockResolvedValue(undefined); await expect(provider.logout(request, state)).resolves.toEqual( - DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out') + DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut) ); expect(mockOptions.tokens.invalidate).toHaveBeenCalledTimes(1); diff --git a/x-pack/plugins/security/server/authentication/providers/pki.ts b/x-pack/plugins/security/server/authentication/providers/pki.ts index 243e5415ad2c2..164a9516f0695 100644 --- a/x-pack/plugins/security/server/authentication/providers/pki.ts +++ b/x-pack/plugins/security/server/authentication/providers/pki.ts @@ -119,9 +119,7 @@ export class PKIAuthenticationProvider extends BaseAuthenticationProvider { return DeauthenticationResult.failed(err); } - return DeauthenticationResult.redirectTo( - `${this.options.basePath.serverBasePath}/security/logged_out` - ); + return DeauthenticationResult.redirectTo(this.options.urls.loggedOut); } /** 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 461ad3e38eca5..c32c1592acdd6 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.test.ts @@ -163,7 +163,7 @@ describe('SAMLAuthenticationProvider', () => { { requestId: 'some-request-id', redirectURL: '', realm: 'test-realm' } ) ).resolves.toEqual( - AuthenticationResult.redirectTo('/base-path/', { + AuthenticationResult.redirectTo('/mock-server-basepath/', { state: { accessToken: 'user-initiated-login-token', refreshToken: 'user-initiated-login-refresh-token', @@ -192,7 +192,7 @@ describe('SAMLAuthenticationProvider', () => { samlResponse: 'saml-response-xml', }) ).resolves.toEqual( - AuthenticationResult.redirectTo('/base-path/', { + AuthenticationResult.redirectTo('/mock-server-basepath/', { state: { accessToken: 'idp-initiated-login-token', refreshToken: 'idp-initiated-login-refresh-token', @@ -351,7 +351,7 @@ describe('SAMLAuthenticationProvider', () => { state ) ).resolves.toEqual( - AuthenticationResult.redirectTo('/base-path/', { + AuthenticationResult.redirectTo('/mock-server-basepath/', { state: { username: 'user', accessToken: 'new-valid-token', @@ -723,7 +723,7 @@ describe('SAMLAuthenticationProvider', () => { await expect(provider.authenticate(request)).resolves.toEqual( AuthenticationResult.redirectTo( '/mock-server-basepath/internal/security/saml/capture-url-fragment', - { state: { redirectURL: '/base-path/s/foo/some-path', realm: 'test-realm' } } + { state: { redirectURL: '/mock-server-basepath/s/foo/some-path', realm: 'test-realm' } } ) ); @@ -753,7 +753,7 @@ describe('SAMLAuthenticationProvider', () => { expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1); expect(mockOptions.logger.warn).toHaveBeenCalledWith( - 'Max URL path size should not exceed 100b but it was 107b. URL is not captured.' + 'Max URL path size should not exceed 100b but it was 118b. URL is not captured.' ); }); @@ -998,7 +998,7 @@ describe('SAMLAuthenticationProvider', () => { await expect(provider.authenticate(request, state)).resolves.toEqual( AuthenticationResult.redirectTo( '/mock-server-basepath/internal/security/saml/capture-url-fragment', - { state: { redirectURL: '/base-path/s/foo/some-path', realm: 'test-realm' } } + { state: { redirectURL: '/mock-server-basepath/s/foo/some-path', realm: 'test-realm' } } ) ); @@ -1054,7 +1054,7 @@ describe('SAMLAuthenticationProvider', () => { expect(mockOptions.logger.warn).toHaveBeenCalledTimes(1); expect(mockOptions.logger.warn).toHaveBeenCalledWith( - 'Max URL path size should not exceed 100b but it was 107b. URL is not captured.' + 'Max URL path size should not exceed 100b but it was 118b. URL is not captured.' ); }); @@ -1124,7 +1124,7 @@ describe('SAMLAuthenticationProvider', () => { }); }); - it('redirects to /security/logged_out if `redirect` field in SAML logout response is null.', async () => { + it('redirects to `loggedOut` URL if `redirect` field in SAML logout response is null.', async () => { const request = httpServerMock.createKibanaRequest(); const accessToken = 'x-saml-token'; const refreshToken = 'x-saml-refresh-token'; @@ -1138,9 +1138,7 @@ describe('SAMLAuthenticationProvider', () => { refreshToken, realm: 'test-realm', }) - ).resolves.toEqual( - DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out') - ); + ).resolves.toEqual(DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut)); expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1); expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.samlLogout', { @@ -1148,7 +1146,7 @@ describe('SAMLAuthenticationProvider', () => { }); }); - it('redirects to /security/logged_out if `redirect` field in SAML logout response is not defined.', async () => { + it('redirects to `loggedOut` URL if `redirect` field in SAML logout response is not defined.', async () => { const request = httpServerMock.createKibanaRequest(); const accessToken = 'x-saml-token'; const refreshToken = 'x-saml-refresh-token'; @@ -1162,9 +1160,7 @@ describe('SAMLAuthenticationProvider', () => { refreshToken, realm: 'test-realm', }) - ).resolves.toEqual( - DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out') - ); + ).resolves.toEqual(DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut)); expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1); expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.samlLogout', { @@ -1174,7 +1170,7 @@ describe('SAMLAuthenticationProvider', () => { it('relies on SAML logout if query string is not empty, but does not include SAMLRequest.', async () => { const request = httpServerMock.createKibanaRequest({ - query: { Whatever: 'something unrelated' }, + query: { Whatever: 'something unrelated', SAMLResponse: 'xxx yyy' }, }); const accessToken = 'x-saml-token'; const refreshToken = 'x-saml-refresh-token'; @@ -1188,9 +1184,7 @@ describe('SAMLAuthenticationProvider', () => { refreshToken, realm: 'test-realm', }) - ).resolves.toEqual( - DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out') - ); + ).resolves.toEqual(DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut)); expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1); expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.samlLogout', { @@ -1210,9 +1204,7 @@ describe('SAMLAuthenticationProvider', () => { refreshToken: 'x-saml-refresh-token', realm: 'test-realm', }) - ).resolves.toEqual( - DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out') - ); + ).resolves.toEqual(DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut)); expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1); expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledWith('shield.samlInvalidate', { @@ -1220,13 +1212,13 @@ describe('SAMLAuthenticationProvider', () => { }); }); - it('redirects to /security/logged_out if `redirect` field in SAML invalidate response is null.', async () => { + it('redirects to `loggedOut` URL if `redirect` field in SAML invalidate response is null.', async () => { const request = httpServerMock.createKibanaRequest({ query: { SAMLRequest: 'xxx yyy' } }); mockOptions.client.callAsInternalUser.mockResolvedValue({ redirect: null }); await expect(provider.logout(request)).resolves.toEqual( - DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out') + DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut) ); expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1); @@ -1235,13 +1227,13 @@ describe('SAMLAuthenticationProvider', () => { }); }); - it('redirects to /security/logged_out if `redirect` field in SAML invalidate response is not defined.', async () => { + it('redirects to `loggedOut` URL if `redirect` field in SAML invalidate response is not defined.', async () => { const request = httpServerMock.createKibanaRequest({ query: { SAMLRequest: 'xxx yyy' } }); mockOptions.client.callAsInternalUser.mockResolvedValue({ redirect: undefined }); await expect(provider.logout(request)).resolves.toEqual( - DeauthenticationResult.redirectTo('/mock-server-basepath/security/logged_out') + DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut) ); expect(mockOptions.client.callAsInternalUser).toHaveBeenCalledTimes(1); @@ -1250,6 +1242,16 @@ describe('SAMLAuthenticationProvider', () => { }); }); + it('redirects to `loggedOut` URL if SAML logout response is received.', async () => { + const request = httpServerMock.createKibanaRequest({ query: { SAMLResponse: 'xxx yyy' } }); + + await expect(provider.logout(request)).resolves.toEqual( + DeauthenticationResult.redirectTo(mockOptions.urls.loggedOut) + ); + + expect(mockOptions.client.callAsInternalUser).not.toHaveBeenCalled(); + }); + it('redirects user to the IdP if SLO is supported by IdP in case of SP initiated logout.', async () => { const request = httpServerMock.createKibanaRequest(); const accessToken = 'x-saml-token'; diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index 3161144023c1f..e1c0b2d3e0a13 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -69,6 +69,14 @@ function isSAMLRequestQuery(query: any): query is { SAMLRequest: string } { return query && query.SAMLRequest; } +/** + * Checks whether request query includes SAML response from IdP. + * @param query Parsed HTTP request query. + */ +function isSAMLResponseQuery(query: any): query is { SAMLResponse: string } { + return query && query.SAMLResponse; +} + /** * Checks whether current request can initiate new session. * @param request Request instance. @@ -234,22 +242,36 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { this.logger.debug(`Trying to log user out via ${request.url.path}.`); // Normally when there is no active session in Kibana, `logout` method shouldn't do anything - // and user will eventually be redirected to the home page to log in. But when SAML is enabled - // there is a special case when logout is initiated by the IdP or another SP, then IdP will - // request _every_ SP associated with the current user session to do the logout. So if Kibana, - // without an active session, receives such request it shouldn't redirect user to the home page, - // but rather redirect back to IdP with correct logout response and only Elasticsearch knows how - // to do that. - const isIdPInitiatedSLO = isSAMLRequestQuery(request.query); - if (!state?.accessToken && !isIdPInitiatedSLO) { + // and user will eventually be redirected to the home page to log in. But when SAML SLO is + // supported there are two special cases that we need to handle even if there is no active + // Kibana session: + // + // 1. When IdP or another SP initiates logout, then IdP will request _every_ SP associated with + // the current user session to do the logout. So if Kibana receives such request it shouldn't + // redirect user to the home page, but rather redirect back to IdP with correct logout response + // and only Elasticsearch knows how to do that. + // + // 2. When Kibana initiates logout, then IdP may eventually respond with the logout response. So + // if Kibana receives such response it shouldn't redirect user to the home page, but rather + // redirect to the `loggedOut` URL instead. + const isIdPInitiatedSLORequest = isSAMLRequestQuery(request.query); + const isSPInitiatedSLOResponse = isSAMLResponseQuery(request.query); + if (!state?.accessToken && !isIdPInitiatedSLORequest && !isSPInitiatedSLOResponse) { this.logger.debug('There is no SAML session to invalidate.'); return DeauthenticationResult.notHandled(); } try { - const redirect = isIdPInitiatedSLO + // It may _theoretically_ (highly unlikely in practice though) happen that when user receives + // logout response they may already have a new SAML session (isSPInitiatedSLOResponse == true + // and state !== undefined). In this case case it'd be safer to trigger SP initiated logout + // for the new session as well. + const redirect = isIdPInitiatedSLORequest ? await this.performIdPInitiatedSingleLogout(request) - : await this.performUserInitiatedSingleLogout(state?.accessToken!, state?.refreshToken!); + : state + ? await this.performUserInitiatedSingleLogout(state.accessToken!, state.refreshToken!) + : // Once Elasticsearch can consume logout response we'll be sending it here. + null; // Having non-null `redirect` field within logout response means that IdP // supports SAML Single Logout and we should redirect user to the specified @@ -259,9 +281,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { return DeauthenticationResult.redirectTo(redirect); } - return DeauthenticationResult.redirectTo( - `${this.options.basePath.serverBasePath}/security/logged_out` - ); + return DeauthenticationResult.redirectTo(this.options.urls.loggedOut); } catch (err) { this.logger.debug(`Failed to deauthenticate user: ${err.message}`); return DeauthenticationResult.failed(err); diff --git a/x-pack/plugins/security/server/authentication/providers/token.test.ts b/x-pack/plugins/security/server/authentication/providers/token.test.ts index 92cea424e575d..84ff7d1f5a1ef 100644 --- a/x-pack/plugins/security/server/authentication/providers/token.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/token.test.ts @@ -179,7 +179,7 @@ describe('TokenAuthenticationProvider', () => { ) ).resolves.toEqual( AuthenticationResult.redirectTo( - '/base-path/login?next=%2Fbase-path%2Fs%2Ffoo%2Fsome-path%20%23%20that%20needs%20to%20be%20encoded' + '/mock-server-basepath/login?next=%2Fmock-server-basepath%2Fs%2Ffoo%2Fsome-path%20%23%20that%20needs%20to%20be%20encoded' ) ); }); @@ -309,9 +309,10 @@ describe('TokenAuthenticationProvider', () => { mockOptions.tokens.refresh.mockResolvedValue(null); await expect(provider.authenticate(request, tokenPair)).resolves.toEqual( - AuthenticationResult.redirectTo('/base-path/login?next=%2Fbase-path%2Fsome-path', { - state: null, - }) + AuthenticationResult.redirectTo( + '/mock-server-basepath/login?next=%2Fmock-server-basepath%2Fsome-path', + { state: null } + ) ); expect(mockOptions.tokens.refresh).toHaveBeenCalledTimes(1); @@ -455,7 +456,7 @@ describe('TokenAuthenticationProvider', () => { mockOptions.tokens.invalidate.mockResolvedValue(undefined); await expect(provider.logout(request, tokenPair)).resolves.toEqual( - DeauthenticationResult.redirectTo('/base-path/login?msg=LOGGED_OUT') + DeauthenticationResult.redirectTo('/mock-server-basepath/login?msg=LOGGED_OUT') ); expect(mockOptions.tokens.invalidate).toHaveBeenCalledTimes(1); @@ -469,7 +470,7 @@ describe('TokenAuthenticationProvider', () => { mockOptions.tokens.invalidate.mockResolvedValue(undefined); await expect(provider.logout(request, tokenPair)).resolves.toEqual( - DeauthenticationResult.redirectTo('/base-path/login?yep=nope') + DeauthenticationResult.redirectTo('/mock-server-basepath/login?yep=nope') ); expect(mockOptions.tokens.invalidate).toHaveBeenCalledTimes(1); diff --git a/x-pack/plugins/security/server/routes/authentication/common.ts b/x-pack/plugins/security/server/routes/authentication/common.ts index 91783140539a5..ad38a158af2b9 100644 --- a/x-pack/plugins/security/server/routes/authentication/common.ts +++ b/x-pack/plugins/security/server/routes/authentication/common.ts @@ -31,7 +31,7 @@ export function defineCommonRoutes({ { path, // Allow unknown query parameters as this endpoint can be hit by the 3rd-party with any - // set of query string parameters (e.g. SAML/OIDC logout request parameters). + // set of query string parameters (e.g. SAML/OIDC logout request/response parameters). validate: { query: schema.object({}, { unknowns: 'allow' }) }, options: { authRequired: false }, }, From beb87fcfec045c240691f3e33457069c9b47ba05 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Wed, 24 Jun 2020 08:51:15 +0200 Subject: [PATCH 2/2] Review#1: handle review comments. --- .../security/server/authentication/authenticator.test.ts | 2 +- .../security/server/authentication/providers/saml.test.ts | 2 +- x-pack/plugins/security/server/authentication/providers/saml.ts | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts index 518d625b57d11..3b77ea3248173 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.test.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.test.ts @@ -112,7 +112,7 @@ describe('Authenticator', () => { ).toThrowError('Provider name "__http__" is reserved.'); }); - it('properly set `loggedOut` URL.', () => { + it('properly sets `loggedOut` URL.', () => { const basicAuthenticationProviderMock = jest.requireMock('./providers/basic') .BasicAuthenticationProvider; 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 38e667e31317e..f7adaa24e9dbb 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.test.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.test.ts @@ -247,7 +247,7 @@ describe('SAMLAuthenticationProvider', () => { { requestId: 'some-request-id', redirectURL: '', realm: 'test-realm' } ) ).resolves.toEqual( - AuthenticationResult.redirectTo('/base-path/', { + AuthenticationResult.redirectTo('/mock-server-basepath/', { state: { accessToken: 'user-initiated-login-token', refreshToken: 'user-initiated-login-refresh-token', diff --git a/x-pack/plugins/security/server/authentication/providers/saml.ts b/x-pack/plugins/security/server/authentication/providers/saml.ts index 436af1829b506..d121cd4979aa7 100644 --- a/x-pack/plugins/security/server/authentication/providers/saml.ts +++ b/x-pack/plugins/security/server/authentication/providers/saml.ts @@ -283,7 +283,7 @@ export class SAMLAuthenticationProvider extends BaseAuthenticationProvider { ? await this.performIdPInitiatedSingleLogout(request) : state ? await this.performUserInitiatedSingleLogout(state.accessToken!, state.refreshToken!) - : // Once Elasticsearch can consume logout response we'll be sending it here. + : // Once Elasticsearch can consume logout response we'll be sending it here. See https://github.com/elastic/elasticsearch/issues/40901 null; // Having non-null `redirect` field within logout response means that IdP