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

  1. Agree on approach.
  2. Implement.
  3. Test.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

acbramley created an issue. See original summary.

Crell’s picture

Wait, 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.

acbramley’s picture

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

acbramley’s picture

Here's a patch that adds the view permission and an access control handler, including a test.

larowlan’s picture

  1. +++ b/src/ModerationStateAccessControlHandler.php
    @@ -0,0 +1,41 @@
    +    $admin_access = AccessResult::allowedIfHasPermission($account, 'administer moderation states')->cachePerPermissions();
    

    I think 'allowedIfHasPermissions' already adds 'cachePerPermissions' could be wrong

  2. +++ b/tests/src/Functional/ModerationStateAccessTest.php
    @@ -0,0 +1,132 @@
    \ No newline at end of file
    

    whitespace issue

Other than that, looks great to me - RTBC+1

acbramley’s picture

Status: Needs review » Needs work

@larowlan, woops you're right, will fix that up

acbramley’s picture

larowlan’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/tests/src/Functional/ModerationStateAccessTest.php
@@ -0,0 +1,133 @@
+  protected function createNodeType($label, $machine_name) {
+    /** @var NodeType $node_type */
+    $node_type = NodeType::create([
+      'type' => $machine_name,
+      'label' => $label,
+    ]);
+    $node_type->setThirdPartySetting('workbench_moderation', 'enabled', TRUE);
+    $node_type->save();
+
+    return $node_type;

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

larowlan’s picture

Confirming this ran on bot:

Crell’s picture

1) No base classes. Utility traits instead, if appropriate.

2) Do we need an update hook for this, since we're adding a permission?

acbramley’s picture

@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).

larowlan’s picture

I 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

acbramley’s picture

@larowlan, agreed since we aren't altering existing functionality with this patch.

  • larowlan committed 661c210 on 8.x-1.x authored by acbramley
    Issue #2708941 by acbramley, larowlan: Allow roles to view but not...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

committed and pushed - thanks

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.