Merge pull request #3180 from ebarault/enable-multiple-user-models

Fix Role.isOwner() for multiple user models

common/models/role.js

@@ -179,8 +179,10 @@ module.exports = function(Role) {
     }
     var modelClass = context.model;
     var modelId = context.modelId;
-    var userId = context.getUserId();
+    var user = context.getUser();
-    Role.isOwner(modelClass, modelId, userId, callback);
+    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() {
+      var userModelName = modelClass.modelName;
-        callback(null, matches(modelId, userId));
+      // 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 callback(err, false);
-        return;
       }
       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 (principalType === Principal.USER && ownerId && 'function' !== typeof ownerId) {
-      if (ownerId && 'function' !== typeof ownerId) {
+        return callback(null, matches(ownerId, userId));
-        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);
       }
 
+      // 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);
         }
       }
     });

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;
 };
 
 /**

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() {
+    describe('getUser()', function() {
-      it('returns userId although principals contain non USER principals',
+      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();
+          var user = accessContext.getUser();
-          expect(userId).to.equal(userFromOneModel.id);
+          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();
+          var user = accessContext.getUser();
-          expect(userId).to.equal(userFromOneModel.id);
+          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();
+          var user = accessContext.getUser();
-          expect(userId).to.equal(userFromThirdModel.id);
+          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(
+            var validContext = {
-              Role.OWNER,
+              principalType: OneUser.modelName,
-              {
+              principalId: userFromOneModel.id,
-                principalType: OneUser.modelName,
+              model: Album,
-                principalId: userFromOneModel.id,
+              id: album.id,
-                model: Album,
+            };
-                id: album.id,
+            return Role.isInRole(Role.OWNER, validContext);
-              });
           })
-          .then(function(isInRole) {
+          .then(function(isOwner) {
-            expect(isInRole).to.be.true();
+            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();
           });
       });
     });

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;