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

[HTTP/OAS] Ability to exclude routes from introspection #192675

Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,23 @@ describe('HttpResources service', () => {
expect(registeredRouteConfig.options?.access).toBe('internal');
});

it('registration defaults to excluded from OAS', () => {
register({ ...routeConfig, options: { access: 'internal' } }, async (ctx, req, res) =>
res.ok()
);
const [[registeredRouteConfig]] = router.get.mock.calls;
expect(registeredRouteConfig.options?.excludeFromOAS).toBe(true);
});

it('registration allows being included in OAS', () => {
register(
{ ...routeConfig, options: { access: 'internal', excludeFromOAS: false } },
async (ctx, req, res) => res.ok()
);
const [[registeredRouteConfig]] = router.get.mock.calls;
expect(registeredRouteConfig.options?.excludeFromOAS).toBe(false);
});

describe('renderCoreApp', () => {
it('formats successful response', async () => {
register(routeConfig, async (ctx, req, res) => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ export class HttpResourcesService implements CoreService<InternalHttpResourcesSe
...route,
options: {
access: 'public',
excludeFromOAS: true,
...route.options,
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,7 @@ export interface InternalRouterRoute extends RouterRoute {

/** @internal */
interface InternalGetRoutesOptions {
/** @default false */
excludeVersionedRoutes?: boolean;
}

Expand Down Expand Up @@ -237,7 +238,7 @@ export class Router<Context extends RequestHandlerContextBase = RequestHandlerCo
if (excludeVersionedRoutes) {
return this.routes.filter((route) => !route.isVersioned);
}
return [...this.routes];
return this.routes;
Copy link
Member

Choose a reason for hiding this comment

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

probably unnecessary but this might remove a potential mutation guard we accedentaly had

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah man, I somehow missed this. That's a good point, will fix.

}

public handleLegacyErrors = wrapErrors;
Expand Down
76 changes: 43 additions & 33 deletions packages/core/http/core-http-server-internal/src/http_service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,13 @@

import { Observable, Subscription, combineLatest, firstValueFrom, of, mergeMap } from 'rxjs';
import { map } from 'rxjs';
import { schema, TypeOf } from '@kbn/config-schema';

import { pick, Semaphore } from '@kbn/std';
import { generateOpenApiDocument } from '@kbn/router-to-openapispec';
import {
generateOpenApiDocument,
type GenerateOpenApiDocumentOptionsFilters,
} from '@kbn/router-to-openapispec';
import { Logger } from '@kbn/logging';
import { Env } from '@kbn/config';
import type { CoreContext, CoreService } from '@kbn/core-base-server-internal';
Expand Down Expand Up @@ -254,49 +258,55 @@ export class HttpService
const baseUrl =
basePath.publicBaseUrl ?? `http://localhost:${config.port}${basePath.serverBasePath}`;

const stringOrStringArraySchema = schema.oneOf([
schema.string(),
schema.arrayOf(schema.string()),
]);
const querySchema = schema.object({
access: schema.maybe(schema.oneOf([schema.literal('public'), schema.literal('internal')])),
excludePathsMatching: schema.maybe(stringOrStringArraySchema),
pathStartsWith: schema.maybe(stringOrStringArraySchema),
pluginId: schema.maybe(schema.string()),
version: schema.maybe(schema.string()),
});

server.route({
path: '/api/oas',
method: 'GET',
handler: async (req, h) => {
const version = req.query?.version;

let pathStartsWith: undefined | string[];
if (typeof req.query?.pathStartsWith === 'string') {
pathStartsWith = [req.query.pathStartsWith];
} else {
pathStartsWith = req.query?.pathStartsWith;
}

let excludePathsMatching: undefined | string[];
if (typeof req.query?.excludePathsMatching === 'string') {
excludePathsMatching = [req.query.excludePathsMatching];
} else {
excludePathsMatching = req.query?.excludePathsMatching;
let filters: GenerateOpenApiDocumentOptionsFilters;
let query: TypeOf<typeof querySchema>;
try {
query = querySchema.validate(req.query);
Copy link
Contributor Author

@jloleysens jloleysens Oct 9, 2024

Choose a reason for hiding this comment

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

Refactor to using config-schema, just cleanup

filters = {
...query,
excludePathsMatching:
typeof query.excludePathsMatching === 'string'
? [query.excludePathsMatching]
: query.excludePathsMatching,
pathStartsWith:
typeof query.pathStartsWith === 'string'
? [query.pathStartsWith]
: query.pathStartsWith,
};
} catch (e) {
return h.response({ message: e.message }).code(400);
}

const pluginId = req.query?.pluginId;

const access = req.query?.access as 'public' | 'internal' | undefined;
if (access && !['public', 'internal'].some((a) => a === access)) {
return h
.response({
message: 'Invalid access query parameter. Must be one of "public" or "internal".',
})
.code(400);
}

return await firstValueFrom(
of(1).pipe(
HttpService.generateOasSemaphore.acquire(),
mergeMap(async () => {
try {
// Potentially quite expensive
const result = generateOpenApiDocument(this.httpServer.getRouters({ pluginId }), {
baseUrl,
title: 'Kibana HTTP APIs',
version: '0.0.0', // TODO get a better version here
filters: { pathStartsWith, excludePathsMatching, access, version },
});
const result = generateOpenApiDocument(
this.httpServer.getRouters({ pluginId: query.pluginId }),
{
baseUrl,
title: 'Kibana HTTP APIs',
version: '0.0.0', // TODO get a better version here
filters,
}
);
return h.response(result);
} catch (e) {
this.log.error(e);
Expand Down
11 changes: 10 additions & 1 deletion packages/core/http/core-http-server/src/router/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ export interface RouteConfigOptions<Method extends RouteMethod> {
/**
* 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.
* In the future, may require an incoming request to contain a specified header.
* - internal. The route is internal and intended for internal access only.
*
* Defaults to 'internal' If not declared,
Expand Down Expand Up @@ -284,6 +284,14 @@ export interface RouteConfigOptions<Method extends RouteMethod> {
*/
deprecated?: boolean;

/**
* Whether this route should be treated as "invisible" and excluded from router
* OAS introspection.
*
* @default false
*/
excludeFromOAS?: boolean;

/**
* Release version or date that this route will be removed
* Use with `deprecated: true`
Expand All @@ -292,6 +300,7 @@ export interface RouteConfigOptions<Method extends RouteMethod> {
* @example 9.0.0
*/
discontinued?: string;

/**
* Defines the security requirements for a route, including authorization and authentication.
*
Expand Down
5 changes: 4 additions & 1 deletion packages/kbn-router-to-openapispec/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,7 @@
* License v3.0 only", or the "Server Side Public License, v 1".
*/

export { generateOpenApiDocument } from './src/generate_oas';
export {
generateOpenApiDocument,
type GenerateOpenApiDocumentOptionsFilters,
} from './src/generate_oas';
9 changes: 9 additions & 0 deletions packages/kbn-router-to-openapispec/src/util.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,15 @@ describe('prepareRoutes', () => {
output: [{ path: '/api/foo', options: { access: pub } }],
filters: { excludePathsMatching: ['/api/b'], access: pub },
},
{
input: [
{ path: '/api/foo', options: { access: pub, excludeFromOAS: true } },
{ path: '/api/bar', options: { access: internal } },
{ path: '/api/baz', options: { access: pub } },
],
output: [{ path: '/api/baz', options: { access: pub } }],
filters: { excludePathsMatching: ['/api/bar'], access: pub },
},
])('returns the expected routes #%#', ({ input, output, filters }) => {
expect(prepareRoutes(input, filters)).toEqual(output);
});
Expand Down
3 changes: 2 additions & 1 deletion packages/kbn-router-to-openapispec/src/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,13 +105,14 @@ export const getVersionedHeaderParam = (
});

export const prepareRoutes = <
R extends { path: string; options: { access?: 'public' | 'internal' } }
R extends { path: string; options: { access?: 'public' | 'internal'; excludeFromOAS?: boolean } }
>(
routes: R[],
filters: GenerateOpenApiDocumentOptionsFilters = {}
): R[] => {
if (Object.getOwnPropertyNames(filters).length === 0) return routes;
return routes.filter((route) => {
if (route.options.excludeFromOAS) return false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where we actually exclude routes from OAS docs.

if (
filters.excludePathsMatching &&
filters.excludePathsMatching.some((ex) => route.path.startsWith(ex))
Expand Down
4 changes: 3 additions & 1 deletion src/core/server/integration_tests/http/oas.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,9 @@ it('only accepts "public" or "internal" for "access" query param', async () => {
const server = await startService({ config: { server: { oas: { enabled: true } } } });
const result = await supertest(server.listener).get('/api/oas').query({ access: 'invalid' });
expect(result.body.message).toBe(
'Invalid access query parameter. Must be one of "public" or "internal".'
`[access]: types that failed validation:
- [access.0]: expected value to equal [public]
- [access.1]: expected value to equal [internal]`
);
expect(result.status).toBe(400);
});