Skip to content

Commit 2e8ee97

Browse files
fabpotfabpot
committed
feature #43066 [Security] Cache voters that will always abstain (jderusse)
This PR was merged into the 5.4 branch. Discussion ---------- [Security] Cache voters that will always abstain | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | - | License | MIT | Doc PR | todo The current implementation of the AccessDecisionManager is to iterate over all the Voters, and stops when the strategy is met. When an application performs a lot of checks (ie. an Admin shows a list of "blog post" and shows buttons to users that are allowed to "edit", "delete", "publish", ... ie `{% if is_granted('blog_delete', item) %}DELETE{% endif %}`). In that case, the number of calls to the `VoterInterface::vote` methods grows exponentially. At some point, adding a new Voter to handle a new case (maybe not related to our blog posts) will have a performance impact Moreover in most of the time, a voter looks like: ```php protected function supports($attribute, $subject) { return \in_array($attribute, ['BLOG_DELETE', 'BLOG_UPDATE'], true) && $subject instanceof BlogPost; } ``` We could leverage this, to cache the list of voters that need to be called or can be ignored In the same way `symfony/serializer` provides a `CacheableSupportsMethodInterface` I suggest to add a new `CacheableAbstainVotesInterface` that will remember voters that will always abstain vote on a given attribute or a given object's type When the Voter does not implements the interface OR returns `false` will provide the current behavior: The voter is always called. How to use this PR: ```php class TokenVoter extends Voter implements CacheableAbstainVotesInterface { public const ATTRIBUTES = [ 'UPDATE', 'DELETE', 'REGENERATE', ]; public function willAlwaysAbstainOnAttribute(string $attribute): bool { return !\in_array($attribute, self::ATTRIBUTES, true); } public function willAlwaysAbstainOnObjectType(string $objectType): bool { return !is_a(Token::class, $objectType); } protected function supports($attribute, $subject) { return \in_array($attribute, self::ATTRIBUTES, true) && $subject instanceof Token; } protected function voteOnAttribute($attribute, $webhookToken, TokenInterface $token) { ... } } ``` Results: For 40 Voters and calling 500 times `isGranted(variousAttributes, variousObjects)` it get -40% perf gain ! Opinionated choices in that PR: * Only for string attributes: handling all cases (resources, callable, ...) would add too much complexity * Only for decision with a single attribute: handling mix of string/null/whatever would add too much complexity TODO * [ ] tests (waiting for feedback on the global design of this feature) * [ ] Doc Commits ------- fc419ed430 Cache voters that will always abstain
2 parents 6ef2b2b + 4e07969 commit 2e8ee97

10 files changed

+380
-7
lines changed

‎Authorization/AccessDecisionManager.php

Lines changed: 51 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
namespace Symfony\Component\Security\Core\Authorization;
1313

1414
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
15+
use Symfony\Component\Security\Core\Authorization\Voter\CacheableVoterInterface;
1516
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
1617
use Symfony\Component\Security\Core\Exception\InvalidArgumentException;
1718

@@ -29,6 +30,8 @@ class AccessDecisionManager implements AccessDecisionManagerInterface
2930
public const STRATEGY_PRIORITY = 'priority';
3031

3132
private $voters;
33+
private $votersCacheAttributes;
34+
private $votersCacheObject;
3235
private $strategy;
3336
private $allowIfAllAbstainDecisions;
3437
private $allowIfEqualGrantedDeniedDecisions;
@@ -80,7 +83,7 @@ public function decide(TokenInterface $token, array $attributes, $object = null/
8083
private function decideAffirmative(TokenInterface $token, array $attributes, $object = null): bool
8184
{
8285
$deny = 0;
83-
foreach ($this->voters as $voter) {
86+
foreach ($this->getVoters($attributes, $object) as $voter) {
8487
$result = $voter->vote($token, $object, $attributes);
8588

8689
if (VoterInterface::ACCESS_GRANTED === $result) {
@@ -119,7 +122,7 @@ private function decideConsensus(TokenInterface $token, array $attributes, $obje
119122
{
120123
$grant = 0;
121124
$deny = 0;
122-
foreach ($this->voters as $voter) {
125+
foreach ($this->getVoters($attributes, $object) as $voter) {
123126
$result = $voter->vote($token, $object, $attributes);
124127

125128
if (VoterInterface::ACCESS_GRANTED === $result) {
@@ -155,7 +158,7 @@ private function decideConsensus(TokenInterface $token, array $attributes, $obje
155158
private function decideUnanimous(TokenInterface $token, array $attributes, $object = null): bool
156159
{
157160
$grant = 0;
158-
foreach ($this->voters as $voter) {
161+
foreach ($this->getVoters($attributes, $object) as $voter) {
159162
foreach ($attributes as $attribute) {
160163
$result = $voter->vote($token, $object, [$attribute]);
161164

@@ -188,7 +191,7 @@ private function decideUnanimous(TokenInterface $token, array $attributes, $obje
188191
*/
189192
private function decidePriority(TokenInterface $token, array $attributes, $object = null)
190193
{
191-
foreach ($this->voters as $voter) {
194+
foreach ($this->getVoters($attributes, $object) as $voter) {
192195
$result = $voter->vote($token, $object, $attributes);
193196

194197
if (VoterInterface::ACCESS_GRANTED === $result) {
@@ -206,4 +209,48 @@ private function decidePriority(TokenInterface $token, array $attributes, $objec
206209

207210
return $this->allowIfAllAbstainDecisions;
208211
}
212+
213+
private function getVoters(array $attributes, $object = null): iterable
214+
{
215+
$keyAttributes = [];
216+
foreach ($attributes as $attribute) {
217+
$keyAttributes[] = \is_string($attribute) ? $attribute : null;
218+
}
219+
// use `get_class` to handle anonymous classes
220+
$keyObject = \is_object($object) ? \get_class($object) : get_debug_type($object);
221+
foreach ($this->voters as $key => $voter) {
222+
if (!$voter instanceof CacheableVoterInterface) {
223+
yield $voter;
224+
continue;
225+
}
226+
227+
$supports = true;
228+
// The voter supports the attributes if it supports at least one attribute of the list
229+
foreach ($keyAttributes as $keyAttribute) {
230+
if (null === $keyAttribute) {
231+
$supports = true;
232+
} elseif (!isset($this->votersCacheAttributes[$keyAttribute][$key])) {
233+
$this->votersCacheAttributes[$keyAttribute][$key] = $supports = $voter->supportsAttribute($keyAttribute);
234+
} else {
235+
$supports = $this->votersCacheAttributes[$keyAttribute][$key];
236+
}
237+
if ($supports) {
238+
break;
239+
}
240+
}
241+
if (!$supports) {
242+
continue;
243+
}
244+
245+
if (!isset($this->votersCacheObject[$keyObject][$key])) {
246+
$this->votersCacheObject[$keyObject][$key] = $supports = $voter->supportsType($keyObject);
247+
} else {
248+
$supports = $this->votersCacheObject[$keyObject][$key];
249+
}
250+
if (!$supports) {
251+
continue;
252+
}
253+
yield $voter;
254+
}
255+
}
209256
}

‎Authorization/Voter/AuthenticatedVoter.php

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
* @author Fabien Potencier <fabien@symfony.com>
2525
* @author Johannes M. Schmitt <schmittjoh@gmail.com>
2626
*/
27-
class AuthenticatedVoter implements VoterInterface
27+
class AuthenticatedVoter implements CacheableVoterInterface
2828
{
2929
public const IS_AUTHENTICATED_FULLY = 'IS_AUTHENTICATED_FULLY';
3030
public const IS_AUTHENTICATED_REMEMBERED = 'IS_AUTHENTICATED_REMEMBERED';
@@ -116,4 +116,23 @@ public function vote(TokenInterface $token, $subject, array $attributes)
116116

117117
return $result;
118118
}
119+
120+
public function supportsAttribute(string $attribute): bool
121+
{
122+
return \in_array($attribute, [
123+
self::IS_AUTHENTICATED_FULLY,
124+
self::IS_AUTHENTICATED_REMEMBERED,
125+
self::IS_AUTHENTICATED_ANONYMOUSLY,
126+
self::IS_AUTHENTICATED,
127+
self::IS_ANONYMOUS,
128+
self::IS_IMPERSONATOR,
129+
self::IS_REMEMBERED,
130+
self::PUBLIC_ACCESS,
131+
], true);
132+
}
133+
134+
public function supportsType(string $subjectType): bool
135+
{
136+
return true;
137+
}
119138
}

‎Authorization/Voter/CacheableVoterInterface.php

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
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\Authorization\Voter;
13+
14+
/**
15+
* Let voters expose the attributes and types they care about.
16+
*
17+
* By returning false to either `supportsAttribute` or `supportsType`, the
18+
* voter will never be called for the specified attribute or subject.
19+
*
20+
* @author Jérémy Derussé <jeremy@derusse.com>
21+
*/
22+
interface CacheableVoterInterface extends VoterInterface
23+
{
24+
public function supportsAttribute(string $attribute): bool;
25+
26+
/**
27+
* @param string $subjectType The type of the subject inferred by `get_class` or `get_debug_type`
28+
*/
29+
public function supportsType(string $subjectType): bool;
30+
}

‎Authorization/Voter/RoleVoter.php

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@
1818
*
1919
* @author Fabien Potencier <fabien@symfony.com>
2020
*/
21-
class RoleVoter implements VoterInterface
21+
class RoleVoter implements CacheableVoterInterface
2222
{
2323
private $prefix;
2424

@@ -55,6 +55,16 @@ public function vote(TokenInterface $token, $subject, array $attributes)
5555
return $result;
5656
}
5757

58+
public function supportsAttribute(string $attribute): bool
59+
{
60+
return str_starts_with($attribute, $this->prefix);
61+
}
62+
63+
public function supportsType(string $subjectType): bool
64+
{
65+
return true;
66+
}
67+
5868
protected function extractRoles(TokenInterface $token)
5969
{
6070
return $token->getRoleNames();

‎Authorization/Voter/TraceableVoter.php

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@
2222
*
2323
* @internal
2424
*/
25-
class TraceableVoter implements VoterInterface
25+
class TraceableVoter implements CacheableVoterInterface
2626
{
2727
private $voter;
2828
private $eventDispatcher;
@@ -46,4 +46,14 @@ public function getDecoratedVoter(): VoterInterface
4646
{
4747
return $this->voter;
4848
}
49+
50+
public function supportsAttribute(string $attribute): bool
51+
{
52+
return !$this->voter instanceof CacheableVoterInterface || $this->voter->supportsAttribute($attribute);
53+
}
54+
55+
public function supportsType(string $subjectType): bool
56+
{
57+
return !$this->voter instanceof CacheableVoterInterface || $this->voter->supportsType($subjectType);
58+
}
4959
}

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ CHANGELOG
44
5.4
55
---
66

7+
* Add a `CacheableVoterInterface` for voters that vote only on identified attributes and subjects
78
* Deprecate `AuthenticationEvents::AUTHENTICATION_FAILURE`, use the `LoginFailureEvent` instead
89
* Deprecate `AnonymousToken`, as the related authenticator was deprecated in 5.3
910
* Deprecate `Token::getCredentials()`, tokens should no longer contain credentials (as they represent authenticated sessions)

‎Tests/Authorization/AccessDecisionManagerTest.php

Lines changed: 138 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
1616
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
1717
use Symfony\Component\Security\Core\Authorization\AccessDecisionManager;
18+
use Symfony\Component\Security\Core\Authorization\Voter\CacheableVoterInterface;
1819
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
1920

2021
class AccessDecisionManagerTest extends TestCase
@@ -120,6 +121,143 @@ public function provideStrategies()
120121
yield [AccessDecisionManager::STRATEGY_PRIORITY];
121122
}
122123

124+
public function testCacheableVoters()
125+
{
126+
$token = $this->createMock(TokenInterface::class);
127+
$voter = $this->getMockBuilder(CacheableVoterInterface::class)->getMockForAbstractClass();
128+
$voter
129+
->expects($this->once())
130+
->method('supportsAttribute')
131+
->with('foo')
132+
->willReturn(true);
133+
$voter
134+
->expects($this->once())
135+
->method('supportsType')
136+
->with('string')
137+
->willReturn(true);
138+
$voter
139+
->expects($this->once())
140+
->method('vote')
141+
->with($token, 'bar', ['foo'])
142+
->willReturn(VoterInterface::ACCESS_GRANTED);
143+
144+
$manager = new AccessDecisionManager([$voter]);
145+
$this->assertTrue($manager->decide($token, ['foo'], 'bar'));
146+
}
147+
148+
public function testCacheableVotersIgnoresNonStringAttributes()
149+
{
150+
$token = $this->createMock(TokenInterface::class);
151+
$voter = $this->getMockBuilder(CacheableVoterInterface::class)->getMockForAbstractClass();
152+
$voter
153+
->expects($this->never())
154+
->method('supportsAttribute');
155+
$voter
156+
->expects($this->once())
157+
->method('supportsType')
158+
->with('string')
159+
->willReturn(true);
160+
$voter
161+
->expects($this->once())
162+
->method('vote')
163+
->with($token, 'bar', [1337])
164+
->willReturn(VoterInterface::ACCESS_GRANTED);
165+
166+
$manager = new AccessDecisionManager([$voter]);
167+
$this->assertTrue($manager->decide($token, [1337], 'bar'));
168+
}
169+
170+
public function testCacheableVotersWithMultipleAttributes()
171+
{
172+
$token = $this->createMock(TokenInterface::class);
173+
$voter = $this->getMockBuilder(CacheableVoterInterface::class)->getMockForAbstractClass();
174+
$voter
175+
->expects($this->exactly(2))
176+
->method('supportsAttribute')
177+
->withConsecutive(['foo'], ['bar'])
178+
->willReturnOnConsecutiveCalls(false, true);
179+
$voter
180+
->expects($this->once())
181+
->method('supportsType')
182+
->with('string')
183+
->willReturn(true);
184+
$voter
185+
->expects($this->once())
186+
->method('vote')
187+
->with($token, 'bar', ['foo', 'bar'])
188+
->willReturn(VoterInterface::ACCESS_GRANTED);
189+
190+
$manager = new AccessDecisionManager([$voter]);
191+
$this->assertTrue($manager->decide($token, ['foo', 'bar'], 'bar', true));
192+
}
193+
194+
public function testCacheableVotersWithEmptyAttributes()
195+
{
196+
$token = $this->createMock(TokenInterface::class);
197+
$voter = $this->getMockBuilder(CacheableVoterInterface::class)->getMockForAbstractClass();
198+
$voter
199+
->expects($this->never())
200+
->method('supportsAttribute');
201+
$voter
202+
->expects($this->once())
203+
->method('supportsType')
204+
->with('string')
205+
->willReturn(true);
206+
$voter
207+
->expects($this->once())
208+
->method('vote')
209+
->with($token, 'bar', [])
210+
->willReturn(VoterInterface::ACCESS_GRANTED);
211+
212+
$manager = new AccessDecisionManager([$voter]);
213+
$this->assertTrue($manager->decide($token, [], 'bar'));
214+
}
215+
216+
public function testCacheableVotersSupportsMethodsCalledOnce()
217+
{
218+
$token = $this->createMock(TokenInterface::class);
219+
$voter = $this->getMockBuilder(CacheableVoterInterface::class)->getMockForAbstractClass();
220+
$voter
221+
->expects($this->once())
222+
->method('supportsAttribute')
223+
->with('foo')
224+
->willReturn(true);
225+
$voter
226+
->expects($this->once())
227+
->method('supportsType')
228+
->with('string')
229+
->willReturn(true);
230+
$voter
231+
->expects($this->exactly(2))
232+
->method('vote')
233+
->with($token, 'bar', ['foo'])
234+
->willReturn(VoterInterface::ACCESS_GRANTED);
235+
236+
$manager = new AccessDecisionManager([$voter]);
237+
$this->assertTrue($manager->decide($token, ['foo'], 'bar'));
238+
$this->assertTrue($manager->decide($token, ['foo'], 'bar'));
239+
}
240+
241+
public function testCacheableVotersNotCalled()
242+
{
243+
$token = $this->createMock(TokenInterface::class);
244+
$voter = $this->getMockBuilder(CacheableVoterInterface::class)->getMockForAbstractClass();
245+
$voter
246+
->expects($this->once())
247+
->method('supportsAttribute')
248+
->with('foo')
249+
->willReturn(false);
250+
$voter
251+
->expects($this->never())
252+
->method('supportsType');
253+
$voter
254+
->expects($this->never())
255+
->method('vote');
256+
257+
$manager = new AccessDecisionManager([$voter]);
258+
$this->assertFalse($manager->decide($token, ['foo'], 'bar'));
259+
}
260+
123261
protected function getVoters($grants, $denies, $abstains)
124262
{
125263
$voters = [];

0 commit comments

Comments
 (0)