Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bearer token use in protected resources server #6

Open
panva opened this issue Apr 26, 2018 · 3 comments
Open

bearer token use in protected resources server #6

panva opened this issue Apr 26, 2018 · 3 comments

Comments

@panva
Copy link

panva commented Apr 26, 2018

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,

  • export OAuth2Bearer with these extra features
  • throw when multiple methods are presented
  • check for req.body access_token param
@ianstormtaylor
Copy link
Owner

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:

  • 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. But it might be good to first get it working that way.

For the more information case, what if:

  • We change both Bearer and Basic to return dictionaries instead of the current string/array values (eg. { token } and { username, password }).
  • That allows us to expose extra information, such that we could expose something like { token, reason } or some other thing to handle the "must not use more than one method". Which developers can check if they want to get specific.

@panva
Copy link
Author

panva commented Apr 27, 2018

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
Copy link
Owner

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:

  • Slack uses bearer tokens but doesn't conform to RFC 6750's error objects. And it allows the token in query string as ?token instead of ?access_token.
  • Digital Ocean uses bearer tokens but also allows them in the username of basic auth.

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.

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

No branches or pull requests

2 participants