Missing default permission

[lcharette]

Chat reference: https://chat.userfrosting.com/channel/support?msg=cRbCQToquBj8YLBNR

The uri_role, uri_roles, uri_permissions and view_role_field permissions are missing from the PermissionsTable. They are not in the DefaultPermissions Seed, but still used in the code:

https://github.com/userfrosting/UserFrosting/blob/adb574f378fb0af1c5eaa3be71458869431e7410/app/sprinkles/admin/templates/navigation/sidebar-menu.html.twig#L23

https://github.com/userfrosting/UserFrosting/blob/adb574f378fb0af1c5eaa3be71458869431e7410/app/sprinkles/admin/src/Controller/RoleController.php#L242

It might have been intentional so only the master user can use theses features, but I guess it's up to the end user to decide if they want to add these permissions to other roles? It's probably best to add them by default then making the user add them via a custom seed. Note if they are not added to any role by default, so only the master user should be allowed to add them by default, thus not creating a security issue.

It would be tricky to add them to V4 install, as seeds are technically already run. This should also affect V5.

Backlog search for uri_roles:

• Permission added here to the Twig Template : https://github.com/userfrosting/UserFrosting/commit/dde5c28f3fe2a9a0cb3252555d9588ea00d8c388
• https://github.com/userfrosting/UserFrosting/commit/599635b94db1669b0a4aba915427df7e9599e9cb#diff-b4a739d3524b1a97d609519eb121fae76bdad804f53aab5a83d4b7d45605a48bR217

[StrykeSlammerII]

It might have been intentional so only the master user can use theses features, but I guess it's up to the end user to decide if they want to add these permissions to other roles?

I second this. As a dev, if I see a role in existing code, I expect I can use it myself in other code. If it was intended to be a "master-only" placeholder, I would have expected a comment somewhere explaining this. (Why bother with different slugs if they are all master-only?) I also don't see any reference in /learn :slightly_frowning_face:

As-is, it almost looks like the permissions were just forgotten between using them and making the PermissionsTable. :thinking:

[lcharette]

As-is, it almost looks like the permissions were just forgotten between using them and making the PermissionsTable. 🤔

From the search I did in the code history, that seems the case,

[lcharette]

I've reviewed this and this is indeed not something simple to fix. Adding the missing permission to the seed wouldn't help with existing install, as that seed is not designed to be run more than once here :

https://github.com/userfrosting/sprinkle-account/blob/36f573c4c4a1fbdbcd964673ebf25d25f5d1e4f5/app/src/Bakery/CreateAdminUser.php#L71-L75

Those permissions should have never been in a seed, and stayed in a migration... One solution would be to create such a migration, but would be a breaking change, so something for UF5.1;

Other solution would be to simply replace the use of theses permissions, but it doesn't solve the issue if we need to add a permission later...

[lcharette]

I've added the missing permissions to the existing seed in the linked commit above. I also added a note in the upgrade guide to run php bakery seed manually if required. Since these permissions are not added to any role by default (it can be done in the UI), it should be enough and should not be an issue if not done during upgrade.

I still believe permissions should be defined in migrations, but for this version it's be enough. I'll reevaluate later, as I think most permissions will change in V6 anyway.

[lcharette]

Reopening, as I found other missing permissions related to roles : delete_role, update_role_field, etc.

[lcharette]

The other missing permissions have been added to the DefaultPermissions seed.

Since these permissions are not added to any role by default (it can be done in the UI), it should be enough and should not be an issue if not done during upgrade.

I ended up also adding the missing permissions to the site-admin role by default. It still won't be an issue for existing installation as long as the seed is not manually run, which is an acceptable risk IMO. However, for new install, I believe it makes more sense for "site admin" to have pretty much every permissions (right now the only permissions not applied by default to the site admin role should be view_system_info and clear_cache), as the root user is not intended for everyday usage (https://github.com/userfrosting/UserFrosting/issues/464).

FYI (or as a note to my future self), permissions for "admin sprinkle stuff" are in "Account sprinkle", which doesn't make sense.