8000 feature #22629 [Security] Trigger a deprecation when a voter is missi… · symfony/symfony@bc4dd8f · GitHub
[go: up one dir, main page]
Skip to content

Commit bc4dd8f

Browse files
fabpotfabpot
committed
feature #22629 [Security] Trigger a deprecation when a voter is missing 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
2 parents e992eae + 9c253e1 commit bc4dd8f

File tree

9 files changed

+190
-9
lines changed

9 files changed

+190
-9
lines changed

‎UPGRADE-3.4.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,9 @@ Process
4747
SecurityBundle
4848
--------------
4949

50+
* Using voters that do not implement the `VoterInterface`is now deprecated in
51+
the `AccessDecisionManager` and this functionality will be removed in 4.0.
52+
5053
* `FirewallContext::getListeners()` now returns `\Traversable|array`
5154

5255
Validator

‎src/Symfony/Bundle/SecurityBundle/CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ CHANGELOG
44
3.4.0
55
-----
66

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

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

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
1717
use Symfony\Component\DependencyInjection\Compiler\PriorityTaggedServiceTrait;
1818
use Symfony\Component\DependencyInjection\Exception\LogicException;
19+
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
1920

2021
/**
2122
* Adds all configured security voters to the access decision manager.
@@ -40,6 +41,19 @@ public function process(ContainerBuilder $container)
4041
throw new LogicException('No security voters found. You need to tag at least one with "security.voter"');
4142
}
4243

44+
foreach ($voters as $voter) {
45+
$class = $container->getDefinition((string) $voter)->getClass();
46+
47+
if (!is_a($class, VoterInterface::class, true)) {
48+
@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);
49+
}
50+
51+
if (!method_exists($class, 'vote')) {
52+
// in case the vote method is completely missing, to prevent exceptions when voting
53+
throw new LogicException(sprintf('%s should implement the %s interface when used as voter.', $class, VoterInterface::class));
54+
}
55+
}
56+
4357
$adm = $container->getDefinition('security.access.decision_manager');
4458
$adm->replaceArgument(0, new IteratorArgument($voters));
4559
}

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

Lines changed: 62 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,11 @@
1414
use PHPUnit\Framework\TestCase;
1515
use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\AddSecurityVotersPass;
1616
use Symfony\Component\DependencyInjection\ContainerBuilder;
17+
use Symfony\Component\DependencyInjection\Exception\LogicException;
1718
use Symfony\Component\DependencyInjection\Reference;
19+
use Symfony\Component\Security\Core\Authorization\AccessDecisionManager;
20+
use Symfony\Component\Security\Core\Authorization\Voter\Voter;
21+
use Symfony\Component\Security\Core\Tests\Authorization\Stub\VoterWithoutInterface;
1822

1923
class AddSecurityVotersPassTest extends TestCase
2024
{
@@ -25,7 +29,7 @@ public function testNoVoters()
2529
{
2630
$container = new ContainerBuilder();
2731
$container
28-
->register('security.access.decision_manager', 'Symfony\Component\Security\Core\Authorization\AccessDecisionManager')
32+
->register('security.access.decision_manager', AccessDecisionManager::class)
2933
->addArgument(array())
3034
;
3135

@@ -37,23 +41,23 @@ public function testThatSecurityVotersAreProcessedInPriorityOrder()
3741
{
3842
$container = new ContainerBuilder();
3943
$container
40-
->register('security.access.decision_manager', 'Symfony\Component\Security\Core\Authorization\AccessDecisionManager')
44+
->register('security.access.decision_manager', AccessDecisionManager::class)
4145
->addArgument(array())
4246
;
4347
$container
44-
->register('no_prio_service')
48+
->register('no_prio_service', Voter::class)
4549
->addTag('security.voter')
4650
;
4751
$container
48-
->register('lowest_prio_service')
52+
->register('lowest_prio_service', Voter::class)
4953
->addTag('security.voter', array('priority' => 100))
5054
;
5155
$container
52-
->register('highest_prio_service')
56+
->register('highest_prio_service', Voter::class)
5357
->addTag('security.voter', array('priority' => 200))
5458
;
5559
$container
56-
->register('zero_prio_service')
60+
->register('zero_prio_service', Voter::class)
5761
->addTag('security.voter', array('priority' => 0))
5862
;
5963
$compilerPass = new AddSecurityVotersPass();
@@ -65,4 +69,56 @@ public function testThatSecurityVotersAreProcessedInPriorityOrder()
6569
$this->assertEquals(new Reference('lowest_prio_service'), $refs[1]);
6670
$this->assertCount(4, $refs);
6771
}
72+
73+
/**
74+
* @group legacy
75+
* @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.
76+
*/
77+
public function testVoterMissingInterface()
78+
{
79+
$container = new ContainerBuilder();
80+
$container
81+
->register('security.access.decision_manager', AccessDecisionManager::class)
82+
->addArgument(array())
83+
;
84+
$container
85+
->register('without_interface', VoterWithoutInterface::class)
86+
->addTag('security.voter')
87+
;
88+
$compilerPass = new AddSecurityVotersPass();
89+
$compilerPass->process($container);
90+
91+
$argument = $container->getDefinition('security.access.decision_manager')->getArgument(0);
92+
$refs = $argument->getValues();
93+
$this->assertEquals(new Reference('without_interface'), $refs[0]);
94+
$this->assertCount(1, $refs);
95+
}
96+
97+
/**
98+
* @group legacy
99+
*/
100+
public function testVoterMissingInterfaceAndMethod()
101+
{
102+
$exception = LogicException::class;
103+
$message = 'stdClass should implement the Symfony\Component\Security\Core\Authorization\Voter\VoterInterface interface when used as voter.';
104+
105+
if (method_exists($this, 'expectException')) {
106+
$this->expectException($exception);
107+
$this->expectExceptionMessage($message);
108+
} else {
109+
$this->setExpectedException($exception, $message);
110+
}
111+
112+
$container = new ContainerBuilder();
113+
$container
114+
->register('security.access.decision_manager', AccessDecisionManager::class)
115+
->addArgument(array())
116+
;
117+
$container
118+
->register('without_method', 'stdClass')
119+
->addTag('security.voter')
120+
;
121+
$compilerPass = new AddSecurityVotersPass();
122+
$compilerPass->process($container);
123+
}
68124
}

