meteor-roles icon indicating copy to clipboard operation
meteor-roles copied to clipboard

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

Open ggerber opened this issue 5 years ago • 5 comments

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'})

ggerber avatar Apr 05 '20 08:04 ggerber

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.

SimonSimCity avatar Apr 06 '20 19:04 SimonSimCity

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.

ggerber avatar Apr 07 '20 06:04 ggerber

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.

SimonSimCity avatar Apr 07 '20 07:04 SimonSimCity

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 [email protected] 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 .

ggerber avatar Apr 07 '20 08:04 ggerber

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 ...

SimonSimCity avatar Apr 07 '20 08:04 SimonSimCity