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

JSON endpoint for auth URL #1305

Open
jhnbyrn opened this issue Aug 11, 2023 · 10 comments · May be fixed by #1306
Open

JSON endpoint for auth URL #1305

jhnbyrn opened this issue Aug 11, 2023 · 10 comments · May be fixed by #1306
Labels

Comments

@jhnbyrn
Copy link
Contributor

jhnbyrn commented Aug 11, 2023

I would like to render the authorization page on the front end rather than via a Django template. The reason is that I want to pull other information into that page that should logically come from other endpoints rather than being added to the template scope.

Anyone doing anything like this currently? I'd prefer not to rewrite the AuthorizationView completely.

I'm willing to work on a PR for this if it would be accepted. How about an optional parameter to that view that makes it handle JSON for GET and POST instead of html? Or should it be a completely new view?

@jhnbyrn
Copy link
Contributor Author

jhnbyrn commented Aug 12, 2023

What I have in mind is something like this: #1306

I still have to add the post handler.

@jhnbyrn
Copy link
Contributor Author

jhnbyrn commented Aug 17, 2023

If someone can let me know if this is approach is likely to be accepted, and/or what I should change, I will work on tests and whatever else is needed to get it up to the necessary standard.

@n2ygk
Copy link
Member

n2ygk commented Aug 17, 2023

@jhnbyrn If I understand what you are looking to do, it seems a common approach to this functionality is to implement a redirect to the authorization site (which can be a different service) which in turn has a callback url. At my shop we only use DOT for the resource server token validation part, having a commercial oauth2 service using redirects to a SAML2 service to initially obtain the access token. I'd love to replace that with DOT through-and-through. I'm wondering if this might be the approach to implement for this use case? That may be a lot more complex that what you are looking for.

Anything to move away from templates toward a REST API approach would be a good thing as far as I'm concerned but I would want to be clear on whether this new feature might add any security exposures. I don't have a good enough understanding myself. Perhaps someone else can comment?

@jhnbyrn
Copy link
Contributor Author

jhnbyrn commented Aug 17, 2023

@n2ygk Yes, I'm using the usual approach of having a redirect to the auth service. The user will get redirected from the client site to my DOT site. But my DOT site is going to have a single page app frontend, and for various reasons I'd like that auth page that clients get redirected to to be rendered by the frontend of my DOT site rather than using a Django template.

FWIW I'm fairly sure there's no security risk with the GET part because I'm really returning exactly the information that is already available in the template context. The POST might be tricker. Django's form handling is probably doing some sanitation to the POSTed data that I might need do in the JSON endpoint. But I'm not sure about this.

@jhnbyrn
Copy link
Contributor Author

jhnbyrn commented Aug 21, 2023

Since there's no interest in this, I could make the json view part of my project instead of DOT, but in that case I still need to split the logic of the existing AuthorizationView GET method into 2 functions - one to get the template context and the other to render it. That way I can reuse the former in a custom view and render it the way I want.

Would a PR that does that be acceptable? It would be basically my existing PR without the new JSON view.

@jhnbyrn
Copy link
Contributor Author

jhnbyrn commented Sep 10, 2023

Having trouble with test coverage on this one. The codecov check is failing because some code I moved (but didn't change) is apparently no longer covered. But when I run tox locally it's showing that code as covered. Has anyone run into anything like this before?

I'm running (locally):

tox -e py311 -- tests/test_application_views.py

and getting

...

oauth2_provider/views/base.py                                148      1    99%   291
...

@n2ygk
Copy link
Member

n2ygk commented Sep 10, 2023

You can run coverage locally and generate an HTML report which highlights what's covered. I've also seen incorrect coverage reports that are not merge blockers....

@dopry
Copy link
Contributor

dopry commented Oct 4, 2023

@jhnbyrn Have you done any research on what the OAuth/OIDC specs say about using pure SPA front ends? IIRC there are some reasons not to do this, mostly revolving around avoiding XSS/code injection risk. Although I'm not sure what the current state of the nation is amongst those leading the security research around this. I'd want to see a strong endorsement from specifiers and maybe similar implementation in keycloak, auth0, okta, etc to verify acceptability before merging something like that into DOT itself.

@jhnbyrn
Copy link
Contributor Author

jhnbyrn commented Oct 5, 2023

@jhnbyrn Have you done any research on what the OAuth/OIDC specs say about using pure SPA front ends? IIRC there are some reasons not to do this, mostly revolving around avoiding XSS/code injection risk. Although I'm not sure what the current state of the nation is amongst those leading the security research around this. I'd want to see a strong endorsement from specifiers and maybe similar implementation in keycloak, auth0, okta, etc to verify acceptability before merging something like that into DOT itself.

I haven't. I don't see how rendering the HTML on the browser side is riskier than rendering it on the server side of the same app. When the DOT authorize view receives a POST from the browser, it has no way to tell whether it was from the button on the page it rendered from the Django template, or a POST that came from some arbitrary JS code. As long as it comes from the same domain and the right info it should be the same. For example, I could write the JS I want now and scrape the HTML that's currently bring returned, and this would achieve exactly the same thing with no changes to DOT. But I'll research it as much as I can based on what you said.

Also, I just want to point out that the latest version of this PR doesn't add the functionality I'm looking for, it just refactors the view slightly so that I can add the functionality I want in a custom view. That custom view doesn't have to be part of DOT. Basically I want to separate the logic of the view from the Django HTML template rendering so that I can reuse the logic but return JSON instead of HTML.

@dopry
Copy link
Contributor

dopry commented Oct 10, 2023

Just because you don't see how something is risky, doesn't mean it isn't. To make me as a maintainer feel comfortable merging something like this, I'd like to see clear examples of large IDPs (google, facebook, etc.) using this approach and some public discussion by security professionals on best practices and risks related to this approach.

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