Problem/Motivation

Entity queries must explicitly set whether the query should be access checked or not. See Drupal\Core\Entity\Query\QueryInterface::accessCheck().

Steps to reproduce

Use the 2.0.x (dev) version of the module with Drupal 10. Create some groups. Try to access the overview of groups.

Proposed resolution

Add $query->accessCheck() in \Drupal\group\Entity\Controller\GroupListBuilder::getEntityIds().

Remaining tasks

  • Create MR
  • Review MR
  • Merge MR

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Issue fork group-3278740

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:

Comments

ricovandevin created an issue. See original summary.

ricovandevin’s picture

Issue summary: View changes
Status: Active » Needs review
valthebald’s picture

Version: 2.0.x-dev » 3.0.x-dev
Status: Needs review » Needs work
Issue tags: +@deprecated

Same issue exists in 3.0.x and 8.x-1.x. I believe 3.0.x should be patched first (the patch is the same), and then the change should be backported to 2.0.x and 8.x-1.x?

I can confirm that this specific changes fixes deprecation warning in all branches, but there are other places where EntityQuery is executed without explicit access check, probably it'd be worth creating several issues (per branch) to fix all access checks.

Here are the calls of EntityQueryInterface::execute() without explicit access checks for the 3.0.x branch:

  1. Drupal\group\Entity\GroupType::getRoleIds()
  2. Drupal\group\Entity\Controller\GroupListBuilder::getEntityIds()
  3. Drupal\group\Entity\Controller\GroupRelationshipListBuilder::getEntityIds()
  4. Drupal\group\Entity\Controller\GroupRoleListBuilder::getEntityIds()
  5. Drupal\group\Entity\Form\GroupRevisionDeleteForm::countRevisions()
  6. Drupal\group\Entity\Storage\GroupRoleStorage::loadByUserAndGroup()
  7. Drupal\group\Plugin\Validation\Constraint\GroupRoleScopeValidator::validate()
  8. Drupal\Tests\group\Kernel\RevisionUiAccessTest::countDefaultLanguageRevisions()
  9. Drupal\Tests\group\Kernel\QueryAlter\EntityQueryAlterCacheabilityTest::testCacheableMetadataLeaks()
valthebald’s picture

Here are the calls of EntityQueryInterface::execute() without explicit access checks for the 2.0.x branch (same list as for 3.0.x)

  1. Drupal\group\Entity\GroupType::getRoleIds()
  2. Drupal\group\Entity\Controller\GroupListBuilder::getEntityIds()
  3. Drupal\group\Entity\Controller\GroupRelationshipListBuilder::getEntityIds()
  4. Drupal\group\Entity\Controller\GroupRoleListBuilder::getEntityIds()
  5. Drupal\group\Entity\Form\GroupRevisionDeleteForm::countRevisions()
  6. Drupal\group\Entity\Storage\GroupRoleStorage::loadByUserAndGroup()
  7. Drupal\group\Plugin\Validation\Constraint\GroupRoleScopeValidator::validate()
  8. Drupal\Tests\group\Kernel\RevisionUiAccessTest::countDefaultLanguageRevisions()
  9. Drupal\Tests\group\Kernel\QueryAlter\EntityQueryAlterCacheabilityTest::testCacheableMetadataLeaks()
valthebald’s picture

Here are the calls of EntityQueryInterface::execute() without explicit access checks for the 8.x-1.x branch:

  1. Drupal\group\GroupRoleSynchronizer::getGroupRoleIdsByGroupTypes()
  2. Drupal\group\GroupRoleSynchronizer::getGroupRoleIdsByUserRoles()
  3. Drupal\group\Access\GroupContentCreateAnyAccessCheck::access()
  4. Drupal\group\Access\SynchronizedGroupPermissionCalculator::calculateOutsiderPermissions()
  5. Drupal\group\Entity\GroupType::getRoleIds()
  6. Drupal\group\Entity\Controller\GroupListBuilder::getEntityIds()
  7. Drupal\group\Entity\Controller\GroupRelationshipListBuilder::getEntityIds()
  8. Drupal\group\Entity\Controller\GroupRoleListBuilder::getEntityIds()
  9. Drupal\group\Entity\Form\GroupRevisionDeleteForm::countRevisions()
  10. Drupal\group\Entity\Storage\GroupRoleStorage::createSynchronized()
  11. Drupal\group\Plugin\Validation\Constraint\GroupRoleScopeValidator::validate()
  12. Drupal\Tests\group\Kernel\EntityQueryAlterCacheabilityTest::testCacheableMetadataLeaks()
  13. Drupal\Tests\group\Kernel\EntityQueryAlterComplexTest::assertQueryAccessResult()
  14. Drupal\Tests\group\Kernel\EntityQueryAlterTest::assertQueryAccessResult()
  15. Drupal\Tests\group\Kernel\RevisionUiAccessTest::countDefaultLanguageRevisions()

