-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Adds route config option access flag to indicate if an API is public or internal #152404
Changes from all commits
28972a4
53d2577
0ddbfc7
ac5eb63
30e903b
e0c356d
ce86192
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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.startsWith('/internal') ? 'internal' : 'public') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can be even stricter and add a trailing slash However, a route that starts with |
||
); | ||
} | ||
|
||
private getAuthRequired(request: RawRequest): boolean | 'optional' { | ||
if (isFakeRawRequest(request)) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -817,6 +817,56 @@ 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) => | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 'includes' would match any string that contains There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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(); | ||
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' }); | ||
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 () => { | ||
const { registerRouter, server: innerServer } = await server.setup(config); | ||
|
||
|
@@ -833,6 +883,7 @@ test('exposes route details of incoming request to a route handler', async () => | |
options: { | ||
authRequired: true, | ||
xsrfRequired: false, | ||
access: 'public', | ||
tags: [], | ||
timeout: {}, | ||
}, | ||
|
@@ -1010,6 +1061,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, | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -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<Ctx extends RqCtx = RqCtx> { | |||||
* const versionedRoute = versionedRouter | ||||||
* .post({ | ||||||
* path: '/api/my-app/foo/{id?}', | ||||||
* options: { timeout: { payload: 60000 } }, | ||||||
* options: { timeout: { payload: 60000 }, access: 'public' }, | ||||||
* }) | ||||||
* .addVersion( | ||||||
* { | ||||||
|
@@ -99,14 +100,28 @@ export interface VersionHTTPToolkit { | |||||
): VersionedRouter<Ctx>; | ||||||
} | ||||||
|
||||||
/** | ||||||
* Converts an input property from optional to required. Needed for making RouteConfigOptions['access'] required. | ||||||
*/ | ||||||
type WithRequiredProperty<Type, Key extends keyof Type> = Type & { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure we already have a util that does this but I couldn't find it. |
||||||
[Property in Key]-?: Type[Property]; | ||||||
}; | ||||||
|
||||||
/** | ||||||
* Versioned route access flag, required | ||||||
* - '/api/foo' is 'public' | ||||||
* - '/internal/my-foo' is 'internal' | ||||||
* Required | ||||||
*/ | ||||||
type VersionedRouteConfigOptions = WithRequiredProperty<RouteConfigOptions<RouteMethod>, 'access'>; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One alternative:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍🏻 . WDYT about moving your type helper to the type utilities package? |
||||||
/** | ||||||
* Configuration for a versioned route | ||||||
* @experimental | ||||||
*/ | ||||||
export type VersionedRouteConfig<Method extends RouteMethod> = Omit< | ||||||
RouteConfig<unknown, unknown, unknown, Method>, | ||||||
'validate' | ||||||
>; | ||||||
'validate' | 'options' | ||||||
> & { options: VersionedRouteConfigOptions }; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes |
||||||
|
||||||
/** | ||||||
* Create an {@link VersionedRoute | versioned route}. | ||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extracted to make it easier to read