From c22cdf6461c6f71857a98d4e8d68ef5f9aabf99f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Benjamin=20Kr=C3=B6ger?= Date: Mon, 29 Feb 2016 16:49:41 +0100 Subject: [PATCH 1/6] changes avail conditions for req.accessToken fixes #2106 --- server/middleware/token.js | 2 +- test/access-token.test.js | 23 ++++++++++++++++++++++- 2 files changed, 23 insertions(+), 2 deletions(-) diff --git a/server/middleware/token.js b/server/middleware/token.js index e80eb560b..4e3d06f9a 100644 --- a/server/middleware/token.js +++ b/server/middleware/token.js @@ -96,7 +96,7 @@ function token(options) { assert(typeof TokenModel === 'function', 'loopback.token() middleware requires a AccessToken model'); - if (req.accessToken !== undefined) { + if (req.accessToken && req.accessToken.id) { rewriteUserLiteral(req, currentUserLiteral); return next(); } diff --git a/test/access-token.test.js b/test/access-token.test.js index 9e57cba2a..f5a739b2f 100644 --- a/test/access-token.test.js +++ b/test/access-token.test.js @@ -65,7 +65,7 @@ describe('loopback.token(options)', function() { .end(done); }); - describe('populating req.toen from HTTP Basic Auth formatted authorization header', function() { + describe('populating req.token from HTTP Basic Auth formatted authorization header', function() { it('parses "standalone-token"', function(done) { var token = this.token.id; token = 'Basic ' + new Buffer(token).toString('base64'); @@ -199,6 +199,27 @@ describe('loopback.token(options)', function() { done(); }); }); + + it('should not skip when req.accessToken is falsy', function(done) { + var token = this.token; + app.use(function(req, res, next) { + req.accessToken = null; + next(); + }); + app.use(loopback.token({ model: Token })); + app.get('/', function(req, res, next) { + res.send(req.accessToken); + }); + + request(app).get('/') + .set('Authorization', token.id) + .expect(200) + .end(function(err, res) { + if (err) return done(err); + expect(res.body.userId).to.eql(token.userId); + done(); + }); + }); }); describe('AccessToken', function() { From 7d9520665232d9afeca4443a4898a26fe743b2b7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Benjamin=20Kr=C3=B6ger?= Date: Mon, 29 Feb 2016 18:18:05 +0100 Subject: [PATCH 2/6] add token middleware config properties As suggested by @bajtos, token middleware now as new options for loading multiple instances: * @property {Boolean} [enableDoublecheck] Execute middleware although an instance mounted earlier in the chain didn't find a token * @property {Boolean} [overwriteExistingToken] only has effect in combination with `enableDoublecheck`. If truthy, will allow to overwrite an existing accessToken. --- server/middleware/token.js | 24 ++++- test/access-token.test.js | 186 ++++++++++++++++++++++--------------- 2 files changed, 129 insertions(+), 81 deletions(-) diff --git a/server/middleware/token.js b/server/middleware/token.js index 4e3d06f9a..cd97894b5 100644 --- a/server/middleware/token.js +++ b/server/middleware/token.js @@ -21,7 +21,7 @@ function rewriteUserLiteral(req, currentUserLiteral) { var urlBeforeRewrite = req.url; req.url = req.url.replace( new RegExp('/' + currentUserLiteral + '(/|$|\\?)', 'g'), - '/' + req.accessToken.userId + '$1'); + '/' + req.accessToken.userId + '$1'); if (req.url !== urlBeforeRewrite) { debug('req.url has been rewritten from %s to %s', urlBeforeRewrite, req.url); @@ -62,6 +62,8 @@ function escapeRegExp(str) { * @property {Array} [headers] Array of header names. * @property {Array} [params] Array of param names. * @property {Boolean} [searchDefaultTokenKeys] Use the default search locations for Token in request + * @property {Boolean} [enableDoublecheck] Execute middleware although an instance mounted earlier in the chain didn't find a token + * @property {Boolean} [overwriteExistingToken] only has effect in combination with `enableDoublecheck`. If truthy, will allow to overwrite an existing accessToken. * @property {Function|String} [model] AccessToken model name or class to use. * @property {String} [currentUserLiteral] String literal for the current user. * @header loopback.token([options]) @@ -80,6 +82,9 @@ function token(options) { currentUserLiteral = escapeRegExp(currentUserLiteral); } + var enableDoublecheck = !!options.enableDoublecheck; + var overwriteExistingToken = !!options.overwriteExistingToken; + return function(req, res, next) { var app = req.app; var registry = app.registry; @@ -96,9 +101,20 @@ function token(options) { assert(typeof TokenModel === 'function', 'loopback.token() middleware requires a AccessToken model'); - if (req.accessToken && req.accessToken.id) { - rewriteUserLiteral(req, currentUserLiteral); - return next(); + if (req.accessToken !== undefined) { + if (!enableDoublecheck) { + // req.accessToken is defined already (might also be "null" or "false") and enableDoublecheck + // has not been set --> skip searching for credentials + rewriteUserLiteral(req, currentUserLiteral); + return next(); + } + if (req.accessToken.id && !overwriteExistingToken) { + // req.accessToken.id is defined, which means that some other middleware has identified a valid user. + // when overwriteExistingToken is not set to a truthy value, skip searching for credentials. + rewriteUserLiteral(req, currentUserLiteral); + return next(); + } + // continue normal operation (as if req.accessToken was undefined) } TokenModel.findForRequest(req, options, function(err, token) { req.accessToken = token || null; diff --git a/test/access-token.test.js b/test/access-token.test.js index f5a739b2f..7646eb34a 100644 --- a/test/access-token.test.js +++ b/test/access-token.test.js @@ -1,7 +1,7 @@ var loopback = require('../'); var extend = require('util')._extend; var Token = loopback.AccessToken.extend('MyToken'); -var ds = loopback.createDataSource({connector: loopback.Memory}); +var ds = loopback.createDataSource({ connector: loopback.Memory }); Token.attachTo(ds); var ACL = loopback.ACL; @@ -32,28 +32,27 @@ describe('loopback.token(options)', function() { }); it('should not search default keys when searchDefaultTokenKeys is false', - function(done) { - var tokenId = this.token.id; - var app = createTestApp( - this.token, - { token: { searchDefaultTokenKeys: false } }, - done); - var agent = request.agent(app); - - // Set the token cookie - agent.get('/token').expect(200).end(function(err, res) { - if (err) return done(err); - - // Make a request that sets the token in all places searched by default - agent.get('/check-access?access_token=' + tokenId) - .set('X-Access-Token', tokenId) - .set('authorization', tokenId) - // Expect 401 because there is no (non-default) place configured where - // the middleware should load the token from - .expect(401) - .end(done); + function(done) { + var tokenId = this.token.id; + var app = createTestApp( + this.token, { token: { searchDefaultTokenKeys: false } }, + done); + var agent = request.agent(app); + + // Set the token cookie + agent.get('/token').expect(200).end(function(err, res) { + if (err) return done(err); + + // Make a request that sets the token in all places searched by default + agent.get('/check-access?access_token=' + tokenId) + .set('X-Access-Token', tokenId) + .set('authorization', tokenId) + // Expect 401 because there is no (non-default) place configured where + // the middleware should load the token from + .expect(401) + .end(done); + }); }); - }); it('should populate req.token from an authorization header with bearer token', function(done) { var token = this.token.id; @@ -144,7 +143,7 @@ describe('loopback.token(options)', function() { .set('authorization', id) .end(function(err, res) { assert(!err); - assert.deepEqual(res.body, {userId: userId}); + assert.deepEqual(res.body, { userId: userId }); done(); }); }); @@ -159,7 +158,7 @@ describe('loopback.token(options)', function() { .set('authorization', id) .end(function(err, res) { assert(!err); - assert.deepEqual(res.body, {userId: userId, state: 1}); + assert.deepEqual(res.body, { userId: userId, state: 1 }); done(); }); }); @@ -174,51 +173,83 @@ describe('loopback.token(options)', function() { .set('authorization', id) .end(function(err, res) { assert(!err); - assert.deepEqual(res.body, {userId: userId, state: 1}); + assert.deepEqual(res.body, { userId: userId, state: 1 }); done(); }); }); - it('should skip when req.token is already present', function(done) { - var tokenStub = { id: 'stub id' }; - app.use(function(req, res, next) { - req.accessToken = tokenStub; - next(); - }); - app.use(loopback.token({ model: Token })); - app.get('/', function(req, res, next) { - res.send(req.accessToken); + describe('loading multiple instances of token middleware', function() { + it('should skip when req.token is already present and no further options are set', function(done) { + var tokenStub = { id: 'stub id' }; + app.use(function(req, res, next) { + req.accessToken = tokenStub; + next(); + }); + app.use(loopback.token({ model: Token })); + app.get('/', function(req, res, next) { + res.send(req.accessToken); + }); + + request(app).get('/') + .set('Authorization', this.token.id) + .expect(200) + .end(function(err, res) { + if (err) return done(err); + expect(res.body).to.eql(tokenStub); + done(); + }); }); - request(app).get('/') - .set('Authorization', this.token.id) - .expect(200) - .end(function(err, res) { - if (err) return done(err); - expect(res.body).to.eql(tokenStub); - done(); + it('should not overwrite valid existing token (has "id" property) when overwriteExistingToken is falsy', function(done) { + var tokenStub = { id: 'stub id' }; + app.use(function(req, res, next) { + req.accessToken = tokenStub; + next(); + }); + app.use(loopback.token({ + model: Token, + enableDoublecheck: true, + })); + app.get('/', function(req, res, next) { + res.send(req.accessToken); }); - }); - it('should not skip when req.accessToken is falsy', function(done) { - var token = this.token; - app.use(function(req, res, next) { - req.accessToken = null; - next(); - }); - app.use(loopback.token({ model: Token })); - app.get('/', function(req, res, next) { - res.send(req.accessToken); + request(app).get('/') + .set('Authorization', this.token.id) + .expect(200) + .end(function(err, res) { + if (err) return done(err); + expect(res.body).to.eql(tokenStub); + done(); + }); }); - request(app).get('/') - .set('Authorization', token.id) - .expect(200) - .end(function(err, res) { - if (err) return done(err); - expect(res.body.userId).to.eql(token.userId); - done(); + it('should overwrite existing token when enableDoublecheck and overwriteExistingToken options are truthy', function(done) { + var token = this.token; + var tokenStub = { id: 'stub id' }; + + app.use(function(req, res, next) { + req.accessToken = tokenStub; + next(); }); + app.use(loopback.token({ + model: Token, + enableDoublecheck: true, + overwriteExistingToken: true + })); + app.get('/', function(req, res, next) { + res.send(req.accessToken); + }); + + request(app).get('/') + .set('Authorization', token.id) + .expect(200) + .end(function(err, res) { + if (err) return done(err); + expect(res.body.userId).to.eql(token.userId); + done(); + }); + }); }); }); @@ -259,16 +290,19 @@ describe('AccessToken', function() { }); function mockRequest(opts) { - return extend( - { + return extend({ method: 'GET', url: '/a-test-path', headers: {}, _params: {}, // express helpers - param: function(name) { return this._params[name]; }, - header: function(name) { return this.headers[name]; } + param: function(name) { + return this._params[name]; + }, + header: function(name) { + return this.headers[name]; + } }, opts); } @@ -295,7 +329,7 @@ describe('app.enableAuth()', function() { }); it('prevent remote call with app setting status on denied ACL', function(done) { - createTestAppAndRequest(this.token, {app:{aclErrorStatus:403}}, done) + createTestAppAndRequest(this.token, { app: { aclErrorStatus: 403 } }, done) .del('/tests/123') .expect(403) .set('authorization', this.token.id) @@ -311,7 +345,7 @@ describe('app.enableAuth()', function() { }); it('prevent remote call with app setting status on denied ACL', function(done) { - createTestAppAndRequest(this.token, {model:{aclErrorStatus:404}}, done) + createTestAppAndRequest(this.token, { model: { aclErrorStatus: 404 } }, done) .del('/tests/123') .expect(404) .set('authorization', this.token.id) @@ -376,7 +410,7 @@ describe('app.enableAuth()', function() { function createTestingToken(done) { var test = this; - Token.create({userId: '123'}, function(err, token) { + Token.create({ userId: '123' }, function(err, token) { if (err) return done(err); test.token = token; done(); @@ -405,8 +439,8 @@ function createTestApp(testToken, settings, done) { app.use(loopback.cookieParser('secret')); app.use(loopback.token(tokenSettings)); app.get('/token', function(req, res) { - res.cookie('authorization', testToken.id, {signed: true}); - res.cookie('access_token', testToken.id, {signed: true}); + res.cookie('authorization', testToken.id, { signed: true }); + res.cookie('access_token', testToken.id, { signed: true }); res.end(); }); app.get('/', function(req, res) { @@ -422,7 +456,7 @@ function createTestApp(testToken, settings, done) { res.status(req.accessToken ? 200 : 401).end(); }); app.use('/users/:uid', function(req, res) { - var result = {userId: req.params.uid}; + var result = { userId: req.params.uid }; if (req.query.state) { result.state = req.query.state; } else if (req.url !== '/') { @@ -438,15 +472,13 @@ function createTestApp(testToken, settings, done) { }); var modelOptions = { - acls: [ - { - principalType: 'ROLE', - principalId: '$everyone', - accessType: ACL.ALL, - permission: ACL.DENY, - property: 'deleteById' - } - ] + acls: [{ + principalType: 'ROLE', + principalId: '$everyone', + accessType: ACL.ALL, + permission: ACL.DENY, + property: 'deleteById' + }] }; Object.keys(modelSettings).forEach(function(key) { From de528a70f142796a200e0bf23dc2cf1d605d2d1b Mon Sep 17 00:00:00 2001 From: Benjamin Kroeger Date: Tue, 29 Mar 2016 09:31:24 +0200 Subject: [PATCH 3/6] revert spacing changes --- test/access-token.test.js | 109 +++++++++++++++++++++++--------------- 1 file changed, 65 insertions(+), 44 deletions(-) diff --git a/test/access-token.test.js b/test/access-token.test.js index 7646eb34a..99c3fb961 100644 --- a/test/access-token.test.js +++ b/test/access-token.test.js @@ -1,7 +1,7 @@ var loopback = require('../'); var extend = require('util')._extend; var Token = loopback.AccessToken.extend('MyToken'); -var ds = loopback.createDataSource({ connector: loopback.Memory }); +var ds = loopback.createDataSource({connector: loopback.Memory}); Token.attachTo(ds); var ACL = loopback.ACL; @@ -32,27 +32,28 @@ describe('loopback.token(options)', function() { }); it('should not search default keys when searchDefaultTokenKeys is false', - function(done) { - var tokenId = this.token.id; - var app = createTestApp( - this.token, { token: { searchDefaultTokenKeys: false } }, - done); - var agent = request.agent(app); - - // Set the token cookie - agent.get('/token').expect(200).end(function(err, res) { - if (err) return done(err); - - // Make a request that sets the token in all places searched by default - agent.get('/check-access?access_token=' + tokenId) - .set('X-Access-Token', tokenId) - .set('authorization', tokenId) - // Expect 401 because there is no (non-default) place configured where - // the middleware should load the token from - .expect(401) - .end(done); - }); + function(done) { + var tokenId = this.token.id; + var app = createTestApp( + this.token, + { token: { searchDefaultTokenKeys: false } }, + done); + var agent = request.agent(app); + + // Set the token cookie + agent.get('/token').expect(200).end(function(err, res) { + if (err) return done(err); + + // Make a request that sets the token in all places searched by default + agent.get('/check-access?access_token=' + tokenId) + .set('X-Access-Token', tokenId) + .set('authorization', tokenId) + // Expect 401 because there is no (non-default) place configured where + // the middleware should load the token from + .expect(401) + .end(done); }); + }); it('should populate req.token from an authorization header with bearer token', function(done) { var token = this.token.id; @@ -143,7 +144,7 @@ describe('loopback.token(options)', function() { .set('authorization', id) .end(function(err, res) { assert(!err); - assert.deepEqual(res.body, { userId: userId }); + assert.deepEqual(res.body, {userId: userId}); done(); }); }); @@ -158,7 +159,7 @@ describe('loopback.token(options)', function() { .set('authorization', id) .end(function(err, res) { assert(!err); - assert.deepEqual(res.body, { userId: userId, state: 1 }); + assert.deepEqual(res.body, {userId: userId, state: 1}); done(); }); }); @@ -173,11 +174,32 @@ describe('loopback.token(options)', function() { .set('authorization', id) .end(function(err, res) { assert(!err); - assert.deepEqual(res.body, { userId: userId, state: 1 }); + assert.deepEqual(res.body, {userId: userId, state: 1}); done(); }); }); + it('should skip when req.token is already present', function(done) { + var tokenStub = { id: 'stub id' }; + app.use(function(req, res, next) { + req.accessToken = tokenStub; + next(); + }); + app.use(loopback.token({ model: Token })); + app.get('/', function(req, res, next) { + res.send(req.accessToken); + }); + + request(app).get('/') + .set('Authorization', this.token.id) + .expect(200) + .end(function(err, res) { + if (err) return done(err); + expect(res.body).to.eql(tokenStub); + done(); + }); + }); + describe('loading multiple instances of token middleware', function() { it('should skip when req.token is already present and no further options are set', function(done) { var tokenStub = { id: 'stub id' }; @@ -290,19 +312,16 @@ describe('AccessToken', function() { }); function mockRequest(opts) { - return extend({ + return extend( + { method: 'GET', url: '/a-test-path', headers: {}, _params: {}, // express helpers - param: function(name) { - return this._params[name]; - }, - header: function(name) { - return this.headers[name]; - } + param: function(name) { return this._params[name]; }, + header: function(name) { return this.headers[name]; } }, opts); } @@ -329,7 +348,7 @@ describe('app.enableAuth()', function() { }); it('prevent remote call with app setting status on denied ACL', function(done) { - createTestAppAndRequest(this.token, { app: { aclErrorStatus: 403 } }, done) + createTestAppAndRequest(this.token, {app:{aclErrorStatus:403}}, done) .del('/tests/123') .expect(403) .set('authorization', this.token.id) @@ -345,7 +364,7 @@ describe('app.enableAuth()', function() { }); it('prevent remote call with app setting status on denied ACL', function(done) { - createTestAppAndRequest(this.token, { model: { aclErrorStatus: 404 } }, done) + createTestAppAndRequest(this.token, {model:{aclErrorStatus:404}}, done) .del('/tests/123') .expect(404) .set('authorization', this.token.id) @@ -410,7 +429,7 @@ describe('app.enableAuth()', function() { function createTestingToken(done) { var test = this; - Token.create({ userId: '123' }, function(err, token) { + Token.create({userId: '123'}, function(err, token) { if (err) return done(err); test.token = token; done(); @@ -439,8 +458,8 @@ function createTestApp(testToken, settings, done) { app.use(loopback.cookieParser('secret')); app.use(loopback.token(tokenSettings)); app.get('/token', function(req, res) { - res.cookie('authorization', testToken.id, { signed: true }); - res.cookie('access_token', testToken.id, { signed: true }); + res.cookie('authorization', testToken.id, {signed: true}); + res.cookie('access_token', testToken.id, {signed: true}); res.end(); }); app.get('/', function(req, res) { @@ -456,7 +475,7 @@ function createTestApp(testToken, settings, done) { res.status(req.accessToken ? 200 : 401).end(); }); app.use('/users/:uid', function(req, res) { - var result = { userId: req.params.uid }; + var result = {userId: req.params.uid}; if (req.query.state) { result.state = req.query.state; } else if (req.url !== '/') { @@ -472,13 +491,15 @@ function createTestApp(testToken, settings, done) { }); var modelOptions = { - acls: [{ - principalType: 'ROLE', - principalId: '$everyone', - accessType: ACL.ALL, - permission: ACL.DENY, - property: 'deleteById' - }] + acls: [ + { + principalType: 'ROLE', + principalId: '$everyone', + accessType: ACL.ALL, + permission: ACL.DENY, + property: 'deleteById' + } + ] }; Object.keys(modelSettings).forEach(function(key) { From 632b8aba8219a63b79f50ed82ef2b2dedcecd753 Mon Sep 17 00:00:00 2001 From: Benjamin Kroeger Date: Tue, 29 Mar 2016 09:33:40 +0200 Subject: [PATCH 4/6] revert spacing change in middleware/token.js --- server/middleware/token.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/middleware/token.js b/server/middleware/token.js index cd97894b5..b093ad57f 100644 --- a/server/middleware/token.js +++ b/server/middleware/token.js @@ -21,7 +21,7 @@ function rewriteUserLiteral(req, currentUserLiteral) { var urlBeforeRewrite = req.url; req.url = req.url.replace( new RegExp('/' + currentUserLiteral + '(/|$|\\?)', 'g'), - '/' + req.accessToken.userId + '$1'); + '/' + req.accessToken.userId + '$1'); if (req.url !== urlBeforeRewrite) { debug('req.url has been rewritten from %s to %s', urlBeforeRewrite, req.url); From 4b89df30bf3c79b3ea0df769e406edb2016374f0 Mon Sep 17 00:00:00 2001 From: Benjamin Kroeger Date: Wed, 30 Mar 2016 10:13:18 +0200 Subject: [PATCH 5/6] apply better verification of response token include all valid (non-private) properties in the test --- test/access-token.test.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/access-token.test.js b/test/access-token.test.js index 99c3fb961..829571b29 100644 --- a/test/access-token.test.js +++ b/test/access-token.test.js @@ -268,7 +268,12 @@ describe('loopback.token(options)', function() { .expect(200) .end(function(err, res) { if (err) return done(err); - expect(res.body.userId).to.eql(token.userId); + expect(res.body).to.eql({ + id: token.id, + ttl: token.ttl, + userId: token.userId, + created: token.created.toJSON() + }); done(); }); }); From 7f35f21a541a2c4465034c153e4a47b1ad16bdd4 Mon Sep 17 00:00:00 2001 From: Benjamin Kroeger Date: Wed, 6 Apr 2016 15:09:38 +0200 Subject: [PATCH 6/6] allow req.accessToken to be null prevents "can not read property id of null" --- server/middleware/token.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/middleware/token.js b/server/middleware/token.js index b093ad57f..146c75d17 100644 --- a/server/middleware/token.js +++ b/server/middleware/token.js @@ -108,7 +108,7 @@ function token(options) { rewriteUserLiteral(req, currentUserLiteral); return next(); } - if (req.accessToken.id && !overwriteExistingToken) { + if (req.accessToken && req.accessToken.id && !overwriteExistingToken) { // req.accessToken.id is defined, which means that some other middleware has identified a valid user. // when overwriteExistingToken is not set to a truthy value, skip searching for credentials. rewriteUserLiteral(req, currentUserLiteral);