I installed this module as I needed to allow my groups to be either public or private (membership request) without actually making different group types for each. Everything works fine until you actually try to save the permissions of a specific group: Saving (even without modifying the permissions) results into

The website encountered an unexpected error. Please try again later.

Drupal\Core\Entity\Query\QueryException: Entity queries must explicitly set whether the query should be access checked or not. See Drupal\Core\Entity\Query\QueryInterface::accessCheck(). in Drupal\Core\Entity\Query\Sql\Query->prepare() (line 141 of core/lib/Drupal/Core/Entity/Query/Sql/Query.php).

Based on other issue I found about entity queries (https://www.drupal.org/node/3201242), the accessCheck() call is now always required (Drupal 9.2 and above, I'm using Drupal 10.1.0). Would love to see this getting fixed quick! I tried to find the correct places to add the function call, but I'm farely new with Drupal 8/9/10 code/source and didn't wanna mess up anything too badly.

CommentFileSizeAuthor
#11 group_permissions-3368882-1.0.0-alpha10-11.patch1.44 KBjan-e
#5 group_permissions-3368882-5.patch1.44 KBmsnassar
#3 add_access_check_to_entity_query.patch764 bytesphonkala

Issue fork group_permissions-3368882

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

phonkala created an issue. See original summary.

phonkala opened merge request !24

phonkala’s picture

phonkala commented
StatusFileSize
new add_access_check_to_entity_query.patch764 bytes

Sorry have never done a patch before so not sure if that merge request was the correct way, didn't see any other way to add the patch file. But found the option for upload now, here's quick patch to remove the entity query access check error.

ekes’s picture

ekes commented

Adding `accessCheck(TRUE)` is correctly re-implementing what is happening now. Looking at the code I think this should actually be `accessCheck(FALSE)`? Because I'm wondering if this does'nt want to avoid duplicates even if the user doesn't have access to it?

This patch, one way or the other, however is needed for D10.

msnassar’s picture

msnassar commented
StatusFileSize
new group_permissions-3368882-5.patch1.44 KB
msnassar’s picture

msnassar commented
Status: Active » Needs review
keszthelyi’s picture

keszthelyi commented

Tested #5 and it fixes the issue, also it is the correct solution IMO:

  1. for UniqueReferenceFieldValidator access should not be checked (see #4)
  2. fixes another case where accessCheck is not set on the query in GroupPermissionsController::getRevisionIds() (where we should check the access)
keszthelyi’s picture

keszthelyi commented
Status: Needs review » Reviewed & tested by the community
ammaletu’s picture

ammaletu commented

I ran into this today and can confirm that the patch from #5 is fixing this. I also think that having accessCheck(FALSE) is the correct behavior. The way it is now, permissions can not be saved for a group under Drupal 10.1+, which is after all the whole point of this module. :-)

It would be great if you could tag a new release for this. It would be even better if this could be ported to the 1.x branch as well. The concerned class looks the same, so the fix can be applied to both branches without modifications.

jan-e’s picture

jan-e commented

The same problem exists in Group Permissions 1.0. Did somebody already make a patch for 1.0.x?

jan-e’s picture

jan-e commented
StatusFileSize
new group_permissions-3368882-1.0.0-alpha10-11.patch1.44 KB

Reroll of #5 for Group permissons 1.0.0-alpha10. #5 applied quite clean against that release, except for a

Hunk #1 succeeded at 467 (offset -13 lines).

I also tested unchecking some outsider permissions in a group instance. Works OK!

lobsterr’s picture

lobsterr commented
Assigned: Unassigned » lobsterr

I will introduce today a new release with this fix

  • LOBsTerr committed 4d5f3cee on 2.0.x authored by phonkala 
    Issue #3368882 by Jan-E: Access checking must be explicitly set on...

  • LOBsTerr committed 8b19ac92 on 1.0.x 
    Issue #3368882 by msnassar, LOBsTerr: Access checking must be explicitly...
lobsterr’s picture

lobsterr commented
Status: Reviewed & tested by the community » Fixed

Thank you all for your contribution

msnassar’s picture

msnassar commented

@LOBsTerr I wonder why we have different access check in v1.0 and v2.0... I believe that both should have same access check like the one in #5. No?

Status: Fixed » Closed (fixed)

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

lobsterr’s picture

lobsterr commented

@msnassar Sorry, I have missed your comment. Now it is fixed