[Security] Cache voters that will always abstain #43066

jderusse · 2021-09-16T19:00:03Z

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:

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:

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

ro0NL · 2021-09-16T19:08:25Z

Should we explore dedicated PHP8 class attributes?

IIUC eg. #[VotesOn(attributes, class)] enables compling a warm cache?

nicolas-grekas · 2021-09-17T08:31:07Z

#[VotesOn(attributes, class)] enables compling a warm cache

I don't know if we need an attribute for that, but what about a method that exposes what a voter cares about @jderusse, instead of the current approach?

stof · 2021-09-17T08:45:34Z

@jderusse what about voters that vote only on null subjects (for instance the RoleVoter) ?

ro0NL · 2021-09-17T09:17:09Z

@nicolas-grekas i generally like attributes enforcing static/cachable rules to us :) but runtime api also fits yes. As an end user taking care of such issues, an attribute looks elegant IMHO.

stof · 2021-09-17T09:25:01Z

An attribute might make the architecture of the AccessDecisionManager harder, if we want to avoid calling Reflection on each decide() call. A $voter instanceof CacheableAbstainVotesInterface check is much more efficient than reading attributes at runtime. Having to implement a caching layer for the metadata of the caching layer seems weird to me.

ro0NL · 2021-09-17T09:30:33Z

Understood. I meant levaraging DI at compile time.

Writing another set of support method implementations already bugs/confuses me somewhat. But it'll be the edge case... WFM.

jderusse · 2021-09-17T09:31:07Z

what about a method that exposes what a voter cares about instead of the current approach?

Yeah, I hesitated with this approach: but it can not work for voters that use a pattern for attributes: ie RoleVoter => ROLES_whatever (Which one of the much-used Voters)

@jderusse what about voters that vote only on null subjects (for instance the RoleVoter) ?

This will work: the current approach is to let voters reporting "I will always abstain for this attribute" and/or "I will always abstain for this object", everything else will always be called.

If voters don't care about subjects (like RoleVoter) they can return false telling I will **not** always abstain... which means call me, in all case.

The code will still be optimized because the voter will not be called when the attribute does not start with ROLE_

note: to be accurate RoleVoter does not vote "only on null subject", they vote on "whatever the content of the subject".

nicolas-grekas

LGTM, except the naming :)

What about CacheableVoterInterface with supportsAttribute() and supportsType() methods?

src/Symfony/Component/Security/Core/Authorization/Voter/RoleVoter.php

src/Symfony/Component/Security/Core/Authorization/Voter/CacheableAbstainVotesInterface.php

jderusse · 2021-10-20T21:51:50Z

Cmments addressed, and tests added

nicolas-grekas

2 comments + fabbot and good to me!

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

src/Symfony/Component/Security/Core/Authorization/Voter/RoleVoter.php

src/Symfony/Component/Security/Core/Authorization/Voter/CacheableVoterInterface.php

nicolas-grekas

found some typos, but still 👍 on my side
/cc @symfony/mergers

src/Symfony/Component/Security/Core/Authorization/Voter/CacheableVoterInterface.php

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

lyrixx

One minor comment, otherwise 👍🏼 thanks !

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

fabpot · 2021-10-28T10:37:26Z

Thank you @jderusse.

dkarlovi · 2021-11-04T09:06:49Z

@jderusse @nicolas-grekas

With the renaming of the interface and methods, the semantics seem reversed when you first look at it.

Since the interface is now called CacheableVoterInterface, the methods support*() returning false to enable feel inverted. My expectation would be that my voter becomes cacheable if it returns true (opts in), just like the serializer equivalent does.

stof · 2021-11-04T09:29:06Z

support* is not about whether the voter is cacheable, but whether it supports that permission (and so must be called for it)

dkarlovi · 2021-11-04T09:36:58Z

@stof in that case the interface is misnamed, IMO, the original names seem more appropriate.

nicolas-grekas · 2021-11-04T09:42:09Z

The original names were confusing to me. Feels like double negation: CacheableAbstainVotesInterface::willAlwaysAbstainOnAttribute raises a "wtf I am supposed to implement inside that?!?". Supports* method are common and straightforward.

I'm not against renaming CacheableVoterInterface if you have a better proposal. Do you?

dkarlovi · 2021-11-04T09:47:35Z

if you have a better proposal. Do you?

It depends who gets to define "better". As I said, the weird part here is returning false to opt-in to being cacheable.

jderusse · 2021-11-04T10:28:08Z

thing is, the feature is about improving performance when something IS NOT able to process the payload.

Having a negation will always be confusing.

One could use CacheableVoterInterface::doesNotSupportAttribute or UnCacheableVoterInterface::supportAttribute that will always be a cognitive load.

In my opinion support* is the way to go: It's not a negation, and consistent with what we do in many other place (including the AbstractVoter)

If we need to change something it should be the name of the interface. Would be happy to make a PR if somebody have a suggestion.

javiereguiluz · 2021-11-04T10:29:34Z

I'm on a phone so I can't explain on detail, but to me the current implementation is very intuitive because I understand it as follows:

Symfony: Hey voter, do you support ARTICLE_EDIT attribute?
Voter: no (returns FALSE to say that)
Symfony: OK. I will never call you again for this attribute then.

dkarlovi · 2021-11-04T10:39:53Z

This means the voter cacheability is a side-effect of this detection, it's inferred and not directly stated. Your votes is not becoming "cacheable", it's becoming "more capable of explaining what it knows how to work with".. which is the information then used to make it cacheable.

It basically has "extended / detailed supports" support.

wouterj · 2021-11-04T11:23:46Z

