feature #22629 [Security] Trigger a deprecation when a voter is missi…

…ng the VoterInterface (iltar)

This PR was squashed before being merged into the 3.4 branch (closes #22629).

Discussion
----------

[Security] Trigger a deprecation when a voter is missing the VoterInterface

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

Right now it's possible to add voters to the access decision manager that do not have a `VoterInterface`.
 - No Interface, no `vote()` method, and it will give a PHP error.
 - No Interface, but `vote()` method, it will still work.
 - If I don't implement the interface _and_ have no `vote()` method, I will get weird exception that's not meaningful: `Attempted to call an undefined method named "vote" of class "App\Voter\MyVoter".`

This PR will deprecate the ability to use voters without the interface, it will also throw a proper exception when missing the interface _and_ the `vote()` method. Why when using and not when setting? Due to the fact that the voters can be set lazily via the `IteratorArgument`. The SecurityBundle will trigger a deprecation if the interface is not implemented and an exception if there's not even a `vote()` method present (to prevent exceptions at run-time).

This should have full backwards compatibility with 3.3, but give more meaningful errors. The only behavioral difference, might be that the container will throw an exception instead of maybe succeeding in voting when 1 voter would be broken at the end of the list (based on strategy). This case however, will be detected during development and deployment, rather than run-time.

Commits
-------

9c253e1 [Security] Trigger a deprecation when a voter is missing the VoterInterface

UPGRADE-3.4.md

@@ -47,6 +47,9 @@ Process
 SecurityBundle
 --------------
 
+ * Using voters that do not implement the `VoterInterface`is now deprecated in
+   the `AccessDecisionManager` and this functionality will be removed in 4.0.
+
  * `FirewallContext::getListeners()` now returns `\Traversable|array`
 
 Validator

src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

@@ -4,6 +4,8 @@ CHANGELOG
 3.4.0
 -----
 
+ * Tagging voters with the `security.voter` tag without implementing the
+   `VoterInterface` on the class is now deprecated and will be removed in 4.0.
  * [BC BREAK] `FirewallContext::getListeners()` now returns `\Traversable|array`
  * added info about called security listeners in profiler 
 

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSecurityVotersPass.php

@@ -16,6 +16,7 @@
 use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
 use Symfony\Component\DependencyInjection\Compiler\PriorityTaggedServiceTrait;
 use Symfony\Component\DependencyInjection\Exception\LogicException;
+use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
 
 /**
  * Adds all configured security voters to the access decision manager.
@@ -40,6 +41,19 @@ public function process(ContainerBuilder $container)
             throw new LogicException('No security voters found. You need to tag at least one with "security.voter"');
         }
 
+        foreach ($voters as $voter) {
+            $class = $container->getDefinition((string) $voter)->getClass();
+
+            if (!is_a($class, VoterInterface::class, true)) {
+                @trigger_error(sprintf('Using a security.voter tag on a class without implementing the %1$s is deprecated as of 3.4 and will be removed in 4.0. Implement the %1$s instead.', VoterInterface::class), E_USER_DEPRECATED);
+            }
+
+            if (!method_exists($class, 'vote')) {
+                // in case the vote method is completely missing, to prevent exceptions when voting
+                throw new LogicException(sprintf('%s should implement the %s interface when used as voter.', $class, VoterInterface::class));
+            }
+        }
+
         $adm = $container->getDefinition('security.access.decision_manager');
         $adm->replaceArgument(0, new IteratorArgument($voters));
     }

src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/AddSecurityVotersPassTest.php

@@ -14,7 +14,11 @@
 use PHPUnit\Framework\TestCase;
 use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\AddSecurityVotersPass;
 use Symfony\Component\DependencyInjection\ContainerBuilder;
+use Symfony\Component\DependencyInjection\Exception\LogicException;
 use Symfony\Component\DependencyInjection\Reference;
+use Symfony\Component\Security\Core\Authorization\AccessDecisionManager;
+use Symfony\Component\Security\Core\Authorization\Voter\Voter;
+use Symfony\Component\Security\Core\Tests\Authorization\Stub\VoterWithoutInterface;
 
 class AddSecurityVotersPassTest extends TestCase
 {
@@ -25,7 +29,7 @@ public function testNoVoters()
     {
         $container = new ContainerBuilder();
         $container
-            ->register('security.access.decision_manager', 'Symfony\Component\Security\Core\Authorization\AccessDecisionManager')
+            ->register('security.access.decision_manager', AccessDecisionManager::class)
             ->addArgument(array())
         ;
 
@@ -37,23 +41,23 @@ public function testThatSecurityVotersAreProcessedInPriorityOrder()
     {
         $container = new ContainerBuilder();
         $container
-            ->register('security.access.decision_manager', 'Symfony\Component\Security\Core\Authorization\AccessDecisionManager')
+            ->register('security.access.decision_manager', AccessDecisionManager::class)
             ->addArgument(array())
         ;
         $container
-            ->register('no_prio_service')
+            ->register('no_prio_service', Voter::class)
             ->addTag('security.voter')
         ;
         $container
-            ->register('lowest_prio_service')
+            ->register('lowest_prio_service', Voter::class)
             ->addTag('security.voter', array('priority' => 100))
         ;
         $container
-            ->register('highest_prio_service')
+            ->register('highest_prio_service', Voter::class)
             ->addTag('security.voter', array('priority' => 200))
         ;
         $container
-            ->register('zero_prio_service')
+            ->register('zero_prio_service', Voter::class)
             ->addTag('security.voter', array('priority' => 0))
         ;
         $compilerPass = new AddSecurityVotersPass();
@@ -65,4 +69,56 @@ public function testThatSecurityVotersAreProcessedInPriorityOrder()
         $this->assertEquals(new Reference('lowest_prio_service'), $refs[1]);
         $this->assertCount(4, $refs);
     }
+
+    /**
+     * @group legacy
+     * @expectedDeprecation Using a security.voter tag on a class without implementing the Symfony\Component\Security\Core\Authorization\Voter\VoterInterface is deprecated as of 3.4 and will be removed in 4.0. Implement the Symfony\Component\Security\Core\Authorization\Voter\VoterInterface instead.
+     */
+    public function testVoterMissingInterface()
+    {
+        $container = new ContainerBuilder();
+        $container
+            ->register('security.access.decision_manager', AccessDecisionManager::class)
+            ->addArgument(array())
+        ;
+        $container
+            ->register('without_interface', VoterWithoutInterface::class)
+            ->addTag('security.voter')
+        ;
+        $compilerPass = new AddSecurityVotersPass();
+        $compilerPass->process($container);
+
+        $argument = $container->getDefinition('security.access.decision_manager')->getArgument(0);
+        $refs = $argument->getValues();
+        $this->assertEquals(new Reference('without_interface'), $refs[0]);
+        $this->assertCount(1, $refs);
+    }
+
+    /**
+     * @group legacy
+     */
+    public function testVoterMissingInterfaceAndMethod()
+    {
+        $exception = LogicException::class;
+        $message = 'stdClass should implement the Symfony\Component\Security\Core\Authorization\Voter\VoterInterface interface when used as voter.';
+
+        if (method_exists($this, 'expectException')) {
+            $this->expectException($exception);
+            $this->expectExceptionMessage($message);
+        } else {
+            $this->setExpectedException($exception, $message);
+        }
+
+        $container = new ContainerBuilder();
+        $container
+            ->register('security.access.decision_manager', AccessDecisionManager::class)
+            ->addArgument(array())
+        ;
+        $container
+            ->register('without_method', 'stdClass')
+            ->addTag('security.voter')
+        ;
+        $compilerPass = new AddSecurityVotersPass();
+        $compilerPass->process($container);
+    }
 }

