[Security] Make it possible to give voters a weight in consensus decisions #16843
Conversation
…sions After watching the "Dig in Security with Symfony" talk at SymfonyCon today, I realized that there was no weighting mechanism in the current implementation. It is not always true that each voter has the equal weight within a majority vote. This feature PR accounts for that in a BC way. | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | None | License | MIT | Doc PR | None Commit recap from rebase: - Introduced the `WeightedVoterInterface` that extends the `VoterInterface` - Added the abstract `WeightedVoter` as an easily usable base class - Added the `Weight` decorator to extend existing Voters with weighting - Refactored test structures for `Voter` - Added missing methods in `Weight` decorator required by `VoterInterface` - Added test case for `Weight` decorator - Added test case for abstract `WeightedVoter` - Added feature tests to `AccessDecisionManagerTest` - Implemented feature in `AccessDecisionManager` - Fixed CodingStandards - Removed deprecated methods from `Weight` decorator - Removed abstract method from `WeightedVoter`, it's provided by the interface
|
I am not sure if I like this to be in the core. Imho we lose a lot of clarity about the voting process compared to the previous way which might lead to much harder to debug issues especially when voters from third-party bundles you use make use of this feature too (it's not so clear anymore then why access was denied/granted). |
| * | ||
| * @author Thomas Ploch <profiploch@gmail.com> | ||
| */ | ||
| abstract class WeightedVoter extends Voter implements WeightedVoterInterface |
There was a problem hiding this comment.
this class does not make sense. It does not bring any benefit compared to extending Voter and implementing the interface directly
|
I don't think we need this feature in the core actually. So my vote is a -1 |
|
@xabbuh I understand your point, but to add this feature yourself you'd have to copy the So another proposed solution could be refactoring the This can be done in a complete BC way without effecting current userland code. |
|
What's that actual, real life, use case? Did anyone already need this?
|
|
I'm 👎 except if it's backed with a real-world use case. |
|
To be honest, I was driven by the real world where "weighted consensus" is a very common concept and I just realized that this concept was missing from the current implementation. I cannot back it up with a real use case and I am actually totally OK with you guys rejecting that feature as is. I think though that what I said in my comment #16843 (comment) still holds true. The |
|
@stof Apparently weighting is a thing in userland though: |
|
I would rather make the access decision strategy configurable than introducing weighted voters in the core. |
…ss decision manager service (xabbuh) This PR was merged into the 3.4 branch. Discussion ---------- [Security] make it possible to configure a custom access decision manager service | Q | A | | --- | --- | | Branch? | 3.4 | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | #942, #14049, #15295, #16828, #16843, | | License | MIT | | Doc PR | TODO | These changes will make it possible to let users define their own voting strategies without the need for custom compiler passes that replace the built-in `AccessDecisionManager` (see linked issues in the PR table for some use cases). Commits ------- e0913a2 add option to define the access decision manager
After watching the "Dig in Security with Symfony" talk at SymfonyCon today, I
realized that there was no weighting mechanism in the current implementation.
It is not always true that each voter has the equal weight within a majority
vote. This feature PR accounts for that in a BC way.
Commit recap from rebase:
WeightedVoterInterfacethat extends theVoterInterfaceWeightedVoteras an easily usable base classWeightdecorator to extend existing Voters with weightingVoterWeightdecorator required byVoterInterfaceWeightdecoratorWeightedVoterAccessDecisionManagerTestAccessDecisionManagerWeightdecoratorWeightedVoter, it's provided by the interfaceSee #16828 for more discussion on this.