8000 Simpler roles check in AuthorizationVoters · Issue #20748 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

Simpler roles check in AuthorizationVoters #20748

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.

Sign up for GitHub

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

Jump to bottom
Closed
thomasez opened this issue Dec 4, 2016 · 11 comments
Closed

Simpler roles check in AuthorizationVoters #20748

thomasez opened this issue Dec 4, 2016 · 11 comments
Labels
Bug Security Status: Needs Review Unconfirmed

Comments

@thomasez
Copy link
thomasez commented Dec 4, 2016

Using Symfony 3.2 with FosUserBundle and needing to check the roles in a voter I followed the instructions in the documentation. http://symfony.com/doc/master/security/voters.html#checking-for-roles-inside-a-voter

Warming up the cache in the dev environment workes and the voter seems to work aswell.

In "prod" on the other hand, I get this one:

[Symfony\Component\DependencyInjection\Exception\ServiceCircularReferenceException]
Circular reference detected for service "security.access.decision_manager",
path: "cache_warmer -> twig -> security.authorization_checker -> security.
access.decision_manager".

Checking roles from inside a voter could be a lot simpler with just checking them against the token instead of having to inject the decisionManager.

But this isn't that easy as long as we use a role hierarchy. Checking against each of the roles in the hierarchy is a hack I would like not to add.

Could it be possible to add access to the calculated ("expanded"?) hierarchy from the token? This could be done in the login process to avoid having to calculate each time.
@xabbuh
Copy link
Member
xabbuh commented Dec 4, 2016

Isn't the real issue we have to solve "simply" that we need to find out why in your case you get the (not expected) circular reference exception?

@xabbuh xabbuh added the Bug label Dec 4, 2016
@thomasez
Copy link
Author
thomasez commented Dec 4, 2016

I ran away from the circular reference issue by just playing with the token roles and then being annoyed by it not handing me all available roles for that login/user.

I guess this is two issues in one and the circular reference was more a digression in this issue, but it may as well be a bug.

I reintroduced the decisionManager to my voter (by pure cut&paste from the docs) and then it's back. I would guess the reason this does not bomb in the dev environment is that there it's the DebugAccessDecisionManager.

./bin/console --env=prod -v cache:clear

Exception trace:
()
at Development/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:271
Symfony\Component\DependencyInjection\Container->get() at Development/var/cache/pro_/appProdProjectContaine_.php:4316
appProdProjectContaine_->getSecurity_Access_DecisionManagerService() at Development/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:310
Symfony\Component\DependencyInjection\Container->get() at Development/var/cache/pro_/appProdProjectContaine_.php:4400
appProdProjectContaine_->getSecurity_AuthorizationCheckerService() at Development/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:310
Symfony\Component\DependencyInjection\Container->get() at Development/var/cache/pro_/appProdProjectContaine_.php:5681
appProdProjectContaine_->getTwigService() at Development/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:310
Symfony\Component\DependencyInjection\Container->get() at Development/var/cache/pro_/appProdProjectContaine_.php:900
appProdProjectContaine_->getCacheWarmerService() at Development/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php:310
Symfony\Component\DependencyInjection\Container->get() at Development/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php:496
Symfony\Component\HttpKernel\Kernel->initializeContainer() at Development/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php:116
Symfony\Component\HttpKernel\Kernel->boot() at Development/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Command/CacheClearCommand.php:134
Symfony\Bundle\FrameworkBundle\Command\CacheClearCommand->warmup() at Development/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Command/CacheClearCommand.php:96
Symfony\Bundle\FrameworkBundle\Command\CacheClearCommand->execute() at Development/vendor/symfony/symfony/src/Symfony/Component/Console/Command/Command.php:255 Symfony\Component\Console\Command\Command->run() at Development/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:846
Symfony\Component\Console\Application->doRunCommand() at Development/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:191
Symfony\Component\Console\Application->doRun() at Development/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Console/Application.php:80
Symfony\Bundle\FrameworkBundle\Console\Application->doRun() at Development/vendor/symfony/symfony/src/Symfony/Component/Console/Application.php:122
Symfony\Component\Console\Application->run() at Development/bin/console:30

@linaori
Copy link
Contributor
linaori commented Dec 5, 2016

@xabbuh the whole issue here is that people need the access decision manager/authorization checker to see if a certain implicit role is present. This means that every isGranted('ROLE_*') check will trigger the (re)calculation of the roles in the hierarchy.

As roles are part of the authentication and not authorization (used to identify), I don't see why the role hierarchy should be calculated doing authorization instead of authentication. My suggestion would be to calculate this during login and store it in the token.

@xabbuh
Copy link
Member
xabbuh commented Dec 8, 2016

@iltar That could indeed be a good idea. But IMO that would be a new feature then and thus wouldn't help @thomasez here.

@thomasez
Copy link
Author
thomasez commented Dec 8, 2016

Well, it would fix what I was asking for in this issue :=) The bug was really a digression I should have dropped. (But it was the reason I felt the need of all roles from the token.)

Maybe it's better to split this into two, this one with the original subject, aka "All roles (on that user) in $token->getRoles()" and a new issue with "Circular reference detected for service "security.access.decision_manager","

@xabbuh
Copy link
Member
xabbuh commented Dec 8, 2016

@thomasez I agree with you. So for your second issue it would be wonderful if could come up with a fork of the Symfony Standard Edition containing the changes that are necessary to reproduce your issue.

@xabbuh
Copy link
Member
xabbuh commented Dec 23, 2016

ping @thomasez Any news here? :)

@linaori linaori mentioned this issue Dec 23, 2016
Closed
@thomasez
Copy link
Author

Started on it, got slabbed with alot of work. I'll see what I can do.

thomasez added a commit to thomasez/symfony-standard that referenced this issue Jan 15, 2017
@thomasez
What I needed to end up with the error reported in symfony/symfony#20748
6310082
@thomasez
Copy link
Author

There it is.

composer update && ./bin/console --env=prod -v cache:clear

Should be enough. No need to create a database.

@thomasez
Copy link
Author

Hmm, I removed jms/security-extra-bundle and the issue goes away.

I presume this can be dropped as a symfony bug and rather report this on the specific bundle.

@javiereguiluz
Copy link
Member

Closing it as per @thomasez comments. @iltar if you think we should do what you proposed in #20748 (comment) please create a new issue. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Security Status: Needs Review Unconfirmed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@javiereguiluz @thomasez @linaori @xabbuh @carsonbot
0