Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.To duplicate:
- In a fresh install, add one or two new roles.
- Delete the default administrator role (i.e. as a precaution against hackers if you don't need it).
- Visit
admin/config/people/accounts, in the Administrator role settings it seems like your current administrator role is the first custom role you created. This may be confusing and panicking for users.
It takes a few minutes to test this on simplytest.
When the related code is inspected it seems like actually the administrator role is disabled and testing verifies this.
But I guess because of array keys' ordering, the default value is not working:
$roles = user_roles();
unset($roles[DRUPAL_ANONYMOUS_RID]);
unset($roles[DRUPAL_AUTHENTICATED_RID]);
$roles[0] = t('disabled');
dpm($roles);
dpm(variable_get('user_admin_role', 0));
$f['user_admin_role'] = array(
'#type' => 'select',
'#default_value' => variable_get('user_admin_role', 0),
'#options' => $roles,
);
dpm(drupal_render($f));
output: <option value="4">role1</option><option value="5">role2</option><option value="0">disabled</option> nothing selected.
Adding a ksort($roles) solves the problem (sorry no time to produce the patch).
| Comment | File | Size | Author |
|---|---|---|---|
| #17 | interdiff.txt | 584 bytes | gaurav.goyal |
| #17 | after_deletion_of-2364629-17.patch | 1.91 KB | gaurav.goyal |
| #13 | after_deletion_of-2364629-13.patch | 1.92 KB | cilefen |
| #13 | after_deletion_of-2364629-13-test-only.patch | 1.4 KB | cilefen |
| #13 | interdiff.txt | 1.34 KB | cilefen |
Comments
Comment #1
dcam CreditAttribution: dcam commentedThank you for this report!
This bug doesn't apply to D8. The Admin role setting is part of the role config entity. When the selected admin role is deleted, the setting is deleted along with it and the select list will display the #empty_option.
Contrary to what is suggested in the OP, the proper solution isn't necessarily to sort the roles. We need to delete the user_admin_role variable in user_role_delete() if the selected role is deleted. With that done, the select list will display "disabled" by default.
I say we probably don't need to sort the roles because I'm not sure. If someone takes it upon themselves to delete the role manually from the database, then they'll end up with the same problem. Since that would be a pretty stupid thing to do I doubt we need to worry about it.
Either way, both of these edits are easy to do, so this is a good Novice issue. Also, this will need tests, but that shouldn't be hard to write either.
This is a minor security bug though and a member of the security team will need to weigh in on whether we need to sort the select options. The reason it's a security bug is that someone could delete the selected admin role, then go to /admin/config/people/accounts, edit some other setting on that form, and save without paying attention to the fact that they just gave some random role admin permissions. Since this is a really one-off scenario, could only result in authenticated users getting admin rights, and then only for modules enabled after the save (as I understand the setting), I think we can take care of this in the core issue queue. So I've not posted in the security team's queue.
Comment #2
Mirakolous CreditAttribution: Mirakolous commentedComment #3
Mirakolous CreditAttribution: Mirakolous commentedComment #4
gaurav.goyal CreditAttribution: gaurav.goyal commentedComment #5
gaurav.goyal CreditAttribution: gaurav.goyal at Innoraft commentedInstead of sorting the array, we should delete the user_admin_role variable because this variable contains the role id of the administrator role which is now deleted.
I have created the patch for this, and tested.
Comment #6
rashidkhan CreditAttribution: rashidkhan as a volunteer commentedIt's working, It deletes the user admin role variable if the user role is deleted and is not just disabled.
Comment #7
rashidkhan CreditAttribution: rashidkhan as a volunteer commentedComment #8
dcam CreditAttribution: dcam commentedI haven't had time to do a proper review of this patch yet, but I can already tell you that it needs work because this issue is tagged as needing tests.
Comment #9
cilefen CreditAttribution: cilefen commentedHas anyone checked if this issue exists in Drupal 8?
Comment #10
dcam CreditAttribution: dcam commentedYes. See #1.
Comment #11
dcam CreditAttribution: dcam commentedComment #12
cilefen CreditAttribution: cilefen commented@dcam That is my fault for not reading - thanks.
Comment #13
cilefen CreditAttribution: cilefen commentedComment #15
dcam CreditAttribution: dcam commentedI have a couple of minor nitpicks.
The grammar in this sentence needs to be improved. I suggest
I'm 100% sure it doesn't matter in terms of functionality, but I wonder why variable_get() is being made to return 0 if the variable doesn't exist. I think it would be best just to leave off the second parameter and let variable_get() return NULL as a default.
The test looks good to me. Thank you for adding it, @cilefen!
Comment #16
dcam CreditAttribution: dcam commentedComment #17
gaurav.goyal CreditAttribution: gaurav.goyal at Innoraft commented@dcam,
Thanks for the review. Issues fixed.
Comment #18
dcam CreditAttribution: dcam commentedThanks @gaurav.goyal! You've done good work on this issue. Assuming #17 comes back green it is RTBC.
Comment #19
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted to 7.x - thanks!