Updates request factory to exclude oauth_* params due to Authorization header #33

Merged
merged 2 commits into from May 7, 2013

3 participants

@jnormore

Requests should not use url or post params if Authorization header is used, see

http://oauth.net/core/1.0/ (5.2. Consumer Request Parameters) states that it should be one of Authorization header, POST params, or URL params (preference in that order).

Twitter API also has issues with this, https://dev.twitter.com/docs/api/1/post/oauth/request_token, states "If you're using HTTP-header based OAuth, you shouldn't include oauth_* parameters in the POST body or querystring"

Personally, I noticed this when using an API that uses django-piston, which creates a multi value dict for params so when there are duplicates I see invalid signatures.

This update removes the passing of the parameters argument to the request creation in requestWithMethod:path:parameters. The request created for the authorization step also had to be updated to use the url parameters because it was using this method before.

@jnormore jnormore Updates requestWithMethod:path:params to not use post or query params…
…. Updates authorization request to use query params.
a67b05f
@nikz

Hi! I think we've run into the same problem - my solution is #34.

I think you still need to post through some parameters - for instance the Xero API still needs the "xml" parameter when creating new objects etc, and it seems like that shouldn't be in the Authorization header from looking at the spec.

I may be wrong though, if so, please correct me! :)

@jnormore

@nikz Yes, seems to be exactly the same problem! I think your solution is better since it doesn't exclude other parameters (I didn't think of that). But I think it should do it for the GET method too (since this is a valid method for this), not just POST, in which case you'll need to do something similar to jnormore@a67b05f#L0R280 for the authorize request.

@nikz

You are quite right! The solution looks good too - I'll try to get a chance to pull that change across from you today (if you get there before me let me know)

@jnormore

@nikz A month late but I finally got around to adding this if you're interested, I pretty much copied your change for that part

@nikz

@jnormore looks sweet! 👍

@mattt mattt merged commit b7e628a into AFNetworking:master May 7, 2013
@mattt

@jnormore @nikz Thank you both for your pull requests. I just merged this in, and will tag this as a new release as soon as I finish clear out a few more open issues. Cheers!

@nikz

Glad we could help :)

(Thank you for AFNetworking and all of the other great open source stuff you've released!)

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