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 and permissions for multi vendor #372

Closed
boboci9 opened this issue Apr 27, 2015 · 8 comments
Closed

Role and permissions for multi vendor #372

boboci9 opened this issue Apr 27, 2015 · 8 comments
Assignees

Comments

@boboci9
Copy link
Contributor

boboci9 commented Apr 27, 2015

Hi Aaron,

As I mentioned on gitter I am working on a PR to help the current version of package permissions work better with the multi vendor solution (it would be a continuation of the PR #128) based on your ideas from #357 but I wanted to clear some of your ideas on the matter before I pull anything. I will list my suggestions please let me know what do you think.

  1. In the Packages publication section we have

    if Roles.userIsInRohle(this.userId, ['dashboard','owner','admin'])
          return Packages.find shopId: shop._id
    

    but what happens when we have multiple shops and the user is the owner of a completely different shop than the shop._id.

    I can think of several possibilities to handle this issue so I wanted to see your point of view on which to include in the PR

    The suggestion would be using the group concept form the meteor-roles package this would work even if we don’t overwrite the whole code at once. This would mean that the shopId would be the name of the group and it would look like the following in the DB:

    user1.roles = {
        'shopId1': ['dashboard','admin']}
    user2.roles = {
        'shopId2': ['owner']}
    

    and we could use Roles.userIsInRole(userId1, ['dashboard','admin']) -> true (this would still work)
    or Roles.userIsInRole(userId1, ['dashboard','admin'], shopId2) -> false

  2. Also here in the Package publications I think we should include a registry check based on the permissions so if the user is not admin we check if he’s a member and check for the userPermissions something like this

        Shops.find().forEach (shop) ->
            member = _.find shop.members, (member) ->
              currentUser is member.userId #this.userId is not defined here
            if member
              Packages.find { shopId: shop._id, 'registry.route' : {$in:member.permissions}}
          return Packages.find { shopId: shop._id},
    

    We would of course have to recheck this on the client side as well because we would have packages returned with several registries but the user might not have access to all so I would check this in the reactionApp helper.

    But there is still an open question for public packages there are some packages that should be present no matter the user permissions, how should we handle these?

  3. Regarding the ReactionCore class properties like userPermissions, in a multi vendor environment should these be calculated by the shop the current user is a member of an not the current shop?

    Something like this in the app.coffee init function

    Shops.find().forEach (shop) ->
                  member = _.find shop.members, (member) ->
                        member.userId is Meteor.userId()
              if member
                self.isMember = true
                self.isAdmin = member.isAdmin
                self.userPermissions = member.permissions
    

Thanks for your help in advance.

@aaronjudd
Copy link
Contributor

Sorry, I refactored your original comment ;-)

I like your suggestions for the package publications (and generally for permissions) - using shopId as a group is the right approach. That's the general direction I had in mind as well .

But there is still an open question for public packages there are some packages that should be present no matter the user permissions, how should we handle these?

I'm not sure, but maybe a role can be defined for the global group, every user getting some wildcard access?

Roles.addUsersToRoles(user, "guest", Roles.GLOBAL_GROUP)

Regarding the ReactionCore class properties like userPermissions, in a multi vendor environment should these be calculated by the shop the current user is a member of an not the current shop?

That's an interesting question, you could also want the user to only be a member of the "marketplace" shop. (never belonging to anything but the marketplace), or even add them at some event, like checkout, to multiple shops.

Where a user is a member of multiple shops, your app.coffee function looks like it would return member of the last shop that the user was a member of - not the current shop, nor the "marketplace" shop. Would that potentially set isAdmin=true for the wrong shop?

If a user was a member of multiple shops, I'd think we'd want to just set permissions for currentShop only adding permissions if user is a member of the current shop, otherwise they are a "guest".

@aaronjudd aaronjudd added this to the Marketplace milestone Apr 30, 2015
@boboci9
Copy link
Contributor Author

boboci9 commented Apr 30, 2015

Great that we are on the same opinion, I will prepare a PR for the issues that I could think about and they are not currently covered.
Regarding 2 this would mean that every every user will be a part of a "guest" role, great idea. When we create the user we will have to add this role.
Regarding 3 about the userPermission the isAdmin flag was the one that confused me a bit in this schema. That would suggest for me that the members are not the buyers but the members of the seller's organization, and this would mean that a user should be a member of only one shop because a seller can have only one shop according to my understanding.
In this case when a shop member logs in, he has in the shop.members.permission array the routes to which he has access to and these should get in the userPermission property, or I misunderstood something?

@aaronjudd
Copy link
Contributor

members are not the buyers but the members of the seller's organization

I think this is correct.

In this case when a shop member logs in, he has in the shop.members.permission array the routes to which he has access to and these should get in the userPermission property

yes, I think this is correct.

I do think a user could be a member of multiple shops with potentially different permissions (what if I had a shop, but you invited me to publish products on your shop).

(my example of promoting users to members after checkout was probably a bad example)

@boboci9
Copy link
Contributor Author

boboci9 commented Apr 30, 2015

Ok, so regarding 3 we set up the userPermissions in the following way:

  • if user is member of a shop we take the permissions from the member array
  • if the user is a member of multiple shops we check if member of main shop than take permissions otherwise empty or a predefined array for guests
  1. Is there a way in the registry.js to set a package permission for a role? If not we can use a predefined guest array and add there the global routes. Or do you have other ideas for that?
  2. Also do you think I should alter the packages publications based on these permission arrays? Or I should leave the publication as it is and add the route/permission check in the reactionApp helper only? I think that would be better to have the packages publication only based on role and shop and if the user is 'guest' we use as it's now:
 return Packages.find { shopId: shop._id},
        fields:
          name: true
          enabled: true
          registry: true
          shopId: true
          'settings.public': true

The reason why I'm asking is that there is a TODO in the packages publication so it's possible it should be checked as well.

 # TODO Filter roles/security here for package routes/template access.
  1. There is an ongoing discussion in our team about what a public package is referring to? I was under the impression that provides=userAccountDropdown packages for example are public packages?

(sorry for the previous unformatted code I just read the documentation about the supported markdowns so this time I used it too)

@aaronjudd
Copy link
Contributor

This is very old code, now that I am digging into it - I think it needs a quick refreshing.

Main thing - I'm thinking we may not need to use shop.members anymore, and can just use the package registry + the roles collections.

I'm about 50% through making this work, I'll publish the branch when its functional again (unless I realize why the current approach was used the first time).

In any case, expect a detailed response to your questions. I just want to make sure my response is working first!

@aaronjudd
Copy link
Contributor

After really digging into the existing roles, I felt this this functionality was probably written before there was groups support, and just in general was lagging behind since the most recent package / registry updates. I had merely kept it running the existing permissions process when I refactored the package registry, and thankfully, it's much easier to manage permissions now using the registry.

I'm not sure why we had the Shops.members functionality, but it's now removed in favor of the roles package methods.

I've refactored to get the current admin working properly just using roles and groups from alanning:roles. The previous implementation wasn't using groups at all, just permissions, so now the permission sets are unique to each group(shop).

We should be able to deprecate packages.permissions, ReactionCore.permissions and shop.members now by reusing the packages.registry routes to establish permissions/roles.

With this approach, you do not need to do anything special to setup permissions for packages, by default we'll use the package.registry.routes to create required permissions.

registry permissions (roles)

server/registry.coffee with optional permission definitions

 registry: [
    {
      route: "dashboard"
      provides: 'dashboard'
      label: 'Core'
      description: 'Reaction Commerce Core'
      icon: 'fa fa-th'
      cycle: 1
      container: "dashboard"
    }
  ]

TODO registry.route and registry.label define the permission and the label. Allow packages.registry.permissions as optional/override.

        permissions: [
        {
          label: "Customers" # use to override a label for a registry label
          permission: "dashboard/customers" # registry route
        }
      ] 

screenshot 2015-05-04 19 20 26

group (by shop)

Note: using shopId as a group, we may need to ensure:

Group names can not start with '$' or numbers. Periods in names '.' are automatically converted to underscores. 

I've updated the packages permissions to be used like this:

Roles.addUsersToRoles([user1, user2], ['dashboard/customers', 'dashboard/orders'], shopId)
Roles.addUsersToRoles(userId, 'admin', Roles.GLOBAL_GROUP)

Since groups are used, we can update the permissions in general publications like:

Roles.userIsInRole(userId, ['admin', currentRoute], shopId))
Roles.userIsInRole(userId, ['admin'], shopId)

To add a new permission (client)

Meteor.call("addUserPermissions",'khzqH84zMetraqkJ6','dashboard/settings/account',ReactionCore.getShopId())

now a role is assigned to the user, specifically for this shop:

Roles.getRolesForUser("khzqH84zMetraqkJ6",ReactionCore.getShopId());
> ["dashboard/settings/account"]

without a shop:

Roles.getRolesForUser("khzqH84zMetraqkJ6")
[]

where groups are represented in the user document as

"roles" : {
    "WvrKDomkYth3THbDD" : [ 
        "dashboard/settings/account"
    ]
}

In templates, we can use the isInRole helper introduced by the roles package:

{{#if isInRole 'editor,user' 'group1'}}
{{/if}}

I've retained the previous ReactionCore methods and Template helpers, although I'm not sure they are really needed.

  hasOwnerAccess: ->
    return Roles.userIsInRole Meteor.userId(), 'owner', @shopId

  # dashboard access
  hasDashboardAccess: ->
    return Roles.userIsInRole Meteor.userId(), 'admin', @shopId

  # permission check
  hasPermission: (permissions) ->
    return Roles.userIsInRole Meteor.userId(), permissions, @shopId

but for now it's a convenient way to override template permissions and should probably just align to the default roles.

#default roles
not defined in the registry, default system roles

  • guest
  • manager
  • administrator
  • owner

I'll further document the default roles - as currently really only admin is used, but I think there should be a small limited number of roles that are used system wide (and not package dependent)

public

There is an ongoing discussion in our team about what a public package is referring to? I was under the impression that provides=userAccountDropdown packages for example are public packages?

    # exposes public settings for packages
    for pkg in enabledPackages
      if pkg?.settings?.public
        for setting, value of pkg.settings.public
          ReactionCore[setting] = value

I'm not even sure. Maybe your interpretation is true - there are "public" packages for the userAccountsDropdown after all. But confusingly - I adopted public in reference to "public settings" as well - the part of the registry that exposes public client variables. We probably just need to clean up references / docs here?

@boboci9
Copy link
Contributor Author

boboci9 commented May 6, 2015

Thanks for your detailed description one more question came up when I went over your description.
Regarding

TODO registry.route and registry.label define the permission and the label. Allow packages.registry.permissions as optional/override.

Should there be also a role in this permissions array next to the label and the route to specify which role is given these permissions?

@aaronjudd aaronjudd self-assigned this May 13, 2015
@aaronjudd
Copy link
Contributor

@boboci9 I hope I answered your question at some time or another. sorry about that. Let's open another issues for enhancements to the permissions from what we have now in 0.6.1

aaronjudd added a commit that referenced this issue Dec 3, 2015
- implement groups
- refactor roles management ui
- remove reactioncore app permissions configuration.
- use registry for package permissions configuration
- updated for issue #372
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

No branches or pull requests

2 participants