[Security][SecurityBundle] Add messages and score on votes #58107

eltharin · 2024-08-27T17:38:55Z

Q	A
Branch?	7.3
Bug fix?	no
New feature?	yes
Deprecations?	no
Issues	Fix #27995, #26343, #35592, #43147
License	MIT

This PR continue the work started by @maidmaid and @noniagriconomie in #35592, continued by @yellow1912 in #43147 continued by @alamirault in #46493.

I rebase on 7.2 and remove deprecation as suggested by nicolas-grekas for not causing backward compatiblity breaks.

It allow to have informations about AccessDecisionManager and Voters (And understand why we have an AccessDeniedException with clear infos).

I add possibility to respond a voter with a score and create a Scoring strategy, looks like consensus but with ponderation.

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

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

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

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

src/Symfony/Component/Security/Core/Exception/AccessDeniedException.php

94noni · 2024-08-28T07:07:00Z

@eltharin hi 👋🏻

nice to see that you are willing to take over this amount of work, incremented overtime by different coders and reviewers :)

i am still interested in such feature even though now I rarely use voter, still thinking that this can be a great addition in core (like workflow transition blocker message is available)

to be honest, I think I would probable split this in 2 separates PRs, just to ease review and reduce the friction already existing around such feature, as it needs to arrive on a simple/BC/evolutive/maintainable way in core

add the feature of a voter message (main issue feature that seem to have lot of traction over the year regarding all PRs reactions)
add the feature of a new decision strategy alone (perhaps even before the voter message)

in any way, I will add a reminder to make a review on this PR as well :)

( PS: @noniagriconomie is one old account of mine no more used since )

eltharin · 2024-08-28T08:08:56Z

@94noni

you and other are the firsts to thank for the start of this work,

For the 2 PR's I thought about it but it's absolutlly linked, both needs Vote Object so getVote function so GetDecision functions,

Remove first or second feature but keep other one will only touch few files compared with all files changed.

At start, i saw your PR's without really needed, but when I wrote myine for scoring, I saw I made the same work as you, so I included your work in these to not have conflict when one will be accepted.

As it's a big update to add all these functions in symfony core I add a new benefit to this to have more chance to see this PR passed :)

kevinpapst · 2024-09-02T22:18:51Z

Did anyone ever run performance checks on these changes? Considering that Voters can be called hundreds of times per request, any overhead created by hundreds of new Vote() instances or checks for existing methods should imo be avoided (e.g. add new interfaces so I can opt-in to the new approach, instead of forcing the code upon everyone).

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

src/Symfony/Component/Security/Core/Authorization/Strategy/ScoringStrategy.php

94noni · 2024-09-09T07:27:46Z

src/Symfony/Component/Security/Core/Authorization/Strategy/ConsensusStrategy.php

