Problem/Motivation

The permissions to view the latest revision tab currently use view latest revision, and view any unpublished content permissions.

It makes sense to have that check for view own unpublished content too.

However, this is somewhat problematic because the node module defines view own unpublished content, while content moderation defines view any unpublished content.

Proposed resolution

Add the check for view own unpublished content.

Remaining tasks

Figure out how to reconcile the node module defining that permission.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#16 2865498-latest-revision-tab-respect-permissions-16.patch704 bytestuwebo
#9 2865498-09.patch9.71 KBjhedstrom
#9 interdiff-2865498-07-09.txt3.9 KBjhedstrom
#7 2865498-7.patch9.24 KBtimmillwood
#4 2865498-04.patch9.32 KBjhedstrom
#4 interdiff-2865498-03-04.txt3.25 KBjhedstrom
#3 2865498-03.patch8.61 KBjhedstrom

Comments

jhedstrom created an issue. See original summary.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

I guess this is related to #2838452: Permissions: View any unpublished content not working?

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Active » Needs review
StatusFileSize
new 2865498-03.patch8.61 KB

I'm not sure how closely it's related to #2838452: Permissions: View any unpublished content not working aside from dealing with permissions...

Here is an initial patch.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
StatusFileSize
new interdiff-2865498-03-04.txt3.25 KB
new 2865498-04.patch9.32 KB

Last patch didn't check for the other permission 'view latest version' in conjunction with 'view own unpublished content'. I also added some code comments to the test cases.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Looks good to me @jhedstrom, thanks!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 4: 2865498-04.patch, failed testing.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new 2865498-7.patch9.24 KB

Needed a re-roll.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/content_moderation/src/Access/LatestRevisionCheck.php
@@ -41,18 +43,25 @@ public function __construct(ModerationInformationInterface $moderation_informati
+      return AccessResult::allowedIfHasPermissions($account, ['view latest version', 'view any unpublished content'])
+        ->orIf(AccessResult::allowedIfHasPermissions($account, ['view latest version', 'view own unpublished content'])
+          ->andif(AccessResult::allowedIf($entity instanceof EntityOwnerInterface && $entity->getOwnerId() == $account->id()))
+        )->addCacheableDependency($entity);

Given this is access related I think we should be more verbose. And easy to read. Perhaps something like:

    if ($this->moderationInfo->hasForwardRevision($entity)) {
      $access_result = AccessResult::allowedIfHasPermissions($account, ['view latest version', 'view any unpublished content']);
      if (!$access_result->isAllowed()) {
        $owner_access = AccessResult::allowedIfHasPermissions($account, ['view latest version', 'view own unpublished content']);
        $owner_access->andif(AccessResult::allowedIf($entity instanceof EntityOwnerInterface && $entity->getOwnerId() == $account->id()));
        $access_result->orIf($owner_access);
      }
      return $access_result->addCacheableDependency($entity);
    }
jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2865498-07-09.txt3.9 KB
new 2865498-09.patch9.71 KB

This should address #9. Unfortunately, I couldn't use the exact logic there because of the way the andIf() and orIf() methods actually work. These don't impact the object they are being called on, but rather return a new access result, so the logic here relies on that return value. Still easier to read than the first attempt.

This approach also caught some issues with the test assumptions.

jhedstrom’s picture

jhedstrom
Primary language English
Location Portland, OR
CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented 
+++ b/core/modules/content_moderation/src/Access/LatestRevisionCheck.php
@@ -41,18 +43,31 @@ public function __construct(ModerationInformationInterface $moderation_informati
+    return AccessResult::forbidden()->addCacheableDependency($entity);

I'd honestly prefer to return neutral here. Expressly forbidding something disallows other modules to grant access, whereas neutral is almost the same since something else needs to explicitly grant access.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs review » Reviewed & tested by the community

Looks to resolve the concerns in #8.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed de122da and pushed to 8.4.x. Thanks!
Committed c560ef9 and pushed to 8.3.x. Thanks!

Credited myself because my review had an affect on the final patch.

Backported to 8.3.x because this is for an experimental module.

  • alexpott committed c560ef9 on 8.3.x 
    Issue #2865498 by jhedstrom, timmillwood, alexpott: Latest revision tab...

  • alexpott committed de122da on 8.4.x 
    Issue #2865498 by jhedstrom, timmillwood, alexpott: Latest revision tab...

Status: Fixed » Closed (fixed)

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

tuwebo’s picture

tuwebo CreditAttribution: tuwebo at Metadrop commented
Version: 8.4.x-dev » 8.5.x-dev
StatusFileSize
new 2865498-latest-revision-tab-respect-permissions-16.patch704 bytes

Sorry about opening this one again.
It is just that we forgot update "view latest version" permission description in the content_moderation.permissions.yml file.

Patch added, it just modifies the description.

timmillwood’s picture

timmillwood
Primary language English
Location 🏴󠁧󠁢󠁷󠁬󠁳󠁿 Wales, UK
CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented

@TuWebO please open a new issue for this.

tuwebo’s picture

tuwebo CreditAttribution: tuwebo at Metadrop commented

Hi @timmillwood thanks for the fast response.
I've added new issue #2924055: Wrong description for "view latest version" permission.