Issue #3442024 by phenaproxima, alexpott, larowlan, amateescu: Account...

Issue #3442024 by phenaproxima, alexpott, larowlan, amateescu: Account switching to user 1 is now fraught

(cherry picked from commit d20f0ea5)

.recipes.gitlab-ci.yml

@@ -20,7 +20,7 @@ variables:
   MYSQL_PASSWORD: drupaltestbotpw
   # Note if you add anything to the lists below you will need to change the root
   # phpunit.xml.dist file.
-  TEST_DIRECTORIES: "core/tests/Drupal/Tests/Core/Recipe core/tests/Drupal/KernelTests/Core/Recipe core/tests/Drupal/FunctionalTests/Core/Recipe core/tests/Drupal/KernelTests/Core/Config/Action core/tests/Drupal/KernelTests/Core/Config/Storage/Checkpoint core/tests/Drupal/Tests/Core/Config/Checkpoint core/tests/Drupal/Tests/Core/Config/Action core/modules/content_moderation/tests/src/Kernel/ConfigAction core/modules/ckeditor5/tests/src/Kernel/ConfigAction core/tests/Drupal/Tests/Core/DefaultContent core/tests/Drupal/FunctionalTests/DefaultContent"
+  TEST_DIRECTORIES: "core/tests/Drupal/Tests/Core/Recipe core/tests/Drupal/KernelTests/Core/Recipe core/tests/Drupal/FunctionalTests/Core/Recipe core/tests/Drupal/KernelTests/Core/Config/Action core/tests/Drupal/KernelTests/Core/Config/Storage/Checkpoint core/tests/Drupal/Tests/Core/Config/Checkpoint core/tests/Drupal/Tests/Core/Config/Action core/modules/content_moderation/tests/src/Kernel/ConfigAction core/modules/ckeditor5/tests/src/Kernel/ConfigAction core/tests/Drupal/Tests/Core/DefaultContent core/tests/Drupal/KernelTests/Core/DefaultContent core/tests/Drupal/FunctionalTests/DefaultContent"
   CODE_DIRECTORIES: "core/lib/Drupal/Core/Recipe core/lib/Drupal/Core/Config/Action core/modules/config/tests/config_action_duplicate_test core/tests/fixtures/recipes core/lib/Drupal/Core/Config/Checkpoint core/modules/content_moderation/src/Plugin/ConfigAction core/modules/ckeditor5/src/Plugin/ConfigAction core/lib/Drupal/Core/DefaultContent"
   ALL_DIRECTORIES: "${CODE_DIRECTORIES} ${TEST_DIRECTORIES}"
 

core/core.services.yml

@@ -73,6 +73,11 @@ services:
     arguments: ['@config.manager', '@config.storage', '@config.typed', '@config.factory']
   Drupal\Core\DefaultContent\Importer:
     autowire: true
+  Drupal\Core\DefaultContent\AdminAccountSwitcher:
+    arguments:
+      $isSuperUserAccessEnabled: '%security.enable_super_user%'
+    autowire: true
+    public: false
   # Simple cache contexts, directly derived from the request context.
   cache_context.ip:
     class: Drupal\Core\Cache\Context\IpCacheContext

core/lib/Drupal/Core/DefaultContent/AdminAccountSwitcher.php

+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\Core\DefaultContent;
+
+use Drupal\Core\Access\AccessException;
+use Drupal\Core\Entity\EntityTypeManagerInterface;
+use Drupal\Core\Session\AccountInterface;
+use Drupal\Core\Session\AccountSwitcherInterface;
+
+/**
+ * @internal
+ *   This API is experimental.
+ */
+final class AdminAccountSwitcher implements AccountSwitcherInterface {
+
+  public function __construct(
+    private readonly AccountSwitcherInterface $decorated,
+    private readonly EntityTypeManagerInterface $entityTypeManager,
+    private readonly bool $isSuperUserAccessEnabled,
+  ) {}
+
+  /**
+   * Switches to an administrative account.
+   *
+   * This will switch to the first available account with a role that has the
+   * `is_admin` flag. If there are no such roles, or no such users, this will
+   * try to switch to user 1 if superuser access is enabled.
+   *
+   * @return \Drupal\Core\Session\AccountInterface
+   *   The account that was switched to.
+   *
+   * @throws \Drupal\Core\Access\AccessException
+   *   Thrown if there are no users with administrative roles.
+   */
+  public function switchToAdministrator(): AccountInterface {
+    $admin_roles = $this->entityTypeManager->getStorage('user_role')
+      ->getQuery()
+      ->condition('is_admin', TRUE)
+      ->execute();
+
+    $user_storage = $this->entityTypeManager->getStorage('user');
+
+    if ($admin_roles) {
+      $accounts = $user_storage->getQuery()
+        ->accessCheck(FALSE)
+        ->condition('roles', $admin_roles, 'IN')
+        ->condition('status', 1)
+        ->sort('uid')
+        ->range(0, 1)
+        ->execute();
+    }
+    else {
+      $accounts = [];
+    }
+    $account = $user_storage->load(reset($accounts) ?: 1);
+    assert($account instanceof AccountInterface);
+
+    if (array_intersect($account->getRoles(), $admin_roles) || ((int) $account->id() === 1 && $this->isSuperUserAccessEnabled)) {
+      $this->switchTo($account);
+      return $account;
+    }
+    throw new AccessException("There are no user accounts with administrative roles.");
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function switchTo(AccountInterface $account): AccountSwitcherInterface {
+    $this->decorated->switchTo($account);
+    return $this;
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function switchBack(): AccountSwitcherInterface {
+    $this->decorated->switchBack();
+    return $this;
+  }
+
+}

core/lib/Drupal/Core/DefaultContent/Importer.php

@@ -13,7 +13,6 @@
 use Drupal\Core\File\FileSystemInterface;
 use Drupal\Core\Installer\InstallerKernel;
 use Drupal\Core\Language\LanguageManagerInterface;
-use Drupal\Core\Session\AccountSwitcherInterface;
 use Drupal\file\FileInterface;
 use Drupal\link\Plugin\Field\FieldType\LinkItem;
 use Drupal\user\EntityOwnerInterface;
@@ -42,7 +41,7 @@ final class Importer implements LoggerAwareInterface {
 
   public function __construct(
     private readonly EntityTypeManagerInterface $entityTypeManager,
-    private readonly AccountSwitcherInterface $accountSwitcher,
+    private readonly AdminAccountSwitcher $accountSwitcher,
     private readonly FileSystemInterface $fileSystem,
     private readonly LanguageManagerInterface $languageManager,
     private readonly EntityRepositoryInterface $entityRepository,
@@ -71,48 +70,50 @@ public function importContent(Finder $content, Existing $existing = Existing::Er
       return;
     }
 
-    /** @var \Drupal\user\UserInterface $root_user */
-    $root_user = $this->entityTypeManager->getStorage('user')->load(1);
-    $this->accountSwitcher->switchTo($root_user);
-
-    /** @var array{_meta: array<mixed>} $decoded */
-    foreach ($content->data as $decoded) {
-      ['uuid' => $uuid, 'entity_type' => $entity_type_id, 'path' => $path] = $decoded['_meta'];
-      assert(is_string($uuid));
-      assert(is_string($entity_type_id));
-      assert(is_string($path));
-
-      $entity_type = $this->entityTypeManager->getDefinition($entity_type_id);
-      /** @var \Drupal\Core\Entity\EntityTypeInterface $entity_type */
-      if (!$entity_type->entityClassImplements(ContentEntityInterface::class)) {
-        throw new ImportException("Content entity $uuid is a '$entity_type_id', which is not a content entity type.");
-      }
+    $account = $this->accountSwitcher->switchToAdministrator();
+
+    try {
+      /** @var array{_meta: array<mixed>} $decoded */
+      foreach ($content->data as $decoded) {
+        ['uuid' => $uuid, 'entity_type' => $entity_type_id, 'path' => $path] = $decoded['_meta'];
+        assert(is_string($uuid));
+        assert(is_string($entity_type_id));
+        assert(is_string($path));
 
-      $entity = $this->entityRepository->loadEntityByUuid($entity_type_id, $uuid);
-      if ($entity) {
-        if ($existing === Existing::Skip) {
-          continue;
+        $entity_type = $this->entityTypeManager->getDefinition($entity_type_id);
+        /** @var \Drupal\Core\Entity\EntityTypeInterface $entity_type */
+        if (!$entity_type->entityClassImplements(ContentEntityInterface::class)) {
+          throw new ImportException("Content entity $uuid is a '$entity_type_id', which is not a content entity type.");
         }
-        else {
-          throw new ImportException("$entity_type_id $uuid already exists.");
+
+        $entity = $this->entityRepository->loadEntityByUuid($entity_type_id, $uuid);
+        if ($entity) {
+          if ($existing === Existing::Skip) {
+            continue;
+          }
+          else {
+            throw new ImportException("$entity_type_id $uuid already exists.");
+          }
         }
-      }
 
-      $entity = $this->toEntity($decoded)->enforceIsNew();
+        $entity = $this->toEntity($decoded)->enforceIsNew();
 
-      // Ensure that the entity is not owned by the anonymous user.
-      if ($entity instanceof EntityOwnerInterface && empty($entity->getOwnerId())) {
-        $entity->setOwner($root_user);
-      }
+        // Ensure that the entity is not owned by the anonymous user.
+        if ($entity instanceof EntityOwnerInterface && empty($entity->getOwnerId())) {
+          $entity->setOwnerId($account->id());
+        }
 
-      // If a file exists in the same folder, copy it to the designated
-      // target URI.
-      if ($entity instanceof FileInterface) {
-        $this->copyFileAssociatedWithEntity($path, $entity);
+        // If a file exists in the same folder, copy it to the designated
+        // target URI.
+        if ($entity instanceof FileInterface) {
+          $this->copyFileAssociatedWithEntity($path, $entity);
+        }
+        $entity->save();
       }
-      $entity->save();
     }
-    $this->accountSwitcher->switchBack();
+    finally {
+      $this->accountSwitcher->switchBack();
+    }
   }
 
   private function copyFileAssociatedWithEntity(string $path, FileInterface $entity): void {

core/tests/Drupal/FunctionalTests/Core/Recipe/CoreRecipesTest.php

@@ -64,6 +64,7 @@ public function providerApplyRecipe(): iterable {
    * @dataProvider providerApplyRecipe
    */
   public function testApplyRecipe(string $path): void {
+    $this->setUpCurrentUser(admin: TRUE);
     $this->applyRecipe($path);
   }
 

core/tests/Drupal/FunctionalTests/Core/Recipe/StandardRecipeTest.php

@@ -7,7 +7,6 @@
 use Drupal\contact\Entity\ContactForm;
 use Drupal\shortcut\Entity\Shortcut;
 use Drupal\Tests\standard\Functional\StandardTest;
-use Drupal\user\Entity\User;
 use Drupal\user\RoleInterface;
 
 /**
@@ -55,7 +54,9 @@ public function testStandard(): void {
     // Clean up roles before recipe import.
     $storage = \Drupal::entityTypeManager()->getStorage('user_role');
     $roles = $storage->loadMultiple();
-    unset($roles[RoleInterface::ANONYMOUS_ID], $roles[RoleInterface::AUTHENTICATED_ID]);
+    // Do not delete the administrator role. There would be no user with the
+    // permissions to create content.
+    unset($roles[RoleInterface::ANONYMOUS_ID], $roles[RoleInterface::AUTHENTICATED_ID], $roles['administrator']);
     $storage->delete($roles);
 
     $this->applyRecipe('core/recipes/standard');
@@ -73,7 +74,6 @@ public function testStandard(): void {
 
     // Add a Home link to the main menu as Standard expects "Main navigation"
     // block on the page.
-    User::load(1)->addRole('administrator')->save();
     $this->drupalLogin($this->rootUser);
     $this->drupalGet('admin/structure/menu/manage/main/add');
     $this->submitForm([

core/tests/Drupal/FunctionalTests/DefaultContent/ContentImportTest.php

@@ -70,6 +70,7 @@ class ContentImportTest extends BrowserTestBase {
    */
   protected function setUp(): void {
     parent::setUp();
+    $this->setUpCurrentUser(admin: TRUE);
 
     BlockContentType::create(['id' => 'basic', 'label' => 'Basic'])->save();
     block_content_add_body_field('basic');
@@ -215,10 +216,11 @@ private function assertContentWasImported(): void {
     $this->assertSame('Useful Info', $block_content->label());
     $this->assertSame("<p>I'd love to put some useful info here.</p>", $block_content->body->value);
 
-    // A node with a non-existent owner should be reassigned to user 1.
+    // A node with a non-existent owner should be reassigned to the current
+    // user.
     $node = $entity_repository->loadEntityByUuid('node', '7f1dd75a-0be2-4d3b-be5d-9d1a868b9267');
     $this->assertInstanceOf(NodeInterface::class, $node);
-    $this->assertSame('1', $node->getOwner()->id());
+    $this->assertSame(\Drupal::currentUser()->id(), $node->getOwner()->id());
 
     // Ensure a node with a translation is imported properly.
     $node = $entity_repository->loadEntityByUuid('node', '2d3581c3-92c7-4600-8991-a0d4b3741198');

core/tests/Drupal/KernelTests/Core/DefaultContent/AdminAccountSwitcherTest.php

+<?php
+
+declare(strict_types=1);
+
+namespace Drupal\KernelTests\Core\DefaultContent;
+
+use Drupal\Core\Access\AccessException;
+use Drupal\Core\DependencyInjection\ContainerBuilder;
+use Drupal\Core\Entity\EntityTypeManagerInterface;
+use Drupal\Core\DefaultContent\AdminAccountSwitcher;
+use Drupal\Core\Session\AccountSwitcherInterface;
+use Drupal\KernelTests\KernelTestBase;
+use Drupal\Tests\user\Traits\UserCreationTrait;
+
+/**
+ * @covers \Drupal\Core\DefaultContent\AdminAccountSwitcher
+ * @group DefaultContent
+ */
+class AdminAccountSwitcherTest extends KernelTestBase {
+
+  use UserCreationTrait;
+
+  /**
+   * {@inheritdoc}
+   */
+  protected static $modules = ['system', 'user'];
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp(): void {
+    parent::setUp();
+    $this->installEntitySchema('user');
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function register(ContainerBuilder $container): void {
+    parent::register($container);
+    $container->getDefinition(AdminAccountSwitcher::class)->setPublic(TRUE);
+  }
+
+  /**
+   * Tests switching to a user with an administrative role.
+   */
+  public function testSwitchToAdministrator(): void {
+    /** @var \Drupal\Core\Session\AccountInterface $account */
+    $account = $this->createUser(admin: TRUE);
+
+    $this->assertSame($account->id(), $this->container->get(AdminAccountSwitcher::class)->switchToAdministrator()->id());
+    $this->assertSame($account->id(), $this->container->get('current_user')->id());
+  }
+
+  /**
+   * Tests that there is an error if there are no administrative users.
+   */
+  public function testNoAdministratorsExist(): void {
+    /** @var \Drupal\Core\Session\AccountInterface $account */
+    $account = $this->createUser();
+    $this->assertSame(1, (int) $account->id());
+
+    $this->expectException(AccessException::class);
+    $this->expectExceptionMessage("There are no user accounts with administrative roles.");
+    $this->container->get(AdminAccountSwitcher::class)->switchToAdministrator();
+  }
+
+  /**
+   * Tests switching to user 1 when the superuser access policy is enabled.
+   */
+  public function testSuperUser(): void {
+    /** @var \Drupal\Core\Session\AccountInterface $account */
+    $account = $this->createUser();
+    $this->assertSame(1, (int) $account->id());
+
+    $switcher = new AdminAccountSwitcher(
+      $this->container->get(AccountSwitcherInterface::class),
+      $this->container->get(EntityTypeManagerInterface::class),
+      TRUE,
+    );
+    $this->assertSame(1, (int) $switcher->switchToAdministrator()->id());
+  }
+
+  public function testSwitchToAndSwitchBack(): void {
+    $this->assertTrue($this->container->get('current_user')->isAnonymous());
+
+    /** @var \Drupal\Core\Session\AccountInterface $account */
+    $account = $this->createUser();
+    $switcher = $this->container->get(AdminAccountSwitcher::class);
+    $this->assertSame($switcher, $switcher->switchTo($account));
+    $this->assertSame($account->id(), $this->container->get('current_user')->id());
+
+    $this->assertSame($switcher, $switcher->switchBack());
+    $this->assertTrue($this->container->get('current_user')->isAnonymous());
+  }
+
+}

phpstan-level9-baseline.neon

@@ -415,11 +415,6 @@ parameters:
 			count: 1
 			path: core/modules/content_moderation/src/Plugin/ConfigAction/AddModerationDeriver.php
 
-		-
-			message: "#^Cannot call method addRole\\(\\) on Drupal\\\\user\\\\Entity\\\\User\\|null\\.$#"
-			count: 1
-			path: core/tests/Drupal/FunctionalTests/Core/Recipe/StandardRecipeTest.php
-
 		-
 			message: "#^PHPDoc tag @var for variable \\$sync_data has no value type specified in iterable type array\\.$#"
 			count: 1

phpunit.xml.dist

@@ -72,6 +72,7 @@
       <directory>core/modules/content_moderation/tests/src/Kernel/ConfigAction</directory>
       <directory>core/modules/ckeditor5/tests/src/Kernel/ConfigAction</directory>
       <directory>core/tests/Drupal/Tests/Core/DefaultContent</directory>
+      <directory>core/tests/Drupal/KernelTests/Core/DefaultContent</directory>
       <directory>core/tests/Drupal/FunctionalTests/DefaultContent</directory>
     </testsuite>
   </testsuites>