Redirect to Logged Out UI on SAML Logout Response. Prefer Login Selector UI to Logged Out UI whenever possible. (#69676)

This commit is contained in:
Aleh Zasypkin 2020-06-24 10:39:39 +02:00 committed by GitHub
parent c8608ed810
commit fc9df7244b
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
15 changed files with 125 additions and 73 deletions

View file

@ -112,6 +112,33 @@ describe('Authenticator', () => {
).toThrowError('Provider name "__http__" is reserved.');
});
it('properly sets `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

View file

@ -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(

View file

@ -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: loggingSystemMock.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',
},
};
}

View file

@ -26,6 +26,9 @@ export interface AuthenticationProviderOptions {
client: IClusterClient;
logger: Logger;
tokens: PublicMethodsOf<Tokens>;
urls: {
loggedOut: string;
};
}
/**

View file

@ -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'
)
);
});
});

View file

@ -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);

View file

@ -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);
}
/**

View file

@ -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', {

View file

@ -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);

View file

@ -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);

View file

@ -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);
}
/**

View file

@ -208,7 +208,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',
@ -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',
@ -276,7 +276,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',
@ -562,7 +562,7 @@ describe('SAMLAuthenticationProvider', () => {
state
)
).resolves.toEqual(
AuthenticationResult.redirectTo('/base-path/', {
AuthenticationResult.redirectTo('/mock-server-basepath/', {
state: {
username: 'user',
accessToken: 'new-valid-token',
@ -999,7 +999,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' } }
)
);
@ -1029,7 +1029,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.'
);
});
@ -1274,7 +1274,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' } }
)
);
@ -1330,7 +1330,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.'
);
});
@ -1400,7 +1400,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';
@ -1414,9 +1414,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', {
@ -1424,7 +1422,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';
@ -1438,9 +1436,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', {
@ -1450,7 +1446,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';
@ -1464,9 +1460,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', {
@ -1486,9 +1480,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', {
@ -1496,13 +1488,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);
@ -1511,13 +1503,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);
@ -1526,6 +1518,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';

View file

@ -70,6 +70,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.
@ -247,22 +255,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. See https://github.com/elastic/elasticsearch/issues/40901
null;
// Having non-null `redirect` field within logout response means that IdP
// supports SAML Single Logout and we should redirect user to the specified
@ -272,9 +294,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);

View file

@ -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);

View file

@ -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 },
},