Move "administrator role" setting to new Role Settings form

i wouldn't say it doesn't makes sense...

i'm okay with both options

Makes sense to me, but the change will have to be made to D8 first. I think it's ok to file this against 8.0.x since we're just talking about moving an existing setting to another page, but I'm not sure.

I don't know that this is backportable. It seems like we probably can't change an existing core form, even just to move it to another page. Someone else will have to weigh in.

I'm just bumping this as it exposes a possible issue with escalation the users rights in the system.

i.e. you can set the Admin role to xyz (any role you have), wait for someone to add a new module and exploit whatever new permissions that have been granted.

This is actually is a serious hidden backdoor on some sites if you rely on Admin users + Role delegation, or similar, to limit access.

Note this is not covered by Drupal security policies due to:

"Any other permission that is defined with "restrict access" set to TRUE"

Or should another issue be created to address this on the side?

i.e. an access restriction at the FAPI using user_access('administer permissions') should probably do it

A re-rolled patch against 9.3.x. Please review.

Re-rolled patch #24, Attached interdiff for same. Please review.

The testbot found 66 style guide violations in core/modules/user/src/AdministratorRoleSettingsForm.php. See: https://www.drupal.org/pift-ci-job/2137867.

Fixed custom command failure errors in #25 and also removed few unnecessary lines from the patch. Attached an interdiff file for an easy review.

thank you!

Fixed fail tests, Please have a look.

On it.

This issue needs a usability review, so tagging it.

Patch working fine after patch we able to access ' "administrator role" setting to admin/people/permissions/roles '
Thanks for the patch for ref sharing screenshot ...

The patch is working fine.

I have done two more roles.

1. RoleForAdmin
2. RoleForUser

Once I changed the admin role to RoleForAdmin and applied it is working fine. But the default users do not remove from the Administrator role.

Same way as I have a login through the RoleForAdmin user and changed the role to Administrator. Now the current user role has expired.

I have added this issue separately.

https://www.drupal.org/project/drupal/issues/3226399

• Added issue summary template;
• Added screenshots from #34 to issue summary;
• Re-worked issue summary text slightly.

This issue has now been queued for usability review at #3226180: Drupal Usability Meeting 2021-08-06 or a future meeting, thanks.

This issue was discussed in the UX meeting. We agreed that this issue makes sense.
There are two improvements to be made in terms of usability to the last patch:

1 - Update the tab title to Role settings.
2 - Update the URL of the new route to /admin/people/role-settings

Changes has been done according to #39, Please have a look.

Updating issue title, added usability meeting issue to summary.

We're also going to need a new screenshot in the issue summary for after patch is applied.

Review of the patch:

1. --- /dev/null
   +++ b/core/modules/user/src/AdministratorRoleSettingsForm.php
   @@ -0,0 +1,117 @@
   +<?php
   +
   +namespace Drupal\user;
   

   This should live under Drupal\user\Form since it's a form class.

2. +/**
   + * Configure administrator role settings for this site.
   + */
   +class AdministratorRoleSettingsForm extends ConfigFormBase {
   

   Class name needs changed, RoleSettingsForm would be more appropriate now.

3. +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getFormId() {
   +    return 'administrator_role_settings';
   +  }
   

   Same as above, use role_settings instead.

4. --- a/core/modules/user/user.routing.yml
   +++ b/core/modules/user/user.routing.yml
   @@ -29,6 +29,14 @@ entity.user.admin_form:
      requirements:
        _permission: 'administer account settings'
    
   +entity.user.administrator_role:
   

   The route name is not appropriate, it's not a Entity Form so we can drop entity. and it also doesn't relate to the user Entity Type. I would think user.role.settings would be the most appropriate route name, user. being the module.

5. --- a/core/modules/user/user.links.task.yml
   +++ b/core/modules/user/user.links.task.yml
   @@ -49,3 +49,9 @@ entity.user_role.collection:
      route_name: entity.user_role.collection
      base_route: entity.user.collection
      weight: 10
   +
   +entity.user.administrator_role:
   

   Similar to the above the link name also needs to be updated, user.role.settings instead of entity.user.administrator_role.

6. +  requirements:
   +    _permission: 'administer account settings'
   

   We probably need a new permission for this route, as the administer account settings permission would not make sense for this form. We would also probably want an update hook, so that any role which has this permission would automatically be given the new permission.

   I'd say administer role settings would be a good option.

Thanks,
-Aaron

Worked on 42.1, 42.2, 42.3, 42.4 and 42.5, Please have a look.

42.6 still pending needs to work.

On it.

- I'm not convinced we actually need a new permission for this. Anyone who can twiddle the boxes at admin/people/permissions can accomplish the same as what this setting is doing. I see no benefit (and some downsides - additional hassle in here, update hook, more permissions for users to make sense of and configure properly, etc) for making a dedicated permission for this. Can we either stick to administer permissions for this, or get a more compelling argument why we need a new one? #42.6 as written is not convincing on its own.

- I'm not even convinced this makes sense as a whole new route / form for a single option and submit button. What happened to the proposal in the summary:

Since it's a per-role setting, and not a per-user setting, this does not make sense. It would make more sense to sit under /admin/people/permissions/roles.

That sounds right to me. Why does this want to be a whole separate form? Why not keep the setting on the page where you're already seeing all your roles and deciding which one should be an admin role?

- I'm not totally sure this is a bug. I'm a big fan of calling UI/UX problems bugs, but this seems more like a task to me. Leaving the category alone for now, but registering my skepticism. ;)

Thanks!
-Derek

p.s. administer permissions is already the perm that controls access to '/admin/people/roles' -- further evidence we should use that perm for wherever this setting ends up, either admin/people/roles itself (no change) or the new route proposed above.

I talked with @berdir and with @AaronMcHaleif about using the 'administer permissions' permission instead of create a new permission and they agreed.

Attaching a patch and an interdiff for it.

Thanks.

Hi @dww,
I didn't read your comment before I had added the patch because I hadn't reloaded the issue page.
So maybe my patch does not cover your points. I'll check it later but today I won't be able to answer you.

That sounds right to me. Why does this want to be a whole separate form? Why not keep the setting on the page where you're already seeing all your roles and deciding which one should be an admin role?

We talked about this on the UX call at #3226180: Drupal Usability Meeting 2021-08-06, there were a few reasons the recommendation was to have a separate form:

1. There is concern that putting this on the Role collection would confuse or clutter the interface, mainly for sites that have a lot of roles. The example was of a site where there are so many roles that you would need to scroll down the page to see this setting.
2. Similar to the above, well evidenced design patterns tell us that we should not have one form doing too many things, this is where things like multi-step forms are considered best practice.
3. The existing pattern in Core is to keep the settings form separate from the entity collection, for example in Views, we have the collection of views and the general views settings on a separate Views Settings form.
4. Part of the reason we recommended dropping the "Admin" from the form name is because we saw real potential here for both Core and Contrib to expand this in the future. If we create a good starting point, this gives us the potential in the future to for example no longer hard-code the anonymous and authenticated roles and provide configuration for those. This could be a real advantage for distributions/profiles where they might want a different naming scheme for roles or redefine what the anonymous and authenticated roles mean.

Thanks,
-Aaron

@paulocs: No worries, thanks!

@AaronMcHale: Appreciate the elaboration. That makes more sense.

Some initial concerns actually looking at the patch:

