Meteor-Community-Packages / meteor-roles

Authorization package for Meteor, compatible with built-in accounts packages
http://meteor-community-packages.github.io/meteor-roles/
MIT License
921 stars 168 forks source link

Hierarchy of roles #143

Closed mitar closed 8 years ago

mitar commented 8 years ago

Currently they are flat. For example, if user is in role foo, and role foo is parent of role bar, then checking for user for role bar should succeed if user is in role foo, but does not have bar assigned. Currently, that is not possible.

alanning commented 8 years ago

Yes, the hierarchy is flat. There is an older fork of this package which does support hierarchies, I think this is it: https://github.com/deepwell/meteor-authorization

For roles 2.0 I'd like to figure out a design that will allow others to add things like hierarchy as a separate package which uses roles as a dependency. If you have some suggestions on how to architect it, please let me know or point me to examples of other packages that have a good plugin api.

mitar commented 8 years ago

Hm, from current API I think you should just provide a hook where you resolve group names. So developers can then continue to do userIsInRole((joesUserId, ['manage-team', 'super-admin']). And the lookup would then do the rest. For example, extend ['manage-team', 'super-admin'] to ['manage-team', 'super-admin', 'admin'], if for example admin is under super-admin in the hierarchy.

This could be made also backwards compatible. So I would not really make it roles 2.0, just a pull request to this version.

In addition, Roles.userIsInRole(joesUserId, ['manage-team', 'super-admin'], 'real-madrid.com')) could be seen as Roles.userIsInRole(joesUserId, ['manage-team', 'super-admin'], options)), where string can be converted to {group: 'real-madrid.com'}. Then options could be passed to hierarchy lookup plugin.

So you could do then things like Roles.userIsInRole(joesUserId, ['manage-team', 'super-admin'], {hierarchy: false}) to disable hierarchy expansion for that particular call.

The registration could be done maybe just by exposing a method call for now and then plugins can monkey-patch it. Alternatively, you can make a registration pattern, when they register some function/callback and then you call those in order. But then the question is in which order. So it gets a bit more complicated with how much package has to provide. Personally, I would for now just provide a method for package to override, and then they can use Meteor order of package dependencies to decide the order in which plugins should apply lookup if they really want to use multiple.

What do you think about this API?

alanning commented 8 years ago

Thanks, @mitar. I love the idea of auto-expanding the roles based on hierarchy and changing the 3rd param to be an options object. Great that those changes can be backwards compatible!

I've been considering cases where multiple plugins might be authoritative (like, hierarchy, timed-access) and I think you're right that just a basic ordered pass-through chain would work. At least I haven't come up with any scenarios where letting the user determine the order (based on registration) would return an incorrect result. So I think that would work too.

Are there any plug-ins or major features that you'd like to see implemented?

mitar commented 8 years ago

No, for me this expansion of roles would be good enough. If you are OK with the proposed design, I can maybe try to find some time to create a pull request according to this design. I would just do it against 1.x because it is backwards compatible.

alanning commented 8 years ago

That would be wonderful!

Given that this could be backwards compatible, the major change in roles v2 would be restructuring the database schema used so that we could index on group. Wouldn't impact hierarchy but just wanted to mention it to give you an idea of what's eventually coming.

The reason its important is because a lot of people want to use this package to also indicate "membership" in a group, besides just checking roles in that group. I'm not sure that's actually a good way to use this package but right now it's impossible to index so running these kinds of queries requires a full table scan.

mitar commented 8 years ago

You mean, you would like to be able to query also on the nested roles?

One easy way, and still backwards compatible, would then to have two fields provided by this package, roles (the current one) and effectiveRoles. So roles are the roles assigned to the user, while effectiveRoles are a flattened list of roles based on the hierarchy. I can do it like that as well, if that would be OK.

But if we are talking about schema change, maybe we should really change everything to the following:

roles: [
  {
    role: 'manage-team',
    group: 'manchester-united_com',
    inferred: false
  }
]

This would also allow group names to contain dots (which current schema does not allow). Also it would unify group and group-less roles.

mitar commented 8 years ago

The latter schema would also allow custom plugins to add custom fields to roles.

mitar commented 8 years ago

I am also OK with creating a backwards non-compatible pull request for 2.0. I do not have problems changing my schema because I use migrations.

alanning commented 8 years ago

My plan for plain 2.0 was in fact just:

roles: [
  {
    roles: ['manage-team', 'goalie'],
    group: 'manchester-united.com'
  }
]

So pretty much the same as what you suggested. The separate group field is required for indexing. MongoDB supports indexing on arrays so I was just planning on having the one entry per group.

Separate entries for each role+group pairing may be better because I think that would allow more flexibility for plugins like you said. For example, I think that would allow for a quite simple implementation of timed-access which would be really nice.

mitar commented 8 years ago

The schema I proposed can also be easily indexed. So, what should we do?

alanning commented 8 years ago

Right, I think your proposed schema is better. I'd say use that as the first step (it will basically be roles v2.0). Then we can build on hierarchy on top of that. May even work as a plugin with the new schema but I'm ok having it baked into core roles if that's easier.

I'd also suggest changing the name from group to resource since it will be a schema change anyways. That would probably help avoid some confusion and also help people draw the comparison with ACLs.

mitar commented 8 years ago

Not sure if "resource" would really make less confusion. Because resource based ACLs should probably be on the document itself (if you mean per-document ACL, what I understand as a resource).

alanning commented 8 years ago

Hmm, I did see your comment on that in another issue. I'd like to discuss that more with you if possible. I'm in EST timezone (eastern US) so it's too late for me now but can you shoot me an email if you are available to Hangout / Skype and we can schedule a time?

mitar commented 8 years ago

I will send you an e-mail and let's coordinate this.

mitar commented 8 years ago

BTW, I do not have strong position on this, but I did implement per-document ACLs in the past and it worked pretty well in my opinion. So groups of roles for me is more like community. So if you have multi-community app. Where you have one installation for various groups of people who want to have their own individuals set of roles. I see groups useful for that purpose.

donwojtallo commented 8 years ago

+1 Hi. I need this feature too. I really miss RBAC system after switching from old PHP framework to Meteor. I just want to tell that I think roles inheritance is a very important feature.

mitar commented 8 years ago

This was implemented with v2.0 (currently in development).