Problem/Motivation

It is not possible to give a user access to administer users without also giving access to all settings and configuration for user accounts. This is because the "Administer Users" permission is too broad, it allows for both the administration of user accounts as well as the user settings.

Proposed resolution

This issues proposes to split this permission into two:
* Administer Users - allowing you to create/edit/delete users
* Administer User Settings - manage the user settings, emails, fields.

Remaining tasks

Steps to reproduce:

  1. Install the latest Drupal 8.x using the standard profile.
  2. Apply patch.
  3. Go to admin/people/roles and add new role "Person manager".
  4. Go to admin/people and add new user with role Person manager. Also create one user for test.
  5. Go to admin/people and add new user with role Person manager.
  6. Go to admin/people/permissions and give that role the permission to Administer users (but not Administer user settings).
  7. Switch to that user and edit a test user account. See that he has access to /admin/people and to edit users.
  8. Go to admin/config/people/accounts see that this user has access denied.
  9. Give that user additional Administer user settings permission
  10. Login with the user again and note differences (now should be possible to access to admin/config/people/accounts and to /admin/people)
  11. Try to make the account settings change back, ensure that access changes accordingly

Pages that the permission will effect:

  • admin/people/permissions
  • admin/config/people
  • admin/people
  • admin/config/people/accounts
  • do we need more pages?

User interface changes

There are no user interface changes proposed by this issue.

API changes

Administer Users permission will no longer allow assess to the manage people section under configuration. You will also need the "Administer User Settings" permission

Original report by [ceardach]

If you grant a user the "Administer Users" permission, that user can also edit the "User Settings" page. This grants more permissions than I think would be intended for anyone to administer users.

The "Administer Users" permission allows the user to create, delete and block users and change their email and password. In addition to the that, it allows all configuration options available on the "User Settings" page, which is configuring the emails sent to users, and enable/disable registration, signatures and user pictures. The two capabilities should be separated.

I do not remember encountering this in Drupal 5. Access to the "User Settings" page may have been tied in to "Administer Site Configuration."

There should be an option to disable the password strength check in the settings for user registration. Right now it can only be disabled by a custom module with a hack messing with the javascript function that checks the password.

Note: You can accomplish most of what's here in 7.x with the User settings access module.

