Problem/Motivation
Currently the "administrator role" setting is in /admin/config/people/accounts
.
Since it's a per-role setting, and not a per-user setting, this does not make sense. It should move to somewhere more closely related to roles.
This could probably also go a fair way toward clarifying a couple of other bugs, like #576304: Three roles now, not two -> explain the Administrator role on the Roles page!.
Steps to reproduce
Proposed resolution
Move this setting to a new form which sits next to /admin/people/roles
.
This approach received UX signoff at #3226180: Drupal Usability Meeting 2021-08-06.
Option B
See #51.4:
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?
So, add this setting as checkbox to this form:
If we do this, might also want to visually display the admin-role-nature of each role on the role overview / collection page at /admin/people/roles. Exactly how this looks / works TBD.
User interface changes
Before
After
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#55 | 987978-55.patch | 10.07 KB | vsujeetkumar |
Comments
Comment #1
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #2
lpalgarvio CreditAttribution: lpalgarvio commentedi wouldn't say it doesn't makes sense...
i'm okay with both options
Comment #3
dcam CreditAttribution: dcam commentedMakes 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.
Comment #4
yogen.prasad CreditAttribution: yogen.prasad commentedComment #5
yogen.prasad CreditAttribution: yogen.prasad commentedComment #6
yogen.prasad CreditAttribution: yogen.prasad commentedComment #10
bisw CreditAttribution: bisw commentedComment #13
bisw CreditAttribution: bisw commentedComment #14
lpalgarvio CreditAttribution: lpalgarvio commentedComment #17
Alan D. CreditAttribution: Alan D. commentedI'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
Comment #23
pameeela CreditAttribution: pameeela commentedComment #24
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedA re-rolled patch against 9.3.x. Please review.
Comment #25
Gauravvvv CreditAttribution: Gauravvvv at OpenSense Labs for Drupal India Association commentedRe-rolled patch #24, Attached interdiff for same. Please review.
Comment #26
daffie CreditAttribution: daffie commentedThe testbot found 66 style guide violations in core/modules/user/src/AdministratorRoleSettingsForm.php. See: https://www.drupal.org/pift-ci-job/2137867.
Comment #27
ankithashettyFixed 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!
Comment #29
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedFixed fail tests, Please have a look.
Comment #31
paulocsOn it.
Comment #32
paulocsComment #33
paulocsThis issue needs a usability review, so tagging it.
Comment #34
vikashsoni CreditAttribution: vikashsoni as a volunteer and at Zyxware Technologies commentedPatch working fine after patch we able to access ' "administrator role" setting to admin/people/permissions/roles '
Thanks for the patch for ref sharing screenshot ...
Comment #35
heni_deepak CreditAttribution: heni_deepak commentedThe 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.
Comment #36
heni_deepak CreditAttribution: heni_deepak commentedI have added this issue separately.
https://www.drupal.org/project/drupal/issues/3226399
Comment #37
AaronMcHaleComment #38
AaronMcHaleThis issue has now been queued for usability review at #3226180: Drupal Usability Meeting 2021-08-06 or a future meeting, thanks.
Comment #39
paulocsThis 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
Comment #40
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedChanges has been done according to #39, Please have a look.
Comment #41
paulocsComment #42
AaronMcHaleUpdating 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:
This should live under
Drupal\user\Form
since it's a form class.Class name needs changed,
RoleSettingsForm
would be more appropriate now.Same as above, use
role_settings
instead.The route name is not appropriate, it's not a Entity Form so we can drop
entity.
and it also doesn't relate to theuser
Entity Type. I would thinkuser.role.settings
would be the most appropriate route name,user.
being the module.Similar to the above the link name also needs to be updated,
user.role.settings
instead ofentity.user.administrator_role
.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
Comment #43
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedWorked on 42.1, 42.2, 42.3, 42.4 and 42.5, Please have a look.
42.6 still pending needs to work.
Comment #45
paulocsOn it.
Comment #46
dww- 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:
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
Comment #47
dwwp.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.Comment #48
paulocsI 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.
Comment #49
paulocsHi @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.
Comment #50
AaronMcHaleWe 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:
Thanks,
-Aaron
Comment #51
dww@paulocs: No worries, thanks!
@AaronMcHale: Appreciate the elaboration. That makes more sense.
Some initial concerns actually looking at the patch:
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?
No longer what the class is called.
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.
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?
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:
Comment #52
AaronMcHale#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.
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.
Comment #53
paulocsOn it.
Comment #54
paulocsLets see if we get more opinions about the correct approach for this issue.
I'm talking specifically about 51.4
Comment #55
vsujeetkumar CreditAttribution: vsujeetkumar at Material for Drupal India Association commentedWorked 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.
Comment #56
dwwThanks, @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
Comment #57
dwwAnother 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
Comment #58
AaronMcHaleQueued for usability review at #3227759: Drupal Usability Meeting 2021-08-13 or a future meeting, thanks.
Comment #59
AaronMcHaleComment #60
AaronMcHaleWe discussed this during #3227759: Drupal Usability Meeting 2021-08-13.
Our recommendation is to stick with Option A, because:
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.Comment #61
AaronMcHaleComment #62
paulocsI updated issue summary and reviewed patch #55.
It works as expected and the code looks good now. Moving to RTBC.
Comment #63
AaronMcHaleForgot 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.
Comment #64
dwwThanks 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
Comment #66
catchOriginally 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!
Comment #67
podarokA lot if tutorials created to date
Would be great to have redirect for the sake of upgrade path
Comment #68
catchThe 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.
Comment #69
AaronMcHaleThanks for the follow-ups @dww, and thanks for committing @catch 🎉
Great to see this make it into 9.3!
Comment #70
podarok@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
Comment #71
dww@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:
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
Comment #72
podarokYes, makes sense. Sorry for bugging you.
Removed comment in CR
Comment #74
AaronMcHale