Container fails with stateless firewalls with the new authenticatior manager enabled

[johnvandeweghe]

Symfony version(s) affected: 5.1.0

Description
If a stateless firewall is defined, and the new authenticator manager is enabled the following error occurs:

Argument 2 of service "security.listener.session" is abstract: stateless firewall keys.

Some digging appears that the need for the second argument was removed in a recent refactor of the Symfony\Component\Security\Http\EventListener\SessionStrategyListener and the \Symfony\Bundle\SecurityBundle\DependencyInjection\SecurityExtension was updated to no longer add the firewall keys, but in an unrelated refactor changed the service definition in Resources/config/security_authenticator.xml to the following:

        <service id="security.listener.session"
                 class="Symfony\Component\Security\Http\EventListener\SessionStrategyListener"
                 abstract="true">
            <argument type="service" id="security.authentication.session_strategy" />
            <argument type="abstract">stateless firewall keys</argument>
        </service>

This appears to be the root of the issue.

How to reproduce
Minimal example repo: https://github.com/johnvandeweghe/sf-session-listener-bug

1. Add enable_authenticator_manager: true to security.yaml
2. Add a stateless firewall:

main:
            pattern: ^/api
            stateless: true
            http_basic: ~

3. Try to run the bin/console script.

Possible Solution
Changing the service definition to the following - removing the abstract definition and the second argument - fixes it for me (and matches up with the class code):

        <service id="security.listener.session"
                 class="Symfony\Component\Security\Http\EventListener\SessionStrategyListener">
            <argument type="service" id="security.authentication.session_strategy" />
        </service>

Additional context
This doesn't appear to be a problem if there are no stateless firewalls defined, which is likely why it has gone unnoticed during development.

This is my first bug report for Symfony, let me know if any more detail is needed!

[wouterj]

Thanks for the very detailed description!

I think there is more trouble here: Seems like I somehow ended up with the complete opposite of what should be done.

symfony/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

Lines 171 to 177 in bdc583c

	if ($this->authenticatorManagerEnabled) {
	foreach ($this->statelessFirewallKeys as $statelessFirewallId) {
	$container
	->setDefinition('security.listener.session.'.$statelessFirewallId, new ChildDefinition('security.listener.session'))
	->addTag('kernel.event_subscriber', ['dispatcher' => 'security.event_dispatcher.'.$statelessFirewallId]);
	}
	}

This is now registering the SessionStrategyListener only for the stateless firewalls, while it should be registered only for stateful firewalls.

So besides removing the abstract argument you mentioned, I think we should also change this loop to add it for all stateful firewalls. I think it then makes most sense to register the listener on these lines in the SecurityExtension when authenticatorManagerEnabled === true:

symfony/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php

Lines 372 to 376 in bdc583c

	if (false === $firewall['stateless']) {
	$contextKey = $firewall['context'] ?? $id;
	$listeners[] = new Reference($contextListenerId = $this->createContextListener($container, $contextKey));
	$sessionStrategyId = 'security.authentication.session_strategy';
	} else {

Would you be up for a PR making the change (as bugfix in 5.1)? If you know how, please also add a regression test so we won't break this in the future.

[johnvandeweghe]

Put a PR up (#37126), let me know if it looks how you'd expect it to. @wouterj