User forms broken for admin without 'administer users'

Drupal core currently tests for "admin" access by checking for permission 'administer users'.

The uploaded patch instead tests for "admin" access if the active user is not the same as the target account. As far as I can see this method is completely safe because it is based on Drupal core entity access. Drupal ensures that it is only possible to access the form targetting another user when holding the appropriate permissions.

Note that this means that the sub-admin will not see the admin interface when managing their own account. This seems to generate the correct behaviour, for example:

• the permissions 'select account cancellation method' and 'change own username' are checked
• the cancellation message specifies 'your account', reducing the chance of deleting your own account accidentally.

Security implications

Mostly it's the same either with or without the patch:

• The sub-admin cannot escalate to admin access by altering roles.
• The sub-admin can impersonate the target account by altering the password and logging in.

As written, the patch adds default access for the sub-admin to enable or disable the target account. This seems like the most useful and common default. However if the maintainers prefer I can alter the patch to turn this off by default for sub-admin, leaving the custom/contrib code to override with a hook as required.

Back-compatibility

• My goal for this patch was to fix the problem with minimum impact to existing users.
• No change to Drupal core behaviour for admin or normal user.
• Existing custom/contrib code that uses the sub-admin concept will work better in 99% of cases.
• Should document a minor change in case the custom/contrib code is already hooking to alter these forms to remove access to fields (such code might need to remove access to one or two more fields).

Next steps

Would appreciate review and feedback from maintainers please.

Fix bugs in original patch and update for new Drupal version

Now I'm confused. #7 reports that the tests pass, yet #8 reports that they failed. Let's try setting it back to "needs review" in case it was a temporary glitch.

@jhedstrom Thanks for taking time to look at this issue.

I will upload a new patch including the comment as you suggest (busy right now - will do in a couple of week's time).

presumably there is an access check elsewhere that would limit this to actually only be admins

Correct. Although the term "admins" is potentially confusing, and that confusion is partly why this issue exists. I would prefer to say that a user can only edit another user if they have permission to do so. This is enforced by the Drupal 8 Entity system.

Thanks @agoradesign. If you could set RTBC that would be helpful.

@jhedstrom I have added the comment you suggested

@agoradesign thanks for fixing the comment alignment, which I have reviewed and is fine. You stated that you have reviewed and tested the main fix, so it seems like we are RTBC.

@xjm thanks for taking time to look at this issue.

I acknowledge the need for tests, but I don't currently have the knowledge to write them. There is a related issue #2773645: Allow hook_entity_field_access() to grant field-level access to User fields: 'forbidden' -> 'neutral' where writing similar tests is in progress.

#18 points 3 and 4 please could you explain what dangers you see? Perhaps the problem is that I need to add comments similar to the one I added in #14?

Drupal Core enforces that users cannot edit or create other users' accounts without the required permissions. I have researched the relevant lines of code below. However (unless you believe there is a Drupal Core security bug) I didn't actually need to find the lines - I trust core to be secure.

3) UserAccessControlHandler grants access only if the two IDs are equal

      case 'update':
        // Users can always edit their own account.
        return AccessResult::allowedIf($account->id() == $entity->id())->cachePerUser();

4) The route /user/register has requirements _access_user_register (user.routing.yml) which maps to RegisterAccessCheck::access that tests isAnonymous.

  public function access(AccountInterface $account) {
    $user_settings = \Drupal::config('user.settings');
    return AccessResult::allowedIf($account->isAnonymous() && $user_settings->get('register') != USER_REGISTER_ADMINISTRATORS_ONLY)->cacheUntilConfigurationChanges($user_settings);
  }

The route /admin/people/create requires permission 'administer users' although a contrib module can modify that.

#19 Might it help to add comments and reconsider the variable name? In the new code, $admin means "a semi-privileged user with permissions to administer another user's account" - it doesn't mean "full admin with permission to do anything". Perhaps we could even create a function isAdminOtherAccount($currentUser, $targetUser) which would give a single place to write a clear comment.

I have little experience of Drupal core dev and would welcome help from anyone else to improve the patch.

OK, I have changed the variable name and improved a few comments as it sounds like there is generally support for that.

