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

Add UiSettings validation & Kibana default route redirection #59694

Merged
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
b20ecc7
add schema to ui settings params
mshustov Mar 9, 2020
7e46aef
add validation for defaults and overrides
mshustov Mar 9, 2020
5b55b83
validate in ui settings client
mshustov Mar 9, 2020
19dae10
ui settings routes validation
mshustov Mar 9, 2020
012a715
clean up tests
mshustov Mar 9, 2020
cc96364
use schema for defaultRoutes
mshustov Mar 9, 2020
269e34e
move URL redirection to NP
mshustov Mar 9, 2020
f2b2dea
fix spaces test
mshustov Mar 10, 2020
39e7405
Merge branch 'master' into issue-50658-default-route-redirection
mshustov Mar 10, 2020
b736ada
update docs
mshustov Mar 10, 2020
b01a668
update kbn pm
mshustov Mar 10, 2020
5518bd0
fix karma test
mshustov Mar 10, 2020
7a5a354
fix tests
mshustov Mar 10, 2020
850d78c
address comments
mshustov Mar 10, 2020
a7d0100
get rid of getDEfaultRoute
mshustov Mar 10, 2020
8ec9103
Merge branch 'master' into issue-50658-default-route-redirection
mshustov Mar 10, 2020
b21efe2
Merge branch 'master' into issue-50658-default-route-redirection
mshustov Mar 11, 2020
cb40a2b
regen docs
mshustov Mar 11, 2020
2f8f956
fix tests
mshustov Mar 11, 2020
f17a82f
fix enter-spaces test
mshustov Mar 11, 2020
6b5f699
validate on relative url format
mshustov Mar 11, 2020
9856d0b
update i18n
mshustov Mar 11, 2020
6e389a6
Merge branch 'master' into issue-50658-default-route-redirection
mshustov Mar 11, 2020
84e3e5a
fix enter-spoace test
mshustov Mar 11, 2020
c72b5cc
Merge branch 'master' into issue-50658-default-route-redirection
mshustov Mar 12, 2020
ed8a9a9
move relative url validation to utils
mshustov Mar 12, 2020
1036d77
add CoreApp containing application logic
mshustov Mar 12, 2020
075fe07
extract public uiSettings params in a separate type
mshustov Mar 12, 2020
3b8ad0c
make schema required
mshustov Mar 12, 2020
57c9aef
Merge branch 'master' into issue-50658-default-route-redirection
mshustov Mar 12, 2020
2ad3292
Merge branch 'master' into issue-50658-default-route-redirection
mshustov Mar 16, 2020
400a167
update docs
mshustov Mar 16, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/kbn-config-schema/src/errors/validation_error.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,5 +44,8 @@ export class ValidationError extends SchemaError {

constructor(error: SchemaTypeError, namespace?: string) {
super(ValidationError.extractMessage(error, namespace), error);

// https://github.com/Microsoft/TypeScript/wiki/Breaking-Changes#extending-built-ins-like-error-array-and-map-may-no-longer-work
Object.setPrototypeOf(this, ValidationError.prototype);
}
}
10 changes: 7 additions & 3 deletions src/core/public/ui_settings/ui_settings_api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,14 @@ export class UiSettingsApi {
},
});
} catch (err) {
if (err.response && err.response.status >= 300) {
throw new Error(`Request failed with status code: ${err.response.status}`);
if (err.response) {
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}`);
Comment on lines +156 to +160
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
}

throw err;
} finally {
this.loadingCount$.next(this.loadingCount$.getValue() - 1);
Expand Down
18 changes: 14 additions & 4 deletions src/core/server/server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -122,8 +122,6 @@ export class Server {
context: contextServiceSetup,
});

this.registerDefaultRoute(httpSetup);

const capabilitiesSetup = this.capabilities.setup({ http: httpSetup });

const elasticsearchServiceSetup = await this.elasticsearch.setup({
Expand Down Expand Up @@ -167,6 +165,7 @@ export class Server {
});

this.registerCoreContext(coreSetup, renderingSetup);
this.registerDefaultRoute(httpSetup);

return coreSetup;
}
Expand Down Expand Up @@ -218,8 +217,19 @@ export class Server {
}

private registerDefaultRoute(httpSetup: InternalHttpServiceSetup) {
mshustov marked this conversation as resolved.
Show resolved Hide resolved
const router = httpSetup.createRouter('/core');
router.get({ path: '/', validate: false }, async (context, req, res) =>
const router = httpSetup.createRouter('/');
router.get({ path: '/', validate: false }, async (context, req, res) => {
const defaultRoute = await context.core.uiSettings.client.get<string>('defaultRoute');
const basePath = httpSetup.basePath.get(req);
const url = `${basePath}${defaultRoute}`;

return res.redirected({
headers: {
location: url,
},
});
});
router.get({ path: '/core', validate: false }, async (context, req, res) =>
res.ok({ body: { version: '0.0.1' } })
);
}
Expand Down
66 changes: 66 additions & 0 deletions src/core/server/ui_settings/integration_tests/routes.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Licensed to Elasticsearch B.V. under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch B.V. licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import { schema } from '@kbn/config-schema';
import * as kbnTestServer from '../../../../test_utils/kbn_server';

describe('ui settings service', () => {
describe('routes', () => {
let root: ReturnType<typeof kbnTestServer.createRoot>;
beforeAll(async () => {
root = kbnTestServer.createRoot();

const { uiSettings } = await root.setup();
uiSettings.register({
custom: {
value: '42',
schema: schema.string(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

},
});

await root.start();
}, 30000);
afterAll(async () => await root.shutdown());

describe('set', () => {
it('validates value', async () => {
const response = await kbnTestServer.request
.post(root, '/api/kibana/settings/custom')
.send({ value: 100 })
.expect(400);

expect(response.body.message).toBe(
'[validation [custom]]: expected value of type [string] but got [number]'
);
});
});
describe('set many', () => {
it('validates value', async () => {
const response = await kbnTestServer.request
.post(root, '/api/kibana/settings')
.send({ changes: { custom: 100 } })
.expect(400);
mshustov marked this conversation as resolved.
Show resolved Hide resolved

expect(response.body.message).toBe(
'[validation [custom]]: expected value of type [string] but got [number]'
);
});
});
});
});
4 changes: 2 additions & 2 deletions src/core/server/ui_settings/routes/set.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { schema } from '@kbn/config-schema';
import { schema, ValidationError } from '@kbn/config-schema';

import { IRouter } from '../../http';
import { SavedObjectsErrorHelpers } from '../../saved_objects';
Expand Down Expand Up @@ -56,7 +56,7 @@ export function registerSetRoute(router: IRouter) {
});
}

if (error instanceof CannotOverrideError) {
if (error instanceof CannotOverrideError || error instanceof ValidationError) {
return response.badRequest({ body: error });
}

Expand Down
4 changes: 2 additions & 2 deletions src/core/server/ui_settings/routes/set_many.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
* specific language governing permissions and limitations
* under the License.
*/
import { schema } from '@kbn/config-schema';
import { schema, ValidationError } from '@kbn/config-schema';

import { IRouter } from '../../http';
import { SavedObjectsErrorHelpers } from '../../saved_objects';
Expand Down Expand Up @@ -50,7 +50,7 @@ export function registerSetManyRoute(router: IRouter) {
});
}

if (error instanceof CannotOverrideError) {
if (error instanceof CannotOverrideError || error instanceof ValidationError) {
return response.badRequest({ body: error });
}

Expand Down
2 changes: 1 addition & 1 deletion src/core/server/ui_settings/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export interface IUiSettingsClient {
/**
* Returns registered uiSettings values {@link UiSettingsParams}
*/
getRegistered: () => Readonly<Record<string, UiSettingsParams>>;
getRegistered: () => Readonly<Record<string, Omit<UiSettingsParams, 'schema'>>>;
/**
* Retrieves uiSettings values set by the user with fallbacks to default values if not specified.
*/
Expand Down
Loading