[SecurityBundle] Fix using same handler for multiple authenticators

Using a reference for custom success/failure handler breaks using the same service for multiple authenticators. This as the options will be overriden by any later authenticators. So use a ChildDefinition instead so every authenticator gets a unique instance. Fixes: #48923
symfony · Jan 13, 2023 · e16ac30 · e16ac30
1 parent 8203ec9
commit e16ac30
Show file tree

Hide file tree

Showing 8 changed files with 171 additions and 5 deletions.
diff --git a/DependencyInjection/Security/Factory/AbstractFactory.php b/DependencyInjection/Security/Factory/AbstractFactory.php
@@ -164,7 +164,7 @@ protected function createAuthenticationSuccessHandler(ContainerBuilder $containe
 
         if (isset($config['success_handler'])) {
             $successHandler = $container->setDefinition($successHandlerId, new ChildDefinition('security.authentication.custom_success_handler'));
-            $successHandler->replaceArgument(0, new Reference($config['success_handler']));
+            $successHandler->replaceArgument(0, new ChildDefinition($config['success_handler']));
             $successHandler->replaceArgument(1, $options);
             $successHandler->replaceArgument(2, $id);
         } else {
@@ -183,7 +183,7 @@ protected function createAuthenticationFailureHandler(ContainerBuilder $containe
 
         if (isset($config['failure_handler'])) {
             $failureHandler = $container->setDefinition($id, new ChildDefinition('security.authentication.custom_failure_handler'));
-            $failureHandler->replaceArgument(0, new Reference($config['failure_handler']));
+            $failureHandler->replaceArgument(0, new ChildDefinition($config['failure_handler']));
             $failureHandler->replaceArgument(1, $options);
         } else {
             $failureHandler = $container->setDefinition($id, new ChildDefinition('security.authentication.failure_handler'));

diff --git a/Tests/DependencyInjection/Security/Factory/AbstractFactoryTest.php b/Tests/DependencyInjection/Security/Factory/AbstractFactoryTest.php
@@ -13,6 +13,7 @@
 
 use PHPUnit\Framework\TestCase;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\AbstractFactory;
+use Symfony\Component\DependencyInjection\ChildDefinition;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
 use Symfony\Component\DependencyInjection\Reference;
 
@@ -69,12 +70,19 @@ public function testDefaultFailureHandler($serviceId, $defaultHandlerInjection)
         $this->assertEquals(new Reference('security.authentication.failure_handler.foo.abstract_factory'), $arguments['index_6']);
         $failureHandler = $container->findDefinition((string) $arguments['index_6']);
 
+        $expectedFailureHandlerOptions = ['login_path' => '/bar'];
         $methodCalls = $failureHandler->getMethodCalls();
         if ($defaultHandlerInjection) {
             $this->assertEquals('setOptions', $methodCalls[0][0]);
-            $this->assertEquals(['login_path' => '/bar'], $methodCalls[0][1][0]);
+            $this->assertEquals($expectedFailureHandlerOptions, $methodCalls[0][1][0]);
         } else {
             $this->assertCount(0, $methodCalls);
+            $this->assertInstanceOf(ChildDefinition::class, $failureHandler);
+            $this->assertEquals('security.authentication.custom_failure_handler', $failureHandler->getParent());
+            $failureHandlerArguments = $failureHandler->getArguments();
+            $this->assertInstanceOf(ChildDefinition::class, $failureHandlerArguments['index_0']);
+            $this->assertEquals($serviceId, $failureHandlerArguments['index_0']->getParent());
+            $this->assertEquals($expectedFailureHandlerOptions, $failureHandlerArguments['index_1']);
         }
     }
 
@@ -108,13 +116,22 @@ public function testDefaultSuccessHandler($serviceId, $defaultHandlerInjection)
         $successHandler = $container->findDefinition((string) $arguments['index_5']);
         $methodCalls = $successHandler->getMethodCalls();
 
+        $expectedSuccessHandlerOptions = ['default_target_path' => '/bar'];
+        $expectedFirewallName = 'foo';
         if ($defaultHandlerInjection) {
             $this->assertEquals('setOptions', $methodCalls[0][0]);
-            $this->assertEquals(['default_target_path' => '/bar'], $methodCalls[0][1][0]);
+            $this->assertEquals($expectedSuccessHandlerOptions, $methodCalls[0][1][0]);
             $this->assertEquals('setFirewallName', $methodCalls[1][0]);
-            $this->assertEquals(['foo'], $methodCalls[1][1]);
+            $this->assertEquals($expectedFirewallName, $methodCalls[1][1][0]);
         } else {
             $this->assertCount(0, $methodCalls);
+            $this->assertInstanceOf(ChildDefinition::class, $successHandler);
+            $this->assertEquals('security.authentication.custom_success_handler', $successHandler->getParent());
+            $successHandlerArguments = $successHandler->getArguments();
+            $this->assertInstanceOf(ChildDefinition::class, $successHandlerArguments['index_0']);
+            $this->assertEquals($serviceId, $successHandlerArguments['index_0']->getParent());
+            $this->assertEquals($expectedSuccessHandlerOptions, $successHandlerArguments['index_1']);
+            $this->assertEquals($expectedFirewallName, $successHandlerArguments['index_2']);
         }
     }
 

diff --git a/Tests/Functional/AuthenticatorTest.php b/Tests/Functional/AuthenticatorTest.php
@@ -102,4 +102,38 @@ public function testMultipleFirewalls()
         $client->request('GET', '/firewall2/profile');
         $this->assertResponseRedirects('http://localhost/login');
     }
+
+    public function testCustomSuccessHandler()
+    {
+        $client = $this->createClient(['test_case' => 'Authenticator', 'root_config' => 'custom_handlers.yml']);
+
+        $client->request('POST', '/firewall1/login', [
+            '_username' => '[email protected]',
+            '_password' => 'test',
+        ]);
+        $this->assertResponseRedirects('http://localhost/firewall1/test');
+
+        $client->request('POST', '/firewall1/dummy_login', [
+            '_username' => '[email protected]',
+            '_password' => 'test',
+        ]);
+        $this->assertResponseRedirects('http://localhost/firewall1/dummy');
+    }
+
+    public function testCustomFailureHandler()
+    {
+        $client = $this->createClient(['test_case' => 'Authenticator', 'root_config' => 'custom_handlers.yml']);
+
+        $client->request('POST', '/firewall1/login', [
+            '_username' => '[email protected]',
+            '_password' => '',
+        ]);
+        $this->assertResponseRedirects('http://localhost/firewall1/login');
+
+        $client->request('POST', '/firewall1/dummy_login', [
+            '_username' => '[email protected]',
+            '_password' => '',
+        ]);
+        $this->assertResponseRedirects('http://localhost/firewall1/dummy_login');
+    }
 }
