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

Reload admin ACL #1714

Merged
merged 1 commit into from
Oct 11, 2022
Merged

Reload admin ACL #1714

merged 1 commit into from
Oct 11, 2022

Conversation

luigifab
Copy link
Contributor

@luigifab luigifab commented Jul 1, 2021

Description

This PR allow to reload admin acl without logout/login:

  • for associated users when a role is saved
  • when admin user is saved

OpenMage 20.0.13 / PHP 7.4.6 + 8.0.18.

Manual testing scenarios

  • Create a new restricted role (for example, only with access to Customers).
  • Create a new admin user, use the previous created role.
  • Open a private window and login into backend with the previous created user.
  • a/ Update the role of the new user: add access to Sales.
  • Press F5 on the private window, and go to Sales / Orders.
  • b/ Update the role of the new user: remove access to Sales.
  • Press F5 on the private window.
  • c/ Go to the Administrator role: associate the new user.
  • Press F5 on the private window.
  • d/ Go to the restricted role: associate the new user.
  • Press F5 on the private window.
  • e/ Update the new user: set role to Administrators.
  • Press F5 on the private window.

You can add the following line in app/code/core/Mage/Admin/Model/Session.php on line 221 to add a success message when ACL is reloaded: Mage::getSingleton('adminhtml/session')->addSuccess('ACL reloaded.');

    public function refreshAcl($user = null)
    {
        if (is_null($user)) {
            $user = $this->getUser();
        }
        if (!$user) {
            return $this;
        }
        if (!$this->getAcl() || $user->getReloadAclFlag()) {
            $this->setAcl(Mage::getResourceModel('admin/acl')->loadAcl());
            Mage::getSingleton('adminhtml/session')->addSuccess('ACL reloaded.');
        }
        if ($user->getReloadAclFlag()) {
            $user->getResource()->saveReloadAclFlag($user, 0);
        }
        return $this;
    }

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)

@github-actions github-actions bot added the Component: Adminhtml Relates to Mage_Adminhtml label Jul 1, 2021
@colinmollenhour
Copy link
Member

What is the benefit of this? The user's role cannot be changed on that page so the ACL shouldn't need to be updated. Also the response will just be sent next and nothing else until the next request.

@luigifab
Copy link
Contributor Author

luigifab commented Jul 1, 2021

This allows admins to reload ACL without logout.
So filters on grids are not lost.

@colinmollenhour
Copy link
Member

Ahh, I see. The stock ACL reload mechanism is rather broken as you noticed.. For one it doesn't make sense to store the ACL in the session, it should be stored in cache. Also it should be possible for one ACL to be cached for all users and any time any role is updated the cache can be invalidated so the next user's request will refresh the cache for all users. That would solve this problem as well as some others and be faster and less demand on the session backend.

@luigifab
Copy link
Contributor Author

luigifab commented Jul 3, 2021

I found this solution on stackexchange...
Now I checked the login method, and it store ACL data in session:

$this->setAcl(Mage::getResourceModel('admin/acl')->loadAcl());

I searched and I found Mage_Admin_Model_Session::refreshAcl called from controller_action_predispatch event.
So I can $user->getResource()->saveReloadAclFlag($user, 1); for all users associated to a role when it is updated.

@luigifab luigifab changed the title Reload ACL on user save from my account Reload admin ACL Jul 3, 2021
@woutersamaey
Copy link
Contributor

@colinmollenhour This would also help cases where you have a dynamic menu in Admin (i.e. we have a report section that is auto-populated with more SQL reports, depending on some settings and the user).
ACK from me.

@github-actions github-actions bot added Component: Admin Relates to Mage_Admin Component: Api PageRelates to Mage_Api labels Jul 31, 2021
@luigifab luigifab requested a review from Flyingmana October 2, 2021 19:55
@luigifab
Copy link
Contributor Author

Tested again, still works. I'm surprised that "a/" is already working...

fballiano
fballiano previously approved these changes Jun 8, 2022
@fballiano
Copy link
Contributor

I think this PR is a great addition, having to logout/login it's simply not a very nice thing to ask users.

Copy link
Member

@colinmollenhour colinmollenhour left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good changes. 👍

@fballiano fballiano dismissed Flyingmana’s stale review October 11, 2022 20:59

review was based on outdated code

@fballiano fballiano merged commit 27d0727 into OpenMage:1.9.4.x Oct 11, 2022
@github-actions
Copy link
Contributor

Unit Test Results

1 files  ±0  1 suites  ±0   0s ⏱️ ±0s
0 tests ±0  0 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 runs  ±0  5 ✔️ ±0  2 💤 ±0  0 ❌ ±0 

Results for commit 27d0727. ± Comparison against base commit 1641eab.

@luigifab luigifab deleted the reloadacl branch October 3, 2023 20:15
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 Component: Adminhtml Relates to Mage_Adminhtml Component: Api PageRelates to Mage_Api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants