Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] Add strategy resolvers#21178
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| return; | ||
| } | ||
| $strategyResolvers =new \SplPriorityQueue(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
As a new feature you're PR should be based on master, you will then be able to use the trait, instead of relying on this as it does not preserve the original order in which the services have been registered (ref#20995).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh sorry I didn't know new feature have to be on master, I only did patch PR until now
I rebased my branch and realized there is still some work that needs to be done because of theTraceableAccessDecisionManager (the fact that the strategy is not unique now).
Unrelated but about your comment on thePriorityTaggedServiceTrait trait : I actually almost copy / pasted theAddSecurityVotersPass when I did mine. I just checked and this one is not using the trait... Should it be done in a separate PR as there might be problems of order too ?
linaori commentedJan 6, 2017
Can you show how this would be used? |
…ecurity web profiler
58e2a60 to50155a1Comparefancyweb commentedJan 6, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@iltar do you mean a real case example of why someone could need this feature, or a code example of how to use it ? I succesfully rebased this on master. I will wait a few days for more opinions before continuing working on this. There are still tests to write, and possible improvements in the web profiler. |
| $strategy ='unknown'; | ||
| if ($this->managerinstanceof AccessDecisionManager) { | ||
| $strategy =$this->manager->getStrategy($token,$attributes,$object); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't like the fact that there is a double call to thegetStrategy method (the other in thedecide call), but I don't see how else I can get the strategy (unless the last strategy used get stored in theAccessDecisionManager)
xabbuh commentedJan 6, 2017
@fancyweb In#19034, one of the first attempts included the extraction of the access decision strategies into a dedicated interface (called |
linaori commentedJan 6, 2017
@fancyweb yeah, some user-land examples would help illustrate what the feature does |
fancyweb commentedJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@xabbuh I found your PR when I searched how to change the strategy, but if I understand it correctly, even if strategies are extracted from the After more analysis, I think this feature is only useful when you are using reusable bundles, otherwise you can always make it work the way you want in your voters, since you've got the control on all of them. @iltar Here is one example that involves a reusable bundle.
<?phpnamespaceCompany\YourBundle\Security;useSymfony\Component\Security\Core\Authentication\Token\TokenInterface;useSymfony\Component\Security\Core\Authorization\Voter\Voter;useCompany\YourBundle\Model\ThreadInterface;class ThreadVoterextends Voter{/** * @var string */constPOST_MESSAGE ='your_bundle_post_message';/** * @param string $attribute * @param mixed $subject * * @return bool */protectedfunctionsupports($attribute,$subject) {returnself::POST_MESSAGE ===$attribute &&$subjectinstanceof ThreadInterface; }/** * @param string $attribute * @param mixed $subject * @param TokenInterface $token * * @return bool */protectedfunctionvoteOnAttribute($attribute,$subject,TokenInterface$token) {if (null ===$token->getUser()) {returnfalse; }/* @var $subject ThreadInterface */switch ($attribute) {caseself::POST_MESSAGE:return !$subject->isLocked(); }thrownew \LogicException(); }} <?phpnamespaceCompany\YourBundle\Security;useSymfony\Component\Security\Core\Authentication\Token\TokenInterface;useSymfony\Component\Security\Core\Authorization\Voter\Voter;useCompany\YourBundle\Model\MessageInterface;class MessageVoterextends Voter{/** * @var string */constDELETE ='your_bundle_delete_message';/** * @param string $attribute * @param mixed $subject * * @return bool */protectedfunctionsupports($attribute,$subject) {returnself::DELETE ===$attribute &&$subjectinstanceof MessageInterface; }/** * @param string $attribute * @param mixed $subject * @param TokenInterface $token * * @return bool */protectedfunctionvoteOnAttribute($attribute,$subject,TokenInterface$token) {if (null ===$token->getUser()) {returnfalse; }/* @var $subject MessageInterface */switch ($attribute) {caseself::DELETE:return$subject->getCreatedBy() ===$token->getUser(); }thrownew \LogicException(); }} This is the default "common" (rules that will never change) behavior of the bundle :
Thanks to the voter system, each application using the bundle can add its own rules, on top of the default ones. <?phpnamespaceAppBundle\Security;useSymfony\Component\Security\Core\Authentication\Token\TokenInterface;useSymfony\Component\Security\Core\Authorization\Voter\Voter;useCompany\YourBundle\Model\ThreadInterface;useCompany\YourBundle\Security\ThreadVoterasYourBundleThreadVoter;useAppBundle\Entity\User;class ThreadVoterextends Voter{/** * @param string $attribute * @param mixed $subject * * @return bool */protectedfunctionsupports($attribute,$subject) {return YourBundleThreadVoter::POST_MESSAGE ===$attribute &&$subjectinstanceof ThreadInterface; }/** * @param string $attribute * @param mixed $subject * @param TokenInterface $token * * @return bool */protectedfunctionvoteOnAttribute($attribute,$subject,TokenInterface$token) {$user =$token->getUser();if (!$userinstanceof User) {returnfalse; }switch ($attribute) {case YourBundleThreadVoter::POST_MESSAGE:return$user->isEmailVerified(); }thrownew \LogicException(); }} <?phpnamespaceAppBundle\Security;useSymfony\Component\Security\Core\Authentication\Token\TokenInterface;useSymfony\Component\Security\Core\Authorization\Voter\Voter;useCompany\YourBundle\Model\MessageInterface;useCompany\YourBundle\Security\MessageVoterasYourBundleMessageVoter;useAppBundle\Entity\User;class MessageVoterextends Voter{/** * @param string $attribute * @param mixed $subject * * @return bool */protectedfunctionsupports($attribute,$subject) {return YourBundleMessageVoter::DELETE ===$attribute &&$subjectinstanceof MessageInterface; }/** * @param string $attribute * @param mixed $subject * @param TokenInterface $token * * @return bool */protectedfunctionvoteOnAttribute($attribute,$subject,TokenInterface$token) {$user =$token->getUser();if (!$userinstanceof User) {returnfalse; }switch ($attribute) {case YourBundleMessageVoter::DELETE:returnin_array('ROLE_ADMIN',$user->getRoles()); }thrownew \LogicException(); }} This is the specific behavior you want in your application :
As you can see having a global strategy limits the possibility of defining specific behaviors in your application. I would understand if a majority find that these kind of cases are too uncommon to make a feature for it. With this feature, you would choose the most common strategy in your application as the default one, and everytime you need another one, you would have to make a <?phpnamespaceAppBundle\Security;useSymfony\Component\Security\Core\Authorizatin\StrategyResolverInterface;useCompany\YourBundle\Security\ThreadVoter;useSymfony\Component\Security\Core\Authorization\AccessDecisionManager;class ThreadStrategyResolverimplements StrategyResolverInterface{/** * @param TokenInterface $token * @param array $attributes * @param mixed $object * * @return string */publicfunctiongetStrategy(TokenInterface$token,array$attributes,$object =null) {// as we only have one case "supported" I directly return the strategy but basically// you can do what you want here.return AccessDecisionManager::STRATEGY_UNANIMOUS; }/** * @param TokenInterface $token * @param array $attributes * @param mixed $object * * @return bool */publicfunctionsupports(TokenInterface$token,array$attributes,$object =null) {returnin_array(ThreadVoter::POST_MESSAGE,$attributes) &&$objectinstanceof ThreadInterface; }} services:app.strategy_resolver.thread:class:AppBundle\Security\ThreadStrategyResolvertags: -{ name: security.strategy_resolver } |
linaori commentedJan 10, 2017
What about a single attribute per voter?
I think you should keep this kind of domain logic in 1 place, especially not in re-usable bundles. |
fancyweb commentedJan 10, 2017
I'm sorry I don't understand, where this one voter would be ? |
linaori commentedJan 10, 2017
I think you might be miss-using bundles:https://stovepipe.systems/post/what-are-bundles-in-symfony Long story short, bundles should provide infrastructure, not domain logic. Voters are the definition of domain logic.
The idea here is that you can have multiple voters for one attribute, but that your application decides how the voting goes (no exceptions). I'm not for or against the feature, I'm merely wondering if the requirements of this feature is justified or if there's another solution. |
fancyweb commentedJan 10, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
OK I really understand your point of view (I'm already a reader of your blog). In my case, the reusable bundles have domain logic because they are "private" bundles used only by the projects of my company, and we are making websites about one domain only. I guess a bundle doesn't have to be infrastructural to be a bundle, sometimes you want domain logic inside voluntarily. |
weaverryan commentedSep 27, 2017
So, could we close this? I'm also not convinced there is enough need here: you need to make your voters work together, they are definitely your business/domain logic. I'm 👎 |
fabpot commentedSep 27, 2017
I'm also not convinced about this change. Let's close. |
Uh oh!
There was an error while loading.Please reload this page.
This PR is a way to be able to use a specific strategy (in the AccessDecisionManager) for particular attributes / object / token. Cf#15130 for more discussion about this.
I know the idea in the issue was rejected but this implementation is different and time have passed since then, so maybe there will be a new look on this.