-
Notifications
You must be signed in to change notification settings - Fork 911
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
base: master
Are you sure you want to change the base?
Conversation
The folks at twitter would like me to point out that |
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 |
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. |
Update internal calls to sign_request() to use include_body_hash parameter
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. |
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. |
merge this |
please |
@rickhanlonii this seems like something that's relevant to your work in #138 as well? |
@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. |
It looks like section 4.1.1 specifies when to include the
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 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 |
Also, I think it is more clear for the parameter to be defined positively as in |
@rickhanlonii I concur all around. @bensonk if you could add tests I'm 👍 after @jaitaiwan has reviewed. |
LGTM after tests are added. @bensonk are you able to provide the tests for this? |
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 toTrue
calledomit_body_hash
. With this small change, my code to consume twitter's streaming API works.