Problem/Motivation

The latest revision tab shows, but when you click on its link you get a 403 page.

This scenario is created when you give anonymous users "View the latest version" permission, but not the "View any unpublished content" permission.

Here are steps to reproduce:

  1. Install Workbench Moderation and enable a content type to be moderated, with all moderation states enabled, and default state of draft
  2. Create a node for that content type and publish it
  3. Create a forward revision for that node by creating a new draft
  4. Set the "View the latest version" permission to "Anonymous User"
  5. Access the node as an anonymous user
  6. Notice you see the "Latest version" tab, click on it
  7. Notice you are taken to a 403 Access denied page

Proposed resolution

TBD

CommentFileSizeAuthor
#16 change-latest-version-permission-to-view-own-unpublished-content-2687789.patch1.12 KBsimfin
#12 2687789-latest-tab.patch5.93 KBCrell
#8 2687789-latest-tab.patch5.45 KBCrell
#6 2687789_6.patch2.64 KBjosephdpurcell

Comments

josephdpurcell created an issue. See original summary.

Crell’s picture

Crell commented

Does this happen without multiversion, or is it a multiversion-specific bug?

josephdpurcell’s picture

josephdpurcell commented
Issue summary: View changes

This is a Workbench Moderation specific bug report. I've updated the description to clarify.

josephdpurcell’s picture

josephdpurcell commented
Issue summary: View changes

I have a clearer idea of what is happening. I've updated the description to reflect it.

I'm not sure this is actually a bug, but rather a need to understand that "View the latest version" permission doesn't make sense if the user cannot view the forward revision, e.g. the user doesn't have "View any unpublished content" permission.

I see two ways to improve this:

  1. Hide the "Latest version" tab if the user does not have permission to access the forward revision
  2. Update the description of the "View the latest version" description from "View the latest version of an entity" to "View the latest version of an entity (depends on "View any unpublished content")", or a similar phrasing
Crell’s picture

Crell commented

Why decide? Let's do both. :-) That's probably best for UX.

josephdpurcell’s picture

josephdpurcell commented
StatusFileSize
new 2687789_6.patch2.64 KB

I don't believe this patch works. It appears to me that $this->moderationInfo->getLatestRevision returns the latest revision even if the user doesn't have access to it.

Follow up would be to not just load the forward revision but verify the user has access, and secondly do item 2 in #4

Crell’s picture

Crell commented
Status: Active » Needs review

Correct. That method is explicitly not user-access-aware. That needs to get checked externally by this patch.

  1. +++ b/src/Access/LatestRevisionCheck.php
@@ -18,13 +19,23 @@ class LatestRevisionCheck implements AccessInterface {
-  public function __construct(ModerationInformationInterface $moderation_information) {
+  public function __construct(ModerationInformationInterface $moderation_information, AccountInterface $current_user) {
     $this->moderationInfo = $moderation_information;
+    $this->currentUser = $current_user;
   }

    The account can be passed into the access() method, as one of the many magic values it can request. See how PermissionAccessCheck does it. 

  2. +++ b/src/Access/LatestRevisionCheck.php
@@ -48,7 +59,14 @@ class LatestRevisionCheck implements AccessInterface {
+    $forward_revision = NULL;

    NULL is a bug. :-) 

  3. +++ b/src/Access/LatestRevisionCheck.php
@@ -48,7 +59,14 @@ class LatestRevisionCheck implements AccessInterface {
+    if ($this->moderationInfo->hasForwardRevision($entity)) {
+      $forward_revision = $this->moderationInfo->getLatestRevision($entity->getEntityTypeId(), $entity->id());
+    }

    getLatestRevision() isn't user-aware either.

Crell’s picture

Crell commented
StatusFileSize
new 2687789-latest-tab.patch5.45 KB

Here's a totally new take.

The test in this patch adds a test for the class, but turns out the fix itself doesn't even need to touch the class. However, writing a proper test for that requires BrowserTestBase, and placing blocks to get the tabs to show up, and after an hour-plus of trying to make that work I am giving up and just trusting that it works because I am sick to death of our testing framework at this point. :-(

If testbot likes the rest of this patch, I'll merge it shortly.

Status: Needs review » Needs work

The last submitted patch, 8: 2687789-latest-tab.patch, failed testing.

The last submitted patch, 8: 2687789-latest-tab.patch, failed testing.

The last submitted patch, 8: 2687789-latest-tab.patch, failed testing.

Crell’s picture

Crell commented
Status: Needs work » Needs review
StatusFileSize
new 2687789-latest-tab.patch5.93 KB

Hm, need to tweak the permissions in the Simpletests. I keep forgetting those are even there.

  • Crell committed f73e543 on 8.x-1.x 
    Issue #2687789 by Crell, josephdpurcell: Latest revision gives access...
Crell’s picture

Crell commented
Status: Needs review » Fixed

Merged.

Status: Fixed » Closed (fixed)

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

simfin’s picture

simfin commented
StatusFileSize
new change-latest-version-permission-to-view-own-unpublished-content-2687789.patch1.12 KB

I have an issue with users who I want to view the Latest version tab, but I don't want them to have the 'View all unpublished content' permission, but only the 'View own unpublished content'. I have attached a patch for this particular situation.

mrdrewkeller’s picture

mrdrewkeller commented

I've also run into the problem of not wanting to give users the "View any unpublished content" permission.

Is there a reason that was decided as a requirement rather than "View own unpublished content?"

So far in my testing the change in #16 works fine but would like to know why "View any unpublished content" is required.