See parent issue #3200985: [meta] Fix undesirable access checking on entity query usages for context and test coverage policy. This issue is major because it blocks #2785449: It's too easy to write entity queries with access checks that must not have them.

MediaRevisionAccessCheck has a helper method countDefaultLanguageRevisions that is used to check whether the default revision is the only revision. It uses an entity query that checks access to the other revisions, but it does not intend to.

Issue fork drupal-3204389

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jonathanshaw created an issue. See original summary.

jonathanshaw opened merge request !442

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented

This is a funny one, as in an access checker we'd expect to want queries to be access sensitive. But the checkAccess() method suggests that countDefaultLanguageRevisions is used only to do a fast bypass if there is only a single revision, it's a performance optimisation rather a fundamental part of the access logic. 

      // There should be at least two revisions. If the revision ID of the
      // given media item and the revision ID of the default revision differ,
      // then we already have two different revisions so there is no need for a
      // separate database check.
      if ($media->isDefaultRevision() && ($this->countDefaultLanguageRevisions($media) == 1)) {
        $this->access[$cid] = FALSE;

Unfortunately the comment seems to be mis-stated: actually if the ids are different, that's when additional checking is needed.

I suggest that under the parent issue's test coverage policy it is acceptable for this not to have test coverage, because it would require unusual customisations to encounter, it's only a minor performance optimisation, and it's more important to progress the blocked issue.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
CreditAttribution: jonathanshaw commented
Status: Active » Needs review
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community

Yes, this query should by-pass!

Btw found similar method in \Drupal\node\NodeStorageInterface::countDefaultLanguageRevisions() which is using more optimized version - direct query

  public function countDefaultLanguageRevisions(NodeInterface $node) {
    return $this->database->query('SELECT COUNT(*) FROM {' . $this->getRevisionDataTable() . '} WHERE [nid] = :nid AND [default_langcode] = 1', [':nid' => $node->id()])->fetchField();

  • catch committed c97c52b on 9.2.x 
    Issue #3204389 by jonathanshaw, andypost: EntityQuery accessCheck:...

  • catch committed f256f58 on 9.1.x 
    Issue #3204389 by jonathanshaw, andypost: EntityQuery accessCheck:...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs followup

Let's go ahead here - however we should open follow-ups both to fix that code comment and also to bring the queries into line with each other.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue tags: -Needs followup

Filed follow-up #3204641: Improve MediaRevisionAccessCheck::countDefaultLanguageRevisions usage

I think entity query just a bit slower then direct one, btw NodeStorage is supposed to work with database directly.

Meantime if query could be improved by disabling alters it could be faster

Status: Fixed » Closed (fixed)

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