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

Oauth2 redirectUrl works only as an absolute path #457

Closed
Joezeppe opened this issue Jul 14, 2021 · 12 comments
Closed

Oauth2 redirectUrl works only as an absolute path #457

Joezeppe opened this issue Jul 14, 2021 · 12 comments
Labels
enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending

Comments

@Joezeppe
Copy link

When setting the oauth2RedirectUrl as an absolute path in settings like this:

    'SWAGGER_UI_SETTINGS': {
        'oauth2RedirectUrl': 'https://<host>/api/endpoint',
    }

It works fine, but when using a relative path, it doesn't build the absolute url

    'SWAGGER_UI_SETTINGS': {
        'oauth2RedirectUrl': '/api/endpoint',
    }
@Joezeppe
Copy link
Author

This could be a simple fix. Perhaps, the only thing needed is to build the absolute oauth2redirecturl before returning the Response in the get method of SpectacularSwaggerView , something like this:

request.build_absolute_uri(oauth2RedirectUrl)

@tfranzel
Copy link
Owner

i see. oauth2 redirect is only really sensible when it's an absolute URL. At the moment we simply pass through your settings unchanged.

The problem is that this absolute/relative thing has bitten me quite a few times before. This usually breaks half of the userbase because of proxy/translation/deployment details, so it is brittle trying to make URLs absolute. i'm not quite sure how to make everyone happy on this topic.

@Joezeppe
Copy link
Author

It would be easier if a relative path can be given to the oauth2RedirectUrl setting, then it can build the URI dynamically regardless of the host running the application. Otherwise, SpectacularSwaggerView can be customized and that functionality can be added therein.

@tfranzel
Copy link
Owner

yes i understand that it would be easier. this may work for you, but it will not work for all users. the last time we attempted to translate relative to absolute, we broke quite a few users. the reason is that some users rewrite urls with a reverse proxy. Django is unable know how to form a valid absolute URL there.

i saw a solution where the url is constructed client-side with js (e.g. "window.location" + "/foo"). we currently do not support raw settings, but that would be easy to add.

@georgy-komarov
Copy link
Contributor

@Joezeppe
I managed to use relative path by passing JS code to settings JSON string

class OAuthFixedSpectacularSwaggerView(SpectacularSwaggerView):
    @extend_schema(exclude=True)
    def get(self, request, *args, **kwargs):
        """Some dirty hack to change oauth2RedirectUrl with JS"""

        response = super().get(request, *args, **kwargs)
        response_data = response.data

        settings = json.loads(response_data['settings'])
        required_config = '`${window.location.protocol}//${window.location.host}/static/swagger-oauth2-redirect.html`'
        settings['oauth2RedirectUrl'] = required_config
        settings = json.dumps(settings).replace(f'"{required_config}"', required_config)

        response_data['settings'] = settings
        return Response(response_data)

This hack with replace works because template gets settings with safe tag: const swagger_settings = {{settings|safe}}
I also had to put oauth2-redirect.html (v. 3.52.0 for example) to my static folder.

tfranzel added a commit that referenced this issue Aug 24, 2021
@tfranzel
Copy link
Owner

@georgy-komarov thanks for that hint. i see you used my suggestion there with ${window.location.protocol}/. this not supposed to be that cumbersome. i added the possibility to provide the settings a raw JS string. that should make that hack unnessary.

can you test if that works for you now?

I also had to put oauth2-redirect.html (v. 3.52.0 for example) to my static folder.

i have no idea why this file is missing. isn't it supposed to be part of the build release on CDN? it is part of their dist here https://github.com/swagger-api/swagger-ui/tree/master/dist

@tfranzel tfranzel added enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending labels Aug 24, 2021
@georgy-komarov
Copy link
Contributor

can you test if that works for you now?

Yes, it works when I set settings in such a way.

'SWAGGER_UI_SETTINGS':      '''{
        "deepLinking": true, 
        "oauth2RedirectUrl": `${window.location.protocol}//${window.location.host}/api/static/swagger-oauth2-redirect.html`
    }''',

i have no idea why this file is missing. isn't it supposed to be part of the build release on CDN? it is part of their dist here

By default oauth2RedirectUrl points at `${window.location.protocol}//${window.location.host}/oauth2-redirect.html`
It seems it is recommended for oauth2RedirectUrl to have the same host as swagger client. I think the reason may be connected with access policy to local storage (or whatever it uses to share auth data between tabs).

I got inspiration from drf-yasg [1], [2]. They host swagger dist as app's static files.

@georgy-komarov
Copy link
Contributor

I tried to set oauth2RedirectUrl: "https://unpkg.com/swagger-ui-dist@3.52.0/oauth2-redirect.html" but now I get this error in browser's console
image

@tfranzel
Copy link
Owner

Yes, it works when I set settings in such a way.

awesome, that is how i initially intended it.

I tried to set oauth2RedirectUrl: "https://unpkg.com/swagger-ui-dist@3.52.0/oauth2-redirect.html" but now I get this error in browser's console

ahh.. CORS... that makes sense i suppose. however i'm still hesitant to include swagger ui dist files into our source. yasg does host it's own version but it is severely outdated and customized. i always tried to stay away from that tight coupling.

@georgy-komarov
Copy link
Contributor

georgy-komarov commented Aug 25, 2021

I tried to set oauth2RedirectUrl: "https://unpkg.com/swagger-ui-dist@3.52.0/oauth2-redirect.html" but now I get this error in browser's console

ahh.. CORS... that makes sense i suppose. however i'm still hesitant to include swagger ui dist files into our source. yasg does host it's own version but it is severely outdated and customized. i always tried to stay away from that tight coupling.

You don't need to include the whole swagger dist. The only file we need is oauth2-redirect.html.
However this may break SWAGGER_UI_DIST setting in the future or break compatability with older versions. On the other hand, it seems, they don't update oauth2-redirect.html often.

@tfranzel
Copy link
Owner

i'd like to not open that can of worms for the moment and push that further until something comes along that we cannot get around. although not that big of a deal in this case, it breaks the separation we tried to achieve very diligently.

the added fix makes this workable even though convenience is a bit lacking.

i'll close this issue as the initial problem is solved. however, we can revisit this again if there are new cirumstances.

@tfranzel
Copy link
Owner

the sidecar is here: https://github.com/tfranzel/drf-spectacular-sidecar

convenience methods and doc added with aeda969

now oauth2-redirect.html can be served from the same origin, which should make this less cumbersome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending
Projects
None yet
Development

No branches or pull requests

3 participants