From 5c395ef8fa7a0b4ccb83c2cda54b5807cdbac5d8 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Wed, 15 Apr 2020 13:11:45 +0200 Subject: [PATCH 01/15] Introduce Access Notice UI. --- .../security/{public => common}/types.ts | 2 +- .../access_notice_page.test.tsx.snap | 198 ++++++++++++++++++ .../access_notice/_access_notice_page.scss | 15 ++ .../authentication/access_notice/_index.scss | 1 + .../access_notice/access_notice_app.test.ts | 62 ++++++ .../access_notice/access_notice_app.ts | 38 ++++ .../access_notice/access_notice_page.test.tsx | 153 ++++++++++++++ .../access_notice/access_notice_page.tsx | 115 ++++++++++ .../authentication/access_notice/index.ts | 7 + .../authentication/authentication_service.ts | 2 + .../authentication_state_page.test.tsx.snap | 46 +++- .../authentication_state_page.test.tsx | 10 + .../authentication_state_page.tsx | 3 +- .../overwritten_session_page.test.tsx.snap | 2 +- x-pack/plugins/security/public/index.ts | 1 - .../public/session/session_timeout.test.tsx | 7 + .../public/session/session_timeout.tsx | 4 +- .../server/audit/audit_logger.test.ts | 20 +- .../security/server/audit/audit_logger.ts | 8 + .../security/server/audit/index.mock.ts | 1 + .../authentication/authenticator.test.ts | 188 ++++++++++++++++- .../server/authentication/authenticator.ts | 131 ++++++++++-- .../server/authentication/index.mock.ts | 1 + .../server/authentication/index.test.ts | 4 + .../security/server/authentication/index.ts | 8 +- x-pack/plugins/security/server/config.test.ts | 45 +--- x-pack/plugins/security/server/config.ts | 28 +-- x-pack/plugins/security/server/plugin.test.ts | 4 - x-pack/plugins/security/server/plugin.ts | 23 +- .../routes/authentication/common.test.ts | 40 ++++ .../server/routes/authentication/common.ts | 14 ++ .../routes/users/change_password.test.ts | 2 +- .../server/routes/views/access_notice.test.ts | 138 ++++++++++++ .../server/routes/views/access_notice.ts | 46 ++++ .../server/routes/views/index.test.ts | 15 +- .../security/server/routes/views/index.ts | 2 + .../server/routes/views/logged_out.test.ts | 2 +- .../server/routes/views/login.test.ts | 104 +++++---- .../security/server/routes/views/login.ts | 9 +- .../api_integration/apis/security/session.ts | 2 +- 40 files changed, 1356 insertions(+), 145 deletions(-) rename x-pack/plugins/security/{public => common}/types.ts (89%) create mode 100644 x-pack/plugins/security/public/authentication/access_notice/__snapshots__/access_notice_page.test.tsx.snap create mode 100644 x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss create mode 100644 x-pack/plugins/security/public/authentication/access_notice/_index.scss create mode 100644 x-pack/plugins/security/public/authentication/access_notice/access_notice_app.test.ts create mode 100644 x-pack/plugins/security/public/authentication/access_notice/access_notice_app.ts create mode 100644 x-pack/plugins/security/public/authentication/access_notice/access_notice_page.test.tsx create mode 100644 x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx create mode 100644 x-pack/plugins/security/public/authentication/access_notice/index.ts create mode 100644 x-pack/plugins/security/server/routes/views/access_notice.test.ts create mode 100644 x-pack/plugins/security/server/routes/views/access_notice.ts diff --git a/x-pack/plugins/security/public/types.ts b/x-pack/plugins/security/common/types.ts similarity index 89% rename from x-pack/plugins/security/public/types.ts rename to x-pack/plugins/security/common/types.ts index e9c4b6e281cf3e..32d15649944f1a 100644 --- a/x-pack/plugins/security/public/types.ts +++ b/x-pack/plugins/security/common/types.ts @@ -8,5 +8,5 @@ export interface SessionInfo { now: number; idleTimeoutExpiration: number | null; lifespanExpiration: number | null; - provider: string; + provider: { type: string; name: string }; } diff --git a/x-pack/plugins/security/public/authentication/access_notice/__snapshots__/access_notice_page.test.tsx.snap b/x-pack/plugins/security/public/authentication/access_notice/__snapshots__/access_notice_page.test.tsx.snap new file mode 100644 index 00000000000000..6dfed36f8af25e --- /dev/null +++ b/x-pack/plugins/security/public/authentication/access_notice/__snapshots__/access_notice_page.test.tsx.snap @@ -0,0 +1,198 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`AccessNoticePage renders as expected when state is available 1`] = ` + + } +> +
+
+
+ +
+ + + +
+ + + +

+ + Access Notice + +

+ + +
+ +
+
+
+
+ +
+ +
+ +
+ +
+ +
+ +
+

+ This is + + link + +

+
+ +
+ +
+ +
+ + +
+ +
+ + + + +
+ +
+ +
+ + +
+ + +
+
+ +`; diff --git a/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss b/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss new file mode 100644 index 00000000000000..25889a40cb7130 --- /dev/null +++ b/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss @@ -0,0 +1,15 @@ +.secAccessNoticePage__text { + @include euiScrollBar; + + max-height: $euiSize * 20; + overflow-y: auto; + padding: $euiSize; +} + +.secAccessNoticePage__acknowledge { + align-self: end; +} + +.secAccessNoticePage .secAuthenticationStatePage__content { + max-width: 600px; +} diff --git a/x-pack/plugins/security/public/authentication/access_notice/_index.scss b/x-pack/plugins/security/public/authentication/access_notice/_index.scss new file mode 100644 index 00000000000000..dc34db4de948f5 --- /dev/null +++ b/x-pack/plugins/security/public/authentication/access_notice/_index.scss @@ -0,0 +1 @@ +@import './access_notice_page'; diff --git a/x-pack/plugins/security/public/authentication/access_notice/access_notice_app.test.ts b/x-pack/plugins/security/public/authentication/access_notice/access_notice_app.test.ts new file mode 100644 index 00000000000000..d53fdb4b7a137f --- /dev/null +++ b/x-pack/plugins/security/public/authentication/access_notice/access_notice_app.test.ts @@ -0,0 +1,62 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +jest.mock('./access_notice_page'); + +import { AppMount, ScopedHistory } from 'src/core/public'; +import { accessNoticeApp } from './access_notice_app'; + +import { coreMock, scopedHistoryMock } from '../../../../../../src/core/public/mocks'; + +describe('accessNoticeApp', () => { + it('properly registers application', () => { + const coreSetupMock = coreMock.createSetup(); + + accessNoticeApp.create({ + application: coreSetupMock.application, + getStartServices: coreSetupMock.getStartServices, + }); + + expect(coreSetupMock.application.register).toHaveBeenCalledTimes(1); + + const [[appRegistration]] = coreSetupMock.application.register.mock.calls; + expect(appRegistration).toEqual({ + id: 'security_access_notice', + chromeless: true, + appRoute: '/security/access_notice', + title: 'Access Notice', + mount: expect.any(Function), + }); + }); + + it('properly renders application', async () => { + const coreSetupMock = coreMock.createSetup(); + const coreStartMock = coreMock.createStart(); + coreSetupMock.getStartServices.mockResolvedValue([coreStartMock, {}, {}]); + const containerMock = document.createElement('div'); + + accessNoticeApp.create({ + application: coreSetupMock.application, + getStartServices: coreSetupMock.getStartServices, + }); + + const [[{ mount }]] = coreSetupMock.application.register.mock.calls; + await (mount as AppMount)({ + element: containerMock, + appBasePath: '', + onAppLeave: jest.fn(), + history: (scopedHistoryMock.create() as unknown) as ScopedHistory, + }); + + const mockRenderApp = jest.requireMock('./access_notice_page').renderAccessNoticePage; + expect(mockRenderApp).toHaveBeenCalledTimes(1); + expect(mockRenderApp).toHaveBeenCalledWith(coreStartMock.i18n, containerMock, { + http: coreStartMock.http, + notifications: coreStartMock.notifications, + fatalErrors: coreStartMock.fatalErrors, + }); + }); +}); diff --git a/x-pack/plugins/security/public/authentication/access_notice/access_notice_app.ts b/x-pack/plugins/security/public/authentication/access_notice/access_notice_app.ts new file mode 100644 index 00000000000000..aa33650cf4662f --- /dev/null +++ b/x-pack/plugins/security/public/authentication/access_notice/access_notice_app.ts @@ -0,0 +1,38 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { i18n } from '@kbn/i18n'; +import { StartServicesAccessor, ApplicationSetup, AppMountParameters } from 'src/core/public'; + +interface CreateDeps { + application: ApplicationSetup; + getStartServices: StartServicesAccessor; +} + +export const accessNoticeApp = Object.freeze({ + id: 'security_access_notice', + create({ application, getStartServices }: CreateDeps) { + application.register({ + id: this.id, + title: i18n.translate('xpack.security.accessNoticeAppTitle', { + defaultMessage: 'Access Notice', + }), + chromeless: true, + appRoute: '/security/access_notice', + async mount({ element }: AppMountParameters) { + const [[coreStart], { renderAccessNoticePage }] = await Promise.all([ + getStartServices(), + import('./access_notice_page'), + ]); + return renderAccessNoticePage(coreStart.i18n, element, { + http: coreStart.http, + notifications: coreStart.notifications, + fatalErrors: coreStart.fatalErrors, + }); + }, + }); + }, +}); diff --git a/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.test.tsx b/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.test.tsx new file mode 100644 index 00000000000000..69e4c9f4b91f5a --- /dev/null +++ b/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.test.tsx @@ -0,0 +1,153 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import React from 'react'; +import { act } from '@testing-library/react'; +import { mountWithIntl, nextTick } from 'test_utils/enzyme_helpers'; +import { findTestSubject } from 'test_utils/find_test_subject'; +import { coreMock } from '../../../../../../src/core/public/mocks'; +import { AuthenticationStatePage } from '../components/authentication_state_page'; +import { AccessNoticePage } from './access_notice_page'; + +describe('AccessNoticePage', () => { + beforeAll(() => { + Object.defineProperty(window, 'location', { + value: { href: 'http://some-host/bar', protocol: 'http' }, + writable: true, + }); + }); + + afterAll(() => { + delete (window as any).location; + }); + + it('renders as expected when state is available', async () => { + const coreStartMock = coreMock.createStart(); + coreStartMock.http.get.mockResolvedValue({ accessNotice: 'This is [link](../link)' }); + + const wrapper = mountWithIntl( + + ); + + // Shouldn't render anything if state isn't yet available. + expect(wrapper.isEmptyRender()).toBe(true); + + await act(async () => { + await nextTick(); + wrapper.update(); + }); + + expect(wrapper.find(AuthenticationStatePage)).toMatchSnapshot(); + expect(coreStartMock.http.get).toHaveBeenCalledTimes(1); + expect(coreStartMock.http.get).toHaveBeenCalledWith('/internal/security/access_notice/state'); + expect(coreStartMock.fatalErrors.add).not.toHaveBeenCalled(); + }); + + it('fails when state is not available', async () => { + const coreStartMock = coreMock.createStart(); + const error = Symbol(); + coreStartMock.http.get.mockRejectedValue(error); + + const wrapper = mountWithIntl( + + ); + + await act(async () => { + await nextTick(); + wrapper.update(); + }); + + expect(coreStartMock.http.get).toHaveBeenCalledTimes(1); + expect(coreStartMock.http.get).toHaveBeenCalledWith('/internal/security/access_notice/state'); + expect(coreStartMock.fatalErrors.add).toHaveBeenCalledTimes(1); + expect(coreStartMock.fatalErrors.add).toHaveBeenCalledWith(error); + }); + + it('properly redirects after successful acknowledgement', async () => { + const coreStartMock = coreMock.createStart({ basePath: '/some-base-path' }); + coreStartMock.http.get.mockResolvedValue({ accessNotice: 'This is [link](../link)' }); + coreStartMock.http.post.mockResolvedValue(undefined); + + window.location.href = `https://some-host/security/access_notice?next=${encodeURIComponent( + '/some-base-path/app/kibana#/home?_g=()' + )}`; + const wrapper = mountWithIntl( + + ); + + await act(async () => { + await nextTick(); + wrapper.update(); + }); + + findTestSubject(wrapper, 'accessNoticeAcknowledge').simulate('click'); + + await act(async () => { + await nextTick(); + }); + + expect(coreStartMock.http.post).toHaveBeenCalledTimes(1); + expect(coreStartMock.http.post).toHaveBeenCalledWith( + '/internal/security/access_notice/acknowledge' + ); + + expect(window.location.href).toBe('/some-base-path/app/kibana#/home?_g=()'); + expect(coreStartMock.notifications.toasts.addError).not.toHaveBeenCalled(); + }); + + it('shows error toast if acknowledgement 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.get.mockResolvedValue({ accessNotice: 'This is [link](../link)' }); + coreStartMock.http.post.mockRejectedValue(failureReason); + + window.location.href = currentURL; + const wrapper = mountWithIntl( + + ); + + await act(async () => { + await nextTick(); + wrapper.update(); + }); + + findTestSubject(wrapper, 'accessNoticeAcknowledge').simulate('click'); + + await act(async () => { + await nextTick(); + }); + + expect(coreStartMock.http.post).toHaveBeenCalledTimes(1); + expect(coreStartMock.http.post).toHaveBeenCalledWith( + '/internal/security/access_notice/acknowledge' + ); + + expect(window.location.href).toBe(currentURL); + expect(coreStartMock.notifications.toasts.addError).toHaveBeenCalledWith(failureReason, { + title: 'Could not acknowledge access notice.', + }); + }); +}); diff --git a/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx b/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx new file mode 100644 index 00000000000000..91f414d434e33d --- /dev/null +++ b/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx @@ -0,0 +1,115 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import './_index.scss'; + +import React, { FormEvent, MouseEvent, useCallback, useEffect, useState } from 'react'; +import ReactDOM from 'react-dom'; +import ReactMarkdown from 'react-markdown'; +import { EuiButton, EuiFlexGroup, EuiFlexItem, EuiPanel, EuiSpacer, EuiText } from '@elastic/eui'; +import { i18n } from '@kbn/i18n'; +import { FormattedMessage } from '@kbn/i18n/react'; +import { CoreStart, FatalErrorsStart, HttpStart, NotificationsStart } from 'src/core/public'; + +import { parseNext } from '../../../common/parse_next'; +import { AuthenticationStatePage } from '../components'; + +interface Props { + http: HttpStart; + notifications: NotificationsStart; + fatalErrors: FatalErrorsStart; +} + +export function AccessNoticePage({ http, fatalErrors, notifications }: Props) { + const [isLoading, setIsLoading] = useState(false); + + const [accessNotice, setAccessNotice] = useState(null); + useEffect(() => { + http + .get<{ accessNotice: string }>('/internal/security/access_notice/state') + .then(response => setAccessNotice(response.accessNotice)) + .catch(err => fatalErrors.add(err)); + }, [http, fatalErrors]); + + const onAcknowledge = useCallback( + async (e: MouseEvent | FormEvent) => { + e.preventDefault(); + + try { + setIsLoading(true); + await http.post('/internal/security/access_notice/acknowledge'); + window.location.href = parseNext(window.location.href, http.basePath.serverBasePath); + } catch (err) { + notifications.toasts.addError(err, { + title: i18n.translate('xpack.security.accessNotice.acknowledgeErrorMessage', { + defaultMessage: 'Could not acknowledge access notice.', + }), + }); + + setIsLoading(false); + } + }, + [http, notifications] + ); + + if (accessNotice == null) { + return null; + } + + return ( + + } + > +
+ + + + + {accessNotice} + + + + + + + + + + + + +
+ ); +} + +export function renderAccessNoticePage( + i18nStart: CoreStart['i18n'], + element: Element, + props: Props +) { + ReactDOM.render( + + + , + element + ); + + return () => ReactDOM.unmountComponentAtNode(element); +} diff --git a/x-pack/plugins/security/public/authentication/access_notice/index.ts b/x-pack/plugins/security/public/authentication/access_notice/index.ts new file mode 100644 index 00000000000000..9f30bc12f13de8 --- /dev/null +++ b/x-pack/plugins/security/public/authentication/access_notice/index.ts @@ -0,0 +1,7 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +export { accessNoticeApp } from './access_notice_app'; diff --git a/x-pack/plugins/security/public/authentication/authentication_service.ts b/x-pack/plugins/security/public/authentication/authentication_service.ts index 979f7095cf933e..1e1c083c0c6b32 100644 --- a/x-pack/plugins/security/public/authentication/authentication_service.ts +++ b/x-pack/plugins/security/public/authentication/authentication_service.ts @@ -8,6 +8,7 @@ import { ApplicationSetup, StartServicesAccessor, HttpSetup } from 'src/core/pub import { AuthenticatedUser } from '../../common/model'; import { ConfigType } from '../config'; import { PluginStartDependencies } from '../plugin'; +import { accessNoticeApp } from './access_notice'; import { loginApp } from './login'; import { logoutApp } from './logout'; import { loggedOutApp } from './logged_out'; @@ -37,6 +38,7 @@ export class AuthenticationService { const getCurrentUser = async () => (await http.get('/internal/security/me', { asSystemRequest: true })) as AuthenticatedUser; + accessNoticeApp.create({ application, getStartServices }); loginApp.create({ application, config, getStartServices, http }); logoutApp.create({ application, http }); loggedOutApp.create({ application, getStartServices, http }); diff --git a/x-pack/plugins/security/public/authentication/components/authentication_state_page/__snapshots__/authentication_state_page.test.tsx.snap b/x-pack/plugins/security/public/authentication/components/authentication_state_page/__snapshots__/authentication_state_page.test.tsx.snap index 3590fa460a4010..76d8b73af68c1d 100644 --- a/x-pack/plugins/security/public/authentication/components/authentication_state_page/__snapshots__/authentication_state_page.test.tsx.snap +++ b/x-pack/plugins/security/public/authentication/components/authentication_state_page/__snapshots__/authentication_state_page.test.tsx.snap @@ -2,7 +2,51 @@ exports[`AuthenticationStatePage renders 1`] = `
+
+
+ + + + + +

+ foo +

+ + +
+
+
+ + hello world + +
+
+`; + +exports[`AuthenticationStatePage renders with custom CSS class 1`] = ` +
{ ) ).toMatchSnapshot(); }); + + it('renders with custom CSS class', () => { + expect( + shallowWithIntl( + + hello world + + ) + ).toMatchSnapshot(); + }); }); diff --git a/x-pack/plugins/security/public/authentication/components/authentication_state_page/authentication_state_page.tsx b/x-pack/plugins/security/public/authentication/components/authentication_state_page/authentication_state_page.tsx index aa306611299783..085e614a481485 100644 --- a/x-pack/plugins/security/public/authentication/components/authentication_state_page/authentication_state_page.tsx +++ b/x-pack/plugins/security/public/authentication/components/authentication_state_page/authentication_state_page.tsx @@ -8,11 +8,12 @@ import { EuiIcon, EuiSpacer, EuiTitle } from '@elastic/eui'; import React from 'react'; interface Props { + className?: string; title: React.ReactNode; } export const AuthenticationStatePage: React.FC = props => ( -
+
diff --git a/x-pack/plugins/security/public/authentication/overwritten_session/__snapshots__/overwritten_session_page.test.tsx.snap b/x-pack/plugins/security/public/authentication/overwritten_session/__snapshots__/overwritten_session_page.test.tsx.snap index 2ff760891fa4e2..632aaffc914338 100644 --- a/x-pack/plugins/security/public/authentication/overwritten_session/__snapshots__/overwritten_session_page.test.tsx.snap +++ b/x-pack/plugins/security/public/authentication/overwritten_session/__snapshots__/overwritten_session_page.test.tsx.snap @@ -11,7 +11,7 @@ exports[`OverwrittenSessionPage renders as expected 1`] = ` } >
{ now, idleTimeoutExpiration: now + 2 * 60 * 1000, lifespanExpiration: null, + provider: { type: 'basic', name: 'basic1' }, }; let notifications: ReturnType['notifications']; let http: ReturnType['http']; @@ -192,6 +193,7 @@ describe('Session Timeout', () => { now, idleTimeoutExpiration: null, lifespanExpiration: now + 2 * 60 * 1000, + provider: { type: 'basic', name: 'basic1' }, }; http.fetch.mockResolvedValue(sessionInfo); await sessionTimeout.start(); @@ -225,6 +227,7 @@ describe('Session Timeout', () => { now, idleTimeoutExpiration: null, lifespanExpiration: now + 2 * 60 * 1000, + provider: { type: 'basic', name: 'basic1' }, }; http.fetch.mockResolvedValue(sessionInfo); await sessionTimeout.start(); @@ -251,6 +254,7 @@ describe('Session Timeout', () => { now: now + elapsed, idleTimeoutExpiration: now + elapsed + 2 * 60 * 1000, lifespanExpiration: null, + provider: { type: 'basic', name: 'basic1' }, }); await sessionTimeout.extend('/foo'); expect(http.fetch).toHaveBeenCalledTimes(3); @@ -303,6 +307,7 @@ describe('Session Timeout', () => { now, idleTimeoutExpiration: now + 64 * 1000, lifespanExpiration: null, + provider: { type: 'basic', name: 'basic1' }, }); await sessionTimeout.start(); expect(http.fetch).toHaveBeenCalled(); @@ -336,6 +341,7 @@ describe('Session Timeout', () => { now: now + elapsed, idleTimeoutExpiration: now + elapsed + 2 * 60 * 1000, lifespanExpiration: null, + provider: { type: 'basic', name: 'basic1' }, }; http.fetch.mockResolvedValue(sessionInfo); await sessionTimeout.extend('/foo'); @@ -358,6 +364,7 @@ describe('Session Timeout', () => { now, idleTimeoutExpiration: now + 4 * 1000, lifespanExpiration: null, + provider: { type: 'basic', name: 'basic1' }, }); await sessionTimeout.start(); diff --git a/x-pack/plugins/security/public/session/session_timeout.tsx b/x-pack/plugins/security/public/session/session_timeout.tsx index bd6dbad7dbf149..b06d8fffd4b629 100644 --- a/x-pack/plugins/security/public/session/session_timeout.tsx +++ b/x-pack/plugins/security/public/session/session_timeout.tsx @@ -6,10 +6,10 @@ import { NotificationsSetup, Toast, HttpSetup, ToastInput } from 'src/core/public'; import { BroadcastChannel } from 'broadcast-channel'; +import { SessionInfo } from '../../common/types'; import { createToast as createIdleTimeoutToast } from './session_idle_timeout_warning'; import { createToast as createLifespanToast } from './session_lifespan_warning'; import { ISessionExpired } from './session_expired'; -import { SessionInfo } from '../types'; /** * Client session timeout is decreased by this number so that Kibana server @@ -127,7 +127,7 @@ export class SessionTimeout implements ISessionTimeout { this.sessionInfo = sessionInfo; // save the provider name in session storage, we will need it when we log out const key = `${this.tenant}/session_provider`; - sessionStorage.setItem(key, sessionInfo.provider); + sessionStorage.setItem(key, sessionInfo.provider.name); const { timeout, isLifespanTimeout } = this.getTimeout(); if (timeout == null) { diff --git a/x-pack/plugins/security/server/audit/audit_logger.test.ts b/x-pack/plugins/security/server/audit/audit_logger.test.ts index f7ee210a21a741..714c640861d3dd 100644 --- a/x-pack/plugins/security/server/audit/audit_logger.test.ts +++ b/x-pack/plugins/security/server/audit/audit_logger.test.ts @@ -62,7 +62,7 @@ describe(`#savedObjectsAuthorizationFailure`, () => { }); describe(`#savedObjectsAuthorizationSuccess`, () => { - test('logs via auditLogger when xpack.security.audit.enabled is true', () => { + test('logs via auditLogger', () => { const auditLogger = createMockAuditLogger(); const securityAuditLogger = new SecurityAuditLogger(() => auditLogger); const username = 'foo-user'; @@ -92,3 +92,21 @@ describe(`#savedObjectsAuthorizationSuccess`, () => { ); }); }); + +describe(`#accessNoticeAcknowledged`, () => { + test('logs via auditLogger', () => { + const auditLogger = createMockAuditLogger(); + const securityAuditLogger = new SecurityAuditLogger(() => auditLogger); + const username = 'foo-user'; + const provider = { type: 'saml', name: 'saml1' }; + + securityAuditLogger.accessNoticeAcknowledged(username, provider); + + expect(auditLogger.log).toHaveBeenCalledTimes(1); + expect(auditLogger.log).toHaveBeenCalledWith( + 'access_notice_acknowledged', + 'foo-user acknowledged access notice (saml/saml1).', + { username, provider } + ); + }); +}); diff --git a/x-pack/plugins/security/server/audit/audit_logger.ts b/x-pack/plugins/security/server/audit/audit_logger.ts index 40b525b5d21888..f40ed8030b1566 100644 --- a/x-pack/plugins/security/server/audit/audit_logger.ts +++ b/x-pack/plugins/security/server/audit/audit_logger.ts @@ -57,4 +57,12 @@ export class SecurityAuditLogger { } ); } + + accessNoticeAcknowledged(username: string, provider: { type: string; name: string }) { + this.getAuditLogger().log( + 'access_notice_acknowledged', + `${username} acknowledged access notice (${provider.type}/${provider.name}).`, + { username, provider } + ); + } } diff --git a/x-pack/plugins/security/server/audit/index.mock.ts b/x-pack/plugins/security/server/audit/index.mock.ts index c14b98ed4781eb..5b9f81fa11f7aa 100644 --- a/x-pack/plugins/security/server/audit/index.mock.ts +++ b/x-pack/plugins/security/server/audit/index.mock.ts @@ -11,6 +11,7 @@ export const securityAuditLoggerMock = { return ({ savedObjectsAuthorizationFailure: jest.fn(), savedObjectsAuthorizationSuccess: jest.fn(), + accessNoticeAcknowledged: jest.fn(), } as unknown) as jest.Mocked; }, }; diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts index a595b63faaf9b3..bd91858d7fcc99 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.test.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.test.ts @@ -21,6 +21,7 @@ import { sessionStorageMock, } from '../../../../../src/core/server/mocks'; import { mockAuthenticatedUser } from '../../common/model/authenticated_user.mock'; +import { securityAuditLoggerMock } from '../audit/index.mock'; import { ConfigSchema, createConfig } from '../config'; import { AuthenticationResult } from './authentication_result'; import { Authenticator, AuthenticatorOptions, ProviderSession } from './authenticator'; @@ -39,6 +40,8 @@ function getMockOptions({ selector?: AuthenticatorOptions['config']['authc']['selector']; } = {}) { return { + auditLogger: securityAuditLoggerMock.create(), + getCurrentUser: jest.fn(), clusterClient: elasticsearchServiceMock.createClusterClient(), basePath: httpServiceMock.createSetupContract().basePath, loggers: loggingServiceMock.create(), @@ -1108,6 +1111,124 @@ describe('Authenticator', () => { expect(mockBasicAuthenticationProvider.authenticate).not.toHaveBeenCalled(); }); }); + + describe('with Access Notice', () => { + const mockUser = mockAuthenticatedUser(); + beforeEach(() => { + mockOptions = getMockOptions({ + providers: { basic: { basic1: { order: 0, accessNotice: 'some notice' } } }, + }); + mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage); + + mockBasicAuthenticationProvider.authenticate.mockResolvedValue( + AuthenticationResult.succeeded(mockUser) + ); + + authenticator = new Authenticator(mockOptions); + }); + + it('does not redirect to Access Notice if there is no active session', async () => { + const request = httpServerMock.createKibanaRequest(); + mockSessionStorage.get.mockResolvedValue(null); + + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.succeeded(mockUser) + ); + }); + + it('does not redirect AJAX requests to Access Notice', async () => { + const request = httpServerMock.createKibanaRequest({ headers: { 'kbn-xsrf': 'xsrf' } }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); + + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.succeeded(mockUser) + ); + }); + + it('does not redirect to Access Notice if request cannot be handled', async () => { + const request = httpServerMock.createKibanaRequest(); + mockSessionStorage.get.mockResolvedValue(mockSessVal); + + mockBasicAuthenticationProvider.authenticate.mockResolvedValue( + AuthenticationResult.notHandled() + ); + + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.notHandled() + ); + }); + + it('does not redirect to Access Notice if authentication fails', async () => { + const request = httpServerMock.createKibanaRequest(); + mockSessionStorage.get.mockResolvedValue(mockSessVal); + + const failureReason = new Error('something went wrong'); + mockBasicAuthenticationProvider.authenticate.mockResolvedValue( + AuthenticationResult.failed(failureReason) + ); + + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.failed(failureReason) + ); + }); + + it('does not redirect to Access Notice if redirect is required to complete authentication', async () => { + const request = httpServerMock.createKibanaRequest(); + mockSessionStorage.get.mockResolvedValue(mockSessVal); + + mockBasicAuthenticationProvider.authenticate.mockResolvedValue( + AuthenticationResult.redirectTo('/some-url') + ); + + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.redirectTo('/some-url') + ); + }); + + it('does not redirect to Access Notice if user has already acknowledged it', async () => { + const request = httpServerMock.createKibanaRequest(); + mockSessionStorage.get.mockResolvedValue({ + ...mockSessVal, + flags: { accessNoticeAcknowledged: true }, + }); + + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.succeeded(mockUser) + ); + }); + + it('does not redirect to Access Notice its own requests', async () => { + const request = httpServerMock.createKibanaRequest({ path: '/security/access_notice' }); + mockSessionStorage.get.mockResolvedValue(mockSessVal); + + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.succeeded(mockUser) + ); + }); + + it('does not redirect to Access Notice if it is not configured', async () => { + mockOptions = getMockOptions({ providers: { basic: { basic1: { order: 0 } } } }); + mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage); + mockSessionStorage.get.mockResolvedValue(mockSessVal); + authenticator = new Authenticator(mockOptions); + + const request = httpServerMock.createKibanaRequest(); + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.succeeded(mockUser) + ); + }); + + it('redirects to Access Notice when needed.', async () => { + mockSessionStorage.get.mockResolvedValue(mockSessVal); + + const request = httpServerMock.createKibanaRequest(); + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.redirectTo( + '/mock-server-basepath/security/access_notice?next=%2Fmock-server-basepath%2Fpath' + ) + ); + }); + }); }); describe('`logout` method', () => { @@ -1228,13 +1349,13 @@ describe('Authenticator', () => { now: currentDate, idleTimeoutExpiration: currentDate + 60000, lifespanExpiration: currentDate + 120000, - provider: 'basic1', + provider: { type: 'basic' as 'basic', name: 'basic1' }, }; mockSessionStorage.get.mockResolvedValue({ idleTimeoutExpiration: mockInfo.idleTimeoutExpiration, lifespanExpiration: mockInfo.lifespanExpiration, state, - provider: { type: 'basic', name: mockInfo.provider }, + provider: mockInfo.provider, path: mockOptions.basePath.serverBasePath, }); jest.spyOn(Date, 'now').mockImplementation(() => currentDate); @@ -1274,4 +1395,67 @@ describe('Authenticator', () => { expect(authenticator.isProviderTypeEnabled('saml')).toBe(true); }); }); + + describe('`acknowledgeAccessNotice` method', () => { + let authenticator: Authenticator; + let mockOptions: ReturnType; + let mockSessionStorage: jest.Mocked>; + let mockSessionValue: any; + beforeEach(() => { + mockOptions = getMockOptions({ providers: { basic: { basic1: { order: 0 } } } }); + mockSessionStorage = sessionStorageMock.create(); + mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage); + mockSessionValue = { + idleTimeoutExpiration: null, + lifespanExpiration: null, + state: { authorization: 'Basic xxx' }, + provider: { type: 'basic', name: 'basic1' }, + path: mockOptions.basePath.serverBasePath, + }; + mockSessionStorage.get.mockResolvedValue(mockSessionValue); + mockOptions.getCurrentUser.mockReturnValue(mockAuthenticatedUser()); + + authenticator = new Authenticator(mockOptions); + }); + + it('fails if user is not authenticated', async () => { + mockOptions.getCurrentUser.mockReturnValue(null); + + await expect( + authenticator.acknowledgeAccessNotice(httpServerMock.createKibanaRequest()) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Cannot acknowledge access notice for unauthenticated user."` + ); + + expect(mockSessionStorage.set).not.toHaveBeenCalled(); + }); + + it('fails if cannot retrieve user session', async () => { + mockSessionStorage.get.mockResolvedValue(null); + + await expect( + authenticator.acknowledgeAccessNotice(httpServerMock.createKibanaRequest()) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Cannot acknowledge access notice for unauthenticated user."` + ); + + expect(mockSessionStorage.set).not.toHaveBeenCalled(); + }); + + it('properly acknowledges access notice for the authenticated user', async () => { + await authenticator.acknowledgeAccessNotice(httpServerMock.createKibanaRequest()); + + expect(mockSessionStorage.set).toHaveBeenCalledTimes(1); + expect(mockSessionStorage.set).toHaveBeenCalledWith({ + ...mockSessionValue, + flags: { accessNoticeAcknowledged: true }, + }); + + expect(mockOptions.auditLogger.accessNoticeAcknowledged).toHaveBeenCalledTimes(1); + expect(mockOptions.auditLogger.accessNoticeAcknowledged).toHaveBeenCalledWith('user', { + type: 'basic', + name: 'basic1', + }); + }); + }); }); diff --git a/x-pack/plugins/security/server/authentication/authenticator.ts b/x-pack/plugins/security/server/authentication/authenticator.ts index caf5b485d05e35..d2b2a61397e9e1 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.ts @@ -14,6 +14,9 @@ import { HttpServiceSetup, IClusterClient, } from '../../../../../src/core/server'; +import { AuthenticatedUser } from '../../common/model'; +import { SessionInfo } from '../../common/types'; +import { SecurityAuditLogger } from '../audit'; import { ConfigType } from '../config'; import { getErrorStatusCode } from '../errors'; @@ -32,7 +35,6 @@ import { import { AuthenticationResult } from './authentication_result'; import { DeauthenticationResult } from './deauthentication_result'; import { Tokens } from './tokens'; -import { SessionInfo } from '../../public'; import { canRedirectRequest } from './can_redirect_request'; import { HTTPAuthorizationHeader } from './http_authentication'; @@ -67,6 +69,16 @@ export interface ProviderSession { * Cookie "Path" attribute that is validated against the current Kibana server configuration. */ path: string; + + /** + * The set of flags used to describe various aspects of the user session. + */ + flags?: { + /** + * Indicates whether user acknowledged access notice or not. + */ + accessNoticeAcknowledged: boolean; + }; } /** @@ -85,6 +97,8 @@ export interface ProviderLoginAttempt { } export interface AuthenticatorOptions { + auditLogger: SecurityAuditLogger; + getCurrentUser: (request: KibanaRequest) => AuthenticatedUser | null; config: Pick; basePath: HttpServiceSetup['basePath']; loggers: LoggerFactory; @@ -109,6 +123,11 @@ const providerMap = new Map< [PKIAuthenticationProvider.type, PKIAuthenticationProvider], ]); +/** + * The route to the access notice UI. + */ +const ACCESS_NOTICE_ROUTE = '/security/access_notice'; + function assertRequest(request: KibanaRequest) { if (!(request instanceof KibanaRequest)) { throw new Error(`Request should be a valid "KibanaRequest" instance, was [${typeof request}].`); @@ -341,14 +360,7 @@ export class Authenticator { const sessionStorage = this.options.sessionStorageFactory.asScoped(request); const existingSession = await this.getSessionValue(sessionStorage); - // If request doesn't have any session information, isn't attributed with HTTP Authorization - // header and Login Selector is enabled, we must redirect user to the login selector. - const useLoginSelector = - !existingSession && - this.options.config.authc.selector.enabled && - canRedirectRequest(request) && - HTTPAuthorizationHeader.parseFromRequest(request) == null; - if (useLoginSelector) { + if (this.shouldRedirectToLoginSelector(request, existingSession)) { this.logger.debug('Redirecting request to Login Selector.'); return AuthenticationResult.redirectTo( `${this.options.basePath.serverBasePath}/login?next=${encodeURIComponent( @@ -368,7 +380,7 @@ export class Authenticator { ownsSession ? existingSession!.state : null ); - this.updateSessionValue(sessionStorage, { + const updatedSession = this.updateSessionValue(sessionStorage, { provider: { type: provider.type, name: providerName }, isSystemRequest: request.isSystemRequest, authenticationResult, @@ -376,6 +388,20 @@ export class Authenticator { }); if (!authenticationResult.notHandled()) { + if ( + authenticationResult.succeeded() && + this.shouldRedirectToAccessNotice(request, updatedSession) + ) { + this.logger.debug('Redirecting user to the access notice screen.'); + return AuthenticationResult.redirectTo( + `${ + this.options.basePath.serverBasePath + }${ACCESS_NOTICE_ROUTE}?next=${encodeURIComponent( + `${this.options.basePath.get(request)}${request.url.path}` + )}` + ); + } + return authenticationResult; } } @@ -441,7 +467,7 @@ export class Authenticator { now: Date.now(), idleTimeoutExpiration: sessionValue.idleTimeoutExpiration, lifespanExpiration: sessionValue.lifespanExpiration, - provider: sessionValue.provider.name, + provider: sessionValue.provider, }; } return null; @@ -455,6 +481,31 @@ export class Authenticator { return [...this.providers.values()].some(provider => provider.type === providerType); } + /** + * Acknowledges access notice on behalf of the currently authenticated user. + * @param request Request instance. + */ + async acknowledgeAccessNotice(request: KibanaRequest) { + assertRequest(request); + + const sessionStorage = this.options.sessionStorageFactory.asScoped(request); + const existingSession = await this.getSessionValue(sessionStorage); + const currentUser = this.options.getCurrentUser(request); + if (!existingSession || !currentUser) { + throw new Error('Cannot acknowledge access notice for unauthenticated user.'); + } + + sessionStorage.set({ + ...existingSession, + flags: { ...(existingSession.flags || {}), accessNoticeAcknowledged: true }, + }); + + this.options.auditLogger.accessNoticeAcknowledged( + currentUser.username, + existingSession.provider + ); + } + /** * Initializes HTTP Authentication provider and appends it to the end of the list of enabled * authentication providers. @@ -545,7 +596,7 @@ export class Authenticator { } ) { if (!existingSession && !authenticationResult.shouldUpdateState()) { - return; + return null; } // If authentication succeeds or requires redirect we should automatically extend existing user session, @@ -563,9 +614,12 @@ export class Authenticator { (authenticationResult.failed() && getErrorStatusCode(authenticationResult.error) === 401) ) { sessionStorage.clear(); - } else if (sessionCanBeUpdated) { + return null; + } + + if (sessionCanBeUpdated) { const { idleTimeoutExpiration, lifespanExpiration } = this.calculateExpiry(existingSession); - sessionStorage.set({ + const updatedSession = { state: authenticationResult.shouldUpdateState() ? authenticationResult.state : existingSession!.state, @@ -573,8 +627,13 @@ export class Authenticator { idleTimeoutExpiration, lifespanExpiration, path: this.serverBasePath, - }); + flags: existingSession?.flags, + }; + sessionStorage.set(updatedSession); + return updatedSession; } + + return existingSession; } private getProviderName(query: any): string | null { @@ -600,4 +659,46 @@ export class Authenticator { return { idleTimeoutExpiration, lifespanExpiration }; } + + /** + * Checks whether request should be redirected to the Login Selector UI. + * @param request Request instance. + * @param session Current session value if any. + */ + private shouldRedirectToLoginSelector(request: KibanaRequest, session: ProviderSession | null) { + // Request should be redirected to Login Selector UI only if all following conditions are met: + // 1. Request can be redirected (not API call) + // 2. Request is not authenticated yet + // 3. Login Selector UI is enabled + // 4. Request isn't attributed with HTTP Authorization header + return ( + canRedirectRequest(request) && + !session && + this.options.config.authc.selector.enabled && + HTTPAuthorizationHeader.parseFromRequest(request) == null + ); + } + + /** + * Checks whether request should be redirected to the Access Notice UI. + * @param request Request instance. + * @param session Current session value if any. + */ + private shouldRedirectToAccessNotice(request: KibanaRequest, session: ProviderSession | null) { + // Request should be redirected to Access Notice UI only if all following conditions are met: + // 1. Request can be redirected (not API call) + // 2. Request is authenticated, but user hasn't acknowledged access notice in the current + // session yet (based on the flag we store in the session) + // 3. Request is authenticated by the provider that has `accessNotice` configured + // 4. And it's not a request to the Access Notice UI itself + return ( + canRedirectRequest(request) && + session != null && + !session.flags?.accessNoticeAcknowledged && + (this.options.config.authc.providers as Record)[session.provider.type]?.[ + session.provider.name + ]?.accessNotice && + request.url.pathname !== ACCESS_NOTICE_ROUTE + ); + } } diff --git a/x-pack/plugins/security/server/authentication/index.mock.ts b/x-pack/plugins/security/server/authentication/index.mock.ts index 8092c1c81017b5..1bd95cd38e13fc 100644 --- a/x-pack/plugins/security/server/authentication/index.mock.ts +++ b/x-pack/plugins/security/server/authentication/index.mock.ts @@ -18,5 +18,6 @@ export const authenticationMock = { invalidateAPIKeyAsInternalUser: jest.fn(), isAuthenticated: jest.fn(), getSessionInfo: jest.fn(), + acknowledgeAccessNotice: jest.fn(), }), }; diff --git a/x-pack/plugins/security/server/authentication/index.test.ts b/x-pack/plugins/security/server/authentication/index.test.ts index 6609f8707976b7..1c1e0ed781f18e 100644 --- a/x-pack/plugins/security/server/authentication/index.test.ts +++ b/x-pack/plugins/security/server/authentication/index.test.ts @@ -19,6 +19,7 @@ import { elasticsearchServiceMock, } from '../../../../../src/core/server/mocks'; import { mockAuthenticatedUser } from '../../common/model/authenticated_user.mock'; +import { securityAuditLoggerMock } from '../audit/index.mock'; import { AuthenticationHandler, @@ -40,9 +41,11 @@ import { InvalidateAPIKeyParams, } from './api_keys'; import { SecurityLicense } from '../../common/licensing'; +import { SecurityAuditLogger } from '../audit'; describe('setupAuthentication()', () => { let mockSetupAuthenticationParams: { + auditLogger: jest.Mocked; config: ConfigType; loggers: LoggerFactory; http: jest.Mocked; @@ -52,6 +55,7 @@ describe('setupAuthentication()', () => { let mockScopedClusterClient: jest.Mocked>; beforeEach(() => { mockSetupAuthenticationParams = { + auditLogger: securityAuditLoggerMock.create(), http: coreMock.createSetup().http, config: createConfig( ConfigSchema.validate({ diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts index 5d7b49de68d286..153fdf68e02897 100644 --- a/x-pack/plugins/security/server/authentication/index.ts +++ b/x-pack/plugins/security/server/authentication/index.ts @@ -10,12 +10,13 @@ import { KibanaRequest, LoggerFactory, } from '../../../../../src/core/server'; +import { SecurityLicense } from '../../common/licensing'; import { AuthenticatedUser } from '../../common/model'; +import { SecurityAuditLogger } from '../audit'; import { ConfigType } from '../config'; import { getErrorStatusCode } from '../errors'; import { Authenticator, ProviderSession } from './authenticator'; import { APIKeys, CreateAPIKeyParams, InvalidateAPIKeyParams } from './api_keys'; -import { SecurityLicense } from '../../common/licensing'; export { canRedirectRequest } from './can_redirect_request'; export { Authenticator, ProviderLoginAttempt } from './authenticator'; @@ -35,6 +36,7 @@ export { } from './http_authentication'; interface SetupAuthenticationParams { + auditLogger: SecurityAuditLogger; http: CoreSetup['http']; clusterClient: IClusterClient; config: ConfigType; @@ -45,6 +47,7 @@ interface SetupAuthenticationParams { export type Authentication = UnwrapPromise>; export async function setupAuthentication({ + auditLogger, http, clusterClient, config, @@ -82,6 +85,8 @@ export async function setupAuthentication({ }; const authenticator = new Authenticator({ + auditLogger, + getCurrentUser, clusterClient, basePath: http.basePath, config: { session: config.session, authc: config.authc }, @@ -171,6 +176,7 @@ export async function setupAuthentication({ logout: authenticator.logout.bind(authenticator), getSessionInfo: authenticator.getSessionInfo.bind(authenticator), isProviderTypeEnabled: authenticator.isProviderTypeEnabled.bind(authenticator), + acknowledgeAccessNotice: authenticator.acknowledgeAccessNotice.bind(authenticator), getCurrentUser, createAPIKey: (request: KibanaRequest, params: CreateAPIKeyParams) => apiKeys.create(request, params), diff --git a/x-pack/plugins/security/server/config.test.ts b/x-pack/plugins/security/server/config.test.ts index 46a7ee79ee60c5..f7217fb41b708e 100644 --- a/x-pack/plugins/security/server/config.test.ts +++ b/x-pack/plugins/security/server/config.test.ts @@ -27,6 +27,7 @@ describe('config schema', () => { "providers": Object { "basic": Object { "basic": Object { + "accessNotice": undefined, "description": undefined, "enabled": true, "order": 0, @@ -69,6 +70,7 @@ describe('config schema', () => { "providers": Object { "basic": Object { "basic": Object { + "accessNotice": undefined, "description": undefined, "enabled": true, "order": 0, @@ -111,6 +113,7 @@ describe('config schema', () => { "providers": Object { "basic": Object { "basic": Object { + "accessNotice": undefined, "description": undefined, "enabled": true, "order": 0, @@ -911,20 +914,12 @@ describe('createConfig()', () => { "sortedProviders": Array [ Object { "name": "saml", - "options": Object { - "description": undefined, - "order": 0, - "showInSelector": true, - }, + "order": 0, "type": "saml", }, Object { "name": "basic", - "options": Object { - "description": undefined, - "order": 1, - "showInSelector": true, - }, + "order": 1, "type": "basic", }, ], @@ -1015,47 +1010,27 @@ describe('createConfig()', () => { Array [ Object { "name": "oidc1", - "options": Object { - "description": undefined, - "order": 0, - "showInSelector": true, - }, + "order": 0, "type": "oidc", }, Object { "name": "saml2", - "options": Object { - "description": undefined, - "order": 1, - "showInSelector": true, - }, + "order": 1, "type": "saml", }, Object { "name": "saml1", - "options": Object { - "description": undefined, - "order": 2, - "showInSelector": true, - }, + "order": 2, "type": "saml", }, Object { "name": "basic1", - "options": Object { - "description": undefined, - "order": 3, - "showInSelector": true, - }, + "order": 3, "type": "basic", }, Object { "name": "oidc2", - "options": Object { - "description": undefined, - "order": 4, - "showInSelector": true, - }, + "order": 4, "type": "oidc", }, ] diff --git a/x-pack/plugins/security/server/config.ts b/x-pack/plugins/security/server/config.ts index 97ff7d00a43362..b9d7182ce6f4dd 100644 --- a/x-pack/plugins/security/server/config.ts +++ b/x-pack/plugins/security/server/config.ts @@ -30,6 +30,7 @@ function getCommonProviderSchemaProperties(overrides: Partial = []; for (const [type, providerGroup] of Object.entries(providers)) { - for (const [name, { enabled, showInSelector, order, description }] of Object.entries( - providerGroup ?? {} - )) { + for (const [name, { enabled, order }] of Object.entries(providerGroup ?? {})) { if (!enabled) { delete providerGroup![name]; } else { - sortedProviders.push({ - type: type as any, - name, - options: { order, showInSelector, description }, - }); + sortedProviders.push({ type: type as any, name, order }); } } } - sortedProviders.sort(({ options: { order: orderA } }, { options: { order: orderB } }) => + sortedProviders.sort(({ order: orderA }, { order: orderB }) => orderA < orderB ? -1 : orderA > orderB ? 1 : 0 ); @@ -253,7 +256,8 @@ export function createConfig( typeof config.authc.selector.enabled === 'boolean' ? config.authc.selector.enabled : !isUsingLegacyProvidersFormat && - sortedProviders.filter(provider => provider.options.showInSelector).length > 1; + sortedProviders.filter(({ type, name }) => providers[type]?.[name].showInSelector).length > + 1; return { ...config, diff --git a/x-pack/plugins/security/server/plugin.test.ts b/x-pack/plugins/security/server/plugin.test.ts index 4767f57de764c2..67994f23966d32 100644 --- a/x-pack/plugins/security/server/plugin.test.ts +++ b/x-pack/plugins/security/server/plugin.test.ts @@ -71,14 +71,10 @@ describe('Security Plugin', () => { "authc": Object { "createAPIKey": [Function], "getCurrentUser": [Function], - "getSessionInfo": [Function], "grantAPIKeyAsInternalUser": [Function], "invalidateAPIKey": [Function], "invalidateAPIKeyAsInternalUser": [Function], "isAuthenticated": [Function], - "isProviderTypeEnabled": [Function], - "login": [Function], - "logout": [Function], }, "authz": Object { "actions": Actions { diff --git a/x-pack/plugins/security/server/plugin.ts b/x-pack/plugins/security/server/plugin.ts index 9dd4aaafa3494f..3572dc5c1a8698 100644 --- a/x-pack/plugins/security/server/plugin.ts +++ b/x-pack/plugins/security/server/plugin.ts @@ -48,7 +48,15 @@ export interface LegacyAPI { * Describes public Security plugin contract returned at the `setup` stage. */ export interface SecurityPluginSetup { - authc: Authentication; + authc: Pick< + Authentication, + | 'isAuthenticated' + | 'getCurrentUser' + | 'createAPIKey' + | 'invalidateAPIKey' + | 'grantAPIKeyAsInternalUser' + | 'invalidateAPIKeyAsInternalUser' + >; authz: Pick; /** @@ -126,7 +134,9 @@ export class Plugin { license$: licensing.license$, }); + const auditLogger = new SecurityAuditLogger(() => this.getLegacyAPI().auditLogger); const authc = await setupAuthentication({ + auditLogger, http: core.http, clusterClient: this.clusterClient, config, @@ -146,7 +156,7 @@ export class Plugin { }); setupSavedObjects({ - auditLogger: new SecurityAuditLogger(() => this.getLegacyAPI().auditLogger), + auditLogger, authz, savedObjects: core.savedObjects, getSpacesService: this.getSpacesService, @@ -167,7 +177,14 @@ export class Plugin { }); return deepFreeze({ - authc, + authc: { + isAuthenticated: authc.isAuthenticated, + getCurrentUser: authc.getCurrentUser, + createAPIKey: authc.createAPIKey, + invalidateAPIKey: authc.invalidateAPIKey, + grantAPIKeyAsInternalUser: authc.grantAPIKeyAsInternalUser, + invalidateAPIKeyAsInternalUser: authc.invalidateAPIKeyAsInternalUser, + }, authz: { actions: authz.actions, diff --git a/x-pack/plugins/security/server/routes/authentication/common.test.ts b/x-pack/plugins/security/server/routes/authentication/common.test.ts index 156c03e90210b7..56d725806aaa97 100644 --- a/x-pack/plugins/security/server/routes/authentication/common.test.ts +++ b/x-pack/plugins/security/server/routes/authentication/common.test.ts @@ -433,4 +433,44 @@ describe('Common authentication routes', () => { }); }); }); + + describe('acknowledge access notice', () => { + let routeHandler: RequestHandler; + let routeConfig: RouteConfig; + beforeEach(() => { + const [acsRouteConfig, acsRouteHandler] = router.post.mock.calls.find( + ([{ path }]) => path === '/internal/security/access_notice/acknowledge' + )!; + + routeConfig = acsRouteConfig; + routeHandler = acsRouteHandler; + }); + + it('correctly defines route.', () => { + expect(routeConfig.options).toBeUndefined(); + expect(routeConfig.validate).toBe(false); + }); + + it('returns 500 if acknowledge throws unhandled exception.', async () => { + const unhandledException = new Error('Something went wrong.'); + authc.acknowledgeAccessNotice.mockRejectedValue(unhandledException); + + const request = httpServerMock.createKibanaRequest(); + await expect(routeHandler(mockContext, request, kibanaResponseFactory)).resolves.toEqual({ + status: 500, + payload: 'Internal Error', + options: {}, + }); + }); + + it('returns 204 if successfully acknowledged.', async () => { + authc.acknowledgeAccessNotice.mockResolvedValue(undefined); + + const request = httpServerMock.createKibanaRequest(); + await expect(routeHandler(mockContext, request, kibanaResponseFactory)).resolves.toEqual({ + status: 204, + options: {}, + }); + }); + }); }); diff --git a/x-pack/plugins/security/server/routes/authentication/common.ts b/x-pack/plugins/security/server/routes/authentication/common.ts index abab67c9cd1d28..2ad495f6bfbea2 100644 --- a/x-pack/plugins/security/server/routes/authentication/common.ts +++ b/x-pack/plugins/security/server/routes/authentication/common.ts @@ -135,4 +135,18 @@ export function defineCommonRoutes({ router, authc, basePath, logger }: RouteDef } }) ); + + router.post( + { path: '/internal/security/access_notice/acknowledge', validate: false }, + async (context, request, response) => { + try { + await authc.acknowledgeAccessNotice(request); + } catch (err) { + logger.error(err); + return response.internalError(); + } + + return response.noContent(); + } + ); } diff --git a/x-pack/plugins/security/server/routes/users/change_password.test.ts b/x-pack/plugins/security/server/routes/users/change_password.test.ts index fd05821f9d5206..c163ff4e256cd2 100644 --- a/x-pack/plugins/security/server/routes/users/change_password.test.ts +++ b/x-pack/plugins/security/server/routes/users/change_password.test.ts @@ -53,7 +53,7 @@ describe('Change password', () => { now: Date.now(), idleTimeoutExpiration: null, lifespanExpiration: null, - provider: 'basic', + provider: { type: 'basic', name: 'basic' }, }); mockScopedClusterClient = elasticsearchServiceMock.createScopedClusterClient(); diff --git a/x-pack/plugins/security/server/routes/views/access_notice.test.ts b/x-pack/plugins/security/server/routes/views/access_notice.test.ts new file mode 100644 index 00000000000000..04011009bb571a --- /dev/null +++ b/x-pack/plugins/security/server/routes/views/access_notice.test.ts @@ -0,0 +1,138 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { + RequestHandler, + RouteConfig, + kibanaResponseFactory, + IRouter, + HttpResources, + HttpResourcesRequestHandler, +} from '../../../../../../src/core/server'; +import { ConfigType } from '../../config'; +import { defineAccessNoticeRoutes } from './access_notice'; + +import { + coreMock, + httpResourcesMock, + httpServerMock, +} from '../../../../../../src/core/server/mocks'; +import { routeDefinitionParamsMock } from '../index.mock'; +import { Authentication } from '../../authentication'; + +describe('Access notice view routes', () => { + let httpResources: jest.Mocked; + let router: jest.Mocked; + let config: ConfigType; + let authc: jest.Mocked; + beforeEach(() => { + const routeParamsMock = routeDefinitionParamsMock.create(); + router = routeParamsMock.router; + httpResources = routeParamsMock.httpResources; + authc = routeParamsMock.authc; + config = routeParamsMock.config; + + defineAccessNoticeRoutes(routeParamsMock); + }); + + describe('View route', () => { + let routeHandler: HttpResourcesRequestHandler; + let routeConfig: RouteConfig; + beforeEach(() => { + const [viewRouteConfig, viewRouteHandler] = httpResources.register.mock.calls.find( + ([{ path }]) => path === '/security/access_notice' + )!; + + routeConfig = viewRouteConfig; + routeHandler = viewRouteHandler; + }); + + it('correctly defines route.', () => { + expect(routeConfig.options).toBeUndefined(); + expect(routeConfig.validate).toBe(false); + }); + + it('renders view.', async () => { + const request = httpServerMock.createKibanaRequest(); + const contextMock = coreMock.createRequestHandlerContext(); + const responseFactory = httpResourcesMock.createResponseFactory(); + + await routeHandler({ core: contextMock } as any, request, responseFactory); + + expect(responseFactory.renderCoreApp).toHaveBeenCalledWith(); + }); + }); + + describe('Access notice state route', () => { + let routeHandler: RequestHandler; + let routeConfig: RouteConfig; + beforeEach(() => { + const [loginStateRouteConfig, loginStateRouteHandler] = router.get.mock.calls.find( + ([{ path }]) => path === '/internal/security/access_notice/state' + )!; + + routeConfig = loginStateRouteConfig; + routeHandler = loginStateRouteHandler; + }); + + it('correctly defines route.', () => { + expect(routeConfig.options).toBeUndefined(); + expect(routeConfig.validate).toBe(false); + }); + + it('returns empty `accessNotice` if session info is not available.', async () => { + const request = httpServerMock.createKibanaRequest(); + const contextMock = coreMock.createRequestHandlerContext(); + + authc.getSessionInfo.mockResolvedValue(null); + + await expect( + routeHandler({ core: contextMock } as any, request, kibanaResponseFactory) + ).resolves.toEqual({ + options: { body: { accessNotice: '' } }, + payload: { accessNotice: '' }, + status: 200, + }); + }); + + it('returns non-empty `accessNotice` only if it is configured.', async () => { + const request = httpServerMock.createKibanaRequest(); + const contextMock = coreMock.createRequestHandlerContext(); + + config.authc = routeDefinitionParamsMock.create({ + authc: { + providers: { + basic: { basic1: { order: 0 } }, + saml: { saml1: { order: 1, realm: 'realm1', accessNotice: 'Some access notice' } }, + }, + }, + }).config.authc; + + const cases: Array<[{ type: string; name: string }, string]> = [ + [{ type: 'basic', name: 'basic1' }, ''], + [{ type: 'saml', name: 'saml1' }, 'Some access notice'], + [{ type: 'unknown-type', name: 'unknown-name' }, ''], + ]; + + for (const [sessionProvider, expectedAccessNotice] of cases) { + authc.getSessionInfo.mockResolvedValue({ + now: Date.now(), + idleTimeoutExpiration: null, + lifespanExpiration: null, + provider: sessionProvider, + }); + + await expect( + routeHandler({ core: contextMock } as any, request, kibanaResponseFactory) + ).resolves.toEqual({ + options: { body: { accessNotice: expectedAccessNotice } }, + payload: { accessNotice: expectedAccessNotice }, + status: 200, + }); + } + }); + }); +}); diff --git a/x-pack/plugins/security/server/routes/views/access_notice.ts b/x-pack/plugins/security/server/routes/views/access_notice.ts new file mode 100644 index 00000000000000..b3b80ca2fdf769 --- /dev/null +++ b/x-pack/plugins/security/server/routes/views/access_notice.ts @@ -0,0 +1,46 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { RouteDefinitionParams } from '..'; + +/** + * Defines routes required for the Access Notice view. + */ +export function defineAccessNoticeRoutes({ + authc, + httpResources, + config, + router, + logger, +}: RouteDefinitionParams) { + httpResources.register( + { path: '/security/access_notice', validate: false }, + async (context, request, response) => response.renderCoreApp() + ); + + router.get( + { path: '/internal/security/access_notice/state', validate: false }, + async (context, request, response) => { + // It's not guaranteed that we'll have session for the authenticated user (e.g. when user is + // authenticated with the help of HTTP authentication), that means we should safely check if + // we have it and can get a corresponding configuration. + try { + const session = await authc.getSessionInfo(request); + const accessNotice = + (session && + (config.authc.providers as Record)[session.provider.type]?.[ + session.provider.name + ]?.accessNotice) || + ''; + + return response.ok({ body: { accessNotice } }); + } catch (err) { + logger.error(err); + return response.internalError(); + } + } + ); +} diff --git a/x-pack/plugins/security/server/routes/views/index.test.ts b/x-pack/plugins/security/server/routes/views/index.test.ts index a8e7e905b119af..9290ae7f702a64 100644 --- a/x-pack/plugins/security/server/routes/views/index.test.ts +++ b/x-pack/plugins/security/server/routes/views/index.test.ts @@ -20,15 +20,18 @@ describe('View routes', () => { expect(routeParamsMock.httpResources.register.mock.calls.map(([{ path }]) => path)) .toMatchInlineSnapshot(` Array [ + "/security/access_notice", "/security/account", "/security/logged_out", "/logout", "/security/overwritten_session", ] `); - expect(routeParamsMock.router.get.mock.calls.map(([{ path }]) => path)).toMatchInlineSnapshot( - `Array []` - ); + expect(routeParamsMock.router.get.mock.calls.map(([{ path }]) => path)).toMatchInlineSnapshot(` + Array [ + "/internal/security/access_notice/state", + ] + `); }); it('registers Login routes if `basic` provider is enabled', () => { @@ -43,6 +46,7 @@ describe('View routes', () => { .toMatchInlineSnapshot(` Array [ "/login", + "/security/access_notice", "/security/account", "/security/logged_out", "/logout", @@ -52,6 +56,7 @@ describe('View routes', () => { expect(routeParamsMock.router.get.mock.calls.map(([{ path }]) => path)).toMatchInlineSnapshot(` Array [ "/internal/security/login_state", + "/internal/security/access_notice/state", ] `); }); @@ -68,6 +73,7 @@ describe('View routes', () => { .toMatchInlineSnapshot(` Array [ "/login", + "/security/access_notice", "/security/account", "/security/logged_out", "/logout", @@ -77,6 +83,7 @@ describe('View routes', () => { expect(routeParamsMock.router.get.mock.calls.map(([{ path }]) => path)).toMatchInlineSnapshot(` Array [ "/internal/security/login_state", + "/internal/security/access_notice/state", ] `); }); @@ -93,6 +100,7 @@ describe('View routes', () => { .toMatchInlineSnapshot(` Array [ "/login", + "/security/access_notice", "/security/account", "/security/logged_out", "/logout", @@ -102,6 +110,7 @@ describe('View routes', () => { expect(routeParamsMock.router.get.mock.calls.map(([{ path }]) => path)).toMatchInlineSnapshot(` Array [ "/internal/security/login_state", + "/internal/security/access_notice/state", ] `); }); diff --git a/x-pack/plugins/security/server/routes/views/index.ts b/x-pack/plugins/security/server/routes/views/index.ts index 255989dfeb90cd..8079dc4c8f7a6c 100644 --- a/x-pack/plugins/security/server/routes/views/index.ts +++ b/x-pack/plugins/security/server/routes/views/index.ts @@ -4,6 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ +import { defineAccessNoticeRoutes } from './access_notice'; import { defineAccountManagementRoutes } from './account_management'; import { defineLoggedOutRoutes } from './logged_out'; import { defineLoginRoutes } from './login'; @@ -20,6 +21,7 @@ export function defineViewRoutes(params: RouteDefinitionParams) { defineLoginRoutes(params); } + defineAccessNoticeRoutes(params); defineAccountManagementRoutes(params); defineLoggedOutRoutes(params); defineLogoutRoutes(params); diff --git a/x-pack/plugins/security/server/routes/views/logged_out.test.ts b/x-pack/plugins/security/server/routes/views/logged_out.test.ts index 3ff05d242d9dde..7cb73c49f9cbc8 100644 --- a/x-pack/plugins/security/server/routes/views/logged_out.test.ts +++ b/x-pack/plugins/security/server/routes/views/logged_out.test.ts @@ -39,7 +39,7 @@ describe('LoggedOut view routes', () => { it('redirects user to the root page if they have a session already.', async () => { authc.getSessionInfo.mockResolvedValue({ - provider: 'basic', + provider: { type: 'basic', name: 'basic' }, now: 0, idleTimeoutExpiration: null, lifespanExpiration: null, diff --git a/x-pack/plugins/security/server/routes/views/login.test.ts b/x-pack/plugins/security/server/routes/views/login.test.ts index d43319efbdfb9f..0f1dfb4eee73bd 100644 --- a/x-pack/plugins/security/server/routes/views/login.test.ts +++ b/x-pack/plugins/security/server/routes/views/login.test.ts @@ -141,6 +141,10 @@ describe('Login view routes', () => { }); describe('Login state route', () => { + function getAuthcConfig(authcConfig: Record = {}) { + return routeDefinitionParamsMock.create({ authc: { ...authcConfig } }).config.authc; + } + let routeHandler: RequestHandler; let routeConfig: RouteConfig; beforeEach(() => { @@ -235,14 +239,14 @@ describe('Login view routes', () => { const request = httpServerMock.createKibanaRequest(); const contextMock = coreMock.createRequestHandlerContext(); - const cases: Array<[boolean, ConfigType['authc']['sortedProviders']]> = [ - [false, []], - [true, [{ type: 'basic', name: 'basic1', options: { order: 0, showInSelector: true } }]], - [true, [{ type: 'token', name: 'token1', options: { order: 0, showInSelector: true } }]], + const cases: Array<[boolean, ConfigType['authc']]> = [ + [false, getAuthcConfig({ providers: { basic: { basic1: { order: 0, enabled: false } } } })], + [true, getAuthcConfig({ providers: { basic: { basic1: { order: 0 } } } })], + [true, getAuthcConfig({ providers: { token: { token1: { order: 0 } } } })], ]; - for (const [showLoginForm, sortedProviders] of cases) { - config.authc.sortedProviders = sortedProviders; + for (const [showLoginForm, authcConfig] of cases) { + config.authc = authcConfig; const expectedPayload = expect.objectContaining({ showLoginForm }); await expect( @@ -261,46 +265,38 @@ describe('Login view routes', () => { const request = httpServerMock.createKibanaRequest(); const contextMock = coreMock.createRequestHandlerContext(); - const cases: Array<[ - boolean, - ConfigType['authc']['sortedProviders'], - LoginState['selector']['providers'] - ]> = [ + const cases: Array<[ConfigType['authc'], LoginState['selector']['providers']]> = [ // selector is disabled, providers shouldn't be returned. [ - false, - [ - { type: 'basic', name: 'basic1', options: { order: 0, showInSelector: true } }, - { type: 'saml', name: 'saml1', options: { order: 1, showInSelector: true } }, - ], + getAuthcConfig({ + selector: { enabled: false }, + providers: { + basic: { basic1: { order: 0 } }, + saml: { saml1: { order: 1, realm: 'realm1' } }, + }, + }), [], ], // selector is enabled, but only basic/token is available, providers shouldn't be returned. [ - true, - [{ type: 'basic', name: 'basic1', options: { order: 0, showInSelector: true } }], + getAuthcConfig({ + selector: { enabled: true }, + providers: { basic: { basic1: { order: 0 } } }, + }), [], ], // selector is enabled, non-basic/token providers should be returned [ - true, - [ - { - type: 'basic', - name: 'basic1', - options: { order: 0, showInSelector: true, description: 'some-desc1' }, - }, - { - type: 'saml', - name: 'saml1', - options: { order: 1, showInSelector: true, description: 'some-desc2' }, + getAuthcConfig({ + selector: { enabled: true }, + providers: { + basic: { basic1: { order: 0 } }, + saml: { + saml1: { order: 1, description: 'some-desc2', realm: 'realm1' }, + saml2: { order: 2, description: 'some-desc3', realm: 'realm2' }, + }, }, - { - type: 'saml', - name: 'saml2', - options: { order: 2, showInSelector: true, description: 'some-desc3' }, - }, - ], + }), [ { type: 'saml', name: 'saml1', description: 'some-desc2' }, { type: 'saml', name: 'saml2', description: 'some-desc3' }, @@ -308,34 +304,30 @@ describe('Login view routes', () => { ], // selector is enabled, only non-basic/token providers that are enabled in selector should be returned. [ - true, - [ - { - type: 'basic', - name: 'basic1', - options: { order: 0, showInSelector: true, description: 'some-desc1' }, + getAuthcConfig({ + selector: { enabled: true }, + providers: { + basic: { basic1: { order: 0 } }, + saml: { + saml1: { + order: 1, + description: 'some-desc2', + realm: 'realm1', + showInSelector: false, + }, + saml2: { order: 2, description: 'some-desc3', realm: 'realm2' }, + }, }, - { - type: 'saml', - name: 'saml1', - options: { order: 1, showInSelector: false, description: 'some-desc2' }, - }, - { - type: 'saml', - name: 'saml2', - options: { order: 2, showInSelector: true, description: 'some-desc3' }, - }, - ], + }), [{ type: 'saml', name: 'saml2', description: 'some-desc3' }], ], ]; - for (const [selectorEnabled, sortedProviders, expectedProviders] of cases) { - config.authc.selector.enabled = selectorEnabled; - config.authc.sortedProviders = sortedProviders; + for (const [authcConfig, expectedProviders] of cases) { + config.authc = authcConfig; const expectedPayload = expect.objectContaining({ - selector: { enabled: selectorEnabled, providers: expectedProviders }, + selector: { enabled: authcConfig.selector.enabled, providers: expectedProviders }, }); await expect( routeHandler({ core: contextMock } as any, request, kibanaResponseFactory) diff --git a/x-pack/plugins/security/server/routes/views/login.ts b/x-pack/plugins/security/server/routes/views/login.ts index 4d6747de713f76..6cf28f7fbdcb4d 100644 --- a/x-pack/plugins/security/server/routes/views/login.ts +++ b/x-pack/plugins/security/server/routes/views/login.ts @@ -57,12 +57,15 @@ export function defineLoginRoutes({ let showLoginForm = false; const providers = []; - for (const { type, name, options } of sortedProviders) { - if (options.showInSelector) { + for (const { type, name } of sortedProviders) { + // Since `config.authc.sortedProviders` is based on `config.authc.providers` config we can + // be sure that config is present for every provider in `config.authc.sortedProviders`. + const { showInSelector, description } = config.authc.providers[type]?.[name]!; + if (showInSelector) { if (type === 'basic' || type === 'token') { showLoginForm = true; } else if (selector.enabled) { - providers.push({ type, name, description: options.description }); + providers.push({ type, name, description }); } } } diff --git a/x-pack/test/api_integration/apis/security/session.ts b/x-pack/test/api_integration/apis/security/session.ts index ef7e48388ff660..fcdf268ff27b0a 100644 --- a/x-pack/test/api_integration/apis/security/session.ts +++ b/x-pack/test/api_integration/apis/security/session.ts @@ -56,7 +56,7 @@ export default function({ getService }: FtrProviderContext) { expect(body.now).to.be.a('number'); expect(body.idleTimeoutExpiration).to.be.a('number'); expect(body.lifespanExpiration).to.be(null); - expect(body.provider).to.be('basic'); + expect(body.provider).to.eql({ type: 'basic', name: 'basic' }); }); it('should not extend the session', async () => { From 13648ac0b3b260bf400c7b46c2c5651514d5c915 Mon Sep 17 00:00:00 2001 From: Ryan Keairns Date: Mon, 20 Apr 2020 08:51:30 -0500 Subject: [PATCH 02/15] design adjustments --- .../access_notice/_access_notice_page.scss | 20 +++++++------- .../access_notice/access_notice_page.tsx | 27 ++++++++++--------- 2 files changed, 25 insertions(+), 22 deletions(-) diff --git a/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss b/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss index 25889a40cb7130..030d164727c95e 100644 --- a/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss +++ b/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss @@ -1,15 +1,17 @@ -.secAccessNoticePage__text { - @include euiScrollBar; +.secAccessNoticePage .secAuthenticationStatePage__content { + max-width: 600px; +} - max-height: $euiSize * 20; - overflow-y: auto; - padding: $euiSize; +.secAccessNoticePage__container { + @include euiBottomShadowSmall; + animation: none; } -.secAccessNoticePage__acknowledge { - align-self: end; +.secAccessNoticePage__text { + max-height: 400px; + padding-top: $euiSize; } -.secAccessNoticePage .secAuthenticationStatePage__content { - max-width: 600px; +.secAccessNoticePage__footer { + justify-content: flex-start; } diff --git a/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx b/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx index 91f414d434e33d..15f900832697df 100644 --- a/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx +++ b/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx @@ -9,7 +9,7 @@ import './_index.scss'; import React, { FormEvent, MouseEvent, useCallback, useEffect, useState } from 'react'; import ReactDOM from 'react-dom'; import ReactMarkdown from 'react-markdown'; -import { EuiButton, EuiFlexGroup, EuiFlexItem, EuiPanel, EuiSpacer, EuiText } from '@elastic/eui'; +import { EuiButton, EuiSpacer, EuiText } from '@elastic/eui'; import { i18n } from '@kbn/i18n'; import { FormattedMessage } from '@kbn/i18n/react'; import { CoreStart, FatalErrorsStart, HttpStart, NotificationsStart } from 'src/core/public'; @@ -67,15 +67,16 @@ export function AccessNoticePage({ http, fatalErrors, notifications }: Props) { } >
- - - - - {accessNotice} - - - - +
+
+
+
+ + {accessNotice} + +
+
+
- - - +
+
+
From 0f6f1897cb4836c36c245e103290cf04f99f5f3f Mon Sep 17 00:00:00 2001 From: Ryan Keairns Date: Mon, 20 Apr 2020 10:07:58 -0500 Subject: [PATCH 03/15] Remove direct use of EuiModal classes --- .../access_notice/_access_notice_page.scss | 10 ++-- .../access_notice/access_notice_page.tsx | 52 ++++++++++--------- 2 files changed, 33 insertions(+), 29 deletions(-) diff --git a/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss b/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss index 030d164727c95e..fb71c81765d768 100644 --- a/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss +++ b/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss @@ -2,16 +2,16 @@ max-width: 600px; } -.secAccessNoticePage__container { - @include euiBottomShadowSmall; - animation: none; +.secAccessNoticePage__textWrapper { + overflow-y: hidden; } .secAccessNoticePage__text { + @include euiYScrollWithShadows; max-height: 400px; - padding-top: $euiSize; + padding: $euiSize $euiSizeL 0; } .secAccessNoticePage__footer { - justify-content: flex-start; + padding: $euiSize $euiSizeL $euiSizeL; } diff --git a/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx b/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx index 15f900832697df..0baf7e62200fad 100644 --- a/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx +++ b/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx @@ -9,7 +9,7 @@ import './_index.scss'; import React, { FormEvent, MouseEvent, useCallback, useEffect, useState } from 'react'; import ReactDOM from 'react-dom'; import ReactMarkdown from 'react-markdown'; -import { EuiButton, EuiSpacer, EuiText } from '@elastic/eui'; +import { EuiButton, EuiPanel, EuiFlexGroup, EuiFlexItem, EuiSpacer, EuiText } from '@elastic/eui'; import { i18n } from '@kbn/i18n'; import { FormattedMessage } from '@kbn/i18n/react'; import { CoreStart, FatalErrorsStart, HttpStart, NotificationsStart } from 'src/core/public'; @@ -67,33 +67,37 @@ export function AccessNoticePage({ http, fatalErrors, notifications }: Props) { } >
-
-
-
-
+ + + +
{accessNotice}
-
-
- - - -
-
-
+ + + + + + + + + + + + From 0410f175e85b420c0a79703aa8b54f4965e0892d Mon Sep 17 00:00:00 2001 From: Ryan Keairns Date: Mon, 20 Apr 2020 10:45:37 -0500 Subject: [PATCH 04/15] address feedback --- .../access_notice/_access_notice_page.scss | 4 +++ .../access_notice/access_notice_page.tsx | 36 +++++++++---------- 2 files changed, 21 insertions(+), 19 deletions(-) diff --git a/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss b/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss index fb71c81765d768..46703a0792a13a 100644 --- a/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss +++ b/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss @@ -15,3 +15,7 @@ .secAccessNoticePage__footer { padding: $euiSize $euiSizeL $euiSizeL; } + +.secAccessNoticePage__footerInner { + text-align: left; +} diff --git a/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx b/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx index 0baf7e62200fad..982d2fc139e4e1 100644 --- a/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx +++ b/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx @@ -67,7 +67,7 @@ export function AccessNoticePage({ http, fatalErrors, notifications }: Props) { } >
- +
@@ -77,24 +77,22 @@ export function AccessNoticePage({ http, fatalErrors, notifications }: Props) {
- - - - - - - +
+ + + +
From 45c51d6fe1f64474e9c9bcb7862eb24c72dd3f78 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Mon, 20 Apr 2020 18:11:31 +0200 Subject: [PATCH 05/15] Update jest snapshot. --- .../access_notice_page.test.tsx.snap | 147 +++++++++--------- 1 file changed, 75 insertions(+), 72 deletions(-) diff --git a/x-pack/plugins/security/public/authentication/access_notice/__snapshots__/access_notice_page.test.tsx.snap b/x-pack/plugins/security/public/authentication/access_notice/__snapshots__/access_notice_page.test.tsx.snap index 6dfed36f8af25e..e9f79bccbc5686 100644 --- a/x-pack/plugins/security/public/authentication/access_notice/__snapshots__/access_notice_page.test.tsx.snap +++ b/x-pack/plugins/security/public/authentication/access_notice/__snapshots__/access_notice_page.test.tsx.snap @@ -71,113 +71,116 @@ exports[`AccessNoticePage renders as expected when state is available 1`] = ` - +
- -
- -
- -
-

- This is - - link - -

-
- -
- -
- + This is + + link + +

+
+ +
+ +
+ +
- -
- - - - + + +
From 6ba5360ee8d8f42dc595d4a7b3710cc079d5f2c5 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Thu, 23 Apr 2020 12:43:43 +0200 Subject: [PATCH 06/15] Review#1: rename access_notice files to access_agreement. --- .../__snapshots__/access_agreement_page.test.tsx.snap} | 0 .../_access_agreement_page.scss} | 0 .../{access_notice => access_agreement}/_index.scss | 0 .../access_agreement_app.test.ts} | 0 .../access_agreement_app.ts} | 0 .../access_agreement_page.test.tsx} | 0 .../access_agreement_page.tsx} | 0 .../authentication/{access_notice => access_agreement}/index.ts | 0 .../views/{access_notice.test.ts => access_agreement.test.ts} | 0 .../server/routes/views/{access_notice.ts => access_agreement.ts} | 0 10 files changed, 0 insertions(+), 0 deletions(-) rename x-pack/plugins/security/public/authentication/{access_notice/__snapshots__/access_notice_page.test.tsx.snap => access_agreement/__snapshots__/access_agreement_page.test.tsx.snap} (100%) rename x-pack/plugins/security/public/authentication/{access_notice/_access_notice_page.scss => access_agreement/_access_agreement_page.scss} (100%) rename x-pack/plugins/security/public/authentication/{access_notice => access_agreement}/_index.scss (100%) rename x-pack/plugins/security/public/authentication/{access_notice/access_notice_app.test.ts => access_agreement/access_agreement_app.test.ts} (100%) rename x-pack/plugins/security/public/authentication/{access_notice/access_notice_app.ts => access_agreement/access_agreement_app.ts} (100%) rename x-pack/plugins/security/public/authentication/{access_notice/access_notice_page.test.tsx => access_agreement/access_agreement_page.test.tsx} (100%) rename x-pack/plugins/security/public/authentication/{access_notice/access_notice_page.tsx => access_agreement/access_agreement_page.tsx} (100%) rename x-pack/plugins/security/public/authentication/{access_notice => access_agreement}/index.ts (100%) rename x-pack/plugins/security/server/routes/views/{access_notice.test.ts => access_agreement.test.ts} (100%) rename x-pack/plugins/security/server/routes/views/{access_notice.ts => access_agreement.ts} (100%) diff --git a/x-pack/plugins/security/public/authentication/access_notice/__snapshots__/access_notice_page.test.tsx.snap b/x-pack/plugins/security/public/authentication/access_agreement/__snapshots__/access_agreement_page.test.tsx.snap similarity index 100% rename from x-pack/plugins/security/public/authentication/access_notice/__snapshots__/access_notice_page.test.tsx.snap rename to x-pack/plugins/security/public/authentication/access_agreement/__snapshots__/access_agreement_page.test.tsx.snap diff --git a/x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss b/x-pack/plugins/security/public/authentication/access_agreement/_access_agreement_page.scss similarity index 100% rename from x-pack/plugins/security/public/authentication/access_notice/_access_notice_page.scss rename to x-pack/plugins/security/public/authentication/access_agreement/_access_agreement_page.scss diff --git a/x-pack/plugins/security/public/authentication/access_notice/_index.scss b/x-pack/plugins/security/public/authentication/access_agreement/_index.scss similarity index 100% rename from x-pack/plugins/security/public/authentication/access_notice/_index.scss rename to x-pack/plugins/security/public/authentication/access_agreement/_index.scss diff --git a/x-pack/plugins/security/public/authentication/access_notice/access_notice_app.test.ts b/x-pack/plugins/security/public/authentication/access_agreement/access_agreement_app.test.ts similarity index 100% rename from x-pack/plugins/security/public/authentication/access_notice/access_notice_app.test.ts rename to x-pack/plugins/security/public/authentication/access_agreement/access_agreement_app.test.ts diff --git a/x-pack/plugins/security/public/authentication/access_notice/access_notice_app.ts b/x-pack/plugins/security/public/authentication/access_agreement/access_agreement_app.ts similarity index 100% rename from x-pack/plugins/security/public/authentication/access_notice/access_notice_app.ts rename to x-pack/plugins/security/public/authentication/access_agreement/access_agreement_app.ts diff --git a/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.test.tsx b/x-pack/plugins/security/public/authentication/access_agreement/access_agreement_page.test.tsx similarity index 100% rename from x-pack/plugins/security/public/authentication/access_notice/access_notice_page.test.tsx rename to x-pack/plugins/security/public/authentication/access_agreement/access_agreement_page.test.tsx diff --git a/x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx b/x-pack/plugins/security/public/authentication/access_agreement/access_agreement_page.tsx similarity index 100% rename from x-pack/plugins/security/public/authentication/access_notice/access_notice_page.tsx rename to x-pack/plugins/security/public/authentication/access_agreement/access_agreement_page.tsx diff --git a/x-pack/plugins/security/public/authentication/access_notice/index.ts b/x-pack/plugins/security/public/authentication/access_agreement/index.ts similarity index 100% rename from x-pack/plugins/security/public/authentication/access_notice/index.ts rename to x-pack/plugins/security/public/authentication/access_agreement/index.ts diff --git a/x-pack/plugins/security/server/routes/views/access_notice.test.ts b/x-pack/plugins/security/server/routes/views/access_agreement.test.ts similarity index 100% rename from x-pack/plugins/security/server/routes/views/access_notice.test.ts rename to x-pack/plugins/security/server/routes/views/access_agreement.test.ts diff --git a/x-pack/plugins/security/server/routes/views/access_notice.ts b/x-pack/plugins/security/server/routes/views/access_agreement.ts similarity index 100% rename from x-pack/plugins/security/server/routes/views/access_notice.ts rename to x-pack/plugins/security/server/routes/views/access_agreement.ts From 3fd22ea9b25339a3e028f4cb1fa24f74a880d7d5 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Thu, 23 Apr 2020 13:09:54 +0200 Subject: [PATCH 07/15] Review#1: replace `notice` with `acknowledgement`. --- .../access_agreement_page.test.tsx.snap | 34 ++++++------- .../_access_agreement_page.scss | 10 ++-- .../access_agreement/_index.scss | 2 +- .../access_agreement_app.test.ts | 18 +++---- .../access_agreement/access_agreement_app.ts | 16 +++---- .../access_agreement_page.test.tsx | 38 ++++++++------- .../access_agreement_page.tsx | 41 ++++++++-------- .../authentication/access_agreement/index.ts | 2 +- .../authentication/authentication_service.ts | 4 +- .../server/audit/audit_logger.test.ts | 8 ++-- .../security/server/audit/audit_logger.ts | 6 +-- .../security/server/audit/index.mock.ts | 2 +- .../authentication/authenticator.test.ts | 48 +++++++++---------- .../server/authentication/authenticator.ts | 42 ++++++++-------- .../server/authentication/index.mock.ts | 2 +- .../security/server/authentication/index.ts | 2 +- x-pack/plugins/security/server/config.test.ts | 6 +-- x-pack/plugins/security/server/config.ts | 4 +- .../routes/authentication/common.test.ts | 8 ++-- .../server/routes/authentication/common.ts | 4 +- .../routes/views/access_agreement.test.ts | 32 +++++++------ .../server/routes/views/access_agreement.ts | 14 +++--- .../server/routes/views/index.test.ts | 16 +++---- .../security/server/routes/views/index.ts | 4 +- 24 files changed, 186 insertions(+), 177 deletions(-) diff --git a/x-pack/plugins/security/public/authentication/access_agreement/__snapshots__/access_agreement_page.test.tsx.snap b/x-pack/plugins/security/public/authentication/access_agreement/__snapshots__/access_agreement_page.test.tsx.snap index e9f79bccbc5686..98cd52b6dd6c92 100644 --- a/x-pack/plugins/security/public/authentication/access_agreement/__snapshots__/access_agreement_page.test.tsx.snap +++ b/x-pack/plugins/security/public/authentication/access_agreement/__snapshots__/access_agreement_page.test.tsx.snap @@ -1,18 +1,18 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP -exports[`AccessNoticePage renders as expected when state is available 1`] = ` +exports[`AccessAgreementPage renders as expected when state is available 1`] = ` } >
- Access Notice + Access Agreement @@ -85,13 +85,13 @@ exports[`AccessNoticePage renders as expected when state is available 1`] = ` className="euiFlexGroup euiFlexGroup--directionColumn euiFlexGroup--responsive" >
- -
-
- -
- -
- - -
- - -
+ link + +

- + `; diff --git a/x-pack/plugins/security/public/authentication/access_agreement/access_agreement_page.test.tsx b/x-pack/plugins/security/public/authentication/access_agreement/access_agreement_page.test.tsx index 217ba8ebbefad8..89b7489d45ebb2 100644 --- a/x-pack/plugins/security/public/authentication/access_agreement/access_agreement_page.test.tsx +++ b/x-pack/plugins/security/public/authentication/access_agreement/access_agreement_page.test.tsx @@ -5,11 +5,12 @@ */ import React from 'react'; +import ReactMarkdown from 'react-markdown'; +import { EuiLoadingContent } from '@elastic/eui'; import { act } from '@testing-library/react'; import { mountWithIntl, nextTick } from 'test_utils/enzyme_helpers'; import { findTestSubject } from 'test_utils/find_test_subject'; import { coreMock } from '../../../../../../src/core/public/mocks'; -import { AuthenticationStatePage } from '../components/authentication_state_page'; import { AccessAgreementPage } from './access_agreement_page'; describe('AccessAgreementPage', () => { @@ -36,15 +37,17 @@ describe('AccessAgreementPage', () => { /> ); - // Shouldn't render anything if state isn't yet available. - expect(wrapper.isEmptyRender()).toBe(true); + expect(wrapper.exists(EuiLoadingContent)).toBe(true); + expect(wrapper.exists(ReactMarkdown)).toBe(false); await act(async () => { await nextTick(); wrapper.update(); }); - expect(wrapper.find(AuthenticationStatePage)).toMatchSnapshot(); + expect(wrapper.find(ReactMarkdown)).toMatchSnapshot(); + expect(wrapper.exists(EuiLoadingContent)).toBe(false); + expect(coreStartMock.http.get).toHaveBeenCalledTimes(1); expect(coreStartMock.http.get).toHaveBeenCalledWith( '/internal/security/access_agreement/state' diff --git a/x-pack/plugins/security/public/authentication/access_agreement/access_agreement_page.tsx b/x-pack/plugins/security/public/authentication/access_agreement/access_agreement_page.tsx index ad630a943d7da7..0315e229c678b9 100644 --- a/x-pack/plugins/security/public/authentication/access_agreement/access_agreement_page.tsx +++ b/x-pack/plugins/security/public/authentication/access_agreement/access_agreement_page.tsx @@ -9,7 +9,15 @@ import './_index.scss'; import React, { FormEvent, MouseEvent, useCallback, useEffect, useState } from 'react'; import ReactDOM from 'react-dom'; import ReactMarkdown from 'react-markdown'; -import { EuiButton, EuiPanel, EuiFlexGroup, EuiFlexItem, EuiSpacer, EuiText } from '@elastic/eui'; +import { + EuiButton, + EuiPanel, + EuiFlexGroup, + EuiFlexItem, + EuiLoadingContent, + EuiSpacer, + EuiText, +} from '@elastic/eui'; import { i18n } from '@kbn/i18n'; import { FormattedMessage } from '@kbn/i18n/react'; import { CoreStart, FatalErrorsStart, HttpStart, NotificationsStart } from 'src/core/public'; @@ -55,9 +63,43 @@ export function AccessAgreementPage({ http, fatalErrors, notifications }: Props) [http, notifications] ); - if (accessAgreement == null) { - return null; - } + const content = accessAgreement ? ( +
+ + + +
+ + {accessAgreement} + +
+ + +
+ + + +
+ + + +
+ ) : ( + + + + ); return ( } > -
- - - -
- - {accessAgreement} - -
- - -
- - - -
- - - - - + {content} +
); } diff --git a/x-pack/plugins/security/public/authentication/components/authentication_state_page/__snapshots__/authentication_state_page.test.tsx.snap b/x-pack/plugins/security/public/authentication/components/authentication_state_page/__snapshots__/authentication_state_page.test.tsx.snap index 76d8b73af68c1d..fbd884dc812c8c 100644 --- a/x-pack/plugins/security/public/authentication/components/authentication_state_page/__snapshots__/authentication_state_page.test.tsx.snap +++ b/x-pack/plugins/security/public/authentication/components/authentication_state_page/__snapshots__/authentication_state_page.test.tsx.snap @@ -43,47 +43,3 @@ exports[`AuthenticationStatePage renders 1`] = `
`; - -exports[`AuthenticationStatePage renders with custom CSS class 1`] = ` -
-
-
- - - - - -

- foo -

- - -
-
-
- - hello world - -
-
-`; diff --git a/x-pack/plugins/security/public/authentication/components/authentication_state_page/authentication_state_page.test.tsx b/x-pack/plugins/security/public/authentication/components/authentication_state_page/authentication_state_page.test.tsx index cbdf0d2488f852..946c58a1c8e998 100644 --- a/x-pack/plugins/security/public/authentication/components/authentication_state_page/authentication_state_page.test.tsx +++ b/x-pack/plugins/security/public/authentication/components/authentication_state_page/authentication_state_page.test.tsx @@ -25,7 +25,7 @@ describe('AuthenticationStatePage', () => { hello world - ) - ).toMatchSnapshot(); + ).exists('.secAuthenticationStatePage.customClassName') + ).toBe(true); }); }); diff --git a/x-pack/plugins/security/server/routes/views/access_agreement.ts b/x-pack/plugins/security/server/routes/views/access_agreement.ts index 9ad280aa949027..d87e6772b984f8 100644 --- a/x-pack/plugins/security/server/routes/views/access_agreement.ts +++ b/x-pack/plugins/security/server/routes/views/access_agreement.ts @@ -4,6 +4,7 @@ * you may not use this file except in compliance with the Elastic License. */ +import { ConfigType } from '../../config'; import { RouteDefinitionParams } from '..'; /** @@ -31,9 +32,9 @@ export function defineAccessAgreementRoutes({ const session = await authc.getSessionInfo(request); const accessAgreement = (session && - (config.authc.providers as Record)[session.provider.type]?.[ - session.provider.name - ]?.accessAgreement?.message) || + config.authc.providers[ + session.provider.type as keyof ConfigType['authc']['providers'] + ]?.[session.provider.name]?.accessAgreement?.message) || ''; return response.ok({ body: { accessAgreement } }); From 300877fb355d1755339cf7d621ae81ad2a9b02e6 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Fri, 24 Apr 2020 10:29:59 +0200 Subject: [PATCH 12/15] Review#1: use `logoElastic` instead of `logoKibana`. --- .../__snapshots__/authentication_state_page.test.tsx.snap | 2 +- .../authentication_state_page/authentication_state_page.tsx | 2 +- .../__snapshots__/overwritten_session_page.test.tsx.snap | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/x-pack/plugins/security/public/authentication/components/authentication_state_page/__snapshots__/authentication_state_page.test.tsx.snap b/x-pack/plugins/security/public/authentication/components/authentication_state_page/__snapshots__/authentication_state_page.test.tsx.snap index fbd884dc812c8c..585dc368da7077 100644 --- a/x-pack/plugins/security/public/authentication/components/authentication_state_page/__snapshots__/authentication_state_page.test.tsx.snap +++ b/x-pack/plugins/security/public/authentication/components/authentication_state_page/__snapshots__/authentication_state_page.test.tsx.snap @@ -18,7 +18,7 @@ exports[`AuthenticationStatePage renders 1`] = ` > = props => (
- +

{props.title}

diff --git a/x-pack/plugins/security/public/authentication/overwritten_session/__snapshots__/overwritten_session_page.test.tsx.snap b/x-pack/plugins/security/public/authentication/overwritten_session/__snapshots__/overwritten_session_page.test.tsx.snap index 632aaffc914338..02b1a7d0d3fa03 100644 --- a/x-pack/plugins/security/public/authentication/overwritten_session/__snapshots__/overwritten_session_page.test.tsx.snap +++ b/x-pack/plugins/security/public/authentication/overwritten_session/__snapshots__/overwritten_session_page.test.tsx.snap @@ -31,10 +31,10 @@ exports[`OverwrittenSessionPage renders as expected 1`] = ` >
From 20f24a880b72c17a4b5b7eb1a8286330b9b5f40c Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Fri, 24 Apr 2020 12:06:05 +0200 Subject: [PATCH 13/15] Review#1: re-expose `areAPIKeysEnabled`. --- x-pack/plugins/security/server/plugin.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/x-pack/plugins/security/server/plugin.ts b/x-pack/plugins/security/server/plugin.ts index 3572dc5c1a8698..e30b0caf76ddc0 100644 --- a/x-pack/plugins/security/server/plugin.ts +++ b/x-pack/plugins/security/server/plugin.ts @@ -52,6 +52,7 @@ export interface SecurityPluginSetup { Authentication, | 'isAuthenticated' | 'getCurrentUser' + | 'areAPIKeysEnabled' | 'createAPIKey' | 'invalidateAPIKey' | 'grantAPIKeyAsInternalUser' @@ -180,6 +181,7 @@ export class Plugin { authc: { isAuthenticated: authc.isAuthenticated, getCurrentUser: authc.getCurrentUser, + areAPIKeysEnabled: authc.areAPIKeysEnabled, createAPIKey: authc.createAPIKey, invalidateAPIKey: authc.invalidateAPIKey, grantAPIKeyAsInternalUser: authc.grantAPIKeyAsInternalUser, From de109763422621b4e0c7417a4df5b38ef3a5f11e Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Tue, 28 Apr 2020 12:31:58 +0200 Subject: [PATCH 14/15] Allow access acknowledgement only if license is Gold+. --- .../common/licensing/license_features.ts | 5 ++ .../common/licensing/license_service.test.ts | 14 ++++- .../common/licensing/license_service.ts | 3 + .../authentication/authenticator.test.ts | 35 +++++++++++ .../server/authentication/authenticator.ts | 10 ++- .../security/server/authentication/index.ts | 1 + .../routes/authentication/common.test.ts | 20 ++++++ .../server/routes/authentication/common.ts | 18 +++++- .../server/routes/licensed_route_handler.ts | 18 +++++- .../routes/views/access_agreement.test.ts | 62 ++++++++++++++----- .../server/routes/views/access_agreement.ts | 17 ++++- .../server/routes/views/login.test.ts | 1 + 12 files changed, 176 insertions(+), 28 deletions(-) diff --git a/x-pack/plugins/security/common/licensing/license_features.ts b/x-pack/plugins/security/common/licensing/license_features.ts index 5184ab0e962bd2..571d2630b2b177 100644 --- a/x-pack/plugins/security/common/licensing/license_features.ts +++ b/x-pack/plugins/security/common/licensing/license_features.ts @@ -33,6 +33,11 @@ export interface SecurityLicenseFeatures { */ readonly showRoleMappingsManagement: boolean; + /** + * Indicates whether we allow users to access agreement UI and acknowledge it. + */ + readonly allowAccessAgreement: boolean; + /** * Indicates whether we allow users to define document level security in roles. */ diff --git a/x-pack/plugins/security/common/licensing/license_service.test.ts b/x-pack/plugins/security/common/licensing/license_service.test.ts index 5bdfa7d4886aac..edb0fd8bfb4ed8 100644 --- a/x-pack/plugins/security/common/licensing/license_service.test.ts +++ b/x-pack/plugins/security/common/licensing/license_service.test.ts @@ -18,6 +18,7 @@ describe('license features', function() { allowLogin: false, showLinks: false, showRoleMappingsManagement: false, + allowAccessAgreement: false, allowRoleDocumentLevelSecurity: false, allowRoleFieldLevelSecurity: false, layout: 'error-es-unavailable', @@ -37,6 +38,7 @@ describe('license features', function() { allowLogin: false, showLinks: false, showRoleMappingsManagement: false, + allowAccessAgreement: false, allowRoleDocumentLevelSecurity: false, allowRoleFieldLevelSecurity: false, layout: 'error-xpack-unavailable', @@ -60,6 +62,7 @@ describe('license features', function() { expect(subscriptionHandler.mock.calls[0]).toMatchInlineSnapshot(` Array [ Object { + "allowAccessAgreement": false, "allowLogin": false, "allowRbac": false, "allowRoleDocumentLevelSecurity": false, @@ -78,6 +81,7 @@ describe('license features', function() { expect(subscriptionHandler.mock.calls[1]).toMatchInlineSnapshot(` Array [ Object { + "allowAccessAgreement": true, "allowLogin": true, "allowRbac": true, "allowRoleDocumentLevelSecurity": true, @@ -94,7 +98,7 @@ describe('license features', function() { } }); - it('should show login page and other security elements, allow RBAC but forbid role mappings, DLS, and sub-feature privileges if license is basic.', () => { + it('should show login page and other security elements, allow RBAC but forbid role mappings, access agreement, DLS, and sub-feature privileges if license is basic.', () => { const mockRawLicense = licensingMock.createLicense({ features: { security: { isEnabled: true, isAvailable: true } }, }); @@ -109,6 +113,7 @@ describe('license features', function() { allowLogin: true, showLinks: true, showRoleMappingsManagement: false, + allowAccessAgreement: false, allowRoleDocumentLevelSecurity: false, allowRoleFieldLevelSecurity: false, allowRbac: true, @@ -131,6 +136,7 @@ describe('license features', function() { allowLogin: false, showLinks: false, showRoleMappingsManagement: false, + allowAccessAgreement: false, allowRoleDocumentLevelSecurity: false, allowRoleFieldLevelSecurity: false, allowRbac: false, @@ -138,7 +144,7 @@ describe('license features', function() { }); }); - it('should allow role mappings and sub-feature privileges, but not DLS/FLS if license = gold', () => { + it('should allow role mappings, access agreement and sub-feature privileges, but not DLS/FLS if license = gold', () => { const mockRawLicense = licensingMock.createLicense({ license: { mode: 'gold', type: 'gold' }, features: { security: { isEnabled: true, isAvailable: true } }, @@ -152,6 +158,7 @@ describe('license features', function() { allowLogin: true, showLinks: true, showRoleMappingsManagement: true, + allowAccessAgreement: true, allowRoleDocumentLevelSecurity: false, allowRoleFieldLevelSecurity: false, allowRbac: true, @@ -159,7 +166,7 @@ describe('license features', function() { }); }); - it('should allow to login, allow RBAC, role mappings, sub-feature privileges, and DLS if license >= platinum', () => { + it('should allow to login, allow RBAC, role mappings, access agreement, sub-feature privileges, and DLS if license >= platinum', () => { const mockRawLicense = licensingMock.createLicense({ license: { mode: 'platinum', type: 'platinum' }, features: { security: { isEnabled: true, isAvailable: true } }, @@ -173,6 +180,7 @@ describe('license features', function() { allowLogin: true, showLinks: true, showRoleMappingsManagement: true, + allowAccessAgreement: true, allowRoleDocumentLevelSecurity: true, allowRoleFieldLevelSecurity: true, allowRbac: true, diff --git a/x-pack/plugins/security/common/licensing/license_service.ts b/x-pack/plugins/security/common/licensing/license_service.ts index 34bc44b88e40d9..7815798d6a9f3d 100644 --- a/x-pack/plugins/security/common/licensing/license_service.ts +++ b/x-pack/plugins/security/common/licensing/license_service.ts @@ -71,6 +71,7 @@ export class SecurityLicenseService { allowLogin: false, showLinks: false, showRoleMappingsManagement: false, + allowAccessAgreement: false, allowRoleDocumentLevelSecurity: false, allowRoleFieldLevelSecurity: false, allowRbac: false, @@ -88,6 +89,7 @@ export class SecurityLicenseService { allowLogin: false, showLinks: false, showRoleMappingsManagement: false, + allowAccessAgreement: false, allowRoleDocumentLevelSecurity: false, allowRoleFieldLevelSecurity: false, allowRbac: false, @@ -102,6 +104,7 @@ export class SecurityLicenseService { allowLogin: true, showLinks: true, showRoleMappingsManagement: isLicenseGoldOrBetter, + allowAccessAgreement: isLicenseGoldOrBetter, allowSubFeaturePrivileges: isLicenseGoldOrBetter, // Only platinum and trial licenses are compliant with field- and document-level security. allowRoleDocumentLevelSecurity: isLicensePlatinumOrBetter, diff --git a/x-pack/plugins/security/server/authentication/authenticator.test.ts b/x-pack/plugins/security/server/authentication/authenticator.test.ts index e57cf11dad5251..49b7b40659cfc1 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.test.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.test.ts @@ -20,8 +20,10 @@ import { elasticsearchServiceMock, sessionStorageMock, } from '../../../../../src/core/server/mocks'; +import { licenseMock } from '../../common/licensing/index.mock'; import { mockAuthenticatedUser } from '../../common/model/authenticated_user.mock'; import { securityAuditLoggerMock } from '../audit/index.mock'; +import { SecurityLicenseFeatures } from '../../common/licensing'; import { ConfigSchema, createConfig } from '../config'; import { AuthenticationResult } from './authentication_result'; import { Authenticator, AuthenticatorOptions, ProviderSession } from './authenticator'; @@ -44,6 +46,7 @@ function getMockOptions({ getCurrentUser: jest.fn(), clusterClient: elasticsearchServiceMock.createClusterClient(), basePath: httpServiceMock.createSetupContract().basePath, + license: licenseMock.create(), loggers: loggingServiceMock.create(), config: createConfig( ConfigSchema.validate({ session, authc: { selector, providers, http } }), @@ -1121,6 +1124,9 @@ describe('Authenticator', () => { }, }); mockOptions.sessionStorageFactory.asScoped.mockReturnValue(mockSessionStorage); + mockOptions.license.getFeatures.mockReturnValue({ + allowAccessAgreement: true, + } as SecurityLicenseFeatures); mockBasicAuthenticationProvider.authenticate.mockResolvedValue( AuthenticationResult.succeeded(mockUser) @@ -1220,6 +1226,18 @@ describe('Authenticator', () => { ); }); + it('does not redirect to Access Agreement if license doesnt allow it.', async () => { + const request = httpServerMock.createKibanaRequest(); + mockSessionStorage.get.mockResolvedValue(mockSessVal); + mockOptions.license.getFeatures.mockReturnValue({ + allowAccessAgreement: false, + } as SecurityLicenseFeatures); + + await expect(authenticator.authenticate(request)).resolves.toEqual( + AuthenticationResult.succeeded(mockUser) + ); + }); + it('redirects to Access Agreement when needed.', async () => { mockSessionStorage.get.mockResolvedValue(mockSessVal); @@ -1416,6 +1434,9 @@ describe('Authenticator', () => { }; mockSessionStorage.get.mockResolvedValue(mockSessionValue); mockOptions.getCurrentUser.mockReturnValue(mockAuthenticatedUser()); + mockOptions.license.getFeatures.mockReturnValue({ + allowAccessAgreement: true, + } as SecurityLicenseFeatures); authenticator = new Authenticator(mockOptions); }); @@ -1444,6 +1465,20 @@ describe('Authenticator', () => { expect(mockSessionStorage.set).not.toHaveBeenCalled(); }); + it('fails if license doesn allow access agreement acknowledgement', async () => { + mockOptions.license.getFeatures.mockReturnValue({ + allowAccessAgreement: false, + } as SecurityLicenseFeatures); + + await expect( + authenticator.acknowledgeAccessAgreement(httpServerMock.createKibanaRequest()) + ).rejects.toThrowErrorMatchingInlineSnapshot( + `"Current license does not allow access agreement acknowledgement."` + ); + + expect(mockSessionStorage.set).not.toHaveBeenCalled(); + }); + it('properly acknowledges access agreement for the authenticated user', async () => { await authenticator.acknowledgeAccessAgreement(httpServerMock.createKibanaRequest()); diff --git a/x-pack/plugins/security/server/authentication/authenticator.ts b/x-pack/plugins/security/server/authentication/authenticator.ts index be5fd80ed2640d..58dea2b23e5463 100644 --- a/x-pack/plugins/security/server/authentication/authenticator.ts +++ b/x-pack/plugins/security/server/authentication/authenticator.ts @@ -14,6 +14,7 @@ import { HttpServiceSetup, IClusterClient, } from '../../../../../src/core/server'; +import { SecurityLicense } from '../../common/licensing'; import { AuthenticatedUser } from '../../common/model'; import { AuthenticationProvider, SessionInfo } from '../../common/types'; import { SecurityAuditLogger } from '../audit'; @@ -96,6 +97,7 @@ export interface AuthenticatorOptions { getCurrentUser: (request: KibanaRequest) => AuthenticatedUser | null; config: Pick; basePath: HttpServiceSetup['basePath']; + license: SecurityLicense; loggers: LoggerFactory; clusterClient: IClusterClient; sessionStorageFactory: SessionStorageFactory; @@ -490,6 +492,10 @@ export class Authenticator { throw new Error('Cannot acknowledge access agreement for unauthenticated user.'); } + if (!this.options.license.getFeatures().allowAccessAgreement) { + throw new Error('Current license does not allow access agreement acknowledgement.'); + } + sessionStorage.set({ ...existingSession, accessAgreementAcknowledged: true }); this.options.auditLogger.accessAgreementAcknowledged( @@ -682,7 +688,8 @@ export class Authenticator { // 2. Request is authenticated, but user hasn't acknowledged access agreement in the current // session yet (based on the flag we store in the session) // 3. Request is authenticated by the provider that has `accessAgreement` configured - // 4. And it's not a request to the Access Agreement UI itself + // 4. Current license allows access agreement + // 5. And it's not a request to the Access Agreement UI itself return ( canRedirectRequest(request) && session != null && @@ -690,6 +697,7 @@ export class Authenticator { (this.options.config.authc.providers as Record)[session.provider.type]?.[ session.provider.name ]?.accessAgreement && + this.options.license.getFeatures().allowAccessAgreement && request.url.pathname !== ACCESS_AGREEMENT_ROUTE ); } diff --git a/x-pack/plugins/security/server/authentication/index.ts b/x-pack/plugins/security/server/authentication/index.ts index d148aeaa0d273c..779b852195b028 100644 --- a/x-pack/plugins/security/server/authentication/index.ts +++ b/x-pack/plugins/security/server/authentication/index.ts @@ -90,6 +90,7 @@ export async function setupAuthentication({ clusterClient, basePath: http.basePath, config: { session: config.session, authc: config.authc }, + license, loggers, sessionStorageFactory: await http.createCookieSessionStorageFactory({ encryptionKey: config.encryptionKey, diff --git a/x-pack/plugins/security/server/routes/authentication/common.test.ts b/x-pack/plugins/security/server/routes/authentication/common.test.ts index 508d3ec0efae4b..14bcee62809109 100644 --- a/x-pack/plugins/security/server/routes/authentication/common.test.ts +++ b/x-pack/plugins/security/server/routes/authentication/common.test.ts @@ -12,6 +12,7 @@ import { RequestHandlerContext, RouteConfig, } from '../../../../../../src/core/server'; +import { SecurityLicense, SecurityLicenseFeatures } from '../../../common/licensing'; import { Authentication, AuthenticationResult, @@ -28,11 +29,13 @@ import { routeDefinitionParamsMock } from '../index.mock'; describe('Common authentication routes', () => { let router: jest.Mocked; let authc: jest.Mocked; + let license: jest.Mocked; let mockContext: RequestHandlerContext; beforeEach(() => { const routeParamsMock = routeDefinitionParamsMock.create(); router = routeParamsMock.router; authc = routeParamsMock.authc; + license = routeParamsMock.license; mockContext = ({ licensing: { @@ -442,6 +445,10 @@ describe('Common authentication routes', () => { ([{ path }]) => path === '/internal/security/access_agreement/acknowledge' )!; + license.getFeatures.mockReturnValue({ + allowAccessAgreement: true, + } as SecurityLicenseFeatures); + routeConfig = acsRouteConfig; routeHandler = acsRouteHandler; }); @@ -451,6 +458,19 @@ describe('Common authentication routes', () => { expect(routeConfig.validate).toBe(false); }); + it('returns 404 if current license doesnt allow access agreement acknowledgement.', async () => { + license.getFeatures.mockReturnValue({ + allowAccessAgreement: false, + } as SecurityLicenseFeatures); + + const request = httpServerMock.createKibanaRequest(); + await expect(routeHandler(mockContext, request, kibanaResponseFactory)).resolves.toEqual({ + status: 404, + payload: 'Not Found', + options: {}, + }); + }); + it('returns 500 if acknowledge throws unhandled exception.', async () => { const unhandledException = new Error('Something went wrong.'); authc.acknowledgeAccessAgreement.mockRejectedValue(unhandledException); diff --git a/x-pack/plugins/security/server/routes/authentication/common.ts b/x-pack/plugins/security/server/routes/authentication/common.ts index fadcd2ff6db695..d0e9b0e98b2c6e 100644 --- a/x-pack/plugins/security/server/routes/authentication/common.ts +++ b/x-pack/plugins/security/server/routes/authentication/common.ts @@ -18,7 +18,13 @@ import { RouteDefinitionParams } from '..'; /** * Defines routes that are common to various authentication mechanisms. */ -export function defineCommonRoutes({ router, authc, basePath, logger }: RouteDefinitionParams) { +export function defineCommonRoutes({ + router, + authc, + basePath, + license, + logger, +}: RouteDefinitionParams) { // Generate two identical routes with new and deprecated URL and issue a warning if route with deprecated URL is ever used. for (const path of ['/api/security/logout', '/api/security/v1/logout']) { router.get( @@ -138,7 +144,13 @@ export function defineCommonRoutes({ router, authc, basePath, logger }: RouteDef router.post( { path: '/internal/security/access_agreement/acknowledge', validate: false }, - async (context, request, response) => { + createLicensedRouteHandler(async (context, request, response) => { + // If license doesn't allow access agreement we shouldn't handle request. + if (!license.getFeatures().allowAccessAgreement) { + logger.warn(`Attempted to acknowledge access agreement when license doesn allow it.`); + return response.notFound(); + } + try { await authc.acknowledgeAccessAgreement(request); } catch (err) { @@ -147,6 +159,6 @@ export function defineCommonRoutes({ router, authc, basePath, logger }: RouteDef } return response.noContent(); - } + }) ); } diff --git a/x-pack/plugins/security/server/routes/licensed_route_handler.ts b/x-pack/plugins/security/server/routes/licensed_route_handler.ts index b113b2ca59e3ef..d8c212aa2d2174 100644 --- a/x-pack/plugins/security/server/routes/licensed_route_handler.ts +++ b/x-pack/plugins/security/server/routes/licensed_route_handler.ts @@ -4,10 +4,22 @@ * you may not use this file except in compliance with the Elastic License. */ -import { RequestHandler } from 'kibana/server'; +import { KibanaResponseFactory, RequestHandler, RouteMethod } from 'kibana/server'; -export const createLicensedRouteHandler = (handler: RequestHandler) => { - const licensedRouteHandler: RequestHandler = (context, request, responseToolkit) => { +export const createLicensedRouteHandler = < + P, + Q, + B, + M extends RouteMethod, + R extends KibanaResponseFactory +>( + handler: RequestHandler +) => { + const licensedRouteHandler: RequestHandler = ( + context, + request, + responseToolkit + ) => { const { license } = context.licensing; const licenseCheck = license.check('security', 'basic'); if (licenseCheck.state === 'unavailable' || licenseCheck.state === 'invalid') { diff --git a/x-pack/plugins/security/server/routes/views/access_agreement.test.ts b/x-pack/plugins/security/server/routes/views/access_agreement.test.ts index a77e0cfc3b21b1..405699cb75df83 100644 --- a/x-pack/plugins/security/server/routes/views/access_agreement.test.ts +++ b/x-pack/plugins/security/server/routes/views/access_agreement.test.ts @@ -11,16 +11,14 @@ import { IRouter, HttpResources, HttpResourcesRequestHandler, + RequestHandlerContext, } from '../../../../../../src/core/server'; +import { SecurityLicense, SecurityLicenseFeatures } from '../../../common/licensing'; import { AuthenticationProvider } from '../../../common/types'; import { ConfigType } from '../../config'; import { defineAccessAgreementRoutes } from './access_agreement'; -import { - coreMock, - httpResourcesMock, - httpServerMock, -} from '../../../../../../src/core/server/mocks'; +import { httpResourcesMock, httpServerMock } from '../../../../../../src/core/server/mocks'; import { routeDefinitionParamsMock } from '../index.mock'; import { Authentication } from '../../authentication'; @@ -29,12 +27,25 @@ describe('Access agreement view routes', () => { let router: jest.Mocked; let config: ConfigType; let authc: jest.Mocked; + let license: jest.Mocked; + let mockContext: RequestHandlerContext; beforeEach(() => { const routeParamsMock = routeDefinitionParamsMock.create(); router = routeParamsMock.router; httpResources = routeParamsMock.httpResources; authc = routeParamsMock.authc; config = routeParamsMock.config; + license = routeParamsMock.license; + + license.getFeatures.mockReturnValue({ + allowAccessAgreement: true, + } as SecurityLicenseFeatures); + + mockContext = ({ + licensing: { + license: { check: jest.fn().mockReturnValue({ check: 'valid' }) }, + }, + } as unknown) as RequestHandlerContext; defineAccessAgreementRoutes(routeParamsMock); }); @@ -56,12 +67,25 @@ describe('Access agreement view routes', () => { expect(routeConfig.validate).toBe(false); }); + it('does not render view if current license does not allow access agreement.', async () => { + const request = httpServerMock.createKibanaRequest(); + const responseFactory = httpResourcesMock.createResponseFactory(); + + license.getFeatures.mockReturnValue({ + allowAccessAgreement: false, + } as SecurityLicenseFeatures); + + await routeHandler(mockContext, request, responseFactory); + + expect(responseFactory.renderCoreApp).not.toHaveBeenCalledWith(); + expect(responseFactory.notFound).toHaveBeenCalledTimes(1); + }); + it('renders view.', async () => { const request = httpServerMock.createKibanaRequest(); - const contextMock = coreMock.createRequestHandlerContext(); const responseFactory = httpResourcesMock.createResponseFactory(); - await routeHandler({ core: contextMock } as any, request, responseFactory); + await routeHandler(mockContext, request, responseFactory); expect(responseFactory.renderCoreApp).toHaveBeenCalledWith(); }); @@ -84,15 +108,26 @@ describe('Access agreement view routes', () => { expect(routeConfig.validate).toBe(false); }); + it('returns `Not Found` if current license does not allow access agreement.', async () => { + const request = httpServerMock.createKibanaRequest(); + + license.getFeatures.mockReturnValue({ + allowAccessAgreement: false, + } as SecurityLicenseFeatures); + + await expect(routeHandler(mockContext, request, kibanaResponseFactory)).resolves.toEqual({ + status: 404, + payload: 'Not Found', + options: {}, + }); + }); + it('returns empty `accessAgreement` if session info is not available.', async () => { const request = httpServerMock.createKibanaRequest(); - const contextMock = coreMock.createRequestHandlerContext(); authc.getSessionInfo.mockResolvedValue(null); - await expect( - routeHandler({ core: contextMock } as any, request, kibanaResponseFactory) - ).resolves.toEqual({ + await expect(routeHandler(mockContext, request, kibanaResponseFactory)).resolves.toEqual({ options: { body: { accessAgreement: '' } }, payload: { accessAgreement: '' }, status: 200, @@ -101,7 +136,6 @@ describe('Access agreement view routes', () => { it('returns non-empty `accessAgreement` only if it is configured.', async () => { const request = httpServerMock.createKibanaRequest(); - const contextMock = coreMock.createRequestHandlerContext(); config.authc = routeDefinitionParamsMock.create({ authc: { @@ -132,9 +166,7 @@ describe('Access agreement view routes', () => { provider: sessionProvider, }); - await expect( - routeHandler({ core: contextMock } as any, request, kibanaResponseFactory) - ).resolves.toEqual({ + await expect(routeHandler(mockContext, request, kibanaResponseFactory)).resolves.toEqual({ options: { body: { accessAgreement: expectedAccessAgreement } }, payload: { accessAgreement: expectedAccessAgreement }, status: 200, diff --git a/x-pack/plugins/security/server/routes/views/access_agreement.ts b/x-pack/plugins/security/server/routes/views/access_agreement.ts index d87e6772b984f8..5b56d0dbdbc623 100644 --- a/x-pack/plugins/security/server/routes/views/access_agreement.ts +++ b/x-pack/plugins/security/server/routes/views/access_agreement.ts @@ -5,6 +5,7 @@ */ import { ConfigType } from '../../config'; +import { createLicensedRouteHandler } from '../licensed_route_handler'; import { RouteDefinitionParams } from '..'; /** @@ -13,18 +14,28 @@ import { RouteDefinitionParams } from '..'; export function defineAccessAgreementRoutes({ authc, httpResources, + license, config, router, logger, }: RouteDefinitionParams) { + // If license doesn't allow access agreement we shouldn't handle request. + const canHandleRequest = () => license.getFeatures().allowAccessAgreement; + httpResources.register( { path: '/security/access_agreement', validate: false }, - async (context, request, response) => response.renderCoreApp() + createLicensedRouteHandler(async (context, request, response) => + canHandleRequest() ? response.renderCoreApp() : response.notFound() + ) ); router.get( { path: '/internal/security/access_agreement/state', validate: false }, - async (context, request, response) => { + createLicensedRouteHandler(async (context, request, response) => { + if (!canHandleRequest()) { + return response.notFound(); + } + // It's not guaranteed that we'll have session for the authenticated user (e.g. when user is // authenticated with the help of HTTP authentication), that means we should safely check if // we have it and can get a corresponding configuration. @@ -42,6 +53,6 @@ export function defineAccessAgreementRoutes({ logger.error(err); return response.internalError(); } - } + }) ); } diff --git a/x-pack/plugins/security/server/routes/views/login.test.ts b/x-pack/plugins/security/server/routes/views/login.test.ts index 0f1dfb4eee73bd..1378629459b437 100644 --- a/x-pack/plugins/security/server/routes/views/login.test.ts +++ b/x-pack/plugins/security/server/routes/views/login.test.ts @@ -163,6 +163,7 @@ describe('Login view routes', () => { it('returns only required license features.', async () => { license.getFeatures.mockReturnValue({ + allowAccessAgreement: true, allowLogin: true, allowRbac: false, allowRoleDocumentLevelSecurity: true, From 3c7cf49364afa4540b4eea466470266c85cc0831 Mon Sep 17 00:00:00 2001 From: Aleh Zasypkin Date: Tue, 28 Apr 2020 16:03:55 +0200 Subject: [PATCH 15/15] Review#1: return 403 instead of 404 if current license does not support access agreement, minor test fixes. --- .../security/common/licensing/license_service.test.ts | 2 +- .../server/routes/authentication/common.test.ts | 8 ++++---- .../security/server/routes/authentication/common.ts | 6 ++++-- .../server/routes/views/access_agreement.test.ts | 10 +++++----- .../security/server/routes/views/access_agreement.ts | 10 ++++++++-- 5 files changed, 22 insertions(+), 14 deletions(-) diff --git a/x-pack/plugins/security/common/licensing/license_service.test.ts b/x-pack/plugins/security/common/licensing/license_service.test.ts index edb0fd8bfb4ed8..9dec665614635f 100644 --- a/x-pack/plugins/security/common/licensing/license_service.test.ts +++ b/x-pack/plugins/security/common/licensing/license_service.test.ts @@ -98,7 +98,7 @@ describe('license features', function() { } }); - it('should show login page and other security elements, allow RBAC but forbid role mappings, access agreement, DLS, and sub-feature privileges if license is basic.', () => { + it('should show login page and other security elements, allow RBAC but forbid paid features if license is basic.', () => { const mockRawLicense = licensingMock.createLicense({ features: { security: { isEnabled: true, isAvailable: true } }, }); diff --git a/x-pack/plugins/security/server/routes/authentication/common.test.ts b/x-pack/plugins/security/server/routes/authentication/common.test.ts index 14bcee62809109..5a0401e6320b46 100644 --- a/x-pack/plugins/security/server/routes/authentication/common.test.ts +++ b/x-pack/plugins/security/server/routes/authentication/common.test.ts @@ -458,16 +458,16 @@ describe('Common authentication routes', () => { expect(routeConfig.validate).toBe(false); }); - it('returns 404 if current license doesnt allow access agreement acknowledgement.', async () => { + it(`returns 403 if current license doesn't allow access agreement acknowledgement.`, async () => { license.getFeatures.mockReturnValue({ allowAccessAgreement: false, } as SecurityLicenseFeatures); const request = httpServerMock.createKibanaRequest(); await expect(routeHandler(mockContext, request, kibanaResponseFactory)).resolves.toEqual({ - status: 404, - payload: 'Not Found', - options: {}, + status: 403, + payload: { message: `Current license doesn't support access agreement.` }, + options: { body: { message: `Current license doesn't support access agreement.` } }, }); }); diff --git a/x-pack/plugins/security/server/routes/authentication/common.ts b/x-pack/plugins/security/server/routes/authentication/common.ts index d0e9b0e98b2c6e..91783140539a5b 100644 --- a/x-pack/plugins/security/server/routes/authentication/common.ts +++ b/x-pack/plugins/security/server/routes/authentication/common.ts @@ -147,8 +147,10 @@ export function defineCommonRoutes({ createLicensedRouteHandler(async (context, request, response) => { // If license doesn't allow access agreement we shouldn't handle request. if (!license.getFeatures().allowAccessAgreement) { - logger.warn(`Attempted to acknowledge access agreement when license doesn allow it.`); - return response.notFound(); + logger.warn(`Attempted to acknowledge access agreement when license doesn't allow it.`); + return response.forbidden({ + body: { message: `Current license doesn't support access agreement.` }, + }); } try { diff --git a/x-pack/plugins/security/server/routes/views/access_agreement.test.ts b/x-pack/plugins/security/server/routes/views/access_agreement.test.ts index 405699cb75df83..3d616575b84131 100644 --- a/x-pack/plugins/security/server/routes/views/access_agreement.test.ts +++ b/x-pack/plugins/security/server/routes/views/access_agreement.test.ts @@ -78,7 +78,7 @@ describe('Access agreement view routes', () => { await routeHandler(mockContext, request, responseFactory); expect(responseFactory.renderCoreApp).not.toHaveBeenCalledWith(); - expect(responseFactory.notFound).toHaveBeenCalledTimes(1); + expect(responseFactory.forbidden).toHaveBeenCalledTimes(1); }); it('renders view.', async () => { @@ -108,7 +108,7 @@ describe('Access agreement view routes', () => { expect(routeConfig.validate).toBe(false); }); - it('returns `Not Found` if current license does not allow access agreement.', async () => { + it('returns `403` if current license does not allow access agreement.', async () => { const request = httpServerMock.createKibanaRequest(); license.getFeatures.mockReturnValue({ @@ -116,9 +116,9 @@ describe('Access agreement view routes', () => { } as SecurityLicenseFeatures); await expect(routeHandler(mockContext, request, kibanaResponseFactory)).resolves.toEqual({ - status: 404, - payload: 'Not Found', - options: {}, + status: 403, + payload: { message: `Current license doesn't support access agreement.` }, + options: { body: { message: `Current license doesn't support access agreement.` } }, }); }); diff --git a/x-pack/plugins/security/server/routes/views/access_agreement.ts b/x-pack/plugins/security/server/routes/views/access_agreement.ts index 5b56d0dbdbc623..49e1ff42a28a2a 100644 --- a/x-pack/plugins/security/server/routes/views/access_agreement.ts +++ b/x-pack/plugins/security/server/routes/views/access_agreement.ts @@ -25,7 +25,11 @@ export function defineAccessAgreementRoutes({ httpResources.register( { path: '/security/access_agreement', validate: false }, createLicensedRouteHandler(async (context, request, response) => - canHandleRequest() ? response.renderCoreApp() : response.notFound() + canHandleRequest() + ? response.renderCoreApp() + : response.forbidden({ + body: { message: `Current license doesn't support access agreement.` }, + }) ) ); @@ -33,7 +37,9 @@ export function defineAccessAgreementRoutes({ { path: '/internal/security/access_agreement/state', validate: false }, createLicensedRouteHandler(async (context, request, response) => { if (!canHandleRequest()) { - return response.notFound(); + return response.forbidden({ + body: { message: `Current license doesn't support access agreement.` }, + }); } // It's not guaranteed that we'll have session for the authenticated user (e.g. when user is