Files: 
CommentFileSizeAuthor
#170 drupal-split_user_permissions_d7-366950-170-do-not-test.patch1.73 KBjenlampton
#157 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--156.patch12.28 KBamontero
PASSED: [[SimpleTest]]: [MySQL] 58,684 pass(es).
[ View ]
#155 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--155.patch14.69 KBbabruix
FAILED: [[SimpleTest]]: [MySQL] 59,214 pass(es), 0 fail(s), and 1,086 exception(s).
[ View ]
#148 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--148.patch12.65 KBamontero
PASSED: [[SimpleTest]]: [MySQL] 58,635 pass(es).
[ View ]
#145 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--145.patch12.79 KBamontero
PASSED: [[SimpleTest]]: [MySQL] 58,612 pass(es).
[ View ]
#136 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--136.patch12.81 KBamontero
PASSED: [[SimpleTest]]: [MySQL] 58,450 pass(es).
[ View ]
#131 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--131.patch10.93 KBamontero
FAILED: [[SimpleTest]]: [MySQL] 57,955 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#133 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--133.patch12.81 KBamontero
PASSED: [[SimpleTest]]: [MySQL] 57,722 pass(es).
[ View ]
#133 interdiff--128-133.txt5.4 KBamontero
#128 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--128.patch8.57 KBamontero
PASSED: [[SimpleTest]]: [MySQL] 58,160 pass(es).
[ View ]
#124 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--124.patch8.56 KBamontero
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal--366950--administer-users-permission-should-be-separate-from-user-settings--124.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#123 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--123.patch7.73 KBamontero
PASSED: [[SimpleTest]]: [MySQL] 56,575 pass(es).
[ View ]
#123 interdiff-120-123.txt674 bytesamontero
#120 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--120.patch8.63 KBamontero
PASSED: [[SimpleTest]]: [MySQL] 55,635 pass(es).
[ View ]
#117 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--117.patch8.28 KBamontero
PASSED: [[SimpleTest]]: [MySQL] 56,906 pass(es).
[ View ]
#112 people_permissions_before_patch.png101.23 KBbookmarvel
#112 people_permossions_with_patch.png137.47 KBbookmarvel
#110 adminster_users_366950-110.patch7.39 KBEllaTheHarpy
PASSED: [[SimpleTest]]: [MySQL] 53,117 pass(es).
[ View ]
#110 interdiff-105-110.txt7.35 KBEllaTheHarpy
#107 Admin-People.png99.02 KBsubru77
#107 Admin-Config-People.png106.09 KBsubru77
#107 Admin-Config-People-Accounts.png127.31 KBsubru77
#106 Admin-People.png131.53 KBsubru77
#106 Admin-config-people.png100.89 KBsubru77
#106 Admin-config-people-accounts.png100.95 KBsubru77
#105 adminster_users_366950-94.patch7.36 KBstpaultim
PASSED: [[SimpleTest]]: [MySQL] 52,638 pass(es).
[ View ]
#103 admin:people.png201.43 KBbabruix
#103 editing-some-user.png206.42 KBbabruix
#103 admin-people-accounts.png129.65 KBbabruix
#101 permissions_before_patch.png166.39 KBbabruix
#101 permissions_after_patch.png150.82 KBbabruix
#102 admin-config-people.png131.75 KBbabruix
#102 admin-config-people-accounts.png193.84 KBbabruix
#102 admin-people.png118.16 KBbabruix
#94 adminster_users_366950-94.patch7.36 KBunivate
PASSED: [[SimpleTest]]: [MySQL] 52,273 pass(es).
[ View ]
#90 adminster_users_366950-90.patch7.28 KBunivate
FAILED: [[SimpleTest]]: [MySQL] 51,404 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#88 split_adminster_users_permisssion_366950-88.patch6.54 KBunivate
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch split_adminster_users_permisssion_366950-88.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#83 drupal--366950--administer-users-permission-should-be-separate-from-user-settings-83.patch7.57 KBamontero
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal--366950--administer-users-permission-should-be-separate-from-user-settings-83.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#81 drupal--366950--administer-users-permission-should-be-separate-from-user-settings-81.patch7.57 KBamontero
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/user.install.
[ View ]
#73 366950--administer-users-permission-should-be-separate-from-user-settings--73.patch9.05 KBamontero
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 366950--administer-users-permission-should-be-separate-from-user-settings--73.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#72 drupal_administer_user_settings-366950-72.patch8.14 KBhefox
PASSED: [[SimpleTest]]: [MySQL] 48,014 pass(es).
[ View ]
#66 366950--administer-users-permission-should-be-separate-from-user-settings--66.patch7.79 KBamontero
PASSED: [[SimpleTest]]: [MySQL] 42,321 pass(es).
[ View ]
#63 366950--administer-users-permission-should-be-separate-from-user-settings--63.patch7.8 KBamontero
PASSED: [[SimpleTest]]: [MySQL] 42,247 pass(es).
[ View ]
#60 366950--administer-users-permission-should-be-separate-from-user-settings--60.patch6.94 KBamontero
FAILED: [[SimpleTest]]: [MySQL] 42,243 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#56 366950--administer-users-permission-should-be-separate-from-user-settings--55.patch7.63 KBamontero
FAILED: [[SimpleTest]]: [MySQL] 42,245 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#52 366950--administer-users-permission-should-be-separate-from-user-settings--52.patch6.23 KBamontero
FAILED: [[SimpleTest]]: [MySQL] 42,243 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#48 366950--administer-users-permission-should-be-separate-from-user-settings--48.patch6.36 KBamontero
PASSED: [[SimpleTest]]: [MySQL] 42,192 pass(es).
[ View ]
#46 366950--administer-users-permission-should-be-separate-from-user-settings--46.patch5.74 KBamontero
FAILED: [[SimpleTest]]: [MySQL] 42,116 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#44 366950--administer-users-permission-should-be-separate-from-user-settings--44.patch4.88 KBamontero
FAILED: [[SimpleTest]]: [MySQL] 42,116 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#36 366950--administer-users-permission-should-be-separate-from-user-settings--35.patch4.93 KBamontero
PASSED: [[SimpleTest]]: [MySQL] 37,260 pass(es).
[ View ]
#34 366950--administer-users-permission-should-be-separate-from-user-settings.patch4.09 KBamontero
FAILED: [[SimpleTest]]: [MySQL] 37,258 pass(es), 6 fail(s), and 0 exception(s).
[ View ]
#32 366950-user-config.patch3.36 KBboombatower
PASSED: [[SimpleTest]]: [MySQL] 34,025 pass(es).
[ View ]
#30 366950-user-config.patch3.34 KBboombatower
FAILED: [[SimpleTest]]: [MySQL] 34,010 pass(es), 2 fail(s), and 0 exception(es).
[ View ]
#28 366950-user-config-D7.patch2.09 KBboombatower
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 366950-user-config-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#27 366950-user-config.patch2.13 KBboombatower
FAILED: [[SimpleTest]]: [MySQL] 33,986 pass(es), 9 fail(s), and 0 exception(es).
[ View ]
#9 366950-split-user-permissions-rev3.patch2.42 KBbrianV
Failed: Failed to install HEAD.
[ View ]
#7 366950-split-user-permissions-rev2.patch2.4 KBbrianV
Passed: 11647 passes, 0 fails, 0 exceptions
[ View ]
#5 366950-split-user-permissions.patch1.49 KBbrianV
Failed: 11113 passes, 4 fails, 0 exceptions
[ View ]

Comments

Title:"Administer Users" permision should be seperate from "User Settings""Administer Users" permission should be separate from "User Settings"

Sorry, poor spelling :(

Version:6.9» 6.10

I'm running into this too. This seems dumb intuitive. Why aren't there separate settings? Has anyone found a work around?

I concur that this is definitely a design error/bug. Permissions should be granular enough to create a user role that can add new users but not modify the global user settings. AFAIK, there is no module that enables this.

Version:6.10» 7.x-dev

This sort of change will have to be done to D7 first. Changing version. This will likely need to be verified that this is still a problem in D7.

Status:Active» Needs review
Issue tags:+Usability, +Needs text review
StatusFileSize
new1.49 KB
Failed: 11113 passes, 4 fails, 0 exceptions
[ View ]

Initial patch to split up the permissions.

I've created a new permission 'Administer User Settings', and used that as the access for 'admin/user/settings'.

I haven't tested it, and I am sure there are tests that will fail. But I would like to get a review on the text for the permission first before I finish off the patch.

Here is the text:

+    'administer user settings' => array(
+      'title' => t('Administer user settings'),
+      'description' => t('Manage administrative settings that apply to how user accounts are handled.'),
+    ),

Is it differentiated enough from the existing 'Administer users' permisson? Should this text be changed to something different?

Tagging for Usability.

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.4 KB
Passed: 11647 passes, 0 fails, 0 exceptions
[ View ]

Since there were only 4 fails, I adjusted the tests to account for the change.

Still need text review.

Status:Needs review» Needs work
Issue tags:+Quick fix

Good idea! Nice work on the patch :)