diff --git a/Tests/Functional/Bundle/AuthenticatorBundle/AuthenticatorBundle.php b/Tests/Functional/Bundle/AuthenticatorBundle/AuthenticatorBundle.php
@@ -0,0 +1,34 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\AuthenticatorBundle;
+
+use Symfony\Bundle\SecurityBundle\DependencyInjection\SecurityExtension;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\HttpKernel\Bundle\Bundle;
+
+class AuthenticatorBundle extends Bundle
+{
+    public function build(ContainerBuilder $container)
+    {
+        parent::build($container);
+
+        $this->configureSecurityExtension($container);
+    }
+
+    private function configureSecurityExtension(ContainerBuilder $container): void
+    {
+        /** @var SecurityExtension $extension */
+        $extension = $container->getExtension('security');
+
+        $extension->addAuthenticatorFactory(new DummyFormLoginFactory());
+    }
+}
diff --git a/Tests/Functional/Bundle/AuthenticatorBundle/DummyFormLoginFactory.php b/Tests/Functional/Bundle/AuthenticatorBundle/DummyFormLoginFactory.php
@@ -0,0 +1,43 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\AuthenticatorBundle;
+
+use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\FormLoginFactory;
+use Symfony\Component\DependencyInjection\ChildDefinition;
+use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Reference;
+
+class DummyFormLoginFactory extends FormLoginFactory
+{
+    public function getKey(): string
+    {
+        return 'dummy_form_login';
+    }
+
+    public function createAuthenticator(ContainerBuilder $container, string $firewallName, array $config, string $userProviderId): string
+    {
+        $authenticatorId = 'security.authenticator.dummy_form_login.'.$firewallName;
+        $options = array_intersect_key($config, $this->options);
+        $authenticator = $container
+            ->setDefinition($authenticatorId, new ChildDefinition('security.authenticator.form_login'))
+            ->replaceArgument(1, new Reference($userProviderId))
+            ->replaceArgument(2, new Reference($this->createAuthenticationSuccessHandler($container, $firewallName, $config)))
+            ->replaceArgument(3, new Reference($this->createAuthenticationFailureHandler($container, $firewallName, $config)))
+            ->replaceArgument(4, $options);
+
+        if ($options['use_forward'] ?? false) {
+            $authenticator->addMethodCall('setHttpKernel', [new Reference('http_kernel')]);
+        }
+
+        return $authenticatorId;
+    }
+}
diff --git a/Tests/Functional/app/Authenticator/bundles.php b/Tests/Functional/app/Authenticator/bundles.php
@@ -12,4 +12,5 @@
 return [
     new Symfony\Bundle\FrameworkBundle\FrameworkBundle(),
     new Symfony\Bundle\SecurityBundle\SecurityBundle(),
+    new Symfony\Bundle\SecurityBundle\Tests\Functional\Bundle\AuthenticatorBundle\AuthenticatorBundle(),
 ];
diff --git a/Tests/Functional/app/Authenticator/custom_handlers.yml b/Tests/Functional/app/Authenticator/custom_handlers.yml
@@ -0,0 +1,34 @@
+imports:
+  - { resource: ./config.yml }
+  - { resource: ./security.yml }
+
+security:
+  enable_authenticator_manager: true
+  firewalls:
+    firewall1:
+      pattern: /firewall1
+      provider: in_memory
+      entry_point: form_login
+      form_login:
+        check_path: /firewall1/login
+        success_handler: success_handler
+        failure_handler: failure_handler
+        default_target_path: /firewall1/test
+        login_path: /firewall1/login
+      dummy_form_login:
+        check_path: /firewall1/dummy_login
+        success_handler: success_handler
+        failure_handler: failure_handler
+        default_target_path: /firewall1/dummy
+        login_path: /firewall1/dummy_login
+
+services:
+  success_handler:
+    class: Symfony\Component\Security\Http\Authentication\DefaultAuthenticationSuccessHandler
+    arguments:
+      - '@security.http_utils'
+  failure_handler:
+    class: Symfony\Component\Security\Http\Authentication\DefaultAuthenticationFailureHandler
+    arguments:
+      - '@http_kernel'
+      - '@security.http_utils'
diff --git a/Tests/Functional/app/Authenticator/routing.yml b/Tests/Functional/app/Authenticator/routing.yml
@@ -22,6 +22,9 @@ security_custom_profile:
 firewall1_login:
     path: /firewall1/login
 
+firewall_dummy_login:
+    path: /firewall1/dummy_login
+
 firewall2_profile:
     path: /firewall2/profile
     defaults: