Add UiSettings validation & Kibana default route redirection #59694

mshustov · 2020-03-09T19:01:55Z

Summary

UiSettings validation

Closes #50658
Covers 1-4 cases from #46717

There are several ways to configure an advanced setting today:

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

Through the Advanced Settings UI - a value is validated on the server-side, Kibana server rejects the request, UI reverts changes and shows a notification popup with an error message.
Locked via kibana.yml's uiSettings.overrides.<key> - validated on the server-side on the start
Through the client-side UI Settings Service a value is validated on the server-side, Kibana server rejects request
Through the server-side UI Settings Service - uiSettings service validates value on write and throws an exception if validation fails.
Through manual changes to the underlying config object - uiSettings service validates values on reading, ignores it if validation fails and logs a warning message.

As a bonus, uiSettings service validates on start defaults values. doesn't work currently since all declarations are done in LP

Default route validation

I got rid of all validation logic and moved the logic to NP

Checklist

Delete any items that are not applicable to this PR.

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

Dev docs

UiSettings definition allows to specify validation functions:

import { schema } from '@kbn/config-schema';

uiSettings.register({
   myUiSetting: {
      name: ...
      value: 'value',
      schema: schema.string()
  }
})

elasticmachine · 2020-03-09T19:02:07Z

Pinging @elastic/kibana-platform (Team:Platform)

pgayvallet · 2020-03-10T08:38:10Z

src/core/public/ui_settings/ui_settings_api.ts

