From a885e9e84f854160459e6176cffb3e600fdc6b8f Mon Sep 17 00:00:00 2001 From: ebarault Date: Fri, 3 Feb 2017 21:53:27 +0100 Subject: [PATCH] Fix Role.isOwner() when multiple user models --- common/models/role.js | 55 ++++++++++++------ lib/access-context.js | 14 ++++- test/multiple-user-principal-types.test.js | 65 +++++++++++++++------- 3 files changed, 94 insertions(+), 40 deletions(-) diff --git a/common/models/role.js b/common/models/role.js index 8bdd43930..205dd38e8 100644 --- a/common/models/role.js +++ b/common/models/role.js @@ -179,8 +179,8 @@ 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(); + Role.isOwner(modelClass, modelId, user.id, user.principalType, callback); }); function isUserClass(modelClass) { @@ -210,16 +210,24 @@ 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 * @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 if (!userId) { @@ -231,9 +239,13 @@ 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; } @@ -244,31 +256,39 @@ module.exports = function(Role) { return; } debug('Model found: %j', inst); + + // loopback v2 implementation alows to resolve isOwner() if principalType is USER, + // instance ownerId (.userId or .owner) exists and is equal to user's id 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)); + if (principalType === Principal.USER && ownerId && 'function' !== typeof ownerId) { + 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); + // relation should be belongsTo and target a User based class + if (rel.type !== 'belongsTo' && !isUserClass(rel.modelTo)) { + 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 and user: %j', modelId, userId); - if (callback) callback(null, false); + debug('No matching belongsTo relation found for model %j and 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); } } }); @@ -371,7 +391,6 @@ module.exports = function(Role) { var inRole = context.principals.some(function(p) { var principalType = p.type || undefined; var principalId = p.id || undefined; - // Check if it's the same role return principalType === RoleMapping.ROLE && principalId === role; }); diff --git a/lib/access-context.js b/lib/access-context.js index d3076a065..63465759d 100644 --- a/lib/access-context.js +++ b/lib/access-context.js @@ -128,6 +128,14 @@ AccessContext.prototype.addPrincipal = function(principalType, principalId, prin * @returns {*} */ AccessContext.prototype.getUserId = function() { + return this.getUser().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 +146,17 @@ 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; + return {}; }; /** diff --git a/test/multiple-user-principal-types.test.js b/test/multiple-user-principal-types.test.js index 9530e22e2..323576793 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, + }); }); }); }); @@ -357,6 +366,7 @@ describe('Multiple users with custom principalType', function() { }); it('supports OWNER', function() { + var baseContext; var Album = app.registry.createModel('Album', { name: String, userId: Number, @@ -373,19 +383,36 @@ 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, - }); + baseContext = { + principalId: userFromOneModel.id, + model: Album, + id: album.id, + }; + var validContext = buildContextWith({principalType: OneUser.modelName}); + var invalidContext = buildContextWith({principalType: AnotherUser.modelName}); + + return isInRole([ + {role: Role.OWNER, context: validContext}, + {role: Role.OWNER, context: invalidContext}, + ]); }) - .then(function(isInRole) { - expect(isInRole).to.be.true(); + .then(function(data) { + expect(data).to.eql([true, false]); }); + + // helper + function buildContextWith(properties) { + return Object.assign(properties, baseContext); + } }); + + // helper + function isInRole(requests) { + var isInRoles = requests.map(function(request) { + return Role.isInRole(request.role, request.context); + }); + return Promise.all(isInRoles); + } }); describe('isMappedToRole()', function() {