Manage administrative settings that apply to how user accounts are handled.

1) We use administer rather than manage
2) administrative is redundant, since the permission uses administer already.
3) The description isn't direct. It doesn't get to the point right away.

I suggest Administer settings that apply to all user accounts..
This uses Drupal terminology (administer and settings) and it explains the settings are global, which makes it different from administer users.

Status:Needs work» Needs review
StatusFileSize
new2.42 KB
Failed: Failed to install HEAD.
[ View ]

After conversing with Xano on IRC, we determined

Manage settings that apply to all user accounts.

would be most consistent with the wording in the rest of core.

New patch reflects this wording.

This does seem a bit different to user administration, but I'm wondering if we need a new permission.

We could either just change the permission to 'administer site configuration - which'd suit this page being moved under settings. Or maybe a custom access callback which checks both administer users and administer site configuration and requires both? Not against a new permission but I'm not sure we need one to get the granularity in this case.

@catch

I would be OK with moving this to 'Settings' and putting it under the 'administer site configuration' permission. However, I would want to get some more opinion before I write a patch for it.

I'm also in a big need of a way to separate the profile data entry and the general administer users permissions. Is there any way that this patch could be done for Drupal v.6 too?

Status:Needs review» Needs work

The last submitted patch failed testing.

Status:Needs work» Needs review

Broken TestBot?

Status:Needs review» Needs work

The last submitted patch failed testing.

Yes, it would be great to have "profile editing" and "user settings" under separate permissions rather than included with "administer user"

Izzy

subscribing.

so did all the hard work up to #9 come to nought - or is the patch useable? cheers

I would like to see much more granularity and hierarchical control:
1- instead of Administer, have View, Edit, Delete, Add, separate permissions
2- Somebody with a given role should not be able to assign, change or remove the roles of users "above" him or her role level.

It is ridiculous that in current D6 core, allowing a user to add other users also allows that user to delete higher lever admin roles. I finally found this module which solved the problem

http://drupal.org/project/administerusersbyrole

Also look at this:

http://drupal.org/project/issues/user_settings_access

Sorry that the current 1.0 version has a small coding error but the -dev version does work.

Version:7.x-dev» 8.x-dev

This is too late for D7 now, moving to 8.x.

I've created a D7 version of my module http://drupal.org/project/user_settings_access
but the problem now is that the Git update I did a few days ago doesn't seem to work (changes not in the files that are downloaded).

I raised an issue #1152580: updates not committed but no-one has looked into it yet.

I've also expanded the D7 code with a new feature (permission for Admin Role viewing).

#25 Helped a lot
This should really go into core !
I think that developpers would like to let administrators manage user accounts (CRUD) but that they would be sure that these ones does not put their hands into user accounts configuration.

Status:Needs work» Needs review
StatusFileSize
new2.13 KB
FAILED: [[SimpleTest]]: [MySQL] 33,986 pass(es), 9 fail(s), and 0 exception(es).
[ View ]

Rerolled.

StatusFileSize
new2.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 366950-user-config-D7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And d7 patch.

Status:Needs review» Needs work

The last submitted patch, 366950-user-config.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.34 KB
FAILED: [[SimpleTest]]: [MySQL] 34,010 pass(es), 2 fail(s), and 0 exception(es).
[ View ]

From reading over the contact tests they want to set the default user setting in regards to contact form, so they simply need the new permission we are creating.

Status:Needs review» Needs work

The last submitted patch, 366950-user-config.patch, failed testing.

StatusFileSize
new3.36 KB
PASSED: [[SimpleTest]]: [MySQL] 34,025 pass(es).
[ View ]

From test:

<?php
// Test allowed access for admin user to a user with contact form disabled.
?>

So that test needs the 'administer users' permission as well (not both tests).

Status:Needs work» Needs review

StatusFileSize
new4.09 KB
FAILED: [[SimpleTest]]: [MySQL] 37,258 pass(es), 6 fail(s), and 0 exception(s).
[ View ]

Attempted to review patch, but it no longer applies to 8.x head. The patch applies correctly for .module file, but tests don't apply.
Seems that #1594260: Convert user tests to PSR-0 and related issues moved the test files locations.
First attempt to reroll patch. I just tried to apply boombatower changes to files at new locations.

Status:Needs review» Needs work

StatusFileSize
new4.93 KB
PASSED: [[SimpleTest]]: [MySQL] 37,260 pass(es).
[ View ]

Fixing testUserPictureAdminFormValidation test error.

Status:Needs work» Needs review

Looks good. I didn't study the tests in detail to verify the changes are correct, so tentatively _not_ setting to RTBC, but shouldn't be far off.

Thanks, tstoeckler.
I just focused on rerolling the patch and making it work again. I still have to do the manual testing, but posted the patch to allow others to retest against current head. I will report as soon as I have some time to test it, since my initial goal was just to review it :)

Had time to review the rerolled patch and it works as expected.
Also grepped and reviewed all occurrences of " 'administer users' " string, and seems that we've not left anything else affected.
@tstoeckler: As soon as you think those tests are OK, I think it can be RTBCed.

Just to clarify: This doesn't need some sort of sign-off or special review from me. I simply wanted to point out, that I did not verify the correctness of the test changes. Pretty much anyone that looks at the surrounding code of the changes can do that.

Sorry about that. Since you mentioned the tests review, I was just saying that because I thought a more experienced tests reviews would be great. Maybe I misunderstood about you planning to test it, but ovbiously *any* two positive reviews will do. I wasn't meaning to assign you any extra responsibility, I should have been more clear.
Maybe I was too enthusiastic about moving the ball forward :) and avoid the patch not to get stale again.

Nothing to be sorry about. :)
I justed wanted to clarify in order to not stop anyone from reviewing this because of me.

StatusFileSize
new4.88 KB
FAILED: [[SimpleTest]]: [MySQL] 42,116 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Patch became stale. Reroll and bump. Please review.

Status:Needs review» Needs work
Issue tags:-Usability

Status:Needs work» Needs review
StatusFileSize
new5.74 KB
FAILED: [[SimpleTest]]: [MySQL] 42,116 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Doh. Added missing access check for user fields UI.
Not sure if entity related testbot failure has anything to do with this.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new6.36 KB
PASSED: [[SimpleTest]]: [MySQL] 42,192 pass(es).
[ View ]

Adding permission for $admin_user in tests.

Status:Needs review» Needs work
Issue tags:-Quick fix, -Needs text review

Status:Needs work» Needs review
Issue tags:+Quick fix, +Needs text review

Status:Needs review» Needs work

This time around, I actually checked all the tests, and for all them 'administer user settings' should actually *replace* 'administer users' and not be added to the permissions. Let's try that.

StatusFileSize
new6.23 KB
FAILED: [[SimpleTest]]: [MySQL] 42,243 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Looks like reasonable, let's try. Reroll.

Status:Needs work» Needs review

Status:Needs review» Needs work

Ok, then. Let's just add both 'administer users' and 'administer user settings' in the following test cases:
Drupal\block\Tests\BlockUserAccountSettingsTest
ExpandDrupal\contact\Tests\ContactPersonalTest
CollapsedDrupal\field_ui\Tests\AlterTest

I don't really see why they should depend on 'administer users', but that's not for this issue.

StatusFileSize
new7.63 KB
FAILED: [[SimpleTest]]: [MySQL] 42,245 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Hi tstoeckler.
I found some more places that might require to be changed instead of the tests.
Let's see what the testbot has to say first. If passes OK, I'll try to make an interdiff.

Status:Needs work» Needs review

Status:Needs review» Needs work

+++ b/core/modules/user/user.pages.inc
@@ -358,7 +358,7 @@ function user_cancel_methods() {
-      'access' => user_access('administer users'),
+      'access' => user_access('administer user settings'),

This one seems incorrect.

The rest looks good. I checked user_menu() manually and came to the same changes as in #56.

So only two test cases remain that need to be discussed:
1. ContactPersonalTest should receive both permissions ('administer users' and 'administer user settings'). That is because _contact_personal_tab_access correctly requires the 'administer users' permission for admins to contact users who have disabled their contact form or are blocked.

2. ManageDisplayTest fails, because the Field API forms currently require the 'administer users' permission. This should be changed to 'administer user settings' in user_entity_info()

Then, this should be truely RTBC.

StatusFileSize
new6.94 KB
FAILED: [[SimpleTest]]: [MySQL] 42,243 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

I was in doubt about user_cancel_methods() and thought at first that was a 'user settings config' one, but when looking after your useful feedback, finally saw it clear. Updated accordingly as suggested.
Let's see if ManageDisplayTest still breaks, since user_entity_info() is already updated since #55.
Let's feed the testbot.

Status:Needs work» Needs review

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new7.8 KB
PASSED: [[SimpleTest]]: [MySQL] 42,247 pass(es).
[ View ]

I think that #59.2 you pointed is due to bad testing, fixing FieldUI tests.

Status:Needs review» Needs work
Issue tags:-Quick fix+Needs usability review

I don't know how I missed the user_entity_info() change, sorry!

I tried this out locally, and the user fields UI worked with only the 'administer user settings' permission. The reason the test fails, is that ManageDisplayTest is not updated in the patch. :-) Must have been overlooked.

Seeing this in action on the user permissions page, seeing

"Administer users"
and
"Administer user settings"
directly beneath one another without a description seems strange. I know what each is for, but I don't think it is trivial to understand the distinction.

Maybe:
Administer users
Manage all user accounts. This includes editing all user information, including e-mail addresses and passwords, issuing e-mails to users and blocking and deleting user accounts.

Administer user settings
Manager user settings, such as the registration and cancellation methods, the text of user e-mails, and fields attached to users.

That might a bit verbose, but since it this is security-related stuff, I wouldn't know what to cut. The only thing that is not security-related (directly) is user fields, but I also find that valuable - and non-trivial - information for an administrator.

Marking "needs usability feedback". Also in no way is this a Quick fix any longer. It's already tagged "Needs text review", but I don't think anyone really watches that. (If so, sorry for the tag abuse.)

