Merge pull request #3140 from pierreclr/feature/allow-mutiple-owners-resolving

Fix OWNER role to handle multiple relations

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()
+      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) {
+        if (err || !user) return callback(err, false);
+
         debug('User found: %j', user.id);
         callback(null, matches(user.id, userId));
-        } else {
-          callback(err, false);
       }
     }
+
+    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);
+          }
+        }
+      }
+      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));
         });
-    return callback.promise;
+      }
+    }
+
+    // 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) {

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 || {};

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,22 +352,23 @@ 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() {
+      describe('$owner', function() {
+        it('supports legacy behavior with relations', function() {
           var Album = app.registry.createModel('Album', {
             name: String,
             userId: Number,
@@ -395,7 +398,36 @@ describe('Multiple users with custom principalType', function() {
           });
         });
 
-      it('expects OWNER to resolve false if owner has incorrect principalType', function() {
+        // 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 authContext = {
+              principalType: OneUser.modelName,
+              principalId: userFromOneModel.id,
+              model: Album,
+              id: album.id,
+            };
+            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,
@@ -412,18 +444,149 @@ describe('Multiple users with custom principalType', function() {
 
           return Album.create({name: 'album', userId: userFromOneModel.id})
           .then(function(album) {
-            var invalidContext = {
-              principalType: AnotherUser.modelName,
+            var invalidPrincipalTypes = [
+              'invalidContextName',
+              'USER',
+              AnotherUser.modelName,
+            ];
+            var invalidContexts = invalidPrincipalTypes.map(principalType => {
+              return {
+                principalType,
                 principalId: userFromOneModel.id,
                 model: Album,
                 id: album.id,
               };
-            return Role.isInRole(Role.OWNER, invalidContext);
+            });
+            return Promise.map(invalidContexts, context => {
+              return Role.isInRole(Role.OWNER, context)
+                .then(isOwner => {
+                  return {
+                    principalType: context.principalType,
+                    isOwner,
+                  };
+                });
+            });
           })
-          .then(function(isOwner) {
-            expect(isOwner).to.be.false();
+          .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() {

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,14 +491,22 @@ describe('role model', function() {
     });
   });
 
-  it('resolves OWNER via "userId" property with no relation', function() {
+  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'},
+    ];
+
+    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'});
 
-    var user;
         return User.create({email: 'test@example.com', password: 'pass'})
         .then(u => {
           user = u;
@@ -504,21 +514,23 @@ describe('role model', function() {
         })
         .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() {
+      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'});
 
-    var user;
         return User.create({email: 'test@example.com', password: 'pass'})
         .then(u => {
           user = u;
@@ -526,13 +538,204 @@ describe('role model', function() {
         })
         .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 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, 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.false());
+    });
+
+    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(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', () => {
     const Album = app.registry.createModel('Album', {name: String});
     app.model(Album, {dataSource: 'db'});