Problem/Motivation

The following happened on the production site of one of the Drupal Dojo Austin attendees. Two persons persons were unable to get access to any of the admin menu items while someone else had a greatly truncated list of admin menu items. The debugging and finding the root cause took a day or two. During that time nothing else got done nor anyone could access the site. In the end it turned out that the attendee changed the administration role on admin/people/role-settings by accident to content editor. That caused that all permissions for the content editor role got checked and greyed out while all administrator role permissions got unchecked. I've illustrated the scenario in the attached video: https://www.drupal.org/files/issues/2022-07-01/lockout.mov (*sorry for the long pageloads but the combination of a screen recording while docker is running is demanding for my old computer). The issues illustrated in the attached video are:

- If you have a user with the administrator and authenticated user role and you change the administrator role as this user from administrator to content editor you get forwarded to an Access denied page. You are completely locked out of the admin menu.
Let's assume the worst case scenario you have a site with a group of four admin accounts and one super admin account. The super admin is on vacation or on a business trip or something else happened to the person so that he or she is unavailable for several days or weeks. Now one of those four accounts changes the role in the Role settings by accident as happened above. The four admin users would be completely locked out. Or in case #540008: Add a container parameter that can remove the special behavior of UID#1 lands there wouldn't be any super admin account able to correct the oversight even and regain access would be even more complicated.

- The description of the select box on the Role settings page is incorrect and creates false expectations:
This role will be automatically assigned new permissions whenever a module is enabled. Changing this setting will not affect existing permissions.
The statement that changing this setting will not affect existing permissions applies to every role except the administrator role. Problem with the administrator role in the standard profile - on install it is set as the administration role and every checkbox is selected. If you change now the administrator role on the Role settings page the permissions of the administrator role get reset to its defaults. If you take a look at the default permissions assigned in the standard profile: https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/profiles/sta... it is clear why after changing the administrator role on the role settings page all checkboxes are unchecked. Since the administrator role is the role that is assigned initially for the standard profile install and changed later on it is a potential source of confusion and problems.

Steps to reproduce

- Create a new user and assign the authenticated and administrator role (aside user #1)
- Log in with that new admin user
- Got to admin/people/role-settings
- Change the administrator role from administrator to content editor
- You will get an Access denied page and be locked out completely

I've checked in Drupal 9.4.1 and Drupal 10.0.x-dev. The problem applies to both versions.

Proposed resolution

- One option would be to add a confirmation dialogue asking the user if he or she is sure about the step and the possible consequences.
- Another level of safety would be to test if the user applying the change on the Roles settings page has still the permission administer roles and permissions in one of his/her roles. That way even if the user lacks permissions to see the admin menu and admin page it would be at least possible still to access the Role settings page:
roles settings page shown in olivero - a user with only the administer roles and permissions ability
That way the user would see immediately something is off and is still able to correct the mistake on his or her own.
- Depending on the changes applied the description underneath the select box on the Role settings page needs an adjustment.

Addendum: The issue was discovered and discussed during the Drupal Dojo in Austin over the course of two or three consecutive weeks. It would be good if someone could provide @cutehair and @rocketeerbkw an issue credit since both helped discovering and understanding this as well as the issue linked as a related issue.

CommentFileSizeAuthor
administer_roles_and_permissions.png104.69 KBrkoller
lockout.mov18.97 MBrkoller

Comments

rkoller created an issue. See original summary.

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented
Issue summary: View changes
aaronmchale’s picture

aaronmchale
Primary language English
Location Edinburgh, Scotland
commented
Issue tags: +Needs usability review

We could definitely work through this at a usability meeting, I have some ideas of how we might mitigate this.

Queued for review at #3300912: Drupal Usability Meeting 2022-08-05 or a future meeting.

benjifisher credited cutehair.

benjifisher credited rocketeerbkw.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented
rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented

We've talked about this issue at #3302079: Drupal Usability Meeting 2022-08-12. It will have a link to the recording and transcript of the meeting. For the record the attendees at the meeting were @antoniya, @benjifisher, @shaal, @simohell, @worldlinemine, and me.

We all agreed on that there should be a confirmation form before applying the change.

How to handle and prevent a possible lockout, aside the confirmation dialogue, we have not reached a consensus yet. The two options we ended up with are as follows:

1) auto assign the new administrator role to the person making the change: If a user has the authenticated user and administrator role and changes the adminstratore role to content editor on the role settings page that content editor role gets automatically assigned to the user. that way it would be ensured that at least one user has the new administrator role.

in the context of the auto assign there came up the edge case about the need that the user making the change doesn't want to remain admin. so it was suggested to add an opt-out option to the confirmation dialogue.

2) disable roles the user isn't assigned to on the roles settings page: to ensure that changing the administrator role is a deliberate step, only the roles the user making the change is member of should be enabled/selectable, the rest of the roles should be disabled. the description should point the user to the user profile page and that the role the administration role is changed to has to been added to his or her account first there. It is made cumbersome intentionally. That way it would be ensured that the change doesn't happen by accident and or the user is even getting locked out.

We then all agreed on that the description on the roles settings page definitely has to be updated and also being adjust to whatever solution is chosen. Either one of the two aforementioned or something else.

Then there are two more points that came up during the discussion that should be mentioned. They are probably out of the scope for this issue but for the record and the creation of a potential follow up I add them here:

a) if the administrator role is getting changed then there should be a notification and or email to the users that belonged to that old role as well. Otherwise that change (by accident or deliberate by a bad actor) might go unnoticed for a few hours or even days.

b) currently there is the option -none- on the role settings page. if chosen there is no administrator role available anymore to anyone. as soon as #540008: Add a container parameter that can remove the special behavior of UID#1 lands it might complicate things as well. there is also no confirmation nor any status message after the change took place.

I keep the tag needs usability review since we haven't completely finished the discussion and we also haven't reached a consensus in every aspect yet. so it makes sense to revisit the issue in a future ux meeting after some discussion happened in here.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

@rkoller, thanks for summarizing the discussion.

in the context of the auto assign there came up the edge case about the need that the user making the change doesn't want to remain admin. so it was suggested to add an opt-out option to the confirmation dialogue.

Another option is to do this after the confirmation form is submitted. The status message could tell the user that the role has been added, with a link to the user-edit page. In this edge case, they can follow the link and remove the newly added role.

We then all agreed on that the description on the roles settings page definitely has to be updated ...

The current text is not accurate: " This role will be automatically assigned new permissions whenever a module is enabled. Changing this setting will not affect existing permissions."

aaronmchale’s picture

aaronmchale
Primary language English
Location Edinburgh, Scotland
commented

Regarding the options in #7, I am definitely a fan of option 1 - "auto assign the new administrator role to the person making the change" - I think this is definitely better than option 2 - "disable roles the user isn't assigned to on the roles settings page".

I see a couple of problems with disabling roles that the user currently does not have:

  1. It's inconvenient: if you're an admin user, you probably only have the current admin role assigned to you, since there's not really a need to have any other roles assigned, so in almost all cases this would create an inconvenient step that the user then has to go through; In that regard option 1 is definitely more efficient and less frustrating for the user.
  2. Accessibility issues: If I remember correctly, form elements marked as disabled can be problematic because sometimes there isn't enough visual distinction between those and non-disabled form elements. I did a quick search to find a source for this but nothing obvious came up, this is just from memory.
  3. It could be confusing: Even if we provide help text, the user might not notice or read the help text, they might get confused or frustrated as to why some roles are disabled and other roles are not.

in the context of the auto assign there came up the edge case about the need that the user making the change doesn't want to remain admin. so it was suggested to add an opt-out option to the confirmation dialogue.

Regarding this, I'd suggest we avoid provide any opt out feature, I would guess that it's probably a quite uncommon edge case, and if we did introduce that, it still presents a route to where the user could end up back in the same situation of being "locked out". For instance, maybe they tick the opt out box but after submitting the form realise they forgot to assign the new admin user. I think given how risky changing the admin role could be, I think we should air on the side of caution.

a) if the administrator role is getting changed then there should be a notification and or email to the users that belonged to that old role as well. Otherwise that change (by accident or deliberate by a bad actor) might go unnoticed for a few hours or even days.

That feels like a follow-up issues to me, better to have a space to properly discuss if that's something we want and what that should look like and how it shoudd behave. Probably sensible given how long it's taking to get another email related issues who shall remain nameless done, let's not weigh this issue down by introducing too much complexity here.

currently there is the option

-none-

on the role settings page. if chosen there is no administrator role available anymore to anyone. as soon as #540008: Add a container parameter that can remove the special behavior of UID#1 lands it might complicate things as well. there is also no confirmation nor any status message after the change took place.

I would leave to #540008: Add a container parameter that can remove the special behavior of UID#1 to address, either in that issues or as a follow-up, probably worth mentioning it over there if it hasn't already been discussed.

Re @benjifisher #8

in the context of the auto assign there came up the edge case about the need that the user making the change doesn't want to remain admin. so it was suggested to add an opt-out option to the confirmation dialogue.

Another option is to do this after the confirmation form is submitted. The status message could tell the user that the role has been added, with a link to the user-edit page. In this edge case, they can follow the link and remove the newly added role.

I like the idea of having a confirm form that appears after you submit the role settings form, like we do for deleting entities. Such a form could ask the user to confirm they definitely want to make the change and explain what will happen, at the point it would even know which roles is the new admin role, so could be something like:

<ul>
  <li>You are changing the administrator role from <strong>Administrator</strong> to <strong>Content Editor</strong>.</li>
  <li>Content Editor will be <strong>automatically assigned all permissions</strong>, as a result any users who already have the role <em>Content Editor</em> will automatically be given all permissions on the site.</li>
  <li>If you are not currently assigned the role <em>Content Editor</em>, <strong>Content Editor will automatically be given to you</strong>.</li>
  <li>The role <strong>Administrator will no longer have any permissions on the site</strong>, after confirming this change you can go to the Permissions page to assign permissions to <em>Administrator</em>.</li>
</ul>
rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented

We've discussed #3293855: Update the outdated user_help text for user.admin_permissions and the description of the select box on the role settings page during #3306259: Drupal Usability Meeting 2022-09-02. The consensus was to extend the scope for #3293855: Update the outdated user_help text for user.admin_permissions and the description of the select box on the role settings page by also updating the description of $form['admin_role']['user_admin_role'] in RoleSettingsForm.php. in a brief follow up conversation in the #ux channel on the drupal slack we agreed not to update this issue yet - just leaving a comment but not to remove the task until #3293855: Update the outdated user_help text for user.admin_permissions and the description of the select box on the role settings page is actually fixed.

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented

re @AaronMcHale #9

the longer i think about the more i think option 1 might be the better choice to go with. less error prone than option 2. also in the context of the possible issues you might run into you've listed.

Regarding this, I'd suggest we avoid provide any opt out feature, I would guess that it's probably a quite uncommon edge case, and if we did introduce that, it still presents a route to where the user could end up back in the same situation of being "locked out". For instance, maybe they tick the opt out box but after submitting the form realise they forgot to assign the new admin user. I think given how risky changing the admin role could be, I think we should air on the side of caution.

100% agree

That feels like a follow-up issues to me, better to have a space to properly discuss if that's something we want and what that should look like and how it shoudd behave. Probably sensible given how long it's taking to get another email related issues who shall remain nameless done, let's not weigh this issue down by introducing too much complexity here.

good point with the complexity and the time the issue you've linked took. would you mind open up a follow up issue or shall i? and in regards of the uid #1 issue i will leave another comment noting the detail about the -none- option and the possible complications.

and a +1 to the idea in #8 having the confirm form after the role selection form was submitted. I also like your draft in #9. A few additional thoughts about the draft:

Content Editor will be automatically assigned all permissions, as a result any users who already have the role Content Editor will automatically be given all permissions on the site.

i wonder if it would make sense to articulate if the user is really sure about the implications? meaning perhaps adding the recommendation to first check the list of all users currently having the content editor role and if those persons are trustworthy enough? maybe even output a list of those users in the confirmation form in case it isnt too long. in contrast if more than for example 20(?) users become administrators by the change output something like 23 users including you will gain administrative privileges?

If you are not currently assigned the role Content Editor, Content Editor will automatically be given to you.

would it make sense to show that line only in case the user doesnt have the new role yet and rephrase the sentence to something like You are currently not assigned to the Content Editor role. Therefore the Content Editor role will be automatically given to you.

The role Administrator will no longer have any permissions on the site, after confirming this change you can go to the Permissions page to assign permissions to Administrator.

that applies only to the case the site is installed with the standard profile and the initial administrator role never got changed. but would it make sense to consider a different output if the active administration role is != Administrator. because in that case "will no longer have any permissions on the site" does not apply?

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented

one brief update i've skimmed through the UID#1 issue again searching if the -none- option was covered. it was discussed for the case when removing the UID#1 (from my understanding). so i guess the case was mentioned and covered in the issue. so dealing with the case changing it to -none- in the scope of the roles settings page i guess this issue here would be the best place to deal with and fix it. so i think it wouldn't be necessary anymore to leave a comment in regards of the -none- option and this issue here in general.

aaronmchale’s picture

aaronmchale
Primary language English
Location Edinburgh, Scotland
commented

Re #11:

i wonder if it would make sense to articulate if the user is really sure about the implications? meaning perhaps adding the recommendation to first check the list of all users currently having the content editor role and if those persons are trustworthy enough? maybe even output a list of those users in the confirmation form in case it isnt too long. in contrast if more than for example 20(?) users become administrators by the change output something like 23 users including you will gain administrative privileges?

I like that idea you suggested to display the number of users who have that role, it could be a good catch in case the user selected the wrong role, showing the number of users that have the selected role might make the user realise that they did not mean to select a role which (in your example) has 23 others assigned to it. Maybe we could even link to a filtered view of the user list showing all the users who have the selected role.

would it make sense to show that line only in case the user doesnt have the new role yet and rephrase the sentence to something like You are currently not assigned to the Content Editor role. Therefore the Content Editor role will be automatically given to you.

I agree, that makes sense.

that applies only to the case the site is installed with the standard profile and the initial administrator role never got changed. but would it make sense to consider a different output if the active administration role is != Administrator. because in that case "will no longer have any permissions on the site" does not apply?

Good catch! We could implement a check that looks at what permissions the role has assigned in it's role config object, as you say in the standard profile that list should be empty so we can show that text, but then show something different if the number of permissions is greater than 0.

Re #12

one brief update i've skimmed through the UID#1 issue again searching if the -none- option was covered. it was discussed for the case when removing the UID#1 (from my understanding). so i guess the case was mentioned and covered in the issue. so dealing with the case changing it to -none- in the scope of the roles settings page i guess this issue here would be the best place to deal with and fix it. so i think it wouldn't be necessary anymore to leave a comment in regards of the -none- option and this issue here in general.

Yeah if we simply remove the -none- option as part of this issue that is probably in the scope of this issue, since we're talking about basically making the Role Settings form more robust.

Once we're settled on the wording and ironed out edge cases, it would probably be a good idea to bring this back to the Usability group for sign off.

rkoller’s picture

rkoller
he/him
Primary language German
Location Nürnberg, Germany
commented

I like that idea you suggested to display the number of users who have that role, it could be a good catch in case the user selected the wrong role, showing the number of users that have the selected role might make the user realise that they did not mean to select a role which (in your example) has 23 others assigned to it. Maybe we could even link to a filtered view of the user list showing all the users who have the selected role.

i like the idea to even show the user names alongside the number. would you display it on a dedicate page, a modal or inline?

Good catch! We could implement a check that looks at what permissions the role has assigned in it's role config object, as you say in the standard profile that list should be empty so we can show that text, but then show something different if the number of permissions is greater than 0.

+1 for the check. one suggestion for your initial draft. the sentence was quite long. i've simply cut it in half and rephrased it a bit. and i've added a rough draft for the other case as well:

Administrator role installed from the standard profile no changes to the administrator role yet:
After confirming this change the Administrator role will have no permissions anymore. You are able to reassign specific permissions on the Permissions page.

Any role that became the Administrator role with a number of permissions greater than zero:
After confirming this change the [name] role will be reset to permissions it had before becoming the Administrator role.

Yeah if we simply remove the -none- option as part of this issue that is probably in the scope of this issue, since we're talking about basically making the Role Settings form more robust.

then let's do the removal of the -none- option within this issue

Once we're settled on the wording and ironed out edge cases, it would probably be a good idea to bring this back to the Usability group for sign off.

as soon as we agreed on those details we should upgrade the issue summary first. but then the issue should be in a good shape to bring it back to the usability group. i agree.

saxmeister’s picture

saxmeister
he/him
Location North Carolina
commented

I would definitely add that a best practice that has been adopted is the removal of user #1/admin as a way of preventing site hacks. So any checks would need to factor that in and not default to UID #1.

One possible solution is:
1. Place a warning above the drop-down box.
2. Add an extra step to warn the user what they are doing.
3. If the user is already Admin then continue to keep their user as Admin to at least allow the option of resetting this or they would otherwise be locked out and would have to depend on reimporting the database or reimporting site configuration to fix the issue.

Also, adding a Drush command that would allow this value to be updated would help. A new command under the drush user commands would be ideal.

Version: 10.0.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.