Problem/Motivation

Adding an optional feature which limits a user's ability to transition content based on the same logic that applies to edit/delete operations would be a great addition to this module.

Proposed resolution

  • Use a ServiceProvider or other means to override the workbench_moderation.state_transition_validation service and add our own which extends workbench_moderation's StateTransitionValidation class.
  • Override the getValidTransitions function (this is what the ModerationForm uses to build out the available transitions, and potentially others to check whether the user has a section that matches up as per edit/delete access checking in this module.
  • Use a separate module for this integration so workbench_moderation is not added as a dependency to workbench_access

Remaining tasks

Tests
Code

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

acbramley created an issue. See original summary.

acbramley’s picture

Issue summary: View changes
acbramley’s picture

Status: Active » Needs review
FileSize
9.2 KB

Here's a patch which adds a new submodule to integrate workbench_access with workbench_moderation. It alters the state transition validation service to also check for edit access via the WorkbenchAccessManager.

I am not opposed to having this in an entirely separate module but thought I'd add it here first and see what maintainers think.

acbramley’s picture

Issue summary: View changes
acbramley’s picture

Issue summary: View changes
agentrickard’s picture

I think my real question here is: Under what conditions can someone perform a transition without being able to edit the node?

acbramley’s picture

@agentrickard that's a good question! Workbench Moderation has a form which you can expose via the display settings on a content type. It means that users with access to the transitions can moderate content without going to the edit screen. The service this module overrides is what checks for the access to those transitions.

fenstrat’s picture

I'm not sure this makes sense as a separate module? It really seems expected behaviour if you've enabled workbench_moderation with workbench_access.

+++ b/workbench_access_state_transition/README.md
@@ -0,0 +1,3 @@
+Integrates workbench_access with workbench_moderation to limit a user's access to transition an entity between moderation states based on the constraints set by workbench_access. ¶

Nit: Whitespace at end of line.

acbramley’s picture

@fenstrat I thought the same too but then I noticed that workbench_access doesn't actually depend on workbench_moderation at all. Not wanting to introduce that dependency, I've gone with a separate module.

acbramley’s picture

Removes whitespace.

Status: Needs review » Needs work

The last submitted patch, 10: 2852717-workbench-access-state-transition-10.patch, failed testing.

acbramley’s picture

Status: Needs work » Needs review
FileSize
9.24 KB
513 bytes

Add test dependencies.

Status: Needs review » Needs work

The last submitted patch, 12: 2852717-workbench-access-state-transition-12.patch, failed testing.

acbramley’s picture

Status: Needs work » Needs review
FileSize
9.47 KB

Messed that up, moving test_dependencies to parent module.

Status: Needs review » Needs work

The last submitted patch, 14: 2852717-workbench-access-state-transition-14.patch, failed testing.

acbramley’s picture

I believe the failures are due to adding test_dependencies in a patch (i.e DrupalCI won't install those dependencies unless they're already part of the module)

larowlan’s picture

Looks great, a couple of comments/questions below:

  1. +++ b/workbench_access_state_transition/tests/src/Functional/WorkbenchAccessStateTransitionValidationTest.php
    @@ -0,0 +1,157 @@
    +    $field_storage = FieldStorageConfig::create([
    

    nit: Could we use the existing wba test trait to simplify this?

agentrickard’s picture

To be fair, that test trait was just committed. Yes, we should use it.

I have no problem adding this to the main module, but I do wonder if there is a bigger issue here.

State transitions need to respect editorial access controls. So this issue isn't limited to Workbench Access. Say you are using Organic Groups or similar, wouldn't the same problem occur?

If so, then we should fix this upstream at the Workbench Moderation / Content Moderation level.

I think the real problem is here in the StateTransitionValidation service:

  /**
   * Determines if a user is allowed to transition from one state to another.
   *
   * This method will also return FALSE if there is no transition between the
   * specified states at all.
   *
   * @param string $from
   *   The origin state machine name.
   * @param string $to
   *   The desetination state machine name.
   * @param \Drupal\Core\Session\AccountInterface $user
   *   The user to validate.
   *
   * @return bool
   *   TRUE if the given user may transition between those two states.
   */
  public function userMayTransition($from, $to, AccountInterface $user) {
    if ($transition = $this->getTransitionFromStates($from, $to)) {
      return $user->hasPermission('use ' . $transition->id() . ' transition');
    }
    return FALSE;
  }

That is not a sufficient permission check. It also needs to check entity access.

Unless that's a design decision by Workbench Moderation, in which case, this would be By Design.

agentrickard’s picture

Project: Workbench Access » Workbench Moderation
Category: Feature request » Bug report
mlhess’s picture

acbramley’s picture

So now that we have come to the conclusion to move this into Workbench Moderation instead, I'm happy to write a patch but I'd like to know how we envisage this working.

It could be:
* A global config setting on the module
* A sub module which alters the workbench_moderation.state_transition_validation service to add various checks on entity edit access
* A config setting per content type on the moderation tab?

larowlan’s picture

larowlan’s picture

How about this

  1. We add a new permission 'moderate entities that cannot edit'
  2. We change \Drupal\workbench_moderation\EntityOperations::entityView to include #access on the render array like so:
    '#access' => $this->currentUser()->hasPermission('moderate entities that cannot edit') || $entity->access('update'),
    

    - would require injecting the current_user service into EntityOperations, but no biggie

Because I think that's the only way you can change the moderation state without edit access.

larowlan’s picture

would require a change notice/release notes as its a change in behaviour

acbramley’s picture

@larowlan sounds good to me!

acbramley’s picture

Title: Limit access to moderation transitions » Limit access to moderation form based on edit access
Component: Documentation » Code
Assigned: Unassigned » acbramley
acbramley’s picture

New patch against WBM as per recommendations in #23, let's see what tests fail.

larowlan’s picture

Looking good

  1. +++ b/src/EntityOperations.php
    @@ -46,6 +47,13 @@ class EntityOperations {
    +   * @var \Drupal\Core\Session\AccountProxyInterface
    
    @@ -58,13 +66,16 @@ class EntityOperations {
    +   * @param \Drupal\Core\Session\AccountProxyInterface $current_user
    

    Should this be AccountInterface? not the proxy?

  2. +++ b/workbench_moderation.permissions.yml
    @@ -20,5 +20,9 @@ view latest version:
    +  title: Moderate entities that cannot edit
    

    I think we need to work on the title, not sure what we should make it though

larowlan’s picture

Moderate entities that cannot edit

how about 'Moderate without edit access' then the description to clarify what it means?

larowlan’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs change record

Ok, we just need a change record here.

larowlan’s picture

acbramley’s picture

acbramley’s picture

Status: Reviewed & tested by the community » Needs work

Found a bug with this on the node's canonical page where the block has been cached for the second user, need to add cache context on the user. Related to #2733127: Can't moderate new content - moderation controls don't appear on new nodes

acbramley’s picture

acbramley’s picture

My bad, this was a nuance with our custom set up to display the form on a node's canonical page. Back to RTBC with the same patch from #30

  • larowlan committed 07829ec on 8.x-1.x authored by acbramley
    Issue #2852717 by acbramley: Limit access to moderation form based on...
larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Thanks @acbramley!

Status: Fixed » Closed (fixed)

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