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

refactor: remove "owner" and other default permissions / groups #6063

Merged
merged 32 commits into from
Jan 31, 2020

Conversation

kieckhafer
Copy link
Member

@kieckhafer kieckhafer commented Jan 29, 2020

Resolves #6030
Impact: breaking|major
Type: feature|refactor

Issue

We'd like to move away from the all encompassing "owner" role.

Solution

  • remove the code which adds users to an "owner" role when creating an account
  • remove code which adds user to "customer" role when creating a non-owner account, users no longer are required to be in a group
  • add code which adds first use to system-manager and accounts-manager GLOBAL groups, and also creates those groups if they do not already exists
  • removes default creation of guest and customer groups
  • remove restriction of inviting user to owner group (meaning we can have more than one owner)
  • remove isOwner logic from canAddAccountToGroup, and leave only policy validation
  • remove owner "override" in hasPermissions
  • remove any code related to owner permissions that should no longer be there

Breaking changes

  • owner is no longer a permission. Since this permission was basically a key to do anything in the app, if you had custom code that was passing owner for permission checks, you'll need to update to make sure the user has more specific permissions

Testing

  • make sure all places where "owner" was being used as the permission still work with more specific permissions

@kieckhafer kieckhafer requested a review from aldeed January 30, 2020 00:05
@kieckhafer
Copy link
Member Author

@aldeed overall I think this is good for a look. Two places I wasn't 100% sure what to do:

  1. I don't think anyone is going to have anonymous anymore, can we remove this? https://github.com/reactioncommerce/reaction/blob/refactor-removeOwner/src/core-services/cart/mutations/reconcileCarts.js#L48:L48
  2. I think you mentioned in another PR we might not even use this anymore? Can this be removed? https://github.com/reactioncommerce/reaction/blob/refactor-removeOwner/src/plugins/notifications/util/sendNewOrderNotifications.js#L57:L57

@aldeed
Copy link
Contributor

aldeed commented Jan 30, 2020

@kieckhafer Both of those code areas will be gone if you merge in release-3.0.0. I had removed them in #6056, which is merged now.

Copy link
Contributor

@aldeed aldeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kieckhafer Looks like you hit everything in the list here. I left a few suggestions, and I'll do some more testing before we merge this.

@kieckhafer
Copy link
Member Author

@aldeed addressed all your comments, and made a few more changes regarding global permission in permissionsByUserId

@kieckhafer kieckhafer requested a review from aldeed January 30, 2020 23:21
@aldeed aldeed merged commit d2c6fb7 into release-3.0.0 Jan 31, 2020
@aldeed aldeed deleted the refactor-removeOwner branch January 31, 2020 16:27
@kieckhafer kieckhafer mentioned this pull request Feb 4, 2020
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