search

elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.63k stars 8.22k forks source link

Build license-checking into the New Platform #56926

Closed cjcenizal closed 4 months ago

cjcenizal commented 4 years ago

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:

elasticmachine commented 4 years ago

Pinging @elastic/kibana-platform (Team:Platform)

sebelga commented 4 years ago

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 kibana.json file).

const licenseCheck = licenseService.getLicenseCheck({ pluginId: 'myApp', minimumLicenseType: 'basic' });

// method could be "*" to match all
router.pre({ method: 'get', path: 'my-app/*' }, [licenseCheck]);
joshdover commented 4 years ago

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:

legrego commented 4 years ago

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:

https://github.com/elastic/kibana/blob/3bdbcd0d1a1a2a44bef3e006c6b596a282ea11c0/x-pack/plugins/security/server/routes/licensed_route_handler.ts#L10-L25

mshustov commented 4 years ago

@joshdover what if the licensing plugin provides withLicense decorator that implements the same logic? like proposed in our example https://github.com/elastic/kibana/blob/master/src/core/MIGRATION_EXAMPLES.md#simple-example

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.

joshdover commented 4 years ago

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.

sebelga commented 4 years ago

With the size of the Kibana codebase, I worry that allowing plugins to register global middleware is going to cause significant problems.

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. Giving flexibility to the consumer to intercept the request and do something with it seems like the most flexible approach.

From the core router perspective, it is just an array of functions to be executed sequentially until an error is thrown or response returns something. With that said, if you prefer not to add this functionality in the core router, we can extend the router ourselves and add it in our es-shared-ui plugin.

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. I would suggest having the configuration inside the namespace of the plugin id to avoid naming collision.

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 */ }
    )
mshustov commented 4 years ago

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.

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. Giving flexibility to the consumer to intercept the request and do something with it seems like the most flexible approach.

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) => ..)
sebelga commented 4 years ago

That sounds like re-inventing the middlewares.

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 .pre( ... ) method instead of inventing our own. But I think I miss some background on pitfalls we want to get away from.

middlewares cannot share the state

Are we trying to avoid consumers to override/extend the context / req / res objects?

joshdover commented 4 years ago

But I think I miss some background on pitfalls we want to get away from.

I'm not sure we've explicitly written this down anywhere, so I'm going just going to braindump my opinions against middleware:

sebelga commented 4 years ago

Thanks for detailing your thoughts @joshdover. Let's forget about the word middleware and think of route guards. It's just naming, but maybe middleware has a negative perception 😊

Introduces another way to accomplish something that is already possible today.

We currently can't declare a guard against /index_managemenet/* API routes. This means that we have to manually add the license check on 25 routes in index management. This also means that we can forget to add the check on a future route. The current solution does not allow us to protect our app as a whole. This is the problem I rose and I proposed the middelware (or guards) path.

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.

A Middleware that requires new route config options cannot be typed enforced automatically. Composition does not have this problem.

I would not go that path either, route guards are just that, route guards. In the above example license.guardApiRoute does not require any additional configuration to the route.

Poor debuggability. Because middleware would be registered on the router and executed in a different call stack from the actual route handler

I hear you. Although if the stack traces show me the error under license.guardApiRoute I think I would know where the error comes from.

Much less explicit. As a developer, I much prefer to have all the information about the context I'm programming in as possible.

I hear you, I guess this is the verbosity and error-prone tradeoff I mentioned.

Middleware ordering can become a problem.

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 router.guard({ path: '/some-path/'}, []) with no notion of request method as I initially added in my example.

In axios terms, what I suggest would be kind of like declaring interceptors on the request (https://github.com/axios/axios#interceptors).

I think you got my points and I got yours. I let you decide what's best for Kibana 😊

cjcenizal commented 3 years ago

@elastic/kibana-core https://github.com/elastic/kibana/pull/95973 adds the license_api_guard plugin, which seems to address this issue. WDYT? If so, perhaps we can close this or open a separate issue to consider merging it into the license plugin.

joshdover commented 3 years ago

@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?

mshustov commented 3 years ago

@joshdover yes, there are a few:

pgayvallet commented 4 months ago

This looks like the license API guard approach solved this issue already, I will go ahead and close it