Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support validation for UI Settings #46717

Closed
legrego opened this issue Sep 26, 2019 · 8 comments
Closed

Support validation for UI Settings #46717

legrego opened this issue Sep 26, 2019 · 8 comments
Labels
enhancement New value added to drive a business result Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc

Comments

@legrego
Copy link
Member

legrego commented Sep 26, 2019

Proposal

The UI Settings Service should allow for individual settings to register custom validators to prevent invalid values from being read or written.

There are several ways to configure an advanced setting today:

  1. Through the Advanced Settings UI
  2. Locked via kibana.yml's uiSettings.overrides.<key>
  3. Through the client-side UI Settings Service
  4. Through the server-side UI Settings Service
  5. Through manual changes to the underlying config object

We can (and should) enforce validation on write for methods 1 - 4 above. Since option 5 is technically possible, even if not supported, we will also need to account for the fact that invalid settings may exist on read, and fallback to the default setting value.

Motivation

#44678 is introducing a new advanced setting to enable space-specific default routes. Previously, this setting was controlled by the server.defaultRoute option in kibana.yml.

Specifying this in the yml file ensured that only true admins had access to this configuration, and the schema validation (must start with a /) was sufficient enough to ensure that an obvious misconfiguration wasn't possible.

Now that this setting is moving to the Advanced Settings UI/UI Settings Service, is is trivial for any user with the appropriate privileges to accidentally misconfigure this setting. While the Advanced Settings UI comes with a warning that "you can break things here", we really should be providing some level of validation for each setting.

We ended up creating a "default route service" in order to encapsulate this logic, otherwise it would need to be duplicated in OSS and X-Pack. If the UI Settings service allowed for validation, this service could be removed altogether, and it would enable a better user experience by preventing the setting from being accidentally misconfigured to begin with.

Example:

.kibana index contains:

{
	"type": "config",
	"config": {
		"defaultRoute": "http://my-evil-kibana.com/app/gone_phishing"
	}
}

Reading and blindly trusting this value would send users to another domain completely. If the defaultRoute setting could register a validator, we could ensure that the route is a valid relative url before using it, without having to manually check every place we need to use this value.

/cc @kobelb

@legrego legrego added the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Sep 26, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-platform

@rudolf
Copy link
Contributor

rudolf commented Feb 26, 2020

If we want to also enforce validation for changes that come through the Saved Objects REST API as per #58437 we would either have to specify that the config object is hidden: true and create and expose a public REST API (#14873) or build the validation on top of Saved Object validation.

@telune
Copy link

telune commented Feb 27, 2020

Just my 2 cents, as I opened #58437: I could also easily use a dedicated Advanced Settings API (#14873), but I used the Saved Objects API as per @joshdover comment on #14873:

Agreed. If anything, we should just point users to the SavedObjects API in the meantime since that can be used as well (only tricky part is the object ID is the Kibana version number).

As correctly said in this thread, my main concern is validation.

@mshustov
Copy link
Contributor

validation schema can be added during uiSettings registration #59694
the next step is to add validation for all uiSettings defaults

@joshdover joshdover removed the Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc label Apr 30, 2021
@botelastic botelastic bot added the needs-team Issues missing a team label label Apr 30, 2021
@joshdover joshdover added Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc and removed needs-team Issues missing a team label labels Apr 30, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-core (Team:Core)

@TinaHeiligers
Copy link
Contributor

An audit of all calls to register uiSettings show that very few have explicit validations on the schema. Validations are defined explicitly in the following cases:

plugin setting type
file_upload fileUpload:maxFileSize string
reporting xpackReporting:customPdfLogo any
banners banners:textColor string
banners:backgroundColor string

Undeclared schema validations:

plugin setting(s) type
bfetch bfetch:disableCompression boolean
charts visualization:colorMapping string
dashboard labs:dashboard:enable_ui boolean
data several --
discover several --
maps_legacy visualization:tileMap:* --
region_map visualization:regionmap:showWarnings boolean
presentation_util labs:* --
saved_objects savedObjects:* --
share csv:* --
timelion timelion:showTutorial, timelion:default_columns, timelion:default_rows boolean, boolean, number
vis_type_timelion timelion: * (remainder) --
vis_type_timeseries metrics:max_buckets number
vis_type_vislib visualization:dimmingOpacity, visualization:heatmap:maxBuckets number
visualizations visualize:enableLabs, visualization:visualize:legacyChartsLibrary boolean, boolean
apm apm:enableServiceOverview boolean
x-pack/banners banners:placement, banners:textContent oneOf(literal), string
canvas labs:canvas:enable_ui boolean
dashboard_mode xpackDashboardMode:roles string[]
ml ml:anomalyDetection:results:enableTimeDefaults, ml:anomalyDetection:results:timeDefaults boolean, object containing from:string to:string
observability observability:enableInspectEsQueries boolean
rollup rollups:enableIndexPatterns boolean
security_solution securitySolution:* --

cc @joshdover

@mshustov
Copy link
Contributor

mshustov commented Jun 4, 2021

Undeclared schema validations:

What does it mean? I checked out the first 3 items from the list and all of them define validation schema:

schema: schema.boolean(),

schema: schema.string(),

schema: schema.boolean(),

We already validate definitions and overrides

private validatesDefinitions() {
for (const [key, definition] of this.uiSettingsDefaults) {
if (!definition.schema) {
throw new Error(`Validation schema is not provided for [${key}] UI Setting`);
}
definition.schema.validate(definition.value, {}, `ui settings defaults [${key}]`);
}
}
private validatesOverrides() {
for (const [key, value] of Object.entries(this.overrides)) {
const definition = this.uiSettingsDefaults.get(key);
// overrides might contain UiSettings for a disabled plugin
if (definition?.schema) {
definition.schema.validate(value, {}, `ui settings overrides [${key}]`);
}
}
}

uiSettings before write them to the store
this.validateKey(key, value);

and when reading them from store
this.validateKey(key, userValue);

It seems we are good to close the issue?

@rudolf
Copy link
Contributor

rudolf commented Jun 16, 2023

Closing as validation seems to be in place for all the ui settings I sampled.

@rudolf rudolf closed this as completed Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New value added to drive a business result Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc
Projects
None yet
Development

No branches or pull requests

7 participants