Remove the special behavior of UID#1

Just mentioning that this could finally be considered fixed if/when 🌱 [Meta, Plan] Pitch-Burgh: Policy based access in core Active lands.

At that point, UID 1 being special is just an access policy like any other in the system and you could choose to turn this off on a per-site basis. So we wouldn't be shoving this change down everyone's throats, but could rather document and recommend how to turn this off on production sites.

The only thing we would have to do to make that fully possible is find all of the places in core where a UID 1 check happens and update them to properly check for a permission instead. That effort is already taken care of in part by the patches here, so looks like we could finally land this issue if/when we get access policies in core.

More specifically here is how we could fix this issue:

1. Commit access policies into core, turning UID1 in an access policy
2. Make all tests run with the UID 1 access policy service turned off so that we are sure we test the right permissions
3. Update code where UID 1 is still being checked and turn into permission check
4. Once all of core tests go green with UID 1 turned off, close this issue
5. Write documentation on how to turn off UID 1 on your (production) site and recommend you do so

Doing all of the above means we get to keep UID 1 for those who are used to relying on it, but also that those who want a more secure website can turn off UID 1's god mode and we can guarantee that all of core works with UID 1 turned off.

Doing all of the above means we get to keep UID 1 for those who are used to relying on it

No, it means that Drupal's contributors and security team now need to support two different admin situations, with one of them being less secure. UID 1 is not something that should remain an admin account option. If someone wants the same functionality, they can create a user with UID 1 if one doesn't exist, and assign it the new admin permission/role.

No, it means that Drupal's contributors and security team now need to support two different admin situations

That's simply not true. With how the new access policy system works, we'd simply support permission checks, no matter how many or few permissions you have. By turning off the UID 1 access policy during tests, we make sure people don't accidentally think their module is safe when in fact UID 1 was giving false results.

Given how much backlash there has already been in this thread, I'd say what I suggested in #331 is a nice first step. It addresses the concerns of people in this thread regarding being locked out of their site (turn on the service again and you're in) and keeps the old behavior OOTB. But, you'd be able to turn off UID 1's god mode without having to patch or break core.

In a later release we could still discuss removing the service altogether, but for now I think this would be a nice way to gradually move away from old habits.

Re #333 I agree. It sounds like a good plan, especially as those who care can reinstate the old behaviour in a forward-compatible way.

Absolutely in support of #540008-331: Remove the special behavior of UID#1 → .

+1 for #540008-331: Remove the special behavior of UID#1 →

kristiaanvandeneynde → changed the visibility of the branch 10.3.x to hidden.

kristiaanvandeneynde → changed the visibility of the branch 11.x to hidden.

kristiaanvandeneynde → changed the visibility of the branch 9.2.x to hidden.

kristiaanvandeneynde → changed the visibility of the branch 9.3.x to hidden.

Right let's give this a spin then. Started a new branch and MR because of how old the other one was and we have a different starting point now: the SuperUserAccessPolicy, which landed in core yesterday.

Seems like it's not properly removing the service yet...

Right, got it to work for Kernel tests. Now need to test locally if it also works for browser tests.

Okay this is awesome.

We can now simply flag the failing tests rather than having to fix them here. This allows us to fix them in individual (perhaps Novice-tagged) follow-ups. It's crazy how small this MR is for what it's doing and that all thanks to the new Access Policy API.

Unit tests are failing but there is no reported failure in the log o.O

Okay if I add $this->assertFalse($this->rootUser->hasPermission('foo')); to a browser test it correctly finds that UID 1 does not have all permissions. But I got a feeling that the actual pages we're visiting still have UID 1 as god mode.

Yeah, this leads to a status code of 200 in the second assertion:

  public function testSuperUser() {
    $this->drupalget('admin');
    $this->assertSession()->statusCodeEquals(403);
    $this->drupalLogin($this->rootUser);
    $this->drupalget('admin');
    $this->assertSession()->statusCodeEquals(403);
  }

Seems like I need to figure out how to persist the container parameter.

