Rename the 'user.permissions' context to 'user.roles.permissions', to ensure correct cache context optimization

Wait what?

Permissions is handled separately, because we don't invalidate a cache tag for the permissions cache context when permissions are added to or removed from a role.

You don't need to invalidate any tags for a cache context to work? The whole point is that it checks your unique identifier at runtime; in this case permissions hash. It then uses that ID to find a cache item and serves it if found.

When you update permissions on a role, the role is saved and as such the role's cache tags are triggered. The permission cache context already adds the user's roles' cache tags in getCacheableMetadata(), so we should be fine?

P.S. I'm currently already using this approach in the Group module if you want to see an example:

• group_membership
• group_membership.roles
• group_membership.roles.permissions

Oh snap, good catch (no pun intended)!

Although, wouldn't that indicate wrong use of the user.roles cache context? If you are varying by roles instead of permissions, you should not rely on the roles' permissions. If you are, however, varying by permissions you should use the user.roles.permissions context instead.

Example: A block that shows your roles would still be valid in the above scenario. Even if you changed the permissions on said roles.

Can we also keep the ticket open until we've reached a consensus? It tends to not show up on my radar when closed.

By doing this issue, anything on the site using user.roles would result in everything using user.roles.permissions getting cached incorrectly. This is the exact reason why they're side by side instead of nested.

No it wouldn't? Because then user.roles.permissions would be optimized away, setting the cache tags of the user's roles. If that happens, the cache is invalidated whenever a role (or its permissions) are updated.

Now the editor role cache tag gets invalidated, but the permissions hash for users with the editor role wouldn't change - they'd have exactly the same permissions.

That's the case for all optimized cache contexts. It's not an ideal system, but it's the only way to assure that the cache will still be valid.

Example: If we optimize away the user.roles context, we set the user's cache tags. If we then save the user, any cache that had user.roles optimized away would be flushed, even though we may have only changed the user's name and not their roles.

I guess you're right.

It would require two large warnings though:

1. If you are using user.roles where you should be using user.permissions, refactor your code!
2. If you are using user.permissions, replace all occurrences with user.roles.permissions if you want to benefit from the optimized contexts.

The one thing we know we'd have to do regardless of the answers to those questions: we just add cache_context.user.roles.permissions and let it effectively be an alias of cache_context.user.permissions. This lets all code continue to work.

Wouldn't it be more ideal to move the code into the new class and have the old class extend the new one? Then add a @deprecated notice in its docblock to indicate that people should start using the new one.

Other than that, the two could indeed coexist perfectly without affecting one another. So that's a nice bonus for a change :)

Setting to active as it would be rather easy to create a patch for #14

Curious to see what tests will say about this.

But, user.roles can also be optimized away by user.permissions. Because the permission hash we generate is not just based on the permissions but also the role ids. So two sets of roles with the same permissions between them *result in a different hash*.

Given that, we can optimize away in both directions, which IMHO makes neither a child of the other, but overlapping siblings, which is what they are now.

I would argue that, conceptually, permissions are still a child context of roles because of granularity. The fact that they are currently overlapping siblings is due to our implementation, namely the generation of a hash based on role IDs instead of the actual permissions.

If we want to have cache contexts that can be optimized away following the concept behind the context instead of the logic we chose to implement the context with, we should change the way the hash is generated.

Also, even though we *can* optimize in both directions, it is more efficient to optimize away user.roles because of at least two reasons:

These do seem like benefits, but they still break rank with the concept of the permissions cache context. I would argue that varying said context by a roles hash is actually a bug.

If Bob with roles X and Y has the same permissions as Alice with role Z, then they will not see the same cache object because of role IDs differing. Even though that has nothing to do with their actual list of permissions and they should ideally be seeing the same cache object.

The only part I'm not up to speed on is the cache redirects (part B). Are you saying that by default, we only have the 'user.permissions' context and if 'user.roles' were optimized away into the former, we'd end up with the same contexts we started with? Somehow meaning we don't redirect? Because if that's the case, then that will only be true for 'user.roles', a rarely used context. As soon as someone adds the more common 'user' to vary by account, we'd still end up with a different context ('user') than the one we started with ('user.permissions').

So while I absolutely appreciate the very thorough feedback and honesty in #19 and I definitely see and understand your point on why reversing the process would be more efficient, I still think we need to optimize permissions into roles.

