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

Role assign collection #276

Closed
wants to merge 17 commits into from
Closed

Role assign collection #276

wants to merge 17 commits into from

Conversation

SimonSimCity
Copy link
Member

@SimonSimCity SimonSimCity commented Jan 23, 2019

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

- no need for updating tests
- removed tests for outdated internal behavior
- added publication of new collection to test-files
…` = true

- Required a lot of changes throughout the tests, which weren't done earlier to make it easy to comprehend that this feature has no BC breaks aside of this
- Using `fullObjects` will ignorer the value set in `onlyAssigned`, which required to change some of the tests
@SimonSimCity
Copy link
Member Author

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

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 SimonSimCity mentioned this pull request Jan 27, 2019
@SimonSimCity
Copy link
Member Author

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

mitar commented Oct 3, 2019

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

@SimonSimCity SimonSimCity deleted the role-assign-collection branch November 13, 2019 19:15
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

Successfully merging this pull request may close these issues.

2 participants