PKCE support with oauthlib 3 #678
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…th-toolkit into PKCE-support-with-oauthlib-3
Pull Request Test Coverage Report for Build 1080
💛 - Coveralls |
Closed
|
could anyone check why the build is failing? |
|
Seems to be because of my attempt to support oauthlib 3 unofficially. |
|
I'll fix it once I get a hold of my pc |
|
ping me after making travis green. |
|
https://github.com/oauthlib/oauthlib/releases seems 3.0.1 is the latest |
|
Any progress? Would love to use this in our new app! |
|
Same here. We're anxiously waiting for this to be able to supply our vendors with the ability to use PKCE. |
auvipy
reviewed
Apr 4, 2019
|
@auvipy Sorry it took so long. Fixed the migration and pulled from master. Should be ready to merge now. |
auvipy
approved these changes
Apr 6, 2019
|
really great work! thanks for tackling this and reviewers for reviewing. |
|
Thanks guys! Awesome work :-) 👍 |
| "code_challenge_method": code_challenge_method, | ||
| } | ||
|
|
||
| response = self.client.post(reverse("oauth2_provider:authorize"), data=authcode_data) |
There was a problem hiding this comment.
I had a quick question about this. Shouldn't this be a GET request? I get csrf errors when I send POST. I have been trying to get PKCE to run against master but couldn't get it to work. I would appreciate any pointers no how to get it working.
Edit: many tests fail when I change it to get. I would love to work on this unless it would be a faster fix for OP.
|
Yes, GET and optionally POST.
https://tools.ietf.org/html/rfc6749#section-3.1
…On Fri, Apr 19, 2019 at 4:58 AM Abhishek Patel ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tests/test_authorization_code.py
<#678 (comment)>
:
> + """
+ Helper method to retrieve a valid authorization code using pkce
+ """
+ oauth2_settings.PKCE_REQUIRED = True
+ authcode_data = {
+ "client_id": self.application.client_id,
+ "state": "random_state_string",
+ "scope": "read write",
+ "redirect_uri": "http://example.org",
+ "response_type": "code",
+ "allow": True,
+ "code_challenge": code_challenge,
+ "code_challenge_method": code_challenge_method,
+ }
+
+ response = self.client.post(reverse("oauth2_provider:authorize"), data=authcode_data)
I had a quick question about this. Shouldn't this be a GET request? I have
been trying to get pkce to run against master but couldn't get it to work.
I would appreciate any pointers no how to get it working.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#678 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABBHS52XI5JMSTPN3NGDN7DPRGCU7ANCNFSM4GKUGSNA>
.
|
|
Here are the two issues I ran into:
1. GET requests fail, both in tests and actual endpoint usage.
2. POST fails due to csrf tokens missing, and I think tests should have
checked for it. Should the tests be checking for this or is it entirely
intentional to skip that?
I think having POST requests will be more secure in that browsers won't
store our code challenges in history, however removing CSRF protection
might introduce bigger issues.
However the spec says GET should work too. So can I open an issue to have
PXCE working on GET requests; and work (get assigned) on it?
On Fri, Apr 19, 2019, 4:49 AM Alan Crosswell <notifications@github.com
wrote:
… Yes, GET and optionally POST.
https://tools.ietf.org/html/rfc6749#section-3.1
On Fri, Apr 19, 2019 at 4:58 AM Abhishek Patel ***@***.***>
wrote:
> ***@***.**** commented on this pull request.
> ------------------------------
>
> In tests/test_authorization_code.py
> <
#678 (comment)
>
> :
>
> > + """
> + Helper method to retrieve a valid authorization code using pkce
> + """
> + oauth2_settings.PKCE_REQUIRED = True
> + authcode_data = {
> + "client_id": self.application.client_id,
> + "state": "random_state_string",
> + "scope": "read write",
> + "redirect_uri": "http://example.org",
> + "response_type": "code",
> + "allow": True,
> + "code_challenge": code_challenge,
> + "code_challenge_method": code_challenge_method,
> + }
> +
> + response = self.client.post(reverse("oauth2_provider:authorize"),
data=authcode_data)
>
> I had a quick question about this. Shouldn't this be a GET request? I
have
> been trying to get pkce to run against master but couldn't get it to
work.
> I would appreciate any pointers no how to get it working.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <
#678 (review)
>,
> or mute the thread
> <
https://github.com/notifications/unsubscribe-auth/ABBHS52XI5JMSTPN3NGDN7DPRGCU7ANCNFSM4GKUGSNA
>
> .
>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#678 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABKEVQNNI2LUVDJKXX4KXNDPRGWVZANCNFSM4GKUGSNA>
.
|
|
This is odd. This pull request does not change anything related to how the requests are handled. It only adds new fields to the existing ones. Would you mind sharing how you are making the requests? |
|
I simply enter the prepared url in browser. Although, the app is
registered as public app and pkce setting is set to true.
I can provide a more thorough way to recreate it by end of today or
tomorrow. I'll also double check to make sure I'm not messing something on
my side.
…On Fri, Apr 19, 2019, 9:06 AM Gustavo Maronato ***@***.*** wrote:
This is odd. This pull request does not change anything related to how the
requests are handled. It only adds new fields to the existing ones. Would
you mind sharing how you are making the requests?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#678 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABKEVQPYFRLH3W34U4ZMFH3PRHUZZANCNFSM4GKUGSNA>
.
|
|
I just tested it and you are correct. I was under the impression that oauthlib would capture PKCE credentials, but it doesn't. I've submitted a PR fixing the issue: #707 |
|
Awesome! That was quick!
…On Fri, Apr 19, 2019, 10:19 AM Gustavo Maronato ***@***.*** wrote:
I just tested it and you are correct. I was under the impression that
oauthlib would capture PKCE credentials, but it doesn't. I've submitted a
PR fixing the issue: #707
<#707>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#678 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABKEVQOWW2SETCB3EY2UTV3PRH5JXANCNFSM4GKUGSNA>
.
|
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR modifies #656's implementation to use oauthlib's built-in support for PKCE.
Since this version of oauthlib is based on version 3, I had to modify some test cases to comply with the changes.
I also modified the tox and setup files to use the new version of oauthlib.
All modifications to the base test cases and tox/setup files are contained in a491bb1.