I think clarity and consistency trumps performance here. Having a context that promises to do X and then have it do Y behind the scenes just doesn't seem right to me. In its current state, 'user.permissions' is actually 'user.roles_hash'. We should definitely fix that in a separate issue first before we can even begin to discuss whether we should commit the patch in #18.

For reference: PermissionsHashGenerator::generate() is right to internally cache the generated hash by user role IDs, but is wrong by making the hash contain role data in ::doGenerate().

The part that's wrong is this:

// Note that for admin roles (\Drupal\user\RoleInterface::isAdmin()), the
// permissions returned will be empty ($permissions = []). Therefore the
// presence of the role ID as a key in $permissions_by_role is essential
// to ensure that the hash correctly recognizes admin roles. (If the hash
// was based solely on the union of $permissions, the admin roles would
// effectively be no-ops, allowing for hash collisions.)
$permissions_by_role[$role] = $permissions;

We should not account for that during, but before the hash generation. Same goes for this part in ::generate().

    if ($account->id() == 1) {
      return $this->hash('is-super-user');
    }

The generator should do what it promises to do: return a hash of permissions. Not special keys, not tainted permissions. How we choose to implement and optimize that is up for discussion, but the end result should always be a permissions hash. Fix that and you automatically fix the user.permissions context to actually do what it advertises.

Edit:
--------
For instance, we could hash the list of all permissions and return that hash when UID === 1 or the user has an admin role. We'd need to internally cache that "super hash" with the right cache tags so that a newly enabled module will purge said cache to make sure the list is rebuilt next time around to show a different permission list.

Right now, if we show a block with "Your permission" to user 1 and enable a new module, the list would be out of date because the generated hash remains the same ('is-super-user').

Thanks for weighing in, Wim.

Regarding the super user I somewhat agree with your logic but would like to point out the system of admin roles that was introduced in D8. They essentially turn any account into a super user.

If we choose to have this special case for user 1, we generate a cache object for one user only. If we instead load all permissions and hash those, we generate a cache object that can not only be served to user 1 but also all accounts that have at least one admin role.

So the logic could be something like:

• Are we dealing with a superuser or does the user have an admin role?
• If yes: Do we have the all-permissions-hash cached? Yes: Return that; No: continue.
• Load all defined permissions and hash those
• Cache the hash and add a 'core.extension' config cache tag, then return it

With the above, we'd make much more use out of the super user cache object.

Disagree on loading permissions, we don't cache that at the moment and only use it on the permissions page,

So let's cache them? It should be as simple as adding the 'core.extension' config cache tag, unless I'm missing something. I also don't see the harm in caching them as they might be expensive to gather anew all the time.

admin role and user 1 even return TRUE for permissions that are not defined and it's even possible to grant a role a permission that's not actually defined (doesn't really make sense of course, but who knows what kind of sites are out there).

Although I don't think we should cater to poorly written modules that check for permissions that don't even exist, a valid use case did just jump to mind. Role X has permissions A, B and C. The module that defined permission C is uninstalled, leaving role X with permission C that is no longer defined.

This would not affect the gathered permissions of the superuser or people with an admin role as they would get the same permission set regardless of the permissions belonging to the defined roles. This would only "break" in two ways:

1. Alice and Bob should have the same permissions even though their roles vary. If one of the roles has a "ghost" permission, their hash would differ.
2. Joe has several roles which, when added to each other, result in him having all permissions. If one of his roles has a "ghost" permission, his hash would differ from the one we use for super users.

There's an easy fix for both of the above: First gathering all of the user's permissions and then do an array_intersect with the "defined" permissions. This will rule out ghost permissions.

this could be a behavior change for existing sites that currently only work because of the user.permissions hash

Changing the logic the way I proposed should do no harm as it will show the exact same thing super/admin users are already seeing, but instead of it coming from either of two cache objects, they'd be served the same cache object. If any site has a different result here, they should break as it will alert them of a misconfiguration that might lead to security issues.

having the roles in there might not be correct semantically, but it allows for better optimizations

I don't argue the benefit of better optimizations. But I don't think a performance gain should come at the cost of writing semantically incorrect code. If the permissions cache context does not actually vary by permissions, we might as well call it the magical access unicorn context :)

Having read through the latest comments again I have changed my mind about the super user hash. It does make more sense to use a special key here because newly introduced permissions (by enabling a module, creating a content type, ...) do not affect super users or admins. So it would not make sense to invalidate their cache objects when permissions are added.

