Skip to content

Feature #1106: Limit admins to stores PART II#1178

Open
bandieramonte wants to merge 17 commits intosmartstore:mainfrom
bandieramonte:feature/multi-store-customers
Open

Feature #1106: Limit admins to stores PART II#1178
bandieramonte wants to merge 17 commits intosmartstore:mainfrom
bandieramonte:feature/multi-store-customers

Conversation

@bandieramonte
Copy link
Copy Markdown

This PR is the mandatory continuation of PR #1113, and intends to finish implementation of feature #1106.
While the previous PR only added filtering for orders, so that customers can only view orders from stores they have access to, this work was far from complete. This PR completes implementation by adding the same filtering to the rest of the models present in the dashboard:

  • products
  • categories
  • manufacturers
  • checkout attributes
  • shipments
  • customers
  • online customers
  • customer reports
  • bestsellers
  • the dashboard home is also filtered, including the statistics in the bottom right corner.

If a customer has access to all of the existing stores, then nothing is filtered away (it works as it originally did).

In addition, the customer role Super Admin has been further developed, to help manage administer these new store restrictions, as follows:

  • Super admins have access to all stores, plus they can also restrict themselves to certain stores just to play with the dashboard and view statistics for certain stores. They are able to add themselves access back to some or all of the stores.
  • Admins have access only to the stores the super admins gave them access to. So super admins control access to admins.
  • Admins cannot give themselves access to additional stores, not can they set themselves as super admins.
  • On a fresh install of Smartstore, there will be no super admins. In this case, any admin is able to give itself super admin privilege. Once at least one super admin exists, this is not anymore possible.
  • Only Super admins can create other super admins.
  • Admins cannot edit super admin customer roles, whereas super admins can edit any roles.
  • Admins cannot see, create or edit super admin customers.

Finally, this PR introduces the implementation restricting customers to create any of the aforementioned models only to stores it has access to. Super admins are able to create any model to any stores. Likewise, when editing any model, non super admins can only add stores they have access to.

@bandieramonte
Copy link
Copy Markdown
Author

@mgesing @Michael-Herzog
I just resolved the conflicts of this branch and it's ready for your review.

@mgesing
Copy link
Copy Markdown
Contributor

mgesing commented Apr 5, 2025

I don't mean to be negative here, as you obviously put a lot of effort into developing this feature, but the solution you chose is the wrong one.

IStoreRestricted has nothing to do with checking administrator permissions. It is used to control the existence of entities in the frontend depending on the current store. Combining the two things leads to a lot of unattractive extra code that is, by the way, irrelevant in single-store mode. With so many changes, it is not possible for me to monitor or assess what their impact is and where there might be problems. For me, accepting your pull request would be a case of "closing my eyes and hoping for the best".

For example, it is unclear what this MediaGallery.cshtml customization has to do with the actual topic. I miss a centralized approach in the code to reduce the many permission checks and data filters. I am therefore concerned that the amount of code required to implement this feature consistently will continue to grow significantly. Some entities are covered here, but what about those that the core does not have access to? Permission checks should not be limited to action methods, as entities can also be accessed via import or Web API.

An admin should not be limited in his permissions. They should be able to do everything in the store without exception. It would be a breaking change to restrict this. I also don't like the idea of introducing an Admin and a Super-Admin to compete in their permissions. This again leads to unattractive extra code. A super administrator is a special case. A role that I would save for as few, very special cases as possible, and only use when something cannot be implemented via an admin role. For example, we once used a SuperAdmin to control special functions (e.g. executing an automatic database reset) of a demo backend store via a plugin that only exist in this particular demo store.

As a solution, I would have pursued an extension of the permission system and introduced a new system CustomerRole, e.g. "Shopkeeper" or "StoreAttendant", which is only used in a multistore environment. Then I would have added a field "StoreId" to the CustomerRole entity to control which store the permissions are restricted to. And with this base I would have started to extend the PermissionService as the central point for any kind of permission checks. What would have to be done additionally (and what you've already covered) is to extend the data queries so, for example, that the storekeeper only sees the data in the data grids for which he has read permission.

@bandieramonte
Copy link
Copy Markdown
Author

bandieramonte commented Apr 8, 2025

Hi @mgesing,
Thank you for taking your time reviewing this PR and for your detailed explanations.

To begin with, MediaGallery.cshtml customization was an accidental commit which I had reversed in this one on Oct 14, 2024.

Second, I want to bring to attention that, as stated in this PR's description, this PR is the continuation of PR 1113, which had been merged on June 24, 2024. That PR follows the same principles which were objected to here.

Lastly, your feedback about how to properly do this change is very useful and I wish I had known this before working on these changes. I did extend PermissionService by creating the new function CanAccessEntity intending to have a centralized approach of reducing permission checks. However, some entities are built differently and so I was forced to create a few specific checks outside of this class for those cases.
The admins can still do everything. The challenge was that I did not want an admin to give himself the permission to access any shop it wants. The idea is that there are many shops, and because of GDPR, an admin can only have access to a certain shop. If we still let admins do anything, then we can break the legal requirement if we let admins change their access so they can see shops they are not supposed to. This is why I thought of the role of a super admin only to control the access of shops to the admins.
Perhaps I should have let the admin access all shops and instead created a new "Manager" role to access only the shops the admin allows them to?

If ever I find the time to redo these changes, I will definitely keep in mind your constructive feedback and check in new commits to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants