-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
Custom AccessDecisionManager not getting list of voters as first argument in __construct #42095
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Comments
Can you create a small example application that allows to reproduce your issue? |
I simply copied the default AccessDecisionManager.php into App\Security directory (namespace) then renamed it and tried to use it. |
What arguments would you expect to be passed to your custom ADM? |
Here you can find the regular arguments that standard ADM receives upon object construction:
Also, you can see that I have two custom voters ( |
By default Symfony sets it's AccesDecisionManager class as a service definition inside the container with an id However when you set access_decision_manager:
service: <service_id> then SecurityExtension removes previously set This worked for the guy in the referenced issue becaused he registered his manager with the same id (i.e. As I said there: "Right now, when you have custom access decision manager you will have to manage all the dependencies by yourself." |
Thank you guys for your replies and helps, I figured it out. WorkaroundWhen Symfony compiles the code, it (somehow) has its own service definitions, so it doesn't get confused. I traced the code into the compiled cache and I found that it can't determine the service definitions of my ADM. So I changed my
Inside
The traces showed that Symfony collects all voters correctly up to this point, so it is not needed to manage dependencies manually. By using this service.yml, I didn't need to define the After finding the problem, I tried to use ConclusionI know AccessDecisionManager is somehow a deep Symfony concept which is rarely used for common projects, but:
Probably this part of Symfony needs to be reviewed. |
It is indeed pretty uncommon to provide a custom implementation of the ADM and I must admit I fail to see the use-case of doing so. The problem here is that if you build your own ADM, it might have an entirely different constructor. Symfony can't possibly know how to wire the list of voters into your custom ADM. In fact, Symfony cannot even make the general assumption that your ADM implementation uses voters at all. If you build your own ADM, you're basically also responsible for collection the voters that it should use. May I ask why you want to replace the core ADM? |
Thanks Alex, you are right, but this is the design matter, I think Symfony should assume that developer might need minor expansions otherwise the design should allow more customization interfaces; to allow collection of further or different parameters and inject them into the new expansions. As a developer, I think Symfony doesn't follow its design patterns which we are accustomed to. However, it seems Symfony follows its pattern half way, it collects voters and prepares parameters as if it were dealing with its core ADM, at some point it doesn't use collected data. I believe it should be like:
The reason why I want to replace core ADM is that I have more than 10 objects that I need two voters to unanimously vote for them, none of predefined strategies worked for me, when I change voters strategy to unanimous, the regular operation of security checking on Request object is affected. I need a mixed unanimous+affirmative decision management, when decision manager is consulted for my objects, I need to change strategy to unanimous otherwise affirmative strategy should be used. |
Okay, but wouldn't the better fix be to allow custom strategies? 🤔 |
Yea, that was exactly what I am doing, here is my ADM and the service configuration which worked for me: src/Security/SpecialAccessDecisionManager.php
service.yml
Note: I didn't add any extra configuration in security.yml |
You can easily inject voters to your custom ADM with You can read about tagged iterators here https://symfony.com/doc/current/service_container/tags.html#reference-tagged-services |
@marbulk , thanks for the tip, I didn't know about My point is that Symfony doesn't comply with the design pattern, I have seen other guys that they found their own methods to fix their problems with ADM as I did. As web applications are getting more complicated, core functionality of frameworks are expected to be more complying. That is why I raised this issue here. |
I tried to implement custom strategies: #42177 Can you evaluate this PR for me and tell me if that feature would allow you to use the default access decision manager again? |
… dedicated classes (derrabus) This PR was merged into the 5.4 branch. Discussion ---------- [Security][SecurityBundle] Implement ADM strategies as dedicated classes | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | yes | Deprecations? | yes | Tickets | Fix #42095 | License | MIT | Doc PR | TODO We currently allow to replace the access decision manager with a fully custom implementation. However, if the app makes use of that feature just because it needs a custom decision strategy, it also has to take care of wiring the voters into the custom implementation. This is a bit cumbersome. This PR introduces a new interface `AccessDecisionStrategyInterface` that allows the app to implement a custom strategy that can be plugged into the default access decision manager. All built-in strategies have been migrated to that new interface. Furthermore, a new option is added to SecurityBundle to leverage this feature: ```YAML security: access_decision_manager: strategy_service: app.custom_access_decision_strategy ``` The "old" way of configuring the strategy will continue to work: ```YAML security: access_decision_manager: strategy: unanimous allow_if_all_abstain: true allow_if_equal_granted_denied: false ``` In that case, SecurityBundle will wire the new strategy classes for us, so we don't have to change anything in our app when upgrading to Symfony 5.4. Additionally, I decided to finalize `AccessDecisionManager` because once the strategies are pluggable, there shouldn't be a use-case left for extending the class that could not be covered with a decorator. Implementing a custom strategy is straightforward. Your class implenting `AccessDecisionStrategyInterface` will receive a generator of voter results and is expected to return its decision as a boolean. This way, the interaction with the individual voters is abstracted away from the strategy and the strategy can stop the voter iteration whenever the decision is final. ```php /** * Always picks the third voter. */ class ThirdVoterStrategy implements AccessDecisionStrategyInterface { public function decide(\Traversable $results): bool { $votes = 0; foreach ($results as $result) { if (++$votes === 3) { return $result === VoterInterface::ACCESS_GRANTED; } } return false; } } ``` Commits ------- d25956a [Security] Implement ADM strategies as dedicated classes
With reference to the issue #41123
I encountered this issue with different symptoms!
Although I knew the service is automatically registered but I did this in service.yml:
then I added decision manager to security.yml:
As you can see below, arguments haven't been passed to the decision manager:
When I use default AccessDecisionManager voters are passed. I need to implement special authorization so I need to create my own decision manager. By the way, my decision manager implements AccessDecisionManagerInterface
I also checked
\SecurityBundle\DependencyInjection\Compiler\AddSecurityVotersPass->process()
, it is not being called when my decision manager is set.Any idea?
UPDATE
For testing, I temporarily renamed default AccessDecisionManager.php and placed my decision manager, it surprisingly worked. But it is not the solution. Still looking for the correct solution.
The text was updated successfully, but these errors were encountered: