We have quite some special behavior for uid #1. This tends to confuse the user, and will do so even more now we have an admin role in core. Do all the special cases we designed around uid #1 really make sense?

Problems with special casing uid 1

  • People are confused whether they should be sharing credentials of this user (bad) or just using a user with the Administrator role.
  • The lack of permission check on user 1 means that uid 1 can take actions on a site even if they do not have a role that grants them the permission. This makes it hard to completely disable unwanted features on a site unless you block uid 1 and never use it.
  • The lack of permission check on user 1 makes them a particularly valuable account for an attacker to take over in some way. Any time there is a focus point for an attack it makes the system weaker.

Potential problems with *not* special casing

If someone is playing around and removes all roles or removes all permissions from all their admin users it could be possible to have no users with administrator permissions on a site. While is is a problem it is easily solved with a FAQ page and some database instructions just like we do with WSOD.

Proposed resolution

The special behaviour of uid 1 should be removed. Instead there is an admin role that always has all permissions. All special treatment for uid 1 should be converted to proper permissions, and as the admin role always has all permissions, any member of this role would have the same access as uid 1 has now. The following steps need to be taken:

  1. Create an admin role by default, just like the anonymous and authenticated role, in the user module, and also make it undeletable, and make it the admin role.
  2. Assign user 1 the admin role on install. (do this in the user module, not the chosen install profile).
  3. Document the steps to take in case you want to grant the admin role to a user when no-one has it. This should include instructions for drush, drupal console or for using a simple insert/update query.
  4. Create a change record and raise awareness. We want to make sure contrib modules are ready by 8.5.x so that their tests won't break all of the sudden or, god forbid, they have a security vulnerability. Modules can already adapt their code now because, in essence, nothing changes. We're just enforcing good practice of not relying on a god-mode user account in our code.

API changes

None. Just need to raise awareness that UID 1 no longer has an all-access pass.

CommentFileSizeAuthor
#169 interdiff-169-168.txt1.01 KBkristiaanvandeneynde
#169 drupal-540008-169.patch52.97 KBkristiaanvandeneynde
#168 drupal-540008-168-do-not-test.patch52.21 KBkristiaanvandeneynde
#162 interdiff-162-156.txt1.24 KBkristiaanvandeneynde
#162 drupal-540008-162.patch71.39 KBkristiaanvandeneynde
#156 interdiff-156-152.txt5.15 KBkristiaanvandeneynde
#156 drupal-540008-156.patch70.41 KBkristiaanvandeneynde
#152 interdiff-152-151.txt2.38 KBkristiaanvandeneynde
#152 drupal-540008-152.patch67.25 KBkristiaanvandeneynde
#151 interdiff-149-144.txt1.18 KBzaporylie
#149 interdiff-149-144.txt7.88 KBzaporylie
#149 remove_the_special-540008-149.patch63.92 KBzaporylie
#144 interdiff-144-134.txt7.48 KBkristiaanvandeneynde
#144 drupal-540008-144.patch66.73 KBkristiaanvandeneynde
#134 interdiff-134-131.txt6.71 KBkristiaanvandeneynde
#134 drupal-540008-134.patch66.14 KBkristiaanvandeneynde
#131 interdiff-131-129.txt6.49 KBkristiaanvandeneynde
#131 drupal-540008-131.patch63.34 KBkristiaanvandeneynde
#129 drupal-540008-129.patch57.11 KBkristiaanvandeneynde
#101 interdiff-99-101.txt1.93 KBkristiaanvandeneynde
#101 drupal-540008-101.patch57.12 KBkristiaanvandeneynde
#99 interdiff-98-99.txt1.6 KBkristiaanvandeneynde
#99 drupal-540008-99.patch56.43 KBkristiaanvandeneynde
#98 interdiff-89-98.txt2.18 KBkristiaanvandeneynde
#98 drupal-540008-98.patch55.08 KBkristiaanvandeneynde
#89 drupal-540008-89.patch55.06 KBkristiaanvandeneynde
#89 interdiff-86-89.txt5.17 KBkristiaanvandeneynde
#86 drupal-540008-86.patch50.17 KBkristiaanvandeneynde
#86 interdiff-78-86.txt1.18 KBkristiaanvandeneynde
#80 interdiff.txt7.62 KBmichaelfavia
#80 540008-uid1.patch53.82 KBmichaelfavia
#79 user-1-edit.png63.01 KBkristiaanvandeneynde
#79 roles.png126.6 KBkristiaanvandeneynde
#79 permissions.png35.62 KBkristiaanvandeneynde
#79 admin-people.png69.91 KBkristiaanvandeneynde
#79 account-settings.png113.79 KBkristiaanvandeneynde
#78 drupal-540008-78.patch49.24 KBkristiaanvandeneynde
#78 interdiff-76-78.txt3.24 KBkristiaanvandeneynde
#76 drupal-540008-76.patch46.63 KBkristiaanvandeneynde
#76 interdiff-74-76.txt2.68 KBkristiaanvandeneynde
#74 drupal-540008-74.patch47.66 KBkristiaanvandeneynde
#74 interdiff-70-74.txt5.91 KBkristiaanvandeneynde
#70 interdiff-67-70.txt5.86 KBkristiaanvandeneynde
#70 drupal-540008-70.patch44.45 KBkristiaanvandeneynde
#67 drupal-540008-67.patch38.9 KBkristiaanvandeneynde
#67 interdiff-65-67.txt6.96 KBkristiaanvandeneynde
#65 interdiff-62-65.txt13.84 KBkristiaanvandeneynde
#65 drupal-540008-65.patch34.05 KBkristiaanvandeneynde
#62 drupal-540008-62.patch21.65 KBkristiaanvandeneynde
#49 interdiff-47-49.txt1.41 KBkristiaanvandeneynde
#49 drupal-540008-49.patch22.7 KBkristiaanvandeneynde
#47 interdiff-45-47.txt14.88 KBkristiaanvandeneynde
#47 drupal-540008-47.patch22.36 KBkristiaanvandeneynde
#45 interdiff-42-45.txt5.05 KBkristiaanvandeneynde
#45 drupal-540008-45.patch10.84 KBkristiaanvandeneynde
#42 drupal-540008-42.patch6.08 KBkristiaanvandeneynde
#21 540008-21-remove-special-user-1.patch3.02 KBianthomas_uk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,162 pass(es), 255 fail(s), and 1 exception(s). View
#19 540008-19-remove-special-user-1.patch1.1 KBianthomas_uk
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,960 pass(es), 250 fail(s), and 1 exception(s). View
#13 540008_stop_special_casing_user_1.patch5.99 KBgreggles
FAILED: [[SimpleTest]]: [MySQL] 57,199 pass(es), 106 fail(s), and 35 exception(s). View
#4 user1sansmagic.patch5.4 KBmichaelfavia
FAILED: [[SimpleTest]]: [MySQL] 32,863 pass(es), 1 fail(s), and 3 exception(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

timmillwood’s picture

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

Bojhan’s picture

Yes.

michaelfavia’s picture

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.

michaelfavia’s picture

FileSize
5.4 KB
FAILED: [[SimpleTest]]: [MySQL] 32,863 pass(es), 1 fail(s), and 3 exception(es). View

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.

aaronbauman’s picture

Version: 7.x-dev » 8.x-dev

Too late for 7.x

michaelfavia’s picture

Status: Active » Needs review

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

Status: Needs review » Needs work

The last submitted patch, user1sansmagic.patch, failed testing.

kika’s picture

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.

michaelfavia’s picture

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?

webchick’s picture

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

greggles’s picture

Issue tags: +Security improvements

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.

tstoeckler’s picture

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".

greggles’s picture

FileSize
5.99 KB
FAILED: [[SimpleTest]]: [MySQL] 57,199 pass(es), 106 fail(s), and 35 exception(s). View

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.

greggles’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 540008_stop_special_casing_user_1.patch, failed testing.

joachim’s picture

> 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?

greggles’s picture

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?

greggles’s picture

Issue summary: View changes

summarize discussion to date

catch’s picture

Priority: Normal » Major
Issue summary: View changes
Issue tags: +D8 upgrade path

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.

ianthomas_uk’s picture

Status: Needs work » Needs review
FileSize
1.1 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,960 pass(es), 250 fail(s), and 1 exception(s). View

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).