+        if (err.response.status === 400) {
+          throw new Error(err.body.message);
+        }
+        if (err.response.status > 400) {
+          throw new Error(`Request failed with status code: ${err.response.status}`);


Probably related to the changes in src/core/server/ui_settings/routes/set.ts , but why the difference between 400 and 4xx?

Request failed with status code: ${err.response.status} is not really useful. I'd rather remove them completely and provide a more descriptive message, but BWC...So I added this for BadRequest only to provide an actionable message.

We really need typing for our errors so that we can evolve these 😢

src/core/server/server.ts

src/core/server/ui_settings/ui_settings_client.ts

pgayvallet · 2020-03-10T08:56:34Z

src/core/server/ui_settings/ui_settings_service.ts

+    for (const [key, definition] of this.uiSettingsDefaults) {
+      if (definition.schema) {
+        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);
+      if (definition?.schema) {
+        definition.schema.validate(value, {}, `ui settings overrides [${key}]`);
+      }
+    }


Maybe try/catch to collect all validation errors to throw an aggregated error to show them all at once in case of multiple errors?

I'd avoid rethrowing an error since it changes the error type and stack trace.

src/core/server/ui_settings/integration_tests/routes.test.ts

mshustov · 2020-03-10T12:20:48Z

src/legacy/core_plugins/tests_bundle/tests_entry_template.js

+        defaults: ${JSON.stringify(
+          defaultUiSettings,
+          (key, value) => {
+            if (value instanceof Type) return null;


That's a hack to fix karma tests.
We might have to refactor karma tests when move ui settings defaults declaration to the NP.

src/core/server/http/integration_tests/default_route_provider_config.test.ts

x-pack/test/functional/apps/spaces/enter_space.ts

mshustov · 2020-03-10T13:04:54Z

test/api_integration/apis/index.js

@@ -33,6 +33,5 @@ export default function({ loadTestFile }) {
    loadTestFile(require.resolve('./status'));
    loadTestFile(require.resolve('./stats'));
    loadTestFile(require.resolve('./ui_metric'));
-    loadTestFile(require.resolve('./core'));


a duplicate #59694 (comment)

mshustov · 2020-03-10T13:43:20Z

src/core/server/ui_settings/ui_settings_client.ts

@@ -147,17 +143,50 @@ export class UiSettingsClient implements IUiSettingsClient {
    return defaultsDeep(userProvided, this.defaults);
  }

+  private validateKey(key: string, value: unknown) {
+    const definition = this.defaults[key];
+    if (value === null || definition === undefined) return;


I think we should throw when set a key without definition. Should I add this logic now or add to the list of future improvements? That would be a breaking change

I agree that would be the preferred behaviour, but it feels like a risky change to make, it could easily cause exceptions in code. Maybe we should start with logging a message similar to setting an invalid key.

I think there's at least two cases where introducing an exception would be a problem:

Any settings that have been removed from the codebase but are still present in the user's config

Any settings that belong to a plugin that was enabled but is now disabled. I don't think we should ever break Kibana in this case.

mshustov · 2020-03-10T13:47:05Z

src/core/types/ui_settings.ts

+   * Value validation schema
+   * Used to validate value on write and read.
+   */
+  schema?: Type<T>;


I think we should make it required. No objections?

src/legacy/core_plugins/kibana/ui_setting_defaults.js

mshustov · 2020-03-10T17:14:12Z

src/core/server/ui_settings/integration_tests/routes.test.ts

+      uiSettings.register({
+        custom: {
+          value: '42',
+          schema: schema.string(),


I was thinking about dynamic vs static declaration and it seems that dynamic is simpler. The only downside I can think of that we cannot get all the ui settings defaults without starting Kibana. That would be useful for such cases as uiSettings.overrides validation.

Same issue as for SO types registration, so this is consistent.

mshustov · 2020-03-12T19:31:39Z

src/core/types/ui_settings.ts

+ * A sub-set of {@link UiSettingsParams} exposed to the client-side.
+ * @public
+ * */
+export type PublicUiSettingsParams = Omit<UiSettingsParams, 'schema'>;


I'm open to other naming suggestions. ther problem that https://github.com/elastic/kibana/pull/59694/files#diff-e3ce52429da99a601c0f3a737a9039daR51 is not an internal type

Fine with me

joshdover · 2020-03-15T21:15:56Z

src/core/server/ui_settings/ui_settings_client.ts

+    // validate value read from saved objects as it can be changed via SO API
+    const filteredValues: UserProvided<T> = {};
+    for (const [key, userValue] of Object.entries(values)) {
+      if (userValue === null || this.isOverridden(key)) continue;


I think this might be simpler & possibly safer if we validated the fully resolved settings (user provided + defaults) rather than just the user provided values. There's a small performance penalty for that, but I imagine it's negligible?

I just worry about this getting refactored at some point and the validation passing on settings that may not actually match the expected types 100%.

We will validate defaults and overrides in the start phase.

As a bonus, uiSettings service validates on start defaults values. doesn't work currently since all declarations are done in LP

I don't wait to waste CPU cycles since users usually adjust a handful of settings, so I'd rather prioritize #59755 to move definitions to the NP.

joshdover · 2020-03-15T21:17:16Z

src/core/server/ui_settings/ui_settings_client.ts

+        filteredValues[key] = {
+          userValue: userValue as T,
+        };
+      } catch (error) {


aside: the more places I see use config-schema, the more I think it shouldn't throw exception messages. Invalid data isn't really an exception, if anything, it's almost entirely expected.

Yeah. In my initial implementation, I had to wrap it in a function returning {error?: string, valid: boolean}. I'd prefer @kbn/config-schema to have such an interface.

@joshdover should I create an issue to investigate refactoring options?

joshdover · 2020-03-15T21:26:59Z

src/core/types/ui_settings.ts

+ * A sub-set of {@link UiSettingsParams} exposed to the client-side.
+ * @public
+ * */
+export type PublicUiSettingsParams = Omit<UiSettingsParams, 'schema'>;


Fine with me

kibanamachine · 2020-03-16T10:58:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 400a167

History

💔 Build #32899 failed 57c9aef
💚 Build #32712 succeeded 075fe07
💚 Build #32546 succeeded 84e3e5a
💔 Build #32529 failed 6e389a6
💔 Build #32488 failed 9856d0b

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

pgayvallet · 2020-03-16T10:56:22Z

src/core/public/types.ts

 export {
  UiSettingsParams,
+  PublicUiSettingsParams,
  UserProvidedValues,
  UiSettingsType,
  ImageValidation,


Not introduced by this PR, but we may want to rename the exposed types to use the UiSettings prefix at some point. Outside of the scope of the PR though.

I will create an issue

added to #48925

pgayvallet · 2020-03-16T11:10:44Z

src/core/server/ui_settings/integration_tests/routes.test.ts

+      uiSettings.register({
+        custom: {
+          value: '42',
+          schema: schema.string(),


Same issue as for SO types registration, so this is consistent.

mattkime

app-arch changes look good to me

…#59694) * add schema to ui settings params * add validation for defaults and overrides * validate in ui settings client * ui settings routes validation * clean up tests * use schema for defaultRoutes * move URL redirection to NP * fix spaces test * update docs * update kbn pm * fix karma test * fix tests * address comments * get rid of getDEfaultRoute * regen docs * fix tests * fix enter-spaces test * validate on relative url format * update i18n * fix enter-spoace test * move relative url validation to utils * add CoreApp containing application logic * extract public uiSettings params in a separate type * make schema required * update docs

* master: (40 commits) skips 'config_open.ts' files from linter check (elastic#60248) [Searchprofiler] Spacing between rendered shards (elastic#60238) Add UiSettings validation & Kibana default route redirection (elastic#59694) [SIEM][CASE] Change configuration button (elastic#60229) [Step 1][App Arch] Saved object migrations from kibana plugin to new platform (elastic#59044) adds new test (elastic#60064) [Uptime] Index Status API to Rest (elastic#59657) [SIEM] Adds 'Closes and opens signals' Cypress test (elastic#59950) [NP] Graph migration (elastic#59409) [ML] Clone analytics job (elastic#59791) Move VALUE_CLICK_TRIGGER and APPLY_FILTER_TRIGGER to ui_action… (elastic#60202) Handle improperly defined Watcher Logging Action text parameter. (elastic#60169) Move select range trigger to uiActions (elastic#60168) [SIEM][CASES] Configure cases: Final (elastic#59358) Closes elastic#59784. Sets xpack.apm.serviceMapEnabled default value true. (elastic#60153) [siem/cypress] update junit filename to be picked up by runbld (elastic#60156) OSS logic added to test environment (elastic#60134) Move canvas setup into app mount (elastic#59926) enable triggers_actions_ui plugin by default (elastic#60137) Expose Elasticsearch from start and deprecate from setup (elastic#59886) ...

…#60257) * add schema to ui settings params * add validation for defaults and overrides * validate in ui settings client * ui settings routes validation * clean up tests * use schema for defaultRoutes * move URL redirection to NP * fix spaces test * update docs * update kbn pm * fix karma test * fix tests * address comments * get rid of getDEfaultRoute * regen docs * fix tests * fix enter-spaces test * validate on relative url format * update i18n * fix enter-spoace test * move relative url validation to utils * add CoreApp containing application logic * extract public uiSettings params in a separate type * make schema required * update docs

mshustov added 7 commits March 9, 2020 15:33

add schema to ui settings params

b20ecc7

add validation for defaults and overrides

7e46aef

validate in ui settings client

5b55b83

ui settings routes validation

19dae10

clean up tests

012a715

use schema for defaultRoutes

cc96364

move URL redirection to NP

269e34e

mshustov added Feature:New Platform Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc labels Mar 9, 2020

pgayvallet reviewed Mar 10, 2020

View reviewed changes

mshustov added 6 commits March 10, 2020 10:26

fix spaces test

f2b2dea

Merge branch 'master' into issue-50658-default-route-redirection

39e7405

update docs

b736ada

update kbn pm

b01a668

fix karma test

5518bd0

fix tests

7a5a354