I didn't yet create a function isManageOtherUser or do any other refactoring pending the discussion coming to a conclusion.

Fixed whitespace

Slightly tweaked version

• Backed out changing $form['administer_users'] to $form['manage_other_user'] because it could break existing config/custom hooks
• Determine access to cancel by checking actual permissions rather than hard-coding default core permissions.
• Improved comments

@agoradesign if you are happy, please can you set RTBC again?

I experimented with changing AccountForm to call EntityAccessControlHandlerInterface::fieldAccess to determine access to fields based on actual permissions rather than hard-coding default core permissions.

    $access_controller = $this->entityManager->getAccessControlHandler('user');
    $fields = $this->entityManager->getFieldDefinitions('user', NULL);
    $form['account']['status']['#access'] = $access_controller->fieldAccess('edit', $fields['status'], NULL, $register ? NULL : $account->get('status')),

I would welcome any thoughts on whether that is a good idea.

Patch with some tests but no fix - will fail.

Same patch as #29, but with the tests, should pass

Coding standards fix of #31

@xjm Thanks again for your time.

Simply saying that (e.g.) the email field is not required only if the account is not your own hardcodes an assumption about what contrib modules might do.

Yes that is a very good point, and I have been thinking about it too (#29). Actually I think the patch didn't change #access for email, but it did for name, status, password. And in fact I think you're right that email probably shouldn't be accessible by default.

If we look at UserAccessControlHandler, it defines the default access for individual fields. This code seems good (subject to fixing #2773645: Allow hook_entity_field_access() to grant field-level access to User fields: 'forbidden' -> 'neutral'). I propose that AccountForm should match and so my current idea is to call

$form['account']['mail'] = [
...
  [#access ] = $account->mail->access('edit', $user, TRUE),
]

Repeat that for every field that is not fully accessible by default. FYI This is inspired by #2846365: [regression] User roles field access is inconsistent, users with 'administer users' permission can gain full access which I guess you will be familiar as you have commented there several times. I really like that way that this does away with complex code carefully duplicating the logic in UserAccessControlHandler.

  [#access ] = ($register || ($user->id() == $account->id() && $user->hasPermission('change own username')) || $admin)

Why not have the Administer Users by Role module just override the whole user form?

As a contrib module developer, I don't favour that idea. I feel it's easy to get wrong in a way that exposes security bugs and it would work badly in case of more than one module that alters access (only one of them will get its form to run). It seems to partially defeat the point of having an Entity Access system, and takes us back to D7 ways.

My preference is to persevere a little longer with the approach in the current patch. The fact that fields such as email are exposed by default indicates to me that this form is in need of some revision.

I'd be in favor of a refactor of the user form that made it easier for a different module to subclass it to override some aspects

If my preferred approach doesn't work out, this would still be valuable. It would make it easier to implement hook_form_FORM_ID_alter. (As mentioned above I currently don't subclass because that seems to require taking over the route entirely and blocks compatibility with other related modules.)

By the way, I would be very grateful if you would look at the wider issues in #2921112: [META] User access control. Do you think it would be useful to work on them all in a single patch? The more I work on this area as part of developing "Administer Users by Role", the more difficulties I find!

Here is a slightly updated patch. The code calls $account->XXX->access for 'name' and 'status'. I now believe the patch is good from a security point of view in relation to "sub-admin" users.

UserAccessControlHandler in D8.4 (unchanged in patch)

• Status and roles are protected.
• Name is protected except for own user with 'change own username' permission.
• For mail and pass "Anyone that can edit the user can also edit this field."*

* The last one is perhaps surprising but it seems to be clearly documented and self-consistent throughout the code.

AccountForm in D8.4

Same as UserAccessControlHandler except name is not protected for sub-admin.

After #33 patch

You were right to raise a concern. The status field is not protected and it should be. (Also name is not protected as in D8.4.)

After #38 patch

Same as UserAccessControlHandler.

Sorry I now see that in #36 you were talking about whether 'mail' is '#required' (I had imagined it as '#access').

I am happy to drop any change to ['mail']['#required']. I guess that would mean that sub-admins were unable to edit or cancel an admin-created user without an email (not a scenario I use personally but I guess some people must). Whichever way we decide, note that there is matching code in \Drupal\user\Plugin\Validation\Constraint\UserMailRequired and I was unable to find an easy way to hook and override that.

In terms of other references to $manage_other_user:

• $form['language']['#access'] - Should replace with a call to $account->XXX->access
• I think all the others are evaluating "new account created by an admin"

So hopefully I am getting close to something you could be happy with - step-by-step I am eliminating anywhere that has changed the existing behaviour.

Do you think it would be useful if we get together on a chat forum to talk through some issues?

New patch

• Take out the change to ['mail']['#required'].
• Fix broken tests from previous patch.
• Improve variable names, hopefully it's clearer now....
• Renamed the confusing $register to more accurate $new_user, with two sub-cases $self_regsiter, $admin_create
• Mostly removed $manage_other_user and hard-coded permission checks.

@xjm many thanks for your time and asking excellent questions to help me improve the patch.

@alexpott Many thanks for your time.

No action take yet on preamble: "canAdminAccount". I understand the idea but have questions/uncertainties:

• You suggest a trait. Please could you help by suggesting the name and namespace?
• We should be aware that with $user->hasPermission('administer users') currently get $admin even on their own account. I guess we need to be careful about changing that. However as the patch improves with more calls to ->access, I'm not sure if there are any places left where it will be a problem. I'll check again after finishing off these changes.

1) No action take yet as in #41 I removed $manage_other_user
2) +1
3) +1
4) "Still should have the > 1 check as far as I can see. "
I had expected that $account->access('delete', $user) would fail for user 1. It doesn't, but I think it should.

