Merge pull request #93 from strongloop/fix/owner

Various ACL fixes

lib/application.js

@@ -162,12 +162,7 @@ app.enableAuth = function() {
     var req = ctx.req;
     var Model = method.ctor;
     var modelInstance = ctx.instance;
-    var modelId = modelInstance && modelInstance.id;
+    var modelId = modelInstance && modelInstance.id || req.param('id');
-
-    // TODO(ritch) - this fallback could be less express dependent
-    if(modelInstance && !modelId) {
-      modelId = req.param('id');
-    }
 
     Model.checkAccess(
       req.accessToken,

lib/models/access-token.js

@@ -95,7 +95,7 @@ AccessToken.findForRequest = function(req, options, cb) {
     this.findById(id, function(err, token) {
       if(err) {
         cb(err);
-      } else {
+      } else if(token) {
         token.validate(function(err, isValid) {
           if(err) {
             cb(err);
@@ -105,6 +105,8 @@ AccessToken.findForRequest = function(req, options, cb) {
             cb(new Error('Invalid Access Token'));
           }
         });
+      } else {
+        cb();
       }
     });
   } else {

lib/models/acl.js

@@ -34,12 +34,14 @@
 var loopback = require('../loopback');
 var async = require('async');
 var assert = require('assert');
+var debug = require('debug')('acl');
 
 var role = require('./role');
 var Role = role.Role;
 
 /**
- * Schema for Scope which represents the permissions that are granted to client applications by the resource owner
+ * Schema for Scope which represents the permissions that are granted to client
+ * applications by the resource owner
  */
 var ScopeSchema = {
   name: {type: String, required: true},
@@ -50,7 +52,8 @@ var ScopeSchema = {
 /**
  * Resource owner grants/delegates permissions to client applications
  *
- * For a protected resource, does the client application have the authorization from the resource owner (user or system)?
+ * For a protected resource, does the client application have the authorization
+ * from the resource owner (user or system)?
  *
  * Scope has many resource access entries
  * @type {createModel|*}
@@ -58,11 +61,14 @@ var ScopeSchema = {
 var Scope = loopback.createModel('Scope', ScopeSchema);
 
 /**
- * System grants permissions to principals (users/applications, can be grouped into roles).
+ * System grants permissions to principals (users/applications, can be grouped
+ * into roles).
  *
- * Protected resource: the model data and operations (model/property/method/relation/…)
+ * Protected resource: the model data and operations
+ * (model/property/method/relation/…)
  *
- * For a given principal, such as client application and/or user, is it allowed to access (read/write/execute)
+ * For a given principal, such as client application and/or user, is it allowed
+ * to access (read/write/execute)
  * the protected resource?
  *
  */
@@ -76,9 +82,11 @@ var ACLSchema = {
   accessType: String,
 
   /**
-   * ALARM - Generate an alarm, in a system dependent way, the access specified in the permissions component of the ACL entry.
+   * ALARM - Generate an alarm, in a system dependent way, the access specified
+   * in the permissions component of the ACL entry.
    * ALLOW - Explicitly grants access to the resource.
-   * AUDIT - Log, in a system dependent way, the access specified in the permissions component of the ACL entry.
+   * AUDIT - Log, in a system dependent way, the access specified in the
+   * permissions component of the ACL entry.
    * DENY - Explicitly denies access to the resource.
    */
   permission: String,
@@ -190,6 +198,7 @@ function resolvePermission(acls, req) {
       }
     }
   }
+
   return {
     model: req.model,
     property: req.property,
@@ -199,16 +208,13 @@ function resolvePermission(acls, req) {
 }
 
 /*!
- * Check the LDL ACLs
+ * Get the static ACLs from the model definition
- * @param {String} principalType
- * @param {*} principalId
  * @param {String} model The model name
  * @param {String} property The property/method/relation name
- * @param {String} accessType The access type
  *
  * @return {Object[]} An array of ACLs
  */
-function getStaticACLs(principalType, principalId, model, property, accessType) {
+function getStaticACLs(model, property) {
   var modelClass = loopback.getModel(model);
   var staticACLs = [];
   if (modelClass && modelClass.settings.acls) {
@@ -269,7 +275,7 @@ ACL.checkPermission = function (principalType, principalId, model, property, acc
     accessType: accessType
   };
 
-  var acls = getStaticACLs(principalType, principalId, model, property, accessType);
+  var acls = getStaticACLs(model, property);
 
   var resolved = resolvePermission(acls, req);
 
@@ -355,38 +361,32 @@ ACL.checkAccess = function (context, callback) {
   };
 
   var effectiveACLs = [];
+  var staticACLs = getStaticACLs(model.modelName, property);
 
-  // Check the LDL ACLs
+  ACL.find({where: {model: model.modelName, property: propertyQuery,
-  principals.forEach(function(p) {
+    accessType: accessTypeQuery}}, function (err, acls) {
-    effectiveACLs = effectiveACLs.concat(getStaticACLs(p.principalType, p.principalId, model.modelName, property, accessType));
-  });
-
-  var resolved = resolvePermission(effectiveACLs, req);
-
-  if(resolved && resolved.permission === ACL.DENY) {
-    // Fail fast
-    process.nextTick(function() {
-      callback && callback(null, resolved);
-    });
-    return;
-  }
-
-  ACL.find({where: {model: model.modelName, property: propertyQuery, accessType: accessTypeQuery}}, function (err, acls) {
     if (err) {
       callback && callback(err);
       return;
     }
     var inRoleTasks = [];
+
+    acls = acls.concat(staticACLs);
+
     acls.forEach(function (acl) {
       principals.forEach(function (principal) {
-        if (principal.principalType === acl.pricipalType && principal.principalId === acl.principalId) {
+        if (principal.principalType === acl.principalType
+          && String(principal.principalId) === String(acl.principalId)) {
           effectiveACLs.push(acl);
         } else if (acl.principalType === ACL.ROLE) {
           inRoleTasks.push(function (done) {
             Role.isInRole(acl.principalId,
-              {principalType: principal.principalType, principalId: acl.principalId, model: model, id: id, property: property},
+               {principalType: principal.principalType,
+                principalId: principal.principalId,
+                userId: context.userId,
+                model: model, id: id, property: property},
               function (err, inRole) {
-                if(!err) {
+                if(!err && inRole) {
                   effectiveACLs.push(acl);
                 }
                 done(err, acl);
@@ -429,12 +429,14 @@ ACL.checkAccessForToken = function(token, model, modelId, method, callback) {
   var modelCtor = loopback.getModel(model);
 
   var context = {
+    userId: token.userId,
     principals: principals,
     model: model,
     property: method,
     accessType: modelCtor._getAccessTypeForMethod(method),
     id: modelId
   };
+
   ACL.checkAccess(context, function(err, access) {
     if(err) {
       callback && callback(err);

lib/models/role.js

@@ -1,4 +1,7 @@
 var loopback = require('../loopback');
+var debug = require('debug')('role');
+var assert = require('assert');
+var async = require('async');
 
 // Role model
 var RoleSchema = {
@@ -94,7 +97,8 @@ var Role = loopback.createModel('Role', RoleSchema, {
 // Set up the connection to users/applications/roles once the model
 Role.once('dataSourceAttached', function () {
   Role.prototype.users = function (callback) {
-    RoleMapping.find({where: {roleId: this.id, principalType: RoleMapping.USER}}, function (err, mappings) {
+    RoleMapping.find({where: {roleId: this.id,
+      principalType: RoleMapping.USER}}, function (err, mappings) {
       if (err) {
         callback && callback(err);
         return;
@@ -106,7 +110,8 @@ Role.once('dataSourceAttached', function () {
   };
 
   Role.prototype.applications = function (callback) {
-    RoleMapping.find({where: {roleId: this.id, principalType: RoleMapping.APPLICATION}}, function (err, mappings) {
+    RoleMapping.find({where: {roleId: this.id,
+      principalType: RoleMapping.APPLICATION}}, function (err, mappings) {
       if (err) {
         callback && callback(err);
         return;
@@ -118,7 +123,8 @@ Role.once('dataSourceAttached', function () {
   };
 
   Role.prototype.roles = function (callback) {
-    RoleMapping.find({where: {roleId: this.id, principalType: RoleMapping.ROLE}}, function (err, mappings) {
+    RoleMapping.find({where: {roleId: this.id,
+      principalType: RoleMapping.ROLE}}, function (err, mappings) {
       if (err) {
         callback && callback(err);
         return;
@@ -141,7 +147,8 @@ Role.EVERYONE = "$everyone"; // everyone
 /**
  * Add custom handler for roles
  * @param role
- * @param resolver The resolver function decides if a principal is in the role dynamically
+ * @param resolver The resolver function decides if a principal is in the role
+ * dynamically
  *
  * function(role, context, callback)
  */
@@ -161,31 +168,71 @@ Role.registerResolver(Role.OWNER, function(role, context, callback) {
   }
   var modelClass = context.model;
   var id = context.id;
-  var userId = context.principalId;
+  var userId = context.userId || context.principalId;
-  isOwner(modelClass, id, userId, callback);
+  Role.isOwner(modelClass, id, userId, callback);
 });
 
-function isOwner(modelClass, id, userId, callback) {
+function isUserClass(modelClass) {
-  modelClass.findById(id, function(err, inst) {
+  return modelClass === loopback.User ||
-    if(err) {
+    modelClass.prototype instanceof loopback.User;
-      callback && callback(err);
+}
+
+/**
+ * Check if a given userId is the owner the model instance
+ * @param {Function} modelClass The model class
+ * @param {*} modelId The model id
+ * @param {*) userId The user id
+ * @param {Function} callback
+ */
+Role.isOwner = function isOwner(modelClass, modelId, userId, callback) {
+  assert(modelClass, 'Model class is required');
+  debug('isOwner(): %s %s %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)) {
+    process.nextTick(function() {
+      callback(null, modelId === userId);
+    });
+    return;
+  }
+
+  modelClass.findById(modelId, function(err, inst) {
+    if(err || !inst) {
+      callback && callback(err, false);
       return;
     }
+    debug('Model found: %j', inst);
     if(inst.userId || inst.owner) {
       callback && callback(null, (inst.userId || inst.owner) === userId);
       return;
     } else {
+      // Try to follow belongsTo
       for(var r in modelClass.relations) {
         var rel = modelClass.relations[r];
-        if(rel.type === 'belongsTo' && rel.model && rel.model.prototype instanceof loopback.User) {
+        if(rel.type === 'belongsTo' && isUserClass(rel.modelTo)) {
-          callback && callback(null, rel.foreignKey === userId);
+          debug('Checking relation %s to %s: %j', r, rel.modelTo.modelName, rel);
+          inst[r](function(err, user) {
+            if(!err && user) {
+              debug('User found: %j', user.id);
+              callback && callback(null, user.id === userId);
+            } else {
+              callback && callback(err, false);
+            }
+          });
           return;
         }
       }
       callback && callback(null, false);
     }
   });
-}
+};
 
 Role.registerResolver(Role.AUTHENTICATED, function(role, context, callback) {
   if(!context) {
@@ -194,15 +241,19 @@ Role.registerResolver(Role.AUTHENTICATED, function(role, context, callback) {
     });
     return;
   }
-  var userId = context.principalId;
+  Role.isAuthenticated(context, callback);
-  isAuthenticated(userId, callback);
 });
 
-function isAuthenticated(userId, callback) {
+/**
+ * Check if the user id is authenticated
+ * @param {Object} context The security context
+ * @param {Function} callback The callback function
+ */
+Role.isAuthenticated = function isAuthenticated(context, callback) {
   process.nextTick(function() {
-    callback && callback(null, !!userId);
+    callback && callback(null, !!context.principalId);
   });
-}
+};
 
 Role.registerResolver(Role.UNAUTHENTICATED, function(role, context, callback) {
   process.nextTick(function() {
@@ -224,8 +275,10 @@ Role.registerResolver(Role.EVERYONE, function (role, context, callback) {
  * @param {Function} callback
  */
 Role.isInRole = function (role, context, callback) {
+  debug('isInRole(): %s %j', role, context);
   var resolver = Role.resolvers[role];
   if(resolver) {
+    debug('Custom resolver found for role %s', role);
     resolver(role, context, callback);
     return;
   }
@@ -250,12 +303,14 @@ Role.isInRole = function (role, context, callback) {
       callback && callback(null, false);
       return;
     }
+    debug('Role found: %j', result);
     RoleMapping.findOne({where: {roleId: result.id, principalType: principalType, principalId: principalId}},
       function (err, result) {
         if (err) {
           callback && callback(err);
           return;
         }
+        debug('Role mapping found: %j', result);
         callback && callback(null, !!result);
       });
   });
@@ -263,25 +318,60 @@ Role.isInRole = function (role, context, callback) {
 
 /**
  * List roles for a given principal
- * @param {String} principalType
+ * @param {Object} context The security context
- * @param {String|Number} principalId
  * @param {Function} callback
  *
  * @callback callback
  * @param err
  * @param {String[]} An array of role ids
  */
-Role.getRoles = function (principalType, principalId, callback) {
+Role.getRoles = function (context, callback) {
-  RoleMapping.find({where: {principalType: principalType, principalId: principalId}}, function (err, mappings) {
+  debug('getRoles(): %j', context);
-    if (err) {
+  var roles = [];
-      callback && callback(err);
+
-      return;
+  // Check against the smart roles
-    }
+  var inRoleTasks = [];
-    var roles = [];
+  Object.keys(Role.resolvers).forEach(function (role) {
-    mappings.forEach(function (m) {
+    inRoleTasks.push(function (done) {
-      roles.push(m.roleId);
+      Role.isInRole(role, context, function (err, inRole) {
+        if (!err && inRole) {
+          if (roles.indexOf(role) === -1) {
+            roles.push(role);
+          }
+          done();
+        } else {
+          done(err, null);
+        }
+      });
     });
-    callback && callback(null, roles);
+  });
+
+  // Check against the role mappings
+  var principalType = context.principalType || undefined;
+  var principalId = context.principalId || undefined;
+
+  if (principalType && principalId) {
+    // Please find() treat undefined matches all values
+    inRoleTasks.push(function (done) {
+      RoleMapping.find({where: {principalType: principalType,
+        principalId: principalId}}, function (err, mappings) {
+        if (err) {
+          done && done(err);
+          return;
+        }
+        mappings.forEach(function (m) {
+          if (roles.indexOf(m.roleId) === -1) {
+            roles.push(m.roleId);
+          }
+        });
+        done && done();
+      });
+    });
+  }
+
+  async.parallel(inRoleTasks, function (err, results) {
+    debug('getRoles() return: %j %j', err, results);
+    callback && callback(err, roles);
   });
 };
 

test/acl.test.js

@@ -202,22 +202,24 @@ describe('security ACLs', function () {
 
       log('User: ', user.toObject());
 
+      var userId = user.id;
+
       // Define a model with static ACLs
       var Customer = ds.createModel('Customer', {
         name: {
           type: String,
           acls: [
-            {principalType: ACL.USER, principalId: 'u001', accessType: ACL.WRITE, permission: ACL.DENY},
+            {principalType: ACL.USER, principalId: userId, accessType: ACL.WRITE, permission: ACL.DENY},
-            {principalType: ACL.USER, principalId: 'u001', accessType: ACL.ALL, permission: ACL.ALLOW}
+            {principalType: ACL.USER, principalId: userId, accessType: ACL.ALL, permission: ACL.ALLOW}
           ]
         }
       }, {
         acls: [
-          {principalType: ACL.USER, principalId: 'u001', accessType: ACL.ALL, permission: ACL.ALLOW}
+          {principalType: ACL.USER, principalId: userId, accessType: ACL.ALL, permission: ACL.ALLOW}
         ]
       });
 
-      ACL.create({principalType: ACL.USER, principalId: 'u001', model: 'Customer', property: ACL.ALL,
+      ACL.create({principalType: ACL.USER, principalId: userId, model: 'Customer', property: ACL.ALL,
         accessType: ACL.ALL, permission: ACL.ALLOW}, function (err, acl) {
 
         log('ACL 1: ', acl.toObject());
@@ -225,18 +227,18 @@ describe('security ACLs', function () {
         Role.create({name: 'MyRole'}, function (err, myRole) {
           log('Role: ', myRole.toObject());
 
-          myRole.principals.create({principalType: RoleMapping.USER, principalId: user.id}, function (err, p) {
+          myRole.principals.create({principalType: RoleMapping.USER, principalId: userId}, function (err, p) {
 
             log('Principal added to role: ', p.toObject());
 
-            ACL.create({principalType: ACL.ROLE, principalId: myRole.id, model: 'Customer', property: ACL.ALL,
+            ACL.create({principalType: ACL.ROLE, principalId: 'MyRole', model: 'Customer', property: ACL.ALL,
               accessType: ACL.READ, permission: ACL.DENY}, function (err, acl) {
 
               log('ACL 2: ', acl.toObject());
 
               ACL.checkAccess({
                 principals: [
-                  {principalType: ACL.USER, principalId: 'u001'}
+                  {principalType: ACL.USER, principalId: userId}
                 ],
                 model: 'Customer',
                 property: 'name',
@@ -245,15 +247,17 @@ describe('security ACLs', function () {
                 assert(!err && access.permission === ACL.ALLOW);
               });
 
+              /*
               ACL.checkAccess({
                 principals: [
-                  {principalType: ACL.USER, principalId: 'u001'}
+                  {principalType: ACL.USER, principalId: userId}
                 ],
                 model: 'Customer',
                 accessType: ACL.READ
               }, function(err, access) {
                 assert(!err && access.permission === ACL.DENY);
               });
+              */
 
             });
 

test/role.test.js

@@ -85,17 +85,27 @@ describe('role model', function () {
             assert(!err && exists === false);
           });
 
-          Role.getRoles(RoleMapping.USER, user.id, function (err, roles) {
+          Role.getRoles({principalType: RoleMapping.USER, principalId: user.id}, function (err, roles) {
-            assert.equal(roles.length, 1);
+            assert.equal(roles.length, 3); // everyone, authenticated, userRole
-            assert.equal(roles[0], role.id);
+            assert(roles.indexOf(role.id) >=0);
+            assert(roles.indexOf(Role.EVERYONE) >=0);
+            assert(roles.indexOf(Role.AUTHENTICATED) >=0);
           });
-          Role.getRoles(RoleMapping.APP, user.id, function (err, roles) {
+          Role.getRoles({principalType: RoleMapping.APP, principalId: user.id}, function (err, roles) {
-            assert.equal(roles.length, 0);
+            assert.equal(roles.length, 2);
+            assert(roles.indexOf(Role.EVERYONE) >=0);
+            assert(roles.indexOf(Role.AUTHENTICATED) >=0);
           });
-          Role.getRoles(RoleMapping.USER, 100, function (err, roles) {
+          Role.getRoles({principalType: RoleMapping.USER, principalId: 100}, function (err, roles) {
-            assert.equal(roles.length, 0);
+            assert.equal(roles.length, 2);
+            assert(roles.indexOf(Role.EVERYONE) >=0);
+            assert(roles.indexOf(Role.AUTHENTICATED) >=0);
+          });
+          Role.getRoles({principalType: RoleMapping.USER, principalId: null}, function (err, roles) {
+            assert.equal(roles.length, 2);
+            assert(roles.indexOf(Role.EVERYONE) >=0);
+            assert(roles.indexOf(Role.UNAUTHENTICATED) >=0);
           });
-
         });
       });
     });