Adding post_headers parameter to getOAuthAccessToken method. #116

Merged
merged 1 commit into from Nov 25, 2012

Conversation

Projects
None yet
2 participants
Contributor

yaru22 commented Nov 20, 2012

Adding post_headers parameter to getOAuthAccessToken method. This would be useful, for example, for specifying bearer token in Authorization header (Stripe Connect is such a case).

Owner

ciaranj commented Nov 20, 2012

Thank you for this, i've written up how I think we should do this in : #117 What do you think?

Owner

ciaranj commented Nov 21, 2012

Thank you for this, in particular for providing some test cases 👍 This is very close to what I had in mind, but I have a few comments ;) (of course)

  1. Would it be possible to squash your two commits together, and then split out the formatting changes into a separate commit ? (just so anyone following the changes can clearly spot the API changes, as opposed to the formatting changes)

  2. I'm not sure that it is correct for us to be sending the Authorization header in the OAuth2#getOAuthAccessToken method, but I admit I struggle following the various OAuth2 specs !

  3. We're not doing anything in the OAuth2#_request method with the new customHeaders property?

Contributor

yaru22 commented Nov 21, 2012

  1. Done. I made changes on my local git repo but haven't pushed it yet. Might need to change some more depending on the point 2). See below.
    Also, I'll just create a separate pull request just containing the formatting (minor syntax fix) changes after this thing goes through.

  2. I was reading Stripe Connect documentation (https://stripe.com/docs/connect/oauth#token-request). What they do is sending the clientSecret over authorization header when sending the authorization grant.

I looked into the OAuth 2 spec (http://tools.ietf.org/html/rfc6749#section-4.1.3) and it says

If the client type is confidential or the client was issued client credentials (or assigned other authentication requirements), the client MUST authenticate with the authorization server as described in Section 3.2.1.

Following Section 3.2.1 (http://tools.ietf.org/html/rfc6749#section-3.2.1),

Confidential clients or other clients issued client credentials MUST authenticate with the authorization server as described in Section 2.3 when making requests to the token endpoint.

Following Section 2.3 (http://tools.ietf.org/html/rfc6749#section-2.3),

The authorization server MAY accept any form of client authentication meeting its security requirements.
So I guess using "Authorization: Bearer " is what Stripe Connect does for client authentication.

Hm... so not every OAuth2#getOAuthAccessToken needs to include Authorization header, but it needs to be able to support one. This kinda goes back to my original commit about adding post_headers to getOAuthAccessToken (https://github.com/yaru22/node-oauth/commit/595dbeaf2d17712c0767857c5bcfac60992cff69). But I don't know if this should fall into hard-core user's use case (i.e. people who need this sort of client authentication does it themselves by accessing OAuth2#_request method).

Stripe is gaining popularity (at least in North America) and if so, a lot of people who would use Stripe Connect would need a way to specify authorization header in OAuth2#getOAuthAccessToken. But I don't know if this should justify adding post_headers parameter to OAuth2#getOAuthAccessToken.

What do you think we should do about this?

  1. Oops, thanks for pointing that out. I fixed it and added a test. Again, I didn't push it yet because I might need to make further changes due to point 2).

Thanks!

Owner

ciaranj commented Nov 21, 2012

Ughh . This is the problem with OAuth2 (imho), it is too open to interpretation :( Having said that, the customHeaders implementation if utilised inside of the _request method (which gets called from getOauthAccessToken) will automatically work for stripe, there would be no need to actually expose _request itself ?

Contributor

yaru22 commented Nov 21, 2012

Alright, I'll just remove setting the authorization header in #getOAuthAccessToken. As you said, the customHeaders implementation would work for Stripe. I'll have the code ready in a few minutes.

Contributor

yaru22 commented Nov 22, 2012

ping @ciaranj. Let me know if you think it needs more refining.

Owner

ciaranj commented Nov 22, 2012

Sorry I'll try and take a look at this again shortly, my freetime ebbs and flows. Tomorrow night probably :(

@ciaranj ciaranj added a commit that referenced this pull request Nov 25, 2012

@ciaranj ciaranj Merge pull request #116 from yaru22/master
Adding post_headers parameter to getOAuthAccessToken method.
282733a

@ciaranj ciaranj merged commit 282733a into ciaranj:master Nov 25, 2012

Owner

ciaranj commented Nov 25, 2012

This looks good, thank you :)

Contributor

yaru22 commented Nov 25, 2012

Thanks @ciaranj!

yaru22 referenced this pull request in jaredhanson/passport-oauth Nov 27, 2012

Merged

Adding options.customHeaders as the last parameter to OAuth2 creation. #11

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