From 9c253e1ff67e9906ca8ed0831a508a1f63670995 Mon Sep 17 00:00:00 2001 From: Iltar van der Berg Date: Thu, 4 May 2017 09:35:02 +0200 Subject: [PATCH] [Security] Trigger a deprecation when a voter is missing the VoterInterface --- UPGRADE-3.4.md | 8 +++ .../Bundle/SecurityBundle/CHANGELOG.md | 6 ++ .../Compiler/AddSecurityVotersPass.php | 14 ++++ .../Compiler/AddSecurityVotersPassTest.php | 68 +++++++++++++++++-- src/Symfony/Component/Security/CHANGELOG.md | 6 ++ .../Authorization/AccessDecisionManager.php | 30 +++++++- .../Core/Exception/LogicException.php | 21 ++++++ .../AccessDecisionManagerTest.php | 33 +++++++++ .../Stub/VoterWithoutInterface.php | 22 ++++++ 9 files changed, 199 insertions(+), 9 deletions(-) create mode 100644 UPGRADE-3.4.md create mode 100644 src/Symfony/Component/Security/Core/Exception/LogicException.php create mode 100644 src/Symfony/Component/Security/Core/Tests/Authorization/Stub/VoterWithoutInterface.php diff --git a/UPGRADE-3.4.md b/UPGRADE-3.4.md new file mode 100644 index 0000000000..28135e44cf --- /dev/null +++ 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 index 9691e5af03..715691a8e1 100644 --- 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 index e907b7d567..67d0785b47 100644 --- 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\ContainerBuilder; 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 @@ class AddSecurityVotersPass implements CompilerPassInterface 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/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/AddSecurityVotersPassTest.php b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/AddSecurityVotersPassTest.php index 66d6bde205..a20b39cee8 100644 --- a/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/AddSecurityVotersPassTest.php +++ b/src/Symfony/Bundle/SecurityBundle/Tests/DependencyInjection/Compiler/AddSecurityVotersPassTest.php @@ -14,7 +14,11 @@ namespace Symfony\Bundle\SecurityBundle\Tests\DependencyInjection\Compiler; 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 @@ class AddSecurityVotersPassTest extends TestCase { $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 @@ class AddSecurityVotersPassTest extends TestCase { $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 @@ class AddSecurityVotersPassTest extends TestCase $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 index 69cdd28528..292c304fc6 100644 --- 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 index 431597940d..f50caf8cfe 100644 --- a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php +++ b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php @@ -13,6 +13,7 @@ namespace Symfony\Component\Security\Core\Authorization; 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 @@ class AccessDecisionManager implements AccessDecisionManagerInterface { $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 @@ class AccessDecisionManager implements AccessDecisionManagerInterface $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 @@ class AccessDecisionManager implements AccessDecisionManagerInterface $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 @@ class AccessDecisionManager implements AccessDecisionManagerInterface 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 new file mode 100644 index 0000000000..b9c63e941f --- /dev/null +++ b/src/Symfony/Component/Security/Core/Exception/LogicException.php @@ -0,0 +1,21 @@ + + * + * 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 + */ +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 index 192fe87e2a..61d85274fc 100644 --- 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 @@ class AccessDecisionManagerTest extends TestCase 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 new file mode 100644 index 0000000000..09c284d3c6 --- /dev/null +++ b/src/Symfony/Component/Security/Core/Tests/Authorization/Stub/VoterWithoutInterface.php @@ -0,0 +1,22 @@ + + * + * 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 + */ +class VoterWithoutInterface +{ + public function vote() + { + } +}