‎src/Symfony/Component/Security/CHANGELOG.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,12 @@
11
CHANGELOG
22
=========
33

4+
3.4.0
5+
-----
6+
7+
* Using voters that do not implement the `VoterInterface`is now deprecated in
8+
the `AccessDecisionManager` and this functionality will be removed in 4.0.
9+
410
3.3.0
511
-----
612

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

Lines changed: 27 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@
1313

1414
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
1515
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
16+
use Symfony\Component\Security\Core\Exception\LogicException;
1617

1718
/**
1819
* AccessDecisionManager is the base class for all access decision managers
@@ -84,7 +85,7 @@ private function decideAffirmative(TokenInterface $token, array $attributes, $ob
8485
{
8586
$deny = 0;
8687
foreach ($this->voters as $voter) {
87-
$result = $voter->vote($token, $object, $attributes);
88+
$result = $this->vote($voter, $token, $object, $attributes);
8889
switch ($result) {
8990
case VoterInterface::ACCESS_GRANTED:
9091
return true;
@@ -125,7 +126,7 @@ private function decideConsensus(TokenInterface $token, array $attributes, $obje
125126
$grant = 0;
126127
$deny = 0;
127128
foreach ($this->voters as $voter) {
128-
$result = $voter->vote($token, $object, $attributes);
129+
$result = $this->vote($voter, $token, $object, $attributes);
129130

130131
switch ($result) {
131132
case VoterInterface::ACCESS_GRANTED:
@@ -166,7 +167,7 @@ private function decideUnanimous(TokenInterface $token, array $attributes, $obje
166167
$grant = 0;
167168
foreach ($this->voters as $voter) {
168169
foreach ($attributes as $attribute) {
169-
$result = $voter->vote($token, $object, array($attribute));
170+
$result = $this->vote($voter, $token, $object, array($attribute));
170171

171172
switch ($result) {
172173
case VoterInterface::ACCESS_GRANTED:
@@ -190,4 +191,27 @@ private function decideUnanimous(TokenInterface $token, array $attributes, $obje
190191

191192
return $this->allowIfAllAbstainDecisions;
192193
}
194+
195+
/**
196+
* TokenInterface vote proxy method.
197+
*
198+
* Acts as a BC layer when the VoterInterface is not implemented on the voter.
199+
*
200+
* @deprecated as of 3.4 and will be removed in 4.0. Call the voter directly as the instance will always be a VoterInterface
201+
*/
202+
private function vote($voter, TokenInterface $token, $subject, $attributes)
203+
{
204+
if ($voter instanceof VoterInterface) {
205+
return $voter->vote($token, $subject, $attributes);
206+
}
207+
208+
if (method_exists($voter, 'vote')) {
209+
@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);
210+
211+
// making the assumption that the signature matches
212+
return $voter->vote($token, $subject, $attributes);
213+
}
214+
215+
throw new LogicException(sprintf('%s should implement the %s interface when used as voter.', get_class($voter), VoterInterface::class));
216+
}
193217
}

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

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\Security\Core\Exception;
13+
14+
/**
15+
* Base LogicException for the Security component.
16+
*
17+
* @author Iltar van der Berg <kjarli@gmail.com>
18+
*/
19+
class LogicException extends \LogicException implements ExceptionInterface
20+
{
21+
}

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,11 @@
1212
namespace Symfony\Component\Security\Core\Tests\Authorization;
1313

1414
use PHPUnit\Framework\TestCase;
15+
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
1516
use Symfony\Component\Security\Core\Authorization\AccessDecisionManager;
1617
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
18+
use Symfony\Component\Security\Core\Exception\LogicException;
19+
use Symfony\Component\Security\Core\Tests\Authorization\Stub\VoterWithoutInterface;
1720

1821
class AccessDecisionManagerTest extends TestCase
1922
{
@@ -138,4 +141,34 @@ protected function getVoter($vote)
138141

139142
return $voter;
140143
}
144+
145+
public function testVotingWrongTypeNoVoteMethod()
146+
{
147+
$exception = LogicException::class;
148+
$message = sprintf('stdClass should implement the %s interface when used as voter.', VoterInterface::class);
149+
150+
if (method_exists($this, 'expectException')) {
151+
$this->expectException($exception);
152+
$this->expectExceptionMessage($message);
153+
} else {
154+
$this->setExpectedException($exception, $message);
155+
}
156+
157+
$adm = new AccessDecisionManager(array(new \stdClass()));
158+
$token = $this->getMockBuilder(TokenInterface::class)->getMock();
159+
160+
$adm->decide($token, array('TEST'));
161+
}
162+
163+
/**
164+
* @group legacy
165+
* @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.
166+
*/
167+
public function testVotingWrongTypeWithVote()
168+
{
169+
$adm = new AccessDecisionManager(array(new VoterWithoutInterface()));
170+
$token = $this->getMockBuilder(TokenInterface::class)->getMock();
171+
172+
$adm->decide($token, array('TEST'));
173+
}
141174
}

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
<?php
2+
3+
/*
4+
* This file is part of the Symfony package.
5+
*
6+
* (c) Fabien Potencier <fabien@symfony.com>
7+
*
8+
* For the full copyright and license information, please view the LICENSE
9+
* file that was distributed with this source code.
10+
*/
11+
12+
namespace Symfony\Component\Security\Core\Tests\Authorization\Stub;
13+
14+
/**
15+
* @author Iltar van der Berg <kjarli@gmail.com>
16+
*/
17+
class VoterWithoutInterface
18+
{
19+
public function vote()
20+
{
21+
}
22+
}

0 commit comments

Comments
 (0)
0