Subgroup role mapper

Either here or in a different issue, we should also add or modify gnode_node_grants() to take into account the same inheritance mappings.

Unfortunately it doesn't work like that, because the permissions check only happens against groups that the user is actually a direct member of:

 $membership_loader = \Drupal::service('group.membership_loader');
  foreach ($membership_loader->loadByUser($account)

Within that loop, we'd need to find the relevant parent/child groups of the current membership group and add them, or we'd have to just load every group on the site instead.

OK loading all groups has some scaling issues, but will be a lot simpler than trying to figure out the parent/child group relationships in that foreach so probably a good option.

I used the following in gnode_node_grants. There's a bit of extra overhead because it will check subgroups that the user may not have inherited roles in, but it's much less than checking every group.

  /** @var \Drupal\group\GroupMembershipLoaderInterface $membership_loader */
  $membership_loader = \Drupal::service('group.membership_loader');
  if (\Drupal::service('module_handler')->moduleExists('ggroup')){
    /* @var \Drupal\ggroup\GroupHierarchyManagerInterface $group_hierarchy_manager*/
    $group_hierarchy_manager = \Drupal::service('ggroup.group_hierarchy_manager');
  }
  $gids = [];
  foreach ($membership_loader->loadByUser($account) as $group_membership) {
    $group = $group_membership->getGroup();

    // Add the groups the user is a member of to use later on.
    $member_gids[] = $gid = $group->id();
    $gids[] = $gid;

    if (isset($group_hierarchy_manager)) {
      $gids = array_unique(array_merge($gids,$group_hierarchy_manager->getGroupSubgroupIds($gid), $group_hierarchy_manager->getGroupSupergroupIds($gid)));
    }
  }

  foreach ($gids as $gid) {
    if ($group = $entity_type_manager->getStorage('group')->load($gid)) {
      foreach ($node_type_ids as $node_type_id) {
        $plugin_id = "group_node:$node_type_id";

        switch ($op) {
          case 'view':
            if ($group->hasPermission("view $plugin_id entity", $account)) {
              $grants_m["gnode:$node_type_id"][] = $gid;
            }
            if ($group->hasPermission("view unpublished $plugin_id entity", $account)) {
              $grants_m["gnode_unpublished:$node_type_id"][] = $gid;
            }
            break;

          case 'update':
          case 'delete':
            // If you can act on any node, there's no need for the author grant.
            if ($group->hasPermission("$op any $plugin_id entity", $account)) {
              $grants_m["gnode:$node_type_id"][] = $gid;
            } elseif ($group->hasPermission("$op own $plugin_id entity", $account)) {
              $uid = $account->id();
              $grants_m["gnode_author:$uid:$node_type_id"][] = $gid;
            }
            break;
        }
      }
    }
  }

  foreach ($gids as $gid) {
    if ($group = $entity_type_manager->getStorage('group')->load($gid)) {

There's a performance issue here though in that rather than doing a single multiple load of groups, we're individually loading the subgroups. So while there are less permission checks, there's more individual database (or cache) queries. The trade-off is going to depend on the specific site, but I think I'd lean towards just loading and checking all the groups, and maybe adding an extra cache layer on top of it.

One other question here, sort of related to the above. It's quite common to want to get 'all groups the current user is a member of' (using the group membership loader), but the usual methods don't work once you're using subgroups. Should we add new methods for this?

Thank you for all of your work separating this module out.

I'm working on a project that has been using the subgroups patches from parent ticket #2736233. We have three levels of groups -- parent, child, grandchild -- with members being added to the child group in order to have access to content in the parent group. Members of the child group also form and join the grandchild groups, and the child group administrators inherit the ability to add and remove members from the grandchild group without actually needing to be added as members to the grandchild groups. We have a couple dozen behat functional tests that we've been running to verify this functionality is working.

We've been pinned to the patch from #244 because some time between that patch and the final patch (#261) on the parent ticket, role inheritance stopped working for us.

Attached is an updated ggroup_role_mapper patch that incorporate a few of the suggestions on this ticket since comment #2 as well as the suggestions on comment #263 on the parent ticket. At least in our case, with these changes in role mapping works now using ggroup and ggroup_role_mapper. All of our tests are passing again. :)

Again, thank you much for all the work you've been doing on this! I hope breaking up the patch into smaller pieces, as you've done, does lead to subgroups and subgroup role mapping making it into the group module before too long.

Just to say I'm using this patch on a project (development only at the moment) and #12 seems to be working well (and fixes a bug in #2 with groups inheriting roles from subgroups).

Hi, I was using a patch #12 and it truly seems to work fine, however, I have faced some issues when working on main user (uid: 1) and get few notcies:
Notice: Undefined offset: 1 in Drupal\ggroup_role_mapper\Access\InheritGroupPermissionCalculator->getInheritedGroupRoleIdsByMembership() (line 276 of modules/contrib/group/modules/ggroup_role_mapper/src/Access/InheritGroupPermissionCalculator.php).

Warning: Invalid argument supplied for foreach() in Drupal\ggroup_role_mapper\Access\InheritGroupPermissionCalculator->calculateMemberPermissions() (line 122 of modules/contrib/group/modules/ggroup_role_mapper/src/Access/InheritGroupPermissionCalculator.php).

As a simple workaround, I replaced in the line 276
return $this->mappedRoles[$account_id];
with
return $this->mappedRoles[$account_id] ?? [];

but I'm not certain if this won't have any implications, also I don't consider that fix as a "clean" solution, I think there might be a problem laying somewhere deep down.

I need to solve the issue that I've mentioned on #14 for the project that I'm currently working on, so here I provide a patch. I introduced a new method getMappedRoles and additional checker hasMappedRoles to unify how we handle mapped roles eg. to always return an array - which was the main issue in my case.

#11 is a pretty important point. There is a getter for membership on the group entity. But no "isMember" method. And the users are not members of the direct parent group in a sub group situation. In some of my site's custom code we have a lot of getMember() calls to find if a user is a member, then do something about it. Some type of fixed API for isMember would be nice. Or maybe "give me the roles" would be better and offer greater flexibility.

+++ b/modules/ggroup_role_mapper/src/Access/InheritGroupPermissionCalculator.php
@@ -0,0 +1,406 @@
+  public function calculateAnonymousPermissions() {
...
+    $groups = $this->entityTypeManager->getStorage('group')->loadMultiple();
...
+    foreach ($groups as $group) {
...
+  public function calculateOutsiderPermissions(AccountInterface $account) {
...
+    $groups = $this->entityTypeManager->getStorage('group')->loadMultiple();
...
+    foreach ($groups as $group) {

These functions are a performance issue and need re-thinking.

Might be OK on a site with 10 groups, but testing with my project with 38k it breaks.

I'm wondering why anon and outsider permissions should be included in this.

Found a bug/performance issue - some calculated permissions items are being created with SCOPE_GROUP_TYPE instead of SCOPE_GROUP. This means that when SCOPE_GROUP items are retrieved in gnode_node_grants, it's calculating for all a lot of groups mis-labeled as group types.

Updated patch for that.

Did a quick review and the changes here make sense. Reviewing the patch in #15 vs #18, there's an obvious issue w/ using CalculatedGroupPermissionsItemInterface::SCOPE_GROUP_TYPE. The latest patch fixes that.

Hi!

I have a weird issue with this patch (in theory the subgroup patch in #3084140 works fine), but the moment I enable this submodule, the issue starts.

I will try to explain the steps and the issue (maybe there is smth i am missing to configure):

- I have a group type created
- I have a content type created. Inside the content type, one of the fields is an 'entity_reference' to the group type created (cardinality infinite)
- That field formatter is autocomplete_deluxe (also tested with normal autocomplete)

Users belongs to one or multiple groups.

Before the patch, each time any user create new content of that content type, in the autocomplete field, they get any of the groups that are created in the platform, based on what they type (ok, desired behaviour)

Now, the moment I enable this module and clear the cache, if the user do the same (create new content type), the moment the user types in the autocomplete field, he only get results if the typed matches any of the groups he is member in (and excluding almost all the groups, not good)

If I disable the module and clear the cache, normal behaviour is back (and all the groups can be listed)

It does not matter if I have any relation created between groups or not, the autocomplete works fine with the submodule core enabled.

Any idea?

Hi again!

The issue i was commenting in 20, it is fixed if I apply the patch in https://www.drupal.org/project/group/issues/3125427

Best,

Hi (me again)

Either I am doing smth wrong or there is an issue with some of the permissions once this module is enabled, making the logged user not seeing all the content, just the one that belongs to its groups. I explain what i have done, adding the question at the end.

Patches I am using:
- "Support for subgroups": "https://www.drupal.org/files/issues/2019-10-23/subgroup_core-3084140-6.p...",
- "Subgroups role mapper": "https://www.drupal.org/files/issues/2020-03-23/3084153-18.patch"

What i have done in a clean installation, to double check the issue:

- Created new site, applied both mentioned patches.
- Enabled subgroup module only
- Created new content type (CT), created new group and enabled the CT in the group as plugin
- Add some groups and some content to the CT created
- Create a view that list the content of the CT created
- Create 2 users
- Create a view that list all the content CT, with 'unrestricted' access (i have edited frontpage).
- In the permissions of the created group, set "Entity: View content entities" for all (anonymous, outsider, member)

So let's say that I have:
- Group 1, Group2 and Group 3.
- Node 1, node 2, node 3, node 4 and node orphan.
- U1 and U2 as users

Assign Node 1 and node 2 to Group 1; node 3 to group 2; node 4 to group 3.
U1 is member of group 1 and U2 of group 2

So far, all good. If any user (anonymous or logged) visits the page, the front page shows everything.

Now: we enable the subgroup role mapper and do a cache rebuild

- If an anonymous user visits the page, frontpage shows all the nodes
- U1 logs: frontpage shows only Node 1, Node 2 and Node orphan (N1 and N2 because they belong to group 1, and orphan as it doesnt have any group relation)
- U2 logs: frontpage shows Node 3, Node orphan.

If I assign Group 3 as subgroup of group 1 (and map the permissions):
- U1 see also now Node 4 once it logs.
- U2 see the same.

Assigning Node orphan to a G1 or G3 makes it visible only to U1.
Assigning Node orphan to G2 makes it visible only to U2.

My question is:
As the view is 'unrestricted', and all the nodes are visible by all the anonymous, should it be the same if a user is logged in, and see all the nodes in the view, independently if they are assigned or not to a group?
Or I am missing though any settings to be configured?

Thnks!

Hello,

As described in the following note "Programatically add a subgroup to a group", I assign the SubGroup via "$parentGroup->addContent($mySubGroup, 'subgroup:' . $mySubGroup->bundle());". The attached error occurs when I try to assign several groups in a loop.

All of the mentioned (sub)groups definitely exist (at the time of the error message) in the system and are also assigned to the group, but the permissions are only correct after a cache refresh.
Something goes wrong here...
If I make the assignment via the UI (and thus individually), it works.

PS:
my_module is the log of "function getSubgroupRelationConfig($group_id, $subgroup_id)"
\Drupal::logger('my_module')->notice('<pre><code>' . print_r($group_id . ', ' . $subgroup_id, TRUE) . '

');
$type = $this->subgroupRelations[$group_id][$subgroup_id];

The whole error:

Notice: Undefined offset: 6 in Drupal\ggroup_role_mapper\GroupRoleInheritance->getSubgroupRelationConfig() (Zeile 284 in /var/www/html/downport/web/modules/contrib/group/modules/ggroup_role_mapper/src/GroupRoleInheritance.php)
#0 /var/www/html/downport/web/core/includes/bootstrap.inc(600): _drupal_error_handler_real(8, 'Undefined offse...', '/var/www/html/d...', 284, Array)
#1 /var/www/html/downport/web/modules/contrib/group/modules/ggroup_role_mapper/src/GroupRoleInheritance.php(284): _drupal_error_handler(8, 'Undefined offse...', '/var/www/html/d...', 284, Array)
#2 /var/www/html/downport/web/modules/contrib/group/modules/ggroup_role_mapper/src/GroupRoleInheritance.php(157): Drupal\ggroup_role_mapper\GroupRoleInheritance->getSubgroupRelationConfig('13', '6')
#3 /var/www/html/downport/web/modules/contrib/group/modules/ggroup_role_mapper/src/GroupRoleInheritance.php(117): Drupal\ggroup_role_mapper\GroupRoleInheritance->build('13')
#4 /var/www/html/downport/web/modules/contrib/group/modules/ggroup_role_mapper/ggroup_role_mapper.module(123): Drupal\ggroup_role_mapper\GroupRoleInheritance->rebuild('13')
#5 [internal function]: ggroup_role_mapper_group_content_insert(Object(Drupal\group\Entity\GroupContent))
#6 /var/www/html/downport/web/core/lib/Drupal/Core/Extension/ModuleHandler.php(403): call_user_func_array('ggroup_role_map...', Array)
#7 /var/www/html/downport/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(204): Drupal\Core\Extension\ModuleHandler->invokeAll('group_content_i...', Array)
#8 /var/www/html/downport/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(843): Drupal\Core\Entity\EntityStorageBase->invokeHook('insert', Object(Drupal\group\Entity\GroupContent))
#9 /var/www/html/downport/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(535): Drupal\Core\Entity\ContentEntityStorageBase->invokeHook('insert', Object(Drupal\group\Entity\GroupContent))
#10 /var/www/html/downport/web/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(728): Drupal\Core\Entity\EntityStorageBase->doPostSave(Object(Drupal\group\Entity\GroupContent), false)
#11 /var/www/html/downport/web/core/lib/Drupal/Core/Entity/EntityStorageBase.php(460): Drupal\Core\Entity\ContentEntityStorageBase->doPostSave(Object(Drupal\group\Entity\GroupContent), false)
#12 /var/www/html/downport/web/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php(837): Drupal\Core\Entity\EntityStorageBase->save(Object(Drupal\group\Entity\GroupContent))
#13 /var/www/html/downport/web/modules/contrib/group/src/Entity/Group.php(165): Drupal\Core\Entity\Sql\SqlContentEntityStorage->save(Object(Drupal\group\Entity\GroupContent))
#14 /var/www/html/downport/web/modules/custom/debitor_subgroup_assignment/debitor_subgroup_assignment.module(102): Drupal\group\Entity\Group->addContent(Object(Drupal\group\Entity\Group), 'subgroup:produc...')
#15 [internal function]: debitor_subgroup_assignment_cron()
#16 /var/www/html/downport/web/core/lib/Drupal/Core/Extension/ModuleHandler.php(392): call_user_func_array('debitor_subgrou...', Array)
#17 /var/www/html/downport/web/core/lib/Drupal/Core/Cron.php(236): Drupal\Core\Extension\ModuleHandler->invoke('debitor_subgrou...', 'cron')
#18 /var/www/html/downport/web/core/lib/Drupal/Core/Cron.php(134): Drupal\Core\Cron->invokeCronHandlers()
#19 /var/www/html/downport/web/core/lib/Drupal/Core/ProxyClass/Cron.php(75): Drupal\Core\Cron->run()
#20 /var/www/html/downport/web/core/modules/system/src/CronController.php(46): Drupal\Core\ProxyClass\Cron->run()
#21 [internal function]: Drupal\system\CronController->run()
#22 /var/www/html/downport/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array(Array, Array)
#23 /var/www/html/downport/web/core/lib/Drupal/Core/Render/Renderer.php(573): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#24 /var/www/html/downport/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext(Object(Drupal\Core\Render\RenderContext), Object(Closure))
#25 /var/www/html/downport/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array)
#26 /var/www/html/downport/vendor/symfony/http-kernel/HttpKernel.php(151): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}()
#27 /var/www/html/downport/vendor/symfony/http-kernel/HttpKernel.php(68): Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object(Symfony\Component\HttpFoundation\Request), 1)
#28 /var/www/html/downport/web/core/lib/Drupal/Core/StackMiddleware/Session.php(57): Symfony\Component\HttpKernel\HttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#29 /var/www/html/downport/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(47): Drupal\Core\StackMiddleware\Session->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#30 /var/www/html/downport/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#31 /var/www/html/downport/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\page_cache\StackMiddleware\PageCache->pass(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#32 /var/www/html/downport/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(47): Drupal\page_cache\StackMiddleware\PageCache->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#33 /var/www/html/downport/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(52): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#34 /var/www/html/downport/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#35 /var/www/html/downport/web/core/lib/Drupal/Core/DrupalKernel.php(708): Stack\StackedHttpKernel->handle(Object(Symfony\Component\HttpFoundation\Request), 1, true)
#36 /var/www/html/downport/web/index.php(19): Drupal\Core\DrupalKernel->handle(Object(Symfony\Component\HttpFoundation\Request))
#37 {main}

Anybody got a good idea?
We are planning a GoLive in July and would like to use this module. Currently, the only way to solve the problem is to clear the cache after the mapping (and the errors mentioned above) or to flush the cache after every assignment:

foreach ($mySubGroups as $mySubGroup) {
  $parentGroup->addContent($mySubGroup, 'subgroup:' . $mySubGroup->bundle());
  drupal_flush_all_caches();
}

I have created a repo where I am working on a re-write of this sub-module

https://github.com/committeehq/drupal-ggroup_role_mapper

The first action was to remove the Anon and Outsider permission mapping.

Next, I think there is a better way to add to the configuration form than using ggroup_role_mapper_form_group_content_type_add_form_alter().

I will then put up an improved patch here.

Great! Thank you! I am ready to test your patch 😃

@dwkitchen

I have tested the modified code in your git repro and I can tell you that the error from #23 did not occure anymore! This is really great! Thank you so much!

I will do some other tests the next days.

Do we need to update this module because of the changes which were made to the groupe 1.0/1.1 release?
I mean the following:

Group takes care of entity access for ALL content entity types. By leaving only node grants in there, it basically means that all other entity types are shit out of luck. Which is exactly why I've been pushing for entity query access for the past few years. It's time core got rid of this "Only works for Node" nonsense and started caring about all entity types. In short: I could not have released a full Group version without getting rid of node grants, because core only seems to care about nodes.

and

Keep in mind that query access will only work for those plugins that declare these handlers. All plugins provided by Group have been updated to declare these handlers.

Another question:

Shouldn´t we use the official contrib modul instead of using this group modul modification?

https://www.drupal.org/project/subgroup

Yes I think this needs to be marked postponed, or probably duplicate (but will let kristiaanvandeneynde do that) of the new contrib module. https://www.drupal.org/project/subgroup

btw:

Within the contrib module there are a few things missing:

1. Views to create/assign subgroups
2. Actions/buttons create/assign subgroups
3. ...

:-(

Ok, as we found out in the discussion with the maintainer of the subgroup module, it is not a duplicate, but takes a different approach.

The difference, for me, is that the subgroup module only allows one "one child group can only belong to one parent group".
This implementation here allows a "one child group can belong to several parent groups".

Maybe this variant, though not in the core of the group module, will make it into a separate Contrib module. I would be very pleased!

As with #3084140: Subgroups core module, because of the creation of subgroup this will move to its own project ggroup

Found a new issue with the patch..

When permissions are calculated, ggroup_role_mapper is calculating permissions for every single group, regardless of membership, in case there is group role mapping set up. This much is fine, however when there is actually no mapping at all, and a user isn't a member of a group directly or via a subgroup, we need to discard those permissions so that the group_type permissions kick in. Otherwise groups module itself uses the group permissions (which may be none) and discards the group type permissions.

For steps to reproduce, I found this with the 'request group access' permission and https://www.drupal.org/project/grequest

Without the debug.

+++ b/modules/ggroup_role_mapper/src/Access/InheritGroupPermissionCalculator.php
@@ -160,13 +160,15 @@ class InheritGroupPermissionCalculator extends GroupPermissionCalculatorBase {
+             (string) $group_id,

@@ -203,14 +205,17 @@ class InheritGroupPermissionCalculator extends GroupPermissionCalculatorBase {
+          $group->id(),

Why do we cast to string for the first, but not the second?

Tested on D8.9 and works as expected. I'm creating a new MR and hiding the patches to focus the collaboration in the MR. Feel free to show the patches again if somebody disagrees.

The MR is based on #37 and I added the following changes in new commits:

• Cast all group_ids to string as suggested on #38
• Fixed core_version_requirements to support D9

MR: https://git.drupalcode.org/issue/ggroup-3084153/-/merge_requests/1

I have created the MR against the project now so it shows up on the issue page here.

• Removed Anon and Outside calculator as identified in #3084153-17: Subgroup role mapper, this removes the need for the changes in #3084153-37: Subgroup role mapper as we can just rely on the base class to calculate the Anon and Outsider permissions - because we haven't mapped any roles.
• Added some error handling from #3084153-23: Subgroup role mapper

@akalam thanks fo getting the set up on the new GitLab MR system - much easier to work on than before.

I detected a bug on the ggroup_role_mapper module. The module works good with 1 level of deep withing the user membership and the tested group. Let me explain myself with an example.

- Create 3 groups (the arrows represent hierarchy):
group 1 - > group 2
group 3

So both "group 1" and "group 3" have no parent group. Now add a user to group 1 as a member. He/she will get the roles for group 1 and group 2, but if you then add the existing group 3 as a child of group 2..
group 1 - > group 2 -> group 3

Then the user won't get the permissions on group 3 until you clean the cache.
The permissions rebuild is done on group_content creation or deletion. This is something that happen in the first scenario but not in the second scenario for group 3.

I added a commit with a fix based on cache tags. We are using the group_content_list:{$bundle} cache tag. After adding it, the permissions rebuild looks unnecessary, so I removed it. Feel free to discuss or to add it again if you find a more efficient way to solve the issue.

@akalam I was just noticing the same issue as I was moving a child group i.e.

Group X > Group Y > Group Z

and then

Group 1 > Group 2 > Group Z

Group X still have access to Group z and Group 1 didn't; I will test with your updates.

Is there no UI associated with this yet? I thought I had seen it before but I must have been thinking of the subgroup module.

update: My mistake, it was because I'd patched the plugin ID from 'subgroup' to 'ggroup' #3209627: Change GroupRelationship id for compatibility with subgroup module.

I found several issues with the role mapping/permission calculation - for example permissions for one group could potentially be overwritten by a different group, and permissions when a user is a member of a subgroup but not the actual group being checked were being discarded. See various changes in the MR.

This could really use some automated test coverage, for example calculating permissions when the user is a member of:

1. Only a subgroup
2. Only a supergroup
3. Both a subgroup and the group being checked
4. Both a supergroup and the group being checked
5. Only the group being checked
6. Outsider permissions
7. Anonymous permissions

Uploading an interdiff vs. the changes prior to around #44, will leave some notes on the interdiff in the next comment to explain some of the changes.

-      $group_role_array = $this->getInheritedGroupRoleIdsByMembership($group_membership, $account);
-       foreach ($group_role_array as $group_id => $group_roles) {
-         $permission_sets = [];
-         foreach ($group_roles as $group_role) {

By getting the inherited group roles by membership, this ends up missing some inherited roles.

Let's say group A is a subgroup of group B. And user 10 is a member only of group A.

When we're checking permissions of group B, we can't base the permissions on membership of group B, but only whether the user is a member of group B, or any subgroups, or any supergroups of group B. This means we need to get all the memberships of the current user, then build the list of group roles for any sub/supergroups they're also in.

The bulk of the bugfix then is here, instead of checking inherited roles based on a group membership, which implies direct membership of the group being checked, we just get all the inherited roles for the user and then the parent checks against the group once those are built:

 
   /**
-   * {@inheritdoc}
+   * Get the inherited roles for groups with subgroup or direct membership.
+   *
+   * @param Drupal\Core\Session\AccountInterface
+   *
+   * @return An array of group roles keyed by group ID.
    */
-  public function getInheritedGroupRoleIdsByMembership(GroupMembership $group_membership, AccountInterface $account) {
+  public function getInheritedGroupRoleIds(AccountInterface $account) {
     $account_id = $account->id();
-    $group = $group_membership->getGroup();
-    $group_id = $group->id();
-    $roles = array_keys($group_membership->getRoles());
-
-    if ($this->hasMappedRoles($account_id, $group_id)) {
-      return $this->getMappedRoles($account_id, $group_id);
+    if ($this->hasMappedRoles($account_id)) {
+      return $this->getMappedRoles($account_id);
     }

The original thing that got me here was a more straightforward logic error:

  $account_id = $account->id();
-    $group = $group_membership->getGroup();
-    $group_id = $group->id();
....
 
-    foreach ($mapped_role_ids as $group_id => $role_ids) {
+    foreach ($mapped_role_ids as $mapped_group_id => $role_ids) {
       if (!empty(array_unique($role_ids))) {
-        $this->mappedRoles[$account_id][$group_id] = array_merge($this->getMappedRoles($account_id, $group_id), $this->entityTypeManager->getStorage('group_role')->loadMultiple(array_unique($role_ids)));
+        $this->mappedRoles[$account_id][$mapped_group_id] = array_merge($this->getMappedRoles($account_id, $mapped_group_id), $this->entityTypeManager->getStorage('group_role')->loadMultiple(array_unique($role_ids)));
       }
     }

As you can see the $group_id variable was getting overwritten in the foreach, this meant the mapped roles were getting completely wiped out depending on particular combinations of group memberships/group relationships.

We need this functionality for the 3.x branch, I imagine it will be complicated to do. If we could sponsor work on the issue to help make it happen, we'd definitely look at that.

I am not sure how much this applies to the Group 3+ way of doing things. Each change in the combination of factors results in a distinct single group role. So maybe it is simpler, in that you can map any source group role to any target group role. Or maybe it has to echo the same scopes as Drupal site roles: one mapping for source group role + member of target group, one for source group role + non-member of target group.

#1 and #3 both seem like they'd add complexity to the current patch, which is already complex enough, so for me I'd prefer not to do this - or at least, to do it in a follow-up once there's a working version in a module somewhere.

If it still works as it did before, where it is a module that has to be enabled separately in addition to ggroup, then whatever is easier to maintain would be the way to go, I think - standalone or component of ggroup.

Being able to optionally have mapping apply to descendants would be nice.