Adding a user to a group does not invalidate his permission cache

Okay so we have this code in

DefaultGroupPermissionCalculator::calculateMemberPermissions()

    // @todo Use a cache tag for memberships (e.g.: when new one is added).
    // If the user gets added to or removed from a group, their account will
    // be re-saved in GroupContent::postDelete() and GroupContent::postSave().
    // This means we can add the user's cacheable metadata to invalidate this
    // list of permissions whenever the user is saved.
    $user = $this->entityTypeManager->getStorage('user')->load($account->id());
    $calculated_permissions->addCacheableDependency($user);

I recall seeing an issue by your hand that complained about this very re-saving of content (in this case user). So maybe you have a patch that prevents this, in which case the user cache tag added to a set of member permissions does not get cleared?

Oh and as you can see by the @todo, I was meaning to introduce a group_memberships:UID cache tag of some sorts that gets cleared whenever you join or leave a group.

If we generalize that to group_content:<bundle>:<target_entity_id>, we would be one step closer to getting rid of that re-saving you disliked. Another important one being the ditching of node grants for entity access queries.

Well I always welcome more tests, so that's a plus :)

It's also wrong as it is always the current user, while the API accepts an account argument.

Same goes for any entity access control handler and other core API's, though. That's what this change record (and issue) was trying to fix, I believe: https://www.drupal.org/node/2633002. So while I recognize the problem, I'm wondering if this particular problem isn't bigger than just Group.

$calculated_permissions->addCacheableDependency($user) is only being done because I haven't had the time yet to properly implement a cache tag which is cleared when your membership is updated. But because we know a user is also re-saved when a membership is updated, we add the user dependency as a poor man's membership cache tag. Both the one you suggest in the IS and the one I mention in various @todo around the codebase show that we could really use such a cache tag.

$calculated_permissions->addCacheableDependency($group_membership) is necessary because your group roles are stored on the membership and so a membership save might mean different roles.

This patch tries to address the last paragraph in #6 and adds suggested test scenarios.

I was getting an error in tests with ::calculateMemberPermissions() in a scenario when a user is removed from a group. I am not sure I fully understand ::calculateMemberPermissions() and whether this method should be called on a user entity that is not part of the group or not... However, adding Cache::invalidateTags(['user:group_memberships:' . $group_content->getEntity()->id()]); seems to fix the issue.

The interdiff in #10 is not fully correct.

Okay, I want to fix this issue before a full release (so soon™). But this obviously needs to see proper cache tags go in first, so let's create an issue for that. See: #3090833: Introduce more list cache tags for GroupContent

I'm in the process of refactoring your patch at DrupalCon. I'd like to explain the intricacies of the new cache context stuff, but can't do that without explaining how the variation cache works. And I guess you don't want to get dragged down that rabbit hole just yet :D

I'll make my changes and run your test locally to see why it's breaking with the user cache context. I'm assuming because the test doesn't set the current user, whereas the user cache context uses that. But as I mentioned before, that is a problem we still have in core which I would prefer to solve there and then mimic here.

See #2667018: Do not use user.permissions cache context in AccessResult? and #2628870: Access result caching per user(.permissions) does not check for correct user

As discussed on Slack, we're going to leave the whole user cache context thingy out of scope for now :)

Also, in regards to cache tags, as I said in #8, this specific cache tags has advantages over a more generic one because it has fewer invalidations, saving a membership without a role change doesn't invalidate it.

True, I was in favor of removing said logic because we might end up special-casing a lot of cache tags then (slippery slope fallacy). I'll focus on adding the other cache tags this week and then see whether we can keep the one introduced here. We might want to make an axception here, but no promises!

Cool, thanks for the feedback.

Your comment block shows that something is trying to invalidate user:ID twice where there is nothing being written to a cache with the user:ID tag in between both cache clears. So it only makes sense that the tag is not invalidated the second time.

It still leaves me wondering how this affects the permissions cache, though as that currently does write to the cache using the user cache tag. I assume it is because you are trying to clear the cache tag of someone other than the active user, which again leads to our whole discussion we had on Slack :)

I'm still somewhat opposed to simplifying this layer because I believe hard-coding cache ID parts that actually represent a cache context leads to inflexible code. I go into detail here https://youtu.be/rAecKDWOPmY?t=909 (4 minutes onwards from the provided timestamp)

In the meantime I've fixed #3090833: Introduce more list cache tags for GroupContent, so I can now have a look at the special members cache tag with the generic ones in place already.

FWIW, I just gave it some more thought and you're right about needing to change the cache ID. Because as far as the permission calculator goes, the question is not "What are the permissions for the current user", but rather what are they for any given user.

Unlike the RouteProvider example in my session, it is valid to ask for a another user's member permissions during the lifespan of a request because A) the Group API supports that and B) I can think of valid use cases where you want to check a number of users' group permissions (e.g.: Send a mail to anyone who can see this group).

That said, it is out of scope for this issue, but should definitely be changed somehow. Might be tricky because some of the code does check for the current user's group permissions and I'd still like that part to vary by cache contexts rather than manually constructed dynamic cache IDs.

Maybe I need to move the cache around a bit, we'll see.

Attached is a patch with the new cache tags being used. The new test will definitely still go red as per #19

This adds new logic to the chain calculator to support accounts other than the current user. It's still out of scope for this issue, but let's upload it here as a proof of concept. Will separate said logic out into its own ticket before committing.

Small interdiff before I split this up into #3099331: Make sure the permission calculator works for non-current-user accounts

#3099331: Make sure the permission calculator works for non-current-user accounts was fixed just now. Please confirm if the issue still persists in dev @Berdir.

I'd like to mark this issue as fixed with full credit to those who contributed, even if the patch moved to another issue.

Please file a new ticket if this persists.