Add a container parameter that can remove the special behavior of UID#1

Do you think we should remove uid #1's specialness, and just give uid #1 the admin role on registration?

I was about to roll a patch and create an issue for just this case. Ill work something up before BADCamp and see if i can get anyone there to review it. Subscribe.

Now that Drupal automatically adds user #1 to the administrator role we dont have to do it ourselves to make this happen.

We probably need to add permissions for the special handling that some of core uses presently. For now i have removed the lines in the patch so it should be fairly obvious what needs to be added back:

1. Block caching was bypassed for user 1. Do we have or want a permission for this or should it just respect drupals defaults as it in the patch as it stands?
2. hook_file_url_alter() always served user 1 local files instead of passing to cdn. Now they are a regular user should we add a permission for this "feature" or does one exist?
3. User 1 was prevented from accidental or intentional deletion. How would we like to handle this now that user 1 has no special powers?
   We can ensure that at least one user with the "administrator role" remains but that role can be stripped of "administer users" and "administer permissions" permissions. To prevent lockout we can either lock those permissions to "on" like is done with comment approvals or just test against the permissions we care about and ignore the role altogether. Thoughts?

The attached patch is only a starting point and removes all of the references to user 1 that i know off the top of my head or could find with a simple grep but doesnt address the issues raised above. If we find consensus on how to treat each above ill add it to the patch.

Too late for 7.x

Sorry about that i even notice it was tagged for 7.x still. the patch was against 8.x queuing for testing.

Alternative: create new "superuser" role (all permission boxes set), assign it to uset 1 with one exception: he should not be able to lock himself out of the permission-setting page.

Note that removing uid superuser powers cripples new module install experience, uid 1 needs to remember to set modules permissions first to access it's admin screens and functionality, then try it out and then set the permissions for the rest of the users.

kika: the point (imo anyway) of this was to not change the user 1 experience in any fashion but remove the special casing of that user so that more users could inhabit that role. THis way people dont have to share credentials, etc.

Everywhere in drupal permissions are handled by roles except in this one case (user 1). This issue squares that circle for us. It's a usability win if my clients are any indication of the normal users of drupal.

With that in mind i would agree with you that new permissions should be added to the "administrator role" as they are made available by enabled modules (i think they already are?) and that we should prevent the administrator role from removing any permissions or at least the "administer permissions" permission. This last one is an open question but id prefer this be a role equivalent to the super user so locking all permissions on works for me personally.

Finally if we prevent the "last remaining user" of the "administrator role" from being unassigned, or deleted then we should be pretty well covered from people locking themselves out unintentionally.

@all: Sound like a reasonable approach for my next version of the patch?

Marked #130213: Stop assuming userid=1 is superadmin as a duplicate of this issue. It was technically older, but this one has a patch.

This would also help improve security, IMO.

And subscribing.

chx points out in irc that if uid 1 is not special cased it would be important to provide a way to let someone back in if they remove all roles on uid 1. I don't consider that to be a big problem. It's a handbook page with 2-3 queries to use to identify what kind of lockout you have and fix it.

Alternatively we could add a form validation handler that checks if you're the last user with the admin role and and stops removing that role in that case. That would handle stuff like VBO role updates, but it would still stop people from "locking themselves out".

For accidentally removing all roles, we already have some code to prevent that issue:// Prevent user administrators from deleting themselves without confirmation. in UserMultipleCancelConfirm.php. And, the admin role already gets all new permissions for people using a profile that creates it. I don't think we should do anything more than what we already do. If there are weird cases where people get locked out we can have a doc page OR fix this problem when we are sure that it's a problem.

Attached is a reroll of #4. I then grepped core for "= 1" and "ser 1" and "ser #1". I mostly found nothing that looked important except for UserSession.php hasPermission which was hardcoding TRUE all permissions for uid 1.

Let's see what testbot thinks.

> now we have an admin role in core

In my experience on D7, the admin role does not work perfectly, and does not get all permissions granted to it.

Also, presumably an admin needs to be able to administer permissions. So even if you did limit the permissions of uid 1, any attacker could just add them back? Or is the intention that the site is locked down so nobody can admin permissions?

Bugs in the admin role should be filed as bugs in the admin role :)

In addition to the usability improvement that started this issue, there are two specific security-related cases that I care about:

1. Paranoia module hides some permissions in the permissions page and ensure that they are not granted with a custom form validate function. An attacker who gets access to a role with administer permissions is still unable to grant those roles to themself. But if UID1 always gets all roles, that protection is not helpful.
2. I've seen modules that use permissions that are not defined in a hook_permission as a way to make the feature usable only by UID 1. In general these are really risky things. That seems like a bad practice to me that we should prevent from being used.

I don't really understand why there are all these test failures. I guess the tests rely on uid1 bypassing all permission checks?

summarize discussion to date

This still seems like a good idea, and there's even less reason for uid 1 special casing now update.php is a route.

Bumping to major, tagging D8 upgrade path since we might need to add a 'run updates' permission and/or add the admin role to uid 1.

Here's an updated patch that just removes the extra permissions from user 1 so we can see what tests will fail.

The other hunks in #13 are either about prevention deletion or disabling block caching. Maybe we still want to do this, but based on a permission instead of uid (at least the deletion, not sure about the caching).

I imagine we can solve a lot of the test failures by replacing use of WebTestBase::rootUser and instead creating a user with the necessary permissions. This fixes the first failure, BlockHtmlTest, and would make a good novice task for someone to go through the other tests and adding the appropriate permissions.

Standard permissions are:
administer blocks
administer modules
administer site configuration
administer software updates
administer themes
link to any page
access administration pages
access site in maintenance mode
access site reports
view the administration theme
administer account settings
administer permissions
administer users
cancel account
change own username
select account cancellation method
access user profiles

I would like to look into this issue at the DDD montpellier today.

I encountered the folling when testing:

A) After patch user 1 cannot access test in the GUI. I needed to create an administrator role and give it all privileges, and set it as the admin role in admin/config/people/accounts, and give user 1 this administrator role. This is a logical consequence of removing the special treatment of user 1.

B) Because of this the Drupal install script that creates user 1 also needs to enable administrator privileges for user 1. This needs to be done as described in point A: by creating a user role, setting that user role to be the administrator user role, and then enable all privileges for all modules that were already enabled at that time (assuming drupal will assign those privileges for any module enabled afterwards). The created user (1) then needs to be assigned the created administrator user role.

C) In WebTestBase the rootUser is assigned uid 1. Here I would suggest the same procedure as on install needs to be followed: create role, assign role admin status, assign all privileges to role, assign role to user.

