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

Revert "getting_started.rst: add JSONOAuthLibCore as part of tutorial" #817

Merged
merged 2 commits into from
Mar 20, 2020

Conversation

n2ygk
Copy link
Member

@n2ygk n2ygk commented Mar 20, 2020

Reverts #734 per #807

@n2ygk n2ygk added the docs label Mar 20, 2020
@n2ygk n2ygk added this to the 1.3.1 milestone Mar 20, 2020
@n2ygk n2ygk requested a review from auvipy March 20, 2020 13:16
@n2ygk
Copy link
Member Author

n2ygk commented Mar 20, 2020

@auvipy see reason why I reverted this at #807 (comment)

@coveralls
Copy link

coveralls commented Mar 20, 2020

Pull Request Test Coverage Report for Build 1340

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.768%

Totals Coverage Status
Change from base Build 1335: 0.0%
Covered Lines: 1268
Relevant Lines: 1338

💛 - Coveralls

1 similar comment
@coveralls
Copy link

coveralls commented Mar 20, 2020

Pull Request Test Coverage Report for Build 1340

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.768%

Totals Coverage Status
Change from base Build 1335: 0.0%
Covered Lines: 1268
Relevant Lines: 1338

💛 - Coveralls

@@ -114,10 +112,6 @@ Also add the following to your `settings.py` module:
)
}

`OAUTH2_PROVIDER` setting parameter sets the backend class that is used to parse OAuth2 requests.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@n2ygk I would leave this portion of the PR in to let other devs know that it is an option to use JSONOAuthLibCore. As I mentioned in my original PR, the Angular HTTP library wasn't set up for using the correct content-type as per the OAuth RFC you linked.

Now that I think about it, maybe we should leave this part in and add on to it:

you can also try to configure your HTTP requests to use the RFC-mandated header for OAuth tokens: Content-Type: application/x-www-form-urlencoded

What are your thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you cite a reference to using JSON-encoded data for the OAuth2 token POST being even allowed? All references I find seem to require form-encoded. Maybe I'm not reading the RFCs carefully enough?

There are angular libraries that work just fine as an OAuth2 client using POST with form-encoding. See https://github.com/manfredsteyer/angular-oauth2-oidc.

I really don't understand why JSONOAuthLibCore was added 5 years ago in the first place and see no usages of it anywhere in the code other than the test case that was added with it. I think it may have been added erroneously.

A documentation PR to clarify the required use of form-encoding would be appreciated.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you cite a reference to using JSON-encoded data for the OAuth2 token POST being even allowed? All references I find seem to require form-encoded. Maybe I'm not reading the RFCs carefully enough?

I think I didn't read the RFC closely enough (or the documentation on OAuth libraries). I ran into this issue while developing a video course.

I really don't understand why JSONOAuthLibCore was added 5 years ago in the first place and see no usages of it anywhere in the code other than the test case that was added with it. I think it may have been added erroneously.

I'm sure someone else was implementing an OAuth client for production use and skipped over the Content-Type issue too.

A documentation PR to clarify the required use of form-encoding would be appreciated.

I think your comments are that clarification, thanks for explaining how it all works and referencing the RFC too

@auvipy auvipy merged commit 7756901 into master Mar 20, 2020
@auvipy
Copy link
Contributor

auvipy commented Mar 20, 2020

thanks for finding out this!

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

Successfully merging this pull request may close these issues.

None yet

3 participants