diff --git a/x-pack/plugins/security/public/authentication/login/__snapshots__/login_page.test.tsx.snap b/x-pack/plugins/security/public/authentication/login/__snapshots__/login_page.test.tsx.snap index 0eccd4692d26e..ecbdfedac1dd3 100644 --- a/x-pack/plugins/security/public/authentication/login/__snapshots__/login_page.test.tsx.snap +++ b/x-pack/plugins/security/public/authentication/login/__snapshots__/login_page.test.tsx.snap @@ -110,6 +110,7 @@ exports[`LoginPage enabled form state renders as expected 1`] = ` "add": [MockFunction], "addDanger": [MockFunction], "addError": [MockFunction], + "addInfo": [MockFunction], "addSuccess": [MockFunction], "addWarning": [MockFunction], "get$": [MockFunction], @@ -143,6 +144,7 @@ exports[`LoginPage enabled form state renders as expected when info message is s "add": [MockFunction], "addDanger": [MockFunction], "addError": [MockFunction], + "addInfo": [MockFunction], "addSuccess": [MockFunction], "addWarning": [MockFunction], "get$": [MockFunction], @@ -176,6 +178,7 @@ exports[`LoginPage enabled form state renders as expected when loginAssistanceMe "add": [MockFunction], "addDanger": [MockFunction], "addError": [MockFunction], + "addInfo": [MockFunction], "addSuccess": [MockFunction], "addWarning": [MockFunction], "get$": [MockFunction], @@ -265,6 +268,7 @@ exports[`LoginPage page renders as expected 1`] = ` "add": [MockFunction], "addDanger": [MockFunction], "addError": [MockFunction], + "addInfo": [MockFunction], "addSuccess": [MockFunction], "addWarning": [MockFunction], "get$": [MockFunction], diff --git a/x-pack/plugins/security/public/authentication/login/components/login_form/__snapshots__/login_form.test.tsx.snap b/x-pack/plugins/security/public/authentication/login/components/login_form/__snapshots__/login_form.test.tsx.snap index 2bd99f2c6afe6..9fd32c2d64310 100644 --- a/x-pack/plugins/security/public/authentication/login/components/login_form/__snapshots__/login_form.test.tsx.snap +++ b/x-pack/plugins/security/public/authentication/login/components/login_form/__snapshots__/login_form.test.tsx.snap @@ -118,7 +118,7 @@ exports[`LoginForm login selector renders as expected with login form 1`] = ` `; -exports[`LoginForm login selector renders as expected without login form 1`] = ` +exports[`LoginForm login selector renders as expected without login form for providers with and without description 1`] = ` - Login w/PKI + `; diff --git a/x-pack/plugins/security/public/authentication/login/components/login_form/login_form.test.tsx b/x-pack/plugins/security/public/authentication/login/components/login_form/login_form.test.tsx index cff41f5d26d2d..c17c10a2c5148 100644 --- a/x-pack/plugins/security/public/authentication/login/components/login_form/login_form.test.tsx +++ b/x-pack/plugins/security/public/authentication/login/components/login_form/login_form.test.tsx @@ -166,7 +166,7 @@ describe('LoginForm', () => { ).toMatchSnapshot(); }); - it('renders as expected without login form', async () => { + it('renders as expected without login form for providers with and without description', async () => { const coreStartMock = coreMock.createStart(); expect( shallowWithIntl( @@ -179,7 +179,7 @@ describe('LoginForm', () => { enabled: true, providers: [ { type: 'saml', name: 'saml1', description: 'Login w/SAML' }, - { type: 'pki', name: 'pki1', description: 'Login w/PKI' }, + { type: 'pki', name: 'pki1' }, ], }} /> @@ -230,5 +230,43 @@ describe('LoginForm', () => { expect(wrapper.find(EuiCallOut).exists()).toBe(false); expect(coreStartMock.notifications.toasts.addError).not.toHaveBeenCalled(); }); + + it('shows error toast if login fails', async () => { + const currentURL = `https://some-host/login?next=${encodeURIComponent( + '/some-base-path/app/kibana#/home?_g=()' + )}`; + + const failureReason = new Error('Oh no!'); + const coreStartMock = coreMock.createStart({ basePath: '/some-base-path' }); + coreStartMock.http.post.mockRejectedValue(failureReason); + + window.location.href = currentURL; + const wrapper = mountWithIntl( + + ); + + wrapper.findWhere(node => node.key() === 'saml1').simulate('click'); + + await act(async () => { + await nextTick(); + wrapper.update(); + }); + + expect(coreStartMock.http.post).toHaveBeenCalledTimes(1); + expect(coreStartMock.http.post).toHaveBeenCalledWith('/internal/security/login_with', { + body: JSON.stringify({ providerType: 'saml', providerName: 'saml1', currentURL }), + }); + + expect(window.location.href).toBe(currentURL); + expect(coreStartMock.notifications.toasts.addError).toHaveBeenCalledWith(failureReason, { + title: 'Could not perform login.', + }); + }); }); }); diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts index 5dc971c8fc6f0..a595b63faaf9b 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.test.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.test.ts @@ -31,17 +31,19 @@ function getMockOptions({ session, providers, http = {}, + selector, }: { session?: AuthenticatorOptions['config']['session']; providers?: Record | string[]; http?: Partial; + selector?: AuthenticatorOptions['config']['authc']['selector']; } = {}) { return { clusterClient: elasticsearchServiceMock.createClusterClient(), basePath: httpServiceMock.createSetupContract().basePath, loggers: loggingServiceMock.create(), config: createConfig( - ConfigSchema.validate({ session, authc: { providers, http } }), + ConfigSchema.validate({ session, authc: { selector, providers, http } }), loggingServiceMock.create().get(), { isTLSEnabled: false } ), @@ -54,7 +56,7 @@ describe('Authenticator', () => { beforeEach(() => { mockBasicAuthenticationProvider = { login: jest.fn(), - authenticate: jest.fn(), + authenticate: jest.fn().mockResolvedValue(AuthenticationResult.notHandled()), logout: jest.fn().mockResolvedValue(DeauthenticationResult.notHandled()), getHTTPAuthenticationScheme: jest.fn(), }; @@ -182,6 +184,7 @@ describe('Authenticator', () => { beforeEach(() => { mockOptions = getMockOptions({ providers: { basic: { basic1: { order: 0 } } } }); mockSessionStorage = sessionStorageMock.create(); + mockSessionStorage.get.mockResolvedValue(null); mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage); mockSessVal = { idleTimeoutExpiration: null, @@ -322,6 +325,7 @@ describe('Authenticator', () => { }, }); mockSessionStorage = sessionStorageMock.create(); + mockSessionStorage.get.mockResolvedValue(null); mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage); authenticator = new Authenticator(mockOptions); @@ -496,6 +500,7 @@ describe('Authenticator', () => { it('clears session if provider asked to do so.', async () => { const user = mockAuthenticatedUser(); const request = httpServerMock.createKibanaRequest(); + mockSessionStorage.get.mockResolvedValue(mockSessVal); mockBasicAuthenticationProvider.login.mockResolvedValue( AuthenticationResult.succeeded(user, { state: null }) @@ -508,6 +513,26 @@ describe('Authenticator', () => { expect(mockSessionStorage.set).not.toHaveBeenCalled(); expect(mockSessionStorage.clear).toHaveBeenCalled(); }); + + it('clears legacy session.', async () => { + const user = mockAuthenticatedUser(); + const request = httpServerMock.createKibanaRequest(); + + // Use string format for the `provider` session value field to emulate legacy session. + mockSessionStorage.get.mockResolvedValue({ ...mockSessVal, provider: 'basic' }); + + mockBasicAuthenticationProvider.login.mockResolvedValue(AuthenticationResult.succeeded(user)); + + await expect( + authenticator.login(request, { provider: { type: 'basic' }, value: {} }) + ).resolves.toEqual(AuthenticationResult.succeeded(user)); + + expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledTimes(1); + expect(mockBasicAuthenticationProvider.login).toHaveBeenCalledWith(request, {}, null); + + expect(mockSessionStorage.set).not.toHaveBeenCalled(); + expect(mockSessionStorage.clear).toHaveBeenCalled(); + }); }); describe('`authenticate` method', () => { @@ -518,6 +543,7 @@ describe('Authenticator', () => { beforeEach(() => { mockOptions = getMockOptions({ providers: { basic: { basic1: { order: 0 } } } }); mockSessionStorage = sessionStorageMock.create(); + mockSessionStorage.get.mockResolvedValue(null); mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage); mockSessVal = { idleTimeoutExpiration: null, @@ -931,14 +957,33 @@ describe('Authenticator', () => { expect(mockSessionStorage.clear).toHaveBeenCalled(); }); + it('clears legacy session.', async () => { + const user = mockAuthenticatedUser(); + const request = httpServerMock.createKibanaRequest(); + + // Use string format for the `provider` session value field to emulate legacy session. + mockSessionStorage.get.mockResolvedValue({ ...mockSessVal, provider: 'basic' }); + + mockBasicAuthenticationProvider.authenticate.mockResolvedValue( + AuthenticationResult.succeeded(user) + ); + + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.succeeded(user) + ); + + expect(mockBasicAuthenticationProvider.authenticate).toHaveBeenCalledTimes(1); + expect(mockBasicAuthenticationProvider.authenticate).toHaveBeenCalledWith(request, null); + + expect(mockSessionStorage.set).not.toHaveBeenCalled(); + expect(mockSessionStorage.clear).toHaveBeenCalled(); + }); + it('does not clear session if provider can not handle system API request authentication with active session.', async () => { const request = httpServerMock.createKibanaRequest({ headers: { 'kbn-system-request': 'true' }, }); - mockBasicAuthenticationProvider.authenticate.mockResolvedValue( - AuthenticationResult.notHandled() - ); mockSessionStorage.get.mockResolvedValue(mockSessVal); await expect(authenticator.authenticate(request)).resolves.toEqual( @@ -954,9 +999,6 @@ describe('Authenticator', () => { headers: { 'kbn-system-request': 'false' }, }); - mockBasicAuthenticationProvider.authenticate.mockResolvedValue( - AuthenticationResult.notHandled() - ); mockSessionStorage.get.mockResolvedValue(mockSessVal); await expect(authenticator.authenticate(request)).resolves.toEqual( @@ -972,9 +1014,6 @@ describe('Authenticator', () => { headers: { 'kbn-system-request': 'true' }, }); - mockBasicAuthenticationProvider.authenticate.mockResolvedValue( - AuthenticationResult.notHandled() - ); mockSessionStorage.get.mockResolvedValue({ ...mockSessVal, provider: { type: 'token', name: 'token1' }, @@ -993,9 +1032,6 @@ describe('Authenticator', () => { headers: { 'kbn-system-request': 'false' }, }); - mockBasicAuthenticationProvider.authenticate.mockResolvedValue( - AuthenticationResult.notHandled() - ); mockSessionStorage.get.mockResolvedValue({ ...mockSessVal, provider: { type: 'token', name: 'token1' }, @@ -1008,6 +1044,70 @@ describe('Authenticator', () => { expect(mockSessionStorage.set).not.toHaveBeenCalled(); expect(mockSessionStorage.clear).toHaveBeenCalled(); }); + + describe('with Login Selector', () => { + beforeEach(() => { + mockOptions = getMockOptions({ + selector: { enabled: true }, + providers: { basic: { basic1: { order: 0 } } }, + }); + mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage); + + authenticator = new Authenticator(mockOptions); + }); + + it('does not redirect to Login Selector if there is an active session', async () => { + const request = httpServerMock.createKibanaRequest(); + mockSessionStorage.get.mockResolvedValue(mockSessVal); + + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.notHandled() + ); + expect(mockBasicAuthenticationProvider.authenticate).toHaveBeenCalled(); + }); + + it('does not redirect AJAX requests to Login Selector', async () => { + const request = httpServerMock.createKibanaRequest({ headers: { 'kbn-xsrf': 'xsrf' } }); + + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.notHandled() + ); + expect(mockBasicAuthenticationProvider.authenticate).toHaveBeenCalled(); + }); + + it('does not redirect to Login Selector if request has `Authorization` header', async () => { + const request = httpServerMock.createKibanaRequest({ + headers: { authorization: 'Basic ***' }, + }); + + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.notHandled() + ); + expect(mockBasicAuthenticationProvider.authenticate).toHaveBeenCalled(); + }); + + it('does not redirect to Login Selector if it is not enabled', async () => { + mockOptions = getMockOptions({ providers: { basic: { basic1: { order: 0 } } } }); + mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage); + authenticator = new Authenticator(mockOptions); + + const request = httpServerMock.createKibanaRequest(); + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.notHandled() + ); + expect(mockBasicAuthenticationProvider.authenticate).toHaveBeenCalled(); + }); + + it('redirects to the Login Selector when needed.', async () => { + const request = httpServerMock.createKibanaRequest(); + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.redirectTo( + '/mock-server-basepath/login?next=%2Fmock-server-basepath%2Fpath' + ) + ); + expect(mockBasicAuthenticationProvider.authenticate).not.toHaveBeenCalled(); + }); + }); }); describe('`logout` method', () => { diff --git a/x-pack/plugins/security/server/authentication/authenticator.ts b/x-pack/plugins/security/server/authentication/authenticator.ts index 22abfb0f05967..2f96d6fa7a00f 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.ts @@ -143,6 +143,15 @@ function isLoginAttemptWithProviderType( ); } +/** + * Determines if session value was created by the previous Kibana versions which had a different + * session value format. + * @param sessionValue The session value to check. + */ +function isLegacyProviderSession(sessionValue: any) { + return typeof sessionValue?.provider === 'string'; +} + /** * Instantiates authentication provider based on the provider key from config. * @param providerType Provider type key. @@ -503,18 +512,19 @@ export class Authenticator { * @param sessionStorage Session storage instance. */ private async getSessionValue(sessionStorage: SessionStorage) { - let sessionValue = await sessionStorage.get(); + const sessionValue = await sessionStorage.get(); - // If for some reason we have a session stored for the provider that is not available - // (e.g. when user was logged in with one provider, but then configuration has changed - // and that provider is no longer available), then we should clear session entirely. - const sessionProvider = sessionValue && this.providers.get(sessionValue.provider?.name); + // If we detect that session is in incompatible format or for some reason we have a session + // stored for the provider that is not available anymore (e.g. when user was logged in with one + // provider, but then configuration has changed and that provider is no longer available), then + // we should clear session entirely. if ( sessionValue && - (!sessionProvider || sessionProvider.type !== sessionValue?.provider.type) + (isLegacyProviderSession(sessionValue) || + this.providers.get(sessionValue.provider.name)?.type !== sessionValue.provider.type) ) { sessionStorage.clear(); - sessionValue = null; + return null; } return sessionValue; diff --git a/x-pack/plugins/security/server/routes/authentication/common.ts b/x-pack/plugins/security/server/routes/authentication/common.ts index f0f33d6624cc1..abab67c9cd1d2 100644 --- a/x-pack/plugins/security/server/routes/authentication/common.ts +++ b/x-pack/plugins/security/server/routes/authentication/common.ts @@ -111,7 +111,7 @@ export function defineCommonRoutes({ router, authc, basePath, logger }: RouteDef const { providerType, providerName, currentURL } = request.body; logger.info(`Logging in with provider "${providerName}" (${providerType})`); - const redirectURL = parseNext(currentURL ?? '', basePath.serverBasePath); + const redirectURL = parseNext(currentURL, basePath.serverBasePath); try { const authenticationResult = await authc.login(request, { provider: { name: providerName }, diff --git a/x-pack/test/login_selector_api_integration/apis/login_selector.ts b/x-pack/test/login_selector_api_integration/apis/login_selector.ts index 489f0265da57a..3be96d27186d9 100644 --- a/x-pack/test/login_selector_api_integration/apis/login_selector.ts +++ b/x-pack/test/login_selector_api_integration/apis/login_selector.ts @@ -275,10 +275,6 @@ export default function({ getService }: FtrProviderContext) { } }); - // Ideally we should be able to abandon intermediate session and let user log in, but for the - // time being we cannot distinguish errors coming from Elasticsearch for the case when SAML - // response just doesn't correspond to request ID we have in intermediate cookie and the case - // when something else has happened. it('should be able to log in via SP initiated login even if intermediate session with other SAML provider exists', async () => { // First start authentication flow with `saml1`. const saml1HandshakeResponse = await supertest @@ -383,6 +379,50 @@ export default function({ getService }: FtrProviderContext) { 'kerberos1' ); }); + + it('should be able to log in from Login Selector even if client provides certificate and PKI is enabled', async () => { + const spnegoResponse = await supertest + .post('/internal/security/login_with') + .ca(CA_CERT) + .pfx(CLIENT_CERT) + .set('kbn-xsrf', 'xxx') + .send({ + providerType: 'kerberos', + providerName: 'kerberos1', + currentURL: 'https://kibana.com/login?next=/abc/xyz/handshake?one=two%20three#/workpad', + }) + .expect(401); + + expect(spnegoResponse.headers['set-cookie']).to.be(undefined); + expect(spnegoResponse.headers['www-authenticate']).to.be('Negotiate'); + + const authenticationResponse = await supertest + .post('/internal/security/login_with') + .ca(CA_CERT) + .pfx(CLIENT_CERT) + .set('kbn-xsrf', 'xxx') + .set('Authorization', `Negotiate ${getSPNEGOToken()}`) + .send({ + providerType: 'kerberos', + providerName: 'kerberos1', + currentURL: 'https://kibana.com/login?next=/abc/xyz/handshake?one=two%20three#/workpad', + }) + .expect(200); + + // Verify that mutual authentication works. + expect(authenticationResponse.headers['www-authenticate']).to.be( + `Negotiate ${getMutualAuthenticationResponseToken()}` + ); + + const cookies = authenticationResponse.headers['set-cookie']; + expect(cookies).to.have.length(1); + + await checkSessionCookie( + request.cookie(cookies[0])!, + 'tester@TEST.ELASTIC.CO', + 'kerberos1' + ); + }); }); describe('OpenID Connect', () => {