Security update introduces breaking changes to content moderation

We debated the API break and opted for a BC break to ensure there wasn't a silent security issue

I'd be in favour of option 2.

+++ b/core/modules/content_moderation/src/Plugin/Validation/Constraint/ModerationStateConstraintValidator.php
@@ -115,7 +115,9 @@ public function validate($value, Constraint $constraint) {
+      $valid_transitions = $this->stateTransitionValidation->getValidTransitions($entity, $this->currentUser);
+      if (!in_array($transition->id(), array_keys($valid_transitions))) {

+++ b/core/modules/content_moderation/src/StateTransitionValidation.php
@@ -49,12 +47,4 @@ public function getValidTransitions(ContentEntityInterface $entity, AccountInter
-    return $user->hasPermission('use ' . $workflow->id() . ' transition ' . $transition->id());

we lost the permissions check

One of the suggestions in the private issue was to remove the additional method. Here was an analysis of that which helps explain some of the motivations and problems with the approach you suggested:

I spent a long time playing with this and tinkering with the approach to remove the extra method. There is a fundamental issue with using getValidTransitions(ContentEntityInterface $entity...: for brand new entities that are unsaved, there is no original entity to load that represents the initial state of content for the purposes of validation.

That is, no matter what happens in all scenarios, we always assume something new starts from draft and then validate around that. For a brand new entity, we'd probably do something like: this entity is new, so the original entity is actually the same as the new one basically. But without cloning that entity and manually resetting the moderation state back to draft, we don't have a viable way of using the existing method to perform validations.

None of the new or existing tests caught this and since the validator uses $original_state which has it's own $original_entity->isNew() checks built in, even the violation messages were making complete sense while being incorrect.

I've uploaded this attempt. We could add further complexity by resetting $original_entity back to having $original_state when the entity is new and unsaved, but I don't like this approach much.

My preference would be use #27 and consider deprecating getValidTransitions in a follow-up in the public issue queue. It currently has 6 quite non-trivial usages, which I think would be much easier to divide and concur with a test-bot handy. I also think it's worth some public discussion to decide if we need to deprecate it at all: both the methods seem to have some utility in different contexts, but I'm not sold either way yet.

I haven't looked into those contrib modules, but perhaps entity could be an additional optional argument for advanced use cases?

Edit: Reviewing your patch in more detail, not sure if it gets around these issues, looks like you've come up with an approach for this.

+++ b/core/modules/content_moderation/src/StateTransitionValidation.php
@@ -42,19 +40,11 @@ public function __construct(ModerationInformationInterface $moderation_info) {
-    $current_state = $entity->moderation_state->value ? $workflow->getTypePlugin()->getState($entity->moderation_state->value) : $workflow->getTypePlugin()->getInitialState($entity);
+    $current_state = $this->moderationInfo->getOriginalOrInitialState($entity);

This has the problem that it's a change implementations would have to make, that would fail silently otherwise. So while the current BC break is annoying, it has forced the issue to the surface.

I think now that the existing fix is in stable releases, we unfortunately might have to do the following:

but perhaps entity could be an additional optional argument for advanced use cases

We could surmise that the method may have been called outside the context of having an entity available (despite the chances of that being pretty slim) and I don't think removing a method from an interface or changing an existing methods signature is an option anymore.

Thanks for sticking with this issue, really appreciate you helping to minimise disruption of this change.

+++ b/core/modules/content_moderation/src/StateTransitionValidationInterface.php
@@ -36,10 +36,12 @@ public function getValidTransitions(ContentEntityInterface $entity, AccountInter
+   * @param \Drupal\Core\Entity\ContentEntityInterface $entity
+   *   The entity to be transitioned.

I think we need to add the (Optional) tag to the comment here and possibly describe why it's optional, that is: the default implementation in core does not rely on this, some implementations do rely on this and that the entity param should be provided whenever possible by users of the API to ensure compatibility with contributed projects.

I wonder if we should actually deprecate NOT passing the entity in and do a trigger_error when it is not provided, that way we respect the existing BC, inform users $entity should be passed and then also be able to make it a required part of the interface for D9. Thoughts? In addition to that, lets add a change record.

Let me know if you have capacity to continue working on this, otherwise I'll see if I can put some time towards this.

Okay great, adding the patch.

+++ b/core/modules/content_moderation/src/StateTransitionValidationInterface.php
@@ -36,10 +36,12 @@ public function getValidTransitions(ContentEntityInterface $entity, AccountInter
+   * @param \Drupal\Core\Entity\ContentEntityInterface $entity
+   *   (optional) The entity to be transitioned.

Should this docblock note that the optional part of this is deprecated as well? Alternatively, since does it need to mention it's optional at all?

Good point, I don't mind either of those options.

Fixing comment.

Whoops, correct patch this time.

Patch in #18 looks RTBC to me. Anything left to be done here? It would be great to get this in so #3007906: Core security release breaks workflow participants can get committed.

Nothing else to do here as far as I'm aware.

Then let's get this in :)

The issue summary is out-of-date with the chosen solution. Given the complexity and how it was arrived at I think it is important that the issue summary is correct. Especially as the chosen solution doesn't appear to be amongst the possible solutions.

We also should have a change record here.

I think the IS slightly mischaracterises the other options. This isn't exactly true:

Remove the new method from the interface ... no API change and no compatibility issues.

Other than that, looks good to me.

Slightly tweaking the IS and RTBCing the CR here: https://www.drupal.org/node/3026087. I think I'm fine to RTBC given no changes were requested with the actual patch.

Committed 73d9d9b and pushed to 8.7.x. Thanks!

Only committing to 8.7.x just to get a second opinion on backport because there is a change record and interface addition. I think this is eligible because it is not a break and critical.

Leaving at rtbc against 8.6.x

diff --git a/core/modules/content_moderation/tests/src/Unit/StateTransitionValidationTest.php b/core/modules/content_moderation/tests/src/Unit/StateTransitionValidationTest.php
index e9d97e49cc..541ff8f64d 100644
--- a/core/modules/content_moderation/tests/src/Unit/StateTransitionValidationTest.php
+++ b/core/modules/content_moderation/tests/src/Unit/StateTransitionValidationTest.php
@@ -52,7 +52,6 @@ protected function setUp() {
       ->addTransition('publish', 'publish', ['needs_review', 'published'], 'published');
   }
 
-
   /**
    * Verifies user-aware transition validation.
    *

Fixed coding standard on commit.

@catch +1'd the backport in slack. Committed 82a677a32e and pushed to 8.6.x. Thanks!