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

A change was committed to Drupal 8.x that split the permission into two:

  • Administer Users - allowing you to create/edit/delete users
  • Administer Account Settings - manage the user settings, emails, fields.

In #196 @eiriksm supplied a patch for Drupal 7.x that takes a slightly different (backwards-compatible) approach:

  • Adds a new user_enable_separate_account_settings user module setting which allows the additional permission to be optionally enabled (opt-in).
  • When this setting is enabled, the administer account settings permission is added and used to control access to admin/config/people/accounts and user entity bundles administration.

Remaining tasks

Review of the latest patches using the approach proposed in #196.

If the approach proposed in #196 is not acceptable and the same approach that was taken for Drupal 8.x needs to be used for Drupal 7.x then more work needs to be done:

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.

CommentFileSizeAuthor
#216 drupal-administer_users_permission-366950-216.patch5.93 KBgenellann
#214 core-administer_users-366950-214.patch5.93 KBjenlampton
#206 administer_users_-366950-206.patch5.93 KBjoegraduate
#196 administer_users_-366950-196.patch5.95 KBeiriksm
#195 drupal-administer_users_permission-366950-195.patch4.11 KBalex.skrypnyk
#185 drupal-administer_users_permission-366950-185.patch4.18 KBPascalAnimateur
#180 interdiff-176-180.txt1.38 KBeiriksm
#180 administer_users_-366950-180.patch4.02 KBeiriksm
#176 interdiff-366950-174-176.txt2.44 KBzaporylie
#176 administer_users_-366950-176.patch3.99 KBzaporylie
#174 interdiff-366950-174-170.txt590 byteszaporylie
#174 administer_users_-366950-174.patch2.3 KBzaporylie
#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
#155 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--155.patch14.69 KBbabruix
#148 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--148.patch12.65 KBamontero
#145 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--145.patch12.79 KBamontero
#136 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--136.patch12.81 KBamontero
#131 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--131.patch10.93 KBamontero
#133 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--133.patch12.81 KBamontero
#133 interdiff--128-133.txt5.4 KBamontero
#128 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--128.patch8.57 KBamontero
#124 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--124.patch8.56 KBamontero
#123 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--123.patch7.73 KBamontero
#123 interdiff-120-123.txt674 bytesamontero
#120 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--120.patch8.63 KBamontero
#117 drupal--366950--administer-users-permission-should-be-separate-from-user-settings--117.patch8.28 KBamontero
#112 people_permissions_before_patch.png101.23 KBbookmarvel
#112 people_permossions_with_patch.png137.47 KBbookmarvel
#110 adminster_users_366950-110.patch7.39 KBshnark
#110 interdiff-105-110.txt7.35 KBshnark
#107 Admin-People.png99.02 KBshariharan
#107 Admin-Config-People.png106.09 KBshariharan
#107 Admin-Config-People-Accounts.png127.31 KBshariharan
#106 Admin-People.png131.53 KBshariharan
#106 Admin-config-people.png100.89 KBshariharan
#106 Admin-config-people-accounts.png100.95 KBshariharan
#105 adminster_users_366950-94.patch7.36 KBstpaultim
#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
#90 adminster_users_366950-90.patch7.28 KBunivate
#88 split_adminster_users_permisssion_366950-88.patch6.54 KBunivate
#83 drupal--366950--administer-users-permission-should-be-separate-from-user-settings-83.patch7.57 KBamontero
#81 drupal--366950--administer-users-permission-should-be-separate-from-user-settings-81.patch7.57 KBamontero
#73 366950--administer-users-permission-should-be-separate-from-user-settings--73.patch9.05 KBamontero
#72 drupal_administer_user_settings-366950-72.patch8.14 KBhefox
#66 366950--administer-users-permission-should-be-separate-from-user-settings--66.patch7.79 KBamontero
#63 366950--administer-users-permission-should-be-separate-from-user-settings--63.patch7.8 KBamontero
#60 366950--administer-users-permission-should-be-separate-from-user-settings--60.patch6.94 KBamontero
#56 366950--administer-users-permission-should-be-separate-from-user-settings--55.patch7.63 KBamontero
#52 366950--administer-users-permission-should-be-separate-from-user-settings--52.patch6.23 KBamontero
#48 366950--administer-users-permission-should-be-separate-from-user-settings--48.patch6.36 KBamontero
#46 366950--administer-users-permission-should-be-separate-from-user-settings--46.patch5.74 KBamontero
#44 366950--administer-users-permission-should-be-separate-from-user-settings--44.patch4.88 KBamontero
#36 366950--administer-users-permission-should-be-separate-from-user-settings--35.patch4.93 KBamontero
#34 366950--administer-users-permission-should-be-separate-from-user-settings.patch4.09 KBamontero
#32 366950-user-config.patch3.36 KBboombatower
#30 366950-user-config.patch3.34 KBboombatower
#28 366950-user-config-D7.patch2.09 KBboombatower
#27 366950-user-config.patch2.13 KBboombatower
#9 366950-split-user-permissions-rev3.patch2.42 KBbrianV
#7 366950-split-user-permissions-rev2.patch2.4 KBbrianV
#5 366950-split-user-permissions.patch1.49 KBbrianV
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ceardach’s picture

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

Sorry, poor spelling :(

redijedi’s picture

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?

mpaler’s picture

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.

ceardach’s picture

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.

brianV’s picture

Status: Active » Needs review
Issue tags: +Usability, +Needs text review
FileSize
1.49 KB

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.

brianV’s picture

Status: Needs work » Needs review
FileSize
2.4 KB

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

Still need text review.

Xano’s picture

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.

brianV’s picture

Status: Needs work » Needs review
FileSize
2.42 KB

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.

catch’s picture

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.

brianV’s picture

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

starscream’s picture

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.

tstoeckler’s picture

Status: Needs work » Needs review

Broken TestBot?

Status: Needs review » Needs work

The last submitted patch failed testing.

BassPlaya’s picture

izmeez’s picture

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

Izzy

tchurch’s picture

subscribing.

petednz’s picture

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

Alan.Guggenheim’s picture

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.

robbdavis’s picture

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

tchurch’s picture

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.

timofey’s picture

catch’s picture

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

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

tchurch’s picture

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

DuaelFr’s picture

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

boombatower’s picture

Status: Needs work » Needs review
FileSize
2.13 KB

Rerolled.

boombatower’s picture

FileSize
2.09 KB

And d7 patch.

Status: Needs review » Needs work

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

boombatower’s picture

Status: Needs work » Needs review
FileSize
3.34 KB

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.

boombatower’s picture

FileSize
3.36 KB

From test:

// 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).

boombatower’s picture

Status: Needs work » Needs review
amontero’s picture

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

Fixing testUserPictureAdminFormValidation test error.

amontero’s picture

Status: Needs work » Needs review
tstoeckler’s picture

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.

amontero’s picture

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 :)

amontero’s picture

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.

tstoeckler’s picture

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.

amontero’s picture

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.

tstoeckler’s picture

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

amontero’s picture

Patch became stale. Reroll and bump. Please review.

Status: Needs review » Needs work
Issue tags: -Usability
amontero’s picture

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

Adding permission for $admin_user in tests.

Status: Needs review » Needs work
Issue tags: -Quick fix, -Needs text review
amontero’s picture

Status: Needs work » Needs review
Issue tags: +Quick fix, +Needs text review
tstoeckler’s picture

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.

amontero’s picture

Looks like reasonable, let's try. Reroll.

amontero’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
tstoeckler’s picture

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.

amontero’s picture

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.

amontero’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
tstoeckler’s picture

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

amontero’s picture

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.

amontero’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
amontero’s picture

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

tstoeckler’s picture

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

tstoeckler’s picture

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!

amontero’s picture

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.

tstoeckler’s picture

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.

amontero’s picture

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

tstoeckler’s picture

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) :-)

Rob C’s picture

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)

hefox’s picture

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.

hefox’s picture

Assigned: hefox » Unassigned
Status: Needs work » Needs review
FileSize
8.14 KB

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.

amontero’s picture

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.

Rob C’s picture

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.

YesCT’s picture

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

YesCT’s picture

adding tag.

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