voleger made their first commit to this issue’s fork.

heddn made their first commit to this issue’s fork.

heddn’s picture

Status: Needs work » Needs review

Let's see how the 3.x branch fares. I just now pushed all the changes needed on localdev to get tests passing green on Drupal 10.

heddn’s picture

Title: D10 compatibility | Entity queries must explicitly set whether the query should be access checked or not » D10 compatibility

Relabeling since this is all about Drupal 10 compatibility in general, not just access checks.

heddn’s picture

Linking some contrib module issues that depend on this landing.

heddn’s picture

This also is blocking #3262067: [META] Make Webform compatible with Drupal 10 from landing because it needs this module to support Drupal 10.

mxh’s picture

Assigned: ricovandevin » Unassigned
Priority: Normal » Critical

Since this is giving an error on group listing pages which makes it unusable, and D10 is now stable, this is critical.

kristiaanvandeneynde’s picture

Priority: Critical » Major
Status: Needs review » Needs work

Can't commit this as is, there's more than just the query access going on in the patch. I see public properties becoming protected, views yml files changing, php type-hinting being added, ...

As it stands this is impossible for me to commit because the diff is too large and diverse and I'd risk missing something crucial that introduces a new bug. Please focus on just the info.yml changes in one issue, just the accessCheck() calls in another, etc.

This can all be done in child issues.

Also, not critical as per the Priority guidelines (there is a workaround: don't upgrade to D10 yet)

heddn’s picture

Status: Needs work » Needs review
StatusFileSize
new6.9 KB

I've split the boilerplate changes for tests into #3328308: Test changes for D10 compatibility and the explicit access checks for entity queries into #3328351: Explicit access checks on content entity queries. Plus, views have been cleaned-up in #3328366: Rebuild views to remove 'core' key.

The patch here is _much_ smaller and addresses the below remaining change records. #3000490 says it is for Drupal 9.4+, but our use of it is in tests only. There is no mention of it outside of a test. However, the symfony route object change came in with 9.1. For that reason I've updated .info.yml files with a minimum version of 9.1. We could be less cautious and mark that 9.4 seeing as 9.3 no longer has support. But I'll leave that up to committers when they land this thing.

https://www.drupal.org/node/3151009
https://www.drupal.org/node/3159012
https://www.drupal.org/node/3000490

Maybe obvious, but we should land the children issues before this one.

kristiaanvandeneynde’s picture

Status: Needs review » Fixed

One event seemed to have flown under the radar but I caught it :) Also not sure why we no longer need $this->moduleHandler->getImplementations('entity_type_build')->willReturn([]); in the relation type manager test, but tests seem to still go green without it.

heddn’s picture

Status: Fixed » Needs review
StatusFileSize
new362 bytes

Adding an empty patch here to start a test run on 10.1, just to confirm everything is OK on the testbot on 10.x.

heddn’s picture

Status: Needs review » Reviewed & tested by the community

Great, they came back green. Moving to RTBC in hopes that we can get test runners setup on commit for 10.x branch so no regressions.

kristiaanvandeneynde’s picture

Status: Reviewed & tested by the community » Fixed

Made default tests run on D10, with commit-based tests also for D9

berdir’s picture

the latest 8.x-1.x release is also tagged as D10 compatible, it might not be too much work to backport these fixes there if you're OK with committing that. The vast majority of sites are still on that version and since it can be installed without actually being compatible it's probably going to create a bunch of support requests and might be more work in the end to deal with those ;)

kristiaanvandeneynde’s picture

As far as I'm concerned and for my own sanity, Group 1 will be EOL as soon as 2/3 are stable early Q1 2023. Having said that, if someone was willing to take all patches here and backport them to 8.x-1.x, I would consider committing them and tagging a final release for the 1.x branch.

For all other intents and purposes, the 1.x branch is "feature complete" as in it won't be getting any new ones. The API is just too old and annoying to cater to new features with compared to v2/3.

berdir’s picture

> I would consider committing them and tagging a final release for the 1.x branch.

That's all I'm asking for, definitely not expecting any new features :) We have a ton of customizations, extra modules and patches in our project, and updating that is likely not going to be trivial.

I might work on that some time early next year, but no promises. Depends on what we expect to be easier.

Status: Fixed » Closed (fixed)

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

ajits’s picture

StatusFileSize
new10.8 KB

Uploading a patch for 8.x-1.x that fixes the fatal errors.