This bit should still be optimized, IMO:

// User 1 is the super user, and can always access all permissions. Use a
// different, unique identifier for the hash.
if ($account->id() == 1) {
  return $this->hash('is-super-user');
}

We need to loop over the user's roles and see if any of them are admin roles. If they are, the user effectively becomes a super user, so we should also return the 'is-super-user' hash.

Re #29:

user 1 (no role other than authenticated, still has all permissions, but role-based access checks don't work for him)

And they rightfully shouldn't! :)

1. Indeed user 1 would have the same roles list as an authenticated user without any roles. But that's the whole point: You should only use the user.roles cache context when you actually need info about the roles. Anything access-related has always been and should always be varied by permissions.

   The easiest example is always to display that which you vary by, so let's assume a block that lists the names of your roles. It would be perfectly acceptable to have that block only show "Authenticated user" for user 1, because that is actually the only role he has (by default). I see no problem in that whatsoever, it's actually the simple truth :)

2. As you have both pointed out earlier and I agreed to in #27, this is not a valid option. We'd have to invalidate the cache objects for user 1 and admin roles every time new permissions are added even though they are very unlikely to actually change that which was cached.

3. This basically proves my point. Roles are more granular than permissions and thus the logical parent.

   In your example: User 1, 2 and 3 have the same effective permissions, but different roles. So when varying by permissions, they should all get the same cache object. But as soon as something on the page varies by roles (e.g.: the roles block mentioned in 1.), they should see different cache objects. So it would be fine to optimize permissions away in favor of roles.

4. See answer 3.

Basically we are facing only one real difficulty here: Implied permissions. See how I used effective permissions in answer 3.? The whole system we currently have would work perfectly under user.roles.permissions, but special care needs to be taken because of implied permissions for UID 1 and admin roles.

Without implied permissions, we could actually even implement Berdir's second statement. Think of the pure, simple blocks I mentioned earlier: A block listing your permissions (varied by user.permissions) would show an empty list for admins and UID 1. But that's actually the truth.

Edit, flawed suggestion, see below: Problem is we need implied permissions to make Drupal usable for admins, which is why we had to come up with the 'is-super-user' hash, which should actually be named 'indirectly-has-all-permissions' and used for both UID 1 and people with admin roles.

It's basically how we treat the aforementioned implied permissions.

Option 1: Treat them as effective permissions

1. We expect a block of "Your permissions" to show all possible permissions on the site for UID1/admin
2. We can therefore drop the special hash and simply grab the hash of all permissions for UID1/admin
3. Yes this means that any time a new permission is added, we need to invalidate the cache for UID1/admin
4. However, when we treat implied permissions as effective permissions, this is the only right choice. Otherwise the "Your permissions" block would contain stale data.

Option 2: Treat them as implied permissions (HEAD)

1. We expect a block of "Your permissions" to show all permissions you actually have
2. For UID1 this would probably be nothing and for admins this could list some permissions. User A in Berdir's example would show Editor roles.
3. Because of this, User A should see a different cache object than UID1, which would not be the case with my 'indirectly-has-all-permissions' suggestion. So consider that one flawed.
4. But even 'is-super-user' is flawed because of this. Suppose UID1 has got the Editor role added to their account. We'd have two cache objects (one for UID1, one for User A) even though they'd see the same result.

