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

authorization_code strategy is not authenticated #128

Closed
robotlovesyou opened this issue Mar 5, 2019 · 3 comments · Fixed by #131 or ueberauth/ueberauth_google#68
Closed

authorization_code strategy is not authenticated #128

robotlovesyou opened this issue Mar 5, 2019 · 3 comments · Fixed by #131 or ueberauth/ueberauth_google#68

Comments

@robotlovesyou
Copy link

robotlovesyou commented Mar 5, 2019

Unless I am misreading the code, there does not seem to be any attempt to authenticate the client when exchanging the code for a token using the authorization_code strategy. This is similar to the issue #115 but your response there seems wrong by my reading of the spec.

The final paragraph of section 4.1.3, which describes the request for an access token (https://tools.ietf.org/html/rfc6749#section-4.1.3)[https://tools.ietf.org/html/rfc6749#section-4.1.3] reads:

If the client type is confidential or the client was issued client credentials (or assigned other authentication requirements), the client MUST authenticate with the authorization server as described in Section 3.2.1.

The default behaviour for most libraries seems to be to use basic authentication, although some implementations require or support placing the client_secret in the body. You have these methods in place for the client credentials strategy but for some reason they are not in place for the authorization_code strategy. For an oauth2 implementation which requires the use of basic authorization to authenticate against the authorization server, I guess maybe this can be worked around using the headers property of the client? Where the implementation supports placing the client_secret in the body of the request the issue can be worked around as described in #115 but given the wording of the spec I don't think it's valid to require users of this strategy to implement the authentication themselves.

@scrogson
Copy link
Member

scrogson commented Mar 28, 2019

@robotlovesyou looking at section 4.1.3, I see no mention of client_secret. Many servers seem to require this, but as it is not part of the spec, I have chosen to not include it. If the server you're interacting with requires it, it can easily be added via put_param or basic_auth/1.

@scrogson
Copy link
Member

@robotlovesyou after looking a bit further, I think I see what you're saying in regards to the basic auth header.

I was reading in section 2.3 and it appears that the preferred method for sending client credentials is via basic auth header.

While this is probably not a complete disaster, I guess I have been misinterpreting the spec all this time. Thank you for bringing it to my attention.

I can put together a PR to fix this...although, I'd like to make sure that the change doesn't break existing clients - which I assume it shouldn't.

@robotlovesyou
Copy link
Author

robotlovesyou commented Mar 29, 2019

Great. Thanks for looking into it.

It's not the easiest spec to follow!

PragTob added a commit to PragTob/ueberauth_google that referenced this issue Aug 21, 2019
oauth2 has a recent possibly backwards incompatible release that
makes sure the spec is followed and authorization headers are
respected (https://github.com/scrogson/oauth2/blob/master/CHANGELOG.md#v200-2019-07-15)
This fixed ueberauth/oauth2#128 hence I think it's important to
include.
Decided to not require 2.x as that might conflict too hard
with other libraries. Also decided to allow minor version bumps
as @scrogson seems to be good about semver <3
As ueberauth#66 isn't merged yet I'd like it if this could get in with the
release.

fixes ueberauth#67
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants