The current behavior for Content Moderation is to return AccessResult::forbidden() when update access is checked for a moderated entity that does not have a valid transition to a different state.

This issue is not about whether this is a correct behavior ( that has been brought up here ), but the fact that it does so without providing a reason.

Due to the current lack of solid documentation (we are implementing workflows and lost a good two days figuring this requirement out), there should at least be a reason for forbidding access -- it's why this functionality exists and would make a HUGE difference in tracking down what is going on where.

I have attached a very simple patch that adds the reason Content Moderation: No allowed transitions exist., so that there is a hint to developers using $entity->access('update', $account,true);. It is a (very) simple change that would have saved us at least a day of debugging our custom hook_node_access_records et al.

CommentFileSizeAuthor
#11 content_moderation_addforbiddenreasons-noprefix-up.patch4.34 KBhexblot
#6 content_moderation_addforbiddenreasons-noprefix.patch4.34 KBhexblot
#4 content_moderation_addforbiddenreasons.patch3.71 KBhexblot
content_moderation_notransitionavailable_message.patch959 byteshexblot

Comments

hexblot created an issue. See original summary.

hexblot’s picture

hexblot commented
Issue summary: View changes
sam152’s picture

sam152 commented
Status: Active » Needs review

I completely agree with this, content_moderation does alter access to entities in ways which aren't always totally obvious. Adding as much context as possibly to indicate what is going on is a great DX/REST improvement.

Doing a quick search, it looks like we forbid access to various things in the following places without providing a reason why:

  1. content_moderation_entity_access (as per the original issue summary).
  2. content_moderation_entity_field_access, restricting the 'published' field, in favour of requiring a moderation_state instead.
  3. LatestRevisionCheck::access, restricting access to to the latest-version route if entity has no pending revision.
  4. ModerationOptOutPublish/ModerationOptOutUnpublish: restricting access to the publish/unpublish actions for the same reasons as #2.

How would you feel about rejigging this issue to include helpful messages for all these scenarios too?

hexblot’s picture

hexblot commented
StatusFileSize
new content_moderation_addforbiddenreasons.patch3.71 KB

Rerolling patch to include reason messages for all AccessResult::forbidden() cases found.

Thanks @Sam152 for the suggestion!

sam152’s picture

sam152 commented
Status: Needs review » Needs work

We're missing one in LatestRevisionCheck::access still. I also think we can safely remove the "Content Moderation:" prefix, so long as we make sure the messages are descriptive enough.

  1. +++ b/core/modules/content_moderation/content_moderation.module
@@ -196,7 +196,7 @@ function content_moderation_entity_access(EntityInterface $entity, $operation, A
-    $access_result = $valid_transition_targets ? AccessResult::neutral() : AccessResult::forbidden();
+    $access_result = $valid_transition_targets ? AccessResult::neutral() : AccessResult::forbidden('Content Moderation: No valid transitions exist.');

    I think for this message it's worth pointing out that it's the current user that doesn't have access to any valid transitions. Valid ones might exist generally speaking. 

  2. +++ b/core/modules/content_moderation/content_moderation.module
@@ -224,7 +224,7 @@ function content_moderation_entity_field_access($operation, FieldDefinitionInter
-      return AccessResult::forbidden();
+      return AccessResult::forbidden('Content Moderation: Cannot edit published field of moderated entities.');

    Maybe in this one we can mention what the alternative is too? Something like "The published field cannot be edited directly for moderated entities. Instead the moderation_state field should be used." 

  3. +++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutPublish.php
@@ -75,7 +75,7 @@ public function access($entity, AccountInterface $account = NULL, $return_as_obj
-      $result = AccessResult::forbidden();
+      $result = AccessResult::forbidden('Content Moderation: Cannot directly publish moderated entities.');

+++ b/core/modules/content_moderation/src/Plugin/Action/ModerationOptOutUnpublish.php
@@ -75,7 +75,7 @@ public function access($entity, AccountInterface $account = NULL, $return_as_obj
-      $result = AccessResult::forbidden();
+      $result = AccessResult::forbidden('Content Moderation: Cannot directly unpublish moderated entities.');

    We don't have an action for moderation states, so sadly there is no alternative to these.

hexblot’s picture

hexblot commented
StatusFileSize
new content_moderation_addforbiddenreasons-noprefix.patch4.34 KB

Changes in attached patch:

  • Added message to LatestRevisionCheck::access
  • Remove module name prefix in messages, to conform with EntityAccessControlHandler::fieldAccess (only core module I could find that includes messages)

Regarding your points:

  1. I think for this message it's worth pointing out that it's the current user that doesn't have access to any valid transitions. Valid ones might exist generally speaking.

    To my understanding, the check is not against the current user -- rather, the passed $account, hence the more generic message. Amended to No valid transitions exist for given account. nonetheless.

  2. Maybe in this one we can mention what the alternative is too? Something like "The published field cannot be edited directly for moderated entities. Instead the moderation_state field should be used."

    Trying to follow the EntityAccessControlHandler::fieldAccess format for messages, since it appears to be the only one in core to include messages - and it does not propose resolution, just points out the reason for the current result. I do also feel that trying to recommend an approach might be presumptuous since other modules might alter the possibilities for resolution, but not the root cause.

sam152’s picture

sam152 commented
Status: Needs work » Needs review

The message seem generally a bit light on detail, but the points in #6 make sense and they are certainly better than what we already have. Setting NR for test bot.

sam152’s picture

sam152 commented
Status: Needs review » Reviewed & tested by the community

Looks like the tests passed. I'll let someone else RTBC for a second opinion on the messages themselves.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented

Am I right in thinking these messages are not user visible, therefore using the word "entities" is ok?

If so, only one small nitpick:

+++ b/core/modules/content_moderation/content_moderation.module
@@ -224,7 +224,7 @@ function content_moderation_entity_field_access($operation, FieldDefinitionInter
+      return AccessResult::forbidden('Cannot edit published field of moderated entities.');

I think this should be "Cannot edit the published field..."

sam152’s picture

sam152 commented
Status: Reviewed & tested by the community » Needs review

Whoops, didn't mean to update the status in #8.

hexblot’s picture

hexblot commented
StatusFileSize
new content_moderation_addforbiddenreasons-noprefix-up.patch4.34 KB

@timmillwood These messages only appear when explicitly calling $entity->access($op, $account,true); (last boolean is optional and FALSE by default, so only visible during debugging or similar).

Rerolling the patch to include "small nitpick".

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
commented
Status: Needs review » Reviewed & tested by the community

Thanks @hexblot, looks fine to me now.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 8.5.x-dev » 8.6.x-dev
Category: Feature request » Task
Status: Reviewed & tested by the community » Fixed

Committed 4a85296 and pushed to 8.6.x. Thanks!

  • alexpott committed 4a85296 on 8.6.x 
    Issue #2952544 by hexblot, Sam152, timmillwood: Provide a reason for...

Status: Fixed » Closed (fixed)

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