So basically our current implementation is flawed. We should either treat permissions as effective or implied, but not a mix of both (which is what we're doing in HEAD).

Just to prove my point, suppose we have user.actual_roles and user.actual_roles.actual_permissions. Two rather useless contexts you'd only use when showing a block of your roles or your permissions. Would you both not agree that in this case user.actual_roles.actual_permissions should definitely be a child of user.actual_roles?

If yes, then this is the real problem: We name our contexts user.roles and user.permissions, but they are in fact not representatives of their names. We currently have user.roles_or_uid_1 and user.permissions_as_seen_by_the_access_layer :D

Nope, that is wrong. Yes, code shouldn't hardcode role checks, but it is sometimes done in custom code and we have to support that. And for views and block configurations it is perfectly valid to have access/visibility checks based on roles.

Visibility? Agreed. Access? Please for the love of all that is holy, no.

I've always taught people to create a custom permission, assign it to a role and then check for that permission. This is easy to pull off with Views, by the way. Roles change, permissions don't. If your entire access layer depends on what roles someone has, you're bound to have a very bad time when those roles change.

Why we still don't have a permission-based access system in the block UI is beyond me. I get that filtering by roles is a nice feature for visibility. But let's please not use that system for access control. The whole section is even named "Visibility".

So my opinion is that we do not cater to people who define access by roles. It's wrong, plain and simple.

Maybe your roles block also has edit link for the roles that only users with administer permissions can see. And because uid 1 and authenticated user have the same roles, they would share the same cache.

This is a very good example of why mixing implied permissions with effective permissions is bad. You're absolutely right that this is a problem. The only reason it currently does not break that way is because of a UID1 check in the user.roles context.

So again, as long as the UID 1 special case exists (and changing that is IMHO not likely to happen any time soon):
* user.permissions can not be optimized away in favor of user.roles
* While maybe not in semantically correct, with the current permission hash implementation, it *is* possible to optimize user.roles away in favor of user.permissions.

As long as the uid 1 special case exists, this is IMHO a won't fix.

Completely agree except for the won't fix. My approach would be to normalize the special case. I.e.: For all intents and purposes refactor user.roles and user.permissions to deal with effective roles and permissions:

1. For cache objects varying by user.roles: Still treat UID1 as an exception. This is something we can't fix unless we introduce a hidden Superuser role and forcibly assign that one to UID1. That alone would fix the access system on so many levels.
2. For cache objects varying by user.roles.permissions: Retrieve all defined permissions for admins/UID1 and return that hash. As a side-effect: Eat the often useless cache invalidations experienced by admins/UID1 when new permissions are introduced.

P.S.: This is exactly how I fixed it in Group. I removed all implied things (by removing a hook that allowed you to alter permissions) in favor of using things that are actually stored in the database. If we store the fact that UID1 has a role Superuser in the database, we could optimize a lot of access checks all over Drupal.

Actually, the more I think about it, the more I think a Superuser role would fix everything. It would even maintain backwards compatibility.

Edit: And we wouldn't even have to store it in the DB because of AccountInterface::getRoles()

  /**
   * Returns a list of roles.
   *
   * @param bool $exclude_locked_roles
   *   (optional) If TRUE, locked roles (anonymous/authenticated) are not returned.
   *
   * @return array
   *   List of role IDs.
   */
  public function getRoles($exclude_locked_roles = FALSE);

With a "Superuser" locked role, we could gradually get rid of a few $user->id() == 1 checks in core in favor of RoleStorage::isPermissionInRoles().

User.roles.permissions would be a possible cache context which would do the following:

1. Loop over roles
2. If any role is an admin role, retrieve the all-permissions-hash (from cache?) and break the loop
3. If not, gather all of the roles' permissions and hash them

Its getCacheableMetadata() would remain unchanged. If new permissions are added, the all-permissions-hash would also change, meaning admins would have their cached objects flushed. In most cases this isn't ideal performance-wise, but it would be right in the purest sense of the cache context.

User.roles would become a lot cleaner:

1. We could get rid of the 'is-super-user' bit altogether as we can now be sure UID1 actually has a role that reflects their admin powers.
2. Its getCacheableMetadata() would also remain unchanged.

It would simplify both cache contexts a lot, introduce more consistency in D8 regarding the UID1 special use case and allow us to gradually remove $user->id() == 1 checks in core in favor for the API we have for all other users.

I have revived the following issue: #540008: Add a container parameter that can remove the special behavior of UID#1. If that one lands, we can actually fix this issue properly as suggested above.

With #3371246: [Meta, Plan] Pitch-Burgh: Policy based access in core around the corner (hopefully?), this issue no longer makes sense.

Current situation: Your permissions are not calculated based merely on your role, but also on whether or not you are user 1, as rightfully pointed out in #33. So until that was fixed, we could never resolve this issue.

Future situation (again, hopefully): Your permissions are calculated based on an unpredictable amount of access policies, some of which being your user roles and your UID1 status. So as soon as we have access policies in core, we will never be able to fold the user.permissions cache context into user.roles and for good reason.

So if everyone agrees, we can close this issue as won't fix as it's looking more likely that access policies will turn this into a moot point. Furthermore, access policies will allow us to land #540008: Add a container parameter that can remove the special behavior of UID#1 after ~14-15 years :)