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 166 forks source link

Reusability: Can we take roles into a separate package, or should I fork? #270

Closed SimonSimCity closed 4 years ago

SimonSimCity commented 5 years ago

I don't know how much sense this will make to you, but I'll try my best explaining what I want in detail.

Personally, I find the package here pretty good to start with and I've used this package in version 2.0 for quite a while now. There are some things I want to address which would require quite some changes to this package, but I'm not sure if we should not collaborate instead of me just forking off and brewing my own soup.

I like the concept of having roles for everything, and this is also the part I would like to reuse in my package. Roles having nested roles, where each of them has a list of children containing the inherited roles. Even though I would not only include the immediate children, but also nested - but let's start from the beginning.

There are some limitations I hit which I need to do address in a different way.

I need to save the assignment of a role, which currently is stored in the property roles in the user document, in a separate collection to keep up with my performance requirements. Here's a list of reasons and features I need:

To keep the first reason valid, I would also need to store the hierarchy of roles in the parent-roles, which would be the only change I see for the roles collection.

So, how does this sound? I think it justifies a separate package, but - at the same time - I would like to cooperate with you on a common basis so users could easily choose one of our packages, even if they only share the roles and all the methods there.

If you say that the differences of our ideas are not compatible, I'd go and brew something on my own and keep it separate from yours.

mitar commented 5 years ago

So the second point I do in my publish endpoint where I limit which permissions get published with a simple filter on the array. The issue is only that Meteor does not auto-merge those together if you have multiple publish endpoints publishing different sets of permissions.

For third, I already do that with new schema where you have a document per assignment. So you can add stuff there. I add auditing information there, for example when and who assigned a permission.

Personally I think that per-resource permissions belong to the document itself. And for hierarchical resources (like you trying to address with grouping), I have logic which knows the hierarchy. For example, in my case I have permissions on a post and then permissions on comments, and some permissions from a post tr to comments. Do you think groups of resources are enough or would some general relation between be better? Also in your example: folder -> file.

But I do see why there could be a separate collection for all permissions. It definitely makes sense.

I do not have an answer about collaboration. Myself, I do not have time at the moment. So I will leave to others to way in here.

But one option would be to or make a switch in this package in which way one wants to record permissions, or to maybe try to keep APIs the same between packages. Also, a question is if storing permissions inside user document have any advantage at all?

alanning commented 5 years ago

Not sure if this is helpful but while we call the base object a "user", the API doesn't really care except that we hard-coded Meteor.users.update in places. If we were to make the underlying Mongo Collection configurable, you could invert the normal use of the Roles package to make it document-centric as opposed to user-centric.

So if you could update the target collection, you could conceivably use of the Roles package to control access to a document resource by storing the user IDs on the resource record itself.

The usage would look something like this:

// Requires API change in Roles
Roles.setTargetCollection = Mongo.myDocuments

// Just regular javascript
Roles.addUsersToDocuments = Roles.addUsersToRoles
Roles.userCanAccessDocument = Roles.userIsInRole

Roles.addUsersToDocuments(docObj, UserIDs, optionalScope)
const hasAccess = Roles.userCanAccessDocument(docObj, userIDToCheck, optionalScope)

Would this cover your use case?

SimonSimCity commented 5 years ago

Thanks for the response of you two, really appreciate that you take yourself time for this.

So the second point I do in my publish endpoint where I limit which permissions get published with a simple filter on the array. The issue is only that Meteor does not auto-merge those together if you have multiple publish endpoints publishing different sets of permissions.

If we would extract all the permissions a user has into an extra collection, we could avoid this problem.

For third, I already do that with new schema where you have a document per assignment. So you can add stuff there. I add auditing information there, for example when and who assigned a permission.

Good point there. If we go the way of saving the assigned roles in a separate collection (one document per role assigned) this looks tempting but might in fact be not the best place if we want to keep this flexible.

Personally I think that per-resource permissions belong to the document itself. [...] For example, in my case I have permissions on a post and then permissions on comments, and some permissions from a post tr to comments.

The reason to me is that I also will have quite an amount of people who will have global permission (how would you store this?) and some documents where the amount of users would exceed 100 users. This would again leave me with the same performance problems as I have of today. And you would have the hassle of adding a bunch of permissions to a new comment on creation.

As I talk about resource groups, this would be very hard to handle it using the approach you suggested, alanning, because I'm not talking about only one type here. I'd have to define multiple target collections then ...


Long story short:

I'll create a PR where I extract the roles from the user-document into a separate collection. This should work by keeping the API the same. Later on I'll find a way for realizing the other improvements I suggested and think of ways to get them working, but the extraction of roles from the user is a major requirement to me as I have an N:N relation here where both of the N's wouldn't be low numbers in the long run.

SimonSimCity commented 5 years ago

The more I think about it, I get on the idea to extend a collection by a set of methods provided by this package. This could cover the approach alanning was suggesting here.

This would then allow users to create permissions per resource, saved on the resource. The field for scope then would most likely be used as a reference to the user (if it now is the user itself or his group). In this approach we'd have to drop that the system grants permission if the scope is not set.

On the other side, our system of permissions is quite fine-grained, which makes it quite flexible but again performance-heavy if we'd take this approach, I feel ...

I'd now start with creating a document per assignment in a new collection called role-assignment.

SimonSimCity commented 5 years ago

