diff --git a/common/models/role.js b/common/models/role.js index 0eabe5af..e807afa8 100644 --- a/common/models/role.js +++ b/common/models/role.js @@ -222,6 +222,8 @@ module.exports = function(Role) { * @promise */ Role.isOwner = function isOwner(modelClass, modelId, userId, principalType, options, callback) { + var _this = this; + if (!callback && typeof options === 'function') { callback = options; options = {}; @@ -238,7 +240,7 @@ module.exports = function(Role) { debug('isOwner(): %s %s userId: %s principalType: %s', modelClass && modelClass.modelName, modelId, userId, principalType); - // Return false if userId is missing + // Resolve isOwner false if userId is missing if (!userId) { process.nextTick(function() { callback(null, false); @@ -246,6 +248,23 @@ module.exports = function(Role) { return callback.promise; } + // At this stage, principalType is valid in one of 2 following condition: + // 1. the app has a single user model and principalType is 'USER' + // 2. the app has multiple user models and principalType is not 'USER' + // multiple user models + var isMultipleUsers = _isMultipleUsers(); + var isPrincipalTypeValid = + (!isMultipleUsers && principalType === Principal.USER) || + (isMultipleUsers && principalType !== Principal.USER); + + // Resolve isOwner false if principalType is invalid + if (!isPrincipalTypeValid) { + process.nextTick(function() { + callback(null, false); + }); + return callback.promise; + } + // Is the modelClass User or a subclass of User? if (isUserClass(modelClass)) { var userModelName = modelClass.modelName; @@ -265,10 +284,25 @@ module.exports = function(Role) { } 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 ownerRelations = modelClass.settings.ownerRelations; + if (!ownerRelations) { + return legacyOwnershipCheck(inst); + } else { + return checkOwnership(inst); + } + }); + return callback.promise; + + // NOTE 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. + // Additionaly, the original implementation did not support the + // possibility for a model to have multiple related users: when + // testing belongsTo relations, the first related user failing the + // ownership check induced the whole isOwner() to resolve as false. + // This behaviour will be pruned at next LoopBack major release. + function legacyOwnershipCheck(inst) { var ownerId = inst.userId || inst.owner; if (principalType === Principal.USER && ownerId && 'function' !== typeof ownerId) { return callback(null, matches(ownerId, userId)); @@ -282,9 +316,16 @@ module.exports = function(Role) { if (!belongsToUser) { continue; } + // checking related user - var userModelName = rel.modelTo.modelName; - if (principalType === Principal.USER || principalType === userModelName) { + var relatedUser = rel.modelTo; + var userModelName = relatedUser.modelName; + var isMultipleUsers = _isMultipleUsers(relatedUser); + // a relation can be considered for isOwner resolution if: + // 1. the app has a single user model and principalType is 'USER' + // 2. the app has multiple user models and principalType is the related user model name + if ((!isMultipleUsers && principalType === Principal.USER) || + (isMultipleUsers && principalType === userModelName)) { debug('Checking relation %s to %s: %j', r, userModelName, rel); inst[r](processRelatedUser); return; @@ -295,15 +336,72 @@ module.exports = function(Role) { callback(null, false); function processRelatedUser(err, user) { - if (!err && user) { - debug('User found: %j', user.id); - callback(null, matches(user.id, userId)); - } else { - callback(err, false); + if (err || !user) return callback(err, false); + + debug('User found: %j', user.id); + callback(null, matches(user.id, userId)); + } + } + + function checkOwnership(inst) { + var ownerRelations = inst.constructor.settings.ownerRelations; + // collecting related users + var relWithUsers = []; + for (var r in modelClass.relations) { + var rel = modelClass.relations[r]; + // relation should be belongsTo and target a User based class + if (rel.type !== 'belongsTo' || !isUserClass(rel.modelTo)) { + continue; + } + + // checking related user + var relatedUser = rel.modelTo; + var userModelName = relatedUser.modelName; + var isMultipleUsers = _isMultipleUsers(relatedUser); + // a relation can be considered for isOwner resolution if: + // 1. the app has a single user model and principalType is 'USER' + // 2. the app has multiple user models and principalType is the related user model name + // In addition, if an array of relations if provided with the ownerRelations option, + // then the given relation name is further checked against this array + if ((!isMultipleUsers && principalType === Principal.USER) || + (isMultipleUsers && principalType === userModelName)) { + debug('Checking relation %s to %s: %j', r, userModelName, rel); + if (ownerRelations === true) { + relWithUsers.push(r); + } else if (Array.isArray(ownerRelations) && ownerRelations.indexOf(r) !== -1) { + relWithUsers.push(r); + } } } - }); - return callback.promise; + if (relWithUsers.length === 0) { + debug('No matching belongsTo relation found for model %j and user: %j principalType: %j', + modelId, userId, principalType); + return callback(null, false); + } + + // check related users: someSeries is used to avoid spamming the db + async.someSeries(relWithUsers, processRelation, callback); + + function processRelation(r, cb) { + inst[r](function processRelatedUser(err, user) { + if (err || !user) return cb(err, false); + + debug('User found: %j (through %j)', user.id, r); + cb(null, matches(user.id, userId)); + }); + } + } + + // A helper function to check if the app user config is multiple users or + // single user. It can be used with or without a reference user model. + // In case no user model is provided, we use the registry to get any of the + // user model by type. The relation with AccessToken is used to check + // if polymorphism is used, and thus if multiple users. + function _isMultipleUsers(userModel) { + var oneOfUserModels = userModel || _this.registry.getModelByType('User'); + var accessTokensRel = oneOfUserModels.relations.accessTokens; + return !!(accessTokensRel && accessTokensRel.polymorphic); + } }; Role.registerResolver(Role.AUTHENTICATED, function(role, context, callback) { diff --git a/lib/application.js b/lib/application.js index 5a304755..b597d497 100644 --- a/lib/application.js +++ b/lib/application.js @@ -458,6 +458,16 @@ app._verifyAuthModelRelations = function() { function verifyUserRelations(Model) { const hasManyTokens = Model.relations && Model.relations.accessTokens; + + // display a temp warning message for users using multiple users config + if (hasManyTokens.polymorphic) { + console.warn( + 'The app configuration follows the multiple user models setup ' + + 'as described in http://ibm.biz/setup-loopback-auth', + 'The built-in role resolver $owner is not currently compatible ' + + 'with this configuration and should not be used in production.'); + } + if (hasManyTokens) return; const relationsConfig = Model.settings.relations || {}; diff --git a/test/multiple-user-principal-types.test.js b/test/multiple-user-principal-types.test.js index 0ff7b2e7..8532d122 100644 --- a/test/multiple-user-principal-types.test.js +++ b/test/multiple-user-principal-types.test.js @@ -8,6 +8,7 @@ var expect = require('./helpers/expect'); var request = require('supertest'); var loopback = require('../'); var ctx = require('../lib/access-context'); +var extend = require('util')._extend; var AccessContext = ctx.AccessContext; var Principal = ctx.Principal; var Promise = require('bluebird'); @@ -22,6 +23,7 @@ describe('Multiple users with custom principalType', function() { beforeEach(function setupAppAndModels() { // create a local app object that does not share state with other tests app = loopback({localRegistry: true, loadBuiltinModels: true}); + app.set('_verifyAuthModelRelations', false); app.dataSource('db', {connector: 'memory'}); var userModelOptions = { @@ -350,37 +352,38 @@ describe('Multiple users with custom principalType', function() { }); }); - describe('built-in role resolver', function() { - it('supports AUTHENTICATED', function() { + describe('built-in role resolvers', function() { + it('supports $authenticated', function() { return Role.isInRole(Role.AUTHENTICATED, userOneBaseContext) .then(function(isInRole) { expect(isInRole).to.be.true(); }); }); - it('supports UNAUTHENTICATED', function() { + it('supports $unauthenticated', function() { return Role.isInRole(Role.UNAUTHENTICATED, userOneBaseContext) .then(function(isInRole) { expect(isInRole).to.be.false(); }); }); - it('supports OWNER', function() { - var Album = app.registry.createModel('Album', { - name: String, - userId: Number, - }, { - relations: { - user: { - type: 'belongsTo', - model: 'OneUser', - foreignKey: 'userId', + describe('$owner', function() { + it('supports legacy behavior with relations', function() { + var Album = app.registry.createModel('Album', { + name: String, + userId: Number, + }, { + relations: { + user: { + type: 'belongsTo', + model: 'OneUser', + foreignKey: 'userId', + }, }, - }, - }); - app.model(Album, {dataSource: 'db'}); + }); + app.model(Album, {dataSource: 'db'}); - return Album.create({name: 'album', userId: userFromOneModel.id}) + return Album.create({name: 'album', userId: userFromOneModel.id}) .then(function(album) { var validContext = { principalType: OneUser.modelName, @@ -393,37 +396,197 @@ describe('Multiple users with custom principalType', function() { .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}) + // With multiple users config, we cannot resolve a user based just on + // his id, as many users from different models could have the same id. + it('legacy behavior resolves false without belongsTo relation', function() { + var Album = app.registry.createModel('Album', { + name: String, + userId: Number, + owner: Number, + }); + app.model(Album, {dataSource: 'db'}); + + return Album.create({ + name: 'album', + userId: userFromOneModel.id, + owner: userFromOneModel.id, + }) .then(function(album) { - var invalidContext = { - principalType: AnotherUser.modelName, + var authContext = { + principalType: OneUser.modelName, principalId: userFromOneModel.id, model: Album, id: album.id, }; - return Role.isInRole(Role.OWNER, invalidContext); + return Role.isInRole(Role.OWNER, authContext); }) .then(function(isOwner) { expect(isOwner).to.be.false(); }); + }); + + it('legacy behavior resolves 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 invalidPrincipalTypes = [ + 'invalidContextName', + 'USER', + AnotherUser.modelName, + ]; + var invalidContexts = invalidPrincipalTypes.map(principalType => { + return { + principalType, + principalId: userFromOneModel.id, + model: Album, + id: album.id, + }; + }); + return Promise.map(invalidContexts, context => { + return Role.isInRole(Role.OWNER, context) + .then(isOwner => { + return { + principalType: context.principalType, + isOwner, + }; + }); + }); + }) + .then(result => { + expect(result).to.eql([ + {principalType: 'invalidContextName', isOwner: false}, + {principalType: 'USER', isOwner: false}, + {principalType: AnotherUser.modelName, isOwner: false}, + ]); + }); + }); + + it.skip('resolves the owner using the corrent belongsTo relation', + function() { + // passing {ownerRelations: true} will enable the new $owner role resolver + // with any belongsTo relation allowing to resolve truthy + var Message = createModelWithOptions( + 'ModelWithAllRelations', + {ownerRelations: true} + ); + + var messages = [ + {content: 'firstMessage', customerId: userFromOneModel.id}, + { + content: 'secondMessage', + customerId: userFromOneModel.id, + shopKeeperId: userFromAnotherModel.id, + }, + + // this is the incriminated message where two foreignKeys have the + // same id but points towards two different user models. Although + // customers should come from userFromOneModel and shopKeeperIds should + // come from userFromAnotherModel. The inverted situation still resolves + // isOwner true for both the customer and the shopKeeper + { + content: 'thirdMessage', + customerId: userFromAnotherModel.id, + shopKeeperId: userFromOneModel.id, + }, + + {content: 'fourthMessage', customerId: userFromAnotherModel.id}, + {content: 'fifthMessage'}, + ]; + return Promise.map(messages, msg => { + return Message.create(msg); + }) + .then(messages => { + return Promise.all([ + isOwnerForMessage(userFromOneModel, messages[0]), + isOwnerForMessage(userFromAnotherModel, messages[0]), + isOwnerForMessage(userFromOneModel, messages[1]), + isOwnerForMessage(userFromAnotherModel, messages[1]), + + isOwnerForMessage(userFromOneModel, messages[2]), + isOwnerForMessage(userFromAnotherModel, messages[2]), + + isOwnerForMessage(userFromAnotherModel, messages[3]), + isOwnerForMessage(userFromOneModel, messages[4]), + ]); + }) + .then(result => { + expect(result).to.eql([ + {userFrom: 'OneUser', msg: 'firstMessage', isOwner: true}, + {userFrom: 'AnotherUser', msg: 'firstMessage', isOwner: false}, + {userFrom: 'OneUser', msg: 'secondMessage', isOwner: true}, + {userFrom: 'AnotherUser', msg: 'secondMessage', isOwner: true}, + + // these 2 tests fail because we cannot resolve ownership with + // multiple owners on a single model instance with a classic + // belongsTo relation, we need to use belongsTo with polymorphic + // discriminator to distinguish between the 2 models + {userFrom: 'OneUser', msg: 'thirdMessage', isOwner: false}, + {userFrom: 'AnotherUser', msg: 'thirdMessage', isOwner: false}, + + {userFrom: 'AnotherUser', msg: 'fourthMessage', isOwner: false}, + {userFrom: 'OneUser', msg: 'fifthMessage', isOwner: false}, + ]); + }); + }); }); + + // helpers + function isOwnerForMessage(user, msg) { + var accessContext = { + principalType: user.constructor.modelName, + principalId: user.id, + model: msg.constructor, + id: msg.id, + }; + return Role.isInRole(Role.OWNER, accessContext) + .then(isOwner => { + return { + userFrom: user.constructor.modelName, + msg: msg.content, + isOwner, + }; + }); + } + + function createModelWithOptions(name, options) { + var baseOptions = { + relations: { + sender: { + type: 'belongsTo', + model: 'OneUser', + foreignKey: 'customerId', + }, + receiver: { + type: 'belongsTo', + model: 'AnotherUser', + foreignKey: 'shopKeeperId', + }, + }, + }; + options = extend(baseOptions, options); + var Model = app.registry.createModel( + name, + {content: String, senderType: String}, + options + ); + app.model(Model, {dataSource: 'db'}); + return Model; + } }); describe('isMappedToRole()', function() { diff --git a/test/role.test.js b/test/role.test.js index cf8d01fc..37ef664b 100644 --- a/test/role.test.js +++ b/test/role.test.js @@ -8,6 +8,7 @@ var assert = require('assert'); var sinon = require('sinon'); var loopback = require('../index'); var async = require('async'); +var extend = require('util')._extend; var expect = require('./helpers/expect'); var Promise = require('bluebird'); @@ -363,7 +364,8 @@ describe('role model', function() { }); }); - it('should support owner role resolver', function(done) { + // this test should be split to address one resolver at a time + it('supports built-in role resolvers', function(done) { Role.registerResolver('returnPromise', function(role, context) { return new Promise(function(resolve) { process.nextTick(function() { @@ -489,48 +491,249 @@ 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'}); + describe('$owner role resolver', function() { + var sender, receiver; + var users = [ + {username: 'sender', email: 'sender@example.com', password: 'pass'}, + {username: 'receiver', email: 'receiver@example.com', password: 'pass'}, + ]; - var user; - return User.create({email: 'test@example.com', password: 'pass'}) + describe('ownerRelations not set (legacy behaviour)', () => { + it('resolves the owner via property "userId"', function() { + var user; + var Album = app.registry.createModel('Album', { + name: String, + userId: Number, + }); + app.model(Album, {dataSource: 'db'}); + + 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 the owner via property "owner"', function() { + var user; + var Album = app.registry.createModel('Album', { + name: String, + owner: Number, + }); + app.model(Album, {dataSource: 'db'}); + + 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()); + }); + + it('resolves the owner via a belongsTo relation', function() { + // passing no options will result calling + // the legacy $owner role resolver behavior + var Message = givenModelWithSenderReceiverRelations('ModelWithNoOptions'); + + return givenUsers() + .then(() => { + var messages = [ + {content: 'firstMessage', senderId: sender.id}, + {content: 'secondMessage', receiverId: receiver.id}, + {content: 'thirdMessage'}, + ]; + return Promise.map(messages, msg => { + return Message.create(msg); + }); + }) + .then(messages => { + return Promise.all([ + isOwnerForMessage(sender, messages[0]), + isOwnerForMessage(receiver, messages[1]), + isOwnerForMessage(receiver, messages[2]), + ]); + }) + .then(result => { + expect(result).to.eql([ + {user: 'sender', msg: 'firstMessage', isOwner: true}, + {user: 'receiver', msg: 'secondMessage', isOwner: false}, + {user: 'receiver', msg: 'thirdMessage', isOwner: false}, + ]); + }); + }); + }); + + it('resolves as false without belongsTo relation', function() { + var user; + var Album = app.registry.createModel( + 'Album', + { + name: String, + userId: Number, + owner: Number, + }, + // passing {ownerRelations: true} will enable the new $owner role resolver + // and hence resolve false when no belongsTo relation is defined + {ownerRelations: true} + ); + app.model(Album, {dataSource: 'db'}); + + return User.create({email: 'test@example.com', password: 'pass'}) .then(u => { user = u; - return Album.create({name: 'Album 1', userId: user.id}); + return Album.create({name: 'Album 1', userId: user.id, owner: user.id}); }) .then(album => { return Role.isInRole(Role.OWNER, { - principalType: ACL.USER, principalId: user.id, - model: Album, id: album.id, + 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, + .then(isInRole => expect(isInRole).to.be.false()); }); - 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, + it('resolves the owner using the corrent belongsTo relation', function() { + // passing {ownerRelations: true} will enable the new $owner role resolver + // with any belongsTo relation allowing to resolve truthy + var Message = givenModelWithSenderReceiverRelations( + 'ModelWithAllRelations', + {ownerRelations: true} + ); + + return givenUsers() + .then(() => { + var messages = [ + {content: 'firstMessage', senderId: sender.id}, + {content: 'secondMessage', receiverId: receiver.id}, + {content: 'thirdMessage'}, + ]; + return Promise.map(messages, msg => { + return Message.create(msg); }); }) - .then(isInRole => expect(isInRole).to.be.true()); + .then(messages => { + return Promise.all([ + isOwnerForMessage(sender, messages[0]), + isOwnerForMessage(receiver, messages[1]), + isOwnerForMessage(receiver, messages[2]), + ]); + }) + .then(result => { + expect(result).to.eql([ + {user: 'sender', msg: 'firstMessage', isOwner: true}, + {user: 'receiver', msg: 'secondMessage', isOwner: true}, + {user: 'receiver', msg: 'thirdMessage', isOwner: false}, + ]); + }); + }); + + it('allows fine-grained control of which relations grant ownership', + function() { + // passing {ownerRelations: true} will enable the new $owner role resolver + // with a specified list of belongsTo relations allowing to resolve truthy + var Message = givenModelWithSenderReceiverRelations( + 'ModelWithCoercedRelations', + {ownerRelations: ['receiver']} + ); + + return givenUsers() + .then(() => { + var messages = [ + {content: 'firstMessage', senderId: sender.id}, + {content: 'secondMessage', receiverId: receiver.id}, + {content: 'thirdMessage'}, + ]; + return Promise.map(messages, msg => { + return Message.create(msg); + }); + }) + .then(messages => { + return Promise.all([ + isOwnerForMessage(sender, messages[0]), + isOwnerForMessage(receiver, messages[1]), + isOwnerForMessage(receiver, messages[2]), + ]); + }) + .then(result => { + expect(result).to.eql([ + {user: 'sender', msg: 'firstMessage', isOwner: false}, + {user: 'receiver', msg: 'secondMessage', isOwner: true}, + {user: 'receiver', msg: 'thirdMessage', isOwner: false}, + ]); + }); + }); + + // helpers + function givenUsers() { + return Promise.map(users, user => { + return User.create(user); + }) + .then(users => { + sender = users[0]; + receiver = users[1]; + }); + } + + function isOwnerForMessage(user, msg) { + var accessContext = { + principalType: ACL.USER, + principalId: user.id, + model: msg.constructor, + id: msg.id, + }; + return Role.isInRole(Role.OWNER, accessContext) + .then(isOwner => { + return { + user: user.username, + msg: msg.content, + isOwner, + }; + }); + } + + function givenModelWithSenderReceiverRelations(name, options) { + var baseOptions = { + relations: { + sender: { + type: 'belongsTo', + model: 'User', + foreignKey: 'senderId', + }, + receiver: { + type: 'belongsTo', + model: 'User', + foreignKey: 'receiverId', + }, + }, + }; + options = extend(baseOptions, options); + var Model = app.registry.createModel( + name, + {content: String}, + options + ); + app.model(Model, {dataSource: 'db'}); + return Model; + } }); it('passes accessToken to modelClass.findById when resolving OWNER', () => {