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
920 stars 164 forks source link

why Roles.setUserRoles(targetUserId, [], scope) instead of roleAssignment.remove({...})? #322

Open ggerber opened 4 years ago

ggerber commented 4 years ago

In the examples it shows the following example to remove a user from a scope // remove roles for target scope Roles.setUserRoles(targetUserId, [], scope)

The problem is that if the scope becomes later meaningless (e.g. the document that scope refers to is removed), then one sits with many orphaned roleAssignment documents referring to a scope that no longer exists.

The edge cases that I can identify are: Is it not better to recommend the following when scope no longer exists(?): db['role-assignment'].remove({scope:'PeKLcyHykdrnkMh9r'})

Is it not better to recommend the following when the member no longer exists(?): db['role-assignment'].remove({'user._id':'rLiT3LT8gSJroXbWF'})

Is it not better to recommend when the member needs to be removed from a scope(?): db['role-assignment'].remove({'user._id':'rLiT3LT8gSJroXbWF', scope:'PeKLcyHykdrnkMh9r'})

SimonSimCity commented 4 years ago

I personally would go for the function Roles.removeScope() in the first use-cases, and Roles.setUserRoles() for the second and third (there in combination with the option anyScope).

It might be weird to set the list of roles to an empty array right before removing a user. The naming is here based on the v2, where roles were just a property on the document of a user. There the naming kind-of made sense. Now in v3 I've refactored this list into a separate collection; now I agree that the naming sounds off. I've left it this way because it's more backwards compatible, but I'm willing to change it in any way if you have a good reason.

Directly accessing the database is something I (as a programmer) want to avoid if the collections are managed by an extension. This way I can be sure I'll be compatible to the next version if they don't change the packages API.

ggerber commented 4 years ago

It also feels weird for me, as a user, to have use two objects (Roles and Meteor.roleAssignment) to manage roles.

Hence, I support extending the API to handle the above use cases.

At the moment I need to manipulate the Meteor.roleAssignment collection to prevent orphaned documents where the scope attribute has become obselete.

SimonSimCity commented 4 years ago

Well, it's not weird to have the two collections. The collection Meteor.roles stores the existance of a role, and Meteor.roleAssignment stores the assignment - the connection between a role and a user, including some meta-data which you can add to it (like e.g. the scope).

As said, if a scope becomes obsolete, you can use Roles.removeScope() to remove all assignments which are on the obsolete scope.

ggerber commented 4 years ago

Apologies,

I thought Roles.removeScope was a proposal for a function, which was not implemented yet (it was not listed in https://meteor-community-packages.github.io/meteor-roles/classes/Roles.html)

If it is implemented then I will happily use it

Thanks

On Tue, 7 Apr 2020 at 09:51, Simon Schick notifications@github.com wrote:

Well, it's not weird to have the two collections. The collection Meteor.roles stores the existance of a role, and Meteor.roleAssignment stores the assignment - the connection between a role and a user, including some meta-data which you can add to it (like e.g. the scope).

As said, if a scope becomes obsolete, you can use Roles.removeScope() to remove all assignments which are on the obsolete scope.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/Meteor-Community-Packages/meteor-roles/issues/322#issuecomment-610234316, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACQLPZS7DKNLXPH5DBB3AM3RLLLQRANCNFSM4MALWEPQ .

SimonSimCity commented 4 years ago

The documentation there still shows the API of v1 🙈- the function Roles.removeScope() was added as part of v2.

A ticket has been opened to discuss which version should be published there in the future: https://github.com/Meteor-Community-Packages/meteor-roles/issues/284 Feel free to get involved here ...