Helper method/service/trait/thing to get the active group?

What would you suggest? We put that in a service instead? Something that could return the Group detected by Group Sites? I'd be up for that, but I'm a bit strapped for time. Feel free to open an MR with your suggestion so we have a starting point for discussion.

Okay I see what you're getting at now. I would approach this slightly differently, so that we don't have to repeat this code if we care about the cacheable metadata. Domain has a pretty good class/method names for this that we can mimic.

Something like: GroupSitesNegotiator::getActiveGroup($cacheable_metadata)

public function getActiveGroup(CacheableMetadata $cacheable_metadata): GroupInterface|null {
    $cacheable_metadata->addCacheTags(['config:group_sites.settings'])
    $context_id = $this->configFactory->get('group_sites.settings')->get('context_provider');
    if ($context_id === NULL) {
      return NULL;
    }

    $contexts = $this->contextRepository->getRuntimeContexts([$context_id]);
    $context = count($contexts) ? reset($contexts) : NULL;
    if ($context) {
      $cacheable_metadata->addCacheableDependency($context);
    }

    $group = $context?->getContextValue();
    if ($group && !$group instanceof GroupInterface) {
      throw new \InvalidArgumentException('Context value is not a Group entity.');
    }

    return $group;
}

Then we can inject that into GroupSitesAccessPolicy and use that there.

Logging these notes about how where we're requiring the group from context in our code base at present. Just posting here, really notes for myself about the cacheable metadata. In the interim I'd made a .module function to get the group, we're calling this, so looking at where we're using it, some are legacy, but:

Decide which page to redirect to on login
https://github.com/localgovdrupal/localgov_microsites_group/blob/98c4e17...
I don't think we mind the cacheable metadata, but we do have it from $form_state.

Adding a relationship entity for content made outside the group forms on a group sites domain
https://github.com/localgovdrupal/localgov_microsites_group/blob/98c4e17...
and also legacy versions of doing that, which we can remove
https://github.com/localgovdrupal/localgov_microsites_group/blob/98c4e17...
https://github.com/localgovdrupal/localgov_microsites_group/blob/98c4e17...
Both cases, we've got the entity, so can pass the cacheable metadata from it, but don't think it makes a difference?

Something clever with the Taxonomy Term form
https://github.com/localgovdrupal/localgov_microsites_group/blob/4.x/mod...
which probably will benefit from having cacheable metadata.

Finally some workaround
https://github.com/localgovdrupal/localgov_microsites_group/blob/98c4e17...
where it looks like the original cache tags aren't yet correct. We can get cacheable metadata from the entity, but it's not going to make a difference here.

Switched to use GroupSitesNegotiator naming, added setting the CacheMetadata. Made this optional. These tested in the Kernel test.
Then changed the AccessPolicy to use the new Negotiator. Updated the Unit tests to reflect the changed internals, and added equivalent Unit tests for the GroupSitesNegotiator.

As we discussed in person, looking good but let's make it more defensive and clear that the negotiator is internal

Think that covered them. The additional Unit test for the Negotiator is left, but might just duplicate what is in the Access Policy test.

Sorry for the LONG delay.

I just had another go at the MR, I also noticed ContainerAwareTrait is dead so need to create an issue where we replace it with a service locator or container dependency.

Should be all green now. Will commit and release after my vacation.

Thanks!

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

I just wanted to chime in, because this has created one of the most subtle bug that I have ever seen.

So in principle I can't see anything wrong with this service. However, I experienced a pretty bad bug when I used this service in a AccessPolicy that I added to my site.
So to describe a bit more what is going on:
I created global user roles for editors with the permission to use ckeditor etc, but not to be assigned to users. Then I created corresponding group roles in my group_sites groups where editors are managed.
Then I added a AccessPolicy to grant the same permissions as the global role would have given you in the drupal scope if you have the corresponding group role in the active group.
Maybe (most likely) there is a smarter way to do this?

But, once I use the service added in this issue, Anonymous users get access to group content of other sites.

So I replaced this service with a service that uses the GroupFromDomainContextTrait and does

$this->domainNegotiator->getActiveDomain(TRUE);
return $this->getGroupFromDomain();

Not only does this return a group more reliably (when I reset the domain negotiator cache) but it doesn't suffer from the same bug.
Debugging a bit I found that the bug also disappears if I add the domain negotiation cache reset to line 69 in GroupFromDomainContextTrait. So maybe there is something going on there?

But also could we use RefinableCacheableDependencyInterface instead of CacheableMetadata that way I could pass the calculated permission directly when getting the group. If you want I can open new issues for those things. But I wanted to chime in here since this has taken me more hours than is reasonable to find out.

Perhaps we should discuss this further in dedicated issues as you suggested. My hunch is there's a cache context missing somewhere.

But also could we use RefinableCacheableDependencyInterface instead of CacheableMetadata that way I could pass the calculated permission directly when getting the group.

Please post the relevant code in the issue summary (for the new issue)

Thanks for the reports!