amontero’s picture

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

amontero’s picture

amontero’s picture

Issue tags: +Novice, +Needs reroll

Tagging

amontero’s picture

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

Status: Needs review » Needs work
amontero’s picture

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

tstoeckler’s picture

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.

amontero’s picture

YesCT’s picture

Issue tags: +Needs reroll

tagging for reroll

YesCT’s picture

Issue summary: View changes

Updated issue summary with remaining steps to help new people

univate’s picture

Issue summary: View changes

Create issue summary.

univate’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
6.54 KB

Re-rolled patch from #83

Status: Needs review » Needs work

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

univate’s picture

Status: Needs work » Needs review
FileSize
7.28 KB

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.

univate’s picture

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.

univate’s picture

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.

univate’s picture

Status: Needs work » Needs review
babruix’s picture

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

babruix’s picture

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.

tstoeckler’s picture

Just to reiterate, this still needs upgrade path tests.

tstoeckler’s picture

Issue summary: View changes

Missing credit to original reporter

babruix’s picture

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?

YesCT’s picture

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

YesCT’s picture

Issue summary: View changes

updated summary of what is left, removed completed tasks

amontero’s picture

Issue summary: View changes

Adding followup issue from comments

babruix’s picture

Permissions page: before and after patch.

babruix’s picture

As user with "Administer user settings" permission.

babruix’s picture

As user with "Administer users" permission.

babruix’s picture

Issue summary: View changes

added steps for reproduce

stpaultim’s picture

Assigned: Unassigned » stpaultim
stpaultim’s picture

Assigned: stpaultim » Unassigned
Status: Needs work » Needs review
FileSize
7.36 KB

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

shariharan’s picture

User with just "Administer users "

shariharan’s picture

User with just "Administer user settings "

disasm’s picture

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?

YesCT’s picture

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.

shnark’s picture

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

shnark’s picture

Status: Needs work » Needs review
bookmarvel’s picture

i took screenshots before and after.
people_permissions_before_patch.png

people_permossions_with_patch.png

YesCT’s picture

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

updating title

YesCT’s picture

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

sun’s picture

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!

YesCT’s picture

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

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

amontero’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
8.28 KB

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

amontero’s picture

Issue tags: +Novice

I swear I didn't removed the tag :)

amontero’s picture

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.

amontero’s picture

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.

tchurch’s picture

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.

amontero’s picture

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.

amontero’s picture

Issue summary: View changes

removed completed tasks

amontero’s picture

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.

amontero’s picture

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.

amontero’s picture

Issue tags: +Configuration system

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

amontero’s picture

amontero’s picture

Status: Needs work » Needs review

Doh.

tstoeckler’s picture

Status: Needs review » Needs work

Still looks good, but still needs upgrade path tests.

amontero’s picture

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.

amontero’s picture

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.

tstoeckler’s picture

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

Yeah, @amontero!
Tests look great.

alexpott’s picture

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

Rebasing against HEAD.

amontero’s picture

amontero’s picture

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 (?).

star-szr’s picture

amontero’s picture

Status: Needs review » Reviewed & tested by the community

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

chx’s picture

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.

alexpott’s picture

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

amontero’s picture

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Still looks good.

alexpott’s picture

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.

amontero’s picture

user_acces removed. Link is helpful, IMO.

tstoeckler’s picture

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

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

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

amontero’s picture

alexpott’s picture

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

Patch no longer applies.

babruix’s picture

Re-rolled.

Status: Needs review » Needs work
amontero’s picture

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 .

alexpott’s picture

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.

amontero’s picture

catch’s picture

Category: bug » task
mtift’s picture

Status: Active » Fixed
Issue tags: -Needs change record
mtift’s picture

Title: Change notice: "Administer Users" permission should be separate from "Administer Account Settings" » "Administer Users" permission should be separate from "Administer Account Settings"
star-szr’s picture

Priority: Critical » Normal

Thanks @mtift :)

tstoeckler’s picture

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

amontero’s picture

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

tstoeckler’s picture

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.

amontero’s picture

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.

Anonymous’s picture

Issue summary: View changes

Link to "User Settings Access" module.

jenlampton’s picture

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

sinasalek’s picture

Backport is definitely needed

eiriksm’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Closed (fixed) » Reviewed & tested by the community

7.x patch looks good to me.

Status: Reviewed & tested by the community » Needs work
zaporylie’s picture

Status: Needs work » Needs review
Issue tags: +Needs backport to 7.x
FileSize
2.3 KB
590 bytes

I've added hook_update to user.install to set new permission up by default, based on 'administer users' permission.

Status: Needs review » Needs work

The last submitted patch, 174: administer_users_-366950-174.patch, failed testing.

zaporylie’s picture

Status: Needs work » Needs review
FileSize
3.99 KB
2.44 KB

Tests should be fixed now.

eiriksm’s picture

Status: Needs review » Reviewed & tested by the community

Looking good!

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work
  1. This certainly needs more discussion about whether it's actually safe to backport. Lots of people above seemed to think it was not.... going back years on this issue. Don't lots of contrib modules rely on "administer users" behaving a certain way?
  2. The new permission is not marked as a permission with security implications, but shouldn't it be? If it lets you change the admin role (which is on that settings page) I don't see how it's a whole lot less dangerous than "administer users".
  3. The update function is not written correctly. See system_update_7067() for an example of how to do this kind of thing in an update function. Also it should be inside the "addtogroup updates-7.x-extra" docblock, not outside it...
xjm’s picture

Issue tags: -Needs backport to 7.x +Needs backport to D7
eiriksm’s picture

OK, so new patch.

Regarding the points in #178:

1. More discussion would indeed be nice. But here is my argument at least:
The patch adds a new permission. It does not revoke the 'administer users' permissions from anyone. It does add a new callback for the user administer page, but that permission is granted to everyone that already has the "old" permission. So all users that were able to access that menu callback earlier, will still be. As far as I see it, this is backwards compatible (meaning it is safe to upgrade). If contrib modules rely on administer users being set for specific users or roles, they would still have it after this update. If they (for some reason) rely on a user or role having access to the Account settings menu callback, those same roles and users would still return the same result after the upgrade. If you actively go to the step to revoke the "administer users" permission for a role on a live site that has been running for quite some time, I would say you probably are doing that intentionally. Are there more use cases?

2. That is probably true. I updated the patch.

3. Updated the update function.

Feedback (and discussion) very welcome!

Have a nice week everyone!

eiriksm’s picture

Status: Needs work » Needs review

Forgot to change status.

justaman’s picture

small hook to close edits on more privileged users. It would be nice to do a matrix like role_delegation to set which roles will be able to edit which roles.

function permissions_extra_user_presave(&$edit, $account, $category) {
if( $account->uid == 1 || in_array('adminrole',$account->roles)){ // trying to edit an admin
global $user;
if($user->uid != 1){//only super admin can do that
drupal_set_message( "You attempted to edit an admin or dev user name and you are not allowed, this attempt was logged." ,'error');
watchdog('permissions_extra','Logged in user %user tried to edit account %account', array('%user' => $user->uid, '%account' => $account->uid), WATCHDOG_ERROR);)
drupal_goto('/admin/people');
}
}
}

MaskOta’s picture

I would suggest we take incremental steps. This permission to be able to edit users but not their settings is really basic.

I say we put in the sepparate permission to administer users and administer their settings ASAP and we take more complex and indepth solutions from there on

IRuslan’s picture

#180 works for me and looks good.
+1 RTBC.

PascalAnimateur’s picture

Updated the patch for use with Drupal 7.50 ... since the is a new user_update_7019() I bumped the number to 7020 and called 7019 to make sure the new update would be applied on sites already up to 7019.

  • alexpott committed 73c2194 on 8.3.x
    Issue #366950 by amontero, boombatower, brianV, univate, hefox,...

  • alexpott committed 73c2194 on 8.3.x
    Issue #366950 by amontero, boombatower, brianV, univate, hefox,...
IRuslan’s picture

Status: Needs review » Fixed

I guess it should be Fixed as already committed.

eiriksm’s picture

Status: Fixed » Needs review

It is fixed in Drupal 8, we are trying to get it fixed in D7 :)