For sure I can leave the account ID 1 test in. Alternatively it might be cleaner to fix UserAccessControlHandler as other code might be relying on it. What do you think?

5/6/7) +1

canAdminAccount

On investigation, "canAdminAccount" is tricky. Such a method tries to make a global yes/no admin decision. I can see why you are attracted to the idea, but I'm not sure reality is that simple. Arguably part of the cause of the issues I have been raising is the temptation for Core developers to rely on a single access flag, whereas one purpose of the entity access and permissions system is to break apart the concept of 'admin' into separately controllable flags:

• EntityAccess which includes "create"/ "edit"/ "delete" - NB it's not going to work if canAdminAccount always passes "edit".
• FieldAccess per field per operation.
• "Can select account cancellation method" - has a permission for own account, but not one for other.
• "UserMailRequired" - access not currently configurable.
• etc

Most of the questions have now been handled because we have converted to calling access() in various places. I think I need two more specific access decisions to complete this patch:

1) Code in RegisterForm/AccountForm to distinguish self-register versus admin-create. How about $account->access('create')?

2) Code in UserCancelForm to determine access to select account cancellation method. The cancel method 'description' fields all say "Your account..." which means they aren't really usable unless it is the same account. Therefore it seems that $this->entity->id() != $user->id() is the way to go.

What I can do is keep your Trait idea. I can create wrapper methods to package the logic: UserAccessTrait::isAdminCreate(), UserAccessTrait::canSelectCancellationMethod.

What do you think?

Combining issues

This issue overlaps with several others in the parent META issue. I am finding they overlap, especially in tests, where I would like to make a thorough SubAdminTest that covers all the aspects (and I currently have code in this patch to workaround one of the other issues). How would you feel about having a combined patch for several issues?

I'm very grateful for your continued help so that I can try and get the patch right. OK, here's another attempt.

I have applied comments from #42 with the following notes. The patch is much simpler now, thanks @alexpott.

• Not done canAdminAccount or a Trait for reasons explained in #44. However I am explicitly checking access with clear comments.
• 1) I have gone back to the original variable names as best I can.
• 4) I have altered UserAccessControlHandler to put the root account check there.

I have now included patches for other closely related issues. Sorry if this is confusing, but the code wasn't working otherwise.

• #2773645: Allow hook_entity_field_access() to grant field-level access to User fields: 'forbidden' -> 'neutral' else forms are broken for sub-admin due to missing field access (when using a module such as Administer Users by Role)
• #2921365: Wrong requirements for route user.admin_create else automatic tests are broken with no access to user creation

@alexpott thanks for your diligence

Did you perhaps run your drush php commands without applying the patch? On my server once I have applied the patch it does work:

