Super user access policy and the installer

Okay so after a lengthy Slack conversation, a few things were brought up:

Summary

Whatever solution we come up with, we know that the super user access policy is one day going away and so will the accompanying security.enable_super_user parameter on the container. So whatever we decide to cook up here should preferably not rely on that parameter because it's more code we later on have to deprecate and/or remove again.

A counterpoint was that the installer will always have to run as a super user but not everyone agreed on that. However, if we do decide to keep that behavior, we could still remove the super user access policy, but move it's logic to an @internal access policy that is only turned on during the installer.

Another disputed point was also made that any content import, whether via default_content, migration, etc.) failing entity validation without user 1 should probably be fixed in the code that handles said import. We can see an example of this in the migrate module's EntityContentBase destination's validateEntity() method, which uses the account switcher before validating content.

Finally, the option was raised that the proper solution could be to set importing content to isSyncing(TRUE) and make sure that any import-supporting code such as the migrate module takes said flag into account. This would take time, tough, so for the time being we'd still need a solution for this issue, even if temporary.

Also keep in mind there still doesn't seem to be a documented consensus on what isSyncing means for content entities, see: 📌 [PP-2] Better describe how SynchronizableInterface should be used for content entities Needs work

Personal opinion

I still as if we should consider not changing any code right now. We advise people using content_import, migrate et al to not disable the super user access policy during their content import. For D11 we change the default behavior for contrib tests to not run with the super user access policy, which core tests already do, meaning modules such as default_content will now start seeing failing tests.

We then encourage these modules to fix their code and tests in the D11 lifecycle and then we remove the super user access policy altogether in D12, provided we addressed everything in 🌱 [Meta] Plan for deprecating and eventually removing the super user access policy Active by then.

This way, we don't have to touch the installer at all, we buy at least two more years to work on the isSyncing stuff and contrib gets test fails starting from D11, telling them their code only works because they rely on UID 1.

All in all, I still feel we can one day have a Drupal core where relying on a super user is a thing of the past. Relying on an admin role being perfectly fine, though.

@kristiaanvandeneynde we can't hold off on tackling this - people have test pipelines and development set ups that install from configuration.

Addendum to point 5... and such setups need to be able to set the super user access policy to false to be like production.

Having said all of the above, one thing I do very much like about this MR is the removal of the admin role from standard_install() and demo_umami_install() to move it someplace central. All installations should get this admin role out of the box and it should become an opt-out rather than the opt-in it currently is; i.e.: Profiles currently have to remember to ship with an admin role, which feels like poor DX.

Making it so all installations get an admin role would also be a way of somehow keeping the super user behavior during install: Simply assign said admin role to the current user being used for the install, which should almost always be UID 1.

Purely based on this the MR might need work because demo_umami_install() still has the same code as standard_install(). Also, both standard and demo_umami also still have the config entity in their config/install folder, we might need to remove those too then.

Okay, how about a second container parameter: security.enable_super_user_during_install

So you can keep it on during install, but off after that. We change SuperUserAccessPolicyPass to account for it as such:

  public function process(ContainerBuilder $container): void {
    if ($container->getParameter('drupal.is_installing') === TRUE) {
      $enable_policy = $container->getParameter('security.enable_super_user_during_install');
    }
    else {
      $enable_policy = $container->getParameter('security.enable_super_user');
    }
    if ($enable_policy === FALSE) {
      $container->removeDefinition('access_policy.super_user');
      $container->removeAlias('Drupal\Core\Session\SuperUserAccessPolicy');
    }
  }

Then in core/lib/Drupal/Core/Installer/NormalInstallerServiceProvider.php, rather than setting security.enable_super_user to TRUE, you set drupal.is_installing to TRUE.

The bonus with this approach is that we could still make it so contrib tests run with security.enable_super_user_during_install disabled for D11, but keep it enabled in D10.3

Profiles currently have to remember to ship with an admin role, which feels like poor DX.

Well once everything is using recipes they can use standard's admin role recipe for this.

The MR kind of implements #8 - but no additional container parameter - just an even more limited access policy that only works during the installer.

The MR has test coverage and re-instates the 10.2.x installer behaviour of having a user 1 with admin access. It consolidates add the admin role to user 1 if an admin role exists.

Well once everything is using recipes they can use standard's admin role recipe for this.

Oh, that would be perfect.

I had a quick review and the cache contexts on the new access policy aren't 100% correct. However, when introducing a new access policy, you need to make sure there are no cache entries, so this wouldn't be an issue. Seeing as after the installer the access policy goes away and caches should be cleared (right?), there is also no risk of polluting the cache with the wrong value.

But from a developer example point of view, we should make sure all of core's access policies show the right cache contexts. Setting to NW for that reason, but could be convinced otherwise.

The tests look really nice too, but I'd stick to one test and use a data provider to set the code, flag and expected results.
E.g.:

$cases['no-super-user-access'] = [
  'install_code' => <<<PHP
      <?php
      function {$this->profile}_install() {
        \$user = \Drupal\user\Entity\User::load(1);
        \Drupal::state()->set('admin_permission_in_installer', \$user->hasPermission('administer software updates'));
      }
      PHP,
  'super-user-policy' => FALSE,
  'expect_user_has_permission_after_install' => FALSE,
  'expect_user_has_permission_during_install' => TRUE,
  'expect_page_shows_no_access_warning' => TRUE,
];

Dataproviders and the InstallerTestBase - too much happens in setUp() and buried in there.

Cache contexts in the installer are moot - we only use a memory cache for everything - there's no permanent caching.

Alex just informed me there is no cache in the installer so I'm not mega-fussed about the missing cache context. On the one hand it sets a bad example, on the other it's @internal and I don't see us adding caches to the installer soon.

We should probably also document the plans for the future on 🌱 [Meta] Plan for deprecating and eventually removing the super user access policy Active .

Some things we discussed were to remove the super user from regular Drupal but keep it in installer until we find a way to enable developers to update their code. This could be further exploring the isSyncing() route discussed in #4. Either way, if we commit the MR here we need to make sure it's kept track of in that meta issue.

Fine, setting to NR again. From my POV it's +1 now, but @catch was also involved in the discussion so we should perhaps have him take a look at this too.

Looks great to me now. Leaving at NR for:

I think we need to improve this message. Maybe point to a page of Drupal.org about administering your site with no admin role.

But no other objections.

Looks good to me. Ubernit on a grammar mistake, but that can be applied on commit.

alexpott → credited Taran2L → .

Crediting @Taran2l for a suggestion in slack.

Does this also need a separate change record to tell other install profiles that they don't need to explicitly set a user 1 role any more?

alexpott → credited catch → .

@longwave great point about the CR - created https://www.drupal.org/node/3443437 →

Gave this another look over, all the nits I found are in, small enough changes I'm going to go ahead. Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks! Nice way to solve this, simplifies install profiles, doesn't box us in for later.

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