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

Created on 4 August 2009, over 14 years ago
Updated 9 April 2024, 4 days ago

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.

Also, because of the long legacy of this in core, both contrib modules and custom code in individual sites might (will, in some cases, see #175) have adopted similar code to grant uid=1 special privileges. When core no longer makes any assumptions about the specialness of uid=1, site maintainers may now strip uid=1 from its admin role, thinking it is now a normal user, only to have hard-coded god-funcitonality lingering around their site.

Proposed resolution

The SuperUserAccessPolicy is put behind a container parameter which keeps the policy on by default for D10. In D11, we either turn it off by default as user 1 gets the Administrator role in standard_install() or we completely remove the SuperUserAccessPolicy.

By that point (the removal) we should, however, have made a recovery.php which is behind a flag in the settings file similar to update.php (update_free_access). If someone locks themselves out of their website, we can use said script to assign the Administrator role to a user of their choosing. if the Administrator role no longer exists, the script will create one. This script should be developed in a follow-up

Remaining tasks

  1. Put the SuperUserAccessPolicy behind a parameter on the container - DONE
  2. Toggle this off for all browser and kernel tests - DONE
  3. Find all failing tests and flag them - DONE
  4. Create an issue πŸ“Œ [Meta] Fix all tests that rely on UID1's super user behavior Active - DONE
  5. Create a followup to create recovery.php and remove the toggle along with the access policy

API changes

None

Release notes snippet

User 1's special privileges are now part of the SuperUserAccessPolicy, which can be turned off. From Drupal 10.3.0 onwards, you can toggle this behavior in your default.services.yml file β†’ by setting the container parameter security.enable_super_user to false.

πŸ“Œ Task
Status

Fixed

Version

10.3 ✨

Component
BaseΒ  β†’

Last updated about 8 hours ago

Created by

πŸ‡³πŸ‡±Netherlands Bojhan

Live updates comments and jobs are added and updated live.
  • Security improvements

    It makes Drupal less vulnerable to abuse or misuse. Note, this is the preferred tag, though the Security tag has a large body of issues tagged to it. Do NOT publicly disclose security vulnerabilities; contact the security team instead. Anyone (whether security team or not) can apply this tag to security improvements that do not directly present a vulnerability e.g. hardening an API to add filtering to reduce a common mistake in contributed modules.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates : some issue and comment data are missing.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • πŸ‡¬πŸ‡§United Kingdom rivimey

    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.

  • πŸ‡­πŸ‡ΊHungary Balu Ertl Budapest πŸ‡ͺπŸ‡Ί
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    kristiaanvandeneynde β†’ changed the visibility of the branch 10.3.x to hidden.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    kristiaanvandeneynde β†’ changed the visibility of the branch 11.x to hidden.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    kristiaanvandeneynde β†’ changed the visibility of the branch 9.2.x to hidden.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    kristiaanvandeneynde β†’ changed the visibility of the branch 9.3.x to hidden.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • Pipeline finished with Failed
    15 days ago
    Total: 525s
    #132432
  • Pipeline finished with Failed
    15 days ago
    Total: 581s
    #132473
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

  • Pipeline finished with Canceled
    15 days ago
    Total: 410s
    #132485
  • Pipeline finished with Failed
    15 days ago
    Total: 492s
    #132492
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • Pipeline finished with Failed
    15 days ago
    Total: 831s
    #132570
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • Issue was unassigned.
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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

  • Status changed to Needs review 15 days ago
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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 πŸŽ‰

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • Pipeline finished with Failed
    15 days ago
    Total: 172s
    #132653
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • Pipeline finished with Failed
    15 days ago
    Total: 1660s
    #132665
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

  • Pipeline finished with Failed
    15 days ago
    Total: 1136s
    #132690
  • Status changed to Needs work 13 days ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

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

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    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.

  • πŸ‡³πŸ‡ΏNew Zealand quietone New Zealand

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

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

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

  • Status changed to Needs review 11 days ago
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • Pipeline finished with Failed
    11 days ago
    Total: 971s
    #135130
  • Pipeline finished with Failed
    11 days ago
    Total: 1002s
    #135175
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • Pipeline finished with Failed
    11 days ago
    Total: 1080s
    #135246
  • Status changed to Needs work 11 days ago
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • Pipeline finished with Failed
    11 days ago
    Total: 957s
    #135293
  • Pipeline finished with Failed
    11 days ago
    Total: 1106s
    #135313
  • Pipeline finished with Canceled
    11 days ago
    Total: 88s
    #135347
  • Pipeline finished with Failed
    11 days ago
    Total: 174s
    #135351
  • Pipeline finished with Success
    11 days ago
    Total: 1045s
    #135378
  • Status changed to Needs review 11 days ago
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    kristiaanvandeneynde β†’ changed the visibility of the branch 540008-remove-the-special to hidden.

  • Status changed to Needs work 10 days ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

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

  • Status changed to Needs review 10 days ago
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

  • Pipeline finished with Success
    10 days ago
    Total: 992s
    #136007
  • Status changed to RTBC 10 days ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    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.

  • Status changed to Needs work 10 days ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    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.

  • Status changed to Needs review 10 days ago
  • Pipeline finished with Canceled
    10 days ago
    Total: 307s
    #136565
  • Pipeline finished with Success
    10 days ago
    Total: 991s
    #136574
  • Status changed to RTBC 10 days ago
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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.

  • Status changed to Needs work 8 days ago
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    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.

  • Status changed to RTBC 8 days ago
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    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

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

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

  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

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

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    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.

  • Pipeline finished with Running
    4 days ago
    #141533
  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

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

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

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

  • Status changed to Fixed 4 days ago
    • alexpott β†’ committed 97c8bdc7 on 10.3.x
      Issue #540008 by kristiaanvandeneynde, Spokje, daffie, clayfreeman,...
    • alexpott β†’ committed 9baa4397 on 11.x
      Issue #540008 by kristiaanvandeneynde, Spokje, daffie, clayfreeman,...
  • πŸ‡§πŸ‡ͺBelgium kristiaanvandeneynde Antwerp, Belgium

    Awesome, thanks! πŸŽ‰

Production build https://api.contrib.social 0.62.1