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

Serve static assets from NP #60490

Merged
merged 24 commits into from
Mar 27, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
a3cc842
Merge branch 'master' into issue-50654-http-resources-service
mshustov Mar 18, 2020
9bb4937
add hapi.inert plugin to NP
mshustov Mar 18, 2020
aaafdc6
update tests
mshustov Mar 18, 2020
475a5c3
move serving static assets
mshustov Mar 18, 2020
6aa6e56
update tests
mshustov Mar 18, 2020
fabb201
add functional tests
mshustov Mar 18, 2020
a2394df
fix type errors. Hapi.Request doesn't support typings for payload
mshustov Mar 18, 2020
e03576a
update docs
mshustov Mar 18, 2020
05e026c
remove comment
mshustov Mar 18, 2020
0dec6ca
move assets to NP
mshustov Mar 18, 2020
048c385
update all assets references
mshustov Mar 18, 2020
70e9ffc
Merge branch 'master' into issue-50654-http-resources-service
mshustov Mar 19, 2020
dc10e99
address Spencer's comments
mshustov Mar 19, 2020
3d7dcb0
move ui settings migration to migration examples
mshustov Mar 19, 2020
862d230
document legacy plugin spec
mshustov Mar 19, 2020
d0678c9
Merge branch 'master' into issue-50654-http-resources-service
mshustov Mar 20, 2020
3021e43
move platform assets test to integration_tests
mshustov Mar 20, 2020
59759d7
address Spencer's comment p.2
mshustov Mar 20, 2020
753f2c4
try to fix type errors
mshustov Mar 20, 2020
38fb7be
Merge branch 'master' into issue-50654-http-resources-service
mshustov Mar 24, 2020
9912b9a
fix merge commit
mshustov Mar 24, 2020
30effed
Merge branch 'master' into issue-50654-http-resources-service
mshustov Mar 27, 2020
ce9fbb5
Merge branch 'master' into issue-50654-http-resources-service
mshustov Mar 27, 2020
4aad487
update tests
mshustov Mar 27, 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
4 changes: 3 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@
"uiFramework:documentComponent": "cd packages/kbn-ui-framework && yarn documentComponent",
"kbn:watch": "node scripts/kibana --dev --logging.json=false",
"build:types": "tsc --p tsconfig.types.json",
"docs:acceptApiChanges": "node scripts/check_published_api_changes.js --accept",
"docs:acceptApiChanges": "node --max-old-space-size=6144 scripts/check_published_api_changes.js --accept",
"kbn:bootstrap": "yarn build:types && node scripts/register_git_hook",
"spec_to_console": "node scripts/spec_to_console",
"backport-skip-ci": "backport --prDescription \"[skip-ci]\"",
Expand Down Expand Up @@ -330,11 +330,13 @@
"@types/glob": "^7.1.1",
"@types/globby": "^8.0.0",
"@types/graphql": "^0.13.2",
"@types/h2o2": "^8.1.1",
"@types/hapi": "^17.0.18",
"@types/hapi-auth-cookie": "^9.1.0",
"@types/has-ansi": "^3.0.0",
"@types/history": "^4.7.3",
"@types/hoek": "^4.1.3",
"@types/inert": "^5.1.2",
"@types/jest": "24.0.19",
"@types/joi": "^13.4.2",
"@types/jquery": "^3.3.31",
Expand Down
13 changes: 11 additions & 2 deletions packages/kbn-optimizer/src/worker/webpack.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,21 @@ export function getWebpackConfig(bundle: Bundle, worker: WorkerConfig) {
loader: 'resolve-url-loader',
options: {
join: (_: string, __: any) => (uri: string, base?: string) => {
if (!base) {
// apply only to legacy platform styles
if (!base || !parseDirPath(base).dirs.includes('legacy')) {
return null;
}

if (uri.startsWith('ui/assets')) {
return Path.resolve(
worker.repoRoot,
'src/core/server/core_app/',
uri.replace('ui/', '')
);
}

// manually force ui/* urls in legacy styles to resolve to ui/legacy/public
if (uri.startsWith('ui/') && parseDirPath(base).dirs.includes('legacy')) {
if (uri.startsWith('ui/')) {
return Path.resolve(
worker.repoRoot,
'src/legacy/ui/public',
Expand Down
2 changes: 1 addition & 1 deletion packages/kbn-storybook/storybook_config/middleware.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,5 @@ const path = require('path');

// Extend the Storybook Middleware to include a route to access Legacy UI assets
module.exports = function(router) {
router.get('/ui', serve(path.resolve(__dirname, '../../../../src/legacy/ui/public/assets')));
router.get('/ui', serve(path.resolve(__dirname, '../../../src/core/server/core_app/assets')));
};
48 changes: 14 additions & 34 deletions src/core/MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
- [Core services](#core-services-1)
- [Plugin services](#plugin-services)
- [UI Exports](#ui-exports)
- [Plugin Spec](#plugin-spec)
- [How to](#how-to)
- [Configure plugin](#configure-plugin)
- [Handle plugin configuration deprecations](#handle-plugin-configuration-deprecations)
Expand Down Expand Up @@ -1264,40 +1265,19 @@ This table shows where these uiExports have moved to in the New Platform. In mos
| `visTypes` | `plugins.visualizations.types` | |
| `visualize` | | |

Examples:

- **uiSettingDefaults**

Before:

```js
uiExports: {
uiSettingDefaults: {
'my-plugin:my-setting': {
name: 'just-work',
value: true,
description: 'make it work',
category: ['my-category'],
},
}
}
```

After:

```ts
// src/plugins/my-plugin/server/plugin.ts
setup(core: CoreSetup){
core.uiSettings.register({
'my-plugin:my-setting': {
name: 'just-work',
value: true,
description: 'make it work',
category: ['my-category'],
},
})
}
```
#### Plugin Spec
| Legacy Platform | New Platform |
| ----------------------------- | ----------------------------------------------------------------------------------------------------------- |
| `id` | [`manifest.id`](/docs/development/core/server/kibana-plugin-core-server.pluginmanifest.md) |
| `require` | [`manifest.requiredPlugins`](/docs/development/core/server/kibana-plugin-core-server.pluginmanifest.md) |
| `version` | [`manifest.version`](/docs/development/core/server/kibana-plugin-core-server.pluginmanifest.md) |
| `kibanaVersion` | [`manifest.kibanaVersion`](/docs/development/core/server/kibana-plugin-core-server.pluginmanifest.md) |
| `configPrefix` | [`manifest.configPath`](/docs/development/core/server/kibana-plugin-core-server.pluginmanifest.md) |
| `config` | [export config](#configure-plugin) |
| `deprecations` | [export config](#handle-plugin-configuration-deprecations) |
| `uiExports` | `N/A`. Use platform & plugin public contracts |
| `publicDir` | `N/A`. Platform serves static assets from `/public/assets` folder under `/plugins/{id}/assets/{path*}` URL. |
| `preInit`, `init`, `postInit` | `N/A`. Use NP [lifecycle events](#services) |

## How to

Expand Down
36 changes: 35 additions & 1 deletion src/core/MIGRATION_EXAMPLES.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ APIs to their New Platform equivalents.
- [Chromeless Applications](#chromeless-applications)
- [Render HTML Content](#render-html-content)
- [Saved Objects types](#saved-objects-types)
- [UiSettings](#uisettings)

## Configuration

Expand Down Expand Up @@ -975,4 +976,37 @@ const migration: SavedObjectMigrationFn = (doc, { log }) => {...}

The `registerType` API will throw if called after the service has started, and therefor cannot be used from
legacy plugin code. Legacy plugins should use the legacy savedObjects service and the legacy way to register
saved object types until migrated.
saved object types until migrated.

## UiSettings
UiSettings defaults registration performed during `setup` phase via `core.uiSettings.register` API.

```js
// Before:
uiExports: {
uiSettingDefaults: {
'my-plugin:my-setting': {
name: 'just-work',
value: true,
description: 'make it work',
category: ['my-category'],
},
}
}
```

```ts
// After:
// src/plugins/my-plugin/server/plugin.ts
setup(core: CoreSetup){
core.uiSettings.register({
'my-plugin:my-setting': {
name: 'just-work',
value: true,
description: 'make it work',
category: ['my-category'],
schema: schema.boolean(),
},
})
}
```
12 changes: 12 additions & 0 deletions src/core/server/core_app/core_app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
* specific language governing permissions and limitations
* under the License.
*/
import Path from 'path';
import { fromRoot } from '../../../core/server/utils';

import { InternalCoreSetup } from '../internal_types';
import { CoreContext } from '../core_context';
import { Logger } from '../logging';
Expand All @@ -29,6 +32,7 @@ export class CoreApp {
setup(coreSetup: InternalCoreSetup) {
this.logger.debug('Setting up core app.');
this.registerDefaultRoutes(coreSetup);
this.registerStaticDirs(coreSetup);
}

private registerDefaultRoutes(coreSetup: InternalCoreSetup) {
Expand All @@ -49,4 +53,12 @@ export class CoreApp {
res.ok({ body: { version: '0.0.1' } })
);
}
private registerStaticDirs(coreSetup: InternalCoreSetup) {
coreSetup.http.registerStaticDir('/ui/{path*}', Path.resolve(__dirname, './assets'));
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'm open to the other options. we can place them under src/core/server/assets, for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

I know this PR doesn't yet expose this to plugins directly, but I wonder if there's really any value into allowing/requiring plugins to add the {path*} part of the path string. Should we do this for them automatically?

I think that if there is a real use case to only serving files of a certain directory depth, this should be exposed more explicitly via an option to registerStaticDir:

registerStaticDir({
  path: '/ui/',
  directory: Path.resolve(__dirname, './assets',
  depth: 3 // accepts positive integers, defaults to `Infinity`
})

Can be dealt with in PR that exposes to plugins. Just wanted to note while I thought about it. Also if there's no current need to specify the depth we probably don't need this option at all and should just automatically append the /{path*} when we register the route with Hapi.

Copy link
Contributor Author

@mshustov mshustov Mar 19, 2020

Choose a reason for hiding this comment

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

Can be dealt with in PR that exposes to plugins.

I don't think we should expose this API directly to the plugins. Otherwise, any plugin can expose an arbitrary folder (config folder with kibana.yml, for example)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We expose /public directory in the LP automatically:

getPublicDir() {
if (this._publicDir === false) {
return null;
}
if (!this._publicDir) {
return resolve(this.getPack().getPath(), 'public');
}
return this._publicDir;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should expose this API directly to the plugins. Otherwise, any plugin can expose an arbitrary folder (config folder with kibana.yml, for example)

We could have an API to only allow to serve from within the plugin's base folder. That would avoid the risk of exposing files from outside their folder.

i.e

// src/core/plugins/my_plugin/server/plugin.ts
coreSetup.http.registerStaticDir('/my-plugin-data/{path*}', 'my_assets_folder');

that would serve the src/core/plugins/my_plugin/my_assets_folder (or maybe src/core/plugins/my_plugin/server/my_assets_folder, need to decide)

I know this PR doesn't yet expose this to plugins directly, but I wonder if there's really any value into allowing/requiring plugins to add the {path*}

I agree, that doesn't seems necessary imho

Previous proposal example should probably be

coreSetup.http.registerStaticDir('/my-plugin-data', 'my_assets_folder');

I think that if there is a real use case to only serving files of a certain directory depth, this should be exposed more explicitly via an option to registerStaticDir

Don't really see any obvious need for that. KISS imho.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we don't need to expose to plugins, then as-is is good with me 👍


coreSetup.http.registerStaticDir(
'/node_modules/@kbn/ui-framework/dist/{path*}',
fromRoot('node_modules/@kbn/ui-framework/dist')
);
Comment on lines +59 to +62
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow. We sure were doing fun things.

}
}
51 changes: 51 additions & 0 deletions src/core/server/core_app/integration_tests/static_assets.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/*
* 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 * as kbnTestServer from '../../../../test_utils/kbn_server';
import { Root } from '../../root';

describe('Platform assets', function() {
let root: Root;

beforeAll(async function() {
root = kbnTestServer.createRoot();

await root.setup();
await root.start();
});

afterAll(async function() {
await root.shutdown();
});

it('exposes static assets', async () => {
await kbnTestServer.request.get(root, '/ui/favicons/favicon.ico').expect(200);
});

it('returns 404 if not found', async function() {
await kbnTestServer.request.get(root, '/ui/favicons/not-a-favicon.ico').expect(404);
});

it('does not expose folder content', async function() {
await kbnTestServer.request.get(root, '/ui/favicons/').expect(403);
});

it('does not allow file tree traversing', async function() {
await kbnTestServer.request.get(root, '/ui/../../../../../README.md').expect(404);
});
});
8 changes: 5 additions & 3 deletions src/core/server/http/base_path_proxy_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import { Agent as HttpsAgent, ServerOptions as TlsOptions } from 'https';
import apm from 'elastic-apm-node';
import { ByteSizeValue } from '@kbn/config-schema';
import { Server, Request, ResponseToolkit } from 'hapi';
import HapiProxy from 'h2o2';
import { sample } from 'lodash';
import BrowserslistUserAgent from 'browserslist-useragent';
import * as Rx from 'rxjs';
Expand Down Expand Up @@ -102,7 +103,7 @@ export class BasePathProxyServer {

// Register hapi plugin that adds proxying functionality. It can be configured
// through the route configuration object (see { handler: { proxy: ... } }).
await this.server.register({ plugin: require('h2o2') });
await this.server.register([HapiProxy]);

if (this.httpConfig.ssl.enabled) {
const tlsOptions = serverOptions.tls as TlsOptions;
Expand Down Expand Up @@ -166,7 +167,8 @@ export class BasePathProxyServer {
host: this.server.info.host,
passThrough: true,
port: this.devConfig.basePathProxyTargetPort,
protocol: this.server.info.protocol,
// typings mismatch. h2o2 doesn't support "socket"
protocol: this.server.info.protocol as HapiProxy.ProxyHandlerOptions['protocol'],
xforward: true,
},
},
Expand Down Expand Up @@ -195,7 +197,7 @@ export class BasePathProxyServer {
agent: this.httpsAgent,
passThrough: true,
xforward: true,
mapUri: (request: Request) => ({
mshustov marked this conversation as resolved.
Show resolved Hide resolved
mapUri: async (request: Request) => ({
uri: Url.format({
hostname: request.server.info.host,
port: this.devConfig.basePathProxyTargetPort,
Expand Down
25 changes: 24 additions & 1 deletion src/core/server/http/http_server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
* under the License.
*/
import { Server } from 'hapi';
import HapiStaticFiles from 'inert';
import url from 'url';

import { Logger, LoggerFactory } from '../logging';
Expand Down Expand Up @@ -44,6 +45,7 @@ export interface HttpServerSetup {
* @param router {@link IRouter} - a router with registered route handlers.
*/
registerRouter: (router: IRouter) => void;
registerStaticDir: (path: string, dirPath: string) => void;
basePath: HttpServiceSetup['basePath'];
csp: HttpServiceSetup['csp'];
createCookieSessionStorageFactory: HttpServiceSetup['createCookieSessionStorageFactory'];
Expand Down Expand Up @@ -97,10 +99,11 @@ export class HttpServer {
this.registeredRouters.add(router);
}

public setup(config: HttpConfig): HttpServerSetup {
public async setup(config: HttpConfig): Promise<HttpServerSetup> {
const serverOptions = getServerOptions(config);
const listenerOptions = getListenerOptions(config);
this.server = createServer(serverOptions, listenerOptions);
await this.server.register([HapiStaticFiles]);
joshdover marked this conversation as resolved.
Show resolved Hide resolved
this.config = config;

const basePathService = new BasePath(config.basePath);
Expand All @@ -109,6 +112,7 @@ export class HttpServer {

return {
registerRouter: this.registerRouter.bind(this),
registerStaticDir: this.registerStaticDir.bind(this),
registerOnPreAuth: this.registerOnPreAuth.bind(this),
registerOnPostAuth: this.registerOnPostAuth.bind(this),
registerOnPreResponse: this.registerOnPreResponse.bind(this),
Expand Down Expand Up @@ -339,4 +343,23 @@ export class HttpServer {
return t.next({ headers: authResponseHeaders });
});
}

private registerStaticDir(path: string, dirPath: string) {
if (this.server === undefined) {
throw new Error('Http server is not setup up yet');
}

this.server.route({
path,
method: 'GET',
handler: {
directory: {
path: dirPath,
listing: false,
lookupCompressed: true,
mshustov marked this conversation as resolved.
Show resolved Hide resolved
},
},
options: { auth: false },
});
}
}
1 change: 1 addition & 0 deletions src/core/server/http/http_service.mock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ const createSetupContractMock = () => {
registerRouteHandlerContext: jest.fn(),
registerOnPreResponse: jest.fn(),
createRouter: jest.fn().mockImplementation(() => mockRouter.create({})),
registerStaticDir: jest.fn(),
basePath: createBasePathMock(),
csp: CspConfig.DEFAULT,
auth: createAuthMock(),
Expand Down
2 changes: 2 additions & 0 deletions src/core/server/http/http_tools.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
*/

jest.mock('fs', () => ({
// Hapi Inert patches native methods
joshdover marked this conversation as resolved.
Show resolved Hide resolved
...jest.requireActual('fs'),
readFileSync: jest.fn(),
}));

Expand Down
1 change: 1 addition & 0 deletions src/core/server/http/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,7 @@ export interface InternalHttpServiceSetup
auth: HttpServerSetup['auth'];
server: HttpServerSetup['server'];
createRouter: (path: string, plugin?: PluginOpaqueId) => IRouter;
registerStaticDir: (path: string, dirPath: string) => void;
getAuthHeaders: GetAuthHeaders;
registerRouteHandlerContext: <T extends keyof RequestHandlerContext>(
pluginOpaqueId: PluginOpaqueId,
Expand Down
10 changes: 9 additions & 1 deletion src/core/server/legacy/legacy_service.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,15 @@ beforeEach(() => {
contracts: new Map([['plugin-id', 'plugin-value']]),
uiPlugins: {
public: new Map([['plugin-id', {} as DiscoveredPlugin]]),
internal: new Map([['plugin-id', { publicTargetDir: 'path/to/target/public' }]]),
internal: new Map([
[
'plugin-id',
{
publicTargetDir: 'path/to/target/public',
publicAssetsDir: '/plugins/name/assets/',
},
],
]),
browserConfigs: new Map(),
},
},
Expand Down
3 changes: 3 additions & 0 deletions src/core/server/legacy/legacy_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,9 @@ export class LegacyService implements CoreService {
plugins: startDeps.plugins,
},
__internals: {
http: {
registerStaticDir: setupDeps.core.http.registerStaticDir,
},
hapiServer: setupDeps.core.http.server,
kibanaMigrator: startDeps.core.savedObjects.migrator,
uiPlugins: setupDeps.core.plugins.uiPlugins,
Expand Down
Loading