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: get roles / permissions directly from group(s), not user object #6031

Merged
merged 51 commits into from
Jan 24, 2020

Conversation

kieckhafer
Copy link
Member

@kieckhafer kieckhafer commented Jan 15, 2020

Impact: major
Type: feature|refactor

Issue

When a user is first created, the meteor user object is given a roles object, which contains arrays that have all "roles" granted to this user inside it. These roles are given to the user based on which Group they are in, i.e. if a user is in the "Admin" group, then all roles that the "Admin" group has are then attached to the user.

We use this roles object directly in the app, meaning anytime a group has its "roles" updated, we need to loop over every single user inside this group, and update the roles object on the user. This can be taxing considering there might be millions of users that need to be updated.

The name roles was also confusing, as the action was more of a permission than a role itself, with the user being in the "role" to perform the action via the permission.

Solution

  • We are renaming roles to permissions where applicable.
  • Instead of directly checking against the user.roles object, we now add userPermissions onto the context. These permissions are generated by taking the permissions directly from the Group on the fly in buildContext, instead of from user.roles, so if a Group is ever updated, the update is made on the fly when the users context is rebuilt, instead of needing to update every single user.

Things that stuck out to me

  • This PR removes individual shop roles from the user. ALL rules that aren't for the __global_roles__ group are no longer in the users collection on the user. This will mostly affect reaction-admin where we use meteor/alanning:roles to do a lot of permission checks for remaining server meteor methods. There is a corresponding PR in the reaction-admin repo that updates the to remove the use of user.roles from there.
    There are a lot of changes here related to removing the admin and owner permissions, which granted users basically all powers with a single permission, and moving towards a permission system where each resource has a specific permission. The shop-manager and owner groups have all of these permissions on them. In the past, we were checking to see if a user has admin permission on their actual user object, where now we are checking to see if the user belongs to a group that has the specific permission. Confusing at first, but once you see it in action it makes sense.

I spent a lot of extra time trying to think through how to refactor a lot of the "Group" related permissions, as they didn't completely make sense anymore with this refactor. Please take an extra look at all of these queries / mutations (and their related tests):

  • groups query and its related tests. Previously, anyone with the admin role on their account could see all groups, and any user could see the group that they were in. I removed the ability for any user to see the group they were in, and changed the admin permission to show ALL groups to users who are in a group that has the reaction:legacy:groups/read permission attached to it. This is only for viewing groups, there are separate permissions for moving users in and out of groups. (20d93b2)
  • administrators query and its related tests. This query is no longer relevant, as we no longer have an admin permission on a shop. This query should be removed, and the accounts query updated to better query for different user types and permissions. This PR removes the existing query, [3.0.0] Delete administrators queries #6006 will take care of the new queries. (b705751 && eddda56)
  • inviteShopMember now looks for a user that is in a group which has reaction:legacy:groups/manage:accounts and reaction:legacy:accounts/invite:group permissions to check for ability to invite, instead of the owner permission.
  • addAccountToGroup now allows for a user to add an account to any group if the user performing the action has the reaction:legacy:groups"/manage:accounts permission. In the past, the user had to have ALL the permissions of the group they were trying to move the user into. This may need to be revisited.
  • There is one place at this time, updateGlobalSettings, where we use the __global_roles__ permissions from the user. I did not update this at this time, we should re-think how the global roles work. :https://github.com/reactioncommerce/reaction/blob/refactor-kieckhafer-getRolesFromGroupNotUser/tests/integration/api/mutations/updateGlobalSettings/updateGlobalSettings.test.js#L36:L36

Breaking changes

Any custom code directly checking against user.roles for permissions (formerly called roles) will need to be updated to check against context.userPermissions instead. Hopefully this isn’t the case anywhere, as you should almost always be using validatePermissions or userHasPermission to check for authorization, not directly checking the user roles.

Notes

It's possible all the extra calls to the Groups will be taxing on resources. We will look to optimize how the permissions are added to context.userPermissions in the future, but want to move forward with this now.

Testing

  1. See that context.userPermissions contains all the roles of the Group(s) a user belongs too.
  2. Integration (and unit) tests have all been updated and should be a good indication that this is working as expected, but I'd suggest playing around with a few random mutations and queries to double check

…issions

Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
…sions

Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
…oreThanOneGroup' into refactor-kieckhafer-getRolesFromGroupNotUser
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
@kieckhafer kieckhafer changed the title refactor: get roles / permissions directly from group(s), not user object [WIP] refactor: get roles / permissions directly from group(s), not user object Jan 22, 2020
@kieckhafer
Copy link
Member Author

kieckhafer commented Jan 22, 2020

@rosshadden @aldeed re-adding the WIP tag here, some new tests failed once I refactored one last roles related function, which is actually a good thing, points out a few more places where we weren't explicitly setting the permissions needed inside tests. I'll re-add you when it's ready again.

Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
@kieckhafer kieckhafer changed the title [WIP] refactor: get roles / permissions directly from group(s), not user object refactor: get roles / permissions directly from group(s), not user object Jan 22, 2020
Signed-off-by: Erik Kieckhafer <ek@ato.la>
@kieckhafer
Copy link
Member Author

@aldeed @rosshadden ready for a look now.

Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
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.

A few comments. Great progress!

src/core-services/account/index.js Outdated Show resolved Hide resolved
@@ -82,7 +82,7 @@ export default async function createAccount(context, input) {
// The identity provider service gives the first created user the global "owner" role. When we
// create an account for this user, they should be assigned to the "owner" group.
if (authUserId === userId) {
const isGlobalOwner = await context.userHasPermission("reaction:legacy:shops", "owner", { shopId, legacyRoles: ["owner"] }); // TODO(pod-auth): update this permissions check
const isGlobalOwner = await context.userHasPermission("reaction:legacy:shops", "owner", { shopId }); // TODO(pod-auth): update this permissions check
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you are planning to tackle this later, but I'd like to see "owner" go away as a permission and instead just give the first user "create" permission for "shops" as well as maybe a global "update:settings" permission.

So I guess that would mean there should be a global permission group (role) auto-created called "owner" which should have these two permissions, and the first account should be placed in that group upon creation (here)?

As is, this is putting them in a shop-specific group, which we actually may not need to do here (unless there's a specific invite). It seems like part of createShop mutation should put the creator account into that shop's owner group. Then after that invites and groups are done explicitly by that first shop user.

Yikes, I should probably draw this out in a diagram.

Copy link
Contributor

Choose a reason for hiding this comment

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

But this isn't new in this PR, so we can fix in a subsequent PR if you want.

@@ -31,5 +28,7 @@ export default async function canAddAccountToGroup(context, group) {
!!ownerGroup && contextUserAccount && contextUserAccount.groups.includes(ownerGroup._id)) ||
userHasPermission("reaction:legacy:shops", "owner", { shopId }); // TODO(pod-auth): update this to figure out what to do with "owner"

return isOwnerAccount || _.difference(groupPermissions, user.roles[shopId] || []).length === 0;
const hasPermissionToAddAccountToGroup = await isOwnerAccount || await userHasPermission("reaction:legacy:groups", "manage:accounts", { shopId });
Copy link
Contributor

Choose a reason for hiding this comment

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

The big comment above is outdated. Also, if we can get rid of the "owner" permission as I suggested in another comment, then we won't need the isOwnerAccount part here, and this becomes a normal simple permission check.

src/core-services/settings/mutations/updateAppSettings.js Outdated Show resolved Hide resolved
src/core-services/tags/queries/tag.js Outdated Show resolved Hide resolved
src/plugins/legacy-authorization/util/hasPermission.js Outdated Show resolved Hide resolved
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
@kieckhafer
Copy link
Member Author

@aldeed all comments addressed, outside of the owner one, which I think should be done on it's own.

aldeed
aldeed previously requested changes Jan 23, 2020
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.

One part needs a little adjustment

src/core-services/tags/queries/tag.js Outdated Show resolved Hide resolved
Signed-off-by: Erik Kieckhafer <ek@ato.la>
@kieckhafer kieckhafer dismissed aldeed’s stale review January 23, 2020 23:39

@aldeed addressed the last comment

Signed-off-by: Erik Kieckhafer <ek@ato.la>
Signed-off-by: Erik Kieckhafer <ek@ato.la>
@kieckhafer kieckhafer merged commit f9942b1 into release-3.0.0 Jan 24, 2020
@kieckhafer kieckhafer deleted the refactor-kieckhafer-getRolesFromGroupNotUser branch January 24, 2020 23:51
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.

3 participants