Still working on figuring the browser tests out but unassigning so someone else can have a go if they better understand what's going on

Never mind, somewhere rootUser is getting the administrator role. Turns out all of our browser tests are already super-user-safe in a sense that they do not test with UID 1 by accident. They do, however, test with an admin account sometimes but that's totally fine.

Tried adding the following crappy test and it went green:

  public function testSuperUser() {
    $this->drupalget('admin');
    $this->assertSession()->statusCodeEquals(403);

    $user = User::load(1);
    $user->removeRole('administrator');
    $user->save();

    $this->drupalLogin($this->rootUser);
    $this->drupalget('admin');
    $this->assertSession()->statusCodeEquals(403);
  }

So that means this MR takes care of browser tests and kernel tests 🎉

Updated the issue summary as I feel this issue can now be considered "done". Provided we figure out why the unit tests are failing seemingly for no reason. Tagging as needs followup for all the tests that need refactoring and the recovery.php script.

The follow-up should adjust hook_file_url_alter() to no longer mention UID1 and replace MigrateAccessCheck. IsSuperUserCacheContext was obviously going to be deleted in the follow-up and 🐛 UserRolesCacheContext can lead to poisoned cache returns for user 1 Active takes care of the UserRolesCacheContext.

I found no other mentions of UID 1 during a brief look, but it's up to the followup issue to be thorough on that.

Aha, now we're seeing progress. The browser test fails I was expecting are coming through in droves

Seems to be feedback. But confused on the follow up to fix tests? Feel they would have to be fixed here to be merged.

So currently these tests only go green because they accidentally test with user 1. The changes I made makes these tests go red, but if you add the lines:

  /**
   * {@inheritdoc}
   *
   * @todo Remove and fix test to not rely on super user.
   */
  protected $usesSuperUserAccessPolicy = TRUE;

The super user access policy is turned on for that test again, making it go green once again.

This enables us to commit the changes here without bloating the MR with dozens of test changes. It's already at 40 files changed and I have yet to change even more Functional tests (but ran out of time Friday evening). I'll get to that on Tuesday probably or later this week.

Then, we indeed need a meta issue to track all of the needed changes to these tests, probably one issue per test (hopefully tagged as Novice).

Then probably would need it’s own feature branch, don’t know the policy around those. But can’t imagine we can merge to 11.x with test failures.

@smustgrave, it is a side effect of removing the special privileges of user 1 that tests are identified that need those privileges. That is a good thing and adding protected $usesSuperUserAccessPolicy = TRUE just allows the test to pass using the same privileges it had before this change. So, in effect is not changing the test. It ran with elevated privileges before and this MR is continuing that. However, we want to check these tests and see if they can be modified to run without special privileges. And this last part can be done in a followup task. I hope that helps.

If the follow ups are novice level I've seen a few issues getting tagged for GreeceSpringSprint2024, may be a good opportunity.

There are 8 files actually being changed, meaning we have found 71 broken tests so far. That's going to be a tall followup list in the meta issue.

Think the nightwatch tests are failing because of this:

exports.command = function drupalInstallModule(module, force, callback) {
  const self = this;
  this.drupalLoginAsAdmin(() => {
    this.drupalRelativeURL('/admin/modules')

I've never worked with Nightwatch tests before so trying to figure out how to get to the PHP version of drupalLoginAsAdmin

Okay, found the culprit:

exports.command = function drupalLoginAsAdmin(callback) {
  const self = this;
  this.drupalUserIsLoggedIn((sessionExists) => {
    if (sessionExists) {
      this.drupalLogout();
    }
    const userLink = execSync(
      commandAsWebserver(
        `php ./scripts/test-site.php user-login 1 --site-path ${this.globals.drupalSitePath}`,
      ),
    );

Ends up calling TestSiteUserLoginCommand, which is intended for logging in a user, not logging in an admin user. So this part of every Nighwatch test is relying on UID 1 being an admin, which it no longer is unless TestSiteApplication uses the standard or umami_demo profile, where user 1 is assigned the admin role.

So let's see if we can either assign UID 1 the admin role or rethink whether we should use drupalLoginAsAdmin() at all. Perhaps that rethinking can be done in a follow-up.

Hmm, so I figured out where Nightwatch is going wrong but I'm not sure I can fix it. I tried adding an extra argument to TestSiteUserLoginCommand as a stopgap, but that's not being picked up it seems.

All green now, will leave the Nightwatch stuff to the JavaScript experts in a dedicated followup.

Followup created and added as an @see to all @todos: 📌 [Meta] Fix all tests that rely on UID1's super user behavior Active

Updated the CR: https://www.drupal.org/node/2910500 →

As far as I am concerned this issue is now "done". It:

• Allows people to turn off the super user on their website
• Turns off the super user for all browser, kernel and nightwatch tests unless opted out
• Opts out all failing core tests, so they can be taken care of in the followup meta issue

The simplicity of turning off UID1 now compared to the old patches/MR is a testament to how powerful the new Access Policy API is.

kristiaanvandeneynde → changed the visibility of the branch 540008-remove-the-special to hidden.

Can the todos point to https://www.drupal.org/project/drupal/issues/3437620 📌 [Meta] Fix all tests that rely on UID1's super user behavior Active please :)

Re #367 I actually did that to get the list of tests for the meta issue but forgot to commit :)

Changes seem good. For the META talking an event organizer for a camp in Greece so maybe a few of them can get knocked out.

I don't think we should be changing the behaviour in contrib and custom tests without going through some sort of deprecation process. This change could break an organisations test suite and cost them work.

We should default the FunctionalTestSetupTrait $usesSuperUserAccessPolicy property to NULL and then do something like:

      if ($this->usesSuperUserAccessPolicy === NULL) {
        $test_file_name = (new \ReflectionClass($this))->getFileName();
        // @todo Decide in https://www.drupal.org/project/drupal/issues/TO_OPEN when/how to trigger deprecation errors or even failures for contrib modules.
        $this->usesSuperUserAccessPolicy = str_starts_with($test_file_name, $this->root . DIRECTORY_SEPARATOR . 'core');
      }

Just before it is used in the trait. This will allow contrib and core tests to adopt is a needed.

Done, see 📌 Decide on when/how we will run contrib/custom tests without the super user access policy Active

Implemented suggestion verbatim bar the negation, which was needed. Tests go green on CI, confirmed that core tests run correctly locally. So back to RTBC I suppose.

I think the code is good to go now... but the CR just needs a few improvements and we need
The CR needs to be updated the explain the test situation. I.e. non core tests still have the super user policy enabled and uid 1 is special. Core tests do not. And it needs to explain the flag and how to set it false if you don;t want your tests to rely on uid 1 specialness.

Please we need to create follow-up to:

1. issue a deprecation if the test flag is set to anything but TRUE... I think the deprecation should be for Drupal 12.
2. Discuss recovery steps eg. the document page or recovery.php idea (I'm deeply suspicious of the recovery.php idea because that's a recipe for an attack target)

Can set back to rtbc once that's done.

Updated the CR and created a few new issues. All of them are now grouped under the following meta issue: 🌱 [Meta] Plan for deprecating and eventually removing the super user access policy Active

Adding a release note. Still need to sort out issue credit... that might take a while.

Hahaha I can imagine. 15 years and 375 comments later...

Updating issue credit. I've tried to credit everyone who has made constructive contributions to the issue including disagreeing to previous patches. Obviously this is subjective so if I've made a mistake please let me know.

Re-titling so that the issue title matches what is happening here and updating issue summary to be correct.

Committed and pushed 9baa43976c to 11.x and 97c8bdc7ec to 10.3.x. Thanks!

Awesome, thanks! 🎉

Automatically closed - issue fixed for 2 weeks with no activity.

Revived ✨ Add a new function to Drupal Core: user_is_admin Active . I think we need a shorthand to determine whether a user is an admin