Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
10 changes: 10 additions & 0 deletions src/DependencyInjection/Configuration.php
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -140,6 +142,7 @@ private function addRefreshTokenNode(ArrayNodeDefinition $rootNode): void
$rootNode
->children()
->arrayNode('refresh_token')
->isRequired()
->addDefaultsIfNotSet()
->children()
->scalarNode('handler_id')->cannotBeEmpty()->isRequired()->end()
Expand All @@ -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();
}
Expand All @@ -160,6 +167,7 @@ private function addPublishableNode(ArrayNodeDefinition $rootNode): void
$rootNode
->children()
->arrayNode('publishable')
->isRequired()
->addDefaultsIfNotSet()
->children()
->scalarNode('permission')->cannotBeEmpty()->isRequired()->end()
Expand Down Expand Up @@ -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')
Expand Down
59 changes: 59 additions & 0 deletions tests/DependencyInjection/ConfigurationTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,72 @@ 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'],
'publishable' => ['permission' => "is_granted('ROLE_ADMIN')"],
];
}

#[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]);
Expand Down
Loading