Status: Needs review » Needs work

The last submitted patch, 19: 540008-19-remove-special-user-1.patch, failed testing.

ianthomas_uk’s picture

Status: Needs work » Needs review
Issue tags: +Novice
FileSize
3.02 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,162 pass(es), 255 fail(s), and 1 exception(s). View

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

Status: Needs review » Needs work

The last submitted patch, 21: 540008-21-remove-special-user-1.patch, failed testing.

finne’s picture

Assigned: Unassigned » finne

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

finne’s picture

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.

Berdir’s picture

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.

ianthomas_uk’s picture

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.

Berdir’s picture

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.

greggles’s picture

@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.

ianthomas_uk’s picture

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.

Berdir’s picture

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.

greggles’s picture

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.

finne’s picture

Issue summary: View changes
finne’s picture

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.

Patrick Storey’s picture

Issue tags: -Novice

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

michaelfavia’s picture

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.

Xano’s picture

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).

michaelfavia’s picture

@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.

mgifford’s picture

Assigned: finne » Unassigned

Unassigning stale issue. Hopefully someone else will pursue this.

catch’s picture

Version: 8.0.x-dev » 8.2.x-dev

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kristiaanvandeneynde’s picture

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:

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

kristiaanvandeneynde’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 42: drupal-540008-42.patch, failed testing.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
Issue tags: -D7UX, -D8 upgrade path
FileSize
10.84 KB
5.05 KB

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

Status: Needs review » Needs work

The last submitted patch, 45: drupal-540008-45.patch, failed testing.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
22.36 KB
14.88 KB

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.

Status: Needs review » Needs work

The last submitted patch, 47: drupal-540008-47.patch, failed testing.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
22.7 KB
1.41 KB

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.

michaelfavia’s picture

@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!

kristiaanvandeneynde’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 49: drupal-540008-49.patch, failed testing.

drakythe’s picture

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.

greggles’s picture

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.

michaelfavia’s picture

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?

drakythe’s picture

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

michaelfavia’s picture

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

drakythe’s picture

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.

greggles’s picture

+1 to #55.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review

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

kristiaanvandeneynde’s picture

Status: Needs review » Needs work
kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
21.65 KB

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.

catch’s picture

#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.

Status: Needs review » Needs work

The last submitted patch, 62: drupal-540008-62.patch, failed testing.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
34.05 KB
13.84 KB

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 :)

Status: Needs review » Needs work

The last submitted patch, 65: drupal-540008-65.patch, failed testing.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
6.96 KB
38.9 KB

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.

Status: Needs review » Needs work

The last submitted patch, 67: drupal-540008-67.patch, failed testing.

michaelfavia’s picture

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.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
44.45 KB
5.86 KB

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.

Status: Needs review » Needs work

The last submitted patch, 70: drupal-540008-70.patch, failed testing.

kristiaanvandeneynde’s picture

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
Wim Leers’s picture

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.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
5.91 KB
47.66 KB

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.

