Added support for scopes

[lfk]

These changes allow for scoping routes by introducing the Scope middleware, which may be used as follows:

router.get('/myRoute', app.oauth.authorise(), app.oauth.scope('scope_key'), function (req, res) {
   res.status(200).json({ message: 'Scope-protected route!' });
});

router.delete('/account', app.oauth.authorise(), app.oauth.scope({ 'account': [ 'write' ] }), function
(req, res) {
  // ...
});

It requires two new methods in the model: checkScope and saveScope. In order to properly support scopes in the authorization code grant flow, the saveAuthCode method must now also accept a scope parameter, which should be stored and later retrieved during the call to getAuthCode.

In implementations, saveScope should sanitize and validate the requested scope keys before proceeding with storage. How scopes are represented is left up to the developer.

When the authorisation flow is executed for a scope-protected route, the middleware passes the token object and the scope requirements to checkScope, which might return an 'invalid_scope' error if the criteria are not matched.

The Readme file has been updated to reflect these changes, and some updates were made to the postgresql example. I'd be happy to provide a more complete example if you feel this is heading in the right direction. I'm working on the tests at the moment, but I'd appreciate any and all feedback/advice on how best to write them, as I don't have much experience in the matter.

Don't hesitate to ask if anything's unclear.
Thanks for reviewing!

[avnersorek]

Hi

Thanks for working on this I think this will be a very valuable addition to this project.

Just a question - why a different middleware function ?
As you wrote - the authorization flow acts a little differently under a specific flow. Also I can't see a case where the scope function is used without the authorize one.

May I suggest that instead of -
app.oauth.authorise(), app.oauth.scope('scope_key')

We can have -
app.oauth.authorise({scope:'scope-key'})

Or maybe
app.oauth.authorise().scope('scope-key')

?

Thanks

Sent from my iPhone

On 28 בינו 2015, at 03:53, Lars F. Karlström [email protected] wrote:

These changes allow for scoping routes by introducing the Scope middleware, which may be used as follows:

router.get('/myRoute', app.oauth.authorise(), app.oauth.scope('scope_key'), function (req, res) {
res.status(200).json({ message: 'Scope-protected route!' });
});
It requires two new methods in the model: checkScope and saveScope. In order to properly support scopes in the authorization code grant flow, the saveAuthCode method must now also accept a scope parameter, which should be stored and later retrieved during the call to getAuthCode.

In implementations, saveScope should sanitize and validate the requested scope keys before proceeding with storage. How scopes are represented is left up to the developer.

When the authorisation flow is executed for a scope-protected route, the middleware passes the token object and the scope requirements to checkScope, which might return an 'invalid_scope' error if the criteria are not matched.

The Readme file has been updated to reflect these changes, and some updates were made to the postgresql example. I'd be happy to provide a more complete example if you feel this is heading in the right direction. I'd appreciate some advice on how to write the tests for these changes.

It might also be a good idea to make scoping a configuration option.

Don't hesitate to ask if anything's unclear.
Thanks for reviewing!

You can view, comment on, or merge this pull request online at:

https://github.com/thomseddon/node-oauth2-server/pull/149

Commit Summary

Implements scope support
Scoping is now implemented using middleware
The checkScope method now receives the entire bearerToken object
Updated Readme and postgresql example
File Changes

M Readme.md (25)
M examples/postgresql/index.js (6)
M examples/postgresql/model.js (47)
M examples/postgresql/schema.sql (1)
M lib/authCodeGrant.js (6)
M lib/error.js (1)
M lib/grant.js (23)
M lib/oauth2server.js (21)
A lib/scope.js (65)
Patch Links:

https://github.com/thomseddon/node-oauth2-server/pull/149.patch
https://github.com/thomseddon/node-oauth2-server/pull/149.diff
—
Reply to this email directly or view it on GitHub.

[lfk]

Hello there,

Thank you for the feedback! There are two reasons that I went with a separate middleware here, please tell me if the following seems odd.

First, since express manages the concept of routers, I thought it might be valid to protect an entire collection of routes with a single "instance" of the auth middleware, and then define finer-grained scope permissions on a per-route level (though you may, of course, also protect the entire router at once as well).

Example:

var account = express.Router();
account.all('/*', app.oauth.authorise());

account.get('/', app.oauth.scope({ account: ['basic'] }), function(req, res) {
  // ...
});

account.put('/', app.oauth.scope({ account: ['write'] }), function(req, res) {
  // ...
});

// ...
app.use('/account', account);

The second reason has a bit to do with the first. I might have some misconceptions here, but I believe that, since every inclusion of middleware basically creates a new function (thus my use of "instance" above), it would be preferable to keep said function as lightweight as possible. I'm afraid I can't back this up with a more solid claim though, since I haven't really investigated how this works.

[avnersorek]

Ok - that's a great example of why you would want those separate. Looks
great.
Thanks for the explanation and the work.

On 28 January 2015 at 21:13, Lars F. Karlström [email protected]
wrote:

Hello there,

Thank you for the feedback! There are two reasons that I went with a
separate middleware here, please tell me if the following seems odd.

First, since express manages the concept of routers, I thought it might be
valid to protect an entire collection of routes with a single "instance" of
the auth middleware, and then define finer-grained scope permissions on a
per-route level (though you may, of course, also protect the entire router
at once as well).

Example:

var account = express.Router();
account.all('/*', app.oauth.authorise());

account.get('/', app.oauth.scope({ account: ['basic'] }), function(req, res) {
// ...
});

account.put('/', app.oauth.scope({ account: 'write'] }), function(req, res) {
// ...
});
// ...
app.use('/account', account);

The second reason has a bit to do with the first. I might have some
misconceptions here, but I believe that, since every inclusion of
middleware basically creates a new function (thus my use of "instance"
above), it would be preferable to keep said method as lightweight as
possible. I'm afraid I can't back this up with a more solid claim though,
since I haven't really investigated how this works.

Reply to this email directly or view it on GitHub
https://github.com/thomseddon/node-oauth2-server/pull/149#issuecomment-71895997
.

[nunofgs]

Any particular reason why the order of parameters differs from checkScope()?

[nunofgs]

@lfk: I was in need of scopes and was planning on doing my own PR. Thank you for working on this!

[nunofgs]

Typo: simly -> simply.

[lfk]

@avnersorek: Thanks for validating the concept. :)

@nunofgs: You're welcome! I do agree with the issues you pointed out in the documentation and examples, and I'll fix them asap -- just got to extend the tests to make the build pass as well.

[thomseddon]

Hi @lfk - firstly thank you for your work on this PR already, it's probably one of the best this project had ever had with regards to implementation quality and code style likeness.

Here's my thoughts:

Authorisation

• As per @avnersorek 's query, I'd never imagined it as another middleware but your use case seems perfectly valid, however to make to majority use case simpler I'm tempted to allow scope to also be an optional argument for auth authorise function
• I think it should be required that scope is returned alongside the access token in getAccessToken. checkScope would simply be passed the scope associated with the access token and the required scope. We could even offer a default implementation that just does something like if (scope.indexOf(requiredScope)) { /* ok */ } else { /* not ok */ }, users could override if they want to get more fancy

Granting

• I think it's preferable for scope to be saved as part of the saveAccessToken method as oppose to updating it once it's in place (as you have done with saveAuthCode)
• This PR seems to focus on the auth_code grant, I would like to cover all grant types and I think by injecting it into the existing saveAccessToken this should be relatively easily achieved.

General

• I think internally we should always treat scope's as an array's, so everything we pass out to model methods should be an array, some backends (e.g. document based like mongo) would then be able to store them natively

Thoughts appreciated

[nunofgs]

This assumes that expired accessToken's are not deleted. This takes away control from the developer since he'll be forced to keep all of the access tokens around.

