Merge pull request strongloop#3720 from STRML/fix/falsy-id-3.x

Fix handling of falsy model ids
atul-github · Dec 8, 2017 · 3bf84ba · 3bf84ba
2 parents 1babfcd + 2bfd67c
commit 3bf84ba
Show file tree

Hide file tree

Showing 4 changed files with 66 additions and 9 deletions.
diff --git a/lib/access-context.js b/lib/access-context.js
@@ -80,17 +80,16 @@ function AccessContext(context) {
   var principalType = context.principalType || Principal.USER;
   var principalId = context.principalId || undefined;
   var principalName = context.principalName || undefined;
-
-  if (principalId) {
+  if (principalId != null) {
     this.addPrincipal(principalType, principalId, principalName);
   }
 
   var token = this.accessToken || {};
 
-  if (token.userId) {
+  if (token.userId != null) {
     this.addPrincipal(Principal.USER, token.userId);
   }
-  if (token.appId) {
+  if (token.appId != null) {
     this.addPrincipal(Principal.APPLICATION, token.appId);
   }
   this.remotingContext = context.remotingContext;
@@ -193,7 +192,7 @@ AccessContext.prototype.getAppId = function() {
  * @returns {boolean}
  */
 AccessContext.prototype.isAuthenticated = function() {
-  return !!(this.getUserId() || this.getAppId());
+  return this.getUserId() != null || this.getAppId() != null;
 };
 
 /**

diff --git a/lib/model.js b/lib/model.js
@@ -161,13 +161,13 @@ module.exports = function(registry) {
         }
       }
 
-      if (id && data) {
+      if (id != null && data) {
         var model = new ModelCtor(data);
         model.id = id;
         fn(null, model);
       } else if (data) {
         fn(null, new ModelCtor(data));
-      } else if (id) {
+      } else if (id != null) {
         var filter = {};
         ModelCtor.findById(id, filter, options, function(err, model) {
           if (err) {

diff --git a/lib/persisted-model.js b/lib/persisted-model.js
@@ -1710,7 +1710,7 @@ module.exports = function(registry) {
         ctx.instance, ctx.currentInstance, ctx.where, ctx.data);
     }
 
-    if (id) {
+    if (id != null) {
       ctx.Model.rectifyChange(id, reportErrorAndNext);
     } else {
       ctx.Model.rectifyAllChanges(reportErrorAndNext);
@@ -1734,7 +1734,7 @@ module.exports = function(registry) {
       debug('context instance:%j where:%j', ctx.instance, ctx.where);
     }
 
-    if (id) {
+    if (id != null) {
       ctx.Model.rectifyChange(id, reportErrorAndNext);
     } else {
       ctx.Model.rectifyAllChanges(reportErrorAndNext);

diff --git a/test/role.test.js b/test/role.test.js
@@ -364,6 +364,64 @@ describe('role model', function() {
     });
   });
 
+  it.only('should be properly authenticated with 0 userId', function(done) {
+    var userData = {name: 'Raymond', email: 'x@y.com', password: 'foobar', id: 0};
+    var TestUser = app.registry.createModel({
+      name: 'TestUser',
+      base: 'User',
+      // forceId is set to false so we can create a user with a known ID,
+      // in this case 0 - which used to fail the falsy checks.
+      forceId: false,
+    });
+    app.model(TestUser, {dataSource: 'db'});
+
+    TestUser.create(userData, function(err, user) {
+      if (err) return done(err);
+      Role.create({name: 'userRole'}, function(err, role) {
+        if (err) return done(err);
+        role.principals.create({principalType: RoleMapping.USER, principalId: user.id},
+        function(err, p) {
+          if (err) return done(err);
+          async.series([
+            function(next) {
+              Role.isInRole(
+                'userRole',
+                {principalType: RoleMapping.USER, principalId: user.id},
+                function(err, inRole) {
+                  if (err) return next(err);
+                  assert(!!inRole);
+                  next();
+                });
+            },
+            function(next) {
+              Role.isInRole(
+                'userRole',
+                {principalType: RoleMapping.APP, principalId: user.id},
+                function(err, inRole) {
+                  if (err) return next(err);
+                  assert(!inRole);
+                  next();
+                });
+            },
+            function(next) {
+              Role.getRoles(
+                {principalType: RoleMapping.USER, principalId: user.id},
+                function(err, roles) {
+                  if (err) return next(err);
+                  expect(roles).to.eql([
+                    Role.AUTHENTICATED,
+                    Role.EVERYONE,
+                    role.id,
+                  ]);
+                  next();
+                });
+            },
+          ], 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) {