Added code verifier as a parameter. #18
Conversation
This allows using PKCE with the client libs.
|
For typed languages, this will be a compile break. That may be ok, it shouldn't be too hard to fix. 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.
|
|
That makes sense. I'll add another json file with that name and the new params. |
This reverts commit c756bcf.
This allows using PKCE with the client libs.
| "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", |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Happy to go with convention and avoid overloading issues.
| "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." |
There was a problem hiding this comment.
Should we update this to indicate this version of the API takes the code_verifier for PKCE?
| { | ||
| "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." |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Removed 'if you are using pkce'
|
Added comments. |
Also made the comments better.
| "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." |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. :-)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@robotdan you comfortable with this as it is now? Would love to get this included in the next point release. |
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.