grktsh / falcon-oas

Design first approach with OpenAPI 3 for Falcon
Apache License 2.0
7 stars 4 forks source link

Allow specifying access control handlers via OAS factory and middleware #25

Closed sisp closed 4 years ago

sisp commented 4 years ago

Currently, it is only possible to specify an access control handler via the x-falcon-oas-implementation property in the OpenAPI specification. Then, the access control handler is dynamically imported when the OAS middleware is instantiated.

https://github.com/grktsh/falcon-oas/blob/fc135f72d27b4eba32b1b80d0486d5fb474a5ddc/src/falcon_oas/middlewares.py#L103-L104

This pattern makes an access control handler not configurable, e.g. providing credentials for a database with user credentials is only possible using global variables and/or environment variables, which is a bad design pattern. Instead, it should be possible to provide an access control handler to the OAS middleware class, e.g.:

mw = Middleware(..., security_handlers={'api_key': <handler function>, ...})

The new security_handlers argument could be optional and its value (dict) could be merged with the parsed handlers from the OpenAPI specification to continue support for the x-falcon-oas-implementation property and make the change backwards compatible. Perhaps the schemes provided in the argument could take precedence over the ones specified in the OpenAPI specification or an exception is raised when the same security scheme is specified in both places.

Since the Middleware class is typically not manually instantiated, the OAS class constructor should provide this argument as well and pass it through to the Middleware instantiation:

api = OAS(..., security_handlers={'api_key': <handler function>, ...}).create_api()

There is a similar problem with using x-falcon-oas-implementation for specifying request handlers, but it is solved by omitting x-falcon-oas-implementation in the endpoint specifications of the OpenAPI specification file and instead creating the router manually, which allows to instantiate resource classes manually and inject dependencies as needed.

I know that using a custom property lke x-falcon-oas-implementation or operationId for linking an endpoint to a request handler is quite popular (e.g. Connexion uses it too), but it facilitates bad design like using global/environment variables for configuration instead of dependency injection.

grktsh commented 4 years ago

How about mutating spec_dict to add x-falcon-oas-implementation key and the handler value before passing it to the factory? In this case it is necessary to add the guard clause (maybe if not isinstance(name, six.string_types): return name) to import_string function but this approach is simple and also applicable to the request handlers.

sisp commented 4 years ago

Do you mean something like this?

from falcon_oas import extensions

def api_key_validator(value, scopes, request):
    # ...

spec_dict = load_spec(...)
spec_dict['components']['securitySchemes']['api_key'][extensions.IMPLEMENTATION] = api_key_validator
spec_dict['paths']['/some/path'][extensions.IMPLEMENTATION] = SomeResource(...)

api = OAS(spec_dict).create_api()

# ...

As you said, then import_string would return name as is when it is not a string.

sisp commented 4 years ago

How about adding helpers to make it a bit more convenient?

set_security(spec_dict, 'api_key', api_key_validator)
set_resource(spec_dict, '/some/path', SomeResource())
grktsh commented 4 years ago

Yes, but SomeResource(...) is lambda: SomeResource(...) since it is instantiated in the factory.

I agree to add helpers to avoid extensions.IMPLEMENTATION. I'd like to use OpenAPI terms. For example:

resolve_security_scheme(spec_dict, 'api_key', api_key_validator)
resolve_path_item(spec_dict, '/some/path', lambda: SomeResource(...))
sisp commented 4 years ago

I like it! I'll close #26 in favor of a new PR, that implements your suggestion, later today.

sisp commented 4 years ago

Yes, but SomeResource(...) is lambda: SomeResource(...) since it is instantiated in the factory.

Good point. In fact, this can be hidden in resolve_path_item:

def resolve_path_item(spec_dict, path, resource):
    spec_dict['paths'][path][extensions.IMPLEMENTATION] = lambda: resource

resolve_path_item(spec_dict, '/some/path', SomeResource(...))

I think it's a bit more intuitive without the explicit lambda.

sisp commented 4 years ago

One more thing: Which option would you prefer?

  1. Make the two helpers independent functions that modify the spec_dict explicitly (as discussed above).
  2. Make the two helpers instance methods of the OAS class to hide the implementation detail that spec_dict (or rather self.spec then) is mutated.
    oas = OAS(spec_dict)
    oas.resolve_security_scheme('api_key', api_key_validator)
    oas.resolve_path_item('/some/path', SomeResource(...))
    api = oas.create_api()

    If you're concerned about losing the one-liner api = OAS(spec_dict).create_api(), the two methods could return self allowing:

    api = OAS(spec_dict)
       .resolve_security_scheme('api_key', api_key_validator)
       .resolve_path_item('/some/path', SomeResource(...))
       .create_api()

I tend to prefer 2. because it feels more like a real feature to me rather than an officially supported "trick".

grktsh commented 4 years ago

Either is fine. If 2, I'd like to avoid mutating the given spec_dict and mutate Spec.data directly or by Spec.__getitem__. I don't worry about the one-liner since I actually use multiple middlewares:

oas = OAS(spec_dict, ...)
app = oas.create_api(middleware=[..., oas.middleware, ...])