Fix Role.isOwner() when multiple user models #3180

ebarault · 2017-02-03T21:07:05Z

Description

When merging #2971 which i authored, @bajtos noted a missing test on the built-in role resolver $owner which internally calls the Role.isOwner() method (see here)
The added test actually failed, allowing users from different models but with the same id), because the Role.isOwner() method was not checking the user's principalType.
(good catch @bajtos 👍 )

This PR fixes this issue (and adds the corresponding test)
see the inlined comment for detailed code review

Related PR

#2971 (merged)
#3140 (in progress, will be impacted)
++ any PR/issue where someone would try to iterate recursively along belongTo relations up to the owning user (need to find those)

Checklist

New tests added or existing tests modified to cover all changes
Code conforms with the style
guide

slnode · 2017-02-03T21:07:12Z

Can one of the admins verify this patch?

slnode · 2017-02-03T21:07:12Z

Can one of the admins verify this patch?

slnode · 2017-02-03T21:07:12Z

Can one of the admins verify this patch?

slnode · 2017-02-03T21:07:12Z

Can one of the admins verify this patch?

ebarault

@bajtos : please review additional test + fix on built-in role-resolver $owner, when using multiple user models

ebarault · 2017-02-03T21:09:35Z

common/models/role.js

-    var userId = context.getUserId();
-    Role.isOwner(modelClass, modelId, userId, callback);
+    var user = context.getUser();
+    Role.isOwner(modelClass, modelId, user.id, user.principalType, callback);
  });


see below, fetching the userId from accessContext is no longer enough, consequently I had to introduce a getUser() method that also returns the user's principalType, and change the signature of Role.isOwner() to include the principalType as an optional parameter

ebarault · 2017-02-03T21:10:30Z

common/models/role.js

+      principalType = undefined;
+    }
+    principalType = principalType || Principal.USER;
+


while principalType is an optional parameter, it defaults internally to USER if undefined for backward compatibility

ebarault · 2017-02-03T21:12:02Z

common/models/role.js

+        process.nextTick(function() {
+          callback(null, matches(modelId, userId));
+        });
+      }
      return callback.promise;


we need now to check if the principalType is either USER (standard, single user model) or equal to user's model name (multiple user models)

ebarault · 2017-02-03T21:13:34Z

common/models/role.js

-      // Ensure ownerId exists and is not a function/relation
-      if (ownerId && 'function' !== typeof ownerId) {
-        if (callback) callback(null, matches(ownerId, userId));
-        return;


this whole part is not valid anymore as it relies only on comparing userIds and cannot be modified to handle principalType, we now need to directly follow belongsTo relations if any

I think this part is valid when principalType === Principal.USER, I am concerned that we will break backwards compatibility if we remove it. In 865e1c1, I added two tests to your feature branch - they are passing on master, but failing with your new implementation.

I assume that you already looked at my comment here and you prefer to keep this behavior in loopback 3 despite the comments.

So unless further comments i'll put back this behavior in place when principalType === 'USER'

I am open to discuss how to handle multiple belongsTo relations, but for the purpose of this pull request, I prefer to stay focused on fixing the issue created by the previous patch, i.e. "allowing users from different models but with the same id), because the Role.isOwner() method was not checking the user's principalType."

ebarault · 2017-02-03T21:15:08Z

common/models/role.js

+        if (principalType === Principal.USER || principalType === userModelName) {
+          debug('Checking relation %s to %s: %j', r, userModelName, rel);
+          inst[r](processRelatedUser);
+          return;


again, principalType must either be equal to USER, or to related user model name

ebarault · 2017-02-03T21:19:09Z

lib/access-context.js

    }
  }
-  return null;
+  return {};


turned the getUserId method in getUser, now returning an object of the like {id: p.id, principalType: p.type}
of course the getUserId is kept for backward compatibility just a few lines above, in a shortened form, reusing the getUser method

I think we should return null when there is no user associated with this AccessContext, to make it easy for the consumers of this API to detect this situation.

if we do this, then this will break the getUserId() method for which is seems normal to return null according to the tests. Should we then modify the getUserId method to return null when getUser() returns null?
If so, then there's not much need to not return {} in the first place

ebarault · 2017-02-03T21:19:40Z

test/multiple-user-principal-types.test.js

+          expect(user).to.eql({
+            id: userFromOneModel.id,
+            principalType: OneUser.modelName,
+          });
        });
      });


adapted new tests to check getUser instead of getUserId

ebarault · 2017-02-03T21:19:50Z

test/multiple-user-principal-types.test.js

+          expect(user).to.eql({
+            id: userFromOneModel.id,
+            principalType: OneUser.modelName,
+          });
        });
      });


ebarault · 2017-02-03T21:20:01Z

