[Security] Add strategy resolvers #21178
Conversation
| return; | ||
| } | ||
|
|
||
| $strategyResolvers = new \SplPriorityQueue(); |
There was a problem hiding this comment.
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.
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 the TraceableAccessDecisionManager (the fact that the strategy is not unique now).
Unrelated but about your comment on the PriorityTaggedServiceTrait trait : I actually almost copy / pasted the AddSecurityVotersPass 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 ?
|
Can you show how this would be used? |
…ecurity web profiler
58e2a60 to
50155a1
Compare
|
@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. |
| @@ -47,10 +47,16 @@ public function decide(TokenInterface $token, array $attributes, $object = null) | |||
| { | |||
| $result = $this->manager->decide($token, $attributes, $object); | |||
|
|
|||
| $strategy = 'unknown'; | |||
| if ($this->manager instanceof AccessDecisionManager) { | |||
| $strategy = $this->manager->getStrategy($token, $attributes, $object); | |||
There was a problem hiding this comment.
I don't like the fact that there is a double call to the getStrategy method (the other in the decide call), but I don't see how else I can get the strategy (unless the last strategy used get stored in the AccessDecisionManager)
|
@fancyweb In #19034, one of the first attempts included the extraction of the access decision strategies into a dedicated interface (called |
|
@fancyweb yeah, some user-land examples would help illustrate what the feature does |
|
@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.
<?php
namespace Company\YourBundle\Security;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authorization\Voter\Voter;
use Company\YourBundle\Model\ThreadInterface;
class ThreadVoter extends Voter
{
/**
* @var string
*/
const POST_MESSAGE = 'your_bundle_post_message';
/**
* @param string $attribute
* @param mixed $subject
*
* @return bool
*/
protected function supports($attribute, $subject)
{
return self::POST_MESSAGE === $attribute && $subject instanceof ThreadInterface;
}
/**
* @param string $attribute
* @param mixed $subject
* @param TokenInterface $token
*
* @return bool
*/
protected function voteOnAttribute($attribute, $subject, TokenInterface $token)
{
if (null === $token->getUser()) {
return false;
}
/* @var $subject ThreadInterface */
switch ($attribute) {
case self::POST_MESSAGE:
return !$subject->isLocked();
}
throw new \LogicException();
}
}<?php
namespace Company\YourBundle\Security;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authorization\Voter\Voter;
use Company\YourBundle\Model\MessageInterface;
class MessageVoter extends Voter
{
/**
* @var string
*/
const DELETE = 'your_bundle_delete_message';
/**
* @param string $attribute
* @param mixed $subject
*
* @return bool
*/
protected function supports($attribute, $subject)
{
return self::DELETE === $attribute && $subject instanceof MessageInterface;
}
/**
* @param string $attribute
* @param mixed $subject
* @param TokenInterface $token
*
* @return bool
*/
protected function voteOnAttribute($attribute, $subject, TokenInterface $token)
{
if (null === $token->getUser()) {
return false;
}
/* @var $subject MessageInterface */
switch ($attribute) {
case self::DELETE:
return $subject->getCreatedBy() === $token->getUser();
}
throw new \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. <?php
namespace AppBundle\Security;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authorization\Voter\Voter;
use Company\YourBundle\Model\ThreadInterface;
use Company\YourBundle\Security\ThreadVoter as YourBundleThreadVoter;
use AppBundle\Entity\User;
class ThreadVoter extends Voter
{
/**
* @param string $attribute
* @param mixed $subject
*
* @return bool
*/
protected function supports($attribute, $subject)
{
return YourBundleThreadVoter::POST_MESSAGE === $attribute && $subject instanceof ThreadInterface;
}
/**
* @param string $attribute
* @param mixed $subject
* @param TokenInterface $token
*
* @return bool
*/
protected function voteOnAttribute($attribute, $subject, TokenInterface $token)
{
$user = $token->getUser();
if (!$user instanceof User) {
return false;
}
switch ($attribute) {
case YourBundleThreadVoter::POST_MESSAGE:
return $user->isEmailVerified();
}
throw new \LogicException();
}
}<?php
namespace AppBundle\Security;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
use Symfony\Component\Security\Core\Authorization\Voter\Voter;
use Company\YourBundle\Model\MessageInterface;
use Company\YourBundle\Security\MessageVoter as YourBundleMessageVoter;
use AppBundle\Entity\User;
class MessageVoter extends Voter
{
/**
* @param string $attribute
* @param mixed $subject
*
* @return bool
*/
protected function supports($attribute, $subject)
{
return YourBundleMessageVoter::DELETE === $attribute && $subject instanceof MessageInterface;
}
/**
* @param string $attribute
* @param mixed $subject
* @param TokenInterface $token
*
* @return bool
*/
protected function voteOnAttribute($attribute, $subject, TokenInterface $token)
{
$user = $token->getUser();
if (!$user instanceof User) {
return false;
}
switch ($attribute) {
case YourBundleMessageVoter::DELETE:
return in_array('ROLE_ADMIN', $user->getRoles());
}
throw new \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 <?php
namespace AppBundle\Security;
use Symfony\Component\Security\Core\Authorizatin\StrategyResolverInterface;
use Company\YourBundle\Security\ThreadVoter;
use Symfony\Component\Security\Core\Authorization\AccessDecisionManager;
class ThreadStrategyResolver implements StrategyResolverInterface
{
/**
* @param TokenInterface $token
* @param array $attributes
* @param mixed $object
*
* @return string
*/
public function getStrategy(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
*/
public function supports(TokenInterface $token, array $attributes, $object = null)
{
return in_array(ThreadVoter::POST_MESSAGE, $attributes) && $object instanceof ThreadInterface;
}
}services:
app.strategy_resolver.thread:
class: AppBundle\Security\ThreadStrategyResolver
tags:
- { name: security.strategy_resolver } |
|
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. |
|
I'm sorry I don't understand, where this one voter would be ? |
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. |
|
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. |
|
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 👎 |
|
I'm also not convinced about this change. Let's close. |
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.