Any thoughts on this solution? If it looks like a good solution I will proceed to implement it.

I think it's time to discuss this issue with a core committer to check if this change is still doable in 8.x. It looks like we would need considerable changes, for example, minimal has no administrator role right now, only standard has, so we would have to move that out of the install profile and create one by default in user.module. And we would need dozens of fixes in tests.

The first thing that you for that is a beta evaluation in the issue summary that explains why this issue is still commit-worthy.

re #24 a/b, it was mentioned in #4 that user 1 is given the administrator role. I've not tested that, but I didn't have to add any extra roles.

In WebTestBase the rootUser is assigned uid 1. Here I would suggest the same procedure as on install needs to be followed: create role, assign role admin status, assign all privileges to role, assign role to user.

That will work and is probably an easier fix, but I thought it was good to document and test the permissions that are required, which happens when you use the pattern I did in #21.

re #24 a/b, it was mentioned in #4 that user 1 is given the administrator role. I've not tested that, but I didn't have to add any extra roles.

Yes. In standard profile. But not for minimal and not for testing or any other install profile that someone else would create.

It's also very easy to accidently delete that role or remove uid 1 from that role, and then your site would be completely broken if you don't have another user with that role.

I'm not sure I see the security improvements there :)

Also, note that the admin role now has *all* permissions as well, just like uid 1. So there really is no difference anymore between the two.

I don't think we should make this change.

@Berdir, thanks for your feedback.

You said:

Also, note that the admin role now has *all* permissions as well, just like uid 1. So there really is no difference anymore between the two.

There is at least one big difference: uid 1 always has all permissions. If a site admin removes a permission from the admin role then uid 1 will still have that permission. For example, the paranoia module removes all permissions from the permission form that are so risky they shouldn't be granted to anyone in a well-secured site (e.g. use PHP for settings'). But uid 1 still has the ability to do things behind that permission. On Drupal.org specifically, which does run the Paranoia module, this behavior of uid 1 makes me rather scared (and puts more responsibility in Dries to properly manage his account than is fair to him).

It's also very easy to accidently delete that role or remove uid 1 from that role, and then your site would be completely broken if you don't have another user with that role.

There are a lot of ways to "shoot yourself in the foot" and we should make it hard to do them and we should make it easy to recover from them. But the examples you talk about are no worse than the others and the benefits of this issue outweigh the usability decrease.

Another disadvantage of UID 1 is that it only applies to one account, so it's harder to keep track of who knows the password to your super account than if you give everyone their own, named account and disable people who should no longer have access.

We currently have checks to stop you deleting UID 1. We should replace/extend those checks to stop people deleting the last user with the administrator role, or something along those lines (probably a lot harder in practise than in principle). We can also look at methods of recovering that access.

Even if we decide not to do this for Drupal 8, I still think it is worth changing the tests, so that they are documenting and testing the permissions required.

No, that difference no longer exists. the admin role is now a flag on the role, if set, then it has all permissions, just like uid 1.

I see the security implications of that, but a) That resolves a huge UX issue that the administrator role was missing a lot of dynamic permissions and b) solves performance issues where we have to storage and load a huge list of permissions and update them when we install modules.

I see that was added in #2435075: Implement admin role as a flag on the role storage, simplify permissions page, remove user_modules_installed.

That change feels like a real bummer for me. I understand how that change solved a bug, but it seems it's solved in a suboptimal way from a security perspective.

I filed #2471763: Ensure no roles are "admin" roles (8.x) to deal with it.

I updated the issue summary and added a beta evaluation. Do I need to tag the issue so it is flagged for evaluation?

Re: #26: I agree that it would be a good idea to test with the proper permissions, instead of the blanket admin user. This might be added as a separate issue, and also added to the test documentation.

I am removing the Novice tag from this issue because it appears this issue may be shifting direction and is lacking a consensus on the next steps.

I’m using this documentation as a source: https://www.drupal.org/core-mentoring/novice-tasks#avoid

Removing the special casing of UID one is still valuable in my opinion. We handle absolutely all other permissions by checking against assigned roles. The UID = 1 is a special case and an great example of a drupalism that makes things more confusing for new developers to the system and introduces special cases in code throughout the logic of drupal. We are just used to it.

Using our shop as an example it causes trouble because people (customers and devs) understandably seek out UID 1 (via drush uli or similar) instead of embracing their own user with the appropriate admin role to make modifications on client sites. As a result you cant ever trust who was making what changes in drupal logging infrastructure because it was most likely someone "sudoing" in drupal. Making all users with the role equal wold remove the impetus to do that.

Whether through sharing UID1 passwords or removing an audit log it continues to cause trouble and confusion.

The primary reason for having a sudo user seems to be development and low-level maintenance. However, I do not presume I can predict how other developers use user 1 on the sites they build, so removing the user entirely could be too radical. We could add a site setting or configuration item to explicitly (dis)allow user 1 to be active on the site. It can be disabled for new installs and enabled for sites that were upgraded from Drupal 7. This would allow developers and site builders to navigate the site using user 1 during the development phase, yet allow the account to be disabled on staging and production sites, forcing people to use individual, more restricted accounts (as described in #35).

@Xano: any reason a developer couldn't just:
drush urol administrator UID

My understanding is that the whole point of this was to simplify the permission handling from a DX and UX perspective and I'd hate to see another edge case get introduced. This has the added benefit of leaving an audit trail in the logs for THAT specific developers actions in the system instead of "somebody sudoing as UID 1".

I think it can and should be very simple to explain to people (new users and confused new developers): Your permissions on a drupal install are the aggregation (bitwise OR) of the roles your user is assigned. Its easy to explain and it makes sense.

Unassigning stale issue. Hopefully someone else will pursue this.

Reviving this issue with a very basic patch that has a slightly different approach.

Instead of creating an admin role that can be assigned to others, it creates a superuser role which will not be assignable through the UI or code. Only user 1 will ever get it. This is essentially the same as suggested in the issue summary, but with full backwards compatibility. It will also unblock the following issue: #2749865: Rename the 'user.permissions' context to 'user.roles.permissions', to ensure correct cache context optimization

Issues that may be closed if/when this lands:

• #497500: Create an administrator account in the default install profile, separate from user 1
• #572240: Explain in the user interface that user/1 and the "administrator" role are not the same thing

Will adjust the patch after CI has had a go at it. It still might need some UI clean-up and test failure fixes.

Still expecting some test failures, for instance RenderCacheTest needs to be rewritten entirely or simply removed.

Fixed a few more test fails. This interestingly shows how much our tests and code rely on UID 1 having certain benefits. One test failed because UID was incorrectly created as blocked (status = 0), but the test still worked because of a UID == 1 test, regardless of blocked status.

Okay so there was a bug in User::preSave() which this patch uncovered. It was calling $this->get('roles')->offsetUnset($index); while inside of a loop. But the thing is, ItemList::offsetUnset() actually re-indexes the value list, causing an exception to be thrown when you try to offset an index that no longer exists.

See interdiff for details. Curious to see how many tests will still fail now.

@kristiaanvandeneynde: Thanks for taking this on. I recognize that fixing this issue might be a bit of an undertaking . My only concern with the approach you outline above is that it introduces yet another drupalism that special cases permissions when my original intent was to remove one altogether.

Most decent systems I know of these days that deal with multiuser access don't attempt to ordain special users via code and instead allows sufficiently privileged users to assign rights to others either through roles, groups or directly to the user itself in the UI. I hope we all agree at this point that treating users in this generalizable way is much more transparent for users and developers alike.

After a quick review it really seems like your approach here gets us most of the way to just programmatically creating and assigning this role to user 1. What stops us from making it the administrative role and disabling its deletion like anonymous and authenticated?

Thanks again!

What stops us from making it the administrative role and disabling its deletion like anonymous and authenticated?

Nothing. The only thing I am worried about is that if we were to make it an actual role that has to be assigned to users explicitly, people will lock themselves out of their own website.

There is one alternative: To call the role Administrator instead of Superuser and make it assignable to other people but irrevocable from user 1. The only downside I see here is that once we go down this road, you just know someone will bypass the API and revoke the role from user 1 through the DB. If we keep the role special and implicit, it cannot be revoked without hacking core.

At least with the patch as it stands now, we have one Drupalism fewer with regard to permissions. We'd be actually using our "admin role" system for user 1 from now on.

The only downside I see here is that once we go down this road, you just know someone will bypass the API and revoke the role from user 1 through the DB. If we keep the role special and implicit, it cannot be revoked without hacking core.

As it currently stands you can delete or otherwise FUBAR user 1 in the DB, I don't think we should shy away from a solution just because the DB could mess it up.

If we wanted to, we could go the Administrator role route, lock it like anonymous and authenticated, and then add a special role deletion to Administrator so anytime it was removed from any user (whether the initially created user 1 or a later updated user) there would be an extra confirmation step making the user aware that if all users have the role removed then they may lose access to certain functionality on their site and require DB access to fix.

Perhaps we could write a Drush function to re-apply the role to UID 1? Once a user has commandline access they pretty much have the site, so I don't think that would be a security issue.

To call the role Administrator instead of Superuser and make it assignable to other people but irrevocable from user 1.

This strikes me as being contrary to the purpose of this issue. The UID 1 user, especially on old sites, may be an "every day" account. It is hard to migrate away from that practice. A person's responsibilities can shift over time (e.g. a person creates a Drupal site so they are uid 1, they write blog posts and edit content and administer the site, this works fine for a while, their company grows and now is an e-commerce business, they are the CEO and should not have every permission on the site as part of PCI Compliance and common security best practice but Drupal forces their account to have these extra permissions).

We should enable people in that scenario to revoke the "special" extra permissions from their uid 1. This might lead to some people getting "stuck" if they do the wrong thing and then they can fix it in the DB or via drush/console or something. That is OK. The original motivation for this issue was UX, so if Bojhan says "let's do this for UX" we should not perpetuate the special-casing of UID1/Roles just because we're concerned about the UX implications of a corner case.

I'd like to take a shot at simplifying the requirements and expectations that I hope we can all agree on at this point. If we agree on the following I'll update the issue summary to reflect them. I'd love to have @Catch's opinion because ultimately we need this in core or its all for naught.

Assigned roles, not user special casing should be where a users permissions are derived from. In practice this means removing the "$user->uid == 1" checks. All permissions should be tested in the usual way without exception for UID. An administrator role that is not editable, undeleteable and always returns true for hasPermission() should be created for this purpose.

While we don't technically NEED for at least one user in the system to have that administrator role, requiring it on the UI side would provide a certain amount of bulletproofing for our users and is a reasonable default. To accomplish that we need to check to make sure at least one other user has that permission when you are unassigning user permissions or deleting users.

This administrative role should be automatically granted to the first user of the site on site creation but should remain removable from that user as long as some other user still has it.

If the user edits the database its all up to them to make the world work. If they can delete permissions in the DB they can re-add them. This is consistent with the make easy things easy and make hard things hard philosophy. This same thought process would allow a determined user to actually physically delete the last administrative user if they really wanted to for some arcane technical reason and has the sufficient skills to do so.

Are we all on the same page?

Sounds reasonable to me. Though I'd add that the administrative role should not be able to be deleted, like anonymous and authenticated.

Agreed 100%. Sorry if I was ambiguous but I assumed non-editable covered that. Adjusted to undeletable as well ;).

You know, if I hadn't somehow magically skipped that line while reading your summary it probably would have covered that for me, but more clarity doesn't (usually) hurt.

+1 to #55.

Adjusting the patch to meet #55 then. Stay tuned.

No interdiff due to radical changes, expecting some test failures again. Note that this patch still contains the bugfix for the bugs found in #47 and #49. Expecting a ton of test failures.

#55 sounds good to me, I think I like the validation more than special casing uid 1 in the UI (although I'm not completely opposed to that either, you could still remove the role via drush if we did).

The central point of 1. create the new role 2. don't let it be deleted 3. assign it to the first user on the site sounds great though.

Tagging for usability review. Screenshots of the validation message (and how the role checkboxes look on uid 1 etc.) would be useful for that.

Well this just goes to show how many tests are currently actually broken because of some happy accident making the user they created UID 1.

1. CommentDefaultFormatterCacheTagsTest never meant to test with UID 1 but did, so the cache tags are different. Please let me know what the actual cache tags should be.
2. CommentViewsKernelTestBase never meant to use UID 1 but did, so it worked with too few permissions. Added 'access comments' and 'post comments'. Please confirm if this is the right fix.
3. Had to refactor AccountPermissionsCacheContext to stop special casing UID 1 and as a result PermissionsHashGeneratorTest

Here goes to another CI round :)

Further fixes of wrong access checks (mainly having UID 1 by accident). UserViewsFieldAccessTest was a doozy because it was actually trying to read out user properties such as 'status' which are explicitly forbidden by UserAccessControlHandler. So I removed all of the offending properties from that test.

