diff --git a/CLAUDE.md b/CLAUDE.md index 6caa8bcae..9ebaf077e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1049,7 +1049,7 @@ Both found by the docs accuracy audit. Neither was quite what the issue describe **#214 — `isRequired()` under a node that carries a default is never enforced.** `ArrayNode::finalizeValue()` inserts the default and `continue`s without finalising, so the required-child check never runs for an omitted node. `user.email_verification` resolved to `['enabled' => true]`, the extension read keys that were not there (26 undefined-array-key warnings on a minimal config), and services typed `bool` were wired with `null`. The same declaration made the node impossible to configure *partially* — supplying anything, including `enabled: false`, ran finalisation and hard-failed on the first missing required child. -**Convention going forward: never put `isRequired()` under a node with `addDefaultsIfNotSet()` or `canBeDisabled()`.** Give the child a real default instead. If a combination is genuinely invalid, express it as a node-level `->validate()` — that runs only when the node is present, which is the only time the combination can be expressed, so it cannot break an application that omits the node. `user.email_verification` now rejects `verify_on_register`/`verify_on_change` without a redirect target that way. +**Convention going forward: never put `isRequired()` on a *child* of a node that carries `addDefaultsIfNotSet()` or `canBeDisabled()`.** Either give the child a real default, or make the **parent node itself** `isRequired()` — `ArrayNode::finalizeValue()` checks `isRequired()` at `:214` *before* inserting a default at `:227`, so a required node is enforced on omission while its own children still resolve their defaults when it is present. A node-level `->validate()` expresses invalid **combinations**, not required **presence**: it runs in `finalize()`, i.e. only when the node is present, which is precisely the case that already worked. `user.email_verification` rejects `verify_on_register`/`verify_on_change` without a redirect target that way, and `refresh_token` demands `options.class` for the doctrine handler the same way (#222). Defaults were chosen to be **all-off** rather than "correct": `deny_unverified_login: true` would lock users out of an application that never configured verification, and `verify_on_register: true` would send emails for which no redirect target exists (`AbstractUserEmailFactory::getTokenPath()` throws). Making the children genuinely required was rejected as a breaking change — it would force config on every application currently omitting the node. diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php index bbb5b6691..4c9a4711e 100644 --- a/src/DependencyInjection/Configuration.php +++ b/src/DependencyInjection/Configuration.php @@ -27,6 +27,8 @@ */ class Configuration implements ConfigurationInterface { + private const string DOCTRINE_REFRESH_TOKEN_STORAGE = 'silverback.api_components.refresh_token.storage.doctrine'; + public function getConfigTreeBuilder(): TreeBuilder { $treeBuilder = new TreeBuilder('silverback_api_components'); @@ -140,6 +142,7 @@ private function addRefreshTokenNode(ArrayNodeDefinition $rootNode): void $rootNode ->children() ->arrayNode('refresh_token') + ->isRequired() ->addDefaultsIfNotSet() ->children() ->scalarNode('handler_id')->cannotBeEmpty()->isRequired()->end() @@ -151,6 +154,10 @@ private function addRefreshTokenNode(ArrayNodeDefinition $rootNode): void ->scalarNode('ttl')->cannotBeEmpty()->isRequired()->end() ->scalarNode('database_user_provider')->cannotBeEmpty()->isRequired()->end() ->end() + ->validate() + ->ifTrue(static fn (array $v): bool => self::DOCTRINE_REFRESH_TOKEN_STORAGE === $v['handler_id'] && empty($v['options']['class'])) + ->thenInvalid('"silverback_api_components.refresh_token.options.class" must name your RefreshToken entity when using the doctrine storage handler.') + ->end() ->end() ->end(); } @@ -160,6 +167,7 @@ private function addPublishableNode(ArrayNodeDefinition $rootNode): void $rootNode ->children() ->arrayNode('publishable') + ->isRequired() ->addDefaultsIfNotSet() ->children() ->scalarNode('permission')->cannotBeEmpty()->isRequired()->end() @@ -187,9 +195,11 @@ private function addUserNode(ArrayNodeDefinition $rootNode): void $rootNode ->children() ->arrayNode('user') + ->isRequired() ->addDefaultsIfNotSet() ->children() ->scalarNode('class_name') + ->cannotBeEmpty() ->isRequired() ->end() ->arrayNode('email_verification') diff --git a/tests/DependencyInjection/ConfigurationTest.php b/tests/DependencyInjection/ConfigurationTest.php index ce8d66d5d..e0839319b 100644 --- a/tests/DependencyInjection/ConfigurationTest.php +++ b/tests/DependencyInjection/ConfigurationTest.php @@ -46,6 +46,7 @@ private static function minimalConfig(): array 'cookie_name' => 'api_components', 'ttl' => 604800, 'database_user_provider' => 'database', + 'options' => ['class' => 'App\\Entity\\RefreshToken'], ], 'website_name' => 'Test Website', 'user' => ['class_name' => 'App\Entity\User'], @@ -53,6 +54,64 @@ private static function minimalConfig(): array ]; } + #[DataProvider('requiredNodeProvider')] + public function test_omitting_a_required_node_is_rejected_by_name(string $node): void + { + $config = self::minimalConfig(); + unset($config[$node]); + + $this->expectException(InvalidConfigurationException::class); + $this->expectExceptionMessage(\sprintf('The child config "%s" under "silverback_api_components" must be configured.', $node)); + + $this->process($config); + } + + public static function requiredNodeProvider(): iterable + { + yield 'user' => ['user']; + yield 'refresh_token' => ['refresh_token']; + yield 'publishable' => ['publishable']; + } + + public function test_a_required_node_still_resolves_its_defaulted_children(): void + { + $user = $this->process(self::minimalConfig())['user']; + + self::assertArrayHasKey('email_verification', $user); + self::assertSame(86400, $user['password_reset']['repeat_ttl_seconds']); + } + + public function test_the_doctrine_refresh_token_storage_demands_an_entity_class(): void + { + $config = self::minimalConfig(); + unset($config['refresh_token']['options']); + + $this->expectException(InvalidConfigurationException::class); + $this->expectExceptionMessage('silverback_api_components.refresh_token.options.class'); + + $this->process($config); + } + + public function test_a_custom_refresh_token_storage_needs_no_entity_class(): void + { + $config = self::minimalConfig(); + $config['refresh_token']['handler_id'] = 'app.refresh_token.storage'; + unset($config['refresh_token']['options']); + + self::assertSame([], $this->process($config)['refresh_token']['options']); + } + + public function test_an_empty_user_class_name_is_rejected(): void + { + $config = self::minimalConfig(); + $config['user']['class_name'] = ''; + + $this->expectException(InvalidConfigurationException::class); + $this->expectExceptionMessage('cannot contain an empty value'); + + $this->process($config); + } + private function process(array $config): array { return (new Processor())->processConfiguration(new Configuration(), [$config]);