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

Update group invitation check to always allow owner #2772

Merged
merged 5 commits into from
Sep 1, 2017

Conversation

impactmass
Copy link
Contributor

@impactmass impactmass commented Aug 30, 2017

Update after #2662 Scoped invitation.

What Happened Earlier:

  • In [Marketplace] Permission to grant scoped permissions should be grantable #2189 we added check so that group invitation is scoped to the permissions that a user has. That means that when a user belongs to a group with role set [a,b,c,d], she can only invite a user to groups with those permissions or a subset of them. We set up Reaction.canInviteToGroup method to do this check.
  • But, there is a case such as where an installed package adds a new role and the author didn't push the role to the owner group using addRolesToGroups. This will leave the owner inadvertently cut out by canInviteToGroup

Changes in this PR:

  • To resolve the above, I added a check to grant access for a user with owner role
  • I also added/copied canInviteToGroup to client Reaction, so it's possible to do Reaction.canInviteToGroup client side. This is useful when deciding dropdown options.
  • I added more comments in the code around all the above

How To test

  • Try multiple scenarios of trying to invite to group as an owner of a shop, either by:
    ** Adding a user to an existing group
    ** Adding a user to a new group created via the Accounts UI
    ** Adding a user to a new group created by Groups.insert(groupData) like in the case of a custom package
    Confirm that an owner can invite, in all those cases.

Confirm that previous functionality stays intact by:

  • Create a group with all Account roles toggled on. Add a user to that group.
  • Login as that user, go to the Accounts dashboard, confirm in the Invite Form that the user doesn't have dropdown for higher groups e.g "Shop Manager"

You can also test all these with a new shop other than the primary shop

@impactmass impactmass changed the title [WIP] Update group invitation check to always allow owner Update group invitation check to always allow owner Aug 30, 2017
Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

Can add to any new group. Looks good.

}

// Users with `owner` and/or `admin` roles can invite to any group
// Also a user with `admin` can invite to only groups they have permissions that are a superset of
Copy link
Contributor

Choose a reason for hiding this comment

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

These two comment lines seem to contradict each other.
Can a user with admin permissions invite to any group, or only groups they have superset of permissions for?

Copy link
Contributor

Choose a reason for hiding this comment

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

@impactmass want to make sure you see this comment as I think these comments should be clarified, but going to merge anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll note to update the comment in next PR I open, but I meant the latter: a user with admin can invite to only groups they have superset of permissions for.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's what I figured. Next PR is good

Copy link
Contributor

@spencern spencern left a comment

Choose a reason for hiding this comment

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

1 confusing comment for me, but looks good and is a good fix.

At some point we should figure out how to split some of the client Reaction methods out of main.js because that file is getting a little long.

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