I must say that I quite liked the willAlwaysAbstainOnAttribute() and willAlwaysAbstainOnObjectType(): it's crystal clear what information is needed from the voter in order to judge whether we should cache it.
I.e. I must only return true if I always 100% of the cases abstain on this attribute. We loose this a bit with the current generic name of supportsAttribute() imho.

That said, this little nuance can be fulfilled easily by documentation.

jderusse requested review from chalasr and wouterj as code owners September 16, 2021 19:00

carsonbot added Status: Needs Review Feature labels Sep 16, 2021

jderusse force-pushed the voter-cache branch from 1f4e591 to 829fe63 Compare September 16, 2021 19:12

jderusse added the Security label Sep 16, 2021

carsonbot changed the title ~~Cache voters that will always abstain~~ [Security] Cache voters that will always abstain Sep 16, 2021

jderusse added this to the 5.4 milestone Sep 16, 2021

nicolas-grekas reviewed Oct 20, 2021

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/Voter/RoleVoter.php Outdated Show resolved Hide resolved

src/Symfony/Component/Security/Core/Authorization/Voter/RoleVoter.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Oct 20, 2021

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/Voter/CacheableAbstainVotesInterface.php Outdated Show resolved Hide resolved

jderusse force-pushed the voter-cache branch from 829fe63 to e896c7b Compare October 20, 2021 21:51

nicolas-grekas approved these changes Oct 20, 2021

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php Outdated Show resolved Hide resolved

src/Symfony/Component/Security/Core/Authorization/Voter/RoleVoter.php Outdated Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Oct 20, 2021

jderusse force-pushed the voter-cache branch 2 times, most recently from 2d557cc to 6508fbb Compare October 20, 2021 22:21

nicolas-grekas reviewed Oct 20, 2021

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/Voter/CacheableVoterInterface.php Outdated Show resolved Hide resolved

jderusse force-pushed the voter-cache branch 3 times, most recently from 33390fe to ef8f125 Compare October 21, 2021 06:48

nicolas-grekas approved these changes Oct 27, 2021

View reviewed changes

Cache voters that will always abstain

fc419ed

jderusse force-pushed the voter-cache branch from ef8f125 to fc419ed Compare October 27, 2021 16:23

lyrixx reviewed Oct 28, 2021

View reviewed changes

src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php Show resolved Hide resolved

chalasr approved these changes Oct 28, 2021

View reviewed changes

lyrixx approved these changes Oct 28, 2021

View reviewed changes

fabpot approved these changes Oct 28, 2021

View reviewed changes

fabpot mentioned this pull request Oct 28, 2021

[Security] Cache voters that will always abstain symfony/symfony-docs#16026

Closed

fabpot merged commit 4202f2a into symfony:5.4 Oct 28, 2021

derrabus mentioned this pull request Oct 29, 2021

[Security][SecurityBundle] Implement ADM strategies as dedicated classes #42177

Merged

jderusse deleted the voter-cache branch November 4, 2021 09:31

This was referenced Nov 5, 2021

Release v5.4.0-BETA1 #43937

Merged

Release v6.0.0-BETA1 #43939

Merged

Nyholm mentioned this pull request Mar 11, 2023

[Security] Make AccessDecisionManager much faster #49670

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Cache voters that will always abstain #43066

[Security] Cache voters that will always abstain #43066

jderusse commented Sep 16, 2021

ro0NL commented Sep 16, 2021 •

edited

Loading

nicolas-grekas commented Sep 17, 2021

stof commented Sep 17, 2021

ro0NL commented Sep 17, 2021 •

edited

Loading

stof commented Sep 17, 2021

ro0NL commented Sep 17, 2021 •

edited

Loading

jderusse commented Sep 17, 2021

nicolas-grekas left a comment

jderusse commented Oct 20, 2021

nicolas-grekas left a comment

nicolas-grekas left a comment

lyrixx left a comment

fabpot commented Oct 28, 2021

dkarlovi commented Nov 4, 2021

stof commented Nov 4, 2021

dkarlovi commented Nov 4, 2021

nicolas-grekas commented Nov 4, 2021

dkarlovi commented Nov 4, 2021

jderusse commented Nov 4, 2021

javiereguiluz commented Nov 4, 2021 •

edited by nicolas-grekas

Loading

dkarlovi commented Nov 4, 2021 •

edited

Loading

wouterj commented Nov 4, 2021

[Security] Cache voters that will always abstain #43066

[Security] Cache voters that will always abstain #43066

Conversation

jderusse commented Sep 16, 2021

ro0NL commented Sep 16, 2021 • edited Loading

nicolas-grekas commented Sep 17, 2021

stof commented Sep 17, 2021

ro0NL commented Sep 17, 2021 • edited Loading

stof commented Sep 17, 2021

ro0NL commented Sep 17, 2021 • edited Loading

jderusse commented Sep 17, 2021

nicolas-grekas left a comment

Choose a reason for hiding this comment

jderusse commented Oct 20, 2021

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

lyrixx left a comment

Choose a reason for hiding this comment

fabpot commented Oct 28, 2021

dkarlovi commented Nov 4, 2021

stof commented Nov 4, 2021

dkarlovi commented Nov 4, 2021

nicolas-grekas commented Nov 4, 2021

dkarlovi commented Nov 4, 2021

jderusse commented Nov 4, 2021

javiereguiluz commented Nov 4, 2021 • edited by nicolas-grekas Loading

dkarlovi commented Nov 4, 2021 • edited Loading

wouterj commented Nov 4, 2021

ro0NL commented Sep 16, 2021 •

edited

Loading

ro0NL commented Sep 17, 2021 •

edited

Loading

ro0NL commented Sep 17, 2021 •

edited

Loading

javiereguiluz commented Nov 4, 2021 •

edited by nicolas-grekas

Loading

dkarlovi commented Nov 4, 2021 •

edited

Loading