adam@svr01:TAGA:~$ d php --user=1
Psy Shell v0.8.0 (PHP 7.0.22-0ubuntu0.16.04.1 — cli) by Justin Hileman
>>> \Drupal::currentUser()->id();
=> "1"
>>> \Drupal::currentUser()->getAccount()->access('delete');
=> false
>>>

Because I added this:

--- a/core/modules/user/src/UserAccessControlHandler.php
+++ b/core/modules/user/src/UserAccessControlHandler.php
@@ -42,6 +42,11 @@ protected function checkAccess(EntityInterface $entity, $operation, AccountInter
       return AccessResult::forbidden();
     }
 
+    // The root user cannot be cancelled.
+    if (($entity->id() == 1) && ($operation == 'delete')) {
+      return AccessResult::forbidden();
+    }
+

That code seems like an improvement as it gives an extra layer of protection for any means to delete root user, not just via ProfIleForm. I also confirmed this in the GUI. However if you are still concerned of course I will change it.

Thanks @alexpott

#48 +1 OK, I understand
#49
2) +1 OK
4) +1 Ah that's from #2921114: Username formatter ignores entity access. Sorry, you are right, it's not a dependency of this work and I will take it out of the patch.

The other two points I would like to discuss further please. They are similar to each other: I don't really like hard-coding a specific behavior, but in each case I can't see a satisfactory way to check access. Do you have any ideas? Is it at all feasible to introduce new values of "$operation" specific to the User entity, e.g. $user->access('choose_cancel_method')? That would allow Core to define a base behaviour that would be very easy to override in hooks.

1) = "Access to set empty email field to empty": on create; to preserve an empty email field on edit/delete. D8.4 checks 'administer users', and the latest patch doesn't change that. @xjm pointed out that any change might be unwanted by other custom modules or existing sites. It's pretty painful to write hooks to overrride the Core defaults: need to hook_form_alter and hook_validation_constraint_alter for UserMailRequired (creating two new classes).

Or perhaps you are asking why I removed the variable $admin? I did that because I feel the key to maintaining the user module in future so that sub-admin access works is to get away from the idea of an all-or-nothing admin. Having a variable called $admin just tempts coders to use it, rather than work out the correct specific access check.

3) = "Access to choose cancellation method". D8.4 checks 'administer users'. I changed that to ($this->entity->id() != $user->id()). That decision was based on a complication: if a user cannot choose cancellation method, then the description is "Your account..." which doesn't work for a sub-admin. Certainly it would be better to allow a sub-admin who can't choose cancel method.
Is it feasible to change these descriptions in a Drupal minor release? I guess it could affect hook_user_cancel_methods_alter, translations, etc.

Thanks. In fact I think I have just found a neat solution without changing strings:

If "not allowed to select cancel method" and "not cancelling own account" use the $method['title'] instead of $method['description'].

That still leaves the question of how to decide if a user is allowed to select cancel method. Am I allowed to do make up a new operation:$user->access('select_cancel_method')?

Next version ready for review please, covers #48 and #49.

Cancel

• Now works for sub-admin both with and without access to select cancellation method.
• Currently I am re-using the existing permission 'select account cancellation method' to apply to sub-admin cancelling other user. I altered the English title slightly. This approach has the advantage of being simple, but I don't know if you will feel this is a misuse - please let me know your view.

Multiple cancel

• Now works for sub-admin both with and without access to select cancellation method.
• Please check carefully: I removed the permission check off the route user.multiple_cancel_confirm. I read the code and tested as carefully as I can but I don't fully understand the potential exploits. I can confirm that if an anonymous user visits the page, the behaviour is a 403 same as now (no temp store so the form redirects to entity.user.collection which is not accessible). Furthermore if the user runs batch including items without permission there is a error message "No access to execute..."

Mail field required

• Created function user_mail_required to avoid duplication.
• Default behavior is same as D8.4: required except if 'administer users' permission
• Added a hook to allow modules to override.

I will add some more tests including hook_user_mail_required_alter and multiple cancel, but thought I should wait to see if the code is OK first.

Fix coding standards and broken test

@agoradesign Ah good question.

