Introduce Access Agreement UI. #63563

azasypkin · 2020-04-15T11:26:50Z

This PR introduces the Access Agreement UI that works like that:

Every provider has an additional accessAgreement configuration option now. Option accepts markdown string as a value.
Every authenticated request will be redirected to /security/access_agreement only if:
- Request is authenticated by the provider that has accessAgreement.message configured
- User hasn't yet acknowledged access notice in the current session (based on the flag we store in the session)
- Request can be redirected (not API call)
- And it's not a request to the Access Agreement UI itself
When request is redirected to the Access Agreement UI original URL is captured and user will be redirected to it once access agreement is acknowledged

UI/UX:

Config:

xpack.security.authc.providers:
  basic.basic1:
    order: 0
    accessAgreement.message: "#### You are accessing a system with U.S. government information \n\n
                   By logging in, you acknowledge that information system usage may be monitored, recorded, and subject to audit. Unauthorized use of the information system is prohibited and subject to criminal and civil penalties. Use of the information system indicates consent to monitoring and recording."

Blocked by: Design Team and stakeholders feedback is pending
Fixes: #18176

"Release Note: every provider can now be configured with the access agreement message (markdown syntax) that will be presented to the users after login. Users won't be able to use Kibana until they acknowledge this agreement."

elasticmachine · 2020-04-15T11:26:52Z

Pinging @elastic/kibana-security (Team:Security)

azasypkin · 2020-04-20T07:47:45Z

...ity/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;


note: motivation behind this change is that I wanted to make body of this component wider for access notice as we have much more text there (600px instead of 480px)

azasypkin · 2020-04-20T07:48:33Z

x-pack/plugins/security/public/index.ts

@@ -15,7 +15,6 @@ import {
 } from './plugin';

 export { SecurityPluginSetup, SecurityPluginStart };
-export { SessionInfo } from './types';


note: it doesn't seem like we want it to be a part of our public API, feels like our own internal thing.

azasypkin · 2020-04-20T07:50:23Z

x-pack/plugins/security/server/authentication/authenticator.ts

+  /**
+   * The set of flags used to describe various aspects of the user session.
+   */
+  flags?: {


note: if we wouldn't have plans to switch to server side sessions, I'd use a bitmask field here instead to save space 🙂 For now will just make it optional.

question I'm not opposed to this structure, but do you have other flags in mind that we'll add in the future?

That's actually a good point, I had something else in mind at the beginning, but now I can't think of anything. Maybe flags is too premature indeed, I'll switch to the top level property for now, thanks!

azasypkin · 2020-04-20T08:00:26Z

x-pack/plugins/security/server/authentication/authenticator.ts

+      (this.options.config.authc.providers as Record<string, any>)[session.provider.type]?.[
+        session.provider.name
+      ]?.accessNotice &&
+      request.url.pathname !== ACCESS_NOTICE_ROUTE


note: technically pathname can have a trailing slash, but I'm ignoring this here to simplify the check because of the following reasons:

When we redirect to this page we don't add trailing slash

If user goes directly to this page and adds a trailing slash, they will end up at /security/access_notice?next=%2Fsecurity%2Faccess_notice%2F assuming they haven't acknowledged access notice yet, that means after acknowledgement they will be redirect to /security/access_notice (since Kibana server will eventually issue a redirected to the same URL but without trailing slash), after second acknowledgement or just by navigating to another page everything will go back to normal.

I'm not worried about the second point since it's a user mistake after all and it doesn't have that bad consequences.

azasypkin · 2020-04-20T08:03:13Z

x-pack/plugins/security/server/config.ts

@@ -225,25 +234,19 @@ export function createConfig(
  const sortedProviders: Array<{


note: since we're adding more common config properties the amount of info we're duplicating in sortedProviders is increasing, so I decided to take a step back and keep authc.providers as the only source of truth.

azasypkin · 2020-04-20T08:05:46Z

x-pack/plugins/security/server/plugin.test.ts

@@ -71,14 +71,10 @@ describe('Security Plugin', () => {
                "authc": Object {
                  "createAPIKey": [Function],
                  "getCurrentUser": [Function],
-                  "getSessionInfo": [Function],


note: technically it's a breaking change (if we already started to treat changed plugin contracts as such), but nobody in Kibana was using it and I'm not sure we want to expose this to 3rd party plugins yet. Starting from 8.0 I'd not do that 🙂

Yeah, now is a great time to clean this up!

azasypkin · 2020-04-20T08:08:36Z

x-pack/plugins/security/server/routes/views/access_notice.ts

+        const session = await authc.getSessionInfo(request);
+        const accessNotice =
+          (session &&
+            (config.authc.providers as Record<string, any>)[session.provider.type]?.[


note: extracting config from config.authc.providers is a bit ugly since we don't know what providers are configured and there is no "type link" between session provider field and this config (and may not be since session can rely on old config). If we see this pattern more and more, we'll create a helper method instead.

ryankeairns · 2020-04-20T13:14:58Z

@azasypkin A few design tweaks coming... I'll have a design PR for you to review.

ryankeairns

@azasypkin here is a design PR with a few changes that more closely aligns to the modal content styling we are after. If it looks good, simply merge: azasypkin#1

Access page design adjustments

azasypkin · 2020-04-20T14:11:21Z

@azasypkin here is a design PR with a few changes that more closely aligns to the modal content styling we are after. If it looks good, simply merge: azasypkin#1

Looks great, thanks a lot! Merged and updated screenshot in description.

ryankeairns · 2020-04-20T14:27:45Z

@azasypkin I'll need to make some changes based upon some feedback I received from @cchaos . I'll get that going straight away.

ryankeairns · 2020-04-20T15:48:48Z

2nd design PR 👉 azasypkin#2

… generalize flags just yet, use object format for `accessAgreement` config.

… generalize flags just yet, use object format for `accessAgreement` config, use loading screen.

cchaos · 2020-04-23T17:59:52Z

should this screen be using the Elastic cluster logo instead of the Kibana logo? I know we recently made this change to the login and space selector screens

Yes indeed, the Kibana logo should now be the cluster logo.

azasypkin · 2020-04-24T08:44:57Z

should this screen be using the Elastic cluster logo instead of the Kibana logo? I know we recently made this change to the login and space selector screens
Yes indeed, the Kibana logo should now be the cluster logo.

Roger that, changed to logoElastic.

azasypkin · 2020-04-24T11:57:11Z

@legrego PR should be ready for another pass! But since we're still waiting for some details on this feature that will require a couple of changes here and there (in a separate commit though), feel free to postpone review.

azasypkin · 2020-04-28T11:25:27Z

x-pack/plugins/security/server/authentication/authenticator.ts

+    }
+
+    if (!this.options.license.getFeatures().allowAccessAgreement) {
+      throw new Error('Current license does not allow access agreement acknowledgement.');


note: this doesn't sound like an audit event, but rather as a developer mistake, hence just throwing error here.

azasypkin · 2020-04-28T11:28:23Z

x-pack/plugins/security/server/routes/authentication/common.ts

+      // 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();


note: I was considering two options here, either to return 403 or 404. I picked 404 and the rational is that license doesn't provide this feature and that basically means that feature is missing (not found).. What do you think?

I think a 403 would make more sense here (with reason in the response body), so that we're consistent with the base licensed route handler functionality:

kibana/x-pack/plugins/security/server/routes/licensed_route_handler.ts

Line 26 in de10976

return responseToolkit.forbidden({ body: { message: licenseCheck.message! } });

Okay, makes sense too, let's go with 403. By reason you mean our own custom message, right? Currently SecurityLicense doesn't keep the optional message that Licensing plugin would return in case of failed check...

Discussed over Slack, we'll go with custom Current license doesn't support access agreement. message.

legrego

Looks great, thanks @azasypkin! Just a couple nits/comments, then I think we're good to go!

legrego · 2020-04-28T12:54:35Z

x-pack/plugins/security/server/routes/authentication/common.ts

+      // 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();


I think a 403 would make more sense here (with reason in the response body), so that we're consistent with the base licensed route handler functionality:

kibana/x-pack/plugins/security/server/routes/licensed_route_handler.ts

Line 26 in de10976

return responseToolkit.forbidden({ body: { message: licenseCheck.message! } });

legrego · 2020-04-28T12:57:31Z

x-pack/plugins/security/server/routes/licensed_route_handler.ts

+>(
+  handler: RequestHandler<P, Q, B, M, R>
+) => {
+  const licensedRouteHandler: RequestHandler<P, Q, B, M, R> = (


note for future us: we should consider updating this to accept a minimum license level once we have more than one route that requires an elevated license

To be honest I have mixed feelings about this since feature that depends on license level can rely on multiple endpoints and keeping all of them in sync may be tedious. Unless we refactor SecurityLicense to include something similar to feature <-> min license level mapping that will be the only source of truth.

That’s a great point, a single source of truth is much more beneficial than the potential duplication of license checks

legrego · 2020-04-28T13:09:15Z

x-pack/plugins/security/common/licensing/license_service.test.ts

@@ -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.', () => {


This is getting a bit out of hand 😅What do you think about something like this?

Suggested change

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.', () => {

legrego · 2020-04-28T13:13:51Z

x-pack/plugins/security/server/routes/authentication/common.test.ts

+      expect(routeConfig.validate).toBe(false);
+    });
+
+    it('returns 404 if current license doesnt allow access agreement acknowledgement.', async () => {


Suggested change

it('returns 404 if current license doesnt allow access agreement acknowledgement.', async () => {

it(`returns 404 if current license doesn't allow access agreement acknowledgement.`, async () => {

legrego · 2020-04-28T13:14:13Z

x-pack/plugins/security/server/routes/authentication/common.ts

+    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.`);


Suggested change

logger.warn(`Attempted to acknowledge access agreement when license doesn allow it.`);

logger.warn(`Attempted to acknowledge access agreement when license doesn't allow it.`);

…rt access agreement, minor test fixes.

kibanamachine · 2020-04-28T15:59:07Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 3c7cf49

History

💚 Build #43705 succeeded 99332b2
💚 Build #43025 succeeded 20f24a8
💔 Build #43000 failed 300877f
💚 Build #42790 succeeded b9d4713
💚 Build #41928 succeeded 45c51d6

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

Co-authored-by: Ryan Keairns <contactryank@gmail.com>

azasypkin · 2020-04-28T18:19:26Z

7.x/7.8.0: a3790c8

azasypkin added blocked release_note:roadmap Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.8.0 labels Apr 15, 2020

azasypkin force-pushed the issue-18176-pre-access-message branch 2 times, most recently from 9320a37 to 5f43585 Compare April 20, 2020 07:17

azasypkin removed the blocked label Apr 20, 2020

azasypkin commented Apr 20, 2020

View reviewed changes

Introduce Access Notice UI.

5c395ef

azasypkin force-pushed the issue-18176-pre-access-message branch from 5f43585 to 5c395ef Compare April 20, 2020 09:23

azasypkin marked this pull request as ready for review April 20, 2020 11:29

azasypkin requested review from a team as code owners April 20, 2020 11:29

azasypkin requested a review from legrego April 20, 2020 11:30

design adjustments

13648ac

ryankeairns requested changes Apr 20, 2020

View reviewed changes

Merge pull request #1 from ryankeairns/issue-18176-pre-access-message

10a6048

Access page design adjustments

ryankeairns added 2 commits April 20, 2020 10:07

Remove direct use of EuiModal classes

0f6f189

address feedback

0410f17

azasypkin added 2 commits April 23, 2020 14:20

Review#1: introduce AuthenticationProvider common interface, do not…

258e597

… generalize flags just yet, use object format for `accessAgreement` config.

Review#1: introduce AuthenticationProvider common interface, do not…

b9d4713

… generalize flags just yet, use object format for `accessAgreement` config, use loading screen.

azasypkin changed the title ~~Introduce Access Notice UI.~~ Introduce Access Agreement UI. Apr 23, 2020

azasypkin added 2 commits April 24, 2020 10:06

Merge branch 'master' into issue-18176-pre-access-message

ab1ed57

Review#1: use logoElastic instead of logoKibana.

300877f

Review#1: re-expose areAPIKeysEnabled.

20f24a8

azasypkin requested a review from legrego April 24, 2020 11:55

azasypkin mentioned this pull request Apr 24, 2020

Improve Login Selector UX #64142

Merged

azasypkin added 3 commits April 28, 2020 08:52

Merge branch 'master' into issue-18176-pre-access-message

be9a7f1

Allow access acknowledgement only if license is Gold+.

de10976

Merge branch 'master' into issue-18176-pre-access-message

99332b2

azasypkin commented Apr 28, 2020

View reviewed changes

legrego approved these changes Apr 28, 2020

View reviewed changes

legrego mentioned this pull request Apr 28, 2020

Lazy load Security plugin management apps. #64511

Merged

Review#1: return 403 instead of 404 if current license does not suppo…

3c7cf49

…rt access agreement, minor test fixes.

azasypkin merged commit 36b4864 into elastic:master Apr 28, 2020

azasypkin deleted the issue-18176-pre-access-message branch April 28, 2020 16:00

azasypkin mentioned this pull request Apr 28, 2020

[7.x] Introduce Access Agreement UI. (#63563) #64676

Merged

azasypkin added a commit to azasypkin/kibana that referenced this pull request Apr 28, 2020

Introduce Access Agreement UI. (elastic#63563)

a3790c8

Co-authored-by: Ryan Keairns <contactryank@gmail.com>

azasypkin added the backport pending label Apr 28, 2020

azasypkin mentioned this pull request Apr 28, 2020

Configurable Pre-Login Warning Screen #18176

Closed

azasypkin added backported and removed backport pending labels Apr 28, 2020

mbarretta mentioned this pull request Jun 2, 2021

Customizable Header and Footer Banners #17298

Open

4 tasks

		@@ -225,25 +234,19 @@ export function createConfig(
		const sortedProviders: Array<{

	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.', () => {

	it('returns 404 if current license doesnt allow access agreement acknowledgement.', async () => {
	it(`returns 404 if current license doesn't allow access agreement acknowledgement.`, async () => {

	logger.warn(`Attempted to acknowledge access agreement when license doesn allow it.`);
	logger.warn(`Attempted to acknowledge access agreement when license doesn't allow it.`);

Introduce Access Agreement UI. #63563

Introduce Access Agreement UI. #63563

Conversation

azasypkin commented Apr 15, 2020 • edited Loading

elasticmachine commented Apr 15, 2020

azasypkin Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Apr 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryankeairns commented Apr 20, 2020

ryankeairns left a comment

Choose a reason for hiding this comment

azasypkin commented Apr 20, 2020

ryankeairns commented Apr 20, 2020

ryankeairns commented Apr 20, 2020

cchaos commented Apr 23, 2020

azasypkin commented Apr 24, 2020

azasypkin commented Apr 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

legrego left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibanamachine commented Apr 28, 2020

💚 Build Succeeded

History

azasypkin commented Apr 28, 2020

azasypkin commented Apr 15, 2020 •

edited

Loading

azasypkin Apr 20, 2020 •

edited

Loading

azasypkin Apr 20, 2020 •

edited

Loading