Skip to content

Commit

Permalink
[6.8] Redirect to Logged Out UI on SAML Logout Response. (#69815)
Browse files Browse the repository at this point in the history
  • Loading branch information
@azasypkin
azasypkin authored Jun 25, 2020
1 parent 5d14cf2 commit 058e535
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 26 deletions.
41 changes: 29 additions & 12 deletions x-pack/plugins/security/server/lib/authentication/authenticator.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,13 +202,20 @@ class Authenticator {
return this._providers.get(sessionValue.provider).deauthenticate(request, sessionValue.state);
}

// Normally when there is no active session in Kibana, `deauthenticate` method shouldn't do anything
// and user will eventually be redirected to the home page to log in. But if SAML is supported 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 active session)
// receives such a 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.
if (this._isSAMLRequest(request) && this._providers.has('saml')) {
// Normally when there is no active session in Kibana, `deauthenticate` method shouldn't do
// anything 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.
if ((this._isSAMLRequestQuery(request) || this._isSAMLResponseQuery(request)) && this._providers.has('saml')) {
return this._providers.get('saml').deauthenticate(request);
}

Expand Down Expand Up @@ -241,7 +248,7 @@ class Authenticator {
// If there is no way to predict which provider to use first, let's use the order providers are configured in.
// Otherwise return provider that either owns session or can handle 3rd-party login request first, and only then
// the rest of providers.
const shouldHandleSAMLResponse = this._isSAMLResponse(request) && this._providers.has('saml');
const shouldHandleSAMLResponse = this._isSAMLResponsePayload(request) && this._providers.has('saml');
if (!sessionValue && !shouldHandleSAMLResponse) {
yield* this._providers;
} else {
Expand Down Expand Up @@ -278,22 +285,32 @@ class Authenticator {
}

/**
* Checks whether specified request represents SAML Request.
* Checks whether specified request has SAML Request in the query.
* @param {Hapi.Request} request HapiJS request instance.
* @returns {boolean}
* @private
*/
_isSAMLRequest(request) {
_isSAMLRequestQuery(request) {
return !!(request.query && request.query.SAMLRequest);
}

/**
* Checks whether specified request represents SAML Response.
* Checks whether specified request has SAML Response in the query.
* @param {Hapi.Request} request HapiJS request instance.
* @returns {boolean}
* @private
*/
_isSAMLResponse(request) {
_isSAMLResponseQuery(request) {
return !!(request.query && request.query.SAMLResponse);
}

/**
* Checks whether specified request has SAML Response in the payload.
* @param {Hapi.Request} request HapiJS request instance.
* @returns {boolean}
* @private
*/
_isSAMLResponsePayload(request) {
return !!(request.payload && request.payload.SAMLResponse)
&& request.path === '/api/security/v1/saml';
}
Expand Down
25 changes: 18 additions & 7 deletions x-pack/plugins/security/server/lib/authentication/providers/__tests__/saml.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,7 @@ describe('SAMLAuthenticationProvider', () => {
);

expect(authenticationResult.redirected()).to.be(true);
expect(authenticationResult.redirectURL).to.be('/logged_out');
expect(authenticationResult.redirectURL).to.be('/test-base-path/logged_out');
});

it('redirects to /logged_out if `redirect` field in SAML logout response is not defined.', async () => {
Expand All @@ -581,11 +581,11 @@ describe('SAMLAuthenticationProvider', () => {
);

expect(authenticationResult.redirected()).to.be(true);
expect(authenticationResult.redirectURL).to.be('/logged_out');
expect(authenticationResult.redirectURL).to.be('/test-base-path/logged_out');
});

it('relies on SAML logout if query string is not empty, but does not include SAMLRequest.', async () => {
const request = requestFixture({ search: '?Whatever=something%20unrelated' });
const request = requestFixture({ search: '?Whatever=something%20unrelated&SAMLResponse=xxx%20yyy' });
const accessToken = 'x-saml-token';
const refreshToken = 'x-saml-refresh-token';

Expand All @@ -603,7 +603,7 @@ describe('SAMLAuthenticationProvider', () => {
);

expect(authenticationResult.redirected()).to.be(true);
expect(authenticationResult.redirectURL).to.be('/logged_out');
expect(authenticationResult.redirectURL).to.be('/test-base-path/logged_out');
});

it('relies SAML invalidate call even if access token is presented.', async () => {
Expand Down Expand Up @@ -631,7 +631,7 @@ describe('SAMLAuthenticationProvider', () => {
);

expect(authenticationResult.redirected()).to.be(true);
expect(authenticationResult.redirectURL).to.be('/logged_out');
expect(authenticationResult.redirectURL).to.be('/test-base-path/logged_out');
});

it('redirects to /logged_out if `redirect` field in SAML invalidate response is null.', async () => {
Expand All @@ -656,7 +656,7 @@ describe('SAMLAuthenticationProvider', () => {
);

expect(authenticationResult.redirected()).to.be(true);
expect(authenticationResult.redirectURL).to.be('/logged_out');
expect(authenticationResult.redirectURL).to.be('/test-base-path/logged_out');
});

it('redirects to /logged_out if `redirect` field in SAML invalidate response is not defined.', async () => {
Expand All @@ -681,7 +681,18 @@ describe('SAMLAuthenticationProvider', () => {
);

expect(authenticationResult.redirected()).to.be(true);
expect(authenticationResult.redirectURL).to.be('/logged_out');
expect(authenticationResult.redirectURL).to.be('/test-base-path/logged_out');
});

it('redirects to /logged_out if SAML logout response is received.', async () => {
const request = requestFixture({ search: '?SAMLResponse=xxx%20yyy' });

const authenticationResult = await provider.deauthenticate(request);

sinon.assert.notCalled(callWithInternalUser);

expect(authenticationResult.redirected()).to.be(true);
expect(authenticationResult.redirectURL).to.be('/test-base-path/logged_out');
});

it('redirects user to the IdP if SLO is supported by IdP in case of SP initiated logout.', async () => {
Expand Down
22 changes: 15 additions & 7 deletions x-pack/plugins/security/server/lib/authentication/providers/saml.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -387,20 +387,22 @@ export class SAMLAuthenticationProvider {
async deauthenticate(request, state) {
this._options.log(['debug', 'security', 'saml'], `Trying to deauthenticate user via ${request.url.path}.`);

if ((!state || !state.accessToken) && !request.query.SAMLRequest) {
const isIdPInitiatedSLORequest = !!(request.query && request.query.SAMLRequest);
const isSPInitiatedSLOResponse = !!(request.query && request.query.SAMLResponse);
if ((!state || !state.accessToken) && !isIdPInitiatedSLORequest && !isSPInitiatedSLOResponse) {
this._options.log(['debug', 'security', 'saml'], 'There is neither access token nor SAML session to invalidate.');
return DeauthenticationResult.notHandled();
}

let logoutArgs;
if (request.query.SAMLRequest) {
if (isIdPInitiatedSLORequest) {
this._options.log(['debug', 'security', 'saml'], 'Logout has been initiated by the Identity Provider.');
logoutArgs = [
'shield.samlInvalidate',
// Elasticsearch expects `queryString` without leading `?`, so we should strip it with `slice`.
{ body: { queryString: request.url.search ? request.url.search.slice(1) : '', acs: this._getACS() } }
];
} else {
} else if (state) {
this._options.log(['debug', 'security', 'saml'], 'Logout has been initiated by the user.');
logoutArgs = [
'shield.samlLogout',
Expand All @@ -409,9 +411,15 @@ export class SAMLAuthenticationProvider {
}

try {
// 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/logout (invalidate)`.
const { redirect } = await this._options.client.callWithInternalUser(...logoutArgs);
// 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 = logoutArgs
// 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/logout (invalidate)`.
? (await this._options.client.callWithInternalUser(...logoutArgs)).redirect
: null;

this._options.log(['debug', 'security', 'saml'], 'User session has been successfully invalidated.');

Expand All @@ -423,7 +431,7 @@ export class SAMLAuthenticationProvider {
return DeauthenticationResult.redirectTo(redirect);
}

return DeauthenticationResult.redirectTo('/logged_out');
return DeauthenticationResult.redirectTo(`${this._options.basePath}/logged_out`);
} catch(err) {
this._options.log(['debug', 'security', 'saml'], `Failed to deauthenticate user: ${err.message}`);
return DeauthenticationResult.failed(err);
Expand Down

0 comments on commit 058e535

Please sign in to comment.