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.

Sign up for GitHub

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

Jump to bottom

[3.0.x.x] Extension Uninstallers not removing access/modify rights #13584

Closed
mhcwebdesign opened this issue Jan 18, 2024 · 4 comments
Closed

[3.0.x.x] Extension Uninstallers not removing access/modify rights #13584

mhcwebdesign opened this issue Jan 18, 2024 · 4 comments
Labels
3.0.x.x Affects the 3.0.x.x maintenance version

Comments

@mhcwebdesign
Copy link
Collaborator

An extension's uninstall method doesn't currently remove its access rights.

For example, when uninstalling an extension/module/filter via the admin backend's uninstall at Extensions > Extensions > Modules > Filter Uninstall it still keeps the access rights, and next time it gets installed at Extensions > Extensions > Modules > Filter Install, it just adds its access/modify rights another time. Hence the DB table oc_user_group gets multiple entries for 'extension/module/filter' for the 'access' and 'modify' in the json field oc_user_group.permission.

Proposed remedy:

In system/config/admin.php add an event handler for the 'controller/extension/*/uninstall/after' trigger:

....
// Action Events
$_['action_event'] = array(
	'controller/*/before' => array(
		'event/language/before'
	),
	'controller/*/after' => array(
		'event/language/after'
	),
	'controller/extension/*/uninstall/after' => array(
		'event/uninstall/after'
	),
	'view/*/before' => array(
		999  => 'event/language',
		1000 => 'event/theme'
	)
);
....

And this new event handler would do something like this:

		....
		if (isset($this->session->data['success'])) {
			// remove access rights
			$this->load->model('user/user_group');
			$user_groups = $this->model_user_user_group->getUserGroups();
			foreach ($user_groups as $user_group) {
				$user_group_id = $user_group['user_group_id'];
				$permission = $user_group['permission'];
				if (!empty($permission)) {
					$permission = json_decode($permission,true);
					if (!empty($permission['access'])) {
						$this->model_user_user_group->removePermission($user_group_id, 'access', $route);
					}
					if (!empty($permission['modify'])) {
						$this->model_user_user_group->removePermission($user_group_id, 'modify', $route);
					}
				}
			}
		}
		....

Any thoughts or feedback on this?
@danielkerr danielkerr added the 3.0.x.x Affects the 3.0.x.x maintenance version label Jan 18, 2024
@ADDCreative
Copy link
Contributor

I see what you mean. Ideally the remove would need to be added to each extension type's uninstall.

Some extension types add more than one path.

opencart/upload/admin/controller/extension/extension/analytics.php

Lines 23 to 28 in de1c968

$this->model_user_user_group->addPermission($this->user->getGroupId(), 'access', 'extension/analytics/' . $this->request->get['extension']);
$this->model_user_user_group->addPermission($this->user->getGroupId(), 'modify', 'extension/analytics/' . $this->request->get['extension']);
// Compatibility
$this->model_user_user_group->addPermission($this->user->getGroupId(), 'access', 'analytics/' . $this->request->get['extension']);
$this->model_user_user_group->addPermission($this->user->getGroupId(), 'modify', 'analytics/' . $this->request->get['extension']);

@mhcwebdesign
Copy link
Collaborator Author

@ADDCreative : I think you are right. Perhaps it might be good to create a new function in the admin/model/user/user_group.php, like this:

	function removePermissionByRoute( string $route ): void {
		$user_groups = $this->getUserGroups();
		foreach ($user_groups as $user_group) {
			$user_group_id = $user_group['user_group_id'];
			$permission = $user_group['permission'];
			if (!empty($permission)) {
				$permission = json_decode($permission,true);
				if (!empty($permission['access'])) {
					$this->removePermission($user_group_id, 'access', $route);
				}
				if (!empty($permission['modify'])) {
					$this->removePermission($user_group_id, 'modify', $route);
				}
			}
		}
	}

and then use it in the relevant uninstalls in the admin/controller/extension/extension/*.php files.

@ADDCreative
Copy link
Contributor

@mhcwebdesign Yes, something like that would do the job.

Looking at the extra 'Compatibility' routes add by each install in admin/controller/extension/extension/. I can't see what they are for. If you goto the User Groups in the admin and save they seem to get removed.

@mhcwebdesign
Copy link
Collaborator Author

I have now merged some bugfixes for this to the 3.0.x.x branch, see #13602

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3.0.x.x Affects the 3.0.x.x maintenance version
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@ADDCreative @mhcwebdesign @danielkerr