From 3eafdf2b4ca61e885bf4fce364a4e2fb1ea8eaf5 Mon Sep 17 00:00:00 2001 From: Thomas Calvet Date: Fri, 6 Jan 2017 14:26:59 +0100 Subject: [PATCH 1/3] [Security] Add strategy resolvers --- .../Compiler/AddStrategyResolversPass.php | 46 +++++++++++++++ .../DependencyInjection/SecurityExtension.php | 1 + .../Bundle/SecurityBundle/SecurityBundle.php | 2 + .../Authorization/AccessDecisionManager.php | 58 +++++++++++++++---- .../StrategyResolverInterface.php | 38 ++++++++++++ 5 files changed, 134 insertions(+), 11 deletions(-) create mode 100644 src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddStrategyResolversPass.php create mode 100644 src/Symfony/Component/Security/Core/Authorization/StrategyResolverInterface.php diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddStrategyResolversPass.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddStrategyResolversPass.php new file mode 100644 index 0000000000000..c2365c2c66b0a --- /dev/null +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddStrategyResolversPass.php @@ -0,0 +1,46 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler; + +use Symfony\Component\DependencyInjection\Reference; +use Symfony\Component\DependencyInjection\ContainerBuilder; +use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface; + +class AddStrategyResolversPass implements CompilerPassInterface +{ + /** + * {@inheritdoc} + */ + public function process(ContainerBuilder $container) + { + if (!$container->hasDefinition('security.access.decision_manager')) { + return; + } + + $strategyResolvers = new \SplPriorityQueue(); + foreach ($container->findTaggedServiceIds('security.strategy_resolver') as $id => $attributes) { + $class = $container->getDefinition($id)->getClass(); + $interface = 'Symfony\Component\Security\Core\Authorization\StrategyResolverInterface'; + if (!is_subclass_of($class, $interface)) { + throw new \InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, $interface)); + } + + $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0; + $strategyResolvers->insert(new Reference($id), $priority); + } + + $strategyResolvers = iterator_to_array($strategyResolvers); + ksort($strategyResolvers); + + $container->getDefinition('security.access.decision_manager')->replaceArgument(4, array_values($strategyResolvers)); + } +} diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php index 1bdd16d0d873b..6bb83a9bfbbdd 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php @@ -84,6 +84,7 @@ public function load(array $configs, ContainerBuilder $container) ->addArgument($config['access_decision_manager']['strategy']) ->addArgument($config['access_decision_manager']['allow_if_all_abstain']) ->addArgument($config['access_decision_manager']['allow_if_equal_granted_denied']) + ->addArgument(array()); ; $container->setParameter('security.access.always_authenticate_before_granting', $config['always_authenticate_before_granting']); $container->setParameter('security.authentication.hide_user_not_found', $config['hide_user_not_found']); diff --git a/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php b/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php index c50aab24e12e4..38f6411745756 100644 --- a/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php +++ b/src/Symfony/Bundle/SecurityBundle/SecurityBundle.php @@ -15,6 +15,7 @@ use Symfony\Component\HttpKernel\Bundle\Bundle; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\AddSecurityVotersPass; +use Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler\AddStrategyResolversPass; use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\FormLoginFactory; use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\FormLoginLdapFactory; use Symfony\Bundle\SecurityBundle\DependencyInjection\Security\Factory\HttpBasicFactory; @@ -57,5 +58,6 @@ public function build(ContainerBuilder $container) $extension->addUserProviderFactory(new InMemoryFactory()); $extension->addUserProviderFactory(new LdapFactory()); $container->addCompilerPass(new AddSecurityVotersPass()); + $container->addCompilerPass(new AddStrategyResolversPass()); } } diff --git a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php index e40d90664c736..096e52fc38866 100644 --- a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php +++ b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php @@ -27,29 +27,35 @@ class AccessDecisionManager implements AccessDecisionManagerInterface const STRATEGY_UNANIMOUS = 'unanimous'; private $voters; - private $strategy; + private $defaultStrategyMethod; private $allowIfAllAbstainDecisions; private $allowIfEqualGrantedDeniedDecisions; + /** + * @var array + */ + private $strategyResolvers; + /** * Constructor. * - * @param VoterInterface[] $voters An array of VoterInterface instances - * @param string $strategy The vote strategy - * @param bool $allowIfAllAbstainDecisions Whether to grant access if all voters abstained or not - * @param bool $allowIfEqualGrantedDeniedDecisions Whether to grant access if result are equals + * @param VoterInterface[] $voters An array of VoterInterface instances + * @param string $defaultStrategy The vote default strategy + * @param bool $allowIfAllAbstainDecisions Whether to grant access if all voters abstained or not + * @param bool $allowIfEqualGrantedDeniedDecisions Whether to grant access if result are equals + * @param StrategyResolverInterface[] $strategyResolvers An array of StrategyResolver instances * * @throws \InvalidArgumentException */ - public function __construct(array $voters = array(), $strategy = self::STRATEGY_AFFIRMATIVE, $allowIfAllAbstainDecisions = false, $allowIfEqualGrantedDeniedDecisions = true) + public function __construct(array $voters = array(), $defaultStrategy = self::STRATEGY_AFFIRMATIVE, $allowIfAllAbstainDecisions = false, $allowIfEqualGrantedDeniedDecisions = true, array $strategyResolvers = array()) { - $strategyMethod = 'decide'.ucfirst($strategy); - if (!is_callable(array($this, $strategyMethod))) { - throw new \InvalidArgumentException(sprintf('The strategy "%s" is not supported.', $strategy)); + $defaultStrategyMethod = $this->getStrategyMethod($defaultStrategy); + if (!is_callable(array($this, $defaultStrategyMethod))) { + throw new \InvalidArgumentException(sprintf('The strategy "%s" is not supported.', $defaultStrategy)); } $this->voters = $voters; - $this->strategy = $strategyMethod; + $this->defaultStrategyMethod = $defaultStrategyMethod; $this->allowIfAllAbstainDecisions = (bool) $allowIfAllAbstainDecisions; $this->allowIfEqualGrantedDeniedDecisions = (bool) $allowIfEqualGrantedDeniedDecisions; } @@ -69,7 +75,27 @@ public function setVoters(array $voters) */ public function decide(TokenInterface $token, array $attributes, $object = null) { - return $this->{$this->strategy}($token, $attributes, $object); + $strategyMethod = $this->defaultStrategyMethod; + /* @var $strategyResolver StrategyResolverInterface */ + foreach ($this->strategyResolvers as $strategyResolver) { + if ($strategyResolver->supports($token, $attributes, $object)) { + $resolvedStrategy = $strategyResolver->getStrategy($token, $attributes, $object); + if (!is_string($resolvedStrategy)) { + continue; + } + + $resolvedStrategyMethod = $this->getStrategyMethod($resolvedStrategy); + if (!is_callable(array($this, $resolvedStrategyMethod))) { + continue; + } + + $strategyMethod = $resolvedStrategyMethod; + + break; + } + } + + return $this->{$strategyMethod}($token, $attributes, $object); } /** @@ -188,4 +214,14 @@ private function decideUnanimous(TokenInterface $token, array $attributes, $obje return $this->allowIfAllAbstainDecisions; } + + /** + * @param string $strategy + * + * @return string + */ + private function getStrategyMethod($strategy) + { + return 'decide' . ucfirst($strategy); + } } diff --git a/src/Symfony/Component/Security/Core/Authorization/StrategyResolverInterface.php b/src/Symfony/Component/Security/Core/Authorization/StrategyResolverInterface.php new file mode 100644 index 0000000000000..c2de257eb399a --- /dev/null +++ b/src/Symfony/Component/Security/Core/Authorization/StrategyResolverInterface.php @@ -0,0 +1,38 @@ + + * + * For the full copyright and license information, please view the LICENSE + * file that was distributed with this source code. + */ + +namespace Symfony\Component\Security\Core\Authorization; + +use Symfony\Component\Security\Core\Authentication\Token\TokenInterface; + +interface StrategyResolverInterface +{ + /** + * This method must return one of the following constants from the AccessDecisionManager : + * STRATEGY_AFFIRMATIVE, STRATEGY_CONSENSUS, or STRATEGY_UNANIMOUS. + * + * @param TokenInterface $token + * @param array $attributes + * @param mixed $object + * + * @return string + */ + public function getStrategy(TokenInterface $token, array $attributes, $object = null); + + /** + * @param TokenInterface $token + * @param array $attributes + * @param null $object + * + * @return bool + */ + public function supports(TokenInterface $token, array $attributes, $object = null); +} From 7c995fa14bf138032ba137fc7d869189652c5178 Mon Sep 17 00:00:00 2001 From: Thomas Calvet Date: Fri, 6 Jan 2017 14:43:38 +0100 Subject: [PATCH 2/3] phpdoc comment fix --- .../Security/Core/Authorization/StrategyResolverInterface.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Symfony/Component/Security/Core/Authorization/StrategyResolverInterface.php b/src/Symfony/Component/Security/Core/Authorization/StrategyResolverInterface.php index c2de257eb399a..a6ca43a35e0a8 100644 --- a/src/Symfony/Component/Security/Core/Authorization/StrategyResolverInterface.php +++ b/src/Symfony/Component/Security/Core/Authorization/StrategyResolverInterface.php @@ -30,7 +30,7 @@ public function getStrategy(TokenInterface $token, array $attributes, $object = /** * @param TokenInterface $token * @param array $attributes - * @param null $object + * @param mixed $object * * @return bool */ From 50155a1f1cc535cd13eda3036db0034a09fdd9c1 Mon Sep 17 00:00:00 2001 From: Thomas Calvet Date: Fri, 6 Jan 2017 16:42:14 +0100 Subject: [PATCH 3/3] rebase on master / adapt the TraceableAccessDecisionManager and the security web profiler --- .../DataCollector/SecurityDataCollector.php | 10 ++-- .../Compiler/AddStrategyResolversPass.php | 25 ++++----- .../views/Collector/security.html.twig | 9 ++-- .../Authorization/AccessDecisionManager.php | 53 +++++++++++-------- .../TraceableAccessDecisionManager.php | 21 ++++---- 5 files changed, 64 insertions(+), 54 deletions(-) diff --git a/src/Symfony/Bundle/SecurityBundle/DataCollector/SecurityDataCollector.php b/src/Symfony/Bundle/SecurityBundle/DataCollector/SecurityDataCollector.php index f0fa6790c3489..abe72a8ebd642 100644 --- a/src/Symfony/Bundle/SecurityBundle/DataCollector/SecurityDataCollector.php +++ b/src/Symfony/Bundle/SecurityBundle/DataCollector/SecurityDataCollector.php @@ -127,14 +127,14 @@ public function collect(Request $request, Response $response, \Exception $except return $decision; }, $this->accessDecisionManager->getDecisionLog()); - $this->data['voter_strategy'] = $this->accessDecisionManager->getStrategy(); + $this->data['voter_default_strategy'] = $this->accessDecisionManager->getDefaultStrategy(); foreach ($this->accessDecisionManager->getVoters() as $voter) { $this->data['voters'][] = get_class($voter); } } else { $this->data['access_decision_log'] = array(); - $this->data['voter_strategy'] = 'unknown'; + $this->data['voter_default_strategy'] = 'unknown'; $this->data['voters'] = array(); } @@ -263,13 +263,13 @@ public function getVoters() } /** - * Returns the strategy configured for the security voters. + * Returns the default strategy configured for the security voters. * * @return string */ - public function getVoterStrategy() + public function getVoterDefaultStrategy() { - return $this->data['voter_strategy']; + return $this->data['voter_default_strategy']; } /** diff --git a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddStrategyResolversPass.php b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddStrategyResolversPass.php index c2365c2c66b0a..a5019fd4cf9cb 100644 --- a/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddStrategyResolversPass.php +++ b/src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddStrategyResolversPass.php @@ -11,12 +11,15 @@ namespace Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler; -use Symfony\Component\DependencyInjection\Reference; +use Symfony\Component\DependencyInjection\Compiler\PriorityTaggedServiceTrait; use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface; +use Symfony\Component\Security\Core\Authorization\StrategyResolverInterface; class AddStrategyResolversPass implements CompilerPassInterface { + use PriorityTaggedServiceTrait; + /** * {@inheritdoc} */ @@ -26,21 +29,15 @@ public function process(ContainerBuilder $container) return; } - $strategyResolvers = new \SplPriorityQueue(); - foreach ($container->findTaggedServiceIds('security.strategy_resolver') as $id => $attributes) { - $class = $container->getDefinition($id)->getClass(); - $interface = 'Symfony\Component\Security\Core\Authorization\StrategyResolverInterface'; - if (!is_subclass_of($class, $interface)) { - throw new \InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $id, $interface)); + $strategyResolverReferences = $this->findAndSortTaggedServices('security.strategy_resolver', $container); + foreach ($strategyResolverReferences as $strategyResolverReference) { + $strategyResolverServiceId = $strategyResolverReference->__toString(); + $class = $container->getDefinition($strategyResolverServiceId)->getClass(); + if (!is_subclass_of($class, StrategyResolverInterface::class)) { + throw new \InvalidArgumentException(sprintf('Service "%s" must implement interface "%s".', $strategyResolverServiceId, StrategyResolverInterface::class)); } - - $priority = isset($attributes[0]['priority']) ? $attributes[0]['priority'] : 0; - $strategyResolvers->insert(new Reference($id), $priority); } - $strategyResolvers = iterator_to_array($strategyResolvers); - ksort($strategyResolvers); - - $container->getDefinition('security.access.decision_manager')->replaceArgument(4, array_values($strategyResolvers)); + $container->getDefinition('security.access.decision_manager')->replaceArgument(4, $strategyResolverReferences); } } diff --git a/src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig b/src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig index 3830e924a190b..f0f44c698d5be 100644 --- a/src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig +++ b/src/Symfony/Bundle/SecurityBundle/Resources/views/Collector/security.html.twig @@ -205,8 +205,8 @@
- {{ collector.voterStrategy|default('unknown') }} - Strategy + {{ collector.voterDefaultStrategy|default('unknown') }} + Default strategy
@@ -235,13 +235,15 @@ + - + + @@ -257,6 +259,7 @@ : 'DENIED' }} + diff --git a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php index 096e52fc38866..2840689da7a2d 100644 --- a/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php +++ b/src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php @@ -27,7 +27,7 @@ class AccessDecisionManager implements AccessDecisionManagerInterface const STRATEGY_UNANIMOUS = 'unanimous'; private $voters; - private $defaultStrategyMethod; + private $defaultStrategy; private $allowIfAllAbstainDecisions; private $allowIfEqualGrantedDeniedDecisions; @@ -49,15 +49,19 @@ class AccessDecisionManager implements AccessDecisionManagerInterface */ public function __construct(array $voters = array(), $defaultStrategy = self::STRATEGY_AFFIRMATIVE, $allowIfAllAbstainDecisions = false, $allowIfEqualGrantedDeniedDecisions = true, array $strategyResolvers = array()) { - $defaultStrategyMethod = $this->getStrategyMethod($defaultStrategy); - if (!is_callable(array($this, $defaultStrategyMethod))) { + if (!in_array($defaultStrategy, array( + self::STRATEGY_AFFIRMATIVE, + self::STRATEGY_CONSENSUS, + self::STRATEGY_UNANIMOUS + ))) { throw new \InvalidArgumentException(sprintf('The strategy "%s" is not supported.', $defaultStrategy)); } $this->voters = $voters; - $this->defaultStrategyMethod = $defaultStrategyMethod; + $this->defaultStrategy = $defaultStrategy; $this->allowIfAllAbstainDecisions = (bool) $allowIfAllAbstainDecisions; $this->allowIfEqualGrantedDeniedDecisions = (bool) $allowIfEqualGrantedDeniedDecisions; + $this->strategyResolvers = $strategyResolvers; } /** @@ -75,27 +79,40 @@ public function setVoters(array $voters) */ public function decide(TokenInterface $token, array $attributes, $object = null) { - $strategyMethod = $this->defaultStrategyMethod; + $strategyMethod = 'decide' . ucfirst($this->getStrategy($token, $attributes, $object)); + + return $this->{$strategyMethod}($token, $attributes, $object); + } + + /** + * @param TokenInterface $token + * @param array $attributes + * @param mixed $object + * + * @return string + */ + public function getStrategy(TokenInterface $token, array $attributes, $object = null) + { + $strategy = $this->defaultStrategy; /* @var $strategyResolver StrategyResolverInterface */ foreach ($this->strategyResolvers as $strategyResolver) { if ($strategyResolver->supports($token, $attributes, $object)) { $resolvedStrategy = $strategyResolver->getStrategy($token, $attributes, $object); - if (!is_string($resolvedStrategy)) { - continue; - } - - $resolvedStrategyMethod = $this->getStrategyMethod($resolvedStrategy); - if (!is_callable(array($this, $resolvedStrategyMethod))) { + if (!in_array($resolvedStrategy, array( + self::STRATEGY_AFFIRMATIVE, + self::STRATEGY_CONSENSUS, + self::STRATEGY_UNANIMOUS + ))) { continue; } - $strategyMethod = $resolvedStrategyMethod; + $strategy = $resolvedStrategy; break; } } - return $this->{$strategyMethod}($token, $attributes, $object); + return $strategy; } /** @@ -214,14 +231,4 @@ private function decideUnanimous(TokenInterface $token, array $attributes, $obje return $this->allowIfAllAbstainDecisions; } - - /** - * @param string $strategy - * - * @return string - */ - private function getStrategyMethod($strategy) - { - return 'decide' . ucfirst($strategy); - } } diff --git a/src/Symfony/Component/Security/Core/Authorization/TraceableAccessDecisionManager.php b/src/Symfony/Component/Security/Core/Authorization/TraceableAccessDecisionManager.php index 6ba2cab1a12ba..cb985464a98d0 100644 --- a/src/Symfony/Component/Security/Core/Authorization/TraceableAccessDecisionManager.php +++ b/src/Symfony/Component/Security/Core/Authorization/TraceableAccessDecisionManager.php @@ -24,7 +24,7 @@ class TraceableAccessDecisionManager implements AccessDecisionManagerInterface { private $manager; - private $strategy; + private $defaultStrategy; private $voters = array(); private $decisionLog = array(); @@ -33,10 +33,10 @@ public function __construct(AccessDecisionManagerInterface $manager) $this->manager = $manager; if ($this->manager instanceof AccessDecisionManager) { - // The strategy is stored in a private property of the decorated service - $reflection = new \ReflectionProperty(AccessDecisionManager::class, 'strategy'); + // The default strategy is stored in a private property of the decorated service + $reflection = new \ReflectionProperty(AccessDecisionManager::class, 'defaultStrategy'); $reflection->setAccessible(true); - $this->strategy = $reflection->getValue($manager); + $this->defaultStrategy = $reflection->getValue($manager); } } @@ -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); + } + $this->decisionLog[] = array( 'attributes' => $attributes, 'object' => $object, 'result' => $result, + 'strategy' => $strategy, ); return $result; @@ -72,12 +78,9 @@ public function setVoters(array $voters) /** * @return string */ - public function getStrategy() + public function getDefaultStrategy() { - // The $strategy property is misleading because it stores the name of its - // method (e.g. 'decideAffirmative') instead of the original strategy name - // (e.g. 'affirmative') - return null === $this->strategy ? '-' : strtolower(substr($this->strategy, 6)); + return null === $this->defaultStrategy ? 'unknown' : $this->defaultStrategy; } /**
# ResultStrategy Attributes Object
{{ decision.strategy }} {{ decision.attributes|length == 1 ? decision.attributes|first : profiler_dump(decision.attributes) }} {{ profiler_dump(decision.object) }}