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

Added code verifier as a parameter. #18

Merged
merged 7 commits into from
Nov 6, 2020
Merged

Conversation

mooreds
Copy link
Contributor

@mooreds mooreds commented Jun 9, 2020

This allows using PKCE code verifier with the client libs.

Fixes issue #17 .

I tested building the java and php client libs, and things looked ok. It compiled.

Wasn't sure quite how to make it optional, looked through https://github.com/inversoft/client-library-plugin/ but didn't see it.

This allows using PKCE with the client libs.
@robotdan
Copy link
Member

robotdan commented Jun 9, 2020

For typed languages, this will be a compile break. That may be ok, it shouldn't be too hard to fix.
One option is to make a new method for use with PKCE to make it more explicit and to not break the existing client method.

I could go either way. In general I would prefer not to break compile if we don't have to, in this case it may not be too bad to just add another method.

exchangeOAuthCodeForAccessTokenUsingPKCE or something like that?

@mooreds
Copy link
Contributor Author

mooreds commented Jun 9, 2020

That makes sense. I'll add another json file with that name and the new params.

@mooreds mooreds requested a review from robotdan June 10, 2020 15:44
@mooreds
Copy link
Contributor Author

mooreds commented Nov 5, 2020

@robotdan can I get a quick review on this? It's blocking @amans330 's PKCE work.

Copy link
Member

@robotdan robotdan left a comment

Choose a reason for hiding this comment

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

Wondering if we want to use PKCE in the method name or not? We could also name it exchangeOAuthCodeForAccessTokenWithCodeVerifier would this be confusing?

"If you will be using the Authorization Code grant, you will make a request to the Token endpoint to exchange the authorization code returned from the Authorize endpoint for an access token."
],
"method": "post",
"methodName": "exchangeOAuthCodeForAccessToken",
Copy link
Member

Choose a reason for hiding this comment

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

Generally, this is the same as the file name, or is this intentionally overloading the method w/ an additional parameter?

If so, that is ok, but we will need to ensure it doesn't break other client libs. Not all of them have the same naming rules that Java has, so you'll want to run a build of all client libs with this and see if they tolerate this or if they fail indicating they have duplicate signatures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Happy to go with convention and avoid overloading issues.

Comment on lines 4 to 5
"Exchanges an OAuth authorization code for an access token.",
"If you will be using the Authorization Code grant, you will make a request to the Token endpoint to exchange the authorization code returned from the Authorize endpoint for an access token."
Copy link
Member

Choose a reason for hiding this comment

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

Should we update this to indicate this version of the API takes the code_verifier for PKCE?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, good catch.

{
"name": "code_verifier",
"comments": [
"The random string you generated previously if you are using PKCE. Will be compared with the code_challenge you sent previously, which allows the OAuth provider to authenticate your app."
Copy link
Member

Choose a reason for hiding this comment

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

The generated value used to build the code_challenge sent on the Authorization request.
This value will be used to produce a code_challenge that will then be compared to the value sent on the authorization request for equality.

This may need to be split up into separate lines to ensure formatting comes out ok.

Copy link
Member

Choose a reason for hiding this comment

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

Your description is actually fine... but the "if you are using PKCE" threw me. Not sure why you would use this method if you aren't using PKCE? Unless we want to eventually deprecate the other method and use this one regardless of if they user is making the request with PKCE.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed 'if you are using pkce'

@robotdan
Copy link
Member

robotdan commented Nov 5, 2020

Added comments.

@mooreds mooreds requested a review from a team as a code owner November 5, 2020 21:51
@mooreds mooreds requested a review from robotdan November 5, 2020 21:58
"uri": "/oauth2/token",
"comments": [
"Exchanges an OAuth authorization code for an access token.",
"If using the Authorization Code grant, you will make a request to the Token endpoint to exchange the authorization code returned from the Authorize endpoint and a code_verifier for an access token."
Copy link
Member

Choose a reason for hiding this comment

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

This was already here, but perhaps should be re-written, not sure what it means. We are using the authorization code grant, otherwise we wouldn't be using this method.

Copy link
Member

Choose a reason for hiding this comment

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

Or if you want to leave that is fine - I can re-word some of this stuff later, I don't want to hold you up too long fixing my technical debt. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Was trying to keep this in line with the other exchange method, defined in exchangeOAuthCodeForAccessToken.json but will rewrite both to make it clearer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, wasn't hard to change the verbiage a bit.

No need to mention the authorization code grant. If you are using this method, you know you are using this grant.

Also made it clear what the difference is in the first comment between the two similarly named methods.
@mooreds
Copy link
Contributor Author

mooreds commented Nov 6, 2020

@robotdan you comfortable with this as it is now? Would love to get this included in the next point release.

@robotdan robotdan merged commit e1e02e0 into master Nov 6, 2020
@mooreds mooreds deleted the add-code-verifier-parameter branch November 6, 2020 16:28
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.

2 participants