This issue only occurs with modules hooks - it has no impact in pure Drupal core.

In Drupal core, there is a single permission 'administer users' that controls whether a user can 'administer' the user account of another user. However the Drupal 8 Entity API makes it possible to refine the access, for example by means of hook_entity_access. This allows modules such as Administer Users by Role to create a "sub-admin" - a user without 'administer users' permissions that nevertheless has access to update some other users' account.

Unfortunately RegisterForm, ProfileForm and UserCancelForm are broken when used by the sub-admin. The problems arise from the fact that each form can be used in two ways: a normal user can update their own account or an admin can administer another user.

Expected behaviour

Tthe sub-admin sees the same interface as the admin, except missing fields such as role that require additionaly access.

Actual behaviour

The sub-admin sees a confusing mixture of the user and admin interfaces, for example:

  • After creating a user the sub-admin is logged in as that user.
  • When cancelling an account the sub-admin see "Your account will be blocked and ..." but it's actually another users account.
  • Several fields are missing due to lack of access.

Steps to reproduce

  • Install Administer Users by Role release 8.x-2.0-alpha3.
  • Create a role with permissions 'Create new users', 'Edit users with no custom roles', 'Cancel users with no custom roles', 'Access the users overview page', 'View user information'.
  • Create user with this role, and log in as that user.
  • Create a user.

Or if you prefer, can reproduce by writing custom code.

Workaround

It is possible to fix all the problems using the extensive hooks provided by the user module. However it took 95 lines of code in Administer Users by Role. What's more, this code has detailed dependencies on the internal implementation in the user module, so may need to be updated for future Drupal versions.

You can test the workaround if you also install the sub-module administerusersbyrole_hack. You must disable this sub-module to see the bug.

Solution

Fortunately it seems possible to fix the forms with some fairly simple changes to the user module, without any negative impact on mainline use cases. This seems valuable and worthwhile because any contrib or custom code hooking into User access is likely to see the same problems.

Patch coming up.

CommentFileSizeAuthor
#63 user.form_.subadmin-2854252-interdiff-61-63.txt1.57 KBAdamPS
#63 user.form_.subadmin-2854252-interdiff-55-63.txt19.4 KBAdamPS
#63 user.form_.subadmin-2854252-63.patch19.5 KBAdamPS
#61 user.form_.subadmin-2854252-interdiff-55-61.txt20.97 KBAdamPS
#61 user.form_.subadmin-2854252-61.patch19.59 KBAdamPS
#55 user.form_.subadmin-2854252-55.patch36.83 KBAdamPS
#53 interdiff-2854252-45-53.txt13.34 KBAdamPS
#53 user.form_.subadmin-2854252-53.patch36.46 KBAdamPS
#45 user.form_.subadmin-2854252-45.patch35.07 KBAdamPS
#41 interdiff-2854252-38-41.txt13.16 KBAdamPS
#41 user.form_.subadmin-2854252-41.patch18.56 KBAdamPS
#38 interdiff-2854252-33-38.txt994 bytesAdamPS
#38 user.form_.subadmin-2854252-38.patch18.92 KBAdamPS
#33 user.form_.subadmin-2854252-33.patch18.95 KBAdamPS
#31 user.form_.subadmin-2854252-31.patch18.95 KBAdamPS
#30 user.form_.subadmin-only.tests-2854252-30.patch4.92 KBAdamPS
#29 interdiff-2854252-27-29.txt3.28 KBAdamPS
#29 user.form_.subadmin-2854252-29.patch14.03 KBAdamPS
#27 user.form_.subadmin-2854252-27.patch12.99 KBAdamPS
#25 user.form_.subadmin-2854252-25.patch12.99 KBAdamPS
#22 user.form_.subadmin-2854252-22.patch12.99 KBAdamPS
#15 user.form_.subadmin-2854252-15.patch6.17 KBagoradesign
#15 interdiff-2854252-14-15.txt1.36 KBagoradesign
#14 user.form_.subadmin-2854252-14.patch6.18 KBAdamPS
#13 user.form_.subadmin-2854252-13.patch5.19 KBagoradesign
#7 user.form_.subadmin-2854252-7.patch6.24 KBAdamPS
#2 user.form_.subadmin-2854252-2.patch5.2 KBAdamPS
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

AdamPS created an issue. See original summary.

AdamPS’s picture

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.

