We also need to invalidate list cache tags now that we no longer save entities upon (un)grouping

Just to add some more information, we actually do have this in EntityQueryAlter:

    // If any new relationship entity is added using any of the retrieved
    // plugins, it might change access.
    $cache_tags = [];
    foreach ($plugin_ids as $plugin_id) {
      $cache_tags[] = "group_relationship_list:plugin:$plugin_id";
    }
    $this->cacheableMetadata->addCacheTags($cache_tags);

That should theoretically fix all of our problems and make this issue moot, but we still have the following core issue blocking that path: #3516034-43: Add cacheable metadata to SelectInterface and entity QueryInterface objects Especially comment 43 proves that we are far from everyone neatly consuming the query's cacheable metadata on their render objects.

Okay existing tests go green but I'd like to add more tests that prove this workaround fixes things and can also prove that things are still working when we remove the workaround after the core issue is fixed.

Okay I have added ample information to the MR now.

So in theory the following is already true:

1. We invalidate the subject entity's individual cache tags upon group relationship save or delete
2. Drupal invalidates the group relationship's list cache tags upon insert or delete (See EntityBase)
3. Our query alters add said list cache tags to whatever list they altered, but only if there was a render context. So API-only modules may be out of luck here

Meaning that this issue wasn't such a big problem after all, but we still want to make sure lists built outside of a render context are also secure.

That is what this issue currently fixes until core issue #3516034: Add cacheable metadata to SelectInterface and entity QueryInterface objects lands. The downside is that we may be invalidating list cache tags we do not need to invalidate because they were covered by our group relationship list cache tags. But we cannot know that for sure, so we need to take the necessary precautions and invalidate them anyway, collateral damage or no.

I've left ample @todos in the code to get rid of our workaround as soon as core helps us out here. We should then also rewrite our query alters to no longer use the render context to push our cacheable metadata up as core should take care of that for us at that point.

@vensires

As long as Group v4 is installed and the changes from MR!314 are implemented, we won't have any extra need for patches from core issue #3516034: Add cacheable metadata to SelectInterface and entity QueryInterface objects, right?

Correct. This is a workaround that we can revert at any given time without breaking things.

If/When the core issue gets fixed, at least from what we currently know, will there be a need for this issue's changes to rollback?

We would be removing code at that point, as core will take care of most things for us. The only change we'd need is QueryAlterBase and its descendants no longer keeping track of cacheability themselves, but rather pass it to the underlying query.

This would also not be a breaking change, as we could change the internals of ::applyCacheability() to only then pass things to the underlying query. We'll see how the core issue evolves first, though.

The issue and MR here is a pre-emptive security fix for lists built outside of a render context. I am not aware of any such lists, but wouldn't be surprised if modules such as jsonapi, rest, etc. may end up creating such lists. Any lists created via render arrays, through views, etc. have been and will remain to be secure.

Okay, so after a long internal discussion with @catch I'm going to go ahead and not commit this. The reasoning is as follows (summarized):

My concerns are theoretically valid, but practically not. All modules that generate generic listings with queries that could be altered currently have no proper means to check whether any cacheable metadata was added to said query. Therefore, even if in theory they should not, they all resort to some sort of render context trickery.

More detail below for those who are interested.

Example: jsonapi's EntityResource::executeQueryInRenderContext()

  * @todo Remove this after https://www.drupal.org/project/drupal/issues/3028976 is fixed.
   */
  protected function executeQueryInRenderContext(QueryInterface $query, CacheableMetadata $query_cacheability) {
    $context = new RenderContext();
    $results = $this->renderer->executeInRenderContext($context, function () use ($query) {
      return $query->execute();
    });
    if (!$context->isEmpty()) {
      $query_cacheability->addCacheableDependency($context->pop());
    }
    return $results;
  }

As you can see, they too realized they need to take care of cacheable metadata and linked to a core issue that is related to the one I'm linking to in this issue. And, as mentioned in the MR and the comments above, when there is a render context, Group is already secure regarding list invalidation.

So with that said, the fallout of this MR could be that we invalidate too many lists when we don't need to, all in the name of security, when there are no practical examples of lists being built outside of a render context or without the above render context trickery.

At this point it's a minor and calculated risk to move forward without this MR as the entirety of core and contrib's query handling will need to be rewritten anyway once we have core support for adding cacheable metadata directly onto queries.

So moving forward as such, but glad we at least took the time to thoroughly revisit this before shipping version 4.0.0.