- Issue created by @kristiaanvandeneynde
- @kristiaanvandeneynde opened merge request.
- Status changed to Needs review
over 1 year ago 12:45pm 14 March 2023 - π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Proof of concept added
- πΊπΈUnited States smustgrave
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.
- π§πͺBelgium borisson_ Mechelen, π§πͺ
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.
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
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.
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
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.
- πΊπΈUnited States smustgrave
Understood on the tests.
Still think it will need a change record as a new service is being added.
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Didn't @borisson_ already add that? Or does the tag need to stay either way?
- π§πͺBelgium borisson_ Mechelen, π§πͺ
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.
- πΊπΈUnited States smustgrave
Think it would be useful to have a before/after code snippet in the CR
Before
You had to do xyzNow
You just call this neat new service.But with code examples.
Sorry for missing the CR before.
- π³π±Netherlands Kingdutch
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 toRoleBasedPermissionChecker
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 withDefaultPermissionChecker
which would also communicate it's a Drupal default and could be swapped out. - π³π±Netherlands Kingdutch
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) :)
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
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.
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Updated the CR with example code as well as I could :D
- Status changed to Needs work
over 1 year ago 5:18pm 16 March 2023 - π¬π§United Kingdom catch
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.
- Status changed to Needs review
over 1 year ago 8:17am 17 March 2023 - π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
Added the requested information to the interface.
- Status changed to RTBC
over 1 year ago 8:39am 17 March 2023 - π³π±Netherlands Kingdutch
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
- π§πͺBelgium BramDriesen Belgium π§πͺ
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? :)
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
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.
- π§πͺBelgium BramDriesen Belgium π§πͺ
public function hasPermission($permission) { if ($this ->isAdmin()) { return TRUE; } return in_array($permission, $this->permissions); }
Which is used by
isPermissionInRoles
does that no? - π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
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.
- π§πͺBelgium BramDriesen Belgium π§πͺ
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. - Status changed to Needs work
over 1 year ago 10:16am 17 March 2023 - Status changed to RTBC
over 1 year ago 11:41am 17 March 2023 - π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
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.
- π§πͺBelgium BramDriesen Belgium π§πͺ
π€¦ββοΈ Think I was sleeping haha. You are indeed correct.
In that case RTBC+++!
- π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
No worries, happens to the best of us.
- Status changed to Needs work
over 1 year ago 4:31am 17 April 2023 - π³πΏNew Zealand quietone New Zealand
Setting this back to NW to remove 'Please'. I have offered a suggestion for the changed text.
- π³πΏNew Zealand quietone New Zealand
I updated the CR as well, someone should check my work.
- last update
over 1 year ago 29,204 pass - Status changed to Needs review
over 1 year ago 8:57am 17 April 2023 - π§πͺBelgium kristiaanvandeneynde Antwerp, Belgium
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.
- Status changed to Needs work
over 1 year ago 9:32am 17 April 2023 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.
- Status changed to Needs review
over 1 year ago 9:49am 17 April 2023 - last update
over 1 year ago 29,283 pass - last update
over 1 year ago 29,300 pass - last update
over 1 year ago 29,302 pass - last update
over 1 year ago 29,300 pass - last update
over 1 year ago 29,361 pass - last update
over 1 year ago 29,366 pass - last update
over 1 year ago 29,367 pass - last update
over 1 year ago 29,374 pass - last update
over 1 year ago 29,378 pass - last update
over 1 year ago 29,379 pass - last update
over 1 year ago 29,380 pass - last update
over 1 year ago 29,383 pass - last update
over 1 year ago 29,388 pass - last update
over 1 year ago 29,388 pass - last update
over 1 year ago 29,388 pass - last update
over 1 year ago 29,388 pass - last update
over 1 year ago 29,388 pass - last update
over 1 year ago 29,395 pass - last update
over 1 year ago 29,395 pass 13:41 7:52 Running- last update
over 1 year ago 29,399 pass - last update
over 1 year ago 29,400 pass - last update
over 1 year ago 29,409 pass - Open on Drupal.org βEnvironment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - last update
over 1 year ago 29,418 pass - last update
about 1 year ago 29,420 pass - last update
about 1 year ago 29,420 pass 28:41 25:29 Running- Status changed to RTBC
about 1 year ago 11:07am 14 June 2023 - Status changed to Fixed
about 1 year ago 11:08am 14 June 2023 - π¬π§United Kingdom catch
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!
- π«π·France andypost
CR published https://www.drupal.org/node/3348054 β - set branch to 11.x but release to 10.2.0
Automatically closed - issue fixed for 2 weeks with no activity.
- Status changed to Fixed
9 months ago 8:44pm 20 December 2023 - π¨π¦Canada Liam Morland Ontario, CA π¨π¦
This has caused π TypeError: PermissionChecker::hasPermission(): $permission must be of type string Needs review .