Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[Security] Make AccessDecisionStrategy extensible. And add highestnotabstainedvoter strategy #15295

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

Closed
wants to merge 7 commits into from
Closed

[Security] Make AccessDecisionStrategy extensible. And add highestnotabstainedvoter strategy #15295

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
38b8cf5
[Security] Fixed #14049
solilokiam Jul 12, 2015
66457b0
Make AccessDecisionManager work as a proxy class for other concrete a…
solilokiam Jul 16, 2015
5894731
Remove AccessDecisionStrategyInterface
solilokiam Jul 16, 2015
78c18ab
Fix wrong coding standards
solilokiam Jul 16, 2015
f78b3df
Make access decision manager configurable.
solilokiam Jul 17, 2015
ffb411b
Improve CustomAccessDecisionManagerCompilerPass
solilokiam Jul 27, 2015
5d5f4ef
Change strategy namespace and naming
solilokiam Jul 31, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
[Security] Fixed #14049 
HighestNotAbstainedVoterStrategy has been added. Also this commit makes access decision strategy extensible.
  • Loading branch information
@solilokiam
solilokiam committed Jul 31, 2015
commit 38b8cf5f64e07b46a507ce868957fb48f43e561c
36 changes: 36 additions & 0 deletions 36 ...fony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddAccessDecisionStrategyPass.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php

namespace Symfony\Bundle\SecurityBundle\DependencyInjection\Compiler;

use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;

class AddAccessDecisionStrategyPass implements CompilerPassInterface
{
/**
* You can modify the container here before it is dumped to PHP code.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't duplicate the interface phpdoc here (especially as it does not really make sense)

*
* @param ContainerBuilder $container
*
* @api
*/
public function process(ContainerBuilder $container)
{
if (!$container->hasDefinition('security.access.decision_manager')) {
return;
}

$strategies = array();
foreach ($container->findTaggedServiceIds('security.access_strategy') as $id => $attributes) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong naming. $attributes is not an array of attributes. It is an array of tags.

And you should support having several security.access_strategy tags on the same service by looping over tags instead of reading only the first one

$strategyName = isset($attributes[0]['strategy']) ? $attributes[0]['strategy'] : 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0 as strategy name ? why

$strategies[$strategyName] = new Reference($id);
}

if (!$strategies) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please use if (empty($strategies)). It will make the intent clearer IMO

throw new LogicException('No access decision strategies found. You need to tag at least one with "security.access_strategy"');
}

$container->getDefinition('security.access.decision_manager')->addMethodCall('setStrategies', $strategies);
}

}
25 changes: 25 additions & 0 deletions 25 src/Symfony/Bundle/SecurityBundle/Resources/config/security.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@

<parameter key="security.access.decision_manager.class">Symfony\Component\Security\Core\Authorization\AccessDecisionManager</parameter>

<parameter key="security.decide_affirmative_strategy.class">Symfony\Component\Security\Core\Authorization\Strategy\DecideAffirmativeStrategy</parameter>
<parameter key="security.decide_consensus_strategy.class">Symfony\Component\Security\Core\Authorization\Strategy\DecideConsensusStrategy</parameter>
<parameter key="security.decide_unanimous_strategy.class">Symfony\Component\Security\Core\Authorization\Strategy\DecideUnanimousStrategy</parameter>
<parameter key="security.decide_highest_not_abstained_voter_strategy.class">
Symfony\Component\Security\Core\Authorization\Strategy\DecideHighestNotAbstainedVoterStrategy
</parameter>

<parameter key="security.access.simple_role_voter.class">Symfony\Component\Security\Core\Authorization\Voter\RoleVoter</parameter>
<parameter key="security.access.authenticated_voter.class">Symfony\Component\Security\Core\Authorization\Voter\AuthenticatedVoter</parameter>
<parameter key="security.access.role_hierarchy_voter.class">Symfony\Component\Security\Core\Authorization\Voter\RoleHierarchyVoter</parameter>
Expand Down Expand Up @@ -111,6 +118,24 @@
<argument>%security.role_hierarchy.roles%</argument>
</service>

<!-- Access Strategies -->
<service id="security.access.decide_affirmative_strategy" class="%security.decide_affirmative_strategy.class%" public="false">
<tag name="security.access_strategy" strategy="affirmative"></tag>
</service>

<service id="security.access.decide_consensus_strategy" class="%security.decide_consensus_strategy.class%" public="false">
<tag name="security.access_strategy" strategy="consensus"></tag>
</service>

<service id="security.access.decide_unanimous_strategy" class="%security.decide_unanimous_strategy.class%" public="false">
<tag name="security.access_strategy" strategy="unanimous"></tag>
</service>

<service id="security.access.decide_highest_not_abstained_boter_strategy"
class="%security.decide_highest_not_abstained_voter_strategy.class%" public="false">
<tag name="security.access_strategy" strategy="highest"></tag>
</service>


<!-- Security Voters -->
<service id="security.access.simple_role_voter" class="%security.access.simple_role_voter.class%" public="false">
Expand Down
167 changes: 44 additions & 123 deletions 167 src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,10 @@

namespace Symfony\Component\Security\Core\Authorization;

use Symfony\Component\Security\Core\Authorization\Strategy\DecideAffirmativeStrategy;
use Symfony\Component\Security\Core\Authorization\Strategy\DecideConsensusStrategy;
use Symfony\Component\Security\Core\Authorization\Strategy\DecideHighestNotAbstainedVoterStrategy;
use Symfony\Component\Security\Core\Authorization\Strategy\DecideUnanimousStrategy;
use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;

Expand All @@ -25,8 +29,10 @@ class AccessDecisionManager implements AccessDecisionManagerInterface
const STRATEGY_AFFIRMATIVE = 'affirmative';
const STRATEGY_CONSENSUS = 'consensus';
const STRATEGY_UNANIMOUS = 'unanimous';
const STRATEGY_HIGHEST_NOT_ABSTAINED = 'highest';

private $voters;
private $strategies;
private $strategy;
private $allowIfAllAbstainDecisions;
private $allowIfEqualGrantedDeniedDecisions;
Expand All @@ -43,13 +49,9 @@ class AccessDecisionManager implements AccessDecisionManagerInterface
*/
public function __construct(array $voters = array(), $strategy = self::STRATEGY_AFFIRMATIVE, $allowIfAllAbstainDecisions = false, $allowIfEqualGrantedDeniedDecisions = true)
{
$strategyMethod = 'decide'.ucfirst($strategy);
if (!is_callable(array($this, $strategyMethod))) {
throw new \InvalidArgumentException(sprintf('The strategy "%s" is not supported.', $strategy));
}

$this->strategies = array();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

injecting an array of strategy objects does not make sense IMO: you only need a single strategy, and changing it after instantiation is forbidden.
You should just allow injecting a Strategy objct rather than a string, and handle other things in the configuration (allowing to specify either a builtin strategy, or a string, for BC reasons)

$this->voters = $voters;
$this->strategy = $strategyMethod;
$this->strategy = $strategy;
$this->allowIfAllAbstainDecisions = (bool) $allowIfAllAbstainDecisions;
$this->allowIfEqualGrantedDeniedDecisions = (bool) $allowIfEqualGrantedDeniedDecisions;
}
Expand All @@ -65,155 +67,74 @@ public function setVoters(array $voters)
}

/**
* {@inheritdoc}
* @param mixed $strategies
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wrong phpdoc

*/
public function decide(TokenInterface $token, array $attributes, $object = null)
public function addStrategy($name,$strategy)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing typehint

{
return $this->{$this->strategy}($token, $attributes, $object);
$this->strategies[$name] = $strategy;
}

