From f0fda63b583ca7d497c248026e47b2781fca026c Mon Sep 17 00:00:00 2001 From: Alex Pott <alex.a.pott@googlemail.com> Date: Mon, 7 Oct 2024 16:12:31 +0100 Subject: [PATCH] Issue #3478332 by phenaproxima, nicxvan, thejimbirch, alexpott: Add a way to prevent recipes' imported config from being compared too strictly to active config --- .../Core/Recipe/AllowListConfigStorage.php | 131 ++++++++++++++++ .../Drupal/Core/Recipe/ConfigConfigurator.php | 51 +++++- core/lib/Drupal/Core/Recipe/Recipe.php | 10 ++ .../Recipe/RecipeExtensionConfigStorage.php | 146 ------------------ .../config/user.role.administrator.yml | 8 + core/recipes/administrator_role/recipe.yml | 10 +- .../config/user.role.content_editor.yml | 8 + core/recipes/content_editor_role/recipe.yml | 14 +- .../Core/Recipe/RecipeTestTrait.php | 18 +++ .../Core/Recipe/ConfigConfiguratorTest.php | 117 ++++++++++++++ .../Core/Recipe/RecipeValidationTest.php | 52 +++++++ 11 files changed, 396 insertions(+), 169 deletions(-) create mode 100644 core/lib/Drupal/Core/Recipe/AllowListConfigStorage.php delete mode 100644 core/lib/Drupal/Core/Recipe/RecipeExtensionConfigStorage.php create mode 100644 core/recipes/administrator_role/config/user.role.administrator.yml create mode 100644 core/recipes/content_editor_role/config/user.role.content_editor.yml diff --git a/core/lib/Drupal/Core/Recipe/AllowListConfigStorage.php b/core/lib/Drupal/Core/Recipe/AllowListConfigStorage.php new file mode 100644 index 000000000000..da8255beaec5 --- /dev/null +++ b/core/lib/Drupal/Core/Recipe/AllowListConfigStorage.php @@ -0,0 +1,131 @@ +<?php + +declare(strict_types=1); + +namespace Drupal\Core\Recipe; + +use Drupal\Core\Config\StorageInterface; + +/** + * A read-only storage wrapper that only allows access to certain config names. + * + * @internal + * This API is experimental. + */ +final class AllowListConfigStorage implements StorageInterface { + + /** + * @param \Drupal\Core\Config\StorageInterface $decorated + * A config storage backend to wrap around. + * @param string[] $allowList + * A list of config names. Only these names will be visible, or readable, + * by this storage. Cannot be empty. + */ + public function __construct( + private readonly StorageInterface $decorated, + private readonly array $allowList, + ) { + if (empty($allowList)) { + throw new \LogicException('AllowListConfigStorage cannot be constructed with an empty allow list.'); + } + } + + /** + * {@inheritdoc} + */ + public function exists($name): bool { + if (in_array($name, $this->allowList, TRUE)) { + return $this->decorated->exists($name); + } + return FALSE; + } + + /** + * {@inheritdoc} + */ + public function read($name): array|false { + return $this->exists($name) ? $this->decorated->read($name) : FALSE; + } + + /** + * {@inheritdoc} + */ + public function readMultiple(array $names): array { + $names = array_intersect($names, $this->allowList); + return $this->decorated->readMultiple($names); + } + + /** + * {@inheritdoc} + */ + public function write($name, array $data): never { + throw new \BadMethodCallException('This storage is read-only.'); + } + + /** + * {@inheritdoc} + */ + public function delete($name): never { + throw new \BadMethodCallException('This storage is read-only.'); + } + + /** + * {@inheritdoc} + */ + public function rename($name, $new_name): never { + throw new \BadMethodCallException('This storage is read-only.'); + } + + /** + * {@inheritdoc} + */ + public function encode($data): string { + return $this->decorated->encode($data); + } + + /** + * {@inheritdoc} + */ + public function decode($raw): array { + return $this->decorated->decode($raw); + } + + /** + * {@inheritdoc} + */ + public function listAll($prefix = ''): array { + return array_intersect($this->decorated->listAll($prefix), $this->allowList); + } + + /** + * {@inheritdoc} + */ + public function deleteAll($prefix = ''): never { + throw new \BadMethodCallException('This storage is read-only.'); + } + + /** + * {@inheritdoc} + */ + public function createCollection($collection): static { + return new static( + $this->decorated->createCollection($collection), + $this->allowList, + ); + } + + /** + * {@inheritdoc} + */ + public function getAllCollectionNames(): array { + return $this->decorated->getAllCollectionNames(); + } + + /** + * {@inheritdoc} + */ + public function getCollectionName(): string { + return $this->decorated->getCollectionName(); + } + +} diff --git a/core/lib/Drupal/Core/Recipe/ConfigConfigurator.php b/core/lib/Drupal/Core/Recipe/ConfigConfigurator.php index 1a46e601e6c0..ad837d5dc35c 100644 --- a/core/lib/Drupal/Core/Recipe/ConfigConfigurator.php +++ b/core/lib/Drupal/Core/Recipe/ConfigConfigurator.php @@ -5,6 +5,7 @@ namespace Drupal\Core\Recipe; use Drupal\Core\Config\FileStorage; +use Drupal\Core\Config\NullStorage; use Drupal\Core\Config\StorageInterface; /** @@ -15,6 +16,8 @@ final class ConfigConfigurator { public readonly ?string $recipeConfigDirectory; + private readonly bool|array $strict; + /** * @param array $config * Config options for a recipe. @@ -25,8 +28,20 @@ final class ConfigConfigurator { */ public function __construct(public readonly array $config, string $recipe_directory, StorageInterface $active_configuration) { $this->recipeConfigDirectory = is_dir($recipe_directory . '/config') ? $recipe_directory . '/config' : NULL; + // @todo Consider defaulting this to FALSE in https://drupal.org/i/3478669. + $this->strict = $config['strict'] ?? TRUE; + $recipe_storage = $this->getConfigStorage(); - foreach ($recipe_storage->listAll() as $config_name) { + if ($this->strict === TRUE) { + $strict_list = $recipe_storage->listAll(); + } + else { + $strict_list = $this->strict ?: []; + } + + // Everything in the strict list needs to be identical in the recipe and + // active storage. + foreach ($strict_list as $config_name) { if ($active_data = $active_configuration->read($config_name)) { // @todo https://www.drupal.org/i/3439714 Investigate if there is any // generic code in core for this. @@ -90,10 +105,10 @@ public function getConfigStorage(): StorageInterface { $module_list = \Drupal::service('extension.list.module'); /** @var \Drupal\Core\Extension\ThemeExtensionList $theme_list */ $theme_list = \Drupal::service('extension.list.theme'); - foreach ($this->config['import'] as $extension => $config) { + foreach ($this->config['import'] as $extension => $names) { // If the recipe explicitly does not want to import any config from this // extension, skip it. - if ($config === NULL) { + if ($names === NULL) { continue; } $path = match (TRUE) { @@ -101,12 +116,36 @@ public function getConfigStorage(): StorageInterface { $theme_list->exists($extension) => $theme_list->getPath($extension), default => throw new \RuntimeException("$extension is not a theme or module") }; - $config = $config === '*' ? [] : $config; - $storages[] = new RecipeExtensionConfigStorage($path, $config); + + $storage = new RecipeConfigStorageWrapper( + new FileStorage($path . '/config/install'), + new FileStorage($path . '/config/optional'), + ); + // If we get here, $names is either '*', or a list of config names + // provided by the current extension. In the latter case, we only want + // to import the config that is in the list, so use an + // AllowListConfigStorage to filter out the extension's other config. + if ($names && is_array($names)) { + $storage = new AllowListConfigStorage($storage, $names); + } + $storages[] = $storage; } } + $storage = RecipeConfigStorageWrapper::createStorageFromArray($storages); + + if ($this->strict) { + return $storage; + } + // If we're not in strict mode, we only want to import config that doesn't + // exist yet in active storage. - return RecipeConfigStorageWrapper::createStorageFromArray($storages); + $names = array_diff( + $storage->listAll(), + \Drupal::service('config.storage')->listAll(), + ); + return $names + ? new AllowListConfigStorage($storage, $names) + : new NullStorage(); } /** diff --git a/core/lib/Drupal/Core/Recipe/Recipe.php b/core/lib/Drupal/Core/Recipe/Recipe.php index f7fb0e2da79b..89e7b0ed742d 100644 --- a/core/lib/Drupal/Core/Recipe/Recipe.php +++ b/core/lib/Drupal/Core/Recipe/Recipe.php @@ -257,6 +257,16 @@ private static function parse(string $file): array { ]), ]), ]), + 'strict' => new Optional([ + new AtLeastOneOf([ + new Type('boolean'), + new All([ + new Type('string'), + new NotBlank(), + new Regex('/^.+\./'), + ]), + ], message: 'This value must be a boolean, or a list of config names.', includeInternalMessages: FALSE), + ]), 'actions' => new Optional([ new All([ new Type('array'), diff --git a/core/lib/Drupal/Core/Recipe/RecipeExtensionConfigStorage.php b/core/lib/Drupal/Core/Recipe/RecipeExtensionConfigStorage.php deleted file mode 100644 index 604b566d7655..000000000000 --- a/core/lib/Drupal/Core/Recipe/RecipeExtensionConfigStorage.php +++ /dev/null @@ -1,146 +0,0 @@ -<?php - -declare(strict_types=1); - -namespace Drupal\Core\Recipe; - -use Drupal\Core\Config\FileStorage; -use Drupal\Core\Config\StorageInterface; - -/** - * Allows the recipe to select configuration from the module. - * - * @internal - * This API is experimental. - */ -final class RecipeExtensionConfigStorage implements StorageInterface { - - protected readonly StorageInterface $storage; - - /** - * @param string $extensionPath - * The path extension to read configuration from - * @param array $configNames - * The list of config to read from the extension. An empty array means all - * configuration. - * @param string $collection - * (optional) The collection to store configuration in. Defaults to the - * default collection. - */ - public function __construct(protected readonly string $extensionPath, protected readonly array $configNames, protected readonly string $collection = StorageInterface::DEFAULT_COLLECTION) { - $this->storage = new RecipeConfigStorageWrapper( - new FileStorage($this->extensionPath . '/config/install', $this->collection), - new FileStorage($this->extensionPath . '/config/optional', $this->collection), - $collection - ); - } - - /** - * {@inheritdoc} - */ - public function exists($name): bool { - if (!empty($this->configNames) && !in_array($name, $this->configNames, TRUE)) { - return FALSE; - } - return $this->storage->exists($name); - } - - /** - * {@inheritdoc} - */ - public function read($name): array|bool { - if (!empty($this->configNames) && !in_array($name, $this->configNames, TRUE)) { - return FALSE; - } - return $this->storage->read($name); - } - - /** - * {@inheritdoc} - */ - public function readMultiple(array $names): array { - if (!empty($this->configNames)) { - $names = array_intersect($this->configNames, $names); - } - return $this->storage->readMultiple($names); - } - - /** - * {@inheritdoc} - */ - public function write($name, array $data): bool { - throw new \BadMethodCallException(); - } - - /** - * {@inheritdoc} - */ - public function delete($name): bool { - throw new \BadMethodCallException(); - } - - /** - * {@inheritdoc} - */ - public function rename($name, $new_name): bool { - throw new \BadMethodCallException(); - } - - /** - * {@inheritdoc} - */ - public function encode($data): string { - throw new \BadMethodCallException(); - } - - /** - * {@inheritdoc} - */ - public function decode($raw): array { - throw new \BadMethodCallException(); - } - - /** - * {@inheritdoc} - */ - public function listAll($prefix = ''): array { - $names = $this->storage->listAll($prefix); - if (!empty($this->configNames)) { - $names = array_intersect($this->configNames, $names); - } - return $names; - } - - /** - * {@inheritdoc} - */ - public function deleteAll($prefix = ''): bool { - throw new \BadMethodCallException(); - } - - /** - * {@inheritdoc} - */ - public function createCollection($collection): static { - return new static( - $this->extensionPath, - $this->configNames, - $collection - ); - } - - /** - * {@inheritdoc} - */ - public function getAllCollectionNames(): array { - return $this->storage->getAllCollectionNames(); - } - - /** - * {@inheritdoc} - */ - public function getCollectionName(): string { - return $this->collection; - } - -} diff --git a/core/recipes/administrator_role/config/user.role.administrator.yml b/core/recipes/administrator_role/config/user.role.administrator.yml new file mode 100644 index 000000000000..ca48a58b4eed --- /dev/null +++ b/core/recipes/administrator_role/config/user.role.administrator.yml @@ -0,0 +1,8 @@ +langcode: en +status: true +dependencies: { } +id: administrator +label: Administrator +weight: 3 +is_admin: true +permissions: { } diff --git a/core/recipes/administrator_role/recipe.yml b/core/recipes/administrator_role/recipe.yml index 5392d24bfedb..24d178494ae6 100644 --- a/core/recipes/administrator_role/recipe.yml +++ b/core/recipes/administrator_role/recipe.yml @@ -2,11 +2,5 @@ name: 'Administrator role' description: 'Provides the Administrator role.' type: 'User role' config: - actions: - user.role.administrator: - # If this role already exists, then this action has no effect. If it doesn't exist, we'll create it with the following values. - createIfNotExists: - id: administrator - label: Administrator - weight: 3 - is_admin: true + # If the administrator role already exists, we don't really care what it looks like. + strict: false diff --git a/core/recipes/content_editor_role/config/user.role.content_editor.yml b/core/recipes/content_editor_role/config/user.role.content_editor.yml new file mode 100644 index 000000000000..f14dc36a914f --- /dev/null +++ b/core/recipes/content_editor_role/config/user.role.content_editor.yml @@ -0,0 +1,8 @@ +langcode: en +status: true +dependencies: { } +id: content_editor +label: 'Content editor' +weight: 2 +is_admin: false +permissions: { } diff --git a/core/recipes/content_editor_role/recipe.yml b/core/recipes/content_editor_role/recipe.yml index 947b930c277e..8fabcf38f5f7 100644 --- a/core/recipes/content_editor_role/recipe.yml +++ b/core/recipes/content_editor_role/recipe.yml @@ -2,14 +2,10 @@ name: 'Content editor role' description: 'Provides the Content editor role.' type: 'User role' config: + # If the content_editor role already exists, we don't really care what it looks like. + strict: false actions: user.role.content_editor: - # If this role already exists, then this action has no effect. If it doesn't exist, we'll create it with the following values. - createIfNotExists: - id: content_editor - label: 'Content editor' - weight: 2 - is_admin: false - permissions: - - 'access administration pages' - - 'view own unpublished content' + grantPermissions: + - 'access administration pages' + - 'view own unpublished content' diff --git a/core/tests/Drupal/FunctionalTests/Core/Recipe/RecipeTestTrait.php b/core/tests/Drupal/FunctionalTests/Core/Recipe/RecipeTestTrait.php index 156b5df00858..5d086ad0f795 100644 --- a/core/tests/Drupal/FunctionalTests/Core/Recipe/RecipeTestTrait.php +++ b/core/tests/Drupal/FunctionalTests/Core/Recipe/RecipeTestTrait.php @@ -97,4 +97,22 @@ protected function applyRecipe(string $path, int $expected_exit_code = 0, array return $process; } + /** + * Alters an existing recipe. + * + * @param string $path + * The recipe directory path. + * @param callable $alter + * A function that will receive the decoded contents of recipe.yml as an + * array. This should returned a modified array to be written to recipe.yml. + */ + protected function alterRecipe(string $path, callable $alter): void { + $file = $path . '/recipe.yml'; + $this->assertFileExists($file); + $contents = file_get_contents($file); + $contents = Yaml::decode($contents); + $contents = $alter($contents); + file_put_contents($file, Yaml::encode($contents)); + } + } diff --git a/core/tests/Drupal/KernelTests/Core/Recipe/ConfigConfiguratorTest.php b/core/tests/Drupal/KernelTests/Core/Recipe/ConfigConfiguratorTest.php index b91ea75b37b4..305e61165ad2 100644 --- a/core/tests/Drupal/KernelTests/Core/Recipe/ConfigConfiguratorTest.php +++ b/core/tests/Drupal/KernelTests/Core/Recipe/ConfigConfiguratorTest.php @@ -5,8 +5,15 @@ namespace Drupal\KernelTests\Core\Recipe; use Drupal\Component\Serialization\Yaml; +use Drupal\Core\Entity\EntityDisplayRepositoryInterface; +use Drupal\Core\Field\Entity\BaseFieldOverride; use Drupal\Core\Recipe\Recipe; +use Drupal\Core\Recipe\RecipePreExistingConfigException; +use Drupal\Core\Recipe\RecipeRunner; +use Drupal\FunctionalTests\Core\Recipe\RecipeTestTrait; use Drupal\KernelTests\KernelTestBase; +use Drupal\node\Entity\NodeType; +use org\bovigo\vfs\vfsStream; /** * @covers \Drupal\Core\Recipe\ConfigConfigurator @@ -14,6 +21,8 @@ */ class ConfigConfiguratorTest extends KernelTestBase { + use RecipeTestTrait; + public function testExistingConfigWithKeysInDifferentOrder(): void { $recipe_dir = uniqid('public://recipe_test_'); mkdir($recipe_dir . '/config', recursive: TRUE); @@ -43,4 +52,112 @@ public function testExistingConfigWithKeysInDifferentOrder(): void { $this->assertInstanceOf(Recipe::class, Recipe::createFromDirectory($recipe_dir)); } + /** + * @testWith [false] + * [[]] + */ + public function testExistingConfigIsIgnoredInLenientMode(array|false $strict_value): void { + $recipe = Recipe::createFromDirectory('core/recipes/page_content_type'); + $this->assertNotEmpty($recipe->config->getConfigStorage()->listAll()); + RecipeRunner::processRecipe($recipe); + + // Clone the recipe into the virtual file system, and opt the clone into + // lenient mode. + $recipe_dir = $this->cloneRecipe($recipe->path); + $this->alterRecipe($recipe_dir, function (array $data) use ($strict_value): array { + $data['config']['strict'] = $strict_value; + return $data; + }); + // The recipe should not have any config to install; all of it already + // exists. + $recipe = Recipe::createFromDirectory($recipe_dir); + $this->assertEmpty($recipe->config->getConfigStorage()->listAll()); + } + + public function testSelectiveStrictness(): void { + $recipe = Recipe::createFromDirectory('core/recipes/page_content_type'); + RecipeRunner::processRecipe($recipe); + + $node_type = NodeType::load('page'); + $original_description = $node_type->getDescription(); + $node_type->set('description', 'And now for something completely different.') + ->save(); + + $form_display = $this->container->get(EntityDisplayRepositoryInterface::class) + ->getFormDisplay('node', 'page'); + $this->assertFalse($form_display->isNew()); + $this->assertIsArray($form_display->getComponent('uid')); + $form_display->removeComponent('uid')->save(); + + // Delete something that the recipe provides, so we can be sure it is + // recreated if it's not in the strict list. + BaseFieldOverride::loadByName('node', 'page', 'promote')->delete(); + + // Clone the recipe into the virtual file system, and opt only the node + // type into strict mode. + $clone_dir = $this->cloneRecipe($recipe->path); + $this->alterRecipe($clone_dir, function (array $data): array { + $data['config']['strict'] = ['node.type.page']; + return $data; + }); + // If we try to instantiate this recipe, we should an exception. + try { + Recipe::createFromDirectory($clone_dir); + $this->fail('Expected an exception but none was thrown.'); + } + catch (RecipePreExistingConfigException $e) { + $this->assertSame("The configuration 'node.type.page' exists already and does not match the recipe's configuration", $e->getMessage()); + } + + // If we restore the node type's original description, we should no longer + // get an error if we try to instantiate the altered recipe, even though the + // form display is still different from what's in the recipe. + NodeType::load('page') + ->set('description', $original_description) + ->save(); + + $recipe = Recipe::createFromDirectory($clone_dir); + RecipeRunner::processRecipe($recipe); + + // Make certain that our change to the form display is still there. + $component = $this->container->get(EntityDisplayRepositoryInterface::class) + ->getFormDisplay('node', 'page') + ->getComponent('uid'); + $this->assertNull($component); + + // The thing we deleted should have been recreated. + $this->assertInstanceOf(BaseFieldOverride::class, BaseFieldOverride::loadByName('node', 'page', 'promote')); + } + + public function testFullStrictness(): void { + $recipe = Recipe::createFromDirectory('core/recipes/page_content_type'); + RecipeRunner::processRecipe($recipe); + + NodeType::load('page') + ->set('description', 'And now for something completely different.') + ->save(); + + // Clone the recipe into the virtual file system, and opt all of its config + // into strict mode. + $clone_dir = $this->cloneRecipe($recipe->path); + $this->alterRecipe($clone_dir, function (array $data): array { + $data['config']['strict'] = TRUE; + return $data; + }); + // If we try to instantiate this recipe, we should an exception. + $this->expectException(RecipePreExistingConfigException::class); + $this->expectExceptionMessage("The configuration 'node.type.page' exists already and does not match the recipe's configuration"); + Recipe::createFromDirectory($clone_dir); + } + + private function cloneRecipe(string $original_dir): string { + // Clone the recipe into the virtual file system. + $name = uniqid(); + $clone_dir = $this->vfsRoot->url() . '/' . $name; + mkdir($clone_dir); + $clone_dir = $this->vfsRoot->getChild($name); + vfsStream::copyFromFileSystem($original_dir, $clone_dir); + return $clone_dir->url(); + } + } diff --git a/core/tests/Drupal/KernelTests/Core/Recipe/RecipeValidationTest.php b/core/tests/Drupal/KernelTests/Core/Recipe/RecipeValidationTest.php index a5360341b4a6..55c140f2b87e 100644 --- a/core/tests/Drupal/KernelTests/Core/Recipe/RecipeValidationTest.php +++ b/core/tests/Drupal/KernelTests/Core/Recipe/RecipeValidationTest.php @@ -258,6 +258,58 @@ public static function providerRecipeValidation(): iterable { '[config][import][0]' => ['This value should satisfy at least one of the following constraints: [1] This value should be identical to string "*". [2] Each element of this collection should satisfy its own set of constraints.'], ], ]; + yield 'config strict is not a boolean or array' => [ + <<<YAML +name: Invalid strict flag +config: + strict: 40 +YAML, + [ + '[config][strict]' => ['This value must be a boolean, or a list of config names.'], + ], + ]; + yield 'config strict is an array of not-strings' => [ + <<<YAML +name: Invalid item in strict list +config: + strict: + - 40 +YAML, + [ + '[config][strict]' => ['This value must be a boolean, or a list of config names.'], + ], + ]; + yield 'config strict list contains blank strings' => [ + <<<YAML +name: Invalid item in strict list +config: + strict: + - '' +YAML, + [ + '[config][strict]' => ['This value must be a boolean, or a list of config names.'], + ], + ]; + yield 'config strict list item does not have a period' => [ + <<<YAML +name: Invalid item in strict list +config: + strict: + - 'something' +YAML, + [ + '[config][strict]' => ['This value must be a boolean, or a list of config names.'], + ], + ]; + yield 'valid strict list' => [ + <<<YAML +name: Valid strict list +config: + strict: + - system.menu.foo +YAML, + NULL, + ]; yield 'config actions list is valid' => [ <<<YAML name: 'Correct config actions list' -- GitLab