Test config settings that are exposed to the browser #129438

jportner · 2022-04-05T05:41:21Z

This PR adds integration tests that assert which config settings are exposed to the browser (both for authenticated and unauthenticated users).
It changes the Core rendering service to detect all config keys that could be exposed to the browser (via exposeToBrowser) and it returns those config keys in kbn-injected-metadata.

Note, these config keys are only injected for integration tests.

Test failures

Here are the three different types of failures you can encounter:

If you stop exposing a config setting to the browser, the test will fail.

Error: Actual config settings exposed to the browser do not match what is expected; this assertion fails if extra settings are present and/or expected settings are missing
+ expected - actual

 {
   "extra": []
-  "missing": [
-    "expected.setting.that.is.not.actually.exposed (string)"
-  ]
+  "missing": []
 }

If you expose a new config setting to the browser, the test will fail.

Error: Actual config settings exposed to the browser do not match what is expected; this
assertion fails if extra settings are present and/or expected settings are missing
+ expected - actual

 {
-  "extra": [
-    "actual.setting.that.is.unexpectedly.exposed (number)"
-  ]
+  "extra": []
   "missing": []
 }

If you do both, the test will fail.

Error: Actual config settings exposed to the browser do not match what is expected; this assertion fails if extra settings are present and/or expected settings are missing
+ expected - actual

 {
-  "extra": [
-    "actual.setting.that.is.unexpectedly.exposed (number)"
-  ]
-  "missing": [
-    "expected.setting.that.is.not.actually.exposed (string)"
-  ]
+  "extra": []
+  "missing": []
 }