I've just finished a PR (#276) which addresses the first two items on my list.

Addressing the 3rd would be done by adding my own logic by wrapping _addUserToRole in a custom extension, the same could be done for the 4th one.

The 5th can be achieved (as @mitar suggested) by adding some custom logic around userIsInRole().

This said, getting #276 would open up for me to solve all my concerns and still stick to this package 🎉

SimonSimCity commented 5 years ago

@alanning, @mitar New thought on #276 - and I want to have your feedback on this.

First and foremost: What do you think about the changes so far?

As I just realized, I've only one tight couple towards the Meteor.user collection, which is getUsersInRole(). To realize your thought @alanning, I'd like to drop this method. Doing this would open up a new possibility. It would allow us to give something (user, group, ...) access to something (the scope).

To realize this I'm asking you for a name of this first. Some name which could be generalize a user or a group. Like assignee, actor, individual - do you have better names?

Removing all the user related things from the methods would then be the next part. We can keep them in for backwards compatibility, but the new name would then be chosen accordingly.

I don't need this myself, but I think it would be a big step towards a direction which makes this tool very useful and handy for many. What's your opinion on this? We could easily change it now ...

SimonSimCity commented 5 years ago

@alanning @mitar have you had any time to take a look at it yet?

mitar commented 5 years ago

Frankly, and sadly, I will not have time to look into this in foreseeable future. :-(

SimonSimCity commented 5 years ago

Thanks for saying it out.

@alanning how is it on your side?

I'll soon start putting my version into production. After some rethinking of things back and force, I came to the conclusion that we don't need any additional changes to realize everything I initially asked in this ticket.

I separated it into different PRs to keep it readable and keep the scope of each change low while still fitting one single scope.

I've lived with a custom package to gain the benefits of 2.x, and I'd be open to keep this repository going by releasing my changes as a 3.x and providing support as far as I can. Feel free to respond if you feel you won't have time to review or help me driving the package into the direction you'd like to see it going. Also tell me if you think this goes off too far. In this case, please provide me with details and I'll see how far I can bend to get both ideas aligned.

gaetandezeiraud commented 5 years ago

Hi @SimonSimCity. I see that your PRs are still waiting. You published your version on atmosphere? Thanks!

SimonSimCity commented 5 years ago

@alanning @mitar since both of you don't have much time at hand, would you help me moving this package over to https://github.com/Meteor-Community-Packages and give me access to publish a new version on Atmosphere so people using an older version would get a notification about the release?

alanning commented 5 years ago

Hi @SimonSimCity, happy to help get the package set up in Meteor-Community-Packages. I would keep this repo open and point to that version in this repo's readme.

It looks like you are not a member of the Meteor-Community-Packages group, would you be able to become one? https://github.com/orgs/Meteor-Community-Packages/people

Also, I'd highly recommend that the package we create in that repo be based on the v2 branch. This repo can live on as the v1 maintenance branch.

SimonSimCity commented 5 years ago

Actually I am - maybe it's hidden for you as well 😊

I'd start off publishing the current v2 branch as a second major version followed by my changes as a v3. Publishing my version as v2 doesn't make sense since the v2 branch is stable and different enough to justify a major release.

Please let me know if I can assist you in moving the repo. Also let me know how I can publish a new version on atmosphere.

gaetandezeiraud commented 5 years ago

Hello, So where are you? The new version is online?

Thanks!

SimonSimCity commented 5 years ago

@alanning @mitar I can start publishing a new version, but I need access to the package administration on Atmosphere. When I get this, I'll start publishing the new versions.

gaetandezeiraud commented 5 years ago

Why not publish the new version under a new name? Like ostrio:flow-router-extra?

SimonSimCity commented 5 years ago

@alanning @mitar it's now almost half a year waiting on your action to grant me permission to atmosphere ... If I don't get access by the end of this month, I'll see forking as the best option to continue. This repository can then focus on v1, maybe the unpublished v2 and I'll continue supporting a v3 which includes all the changes I've made.

mitar commented 5 years ago

I am leaving to @alanning to decide about the future of the project. I do not feel comfortable enough to give access for something which feels mostly by @alanning.

mitar commented 4 years ago

I would suggest that we move this project to https://github.com/Meteor-Community-Packages and then leave to @SimonSimCity to merge his changes into the project and continue with it, given ownership of the repository. I think his proposal is good and it seems both @alanning and me do not have really time to deal with this project.

If @alanning agrees, then let's do that?

@SimonSimCity would that be something you would agree to do?

gaetandezeiraud commented 4 years ago

Great solution!

SimonSimCity commented 4 years ago

@mitar yes! Nice to see this moving! Let me know if there's anything I can help you here. Would be nice to get it moving before the end of this month.

mitar commented 4 years ago

I can also help with this a bit. I think we should:

Let's wait for @alanning to OK this plan (I am not an admin of this repository to be able to move it). Maybe you can make me one?

alanning commented 4 years ago

@mitar @SimonSimCity Sounds good! Reference for further discussions: https://github.com/Meteor-Community-Packages/organization/issues/25

mitar commented 4 years ago

I think this has been resolved. Let's move discussion to other issues.

@SimonSimCity feel free to push your stuff to master branch. There is now also v1 and v2 branches with older versions.

SimonSimCity commented 4 years ago

Done, I'll release a v3 soon.