The previous was a crosspost with the new patch, but since it's already off to the testbot, leaving at needs work for the following and more importantly for the descriptions noted above.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Tests/FieldUiTestBase.php
@@ -25,7 +25,8 @@ function setUp() {
+    $admin_user = $this->drupalCreateUser(array(
+      'access content', 'administer content types', 'administer taxonomy', 'administer users', 'administer user settings'));

While it's not quite my taste, I'm not against breaking the array here (although I don't know why you did not do it elsewhere), but in that case, please put, the "));" (the closing of the array and the function call) on a third line, that is indented like the first line. Thanks!

Status:Needs work» Needs review
StatusFileSize
new7.79 KB
PASSED: [[SimpleTest]]: [MySQL] 42,321 pass(es).
[ View ]

Hi Tobias.
You're right. I miss a specific array wrap in the Coding Standards, and I still wrap 'at sight'. I have no strong personal preference on that, anyway. Just fixing that so it's easier to review.
I think it definitely needs descriptions as you say and +1 for asking for usability review but, how about spawning a followup novice issue for the descriptions?
We can commit this sooner and avoid the risk of becoming stale while discussing texts. And while we are at it, we're 'seeding' novice issues, too. Texts may come in time, if appliable, for backport. Just an update hook for copying to currently granted roles the new permission, so we not break anything.

I don't think we can backport this. Since 'administer users' doesn't have a description currently I'm fine with punting on that.
Good thing you mention the upgrade path. We definitely need that (+ upgrade path tests). After that this should be done.

So, by now, assuming this just awaits to get 2 positive reviews and can go RTBC, and deal with texts and backporting later.
The folloup issue is ready (in postponed state) at:
#1813488: Add descriptions to clarify "administer users" and "administer user settings" permissions

Actually this needs an upgrade path and accompanying upgrade path tests. We should grant anyone who had 'administer users' before 'administer user settings' as well.
I'll try to take a stab at this tomorrow (unless you beat me to it) :-)

Status:Needs review» Needs work

Any update on this?

I have put it back to 'needs work', because of:
Hunk #1 succeeded at 54 (offset 17 lines).
Hunk #1 FAILED at 164.
1 out of 4 hunks FAILED

I'm working on a proposal to refactor the account settings page, fixing separate permisions would really be great for this!
#1837054: [META] Refactor account workflow (and config)
(see screenshots + demo linked on the issue)

Assigned:Unassigned» hefox

I'm taking a stab of it. I got the hunk #1 fixed. Was working on the upgrade path when a sudden sushi trip happened.

Assigned:hefox» Unassigned
Status:Needs work» Needs review
StatusFileSize
new8.14 KB
PASSED: [[SimpleTest]]: [MySQL] 48,014 pass(es).
[ View ]

Added update hook

Is upgrade path test required for such a simple update? The documentation for upgrade path looks like needs update, http://drupal.org/node/1429136, for d8 as grep -R for UpgradePathTestCase returns no results.

StatusFileSize
new9.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 366950--administer-users-permission-should-be-separate-from-user-settings--73.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Manually tested the upgrade path and works OK.
Rerolling patch just to remove trailing whitespace and a drupal_set_message in update hook that I assume it's a debugging leftover, but it is the same code of hefox.

Looks good, tested both, with the whitespace on #72 indeed, but all seems to work ok. Can use a couple more people testing it, but very close i guess.

adding tags. updating issue summary.

@Rob C, what kind of more testing do you think this needs?

This could use an interdiff between #72 and #73

adding tag.

Also, patch in #73 has changes to Tests. Were any new tests added that I missed? Are any new tests needed?

@YesCT: What tests do you mean? A plain diff of patches #72 & #73 does not show changes in tests.

Issue tags:+Novice, +Needs reroll

Tagging

Status:Needs work» Needs review
Issue tags:-Novice, -Needs reroll
StatusFileSize
new7.57 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/user.install.
[ View ]

Reroll for the testbot. Manual tests will follow if green.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new7.57 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal--366950--administer-users-permission-should-be-separate-from-user-settings-83.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ops, fixing typo. Reroll for the testbot. Manual tests will follow if green.

This still needs tests for the upgrade path. It should be as simple as having a user that has the 'administer users' permission in D7. And then visiting admin/config/people/accounts after the upgrading and asserting the page can be accessed.

Issue tags:+Needs reroll

tagging for reroll

Issue summary:View changes

Updated issue summary with remaining steps to help new people

Issue summary:View changes

Create issue summary.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new6.54 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch split_adminster_users_permisssion_366950-88.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-rolled patch from #83

Status:Needs review» Needs work

The last submitted patch, split_adminster_users_permisssion_366950-88.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.28 KB
FAILED: [[SimpleTest]]: [MySQL] 51,404 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Not sure the issue with that patch - trying again.

Status:Needs review» Needs work
Issue tags:-Needs issue summary update, -refactor account workflow, -Needs upgrade path tests

The last submitted patch, adminster_users_366950-90.patch, failed testing.

Status:Needs work» Needs review

#90: adminster_users_366950-90.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+Needs issue summary update, +refactor account workflow, +Needs upgrade path tests

The last submitted patch, adminster_users_366950-90.patch, failed testing.

StatusFileSize
new7.36 KB
PASSED: [[SimpleTest]]: [MySQL] 52,273 pass(es).
[ View ]

The admin test failing above was due to not having the new permission "Administer user settings", so it was correctly failing on that test. Adding the permission to the user in that test should pass now.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

(done) manual testing: passed all simpletest cases and manual testing
(done) list pages that the permission will effect: admin/config/people, admin/config/people/accounts
(done) checked coding standards and documentation look good

Assigned:Unassigned» babruix
Status:Reviewed & tested by the community» Needs work

Trying to improve summary to accurately describe the current state of that issue and what needs to be done moving forward.

Just to reiterate, this still needs upgrade path tests.

Issue summary:View changes

Missing credit to original reporter

Assigned:babruix» Unassigned

Updated summary. It is not clear, what should be "steps to test/reproduce" - are there only to check pages with user who has appropriate permissions?
Also not clear what this means "before and after screenshot of the pages that the permission effects" - is that means upload here screenshots of all pages where 'administer user settings' permission is used for checking access?

@babruix

For steps to reproduce (contributor task doc: http://drupal.org/node/1468198) they are more like steps to test in this case. to show how the new functionality works.

start with:
get recent drupal 8
apply patch
install
etc.

then something like:
go to people roles
make a user with role personel manager (or something)
give that role the permission to administer users (but not user settings)
switch to that user and edit a user account.. see that they can ... whatever they should be able to do.
try to change the account settings, for example ... whatever an example is... see that they cannot.
give that user additional admin user settings permission
edit the user again and note differences
try to make the account settings change again, see that they can

I would advice putting the steps to reproduce in the issue summary, so that you and others can edit and improve them as needed.

For the before and after screenshots. (contributor task doc: http://drupal.org/node/1859584)
a before of the permissions page (showing just the joint permission)
and an after of the permissions page (showing the admin users, and the admin account settings)
would be good.

optional would also be to include some screenshots of what editing users looks like for a user with the role that has just the admin users and not the account settings permission (if there is a difference)..

Issue summary:View changes

updated summary of what is left, removed completed tasks

Issue summary:View changes

Adding followup issue from comments

StatusFileSize
new150.82 KB
new166.39 KB

Permissions page: before and after patch.

StatusFileSize
new118.16 KB
new193.84 KB
new131.75 KB

As user with "Administer user settings" permission.

StatusFileSize
new129.65 KB
new206.42 KB
new201.43 KB

As user with "Administer users" permission.

Issue summary:View changes

added steps for reproduce

Assigned:Unassigned» stpaultim

Assigned:stpaultim» Unassigned
Status:Needs work» Needs review
StatusFileSize
new7.36 KB
PASSED: [[SimpleTest]]: [MySQL] 52,638 pass(es).
[ View ]

Maryann and I just re-rolled this patch as part of Drupal Sprint Weekend. Hope all is well.

StatusFileSize
new100.95 KB
new100.89 KB
new131.53 KB

User with just "Administer users "

StatusFileSize
new127.31 KB
new106.09 KB
new99.02 KB

User with just "Administer user settings "

Good work Maryann and stpaultim on the reroll! I tested it and it applies to latest HEAD. Also, compared it to most recent patch, and it looks like no changes were lost and nothing unexpected showed up in the patch. So who wants to do the upgrade path tests for this?

Status:Needs review» Needs work

looking at #101
permissions page

I think it would make sense to name the new permission Administer account settings instead of Administer user settings

because, looking at #107
(the screenshots for a user with just admin user settings)

admin/people
admin people

admin/config/people (*has* a link to Account settings)

and the stuff they *can* do is at: admin/config/people/accounts with the title "Account settings"

Setting to needs work for the upgrade path tests, and to change the permission name.

StatusFileSize
new7.35 KB
new7.39 KB
PASSED: [[SimpleTest]]: [MySQL] 53,117 pass(es).
[ View ]

I made the change that YesCT recomended in comment #109
I changed administer user settings to administer account settings.

Status:Needs work» Needs review

i took screenshots before and after.
people_permissions_before_patch.pngpeople_permossions_with_patch.png

Title:"Administer Users" permission should be separate from "User Settings""Administer Users" permission should be separate from "Administer Account Settings"

updating title

@subru77 Thanks for adding those screenshots in #106 and #107

I was able to embed them. They were helpful.

I love these contributor task documents: http://drupal.org/node/1489010

You can consult it again next time and it has tips like embedding and other good info.

Component:user system» user.module

Yes, please! This is badly needed. Thanks everyone for working on this.

Most of the patch looks good already, but I have some concerns around labeling, exposure, and UX:

+++ b/core/modules/user/user.module
@@ -542,6 +542,10 @@ function user_permission() {
+    'administer account settings' => array(
+      'title' => t('Administer account settings'),

1) "Account settings" is highly ambiguous in this list.

'cos which account settings are meant? The settings for each user account? As in, the edit form for each user account? Surely not! :)

I think the permission label (as well as the internal permission name) should at least contain the term configuration. That leads to a 1:1 cognitive mapping to the "Configuration" top-level toolbar item. For example:

"Administer user account configuration"
[administer user configuration]

2) To further clarify what is being meant with this permission, let's add an accompanying description, which should ideally even contain a link to the People configuration category; i.e., /admin/config/people — e.g.:

"Configure site-wide settings and behavior for user accounts and registration."

The 'restrict access' needs to be kept additionally.

3) In terms of security, this new permission is either the topmost or the second topmost important User module permission.

We should move it right below Administer permissions and before Administer users. (Permissions are output exactly in the order in which they are defined.)

Would be great to get this done for D8. Rock on!

Status:Needs review» Needs work
Issue tags:+Novice

Those are well described changes that need to be made. A good novice task. (adding tag)

Status:Needs work» Needs review
Issue tags:-Novice
StatusFileSize
new8.28 KB
PASSED: [[SimpleTest]]: [MySQL] 56,906 pass(es).
[ View ]

Rebasing. Feed the testbot. The menu item permission has been moved to a YAML, hope it works.
Will give it a shot if passes.

Issue tags:+Novice

I swear I didn't removed the tag :)

For the record and as a 'manual interdiff', the only code needing manual merging was this block:

@@ -1004,7 +1008,7 @@ function user_menu() {
     'description' => 'Configure default behavior of users, including registration requirements, e-mails, and fields.',
     'page callback' => 'drupal_get_form',
     'page arguments' => array('user_admin_settings'),
-    'access arguments' => array('administer users'),
+    'access arguments' => array('administer account settings'),
     'file' => 'user.admin.inc',
     'weight' => -10,
   );


Since the menu functionality changed, modification to user_menu function is no longer needed. Instead, I've added this:
--- a/core/modules/user/user.routing.yml
+++ b/core/modules/user/user.routing.yml
@@ -24,7 +24,7 @@ user_account_settings:
   defaults:
     _form: '\Drupal\user\AccountSettingsForm'
   requirements:
-    _permission: 'administer users'
+    _permission: 'administer account settings'
user_role_list:
   pattern: '/admin/people/roles'


The remaining patch modifications rebased successfully.

StatusFileSize
new8.63 KB
PASSED: [[SimpleTest]]: [MySQL] 55,635 pass(es).
[ View ]

Based on latest working patch, addressing sun's comments in #115:
1) I'm +1 with it, but when going for it, I've found "Account settings" elsewhere in Drupal's UI and code, or for instance in 'cancel account' perm description it's referred as 'user settings'. To allow proper UX discussion and to keep patch simpler, I think that opening a separate UX issue would be better. I agree the reasoning and luckily #115.2 description link alleviates this. I've skipped this.
2) Added description as proposed, linked to admin/config/people and kept 'restrict access' as TRUE.
3) Moved to 2nd place as proposed.

