Content moderation can wrongly set old revisions as default when they're resaved

As per #4, this is apparently happening with people using core's Content Moderation module (which makes sense, since it was mostly a port of the contrib Workbench Moderation module). Moving this to the core issue queue. Also marking 🐛 Deleting user account reverted all content authored by user to old revisions Closed: duplicate as a duplicate of this, as I assume that user was also using Content Moderation.

See also 🐛 Revision user incorrectly appears as anonymous user when node author is cancelled Needs work . The user in comment #29 experienced this as well, though it's not clear they were using Content Moderation or Workbench Moderation at the time.

In our case an admin deleted a user and suddenly web reverted back to few months ago, we had to use a backup. This is a major bug because the damage that can be done to the website is enormous.

I can't provide more data because we are still investigating to reproduce the bug.

In our case, we have identified the cause of the activation of an outdated revision as the default revision and the duplication of revisions.

It is caused by the Content Moderation module.

This module intervenes in the process through a "presave" hook, and under certain circumstances, it incorrectly designates that revision as new and sets it as the default revision.

namespace Drupal\content_moderation\Entity\Handler;

[.....]

 /**
   * {@inheritdoc}
   */
  public function onPresave(ContentEntityInterface $entity, $default_revision, $published_state) {
    // When entities are syncing, content moderation should not force a new
    // revision to be created and should not update the default status of a
    // revision. This is useful if changes are being made to entities or
    // revisions which are not part of editorial updates triggered by normal
    // content changes.
    if (!$entity->isSyncing()) {
     $entity->setNewRevision(TRUE);
     $entity->isDefaultRevision($default_revision);
    }

    // Update publishing status if it can be updated and if it needs updating.
    if (($entity instanceof EntityPublishedInterface) && $entity->isPublished() !== $published_state) {
      $published_state ? $entity->setPublished() : $entity->setUnpublished();
    }
  }

Conditions for this case to occur:

• - The Content Moderation module is enabled.
• - The content type of the revision being modified is a moderated content.
• - The Moderation State of the revision is not empty.

At this point, a new revision will be created.

For this revision to be set as the default, the current default revision has to belong to other user and meet one of these conditions:

• - That the revision's state is considered a default revision state.
• - Or that the default revision at this moment is not in a published state.

It is clear that this problem is introduced by a contributed module and not the core, but we believe that the consequences are too severe, and it would be advisable to devise a strategy to ensure that at the very least, old revisions are not activated by any module.

As I mentioned in my previous comment ( https://www.drupal.org/project/drupal/issues/3089747#comment-15212151 🐛 Content moderation can wrongly set old revisions as default when they're resaved Needs review ), there is a patch that addresses the issue of obsolete revisions being triggered as default revisions in our project, but we believe it's not the best solution.

The revision that is being updated should not pass through the Content Moderation presave hook, as this modification is unrelated to a publishing workflow.

Here is the process of replacement:

• First, obtain the identifiers of all revisions generated by the user.
• Load the node of each revision.
• Replace the uid and review_uid with 0 in all active translations for the content (I've found the same revision ID assigned to a user in both French and English).
• Proceed with saving the node.

This process cause that any presave or postsave hook can alter the entity or run inappropriate processes in this context.

Bumping to critical and changing the title to make it clearer what the bug is.

It's a very drastic solution that needs careful consideration, but since this type of modification doesn't belong to an editorial workflow, would it be unreasonable for the solution to involve preventing the Content Moderation module hooks from being triggered?

We conducted a test, and by disabling these hooks, the process went smoothly

I think that I agree with the approach in #8 (setting the syncing flag prior to re-saving).

I think an optional parameter added to node_mass_update would be a fairly elegant solution:

function node_mass_update(array $nodes, array $updates, $langcode = NULL, $load = FALSE, $revisions = FALSE, $syncing = FALSE) {

  // ...
  // ...

    foreach ($nodes as $node) {
      if ($load && $revisions) {
        $node = $storage->loadRevision($node);
      }
+    if ($syncing) {
+      $node->setSyncing(TRUE);
+    }
      _node_mass_update_helper($node, $updates, $langcode);
    }
    \Drupal::messenger()->addStatus(t('The update has been performed.'));
}

then invoked via node_user_cancel:

      node_mass_update($vids, [
        'uid' => 0,
        'revision_uid' => 0,
      ], NULL, TRUE, TRUE, TRUE);

Yeah I also think ::setSyncing() is the right approach here, user module is going back in time to alter metadata for old revisions, it's not really updating the content or anything.

Perfect. So, I'm going to try to improve the patch with Luke Leber's proposal.

I will also attempt to address the issue of replacing all authors of revisions with an anonymous user since the current patch doesn't resolve that.

This case doesn't occur exactly in the code snippet mentioned in my previous comment 8, so I'll need to find the exact location where it occurs.

Thank you!

I have just committed the proposed solution in the previous comments and generated the Merge Request.

Ready for review.

Thanks.

Appears to be an open thread.

Also can we get a test case showing the issue?

Eduardo Morales Alberti → made their first commit to this issue’s fork.

Added PHPUnit tests, waiting to see their success on the build.
Added tests only patch to check that is failing without the fixes on the MR.

Upload the right patch.

Seems like on the last version of Drupal (11.x) this error is not happening.

Currently, we are not able to reproduce this problem, so the PHPUnit test is not accurate.
Someone could provide the step by step to reproduce this problem?

Hate putting a critical in PNMI but seems steps to reproduce are needed to continue.

So if anyone experiencing this could add those.

Also tagged for IS so steps could be added in addition to the solution was used.

Leaving tests tag as #22 shows current test passes as is.