Build license-checking into the New Platform #56926
Comments
cjcenizal
added
Team:Core
|
Pinging @elastic/kibana-platform (Team:Platform) |
|
This is the idea I had in mind for supporting middleware. In my opinion, it would be the most flexible way to extend the current router and not have to worry about x-pack code (all x-pack plugin should have "licensing" as dependencies under their const licenseCheck = licenseService.getLicenseCheck({ pluginId: 'myApp', minimumLicenseType: 'basic' });
// method could be "*" to match all
router.pre({ method: 'get', path: 'my-app/*' }, [licenseCheck]); |
|
Simplifying license consumption and behavior is great idea. I think we should make this easier for x-pack plugins. Just one less thing developers should need to worry about! Have some thoughts to dump here: With the size of the Kibana codebase, I worry that allowing plugins to register global middleware is going to cause significant problems. The only case we've allowed so far is allowing a plugin to register an authentication mechanism (for Security) which makes sense from a global behavior perspective. What I would prefer is an opt-in mechanism that plugins can utilize. The main reason I prefer this is that it enables gradual or piece-meal adoption by plugins. This avoids problems where we have to make this middleware work with all plugins and their specific needs. It also makes changing this mechanism a bit simpler. That said, @sebelga's approach does fit that bill if the middleware is isolated to a single plugin's Router. However, I would like to propose a different approach which does not require any changes to Core's Router, which is to use a wrapper around IRouter: class Plugin {
setup(core, plugins) {
// Licensing plugin provides a wrapper for Http's IRouter
const licensedRouter = plugins.licensing.createLicensedRouter(
core.http.createRouter()
);
licenseRouter.get(
{
path: '/my-route',
validate: /** schema */,
// The types for this wrapper could require a license level
// in the route config
licenseRequired: 'gold'
},
async (context, req, res) => { /** normal route body */ }
)
}
}Benefits of this approach:
|
|
I like Josh's proposal. We've taken a similar approach with the Security and Spaces plugins to date, by creating a less flexible "licensed route handler" which encapsulates the logic for our routes. It still requires us to call the factory function though, we we have to remember to do: |
|
@joshdover what if the licensing plugin provides function withLicense(level: LicenseType){...}
licenseRouter.get(
{
path: '/my-route',
validate: ...
},
withLicense('gold')(async (context, req, res) => { ... })
)
}that would allow us to compose behaviour via HOF. |
|
I think that would work too, and maybe we should do both, but one advantage the IRouter wrapper is that you can require all routes in the plugin to specify a license level. Or you could even have an option to use the same default license level for all routes in the plugin, and just override the level for some key routes. |
I'd need more context to understand the problem. But yes, my idea is that a plugin gets an isolate "slice" of the router, and its middleware does not affect other plugin. The solution that you propose does not solve the main problem that I'd like to address: define one or multiple middleware against a URL pattern (regEx). We are now trying to find a solution for "license". In the past, you had to find a solution for "security", maybe tomorrow we need a solution to intercept "metrics" and do something with them. From the core router perspective, it is just an array of functions to be executed sequentially until an error is thrown or As for the convention on how to extend, I like your suggestion as we can nest the extensions class Plugin {
setup(core, plugins) {
// Licensing plugin provides a wrapper for Http's IRouter
const router = plugins.licensing.createRouter(
plugins.metrics.createRouter(core.http.createRouter()
);
}We just need to make sure that each plugins that extends the router, export an interface that defines what needs to be provided to each path definition. import { IRouter, CoreRouterPathConfig } from 'src/core/server';
import { LicenseRouterPathConfig } from '../licensing';
...
const router: RouterI<CoreRouterPathConfig & LicenseRouterPathConfig> = plugins.licensing.createRouter(core.http.createRouter());
licenseRouter.get(
{
path: '/my-route',
validate: /** schema */,
// The types for this wrapper could require a license level in the route config
{
licensing: { // namespace to avoid collision
licenseRequired: 'gold'
}
}
},
async (context, req, res) => { /** normal route body */ }
)
|
That sounds like re-inventing the middlewares. Would adding a router and a route handler middlewares solve this problem in a more composable way? The only restriction that I'd like to unforce: middlewares cannot share the state. If a plugin needs it, it can implement this with private fields emulation via WeakMap. const router = createRouter({middlewares: [withLicense('gold')]});
router.get(....{middlewares: [metrics]}, (context, req, res) => ..) |
We don't have to re-invent them, just allow them in our router 😊 I am trying to understand why we can't have them and why we don't use a known pattern with
Are we trying to avoid consumers to override/extend the |
I'm not sure we've explicitly written this down anywhere, so I'm going just going to braindump my opinions against middleware:
|
|
Thanks for detailing your thoughts @joshdover. Let's forget about the word
We currently can't declare a guard against So the current solution is wrapping handlers for all routes we have, like this: export function registerRoute({ router, license, someOtherPlugin, lib }: RouteDependencies) {
router.post(
{ path: addBasePath('/indices/open'), validate: { body: bodySchema } },
someOtherPlugin.doSomething(
license.guardApiRoute(async (ctx, req, res) => {
const body = req.body;
// ...
})
)
);
}So yes it is clear, on each route, what is going on. But this seems very verbose to me and we can easily forget to add the license check.
I would not go that path either, route guards are just that, route guards. In the above example
I hear you. Although if the stack traces show me the error under
I hear you, I guess this is the verbosity and error-prone tradeoff I mentioned.
If we limit the discussion to route "guards" then there is no problem. It is just reordering the guards in the array. So it would be In I think you got my points and I got yours. I let you decide what's best for Kibana 😊 |
|
@elastic/kibana-core #95973 adds the |
|
@cjcenizal Contributions are always welcome and I see no reason we can't include some version of this in the licensing plugins directly. @mshustov since you reviewed it in detail already, are there any significant changes you think we should make before moving this into our core licensing plugin? |
|
@joshdover yes, there are a few:
|
|
This looks like the license API guard approach solved this issue already, I will go ahead and close it |
As part of his migration of the Index Management plugin, @sebelga has created a License class that we configure and use within every API route. It seems like it'd be convenient if we could just configure a license level for each route definition, or even better define it for the base path of the API itself so it applies to all routes built off of it.
Other benefits:
In a brief conversation with @joshdover, we came up with a few different ideas on how this could be implemented:
httpservice that provides this option.IRouterwithThe text was updated successfully, but these errors were encountered: