From f0e70dd8a9dbb37e15a05b5d0bb981cd602e494e Mon Sep 17 00:00:00 2001 From: ebarault Date: Fri, 3 Feb 2017 21:53:27 +0100 Subject: [PATCH] Fix Role.isOwner() for multiple user models Fix `Role.isOwner()` to check both principalId and principalType. This fixes a bug where users from different User model were treated as owners as long as their user id was the same as owner's id. --- common/models/role.js | 80 ++++++++++++++-------- lib/access-context.js | 14 +++- test/multiple-user-principal-types.test.js | 76 +++++++++++++++----- test/role.test.js | 44 ++++++++++++ 4 files changed, 164 insertions(+), 50 deletions(-) diff --git a/common/models/role.js b/common/models/role.js index 8bdd4393..93a98998 100644 --- a/common/models/role.js +++ b/common/models/role.js @@ -179,8 +179,10 @@ module.exports = function(Role) { } var modelClass = context.model; var modelId = context.modelId; - var userId = context.getUserId(); - Role.isOwner(modelClass, modelId, userId, callback); + var user = context.getUser(); + var userId = user && user.id; + var principalType = user && user.principalType; + Role.isOwner(modelClass, modelId, userId, principalType, callback); }); function isUserClass(modelClass) { @@ -210,18 +212,26 @@ module.exports = function(Role) { * @param {Function} modelClass The model class * @param {*} modelId The model ID * @param {*} userId The user ID + * @param {String} principalType The user principalType (optional) * @callback {Function} [callback] The callback function * @param {String|Error} err The error string or object * @param {Boolean} isOwner True if the user is an owner. * @promise */ - Role.isOwner = function isOwner(modelClass, modelId, userId, callback) { + Role.isOwner = function isOwner(modelClass, modelId, userId, principalType, callback) { + if (!callback && typeof principalType === 'function') { + callback = principalType; + principalType = undefined; + } + principalType = principalType || Principal.USER; + assert(modelClass, 'Model class is required'); if (!callback) callback = utils.createPromiseCallback(); - debug('isOwner(): %s %s userId: %s', modelClass && modelClass.modelName, modelId, userId); + debug('isOwner(): %s %s userId: %s principalType: %s', + modelClass && modelClass.modelName, modelId, userId, principalType); - // No userId is present + // Return false if userId is missing if (!userId) { process.nextTick(function() { callback(null, false); @@ -231,44 +241,58 @@ module.exports = function(Role) { // Is the modelClass User or a subclass of User? if (isUserClass(modelClass)) { - process.nextTick(function() { - callback(null, matches(modelId, userId)); - }); + var userModelName = modelClass.modelName; + // matching ids is enough if principalType is USER or matches given user model name + if (principalType === Principal.USER || principalType === userModelName) { + process.nextTick(function() { + callback(null, matches(modelId, userId)); + }); + } return callback.promise; } modelClass.findById(modelId, function(err, inst) { if (err || !inst) { debug('Model not found for id %j', modelId); - if (callback) callback(err, false); - return; + return callback(err, false); } debug('Model found: %j', inst); + + // Historically, for principalType USER, we were resolving isOwner() + // as true if the model has "userId" or "owner" property matching + // id of the current user (principalId), even though there was no + // belongsTo relation set up. var ownerId = inst.userId || inst.owner; - // Ensure ownerId exists and is not a function/relation - if (ownerId && 'function' !== typeof ownerId) { - if (callback) callback(null, matches(ownerId, userId)); - return; - } else { - // Try to follow belongsTo - for (var r in modelClass.relations) { - var rel = modelClass.relations[r]; - if (rel.type === 'belongsTo' && isUserClass(rel.modelTo)) { - debug('Checking relation %s to %s: %j', r, rel.modelTo.modelName, rel); - inst[r](processRelatedUser); - return; - } - } - debug('No matching belongsTo relation found for model %j and user: %j', modelId, userId); - if (callback) callback(null, false); + if (principalType === Principal.USER && ownerId && 'function' !== typeof ownerId) { + return callback(null, matches(ownerId, userId)); } + // Try to follow belongsTo + for (var r in modelClass.relations) { + var rel = modelClass.relations[r]; + // relation should be belongsTo and target a User based class + var belongsToUser = rel.type === 'belongsTo' && isUserClass(rel.modelTo); + if (!belongsToUser) { + continue; + } + // checking related user + var userModelName = rel.modelTo.modelName; + if (principalType === Principal.USER || principalType === userModelName) { + debug('Checking relation %s to %s: %j', r, userModelName, rel); + inst[r](processRelatedUser); + return; + } + } + debug('No matching belongsTo relation found for model %j - user %j principalType %j', + modelId, userId, principalType); + callback(null, false); + function processRelatedUser(err, user) { if (!err && user) { debug('User found: %j', user.id); - if (callback) callback(null, matches(user.id, userId)); + callback(null, matches(user.id, userId)); } else { - if (callback) callback(err, false); + callback(err, false); } } }); diff --git a/lib/access-context.js b/lib/access-context.js index d3076a06..96bef34d 100644 --- a/lib/access-context.js +++ b/lib/access-context.js @@ -128,6 +128,15 @@ AccessContext.prototype.addPrincipal = function(principalType, principalId, prin * @returns {*} */ AccessContext.prototype.getUserId = function() { + var user = this.getUser(); + return user && user.id; +}; + +/** + * Get the user + * @returns {*} + */ +AccessContext.prototype.getUser = function() { var BaseUser = this.registry.getModel('User'); for (var i = 0; i < this.principals.length; i++) { var p = this.principals[i]; @@ -138,17 +147,16 @@ AccessContext.prototype.getUserId = function() { // the principalType must either be 'USER' if (p.type === Principal.USER) { - return p.id; + return {id: p.id, principalType: p.type}; } // or permit to resolve a valid user model var userModel = this.registry.findModel(p.type); if (!userModel) continue; if (userModel.prototype instanceof BaseUser) { - return p.id; + return {id: p.id, principalType: p.type}; } } - return null; }; /** diff --git a/test/multiple-user-principal-types.test.js b/test/multiple-user-principal-types.test.js index 9530e22e..0ff7b2e7 100644 --- a/test/multiple-user-principal-types.test.js +++ b/test/multiple-user-principal-types.test.js @@ -204,8 +204,8 @@ describe('Multiple users with custom principalType', function() { accessContext = new AccessContext({registry: OneUser.registry}); }); - describe('getUserId()', function() { - it('returns userId although principals contain non USER principals', + describe('getUser()', function() { + it('returns user although principals contain non USER principals', function() { return Promise.try(function() { addToAccessContext([ @@ -214,12 +214,15 @@ describe('Multiple users with custom principalType', function() { {type: Principal.SCOPE}, {type: OneUser.modelName, id: userFromOneModel.id}, ]); - var userId = accessContext.getUserId(); - expect(userId).to.equal(userFromOneModel.id); + var user = accessContext.getUser(); + expect(user).to.eql({ + id: userFromOneModel.id, + principalType: OneUser.modelName, + }); }); }); - it('returns userId although principals contain invalid principals', + it('returns user although principals contain invalid principals', function() { return Promise.try(function() { addToAccessContext([ @@ -227,8 +230,11 @@ describe('Multiple users with custom principalType', function() { {type: 'invalidModelName'}, {type: OneUser.modelName, id: userFromOneModel.id}, ]); - var userId = accessContext.getUserId(); - expect(userId).to.equal(userFromOneModel.id); + var user = accessContext.getUser(); + expect(user).to.eql({ + id: userFromOneModel.id, + principalType: OneUser.modelName, + }); }); }); @@ -238,8 +244,11 @@ describe('Multiple users with custom principalType', function() { return ThirdUser.create(commonCredentials) .then(function(userFromThirdModel) { accessContext.addPrincipal(ThirdUser.modelName, userFromThirdModel.id); - var userId = accessContext.getUserId(); - expect(userId).to.equal(userFromThirdModel.id); + var user = accessContext.getUser(); + expect(user).to.eql({ + id: userFromThirdModel.id, + principalType: ThirdUser.modelName, + }); }); }); }); @@ -373,17 +382,46 @@ describe('Multiple users with custom principalType', function() { return Album.create({name: 'album', userId: userFromOneModel.id}) .then(function(album) { - return Role.isInRole( - Role.OWNER, - { - principalType: OneUser.modelName, - principalId: userFromOneModel.id, - model: Album, - id: album.id, - }); + var validContext = { + principalType: OneUser.modelName, + principalId: userFromOneModel.id, + model: Album, + id: album.id, + }; + return Role.isInRole(Role.OWNER, validContext); }) - .then(function(isInRole) { - expect(isInRole).to.be.true(); + .then(function(isOwner) { + expect(isOwner).to.be.true(); + }); + }); + + it('expects OWNER to resolve false if owner has incorrect principalType', function() { + var Album = app.registry.createModel('Album', { + name: String, + userId: Number, + }, { + relations: { + user: { + type: 'belongsTo', + model: 'OneUser', + foreignKey: 'userId', + }, + }, + }); + app.model(Album, {dataSource: 'db'}); + + return Album.create({name: 'album', userId: userFromOneModel.id}) + .then(function(album) { + var invalidContext = { + principalType: AnotherUser.modelName, + principalId: userFromOneModel.id, + model: Album, + id: album.id, + }; + return Role.isInRole(Role.OWNER, invalidContext); + }) + .then(function(isOwner) { + expect(isOwner).to.be.false(); }); }); }); diff --git a/test/role.test.js b/test/role.test.js index dc13775e..8cacaecd 100644 --- a/test/role.test.js +++ b/test/role.test.js @@ -489,6 +489,50 @@ describe('role model', function() { }); }); + it('resolves OWNER via "userId" property with no relation', function() { + var Album = app.registry.createModel('Album', { + name: String, + userId: Number, + }); + app.model(Album, {dataSource: 'db'}); + + var user; + return User.create({email: 'test@example.com', password: 'pass'}) + .then(u => { + user = u; + return Album.create({name: 'Album 1', userId: user.id}); + }) + .then(album => { + return Role.isInRole(Role.OWNER, { + principalType: ACL.USER, principalId: user.id, + model: Album, id: album.id, + }); + }) + .then(isInRole => expect(isInRole).to.be.true()); + }); + + it('resolves OWNER via "owner" property with no relation', function() { + var Album = app.registry.createModel('Album', { + name: String, + owner: Number, + }); + app.model(Album, {dataSource: 'db'}); + + var user; + return User.create({email: 'test@example.com', password: 'pass'}) + .then(u => { + user = u; + return Album.create({name: 'Album 1', owner: user.id}); + }) + .then(album => { + return Role.isInRole(Role.OWNER, { + principalType: ACL.USER, principalId: user.id, + model: Album, id: album.id, + }); + }) + .then(isInRole => expect(isInRole).to.be.true()); + }); + describe('isMappedToRole', function() { var user, app, role;