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)

#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.

[matdave]

• Revert setup/includes/upgrades/mysql/3.0.0-pl.php
• Create a link in setup/includes/upgrades/mysql/3.1.0-pl.php to the 3.1.0-policy-permissions-menu-update.php

[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.

The requested change was addressed.

[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.