search

maraujop / requests-oauth

Python's Requests OAuth (Open Authentication) plugin
BSD 3-Clause "New" or "Revised" License
188 stars 33 forks source link

fix for issue #12 breaks GET request with params #17

Closed joeshaw closed 12 years ago

joeshaw commented 12 years ago

Doing a simple GET request passing in params when header_auth is False breaks. Ie,

session.get("http://www.example.com", params={"foo": "bar"})

This is because get_normalized_parameters() only sets request.data_and_params if the Content-Type is application/x-www-form-urlencoded or request.data is a basestring. For most GET requests neither is true: Content-Type is unset and request.data is None because there is no body. (Things may work if you pass in the values as data because this sends a body, but sending a body with a GET request is considered bad form.)

When header_auth is False, __call__() replaces the request.url with a call to to_url(), which iterates over the values in the now-unset request.data_and_params. As a result, your parameters are never passed through.

maxcountryman commented 12 years ago

+1

maraujop commented 12 years ago

Thanks for reporting this Joe and confirming it Max. This is true.

I'm guessing you are both trying dev branch. In order for dev branch to work, you will need to use my fork of requests, because of this issue: https://github.com/kennethreitz/requests/issues/445

As requests, sometimes fails to set a Content-Type, I'm thinking that if Content-Type header is not set, I will set request.data_and_params as a fallback.

Do you both agree?

Thanks, cheers Miguel

joeshaw commented 12 years ago

Indeed, I wasn't using the fork.

I wouldn't characterize requests as "failing" to set a Content-Type... it's inappropriate to set it for a request that has no body, like most GET requests.

It seems to me like you'd always want to use data_and_params unless explicitly provided as a string by the user.

maraujop commented 12 years ago

I wouldn't characterize requests as "failing" to set a Content-Type... it's inappropriate to set it for a request that has no body, like most GET requests.

Yes, you are right. Fail wasn't the right word.

It seems to me like you'd always want to use data_and_params unless explicitly provided as a string by the user.

Well, the problem here is that depending on the Content-Type, data_and_params have to be or not included. You can see issue #12 as a reference. So I will push a fix doing what I mention, if no Content-Type is set, then data_and_params are set.

maraujop commented 12 years ago

I have added a test for a GET with params against Twitter API.

Please, confirm if this fixes the problem :)

Cheers, Miguel

istruble commented 12 years ago

I believe that the issue was affecting both the GET + header_auth=False case and the non-GET + header_auth=True cases (eg. PUT or POST). The only test I have for the non-GET + header_auth=True is a Ruby on Rails app and it may be a case of OAuth v1.0 vs. v1.0a and how people interpret the specs.

Confirmed, the problem called out in this issue (GET requests when header_auth=True) is fixed as well as the extra case that I just mentioned of non-GET requests when header_auth=False.. Thanks for fixing it before I had a chance to do more than just pull dev and start looking at the code.

Best, Ian

maraujop commented 12 years ago

Thanks for confirming it Joe.

I'm waiting for @kennethreitz to merge a solution for the hook system problem, so I can release a new stable version including this an other fixes.

Cheers, Miguel

mart-e commented 12 years ago

I just tried with your fork of requests and the dev version. The test module still fails for test_twitter_status_GET_with_params with header_auth set at True (401 error) and for test_twitter_create_delete_list I have an error Could not authenticate with OAuth when header_auth is false.

maxcountryman commented 12 years ago

I believe this will also fail is header_auth is set to False. This is due to the fact that URL params are evaluated before a pre-request hook is entered. Sadly, Requests' hooks are quite limited at the moment.

maraujop commented 12 years ago

@mart-e I've just pushed a couple fixes that should make:

Can you please confirm, latest dev branch with my requests fork fix your issues?

@maxcountryman I think I've found a fix for that. But you are right, hooks are quite limited at the moment.

The only Twitter test that fails now is test_update_profile_image with header_auth=False. I believe this could be due to Twitter provider itself.

mart-e commented 12 years ago

Great, it works for everything (except test_update_profile_image with header_auth=False but you know that).

maraujop commented 12 years ago

@mart-e Thanks for confirming it!

I've just pushed another fix, that gets test_update_profile_image passing. I think this last commit, closes the issue. Let me know if it works for you, please.

Cheers, Miguel

mart-e commented 12 years ago

Ran 6 tests in 8.566s

OK

I guess you can close the issue

maraujop commented 12 years ago

Thanks @mart-e for confirming it.

I will close the issue as soon as I release a new stable version of requests-oauth, when the hook system in requests get some tuning.

maraujop commented 12 years ago

Version 0.4.0 has been released, it works well with requests 0.12.1 or higher. I'm closing this issue.

Cheers