From 28972a4706cb7498308b5e941f6acd82acd844b0 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Tue, 28 Feb 2023 12:03:15 -0700 Subject: [PATCH 1/7] Adds option API access flag to indicate if an API is public or internal --- .../src/request.ts | 8 ++++ .../src/route.ts | 6 ++- .../src/router.mock.ts | 2 +- .../src/http_server.test.ts | 45 +++++++++++++++++++ .../src/http_server.ts | 2 + .../src/lifecycle_handlers.ts | 1 + .../core-http-server/src/router/request.ts | 1 + .../http/core-http-server/src/router/route.ts | 12 +++++ 8 files changed, 75 insertions(+), 2 deletions(-) diff --git a/packages/core/http/core-http-router-server-internal/src/request.ts b/packages/core/http/core-http-router-server-internal/src/request.ts index 7e8e927bd4671..2a4d8667c2238 100644 --- a/packages/core/http/core-http-router-server-internal/src/request.ts +++ b/packages/core/http/core-http-router-server-internal/src/request.ts @@ -201,6 +201,7 @@ export class CoreKibanaRequest< xsrfRequired: ((request.route?.settings as RouteOptions)?.app as KibanaRouteOptions)?.xsrfRequired ?? true, // some places in LP call KibanaRequest.from(request) manually. remove fallback to true before v8 + access: this.getAccess(request), tags: request.route?.settings?.tags || [], timeout: { payload: payloadTimeout, @@ -222,6 +223,13 @@ export class CoreKibanaRequest< options, }; } + /** infer route access from path if not declared */ + private getAccess(request: RawRequest): 'internal' | 'public' { + return ( + ((request.route?.settings as RouteOptions)?.app as KibanaRouteOptions)?.access ?? + (request.path.includes('internal') ? 'internal' : 'public') + ); + } private getAuthRequired(request: RawRequest): boolean | 'optional' { if (isFakeRawRequest(request)) { diff --git a/packages/core/http/core-http-router-server-internal/src/route.ts b/packages/core/http/core-http-router-server-internal/src/route.ts index 2713d0cc19abc..7bbc208a08963 100644 --- a/packages/core/http/core-http-router-server-internal/src/route.ts +++ b/packages/core/http/core-http-router-server-internal/src/route.ts @@ -6,8 +6,12 @@ * Side Public License, v 1. */ -import type { RouteMethod, SafeRouteMethod } from '@kbn/core-http-server'; +import type { RouteConfigOptions, RouteMethod, SafeRouteMethod } from '@kbn/core-http-server'; export function isSafeMethod(method: RouteMethod): method is SafeRouteMethod { return method === 'get' || method === 'options'; } + +export function isPublicRoute(options: RouteConfigOptions) { + return options.access === 'public'; +} diff --git a/packages/core/http/core-http-router-server-mocks/src/router.mock.ts b/packages/core/http/core-http-router-server-mocks/src/router.mock.ts index 1a3262fdf1f80..9b2d90e18640b 100644 --- a/packages/core/http/core-http-router-server-mocks/src/router.mock.ts +++ b/packages/core/http/core-http-router-server-mocks/src/router.mock.ts @@ -71,7 +71,7 @@ function createKibanaRequestMock