1. +++ b/core/modules/user/src/Form/RoleSettingsForm.php
   @@ -0,0 +1,119 @@
   +class RoleSettingsForm extends ConfigFormBase {
   

   Curious, since this isn't actually saving a value in the config system, is this an appropriate base class to use? Would it be better to directly extend FormBase? Does ConfigFormBase get us anything we need / want?

2. +++ b/core/modules/user/src/Form/RoleSettingsForm.php
   @@ -0,0 +1,119 @@
   +   * Constructs a \Drupal\user\AdministratorRoleSettingsForm object.
   

   No longer what the class is called.

3. +++ b/core/modules/user/src/Form/RoleSettingsForm.php
   @@ -0,0 +1,119 @@
   +  protected function getEditableConfigNames() {
   +    return [
   +      'system.site',
   +      'user.mail',
   +      'user.settings',
   +    ];
   +  }
   

   These aren't the settings that this form lets you set.

   I don't know what good this method is, honestly. ;) But if we have to define it, we should probably return an empty array since this isn't a normal config form at all, and we store the results outside of the config system.

4. +++ b/core/modules/user/src/Form/RoleSettingsForm.php
   @@ -0,0 +1,119 @@
   +      '#type' => 'select',
   ...
   +      // Don't allow to select a single admin role in case multiple roles got
   +      // marked as admin role already.
   

   Probably out of scope here, but since we're actually storing a flag on each role, and there are potentially more than 1 admin role, why does the user have to pick from a single drop-down select as the widget for this?

5. +++ b/core/modules/user/tests/src/Functional/UserPermissionsTest.php
   @@ -108,7 +108,7 @@ public function testUserPermissionChanges() {
   -    $this->drupalGet('admin/config/people/accounts');
   +    $this->drupalGet('/admin/people/role-settings');
   
   @@ -118,7 +118,7 @@ public function testAdministratorRole() {
   -    $this->drupalGet('admin/config/people/accounts');
   +    $this->drupalGet('/admin/people/role-settings');
   
   @@ -133,7 +133,7 @@ public function testAdministratorRole() {
   -    $this->drupalGet('admin/config/people/accounts');
   +    $this->drupalGet('/admin/people/role-settings');
   
   @@ -144,7 +144,7 @@ public function testAdministratorRole() {
   -    $this->drupalGet('admin/config/people/accounts');
   +    $this->drupalGet('/admin/people/role-settings');
   

   Not sure why we're using the leading slash for the new code in all of these...

Point #4 makes me wonder: if this is really a flag on each individual role entity, why have this UI at all? Why not a checkbox on each role entity for it? Seems like it'd still satisfy the "keep each form nice and small" to add a single checkbox to this:

#61.1: I think you're right and FormBase would work fine for this form.

#61.3: Switching to FormBase I believe means that method wouldn't be needed at all.

Point #4 makes me wonder: if this is really a flag on each individual role entity, why have this UI at all? Why not a checkbox on each role entity for it? Seems like it'd still satisfy the "keep each form nice and small" to add a single checkbox to this:

That's an interesting idea. Technically it is a configuration on each Role Entity, so in theory its possible for more than one role to be marked as an Administrator Role. I wonder how the wider permissions form/system handles more than one Role being marked as the Administrator role? Would be easy to test by simply editing the role configs to make more than one role an admin role. As in, does the permissions system have a hard-coded assumption anywhere that there will only be one admin role.

On it.

Lets see if we get more opinions about the correct approach for this issue.
I'm talking specifically about 51.4

Worked on #51. Please have a look.

51.1: Changed ConfigFormBase to FormBase.
51.2: Updated the class name in comment.
51.3: After switching to FormBase removed method "getEditableConfigNames".
51.4: Needs to Work.
51.5: Remove the leading slash.

Thanks, @vsujeetkumar. However, I fear that might have been a waste of effort. As @paulocs pointed out in #54, we need to decide what approach we're going to take for this UI, and what's up with #51.4. Fixing the code for the current approach might be irrelevant if we do what I propose at the end of #51 and change this UI entirely.

Re-tagging this for usability review. I also just pinged the #ux channel in Slack to try to get it on the agenda for this week's meeting.

Also updated the summary to include both proposed solutions, and an accurate "Remaining tasks" section. Step 1 is decide on the approach, so all other effort should be blocked on that.

Thanks again!
-Derek

Another thought: If we go with option B, we probably want to visually promote the value to the role overview page. Sort of the "settings summary" pattern. Or a whole new column in that table (there's plenty of space for it) to indicate anything special about each role. Not sure what the column should be labeled, what values it would contain, etc. But if I'm scanning my collection of roles, it'd be nice to know which one(s) are the admin role(s) without having to click through to every edit form. Maybe this can/should be a follow-up, but as we're considering the UI/UX of all this, wanted to raise it here for consideration.

Thanks,
-Derek

Queued for usability review at #3227759: Drupal Usability Meeting 2021-08-13 or a future meeting, thanks.

We discussed this during #3227759: Drupal Usability Meeting 2021-08-13.

Our recommendation is to stick with Option A, because:

1. It facilitates the previously agreed new pattern of being able to configure which roles are designated for a specific purpose (for example in future we can configure which role is the anonymous role.
2. Thereby keeping everything relating to "role settings" in one place. This simplifies things for users, they know that anything specific to an individual role is on the Role Edit Form and anything "global" is on the Role Settings Form.
3. The concept of more than one Administrator role does not make sense, there would be no reason for a site to have more than one role of that type. In the same way that it would not make sense for a site to have more than one Anonymous role. In the context of what roles are meant to be used for - a collection of permissions that can be applied to users - there would be absolutely nothing that would be gained from having more than one "admin role".
4. Similar to 4, this is consistent with most other systems that people interact with, where you have one super admin role; There may be more than one admin role, but those additional admin roles are usually delegated a sub-set of administration level permissions, so wouldn't be given "super-admin level permissions".
5. That all means it is easy for users to understand the impact of what setting a role as the admin role means, it becomes easier to communicate that in documentation and in the UI if we follow previously established patterns, as Option A does.

Therefor, our recommendation is to stick with Option A, that being the existing behaviour of selecting the Admin Role on a settings form.

We also found a potential issue in this value being stored per role: The current configuration of this setting allows someone to basically hack in more than one role to be the "admin role". This could potentially result in confusion and be a security concern because we don't expose this in the UI. We think that the suggestion by @dww in comment #57 should be explored further, some kind of indicator on the Role Collection is a good first step to addressing this, however we think a follow-up issue is needed to explore this in more detail.

A follow-up issue may seek to introduce a role.settings config object and move this out of the Role Entity config completely. This would likely be required anyway if we were to replace make the Anonymous and Authenticated roles configurable in the future. This is something that definitely needs further discussion, but something that can be done in a follow-up.

I updated issue summary and reviewed patch #55.
It works as expected and the code looks good now. Moving to RTBC.

Forgot to add the "Needs follow-up" tag as per my last paragraph in #60. This can stay RTBC as it doesn't impact the issue :)

Do we need a change record? We probably need a change record.

Thanks for the UX review! Agreed that allowing more than 1 is weird, we should keep the single-select UI, but move it to the new, more restrictive form. We can move the data model to match the UI in the long run.

CR: https://www.drupal.org/node/3229027
followup for #57: #3229028: Display which role is the 'Administrator role' on /admin/people/roles
followup for #60: #3229029: Introduce a role.settings config object and move 'Administrator role' out of the Role entity config

#NeedsFollowUp
#NeedsChangeRecord

RTBC indeed!

Thanks again,
-Derek

Originally read this as moving to a tab under admin/config/people and wasn't sure, then looked closer at the screenshots and realised it's moving it to a tab on admin/people next to roles and permissions, which makes complete sense. Thanks for opening the follow-ups as well.

Committed 5787a57 and pushed to 9.3.x. Thanks!

A lot if tutorials created to date

Would be great to have redirect for the sake of upgrade path

The new tab is splitting out this setting from the admin/config/people page, so it's not possible to add an actual redirect. If you mean a link from that form, I'm not sure about that either, because it will be redundant information for new users.

Thanks for the follow-ups @dww, and thanks for committing @catch 🎉

Great to see this make it into 9.3!

@catch , no, we can just keep some redirects in redirect table in order for old tutorials/user habits to continue working when accessing old URI

@podarok: There's nothing to redirect. You seem to be misunderstanding us, so let's see if pictures help.

The /admin/config/people/accounts path still exists, and still contains a (very full) "Account settings" form:

It just no longer includes a single setting, "Administrator role" that now lives in a new form on a different path, /admin/people/role-settings called "Role settings":

The only way to "redirect" users looking at the old form to the new location for this one setting would be what @catch wrote above:

If you mean a link from that form, I'm not sure about that either, because it will be redundant information for new users.

We could add a "Hey, if you're looking for the 'Administrator role' setting that used to live on this form, it's over at Role settings now", but that's confusing and weird for all sorts of reasons, and not the kind of UI bloat we do in core.

@podarok: Does that make more sense? Would you be willing to remove your comment from the CR about this?

Thanks,
-Derek

Yes, makes sense. Sorry for bugging you.
Removed comment in CR