test/multiple-user-principal-types.test.js

+          expect(user).to.eql({
+            id: userFromThirdModel.id,
+            principalType: ThirdUser.modelName,
+          });
        });
      });
    });


ebarault · 2017-02-03T21:22:21Z

test/multiple-user-principal-types.test.js

+          return Role.isInRole(request.role, request.context);
+        });
+        return Promise.all(isInRoles);
+      }
    });



adjusted the test on built-in $owner role resolver to verify a user with similar id than an authorized user userOne but from another user model is denied access to album instance owned by userOne

ebarault · 2017-02-04T12:27:20Z

common/models/role.js

+        var userModelName = rel.modelTo.modelName;
+        if (principalType === Principal.USER || principalType === userModelName) {
+          debug('Checking relation %s to %s: %j', r, userModelName, rel);
+          relWithUsers.push(r);
        }


collecting related users rather than checking them right away : there might be several belongsTo relations linking the model instance to users, rejecting the role resolver at the first returned falsy is not right

I have two concerns about this change, I have expressed them in #3140:

backwards compatibility - with the change in place, our ACL system will start accepting request that are rejected in existing LB versions

what if a model has a belongsTo relation to a User-like model, but this relation does not create ownership?

Can we leave this change out of scope of this pull request please?

ebarault · 2017-02-04T12:29:20Z

common/models/role.js

+            debug('User found: %j (through %j)', user.id, r);
+            return cb(null, matches(user.id, userId));
+          }
+          cb(null, false);
        }


several things to note :

i chose asyncSeries so not to spam the db but it's clearly a tradeoff to make between performance and load here

i chose to handle errors in processRelatedUser as rejecting the overall async.someSeries: we could just callback them falsy, thoughts ?

i chose asyncSeries so not to spam the db but it's clearly a tradeoff to make between performance and load here

Yeah, it's a tradeoff and I don't have a strong opinion on which side is better. I am fine to use asyncSeries here.

i chose to handle errors in processRelatedUser as rejecting the overall async.someSeries: we could just callback them falsy, thoughts ?

I think the question is what to do when we run let's say 4 queries, one of them fails, two returns information that the user is not owner, and one says that the user is owner. Should we fail the whole request because of the error? Should we ignore the error and process the request, because we know that the user is authorized,
despite the error?

I think I like the current version better:

When there is an error, it's better to let the user and app administrators know about the problem

We can relax this behaviour later in the future if needed. Going the other way, throwing an error in situation that used to work, would be a semver-major change IMO.

Please confirm: I understand that in the following case :

4 queries, one of them fails, two returns information that the user is not owner, and one says that the user is owner

you propose we resolve isOwner truthy

let the app admins know about the error -> log an error msg in the console ?

let the app users know about the error -> how ?

Sorry for the confusion. What I meant is that your current implementation is good.

isOwner fails with an error

error-handling middleware reports this error e.g. to console.error

the app user sees that their request failed

Unless I am misunderstanding the code? Can you add a unit test executing this scenario and ensuring LoopBack behaves as we expect it to?

Having said that, this discussion is out of scope of this patch if we agree to keep current handling of "belongsTo" relations.

bajtos · 2017-02-06T08:35:06Z

@slnode ok to test

bajtos

Great, thank you @ebarault for this follow-up pull request!

You are on the right path, I left few comments to address or discuss:

bajtos · 2017-02-06T08:37:03Z

