Problem/Motivation

When modules that implement node grants are enabled, access checking is happening in this query in RevisionOverviewForm::buildForm():

    $query = $this->entityQuery->get('node')
      ->condition($node->getEntityType()->getKey('id'), $node->id())
      ->pager($pagerLimit)
      ->allRevisions()
      ->sort($node->getEntityType()->getKey('revision'), 'DESC')
      ->execute();

This causes issues when folks are granted access to the node by other means (eg, hook_entity_access, etc). Once content has a published version, then things behave normally again.

Proposed resolution

  • Remove query access checking since access to the node itself has already been verified once this form is being built.
  • Write a test to demonstrate the issue.

Remaining tasks

User interface changes

API changes

Data model changes

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhedstrom created an issue. See original summary.

jhedstrom’s picture

Status: Active » Needs review
FileSize
1.85 KB
2.47 KB

Let's see how this does with the rest of the tests. A similar fix for content moderation was done in #2932154: ModerationInformation::getLatestRevisionId returns access-specific results, for reference.

The last submitted patch, 2: 2935222-02-TEST-ONLY.patch, failed testing. View results

kevin.dutra’s picture

Status: Needs review » Reviewed & tested by the community

This makes sense. The patch looks good to me and appears to work as expected, so moving to RTBC.

namrata.honnavar’s picture

The last submitted patch, 5: 2935222-05-TEST-ONLY.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

namrata.honnavar’s picture

Status: Reviewed & tested by the community » Needs review

Made some more changes - The compare selected revisions does not time out now.

kevin.dutra’s picture

Status: Needs review » Reviewed & tested by the community

It makes sense to extend the access control change to that other spot too. Just FYI @namrata.honnavar, it's helpful when submitting a new patch to include an interdiff so that a reviewer can more easily see what has changed in the new patch. At any rate, things still appear to be in order, so moving back to RTBC.

kevin.dutra’s picture

miro_dietiker’s picture

Status: Reviewed & tested by the community » Fixed

Thank you, committed this.

Status: Fixed » Closed (fixed)

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