Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
ggerber opened this issue Apr 5, 2020 · 5 comments

Comments

@ggerber
Copy link

ggerber commented Apr 5, 2020

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
Copy link
Member

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
Copy link
Author

ggerber commented Apr 7, 2020

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
Copy link
Member

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
Copy link
Author

ggerber commented Apr 7, 2020 via email

@SimonSimCity
Copy link
Member

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: #284 Feel free to get involved here ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants