Centralize permission checks in a service

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.

Understood on the tests.

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

There is a cr here: #3348054: Permissions checking has been centralized to a new PermissionChecker service, 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 (#540008: Add a container parameter that can remove the special behavior of UID#1), 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) :)

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.

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? I have to admit I didn't test or look into the isPermissionInRoles() function 😉.

Just asking because I'm usually against the UID=1 check in my own projects.

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

Which is used by isPermissionInRoles does that no?

The issue you linked to remove that logic bottomline. But here we are adding it to a service which I think does not need this check.

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. Even if user 1 doesn't have the administrator role assigned to him that function returns TRUE.

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

In that case RTBC+++!

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.

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.

Should be NR.

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

This has caused #3410098: Deprecate passing non-strings to UserSession::hasPermission() and User::hasPermission().