Add a new function to Drupal Core: user_is_admin

I think this still makes sense at least after UID1 can be disabled as super user. Many projects used (wrongly) this test if ($account->id() == 1) {...} but we shill need a shorthand to get if a use is admin.

Instead of...

$isAdmin = FALSE;
foreach ($account->get('roles')->referencedEntities() as $role) {
   if ($role->isAdmin()) {
     $isAdmin = TRUE;
     break;
   }
}

...let's add this shorthand:

$isAdmin = $account->isAdmin()

Adding release note snippet

Added change record https://www.drupal.org/node/3484833 →

Ready

Typo

Tagging

Tagging

Thank you for having everything ready from the start.

Read the CR and clear of the change, before/after examples are useful too.

Looking at the code
Everything is typehinted, docs read fine, has test coverage.

Summary is complete

Believe this is good.

if ($account->id() == 1) {...} was horrible, and thankfully will now slowly start going away.

However just because we can add isAdmin() does that mean that we should?

If a user is an admin they will already have all the permissions the module grants. Would it not be better for modules to check in context of module permissions compared to an over encompassing special exit for isAdmin()? I believe this was somewhat implied with #4.

I am of the mindset that if your falling back to "check if it is the god role" compared to "is the user authorized to do this" there is likely something wrong with the access control implementation (either in design, assignment, or validation).

@cmlara, I do agree with “check permission “ over “check roles”. However, there are situations when you need to find out if the user is “god”. I came to this by trying to write an access policy → class and, while I wanted to pass the 2nd argument to CalculatedPermissionsItem constructor (which is “is admin”), I realized that the user account is missing this simple method.

+1 on #17, we should not reinvent the super admin definitionan. Maybe your findings in #18 means that the is admin parameter can be marked as deprecated and removed in D12.

#18 makes me realize, isn't the access policy API a reason to NOT implement this? If I understand correct if this was committed we would have a system (on the User Entity) that is outside the access policy API ability to control permissions.

What currently makes a role that has all permissions (an admin role) special is the UserRolesAccessPolicy.

while I wanted to pass the 2nd argument to CalculatedPermissionsItem constructor (which is “is admin”), I realized that the user account is missing this simple method.

I am not an expert at the new PolicyAPI system that has been deployed, however the constructor documents states bool $isAdmin: (optional) Whether the item grants admin privileges. and the linked documentation page says You can add an array of permissions names or, alternatively, flag that the account should have admin access by setting the second parameter to TRUE. (See SuperUserAccessPolicy in core).

To me this reads as if you are defining the permission item as an admin permission explicitly. I would expect that normally you would leave this as FALSE, and only set it it if you yourself are declaring a user an admin based on your own unique factors similar to SuperUserAccessPolicy. Usually the isAdmin in this case would have been set by UserRolesAccessPolicy.

RefinableCalculatedPermissions::mergeItems() would appear to back up this assertion that it merges the admin tags from many sources into one and if any asserts the user is an admin the permissions hold true. You would not need to 'mint' them to an admin, unless you revoked all the existing permissions.

Yes previous policies can be completely overridden, however I'm having trouble imagining a scenario where a site would want to override UserRolesAccessPolicy's isAdmin determinations just to re-implement the same check?

Yeah groups for example has a system where global admin users don't necessarily have all group admin permissions (although it's weird if they don't so it's unlikely a site would use it like that on purpose, but it caught me out with one groups update once), so an isAdmin() check as opposed to a permissions check wouldn't work there. Moving back to needs review and tagging for subsystem maintainer review since @kristiaanvaneyde should have a look at this.

makes me realize, isn't the access policy API a reason to NOT implement this?

Correct.

Furthermore, the whole point that we have the old UID1 logic in a separate access policy that can be turned off, is that this behavior is undesired in a secure system. The goal is to get rid of UID1 altogether and come up with a better alternative. Introducing an isAdmin() flag on users would be taking a step back in the wrong direction.

To me this reads as if you are defining the permission item as an admin permission explicitly. I would expect that normally you would leave this as FALSE, and only set it it if you yourself are declaring a user an admin based on your own unique factors similar to SuperUserAccessPolicy. Usually the isAdmin in this case would have been set by UserRolesAccessPolicy.

Again, correct.

If there are any further questions, Ill be glad to answer them. As it stands -now that we have the Access Policy API- and with the goal of removing the UID1 policy, I don't see any future for this issue. So for me, this can return to Closed (Won't fix), but I'll leave it open for a little while for the discussion to take place.

Okay no more discussion, so closing as won't fix :)

Removing tags that might make it appear on people's personal trackers.