symfony · linaori · May 4, 2017 · May 4, 2017 · May 4, 2017 · May 4, 2017
diff --git a/UPGRADE-3.4.md b/UPGRADE-3.4.md
@@ -0,0 +1,8 @@
+UPGRADE FROM 3.3 to 3.4
+=======================
+
+Security
+--------
+
+ * Using voters that do not implement the `VoterInterface`is now deprecated in
+   the `AccessDecisionManager` and this functionality will be removed in 4.0.
diff --git a/src/Symfony/Bundle/SecurityBundle/CHANGELOG.md b/src/Symfony/Bundle/SecurityBundle/CHANGELOG.md
@@ -1,6 +1,12 @@
 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.
+
 3.3.0
 -----
 

diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSecurityVotersPass.php b/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));
     }

diff --git a/...ny/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/AddSecurityVotersPassTest.php b/...ny/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);
+    }
 }
diff --git a/src/Symfony/Component/Security/CHANGELOG.md b/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
 -----
 

diff --git a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php b/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));
+    }
 }
diff --git a/src/Symfony/Component/Security/Core/Exception/LogicException.php b/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
+{
+}
diff --git a/src/Symfony/Component/Security/Core/Tests/Authorization/AccessDecisionManagerTest.php b/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'));
+    }
 }
diff --git a/src/Symfony/Component/Security/Core/Tests/Authorization/Stub/VoterWithoutInterface.php b/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()
+    {
+    }
+}