Figure out a way to cache lists using group permissions

Cleaned up the description a little bit.

I also ran into caching problems with JSONAPI before. But, completely different set up to the case above.

Thanks for the report @skyredwang. However the jsonapi module is not doing anything special about authentication. It only uses the authentication providers.

Can you test that same scenario but using REST core instead of JSON API? That may be able to confirm that the problem is outside of JSON API.

Unpublishing.

This was unpublished due to the security vulnerability, but given that there is no stable release, this can be handled in public.

How can this be RTBC if there is no patch?

I just noticed this issue, but I hardly think the caching that is being fixed in the other issue has any effect on this one.

That said, the access control handler for Group does not allow anonymous access to group/1 unless you have the permission to view that group. Correct me if I'm wrong, but doesn't the REST layer respect the entity types' access control handlers? If so, you should not get different results when switching between a browser request or a REST request.

This is a just an educated guess, but maybe you could try adding the group_membership.roles.permissions cache context to every page? It's a change I've been contemplating for doing in Group as I fear we'll have to do that either way.

The X-Drupal-Dynamic-Cache:HIT indicates that the cache come from Dynamic_page_cache module.

when visit /jsonapi/group/business
The cache_dynaic_pagic_cache table will generate two records:

The two cids are:

1. response:[request_format]=html:[route]=jsonapi.group--business.collection35786c7117b4e38d0f169239752ce71158266ae2f6e4aa230fbbb87bd699c0e3
2. response:[request_format]=html:[route]=jsonapi.group--business.collection35786c7117b4e38d0f169239752ce71158266ae2f6e4aa230fbbb87bd699c0e3:[url.query_args:fields]=:[url.query_args:filter]=:[url.query_args:include]m-PWCW9TcAb0aNazDIBPbvqdWfBpkO_U1gfeV6d9Gxk

We can see the two cids don't have user or user role or group role or user permission information. So every user visit /jsonapi/group/business will get the same cache result.

Cache cid was generated by cache key and cache context.
The root problem is group's cache context.

proposed resolution:
Add "group" and "group_membership.roles" or group_membership.roles.permissions cache context to group's cache like #18 said.

I set http.response.debug_cacheability_headers: true in development.services.yml.

Then we can see the X-Drupal-cache-Contexts:

url.query_args:fields url.query_args:filter url.query_args:include url.query_args:page url.query_args:sort url.site user.permissions

It already has user.permissions

Then I'll try whether group_membership.roles.permissions cache context which @kristiaanvandeneynde suggest on #18 can solve this problem.

@caseylau thanks for looping back into this. Let us know if that fixes your problem. Also, please link any issues in the Group module here as well.

delete

@e0ipso @kristiaanvandeneynde Add "group_membership.roles.permissions" to group entity can't solve this issue.

I add

  /**
   * {@inheritdoc}
   */
  public function getCacheContexts() {
    $this->cacheContexts[] = 'group_membership.roles.permissions';
    return $this->cacheContexts;
  }

directly to Group.php to make a test, and ensure that the context was added successfull, But the cache issue still there.

After adding group_membership.roles.permissions

GroupMembershipPermissionsCacheContext->getContext

  public function getContext() {
    // If there was no existing group on the route, there can be no membership.
    if (!$this->hasExistingGroup()) {
      return 'none';
    }

$this->hasExistingGroup() get null cause it don't take effect.

\Drupal\group\Cache\Context\GroupMembershipCacheContextBase->hasExistingGroup()

  public function __construct(RouteMatchInterface $current_route_match, AccountInterface $user, EntityTypeManagerInterface $entity_type_manager) {
    $this->currentRouteMatch = $current_route_match;
    $this->group = $this->getGroupFromRoute();
    $this->user = $user;
    $this->entityTypeManager = $entity_type_manager;
  }

  /**
   * Checks whether this context got an existing group from the route.
   *
   * @return bool
   *   Whether we've got an existing group.
   */
  protected function hasExistingGroup() {
    return !empty($this->group) && $this->group->id();
  }

\Drupal\group\Context\GroupRouteContextTrait->getGroupFromRoute()

  public function getGroupFromRoute() {
    $route_match = $this->getCurrentRouteMatch();
    
    // See if the route has a group parameter and try to retrieve it.
    if (($group = $route_match->getParameter('group')) && $group instanceof GroupInterface) {
      return $group;
    }
    // Create a new group to use as context if on the group add form.
    elseif ($route_match->getRouteName() == 'entity.group.add_form') {
      $group_type = $route_match->getParameter('group_type');
      return Group::create(['type' => $group_type->id()]);
    }

    return NULL;
  }

We can see group information come from route object.
But route jsonapi/group/business don't come from group module.
So getGroupFromRoute() return Null
Maybe we can change the method of getting group information from route.

Ok so now I see what's going on here. What you're dealing with is yet another example of why we need something like the node grant system for all entity types. The cache context I suggested you use only works when there is a single group in the route; e.g.: group/1, group/2/edit, ...

Because you're dealing with a list of groups and there is no way for Drupal to figure out which ones any given user has access to from a cache context perspective, you'll end up with stale caches when it comes to listing groups. Until #777578: Add an entity query access API and deprecate hook_query_ENTITY_TYPE_access_alter() lands, there is no easy solution but to turn off caching for listings of groups by setting max-age to -1.

Edit: Or add the "user" cache context.

@kristiaanvandeneynde
Thanks for the advice.

add the "user" cache context

I have tested it, works:).

