-
-
Notifications
You must be signed in to change notification settings - Fork 436
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
Fix admin menu ACL mismatch in OAuth (#3272) #3273
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to change the menu's children, will it break BC? I prefer to leave it as is for backward compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Menu children needs to be modified as ACL needs to match menu items. Backward compatibility is preserved but if some vendors already hooked these rarely used OAuth menu nodes with their extensions parents would need to be adapted. If we would try to adapt ACL nodes instead admins with REST access given previously still won't be able to see these positions and exception would be logged in https://github.com/OpenMage/magento-lts/blob/main/app/code/core/Mage/Admin/Model/Resource/Acl.php#L135 as long (custom) admin role would not be resaved.
BTW. it's worth to mention that I've spotted the issue thanks to the catch mentioned by @fballiano in https://github.com/OpenMage/magento-lts/pull/2339/files#diff-1048571d4baf38d246708d5953af464948b5591c2c8d648036394d6101ba8022 while actually I consider deleting bad entries automatically better solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For matching nodes, I would prefer to change the acl
node to match the menu
node:
- menu node follows the snake_case standard format
- the acl node doesn't work anyway, so it can be changed without BC issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, if so let's merge #3274 instead but I hope you are aware that in this approach https://github.com/OpenMage/magento-lts/blob/main/app/code/core/Mage/Admin/Model/Resource/Acl.php#L135 would start logging exceptions about missing resources unless custom roles would be resaved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
G8. I haven't seen that upgrade script before. So we have a deal here where additionally admin users would not automatically regain access to these OAuth areas before superadmin re-consent. I've added a file in Mage_OAuth as in opposition to many other core modules it hosts admin controllers on it's own (like Mage_Oauth_Adminhtml_Oauth_ConsumerController)
Replaced with PR #3274 |
Description (*)
This PR fixes ACL issue in Mage_Oauth adminhtml.xml that causes silent Zend_Acl_Exception throw on every admin page and no access to REST consumers/tokens for users with custom role resource access
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)