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

Role assign collection #276

Closed SimonSimCity closed 4 years ago

SimonSimCity commented 5 years ago

In addition to my previous PRs, this PR contains a rewrite of the library in which I separated the role-assignments from the user document. This is the first attempt of going into the direction I proposed here: https://github.com/alanning/meteor-roles/issues/270

@alanning @mitar what are your ideas on this? It changes the output of only one of the methods and replaces/removes some of the internal methods - otherwise there are no changes to the API, nor to the tests. All tests are passing.

Due to my changes here you need to publish the role-assignments yourself. See the client-tests as an example.

It still has the ability to move scopes to objects. I thought of also having an object there where the current string could represent the _id property, where you can add your stuff to.

SimonSimCity commented 5 years ago

In case you've already started with the code-review: I've now made it easier for you to get the full perspective of changes. I split up the changes into one commit, which changes all the internal structuring, where the tests are only minimally changed.

In a separate commit I've introduced the only breaking change, which is due to the change of the internal structure. getRolesForUser with using the option fullObjects will change, which makes more sense to me than keeping it as it was before. This, of course, requires a lot of changes in the tests file, which I thought would be good to have separated to show that none of the other public API methods have changed.

SimonSimCity commented 5 years ago

The only thing I'm still concerned about is that a call to Roles.deleteRole() would incrementally take more time the more often this role has been assigned to a user or got adopted by another role which in turn might be assigned to users; the same applies to Roles.renameRole(), Roles.removeRolesFromParent(), Roles.addRolesToParent().

Other methods (like Roles.getScopesForUser()) will grow by nature, as you might expect. Maybe a future version there should return a cursor instead of the strings ...

SimonSimCity commented 5 years ago

I've now integrated these changes into my codebase, which was a POC and also forced me to change a bit of untested things on the codebase, now covered by tests.

In addition I had to do the following in the codebase of my project:

  1. Make sure I do not rely on the internal behavior of the roles-extension (e.g. setting or checking user.roles directly)
  2. Extend the resetDatabase({}) call in my test files because I have a default set of users and permissions across the tests
  3. Update the StubCollections.stub() definition on client-side tests to mock Meteor.rolesAssignment as well

I've also added a migration and rollback script. Please be aware of that a rollback will take significantly longer (10 min migration - rollback 2-3h), so you might be faster (it's a general advice after all) taking a backup of the users collection. The cursor in the rollback script also timed out once for me, which required a restart of the application, after which it continued without anything noticeable on the resulting data. If you want to share it across several instances, make sure that they all are working with different user-ids. The migration part is not covered by tests.

Any user using the platform during migration will encounter a situation where he is not allowed to do stuff which he actually should be able to. If you need a continuous migration, you'd need to extend the script by hand. Just add a custom property instead of removing the roles and have a second migration-script which removes the roles and this extra property after you've verified that your system is working. This way you would not even need a rollback, but only remove the role-assignment collection after rolling back.

Hint: When you're using SimpleSchema on your Meteor.users collection, make sure this allows the saving of the property roles when rolling back! Gladly, it's not the way around - this said, you can disallow saving a roles property on the users collection and still be able to remove it.

mitar commented 4 years ago

This got closed because I renamed v2.0 branch into v2. I think this should go into (new) master branch anyway.