common/models/role.js


    // No userId is present
    if (!userId) {
      process.nextTick(function() {
-        callback(null, false);
+        callback(null);


Is there any particular reason for changing the result from false to undefined? I think it may be better to preserve callback(null, false).

my bad, there's no reason why to change this

bajtos · 2017-02-06T08:54:36Z

common/models/role.js

-      // Ensure ownerId exists and is not a function/relation
-      if (ownerId && 'function' !== typeof ownerId) {
-        if (callback) callback(null, matches(ownerId, userId));
-        return;


I think this part is valid when principalType === Principal.USER, I am concerned that we will break backwards compatibility if we remove it. In 865e1c1, I added two tests to your feature branch - they are passing on master, but failing with your new implementation.

bajtos · 2017-02-06T08:55:59Z

common/models/role.js

      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);


Ditto, I think it's better to keep the second argument as false? This can happen when err is falsy and the second condition !inst was triggered.

hmm, too quickly assumed err wasn't ever falsy in that case (overlooked the !inst)

bajtos · 2017-02-06T09:26:22Z

common/models/role.js

+        var userModelName = rel.modelTo.modelName;
+        if (principalType === Principal.USER || principalType === userModelName) {
+          debug('Checking relation %s to %s: %j', r, userModelName, rel);
+          relWithUsers.push(r);


Could you please rewrite this loop using Array.prototype.filter?

var relWithUsers = modelClass.relations.filter(r => { const rel = modelClass.relations[r]; // ... }); ``

Based on our recent discussions, I prefer to leave this change out of scope of this pull request, so that we don't mix several changes into one patch.

bajtos · 2017-02-06T09:27:27Z

common/models/role.js

+
+      function processRelation(r, cb) {
+        inst[r](processRelatedUser);
+        function processRelatedUser(err, user) {


Nitpic:

inst[r](function processRelatedUser(err, user) { // ... });

bajtos · 2017-02-06T09:32:34Z

common/models/role.js

+            debug('User found: %j (through %j)', user.id, r);
+            return cb(null, matches(user.id, userId));
+          }
+          cb(null, false);
        }


i chose asyncSeries so not to spam the db but it's clearly a tradeoff to make between performance and load here

Yeah, it's a tradeoff and I don't have a strong opinion on which side is better. I am fine to use asyncSeries here.

i chose to handle errors in processRelatedUser as rejecting the overall async.someSeries: we could just callback them falsy, thoughts ?

I think the question is what to do when we run let's say 4 queries, one of them fails, two returns information that the user is not owner, and one says that the user is owner. Should we fail the whole request because of the error? Should we ignore the error and process the request, because we know that the user is authorized,
despite the error?

I think I like the current version better:

When there is an error, it's better to let the user and app administrators know about the problem

We can relax this behaviour later in the future if needed. Going the other way, throwing an error in situation that used to work, would be a semver-major change IMO.

bajtos · 2017-02-06T09:33:57Z

lib/access-context.js

    }
  }
-  return null;
+  return {};


I think we should return null when there is no user associated with this AccessContext, to make it easy for the consumers of this API to detect this situation.

bajtos · 2017-02-06T09:36:14Z

test/multiple-user-principal-types.test.js

-          .then(function(isInRole) {
-            expect(isInRole).to.be.true();
+          .then(function(data) {
+            expect(data).to.eql([true, false]);


This assertion produces an error message that's difficult to understand without reading the source code of the test. Can you please split the test into two, where each tests asserts a single scenario (valid context, invalid context)?

ebarault

@bajtos : some comments/answers needed back before i move on. Thanks

ebarault · 2017-02-06T10:16:16Z

common/models/role.js


    // No userId is present
    if (!userId) {
      process.nextTick(function() {
-        callback(null, false);
+        callback(null);


my bad, there's no reason why to change this

ebarault · 2017-02-06T10:17:29Z

common/models/role.js

      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);


hmm, too quickly assumed err wasn't ever falsy in that case (overlooked the !inst)

ebarault · 2017-02-06T10:19:17Z

common/models/role.js

-      // Ensure ownerId exists and is not a function/relation
-      if (ownerId && 'function' !== typeof ownerId) {
-        if (callback) callback(null, matches(ownerId, userId));
-        return;


I assume that you already looked at my comment here and you prefer to keep this behavior in loopback 3 despite the comments.

So unless further comments i'll put back this behavior in place when principalType === 'USER'

ebarault · 2017-02-06T10:19:31Z

common/models/role.js

+        var userModelName = rel.modelTo.modelName;
+        if (principalType === Principal.USER || principalType === userModelName) {
+          debug('Checking relation %s to %s: %j', r, userModelName, rel);
+          relWithUsers.push(r);


ebarault · 2017-02-06T10:23:40Z

common/models/role.js

+            debug('User found: %j (through %j)', user.id, r);
+            return cb(null, matches(user.id, userId));
+          }
+          cb(null, false);
        }


Please confirm: I understand that in the following case :

4 queries, one of them fails, two returns information that the user is not owner, and one says that the user is owner

you propose we resolve isOwner truthy

let the app admins know about the error -> log an error msg in the console ?

let the app users know about the error -> how ?

ebarault · 2017-02-06T10:24:08Z

test/multiple-user-principal-types.test.js

-          .then(function(isInRole) {
-            expect(isInRole).to.be.true();
+          .then(function(data) {
+            expect(data).to.eql([true, false]);


ebarault · 2017-02-06T17:00:52Z

@bajtos : please review my comments so i can finalize the PR

bajtos · 2017-02-08T14:28:30Z

@ebarault sorry for the delay, I read your comments only now. I'll try to review and come back to you by the end of this week.

ebarault · 2017-02-16T18:57:07Z

common/models/role.js

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


we now need this additional check
if (principalType === Principal.USER || principalType === userModelName) {...
to make sure we don't follow a belongTo relation towards an incorrect user model.

ps. in order to keep the level of indent controlled, i chose to first continue if rel.type !== 'belongsTo' && !isUserClass(rel.modelTo)

The negation of rel.type === 'belongsTo' && isUserClass(rel.modelTo) is rel.type !== 'belongsTo' || !isUserClass(rel.modelTo), I'll fix this myself.

ebarault · 2017-02-16T23:35:36Z

common/models/role.js

+    var user = context.getUser();
+    var userId = user && user.id;
+    var principalType = user && user.principalType;
+    Role.isOwner(modelClass, modelId, userId, principalType, callback);
  });



now that context.getUser() can return null, we need to test user before getting user.id and user.principalType

ebarault · 2017-02-16T23:40:12Z

lib/access-context.js

    }

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


context.getUser() now returns null instead of {} to enable developers to more easily detect issues when accessContext misses the user

ebarault · 2017-02-16T23:41:58Z

lib/access-context.js

+  var user = this.getUser();
+  return user && user.id;
+};
+


shrinked version of getUserId(), leveraging getUser()

ebarault · 2017-02-16T23:43:07Z

test/multiple-user-principal-types.test.js

+            return Role.isInRole(Role.OWNER, invalidContext);
+          })
+          .then(function(isOwner) {
+            expect(isOwner).to.be.false();
          });


split the OWNER test in 2 to check apart if resolver returns false when principalType is incorrect

ebarault · 2017-02-16T23:46:14Z

@bajtos : i reverted the PR to only cover the required fix as discussed, and did the required modifications following your code review. You'll still have to squash the 2 commits to merge the tests you previously added. Thanks for carrying this over.

Fix `Role.isOwner()` to check both principalId and principalType. This fixes a bug where users from different User model were treated as owners as long as their user id was the same as owner's id.

bajtos · 2017-02-17T10:09:40Z

common/models/role.js

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


The negation of rel.type === 'belongsTo' && isUserClass(rel.modelTo) is rel.type !== 'belongsTo' || !isUserClass(rel.modelTo), I'll fix this myself.

bajtos · 2017-02-17T11:51:16Z

I made few last formatting improvements and landed the patch. Most importantly, I added more details to the commit message to explain the changes, I would appreciate if you could do that yourself in the future, @ebarault.

Thank you for the contribution! 🙇

ebarault · 2017-02-17T13:47:37Z

@bajtos : thanks. Yes i will take care of detailed commit messages myself in the future. See you soon.

crandmck added the #community contribution label Feb 3, 2017

ebarault commented Feb 3, 2017

View reviewed changes

ebarault mentioned this pull request Feb 4, 2017

Fix OWNER role to handle multiple relations #3140

Merged

ebarault force-pushed the enable-multiple-user-models branch from 6fd3ab2 to bc1e1e6 Compare February 4, 2017 12:22

ebarault commented Feb 4, 2017

View reviewed changes

ebarault force-pushed the enable-multiple-user-models branch from bc1e1e6 to 03354cc Compare February 4, 2017 12:30

bajtos self-assigned this Feb 6, 2017

bajtos suggested changes Feb 6, 2017

View reviewed changes

bajtos added bug major labels Feb 6, 2017

ebarault commented Feb 6, 2017

View reviewed changes

ebarault force-pushed the enable-multiple-user-models branch 3 times, most recently from 9787025 to a885e9e Compare February 16, 2017 18:38

ebarault commented Feb 16, 2017

View reviewed changes

ebarault force-pushed the enable-multiple-user-models branch 3 times, most recently from fddadd4 to d732a83 Compare February 16, 2017 23:34

ebarault commented Feb 16, 2017

View reviewed changes

Fix Role.isOwner() for multiple user models

f0e70dd

Fix `Role.isOwner()` to check both principalId and principalType. This fixes a bug where users from different User model were treated as owners as long as their user id was the same as owner's id.

bajtos force-pushed the enable-multiple-user-models branch from d732a83 to f0e70dd Compare February 17, 2017 10:19

bajtos approved these changes Feb 17, 2017

View reviewed changes

bajtos merged commit c827149 into strongloop:master Feb 17, 2017

bajtos removed the #community contribution label Feb 17, 2017

Fix Role.isOwner() when multiple user models #3180

Fix Role.isOwner() when multiple user models #3180

Conversation

ebarault commented Feb 3, 2017

Description

Related PR

Checklist

slnode commented Feb 3, 2017

slnode commented Feb 3, 2017

slnode commented Feb 3, 2017

slnode commented Feb 3, 2017

ebarault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bajtos commented Feb 6, 2017

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebarault left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebarault commented Feb 6, 2017

bajtos commented Feb 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ebarault commented Feb 16, 2017

Choose a reason for hiding this comment

bajtos commented Feb 17, 2017

ebarault commented Feb 17, 2017