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
#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
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

jhedstrom created an issue. See original summary.

timmillwood’s picture

timmillwood 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 CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Active » Needs review
FileSize
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 CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
FileSize
interdiff-2865498-03-04.txt3.25 KB
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 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 CreditAttribution: timmillwood at Appnovation for Pfizer, Inc. commented
Status: Needs work » Reviewed & tested by the community
FileSize
2865498-7.patch9.24 KB

Needed a re-roll.

alexpott’s picture

alexpott 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 CreditAttribution: jhedstrom at Phase2 for Workday, Inc. commented
Status: Needs work » Needs review
FileSize
interdiff-2865498-07-09.txt3.9 KB
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 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 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 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.