feat(oauth): Add RFC7636 aka PKCE support. #244
Conversation
There was a problem hiding this comment.
Thank you for your work and these changes! I did not have the time yet to go through all the changes as it's a ton of code (~2500 LOCs). The first thing I noticed is that PKCE is hard-coded in the request handlers, but in the terms of the RFC it's really an add-in on top of OAuth2. Thus I'm really not sure if this should be handled there but instead be seen as a custom handler (like the authorize code flow or openid connect) which would fit more in the existing composition process (instead of setting some boolean in the main struct)
What do you think?
| @@ -89,6 +91,30 @@ func (f *Fosite) NewAccessRequest(ctx context.Context, r *http.Request, session | |||
| } | |||
| accessRequest.Client = client | |||
|
|
|||
| // PKCE Handling | |||
There was a problem hiding this comment.
I really think this should be a custom handler and not hardcoded here
There was a problem hiding this comment.
Ok it's more like an OAuth2 extension. I'm not really aware of project code organization yet.
There was a problem hiding this comment.
Most of the code are (re)generated mocks ^^
There was a problem hiding this comment.
About PKCE settings (RequiredForPublicClient), maybe via a decorator (sort of middleware) in the authorize and token endpoints.
This could be helpful to add / process parameters before handling core OAuth operations.
There was a problem hiding this comment.
The idea of this framework is to have handlers such as this one for the auth endpoint which implement parts of the oauth2 spec. What you would do (probably) is some checks at the /auth endpoint and then some checks or updates to the response at the /token endpoint
| @@ -25,6 +25,11 @@ type AuthorizeRequest struct { | |||
| State string `json:"state" gorethink:"state"` | |||
| HandledResponseTypes Arguments `json:"handledResponseTypes" gorethink:"handledResponseTypes"` | |||
|
|
|||
| // Optional code_challenge as described in rfc7636 | |||
| CodeChallenge string `json:"code_challenge" gorethink:"code_challenge"` | |||
There was a problem hiding this comment.
I really think this can be covered by Request.Form
There was a problem hiding this comment.
I consider that CodeChallenge is like State that's why in coded it here.
There was a problem hiding this comment.
My first problem was the Session usage, due to the fact it's an interface and shared in openid and oauth2 handler (I fix missing implementation on first commit). I had to store code_challenge to compare it on the token retrieval step, maybe Request.Form is a better place if it is shareable like session.
There was a problem hiding this comment.
So the Request.Form thing will contain all request parameters from the /auth endpoint, it will thus also contain code_challenge if that parameter is given to the /auth endpoint. You won't have to deal with any extra sessions or stuff there.
| // https://tools.ietf.org/html/rfc7636 | ||
| // | ||
| // PKCE is only for code response type | ||
| if request.ResponseTypes.Has("code") { |
There was a problem hiding this comment.
Same thing here, this shouldn't be a hardcoded thing but instead an add-in via handlers
There was a problem hiding this comment.
Yep I'm agree. Applying "Do it, do it right, to do better" philosophy ^^
|
@Zenithar can I help you some way to push this forward? :) |
|
Yes no problem ^^ |
|
Ok, so I'm not really fluent yet on PCKE internals but as far as I understood it works like this:
This is very easy to achieve with a new custom handler:
Does this make sense? |
|
Yes, it is exactly what it should work. |
|
This flow is mainly used by mobile application. |
|
I took a shot at it in #246 |
Ref #213
I have implemented RFC7636 aka PKCE.
But not really happy about Session usage in order to recevive CodeChallenge on token endpoint, maybe need some refactoring.