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

solilokiam · Jul 16, 2015

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#14049
License	MIT
Doc PR

submit changes to the documentation

This PR adds a new access decision strategy, called highest not abstained voter. The main idea of the voter is documented on issue #14049 .

At the same time the PR tries to make access decision strategy extensible. The way it's done is via any service that implements the newly created AccessDecisionManagerInterface and has the tag security.access_strategy

linaori · Jul 16, 2015

src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php

+        $strategy = $this->getStrategy($this->strategy);
+        $strategy->setVoters($this->voters);
+        $strategy->setAllowIfAllAbstainDecisions($this->allowIfAllAbstainDecisions);
+        $strategy->setAllowIfEqualGrantedDeniedDecisions($this->allowIfEqualGrantedDeniedDecisions);


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

stof · Jul 16, 2015

Actually, each strategy is just a different implementation of the AccessDecisionManagerInterface (especially if we remove supportsClass and supportsAttribute in 3.0 as we never use them in Symfony).

So I suggest refactoring this as 3 separate implementations, and having the existing class being a proxy class instantiating the right implementation in its constructor and then delegating decision calls.
This would avoid the need to introduce a new concept of a StrategyInterface with a weird API making it mutable just because of the need to enforce sharing the same state.

solilokiam · Jul 16, 2015

Yes, I like the idea of getting rid of the StrategyInterface. I'm working on it.

hhamon · Jul 16, 2015

src/Symfony/Component/Security/Core/Authorization/AccessDecisionManager.php

-        foreach ($this->voters as $voter) {
-            if ($voter->supportsAttribute($attribute)) {
-                return true;
+        if(!array_key_exists($strategyName,$this->strategies))


Space missing before (

wouterj · Jul 21, 2015

I like the idea of making this extendable, so a big thanks for starting this!

I've left some comments, most of them are really debatable things so if you don't agree with them, don't hesitate to comment instead of apply the change :)

Status: Needs Work

solilokiam · Jul 29, 2015

Hi, one of the tests is failing in travis and I've got no idea why. Can anyone help me?

wouterj · Jul 29, 2015

Afaik, it's failing because this tests installs 3.0-dev. In 3.0-dev, the new constrant in the Security component does not exists, so there is an error. After this PR is merged, it will be merged into master/3.0-dev as well. This means that after merging, the tests should pass.

stof · Jul 29, 2015

src/Symfony/Component/Security/Core/Authorization/Strategy/AbstractDecideStrategy.php

+    /**
+     * {@inheritdoc}
+     */
+    public function setVoters(array $voters)


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

ah no sorry, we have a setter currently

HighestNotAbstainedVoterStrategy has been added. Also this commit makes access decision strategy extensible.

…ccess decision strategies

hhamon · Oct 9, 2015

...y/Core/Authorization/AccessDecisionManager/HighestNotAbstainedVoterAccessDecisionManager.php

+use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
+
+/**
+ * Grants access depending on the first not abstained voter decission.


Typo in decission word!

by the way, the class is called Highest ... and here we mention first ... Should we change HighestNotAbstainedVoterAccessDecisionManager by FirstNotAbstainedVoterAccessDecisionManager ?

Yes, I like the idea of calling it FirstNotAbstainedVoterAccessDecisionManager maybe it makes it clearer on what it does.

fabpot · Jan 25, 2016

@solilokiam Can you rebase this one on current master and submit a new PR? As master is going to be 3.1, deprecated feature have been removed and this should make this PR easier.

fabpot · Mar 2, 2016

@solilokiam Are you still interested in finishing this one?

fabpot · Jun 9, 2016

Closing as there is no more activity here. Feel free to reopen with a new version of the patch that applies on current master.

…ss decision manager service (xabbuh) This PR was merged into the 3.4 branch. Discussion ---------- [Security] make it possible to configure a custom access decision manager service | Q | A | | --- | --- | | Branch? | 3.4 | | Bug fix? | no | | New feature? | yes | | BC breaks? | no | | Deprecations? | no | | Tests pass? | yes | | Fixed tickets | #942, #14049, #15295, #16828, #16843, | | License | MIT | | Doc PR | TODO | These changes will make it possible to let users define their own voting strategies without the need for custom compiler passes that replace the built-in `AccessDecisionManager` (see linked issues in the PR table for some use cases). Commits ------- e0913a2 add option to define the access decision manager

carsonbot added the Status: Needs Review label Jul 16, 2015

solilokiam force-pushed the extensible-accessdecision-strategy branch 3 times, most recently from e28edf1 to 52042eb Compare July 16, 2015 16:29

linaori reviewed Jul 16, 2015
View reviewed changes

hhamon reviewed Jul 16, 2015
View reviewed changes

carsonbot removed the Status: Needs Review label Jul 21, 2015

carsonbot added the Status: Needs Work label Jul 21, 2015

solilokiam force-pushed the extensible-accessdecision-strategy branch 2 times, most recently from 16bee34 to 74142ac Compare July 27, 2015 15:23

stof reviewed Jul 29, 2015
View reviewed changes

solilokiam added 6 commits July 31, 2015 11:07

[Security] Fixed symfony#14049

38b8cf5

HighestNotAbstainedVoterStrategy has been added. Also this commit makes access decision strategy extensible.

Make AccessDecisionManager work as a proxy class for other concrete a…

66457b0

…ccess decision strategies

Remove AccessDecisionStrategyInterface

5894731

Fix wrong coding standards

78c18ab

Make access decision manager configurable.

f78b3df

Improve CustomAccessDecisionManagerCompilerPass

ffb411b

solilokiam force-pushed the extensible-accessdecision-strategy branch from 74142ac to c62e0bc Compare July 31, 2015 09:36

Change strategy namespace and naming

5d5f4ef

solilokiam force-pushed the extensible-accessdecision-strategy branch from c62e0bc to 5d5f4ef Compare July 31, 2015 09:38

hhamon reviewed Oct 9, 2015
View reviewed changes

javiereguiluz added Security Feature labels Mar 3, 2016

fabpot closed this Jun 9, 2016

xabbuh mentioned this pull request Jun 12, 2016

[Security] make it possible to configure a custom access decision manager service #19034

Merged

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

Uh oh!

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

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

Uh oh!

Conversation

solilokiam commented Jul 16, 2015

Uh oh!

linaori Jul 16, 2015

Choose a reason for hiding this comment

Uh oh!

stof commented Jul 16, 2015

Uh oh!

solilokiam commented Jul 16, 2015

Uh oh!

hhamon Jul 16, 2015

Choose a reason for hiding this comment

Uh oh!

wouterj commented Jul 21, 2015

Uh oh!

solilokiam commented Jul 29, 2015

Uh oh!

wouterj commented Jul 29, 2015

Uh oh!

stof Jul 29, 2015

Choose a reason for hiding this comment

Uh oh!

stof Jul 29, 2015

Choose a reason for hiding this comment

Uh oh!

hhamon Oct 9, 2015

Choose a reason for hiding this comment

Uh oh!

javiereguiluz Oct 9, 2015

Choose a reason for hiding this comment

Uh oh!

solilokiam Oct 9, 2015

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jan 25, 2016

Uh oh!

fabpot commented Mar 2, 2016

Uh oh!

fabpot commented Jun 9, 2016

Uh oh!

Uh oh!