search

gravitee-io / issues

Gravitee.io - API Platform - Issues
65 stars 26 forks source link

Provide better and more strict(er) support for OpenAPI Specification (OAS) based API definitions #1561

Open ate opened 6 years ago

ate commented 6 years ago

Problem

While Gravitee can create/import an API based on an OAS document, the resulting definition is hardly useful or practical, yet.

The most critical difference (mismatch) between OAS and Gravitee APIs is how path mapping and matching is handled, and method restrictions are enforced.

OAS paths are expected to be matched exactly, and only those, while Gravitee (see: ApiPathResolver#resolve(String)) uses prefix matching.

Furthermore, OAS definitions are strict in the supported methods for a path, while Gravitee (API) policy resolving AFAICT is not capable of doing this. You could write a custom policy which (must be) active for all methods and then configure it to (then) only allow a subset thereof. But that's pretty awkward, and requires awkward custom configuration for each and every path.

There is currently another way to enforce these restrictions in Gravitee by using the ResourceFilteringPolicy on a Plan (whitelisted paths), but that also is cumbersome and error prone because it uses a completely separate definition/configuration context, and thus is not part of the API paths definitions where IMO it belongs functionally. And I think the Plan path restrictions (should) have a different purpose anyway.

BTW: another annoyance (bug?) of the ResourceFilteringPolicy configuration is that AFAICT it requires every path to be prefixed with the current API contextpath to actually work as expected because it matches on request.path instead of request.pathInfo ...

Possible Solution

A possible solution without much impact or change to the existing model, APIs and behaviour might be something like the following:

  1. Add an optional bolean prefix_matching property for an API definition, default: true, representing the current path matching logic. By setting this property this to false, a different exact path matching algorithm (still allowing path parameter variables of course) can be implemented in ApiPathResolver#resolve(String)

  2. Add an optional Set<HttpMethod> restricted_methods property to Path, which when not empty only allows a request method matching one of these restricted methods. If none match, the request is denied.

  3. The Portal UI should be improved to support these changes and ideally be smart enough to auto deselect available methods when adding a new policy to a path...

  4. When importing an OAS document, a root path should not be added (as is done now, automatically) unless defined within the OAS document.

Additional Improvements

Last but not least, one other improvement would greatly help as well: automatically adding an Oauth2Policy configuration for OAS based paths with an (type: oauth2) Security Requirements Object.

That also assumes and requires defining an OAuth2 Resource, which definition is not fully derivable from an OAS document (unless Gravitee starts supporting OAS Extensions. But the name for the OAuth2 Resource can already be derived from the Security Requirements Object, so a valid OAuth2Policy configuration can be defined. The developer thereafter can (must) add and configure the needed named OAuth2 Resource manually.

brasseld commented 6 years ago

Hi @ate

Thanks for this interesting feedback. I will try to respond to all the points.

OAS paths are expected to be matched exactly, and only those, while Gravitee (see: ApiPathResolver#resolve(String)) uses prefix matching.

I think we should not compare the way to resolve paths between OAS and Gravitee because the target is not exactly the same. In OAS you're defining path to the services while, within Gravitee.io, you're resolving path to load policies to apply.

You must consider that, initially, Gravitee.io was developed to act as a simple HTTP proxy / API Gateway between consumers and backend services. This may be interesting in case where you don't have Swagger definition (because you're exposing WS for example) or because you don't need a fine control on invoked paths or, sadly, developers are not maintaining their Swagger...

Furthermore, OAS definitions are strict in the supported methods for a path, while Gravitee (API) policy resolving AFAICT is not capable of doing this. You could write a custom policy which (must be) active for all methods and then configure it to (then) only allow a subset thereof. But that's pretty awkward, and requires awkward custom configuration for each and every path.

I'm not sure that providing a new policy for such stuff would be recommended because, as you said, it may be tricky to maintain it.

Also, I would like to make a clear distinction between this custom policy and the existing resource-filtering policy:

An API is, by default, exposed to all the consumers, with no resource restriction (per path / method) applied by consumers. This is the main purpose of the resource-filtering policy: allowing only a subset of the resources exposed by the backend services.

In the case of OAS, it's not exactly the same and, I would say that both can be complementary. I mean, we can restrict paths to those defined from the OAS definition yes, but how to allow only a sub-set of these paths, because you would like to create API's plans with such possibility ?

BTW: another annoyance (bug?) of the ResourceFilteringPolicy configuration is that AFAICT it requires every path to be prefixed with the current API contextpath to actually work as expected because it matches on request.path instead of request.pathInfo ...

Yes, unfortunately... and for historical reasons... that's really a pain... The best would be to provide a new version of the policy... but still don't know how to maintain compatibility.... we have to think seriously about this...

Possible solution IMHO, the best solution would be to let the gateway doing its stuff as it does today. Now, to manage paths (and methods) restrictions according to an OAS definition, the solution would be as follow: 1) Add a new boolean property somewhere in the API to configure if paths / methods must follow OAS definition 2) Develop in the gateway a new OAS processor which will take place between the execution of plan's policies and the api's policies.

From my POV, the main drawback of this solution is that we are no more relying on policy. Currently, I'm not sure it is really a drawback or not. The other point is that, by implementing things this way, a strong dependency will be added between gravitee gateway and swagger libraries, to be able to parse swagger descriptors and apply restrictions.

Then the question arose as to who should bear the responsibility of applying such restrictions. Is it the responsibility of the management-api to extract paths from Swagger and put them in a policy to let the gateway allowing or refusing HTTP requests. OR should we have to put the swagger to the gateway, and use swagger library to look for restrictions ?

I would prefer to give this responsibility to he management-api because, IMHO, the gateway is still a simple HTTP proxy...

ate commented 6 years ago

Hi @brasseld

Thanks for your thoughts on this.

I think we are aligned with your last remark with preferring to give this responsibility to the management-api. IMO that is what my proposed solution is about.

I'm confused though about your comments in between. A new OAS processor sounds like moving the responsibility into the gateway instead, including loading/parsing OAS documents etc. Which I think indeed will change (enhance) the gateway from proxy into something much more specific.

I don't think that is needed and likely a much more invasive effort. If/when that becomes a choice, then maybe even consider adopting or adapting the Vert.x OpenAPI 3 support directly...

But my proposed solution shouldn't change the characteristic of the gateway being a simple HTTP proxy. The optional strict path matching (not reserved to or be OAS specific) IMO is just another way of path mapping, just as is the optional restricted methods configuration. I don't see that fundamentally different from the capability today using the Plan path whitelistings, just a much more clean and manageable way to do it.

The only really new capability is denying non-configured methods on a path, not configured though a policy. Which IMO is a fundamentally missing capability anyway, if you don't want to use an awkward custom policy 'hack' as I described above.