Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
This isn't a bug report - it would just be nice if we could open up permissions for the log clone action a bit more. In fact we'll need this for farmOS :-)
The route to the log clone confirmation form is what is requiring the administer log permission right now.
In chat @pcambra mentioned making a new dedicated permission "clone log". That might be the best option?
While working on another action I referenced the core entity DeleteAction which uses a similar tempStore approach as the clone log action to save the selected entities before redirecting to a confirmation form that loads the entities back up, performs the action, etc... Just want to document what I found because the pieces are scattered over different parts of core which makes it quite hard to put together:
Basically it adds a more complex permission check system - a custom _entity_delete_multiple_access access check service is provided that loads the selected entities from the temp store & verifies the user at least has access to 1 of them (code) before going to the form. The form then provides additional validation to only perform the delete action on the entities the user has access too.
It seems like the form route could have _acccess = 'TRUE' (or at least require an authenticated user?) and just allow the form logic to handle the rest of the permission checking.. the nature of this just makes it a bit complicated.
Steps to reproduce
N/A
Proposed resolution
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Issue fork log-3209795
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
m.stentaAll sounds good! Just one thought...
Still thinking through all the considerations here... and wondering if it might make more sense to just use a combination of the existing "view" and "create" permissions.
Ultimately, cloning is about reading and writing (reading the existing log and creating a new one with all the same data). If we have a single "clone logs" permission, it seems like that would add an implicit assumption that anyone with the permission therefore has the "view" permission. And maybe that's not always going to be the case.
For example, imagine in the future a user has very limited permissions, and can't see all logs - just a subset - but they need to be able to clone their own logs. A single "clone logs" permission would grant them too much access. They would probably need a "clone OWN logs" permission, or something like that.
Alternatively, maybe we don't need a dedicated permission for cloning, and instead we just have some access checking in the "clone log" form that checks that the user has access to a) View the log being cloned, and b) create new logs.
This would keep the permissions simpler, and allow for a wider range of possibilities I think? Does that make sense?
Comment #4
paul121 CreditAttribution: paul121 commentedYup!
The change to require both "view" and "create" permissions for the log clone action was pretty easy. I modified the clone action's
accessmethod and included additional permission checking the clone action form. Then there is the issue of the route permission... I found that there is a_entity_create_any_accessaccess check we can use there which should work. The form permission checks will prevent editing a log that the user doesn't have access to.This same issue was effecting the log reschedule action as well. I added similar permission checking to the reschedule action form and changed the route requirement to
_user_is_logged_in = 'TRUE'. Unfortunately there isn't aupdate any logpermission, and we can include a specific bundle in the route eg:update any {bundle} log. Because there is additional permission checking in the action form itself it should be OK to allow access to authenticated users.If we want anything better than this I think we'll need to create a custom
access_checkthe same way the core_entity_delete_multiple_access_checkaccess check works. It's either that or figure out a different mechanism to redirect the user to action form.. but I'm not sure we have much control over this.Comment #5
m.stentaThanks for digging into this @paul121! Makes good sense! I just noticed one small thing (commented in MR) that needs fixing. Changing this to "Needs Work".
Comment #6
m.stentaI added a commit and force pushed to address the point above.
@paul121 - Want to take a quick look and give a final thumbs up before I merge this?
Comment #7
paul121 CreditAttribution: paul121 commentedLooks good, but for rescheduling we should make that timestamp access check change on the Action plugin too: https://git.drupalcode.org/project/log/-/blob/2.x/src/Plugin/Action/LogR...
Comment #8
paul121 CreditAttribution: paul121 commentedOh nevermind, looks like this will be tackled here: #3226303: Log reschedule action requires edit access to status field
Comment #10
m.stenta