-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Bulk ACL management for AsynchronousOperations Admin UI #27580
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
Conversation
Hi @nuzil. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
@magento create issue |
* @return BulkSummaryInterface[] | ||
* @since 100.2.0 | ||
*/ | ||
public function getBulksByUserAndType($userId, $userTypeId); |
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.
Adding the new methods to the interface is backward-incompatible change. Please, introduce new interface with this method.
/** | ||
* @inheritDoc | ||
*/ | ||
public function getBulksByUserAndType($userId, $userTypeId) |
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.
Should be extracted to the separate class
/** | ||
* @inheritDoc | ||
*/ | ||
public function getBulksByUserAndType($userId, $userTypeId) |
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.
Should be extracted to separate class
*/ | ||
class AccessManager | ||
{ | ||
public const BULK_LOGGING_ACL_GUESTS |
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 possible to make constants private?
* @param int $bulkUuid | ||
* @return bool | ||
*/ | ||
public function isAllowedForBulkUuid($bulkUuid) |
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.
I prefer to split this class into three with one method.
@nuzil can you please look at the requested changes? |
Hi @nuzil, I'm closing this PR now due to inactivity. |
Hi @nuzil, thank you for your contribution! |
Hi @engcom-Charlie |
Hi @nuzil. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. |
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.
Moving the PR to the correct state, as @nuzil mentioned, it is being worked on.
Hi, @nuzil will you continue on this PR? |
Hi @nuzil, I'm closing this PR now due to inactivity. |
Hi @nuzil, thank you for your contribution! |
Closed as duplicate #30806. |
Hi @nuzil, thank you for your contribution! |
Description (*)
After Migrating of Asynchronous Operations from Magento Commerce to Magento Open Source, looks like part of functionality was extended.
In details:
In magento_bulk table was added user_type, which defines type of the user who created Bulk Operation.
Possible types are:
In current implementation all Admin UI components have no idea about user type:
https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/AsynchronousOperations/view/adminhtml/ui_component/bulk_listing.xml - in default Grid there are NO DataSource is defined, so Admin see the whole operations, but at the same time, he cannot see Details of those operations:
https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/AsynchronousOperations/Controller/Adminhtml/Bulk/Details.php#L52
But at you can see from implementation,
https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/AsynchronousOperations/Model/AccessValidator.php#L58
that permissions are checked based on UserID and fully ignoring UserType. Which means, that Admin has access to All transactions or all user types with the same ID.
Fixed Issues (if relevant)
Current implementation will add:
Questions or comments
Auto tests still in process, but main implementation can be already reviewed.
Contribution checklist (*)
Resolved issues: