From 7b64a5435dc3392c31ae2865bc2ff9cc729f9117 Mon Sep 17 00:00:00 2001 From: Nick Peihl Date: Fri, 8 Apr 2022 17:42:39 -0400 Subject: [PATCH 1/2] [Maps] Apply styles to icon SVGs in legend via descendent selectors (#129255) --- .../__snapshots__/symbol_utils.test.js.snap | 63 +++++++++++++++++++ .../vector/components/legend/symbol_icon.tsx | 2 +- .../classes/styles/vector/symbol_utils.js | 20 +++--- .../styles/vector/symbol_utils.test.js | 25 +++++--- .../map_settings_panel/map_settings_panel.tsx | 12 ++-- 5 files changed, 97 insertions(+), 25 deletions(-) create mode 100644 x-pack/plugins/maps/public/classes/styles/vector/__snapshots__/symbol_utils.test.js.snap diff --git a/x-pack/plugins/maps/public/classes/styles/vector/__snapshots__/symbol_utils.test.js.snap b/x-pack/plugins/maps/public/classes/styles/vector/__snapshots__/symbol_utils.test.js.snap new file mode 100644 index 0000000000000..d4936639523c3 --- /dev/null +++ b/x-pack/plugins/maps/public/classes/styles/vector/__snapshots__/symbol_utils.test.js.snap @@ -0,0 +1,63 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`styleSvg Should add fill style property to svg element 1`] = ` +" + + + +" +`; + +exports[`styleSvg Should add stroke and stroke-wdth style properties to svg element 1`] = ` +" + + + +" +`; + +exports[`styleSvg Should not add style property when style not provided 1`] = ` +" + + + +" +`; + +exports[`styleSvg Should override any inherent fill and stroke styles in SVGs 1`] = ` +" + + + + + +" +`; diff --git a/x-pack/plugins/maps/public/classes/styles/vector/components/legend/symbol_icon.tsx b/x-pack/plugins/maps/public/classes/styles/vector/components/legend/symbol_icon.tsx index 4cc4d4169d7e0..fd9b952dbbdea 100644 --- a/x-pack/plugins/maps/public/classes/styles/vector/components/legend/symbol_icon.tsx +++ b/x-pack/plugins/maps/public/classes/styles/vector/components/legend/symbol_icon.tsx @@ -7,7 +7,7 @@ import React, { Component, CSSProperties } from 'react'; // @ts-expect-error -import { CUSTOM_ICON_PREFIX_SDF, getSymbolSvg, styleSvg, buildSrcUrl } from '../../symbol_utils'; +import { styleSvg, buildSrcUrl } from '../../symbol_utils'; interface Props { symbolId: string; diff --git a/x-pack/plugins/maps/public/classes/styles/vector/symbol_utils.js b/x-pack/plugins/maps/public/classes/styles/vector/symbol_utils.js index af165863ffc9c..108a2eb686dcf 100644 --- a/x-pack/plugins/maps/public/classes/styles/vector/symbol_utils.js +++ b/x-pack/plugins/maps/public/classes/styles/vector/symbol_utils.js @@ -106,15 +106,17 @@ export function buildSrcUrl(svgString) { export async function styleSvg(svgString, fill, stroke) { const svgXml = await parseXmlString(svgString); - let style = ''; - if (fill) { - style += `fill:${fill};`; - } - if (stroke) { - style += `stroke:${stroke};`; - style += `stroke-width:1;`; - } - if (style) svgXml.svg.$.style = style; + + // Elements nested under svg root may define style attribute + // Wildcard descendent selector provides more specificity to ensure root svg style attribute is applied instead of children style attributes + svgXml.svg.style = ` + svg * { + ${fill ? `fill: ${fill}` : '#000'} !important; + ${stroke ? `stroke: ${stroke}` : '#000'} !important; + stroke-width: 1 !important; + vector-effect: non-scaling-stroke !important; + } + `; const builder = new xml2js.Builder(); return builder.buildObject(svgXml); } diff --git a/x-pack/plugins/maps/public/classes/styles/vector/symbol_utils.test.js b/x-pack/plugins/maps/public/classes/styles/vector/symbol_utils.test.js index 8c85702b19579..f6d40bd70dbea 100644 --- a/x-pack/plugins/maps/public/classes/styles/vector/symbol_utils.test.js +++ b/x-pack/plugins/maps/public/classes/styles/vector/symbol_utils.test.js @@ -20,26 +20,33 @@ describe('styleSvg', () => { const unstyledSvgString = ''; const styledSvg = await styleSvg(unstyledSvgString); - expect(styledSvg.split('\n')[1]).toBe( - '' - ); + expect(styledSvg).toMatchSnapshot(); }); it('Should add fill style property to svg element', async () => { const unstyledSvgString = ''; const styledSvg = await styleSvg(unstyledSvgString, 'red'); - expect(styledSvg.split('\n')[1]).toBe( - '' - ); + expect(styledSvg).toMatchSnapshot(); }); it('Should add stroke and stroke-wdth style properties to svg element', async () => { const unstyledSvgString = ''; const styledSvg = await styleSvg(unstyledSvgString, 'red', 'white'); - expect(styledSvg.split('\n')[1]).toBe( - '' - ); + expect(styledSvg).toMatchSnapshot(); + }); + + it('Should override any inherent fill and stroke styles in SVGs', async () => { + const unstyledSvgString = ` + + + + + + `; + + const styledSvg = await styleSvg(unstyledSvgString, 'blue', 'black'); + expect(styledSvg).toMatchSnapshot(); }); }); diff --git a/x-pack/plugins/maps/public/connected_components/map_settings_panel/map_settings_panel.tsx b/x-pack/plugins/maps/public/connected_components/map_settings_panel/map_settings_panel.tsx index 1efa07e280039..005c40f48cce6 100644 --- a/x-pack/plugins/maps/public/connected_components/map_settings_panel/map_settings_panel.tsx +++ b/x-pack/plugins/maps/public/connected_components/map_settings_panel/map_settings_panel.tsx @@ -74,6 +74,12 @@ export function MapSettingsPanel({
+ + - -
From 27ff7d342416bda8b75600f45a07a6d82d3e7560 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Sat, 9 Apr 2022 10:04:14 -0400 Subject: [PATCH 2/2] Test config settings that are exposed to the browser (#129438) --- .github/CODEOWNERS | 1 + packages/kbn-config-schema/src/index.ts | 3 +- packages/kbn-config-schema/src/types/index.ts | 1 + .../src/types/object_type.test.ts | 73 ++++++ .../src/types/string_type.ts | 1 + packages/kbn-config-schema/src/types/type.ts | 23 ++ .../http_resources/http_resources_service.ts | 2 + src/core/server/http_resources/types.ts | 5 + .../plugins/create_browser_config.test.ts | 231 ++++++++++-------- .../server/plugins/create_browser_config.ts | 52 +++- .../server/plugins/plugins_service.test.ts | 6 +- src/core/server/plugins/plugins_service.ts | 4 +- .../server/rendering/rendering_service.tsx | 21 +- src/core/server/rendering/types.ts | 6 + src/core/server/server.api.md | 4 + .../plugins/rendering_plugin/server/plugin.ts | 4 +- .../test_suites/core_plugins/rendering.ts | 211 +++++++++++++++- 17 files changed, 521 insertions(+), 127 deletions(-) diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 9c1599a60dd0c..3c991e6a61d53 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -324,6 +324,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 /x-pack/plugins/spaces/ @elastic/kibana-security /x-pack/plugins/encrypted_saved_objects/ @elastic/kibana-security /x-pack/plugins/security/ @elastic/kibana-security diff --git a/packages/kbn-config-schema/src/index.ts b/packages/kbn-config-schema/src/index.ts index f9db84f255ec6..af545ac5eb9e6 100644 --- a/packages/kbn-config-schema/src/index.ts +++ b/packages/kbn-config-schema/src/index.ts @@ -38,6 +38,7 @@ import { NullableProps, RecordOfOptions, RecordOfType, + SchemaStructureEntry, StringOptions, StringType, Type, @@ -49,7 +50,7 @@ import { StreamType, } from './types'; -export type { AnyType, ConditionalType, TypeOf, Props, NullableProps }; +export type { AnyType, ConditionalType, TypeOf, Props, SchemaStructureEntry, NullableProps }; export { ObjectType, Type }; export { ByteSizeValue } from './byte_size_value'; export { SchemaTypeError, ValidationError } from './errors'; diff --git a/packages/kbn-config-schema/src/types/index.ts b/packages/kbn-config-schema/src/types/index.ts index 5152137985ff3..23b114fd824df 100644 --- a/packages/kbn-config-schema/src/types/index.ts +++ b/packages/kbn-config-schema/src/types/index.ts @@ -7,6 +7,7 @@ */ export type { TypeOptions } from './type'; +export type { SchemaStructureEntry } from './type'; export { Type } from './type'; export { AnyType } from './any_type'; export type { ArrayOptions } from './array_type'; diff --git a/packages/kbn-config-schema/src/types/object_type.test.ts b/packages/kbn-config-schema/src/types/object_type.test.ts index 67f0963fefdda..6548e808395b9 100644 --- a/packages/kbn-config-schema/src/types/object_type.test.ts +++ b/packages/kbn-config-schema/src/types/object_type.test.ts @@ -490,3 +490,76 @@ describe('#extends', () => { expect(extended.validate(undefined)).toEqual({ initial: 'bar', added: 42 }); }); }); + +test('returns schema structure', () => { + // This test covers different schema types that may or may not be nested + const objSchema = schema.object({ + any: schema.any(), + array: schema.arrayOf(schema.string()), + boolean: schema.boolean(), + buffer: schema.buffer(), + byteSize: schema.byteSize(), + conditional: schema.conditional( + schema.contextRef('context_value_1'), + schema.contextRef('context_value_2'), + schema.string(), + schema.string() + ), + duration: schema.duration(), + ip: schema.ip(), + literal: schema.literal('foo'), + map: schema.mapOf(schema.string(), schema.string()), + maybe: schema.maybe(schema.string()), + never: schema.never(), + nullable: schema.nullable(schema.string()), + number: schema.number(), + record: schema.recordOf(schema.string(), schema.string()), + stream: schema.stream(), + string: schema.string(), + union: schema.oneOf([schema.string()]), + uri: schema.uri(), + }); + const type = objSchema.extends({ + nested: objSchema, + }); + expect(type.getSchemaStructure()).toEqual([ + { path: ['any'], type: 'any' }, + { path: ['array'], type: 'array' }, + { path: ['boolean'], type: 'boolean' }, + { path: ['buffer'], type: 'binary' }, + { path: ['byteSize'], type: 'bytes' }, + { path: ['conditional'], type: 'any' }, + { path: ['duration'], type: 'duration' }, + { path: ['ip'], type: 'string' }, + { path: ['literal'], type: 'any' }, + { path: ['map'], type: 'map' }, + { path: ['maybe'], type: 'string' }, + { path: ['never'], type: 'any' }, + { path: ['nullable'], type: 'alternatives' }, + { path: ['number'], type: 'number' }, + { path: ['record'], type: 'record' }, + { path: ['stream'], type: 'stream' }, + { path: ['string'], type: 'string' }, + { path: ['union'], type: 'alternatives' }, + { path: ['uri'], type: 'string' }, + { path: ['nested', 'any'], type: 'any' }, + { path: ['nested', 'array'], type: 'array' }, + { path: ['nested', 'boolean'], type: 'boolean' }, + { path: ['nested', 'buffer'], type: 'binary' }, + { path: ['nested', 'byteSize'], type: 'bytes' }, + { path: ['nested', 'conditional'], type: 'any' }, + { path: ['nested', 'duration'], type: 'duration' }, + { path: ['nested', 'ip'], type: 'string' }, + { path: ['nested', 'literal'], type: 'any' }, + { path: ['nested', 'map'], type: 'map' }, + { path: ['nested', 'maybe'], type: 'string' }, + { path: ['nested', 'never'], type: 'any' }, + { path: ['nested', 'nullable'], type: 'alternatives' }, + { path: ['nested', 'number'], type: 'number' }, + { path: ['nested', 'record'], type: 'record' }, + { path: ['nested', 'stream'], type: 'stream' }, + { path: ['nested', 'string'], type: 'string' }, + { path: ['nested', 'union'], type: 'alternatives' }, + { path: ['nested', 'uri'], type: 'string' }, + ]); +}); diff --git a/packages/kbn-config-schema/src/types/string_type.ts b/packages/kbn-config-schema/src/types/string_type.ts index 1442c5b9b4efb..f2792e4031e0a 100644 --- a/packages/kbn-config-schema/src/types/string_type.ts +++ b/packages/kbn-config-schema/src/types/string_type.ts @@ -53,6 +53,7 @@ export class StringType extends Type { ); } + schema.type = 'string'; super(schema, options); } diff --git a/packages/kbn-config-schema/src/types/type.ts b/packages/kbn-config-schema/src/types/type.ts index 696101fb2c223..d3bab0106e5c4 100644 --- a/packages/kbn-config-schema/src/types/type.ts +++ b/packages/kbn-config-schema/src/types/type.ts @@ -15,6 +15,11 @@ export interface TypeOptions { validate?: (value: T) => string | void; } +export interface SchemaStructureEntry { + path: string[]; + type: string; +} + export const convertValidationFunction = ( validate: (value: T) => string | void ): CustomValidator => { @@ -98,6 +103,10 @@ export abstract class Type { return this.internalSchema; } + public getSchemaStructure() { + return recursiveGetSchemaStructure(this.internalSchema); + } + protected handleError( type: string, context: Record, @@ -141,3 +150,17 @@ export abstract class Type { return new SchemaTypeError(message || code, convertedPath); } } + +function recursiveGetSchemaStructure(internalSchema: AnySchema, path: string[] = []) { + const array: SchemaStructureEntry[] = []; + // Note: we are relying on Joi internals to obtain the schema structure (recursive keys). + // This is not ideal, but it works for now and we only need it for some integration test assertions. + // If it breaks in the future, we'll need to update our tests. + for (const [key, val] of (internalSchema as any)._ids._byKey.entries()) { + array.push(...recursiveGetSchemaStructure(val.schema, [...path, key])); + } + if (!array.length) { + array.push({ path, type: internalSchema.type ?? 'unknown' }); + } + return array; +} diff --git a/src/core/server/http_resources/http_resources_service.ts b/src/core/server/http_resources/http_resources_service.ts index 25467152ce4fe..95610d36d4230 100644 --- a/src/core/server/http_resources/http_resources_service.ts +++ b/src/core/server/http_resources/http_resources_service.ts @@ -98,6 +98,7 @@ export class HttpResourcesService implements CoreService { it('picks nothing by default', () => { + const configSchema = schema.object({ + notExposed1: schema.string(), + nested: schema.object({ + notExposed2: schema.boolean(), + notExposed3: schema.maybe(schema.number()), + }), + }); const config = { - foo: 'bar', + notExposed1: '1', nested: { - str: 'string', - num: 42, + notExposed2: true, + notExposed3: 3, }, }; - const descriptor: ExposedToBrowserDescriptor = {}; - - const browserConfig = createBrowserConfig(config, descriptor); + const descriptor: PluginConfigDescriptor> = { + schema: configSchema, + }; - expect(browserConfig).toEqual({}); + const result = createBrowserConfig(config, descriptor); + expect(result).toEqual({ browserConfig: {}, exposedConfigKeys: {} }); }); it('picks all the nested properties when using `true`', () => { - const config = { - foo: 'bar', - nested: { - str: 'string', - num: 42, - }, - }; - - const descriptor: ExposedToBrowserDescriptor = { - foo: true, - nested: true, - }; - - const browserConfig = createBrowserConfig(config, descriptor); - - expect(browserConfig).toEqual({ - foo: 'bar', - nested: { - str: 'string', - num: 42, - }, + const configSchema = schema.object({ + exposed1: schema.string(), + nested: schema.object({ + exposed2: schema.boolean(), + exposed3: schema.maybe(schema.number()), + }), + notExposed4: schema.string(), }); - }); - - it('picks specific nested properties when using a nested declaration', () => { const config = { - foo: 'bar', + exposed1: '1', nested: { - str: 'string', - num: 42, + exposed2: true, + exposed3: 3, }, + notExposed4: '4', }; - - const descriptor: ExposedToBrowserDescriptor = { - foo: true, - nested: { - str: true, - num: false, + const descriptor: PluginConfigDescriptor> = { + schema: configSchema, + exposeToBrowser: { + exposed1: true, + nested: true, }, }; - const browserConfig = createBrowserConfig(config, descriptor); - - expect(browserConfig).toEqual({ - foo: 'bar', - nested: { - str: 'string', + const result = createBrowserConfig(config, descriptor); + expect(result).toEqual({ + browserConfig: { + exposed1: '1', + nested: { exposed2: true, exposed3: 3 }, + // notExposed4 is not present + }, + exposedConfigKeys: { + exposed1: 'string', + 'nested.exposed2': 'boolean', + 'nested.exposed3': 'number', + // notExposed4 is not present }, }); }); - it('accepts deeply nested structures', () => { + it('picks specific nested properties, omitting those which are not specified', () => { + const configSchema = schema.object({ + exposed1: schema.string(), + nested: schema.object({ + exposed2: schema.boolean(), + notExposed3: schema.maybe(schema.number()), + }), + notExposed4: schema.string(), + }); const config = { - foo: 'bar', - deeply: { - str: 'string', - nested: { - hello: 'dolly', - structure: { - propA: 'propA', - propB: 'propB', - }, - }, + exposed1: '1', + nested: { + exposed2: true, + notExposed3: 3, }, + notExposed4: '4', }; - - const descriptor: ExposedToBrowserDescriptor = { - foo: false, - deeply: { - str: false, - nested: { - hello: true, - structure: { - propA: true, - propB: false, - }, - }, + const descriptor: PluginConfigDescriptor> = { + schema: configSchema, + exposeToBrowser: { + exposed1: true, + nested: { exposed2: true }, }, }; - const browserConfig = createBrowserConfig(config, descriptor); - - expect(browserConfig).toEqual({ - deeply: { - nested: { - hello: 'dolly', - structure: { - propA: 'propA', - }, - }, + const result = createBrowserConfig(config, descriptor); + expect(result).toEqual({ + browserConfig: { + exposed1: '1', + nested: { exposed2: true }, + // notExposed3 and notExposed4 are not present + }, + exposedConfigKeys: { + exposed1: 'string', + 'nested.exposed2': 'boolean', + // notExposed3 and notExposed4 are not present }, }); }); - it('only includes leaf properties that are `true` when in nested structures', () => { + it('picks specific deeply nested properties, omitting those which are not specified', () => { + const configSchema = schema.object({ + exposed1: schema.string(), + deeply: schema.object({ + exposed2: schema.boolean(), + nested: schema.object({ + exposed3: schema.maybe(schema.number()), + structure: schema.object({ + exposed4: schema.string(), + notExposed5: schema.string(), + }), + notExposed6: schema.string(), + }), + notExposed7: schema.string(), + }), + notExposed8: schema.string(), + }); const config = { - foo: 'bar', + exposed1: '1', deeply: { - str: 'string', + exposed2: true, nested: { - hello: 'dolly', + exposed3: 3, structure: { - propA: 'propA', - propB: 'propB', + exposed4: '4', + notExposed5: '5', }, + notExposed6: '6', }, + notExposed7: '7', }, + notExposed8: '8', }; - - const descriptor: ExposedToBrowserDescriptor = { - deeply: { - nested: { - hello: true, - structure: { - propA: true, + const descriptor: PluginConfigDescriptor> = { + schema: configSchema, + exposeToBrowser: { + exposed1: true, + deeply: { + exposed2: true, + nested: { + exposed3: true, + structure: { + exposed4: true, + }, }, }, }, }; - const browserConfig = createBrowserConfig(config, descriptor); - - expect(browserConfig).toEqual({ - deeply: { - nested: { - hello: 'dolly', - structure: { - propA: 'propA', + const result = createBrowserConfig(config, descriptor); + expect(result).toEqual({ + browserConfig: { + exposed1: '1', + deeply: { + exposed2: true, + nested: { + exposed3: 3, + structure: { + exposed4: '4', + }, }, }, + // notExposed5, notExposed6, notExposed7, and notExposed8 are not present + }, + exposedConfigKeys: { + exposed1: 'string', + 'deeply.exposed2': 'boolean', + 'deeply.nested.exposed3': 'number', + 'deeply.nested.structure.exposed4': 'string', + // notExposed5, notExposed6, notExposed7, and notExposed8 are not present }, }); }); diff --git a/src/core/server/plugins/create_browser_config.ts b/src/core/server/plugins/create_browser_config.ts index 95c8de7f4c8cd..0bf812d2e5cce 100644 --- a/src/core/server/plugins/create_browser_config.ts +++ b/src/core/server/plugins/create_browser_config.ts @@ -6,19 +6,25 @@ * Side Public License, v 1. */ -import { ExposedToBrowserDescriptor } from './types'; +import { ExposedToBrowserDescriptor, PluginConfigDescriptor } from './types'; export const createBrowserConfig = ( config: T, - descriptor: ExposedToBrowserDescriptor -): unknown => { - return recursiveCreateConfig(config, descriptor); + descriptor: PluginConfigDescriptor +) => { + if (!descriptor.exposeToBrowser) { + return { browserConfig: {}, exposedConfigKeys: {} }; + } + return { + browserConfig: recursiveCreateConfig(config, descriptor.exposeToBrowser), + exposedConfigKeys: getExposedConfigKeys(descriptor), + }; }; const recursiveCreateConfig = ( config: T, descriptor: ExposedToBrowserDescriptor = {} -): unknown => { +) => { return Object.entries(config || {}).reduce((browserConfig, [key, value]) => { const exposedConfig = descriptor[key as keyof ExposedToBrowserDescriptor]; if (exposedConfig && typeof exposedConfig === 'object') { @@ -30,3 +36,39 @@ const recursiveCreateConfig = ( return browserConfig; }, {} as Record); }; + +/** + * Given a plugin descriptor, this function returns an object that contains a flattened list of exposed config keys. This is used for a CI + * check to ensure that consumers don't accidentally expose more config settings to the browser than intended. + */ +function getExposedConfigKeys(descriptor: PluginConfigDescriptor) { + const schemaStructure = descriptor.schema.getSchemaStructure(); + const flattenedConfigSchema: Record = {}; + for (const { path, type } of schemaStructure) { + if (checkIsPathExposed(path, descriptor.exposeToBrowser!)) { + flattenedConfigSchema[path.join('.')] = type; + } + } + return flattenedConfigSchema; +} + +function checkIsPathExposed( + path: string[], + descriptor: ExposedToBrowserDescriptor +) { + let isExposed = false; + for (const key of path) { + // Traverse the path to see if it is exposed or not + const exposedConfig = descriptor[key as keyof ExposedToBrowserDescriptor]; + if (exposedConfig && typeof exposedConfig === 'object') { + // @ts-expect-error Type 'undefined' is not assignable to type 'ExposedToBrowserDescriptor' + descriptor = exposedConfig; + continue; + } + if (exposedConfig === true) { + isExposed = true; + } + break; + } + return isExposed; +} diff --git a/src/core/server/plugins/plugins_service.test.ts b/src/core/server/plugins/plugins_service.test.ts index 65ef756b39e17..7c6938bdde224 100644 --- a/src/core/server/plugins/plugins_service.test.ts +++ b/src/core/server/plugins/plugins_service.test.ts @@ -1012,14 +1012,16 @@ describe('PluginsService', () => { const prebootUIConfig$ = preboot.uiPlugins.browserConfigs.get('plugin-with-expose-preboot')!; await expect(prebootUIConfig$.pipe(take(1)).toPromise()).resolves.toEqual({ - sharedProp: 'sharedProp default value plugin-with-expose-preboot', + browserConfig: { sharedProp: 'sharedProp default value plugin-with-expose-preboot' }, + exposedConfigKeys: { sharedProp: 'string' }, }); const standardUIConfig$ = standard.uiPlugins.browserConfigs.get( 'plugin-with-expose-standard' )!; await expect(standardUIConfig$.pipe(take(1)).toPromise()).resolves.toEqual({ - sharedProp: 'sharedProp default value plugin-with-expose-standard', + browserConfig: { sharedProp: 'sharedProp default value plugin-with-expose-standard' }, + exposedConfigKeys: { sharedProp: 'string' }, }); }); diff --git a/src/core/server/plugins/plugins_service.ts b/src/core/server/plugins/plugins_service.ts index f202f09735d45..2c1ba26b96ebb 100644 --- a/src/core/server/plugins/plugins_service.ts +++ b/src/core/server/plugins/plugins_service.ts @@ -231,9 +231,7 @@ export class PluginsService implements CoreService createBrowserConfig(config, configDescriptor.exposeToBrowser!)) - ), + .pipe(map((config: any) => createBrowserConfig(config, configDescriptor))), ]; }) ); diff --git a/src/core/server/rendering/rendering_service.tsx b/src/core/server/rendering/rendering_service.tsx index 9d15c7df15715..73746a8f202ff 100644 --- a/src/core/server/rendering/rendering_service.tsx +++ b/src/core/server/rendering/rendering_service.tsx @@ -80,7 +80,7 @@ export class RenderingService { { http, uiPlugins, status }: RenderOptions, request: KibanaRequest, uiSettings: IUiSettingsClient, - { isAnonymousPage = false, vars }: IRenderOptions = {} + { isAnonymousPage = false, vars, includeExposedConfigKeys }: IRenderOptions = {} ) { const env = { mode: this.coreContext.env.mode, @@ -135,11 +135,15 @@ export class RenderingService { externalUrl: http.externalUrl, vars: vars ?? {}, uiPlugins: await Promise.all( - filteredPlugins.map(async ([id, plugin]) => ({ - id, - plugin, - config: await getUiConfig(uiPlugins, id), - })) + filteredPlugins.map(async ([id, plugin]) => { + const { browserConfig, exposedConfigKeys } = await getUiConfig(uiPlugins, id); + return { + id, + plugin, + config: browserConfig, + ...(includeExposedConfigKeys && { exposedConfigKeys }), + }; + }) ), legacyMetadata: { uiSettings: settings, @@ -155,5 +159,8 @@ export class RenderingService { const getUiConfig = async (uiPlugins: UiPlugins, pluginId: string) => { const browserConfig = uiPlugins.browserConfigs.get(pluginId); - return ((await browserConfig?.pipe(take(1)).toPromise()) ?? {}) as Record; + return ((await browserConfig?.pipe(take(1)).toPromise()) ?? { + browserConfig: {}, + exposedConfigKeys: {}, + }) as { browserConfig: Record; exposedConfigKeys: Record }; }; diff --git a/src/core/server/rendering/types.ts b/src/core/server/rendering/types.ts index 121c272089a7c..2c0aafe61e018 100644 --- a/src/core/server/rendering/types.ts +++ b/src/core/server/rendering/types.ts @@ -93,6 +93,12 @@ export interface IRenderOptions { * @internal */ vars?: Record; + + /** + * @internal + * This is only used for integration tests that allow us to verify which config keys are exposed to the browser. + */ + includeExposedConfigKeys?: boolean; } /** @internal */ diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index daba13d656f52..fbd6fd1492094 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -1135,6 +1135,8 @@ export interface HttpResources { // @public export interface HttpResourcesRenderOptions { headers?: ResponseHeaders; + // @internal + includeExposedConfigKeys?: boolean; } // @public @@ -1308,6 +1310,8 @@ export interface IntervalHistogram { // @public (undocumented) export interface IRenderOptions { + // @internal + includeExposedConfigKeys?: boolean; isAnonymousPage?: boolean; // @internal @deprecated vars?: Record; diff --git a/test/plugin_functional/plugins/rendering_plugin/server/plugin.ts b/test/plugin_functional/plugins/rendering_plugin/server/plugin.ts index c1c929b0e2ce7..d2e4d4386cbfd 100644 --- a/test/plugin_functional/plugins/rendering_plugin/server/plugin.ts +++ b/test/plugin_functional/plugins/rendering_plugin/server/plugin.ts @@ -31,9 +31,9 @@ export class RenderingPlugin implements Plugin { const { isAnonymousPage } = req.query; if (isAnonymousPage) { - return res.renderAnonymousCoreApp(); + return res.renderAnonymousCoreApp({ includeExposedConfigKeys: true }); } - return res.renderCoreApp(); + return res.renderCoreApp({ includeExposedConfigKeys: true }); } ); } diff --git a/test/plugin_functional/test_suites/core_plugins/rendering.ts b/test/plugin_functional/test_suites/core_plugins/rendering.ts index 4752f77f6cdae..c18e38cc1a4d6 100644 --- a/test/plugin_functional/test_suites/core_plugins/rendering.ts +++ b/test/plugin_functional/test_suites/core_plugins/rendering.ts @@ -6,6 +6,7 @@ * Side Public License, v 1. */ +import _ from 'lodash'; import expect from '@kbn/expect'; import '../../plugins/core_provider_plugin/types'; @@ -21,6 +22,9 @@ declare global { } } +const EXPOSED_CONFIG_SETTINGS_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'; + export default function ({ getService }: PluginFunctionalProviderContext) { const appsMenu = getService('appsMenu'); const browser = getService('browser'); @@ -41,6 +45,10 @@ export default function ({ getService }: PluginFunctionalProviderContext) { }); }; + const getInjectedMetadata = () => + browser.execute(() => { + return JSON.parse(document.querySelector('kbn-injected-metadata')!.getAttribute('data')!); + }); const getUserSettings = () => browser.execute(() => { return JSON.parse(document.querySelector('kbn-injected-metadata')!.getAttribute('data')!) @@ -53,9 +61,197 @@ export default function ({ getService }: PluginFunctionalProviderContext) { return window.__RENDERING_SESSION__; }); - // Talked to @dover, he aggreed we can skip these tests that are unexpectedly flaky - describe.skip('rendering service', () => { - it('renders "core" application', async () => { + describe('rendering service', () => { + it('exposes plugin config settings to authenticated users', async () => { + await navigateTo('/render/core'); + const injectedMetadata = await getInjectedMetadata(); + expect(injectedMetadata).to.not.be.empty(); + expect(injectedMetadata.uiPlugins).to.not.be.empty(); + + const actualExposedConfigKeys = []; + for (const { plugin, exposedConfigKeys } of injectedMetadata.uiPlugins) { + const configPath = Array.isArray(plugin.configPath) + ? plugin.configPath.join('.') + : plugin.configPath; + for (const [exposedConfigKey, type] of Object.entries(exposedConfigKeys)) { + actualExposedConfigKeys.push(`${configPath}.${exposedConfigKey} (${type})`); + } + } + 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 few other data types. This is only intended to be a hint to make it easier for future reviewers to understand + // what types of config settings can be exposed to the browser. + // 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)', + 'data.autocomplete.valueSuggestions.tiers (array)', + 'data.autocomplete.valueSuggestions.timeout (duration)', + 'data.search.aggs.shardDelay.enabled (boolean)', + 'enterpriseSearch.host (string)', + 'home.disableWelcomeScreen (boolean)', + 'map.emsFileApiUrl (string)', + 'map.emsFontLibraryUrl (string)', + 'map.emsLandingPageUrl (string)', + 'map.emsTileApiUrl (string)', + 'map.emsTileLayerId.bright (string)', + 'map.emsTileLayerId.dark (string)', + 'map.emsTileLayerId.desaturated (string)', + 'map.emsUrl (string)', + 'map.includeElasticMapsService (boolean)', + 'map.tilemap.options.attribution (string)', + 'map.tilemap.options.bounds (array)', + 'map.tilemap.options.default (boolean)', + 'map.tilemap.options.errorTileUrl (string)', + 'map.tilemap.options.maxZoom (number)', + 'map.tilemap.options.minZoom (number)', + 'map.tilemap.options.reuseTiles (boolean)', + 'map.tilemap.options.subdomains (array)', + 'map.tilemap.options.tileSize (number)', + 'map.tilemap.options.tms (boolean)', + 'map.tilemap.url (string)', + 'monitoring.kibana.collection.enabled (boolean)', + 'monitoring.kibana.collection.interval (number)', + 'monitoring.ui.ccs.enabled (boolean)', + 'monitoring.ui.container.apm.enabled (boolean)', + 'monitoring.ui.container.elasticsearch.enabled (boolean)', + 'monitoring.ui.container.logstash.enabled (boolean)', + 'monitoring.ui.enabled (boolean)', + 'monitoring.ui.min_interval_seconds (number)', + 'monitoring.ui.show_license_expiration (boolean)', + 'newsfeed.fetchInterval (duration)', + 'newsfeed.mainInterval (duration)', + 'newsfeed.service.pathTemplate (string)', + 'newsfeed.service.urlRoot (any)', + 'telemetry.allowChangingOptInStatus (boolean)', + 'telemetry.banner (boolean)', + 'telemetry.enabled (boolean)', + 'telemetry.optIn (any)', + 'telemetry.sendUsageFrom (alternatives)', + 'telemetry.sendUsageTo (any)', + 'usageCollection.uiCounters.debug (boolean)', + 'usageCollection.uiCounters.enabled (boolean)', + 'vis_type_vega.enableExternalUrls (boolean)', + 'xpack.apm.profilingEnabled (boolean)', + 'xpack.apm.serviceMapEnabled (boolean)', + 'xpack.apm.ui.enabled (boolean)', + 'xpack.apm.ui.maxTraceItems (number)', + 'xpack.apm.ui.transactionGroupBucketSize (number)', + 'xpack.cases.markdownPlugins.lens (boolean)', + 'xpack.ccr.ui.enabled (boolean)', + 'xpack.cloud.base_url (string)', + 'xpack.cloud.chat.chatURL (string)', + 'xpack.cloud.chat.enabled (boolean)', + 'xpack.cloud.cname (string)', + 'xpack.cloud.deployment_url (string)', + 'xpack.cloud.full_story.enabled (boolean)', + 'xpack.cloud.full_story.org_id (any)', + 'xpack.cloud.id (string)', + 'xpack.cloud.organization_url (string)', + 'xpack.cloud.profile_url (string)', + 'xpack.data_enhanced.search.sessions.cleanupInterval (duration)', + 'xpack.data_enhanced.search.sessions.defaultExpiration (duration)', + 'xpack.data_enhanced.search.sessions.enabled (boolean)', + 'xpack.data_enhanced.search.sessions.expireInterval (duration)', + 'xpack.data_enhanced.search.sessions.management.expiresSoonWarning (duration)', + 'xpack.data_enhanced.search.sessions.management.maxSessions (number)', + 'xpack.data_enhanced.search.sessions.management.refreshInterval (duration)', + 'xpack.data_enhanced.search.sessions.management.refreshTimeout (duration)', + 'xpack.data_enhanced.search.sessions.maxUpdateRetries (number)', + 'xpack.data_enhanced.search.sessions.monitoringTaskTimeout (duration)', + 'xpack.data_enhanced.search.sessions.notTouchedInProgressTimeout (duration)', + 'xpack.data_enhanced.search.sessions.notTouchedTimeout (duration)', + 'xpack.data_enhanced.search.sessions.pageSize (number)', + 'xpack.data_enhanced.search.sessions.trackingInterval (duration)', + 'xpack.discoverEnhanced.actions.exploreDataInChart.enabled (boolean)', + 'xpack.discoverEnhanced.actions.exploreDataInContextMenu.enabled (boolean)', + 'xpack.fleet.agents.enabled (boolean)', + 'xpack.global_search.search_timeout (duration)', + 'xpack.graph.canEditDrillDownUrls (boolean)', + 'xpack.graph.savePolicy (alternatives)', + 'xpack.ilm.ui.enabled (boolean)', + 'xpack.index_management.ui.enabled (boolean)', + 'xpack.infra.sources.default.fields.message (array)', + 'xpack.license_management.ui.enabled (boolean)', + 'xpack.maps.preserveDrawingBuffer (boolean)', + 'xpack.maps.showMapsInspectorAdapter (boolean)', + 'xpack.observability.unsafe.alertingExperience.enabled (boolean)', + 'xpack.observability.unsafe.cases.enabled (boolean)', + 'xpack.observability.unsafe.overviewNext.enabled (boolean)', + 'xpack.observability.unsafe.rules.enabled (boolean)', + 'xpack.osquery.actionEnabled (boolean)', + 'xpack.osquery.packs (boolean)', + 'xpack.osquery.savedQueries (boolean)', + 'xpack.remote_clusters.ui.enabled (boolean)', + /** + * NOTE: The Reporting plugin is currently disabled in functional tests (see test/functional/config.js). + * It will be re-enabled once #102552 is completed. + */ + // 'xpack.reporting.roles.allow (array)', + // 'xpack.reporting.roles.enabled (boolean)', + // 'xpack.reporting.poll.jobCompletionNotifier.interval (number)', + // 'xpack.reporting.poll.jobCompletionNotifier.intervalErrorMultiplier (number)', + // 'xpack.reporting.poll.jobsRefresh.interval (number)', + // 'xpack.reporting.poll.jobsRefresh.intervalErrorMultiplier (number)', + 'xpack.rollup.ui.enabled (boolean)', + 'xpack.saved_object_tagging.cache_refresh_interval (duration)', + 'xpack.security.loginAssistanceMessage (string)', + 'xpack.security.sameSiteCookies (alternatives)', + 'xpack.security.showInsecureClusterWarning (boolean)', + 'xpack.securitySolution.enableExperimental (array)', + 'xpack.snapshot_restore.slm_ui.enabled (boolean)', + 'xpack.snapshot_restore.ui.enabled (boolean)', + 'xpack.trigger_actions_ui.enableExperimental (array)', + 'xpack.trigger_actions_ui.enableGeoTrackingThresholdAlert (boolean)', + 'xpack.upgrade_assistant.readonly (boolean)', + 'xpack.upgrade_assistant.ui.enabled (boolean)', + ]; + // We don't assert that actualExposedConfigKeys and expectedExposedConfigKeys are equal, because test failure messages with large + // arrays are hard to grok. Instead, we take the difference between the two arrays and assert them separately, that way it's + // abundantly clear when the test fails that (A) Kibana is exposing a new key, or (B) Kibana is no longer exposing a key. + const extra = _.difference(actualExposedConfigKeys, expectedExposedConfigKeys).sort(); + const missing = _.difference(expectedExposedConfigKeys, actualExposedConfigKeys).sort(); + expect({ extra, missing }).to.eql({ extra: [], missing: [] }, EXPOSED_CONFIG_SETTINGS_ERROR); + }); + + it('exposes plugin config settings to unauthenticated users', async () => { + await navigateTo('/render/core?isAnonymousPage=true'); + const injectedMetadata = await getInjectedMetadata(); + expect(injectedMetadata).to.not.be.empty(); + expect(injectedMetadata.uiPlugins).to.not.be.empty(); + + const actualExposedConfigKeys = []; + for (const { plugin, exposedConfigKeys } of injectedMetadata.uiPlugins) { + const configPath = Array.isArray(plugin.configPath) + ? plugin.configPath.join('.') + : plugin.configPath; + for (const [exposedConfigKey, type] of Object.entries(exposedConfigKeys)) { + actualExposedConfigKeys.push(`${configPath}.${exposedConfigKey} (${type})`); + } + } + 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 few other data types. This is only intended to be a hint to make it easier for future reviewers to understand + // what types of config settings can be exposed to the browser. + // 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! + 'xpack.security.loginAssistanceMessage (string)', + 'xpack.security.sameSiteCookies (alternatives)', + 'xpack.security.showInsecureClusterWarning (boolean)', + ]; + // We don't assert that actualExposedConfigKeys and expectedExposedConfigKeys are equal, because test failure messages with large + // arrays are hard to grok. Instead, we take the difference between the two arrays and assert them separately, that way it's + // abundantly clear when the test fails that (A) Kibana is exposing a new key, or (B) Kibana is no longer exposing a key. + const extra = _.difference(actualExposedConfigKeys, expectedExposedConfigKeys).sort(); + const missing = _.difference(expectedExposedConfigKeys, actualExposedConfigKeys).sort(); + expect({ extra, missing }).to.eql({ extra: [], missing: [] }, EXPOSED_CONFIG_SETTINGS_ERROR); + }); + + // FLAKY + it.skip('renders "core" application', async () => { await navigateTo('/render/core'); const [loadingMessage, userSettings] = await Promise.all([ @@ -70,7 +266,8 @@ export default function ({ getService }: PluginFunctionalProviderContext) { expect(await exists('renderingHeader')).to.be(true); }); - it('renders "core" application without user settings', async () => { + // FLAKY + it.skip('renders "core" application without user settings', async () => { await navigateTo('/render/core?isAnonymousPage=true'); const [loadingMessage, userSettings] = await Promise.all([ @@ -85,7 +282,8 @@ export default function ({ getService }: PluginFunctionalProviderContext) { expect(await exists('renderingHeader')).to.be(true); }); - it('navigates between standard application and one with custom appRoute', async () => { + // FLAKY + it.skip('navigates between standard application and one with custom appRoute', async () => { await navigateTo('/'); await find.waitForElementStale(await findLoadingMessage()); @@ -108,7 +306,8 @@ export default function ({ getService }: PluginFunctionalProviderContext) { ]); }); - it('navigates between applications with custom appRoutes', async () => { + // FLAKY + it.skip('navigates between applications with custom appRoutes', async () => { await navigateTo('/'); await find.waitForElementStale(await findLoadingMessage());