feature #27336 [Security][SecurityBundle] FirewallMap/FirewallContext…

… deprecations (chalasr)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Security][SecurityBundle] FirewallMap/FirewallContext deprecations

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes/no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #...   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

Next to #24805.

Commits
-------

a71ba78 [Security][SecurityBundle] FirewallMap/FirewallContext deprecations

UPGRADE-4.2.md

@@ -5,3 +5,11 @@ Security
 --------
 
  * Using the `has_role()` function in security expressions is deprecated, use the `is_granted()` function instead.
+ * Not returning an array of 3 elements from `FirewallMapInterface::getListeners()` is deprecated, the 3rd element 
+   must be an instance of `LogoutListener` or `null`.
+
+SecurityBundle
+--------------
+
+ * Passing a `FirewallConfig` instance as 3rd argument to the `FirewallContext` constructor is deprecated, 
+   pass a `LogoutListener` instance instead.

UPGRADE-5.0.md

@@ -78,13 +78,17 @@ Security
  * The `ContextListener::setLogoutOnUserChange()` method has been removed.
  * The `Symfony\Component\Security\Core\User\AdvancedUserInterface` has been removed.
  * The `ExpressionVoter::addExpressionLanguageProvider()` method has been removed.
+ * The `FirewallMapInterface::getListeners()` method must return an array of 3 elements,
+   the 3rd one must be either a `LogoutListener` instance or `null`.
 
 SecurityBundle
 --------------
 
  * The `logout_on_user_change` firewall option has been removed.
  * The `switch_user.stateless` firewall option has been removed.
  * The `SecurityUserValueResolver` class has been removed.
+ * Passing a `FirewallConfig` instance as 3rd argument to  the `FirewallContext` constructor 
+   now throws a `\TypeError`, pass a `LogoutListener` instance instead.
 
 Translation
 -----------

src/Symfony/Bundle/SecurityBundle/Security/FirewallContext.php

@@ -36,6 +36,7 @@ public function __construct(iterable $listeners, ExceptionListener $exceptionLis
         $this->exceptionListener = $exceptionListener;
         if ($logoutListener instanceof FirewallConfig) {
             $this->config = $logoutListener;
+            @trigger_error(sprintf('Passing an instance of %s as 3rd argument to %s() is deprecated since Symfony 4.2. Pass a %s instance instead.', FirewallConfig::class, __METHOD__, LogoutListener::class), E_USER_DEPRECATED);
         } elseif (null === $logoutListener || $logoutListener instanceof LogoutListener) {
             $this->logoutListener = $logoutListener;
             $this->config = $config;

src/Symfony/Bundle/SecurityBundle/Tests/DataCollector/SecurityDataCollectorTest.php

@@ -208,7 +208,7 @@ public function testGetListeners()
             ->expects($this->once())
             ->method('getListeners')
             ->with($request)
-            ->willReturn(array(array($listener), null));
+            ->willReturn(array(array($listener), null, null));
 
         $firewall = new TraceableFirewallListener($firewallMap, new EventDispatcher(), new LogoutUrlGenerator());
         $firewall->onKernelRequest($event);

src/Symfony/Bundle/SecurityBundle/Tests/Debug/TraceableFirewallListenerTest.php

@@ -51,7 +51,7 @@ public function testOnKernelRequestRecordsListeners()
             ->expects($this->once())
             ->method('getListeners')
             ->with($request)
-            ->willReturn(array(array($listener), null));
+            ->willReturn(array(array($listener), null, null));
 
         $firewall = new TraceableFirewallListener($firewallMap, new EventDispatcher(), new LogoutUrlGenerator());
         $firewall->onKernelRequest($event);

src/Symfony/Bundle/SecurityBundle/Tests/Security/FirewallContextTest.php

@@ -40,6 +40,15 @@ public function testGetters()
         $this->assertEquals($config, $context->getConfig());
     }
 
+    /**
+     * @group legacy
+     * @expectedDeprecation Passing an instance of Symfony\Bundle\SecurityBundle\Security\FirewallConfig as 3rd argument to Symfony\Bundle\SecurityBundle\Security\FirewallContext::__construct() is deprecated since Symfony 4.2. Pass a Symfony\Component\Security\Http\Firewall\LogoutListener instance instead.
+     */
+    public function testFirewallConfigAs3rdConstructorArgument()
+    {
+        new FirewallContext(array(), $this->getExceptionListenerMock(), new FirewallConfig('main', 'user_checker', 'request_matcher'));
+    }
+
     private function getExceptionListenerMock()
     {
         return $this

src/Symfony/Component/Security/Http/Firewall.php

@@ -16,6 +16,7 @@
 use Symfony\Component\HttpKernel\Event\FinishRequestEvent;
 use Symfony\Component\EventDispatcher\EventDispatcherInterface;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\Security\Http\Firewall\LogoutListener;
 
 /**
  * Firewall uses a FirewallMap to register security listeners for the given
@@ -49,9 +50,14 @@ public function onKernelRequest(GetResponseEvent $event)
         // register listeners for this firewall
         $listeners = $this->map->getListeners($event->getRequest());
 
+        if (3 !== \count($listeners)) {
+            @trigger_error(sprintf('Not returning an array of 3 elements from %s::getListeners() is deprecated since Symfony 4.2, the 3rd element must be an instance of %s or null.', FirewallMapInterface::class, LogoutListener::class), E_USER_DEPRECATED);
+            $listeners[2] = null;
+        }
+
         $authenticationListeners = $listeners[0];
         $exceptionListener = $listeners[1];
-        $logoutListener = isset($listeners[2]) ? $listeners[2] : null;
+        $logoutListener = $listeners[2];
 
         if (null !== $exceptionListener) {
             $this->exceptionListeners[$event->getRequest()] = $exceptionListener;

src/Symfony/Component/Security/Http/FirewallMapInterface.php

@@ -30,7 +30,10 @@ interface FirewallMapInterface
      * If there is no exception listener, the second element of the outer array
      * must be null.
      *
-     * @return array of the format array(array(AuthenticationListener), ExceptionListener)
+     * If there is no logout listener, the third element of the outer array
+     * must be null.
+     *
+     * @return array of the format array(array(AuthenticationListener), ExceptionListener, LogoutListener)
      */
     public function getListeners(Request $request);
 }

src/Symfony/Component/Security/Http/Tests/FirewallTest.php

@@ -12,10 +12,14 @@
 namespace Symfony\Component\Security\Http\Tests;
 
 use PHPUnit\Framework\TestCase;
+use Symfony\Component\EventDispatcher\EventDispatcherInterface;
+use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpFoundation\Response;
 use Symfony\Component\HttpKernel\Event\GetResponseEvent;
 use Symfony\Component\HttpKernel\HttpKernelInterface;
 use Symfony\Component\Security\Http\Firewall;
+use Symfony\Component\Security\Http\Firewall\ExceptionListener;
+use Symfony\Component\Security\Http\FirewallMapInterface;
 
 class FirewallTest extends TestCase
 {
@@ -37,7 +41,7 @@ public function testOnKernelRequestRegistersExceptionListener()
             ->expects($this->once())
             ->method('getListeners')
             ->with($this->equalTo($request))
-            ->will($this->returnValue(array(array(), $listener)))
+            ->will($this->returnValue(array(array(), $listener, null)))
         ;
 
         $event = new GetResponseEvent($this->getMockBuilder('Symfony\Component\HttpKernel\HttpKernelInterface')->getMock(), $request, HttpKernelInterface::MASTER_REQUEST);
@@ -66,7 +70,7 @@ public function testOnKernelRequestStopsWhenThereIsAResponse()
         $map
             ->expects($this->once())
             ->method('getListeners')
-            ->will($this->returnValue(array(array($first, $second), null)))
+            ->will($this->returnValue(array(array($first, $second), null, null)))
         ;
 
         $event = $this->getMockBuilder('Symfony\Component\HttpKernel\Event\GetResponseEvent')
@@ -107,4 +111,33 @@ public function testOnKernelRequestWithSubRequest()
 
         $this->assertFalse($event->hasResponse());
     }
+
+    /**
+     * @group legacy
+     * @expectedDeprecation Not returning an array of 3 elements from Symfony\Component\Security\Http\FirewallMapInterface::getListeners() is deprecated since Symfony 4.2, the 3rd element must be an instance of Symfony\Component\Security\Http\Firewall\LogoutListener or null.
+     */
+    public function testMissingLogoutListener()
+    {
+        $dispatcher = $this->getMockBuilder(EventDispatcherInterface::class)->getMock();
+
+        $listener = $this->getMockBuilder(ExceptionListener::class)->disableOriginalConstructor()->getMock();
+        $listener
+            ->expects($this->once())
+            ->method('register')
+            ->with($this->equalTo($dispatcher))
+        ;
+
+        $request = new Request();
+
+        $map = $this->getMockBuilder(FirewallMapInterface::class)->getMock();
+        $map
+            ->expects($this->once())
+            ->method('getListeners')
+            ->with($this->equalTo($request))
+            ->willReturn(array(array(), $listener))
+        ;
+
+        $firewall = new Firewall($map, $dispatcher);
+        $firewall->onKernelRequest(new GetResponseEvent($this->getMockBuilder(HttpKernelInterface::class)->getMock(), $request, HttpKernelInterface::MASTER_REQUEST));
+    }
 }