I see two possible solutions for this:

1. We can add the scopes column to the oauth_refresh_tokens table.
2. We can merge the two tables into one: oauth_access_tokens and oauth_refresh_tokens would become oauth_tokens with a type column. There really isn't any difference between these tables right now.

@thomseddon Maybe I'm missing something. What is the reason for having two tables right now? Is it for the purpose of this example or is it recommended by the spec?

[nunofgs]

One more note. The OAuth RFC 6749 states in section 6 that when using the refresh_token grant_type, the client can send a "scope" and the server MUST validate that the scope cannot include any scopes that were not part of the original access_token. Also, if the scope is omitted, it must be inherited from the last access_token.

However, that implementation has been left to the model in this PR. Should this be something that node-oauth2-server is responsible for handling?

[lfk]

@thomseddon: Thank you for the encouraging words! Addressing your feedback in order:

• I tried out an optional scope param for authorise. It was trivial to implement and stacks well with the custom scope middleware; it's even possible to define a "top-level" scope on a router using authorise, and add finer-grained checks with scope on specific routes. The only "con" I see here is that it opens up for multiple ways to get the job done, which might add some confusion. I'll tinker with it a bit more and add it to the PR.
• Agreed. That's how I've got it implemented on my project at the moment, and it feels like the right way to go. I do like the idea of a default checkScope implementation.
• There's no reason not to merge the two methods, so no objections there. I went with the isolated saveScope method for this PR in order to keep the proposed changes as isolated as possible from the master code.
  • One note here though is that, before saving, there should be some validation performed. Up to the developer, of course, but still. Again making reference to my application, I've got a setup in which clients are assigned a set of scopes they may solicit from the user; any other key is filtered out before saving, thus avoiding injection of undesired privileges by injecting code on authorisation dialog pages, etc.
  • Though the auth_code flow is what is currently (somewhat) documented in the examples, the present implementation does work for all available grant types. saveScope is always run after saveAccessToken, regardless of which grant flow is being utilized.
• As for internal representation, is there any reason not to go with Objects? After parsing the requested string, that's how I'm working with it in my application at the moment; being able to define required scopes as { "account": ["read", "write"] } feels straightforward, and it supports arbitrarily complex, multi-level setups with the same advantages of possible native storage. A drawback here is that it adds quite a bit of complexity to the default checkScope implementation though.

@nunofgs: You're absolutely correct -- whith the understanding that this is not the single, mandated manner of implementing scope storage. I opted for the most straightforward, string-based example here, leveraging the available example code, but there's nothing limiting the developer from creating, say, a connection table between access tokens and a table of available scopes. Since all interaction between the library and the application is defined via oauth.model, there shouldn't be anything hindering the developer from using a single token table at the moment, adding "WHERE" clauses to the getAcessToken and getRefreshToken methods, respectively. It might be of interest to make this the reference implementation though.

[nunofgs]

The saveScope function will be called on the model even if the user did not send a scope. In fact, scope here will be undefined.

I suggest following the pattern set by the saveRefreshToken function (and even your checkScope):

if (!scope) return done();

[lfk]

What is the appropiate course of action if no scope is solicited by the client though? Either the system could assign a default scope and indicate that in the returned payload, or an error message could be displayed. Currently, the decision-making is delegated to the model's saveScope method. (Note, also, that this whole method might well come to be merged into saveAccessToken),

[nunofgs]

That's a very good point. I agree.

Also, 👍 for merging this into saveAccessToken.

[nunofgs]

@lfk: Are you working on this PR? I'd like to give you a hand but I don't want to spend time implementing anything you've already done :)

[lfk]

@nunofgs: Absolutely, I'm still working on it. :) I'm awaiting some response on the above discussion before proceeding with any major changes (such as creating a default scope validation method), but there's still plenty to be done, and your help would be much appreciated. I suppose the easiest way to proceed here would be for you to fork ubilogix/node-oauth2-server and submit a PR to the feature-scope branch.

