[OAUTH2] - Add in Authorization Header and support extra headers by default #117

Closed
ciaranj opened this Issue Nov 20, 2012 · 7 comments

Projects

None yet

5 participants

@ciaranj
Owner
ciaranj commented Nov 20, 2012

It would be sensible to add in support for sending extra headers on the request.

In particular 'OAuth2#get' should be setting a header to 'Authorization Bearer <access_token>' (but to support different server implementations we also need to support customisation of the word between Authorization and <access_token> ... there was a period of time where OAuth2 was the correct term)

So I propose we add a new optional argument to the OAuth2 constructor function 'customHeaders' that will be utilised internally by _request (and thus by both 'OAuth2#get' and 'OAuth2#getOAuthAccessToken'). This should be an object literal)

We should also expose a property than can be used to configure the 'Authorization type' (defaulting it to 'Bearer') that can then be used within the 'OAuth2#get' method to pass through to the internal _request method and mixed in with the existing headers.

All tests / Pull requests gratefully received.

@yaru22
Contributor
yaru22 commented Nov 20, 2012

I think it's a good idea. One question though; is there a need for configuring the authorization type (i.e. Basic, Bearer, OAuth2, etc.)? The user will specify it in customHeaders, right?
e.g.

var customHeaders = {
  Authorization: "Bearer mF_9.B5f-4.1JqM"
};
var oauth2 = new OAuth2(clientId, clientSecret, baseSite, authorizePath, accessTokenPath, customHeaders);

Also, you are envisioning customHeaders to be applied to every request?
Even with customHeaders in the constructor, I think we should have 'headers' parameter in 'OAuth2#get' and 'OAuth2#getOAuthAccessToken' (as in Pull Request #116) because some requests might contain different header values. So the user should be able to specify certain headers request by request.

What do you think?

@ciaranj
Owner
ciaranj commented Nov 20, 2012

Ok. Good questions. I wasn't expecting the user to specify the Authorization header in the custom headers, no I was envisaging that it would be applied automatically in the OAuth2#get method (I feel that it should be 'hidden' from most users, remember hard-core users can always access the OAuth2#_request method and implement whatever headers they want/nead already)

And yes whilst it makes sense to specify certain headers per request (it does, it really does) this isn't unfortunately the approach taken with the OAuth(1) API, so doing it in this way is consistent between the two libraries. If one did need to do it per request then you could always recreate the OAuth2 instance with the request specific request headers each time (which is less than ideal, but a) an edge case and b) more importantly [to me] consistent with the OAuth(1) API.

@yaru22
Contributor
yaru22 commented Nov 21, 2012

Okay, my first attempt to implement the approach you mentioned is added to Pull Request #116. Could you take a look at it? Thanks!

@ciaranj
Owner
ciaranj commented Nov 25, 2012

This has been implemented as discussed by @yaru22 (thank you ) in #116

@ciaranj ciaranj closed this Nov 25, 2012
@sreuter
sreuter commented Dec 12, 2012

Can we have a new package in npm please? This seems mandatory for lots of oauth2 providers.

Please make 0.9.9 (not 1.0.0), as it shouldn't break anything but will then automatically included as most recent version by most package.json (0.9.x) links ;-)

@jaredhanson

As currently implemented, this is setting the access token both in the headers and the query string.

Here, it builds headers containing a bearer token:
https://github.com/ciaranj/node-oauth/blob/master/lib/oauth2.js#L174

and, by passing access_token to _request, the token is also set in query parameters:
https://github.com/ciaranj/node-oauth/blob/master/lib/oauth2.js#L67

The spec mandates that the token appear in only one of headers, query params, or body. As such, many providers may reject requests.

@woloski
woloski commented Mar 5, 2013

👍
This broke our system in production because we werent usinh shrinkwrap and this was a minor bump.

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