[Security] Use IteratorArgument for voters #21437

jvasseur · 2017-01-27T18:16:17Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
License	MIT

Use an IteratorArgument for injecting voters into the AccessDecisionManager.

stof · 2017-01-27T18:27:45Z

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

@@ -81,7 +88,7 @@ public function getStrategy()
    }

    /**
-     * @return array
+     * @return iterable


technically, this is a BC break if the calling code passes it to a function requiring an array

True, but this class is marked as internal, so the BC break should be ok.

iterable|VoterInterface[] for helping IDE?

Would IDEs support the iterable<VoterInterface> syntax from PSR-5 ?

stof · 2017-01-27T18:28:10Z

What is the benefit of this change ?

jvasseur · 2017-01-27T18:32:44Z

The benefits are :

prevent instancing all the voters when not needed.
remove the need for the setVoters method to allow voters to depend on the AccessDecisionManager.

nicolas-grekas · 2017-01-29T19:16:45Z

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

     */
    public function setVoters(array $voters)
    {
+        @trigger_error(sprintf('The %s() method is deprecated since version 3.3 and will be removed in 4.0. Pass the voters to the constructor instead.', __METHOD__), E_USER_DEPRECATED);


Do we gain anything by deprecating this method? Does it hurt to keep it?

Changing the type of the $voters argument from array to iterable would be a BC break so this would mean having a different argument type for the constructor parameter and this method.

nicolas-grekas · 2017-01-29T19:17:17Z

👍

xabbuh · 2017-01-30T13:38:10Z

src/Symfony/Bundle/SecurityBundle/DependencyInjection/Compiler/AddSecurityVotersPass.php

@@ -45,7 +46,7 @@ public function process(ContainerBuilder $container)
            throw new LogicException('No security voters found. You need to tag at least one with "security.voter"');
        }

-        $adm = $container->getDefinition($container->hasDefinition('debug.security.access.decision_manager') ? 'debug.security.access.decision_manager' : 'security.access.decision_manager');
-        $adm->addMethodCall('setVoters', array(array_values($voters)));
+        $adm = $container->getDefinition('security.access.decision_manager');


Won't this change break the DebugAccessDecisionManager?

I updated it to grab the voters properties from the AccessDecisionManager in the same way it is doing it for the strategy : https://github.com/symfony/symfony/pull/21437/files#diff-76aa9de2473eccdc05075961beaed023R42

