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

allow for swagger oauth redirect #89

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Conversation

vjm
Copy link

@vjm vjm commented Aug 13, 2019

alllow for swagger to redirect properly for oauth

@vjm
Copy link
Author

vjm commented Aug 13, 2019

@lafrech let me know if you have questions about this -- would love to see this get merged!

@lafrech
Copy link
Member

lafrech commented Aug 13, 2019

Hi. Thanks for your contribution.

Could you please elaborate on the use case? (Let's assume I know very little about OAuth.)

Is the template something you crafted from scratch, or is it borrowed from swagger-ui or elsewhere? I'd rather avoid duplication, so it would be nice if we could just point to an external JS file like we do with swagger-ui already.

@vjm
Copy link
Author

vjm commented Aug 13, 2019

The use case is to use Swagger's built-in oauth features which are currently not possible with the implementation built into this library. Here's some more details on how you can use OAuth in Swagger: https://swagger.io/docs/specification/authentication/oauth2/

The template is borrowed from here: https://github.com/swagger-api/swagger-ui/blob/master/dist/oauth2-redirect.html
I am not sure that it's easily possible to extract this and use it as an external javascript file -- I am open to suggestions though.

Looks like the build failed due to linting, I'll take a look at that.

@lafrech
Copy link
Member

lafrech commented Aug 13, 2019

Looks like there is no easy way not to copy the whole html file.

I'd rather not include a copy of a part of swagger-ui in the code (to minimize maintenance issues, having to update it when it changes).

Until we find a way, you could do that in you app by subclassing DocBlueprintMixin, I suppose.

The only missing piece would be the added line in the swagger-ui template:

    oauth2RedirectUrl: "{{ url_for('api-docs.openapi_swagger_ui_redirect',_external=True) }}",

Maybe we could add an application parameter option for that. In fact, in any case we would, to avoid hardcoding the path.

This, plus a section in the docs explaining how to proceed.

I'm not really familiar with swagger-ui (we use ReDoc). The CDN says

Swagger UI is a dependency-free collection of HTML, JavaScript, and CSS assets that dynamically generate beautiful documentation from a Swagger-compliant API

but there is no HTML in the package.

Maybe we could include the html file. After all, we already have swagger_ui.html. Except this one looks really minimal.

I'm also surprised the whole html file is a JS script. Why didn't they use an external JS script file and import it in the HTML?

@vjm
Copy link
Author

vjm commented Aug 13, 2019

I've updated the PR to remotely pull the template that matches the swagger UI version in order to not have to store the template.

I've also added some documentation -- please take a look and let me know if we can merge this in! Would love to get the new release of this.

@vjm
Copy link
Author

vjm commented Aug 14, 2019

@lafrech let me know what you think and if we can merge this in and release!

@lafrech
Copy link
Member

lafrech commented Aug 14, 2019

Don't hold your breath.

I'd like to finish ongoing work on #46/#83 and release 0.17 first.

I don't think pulling the file from an external GH repo is a nice option either.

Perhaps adding the file to the repo is the way to go. I'd like to avoid having to update it regularly, or to manage compatibility issues with different swagger-ui versions. We should at least check how regularly it is modified upstream and how they deal with backward compatibility.

The point is I want to limit the features of this lib to the minimum, to minimize maintenance load, so I try not to open doors to issues by adding features that can be easily achieved in client code. Especially features I don't use and I am not familiar with.

Maybe we could rework this to conditionally add the missing line in the swagger-ui template and add a note to the docs explaining how to achieve OAuth in client code.

@vjm
Copy link
Author

vjm commented Aug 14, 2019

I don't think pulling the file from an external GH repo is a nice option either.
Perhaps adding the file to the repo is the way to go. I'd like to avoid having to update it regularly, or to manage compatibility issues with different swagger-ui versions. We should at least check how regularly it is modified upstream and how they deal with backward compatibility.

It hasn't been updated since Feb 13, 2018 so I think it's pretty stable.

Maybe we could rework this to conditionally add the missing line in the swagger-ui template and add a not to the docs explaining how to achieve OAuth in client code.

@lafrech I have submitted more updates based on your feedback, please take another look and let me know.

@coveralls
Copy link

coveralls commented Aug 14, 2019

Coverage Status

Coverage decreased (-0.3%) to 99.662% when pulling faaf872 on vjm:master into d3d5885 on Nobatek:master.

@vjm
Copy link
Author

vjm commented Aug 26, 2019

@lafrech any thoughts here?

@vjm
Copy link
Author

vjm commented Sep 16, 2019

@lafrech any thoughts?

@lafrech
Copy link
Member

lafrech commented Apr 22, 2022

Looks like this could be achieved with no change to the code here by passing the swagger oauth2RedirectUrl option.

nelmio/NelmioApiDocBundle#1097 (comment)

No time to check/confirm right now.

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

Successfully merging this pull request may close these issues.

3 participants