-
-
Notifications
You must be signed in to change notification settings - Fork 423
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
Forcing HTTPBasicAuth in fetch_token results in invalid_request from Google #218
Comments
This sounds like we need a Google compliance fix. We unconditionally added this auth header because the OAuth2 spec mandates that servers support Basic Auth, as discussed in #206. |
For sure, I don't know enough about the spec to create a pull request, and I figured the change was made for a reason. |
Doesn't need a compliance fix since the underlying issue was fixed with #212. Spent way too much time debugging it and was about to make a PR when I figured this out. Any chance you can release 0.6.1 with this fix so that others like me aren't blocked by this? |
If someone else is struggling with this, downgrading to requests-oauthlib==0.5.0 also solves the problem |
Please can we get a new release with this fix! Will be downgrading to 0.5.0 for now but just spent WAY to much time trying to debug this. I should note that pip installing 0.6.0 on Windows seems to give a different version than Linux. Which made this extra fun to debug. |
@regner Unfortunately, the fix for this will only come when someone sets aside the time to write it. I may get around to it at some point but I'm stretched pretty thin. |
Sorry, thought this fixed it: https://github.com/requests/requests-oauthlib/pull/212/files |
@regner It does not. That fixed one issue, but this specific issue results from having the Basic auth header present at all. |
@Lukasa I can take a shot at fixing it with a compliance fix, I'll make a PR later today. |
RFC 6749 is very permissive when it comes to authentication. The wording is
and
Unfortunately, a lot of OAuth 2 implementations go with what the RFC does not recommend. So, really, both ways of of authentication need to be available despite the RFC considering authentication via the request body a rare edge case. |
@jsfan Are you saying that some implementations don't support authentication using Basic auth at all? This is where the protocol implementer in me gets mad. I don't understand the point of having an RFC if everyone will ignore it. Regardless, does this mean we need a configuration option of some kind? |
@Lukasa Google doesn't support authentication using Basic auth at all, I think. |
You might want to test it again now that #227 is closed since it could also be that the username was being passed as if it was the |
But, looking at it again I guess that maybe isn't relevant in this grant type... |
@Lukasa: Until I saw the Bitbucket authentication, I didn't even know that Basic Auth is the RFC's recommendation. I have never tried to find out if providers who use authentication in the body will accept Basic Auth as well. However, most documentation and sample code I have seen across multiple providers did not mention Basic Auth as an option at all. So, it's probably a fair assumption that some would have ignored the RFC's recommendation altogether. |
Pretty late into the scene, but I also encountered this problem with a different project (not Google) where I cannot authenticate because of the extra Basic Auth headers. I created PR #237 to address this issue. |
0.6.0 is no longer the latest version of requests-oauthilb, and the last comment on this issue is two years old. I'm going to close this issue. If it's still a problem on the latest version of requests-oauthlib, please open a new GitHub issue and reference this one. |
Using Flask_OAuth2_Login (for example):
results in:
This is due to a change introduced in 0.6.0 in oauth2_session.py/fetch_token:
whereas previously auth was allowed to remain empty. Google responds with:
and everything falls down from there. Commenting out the line allows the request to complete normally.
The text was updated successfully, but these errors were encountered: