Adding Global Access Agreement #139217

kc13greiner · 2022-08-22T14:33:09Z

Summary

Adding a config for a default, global access agreement message for all configured authc providers that do no have a local access agreement message

Release Notes

A default Access Agreement can now be set for all authentication providers at the xpack.security level

…-ref HEAD~1..HEAD --fix'

elasticmachine · 2022-08-22T18:29:24Z

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

kc13greiner · 2022-08-22T18:31:14Z

@legrego @azasypkin Should the docs be updated to reflect this as well?

legrego · 2022-08-22T18:38:22Z

@kc13greiner yes, docs changes need to be done to teach our users about this new setting:

docs/user/security/access-agreement.asciidoc
docs/settings/security-settings.asciidoc

Additionally, we will need to add a corresponding changes to allow this new setting to be configured by end users within the Elastic Cloud console. We'll follow up with you separately about that.

azasypkin · 2022-08-24T12:30:16Z

ACK: will review today

azasypkin

Looks good! Just two questions.

azasypkin · 2022-08-24T15:19:54Z

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

@@ -237,6 +238,7 @@ export const ConfigSchema = schema.object({
  }),
  authc: schema.object({
    selector: schema.object({ enabled: schema.maybe(schema.boolean()) }),
+    accessAgreement: schema.maybe(schema.object({ message: schema.string() })),


question: I'm wondering if it would make sense to move accessAgreement one level above authc, like we did with global session since it's not strictly related to the authentication and it's fine if provider's config overrides any config, whether it's auth related or not?

What do you think @kc13greiner @legrego?

question: I'm wondering if it would make sense to move accessAgreement one level above authc, like we did with global session since it's not strictly related to the authentication and it's fine if provider's config overrides any config, whether it's auth related or not?

Hmm, that's a good question! I could make an argument either way, but since we have session at the global level, I think it would be slightly more consistent to move accessAgreement one level up.

I don't have a strong opinion here, so I'm happy to defer to you both.

(Also @/kurt should be @kc13greiner)

Sounds good to me! Ill move it up

Sounds good to me! Ill move it up

Awesome, thanks!

(Also @/kurt should be @kc13greiner)

😬 ugh

azasypkin · 2022-08-24T15:27:00Z

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

  for (const [type, providerGroup] of Object.entries(providers)) {
    for (const [name, { enabled, order, accessAgreement }] of Object.entries(providerGroup ?? {})) {
+      const hasLocalAccessAgreement: boolean = !!accessAgreement?.message;


question: wouldn't it be easier (implementation/readability) if we keep global and provider-specific accessAgreement configs separate and just slightly update Authenticator.shouldRedirectToAccessAgreement and /security/access_agreement route handler to account for the global access agreement message if provider doesn't have any? It'd also allow us to diverge these configs in the future if we want to (e.g. we can restrict global access agreement settings that providers can or cannot override, like we did for xpack.session.cleanupInterval).

What do you think?

Yea, good call! Ill make the change

kc13greiner · 2022-08-24T22:20:03Z

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

@@ -814,20 +815,26 @@ export class Authenticator {
   */
  private shouldRedirectToAccessAgreement(sessionValue: SessionValue | null) {
    // Request should be redirected to Access Agreement UI only if all following conditions are met:


@azasypkin Can you help me clean up the comments in this function?

Sure, let me try:

// Request can be redirected (not API call)
// And it's not a request to the Access Agreement UI itself

We can remove these two - since we check for canRedirectRequest and access agreement URL before we invoke this function.

// If user doesn't have an active session or if they already acknowledged // access agreement (based on the flag we store in the session) - bail out. if (sessionValue == null || sessionValue.accessAgreementAcknowledged) { return false; } // If access agreement is neither enabled globally (for all providers) // nor for the provider that authenticated user request - bail out. const providerConfig = this.options.config.authc.providers as Record<string, any>)[ sessionValue.provider.type ]?.[sessionValue.provider.name]; if (!this.options.config.accessAgreement && !providerConfig?.accessAgreement) { return false; } // Check if the current license allows access agreement. return this.options.license.getFeatures().allowAccessAgreement;

Perfect! Fixed in latest commit.

…-ref HEAD~1..HEAD --fix'

azasypkin · 2022-08-25T08:27:17Z

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

@@ -343,6 +343,7 @@ export class AuthenticationService {
      license: this.license,
      session,
      isElasticCloudDeployment,
+      accessAgreement: { message: config.accessAgreement?.message },


nit: I think you can just add accessAgreement property directly to the config argument above (we do the same with authc).

Fixed in latest commit!

azasypkin · 2022-08-25T08:59:54Z

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

@@ -814,20 +815,26 @@ export class Authenticator {
   */
  private shouldRedirectToAccessAgreement(sessionValue: SessionValue | null) {
    // Request should be redirected to Access Agreement UI only if all following conditions are met:


Sure, let me try:

// Request can be redirected (not API call)
// And it's not a request to the Access Agreement UI itself

We can remove these two - since we check for canRedirectRequest and access agreement URL before we invoke this function.

// If user doesn't have an active session or if they already acknowledged // access agreement (based on the flag we store in the session) - bail out. if (sessionValue == null || sessionValue.accessAgreementAcknowledged) { return false; } // If access agreement is neither enabled globally (for all providers) // nor for the provider that authenticated user request - bail out. const providerConfig = this.options.config.authc.providers as Record<string, any>)[ sessionValue.provider.type ]?.[sessionValue.provider.name]; if (!this.options.config.accessAgreement && !providerConfig?.accessAgreement) { return false; } // Check if the current license allows access agreement. return this.options.license.getFeatures().allowAccessAgreement;

azasypkin · 2022-08-25T09:01:35Z

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

+      let accessAgreement: string = '';
+
+      if (sessionValue) {
+        const localAccessAgreement =


optional nit: we used "global vs provider-specific" terminology in the past, but I'm happy to change that if you think "local" makes more sense.

const accessAgreementMessage = providerSpecificAccessAgreement?.message ?? globalAccessAgreement?.message ?? '';

…-ref HEAD~1..HEAD --fix'

azasypkin

Looks great, just two more minor questions and I think we're good to go!

azasypkin · 2022-08-31T14:45:06Z

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

@@ -235,6 +236,7 @@ export const ConfigSchema = schema.object({
    hostname: schema.maybe(schema.string({ hostname: true })),
    port: schema.maybe(schema.number({ min: 0, max: 65535 })),
  }),
+  accessAgreement: schema.maybe(schema.object({ message: schema.maybe(schema.string()) })),


question: is there any value in making message optional (schema.maybe) in addition to already optional accessAgreement top-level property? It feels like message would be always required if access agreement is required.

I believe I have it how you're recommending, please double check I got it right!

azasypkin · 2022-08-31T14:46:51Z

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

+      const hasProviderSpecificAccessAgreement: boolean = !!accessAgreement?.message;
+      const hasGlobalAccessAgreement: boolean = !!config.accessAgreement?.message;
+
      if (!enabled) {
        delete providerGroup![name];
      } else {
+        const hasAccessAgreement: boolean =
+          hasProviderSpecificAccessAgreement || hasGlobalAccessAgreement;
+
        sortedProviders.push({
          type: type as any,
          name,
          order,
-          hasAccessAgreement: !!accessAgreement?.message,
+          hasAccessAgreement,


question: do we really need this change? I'm just trying to reduce changes only to those that are relevant to the change we'd like to make and are absolutely necessary (makes reviewing much easier), and it looks like we only need change in line 239 🙂

The security_usage_collector.ts references that field. Do you think the change would be better there or should I remove it?

Ah, I didn't realize that we introduced that field just for usage collector! I'd still make this check directly in the security_usage_collector.ts (the place that actually needs these two settings to be merged to a single accessAgreementEnabled field), but since hasAccessAgreement is used solely for the usage collector feel free to pick the option you like the most.

Ill make the change in the collector!

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

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

…-ref HEAD~1..HEAD --fix'

…-fix'

…-ref HEAD~1..HEAD --fix'

azasypkin

LGTM, great job!

azasypkin · 2022-09-01T14:44:46Z

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

  for (const [type, providerGroup] of Object.entries(providers)) {
    for (const [name, { enabled, order, accessAgreement }] of Object.entries(providerGroup ?? {})) {
      if (!enabled) {
        delete providerGroup![name];
      } else {
+        const hasAccessAgreement: boolean = !!accessAgreement?.message;


optional nit: boolean is inferred automatically.

Suggested change

const hasAccessAgreement: boolean = !!accessAgreement?.message;

const hasAccessAgreement = !!accessAgreement?.message;

azasypkin · 2022-09-01T15:04:32Z

Sorry @kc13greiner, I forgot to leave a note during review: would you mind allowing this new setting in our docker image (setting needs to be included in https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)?

jbudz

kibana-docker

kc13greiner · 2022-09-01T18:48:45Z

@gchaps I think this PR is finally ready - do the docs changes look alright?

gchaps

A few minor comments but otherwise LGTM.

gchaps · 2022-09-01T21:44:21Z

docs/settings/security-settings.asciidoc

@@ -162,6 +162,15 @@ Adds a message accessible at the login UI with additional help information for t
 xpack.security.authc.selector.enabled {ess-icon}::
 Determines if the login selector UI should be enabled. By default, this setting is set to `true` if more than one authentication provider is configured.

+[float]
+[[authentication-access-agreement-settings]]
+==== Configure a default access agreement


Other sections include this as a lead-in sentence:

You can configure the following settings in the kibana.yml file.

Oo! I'll add that in, thanks!!

docs/user/security/access-agreement.asciidoc

Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>

kibana-ci · 2022-09-06T15:12:48Z

💚 Build Succeeded

Buildkite Build
Commit: 4f2350a

Metrics [docs]

✅ unchanged

History

💔 Build #69677 failed d534b4b
💔 Build #69649 failed fd81dcc
💚 Build #69135 succeeded 8b49ada
💚 Build #68877 succeeded a15e0aa
💔 Build #68857 failed 9c77a54

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

Adding Global Access Agreement

680dc12

kc13greiner added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! release_note:feature Makes this part of the condensed release notes v8.5.0 ci:cloud-deploy Create or update a Cloud deployment labels Aug 22, 2022

kibanamachine and others added 2 commits August 22, 2022 14:42

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

055b6ae

…-ref HEAD~1..HEAD --fix'

Merge branch 'main' into feature/global_access_agreement

76049fe

kc13greiner marked this pull request as ready for review August 22, 2022 18:29

kc13greiner requested a review from a team as a code owner August 22, 2022 18:29

kc13greiner requested review from azasypkin and legrego August 22, 2022 18:29

kc13greiner added 2 commits August 23, 2022 13:58

Adding docs

f6135fc

Merge branch 'main' into feature/global_access_agreement

ddf492d

legrego removed their request for review August 24, 2022 13:03

Merge branch 'main' into feature/global_access_agreement

c68be02

azasypkin reviewed Aug 24, 2022

View reviewed changes

kc13greiner added 2 commits August 24, 2022 12:22

Merge branch 'main' into feature/global_access_agreement

d8d8f1d

PR Review changes

60e0c5f

kc13greiner commented Aug 24, 2022

View reviewed changes

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

af8a6fd

…-ref HEAD~1..HEAD --fix'

azasypkin reviewed Aug 25, 2022

View reviewed changes

kc13greiner and others added 5 commits August 26, 2022 09:28

PR Review changes

a3f6250

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

809d491

…-ref HEAD~1..HEAD --fix'

Fixing global access agreement redirect logic

613ae7b

Merge branch 'main' into feature/global_access_agreement

1a2fc33

Fixing unit test

a04f166

azasypkin reviewed Aug 31, 2022

View reviewed changes

kc13greiner and others added 8 commits August 31, 2022 11:35

Update x-pack/plugins/security/server/routes/views/access_agreement.ts

6c059dd

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

PR Feedback

4ed8ce4

Merge branch 'main' into feature/global_access_agreement

9b9128d

removing only

e70f02e

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

255a56a

…-ref HEAD~1..HEAD --fix'

[CI] Auto-commit changed files from 'node scripts/eslint --no-cache -…

9c77a54

…-fix'

Changing the accessAgreement to be optional

fcc2b9a

[CI] Auto-commit changed files from 'node scripts/precommit_hook.js -…

a15e0aa

…-ref HEAD~1..HEAD --fix'

kc13greiner requested a review from azasypkin August 31, 2022 22:52

azasypkin approved these changes Sep 1, 2022

View reviewed changes

Adding docker config

8b49ada

kc13greiner requested a review from a team as a code owner September 1, 2022 15:43

jbudz approved these changes Sep 1, 2022

View reviewed changes

gchaps approved these changes Sep 1, 2022

View reviewed changes

kc13greiner and others added 5 commits September 6, 2022 07:56

Update docs/user/security/access-agreement.asciidoc

6f7c9c9

Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>

Update docs/user/security/access-agreement.asciidoc

319c2c9

Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com>

Adding PR Review feedback

11d0ec6

Merge branch 'main' into feature/global_access_agreement

fd81dcc

Merge branch 'main' into feature/global_access_agreement

d534b4b

kc13greiner removed the ci:cloud-deploy Create or update a Cloud deployment label Sep 6, 2022

Merge branch 'main' into feature/global_access_agreement

4f2350a

kc13greiner merged commit c6fb0bc into elastic:main Sep 6, 2022

kibanamachine added the backport:skip This commit does not require backporting label Sep 6, 2022

kc13greiner deleted the feature/global_access_agreement branch September 6, 2022 15:55

kc13greiner linked an issue Sep 7, 2022 that may be closed by this pull request

Add a Kibana setting that applies an access agreement to all realms/providers #77269

Closed

legrego mentioned this pull request Sep 7, 2022

Add a Kibana setting that applies an access agreement to all realms/providers #77269

Closed

	const hasAccessAgreement: boolean = !!accessAgreement?.message;
	const hasAccessAgreement = !!accessAgreement?.message;

Adding Global Access Agreement #139217

Adding Global Access Agreement #139217

Conversation

kc13greiner commented Aug 22, 2022 • edited Loading

Summary

Release Notes

elasticmachine commented Aug 22, 2022

kc13greiner commented Aug 22, 2022

legrego commented Aug 22, 2022

azasypkin commented Aug 24, 2022

azasypkin left a comment

Choose a reason for hiding this comment

azasypkin Aug 24, 2022 • 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

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

azasypkin 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

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin commented Sep 1, 2022

jbudz left a comment

Choose a reason for hiding this comment

kc13greiner commented Sep 1, 2022

gchaps left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kibana-ci commented Sep 6, 2022

💚 Build Succeeded

Metrics [docs]

History

kc13greiner commented Aug 22, 2022 •

edited

Loading

azasypkin Aug 24, 2022 •

edited

Loading