Besides jsonapi/group_content/* has the same issue.

add the "user" cache context

Do this

The patch you suggest won't do because it will add the user cache context all over the place, even in places where we absolutely don't need it. This is no longer a bug report, but a feature request. It could do with an issue summary update to reflect that.

What we need is a cache context that varies by group permissions, which is hard to pull off for all groups on a site at once. The new entity access policies system might help with that, but to be fair I think we should advise all lists that depend on group permissions to use the 'user' cache context for now.

I create a issue about jsonapi/user/user, Not same problem,
But has some similarity, Both have cache issue.

#28's patch has two unneeded line code

The issue summary needs to be updated.

1. Change summary to be a feature request.
2. Change the priority to major, because jsonapi or resetapi can't be used correctly on group or group content now.
3. And add enough information on comment in this patch.

Have a small comment error.
I'm very sorry for uploading so many patches and comments to influence you because of my negligence.

I'm sorry but I can't accept a patch that adds the user cache context to lists. Some lists might not care about who can see what because they know everyone with access to the list has access to all entries. Not serving all of those users the same cache entry then is a step backwards in terms of performance.

The real issue is that core does not hand us any tools to cache entity lists based on access other than permissions or roles. So the best fix would be to figure out a way to create a cache context that compares someone's group permissions in a way that it's relevant to the page content. Given the complexity of that, it might be better to figure out another way of doing it.

Maybe for every entry in the table, add a cache context that says entity_access:entitytype.id.operation where you substitute the last three parts for the actual entity type, id and operation. Although that would increase the potential amount of cache entries by a lot, even when the set of group permissions would only imply a handful of possibilities.

This seems the same as #2952714: Group module's GroupAccessResult::allowedIfHasGroupPermission(s)() does not include cacheability? If not the same, then definitely closely related.

2906082-34.patch can't solve this issue completely，

Like https://www.drupal.org/project/drupal/issues/2982770
Two user, If one user request(use right password) the jsonapi/group_content/...... get empty list.
The other user will get the empty list too.

This comment's patch both solve this issue and Group module's GroupAccessResult::allowedIfHasGroupPermission(s)() does not include cacheability

But whether this patch is just a temporary or the most appropriate solution, this needs more exploration

For those interested, I have a fix in a separate branch that I hope to merge into the main one soon. It still needs tests and some polish first, though.

Okay so after weeks of working on this, I finally have a patch with tests to try!

This introduces three new cache contexts, one of which is now actively being used in GroupAccessResult:

• user.group_permissions (used in GroupAccessResult)
• user.is_group_member:%group_id
• route.group

Fixed a minor mistake and fixed failing tests in other issue.

Might have to reconsider user.is_group_member:%group_id to become user.group_memberships as it's too dynamic on websites with many groups. Currently asking around on Slack about that.

Also noticed the user.group_permissions cache context could do with a unit test. Let's see what testbot thinks about this first, though.

Guess we can keep user.is_group_member:%group_id, but have to document it should not be used for very dynamic elements (such as the group operations block). It could still prove useful for, let's say, a call-to-action urging people to join a particular group.

This means I need to adjust the group operations block to cache per user, user.group_memberships (not a fan of that) or a combination of user.is_anonymous and user or max-age 0, given how we should be able to cache that block just fine for all anonymous users.

This adds the missing unit test and fixes a few minor flaws. Still needs a final pass, especially regarding the cacheable metadata of the GroupOperationsBlock.

After use this patch, when we use member account to visit /jsonapi/group_content/....., we got error:

The website encountered an unexpected error. Please try again later.
Error: Call to undefined method Drupal\Core\Session\AccountProxy::getCacheTagsToInvalidate() in Drupal\group\Access\GroupPermissionsHashGenerator->buildMemberPermissions() (line 297 of modules/contrib/group/src/Access/GroupPermissionsHashGenerator.php).

#46 I got a fix coming for that one!

test for group-2906082 -51.patch

1. this issue's summary
2. https://www.drupal.org/project/group/issues/2906082#comment-12697232 .
3. https://www.drupal.org/project/group/issues/2952714#comment-12581508 .

All passed 🎉

Excellent news! 🎉🎉🎉

Moved the cacheable metadata for group operations to a dedicated method. This is the only way to reliably set cache contexts when there are no links because of a context value. Let's see what the tests have to say, especially regarding coding standards and then we can commit if it stays green.

Special thanks to casylau and Wim Leers for helping out on this issue and the related one(s)! Also adding berdir because he gave me meaningful insights into the caching system over Slack, which allowed this patch to be written.