/**
* {@inheritdoc}
*/
public function supportsAttribute($attribute)
private function getStrategy($strategyName)
{
foreach ($this->voters as $voter) {
if ($voter->supportsAttribute($attribute)) {
return true;
if(!array_key_exists($strategyName,$this->strategies))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space missing before (

{
switch($strategyName){
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space missing before (

case self::STRATEGY_UNANIMOUS:
return new DecideUnanimousStrategy();
case self::STRATEGY_CONSENSUS:
return new DecideConsensusStrategy();
case self::STRATEGY_AFFIRMATIVE:
return new DecideAffirmativeStrategy();
case self::STRATEGY_HIGHEST_NOT_ABSTAINED:
return new DecideHighestNotAbstainedVoterStrategy();
default:
break;
}
} elseif($this->strategies[$strategyName] instanceof AccessDecisionStrategyInterface) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space missing before (

return $this->strategies[$strategyName];
}

return false;
throw new \InvalidArgumentException(sprintf('The strategy "%s" is not supported.', $strategyName));
}

/**
* {@inheritdoc}
*/
public function supportsClass($class)
public function decide(TokenInterface $token, array $attributes, $object = null)
{
foreach ($this->voters as $voter) {
if ($voter->supportsClass($class)) {
return true;
}
}
$strategy = $this->getStrategy($this->strategy);
$strategy->setVoters($this->voters);
$strategy->setAllowIfAllAbstainDecisions($this->allowIfAllAbstainDecisions);
$strategy->setAllowIfEqualGrantedDeniedDecisions($this->allowIfEqualGrantedDeniedDecisions);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the desired way, why set it every decision? Why not just do it when building the container?


return false;
return $strategy->decide($token, $attributes, $object);
}

/**
* Grants access if any voter returns an affirmative response.
*
* If all voters abstained from voting, the decision will be based on the
* allowIfAllAbstainDecisions property value (defaults to false).
* {@inheritdoc}
*/
private function decideAffirmative(TokenInterface $token, array $attributes, $object = null)
public function supportsAttribute($attribute)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please keep these methods before decide, to avoid useless diffs, making the review easier

{
$deny = 0;
foreach ($this->voters as $voter) {
$result = $voter->vote($token, $object, $attributes);
switch ($result) {
case VoterInterface::ACCESS_GRANTED:
return true;

case VoterInterface::ACCESS_DENIED:
++$deny;

break;

default:
break;
if ($voter->supportsAttribute($attribute)) {
return true;
}
}

if ($deny > 0) {
return false;
}

return $this->allowIfAllAbstainDecisions;
return false;
}

/**
* Grants access if there is consensus of granted against denied responses.
*
* Consensus means majority-rule (ignoring abstains) rather than unanimous
* agreement (ignoring abstains). If you require unanimity, see
* UnanimousBased.
*
* If there were an equal number of grant and deny votes, the decision will
* be based on the allowIfEqualGrantedDeniedDecisions property value
* (defaults to true).
*
* If all voters abstained from voting, the decision will be based on the
* allowIfAllAbstainDecisions property value (defaults to false).
* {@inheritdoc}
*/
private function decideConsensus(TokenInterface $token, array $attributes, $object = null)
public function supportsClass($class)
{
$grant = 0;
$deny = 0;
foreach ($this->voters as $voter) {
$result = $voter->vote($token, $object, $attributes);

switch ($result) {
case VoterInterface::ACCESS_GRANTED:
++$grant;

break;

case VoterInterface::ACCESS_DENIED:
++$deny;

break;
}
}

if ($grant > $deny) {
return true;
}

if ($deny > $grant) {
return false;
}

if ($grant > 0) {
return $this->allowIfEqualGrantedDeniedDecisions;
}

return $this->allowIfAllAbstainDecisions;
}

/**
* Grants access if only grant (or abstain) votes were received.
*
* If all voters abstained from voting, the decision will be based on the
* allowIfAllAbstainDecisions property value (defaults to false).
*/
private function decideUnanimous(TokenInterface $token, array $attributes, $object = null)
{
$grant = 0;
foreach ($attributes as $attribute) {
foreach ($this->voters as $voter) {
$result = $voter->vote($token, $object, array($attribute));

switch ($result) {
case VoterInterface::ACCESS_GRANTED:
++$grant;

break;

case VoterInterface::ACCESS_DENIED:
return false;

default:
break;
}
if ($voter->supportsClass($class)) {
return true;
}
}

// no deny votes
if ($grant > 0) {
return true;
}

return $this->allowIfAllAbstainDecisions;
return false;
}
}
45 changes: 45 additions & 0 deletions 45 src/Symfony/Component/Security/Core/Authorization/AccessDecisionStrategyInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php

namespace Symfony\Component\Security\Core\Authorization;

use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;

/**
* AccessDecisionStrategyInterface contains the strategy to make access decissions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/decissions/decisions

*
* @author Fabien Potencier <fabien@symfony.com>
*/
interface AccessDecisionStrategyInterface
{
/**
* Configures the voters.
*
* @param VoterInterface[] $voters An array of VoterInterface instances
*/
public function setVoters(array $voters);

/**
* Set whether to grant access if all voters abstained or not.
*
* @param bool $allowIfAllAbstainDecisions
*/
public function setAllowIfAllAbstainDecisions($allowIfAllAbstainDecisions);

/**
* Set whether to grant access if result are equals.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/equals/equal/?

*
* @param bool $allowIfEqualGrantedDeniedDecisions
*/
public function setAllowIfEqualGrantedDeniedDecisions($allowIfEqualGrantedDeniedDecisions);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these setters don't make sense for all strategies. They only make sense for the 3 existing strategies


/**
* Decides whether the access is possible or not.
*
* @param TokenInterface $token
* @param array $attributes
* @param null $object
*
* @return true if this decision strategy decides that the access can be made
*/
public function decide(TokenInterface $token, array $attributes, $object = null);
}
36 changes: 36 additions & 0 deletions 36 src/Symfony/Component/Security/Core/Authorization/Strategy/AbstractDecideStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php

namespace Symfony\Component\Security\Core\Authorization\Strategy;

abstract class AbstractDecideStrategy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As each "stategy" is an implementation of AccessDecisionManagerInterface now, I think we should call this class AbstractAccessDecisionManager and let it implement the interface. I think we should also rename/remove the Strategy namespace.

{
protected $voters;

protected $allowIfAllAbstainDecisions;

protected $allowIfEqualGrantedDeniedDecisions;

/**
* {@inheritdoc}
*/
public function setVoters(array $voters)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there should be not setter here. this should be a constructor argument

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah no sorry, we have a setter currently

{
$this->voters = $voters;
}

/**
* {@inheritdoc}
*/
public function setAllowIfAllAbstainDecisions($allowIfAllAbstainDecisions)
{
$this->allowIfAllAbstainDecisions = $allowIfAllAbstainDecisions;
}

/**
* {@inheritdoc}
*/
public function setAllowIfEqualGrantedDeniedDecisions($allowIfEqualGrantedDeniedDecisions)
{
$this->allowIfEqualGrantedDeniedDecisions = $allowIfEqualGrantedDeniedDecisions;
}
}
Loading
Morty Proxy This is a proxified and sanitized view of the page, visit original site.