This looks good and it means that I might not need to develop a Drupal 8 version of my module.

http://drupal.org/project/user_settings_access

One additional feature that I added (on request) was to control who can edit the admin role. That way you can give someone access to change all the settings (email templates and such) but not change the admin role.

Maybe this could also be added to Drupal 8 and then my module is completely merged into the core.

Hi tchurch.
I'm afraid we're too late in the release cycle to add new features.
Maybe someone more qualified can say if it could be accounted as a polishing. I would love to see a patch with that one added!
BTW, I've linked your module in the issue summary for completion. Nice to know.

Issue summary:View changes

removed completed tasks

StatusFileSize
new674 bytes
new7.73 KB
PASSED: [[SimpleTest]]: [MySQL] 56,575 pass(es).
[ View ]

Patch became stale. Chasing HEAD.
As result of #1875992: Add EntityFormDisplay objects for entity forms:

hook_field_widget_properties_alter() is removed in favor of the new hook_entity_form_display_alter().

And now the test has been moved to EntityFormTest and no longer performs tests on user entities but on test_entity, so looks like it is no longer needed.

StatusFileSize
new8.56 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal--366950--administer-users-permission-should-be-separate-from-user-settings--124.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch became stale again. Rerolling.
Beware: to avoid rewritting the hook_update for permissions I've had to move it up so the current code makes sense. This is, making this patch update hook be run before the permissions and roles are migrated to config. This may create a black hole in your Drupal install and swallow all the Universe and blah, blah... dunno if it's too late in the dev cycle to do this (and yes, I'm a bit lazy) but looks no big deal to me. You have been warned ;)
Aside from this, it's just a reroll of #123.

Issue tags:+Configuration system

Tagging to highlight hook_update reordering to CMI interested parties before we leave alpha.

StatusFileSize
new8.57 KB
PASSED: [[SimpleTest]]: [MySQL] 58,160 pass(es).
[ View ]

Reroll of #124, since it became stale.

Status:Needs work» Needs review

Doh.

Status:Needs review» Needs work

Still looks good, but still needs upgrade path tests.

StatusFileSize
new10.93 KB
FAILED: [[SimpleTest]]: [MySQL] 57,955 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

First stab at the UP tests. It's #128 without the upgrade hook plus these test files:

create mode 100644 core/modules/system/lib/Drupal/system/Tests/Upgrade/UserPermissionUpgradePathTest.php
create mode 100644 core/modules/system/tests/upgrade/drupal-7.user_permission.database.php

It's meant to fail.

Status:Needs work» Needs review
StatusFileSize
new5.4 KB
new12.81 KB
PASSED: [[SimpleTest]]: [MySQL] 57,722 pass(es).
[ View ]

Fails correctly :) in "Administer account settings" page was found test.
So, here it is with the update hooks re-added.
It's the same as in #128 (no changes) with the above two testing files added. This one should pass tests.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs upgrade path tests