For anyone trying to get a working site (accepting risks of alpha releases and patching core) then it makes sense to stick with 2.0-alpha4 plus #33.

However I'm really hoping that we can get these core patches accepted soon and get the module into a more usable/stable state, and that requires someone to help out with using the latest versions and reporting problems or setting RTBC - if you can great. That would mean module version 8.x-2.x-dev plus the latest patch here, currently #55. I just noticed I had some changes locally not pushed, so there is now a new dev. Let me know any problems that you see.

Yes #55 includes the changes from #2773645: Allow hook_entity_field_access() to grant field-level access to User fields: 'forbidden' -> 'neutral' (this is because there is a dependency to make the tests work on this issue) so drop the other one from your config.

Actually better if you can give me a few days and I'll tidy up the patch and make some new releases of AUBR.

Updated patch

• Remove redundant parameters on calls to ->access.
• Minimise the patch by undoing non-essential edits.
• Backout user_mail_required - I wasn't really happy with the code it's not clear it's part of this issue.
• Backout user.multiple_cancel_confirm - potentially risky, and it's not clear it's part of this issue.
• Update to latest version of the code that has been borrowed from #2773645: Allow hook_entity_field_access() to grant field-level access to User fields: 'forbidden' -> 'neutral' but delete test code

Matching AUBR module version coming soon.

Newer patch for #2773645 fails tests so going back to previous code

AUBR 8.x-2.0-alpha5 ready for testing out user.form_.subadmin-2854252-63.patch.

@agoradesign It would be great if you can help perhaps on a test site. I understand you want to keep your live site "stable", but on the other hand that alpha4 isn't go to be work forever. I really hope we can get a commit in 8.6.x.

@Berdir You have vastly more Drupal experience than me, but I guess I can call on a long career as a security expert in other fields. Personally speaking I would strongly advise anyone against giving the administer users permission to sub-admins. Sorry, long post, but I think it can help people have safer sites so is important.

Reduce Access strategy

As per #65, this approach is to give sub-admins administer users then try to reduce permissions. AUBR 1.x did that and it was full of security bugs; I took over as maintainer and rewrote as v2. I concede that you could build a secure site if you are a Drupal security expert with detailed knowledge of every module that you add to the site. However it seem to be a "fail-unsafe" approach. You have to identify all the possible ways that a user with "administer users" can escalate access and then shut each one down. If you miss just one, the site is vulnerable.

There is another widely-deployed user/role management module based on "reduce access". I raised a security issue, the vulnerabilities were confirmed, but the issue itself was eventually rejected based on policy (see "What About Vulnerabilities Which Require Advanced Permissions?") and I was given permission to raise the issue in public (which I chose not to do). So in a sense, with this approach the site is no longer protected by Drupal security advisories.

Grant access strategy

On the other hand the AUBR approach to grant access selectively is a "fail-safe" approach. I mean in the sense that if you miss one way that you should have granted access it's not a security bug, it's just a minor nuisance.

Contrib modules

there are still going to be cases where contrib modules check for that permission to show information on the user edit or view page to only show it to admins

Exactly my point! What if the contrib module adds a new security-critical field to user edit? UserAccessControlHandler::checkFieldAccess will by design grant access to any user with administer users. In the "reduce access" approach the sub-admin has access to the new field and can escalate their own account to full admin.

I expect the example you had in mind is that a contrib module creates a new "semi-secure" field. The sub-admin could in fact safely access it, but with "grant access" by default they won't be able to.

In summary, either strategy might need to write a hook for the new field. However the difference is that in the "reduce access" strategy if you fail to notice you have a critical security issue.

Entity API

Here's another way to look at it: the grant access approach is wholly aligned with the D8 Core entity access API - it uses access hooks to grant access. So interaction with any other contrib module is safe by design provided that the other module also uses the access API correctly. On the other hand with the reduce access strategy, once a user has "administer users" it effectively acts as a bypass of the entity access API for users because of @ContentEntityType[user][admin_permission] = "administer users".

Summary

In conclusion, I claim that this issue (and related ones under the parent META issue) are important because they are fixing the D8 API-compliant/safe/recommended way of selectively granting access to users. Currently modules/sites are reacting to these issues by adopting other strategies that are riskier.