Status: Needs review » Needs work

The last submitted patch, 74: drupal-540008-74.patch, failed testing.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
2.68 KB
46.63 KB

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.

kristiaanvandeneynde’s picture

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.

kristiaanvandeneynde’s picture

FileSize
3.24 KB
49.24 KB

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.

kristiaanvandeneynde’s picture

FileSize
113.79 KB
69.91 KB
35.62 KB
126.6 KB
63.01 KB

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.
michaelfavia’s picture

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).

Status: Needs review » Needs work

The last submitted patch, 80: 540008-uid1.patch, failed testing.
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristiaanvandeneynde’s picture

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.

michaelfavia’s picture

@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 :).

kristiaanvandeneynde’s picture

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().

kristiaanvandeneynde’s picture

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 :)

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
Issue tags: -Usability, -Needs screenshots
FileSize
1.18 KB
50.17 KB

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.

Status: Needs review » Needs work

The last submitted patch, 86: drupal-540008-86.patch, failed testing. View results

kristiaanvandeneynde’s picture

Status: Needs work » Needs review

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.

kristiaanvandeneynde’s picture

FileSize
5.17 KB
55.06 KB
kristiaanvandeneynde’s picture

Retesting to see if it still applies.

kristiaanvandeneynde’s picture

johnennew’s picture

Status: Needs review » Reviewed & tested by the community

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

cilefen’s picture

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

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
  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.

kristiaanvandeneynde’s picture

Status: Needs work » Needs review

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.

kristiaanvandeneynde’s picture

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

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kristiaanvandeneynde’s picture

FileSize
55.08 KB
2.18 KB

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

kristiaanvandeneynde’s picture

FileSize
56.43 KB
1.6 KB

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

Status: Needs review » Needs work

The last submitted patch, 99: drupal-540008-99.patch, failed testing. View results

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
57.12 KB
1.93 KB

This should work.

heddn’s picture

Issue tags: +Needs change record

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.

SKAUGHT’s picture

Issue tags: +DX

@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.

tim.plunkett’s picture

timmillwood’s picture

Assigned: kristiaanvandeneynde » Unassigned

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.

SKAUGHT’s picture

true: change is hard sometimes.

kristiaanvandeneynde’s picture

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.
greggles’s picture

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.

andypost’s picture

kristiaanvandeneynde’s picture

Issue summary: View changes

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

harings_rob’s picture

  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

esolitos’s picture

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

efpapado’s picture

Status: Needs review » Needs work

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.
kristiaanvandeneynde’s picture

Status: Needs work » Needs review

@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.

kristiaanvandeneynde’s picture

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.

efpapado’s picture

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

eelkeblok’s picture

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.

kristiaanvandeneynde’s picture

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.

tacituseu’s picture

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

michaelfavia’s picture

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.

malcomio’s picture

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.

kristiaanvandeneynde’s picture

Issue tags: -Needs change record

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

ndf’s picture

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.

ndf’s picture

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?

daften’s picture

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

kristiaanvandeneynde’s picture

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.

kristiaanvandeneynde’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 101: drupal-540008-101.patch, failed testing. View results

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
57.11 KB

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

Status: Needs review » Needs work

The last submitted patch, 129: drupal-540008-129.patch, failed testing. View results

kristiaanvandeneynde’s picture

Status: Needs work » Needs review
FileSize
63.34 KB
6.49 KB

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.

webchick’s picture

Issue tags: -Needs usability review

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.

kristiaanvandeneynde’s picture

Thanks webchick and the rest of the UX team!

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

kristiaanvandeneynde’s picture

FileSize
66.14 KB
6.71 KB

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

kristiaanvandeneynde’s picture

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?

zaporylie’s picture

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

kristiaanvandeneynde’s picture

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>

zaporylie’s picture

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 * :)

SKAUGHT’s picture

*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.

kristiaanvandeneynde’s picture

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.

eelkeblok’s picture

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.

kristiaanvandeneynde’s picture

That could work.

zaporylie’s picture

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

kristiaanvandeneynde’s picture

FileSize
66.73 KB
7.48 KB

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?

kristiaanvandeneynde’s picture

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

vaplas’s picture

Many thanks to @kristiaanvandeneynde and all not indifferent here! This issue is super! ⭐⭐⭐

Honestly, I'm not a big expert, and I did not even know about the 'user 1' problem until I saw it with this issue. But now I'm very glad that it's going to fix state. Just look at these changes:

+++ b/core/lib/Drupal/Core/Cache/Context/UserRolesCacheContext.php
@@ -27,13 +27,6 @@ public static function getLabel() {
   public function getContext($role = NULL) {
...
-    if ($this->user->id() == 1) {
-      return 'is-super-user';
-    }

+++ b/core/lib/Drupal/Core/Session/UserSession.php
@@ -103,11 +103,6 @@ public function getRoles($exclude_locked_roles = FALSE) {
   public function hasPermission($permission) {
-    // User #1 has all privileges.
-    if ((int) $this->id() === 1) {
-      return TRUE;
-    }
-
     return $this->getRoleStorage()->isPermissionInRoles($permission, $this->getRoles());
   }

+++ b/core/modules/views/tests/src/Kernel/Plugin/RowRenderCacheTest.php
@@ -188,11 +188,10 @@ protected function doTestRenderedOutput(AccountInterface $account, $check_cache
-        $user_context = !$account->hasPermission('edit any test content') ? 'user' : 'user.permissions';
...
-            'contexts' => ['languages:language_interface', 'theme', $user_context],
+            'contexts' => ['languages:language_interface', 'theme', 'user.permissions'],

+++ b/core/profiles/standard/standard.install
@@ -5,7 +5,6 @@
-use Drupal\user\Entity\User;
...
@@ -28,11 +27,6 @@ function standard_install() {

@@ -28,11 +27,6 @@ function standard_install() {
-  // Assign user 1 the "administrator" role.
-  $user = User::load(1);
-  $user->roles[] = 'administrator';
-  $user->save();

They are wonderful!

+++ b/core/modules/user/src/AccountSettingsForm.php
@@ -104,8 +104,8 @@ public function buildForm(array $form, FormStateInterface $form_state) {
-    // Do not allow users to set the anonymous or authenticated user roles as the
-    // administrator role.
+    // Do not allow users to set the anonymous or authenticated user roles as
+    // the administrator role.

Hah! Achievement for opening this hiding place 🥇

'GOD' regim with uid 1 definitely a problem for the correct account settings. And, of course, still admire how this fix helps to identify a series of unluck tests! Great works again!


Going to post two additional review (one with major points, and one with minor points on #144 patch).
vaplas’s picture

3 major points:

1.
As already mentioned, this issue perfectly reveals a number of problems with our tests. It would be great to solve them separately. The current patch will become easier, incorrect tests will be quickly fixed (and possibly further improved). I'll be glad to open follow-up issues.

For test-only and prevent regression, we can just add unused user, like this:

test() {
...
+ // @todo: Remove $_user1 when https://www.drupal.org/node/540008.
+ $_user1 = User::create([]);
   $user = User::create([
...

2.
Looks like IsSuperUserCacheContext class still relies on the user1.

3.

+++ b/core/lib/Drupal/Core/Session/PermissionsHashGenerator.php
@@ -55,10 +66,14 @@ public function __construct(PrivateKey $private_key, CacheBackendInterface $cach
   public function generate(AccountInterface $account) {
-    // User 1 is the super user, and can always access all permissions. Use a
-    // different, unique identifier for the hash.
-    if ($account->id() == 1) {
-      return $this->hash('is-super-user');
+    // Admin roles have all permissions implicitly assigned. Use a different,
+    // unique identifier for the hash.
+    $storage = $this->entityTypeManager->getStorage('user_role');
+    foreach ($storage->loadMultiple($account->getRoles()) as $role) {
+      /** @var \Drupal\user\RoleInterface $role */
+      if ($role->isAdmin()) {
+        return $this->hash('is-admin');
+      }
     }

Why we need save this behavior? In other places of patch, such places are simply deleted. Do not harm the same hash key when there are several admin users?

vaplas’s picture

Minor points (i will glad to help with fix all of them):

  1. +++ b/core/modules/comment/tests/src/Kernel/CommentDefaultFormatterCacheTagsTest.php
    @@ -48,7 +50,7 @@ protected function setUp() {
         // Set the current user to one that can access comments. Specifically, this
         // user does not have access to the 'administer comments' permission, to
    ...
    -    $current_user->setAccount($this->createUser([], ['access comments']));
    +    $current_user->setAccount($this->createUser([], ['access comments', 'post comments']));
    

    Nit:

    -     // Set the current user to one that can access comments...
    +     //  Set the current user to one that can access and post comments...
  2. +++ b/core/modules/filter/tests/src/Kernel/TextFormatElementFormTest.php
    @@ -44,15 +44,16 @@ protected function setUp() {
    -    $role->grantPermission($filter_test_format->getPermissionName());
    +    foreach (FilterFormat::loadMultiple(['full_html', 'filtered_html', 'filter_test']) as $format) {
    

    It may be better just to check that other formats are inaccessible, instead of providing access to them:

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

    This is just one of those moments that it is great to solve in a separate issue with the participation of the mantainer (first point in #148)

  3. +++ b/core/modules/system/tests/src/Kernel/DateFormatAccessControlHandlerTest.php
    @@ -53,18 +53,10 @@ protected function setUp() {
       public function testAccess($which_user, $which_entity, $view_label_access_result, $view_access_result, $update_access_result, $delete_access_result, $create_access_result) {
    ...
    +    $permissions = ($which_user === 'admin')
    +      ? ['administer site configuration']
    +      : [];
    +    $user = $this->drupalCreateUser($permissions);
    

    Can we immediately set permissions in the data provider? Like:

    'permissionless + locked' => [
    -        'permissionless',
    +        []',
    ...
    'admin + unlocked' => [
    -        'admin',
    +        ['administer site configuration']
  4. +++ b/core/modules/user/src/Entity/User.php
    @@ -82,11 +82,14 @@ public function preSave(EntityStorageInterface $storage) {
    -    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]);
           }
         }
    

    #49: very useful information, can add as comment to patch?
    Also maybe nit renaming $internal/$locked. Because ADMINISTRATOR_ID also is internal, but not in this list.

  5. +++ b/core/modules/user/src/Tests/Update/UserUpdateUserOneAdminTest.php
    @@ -0,0 +1,106 @@
    +class UserUpdateUserOneAdminTest extends UpdatePathTestBase {
    

    It should be phpunit.

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

    nit rudiment in assert message:

    - 'Administration role defaults to none.'
    + 'Administration role defaults to "administration".'
    
  7. +++ b/core/modules/views/tests/src/Kernel/Handler/FieldFieldAccessTestBase.php
    @@ -42,7 +42,7 @@ protected function setUp($import_test_views = TRUE) {
    -      'permissions' => ['view test entity field'],
    +      'permissions' => ['view test entity field', 'access content'],
    

    There's no problem here, just wanted to be happy again that it's great. Once upon a time, I wanted to remove the 'view test entity field' while working on another issue. But @Wim Leers prevented me. Now I understand why the test worked without it. Fix user1 is really very useful step!

  8. +++ b/core/modules/views/tests/src/Kernel/Handler/FieldFieldTest.php
    @@ -74,7 +74,7 @@ protected function setUp($import_test_views = TRUE) {
    +    $this->adminUser = User::create(['name' => $this->randomString(), 'roles' => ['administrator']]);
    

    This should use the constant like #94.9.

  9. +++ b/core/tests/Drupal/KernelTests/Core/Render/RenderCacheTest.php
    @@ -32,63 +32,52 @@ protected function setUp() {
    -  public function testUser1PermissionContext() {
    +  public function testPermissionContext() {
    

    Maybe save 'User' in name of function, like :

    -  public function testUser1PermissionContext() {
    +  public function testUserPermissionContext() {
  10. #111: seems without feedback.
  11. Perhaps after the commit, it is worth open issue for clean-up for comments like user 1 (59 matches now), The "#1" admin user with replace on the 'admin'. Not now for avoid unnecessary noise
zaporylie’s picture

vaplas’s picture

@zaporylie, thank you! Can you made interdiff with diff --find-copies (or -M), for a more readable comparison of differences after convert to phpunit?

zaporylie’s picture

FileSize
1.18 KB

true...

kristiaanvandeneynde’s picture

Thanks for the thorough review vaplas, below are my answers.

147.1: No, this patch will break several tests because of the way they were written. Not fixing them properly here and instead offloading that to separate issues is a bad idea. We can't add something to core and then have every patch in the issue queue go red until we commit all of the separate test fixes.

147.2 Good catch! We can't remove that one though because it would be a backwards compatibility break. We could open up a separate issue for that one after this patch lands, asking for it to be marked as deprecated and making it return 0 all the time.

147.3: See #94 and other mentions of is-admin in this issue.

148.1: Why would you add 2 spaces there? Coding standards dictate only one leading space for a comment.

148.2: This patch fixes that test's permissions only. If you want to refactor it to make more sense, that should indeed happen in a separate issue.

148.3: Perhaps that could be moved to the data provider, yes. But same answer as 184.2: Separate issue :)

148.4: Even though #49 explains why the old code was broken, the new code is self-explanatory. Adding a comment saying why we can't use offsetUnset() would only confuse people reading the code that is actually there.

148.5: Yup, it should be. I couldn't find any tests for post-update hooks using PhpUnit so I went with the one that is used elsewhere by core. Thanks vaplas and zaporylie for taking care of that one.

148.6: Good catch, this needs fixing (patch attached).

148.7: Nothing to say here.

148.8: Good catch, this could be cleaned up (patch attached).

148.9: The cache context is based on user permissions, yes. But the full name should be user.roles.permissions (there's an issue about this). So then we'd end up with a function like testUserRolesPermissionContext. I'd rather keep it short, self-explanatory and independent of the cache context's internal name. That said, I don't care about this one that much. If people want to change it, that's fine.

148.10: The FileManagedUnitTestBase test could use some refactoring in createFile() to use something like $this->user->id() but other than that, its only vice is hard-coding a user ID. So that test doesn't need to be fixed here but in a separate issue. Using a db_query() in a test is the least of my concern right now. It's readable and easily picked up by a huge find-and-replace ticket when we're closer to 9.0.x. Feel free to fix it if you're really bothered by it, though.

148.11: Yup, completely agree. Some text clean-up will be needed but that's non-functional so can be done in a follow-up.

vaplas’s picture

Thanks for full answer @kristiaanvandeneynde. And sorry for delay with feedback:

147.1:
Yep. But we could:

  1. Сreate a parallel [meta] issue about the implicit use of user1 in the tests.
  2. Then open the issues for each problem test.
  3. After fixing all of them, somehow temp prevent the use user1 in test (example, via create two user in User::create, if the user/1 does not exist, or pre-creating the first user).

We do not need to remove any parts from this patch. Simply, after one of the associated issues is commited, remove part of it from here.
Pros:

  • Quick (i hope) fix problems with the check of permissions, which is now in our core tests.
  • Popularize the "user 1" problem.
  • Increase the synchronization between branches (if for some reason this issue is not comitted to both branches)
  • Simplify the study of the patch, because its size and area of change will decrease with each commit

Cons:

  • We need to expend energy on worries about this.

Of course, if someone from committers to say, that the fuss with the partition is not needed, I'll be glad!)

147.2: ok
147.3: I found only #94 and #95 with 'is-admin' word. I just wanted to draw attention to the fact that the:
before: 1 hash <-> 1 user (with uid 1),
after: 1 hash <-* many users (with admin roles)
But because isAdmin() always means all permissions. And saving the 'special hash' is necessary for BC (maybe, performance ). Ok. Sorry for noise.

148.1: Hah) looks really like this, but i want to point on

- access
+ access and post

This nit fix of comments after changed permissions:

-    $current_user->setAccount($this->createUser([], ['access comments']));
+    $current_user->setAccount($this->createUser([], ['access comments', 'post comments']));

148.2-11: Thank you for the explanations. Ok for me.

Added few additional issues about 'user 1' problem. TL;DR for me: without the user1 is better :)

vaplas’s picture

One more question. Now we can not delete the 'administrator' role, because

+++ b/core/modules/user/src/RoleAccessControlHandler.php
@@ -20,7 +20,12 @@ class RoleAccessControlHandler extends EntityAccessControlHandler {
       case 'delete':
...
+        $internal_roles = [
+          RoleInterface::ANONYMOUS_ID,
+          RoleInterface::AUTHENTICATED_ID,
+          RoleInterface::ADMINISTRATOR_ID,
+        ];
+        if (in_array($entity->id(), $internal_roles)) {

But we can change is-admin flag for this role:

class Role extends ConfigEntityBase implements RoleInterface {
...
  public function setIsAdmin($is_admin) {
    $this->is_admin = $is_admin;
    return $this;
  }

There may be a potential problem, when on site this role is turned into non-admin (it is not deleted, because it's impossible) and in some place of code will be use ADMINISTRATOR_ID (instead of isAdmin()).

It may be worthwhile to prevent the delete by isAdmin(), or prevent set is_admin flag to FALSE for ADMINISTRATOR_ID?

kristiaanvandeneynde’s picture

Re 153 re 147.1: I really don't see the benefit and think it will only add more red tape, preventing this issue from getting committed before 8.5.x so I'm sorry but I'm not going to be spending any time on that idea.

Re #154: Good catch, I'll attach a patch that does exactly this and removes the part of the UI that allows you to choose your administrator role at admin/config/people/accounts because that UI no longer makes sense.

kristiaanvandeneynde’s picture

FileSize
70.41 KB
5.15 KB

Okay so attached is a patch that removes the UI where you can choose which role is the Admin role. I did not put extra checks behind the use of the API methods isAdmin() and setIsAdmin() because of the fact that we also do not prevent anyone from deleting the Anonymous, Authenticated or Administrator role through code.

The code in RoleAccessControlHandler prevents you from deleting said roles through the Drupal admin UI, but you can still break your system by calling Role::load('anonymous')->delete(); so it would not make sense to babysit people regarding the isAdmin() flag when we don't do the same for the deletion of important roles. If you want to see this changed (in both cases), a follow-up issue may be the right place to do so.

I'll update the CR to list the UI change.

vaplas’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @kristiaanvandeneynde. Super!
Loss of flexibility in choosing an admin role seems like an excellent solution! The access rules must be clear and simple as a stick.

Of course, as the Challenge, you can think of a case where such switching will be useful. Example:

On the site there are two groups of people, between which you need to switch administrator permissions from time to time.

But this is an extremely specific case, which is easily implemented through the API:

Role::load('admin1')->setIsAdmin(FALSE)->save();
Role::load('admin2')->setIsAdmin(TRUE)->save();

It is more important not to create excessive noise in the settings, which can cause loss of control over the site.

Now we have hold 'Administrator' role by default, without the possibility to spoil it through the UI (which is available to untrained users), but with the easy possibility of changing through the API.

So, this issue gives the mass of profit:

  • preventing uncontrolled (and unclear) permissions of user/1.
  • fixing, preventing future occurrences, and preventing regression in test permission assertions.
  • terminationing misconduct in all contrib/custom code, where the permission logic does not include 'god mode' for the user/1.
  • allows you to remove the hacker's sight from user/1

And backward compatibility is saved, since user/1 continues to have the role of administrator.
And super-careful update (with support legacy transfer of exist role with same id, and simply configuration of name in rare case of double collisions).

It seems that the questions of other participants were also answered. Therefore, I pick up the status to RTBC, because the further movement depends on commiters.

Thanks for amazing works again!

kristiaanvandeneynde’s picture

Status: Reviewed & tested by the community » Needs review

Cool, thanks for the very thorough reviews vaplas!

kristiaanvandeneynde’s picture

Status: Needs review » Reviewed & tested by the community

Lol, my bad.

kristiaanvandeneynde’s picture

Out of curiosity: Who is able to commit this?

Because it's such a big patch I could understand them being a bit hesitant to commit it. So if they'd like to go over the thing as a whole, I'm willing to jump on a call and talk them through it.

Wim Leers’s picture

Shouldn't this also deprecate \Drupal\Core\Cache\Context\IsSuperUserCacheContext and mark it for removal in Drupal 9?

kristiaanvandeneynde’s picture

FileSize
71.39 KB
1.24 KB

Will do. I thought I'd do it in a follow-up as per #152:

147.2 Good catch! We can't remove that one though because it would be a backwards compatibility break. We could open up a separate issue for that one after this patch lands, asking for it to be marked as deprecated and making it return 0 all the time.

I've marked it as deprecated and made it return 0 all the time as there is now no longer a "superuser".

catch’s picture

Please move the test fixes into one spin-off issue that we can commit before this one. That will mean two smaller patches to review and will likely result in this issue getting committed sooner rather than later. I'm still catching up with the past couple of dozen comments or so and haven't reviewed the patch recently.

catch’s picture

Status: Reviewed & tested by the community » Needs work
kristiaanvandeneynde’s picture

Before I go ahead and do that: What do we do about new tests that break this patch?

Suppose I go through the effort of splitting this patch up into two parts and the test-fixes-only part gets committed. Then another few weeks pass before someone looks at this issue and in the mean time even more "broken" tests were added to core. That would mean this issue's patch would again go red until we fix those tests. At that point we'd be back to square one: Splitting off tests to make this issue readable, chasing HEAD for god knows how long.

Also: What about the gray area fixes? Tests that were broken but after the UID 1 change made no sense any more or needed drastic change. Do those remain in this issue's patch or go into the spin-off issue's patch as they may make that patch go red without the code from this patch.

Or shall I start by splitting off only those tests that have clear fixes to obvious problems?

catch’s picture

Suppose I go through the effort of splitting this patch up into two parts and the test-fixes-only part gets committed. Then another few weeks pass before someone looks at this issue and in the mean time even more "broken" tests were added to core. That would mean this issue's patch would again go red until we fix those tests. At that point we'd be back to square one: Splitting off tests to make this issue readable, chasing HEAD for god knows how long.

That can happen while this issue is being reviewed too, I don't see the difference. And it wouldn't be square one, because one or two new test failures doesn't suddenly reintroduce the already fixed ones.

Tests that were broken but after the UID 1 change made no sense any more or needed drastic change. Do those remain in this issue's patch or go into the spin-off issue's patch as they may make that patch go red without the code from this patch.

If there are test changes that are actually required for the patch it's fine to leave those in, that's a good reason to split the patch as well, since it makes it easier to review those changes vs. the fixes to tests with bad assumptions.

kristiaanvandeneynde’s picture

Cool, I'll start by splitting off the obvious ones then.

kristiaanvandeneynde’s picture

Title: Remove the special behavior of uid #1. » [PP-1] Remove the special behavior of uid #1.
Status: Needs work » Needs review
FileSize
52.21 KB

Okay so renaming this issue to reflect that it needs a different patch first: #2917584: Some tests only go green because they happen to run as UID1 Managed to split off quite a few tests, although this patch remains pretty big. Not running tests because they'd go red anyway.

Reasons why the remaining tests are still here (bold for edge case):

  • FunctionalTestSetupTrait: Test changed to reflect new behavior, i.e.: test wasn't broken
  • FilterDefaultConfigTest: Test changed to reflect new behavior, i.e.: test wasn't broken
  • RoleResourceTestBase: Test changed to reflect new behavior, i.e.: test wasn't broken
  • UninstallTest: Test changed to reflect new behavior, i.e.: test wasn't broken
  • DateFormatAccessControlHandlerTest: This test was written with this issue in mind (thanks Wim Leers), this patch removes the UID 1 special casing
  • MenuAccessControlHandlerTest: This test was written with this issue in mind (thanks Wim Leers), this patch removes the UID 1 special casing
  • UserPermissionsTest: Test changed to reflect new behavior, i.e.: test wasn't broken
  • UserUpdateUserOneAdminTest: This test was introduced by this patch
  • UserEntityTest: Test changed to reflect new behavior, i.e.: test wasn't broken
  • UserInstallTest: Test changed to reflect new behavior, i.e.: test wasn't broken
  • FieldFieldTest: This test was broken, but the fix is much cleaner here. See: #2917584-4: Some tests only go green because they happen to run as UID1
  • RowRenderCacheText: This test was broken, but the fix is much cleaner here. See: #2917584-4: Some tests only go green because they happen to run as UID1. Also, see comment #70
  • RenderCacheTest: This test was explicitly testing for UID1. That has been removed to reflect the new behavior.
  • PermissionsHashGeneratorTest: Test changed to reflect new behavior, i.e.: test wasn't broken
kristiaanvandeneynde’s picture

FileSize
52.97 KB
1.01 KB

I had to re-add the FileManagedUnitTestBase change here because we could not fix it separately in #2917584: Some tests only go green because they happen to run as UID1 due to it being a base class and thus we would be at risk of breaking BC. See #2917584-23: Some tests only go green because they happen to run as UID1 for more info (last paragraph of that comment).

So I've added it back into this issue's patch. Still do not test as this patch requires the other one to go in first.

David_Rothstein’s picture

As much as I like the premise behind this issue, I am concerned about the backwards compatibility implications of doing this in a stable release. As far as I can tell, committing this patch would basically mean that code which previously checked $user->id() == 1 in order to grant access to something would suddenly lead to an access bypass security issue on sites which take advantage of this new feature.

vaplas’s picture

#170: If I correctly understood the question, then @kristiaanvandeneynde answered it several times, example #115.

kristiaanvandeneynde’s picture

@David_Rothstein If anyone is writing code with $user->id() == 1 in it, then that code is the security issue. We've been training people for years now to use permission checks instead of role or UID checks. I've answered this quite a few times in this issue already: we're not breaking backwards compatibility in any way; we're merely exposing poorly written code.

The sooner this goes in before 8.5.0, the more time we'll have to start raising awareness. Suppose this lands by the end of the year, that will still give people 3 months to scan their code for god-mode checks and replace them with proper permission checks.

I really don't think we should count exposing bad code as a BC break. There are no API changes in this patch, only behavioral changes. And very bad behavior at that if I might say so myself :)

heddn’s picture

I think there's a point in #170. Many contrib modules followed what core has done for years and checked on user == 1. I even see a lot of custom code littered through many sites that do the same thing. The second this patch lands, we now have the potential for access bypass. Its unfortunate, isn't it. That by making things more secure, we run the risk of making things less secure. Can we get a coder rule written that checks for this type of behavior? It won't catch everything, but it might catch some. Something like $user->id == 1 || $user->id() == 1 is bad form.

I also see behat tests and drupal behat that assume this too. I'm asking us to brainstorm the worst case potential here and see if we can head any of it off.

eelkeblok’s picture

I wonder how big that problem is; I expect most modules - especially those written for D8 - will simply do access checks through the regular APIs. I have encountered code that specifically checks for uid = 1, but that was exactly because core treats it as special and thus it can't be treated as "just another user".

Either way, for any sites that are upgraded to 8.5.0, uid = 1 will get the admin role assigned, so functionally, there shouldn't be any change. Even if something goes wrong, uid = 1 being special is deeply ingrained in any workflow around any existing Drupal website, so if the user does not get the role assigned, it will be noticed rather quickly (and it will get less priviliges, not more).

What might be a problem is new sites, that do not make their uid 1 special (I'm not sure how the current patch handles this situation). Should they install one such module that does give uid 1 special powers, that would indeed be a security risk.

I wonder whether this is something that should be handled the same as API deprecation as much as possible. Obviously, there is no actual API to mark as deprecated, unfortunately, but the suggestion of adding this check to coder is probably a good one. Also, modules that do this will need to get the same treatment when a new security issue is found, at some point, I suppose. If they indeed allow certain functionality based on the user being logged in as uid 1 which should really be based on assigned roles. Maybe that point should be when they claim to be compatible with Drupal 9, with the background of treating it as any deprecated API. Also, maybe it means that uid = 1 should also get the admin role on new sites and that we may need to prevent uid=1 from being stripped of its god role just yet. Or, display a warning if it is and optionally provide a configuration key to turn that warning into an informational message.

vaplas’s picture

IMO, the fact that we can not fix this flaw for many years (11!) can not be an argument that it should remain so. And if the modules use magic numbers, instead of check of permissions, these are their problems. I've never seen Drupal recommend doing such checks.

#174:

I wonder how big that problem is

I downloaded all available modules for 8.x and checked various cases with uid/id/id() ===? 1. Near 37/4166 modules check user/1.

And it's really terrible! Because the examples of use really once again emphasize that this flaw needs to be eliminated as quickly as possible.

Random examplse:

  1. This provides part of their functionality for user/1 instead of admin users:
    http://cgit.drupalcode.org/simple_fb_connect/tree/src/SimpleFbConnectUse...
    protected function loginDisabledForAdmin(User $drupal_user) {
        // Check if current user is admin.
        if ($drupal_user->id() == 1) {
    
          // Check if admin FB login is disabled.
          if ($this->configFactory
            ->get('simple_fb_connect.settings')
            ->get('disable_admin_login')) {
    
            $this->loggerFactory
              ->get('simple_fb_connect')
              ->warning('Facebook login for user @user prevented. Facebook login for site administrator (user 1) is disabled in module settings.', ['@user' => $drupal_user->getAccountName()]);
            return TRUE;
          }
        }
    
        // User is not admin or admin login is not disabled.
        return FALSE;
      }
    
  2. This should use own permissions instead of magic number:
    http://cgit.drupalcode.org/photos/tree/photos.module?h=8.x-4.x#n53
          // Value is fid, check if user can view this photo's album.
          if ($user->id() == 1) {
            return TRUE;
          }
    
  3. This based on non-existent super admin:
    http://cgit.drupalcode.org/search_config/tree/search_config.node.inc?h=8...
      // Return the form for super admin unchanged
      if ($user->id() == 1 && !empty($settings['restrictions']['admin_bypass'])) {
        return $form;
      }
  4. This just for prevent user/1 mode.=:
    http://cgit.drupalcode.org/profile/tree/tests/src/Kernel/ProfileRoleAcce...
        // Do not allow uid == 1 to skew tests.
        $this->createUser();

Full list of files where this was found (some modules are obviously outdated):

Should they install one such module that does give uid 1 special powers, that would indeed be a security risk.

Yes, it will be a problem on sites where user/1 will change on no-admin and used one of these unsecure-modules. Now I understand what point @David_Rothstein and @heddn. But contrib modules are always full of security flaws, and we have a time of wide notification about this.

eelkeblok’s picture

Thanks for doing that investigation.

But contrib modules are always full of security flaws, and we have a time of wide notification about this.

That's too easy, though :) From one day to the next, apparently 37 modules will have a security issue. Going through a few entries in your list, there's at least a few that are covered by the security policy, so they will either need a security update at some point (which point? There is probably going to need to be a deadline, since this is basically a zero-day issue of our own creation), or they will need to be marked as abandoned.

Edit: I just deleted two paragraphs about handling this as a deprecation because I realised there is an essential difference with stuff that is deprecated; that is usually the previous "right" way of doing things. If this ever was the "right" way of doing things, that's a long time ago. So I guess it is basically about determining the deadline for module developers to fix their modules if they want to keep their security team support.

eelkeblok’s picture

(Removed, irrelevant).

andypost’s picture

+++ b/core/lib/Drupal/Core/Cache/Context/UserRolesCacheContext.php
@@ -27,13 +27,6 @@ public static function getLabel() {
   public function getContext($role = NULL) {
-    // User 1 does not actually have any special behavior for roles; this is
-    // added as additional security and backwards compatibility protection for
-    // SA-CORE-2015-002.
-    // @todo Remove in Drupal 9.0.0.
-    if ($this->user->id() == 1) {
-      return 'is-super-user';

Also some contrib could rely on "is-super-user" cache context value

rootwork’s picture

37 modules seems like a manageable thing, to the point that we could just contact all of them and point them to a change record with proper code they could drop in (i.e. even if they don't have security coverage). This doesn't seem like a reason to me not to fix this.

eelkeblok’s picture

I don't think anyone said it's a reason not to fix this problem.

kristiaanvandeneynde’s picture

Also, maybe it means that uid = 1 should also get the admin role on new sites and that we may need to prevent uid=1 from being stripped of its god role just yet. Or, display a warning if it is and optionally provide a configuration key to turn that warning into an informational message.

UID 1 will receive the admin role on new sites, it's in the patch. We discussed adding a warning or preventing the removal of a role in this issue before and even the usability review thought it was fine to not bother.

From one day to the next, apparently 37 modules will have a security issue.

No, they already have one. One we're conveniently masking because UID1 = god and one that will continue to be masked because the update hook in the patch makes sure UID1 will receive the admin role post-update.

Also some contrib could rely on "is-super-user" cache context value

Which is why the context isn't going anywhere. It's merely returning a value which is consistent with the new behavior. It's basically asking whether the cache object needs to vary by super user. Seeing as we no longer have one, it makes sense for the context to always respond with "no".

@vaplas in #175: Awesome detective work! <3

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.