Yeah, @amontero!
Tests look great.

Status:Reviewed & tested by the community» Needs work

Needs a reroll

git ac https://drupal.org/files/drupal--366950--administer-users-permission-should-be-separate-from-user-settings--133.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 13118  100 13118    0     0   6264      0  0:00:02  0:00:02 --:--:--  7160
error: patch failed: core/modules/contact/lib/Drupal/contact/Tests/ContactPersonalTest.php:54
error: core/modules/contact/lib/Drupal/contact/Tests/ContactPersonalTest.php: patch does not apply
error: patch failed: core/modules/user/user.install:1118
error: core/modules/user/user.install: patch does not apply

Status:Needs work» Needs review
StatusFileSize
new12.81 KB
PASSED: [[SimpleTest]]: [MySQL] 58,450 pass(es).
[ View ]

Rebasing against HEAD.

Looks like testbot is having issues. Both runs run failed with an error message "The test did not complete due to a fatal error" in group "Completion check".
First time failed in DateTime tests and now fails at MenuLanguageTest with no commits inbetween (?).

Status:Needs review» Reviewed & tested by the community

The reroll passed tests OK. (!?)
RTBCing again.

Note: I am OK with renumbering updates. In fact, we probably should do more of that, to get rid of the emptied out updates. The "thou shalt not edit your updates" decree only comes in effect at beta1.