Funny how many tests we have that were incorrectly passing because of the UID 1 thing. Will fix the last few failing tests next week. Then I need to write an update hook to add the role and perhaps a test that verifies if user 1 gets the admin role on install.

Excellent work @kristiaanvandeneynde. Glad you were able to expose the tests too. Yet another good reason to get this into core. ;) Thanks for your effort and ill try and load this up this weekend or early next week to test and take a look myself as well.

FieldFieldTest flat out relied on UID 1 being special. If not, the whole test broke because it tried to access the forbidden timezone property (see above).

CommentDefaultFormatterCacheTagsTest is failing because it used to run as UID 1 who was shown far more cache tags. Probably because of all the edit links. Right now I've already fixed the first comparison which should only show the cache tags for the actual entity.

The second comparison only succeeds if the user has the 'administer comments' permission. I have no clue why user:2 and comment:1 are added only then. Because when changing a user's name, the label in the comment is updated for everyone. Even people who can't administer comments.

RowRenderCacheTest is very much broken as explained in #2473873: Views entity operations lack cacheability support, resulting in incorrect dropbuttons.

It used to use the following cache contexts:

• UID 1: user.permissions
• UID 2: user
• UID 3: user

But after the UID 1 normalization it uses:

• UID 1: user
• UID 2: user
• UID 3: user

The funny thing is: The test checks for the 'edit any test content' permission which is never assigned to anyone. Anyone with the permissions gets the 'user.permissions' context instead of 'user'. So UID1 got a different context which for some odd reason made the test go green. Reason is probably explained better in the issue I linked.

By currently giving everyone user.permissions just like in ::testNoCaching(), the test still goes green. So I have done that. The people working on #2473873: Views entity operations lack cacheability support, resulting in incorrect dropbuttons can take care of properly fixing that test.

So it's just CommentDefaultFormatterCacheTagsTest failing now because of the weird cache tags I can't quite get my head around.

After that, this patch needs:

• An update hook to add the role to sites that don't have it and assign it to their UID1
• A test to confirm that on any install, UID gets the administrator role
• A user constraint that disallows the last admin user from having their admin role stripped. Perhaps for better UX we could disable the checkbox on forms, but a constraint sounds better as it also works with other layers of the API
• Make sure this is also true when bulk updating users through the UI

It's not immediately clear why so many cache tags are no longer showing up when uid=1 in CommentDefaultFormatterCacheTagsTest. When the comments are being rendered in CommentViewBuilder extends EntityViewbuilder, it's \Drupal\Core\Entity\EntityViewBuilder::getCacheTags() that e.g. generates the comment_view cache tag that is currently missing. Stepping through comment rendering with a debugger as uid=1 should allow you to find the root cause.

I found it! :)

The comment_install() function never runs during the test which does not set the state entry that tracks comment count. Without that, only admins can see the rendered comment field, which explains the massive lack of cache tags. Running comment_install() and granting the user the 'post comments' permission fixed things.

Also: Thanks for answering my call to look into this, Wim.

We actually need the add/remove administrator role actions. Also the administrator role will show up as such in the UI. Should be close to if not at 0 failing tests now.

Excellent! So now the question becomes: Where do we go from here?

We could add UI limitations and/or constraints to prevent the removal of the admin role from the final admin user. Or we could go with the suggestion in the issue summary outlined under "Potential problems with *not* special casing". I.e.: Create a page with instructions as to how we can restore access to a user when all admins are gone.

I'll play around with the patch on simplytest.me later on to see what it looks like in the UI right now. I'll also add a check to UserInstallTest to check for the admin role and an update hook to add it to existing sites.

This patch adds an update and post-update hook for making sure the role exists and UID 1 has it. It also extends UserInstallTest to check whether UID 1 gets the role upon site install.

The only part I'm worried about is that the update hook grants is_admin to the administrator role on existing sites. If anyone for some reason turned that flag off, they will suddenly have it again. Probably no-one would have an 'administrator' role without admin privileges, but still mentioning it.

These screenshots were taken after a fresh install on simplytest.me. Note that UID 1 can remove his own admin role and currently the UI shows a delete action in the dropbutton for the admin role as well.

• Whether or not we want to babysit websites into not removing the admin role from the last admin is still up for discussion.
• We can make sure the delete action does not show by adding the administrator role ID to RoleAccessControlHandler::checkAccess. It depends on whether we want the admin role to be irremovable. I would strongly recommend making it persist, as mentioned in the issue summary.

Today was FOSS day at our office today so Cole, Mika Travis and I took this a little further.

We prevented the deletion of the last user with the administrative role in the bulk operations screen. We chose to abort the operation entirely instead of let it proceed deleting until the very last user because it seemed very arbitrary to leave one unlucky soul at the end to be chased down for access.

We did:
* We prevented the deletion of the administrative role.
* We removed some special casing preventing you from deleting UID1 in both bulk and user/#/delete.
* We prevented users from deleting last user with admin role via bulk.

What remains:
* Prevent individual last user from deleting self on user/#/delete
* Prevent removal of last administrative role in both bulk and individual form. (we took a shot at this and its also almost done. Will update monday).

Any user with the currently
+ * designated "administrator" role will always pass this check.

This implies there can only be one admin role. While that might be a good idea for D9, we can't put the genie back in the bottle for D8. We have the possibility for multiple admin roles, so we should refer to that concept as such. There is no "the" admin role.

Users with this role will bypass all permission access checks.

This is not true. They don't bypass any access check, they just receive all permissions. The current text is actually more accurate. I would leave it as such.

+    $admin_roles = $this->roleStorage->getQuery()
+      ->condition('is_admin', TRUE)
+      ->execute();
+    $admin_role = reset($admin_roles);

Same comment as above. There is no one admin role to rule them all. We need to loop over all "is_admin" roles.

+        $perm_roles = array(RoleInterface::ANONYMOUS_ID, RoleInterface::AUTHENTICATED_ID, RoleInterface::ADMINISTRATOR_ID);
+        if (in_array($entity->id(), $perm_roles)) {

I'd use a more descriptive variable name ($internal_roles for instance) and we should use in_array() with the third argument set to TRUE.

@kristiaanvandeneynde: Thanks for taking a look. Unless I'm mistaken in drupal 8.4.x branch hasPermission() in Role.php does indeed bypass the permission checks if the isAdmin property:

  public function hasPermission($permission) {
    if ($this->isAdmin()) {
      return TRUE;
    }
    return in_array($permission, $this->permissions);
  }

You'll notice that the permissions are not actually added to the role and are instantly removed if you undesignated it as the admin role.We learned this the hard way by locking ourselves out of the site upon redesignation of that role. :)

Regarding the multiple role selection, you can see on AccountSettingsForm->buildForm():

$form['admin_role']['user_admin_role'] = [
      '#type' => 'select',
      '#title' => $this->t('Administrator role'),
      '#empty_value' => '',
      '#default_value' => $default_value,
      '#options' => $roles,
      '#description' => $this->t('Users with this role will bypass all permission access checks. Changing this setting will not affect existing permissions.'),
      // Don't allow to select a single admin role in case multiple roles got
      // marked as admin role already.
      '#access' => count($admin_roles) <= 1,
    ];

This is currently a single select with an #access check that is disabled from changing if someone had successfully set more than one in a prior version of the form perhaps? So it looks like it is a vestigial option as well. There was special casing for handling these historical possibilities elsewhere and we probably need to allow this in our changes I agree. I'd love some explanation form anyone familiar with the evolution of this option but I can dig into annotate if nobody remembers.

Also happy to change variable names like you mentioned as its probably the hardest thing I do all day and have almost no preference :).

I said they don't bypass any access check. They do bypass permission checks. This is an important difference because we can now test access checks without having to worry about some users not being pulled through all of the functions. I'm aware of the code in hasPermission(), but the subtle difference here is key.

Re multiple admin roles: It's something we can't undo now, I guess. In D9 we can force one admin role and be done with it, but now that the cat's out of the bag already, we can't put it back in for D8. If we remove the concept of multiple admin roles, we might break someone's build. Silly as it may be to have multiple roles that return TRUE for isAdmin().

Also happy to change variable names like you mentioned as its probably the hardest thing I do all day and have almost no preference :)

If you want something harder to do, you could fix the tests you broke in your last patch :)

Right, reverting back to the patch from #78, including the prevention of deleting the admin role (see interdiff). This is now a complete MVP (IMO) for this issue.

Whether we want to babysit users by preventing delete-last-admin actions from the UI or document a fix on drupal.org like we have in the past is still up for debate but should not affect the patch in its current form. If we choose to document, we can do that before this patch goes in. If we choose to babysit, we should consider doing that in a follow-up perhaps to avoid polluting the patch from this issue.

One patch is about fixing a drupalism, the other would be about making the fixed drupalism more mistake-proof.

This patch broke 2 tests: DateFormatAccessControlHandlerTest and MenuAccessControlHandlerTest, which were both introduced June 12th in #2843772: EntityResource: Provide comprehensive test coverage for DateFormat entity.

This is why we need this in core ASAP. As long as we keep UID 1 being special, we will be able to write poor test cases because we make assumptions about UID 1. All tests should be explicit about the circumstances under which they should run.

Retesting to see if it still applies.

Still green according to https://dispatcher.drupalci.org/job/drupal_patches/23590/testReport/

This looks good to me and addresses the fixes in the tests as described.

It does not look as if this has had signoff from the usability maintainers.

1. +++ b/core/lib/Drupal/Core/Session/PermissionsHashGenerator.php
   @@ -55,10 +66,14 @@ public function __construct(PrivateKey $private_key, CacheBackendInterface $cach
   -    if ($account->id() == 1) {
   -      return $this->hash('is-super-user');
   ...
   +      if ($role->isAdmin()) {
   +        return $this->hash('is-super-user');
   

   A) this looks scarily different and looks like a sec problem

   B) 'is-super-user' no longer makes sense, this should now become 'is-admin'

2. +++ b/core/lib/Drupal/Core/Session/PermissionsHashGenerator.php
   @@ -55,10 +66,14 @@ public function __construct(PrivateKey $private_key, CacheBackendInterface $cach
   +    // Admin roles can always access all permissions. Use a different, unique
   

   "access all permissions" sounds very strange.

3. +++ b/core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php
   index bdea71d..bed4521 100644
   --- a/core/modules/comment/tests/src/Kernel/CommentDefaultFormatterCacheTagsTest.php
   

   I don't understand why this test is being modified?

4. +++ b/core/modules/comment/tests/src/Kernel/Views/CommentViewsKernelTestBase.php
   @@ -63,6 +63,8 @@ protected function setUp($import_test_views = TRUE) {
        $admin_role = Role::create(['id' => 'admin']);
        $admin_role->grantPermission('administer comments');
   +    $admin_role->grantPermission('access comments');
   +    $admin_role->grantPermission('post comments');
        $admin_role->save();
    
   

   This should not be necessary AFAICT.

5. +++ b/core/modules/filter/tests/src/Kernel/TextFormatElementFormTest.php
   @@ -44,15 +44,16 @@ protected function setUp() {
        /* @var \Drupal\user\RoleInterface $role */
        $role = Role::create([
          'id' => 'admin',
          'label' => 'admin',
        ]);
   -    $role->grantPermission($filter_test_format->getPermissionName());
   +    foreach (FilterFormat::loadMultiple(['full_html', 'filtered_html', 'filter_test']) as $format) {
   +      /* @var \Drupal\filter\FilterFormatInterface $format */
   +      $role->grantPermission($format->getPermissionName());
   +    }
   

   Why does this suddenly need access to so many more formats?

6. +++ b/core/modules/user/src/Entity/User.php
   @@ -81,11 +81,14 @@ public function preSave(EntityStorageInterface $storage) {
        // Make sure that the authenticated/anonymous roles are not persisted.
   -    foreach ($this->get('roles') as $index => $item) {
   -      if (in_array($item->target_id, [RoleInterface::ANONYMOUS_ID, RoleInterface::AUTHENTICATED_ID])) {
   -        $this->get('roles')->offsetUnset($index);
   +    $internal = [RoleInterface::ANONYMOUS_ID, RoleInterface::AUTHENTICATED_ID];
   +    $role_refs = $this->get('roles')->getValue();
   +    foreach ($role_refs as $index => $role_ref) {
   +      if (in_array($role_ref['target_id'], $internal)) {
   +        unset($role_refs[$index]);
          }
        }
   +    $this->get('roles')->setValue($role_refs);
   

   I have no idea what is changing here.

7. +++ b/core/modules/user/src/RoleAccessControlHandler.php
   @@ -20,7 +20,12 @@ class RoleAccessControlHandler extends EntityAccessControlHandler {
   +        $internal_roles = [
   +          RoleInterface::ANONYMOUS_ID,
   +          RoleInterface::AUTHENTICATED_ID,
   +          RoleInterface::ADMINISTRATOR_ID,
   +        ];
   

   Why is this list not a constant on RoleInterface?

8. +++ b/core/modules/user/src/Tests/UserPermissionsTest.php
   @@ -47,10 +47,6 @@ public function testUserPermissionChanges() {
   -    // Create an additional role and mark it as admin role.
   -    Role::create(['is_admin' => TRUE, 'id' => 'administrator', 'label' => 'Administrator'])->save();
   -    $storage->resetCache();
   

   This is no longer necessary because it now exists by default. That makes sense.

9. +++ b/core/modules/user/src/Tests/UserPermissionsTest.php
   @@ -96,8 +92,8 @@ public function testAdministratorRole() {
   +    $this->assertOptionSelected('edit-user-admin-role', 'administrator', 'Administration role defaults to none.');
   

   This should use the constant.

10. +++ b/core/modules/user/user.install
    @@ -98,3 +103,27 @@ function user_update_8100() {
    +function user_update_8101() {
    
    +++ b/core/modules/user/user.post_update.php
    @@ -20,3 +22,12 @@ function user_post_update_enforce_order_of_permissions() {
    +function user_post_update_grant_user_1_admin_role() {
    

    Great, there's an update path! But I don't see test coverage for it yet.

A) this looks scarily different and looks like a sec problem
B) 'is-super-user' no longer makes sense, this should now become 'is-admin'

A) It's not a security problem, it basically does what Drupal has been doing for user 1. Both user 1 and admin roles had the concept of "has all permissions", yet we were only short-circuiting hash generation for UID1. Now we are consistently doing so for all admin roles.
B) I kept it as is to not mess with existing cache entries, but I guess we should change it to remove all references to the concept of super users. Will change to is-admin.

"access all permissions" sounds very strange.

Will change to "Admin roles have all permissions implicitly assigned."

I don't understand why this test is being modified?

This should not be necessary AFAICT.

It's because of comment_install needing to run for the comment tracker to work. Before it didn't care about the amount of comments because UID1=god but now it does and it can't find any comments so it incorrectly fails. See #74

Why does this suddenly need access to so many more formats?

Because the test checked for those formats being available, which they weren't. So we need to grant the test user access to them in order for them to show up again:

    $this->assertRaw('<h4>Full HTML</h4>');
    $this->assertRaw('<h4>Filtered HTML</h4>');
    $this->assertRaw('<h4>Test format</h4>');

I have no idea what is changing here.

Some weird bug with offsets that previously didn't rear its ugly head but did when we added a third role to the logic. See #49. That said, we are now back to 2 special roles in that part of the code, so this change could be reverted. But the bug would still remain, albeit hidden once again. So perhaps leave the fix in place?

Why is this list not a constant on RoleInterface?

The roles themselves are already constants. Sometimes we need just anonymous/authenticated (User::preSave), sometimes we need anon/auth/admin (RoleAccessControlHandler). It seems a bit strange to create constant arrays that we may need multiple variations of.

This should use the constant.

Correct, will fix.

Great, there's an update path! But I don't see test coverage for it yet.

I could write one. I haven't written post update tests yet, so I'd need to find a good example.

To summarize #95, I need to:

• Minor: Alter a string
• Minor: Alter a line of docs
• Minor: Use a constant instead of a string
• Major: Add a post-update test
• Arguably: Revert a bugfix

Fixed the above nitpicks. Still needs a post-update test and did not revert the bugfix mentioned above.

Added the update test, never written one before and can't really run it locally so curious to see whether it goes green.

This should work.

I wonder if this deserves mention in some type of core announcement, prior to commit? We have a ton of contrib modules, drush extensions, behat tests, etc that are going to fall flat on their face if this is committed. They would still all assume the prior special logic around user 1.

@change record

dare i ask: is this really going to be committed?

certainly i understand the ticket as a request and respect the work toward it...but.. i like having user 1. (: He's very much as core to drupal as Node is.

In terms of develop workflow it certainly is a major change. Disruptive, even I dare say.

Unassigning @kristiaanvandeneynde, looks like his awesome work is done and we all now need to review.

@SKAUGHT - I hope this gets committed, it'd be an awesome change, and I don't see it being that disruptive. As long as your user 1 as the "administrator" role, nothing will change.

true: change is hard sometimes.

I'm glad to see newfound interest in this issue! Hope to get it committed in 8.5.x

@SKAUGHT: Change is hard, but sometimes it's also for the better. In this case there are several benefits, some of them rather major:

• Security improvement: Once a site has been built or has proper roles defined, you can take away the admin role from all users. This ensures there are no accounts that put your entire website at risk should they be compromised.
• Code stability: Just look at the amount of tests I had to fix because they relied on user 1 being special. This means those tests were actually not functioning and as a result means the code they were covering was actually not properly being covered. Removing the UID1 Drupalism will ensure our tests need to run with the right permissions defined.
• Consistency: What good is an access layer if there is a special exception that can bypass everything? An example of this being a downside is a bunch of administrative local tasks (tabs) or actions ("+"-icon links) being put behind sensible access checks, only to have all gazillion of them clutter the UI for user 1 because he has god-mode haxx turned on.
• One weird Drupalism fewer: We need to distinguish between Drupalisms that define what Drupal is and those that negatively characterize Drupal by needlessly increasing its learning curve. The special case of UID1 belongs to the latter category. There are very few systems that still have god-mode accounts. And for good reason (see above items). So let's destroy yet another barrier for outside devs to join our project.
• etc.

I will add that for security auditing reasons the "uid 1 = god mode" is an item I've seen on several checklists: "Does the system have an account that has unlimited access?" A yes answer might need to be justified with business requirements or it might require compensating controls added to protect against the risks created. I've seen checklists where the existence of that account means a piece of software may not be used by the organization.

My advice these days is to simply block the uid 1 account immediately after install and remove any "admin" roles.

Updated the issue summary to reflect what the most recent patch is doing and what still needs to be done: documentation and CR.

1. +++ b/core/modules/file/tests/src/Kernel/FileManagedUnitTestBase.php
   @@ -32,7 +32,7 @@ protected function setUp() {
        // Make sure that a user with uid 1 exists, self::createFile() relies on
   

   Not sure, but as the comment here implies uid 1 is needed, we should handle this in another way.

   Maybe it is already done, but then we could test with a user id different then 1.

2. +++ b/core/modules/user/tests/src/Kernel/UserInstallTest.php
   @@ -49,6 +50,10 @@ public function testUserInstall() {
   +    $rids = db_query('SELECT roles_target_id FROM {user__roles} WHERE entity_id = 1')->fetchCol();
   

   I know its a test, but db_query is deprecated.

I will have a look in the codebase and do some functional test

I just discovered this issue and I am a bit perplex: I think the proposed solution is not a good one at the end of the day.
We could/should compare Drupal's user 1 as *nix user 0 (aka root) and I don't see any plan to remove that in the nearany future. :)

In my opinion a much better solution, would be not to remove User 1, but create a non super-user during the Site installation (which will be User's user, and for example having an empty password field by default to user 1 (which would prevent the ability to login via UI).
In this way the login would be only possible using a reset-link that could be generated via drush or for example via a form.

In this way the less experienced users would not gain access easily to this user and they would not be able to share it, intermediate users would need to go trough a few steps to gain access so it would be clear that it's a "special user", and yet experienced users will retain this old behaviour which many "rely" on.

Also I have seen quite a few contrib modules doing if($user->uid == 1), so removing it from core would just solve the "issue" partially.

m2c

I disagree with the proposed solution.
The user 1 is supposed to be used by people who know what they're doing. For all other cases, there is the role delegation.

There could be other solutions, like:

• Hard-coding some config on settings.php that will force disable the user 1 and/or the admin role.
• Interfering to the login callback so that user 1 cannot login through UI, only by a single sign-in link.

@esolitos (#112)

We could/should compare Drupal's user 1 as *nix user 0.

We definitely should not! Comparing a user that needs to be there because a system could otherwise become inaccessible to a user that should not be there because we can always make the system accessible again using several tools (drush, console, database, ...) is comparing apples to oranges. The root user on *nix is a necessary evil, the UID 1 privileges in Drupal is a needless security risk.

In my opinion a much better solution, would be not to remove User 1, but create a non super-user during the Site installation (which will be User's user, and for example having an empty password field by default to user 1 (which would prevent the ability to login via UI).
In this way the login would be only possible using a reset-link that could be generated via drush or for example via a form.

We'd be having a god-mode user then who no-one can use except those with extra tools. As explained above: once you have those tools you can turn any user into a god-mode user, but only when needed. So why keep user 1?

yet experienced users will retain this old behaviour which many "rely" on.

We should not keep broken features because it would otherwise force people out of their comfort zone. If we did, we wouldn't be using OO and Symfony now.

Also I have seen quite a few contrib modules doing if($user->uid == 1)

Shame on them, really. An outsider looking at any $uid === 1 check will rightfully shiver when they find out it's a magic all-access pass. We should always use the API for access checks. If those modules had properly checked for access, which would have returned TRUE for user 1 all the same, then they wouldn't suffer from this change.

@efpapado (#113)

Hard-coding some config on settings.php that will force disable the user 1 and/or the admin role.

Putting a security risk behind a flag doesn't make it less of a security risk.

Interfering to the login callback so that user 1 cannot login through UI, only by a single sign-in link.

See reply to 2nd quote from the top.

Also, the patch in this issue is ready to go in unless it breaks some recent tests or has flawed code. So the correct status is still Needs review. Please don't change it to "Needs work" unless it actually needs work. You can disagree just fine while the patch is still under review and then we can alter the patch should there be a consensus.

AFAICT this issue needs a change record, a documentation page and people stepping out of their comfort zone. Code-wise it looks just about ready to me. Even though I'm biased for writing the patch, obviously.

Maybe I need to clarify this in a separate comment:

This will not change any new or existing sites unless you choose so.

UID 1 will still have an all-access pass, but not because of them being user 1. Instead it will be because they have the all-access admin role, which makes way more sense. They will now have to go through all access checks just like any other user. And most importantly, they can now be stripped from their god mode haxx when the situation or best practices call for it.

Hard-coding some config on settings.php that will force disable the user 1 and/or the admin role.

Putting a security risk behind a flag doesn't make it less of a security risk.

No, but it makes it harder to exploit the permissions accidentally, if this is what it's all about.

A module could very easily implement some code that will bypass any restrictions. Then it's again "some extra tools that provide god-mode".

One last argument from my side, and I will not continue participating in this discussion.
This is a very fundamental change to the Drupal functionality and AX/DX*. I believe (maybe I'm wrong, but that's my opinion) that this type of changes should not be applied so easily, after only a short discussion between 5-10 people.

*A=Admin, D=Developer

No, but it makes it harder to exploit the permissions accidentally, if this is what it's all about.

It's not what it's all about. It is also about simplifying code, AFAICT. Removing special cases reduces WTFs per minute and potential number of bugs, because less code.

I feel strongly opposed to keeping any of the UID1 Drupalism in. Either we rip it out altogether or don't touch it whatsoever and be considered a CMS with a built-in security risk for years to come.

Putting it away in a dark corner where no-one will find it is arguably worse. Because years from now someone who doesn't know about what UID1 used to mean might go: "Oh, what's this? This seems awfully useful" and still expose their website to a needless security risk. It would also mean having to keep supporting broken access checks such as $uid === 1, meaning we will never have a fully reliable access API.

One last argument from my side, and I will not continue participating in this discussion.
This is a very fundamental change to the Drupal functionality and AX/DX*. I believe (maybe I'm wrong, but that's my opinion) that this type of changes should not be applied so easily, after only a short discussion between 5-10 people.

This issue has been around for 8 years, I only 'recently' started working on a functional patch for D8. And by all means keep participating if you feel like it. Just because we disagree with one another doesn't mean your opinion is less valid. I might disagree with 9 out of 10 things you say and be baffled by the 10th argument.

Great work, 100% for it, magic numbers in code are never a good thing, especially those bypassing access checks.

I feel strongly opposed to keeping any of the UID1 Drupalism in.

@kristiaanvandeneynde: While I don't speak for anyone but myself and my team, I think you have near universal support and agreement in this from anyone who has taken a strong look at the situation. Tying permissions to roles and assigning additive roles to users to allow multiple "root" accounts without sharing passwords is a very common pattern and one we should adopt. Attempting to work on any significantly complex project that requires collaboration between multiple agencies or stakeholders drives the point home very well if nothing else. Thanks for carrying the torch as far as you have.

Seems like a sensible change to me - as mentioned a few times above, it'll need clear communication, but we should be encouraging contrib and custom modules to use proper access checks based on permissions.

Here's the change record draft. We still need the documentation on how to restore the admin role using drush, drupal console and a database query and link to that documentation from the change record.

https://www.drupal.org/node/2910500

Regarding the uid == good (god?)
esolitos #112
efpapado #113
michaelfavia #120

The current situation makes development harder, because only as user 1, you can expect that (as a site builder) the website behaves as expected.
I strongly agree with kristiaanvandeneynde and others reasoning above.

Timing
When this patch gets in, it will be disruptive though. Timing and communication is therefor critical.

Something that we should discuss is if this patch must be committed early or late in the development cycle.

In this case I think early is better (today in 8.x-5.x or in 6 months early in 8.x-6.x) so that contrib maintainer can catch up.
Two other alternatives (somewhere in the middle, or as late as possible) could fool contrib maintainers and custom module developers.

We can put this in Drupal 8
I don't think we have to wait for Drupal 9. Only newish websites that are in 'build' modus are affected. Most 'in production' websites will have multiple non-user-1 accounts and those are not affected at all.

Regarding the change record

Module developers
Can we add a code-snippet for module-developers how they must replace their $uid === 1 expressions?

Should we add some example searches/greps how to find those.
($uid === 1, $uid == 1, stuff like that)?

Site builders / upgrade path
What are implications / upgrade paths when a website chose a specific name for the admin-role that is different or equal to the new admin-role? (admin, administrator, thewebsite_admin)
Should that website change something or is this always without side-effects?

Side effects?
If there are side-effects; should we consider to split the commit in a first one that makes the conversion possible and a final one that removes the $uid === 1 behaviour? Or is the conversion (of contrib and custom modules) already possible without this commit?

Don't forget the impact on e.g. drush uli which logs the super-user in when no arguments are given.

Should that website change something or is this always without side-effects?

No side-effects really. The patch checks if the role exists already. If so, it turns it into an admin role. If not, it creates it.

Or is the conversion (of contrib and custom modules) already possible without this commit?

Already possible. We're talking about broken code that can already be fixed by properly calling the access layer. Tests need to run with any user other than UID 1 in order to be fixed now. Once this patch lands, they don't need to worry about their tests going green by accident because they happened to have received UID1 as their test user.

Re-testing to see whether any new tests break. The EntityResource work might break, but those tests should have been written with this patch in mind, meaning it all boils down to deleting some UID1 code.

Straight up re-roll using an automated three-way merge, no changes.

Well this goes to show why we need this in core ASAP. A comment test was leaking state but went green anyway because UID 1 = god and they can see all the things all the time. I fixed the state leak and it works fine now.

Discussed this patch in today's UX meeting.

Because of the way that this patch is written, there won't be any changes to the existing admin role UX to be concerned about here, so from that POV we are good.

The two concerns that were brought up were around:

1) What happens if a user removes the Administrator role from all users, thus removing their ability to re-add it? It's covered above the idea of a Drush/Console command to get out of this situation, and a documentation page with a vanilla SQL query. That seemed sufficient to the group (particularly the documentation page that someone can find in Google that mentions the various options). We briefly talked about the idea of in-UI validation that prevents the change, such as what we do around removing the System content block, but we do it there because a) it's really easy to do since that block is not specially marked in any way, and b) you are completely screwed if this happens because you remove the output of content from all of your pages. In this case, the only reason you'd remove the Administrator role from all users is because you want to explicitly lock down your site. You're much more unlikely to ever tamper with it otherwise.

2) Emilie also brought up what happens if a site already has a role with the machine name of 'administrator' but with a reduced set of permissions? (e.g. they actually mean "content administrator" or "spam administrator" or something) We would not want to accidentally expand peoples' permissions as part of the upgrade; this would be really easy to miss. Suggestion was that during the upgrade if we encounter a site that already has an 'administrator' role, and it isn't specially marked as an admin role/ doesn't have all checkboxes checked / whatever, then migrate the existing role to "administrator_legacy" before adding the new one and assigning it to user 1. This way there's no behaviour change.

There was another concern brought up around what happens with config managed in Features which I didn't quite grok, but it's not a UX concern, so will leave that up to the folks who brought that up. Removing the tag.

Thanks webchick and the rest of the UX team!

I'll address concern #2 ASAP in the update hook and test coverage.

Let's try this one for size. Should take care of the concern outlined in #132.2

Oh wow, green in one go!

I've updated the change request to link to a documentation page I just added. See: https://www.drupal.org/node/2910500 It did mention something about migrating documentation to the new section, but I'm not sure where I'd need to put it then.

That said: The patch goes green and the UX concerns have been dealt with. Anyone willing to review and set to RTBC if satisfied?

What if administrator_legacy role already exists? I believe we should be more dynamic about legacy role name.

What if administrator_legacy role already exists? I believe we should be more dynamic about legacy role name.

I actually raised that during the UX meeting and webchick mentioned the odds of that happening are so slim we shouldn't account for it.

It's basically: <odds of having a non-admin administrator role> * <odds of having an administrator_legacy role>

Sure - it's all about odds - but since we can mitigate risk of having role name conflict can't we just cover that scenario as well? Or, if we really don't want write that piece of code, can we at least check if _legacy role already exists and throw nice update exception with information what actually happened?

By - * we * - I mean - * I can help * :)

*admin* clearly is a common base in many projects..perhaps a name other than some with *admin* at all. Naming the role "rooted" seems appropriate and less likely to be used by an exiting project.

I don't know. Normally I write code that accounts for all possible scenarios, but in this case it seems the chances are slim.

We even discussed naming the role magical_sparkling_pony to make sure no-one has it, but then there's the same discussion: "What if someone does happen to have a magical sparkling pony role?"

I'd also argue to use an incremental suffix, e.g.: administrator_legacy_3, but then the post-update hook would have to guess which role it needs to check for. We could try storing the role name in temporary storage such as state, but if that gets flushed between the update and post-update hook we'd have nothing to go on.

Maybe just do a check if the backup exists and if it does, bail out with instructions on how to configure a unique name in the settings.php.

That could work.

I'm ok with suggestion in #141. Edge-case instructions could be provided in change record ("In unlikely event of a name collision...").

Okay I've taken care of the concern in #141. If the patch goes green I'll update the CR.

P.S.: I also updated the test to meet coding standards and hard-coded the 'administrator' and 'administrator_legacy' role IDs because update hooks should hard-code such values instead of using variables that might change over time.

Edit: Instead of bailing out and then providing instructions, I simply opted for a configuration setting. You're supposed to read update notes when upgrading from one major version to another, no?

Updated the change record. We good to go RTBC now?

This patch addresses #148.5

true...