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

xabbuh · 2016-06-12T06:33:23Z

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).

xabbuh · 2016-06-12T06:35:25Z

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

+ *
+ * @author Christian Flothmann <christian.flothmann@xabbuh.de>
+ */
+class StrategyAwareAccessDecisionManager implements AccessDecisionManagerInterface


I don't really like this name. So I welcome any suggestions for something better.

I'm not really sure why you introduced a new class here, it looks like the old class has equal behaviour?

If we want to deprecate the old one in favor of this one, I would call it VotingAccessDecisionManager or the like. The special thing about this implementation is that it uses voters to decide the access.

xabbuh · 2016-06-12T06:57:00Z

I just realised that @hhamon suggested almost the same years ago (see #942) which was closed in favour of creating more specialised access decision managers (though that never happened). What do you think about that idea? Should we create one class per access decision strategy instead and then make it easier to inject your own access decision manager by introducing a new config option in the SecurityBundle?

xabbuh · 2016-06-23T12:04:07Z

@symfony/deciders What are your thoughts on this?

fabpot · 2016-06-23T12:10:18Z

Not sure if we need to create classes for existing strategies, we could keep them inline as today. If not, I would make those classes final.

wouterj · 2016-07-02T18:03:46Z

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

+
+        if ($strategy instanceof VoteStrategyInterface) {
+            $this->voteStrategy = $strategy;
+        } elseif (self::STRATEGY_AFFIRMATIVE === $strategy) {


what about deprecating passing a string as strategy?

And maybe removing allowIfAllAbstainDecisions, allowIfEqualGrantedDeniedDecisions from constructor of this class in future?

wouterj · 2016-07-02T18:09:21Z

I like the idea of having all strategies in seperate classes and configuring the service ID of the strategy to use (with a shortcut for security.strategy.%s IDs, so affirmative will set the security.strategy.affirmative service and app.custom_strategy will use that ID).

wouterj · 2016-07-02T18:09:28Z

Status: needs work

xabbuh · 2016-07-04T07:38:27Z

I like the idea of having all strategies in seperate classes and configuring the service ID of the strategy to use [...]

@wouterj My main concern is if we really need to go the way of custom strategies then or if it wasn't enough to configure a service implementing the AuthenticationManagerInterface and then making it easy to change that service?

fabpot · 2016-11-09T23:40:54Z

@xabbuh Any interest in finishing this one by taking into account comments?

xabbuh · 2016-11-10T08:40:27Z

Well, we should first decide which way we would like to go (i.e. making the vote strategy configurable or making it possible to configure the access decision manager like suggested in #942). I think my favourite solution is to keep the separated vote strategies for greater flexibility (as they are now in this PR), but remove the StrategyAwareAccessDecisionManager and instead allow just configure the access decision manager service in the SecurityBundle (you will only be able to configure either the access decision manager or the vote strategy). What do you think?

wouterj · 2016-11-10T22:13:44Z

On second thought, I think we should remove the strategy stuff and just allow to configure the AccessDecisionManager service ID. With the VoteStrategyInterface, we add an extra layer of complexity for almost nothing. The AccessDecisionManager simply only proxies it's decide() call to the VoteStrategy and there is no other code in the manager.

So simply creating a custom manager and setting it through a service should be more than enough.

xabbuh · 2016-11-28T08:35:49Z

I pushed an update that reverts the changes made to the AccessDecisionManager, but makes it possible to configure a service providing an AccessDecisionManagerInterface implementation.

xabbuh · 2016-12-14T20:49:22Z

ping @symfony/deciders

HeahDude · 2016-12-17T10:56:56Z

Does it make sense to keep the strategy configurable for a custom access decision manager as a service?

xabbuh · 2016-12-17T20:53:14Z

@HeahDude I first thought when I introduced the VoteStrategyInterface. But thinking about it again I don't think there's much value in having reusable voting strategies. Reusing them wouldn't be much different from using the built-in access decision manager IMO.

xabbuh · 2016-12-27T10:48:21Z

Status: Needs Review

nicolas-grekas

👍

nicolas-grekas · 2017-04-28T16:09:06Z

Moving to milestone as this is not a blocker for 3.3. If you don't agree, please explain why.

xabbuh · 2017-06-03T07:55:33Z

PR description updated to match the actual state of this PR (the idea of a VoteStrategyInterface was dropped in favour of being able to configure your own AccessDecisionManager service).

nicolas-grekas · 2017-07-12T07:40:26Z

ping @symfony/deciders

nicolas-grekas · 2017-07-12T11:43:59Z

Thank you @xabbuh.

nicolas-grekas · 2017-07-12T11:44:11Z

Doc PR welcomed ;)

…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 Status: Needs Review Security Feature labels Jun 12, 2016

xabbuh reviewed Jun 12, 2016
View reviewed changes

xabbuh mentioned this pull request Jun 12, 2016

Missing access decision strategy "highest not abstained voter" #14049

Closed

xabbuh force-pushed the configurable-access-decision-strategy branch from 935c6a5 to 6ef3459 Compare June 12, 2016 06:46

wouterj reviewed Jul 2, 2016
View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Jul 2, 2016

xabbuh force-pushed the configurable-access-decision-strategy branch from 6ef3459 to 7ec99f0 Compare November 16, 2016 08:09

xabbuh force-pushed the configurable-access-decision-strategy branch 2 times, most recently from 6417e74 to c28ca0a Compare November 28, 2016 08:09

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

xabbuh force-pushed the configurable-access-decision-strategy branch from c28ca0a to 4abe705 Compare December 27, 2016 10:47

carsonbot added Status: Needs Review and removed Status: Needs Work labels Dec 27, 2016

xabbuh mentioned this pull request Jan 6, 2017

[Security] Add strategy resolvers #21178

Closed

xabbuh force-pushed the configurable-access-decision-strategy branch 2 times, most recently from 099c769 to 9a8779f Compare January 30, 2017 16:08

nicolas-grekas approved these changes Mar 7, 2017

View reviewed changes

nicolas-grekas modified the milestones: 3.3, 3.x Mar 7, 2017

HeahDude approved these changes Mar 26, 2017

View reviewed changes

chalasr approved these changes Mar 29, 2017

View reviewed changes

xabbuh force-pushed the configurable-access-decision-strategy branch from 9a8779f to 1d5a5c6 Compare April 3, 2017 15:27

nicolas-grekas modified the milestones: 3.3, 3.4 Apr 28, 2017

xabbuh changed the base branch from master to 3.4 May 18, 2017 07:16

xabbuh force-pushed the configurable-access-decision-strategy branch from 1d5a5c6 to ba9fd2f Compare June 3, 2017 07:52

xabbuh changed the title ~~[Security] make it possible to configure the voting strategy~~ [Security] make it possible to configure a custom access decision manager service Jun 3, 2017

add option to define the access decision manager

e0913a2

xabbuh force-pushed the configurable-access-decision-strategy branch from ba9fd2f to e0913a2 Compare July 3, 2017 16:47

weaverryan approved these changes Jul 12, 2017

View reviewed changes

nicolas-grekas merged commit e0913a2 into symfony:3.4 Jul 12, 2017

xabbuh deleted the configurable-access-decision-strategy branch July 12, 2017 16:19

xabbuh mentioned this pull request Jul 12, 2017

[Security] Custom access decision manager services symfony/symfony-docs#8149

Closed

This was referenced Oct 18, 2017

Release v3.4.0-BETA1 #24609

Merged

Release v4.0.0-BETA1 #24612

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

xabbuh commented Jun 12, 2016 •

edited

Loading

xabbuh Jun 12, 2016

wouterj Jul 2, 2016

wouterj Jul 2, 2016

xabbuh commented Jun 12, 2016

xabbuh commented Jun 23, 2016

fabpot commented Jun 23, 2016

wouterj Jul 2, 2016

Koc Nov 2, 2016

wouterj commented Jul 2, 2016

wouterj commented Jul 2, 2016

xabbuh commented Jul 4, 2016

fabpot commented Nov 9, 2016

xabbuh commented Nov 10, 2016

wouterj commented Nov 10, 2016

xabbuh commented Nov 28, 2016

xabbuh commented Dec 14, 2016

HeahDude commented Dec 17, 2016

xabbuh commented Dec 17, 2016

xabbuh commented Dec 27, 2016

nicolas-grekas left a comment

nicolas-grekas commented Apr 28, 2017

xabbuh commented Jun 3, 2017

nicolas-grekas commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017

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

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

Conversation

xabbuh commented Jun 12, 2016 • edited Loading

xabbuh Jun 12, 2016

Choose a reason for hiding this comment

wouterj Jul 2, 2016

Choose a reason for hiding this comment

wouterj Jul 2, 2016

Choose a reason for hiding this comment

xabbuh commented Jun 12, 2016

xabbuh commented Jun 23, 2016

fabpot commented Jun 23, 2016

wouterj Jul 2, 2016

Choose a reason for hiding this comment

Koc Nov 2, 2016

Choose a reason for hiding this comment

wouterj commented Jul 2, 2016

wouterj commented Jul 2, 2016

xabbuh commented Jul 4, 2016

fabpot commented Nov 9, 2016

xabbuh commented Nov 10, 2016

wouterj commented Nov 10, 2016

xabbuh commented Nov 28, 2016

xabbuh commented Dec 14, 2016

HeahDude commented Dec 17, 2016

xabbuh commented Dec 17, 2016

xabbuh commented Dec 27, 2016

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Apr 28, 2017

xabbuh commented Jun 3, 2017

nicolas-grekas commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017

nicolas-grekas commented Jul 12, 2017

xabbuh commented Jun 12, 2016 •

edited

Loading