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

Fix builtin Swagger-UI OAuth login #1142

Merged
merged 1 commit into from
Mar 18, 2024

Conversation

ftsell
Copy link
Contributor

@ftsell ftsell commented Jan 14, 2024

This PR intends to fix an issue when using the OAuth authentication mechanism built into Swagger-UI.

When using this mechanism, Swagger-UI authenticates against an OAuth provider on its own and then uses the retrieved access tokens for making requests to the django api. However this does not always work because Swagger-UI implements its authentication using two tabs and then accessing the initiating one via the window.opener property. This property can be null under some circumstances. One of them is that the tab where the main Swagger-UI is open, sets a Cross-Origin-Opener-Policy to a restrictive value. Django does the right thing (generally speaking) and sets it to such a restrictive value (see django docs).
This leads to errors as described in swagger-api/swagger-ui#8030.

This PR overwrites the required header for the Swagger-UI view only so that Swagger-UI can perform its authentication as intended.


Side note: Choosing the value same-origin-allow-popups as COOP also works depending on the OAuth provider that someone is using. Since we cannot ensure that a compatible OAuth provider is used, I chose to go for increased compatibility and a little less security here since Swagger-UI is mainly used for dev setups and documentation anyways.

Copy link

codecov bot commented Jan 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (aeca119) 98.63% compared to head (d3eb263) 98.63%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1142   +/-   ##
=======================================
  Coverage   98.63%   98.63%           
=======================================
  Files          71       71           
  Lines        8711     8716    +5     
=======================================
+ Hits         8592     8597    +5     
  Misses        119      119           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This is required because Swagger-UI uses `window.opener` references
to communicate to itself when doing OAuth authentication.
Djangos default COOP however blocks these references so that swagger
cannot correctly pass its authentication state between windows.
@ftsell ftsell force-pushed the fix/oauth_login_window_opener branch from b578ae4 to d3eb263 Compare January 14, 2024 19:14
@ftsell
Copy link
Contributor Author

ftsell commented Mar 6, 2024

Hey @tfranzel 👋
Do you have open questions or is there another reason you haven't commented on this yet?

@tfranzel
Copy link
Owner

tfranzel commented Mar 7, 2024

@ftsell sry it took a while. I had not much time lately and I'm cautious of merging stuff I don't completely understand, especially security related stuff. The devil is in the details as you certainly know. Starting looking at this today, will update you.

@tfranzel
Copy link
Owner

Sry this took a while. Yes, this is the right thing to do given that django's default is strict and the oauth2 login mechanism of swaggerui (via popup) must somehow hand back the token that it received from the auth server for usage in swaggerui.

As I see it, there is no way around this relaxation if someone wants to use oauth2 with a different origin auth server.

@tfranzel tfranzel merged commit 06d3b47 into tfranzel:master Mar 18, 2024
35 checks passed
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.

None yet

2 participants