Centralize permission checks in a service

Proof of concept added

This seems like a great idea!

Tagging for framework manager for their thoughts on it.

Wasn't sure if this will need special tests or if the current tests work.

Wasn't sure if this will need special tests or if the current tests work.

I don't think this needs specific test coverage, so much of our tests already having permissions needed to function.

I was going to rtbc this issue, because it's a simple refactor. The code was only there 2 times - instead of the 3 usually needed for something to be refactored away, but I think the argument for this code needing to decorated by multiple modules is strong in this case.

I can't actually move this to rtbc, but consider this a +1.

Also started a CR.

Regarding tests: I had to ever so slightly change one test that was testing the code that is now moved to the service. As @borisson_ said: Any test that calls for a permission check inherently also covers this code.

Another great example of what this could allow (with Flexible Permissions or similar): Only get "dangerous" permissions once your account has 2FA enabled.

You could also turn off UID 1's god mode switch, revoke permissions based on certain policies, impose content limits (deny "add comment" after spam is detected, etc.

Understood on the tests.

Still think it will need a change record as a new service is being added.

Didn't @borisson_ already add that? Or does the tag need to stay either way?

There is a cr here: [#3348054], I understand that this is fairly short, but without going super deep into groups/domains/flexible permissions/simple oauth like things, this doesn't actually do a lot, so to keep the CR based on what core does; it is just a way to extract the current duplicated code to an overridable service.

Think it would be useful to have a before/after code snippet in the CR

Before
You had to do xyz

Now
You just call this neat new service.

But with code examples.

Sorry for missing the CR before.

This would be a great addition to Drupal core :) At Open Social we've previously had discussions where we'd love to be able to augment all hasPermission calls and this issue would make that possible.

We eventually implemented a workaround in the simple_oauth module which decorates the User, but that's quite a heavy handed approach just to change permission checking. Being able to swap out a service would've been a great help.

The patch itself looks good and I agree that Drupal's current tests should already cover this (since the whole point of the issue is making a drop-in replacement that provides more extensibility).

The only thing that's stopping me from moving this to RTBC is the PermissionChecker name of the class. Though I think it's a great name for the interface, I think for the class it's too generic and suggests only a single right way to do it. My proposal would be to rename it to RoleBasedPermissionChecker which better communicates its current functionality and that other implementations are possible. The uid 1 check makes it do slightly more than just role based permission checks, but I think since we want to get rid of the UID 1 behaviour that's ignorable ( 📌 Add a container parameter that can remove the special behavior of UID#1 Fixed ), or handled with a comment "Drupal assumes user 1 has all roles/permissions".

If the above is a problem and we think RoleBasedPermissionChecker doesn't sufficiently cover the class' behavior then we could also go with DefaultPermissionChecker which would also communicate it's a Drupal default and could be swapped out.

I don't think there's a sensible code snippet to add to the CR since the whole idea of the issue is that all the calling code remains exactly the same. I've slightly altered the wording of the CR to put more emphasis on the advanced use-cases (which Domain and Group are) :)

Re #15:
I'm more fan of calling it DefaultPermissionChecker right now as RoleBased does not cover what's currently happening.

In a later update we could change it to RoleBasedPermissionChecker and decorate it with User1AccessPermissionChecker. This service could then be opted out of at first (e.g. D11) to allow people to turn off this dreaded god-mode switch and in an even later update (e.g. D12) have the god mode decorator stay, but be turned off by default. Then people who locked themselves out of their site only need to temporarily enable said service to regain access.

Updated the CR with example code as well as I could :D

This seems reasonable to enable contrib overrides.

I think PermissionChecker is fine for the naming here - we don't usually prefix single core services anticipating how contrib might swap them out.

I tried to think of a way to not have to call \Drupal::service() - like adding setter injection and then injecting the role manager before calling the method, but of course that would completely undermine the point of this being swappable, so... best we can do.

I think the permission checker interface needs extra documentation - i.e. it should have some kind of warning about swapping it out that you become responsible for security etc. Needs work for those extra docs, but untagging for framework manager review.

Added the requested information to the interface.

I'm content to skip the renaming of the class as per #19. The update in the CR and the interface provide good additional context.

With those changes I'm happy to see this go in :D

One question though.

Doesn't the following:
$this->entityTypeManager->getStorage('user_role')->isPermissionInRoles($permission, $account->getRoles());

Always return TRUE for user 1? Making the IF uid = 1 check redundant? Or is that there for performance reasons? :)

Re #22, no it does not:

  /**
   * {@inheritdoc}
   */
  public function isPermissionInRoles($permission, array $rids) {
    foreach ($this->loadMultiple($rids) as $role) {
      /** @var \Drupal\user\RoleInterface $role */
      if ($role->hasPermission($permission)) {
        return TRUE;
      }
    }

    return FALSE;
  }

I'm also against the UID 1 check by the way, but this issue is not the place to get rid of that. 📌 Add a container parameter that can remove the special behavior of UID#1 Fixed is.

public function hasPermission($permission) {
  if ($this
    ->isAdmin()) {
    return TRUE;
  }
  return in_array($permission, $this->permissions);
}

Which is used by isPermissionInRoles does that no?

No, that isAdmin() check is to see if the role is flagged as admin. Anyone could have that role, it's not limited to UID1.

I still think the if UID=1 could be removed.

I just tested and \Drupal::currentUser()->hasPermission('some permission'); always returns true for user 1. Making the check thus redundant.

With all due respect, Bram, but you are wrong here.

I just tested and \Drupal::currentUser()->hasPermission('some permission'); always returns TRUE for user 1. Making the check thus redundant. Even if user 1 doesn't have the administrator role assigned to him that function returns TRUE.

The code you are quoting is the exact same code that is being moved to this new service. Of course it works like that without this patch, it is the very same piece of code! You can even see this because I initially removed the (int) type-casting of UID 1 in this MR and over a hundred tests failed. When I reintroduced the casting, it immediately went fully green.

I honestly don't see how we could have a UID 1 check in core now, move the code VERBATIM to another place and then decide to change it so user 1 loses their god mode. Please try the patch locally and then remove the UID 1 check. You'll see your core tests will fail.

🤦‍♂️ Think I was sleeping haha. You are indeed correct.

In that case RTBC+++!

No worries, happens to the best of us.

Setting this back to NW to remove 'Please'. I have offered a suggestion for the changed text.

I updated the CR as well, someone should check my work.

Setting back to NR and then RTBC if things still go green as this was merely a text change. Change record changes look fine be me.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

The needs review bot in #35 was correct, however I was able to get the MR diff to apply with patch -p1. This looks good to me and will enable improvements in contrib.

Committed/pushed to 11.x (10.2.x), thanks!

CR published https://www.drupal.org/node/3348054 → - set branch to 11.x but release to 10.2.0

Awesome, thanks!

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

This has caused 🐛 TypeError: PermissionChecker::hasPermission(): $permission must be of type string Needs review .