-
Notifications
You must be signed in to change notification settings - Fork 335
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
Support a configurable oauth redirect URL #442
base: master
Are you sure you want to change the base?
Support a configurable oauth redirect URL #442
Conversation
Addresses: #438 |
0db7d06
to
4eaf373
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologies, it has taken me SO LONG to get to this. Just one comment from me, just to double-check we aren't introducing a small behaviour change here.
{% if config.SWAGGER_UI_OAUTH_REDIRECT_URL -%} | ||
oauth2RedirectUrl: "{{ config.SWAGGER_UI_OAUTH_REDIRECT_URL }}", | ||
{% else -%} | ||
oauth2RedirectUrl: "{{ url_for('restx_doc.static', filename='oauth2-redirect.html', _external=True) }}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong here, but does this introduce a small behaviour change to previously? By default, config.SWAGGER_UI_OAUTH_CLIENT_ID
is not defined and therefore no oauth2RedirectUrl
should be set? I think we maybe just need to introduce another if statement? I don't have any OAUTH flows set up to easily test this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've put back the original check and then nested the new check inside it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing this.... hold off merging...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK testing done, could you re-review please
Currently an oauth redirect URL is hardcoded as
oauth2-redirect.html
, this works fine for example applications but for production apps you probably want to specify your own URL so you can add logic such as creating a user account etc.This change adds a new config variable called
SWAGGER_UI_OAUTH_REDIRECT_URL
which will tell swagger which URL to use when submitting to the oauth endpoint. This will need to match what has been previously configured with the oauth provider.I've also included a simple github oauth example for reference.
As this is my first PR into this project please let me know if there are any changes or additions I need to make this to comply with the prioject standards.
Resolves: #438