Roles bypassing allow/deny rules about meteor-roles HOT 7 CLOSED

Hi @vonvo-carlo ,

Thanks for reporting this. I missed this issue earlier so just checking it out now.

I tried the addUsersToRoles call from the client-side in our production app and it doesn't allow the user to modify their permissions. We don't use allow/deny rules though so maybe in that case the default is to always deny.

Strange that it wouldn't behave the same though since, as you point out, Meteor.call methods don't follow allow/deny anyways.

I'll see if I can work up a repro. If you have one handy, please let me know.

from meteor-roles.

@alanning I spent a couple of hours really digging into meteor-router in detail today and ultimately, I was able to get what I'm looking for (allowing a user to update/remove his/her and other users' roles but NOT make themselves or others admins UNLESS already an app-wide admin) by doing the following in my deny rule for Meteor.users:

if(_.contains(fields, 'roles')) {

  if(modifier.$set) {
    return (_.contains(_.values(modifier.$set)[0], 'admin') && (!Roles.userIsInRole(currentUser, 'admin', 'vonvo')));
  }

  if(modifier.$addToSet) {
    return (_.contains(_.values(modifier.$addToSet)[0].$each, 'admin') && (!Roles.userIsInRole(currentUser, 'admin', 'vonvo')));
  }

  if(modifier.$pullAll) {
    return (_.contains(_.values(modifier.$pullAll)[0], 'admin') && (!Roles.userIsInRole(currentUser, 'admin', 'vonvo')));
  }
}

Pretty heinous rules, I know, but I don't see any other way to get the fine-tuned control I'm looking for besides maybe adding hooks to meteor-roles as a separate package (like collection-hooks).

In any case, the strange behavior I was seeing before was (I believe) caused by the fact that I was not treating each case (addUsersToRoles, which uses $addToSet, setUserRoles, which uses $set, and removeUsersFromRoles, which uses $pullAll) as distinct cases but trying to use a single rule for all of those scenarios.

If all you want to do is block users from updating roles, that's pretty easy. However, in most real-world apps, that's not too realistic since you might have users with admin rights within their group. So, to get that level of control, it's necessary to create special rules for each individual case.

Of course, even the above rules aren't perfect. For example, a user with admin rights from one group could conceivably alter the roles of a user in another group under these rules. So, there's still some fine-tuning needed to limit this access to JUST that user's group IF s/he has the admin role. All this will make the code pretty gnarly as well as not too performant but it does work properly.

from meteor-roles.

@alanning I think that the errors I'm seeing in Chrome Dev Tools are because I don't have any rules for Roles, so it's failing to update that collection.

The improved, refactored for conciseness version (works perfectly!):

Edit: The business rule this is meant to reflect is that only a group admin (in role 'admin' for a specific group) or an app-wide admin should be allowed to alter roles for members of a given group.

Meteor.users.allow({
  insert: function(userId, doc) {
    return true;
  },

  update: function(userId, doc, fields, modifier) {

    var currentUser = Meteor.user();
    // is this an app admin?
    var isAppAdmin = Roles.userIsInRole(currentUser, 'admin', 'vonvo');

    // if the user is trying to alter roles
    if(_.contains(fields, 'roles')) {

      var operator, roleGroup, group, role, isGroupAdmin;

      // this stores $pullAll, $set, $addToSet, etc.
      operator = _.keys(modifier)[0];

      // get the group
      roleGroup = _.keys(modifier[operator])[0];
      group = roleGroup.split('.')[1];

      isGroupAdmin = Roles.userIsInRole(currentUser, 'admin', group);

      // If this user is the group's admin or an app admin, allow the update
      return (isGroupAdmin || isAppAdmin);
    } else {
      // update allow rules, if any, for other parts of the user record *besides* roles
    }
  },

  remove: function(userId, doc) {
    return true;
  },
  fetch: ['_id']
});

I thought about a Roles.getGroupsForUser but I'd still need to make the comparison, anyway, so I'm not too sure about how much of a benefit it would bring to the table. I'd have to get the groups for the user, find the one I'm actually testing for, and then check to see if the user is an admin (or whatever) for that group. Doesn't seem that great of an option when all of that information is already being passed in, anyway...

But at any rate, this isn't actually an issue but thanks for looking at it. If you're interested, I put a test app up here.

from meteor-roles.

Thanks for following through with this @carlodicelico!

With your permission, I'd like to add this to the readme as I think others will probably find it useful. I would also like to link to your test app so people can play with it.

Does that sound good?

from meteor-roles.

Of course, go for it!

from meteor-roles.

Use of allow/deny rules is being slowly deprecated in Meteor exactly because of such issues where it is really hard to create good rules for all cases.

In practice, you should probably not allow any modifications to roles field at all through allow/deny rules. And create a Meteor method which calls addUsersToRoles with your change, after you validate anything. So what I would suggest is that you do:

Meteor.users.deny({
  update: function(userId, doc, fields, modifier) {
    return _.contains(fields, 'rules');
  }
});

Meteor.methods({
  addUserToRole: function (userId, role, partition) {
    check(userId, String);
    check(role, String);
    check(partition, String);

    if (!this.userId || !Roles.userIsInRole(this.userId, 'admin', 'vonvo')) throw new Meteor.Error('unauthorized', "Unauthorized.");
    if (this.userId === userId) throw new Meteor.Error('unauthorized', "Unauthorized.");

    Roles.addUsersToRoles(userId, role, partition);
  }
});

from meteor-roles.

@alanning How do I create allow/deny rules for the roles collection? In my app I' am able to call addUsersToRole from the console and get an update access denied from my users collection, but the role if does not exists is inserted.

from meteor-roles.