[thomseddon]

I'm thinking we roll this, full state support and a few other changes into a 3.0.0 release so I'd rather make the implementation correct rather than compromising for backwards compatability - so please go ahead with changing the signature for the getAccessToken etc. to include scope.

On internal representation, the spec specifies that scopes should be sent as a space separated string so we can't really make any more assumptions that that - I see you've implemented nested scopes (JSON body?) but I think trying to support that is outside the scope of the module, of course it would be relatively trivial to parse the scopes yourself...
If this was a common use case we could even offer a parseScope model method, the default implementation would just be function (scope) { return scope.split(' ') } but if you want to get fancy you could - does anyone else think this is needed?

Have I missed any other outstanding questions?

[nunofgs]

@thomseddon I agree that the getAccessToken method should be called with a space-delimited string. If the developer wants to store that as an array, they can go ahead and parse it themselves, i.e., the parseScope idea could be the way to go but IMHO is a bit too much to introduce now.

In fact, the more I think about it, the more I believe that scope should be a string. This library only intends to implement the spec, not make the choice of data type for every field. The spec says it is a string and that's where the responsibility of this library should end :)

[OllieJennings]

I agree with @nunofgs above, scope should be a string. In order for this library to maintain its portablity you shouldn't make any assumptions as you have no idea of how everyone will implement their scope setup.

Plus it takes no effort to parse a string of scopes if they are comma or space separated.

[nunofgs]

@lfk: I was in the middle of writing a PR (to your PR 😄) and came upon a snag. In the saveScope implementation you are returning a server_error instead of invalid_scope.

This means that when requesting a token, if you pass an invalid scope, the model has no chance to return a 400 error. It can only return an error which eventually gets mapped as a 503 server error. The only thing the model can do is ignore the invalid scope which will then generate a token anyway.

Also, the AuthCodeGrant middleware also passes the scope parameter to the saveAuthCode model function but it too can only cause a 503 server error.

So, right now I don't see a quick way of validating whether the scope is invalid or not, other than using the scope middleware.

Do you have any suggestions on how to proceed?

[lfk]

@nunofgs: Yeah, I encountered the same issue when I first wrote that part, and I couldn't really find a clean workaround. Nowhere else in the library are we handling "cases" of returned errors, so it would feel awkward to introduce something of the sort here.

The only thing I can think of is to have a dedicated model method, such as validateScope. I'm writing some more notes on this topic in the PR #149 discussion in a moment, including the saveAuthCode matter.

The scope middleware is utilized for access validation once a token has already been created, it does not influence how scopes are first processed and stored.

[nunofgs]

Yup, I felt the same pain. I had your PR ready to go but I'm holding off on submitting because I felt the need to create a new error architecture, exactly as you described.

I incorporated almost every change we discussed in this PR, with the exception of passing scope to the saveAccessToken() function since it prevents me from throwing an "invalid scope" error, as I described previously. I also added tests for all of the new functionality.

You can take a look at my commits here: https://github.com/seegno-forks/node-oauth2-server/commits/feature-scope but I'll hold off on submitting it until @thomseddon weighs in on this.

[lfk]

I think you solved the aforementioned issue in a very neat fashion here. 👍
Regardless of how the scope middleware matter turns out, I'd definitely like to incorporate these fixes into the PR.

[thomseddon]

Looking good!!

Two remaining things:

1. Remove remaining references to saveScope
2. Resolve the issues highlighted by @nunofgs regarding how scope is returned with refresh tokens in the postgres reference implementation: https://github.com/thomseddon/node-oauth2-server/pull/149/files#r24001262

For 2 I prefer the first solution proposed by @nunofgs of adding a "scope" column to the refresh_tokens table?

[lfk]

Since saveRefreshToken does not pass the scope string to the model, it'll be necessary to query for the access token's scope string regardless; not too pretty.