There is for example also the check in the \Drupal\user\Plugin\EntityReferenceSelection\UserSelection that only allows users with that permission to reference blocked users.

Thanks for pointing that out - yes it looks like that would need fixing too. D8 introduced an amazing entity access API, but lots of code for the user entity ignores it in favour of hard-coded permissions! So far no one has raised it so perhaps it's not that important to people. I don't plan to tackle it right now and I don't really understand the implications enough to raise an issue properly. However I've added it to my personal list of things to be aware of.

It seems that for example

    // In order to create a referenceable user, it needs to be active.
    if (!$this->currentUser->hasPermission('administer users')) {
      /** @var \Drupal\user\UserInterface $user */
      $user->activate();
    }

could become something like this

    // In order to create a referenceable user, it needs to be viewable.
    if (!$user->access('view')) {
      /** @var \Drupal\user\UserInterface $user */
      $user->activate();
    }

Some of the others are probably harder though.

Impact on the two strategies

I would say that code like this relates to "reduce access" versus "grant access" on a case by cases basis

• Maybe the sub-admin is entitled to access the particular operation in question: "grant access" has some missing function
• Or maybe the sub-admin must not have it: "reduce access" has a critical security bug

The "amazing entity access API" has holes, unfortunately :)

Yes I noticed that:-) I will send you a PM with a specific hole that I think affects security of the site you are referring to.

One way of analysing that is to find all the places where there is a hard-coded permission of administer users. I guess what I am trying to say is:

• Correct = sub-admin has access for some of these but not others
• Reduce access => sub-admin has access to all of them except where altered by hooks
• Grant access => sub-admin has access to none of them except for hooks
• Personally I'd rather start by granting too little access than too much - especially in a contrib module that runs on 1000s of sites with different collections of other modules

Specifically for buildEntityQuery my understanding is:

• Correct = sub-admin can make a reference to users that they can view.
• Reduce access => sub-admin can reference no blocked users
• Grant access => sub-admin can reference all blocked users. In certain scenarios this could potentially be a security bug - perhaps if the entity reference widget was a view of teaser which included data about the blocked user that should not be available to the sub-admin.
• Sadly the correct behavior is presumably impossible to implement as an SQL query in general.
• However I suspect that AUBR could implement it in its specific case using a query on user roles.

What we are trying to do with this issue and similar ones is to reduce some of the holes in the "amazing entity access API" which I think benefits everyone whichever strategy they pick.

And equally I should be clear that I agree the approach you chose is fine for anyone who has enough time and knowledge to do it right. For example you mentioned the need for "masquerade with some patches to properly limit them to certain roles" and anyone who doesn't understand that should be warned they are likely out of their depth.

@Rob230 Thanks for trying out the module and patch.

8.3 is old, 8.4 is current, 8.5 is nearly ready.

If you want to try out new modules that rely on core patches then I think it's essential to be on as new a Core version as possible.

New patch with a minor enhancement.

Allow modules to customise whether the user mail field is required. By default in core, this is controlled by 'administer users' permission. This patch allows that to be customised via a hook. The module "Administer Users by Role" version 8.x-2.0-alpha6 implements that hook by means of a new permission 'allow empty user mail'.

Please can someone do a quick review and set back to RTBC?

@borisson_ thanks for trying this patch.

the problem I'm having is that the status and roles fields are not visible. If that is not the problem that this patch is trying to solve.

This patch mostly tries to take whatever access has been set on a site and make that access work correctly.

The patch does not force status and roles fields to be visible because that is not necessarily what is wanted on all sites. In particular, the roles field is not generally safe to expose because a sub-admin or group-admin could edit their own account to gain an admin role.

To alter field access a module should implement hook_entity_field_access - for example see administerusersbyrole_entity_field_access.

I think that means that we should improve the documentation about the difference between admin creation and self registration and probably only mention it once (and use an @see to link to the other documentation).

I am open to trying to help with that on a small scale. However this is an issue with a primary goal to fix a bug so I don't necessarily think it makes sense to get sucked into a major documentation project.

Can you make a specific proposal please? Ideally that would be in the form of a patch.