Adds an integration test for all config keys that can be exposed to the browser (even if those config settings aren't used).

jportner

Author's notes for reviewers.

jportner · 2022-04-05T05:47:36Z

packages/kbn-config-schema/src/types/type.ts

+  public getSchemaStructure() {
+    return recursiveGetSchemaStructure(this.internalSchema);
+  }


We need to expose the schema structure to the rendering service so it can traverse the schema config key paths.

Seems that the cleanest way to do this is to add a getSchemaStructure API to the root schema type like this.

packages/kbn-config-schema/src/types/type.ts

jportner · 2022-04-05T05:48:11Z

src/core/server/plugins/create_browser_config.ts

+      // @ts-expect-error Type 'undefined' is not assignable to type 'ExposedToBrowserDescriptor<T>'
+      descriptor = exposedConfig;


🤷‍♂️
@pgayvallet any ideas?

src/core/server/rendering/rendering_service.tsx

jportner · 2022-04-05T05:50:10Z

test/plugin_functional/test_suites/core_plugins/rendering.ts

-  // Talked to @dover, he aggreed we can skip these tests that are unexpectedly flaky
-  describe.skip('rendering service', () => {


😬
I had to unskip the suite itself, but I skipped the individual tests below because I am not sure if they are still flaky or not.

…-config-exposure

src/core/server/plugins/plugins_service.ts

src/core/server/rendering/rendering_service.tsx

pgayvallet · 2022-04-05T21:26:05Z

src/core/server/plugins/create_browser_config.test.ts

+        // notExposed4 is not present
+      },
+      exposedConfigKeys: {
+        exposed1: 'any', // 'any' is the Joi schema type for strings


Do we really need to implement this logic trying to infer the type of each key from the schema?

If we only need the list of keys for the purpose of this PR, I'd rather just use and return that, the list of underlying keys, rather than having an half-working type detection.

WDYT?

The motivation behind this is:

In test/plugin_functional/test_suites/core_plugins/rendering.ts, we can clearly see what config keys are exposed and we can identify which ones are definitely harmless to expose (boolean, number, etc.):

const expectedExposedConfigKeys = [ // NOTE: each exposed config key has its schema type at the end in "(parentheses)". // The schema type comes from Joi; in particular, "(any)" can mean a string or a few other data types. // When plugin owners make a change that exposes additional config values, the changes will be reflected in this test assertion. // Ensure that your change does not unintentionally expose any sensitive values! 'console.ui.enabled (boolean)', 'dashboard.allowByValueEmbeddables (boolean)', 'data.autocomplete.querySuggestions.enabled (boolean)', 'data.autocomplete.valueSuggestions.enabled (boolean)', 'data.autocomplete.valueSuggestions.terminateAfter (duration)',

This isn't intended to be the "source of truth" for these config keys, just a bit of assistance for reviewers in the future that have to maintain that integration test.

packages/kbn-config-schema/src/types/type.ts

…-config-exposure

Some Fleet config keys are no longer exposed.

The only time we need to include exposedConfigKeys in injected metadata is for integration test assertions. This commit changes the approach so that these are omitted in normal operation.

jportner · 2022-04-07T14:53:32Z

.github/CODEOWNERS

@@ -327,6 +327,7 @@
 /src/plugins/interactive_setup/ @elastic/kibana-security
 /test/interactive_setup_api_integration/ @elastic/kibana-security
 /test/interactive_setup_functional/ @elastic/kibana-security
+/test/plugin_functional/test_suites/core_plugins/rendering.ts @elastic/kibana-security


Added the integration test to my team in CODEOWNERS so we get pinged when this test is changed 👍

lukeelmers

This LGTM. I also don't love the workaround of relying on Joi's internals to attempt to guess the schema type. However, since the security team will be doing CODEOWNERS reviews on these, I see how this should make your life much easier -- so I don't feel too strongly about it overall.

packages/kbn-config-schema/src/types/object_type.test.ts

…-config-exposure

pgayvallet

LGTM once the last failing tests have been fixed

…-config-exposure

jportner · 2022-04-09T12:32:09Z

@elasticmachine merge upstream

kibana-ci · 2022-04-09T14:04:14Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`@kbn/config-schema`	123	127	+4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`cloudSecurityPosture`	370.1KB	370.3KB	+259.0B

Unknown metric groups

API count

id	before	after	diff
`@kbn/config-schema`	125	129	+4

History

💔 Build #37113 failed eaf9d8c
💔 Build #37106 failed 419cd27
💔 Build #37094 failed f523fcc
💔 Build #37089 failed 46a9046
💛 Build #36972 was flaky 131288b

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

(cherry picked from commit 27ff7d3) # Conflicts: # .github/CODEOWNERS # packages/kbn-config-schema/src/index.ts

…#129857) * Test config settings that are exposed to the browser (#129438) (cherry picked from commit 27ff7d3) # Conflicts: # .github/CODEOWNERS # packages/kbn-config-schema/src/index.ts * Enable security plugin in OSS tests This is a partial backport of #111681, so the Kibana security plugin is enabled but Elasticsearch security is still disabled. * Fix exposed config key tests The exposed config keys are slightly different in the 7.17 branch. * Fix UI Capabilities tests The enterpriseSearch plugin does not have a required dependency on the security plugin in the 7.17 branch, so our bacported assertions for these tests needed to change accordingly.

jportner added release_note:skip Skip the PR/issue when compiling release notes v8.3.0 v7.17.3 labels Apr 5, 2022

WIP

a4738c4

Adds an integration test for all config keys that can be exposed to the browser (even if those config settings aren't used).

jportner force-pushed the test-plugin-browser-config-exposure branch from ff0d30f to a4738c4 Compare April 5, 2022 05:47

jportner commented Apr 5, 2022

View reviewed changes

jportner requested review from lukeelmers and pgayvallet April 5, 2022 05:51

Joe Portner added 3 commits April 5, 2022 09:24

Merge remote-tracking branch 'upstream/main' into test-plugin-browser…

cde15eb

…-config-exposure

Fix type export

249d74a

Add commented config keys for reporting plugin

49d3bd1

pgayvallet reviewed Apr 5, 2022

View reviewed changes

Joe Portner added 5 commits April 7, 2022 10:06

Merge remote-tracking branch 'upstream/main' into test-plugin-browser…

93305e1

…-config-exposure

Fix integration test failure

ba1e5da

Some Fleet config keys are no longer exposed.

Add separate test for anonymous pages

aa8cd78

Only include exposedConfigKeys in integration tests

c553bc3

The only time we need to include exposedConfigKeys in injected metadata is for integration test assertions. This commit changes the approach so that these are omitted in normal operation.

PR review feedback

810bdba

jportner marked this pull request as ready for review April 7, 2022 14:46

jportner requested a review from a team as a code owner April 7, 2022 14:46

jportner requested a review from pgayvallet April 7, 2022 14:46

Add test file to CODEOWNERS

cd124dc

jportner commented Apr 7, 2022

View reviewed changes

lukeelmers approved these changes Apr 7, 2022

View reviewed changes

Swap test assertions

131288b

pgayvallet reviewed Apr 8, 2022

View reviewed changes

packages/kbn-config-schema/src/types/object_type.test.ts Outdated Show resolved Hide resolved

pgayvallet mentioned this pull request Apr 8, 2022

[@kbn/config-schema] properly identify our string schema to be of string type #129841

Closed

Joe Portner and others added 3 commits April 8, 2022 09:38

Merge remote-tracking branch 'upstream/main' into test-plugin-browser…

c7f6c5c

…-config-exposure

Fix schema type and clarify test failure errors

46a9046

Whoops

f523fcc

pgayvallet approved these changes Apr 8, 2022

View reviewed changes

Fix test

03214bd

jportner enabled auto-merge (squash) April 8, 2022 21:12

Joe Portner added 3 commits April 8, 2022 17:14

Tweak assertion again

419cd27

Merge remote-tracking branch 'upstream/main' into test-plugin-browser…

cfc0e1c

…-config-exposure

Fix more tests

eaf9d8c

Merge branch 'main' into test-plugin-browser-config-exposure

4db7097

jportner merged commit 27ff7d3 into elastic:main Apr 9, 2022

jportner mentioned this pull request Apr 9, 2022

[7.17] Test config settings that are exposed to the browser (#129438) #129857

Merged

jportner added a commit to jportner/kibana that referenced this pull request Apr 9, 2022

Test config settings that are exposed to the browser (elastic#129438)

ef3aa64

(cherry picked from commit 27ff7d3) # Conflicts: # .github/CODEOWNERS # packages/kbn-config-schema/src/index.ts

jportner deleted the test-plugin-browser-config-exposure branch April 10, 2022 01:12

lukeelmers mentioned this pull request Apr 12, 2022

[Discuss] exposeToBrowser: controlling which vars are included on anonymous pages #129689

Open

pgayvallet mentioned this pull request Apr 20, 2022

Ability to extract all available kibana.yml settings for a CLI tool #130463

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test config settings that are exposed to the browser #129438

Test config settings that are exposed to the browser #129438

jportner commented Apr 5, 2022 •

edited

Loading

jportner left a comment

jportner Apr 5, 2022

jportner Apr 5, 2022

jportner Apr 5, 2022

pgayvallet Apr 5, 2022 •

edited

Loading

jportner Apr 7, 2022

jportner Apr 7, 2022

lukeelmers left a comment

pgayvallet left a comment •

edited

Loading

jportner commented Apr 9, 2022

kibana-ci commented Apr 9, 2022

API count

		// @ts-expect-error Type 'undefined' is not assignable to type 'ExposedToBrowserDescriptor<T>'
		descriptor = exposedConfig;

		// Talked to @dover, he aggreed we can skip these tests that are unexpectedly flaky
		describe.skip('rendering service', () => {

Test config settings that are exposed to the browser #129438

Test config settings that are exposed to the browser #129438

Conversation

jportner commented Apr 5, 2022 • edited Loading

Test failures

jportner left a comment

Choose a reason for hiding this comment

jportner Apr 5, 2022

Choose a reason for hiding this comment

jportner Apr 5, 2022

Choose a reason for hiding this comment

jportner Apr 5, 2022

Choose a reason for hiding this comment

pgayvallet Apr 5, 2022 • edited Loading

Choose a reason for hiding this comment

jportner Apr 7, 2022

Choose a reason for hiding this comment

jportner Apr 7, 2022

Choose a reason for hiding this comment

lukeelmers left a comment

Choose a reason for hiding this comment

pgayvallet left a comment • edited Loading

Choose a reason for hiding this comment

jportner commented Apr 9, 2022

kibana-ci commented Apr 9, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

API count

History

jportner commented Apr 5, 2022 •

edited

Loading

pgayvallet Apr 5, 2022 •

edited

Loading

pgayvallet left a comment •

edited

Loading