search

maraujop / requests-oauth

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

Implement OAuth callbacks in authentication headers #13

Closed fletom closed 12 years ago

fletom commented 12 years ago

See: https://dev.twitter.com/docs/auth/implementing-sign-twitter

I am working on this in my fork ATM.

maraujop commented 12 years ago

Yes, you are right Fletcher.

I've totally missed this important parameter. It should be quite easy to add. As you are working on it, I will let you do the work. Let me know if you need help.

Thanks, regards Miguel

fletom commented 12 years ago

I'm working on both of these, and I'll submit a pull request soon.

maraujop commented 12 years ago

I saw your patch for this and I appreciate your time and effort. But I didn't like the idea of adding a callback parameter to the hook class, for something that is only used in connection initialization.

I've pushed a fix for this, that doesn't need an extra parameter. Instead I extract from data the parameter oauth_callback if it's there and inject it in the headers. There is a test working with this system. You can now do:

twitter_oauth_hook = OAuthHook(header_auth=True)
client = requests.session(hooks={'pre_request': twitter_oauth_hook})
response = client.post('http://api.twitter.com/oauth/request_token', {'oauth_callback': 'oob'})

I'm not releasing an official upgrade because for getting latest dev branch from requests-oauth to work, right now you will need to use my fork of requests. I've submitted a pull request to requests that fixes and issue with hooks. Specifically this one: https://github.com/kennethreitz/requests/issues/445

Let me know what you think and if it works for you.

Cheers, Miguel

fletom commented 12 years ago

Hey Miguel,

So as I'm sure you saw my patch was basically one or two lines and very little effort or testing anyways. I had planned on coming back to this to finish the two tickets I opened, but work is a bit busy these days (my startup is launching soon).

However, I personally disagree with the "magic" of intercepting oauth_* parameters and such. In my opinion, all parts of the OAuth protocol should be encapsulated in the hook itself. There are two reasons why what you mentioned isn't really a problem:

  1. There's nothing in the protocol saying you can't add the callback header to all requests. So the only downside is a few extra characters to send over the network.
  2. The way this callback thing works, you'll only want to make this one request to Twitter's request_token anyways before redirecting the user. In my code at least, I have global stuff like my app's keys built-in to a basic subclass of OAuthHook, and then I create instances of that subclass by passing in whatever data I need for this particular session, like the user's keys or the callback for unauthenticated users. So the above it unlikely to happen in the first place.

I believe that passing the callback as a parameter is logically and semantically sound. It's a bit like the unauthenticated version of passing in the user's keys for a specific request.

Let's keep this discussion open. As you said, let me know what you think.

Thanks for the time you're putting into this really nice tool.

Regards, Fletcher

maraujop commented 12 years ago

Hi Fletcher,

So as I'm sure you saw my patch was basically one or two lines and very little effort or testing anyways. I had planned on coming back to this to finish the two tickets I opened, but work is a bit busy these days (my startup is launching soon).

Yes, your patch was totally legit. No worries about this, we all have deadlines and have to rush.

There's nothing in the protocol saying you can't add the callback header to all requests. So the only downside is a few extra characters to send over the network.

True, nothing prevents you from doing so and byte performance is not something I care about.

The way this callback thing works, you'll only want to make this one request to Twitter's request_token anyways before redirecting the user. In my code at least, I have global stuff like my app's keys built-in to a basic subclass of OAuthHook, and then I create instances of that subclass by passing in whatever data I need for this particular session, like the user's keys or the callback for unauthenticated users. So the above it unlikely to happen in the first place.

I've read this paragraph several times and I'm quite sure I don't understand and it's a good reasoning for your argument. Could you please rephrase in detail?

I believe that passing the callback as a parameter is logically and semantically sound. It's a bit like the unauthenticated version of passing in the user's keys for a specific request.

I see what you mean. The reason behind my way of doing it, is that this parameter is used once for connection establishment. After that, you don't need the callback, other parameters are used in the whole connection, so they are not really semantically equal, right?

I believe that passing the callback as a parameter is logically and semantically sound. It's a bit like the unauthenticated version of passing in the user's keys for a specific request.

If that is the case, then verifier has probably the same space.

Let's keep this discussion open. As you said, let me know what you think.

Sorry, to take so long to answer, too much Open Source work lately :)

Thanks for the time you're putting into this really nice tool.

Thanks for the kind words. Thanks for your time.

Cheers, Miguel

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