andrewfn’s picture

#185 works for me and solves a very important problem. I'm surprised it this is not committed yet.

andrewfn’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work
  1. Why is this using "administer user settings" for the machine name, when Drupal 8 uses "administer account settings"? Also I would think the title/description of the permission should be exactly the same as what's currently in Drupal 8...
  2. +function user_update_7020() {
    +  $user_admin_roles = user_roles(FALSE, 'administer users');
    +  foreach (array_keys($user_admin_roles) as $rid => $role) {
    +    user_role_grant_permissions($rid, array('administer user settings'));
    +  }
    

    Did anyone test this update function?

    Because it doesn't look right. The keys of array_keys() are literally just sequential numbers like 0,1,2... so won't this assign the new permission to random roles on the site, potentially even including anonymous users (which have a role ID of 1)? I would think the => $role part shouldn't be there.

  3. +  // Make sure the new 7019 update from drupal-7.50 is also applied.
    +  user_update_7019();
    

    I don't understand why this is here.

  4. Following up on #178, the fact that this patch needs to make changes to other modules' tests shows one reason why it would be disruptive to contrib modules and sites. We did something similar recently (added a new permission) in #611294: Refine permissions for Field UI but the reason for that was very compelling and security-related.

    I can see why this one is nice to have in core, but is there a way we can make it an opt-in feature or something (so that by default "administer users" is still enough to continue letting you see this page)? Either an optional module (though a very small one), or a configuration setting, or something...

David_Rothstein’s picture

In Drupal 8, the permission description is this:

Configure site-wide settings and behavior for user accounts and registration. This includes account cancellation methods, the content of user emails and fields attached to users.

I haven't tested the Drupal 7 patch, but I'm not sure the last part ("fields attached to users") is true for the Drupal 7 patch - it doesn't look like it to me since https://api.drupal.org/api/drupal/modules!user!user.module/function/user... is still checking "administer users".

So should it be the permission that is checked for configuring fields also? That seems like a logical way to split it up, especially given the stated goal of this issue.

alex.skrypnyk’s picture

I've updated the patch from #185 with changes proposed in #193. We may still need to do more work or even a separate module, but at least this patch can be used until then.

eiriksm’s picture

Status: Needs work » Needs review
FileSize
5.95 KB

OK, so even more in line with #193:

- Made the feature opt-in.
- No update hook (so no need to test that)
- No changes to other tests (meaning no changes to contrib tests are needed).

So should be 100% backwards compatible.

Also, regarding #194: This is correct. Also incorporated this in the patch.

Status: Needs review » Needs work

The last submitted patch, 196: administer_users_-366950-196.patch, failed testing.

eiriksm’s picture

Status: Needs work » Needs review

Back to needs review after random test failure

The last submitted patch, 195: drupal-administer_users_permission-366950-195.patch, failed testing.

  • alexpott committed 73c2194 on 8.4.x
    Issue #366950 by amontero, boombatower, brianV, univate, hefox,...

  • alexpott committed 73c2194 on 8.4.x
    Issue #366950 by amontero, boombatower, brianV, univate, hefox,...
Alan D.’s picture

joegraduate’s picture

Status: Needs review » Reviewed & tested by the community

Patch #196 worked well in my testing.

After enabling the "Separate administer account settings permission" setting:
- I was able to access /admin/config/people/accounts while logged in with a role that had been granted the "administer account settings" permission but not the "administer users" permission.
- I was not able to access /admin/config/people/accounts while logged in with a role that been granted the "administer users" permission but not the "administer account settings" permission.
- I was able to access /admin/people while logged in with a role that had been granted the "administer users" permission but not the "administer account settings" permission.
- I was notable to access /admin/people while logged in with a role that had been granted the "administer account settings" permission but not the "administer users" permission.
- I was able to access /admin/config/people/accounts/fields and /admin/config/people/accounts/display while logged in with a role that had been granted the "administer account settings" and "administer fields" permission but not the "administer users" permission.
- I was not able to access /admin/config/people/accounts/fields and /admin/config/people/accounts/display while logged in with a role that had been granted the "administer users" and "administer fields" permission but not the "administer account settings" permission.

joegraduate’s picture

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

Patch #196 no longer applies cleanly against 7.67 or the latest 7.x-dev when using the patch command.

joegraduate’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.93 KB

The attached patch is a straight re-roll of #196.

joegraduate’s picture

Assigned: Unassigned » joegraduate
Issue tags: +Needs issue summary update, +Global2020
joegraduate’s picture

Assigned: joegraduate » Unassigned
Issue summary: View changes
Issue tags: -Needs issue summary update
joegraduate’s picture

Issue summary: View changes
joegraduate’s picture

Issue summary: View changes
jenlampton’s picture

Status: Needs review » Reviewed & tested by the community

Patch in #206 still applies to 7.77. Back to RTBC.

izmeez’s picture

Both the patch in #206 and the patch it was re-rolled from in #196 have a minor typo in "Settings for separating the administer users and edit user settings." The description, "With this option you can have seperate permissions ..." the word "separate" is spelled wrong.

Also, even though this permission provides backward compatibility and requires opting in to separate the permissions, I wonder would a change record similar to the one for Drupal 8 (https://www.drupal.org/node/2083321) be helpful?

izmeez’s picture

This issue has not yet been added to #3192080: [meta] Priorities for 2021-04-07 release of Drupal 7. I'm not sure it's ready to add to the priorities listed there.

jenlampton’s picture

Patch here is a duplicate of #206 but with the spelling mistake corrected as per #212.

izmeez’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @jenlampton the patch in #214 looks good, applies to current Drupal 7.78 and 7.x-dev and works as expected.
If a change record is required the one for D8 is referenced in comment #212 and can be modified as needed for use.

genellann’s picture

The naming convention for patches to core is to begin the file name with "drupal-" and to keep the patch name the same except for the comment number.

Same patch as #214 with correct name.

poker10’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Novice

Thanks @genellann, but there is no need to reupload patches just because the filename is not according to the conventions.

---------

Thanks everyone for working on the D7 patch! I tried to go through the whole issue and relevant comments, but I am sorry if I have missed something.

There is one scenario, where two concrete steps worries me and that is (see steps 3 and 5):

  1. Install clean D7, create a separate non-admin user and a role with administer users permission
  2. Assign the role with the permission to the newly created non-admin user - the user now can access both admin/config/people/accounts and admin/people (this is OK)
  3. Enable the Separate administer account settings permission checkbox - the user suddenly cannot access admin/config/people/accounts. What was the reason to remove the update hook? I think if we want to be 100% safe, we should grant the new permission to all roles which have administer users once that checkbox is enabled
  4. Grant the new administer account settings permission to the role - user now can access admin/config/people/accounts again (this is OK)
  5. And then disable the checkbox Separate administer account settings permission

The step 5 is the most problematic I think. Because when you uncheck that checkbox, the permission is still granted to the role, despite the fact that it is not displayed on admin/people/permissions anymore. I am not a great fan of such leftovers in database, as these can cause subsequent issues. For example, after the admin uncheck that checkbox, this

user_access('administer account settings', $non_admin_account)

will still return TRUE, even the permission does not exists anymore. This can cause issues in custom code/contrib modules, etc.

Just to summarize, I think the most safe approach will be to grant the permission to all roles having administer users once enabled (not entirely sure why this was removed) and once disabled, we need to remove the permission from all roles. We should also update the tests to check this process.

Also found a minor nitpick (see the extra whitespace):

+    $GLOBALS['conf'] ['user_enable_separate_account_settings'] = $form_state['values']['user_enable_separate_account_settings'];

---------

According to the backport policy, this D7 part should be split to the separate issue. I am not going to do it yet, because the patch seems to be mostly ready. Let's see if there will be any response/activity after my review and we will see what next. In case we will need more discussion or patch iterations to update it, we can split it to not cause additional noise here. Thanks!

poker10’s picture

Just to clarify this a bit, when speaking about "What was the reason to remove the update hook?", I meant the process of granting the new permission to all users. I know that the update hook was not suitable after the patch was changed to opt-in, but the logic could be moved to the submit handler, not removed.