shawn_smiley’s picture

I tested this on Drupal 8.2.6 and have not experienced any problems.

AdamPS’s picture

AdamPS’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 2: user.form_.subadmin-2854252-2.patch, failed testing.

AdamPS’s picture

Status: Needs work » Needs review
FileSize
6.24 KB

Fix bugs in original patch and update for new Drupal version

Status: Needs review » Needs work

The last submitted patch, 7: user.form_.subadmin-2854252-7.patch, failed testing.

AdamPS’s picture

Status: Needs work » Needs review

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.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Status: Needs review » Needs work

This patch no longer applies.

+++ b/core/modules/user/src/AccountForm.php
@@ -69,7 +69,7 @@ public function form(array $form, FormStateInterface $form_state) {
+    $admin = $user->hasPermission('administer users') || ($user->id() != $account->id());

I think a code comment here would be prudent. It implies that any user editing a user that isn't theirs is an admin (which is probably true), but presumably there is an access check elsewhere that would limit this to actually only be admins?

AdamPS’s picture

@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.

agoradesign’s picture

Status: Needs work » Needs review
FileSize
5.19 KB

Re-rolled #7

AdamPS’s picture

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

@jhedstrom I have added the comment you suggested

agoradesign’s picture

thanks for adding the comment :)

Sorry for being nitpicky, but I saw two double spaces in your comment and removed then (plus optimized line lengths). So I'll leave it to "needs review" then, but for me it's RTBC

AdamPS’s picture

Status: Needs review » Reviewed & tested by the community

@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.

agoradesign’s picture