src/Symfony/Component/Security/CHANGELOG.md

@@ -1,6 +1,12 @@
 CHANGELOG
 =========
 
+3.4.0
+-----
+
+ * Using voters that do not implement the `VoterInterface`is now deprecated in
+   the `AccessDecisionManager` and this functionality will be removed in 4.0.
+
 3.3.0
 -----
 

src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php

@@ -13,6 +13,7 @@
 
 use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Core\Exception\LogicException;
 
 /**
  * AccessDecisionManager is the base class for all access decision managers
@@ -84,7 +85,7 @@ private function decideAffirmative(TokenInterface $token, array $attributes, $ob
     {
         $deny = 0;
         foreach ($this->voters as $voter) {
-            $result = $voter->vote($token, $object, $attributes);
+            $result = $this->vote($voter, $token, $object, $attributes);
             switch ($result) {
                 case VoterInterface::ACCESS_GRANTED:
                     return true;
@@ -125,7 +126,7 @@ private function decideConsensus(TokenInterface $token, array $attributes, $obje
         $grant = 0;
         $deny = 0;
         foreach ($this->voters as $voter) {
-            $result = $voter->vote($token, $object, $attributes);
+            $result = $this->vote($voter, $token, $object, $attributes);
 
             switch ($result) {
                 case VoterInterface::ACCESS_GRANTED:
@@ -166,7 +167,7 @@ private function decideUnanimous(TokenInterface $token, array $attributes, $obje
         $grant = 0;
         foreach ($this->voters as $voter) {
             foreach ($attributes as $attribute) {
-                $result = $voter->vote($token, $object, array($attribute));
+                $result = $this->vote($voter, $token, $object, array($attribute));
 
                 switch ($result) {
                     case VoterInterface::ACCESS_GRANTED:
@@ -190,4 +191,27 @@ private function decideUnanimous(TokenInterface $token, array $attributes, $obje
 
         return $this->allowIfAllAbstainDecisions;
     }
+
+    /**
+     * TokenInterface vote proxy method.
+     *
+     * Acts as a BC layer when the VoterInterface is not implemented on the voter.
+     *
+     * @deprecated as of 3.4 and will be removed in 4.0. Call the voter directly as the instance will always be a VoterInterface
+     */
+    private function vote($voter, TokenInterface $token, $subject, $attributes)
+    {
+        if ($voter instanceof VoterInterface) {
+            return $voter->vote($token, $subject, $attributes);
+        }
+
+        if (method_exists($voter, 'vote')) {
+            @trigger_error(sprintf('Calling vote() on an voter without %1$s is deprecated as of 3.4 and will be removed in 4.0. Implement the %1$s on your voter.', VoterInterface::class), E_USER_DEPRECATED);
+
+            // making the assumption that the signature matches
+            return $voter->vote($token, $subject, $attributes);
+        }
+
+        throw new LogicException(sprintf('%s should implement the %s interface when used as voter.', get_class($voter), VoterInterface::class));
+    }
 }

src/Symfony/Component/Security/Core/Exception/LogicException.php

@@ -0,0 +1,21 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Core\Exception;
+
+/**
+ * Base LogicException for the Security component.
+ *
+ * @author Iltar van der Berg <kjarli@gmail.com>
+ */
+class LogicException extends \LogicException implements ExceptionInterface
+{
+}

src/Symfony/Component/Security/Core/Tests/Authorization/AccessDecisionManagerTest.php

@@ -12,8 +12,11 @@
 namespace Symfony\Component\Security\Core\Tests\Authorization;
 
 use PHPUnit\Framework\TestCase;
+use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Authorization\AccessDecisionManager;
 use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
+use Symfony\Component\Security\Core\Exception\LogicException;
+use Symfony\Component\Security\Core\Tests\Authorization\Stub\VoterWithoutInterface;
 
 class AccessDecisionManagerTest extends TestCase
 {
@@ -138,4 +141,34 @@ protected function getVoter($vote)
 
         return $voter;
     }
+
+    public function testVotingWrongTypeNoVoteMethod()
+    {
+        $exception = LogicException::class;
+        $message = sprintf('stdClass should implement the %s interface when used as voter.', VoterInterface::class);
+
+        if (method_exists($this, 'expectException')) {
+            $this->expectException($exception);
+            $this->expectExceptionMessage($message);
+        } else {
+            $this->setExpectedException($exception, $message);
+        }
+
+        $adm = new AccessDecisionManager(array(new \stdClass()));
+        $token = $this->getMockBuilder(TokenInterface::class)->getMock();
+
+        $adm->decide($token, array('TEST'));
+    }
+
+    /**
+     * @group legacy
+     * @expectedDeprecation Calling vote() on an voter without Symfony\Component\Security\Core\Authorization\Voter\VoterInterface is deprecated as of 3.4 and will be removed in 4.0. Implement the Symfony\Component\Security\Core\Authorization\Voter\VoterInterface on your voter.
+     */
+    public function testVotingWrongTypeWithVote()
+    {
+        $adm = new AccessDecisionManager(array(new VoterWithoutInterface()));
+        $token = $this->getMockBuilder(TokenInterface::class)->getMock();
+
+        $adm->decide($token, array('TEST'));
+    }
 }

src/Symfony/Component/Security/Core/Tests/Authorization/Stub/VoterWithoutInterface.php

@@ -0,0 +1,22 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <fabien@symfony.com>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Security\Core\Tests\Authorization\Stub;
+
+/**
+ * @author Iltar van der Berg <kjarli@gmail.com>
+ */
+class VoterWithoutInterface
+{
+    public function vote()
+    {
+    }
+}