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

Added Request.sign() option to omit oauth_body_hash #103

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

bensonk
Copy link

@bensonk bensonk commented Jan 10, 2012

I'm using oauth2 to consume Twitter's streaming API. To make the library compatible with their API, the oauth_body_hash header must be omitted. To allow that, without breaking any existing functionality, I added a parameter defaulted to True called omit_body_hash. With this small change, my code to consume twitter's streaming API works.

@bensonk
Copy link
Author

bensonk commented Jan 10, 2012

The folks at twitter would like me to point out that oauth_body_hash is a nonstandard addition to the protocol, and that when you're connecting to twitter it shouldn't ever be included. While the oauth2 library works with their REST API currently, it's possible they will be more strict in the future and it will cease to work. To maintain compatibility with twitter in the long run, it would be best to make omit_body_hash true by default.

@ghost
Copy link

ghost commented Jan 19, 2012

I'd love to see this merged as soon as possible. Not having a way of controlling the body_hash results in new versions of oauth2 immediately breaking when used against Oauth providers that do not support this extension. I

@bensonk
Copy link
Author

bensonk commented Jan 19, 2012

Sadly, I've heard simplegeo is defunct, so I'm not sure it will ever get merged. I guess somebody else needs to take up the torch and maintain this.

tomds and others added 2 commits February 15, 2012 00:05
Update internal calls to sign_request() to use include_body_hash parameter
@groodt
Copy link
Contributor

groodt commented Feb 29, 2012

I just ran into the same issue. I've hacked it to work by setting is_form_encoded=True when I create my Request. Clearly a massive hack. I think this OAuth library needs a bit of an overhaul. There are a lot of open issues, which is a pity since it appears to be the best Python OAuth lib out there.

@bensonk
Copy link
Author

bensonk commented Mar 1, 2012

I'd love to see somebody take it and run with it, but I don't know who would do it. I don't think I have time to maintain it full time, but I'll keep patching it as I see bugs that I know how to fix.

@domino14
Copy link

merge this

@domino14
Copy link

please

@joestump
Copy link
Owner

@rickhanlonii this seems like something that's relevant to your work in #138 as well?

@joestump
Copy link
Owner

@bensonk thanks for the PR! @jaitaiwan and @rickhanlonii are probably going to chime in on this as well. We need tests on this before we merge, but feel free to wait on those for now.

@rickhanlonii
Copy link
Collaborator

It looks like section 4.1.1 specifies when to include the oauth_body_hash:

4.1.1. When to Include the Body Hash

Not all requests should contain the oauth_body_hash parameter.

  • OAuth Consumers SHOULD NOT include an oauth_body_hash parameter when making Request Token or Access Token OAuth requests.
  • OAuth Consumers MUST NOT include an oauth_body_hash parameter on requests with form-encoded request bodies.
  • OAuth Consumers SHOULD include the oauth_body_hash parameter on all other requests.

So I think this change in general is necessary according to 4.1.1.

The only question I have here is whether or not the default value should be True. It seems to me that it would be straightforward to pass omit_body_hash=True for only calls to Request Token or Access Token OAuth and default to False for all other requests.

In other words: I think the default value should match the requirements of "all other requests" rather than the two special defined cases. I understand from the above discussion that the requirements of "all other requests" to the twitter API would require omit_body_hash=True, but supporting a particular third party API should not be a motivating factor in the design at hand.

@rickhanlonii
Copy link
Collaborator

Also, I think it is more clear for the parameter to be defined positively as in include_body_hash=True, rather than negatively as in omit_body_hash=False since the latter requires parsing a double negative to understand that the body hash will be included.

@joestump
Copy link
Owner

@rickhanlonii I concur all around. @bensonk if you could add tests I'm 👍 after @jaitaiwan has reviewed.

@joestump joestump added this to the 2.0 milestone Jul 29, 2015
@jaitaiwan
Copy link
Contributor

@bensonk @joestump adding this to my long list of things to review.

@jaitaiwan
Copy link
Contributor

LGTM after tests are added. @bensonk are you able to provide the tests for this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants