symfony / symfony

The Symfony PHP framework
https://symfony.com
MIT License
29.75k stars 9.46k forks source link

AccessDecisionManager can not be injected into Voter anymore #18554

Closed gharlan closed 8 years ago

gharlan commented 8 years ago

http://symfony.com/doc/master/cookbook/security/voters.html#checking-for-roles-inside-a-voter

This works with current release (3.0.4), but not with current master. I get this error:

[Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException]

Circular reference detected for service "debug.security.access.decision_manager", path: "debug.security.access.decision_manager".

Reproducible by this branch of symfony-standard: https://github.com/gharlan/symfony-standard/tree/voter-with-decision-manager

You can see the error in composer install (ScriptHandler::clearCache).

linaori commented 8 years ago

@gharlan can you verify if it does work without dev? I think the issue could be related to a service only loaded in dev. Possibly introduced in symfony/symfony#17887, can you verify this?

/cc @javiereguiluz

gharlan commented 8 years ago

can you verify if it does work without dev?

I can confirm this.

bin/console cache:clear --env=dev -> error bin/console cache:clear --env=prod -> no error

javiereguiluz commented 8 years ago

@iltar do you have any clue about how to fix this? Thanks!

linaori commented 8 years ago

@javiereguiluz I can't seem to find a fix for this just yet. I've encountered this issue myself for a completely other case (if I inject the ADM in my voter):

Circular reference detected for service "debug.security.access.decision_manager", path: "
   cache_warmer 
-> hostnet_webpack.bridge.cache_warmer 
-> hostnet_webpack.bridge.asset_compiler 
-> hostnet_webpack.bridge.asset_twig_parser 
-> twig 
-> security.authorization_checker 
-> debug.security.access.decision_manager 
-> app.security.voter.contract".

This one is created because of the SecurityExtension in the TwigBridge. If I lazy load the service by injecting the container here, it works fine and I get the above error.

I'm not entirely sure why that error is present, but this is in fact a circular reference. When creating the voter, it needs to inject the access decision manager and this one needs the voters. I'm curious how this can work in the first place.

Imo the access decision manager should not be directly accessible in another voter and the container should be injected instead. One option you could do is to wrap the container in an implementation using the designated ADM interface.

HeahDude commented 8 years ago

See #18566, tested with the fork.

ghost commented 8 years ago

Same issue here, hopefully to get a fast fix for this. I really like to use Symfony 3.1.* :)

javiereguiluz commented 8 years ago

@JHGitty could you please test the fix proposed by @HeahDude in #18566? Thanks!

ghost commented 8 years ago

@javiereguiluz The fix in #18566 works fine here. Please merge it! :+1:

hhamon commented 8 years ago

Is there any valid reason to have access to the ADM inside a voter? It does sound weird to me!

javiereguiluz commented 8 years ago

@hhamon checking for roles inside the voter. See: http://symfony.com/doc/current/cookbook/security/voters.html#checking-for-roles-inside-a-voter

hhamon commented 8 years ago

For this use case I just inject the security.role_hierarchy service to get all reachable roles and iterate over it. No need for the whole ADM into the voter.

javiereguiluz commented 8 years ago

@hhamon I still think that checking for votes is not done well in Symfony. I proposed some changes in #14048.