How about this:

    // For a new account, there are 2 sub-cases:
    // $self_register: A user creates their own, new, account
    //   (route = 'user.register').
    // $admin_create: An administrator creates a new account for another user
    //   (route = 'user.admin_create').
    // If the current user is logged in and has permission to create users 
    // then it must be the second case.

it's easy enough to display the form on another route.

True but almost anything in Drupal can be customised so I would think many of the comments in Core might no longer apply with enough hooks. I'm trying to give an extra bit of information to help people understand what the two cases are. Now I think about it, I feel it's probably clearer for most people to put paths rather than routes - how about this?

    // For a new account, there are 2 sub-cases:
    // $self_register: A user creates their own, new, account
    //   (path '/user/register')
    // $admin_create: An administrator creates a new account for another user
    //   (path '/admin/people/create')

OK, here is a patch as per #84. Only comment changes compared with #78. I would be very grateful for a RTBC please.

Reroll patch now that #2773645: Allow hook_entity_field_access() to grant field-level access to User fields: 'forbidden' -> 'neutral' has been fixed.

Sorry merge error try again

OK please can I try and revive this issue which seems pretty to close to ready but has stalled and now missed 8.6.

Removing tags "Needs subsystem maintainer review, Needs framework manager review" as AlexPott has reviewed most of the patch. The main parts he didn't review are

1. UserCancelForm.php how to control permission for sub-admin to select cancellation method using existing permission 'select account cancellation method'
2. New user_mail_required function and hook_user_mail_required_alter

Setting back to RTBC. Many reviewers have reviewed different levels of the patch, except item (2) in the above list - which I could remove from the patch if necessary.

It would be great to get some feedback from the core committers please, many thanks.

OK, I have learnt from other core issues I am working on that it is required to keep each issue as small as possible and not to merge separate changes together.

Here is a new patch without user_mail_required so same as #63 plus the reroll to get the tests working and the improved comment from @borisson_. Apologies for going round in circles; I will raise a separate issue for user_mail_required once this one has been committed.

Setting to RTBC because #63 was reviewed and this is the same apart from reroll and comment.

AlexPott has reviewed most of the patch apart from UserCancelForm.php how to control permission for sub-admin to select cancellation method using existing permission 'select account cancellation method'.

@alexpott thanks again for your time and expertise.

1. I agree with your analysis including the 'BUT'. For sure a security expert review would be an advantage. If you have someone in mind then perhaps you could assign the issue? @Berdir has commented on the issue, but made it clear he didn't do a full review.
2. I created a change record as best as I can.
3. I raised #2992826: Confusing call to isAnonymous in UserAccessControlHandler.

Set to needs review for the CR and perhaps a security review as per 1).

I raised a separate issue #2992848: Allow customisation of permission to create user with blank email for the hook_user_mail_required_alter part that was present in some of the earlier patches.

@joachim thanks for your time

What do we need a CR for

I guess that is primarily a question for alexpott. Not sure if you read what I put in the CR - perhaps you could and then say whether you think each part of it is necessary or not?

UserAccessControlHandler::checkFieldAccess() seems to not allow admins to change the username.

I think you missed the code at the top underneath the comment Administrative users are allowed to edit and view all fields.

For clarity setting back to RTBC, because that's the status of the patch, and this issue is ready for the attention of a core committer please.

The CR still needs a review please.

Thanks @alexpott and @Berdir

1. In #36 @xjm raised concerns about altering the behavior of ['email']['#required'] as it would not be BC for existing sites, so I split off a separate issue #2992848: Allow customisation of permission to create user with blank email.

2. Done

3. I suggested the same for the uid>1 check, but see @alexpott response in #47,#48.

4. So I guess we leave it given that it already exists.

5. On balance I think combining the if is more clear, so I did it. It's still a shorter line than the function definition. I also fixed the coding standards error on the comment too long.

Excellent, many thanks @alexpott and everyone else that helped.

• A very similar issue #2921114: Username formatter ignores entity access is also RTBC and hopefully ready for commit.
• Follow-on issue #2992848: Allow customisation of permission to create user with blank email is no longer postponed, and is now "Needs Review".
• This commit includes the fix for #2921365: Wrong requirements for route user.admin_create so I have marked that as fixed too.