Remove unused / update menu permissions

[Ruslan-Aleev]

What does it do?

Remove unused menu permissions:

• about;
• credits;
• export_static;
• menu_security;
• menu_support;
• menu_tools;

Update menu permissions:

• menu_site;
• menu_trash;
• components;
• for Settings menu (cog icon);
• for Media menu;
• for Access menu;

Brought permissions to a single format 'menu_*', added permissions for menu folders. For example, the entire settings folder (cog icon) was hidden when disabling the system settings item permission, because the same permission settings was set (although there was a separate menu_system permission, but for some reason it was not used).

Related issue(s)/PR(s)

https://github.com/modxcms/revolution/issues/14498

[matdave]

While I like this, I'm having trouble getting the 3.0.0-policy-permissions-menu-update.php file to actually correct my permissions on an update. Additionally, It is possible for someone to have duplicated or created a new Admin Policy Template, in which case I believe this would only correct one template (if it fired), since it uses getObject.

[matdave]

I just tested, these should be in the 3.1.0-pl upgrade file.

[Ruslan-Aleev]

Additionally, It is possible for someone to have duplicated or created a new Admin Policy Template, in which case I believe this would only correct one template (if it fired), since it uses getObject.

I don't quite understand, isn't the permission key enough to update/delete it? The template shouldn't play a role, or am I wrong?

[matdave]

I don't quite understand, isn't the permission key enough to update/delete it? The template shouldn't play a role, or am I wrong?

The primary key of Access Permissions is their ID. When you duplicate a policy, all of their permission names are duplicated as well. So it is possible for there to be multiple entries for 'about', 'credits', 'export_static', 'menu_security', and 'menu_support' for example.

[Ruslan-Aleev]

So it is possible for there to be multiple entries for 'about', 'credits', 'export_static', 'menu_security', and 'menu_support' for example.

I still don't understand the problem, wouldn't a request like $modx->getObject(modAccessPermission::class, ['name' => 'about']); get all about entries?

[matdave]

I still don't understand the problem, wouldn't a request like $modx->getObject(modAccessPermission::class, ['name' => 'about']); get all about entries?

No, getObject gets a single object. It is the same as adding limit->(1) to a query

[matdave]

Basically, I'd recommend changing:

$permission = $modx->getObject(modAccessPermission::class, ['name' => $permissionItem]);

    if ($permission instanceof modAccessPermission) {

To

$c = $modx->newQuery(modAccessPermission::class);
$c->where(['name' => $permissionItem]);
$permissions = $modx->getIterator(modAccessPermission::class, $c);

foreach($permissions as $permission) {

    if ($permission instanceof modAccessPermission) {

[opengeek]

Further, we need to make sure the permissions being updated are from the appropriate templates. You would need to limit the query by template ids for the targeted template group( s ).

[Ruslan-Aleev]

@opengeek

You would need to limit the query by template ids for the targeted template group( s ).

Why do we need a template limit? I change/delete these permissions everywhere.

[opengeek]

@opengeek

You would need to limit the query by template ids for the targeted template group( s ).

Why do we need a template limit? I change/delete these permissions everywhere.

Because permissions with the same key can have different meanings in different template groups. There may not be any of those in this batch, but users can create their own template groups/templates/permissions and the permission names could match. If they did, you would be changing those permissions and breaking their use of them. So while I get in this case there will probably not be any such conflicts (or at least the chances are very low), we need to treat the situation as if they could and write the upgrade scripts appropriately.

[Ruslan-Aleev]

@matdave @opengeek I think I fixed it (as far as I understood), check.

[theboxer]

@Ruslan-Aleev can you please rebase this PR to the current 3.x branch, or do you want me to handle that?

[Ruslan-Aleev]

@theboxer Corrected, please check.

[Ruslan-Aleev]

I'll have to edit it again after updating the 3.x branch, I'll do it in a few days.

[rthrash]

Hi @Ruslan-Aleev will you have a chance to get to this soon? We've only got one additional PR to sort out plus this one to get to the 3.1 release.

[Ruslan-Aleev]

@rthrash @theboxer @matdave @opengeek Conflicts corrected, please check.

[theboxer]

Further, we need to make sure the permissions being updated are from the appropriate templates. You would need to limit the query by template ids for the targeted template group( s ).

@opengeek Are you sure this change won't break custom permissions? You can easily create a custom policy template and add there regular permissions like components, which will control same thing as components in pre-installed policy templates.

So with the limit to update only built in policy templates, all custom ones will remain untouched and permissions may/will break for users.

The fact that the custom permissions may match those renames is quite dangerous on it's own.

My vote would be to NOT rename any permissions as it's very dangerous and unsafe land.

For removing unused permissions, that's also debatable. As you can do basically anything asa MODX admin, you can easily tie custom menu entries/CMPs into existing permissions. Should you do that? Probably not, but nothing stops you (and I don't think we warn somewhere from this practice). Should an upgrade potentially leak parts of admin area to unauthorised users? Probably not.

[Ruslan-Aleev]

The fact that the custom permissions may match those renames is quite dangerous on it's own.

This could happen, but I think it's very unlikely.

The problem is that the access settings are now just a mess, without logic, and it would be better to put things in order there, and that's what I'm thinking of doing.

[smg6511]

The problem is that the access settings are now just a mess, without logic, and it would be better to put things in order there

I do very much agree with @Ruslan-Aleev on this, and we shouldn't let ourselves get stuck with the way it is because it's cumbersome to change. However, like @theboxer was pointing out, what we can't account for in an upgrade script are CMPs, static snippets, or static plugins where existing permissions might be referred to and relied upon. So maybe the strategy should be to:

1. agree upon these name changes and deletions in principle (and perhaps even more changes if desirable, since this would be a bit of a longer term process);
2. begin issuing deprecation notices and target a future release for implementation; notification could be output in the log during installation, in the website download area, the release notes, and wherever else might be helpful. This would need a preliminary PR with logic for any notifications within the realm of installation; and
3. issue a final PR ahead of the targeted release to implement the changes and remove the preliminary PRs notifications.

@theboxer - I wonder if you could expand upon your thought re leaking parts of the admin area, I wasn't sure how you thought that might happen but it'd be good to know and avoid ;-)