Skip to content
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

Enable security stuff in Open API definition #4

Closed
wants to merge 2 commits into from

Conversation

xalioth
Copy link

@xalioth xalioth commented Aug 1, 2018

Allows to specify a swagger securitySchemes in the top level API doc
Also allows to specify the security scheme for a whole Blueprint at once rather than specifying the security='xxx' for each functions

xalioth added 2 commits August 1, 2018 22:58
It can be used to document the security scheme to use for all
methods of a Blueprint at once.
@coveralls
Copy link

coveralls commented Aug 1, 2018

Coverage Status

Coverage decreased (-0.2%) to 99.371% when pulling 9ecf8b0 on xalioth:enable_security into 318807f on Nobatek:master.

@lafrech
Copy link
Member

lafrech commented Aug 2, 2018

Ideally, the registration of the security definition would be done the same way definitions are registered. See this issue: marshmallow-code/apispec#245. It shouldn't be too much work, but until it is done, or to allow more flexibility, being able to pass a dictionary makes sense.

We could also add a spec_kwargs argument to Api to pass kwargs to the APISpec instance, like it currently does with spec_plugins, for top-level options that don't need to be an application parameter but are hardcoded in the application.

Regarding the security argument to Blueprint, I'd like to think twice before doing this. Do you think it could be done application-wise (excluding the blueprint that serves the spec, of course)? Or would it be too much of a constraint and a blueprint granularity is needed?

And would it make sense to make it more generic, like adding a view_docs parameter to Blueprint (TODO: find a better name), so that any keyword can be specified, not only security:

    def __init__(self, *args, **kwargs):
        self.description = kwargs.pop('description', '')
        self.view_docs = kwargs.pop('view_docs', {})

        [...]
 
        def store_method_docs(method, function):
            [...]
            endpoint_doc[method_l].update(self.view_docs)

@xalioth
Copy link
Author

xalioth commented Aug 2, 2018

Yes at first I wanted to add a decorator to add a security definition, but the APISpec code doesn't seem to allow adding that after initialization, i.e. it has to be passed to the APISpec constructor.

I then also implemented a solution as you suggested (adding arguments to Api constructor), but at the end it was not so clean because the args had to be passed from functions to functions internally, so I prefered the config solution, which is somewhat similar to what you did for passing the versions.

For the blueprint, I agree it could be made more generic, maybe rename view_docs to default_doc or fallback_doc ?

@xalioth
Copy link
Author

xalioth commented Aug 2, 2018

In any case, I am completely fine with another solution as long as I got a way to define my security schemes. I need it to easily test my API directly from swaggerdoc

@lafrech
Copy link
Member

lafrech commented Aug 2, 2018

I then also implemented a solution as you suggested (adding arguments to Api constructor), but at the end it was not so clean because the args had to be passed from functions to functions internally, so I prefered the config solution, which is somewhat similar to what you did for passing the versions.

Both ways have their use case. I think using an app parameter makes sense if it is really something that may be set for a specific application instance (like URL, local path, or various settings). Hardcoded parameters are for stuff that is not meant to be changed when deploying, especially if it is made of complicated python objects you wouldn't put in a config file, or if it needs a processing (for instance, parameters, responses, errors, that you'd like to parse using MarshmallowPlugin) before being passed.

We can have both.

Yes, default_doc seems fine. Let's investigate if we can do it application-wise. And maybe think of an opt-out if an exception is needed (passing {'attribute': None}, for instance, could cancel a default locally). This mechanism, or a similar one, would be useful to specify error responses globally.

I won't be able to merge, let alone release, any of this before one or two weeks. In the meantime, I think you can safely use API_SPEC_OPTIONS with your patch, as ultimately, it should be released.

@xalioth
Copy link
Author

xalioth commented Aug 2, 2018

OK, fine for me, I'm not in a hurry, I have mu own branch. Thanks Jérôme!

@lafrech
Copy link
Member

lafrech commented Sep 20, 2018

Hi @xalioth. I just released a 0.8.0 version implementing API_SPEC_OPTIONS and an equivalent spec_kwargs parameter. Thanks for the suggestion.

I didn't take much time to think about default_doc but feel free to open a dedicated issue / PR if you want to discuss / implement such a feature. I have no objection. Just want to get it right before releasing.

@lafrech lafrech closed this Sep 20, 2018
@lafrech
Copy link
Member

lafrech commented Feb 1, 2019

Also allows to specify the security scheme for a whole Blueprint at once rather than specifying the security='xxx' for each functions

If suppose you're using a decorator to enforce security. Can't you change it to auto-document the security scheme applied to the route? (See #36 for a similar issue.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants