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

Allow custom acl path in adminhtml.xml by using <acl> tag #2321

Closed
wants to merge 1 commit into from

Conversation

justinbeaty
Copy link
Contributor

@justinbeaty justinbeaty commented Jul 15, 2022

Description (*)

I have not fully tested this change yet, but wanted to seek feedback on if this is a good idea or not.

This PR will let you set a custom ACL resource_id in adminhtml.xml that is different than the XML structure. For example:

<acl>
    <resources>
        <admin>
            <children>
                <catalog translate="title" module="catalog">
                    <title>Catalog</title>
                    <sort_order>30</sort_order>
                    <children>
                        <attributes translate="title">
                            <title>Attributes</title>
                            <children>
                                <product translate="title">
                                    <title>Products</title>
                                    <children>
                                        <attributes translate="title">
                                            <title>Attributes</title>
                                            <acl>admin/catalog/attributes/attributes</acl>
                                        </attributes>
                                        <sets translate="title">
                                            <title>Attribute Sets</title>
                                            <acl>admin/catalog/attributes/sets</acl>
                                        </sets>
                                    </children>
                                </product>
                                <category translate="title">
                                    <title>Categories</title>
                                    <children>
                                        <attributes translate="title">
                                            <title>Attributes</title>
                                            <acl>admin/eav/catalog_category/attributes</acl>
                                        </attributes>
                                        <sets translate="title">
                                            <title>Attribute Sets</title>
                                            <acl>admin/eav/catalog_category/sets</acl>
                                        </sets>
                                    </children>
                                </category>
                            </children>
                        </attributes>
                    </children>
                </catalog>
            </children>
        </admin>
    </resources>
</acl>

Note the lines:

<acl>admin/catalog/attributes/attributes</acl>
<acl>admin/catalog/attributes/sets</acl>
<acl>admin/eav/catalog_category/attributes</acl>
<acl>admin/eav/catalog_category/sets</acl>

I think this could be a useful addition because in PR #2317 we may want to change the menu structure, but do not want to break current ACL permissions. Also, that PR expects to find generic EAV permissions at admin/eav/$ENTITY/attributes and admin/eav/$ENTITY/sets but we don't want to force the user to conform to our menu or ACL structure.

Edit: this PR is actually not necessary for #2317 but I will leave this PR open in case there might be other use-cases for changing the ACL path.

Related Pull Requests

#2317

Fixed Issues (if relevant)

Manual testing scenarios (*)

  1. Create a role with Catalog > Attributes > Attributes & Catalog > Attributes > Sets allowed
  2. Update the Mage/Catalog/etc/adminhtml.xml to reflect a different menu and ACL structure
  3. Apply this patch and make sure the permissions are still allowed

I also should test to make sure nothing else breaks with the API/API2 module. I honestly don't use either of those API modules nor do I use ACL often, so if anyone can see where this might break something, please let me know. I do not expect this PR to be merged quickly.

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

Comment on lines -169 to +176
$result[$resourceName]['name'] = Mage::helper($module)->__((string)$resource->title);
$result[$resourceName]['level'] = $level;
$result[$aclpath]['name'] = Mage::helper($module)->__((string)$resource->title);
$result[$aclpath]['level'] = $level;
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 am not positive it's okay to change the array key here. Before I had something like:

$result[$resourceName]['name']  = Mage::helper($module)->__((string)$resource->title);
$result[$resourceName]['level'] = $level;
$result[$resourceName]['aclpath'] = $aclpath; // New array value

But that will require more code changes in core. For example, in the admin role edit screen:

foreach ($rules_set->getItems() as $item) {
$itemResourceId = $item->getResource_id();
if (array_key_exists(strtolower($itemResourceId), $resources)) {
if ($item->isAllowed()) {
$resources[$itemResourceId]['checked'] = true;
array_push($selrids, $itemResourceId);
}
}
}

We only check array_key_exists, but would need to also check for aclpath sub-array key.

@fballiano
Copy link
Contributor

IMHO I wouldn't implement this new feature since it addresses a situation that almost never happens and (if people start using it) it may lead to cumbersome situations with ACLs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Admin Relates to Mage_Admin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants