feature #43066 [Security] Cache voters that will always abstain (jderusse)

fabpot · fabpot · commit 2e8ee97e5708 · 2021-10-28T12:37:22.000+02:00
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) &amp;&amp; $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) &amp;&amp; $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
diff --git a/Authorization/AccessDecisionManager.php b/Authorization/AccessDecisionManager.php
@@ -12,6 +12,7 @@
 namespace Symfony\Component\Security\Core\Authorization;
 
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
+use Symfony\Component\Security\Core\Authorization\Voter\CacheableVoterInterface;
 use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
 use Symfony\Component\Security\Core\Exception\InvalidArgumentException;
 
@@ -29,6 +30,8 @@ class AccessDecisionManager implements AccessDecisionManagerInterface
     public const STRATEGY_PRIORITY = 'priority';
 
     private $voters;
+    private $votersCacheAttributes;
+    private $votersCacheObject;
     private $strategy;
     private $allowIfAllAbstainDecisions;
     private $allowIfEqualGrantedDeniedDecisions;
@@ -80,7 +83,7 @@ public function decide(TokenInterface $token, array $attributes, $object = null/
     private function decideAffirmative(TokenInterface $token, array $attributes, $object = null): bool
     {
         $deny = 0;
-        foreach ($this->voters as $voter) {
+        foreach ($this->getVoters($attributes, $object) as $voter) {
             $result = $voter->vote($token, $object, $attributes);
 
             if (VoterInterface::ACCESS_GRANTED === $result) {
@@ -119,7 +122,7 @@ private function decideConsensus(TokenInterface $token, array $attributes, $obje
     {
         $grant = 0;
         $deny = 0;
-        foreach ($this->voters as $voter) {
+        foreach ($this->getVoters($attributes, $object) as $voter) {
             $result = $voter->vote($token, $object, $attributes);
 
             if (VoterInterface::ACCESS_GRANTED === $result) {
@@ -155,7 +158,7 @@ private function decideConsensus(TokenInterface $token, array $attributes, $obje
     private function decideUnanimous(TokenInterface $token, array $attributes, $object = null): bool
     {
         $grant = 0;
-        foreach ($this->voters as $voter) {
+        foreach ($this->getVoters($attributes, $object) as $voter) {
             foreach ($attributes as $attribute) {
                 $result = $voter->vote($token, $object, [$attribute]);
 
@@ -188,7 +191,7 @@ private function decideUnanimous(TokenInterface $token, array $attributes, $obje
      */
     private function decidePriority(TokenInterface $token, array $attributes, $object = null)
     {
-        foreach ($this->voters as $voter) {
+        foreach ($this->getVoters($attributes, $object) as $voter) {
             $result = $voter->vote($token, $object, $attributes);
 
             if (VoterInterface::ACCESS_GRANTED === $result) {
@@ -206,4 +209,48 @@ private function decidePriority(TokenInterface $token, array $attributes, $objec
 
         return $this->allowIfAllAbstainDecisions;
     }
+
+    private function getVoters(array $attributes, $object = null): iterable
+    {
+        $keyAttributes = [];
+        foreach ($attributes as $attribute) {
+            $keyAttributes[] = \is_string($attribute) ? $attribute : null;
+        }
+        // use `get_class` to handle anonymous classes
+        $keyObject = \is_object($object) ? \get_class($object) : get_debug_type($object);
+        foreach ($this->voters as $key => $voter) {
+            if (!$voter instanceof CacheableVoterInterface) {
+                yield $voter;
+                continue;
+            }
+
+            $supports = true;
+            // The voter supports the attributes if it supports at least one attribute of the list
+            foreach ($keyAttributes as $keyAttribute) {
+                if (null === $keyAttribute) {
+                    $supports = true;
+                } elseif (!isset($this->votersCacheAttributes[$keyAttribute][$key])) {
+                    $this->votersCacheAttributes[$keyAttribute][$key] = $supports = $voter->supportsAttribute($keyAttribute);
+                } else {
+                    $supports = $this->votersCacheAttributes[$keyAttribute][$key];
+                }
+                if ($supports) {
+                    break;
+                }
+            }
+            if (!$supports) {
+                continue;
+            }
+
+            if (!isset($this->votersCacheObject[$keyObject][$key])) {
+                $this->votersCacheObject[$keyObject][$key] = $supports = $voter->supportsType($keyObject);
+            } else {
+                $supports = $this->votersCacheObject[$keyObject][$key];
+            }
+            if (!$supports) {
+                continue;
+            }
+            yield $voter;
+        }
+    }
 }
diff --git a/Authorization/Voter/AuthenticatedVoter.php b/Authorization/Voter/AuthenticatedVoter.php
@@ -24,7 +24,7 @@
  * @author Fabien Potencier <fabien@symfony.com>
  * @author Johannes M. Schmitt <schmittjoh@gmail.com>
  */
-class AuthenticatedVoter implements VoterInterface
+class AuthenticatedVoter implements CacheableVoterInterface
 {
     public const IS_AUTHENTICATED_FULLY = 'IS_AUTHENTICATED_FULLY';
     public const IS_AUTHENTICATED_REMEMBERED = 'IS_AUTHENTICATED_REMEMBERED';
@@ -116,4 +116,23 @@ public function vote(TokenInterface $token, $subject, array $attributes)
 
         return $result;
     }
+
+    public function supportsAttribute(string $attribute): bool
+    {
+        return \in_array($attribute, [
+            self::IS_AUTHENTICATED_FULLY,
+            self::IS_AUTHENTICATED_REMEMBERED,
+            self::IS_AUTHENTICATED_ANONYMOUSLY,
+            self::IS_AUTHENTICATED,
+            self::IS_ANONYMOUS,
+            self::IS_IMPERSONATOR,
+            self::IS_REMEMBERED,
+            self::PUBLIC_ACCESS,
+        ], true);
+    }
+
+    public function supportsType(string $subjectType): bool
+    {
+        return true;
+    }
 }
diff --git a/Authorization/Voter/CacheableVoterInterface.php b/Authorization/Voter/CacheableVoterInterface.php
@@ -0,0 +1,30 @@
+<?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\Authorization\Voter;
+
+/**
+ * Let voters expose the attributes and types they care about.
+ *
+ * By returning false to either `supportsAttribute` or `supportsType`, the
+ * voter will never be called for the specified attribute or subject.
+ *
+ * @author Jérémy Derussé <jeremy@derusse.com>
+ */
+interface CacheableVoterInterface extends VoterInterface
+{
+    public function supportsAttribute(string $attribute): bool;
+
+    /**
+     * @param string $subjectType The type of the subject inferred by `get_class` or `get_debug_type`
+     */
+    public function supportsType(string $subjectType): bool;
+}
diff --git a/Authorization/Voter/RoleVoter.php b/Authorization/Voter/RoleVoter.php
@@ -18,7 +18,7 @@
  *
  * @author Fabien Potencier <fabien@symfony.com>
  */
-class RoleVoter implements VoterInterface
+class RoleVoter implements CacheableVoterInterface
 {
     private $prefix;
 
@@ -55,6 +55,16 @@ public function vote(TokenInterface $token, $subject, array $attributes)
         return $result;
     }
 
+    public function supportsAttribute(string $attribute): bool
+    {
+        return str_starts_with($attribute, $this->prefix);
+    }
+
+    public function supportsType(string $subjectType): bool
+    {
+        return true;
+    }
+
     protected function extractRoles(TokenInterface $token)
     {
         return $token->getRoleNames();
diff --git a/Authorization/Voter/TraceableVoter.php b/Authorization/Voter/TraceableVoter.php
@@ -22,7 +22,7 @@
  *
  * @internal
  */
-class TraceableVoter implements VoterInterface
+class TraceableVoter implements CacheableVoterInterface
 {
     private $voter;
     private $eventDispatcher;
@@ -46,4 +46,14 @@ public function getDecoratedVoter(): VoterInterface
     {
         return $this->voter;
     }
+
+    public function supportsAttribute(string $attribute): bool
+    {
+        return !$this->voter instanceof CacheableVoterInterface || $this->voter->supportsAttribute($attribute);
+    }
+
+    public function supportsType(string $subjectType): bool
+    {
+        return !$this->voter instanceof CacheableVoterInterface || $this->voter->supportsType($subjectType);
+    }
 }
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,7 @@ CHANGELOG
 5.4
 ---
 
+ * Add a `CacheableVoterInterface` for voters that vote only on identified attributes and subjects
  * Deprecate `AuthenticationEvents::AUTHENTICATION_FAILURE`, use the `LoginFailureEvent` instead
  * Deprecate `AnonymousToken`, as the related authenticator was deprecated in 5.3
  * Deprecate `Token::getCredentials()`, tokens should no longer contain credentials (as they represent authenticated sessions)
diff --git a/Tests/Authorization/AccessDecisionManagerTest.php b/Tests/Authorization/AccessDecisionManagerTest.php
@@ -15,6 +15,7 @@
 use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Symfony\Component\Security\Core\Authorization\AccessDecisionManager;
+use Symfony\Component\Security\Core\Authorization\Voter\CacheableVoterInterface;
 use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
 
 class AccessDecisionManagerTest extends TestCase
@@ -120,6 +121,143 @@ public function provideStrategies()
         yield [AccessDecisionManager::STRATEGY_PRIORITY];
     }
 
+    public function testCacheableVoters()
+    {
+        $token = $this->createMock(TokenInterface::class);
+        $voter = $this->getMockBuilder(CacheableVoterInterface::class)->getMockForAbstractClass();
+        $voter
+            ->expects($this->once())
+            ->method('supportsAttribute')
+            ->with('foo')
+            ->willReturn(true);
+        $voter
+            ->expects($this->once())
+            ->method('supportsType')
+            ->with('string')
+            ->willReturn(true);
+        $voter
+            ->expects($this->once())
+            ->method('vote')
+            ->with($token, 'bar', ['foo'])
+            ->willReturn(VoterInterface::ACCESS_GRANTED);
+
+        $manager = new AccessDecisionManager([$voter]);
+        $this->assertTrue($manager->decide($token, ['foo'], 'bar'));
+    }
+
+    public function testCacheableVotersIgnoresNonStringAttributes()
+    {
+        $token = $this->createMock(TokenInterface::class);
+        $voter = $this->getMockBuilder(CacheableVoterInterface::class)->getMockForAbstractClass();
+        $voter
+            ->expects($this->never())
+            ->method('supportsAttribute');
+        $voter
+            ->expects($this->once())
+            ->method('supportsType')
+            ->with('string')
+            ->willReturn(true);
+        $voter
+            ->expects($this->once())
+            ->method('vote')
+            ->with($token, 'bar', [1337])
+            ->willReturn(VoterInterface::ACCESS_GRANTED);
+
+        $manager = new AccessDecisionManager([$voter]);
+        $this->assertTrue($manager->decide($token, [1337], 'bar'));
+    }
+
+    public function testCacheableVotersWithMultipleAttributes()
+    {
+        $token = $this->createMock(TokenInterface::class);
+        $voter = $this->getMockBuilder(CacheableVoterInterface::class)->getMockForAbstractClass();
+        $voter
+            ->expects($this->exactly(2))
+            ->method('supportsAttribute')
+            ->withConsecutive(['foo'], ['bar'])
+            ->willReturnOnConsecutiveCalls(false, true);
+        $voter
+            ->expects($this->once())
+            ->method('supportsType')
+            ->with('string')
+            ->willReturn(true);
+        $voter
+            ->expects($this->once())
+            ->method('vote')
+            ->with($token, 'bar', ['foo', 'bar'])
+            ->willReturn(VoterInterface::ACCESS_GRANTED);
+
+        $manager = new AccessDecisionManager([$voter]);
+        $this->assertTrue($manager->decide($token, ['foo', 'bar'], 'bar', true));
+    }
+
+    public function testCacheableVotersWithEmptyAttributes()
+    {
+        $token = $this->createMock(TokenInterface::class);
+        $voter = $this->getMockBuilder(CacheableVoterInterface::class)->getMockForAbstractClass();
+        $voter
+            ->expects($this->never())
+            ->method('supportsAttribute');
+        $voter
+            ->expects($this->once())
+            ->method('supportsType')
+            ->with('string')
+            ->willReturn(true);
+        $voter
+            ->expects($this->once())
+            ->method('vote')
+            ->with($token, 'bar', [])
+            ->willReturn(VoterInterface::ACCESS_GRANTED);
+
+        $manager = new AccessDecisionManager([$voter]);
+        $this->assertTrue($manager->decide($token, [], 'bar'));
+    }
+
+    public function testCacheableVotersSupportsMethodsCalledOnce()
+    {
+        $token = $this->createMock(TokenInterface::class);
+        $voter = $this->getMockBuilder(CacheableVoterInterface::class)->getMockForAbstractClass();
+        $voter
+            ->expects($this->once())
+            ->method('supportsAttribute')
+            ->with('foo')
+            ->willReturn(true);
+        $voter
+            ->expects($this->once())
+            ->method('supportsType')
+            ->with('string')
+            ->willReturn(true);
+        $voter
+            ->expects($this->exactly(2))
+            ->method('vote')
+            ->with($token, 'bar', ['foo'])
+            ->willReturn(VoterInterface::ACCESS_GRANTED);
+
+        $manager = new AccessDecisionManager([$voter]);
+        $this->assertTrue($manager->decide($token, ['foo'], 'bar'));
+        $this->assertTrue($manager->decide($token, ['foo'], 'bar'));
+    }
+
+    public function testCacheableVotersNotCalled()
+    {
+        $token = $this->createMock(TokenInterface::class);
+        $voter = $this->getMockBuilder(CacheableVoterInterface::class)->getMockForAbstractClass();
+        $voter
+            ->expects($this->once())
+            ->method('supportsAttribute')
+            ->with('foo')
+            ->willReturn(false);
+        $voter
+            ->expects($this->never())
+            ->method('supportsType');
+        $voter
+            ->expects($this->never())
+            ->method('vote');
+
+        $manager = new AccessDecisionManager([$voter]);
+        $this->assertFalse($manager->decide($token, ['foo'], 'bar'));
+    }
+
     protected function getVoters($grants, $denies, $abstains)
     {
         $voters = [];
diff --git a/Tests/Authorization/Voter/AuthenticatedVoterTest.php b/Tests/Authorization/Voter/AuthenticatedVoterTest.php
diff --git a/Tests/Authorization/Voter/RoleVoterTest.php b/Tests/Authorization/Voter/RoleVoterTest.php
diff --git a/Tests/Authorization/Voter/TraceableVoterTest.php b/Tests/Authorization/Voter/TraceableVoterTest.php

Original file line number	Diff line number	Diff line change
`@@ -18,7 +18,7 @@`
`18`	`18`	`*`
`19`	`19`	`* @author Fabien Potencier <fabien@symfony.com>`
`20`	`20`	`*/`
`21`		`-class RoleVoter implements VoterInterface`
	`21`	`+class RoleVoter implements CacheableVoterInterface`
`22`	`22`	`{`
`23`	`23`	`private $prefix;`
`24`	`24`
`@@ -55,6 +55,16 @@ public function vote(TokenInterface $token, $subject, array $attributes)`
`55`	`55`	`return $result;`
`56`	`56`	`}`
`57`	`57`
	`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`	`+`
`58`	`68`	`protected function extractRoles(TokenInterface $token)`
`59`	`69`	`{`
`60`	`70`	`return $token->getRoleNames();`
Original file line number	Diff line number	Diff line change
`@@ -22,7 +22,7 @@`
`22`	`22`	`*`
`23`	`23`	`* @internal`
`24`	`24`	`*/`
`25`		`-class TraceableVoter implements VoterInterface`
	`25`	`+class TraceableVoter implements CacheableVoterInterface`
`26`	`26`	`{`
`27`	`27`	`private $voter;`
`28`	`28`	`private $eventDispatcher;`
`@@ -46,4 +46,14 @@ public function getDecoratedVoter(): VoterInterface`
`46`	`46`	`{`
`47`	`47`	`return $this->voter;`
`48`	`48`	`}`
	`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`	`+ }`
`49`	`59`	`}`