Other than keeping the model methods lean, is there any reason we don't run generateAccessToken, then generateRefreshToken, and then a combined saveTokens method rather than AT, RT? Perhaps the convenience of having the scope string immediately available for both tokens here would take precedence? Thoughts?

[lfk]

This currently always assigns undefined. I see two possibilities here: either the client is allowed to solicit scopes on its own behalf when issuing the client_credentials grant request, or the model's getClient will need to include a scope string (as proposed previously by @nunofgs). Is it sound to let the client request its own scope? Any input here?

[thomseddon]

I think this should be done on a per request basis and so should be read from the request?

[thomseddon]

I think my preference would be to just add the scope to the saveRefreshToken model method?

[ccamarat]

Question - when issuing access tokens, how does one filter scopes that are granted to the user who's logging in? If the scopes 'readonly', 'edit', 'admin' are valid for my app, how do I use this PR to allow me to selectively issue only the "admin" scope to certain users?

I would have expected the authorization server to check which scopes the user is authorized for in the client's context. The updated samples have the client passing in scope to the /token endpoint, which seems to indicate they already know which scopes a user should have, and the auth server simply validates that the requested scopes are whitelisted. It seems to me that both clientId and user should be passed to validateScope to ensure the proper scopes are issued.

...unless of course I'm missing something completely, which wouldn't be unprecedented. Much appreciated.

[ChrisTerBeke]

Scopes are pretty handy, yes. I found a way to implement them without having to change this library:

OAuthAccessTokensSchema.static('saveAccessToken', function(token, clientId, expires, userId, next) {
    APP.models.OAuthClientsModel.findOne({ clientId: clientId }, function(err, oauthClient) {
        if (err) return next(err);
        if (!oauthClient) return next(new APP.errors.NotFound('Oauth cient not found'));

        var fields = {
            clientId: clientId,
            userId: userId,
            expires: expires,
            scopes: oauthClient.scopes
        };

        OAuthAccessTokensModel.update({ accessToken: token }, fields, { upsert: true }, function(err) {
            if (err) {
                console.error(err);
            }

            next(err);
        });
    });
});

And a piece of middleware to check:

module.exports = function(scope) {
    return function(req, res, next) {
        var errors  = APP.errors;
        if(req.oauth.bearerToken) {
            if(typeof scope === 'string') {
                if(req.oauth.bearerToken.scopes.indexOf(scope) >= 0) {
                    next();
                }
                else {
                    next(new errors.Unauthorized('Scope ' + scope + ' needed to access this resource'));
                }
            }
            else if(Array.isArray(scope)) {
                for(value in scope) {
                    if(req.oauth.bearerToken.scopes.indexOf(scope[value]) === -1) {
                        next(new errors.Unauthorized('Scopes ' + scope.toString() + ' needed to access this resource'));
                    }
                }
                next();
            }
            else {
                next(new errors.Internal('Wrong scope format'));
            }
        }
        else {
            next(new errors.Unauthorized('No access token found'));
        }
    }
};

Feel free to reach out for some more details.

[ccamarat]

Out of curiosity, why not just pass in the scope?

[thomseddon]

I also agree: https://github.com/thomseddon/node-oauth2-server/pull/149#issuecomment-92426451 :)

[ccamarat]

Sweet. I've updated my PR with this change.

[facultymatt]

What is the status on this? Looks about complete, would be great to get this merged in!

[jack-guy]

+1 for a merge!

[fixe]

This was added in #203.

[jacoscaz]

@lfk is this branch stable? I could definitely use scopes while I wait for the #203 merge to go through.

[ccamarat]

@jacoscaz - I've been using it with postgresql in production since May and haven't noted any issues. #203 might be a better baseline though since it has more traction and obsoletes this PR.

[jacoscaz]

Thank you @ccamarat for your insight. I'll wait a couple of days more and then decide what to do.

[lfk]

@mjsalinger: This is now deprecated.

[ruimarinho]

This is now covered by the 3.0.0 rewrite. @lfk, please open a new issue if anything is missing from this feature.