Status:Reviewed & tested by the community» Needs work

Minor nit.

+++ b/core/modules/user/user.installundefined
@@ -1013,11 +1013,32 @@ function user_update_8016() {
+  $rids = array();
+  $rids = db_query("SELECT rid FROM {role_permission} WHERE permission = :perm", array(':perm' => 'administer users'))->fetchCol();

$rids = array(); is not needed since $rids is assigned on the next line

Status:Needs work» Needs review
StatusFileSize
new12.79 KB
PASSED: [[SimpleTest]]: [MySQL] 58,612 pass(es).
[ View ]

Done.

Status:Needs review» Reviewed & tested by the community

Still looks good.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/user/user.module
@@ -479,6 +479,13 @@ function user_permission() {
+      'description' => user_access('administer account settings')
+        ? t('Configure site-wide settings and behavior for <a href="@url">user accounts and registration</a>.', array('@url' => url('admin/config/people')))
+        : t('Configure site-wide settings and behavior for user accounts and registration.'),

user_access is deprecated for Drupal 8 and should not be used. In a recent discussion on IRC we've decided that access checking in hook_permission is a bit silly so we should not do it. If the link adds a lot of value we should just leave it in.

Status:Needs work» Needs review
StatusFileSize
new12.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,635 pass(es).
[ View ]

user_acces removed. Link is helpful, IMO.

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Needs work
Issue tags:-Needs issue summary update, -Novice, -Configuration system, -refactor account workflow

Status:Needs review» Reviewed & tested by the community

I don't think line 51 of FeedProcessorPluginTest is actually related to this...

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Needs review
StatusFileSize
new14.69 KB
FAILED: [[SimpleTest]]: [MySQL] 59,214 pass(es), 0 fail(s), and 1,086 exception(s).
[ View ]

Re-rolled.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new12.28 KB
PASSED: [[SimpleTest]]: [MySQL] 58,684 pass(es).
[ View ]

Above patch is not only a reroll. It contains changes:
1) function user_validate_name($name)

2)

   // Administration pages.
   $items['admin/config/people'] = array(
-    'title' => 'People',
-    'description' => 'Configure user accounts.',
-    'position' => 'left',
-    'weight' => -20,
-    'route_name' => 'user_admin_index',
+   'title' => 'People',
+   'description' => 'Configure user accounts.',
+   'position' => 'left',
+   'weight' => -20,
+   'route_name' => 'user_admin_index',
+   'page callback' => 'system_admin_menu_block_page',
+   'access arguments' => array('administer account settings'),
+   'file' => 'system.admin.inc',
+   'file path' => drupal_get_path('module', 'system'),
   );

(also the indenting is incorrect)

3) and several:

-    'version' => Drupal::VERSION,
+    'version' => VERSION,

All of them look like have been introduced during the merge/rebase.

Straight reroll of #148. Had to rebase manually this code: http://pastebin.com/QzPbx7wv .

Title:"Administer Users" permission should be separate from "Administer Account Settings"Change notice: "Administer Users" permission should be separate from "Administer Account Settings"
Priority:Normal» Critical
Status:Needs review» Active
Issue tags:-Needs issue summary update, -Needs reroll+Needs change record

Committed 73c2194 and pushed to 8.x. Thanks!

We need a change notice for this for site builders for this.

Category:bug» task

Status:Active» Fixed
Issue tags:-Needs change record

Title:Change notice: "Administer Users" permission should be separate from "Administer Account Settings""Administer Users" permission should be separate from "Administer Account Settings"

Priority:Critical» Normal

Thanks @mtift :)

Wow, epic issue. Thanks for everyone who stuck with this!

Status:Fixed» Active

Shouldn't we warn someway in the change notice to admins upgrading from 7.x and having installed user_settings_access module? The update hook would suddenly give rights to roles not having it. :S

Status:Active» Fixed

I think that module should warn about that itself, perhaps on the project page. People having it installed will be looking for a Drupal 8 version of that anyway.

Please feel free to add a note to the change notice, nonetheless. Such hints never hurt IMO. I don't think we need this issue open for that, however.

Done. Added "Security considerations" section in the change notice:
https://drupal.org/node/2083321

Also reported in "User Settings Access" module issue queue:
#2098741: Warn users of added permission in core when upgrading to 8.x

Status:Fixed» Closed (fixed)
Issue tags:-Novice, -Configuration system, -refactor account workflow

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes

Link to "User Settings Access" module.

And for those of us who would like this feature on D7, here's a patch for that too.