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
Sites may have a view which displays the state a revision/node is in, but not allow the user to administer those states.
Proposed resolution
Remove "admin_permission" from the annotation in ModerationState.php and add an access handler.
Add permission for viewing moderation states, and use this to determine access to view operation. Everything else could still use "administer moderation states".
Remaining tasks
- Agree on approach.
- Implement.
- Test.
Comment | File | Size | Author |
---|---|---|---|
#9 | Screenshot 2016-04-21 10.21.20.png | 166.34 KB | larowlan |
#7 | 2708941-view-state-perm-7.patch | 7.5 KB | acbramley |
Comments
Comment #2
Crell CreditAttribution: Crell at Palantir.net for Acquia commentedWait, currently if you don't have the admin permission you can't view the ER field pointing to a state? Is that what you mean?
ModerationState has no "view" representation (like most config entities), so I'm not sure when the view operation access comes into play.
Comment #3
acbramley CreditAttribution: acbramley commented@Crell, yep, we have a view exposing the moderation state of each revision. As a user with a role that doesn't have the "Administer moderation states" permission, I can't see the moderation state in the view, adding the permission fixes that.
I believe this is due to the entity annotation using the admin_permission = "administer moderation states" key.
Comment #4
acbramley CreditAttribution: acbramley commentedHere's a patch that adds the view permission and an access control handler, including a test.
Comment #5
larowlanI think 'allowedIfHasPermissions' already adds 'cachePerPermissions' could be wrong
whitespace issue
Other than that, looks great to me - RTBC+1
Comment #6
acbramley CreditAttribution: acbramley commented@larowlan, woops you're right, will fix that up
Comment #7
acbramley CreditAttribution: acbramley commentedTidied up as per #5
Comment #8
larowlanFollow up, we should create a base BTB class with this in it
I think this is ready
Will commit later in the week unless someone objects/beats me to it.
Comment #9
larowlanConfirming this ran on bot:
Comment #10
Crell CreditAttribution: Crell as a volunteer commented1) No base classes. Utility traits instead, if appropriate.
2) Do we need an update hook for this, since we're adding a permission?
Comment #11
acbramley CreditAttribution: acbramley commented@Crell,
1) I'm not sure I understand what you mean?1) There's no base test class in my patch so no worries.
2) We definitely could do that, I'm not sure what it would contain though since we're not altering existing functionality (roles would currently need administer moderation states and that still grants view access).
Comment #12
larowlanI don't think we need an update hook.
We should be erring on side of caution - an update hook that grants permissions is bad form in my book
Comment #13
acbramley CreditAttribution: acbramley commented@larowlan, agreed since we aren't altering existing functionality with this patch.
Comment #15
larowlancommitted and pushed - thanks