Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
See #3200985: [meta] Fix undesirable access checking on entity query usages.
Cron should never care about the current user:
- core/core.api.php hook cron
- core/modules/file/file.module file_cron
- core/modules/node/node.module node_cron
- core/modules/tracker/tracker.module tracker_cron
Issue fork drupal-3201470
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:
Comments
Comment #2
jonathanshawComment #4
jonathanshawComment #5
jonathanshawComment #6
jonathanshaw@catch wanted tests for things that can be triggered by node access modules. I think I've found a simple test change that might work for tracker, let's see what the bot says.
Comment #7
jonathanshawnode_cron isn't checking access properly, but the only consequence of this is that it is not getting the right oldest and most recent node timestamps, which are used for search rankings:
So this won't cause nodes not to be indexed and searchable, it will just make the rankings decay function work a bit differently. And it doesn't handle access considerations particularly well anyway. I'm not sure adding test coverage for the node_cron fix makes sense.
Comment #8
catch#5 looks like a good approach.
Agreed on #7.
Comment #9
jonathanshawThe test in #5 looks like it failed properly. Adding back the tracker_cron() node fix now, the test should now pass.
Comment #10
jonathanshawThe test approach in #5 did not work, it was failing for reasons I didn't understand. I'm trying a different more explicit approach here.
Comment #11
jonathanshawBot did not run. Will try to tickle it.
Comment #12
catchNew tracker test looks good.
Comment #13
jonathanshawIt seems like Drupal CI is misbehaving in what it's reporting: 51116f35 is reported above as failing and f168a0ef as passing.
But there's no difference in the code between the two.
Comment #14
jonathanshawI suggest we don't provide test coverage for the file case, because (a) you'd only encounter it with esoteric custom code and (b) it's only going to affect cleanup of temporary files rather than cause a data integrity problem or break in functionality on all but the largest sites.
Comment #15
longwaveOK, point taken. The tracker test is good, the core.api.php docs fix is good and the other two cases seem too obscure to be worth testing as the bugs they cause are technically not that important.
Comment #16
catchYeah file access you'd have to be implementing entity query alter with the access tag to affect listings, whereas most file access is using hook_file_download()/hook_file_access() or similar which has no effect on the query.
Per discussion in #3200985: [meta] Fix undesirable access checking on entity query usages we have a really bad pattern across core (and it's also a huge problem in contrib and custom code), so adding test coverage for the serious cases like node access, but not the theoretical cases like files is a good compromise for me to avoid needing to add 100+ test cases before we can add the deprecation itself. Once we have that deprecation it'll be much harder to add bad queries or regress existing ones.
Comment #17
jonathanshawI added a link to @catch's test coverage explanantion in #16 to the IS in #3200985: [meta] Fix undesirable access checking on entity query usages together with an expanded version of this reasoning.
Comment #20
catchCommitted/pushed to 9.2.x, cherry-picked to 9.1.x and 8.9.x, thanks!