Yes, it still works for me :)

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for working on this!

  1. I think we need to have added test coverage for this change, to set up a scenario similar to the one needed by the contrib module.
  2. +++ b/core/modules/user/src/AccountForm.php
    @@ -69,7 +69,15 @@ public function form(array $form, FormStateInterface $form_state) {
    +    // This form is used for two cases: a user editing their own account or an
    +    // admin editing another user's account. Check for the admin case by
    +    // comparing the user IDs. If a user has 'administer users' permission then
    +    // they count as an admin even when editing their own account.
    +    // Note that the Entity framework checks access before calling this function
    +    // to ensure that a user can only admin another user if they have core or
    +    // contrib permissions to allow it.
    +    $admin = $user->hasPermission('administer users') || ($user->id() != $account->id());
    

    We need to add test coverage for the second scenario. (Presumably the first scenario already has test coverage.)

  3. +++ b/core/modules/user/src/Form/UserCancelForm.php
    @@ -75,7 +76,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -    $admin_access = $user->hasPermission('administer users');
    +    $admin_access = $user->hasPermission('administer users') || ($this->entity->id() != $user->id());
    

    This change looks wrong. I don't think $admin_access should be true just because the entity ID doesn't match the user ID. That seems dangerous.

  4. +++ b/core/modules/user/src/RegisterForm.php
    @@ -18,7 +18,7 @@ public function form(array $form, FormStateInterface $form_state) {
    -    $admin = $user->hasPermission('administer users');
    +    $admin = !$user->isAnonymous();
    

    This doesn't look right at all...

xjm’s picture

Rather than changing the form internally and (possibly dangerously, or at least confusingly) changing internal flags about whether the user is an admin, should we refactor the form so that the separate code paths are actually separate?

Also, agreed on requiring subsystem maintainer review for this, if possible. That's Moshe currently, although he hasn't really been very active as a core maintainer in a long while.

AdamPS’s picture

Issue tags: +Needs tests

@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.

agoradesign’s picture

I partially agree and disagree (with #19):

I fully agree that the wording is confusing. The "admin" inside the variable names just emphazises that core user management is unfortunately too limited - in terms of either allowing everything or nothing and not taking possible contrib extensions offering more granular permissions into account.

But, whatever the solution in the end looks like, the naming here is unfortunate and wrong (or let's say to limited in scope), as the permission check in these forms is not used for access control of the form itself, but only to certain fields like the existing password.

I thought about if it would make sense to introduce a service that you can ask whether the user "is admin" (== "is a foreign user"), allowing contrib modules to alter the result. However I'm unsure, if this would be satisfying...

Refactoring into separate parts sounds great, but how? Split up the forms into different ones? But as the route is the same, when editing an user by yourself and by another person ("admin"), this seems to be impossible?!

Would be great to hear Moshe's opinion

AdamPS’s picture

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.

AdamPS’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 22: user.form_.subadmin-2854252-22.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

AdamPS’s picture

Status: Needs work » Needs review
FileSize
12.99 KB

Fixed whitespace

Status: Needs review » Needs work

The last submitted patch, 25: user.form_.subadmin-2854252-25.patch, failed testing. View results

AdamPS’s picture

Status: Needs work » Needs review
FileSize
12.99 KB
agoradesign’s picture

way better! :)

AdamPS’s picture

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.

AdamPS’s picture

Patch with some tests but no fix - will fail.

AdamPS’s picture

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

The last submitted patch, 30: user.form_.subadmin-only.tests-2854252-30.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

AdamPS’s picture

Coding standards fix of #31

agoradesign’s picture

Status: Needs review » Reviewed & tested by the community

still works for me

AdamPS’s picture

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs framework manager review

Thanks for the cleanup and the tests; that helps demonstrate what this issue is trying to accomplish.

I still am really not comfortable with this approach. 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. I could create a different module that granted access to only certain fields on the user profile form, and this change would unexpectedly make the email field not required for them, because a different contrib module wants a different behavior.

Why not have the Administer Users by Role module just override the whole user form? That way the module could customize the administrative experience that's appropriate to its permissions and configurations, rather than introducing this change in core. 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, but changing the core form logic to support logic that doesn't exist in core isn't the best approach IMO.

We still need a subsystem review. Since the user maintainer isn't really active, I'm also tagging for framework manager review since we escalate to the framework managers in cases where a subsystem maintainer doesn't provide feedback.

Thanks!

AdamPS’s picture

@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: 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!

AdamPS’s picture

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.

AdamPS’s picture

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?

Status: Needs review » Needs work

The last submitted patch, 38: user.form_.subadmin-2854252-38.patch, failed testing. View results

AdamPS’s picture

Status: Needs work » Needs review
FileSize
18.56 KB
13.16 KB

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’s picture

Status: Needs review » Needs work

Looking at this patch from a framework manager perspective. I share @xjm's concerns about just checking that the IDs aren't equal. (Also just checking anonymity in the latest patch I have the same concerns - we should not just be relying on the access checking from the routing system here) However we could just check that the user has edit access to the account entity as well. If it does and the IDs are not equal that it has admin permissions. I'd be tempted to add a trait to forms to determine administrative access. Which has a method like:

protected function canAdminAccount($account, $user) {
  return $account->id() != $user->id() && $account->access('edit', $user, TRUE);
}

That way we can centralise this logic and provide a really good comment in a single location as to what is going on and why it is important. I do agree with what @AdamPS says about the additional security vulnerabilities of making contrib override the entire form. And the direction of the patch to use more entity access in these forms is a good one from the perspective of making them more re-usable and making the security concerns easier to review because all the access is being checked in the same place.

Some review points based on #38 - this was a xpost with #41

  1. +++ b/core/modules/user/src/AccountForm.php
    @@ -69,7 +69,15 @@ public function form(array $form, FormStateInterface $form_state) {
    +    $manage_other_user = $user->hasPermission('administer users') || ($user->id() != $account->id());
    

    I think that the variable name of $admin is better than $manage_other_user since if you have the 'administer users' permission you are not necessarily here managing another user. Where if the user IDs are not equal then you are administering a user.

    There would then way less change to the form too.

  2. +++ b/core/modules/user/src/AccountForm.php
    @@ -103,7 +111,7 @@ public function form(array $form, FormStateInterface $form_state) {
    -      '#access' => ($register || ($user->id() == $account->id() && $user->hasPermission('change own username')) || $admin),
    +      '#access' => ($register || $account->name->access('edit', $user, TRUE)),
    
    @@ -181,7 +189,7 @@ public function form(array $form, FormStateInterface $form_state) {
    -      '#access' => $admin,
    +      '#access' => $account->status->access('edit', $user, TRUE),
    

    +1 to changing these to field access checks.

  3. +++ b/core/modules/user/src/Form/UserCancelForm.php
    @@ -75,18 +76,17 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -    $admin_access = $user->hasPermission('administer users');
    +    $manage_other_user = $user->hasPermission('administer users') || ($this->entity->id() != $user->id());
    

    As above I wouldn't bother changing the variable name.

  4. +++ b/core/modules/user/src/ProfileForm.php
    @@ -25,7 +25,7 @@ protected function actions(array $form, FormStateInterface $form_state) {
    -    $element['delete']['#access'] = $account->id() > 1 && (($account->id() == $user->id() && $user->hasPermission('cancel account')) || $user->hasPermission('administer users'));
    +    $element['delete']['#access'] = $account->access('delete', $user);
    

    Still should have the > 1 check as far as I can see.

  5. +++ b/core/modules/user/src/RegisterForm.php
    @@ -18,28 +18,37 @@ public function form(array $form, FormStateInterface $form_state) {
    +    $manage_other_user = !$user->isAnonymous();
    

    Let's just call it $admin - as above the variable name change does not really add anything.

  6. +++ b/core/modules/user/src/RegisterForm.php
    @@ -62,9 +71,9 @@ protected function actions(array $form, FormStateInterface $form_state) {
    -    $admin = $form_state->getValue('administer_users');
    +    $manage_other_user = $form_state->getValue('administer_users');
    

    Unnecessary change.

  7. +++ b/core/modules/user/src/RegisterForm.php
    @@ -86,7 +95,7 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    -    $admin = $form_state->getValue('administer_users');
    +    $manage_other_user = $form_state->getValue('administer_users');
    

    As above - why change the name?

AdamPS’s picture

@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

AdamPS’s picture

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?

AdamPS’s picture

Status: Needs work » Needs review
FileSize
35.07 KB

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.

alexpott’s picture

One quick thought... More review to come later...

+++ b/core/modules/user/src/ProfileForm.php
@@ -20,12 +20,10 @@ protected function actions(array $form, FormStateInterface $form_state) {
-    $element['delete']['#access'] = $account->id() > 1 && (($account->id() == $user->id() && $user->hasPermission('cancel account')) || $user->hasPermission('administer users'));
+    $element['delete']['#access'] = $account->access('delete');

This still does not look correct. See the following:

$ drush php --user=1
Psy Shell v0.8.0 (PHP 7.1.0 — cli) by Justin Hileman
>>> \Drupal::currentUser()->id();
=> "1"
>>> \Drupal::currentUser()->getAccount()->access('delete');
=> true

The $account->id() > 1 check needs to remain.

AdamPS’s picture

@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.

alexpott’s picture

@AdamPS ah I missed that. I agree it adds protection but I don't think we should make that change to UserAccessControlHandler because of #540008: [PP-1] Remove the special behavior of uid #1. and just leave the protection in the form for now.

alexpott’s picture

  1. +++ b/core/modules/user/src/AccountForm.php
    @@ -85,11 +94,11 @@ public function form(array $form, FormStateInterface $form_state) {
    -      '#required' => !(!$account->getEmail() && $admin),
    +      '#required' => !(!$account->getEmail() && $user->hasPermission('administer users')),
    

    This change looks strange. Why are we just checking the perm here?

  2. +++ b/core/modules/user/src/AccountForm.php
    @@ -85,11 +94,11 @@ public function form(array $form, FormStateInterface $form_state) {
    -    // Only show name field on registration form or user can change own username.
    +    // Only show name field if access.
    

    I don't think this comment needs changing - what it used to say is still true - no?

  3. +++ b/core/modules/user/src/Form/UserCancelForm.php
    @@ -49,7 +49,8 @@ public function getCancelUrl() {
    -    if ($this->currentUser()->hasPermission('administer users') || $this->currentUser()->hasPermission('select account cancellation method')) {
    +    $admin_access = $this->currentUser()->hasPermission('administer users') || ($this->entity->id() != $this->currentUser()->id());
    +    if ($admin_access || $this->currentUser()->hasPermission('select account cancellation method')) {
    
    @@ -75,7 +76,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -    $admin_access = $user->hasPermission('administer users');
    +    $admin_access = $user->hasPermission('administer users') || ($this->entity->id() != $user->id());
    
    @@ -83,10 +84,9 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -    // Allow user administrators to skip the account cancellation confirmation
    -    // mail (by default), as long as they do not attempt to cancel their own
    -    // account.
    -    $override_access = $admin_access && ($this->entity->id() != $user->id());
    +    // When managing another user, can skip the account cancellation
    +    // confirmation mail (by default).
    +    $override_access = $this->entity->id() != $user->id();
    

    Do we still have to do the ID comparison - is there no access check we can do?

  4. +++ b/core/modules/user/src/Tests/UserAdminListingTest.php
    @@ -63,7 +63,7 @@ public function testUserListing() {
    -      $name = (string) $account->td[0]->span;
    +      $name = (string) $account->td[0]->a;
    
    +++ b/core/modules/user/src/Tests/UserAdminTest.php
    @@ -66,12 +66,12 @@ public function testUserAdmin() {
    -    $this->assertEqual($user_a->getUsername(), (string) $result[0]->td[1]->span, 'Filter by username returned the right user.');
    +    $this->assertEqual($user_a->getUsername(), (string) $result[0]->td[1]->a, 'Filter by username returned the right user.');
    ...
    -    $this->assertEqual($user_a->getUsername(), (string) $result[0]->td[1]->span, 'Filter by username returned the right user.');
    +    $this->assertEqual($user_a->getUsername(), (string) $result[0]->td[1]->a, 'Filter by username returned the right user.');
    
    +++ b/core/modules/user/user.module
    @@ -502,7 +503,12 @@ function template_preprocess_username(&$variables) {
    -  $variables['profile_access'] = \Drupal::currentUser()->hasPermission('access user profiles');
    +  if ($account instanceof AccessibleInterface) {
    +    $variables['profile_access'] = $account->access('view');
    +  }
    +  else {
    +    $variables['profile_access'] = \Drupal::currentUser()->hasPermission('access user profiles');
    +  }
    

    Why are we making these changes? I don't see how this is related to the scope of the issue?

AdamPS’s picture

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.

alexpott’s picture

Forms are not API especially the descriptions. See https://www.drupal.org/core/d8-bc-policy#strings

AdamPS’s picture

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')?

AdamPS’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 53: user.form_.subadmin-2854252-53.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

AdamPS’s picture

Status: Needs work » Needs review
FileSize
36.83 KB

Fix coding standards and broken test

agoradesign’s picture

@AdamPS is the current 2.0-alpha4 of administerusersbyrole supposed to work with the patch from #55?

I just tried to update from #33 to #55 - and besides the fact that Composer won't patch both this and #2773645-39: Allow hook_entity_field_access() to grant field-level access to User fields: 'forbidden' -> 'neutral', as the same test class file should be created by both patches - administerusersbyrole 2.0 alpha4 stopped working for me... I then returned to patch #33 and it worked again

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

AdamPS’s picture

@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.

AdamPS’s picture

Status: Needs review » Needs work

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

agoradesign’s picture

No problem. I'm comfortable with the current situation, As the site is already published, I'd hesitate anyway updating between different patches. Looking forward to see this landing in core

AdamPS’s picture

Updated patch

Matching AUBR module version coming soon.

Status: Needs review » Needs work

The last submitted patch, 61: user.form_.subadmin-2854252-61.patch, failed testing. View results

AdamPS’s picture

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

AdamPS’s picture

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’s picture

Just to better understand this, the way we solved this is by giving users the administer users permission but then use the userprotect to prevent them from editing admin users and roleassign.module to control which user roles they can assign to existing and new users. (As well as masquerade with some patches to properly limit them to certain roles).

What exactly is the difference to the approach used by administerusersbyrole? Seems kind of like the opposite, userprotect excludes certain roles, this is an opt-in for specific roles? But it could probably be implemented in the same way?

I guess even if we fix core, 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. 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.

Just some random thoughts/input, not saying that we shouldn't do this. also didn't review yet.

AdamPS’s picture

@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.

AdamPS’s picture

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
Berdir’s picture

The "amazing entity access API" has holes, unfortunately :). I specifically meant buildEntityQuery(). There is no $user in that context, this is a query against all users. And entity access only works on a single entity (with the exception of create, which is a special case).

The second example could possibly be more generic yes, but with operation edit not view :)

AdamPS’s picture

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.

Berdir’s picture

To be clear, I did not want to say that we should do this. I was just trying to better understand the difference between your module and the ones I mentioned. Didn't know yours was rewritten in that way and your arguments make sense. I think the approach we chose with our sites is fine for our use case but I agree that might not be true for everyone.

AdamPS’s picture

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’s picture

None of the patches I tried from this thread would apply for me. Maybe because I'm on 8.3.x. Is there any way you could reroll for 8.3? Or tell me which patch to use to get the administerusersbyrole module to work?

AdamPS’s picture

@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.