({ routeTags, routeAuthRequired, validation = {}, - kibanaRouteOptions = { xsrfRequired: true }, + kibanaRouteOptions = { xsrfRequired: true, access: 'public' }, kibanaRequestState = { requestId: '123', requestUuid: '123e4567-e89b-12d3-a456-426614174000', diff --git a/packages/core/http/core-http-server-internal/src/http_server.test.ts b/packages/core/http/core-http-server-internal/src/http_server.test.ts index 92fa63c502558..cebcd61a3fa22 100644 --- a/packages/core/http/core-http-server-internal/src/http_server.test.ts +++ b/packages/core/http/core-http-server-internal/src/http_server.test.ts @@ -817,6 +817,49 @@ test('allows attaching metadata to attach meta-data tag strings to a route', asy await supertest(innerServer.listener).get('/without-tags').expect(200, { tags: [] }); }); +test('allows declaring route access to flag a route as public or internal', async () => { + const access = 'internal'; + const { registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router('', logger, enhanceWithContext); + router.get({ path: '/with-access', validate: false, options: { access } }, (context, req, res) => + res.ok({ body: { access: req.route.options.access } }) + ); + router.get({ path: '/without-access', validate: false }, (context, req, res) => + res.ok({ body: { access: req.route.options.access } }) + ); + registerRouter(router); + + await server.start(); + await supertest(innerServer.listener).get('/with-access').expect(200, { access }); + + await supertest(innerServer.listener).get('/without-access').expect(200, { access: 'public' }); +}); + +test('infers access flag from path if not defined', async () => { + const { registerRouter, server: innerServer } = await server.setup(config); + + const router = new Router('', logger, enhanceWithContext); + router.get({ path: '/internal/foo', validate: false }, (context, req, res) => + res.ok({ body: { access: req.route.options.access } }) + ); + router.get({ path: '/random/foo', validate: false }, (context, req, res) => + res.ok({ body: { access: req.route.options.access } }) + ); + router.get({ path: '/random/internal/foo', validate: false }, (context, req, res) => + res.ok({ body: { access: req.route.options.access } }) + ); + registerRouter(router); + + await server.start(); + await supertest(innerServer.listener).get('/internal/foo').expect(200, { access: 'internal' }); + + await supertest(innerServer.listener).get('/random/foo').expect(200, { access: 'public' }); + await supertest(innerServer.listener) + .get('/random/internal/foo') + .expect(200, { access: 'public' }); +}); + test('exposes route details of incoming request to a route handler', async () => { const { registerRouter, server: innerServer } = await server.setup(config); @@ -833,6 +876,7 @@ test('exposes route details of incoming request to a route handler', async () => options: { authRequired: true, xsrfRequired: false, + access: 'public', tags: [], timeout: {}, }, @@ -1010,6 +1054,7 @@ test('exposes route details of incoming request to a route handler (POST + paylo options: { authRequired: true, xsrfRequired: true, + access: 'public', tags: [], timeout: { payload: 10000, diff --git a/packages/core/http/core-http-server-internal/src/http_server.ts b/packages/core/http/core-http-server-internal/src/http_server.ts index fb19795d77dce..c02f1175d23e6 100644 --- a/packages/core/http/core-http-server-internal/src/http_server.ts +++ b/packages/core/http/core-http-server-internal/src/http_server.ts @@ -524,7 +524,9 @@ export class HttpServer { const kibanaRouteOptions: KibanaRouteOptions = { xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method), + access: route.options.access ?? (route.path.startsWith('/internal') ? 'internal' : 'public'), }; + this.log.debug(`kibanaRouteOptions [${kibanaRouteOptions.access}] for path [${route.path}]`); this.server!.route({ handler: route.handler, diff --git a/packages/core/http/core-http-server-internal/src/lifecycle_handlers.ts b/packages/core/http/core-http-server-internal/src/lifecycle_handlers.ts index 3fe9c8ac727ff..af148413265e8 100644 --- a/packages/core/http/core-http-server-internal/src/lifecycle_handlers.ts +++ b/packages/core/http/core-http-server-internal/src/lifecycle_handlers.ts @@ -60,6 +60,7 @@ export const createVersionCheckPostAuthHandler = (kibanaVersion: string): OnPost }; }; +// TODO: implement header required for accessing internal routes. See https://github.com/elastic/kibana/issues/151940 export const createCustomHeadersPreResponseHandler = (config: HttpConfig): OnPreResponseHandler => { const { name: serverName, diff --git a/packages/core/http/core-http-server/src/router/request.ts b/packages/core/http/core-http-server/src/router/request.ts index ef33bec14f841..e0664cb1ea29a 100644 --- a/packages/core/http/core-http-server/src/router/request.ts +++ b/packages/core/http/core-http-server/src/router/request.ts @@ -19,6 +19,7 @@ import type { Headers } from './headers'; */ export interface KibanaRouteOptions extends RouteOptionsApp { xsrfRequired: boolean; + access: 'internal' | 'public'; } /** diff --git a/packages/core/http/core-http-server/src/router/route.ts b/packages/core/http/core-http-server/src/router/route.ts index 78d76bb4ba7b8..f11a204dcb321 100644 --- a/packages/core/http/core-http-server/src/router/route.ts +++ b/packages/core/http/core-http-server/src/router/route.ts @@ -120,6 +120,18 @@ export interface RouteConfigOptions { */ xsrfRequired?: Method extends 'get' ? never : boolean; + /** + * Defines access permission for a route + * - public. The route is public, declared stable and intended for external access. + * In the future, may require an incomming request to contain a specified header. + * - internal. The route is internal and intended for internal access only. + * + * If not declared, infers access from route path: + * - access =`internal` for '/internal' route path prefix + * - access = `public` for '/api' route path prefix + */ + access?: 'public' | 'internal'; + /** * Additional metadata tag strings to attach to the route. */ From 53d2577201f84b8a04722bbfc21fc86a268b9e52 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Tue, 28 Feb 2023 15:46:31 -0700 Subject: [PATCH 2/7] Versioned routes must declare access --- .../core-version-http-server/src/example.ts | 2 +- .../src/version_http_toolkit.ts | 18 +++++++++++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/packages/core/versioning/core-version-http-server/src/example.ts b/packages/core/versioning/core-version-http-server/src/example.ts index de529ccb07d9d..b63c75e86a562 100644 --- a/packages/core/versioning/core-version-http-server/src/example.ts +++ b/packages/core/versioning/core-version-http-server/src/example.ts @@ -22,7 +22,7 @@ const versionedRouter = vtk.createVersionedRouter({ router }); const versionedRoute = versionedRouter .post({ path: '/api/my-app/foo/{id?}', - options: { timeout: { payload: 60000 } }, + options: { timeout: { payload: 60000 }, access: 'public' }, }) .addVersion( { diff --git a/packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts b/packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts index 719e0075c0070..ca1b90afe8788 100644 --- a/packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts +++ b/packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts @@ -13,6 +13,7 @@ import type { RequestHandler, RouteValidatorFullConfig, RequestHandlerContextBase, + RouteConfigOptions, } from '@kbn/core-http-server'; type RqCtx = RequestHandlerContextBase; @@ -45,7 +46,7 @@ export interface CreateVersionedRouterArgs { * const versionedRoute = versionedRouter * .post({ * path: '/api/my-app/foo/{id?}', - * options: { timeout: { payload: 60000 } }, + * options: { timeout: { payload: 60000 }, access: 'public' }, * }) * .addVersion( * { @@ -99,14 +100,25 @@ export interface VersionHTTPToolkit { ): VersionedRouter; } +type WithRequiredProperty = Type & { + [Property in Key]-?: Type[Property]; +}; + +/** + * Versioned route access flag, required + * - '/api/foo' is 'public' + * - '/internal/my-foo' is 'internal' + * Required + */ +type VersionedRouteConfigOptions = WithRequiredProperty, 'access'>; /** * Configuration for a versioned route * @experimental */ export type VersionedRouteConfig = Omit< RouteConfig, - 'validate' ->; + 'validate' | 'options' +> & { options: VersionedRouteConfigOptions }; /** * Create an {@link VersionedRoute | versioned route}. From 0ddbfc7b3a1c72623e709250f71ae8f022299c76 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Tue, 28 Feb 2023 16:38:23 -0700 Subject: [PATCH 3/7] Fix lifecycle_handlers test --- .../core-http-server-internal/src/lifecycle_handlers.test.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/core/http/core-http-server-internal/src/lifecycle_handlers.test.ts b/packages/core/http/core-http-server-internal/src/lifecycle_handlers.test.ts index 5e182005fd40c..d13bd001bbbb9 100644 --- a/packages/core/http/core-http-server-internal/src/lifecycle_handlers.test.ts +++ b/packages/core/http/core-http-server-internal/src/lifecycle_handlers.test.ts @@ -167,6 +167,7 @@ describe('xsrf post-auth handler', () => { path: '/some-path', kibanaRouteOptions: { xsrfRequired: false, + access: 'public', }, }); From ac5eb6332fae8a5b7dcd485620068ce80fd3d5e7 Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Tue, 28 Feb 2023 16:47:51 -0700 Subject: [PATCH 4/7] Remove unused log and helper --- .../core/http/core-http-router-server-internal/src/route.ts | 6 +----- .../core/http/core-http-server-internal/src/http_server.ts | 1 - 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/core/http/core-http-router-server-internal/src/route.ts b/packages/core/http/core-http-router-server-internal/src/route.ts index 7bbc208a08963..2713d0cc19abc 100644 --- a/packages/core/http/core-http-router-server-internal/src/route.ts +++ b/packages/core/http/core-http-router-server-internal/src/route.ts @@ -6,12 +6,8 @@ * Side Public License, v 1. */ -import type { RouteConfigOptions, RouteMethod, SafeRouteMethod } from '@kbn/core-http-server'; +import type { RouteMethod, SafeRouteMethod } from '@kbn/core-http-server'; export function isSafeMethod(method: RouteMethod): method is SafeRouteMethod { return method === 'get' || method === 'options'; } - -export function isPublicRoute(options: RouteConfigOptions) { - return options.access === 'public'; -} diff --git a/packages/core/http/core-http-server-internal/src/http_server.ts b/packages/core/http/core-http-server-internal/src/http_server.ts index c02f1175d23e6..1ef5be6c67a54 100644 --- a/packages/core/http/core-http-server-internal/src/http_server.ts +++ b/packages/core/http/core-http-server-internal/src/http_server.ts @@ -526,7 +526,6 @@ export class HttpServer { xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method), access: route.options.access ?? (route.path.startsWith('/internal') ? 'internal' : 'public'), }; - this.log.debug(`kibanaRouteOptions [${kibanaRouteOptions.access}] for path [${route.path}]`); this.server!.route({ handler: route.handler, From 30e903bb2aa83ec7bae952041968108a82b164eb Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Wed, 1 Mar 2023 08:48:07 -0700 Subject: [PATCH 5/7] Match path correctly, PR enhancements --- .../http/core-http-router-server-internal/src/request.ts | 2 +- packages/core/http/core-http-server/src/router/route.ts | 4 ++-- .../core-version-http-server/src/version_http_toolkit.ts | 6 +----- 3 files changed, 4 insertions(+), 8 deletions(-) diff --git a/packages/core/http/core-http-router-server-internal/src/request.ts b/packages/core/http/core-http-router-server-internal/src/request.ts index 2a4d8667c2238..26ac377c00bf3 100644 --- a/packages/core/http/core-http-router-server-internal/src/request.ts +++ b/packages/core/http/core-http-router-server-internal/src/request.ts @@ -227,7 +227,7 @@ export class CoreKibanaRequest< private getAccess(request: RawRequest): 'internal' | 'public' { return ( ((request.route?.settings as RouteOptions)?.app as KibanaRouteOptions)?.access ?? - (request.path.includes('internal') ? 'internal' : 'public') + (request.path.startsWith('/internal') ? 'internal' : 'public') ); } diff --git a/packages/core/http/core-http-server/src/router/route.ts b/packages/core/http/core-http-server/src/router/route.ts index f11a204dcb321..e2b11aec08e1a 100644 --- a/packages/core/http/core-http-server/src/router/route.ts +++ b/packages/core/http/core-http-server/src/router/route.ts @@ -121,14 +121,14 @@ export interface RouteConfigOptions { xsrfRequired?: Method extends 'get' ? never : boolean; /** - * Defines access permission for a route + * Defines intended request origin of the route: * - public. The route is public, declared stable and intended for external access. * In the future, may require an incomming request to contain a specified header. * - internal. The route is internal and intended for internal access only. * * If not declared, infers access from route path: * - access =`internal` for '/internal' route path prefix - * - access = `public` for '/api' route path prefix + * - access = `public` for everything else */ access?: 'public' | 'internal'; diff --git a/packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts b/packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts index ca1b90afe8788..be9532a721e2b 100644 --- a/packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts +++ b/packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts @@ -100,17 +100,13 @@ export interface VersionHTTPToolkit { ): VersionedRouter; } -type WithRequiredProperty = Type & { - [Property in Key]-?: Type[Property]; -}; - /** * Versioned route access flag, required * - '/api/foo' is 'public' * - '/internal/my-foo' is 'internal' * Required */ -type VersionedRouteConfigOptions = WithRequiredProperty, 'access'>; +type VersionedRouteConfigOptions = Required, 'access'>>; /** * Configuration for a versioned route * @experimental From e0c356dfaa15fd7ea1e22dae4ae6c629282b58ff Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Wed, 1 Mar 2023 12:20:43 -0700 Subject: [PATCH 6/7] revert utility type change --- .../core-version-http-server/src/version_http_toolkit.ts | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts b/packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts index be9532a721e2b..7d8dd7765e476 100644 --- a/packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts +++ b/packages/core/versioning/core-version-http-server/src/version_http_toolkit.ts @@ -100,13 +100,20 @@ export interface VersionHTTPToolkit { ): VersionedRouter; } +/** + * Converts an input property from optional to required. Needed for making RouteConfigOptions['access'] required. + */ +type WithRequiredProperty = Type & { + [Property in Key]-?: Type[Property]; +}; + /** * Versioned route access flag, required * - '/api/foo' is 'public' * - '/internal/my-foo' is 'internal' * Required */ -type VersionedRouteConfigOptions = Required, 'access'>>; +type VersionedRouteConfigOptions = WithRequiredProperty, 'access'>; /** * Configuration for a versioned route * @experimental From ce8619272911d96806e2e75d9edfeb692827bd4a Mon Sep 17 00:00:00 2001 From: "Christiane (Tina) Heiligers" Date: Wed, 1 Mar 2023 12:41:20 -0700 Subject: [PATCH 7/7] Adds test specifically for route path containing both api and internal --- .../http/core-http-server-internal/src/http_server.test.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/core/http/core-http-server-internal/src/http_server.test.ts b/packages/core/http/core-http-server-internal/src/http_server.test.ts index cebcd61a3fa22..b6a120e06ab8d 100644 --- a/packages/core/http/core-http-server-internal/src/http_server.test.ts +++ b/packages/core/http/core-http-server-internal/src/http_server.test.ts @@ -849,6 +849,10 @@ test('infers access flag from path if not defined', async () => { router.get({ path: '/random/internal/foo', validate: false }, (context, req, res) => res.ok({ body: { access: req.route.options.access } }) ); + + router.get({ path: '/api/foo/internal/my-foo', validate: false }, (context, req, res) => + res.ok({ body: { access: req.route.options.access } }) + ); registerRouter(router); await server.start(); @@ -858,6 +862,9 @@ test('infers access flag from path if not defined', async () => { await supertest(innerServer.listener) .get('/random/internal/foo') .expect(200, { access: 'public' }); + await supertest(innerServer.listener) + .get('/api/foo/internal/my-foo') + .expect(200, { access: 'public' }); }); test('exposes route details of incoming request to a route handler', async () => {