From b091ea28e47035648339bc6cc59f3479a66da55f Mon Sep 17 00:00:00 2001 From: pierreclr Date: Mon, 16 Jan 2017 14:05:37 +0100 Subject: [PATCH 1/5] NEW fetch belongsTo relation first before resolving role --- common/models/role.js | 54 ++++++++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/common/models/role.js b/common/models/role.js index 056388635..18dd7e518 100644 --- a/common/models/role.js +++ b/common/models/role.js @@ -171,13 +171,6 @@ module.exports = function(Role) { Role.isOwner = function isOwner(modelClass, modelId, userId, callback) { assert(modelClass, 'Model class is required'); debug('isOwner(): %s %s userId: %s', modelClass && modelClass.modelName, modelId, userId); - // No userId is present - if (!userId) { - process.nextTick(function() { - callback(null, false); - }); - return; - } // Is the modelClass User or a subclass of User? if (isUserClass(modelClass)) { @@ -193,34 +186,43 @@ module.exports = function(Role) { if (callback) callback(err, false); return; } - debug('Model found: %j', inst); - 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; - } + + // 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); + if (inst[r](processRelatedUser)) return callback(null, true); } + debug('No matching belongsTo relation found for model %j and user: %j', modelId, userId); - if (callback) callback(null, false); } function processRelatedUser(err, user) { if (!err && user) { debug('User found: %j', user.id); - if (callback) callback(null, matches(user.id, userId)); - } else { - if (callback) callback(err, false); + if (callback && matches(user.id, userId)) { + return true; + } + return false; } } + + // Checking the userId or owner field for resolving owner role + // is now done after fetching belongsTo relation to make possible + // to have multiple resolver role + var ownerId = inst.userId || inst.owner; + // Ensure ownerId exists and is not a function/relation + if (ownerId && 'function' !== typeof ownerId) { + console.log('\n', 'callback line 219', '\n'); + if (callback && matches(ownerId, userId)) callback(null, true); + return; + } + + // Finally DENY the owner role after sending + console.log('\n', 'callback line 225', '\n'); + if (callback) callback(null, false); }); }; From 39c68af04ba6795389cde634adb5a9028a9152d8 Mon Sep 17 00:00:00 2001 From: pierreclr Date: Mon, 16 Jan 2017 18:02:57 +0100 Subject: [PATCH 2/5] Remove console.log --- common/models/role.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/common/models/role.js b/common/models/role.js index 18dd7e518..0f6d21d4a 100644 --- a/common/models/role.js +++ b/common/models/role.js @@ -215,13 +215,11 @@ module.exports = function(Role) { var ownerId = inst.userId || inst.owner; // Ensure ownerId exists and is not a function/relation if (ownerId && 'function' !== typeof ownerId) { - console.log('\n', 'callback line 219', '\n'); if (callback && matches(ownerId, userId)) callback(null, true); return; } // Finally DENY the owner role after sending - console.log('\n', 'callback line 225', '\n'); if (callback) callback(null, false); }); }; From 0c1e15b121c9ad750a19fec9127c001fc9700047 Mon Sep 17 00:00:00 2001 From: pierreclr Date: Thu, 19 Jan 2017 09:59:41 +0100 Subject: [PATCH 3/5] NEW add unit test for handling multi owners role --- test/role.test.js | 54 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 53 insertions(+), 1 deletion(-) diff --git a/test/role.test.js b/test/role.test.js index d6eccf79f..e37ea14cd 100644 --- a/test/role.test.js +++ b/test/role.test.js @@ -15,7 +15,7 @@ function checkResult(err, result) { assert(!err); } -describe('role model', function() { +describe.only('role model', function() { this.timeout(10000); var app, Role, RoleMapping, User, Application, ACL; @@ -434,6 +434,58 @@ describe('role model', function() { }); }); + it('should be able to handle multiple owners roles', function(done) { + var Message = app.registry.createModel('Message', { + name: String, + userId: Number, + senderId: Number, + }, { + relations: { + user: { + type: 'belongsTo', + model: 'User', + foreignKey: 'userId', + }, + sender: { + type: 'belongsTo', + model: 'User', + foreignKey: 'senderId', + }, + }, + }); + + app.model(Message, {dataSource: 'db'}); + + User.create([ + {name: 'pierre', email: 'pierre@xyz.com', password: 'foobar'}, + {name: 'henry', email: 'henry@xyz.com', password: 'foobar'}, + ], function(err, users) { + if (err) return done(err); + Message.create([ + {name: 'message1', userId: users[0].id, senderId: users[1].id}, + {name: 'message2'}, + ], function(err, messages) { + var role1 = { + principalType: ACL.USER, principalId: users[0].id, + model: Message, id: messages[0].id, + }; + var role2 = { + principalType: ACL.USER, principalId: users[1].id, + model: Message, id: messages[0].id, + }; + Role.isInRole(Role.OWNER, role1, function(err, yes) { + if (err) return done(err); + assert(yes); + Role.isInRole(Role.OWNER, role2, function(err, yes) { + if (err) return done(err); + assert(yes); + done(); + }); + }); + }); + }); + }); + describe('isMappedToRole', function() { var user, app, role; From de8cc150117fb1e9d2b78a62494a6f86a8daf74e Mon Sep 17 00:00:00 2001 From: pierreclr Date: Thu, 19 Jan 2017 14:48:59 +0100 Subject: [PATCH 4/5] FIX deny owner resolve if no userId in request --- common/models/role.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/common/models/role.js b/common/models/role.js index 0f6d21d4a..722f9f641 100644 --- a/common/models/role.js +++ b/common/models/role.js @@ -172,6 +172,14 @@ module.exports = function(Role) { assert(modelClass, 'Model class is required'); debug('isOwner(): %s %s userId: %s', modelClass && modelClass.modelName, modelId, userId); + // If no requester id, deny the resolver + if (!userId) { + process.nextTick(function() { + callback(null, false); + }); + return; + } + // Is the modelClass User or a subclass of User? if (isUserClass(modelClass)) { process.nextTick(function() { From 8d68b85e5da980b70468f2c2614f38e09097f065 Mon Sep 17 00:00:00 2001 From: pierreclr Date: Thu, 19 Jan 2017 15:00:25 +0100 Subject: [PATCH 5/5] Remove describe only --- test/role.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/role.test.js b/test/role.test.js index e37ea14cd..a3de8cfd1 100644 --- a/test/role.test.js +++ b/test/role.test.js @@ -15,7 +15,7 @@ function checkResult(err, result) { assert(!err); } -describe.only('role model', function() { +describe('role model', function() { this.timeout(10000); var app, Role, RoleMapping, User, Application, ACL;