-
-
Notifications
You must be signed in to change notification settings - Fork 932
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
Added support for scopes #149
Changes from 6 commits
8ebe219
c199992
ddac89f
4e281d6
fd69c79
01450c3
eec2eed
ceb8fab
2aeffb0
97e17cb
28b55ac
0726291
92d4ea7
bb1206c
036440f
eef7652
ed89b4e
715f4f7
0f4c59e
7512224
f04d32b
a073262
4cb4d8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,6 +75,12 @@ app.get('/secret', app.oauth.authorise(), function (req, res) { | |
res.send('Secret area'); | ||
}); | ||
|
||
app.get('/scoped', app.oauth.authorise(), app.oauth.scope('demo'), | ||
function (req, res) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is just personal preference but I would prefer removing the newline before |
||
// Will require that the access_token possesses the 'demo' scope key | ||
res.send('Secret and scope-controlled area'); | ||
}); | ||
|
||
app.get('/public', function (req, res) { | ||
// Does not require an access_token | ||
res.send('Public area'); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,7 +25,7 @@ var pg = require('pg'), | |
model.getAccessToken = function (bearerToken, callback) { | ||
pg.connect(connString, function (err, client, done) { | ||
if (err) return callback(err); | ||
client.query('SELECT access_token, client_id, expires, user_id FROM oauth_access_tokens ' + | ||
client.query('SELECT access_token, scope, client_id, expires, user_id FROM oauth_access_tokens ' + | ||
'WHERE access_token = $1', [bearerToken], function (err, result) { | ||
if (err || !result.rowCount) return callback(err); | ||
// This object will be exposed in req.oauth.token | ||
|
@@ -37,7 +37,8 @@ model.getAccessToken = function (bearerToken, callback) { | |
accessToken: token.access_token, | ||
clientId: token.client_id, | ||
expires: token.expires, | ||
userId: token.userId | ||
userId: token.userId, | ||
scope: token.scope.split(' ') // Assumes a flat, space-separated scope string | ||
}); | ||
done(); | ||
}); | ||
|
@@ -69,12 +70,14 @@ model.getClient = function (clientId, clientSecret, callback) { | |
model.getRefreshToken = function (bearerToken, callback) { | ||
pg.connect(connString, function (err, client, done) { | ||
if (err) return callback(err); | ||
client.query('SELECT refresh_token, client_id, expires, user_id FROM oauth_refresh_tokens ' + | ||
'WHERE refresh_token = $1', [bearerToken], function (err, result) { | ||
// The returned user_id will be exposed in req.user.id | ||
callback(err, result.rowCount ? result.rows[0] : false); | ||
done(); | ||
}); | ||
// Note: To avoid replicating the scope string in both token tables, the old | ||
// access token's scope string must be retrieved and passed along from here. | ||
client.query('SELECT rt.refresh_token, rt.client_id, rt.expires, rt.user_id, at.scope FROM ' + | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This assumes that expired I see two possible solutions for this:
@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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One more note. The OAuth RFC 6749 states in section 6 that when using the However, that implementation has been left to the model in this PR. Should this be something that node-oauth2-server is responsible for handling? |
||
'oauth_refresh_tokens AS rt, oauth_access_tokens AS at WHERE rt.user_id = ' + | ||
'at.user_id AND rt.client_id = at.client_id AND rt.refresh_token = $', | ||
[bearerToken], function (err, result) { | ||
callback(err, result.rowCount ? result.rows[0] : false); | ||
}); | ||
}); | ||
}; | ||
|
||
|
@@ -113,6 +116,34 @@ model.saveRefreshToken = function (refreshToken, clientId, expires, userId, call | |
}); | ||
}; | ||
|
||
model.saveScope = function (scope, accessToken, callback) { | ||
// Here you will want to validate that what the client is soliciting | ||
// makes sense. You might then proceed by storing the validated scope. | ||
// In this example, the scope is simply stored as a string in the | ||
// oauth_access_tokens table, but you could also handle them as entries | ||
// in a connection table. | ||
var acceptedScope = scope; | ||
|
||
pg.connect(connString, function (err, client, done) { | ||
if (err) return callback(err); | ||
client.query('UPDATE oauth_access_tokens SET scope=$1 WHERE access_token = $2', | ||
[acceptedScope, accessToken], function (err, result) { | ||
callback(err, acceptedScope); | ||
done(); | ||
}); | ||
}; | ||
|
||
model.checkScope = function (accessToken, requiredScope, callback) | ||
// requiredScope is set through the scope middleware. | ||
// You may pass anything from a simple string, as this example illustrates, | ||
// to representations including scopes and subscopes such as | ||
// { "account": [ "edit" ] } | ||
if(accessToken.scope.indexOf(requiredScope) === -1) { | ||
return callback('Required scope: ' + requiredScope); | ||
} | ||
callback(); | ||
}; | ||
|
||
/* | ||
* Required to support password grant type | ||
*/ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -32,15 +32,17 @@ var fns = [ | |
/** | ||
* Authorise | ||
* | ||
* @param {Object} config Instance of OAuth object | ||
* @param {Object} config Instance of OAuth object | ||
* @param {Object} req | ||
* @param {Object} res | ||
* @param {Object} options May indicate required scope(s) | ||
* @param {Function} next | ||
*/ | ||
function Authorise (config, req, next) { | ||
function Authorise (config, req, scope, next) { | ||
this.config = config; | ||
this.model = config.model; | ||
this.req = req; | ||
this.scope = scope; | ||
|
||
runner(fns, this, next); | ||
} | ||
|
@@ -125,6 +127,13 @@ function checkToken (done) { | |
self.req.oauth = { bearerToken: token }; | ||
self.req.user = token.user ? token.user : { id: token.userId }; | ||
|
||
done(); | ||
if(self.scope) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: change this to match the (proposed) if (!scope) return done(); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, this is definitely cleaner. |
||
self.model.checkScope(self.scope, token, function (err) { | ||
if (err) { return done(new error('invalid_scope', err)); } | ||
done(); | ||
}); | ||
} else { | ||
done(); | ||
} | ||
}); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,6 +36,7 @@ var fns = [ | |
saveAccessToken, | ||
generateRefreshToken, | ||
saveRefreshToken, | ||
saveScope, | ||
sendResponse | ||
]; | ||
|
||
|
@@ -193,6 +194,7 @@ function useAuthCodeGrant (done) { | |
} | ||
|
||
self.user = authCode.user || { id: authCode.userId }; | ||
self.scope = authCode.scope || ''; | ||
if (!self.user.id) { | ||
return done(error('server_error', false, | ||
'No user/userId parameter returned from getauthCode')); | ||
|
@@ -257,6 +259,7 @@ function useRefreshTokenGrant (done) { | |
} | ||
|
||
self.user = refreshToken.user || { id: refreshToken.userId }; | ||
self.scope = refreshToken.scope || ''; | ||
|
||
if (self.model.revokeRefreshToken) { | ||
return self.model.revokeRefreshToken(token, function (err) { | ||
|
@@ -449,14 +452,32 @@ function saveRefreshToken (done) { | |
} | ||
|
||
/** | ||
* Create an access token and save it with the model | ||
* Pass the scope string to the model for saving | ||
* | ||
* @param {Function} done | ||
* @this OAuth | ||
*/ | ||
function saveScope (done) { | ||
var scope = this.scope || this.req.body.scope; | ||
|
||
var self = this; | ||
this.model.saveScope(scope, self.accessToken, function (err, acceptedScope) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The saveScope function will be called on the model even if the user did not send a scope. In fact, scope here will be I suggest following the pattern set by the if (!scope) return done(); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a very good point. I agree. Also, 👍 for merging this into |
||
if (err) return done(error('server_error', false, err)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @lfk: I was in the middle of writing a PR (to your PR 😄) and came upon a snag. In the 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 So, right now I don't see a quick way of validating whether the scope is invalid or not, other than using the Do you have any suggestions on how to proceed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you solved the aforementioned issue in a very neat fashion here. 👍 |
||
self.scope = acceptedScope; | ||
done(); | ||
}); | ||
} | ||
|
||
/** | ||
* Sends the resulting token(s) and related information to the client | ||
* | ||
* @param {Function} done | ||
* @this OAuth | ||
*/ | ||
function sendResponse (done) { | ||
var response = { | ||
token_type: 'bearer', | ||
scope: this.scope, | ||
access_token: this.accessToken | ||
}; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason why the order of parameters differs from
checkScope()
?