token endpoint should not require client secret in request

[tiziano88]

Currently the server requires that the client sends back its id and secret to the server when exchanging an authorization code for an access token:

• https://github.com/go-oauth2/oauth2/blob/master/server/server.go#L280
• https://github.com/go-oauth2/oauth2/blob/master/server/handler.go#L49

But the OAuth2 RFC does not require this: https://tools.ietf.org/html/rfc6749#section-4.1.3 i.e. the code itself should be enough to uniquely identify the client (passing the client_id parameter is allowed, though optional).

This means that currently this server implementation does not correctly work with the standard Go oauth2 library, and I guess many others.

I think a fix could be to maintain a map from authorization codes to clients, and use it to look up the client instead of expecting id and secret to be provided in the request.

[LyricTian]

Thank you for your issue, I have been fixed.

[kulti]

What is the coincidence! I've analyzed same bug several hours ago (for PasswordCredentials and Refreshing). I found solution in custom ClientInfoHandler. Because TokenInfo required client_id and client_secret, I bind username/password to client_id in my store implementation.

[tiziano88]

Uhmm, actually maybe the existing version was correct? Looking at https://developers.google.com/identity/protocols/OAuth2WebServer#handlingresponse , it seems that client_id and client_secret are actually passed at that step, and in fact now the client secret is effectively never used, which can't be correct? I'm still looking into it

[tiziano88]

https://github.com/golang/oauth2/blob/master/internal/token.go#L165 for reference, from the official OAuth2 Go package; it seems that we should send and receive client_id and client_secret over basic auth.

[tiziano88]

I think the course of action should be to undo the recent change, but let the server check both client_id/client_secret URL parameter and also HTTP basic auth, and use whichever is available. That way it would work fine in all cases.

[tiziano88]

Relevant part of the spec: https://tools.ietf.org/html/rfc6749#section-2.3.1

[tiziano88]

Never mind, I have just looked at the change again, and it did already change the server to use basic auth instead of URL params, so please disregard my last few comments -- it's all good! The only remaining good-to-have thing now would be the ability to fall back on URL parameters in case basic auth is not provided, therefore allowing both mechanisms to coexist.

[tiziano88]

Also, a possible bug: https://github.com/go-oauth2/oauth2/blob/master/server/server.go#L296 this line is still checking for the client_id form value; I think that check could be removed now?