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 its 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: #3057483: Better describe how SynchronizableInterface should be used for content entities

Personal opinion

I still feel 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 #3438901: [Meta] Plan for deprecating and eventually removing the super user access policy 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.

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

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 because it is added before any permissions are checked (right?). 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. It would also completely rule out any security edge cases we may be overlooking here.

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,
];

Seeing as there is no persistent cache in the installer, 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 real caching to the installer soon.

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

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.

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?

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.