-
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
[http] Adds route config option access flag to indicate if an API is public or internal #152404
Conversation
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.
draft self-review
@@ -222,6 +223,13 @@ export class CoreKibanaRequest< | |||
options, | |||
}; | |||
} | |||
/** infer route access from path if not declared */ | |||
private getAccess(request: RawRequest): 'internal' | 'public' { |
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
packages/core/http/core-http-router-server-internal/src/route.ts
Outdated
Show resolved
Hide resolved
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 comment
The reason will be displayed to describe this comment to others. Learn more.
'includes' would match any string that contains /internal
, so paths such as /api/foo/internal/my-foo
would have been inferred as public. This is a simple test to verify a 'mixed' path string.
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.
Looks like just /api/foo/internal/my-foo
would be inferred to internal
?
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.
/api/foo/internal/my-foo
should be inferred as public
actually but the commit where I changed the string matching didn't make it to the PR 🤕 .
I'm ok with this though, if folks want to use a "mixed" path string they can declare access
upfront.
packages/core/http/core-http-server-internal/src/http_server.ts
Outdated
Show resolved
Hide resolved
@@ -99,14 +100,25 @@ export interface VersionHTTPToolkit { | |||
): VersionedRouter<Ctx>; | |||
} | |||
|
|||
type WithRequiredProperty<Type, Key extends keyof Type> = Type & { |
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.
I'm pretty sure we already have a util that does this but I couldn't find it.
'validate' | ||
>; | ||
'validate' | 'options' | ||
> & { options: VersionedRouteConfigOptions }; |
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.
makes access
required for versioned routes
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.
Great work @TinaHeiligers ! Overall this is looking great and I think this will be a really nice improvement. I left a few comments I'd like to get your thoughts on before approving.
One other thought: I see we are determining internal
/public
at the CoreKibanaRequest
level. However, we know this at the Router
level (i.e. we know this when the route is registered, not only when a request is made). We could calculate it once and pass it in to CoreKibanaRequest.from
factory as an additional arg. I get this is more of a refactor and maybe not worth it for this work though.
private getAccess(request: RawRequest): 'internal' | 'public' { | ||
return ( | ||
((request.route?.settings as RouteOptions)?.app as KibanaRouteOptions)?.access ?? | ||
(request.path.includes('internal') ? 'internal' : 'public') |
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.
This algo may have unintended consequences. The following will be classified as internal
and I'm not sure they should be:
post /api/monitoring/v1/elasticsearch_settings/check/internal_monitoring
post /api/monitoring/v1/setup/collection/{clusterUuid}/disable_internal_collection
imo matching more strictly would reduce the potential for surprising outcomes which is better than missing a few routes. If routes should be internal they can add the access: internal
flag. It looks like 99.9% of "internal" routes follow the convention of starting with /internal/
so I would match that. Just my opinion, let me know what you think!
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.
you're totally right! I'm fixing it
@@ -120,6 +120,18 @@ export interface RouteConfigOptions<Method extends RouteMethod> { | |||
*/ | |||
xsrfRequired?: Method extends 'get' ? never : boolean; | |||
|
|||
/** | |||
* Defines access permission for a 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.
Maybe: "Intended request origin of the route". I think it is OK to call the property access
but I think we should make it clear in the doc comment this is not a security feature.
* | ||
* If not declared, infers access from route path: | ||
* - access =`internal` for '/internal' route path prefix | ||
* - access = `public` for '/api' route path prefix |
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.
* - access = `public` for '/api' route path prefix | |
* - access = `public` for everything else |
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like just /api/foo/internal/my-foo
would be inferred to internal
?
* - '/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 comment
The reason will be displayed to describe this comment to others. Learn more.
One alternative:
type VersionedRouteConfigOptions = WithRequiredProperty<RouteConfigOptions<RouteMethod>, 'access'>; | |
type VersionedRouteConfigOptions = Required<Pick<RouteConfigOptions<RouteMethod>, 'access'>>; |
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.
Using Pick
doesn't allow us to specify timeout
.
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.
👍🏻 . WDYT about moving your type helper to the type utilities package?
private getAccess(request: RawRequest): 'internal' | 'public' { | ||
return ( | ||
((request.route?.settings as RouteOptions)?.app as KibanaRouteOptions)?.access ?? | ||
(request.path.includes('internal') ? 'internal' : 'public') |
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.
Um, I thought I had changed the string match to startsWith
. Not sure where that commit got lost.
It's an interesting approach and may even be more optimal, depending on how the rest of the work progresses. |
💚 Build Succeeded
Metrics [docs]Public APIs missing comments
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: |
@jloleysens I've addressed your comments. Could you TAL please? |
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.
Good work @TinaHeiligers! I left a couple comments based on your responses. Otherwise this looks good to me.
consumers who don't use CoreKibanaRequest.from
That's a good point. I hadn't considered the instances of fake raw requests or wrapping raw requests (mainly like what happens in security plugin) outside the context of our Router.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
We can be even stricter and add a trailing slash /internal/
or use RegExp: new RegExp('^(\/internal$|\/internal\/)')
However, a route that starts with /internal...
will almost certainly be intended for access: internal
unless we ever have a route /internalize
or something 😅
* - '/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 comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏻 . WDYT about moving your type helper to the type utilities package?
It's not fully tested and will have to be if we move that to a general type-utils package. Once we need to use it more widely, we can put the work in to move that. |
…public or internal (elastic#152404)
Summary
Fix #152269
Fix #152276
Part of #151940
The current naming convention used to indicate if an API is intended for internal or public use has proven to be insufficient. This limits what we can change in internal APIs, since we risk breaking external integrations. As such, we want to formalize the distinction, both to clarify an API's intended use and reduce the number of public APIs we have to maintain for a long time.
This PR adds an optional
access
option to Core'sRouteConfigOptions
that allows consumers to mark their API's intended consumption.ATM, the option is only a flag, but we intend to develop the formalization further at some later stage.
If
access
is not declared, core's HTTP service infers it from a route path:/internal/my-foo
is taken to beaccess: 'internal'
, whereas any other path string is interpreted as 'public'.This PR also makes the
access
parameter required forVersioned
routers.For maintainers
Note: the new config option is optional in the core implementation specifically to avoid breaking changes.