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 toadmin/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:
- needs an upgrade path and accompanying upgrade path tests (how to: http://drupal.org/node/1429136) 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.
- #1813488: Add descriptions to clarify "administer users" and "administer user settings" permissions
Steps to reproduce:
- Install the latest Drupal 8.x using the standard profile.
- Apply patch.
- Go to
admin/people/roles
and add new role "Person manager". - Go to
admin/people
and add new user with role Person manager. Also create one user for test. - Go to
admin/people
and add new user with role Person manager. - Go to
admin/people/permissions
and give that role the permission to Administer users (but not Administer user settings). - Switch to that user and edit a test user account. See that he has access to
/admin/people
and to edit users. - Go to
admin/config/people/accounts
see that this user has access denied. - Give that user additional Administer user settings permission
- Login with the user again and note differences (now should be possible to access to
admin/config/people/accounts
and to/admin/people
) - 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.
Comments
Comment #1
ceardach CreditAttribution: ceardach commentedSorry, poor spelling :(
Comment #2
redijedi CreditAttribution: redijedi commentedI'm running into this too. This seems dumb intuitive. Why aren't there separate settings? Has anyone found a work around?
Comment #3
mpaler CreditAttribution: mpaler commentedI 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.
Comment #4
ceardach CreditAttribution: ceardach commentedThis 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.
Comment #5
brianV CreditAttribution: brianV commentedInitial 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:
Is it differentiated enough from the existing 'Administer users' permisson? Should this text be changed to something different?
Tagging for Usability.
Comment #7
brianV CreditAttribution: brianV commentedSince there were only 4 fails, I adjusted the tests to account for the change.
Still need text review.
Comment #8
XanoGood idea! Nice work on the patch :)
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
.Comment #9
brianV CreditAttribution: brianV commentedAfter conversing with Xano on IRC, we determined
would be most consistent with the wording in the rest of core.
New patch reflects this wording.
Comment #10
catchThis 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.
Comment #11
brianV CreditAttribution: brianV commented@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.
Comment #12
starscream CreditAttribution: starscream commentedI'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?
Comment #14
tstoecklerBroken TestBot?
Comment #16
BassPlaya CreditAttribution: BassPlaya commentedcheck this out: http://drupal.org/project/protect_critical_users
Comment #17
izmeez CreditAttribution: izmeez commentedYes, it would be great to have "profile editing" and "user settings" under separate permissions rather than included with "administer user"
Izzy
Comment #18
tchurch CreditAttribution: tchurch commentedsubscribing.
Comment #19
petednz CreditAttribution: petednz commentedso did all the hard work up to #9 come to nought - or is the patch useable? cheers
Comment #20
Alan.Guggenheim CreditAttribution: Alan.Guggenheim commentedI 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.
Comment #21
robbdavis CreditAttribution: robbdavis commentedIt 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
Comment #22
tchurch CreditAttribution: tchurch commentedAlso 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.
Comment #23
timofey CreditAttribution: timofey commentedupdated link
http://drupal.org/project/issues/user_settings_access?status=All&categor...
Comment #24
catchThis is too late for D7 now, moving to 8.x.
Comment #25
tchurch CreditAttribution: tchurch commentedI'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).
Comment #26
DuaelFr#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.
Comment #27
boombatower CreditAttribution: boombatower commentedRerolled.
Comment #28
boombatower CreditAttribution: boombatower commentedAnd d7 patch.
Comment #30
boombatower CreditAttribution: boombatower commentedFrom 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.
Comment #32
boombatower CreditAttribution: boombatower commentedFrom test:
So that test needs the 'administer users' permission as well (not both tests).
Comment #33
boombatower CreditAttribution: boombatower commentedComment #34
amonteroAttempted 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.
Comment #36
amonteroFixing testUserPictureAdminFormValidation test error.
Comment #37
amonteroComment #38
tstoecklerLooks 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.
Comment #39
amonteroThanks, 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 :)
Comment #40
amonteroHad 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.
Comment #41
tstoecklerJust 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.
Comment #42
amonteroSorry 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.
Comment #43
tstoecklerNothing to be sorry about. :)
I justed wanted to clarify in order to not stop anyone from reviewing this because of me.
Comment #44
amonteroPatch became stale. Reroll and bump. Please review.
Comment #46
amonteroDoh. Added missing access check for user fields UI.
Not sure if entity related testbot failure has anything to do with this.
Comment #48
amonteroAdding permission for $admin_user in tests.
Comment #50
amontero#48: 366950--administer-users-permission-should-be-separate-from-user-settings--48.patch queued for re-testing.
Comment #51
tstoecklerThis 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.
Comment #52
amonteroLooks like reasonable, let's try. Reroll.
Comment #53
amonteroComment #55
tstoecklerOk, 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.
Comment #56
amonteroHi 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.
Comment #57
amonteroComment #59
tstoecklerThis 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.
Comment #60
amonteroI 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.
Comment #61
amonteroComment #63
amonteroI think that #59.2 you pointed is due to bad testing, fixing FieldUI tests.
Comment #64
tstoecklerI 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.)
Comment #65
tstoecklerThe 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.
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!
Comment #66
amonteroHi 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.
Comment #67
tstoecklerI 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.
Comment #68
amonteroSo, 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
Comment #69
tstoecklerActually 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) :-)
Comment #70
Rob C CreditAttribution: Rob C commentedAny 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)
Comment #71
hefox CreditAttribution: hefox commentedI'm taking a stab of it. I got the hunk #1 fixed. Was working on the upgrade path when a sudden sushi trip happened.
Comment #72
hefox CreditAttribution: hefox commentedAdded 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.
Comment #73
amonteroManually 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.
Comment #74
Rob C CreditAttribution: Rob C commentedLooks 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.
Comment #75
YesCT CreditAttribution: YesCT commentedadding 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
Comment #76
YesCT CreditAttribution: YesCT commentedadding tag.
Also, patch in #73 has changes to Tests. Were any new tests added that I missed? Are any new tests needed?
Comment #77
amontero@YesCT: What tests do you mean? A plain diff of patches #72 & #73 does not show changes in tests.
Comment #78
amontero#73: 366950--administer-users-permission-should-be-separate-from-user-settings--73.patch queued for re-testing.
Comment #80
amonteroTagging
Comment #81
amonteroReroll for the testbot. Manual tests will follow if green.
Comment #83
amonteroOps, fixing typo. Reroll for the testbot. Manual tests will follow if green.
Comment #84
tstoecklerThis 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.
Comment #85
amontero#83: drupal--366950--administer-users-permission-should-be-separate-from-user-settings-83.patch queued for re-testing.
Comment #87
YesCT CreditAttribution: YesCT commentedtagging for reroll
Comment #87.0
YesCT CreditAttribution: YesCT commentedUpdated issue summary with remaining steps to help new people
Comment #87.1
univate CreditAttribution: univate commentedCreate issue summary.
Comment #88
univate CreditAttribution: univate commentedRe-rolled patch from #83
Comment #90
univate CreditAttribution: univate commentedNot sure the issue with that patch - trying again.
Comment #92
univate CreditAttribution: univate commented#90: adminster_users_366950-90.patch queued for re-testing.
Comment #94
univate CreditAttribution: univate commentedThe 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.
Comment #95
univate CreditAttribution: univate commentedComment #96
babruix CreditAttribution: babruix commented(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
Comment #97
babruix CreditAttribution: babruix commentedTrying to improve summary to accurately describe the current state of that issue and what needs to be done moving forward.
Comment #98
tstoecklerJust to reiterate, this still needs upgrade path tests.
Comment #98.0
tstoecklerMissing credit to original reporter
Comment #99
babruix CreditAttribution: babruix commentedUpdated 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?
Comment #100
YesCT CreditAttribution: YesCT commented@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)..
Comment #100.0
YesCT CreditAttribution: YesCT commentedupdated summary of what is left, removed completed tasks
Comment #100.1
amonteroAdding followup issue from comments
Comment #101
babruix CreditAttribution: babruix commentedPermissions page: before and after patch.
Comment #102
babruix CreditAttribution: babruix commentedAs user with "Administer user settings" permission.
Comment #103
babruix CreditAttribution: babruix commentedAs user with "Administer users" permission.
Comment #103.0
babruix CreditAttribution: babruix commentedadded steps for reproduce
Comment #104
stpaultim CreditAttribution: stpaultim commentedComment #105
stpaultim CreditAttribution: stpaultim commentedMaryann and I just re-rolled this patch as part of Drupal Sprint Weekend. Hope all is well.
Comment #106
shariharan CreditAttribution: shariharan commentedUser with just "Administer users "
Comment #107
shariharan CreditAttribution: shariharan commentedUser with just "Administer user settings "
Comment #108
disasm CreditAttribution: disasm commentedGood 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?
Comment #109
YesCT CreditAttribution: YesCT commentedlooking at #101
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/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.
Comment #110
shnark CreditAttribution: shnark commentedI made the change that YesCT recomended in comment #109
I changed administer user settings to administer account settings.
Comment #111
shnark CreditAttribution: shnark commentedComment #112
bookmarvel CreditAttribution: bookmarvel commentedi took screenshots before and after.
Comment #113
YesCT CreditAttribution: YesCT commentedupdating title
Comment #114
YesCT CreditAttribution: YesCT commented@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.
Comment #115
sunYes, 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:
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!
Comment #116
YesCT CreditAttribution: YesCT commentedThose are well described changes that need to be made. A good novice task. (adding tag)
Comment #117
amonteroRebasing. Feed the testbot. The menu item permission has been moved to a YAML, hope it works.
Will give it a shot if passes.
Comment #118
amonteroI swear I didn't removed the tag :)
Comment #119
amonteroFor the record and as a 'manual interdiff', the only code needing manual merging was this block:
Since the menu functionality changed, modification to user_menu function is no longer needed. Instead, I've added this:
The remaining patch modifications rebased successfully.
Comment #120
amonteroBased 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.
Comment #121
tchurch CreditAttribution: tchurch commentedThis 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.
Comment #122
amonteroHi 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.
Comment #122.0
amonteroremoved completed tasks
Comment #123
amonteroPatch became stale. Chasing HEAD.
As result of #1875992: Add EntityFormDisplay objects for entity forms:
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.
Comment #124
amonteroPatch 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.
Comment #125
amonteroTagging to highlight hook_update reordering to CMI interested parties before we leave alpha.
Comment #126
amontero#124: drupal--366950--administer-users-permission-should-be-separate-from-user-settings--124.patch queued for re-testing.
Comment #128
amonteroReroll of #124, since it became stale.
Comment #129
amonteroDoh.
Comment #130
tstoecklerStill looks good, but still needs upgrade path tests.
Comment #131
amonteroFirst stab at the UP tests. It's #128 without the upgrade hook plus these test files:
It's meant to fail.
Comment #133
amonteroFails 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.
Comment #134
tstoecklerYeah, @amontero!
Tests look great.
Comment #135
alexpottNeeds a reroll
Comment #136
amonteroRebasing against HEAD.
Comment #138
amontero#136: drupal--366950--administer-users-permission-should-be-separate-from-user-settings--136.patch queued for re-testing.
Comment #140
amonteroLooks 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 (?).
Comment #141
star-szr#136: drupal--366950--administer-users-permission-should-be-separate-from-user-settings--136.patch queued for re-testing.
Comment #142
amonteroThe reroll passed tests OK. (!?)
RTBCing again.
Comment #143
chx CreditAttribution: chx commentedNote: 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.
Comment #144
alexpottMinor nit.
$rids = array();
is not needed since $rids is assigned on the next lineComment #145
amonteroDone.
Comment #146
tstoecklerStill looks good.
Comment #147
alexpottuser_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.
Comment #148
amonterouser_acces removed. Link is helpful, IMO.
Comment #149
tstoecklerComment #151
tstoeckler#148: drupal--366950--administer-users-permission-should-be-separate-from-user-settings--148.patch queued for re-testing.
Comment #152
tstoecklerI don't think line 51 of FeedProcessorPluginTest is actually related to this...
Comment #153
amontero#148: drupal--366950--administer-users-permission-should-be-separate-from-user-settings--148.patch queued for re-testing.
Comment #154
alexpottPatch no longer applies.
Comment #155
babruix CreditAttribution: babruix commentedRe-rolled.
Comment #157
amonteroAbove patch is not only a reroll. It contains changes:
1) function user_validate_name($name)
2)
(also the indenting is incorrect)
3) and several:
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 .
Comment #158
alexpottCommitted 73c2194 and pushed to 8.x. Thanks!
We need a change notice for this for site builders for this.
Comment #159
amonteroYay!!
Un-postponed #1813488: Add descriptions to clarify "administer users" and "administer user settings" permissions.
Comment #160
catchComment #161
mtiftI created https://drupal.org/node/2083321
Comment #162
mtiftComment #163
star-szrThanks @mtift :)
Comment #164
tstoecklerWow, epic issue. Thanks for everyone who stuck with this!
Comment #165
amonteroShouldn'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
Comment #166
tstoecklerI 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.
Comment #167
amonteroDone. 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
Comment #168.0
(not verified) CreditAttribution: commentedLink to "User Settings Access" module.
Comment #170
jenlamptonAnd for those of us who would like this feature on D7, here's a patch for that too.
Comment #171
sinasalek CreditAttribution: sinasalek commentedBackport is definitely needed
Comment #172
eiriksm7.x patch looks good to me.
Comment #174
zaporylieI've added hook_update to user.install to set new permission up by default, based on 'administer users' permission.
Comment #176
zaporylieTests should be fixed now.
Comment #177
eiriksmLooking good!
Comment #178
David_Rothstein CreditAttribution: David_Rothstein commentedComment #179
xjmComment #180
eiriksmOK, 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!
Comment #181
eiriksmForgot to change status.
Comment #182
justaman CreditAttribution: justaman as a volunteer commentedsmall 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');
}
}
}
Comment #183
MaskOta CreditAttribution: MaskOta commentedI 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
Comment #184
IRuslan CreditAttribution: IRuslan as a volunteer and at DrupalJedi commented#180 works for me and looks good.
+1 RTBC.
Comment #185
PascalAnimateur CreditAttribution: PascalAnimateur commentedUpdated 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.Comment #188
IRuslan CreditAttribution: IRuslan as a volunteer and at DrupalJedi commentedI guess it should be Fixed as already committed.
Comment #189
eiriksmIt is fixed in Drupal 8, we are trying to get it fixed in D7 :)
Comment #191
andrewfn CreditAttribution: andrewfn as a volunteer commented#185 works for me and solves a very important problem. I'm surprised it this is not committed yet.
Comment #192
andrewfn CreditAttribution: andrewfn as a volunteer commentedComment #193
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedDid 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.I don't understand why this is here.
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...
Comment #194
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIn Drupal 8, the permission description is this:
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.
Comment #195
alex.skrypnykI'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.
Comment #196
eiriksmOK, 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.
Comment #198
eiriksmBack to needs review after random test failure
Comment #202
Alan D. CreditAttribution: Alan D. commented#987978: Move "administrator role" setting to new Role Settings form is related to this too.
Comment #204
joegraduatePatch #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.
Comment #205
joegraduatePatch #196 no longer applies cleanly against 7.67 or the latest 7.x-dev when using the patch command.
Comment #206
joegraduateThe attached patch is a straight re-roll of #196.
Comment #207
joegraduateComment #208
joegraduateComment #209
joegraduateComment #210
joegraduateComment #211
jenlamptonPatch in #206 still applies to 7.77. Back to RTBC.
Comment #212
izmeez CreditAttribution: izmeez commentedBoth 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?
Comment #213
izmeez CreditAttribution: izmeez commentedThis 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.
Comment #214
jenlamptonPatch here is a duplicate of #206 but with the spelling mistake corrected as per #212.
Comment #215
izmeez CreditAttribution: izmeez commentedThanks @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.
Comment #216
genellann CreditAttribution: genellann commentedThe 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.
Comment #217
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedThanks @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):
administer users
permissionadmin/config/people/accounts
andadmin/people
(this is OK)Separate administer account settings permission
checkbox - the user suddenly cannot accessadmin/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 haveadminister users
once that checkbox is enabledadminister account settings
permission to the role - user now can accessadmin/config/people/accounts
again (this is OK)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, thiswill 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):
---------
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!
Comment #218
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedJust 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.