@@ -40,29 +43,38 @@ public function __construct(

    public function decide(\Traversable $results): bool
    {
+        return $this->getDecision(new \ArrayIterator(array_map(fn ($vote) => new Vote($vote), iterator_to_array($results))))->isGranted();


perhaps create a trait to avoid copy/past such internal ?

instead of requiring an array of Vote, it might be more convenient to allow an array of Vote|int?
this would make the implementation trivial

Problem is for those who have custom classes, if someone made a custom strategy with decide function witch take only booleans, tomorrow we will pass Vote object and booleans IT will fail,
To avoid BC, we are obliged to have two functions,
Same for Voter with vote/getVote

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

src/Symfony/Component/Security/Core/Event/VoteEvent.php

eltharin · 2024-09-10T08:43:31Z

Did anyone ever run performance checks on these changes? Considering that Voters can be called hundreds of times per request, any overhead created by hundreds of new Vote() instances or checks for existing methods should imo be avoided (e.g. add new interfaces so I can opt-in to the new approach, instead of forcing the code upon everyone).

I look with Symfony Profiler, with 3000 voters With 7.2 actual version VoteListener take 120MiB in 8ms with this PR, 118MiB for 8.4ms.
I don't made more performance test with a performance tool

eltharin · 2024-09-30T08:45:53Z

friendly ping @chalasr as default reviewer

nicolas-grekas · 2025-02-12T11:41:31Z

I think I nailed it: instead of passing by reference, we can pass pre-constructed objects.
Here is a patch that implements what I mean. (see last commit there for my specific contribution)
Note that I didn't update tests and that I removed the message on the AccessDecision object as I see it of low value.

nicolas-grekas · 2025-02-12T12:00:59Z

I removed the message on the AccessDecision object as I see it of low value.

BTW, this makes me realize that the AccessDecision object is never used in the end. We pass it around, we collect votes in it, and we trash it. The profiler doesn't need that object. Maybe we can remove this part?

eltharin · 2025-02-12T16:32:21Z

AccessDecision Object is send to Profiler AND Exception to retreive why this reason with votes.
I kept "int" votes and objects votes in this object to see whitch voter respond in int and whitch respond in object.
And I keep in mind my ScoringStragy need (witch I'll put in PR later or in a personal bundle) but with your solution it can be impossible to do that.
That's what I want with a VoteInterface instead directly Vote that if some more crazier than me (yes it can be possible) want more attributes in a vote It can have it own VoteObject with Own strategy.
I think your last commit lost very much of that

nicolas-grekas · 2025-02-12T16:57:47Z

I'm sorry I don't understand your comment. What did my approach lose? The only thing I removed from a feature pov is adding a reason to the access decision itself.

eltharin · 2025-02-12T17:23:31Z

sorry I miss something in your code...
Don't see you put Vote Object in adm

eltharin · 2025-02-12T17:31:44Z

for allowMultipleAttributes argument, it's not from my PR, I just put it into comments in interface to rember it but It already been there.

eltharin · 2025-02-12T17:39:32Z

And in the strategy we don't have Vote Object ?
Can't we keep accessdecision in second argument of decide and get Vote Object by end($accessDecision->votes) ?

nicolas-grekas · 2025-02-12T21:37:57Z

Don't see you put Vote Object in adm

I don't get what this means sorry.

allowMultipleAttributes argument

this argument is an implementation detail, it shouldn't become an API. I moved it to the AccessDecision object but I suppose it can be kept as an argument, after the added $accessDecision. I'll see how this idea fits on by PR.

And in the strategy we don't have Vote Object ?
Can't we keep accessdecision in second argument of decide and get Vote Object by end($accessDecision->votes) ?

No, because it's not needed to achieve the purpose of this PR.
Why would we do this if we can make everything work without?
Said another way: what would this provide that'd not be possible with my approach?

eltharin · 2025-02-12T22:04:06Z

What I say is the argument allowMultipleAttributes already exist now, it's not me who create it.

No, because it's not needed to achieve the purpose of this PR.

purpose is to open Vote process, by creating a VoteInterface, it allow developer to integrate a custom configuration with its own properties to have a custom strategy using it.

I don't think to make all thoses changes "just" to print some message is very useful...

nicolas-grekas · 2025-02-13T10:12:02Z

I'm going to submit my patch as a separate PR, with only the "reason" feature. That's what people are asking for in all linked PRs/issues so many others do think it would be useful.

Then, I'll invite you to submit the scoring strategy as a separate and follow up PR. At the moment, I don't get what you want to achieve on this topic so having it split will help understand where we want to go, and why.

nicolas-grekas · 2025-02-13T16:28:51Z

Follow up PR at #59771
I'm closing here as I'm quite convinced my approach works and fully accounts for BC issues.
As mentioned in my previous comment, I'll let you submit the scoring strategy as a separate PR.
Thanks a lot for helping this move forward!

eltharin · 2025-02-14T08:24:03Z

I agree your approach works, I just said with a small patch (post below) we improve DX from "add a message in a vote" to make a full customizable Vote Process. I really think it's a shame to stop work for so little benefic

Now for your PR, I think it can be a BC when Dev create a custom AccesManager copying decide function (with existing $allowMultipleAttributes argument, they receive message :
App\Security\MyCustomAccessDecisionManager::decide(): Argument #4 ($allowMultipleAttributes) must be of type bool, Symfony\Component\Security\Core\Authorization\AccessDecision given

eltharin · 2025-02-14T08:24:25Z

patch.patch

nicolas-grekas · 2025-02-14T09:32:09Z

MyCustomAccessDecisionManager::decide(): Argument #4 ($allowMultipleAttributes)

Let's resolve this once for all: this argument is not part of any abstract API. It's only part of the concrete (Traceable)AccessDecisionManager, and if you extend it, BC is kept.

About your patch:

I realized the by-ref approach I suggested doesn't work with fun_get_args(), so it's not BC.
I removed the final modifier on AccessDecision and Vote to open things a bit for extensibility.
I've yet to see why a strategy would need to get the $accessDecision. The challenge is on you, in a follow up PR ;)

eltharin · 2025-02-14T10:43:43Z

OK.
please for my culture,
this line :
$vote = 3 < \func_num_args() ? func_get_arg(3) : new Vote();
is used to avoid create $vote argument that whould have caused BC ?

nicolas-grekas · 2025-02-14T10:47:28Z

Absolutely

…e (nicolas-grekas) This PR was merged into the 7.3 branch. Discussion ---------- [Security] Add ability for voters to explain their vote | Q | A | ------------- | --- | Branch? | 7.3 | Bug fix? | no | New feature? | no | Deprecations? | no | Issues | Fix #27995, #26343, #35592, #43147 | License | MIT This PR takes over #58107, which itself took over from #46493, etc. It takes the only approach I could think of that would preserve BC. The visible tip of what this achieves is: ![image](https://github.com/user-attachments/assets/67bd0678-868f-40ed-b2c6-3b1f10ffe101) ![image](https://github.com/user-attachments/assets/715814d8-e4de-47f0-aa0e-c412092389ff) Internally, this provides a new access audit log infrastructure that relies on new `AccessDecision` and `Vote` objects. Note that I didn't add (nor fix) tests for now. I'll borrow (and credit) from #58107 / #46493 as much as possible. Commits ------- c6eb9ae [Security] Add ability for voters to explain their vote

eltharin · 2025-04-17T07:38:23Z

* I've yet to see why a strategy would need to get the $accessDecision. The challenge is on you, in a follow up PR ;)

Here is : #60085

eltharin requested a review from chalasr as a code owner August 27, 2024 17:38

carsonbot added Status: Needs Review Feature labels Aug 27, 2024

carsonbot added this to the 7.2 milestone Aug 27, 2024

eltharin force-pushed the add_messages_and_score_on_votes branch from 5a4fbe1 to fc77e53 Compare August 27, 2024 17:49

OskarStark reviewed Aug 27, 2024

View reviewed changes

eltharin force-pushed the add_messages_and_score_on_votes branch 8 times, most recently from 6e67169 to 71d7744 Compare August 27, 2024 21:41

eltharin requested a review from OskarStark August 29, 2024 07:56

eltharin force-pushed the add_messages_and_score_on_votes branch from 46b15e0 to e9f63ce Compare August 30, 2024 08:51

eltharin force-pushed the add_messages_and_score_on_votes branch from e9f63ce to 278f259 Compare September 9, 2024 06:25