But now the debug.security.access.decision_manager service will not be aware of the available voters, will it (see #18566 for more context)?

The voters will be injected into the security.access.decision_manager service that will then be injected into the debug.security.access.decision_manager service that will grab the voters so everything should be fine.

But then these lines will never be executed: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Security/Core/Authorization/TraceableAccessDecisionManager.php#L62-L70 So getVoters() would always return an empty list.

It tried different solutions to fix this problems but either they break BC in a worse way or introduce a risk that the voters list shown in the profiler is different than the one used by the access decision manager.

Or maybe someone else has solution to fix this problem.
Maybe this would be ok keeping it like this, the only thing broken would be getting an empty voter list in the profiler when using such an implementation.

IIRC we explicitly have this handling because there were issue otherwise when you were, for example, using the JMSSecurityExtraBundle which ships with its own RememberingAccessDecisionManager.

What's the conclusion here?

Did someone test the changes from this PR in a real project to ensure that the profiler still works as expected?

My opinion here would be to keep it like this and accept that you lose the list of voters in the profiler in some cases (you already lose the configured strategy in those cases).

The alternatives are IMO not worth it. One of them would be to pass the list of voters to the DebugAccessDecisionManager independently, but that would introduce the possibility that the list shown in the profiler is not the same as the one used by the AccessDecisionManager (if someone modify it in another compile pass). And the other possibilities I could think of would either have worst BC breaks or introduce possible differences in behavior between prod and dev environments.

fabpot · 2017-01-30T16:19:23Z

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

-     * @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 iterable $voters                             An iterator of VoterInterface instances


That's a BC break, we should accept iterable|VoterInterface[]

iterable is equivalent to array|\Traversable so we still accept arrays as before.

iterable|VoterInterface[] would still allow IDEs to provide type hints

nicolas-grekas · 2017-01-31T07:52:24Z

I did not look at the exact logic: in AccessDecisionManager::decideUnanimous, would it be possible to swap the two foreach, ie iterate over voters, then over attributes inside? That may prevent instantiating some voters if eg a grant is denied early.

nicolas-grekas

👍

nicolas-grekas · 2017-03-01T16:14:31Z

@jvasseur can you please rebase? there are failures related to this PR isn't it?

jvasseur · 2017-03-01T16:42:26Z

Rebased and fix the test failure that was related to the changes in this PR.

chalasr

changelog/upgrade files should mention this

fabpot · 2017-03-22T22:58:32Z

👍

fabpot · 2017-03-22T22:58:52Z

A quick rebase to ensure that tests are all-right would be good as well.

jvasseur · 2017-03-23T08:53:07Z

@fabpot rebased on current master.

Remaining test failures are unrelated to this PR.

xabbuh · 2017-03-23T09:49:32Z

So did someone try this PR in a real project and made sure that this will not break the security profiler?

jvasseur · 2017-03-23T10:00:53Z

I did try it on a real project and everything was working. But since I made some changes to the PR since then I will try to do it again when I have some free time.

chalasr · 2017-03-23T10:01:51Z

CHANGELOG/UPGRADE-* still need an update.

nicolas-grekas · 2017-04-03T10:10:24Z

ping @jvasseur

jvasseur · 2017-04-04T13:03:29Z

Note added to CHANGELOG/UPGRADE.

nicolas-grekas · 2017-04-04T17:12:22Z

UPGRADE-3.3.md

@@ -258,6 +258,8 @@ Security
 * The `LogoutUrlGenerator::registerListener()` method will expect a 6th `$context = null` argument in 4.0.
   Define the argument when overriding this method.

+ * The `AccessDecisionManager::setVoters()` has been deprecated. Pass the voters to the constructor instead.


corresponding entry missing from UPGRADE-4.0.md (the ... has been removed. ...)

nicolas-grekas

👍

fabpot · 2017-04-04T19:44:49Z

Thank you @jvasseur.

This PR was merged into the 3.3-dev branch. Discussion ---------- [Security] Use IteratorArgument for voters | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | License | MIT Use an IteratorArgument for injecting voters into the AccessDecisionManager. Commits ------- 4ec80b1 Use IteratorArgument for voters

carsonbot added Status: Needs Review Security Feature Deprecation labels Jan 27, 2017

stof reviewed Jan 27, 2017

View reviewed changes

nicolas-grekas reviewed Jan 29, 2017

View reviewed changes

nicolas-grekas added this to the 3.3 milestone Jan 29, 2017

xabbuh reviewed Jan 30, 2017

View reviewed changes

fabpot reviewed Jan 30, 2017

View reviewed changes

jvasseur force-pushed the voter-iterator branch 2 times, most recently from 5a25337 to 58797f3 Compare January 31, 2017 22:20

nicolas-grekas approved these changes Feb 14, 2017

View reviewed changes

jvasseur force-pushed the voter-iterator branch 5 times, most recently from 926a51c to a4b574e Compare March 1, 2017 15:15

jvasseur force-pushed the voter-iterator branch from a4b574e to e012064 Compare March 1, 2017 16:41

chalasr approved these changes Mar 22, 2017

View reviewed changes

jvasseur force-pushed the voter-iterator branch from e012064 to 794a1b7 Compare March 23, 2017 08:42

jvasseur force-pushed the voter-iterator branch from 794a1b7 to bbcd44c Compare April 4, 2017 12:56

nicolas-grekas reviewed Apr 4, 2017

View reviewed changes

Use IteratorArgument for voters

4ec80b1

jvasseur force-pushed the voter-iterator branch from bbcd44c to 4ec80b1 Compare April 4, 2017 17:58

nicolas-grekas approved these changes Apr 4, 2017

View reviewed changes

fabpot merged commit 4ec80b1 into symfony:master Apr 4, 2017

jvasseur deleted the voter-iterator branch April 4, 2017 21:00

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged

[Security] Use IteratorArgument for voters #21437

[Security] Use IteratorArgument for voters #21437

Conversation

jvasseur commented Jan 27, 2017

Choose a reason for hiding this comment

jvasseur Jan 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stof commented Jan 27, 2017

jvasseur commented Jan 27, 2017

Choose a reason for hiding this comment

jvasseur Jan 29, 2017 • edited Loading

Choose a reason for hiding this comment

nicolas-grekas commented Jan 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas commented Jan 31, 2017

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Mar 1, 2017

jvasseur commented Mar 1, 2017

chalasr left a comment

Choose a reason for hiding this comment

fabpot commented Mar 22, 2017

fabpot commented Mar 22, 2017

jvasseur commented Mar 23, 2017

xabbuh commented Mar 23, 2017

jvasseur commented Mar 23, 2017

chalasr commented Mar 23, 2017

nicolas-grekas commented Apr 3, 2017

jvasseur commented Apr 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

fabpot commented Apr 4, 2017

jvasseur Jan 27, 2017 •

edited

Loading

jvasseur Jan 29, 2017 •

edited

Loading