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.

comment_user_predelete() contains an entity query that checks access, but should not.

Issue fork drupal-3208265

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

Comments

jonathanshaw created an issue. See original summary.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Title: EntityQuery accessCheck: commer_user_predelete() should not check access » EntityQuery accessCheck: comment_user_predelete() should not check access

jonathanshaw opened merge request !536

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Status: Active » Needs review

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, and it's more important to progress the blocked issue.

jonathanshaw’s picture

jonathanshaw
Primary language English
Location Stroud, UK
commented
Issue summary: View changes
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

This one seems pretty obvious that we shouldn't be checking access when explicitly deleting all comments belonging to a user.

  • catch committed d4151a0 on 9.2.x 
    Issue #3208265 by jonathanshaw, longwave: EntityQuery accessCheck:...

  • catch committed cc1962f on 9.1.x 
    Issue #3208265 by jonathanshaw, longwave: EntityQuery accessCheck:...
catch’s picture

catch
he/him
Primary language English
commented
Version: 9.2.x-dev » 9.1.x-dev
Status: Reviewed & tested by the community » Fixed

I had to think about test coverage for a bit - access to comments can be restricted via node access, but this only happens in the comment selection handler (which is hard-coded) and via entity access checks to the commented entity, not via altering comment entity queries. So if we added special handling to comment queries it might happen, but in practice we don't, and that's not worth holding up the many other dozens of critical bugs in contrib that will be fixed by the parent issue.

Committed/pushed to 9.2.x and cherry-picked to 9.1.x, thanks!

Status: Fixed » Closed (fixed)

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