ianstormtaylor / permit

An unopinionated authentication library for building Node.js APIs.
MIT License
1.69k stars 70 forks source link

bearer token use in protected resources server #6

Open panva opened 6 years ago

panva commented 6 years ago

Hello @ianstormtaylor,

coming from Bearer Token Usage environment there are a couple of things I couldn't find in your library. These might make sense to adopt so that the library is ready for use for Resource Servers.

1) Clients MUST NOT use more than one method to transmit the token in each request. Currently when both header and query is presented header is returned. An error should be thrown instead. 2) Three methods of sending bearer access tokens are defined, application/x-www-form-urlencoded body is missing at the moment. I understand this might be tricky to explain to users but most commonly req.body is populated by popular body parsers in frameworks such as express, for koa-body an option needs to be passed ({ patchNode: true }).

What's your opinion on this and would you accept a PR filling it in? My proposal,

ianstormtaylor commented 6 years ago

Hey @panva, good question. I like where you're going with this.

Is the OAuth2Bearer usage specifically different from the regular Bearer? I think it would be nice to be able to maintain a single Bearer implementation that could be used for OAuth usage as well as non-OAuth usage?

What if:

The only downside is that multiple tokens will look like authentication_required. I'd be open to another solution that handles that case better. But it might be good to first get it working that way.

For the more information case, what if:

panva commented 6 years ago

Is the OAuth2Bearer usage specifically different from the regular Bearer?

Is there an RFC other than https://tools.ietf.org/html/rfc6750 that Bearer was modelled after?

What if:

  • We add support for options.body to Bearer.
  • We check for multiple tokens and if so, return undefined.

The only downside is that multiple tokens will look like authentication_required. I'd be open to another solution that handles that case better.

The usual result of such a request would be invalid_request.

ianstormtaylor commented 6 years ago

Hey @panva, sorry for the delay there.

Okay, so yeah I based it on the RFC 6750 idea of "bearer tokens", but I did it loosely, with the goal of making it useful for implementation by lots of the APIs that I see in the wild that use tokens, but not always exactly as OAuth designates them. For example:

Essentially, I like to think of the "basic" and "bearer" permits as more abstract goals, to make them flexible enough to model most common APIs, where they aren't specifically "to spec". The "basic" permit's goal is a username and password, whereas the "bearer" permit's goal is a single bearer token.

This mean they can be used for anything that attempts to get at those two goals. For example, OAuth also uses the basic scheme for the client credentials grant, and it does so with the configuration:

const clientCredentials = new Basic({
  query: ['client_id', 'client_secret']
})

Which is fairly easy to configure, but the Basic permit itself isn't tightly coupled to that specific implementation.

However, I agree with you that it should be tweaked to solve the issues you mentioned, since it should be possible to configure the Bearer permit exactly to the OAuth Bearer in RFC 6750. Ideally doing so would be as simple as...

const oauthBearer = new Bearer({
  query: 'access_token',
  body: 'access_token',
})

...with the proper error handling.

So it sounds like we'd need to:

  1. Support body searching as an option. We might as well do this in both Basic and Bearer.
  2. Change the API to return a reason for failure. This could either be done as returning an object with result.reason, or by changing the API to throw for failures with an error.code.