Routes are using non existing global permission

I have noticed other issues with the access to the routes and the group permission entity. I am investigating the issue. I will upload another patch when I am done.

I have added MR to fix all issues listed above...

@msnassar I don't like completely an approach you used to check route access, I will check it a bit later this week, I wonder if I could find a better way to handle it. Thanks as usual for your contribution

@msnassar
1) Since group permission entity can't exist without group, there is no sense to keep global permissions, it was a mistake, when I created module in the first place
2) I don't think we need so many permissions to handle group permission entities. So, I simplified everything and I keep only one permission, which will give you access to all operations, including revisions. If there will be demand to split them I can introduce new permissions later. For now let's keep it simple
4) I dropped GroupPermissionEntityAccessCheck class since we don't need it anymore and also refactor a bit GroupPermissionHtmlRouteProvider

Please test it and I will proceed with other tickets

Hello @LOBsTerr
I agree with the first 2 points.
I wonder if the last one is the way to go. Here is why:

1. Usually the access to entity routes are handled by adding _entity_access as a requirements. It is not possible to do this with some group permission routes (e.g. delete route) because we do not have the group permission entity in the route
2. The _entity_access grantees a correct access check on the route based on the entity access handler. So we don't have to add other access requirements (e.g. permissions) to the route explicitly. This grantees that any change that is being applied to the entity access handler, will automatically be applied to the route access without having to change the access requirements on the route.
3. Because of the above 2 points, I have added a custom GroupPermissionEntityAccessCheck.
4. Without applying the entity access check on routes (e.g. delete route and revisions route) and having only the _group_permission requirements, users will get fatal error (see below) when visiting the delete/revisions pages for not existing group permission entity. This is because the user has the access to the route anyway. Meaning the user has access to the route even if the group permission entity is not exist!

Here are the fatal errors:

The website encountered an unexpected error. Please try again later.
Error: Call to a member function getGroup() on null in Drupal\group_permissions\Form\GroupPermissionDeleteForm->getQuestion() (line 64 of modules/contrib/group_permissions/src/Form/GroupPermissionDeleteForm.php). 

The website encountered an unexpected error. Please try again later.
Error: Call to a member function getGroup() on null in Drupal\group_permissions\Controller\GroupPermissionsController->revisionOverview() (line 316 of modules/contrib/group_permissions/src/Controller/GroupPermissionsController.php).

I think we have 2 options to solve the issue:

1. Keep the GroupPermissionEntityAccessCheck. But we should simplify it, as now we have one access point check "one group permission". So we no longer need to pass the operations but instead we should pass TRUE (e.g. ->setRequirement('_group_permission_entity_access', 'TRUE'))
2. Add the group permission entity to all routes and then use _entity_access.

Let my know your thoughts please.

1. It looks like we still need custom access check, because of this ticket ✨ Enabling per-group permissions by group type Needs work
2. Are you sure you checked the latest changes, we have group entity in every route (including revisions), and _group_permission access check works without any fatal errros.

Also we can drop GroupPermissionAccessControlHandler, since it is not used.

@msnassar

1) I have fixed some small issues not related to this ticket
2) I have removed access handler, since we are not using it
3) My idea is still to use "override group permissions" to control access to all routes related to group permissions
4) I can't see any fatal errors, because we have group entity for all group permissions related routes. I have tested.
5) For the related ticket we will introduce custom route access handler to check if group permissions are enabled or not.

Could you please check it again?

@LOBsTerr
Thanks for moving on with this issue. Here is my feedback about the latest changes and my previous comment #13

Are you sure you checked the latest changes, we have group entity in every route (including revisions), and _group_permission access check works without any fatal errros.

Yes, I have checked and test it the last changes. I know we have the group entity in all routes :) BUT what I have mentioned in #13 is the "group permission" entity!
See below for more info. about the fatal errors.

Also we can drop GroupPermissionAccessControlHandler, since it is not used.

I wonder if this is ideal! By doing this, we lose the access check on the entity type. Meaning websites that are using a different way to create "group permission" entities for example using "rest/json api", might end up not being able to do so or might have vulnerability impact.
This is because they might end up using the "administer group permission entities" global permission. Granting this permission to all global roles that should create "group permissions" entities, will skip the permission granted in the group level. This will have a vulnerability impact. Because if they do so, unprivileged users, will then be able to create "group permissions" entities in the groups that are not suppose to do so! On the other hand, a members who should be able to create "group permissions" entities based on the permission in the group, might not be able to create entities because they do not have the global permission "administer group permission entities" granted to them!

My idea is still to use "override group permissions" to control access to all routes related to group permissions

I am still in favor of applying the entity access on the routes :) This is because of the reasons I mentioned in #13...
On the other hand, every time we need to apply new access rules to the entity, we have to apply them in the routes! Example ✨ Enabling per-group permissions by group type Needs work

I can't see any fatal errors, because we have group entity for all group permissions related routes. I have tested.

Here are the steps to reproduce:

• Create new group. Make sure there is no group permission entity for that entity
• Go to: Group permissions by clicking on the "Group permissions" tab
• You should see the delete and revisions tabs.
• When you click on the tabs or visit group/XX/permissions/delete, you should see the fatal errors.

The issue here is that, the user has access to the delete and revisions pages for a non existing "group permission" entity
!
Let my know your thoughts please.

ok, I agree, I will update it

So, I did a big refactoring you can review changes here https://git.drupalcode.org/project/group_permissions/-/merge_requests/27

1) I remove group_permission from all entity link template, because we need only group
2) I added a standard link template like (view, edit, add). No UI should be more clear to manage
3) I have added necessary controller handlers for it too
4) I updated the routes accordingly. From this moment we will not see any links or will not get any exceptions

@msnassar Could you please check it?

I will tag a release then

@LOBsTerr Thanks for the quick fix...
Just one has been missed... see here https://git.drupalcode.org/project/group_permissions/-/merge_requests/27...

but you have deleted it already in this commit https://git.drupalcode.org/issue/group_permissions-3360880/-/commit/08f6...

I think we good to go to merge it.

Thank you for your help with this ticket

@LOBsTerr Sorry I didn't noticed that the other PR has been merged... Thanks
I wonder what is the plan for Group permission 2.0?

The next step is version 2.0

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