NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Security] Use IteratorArgument for voters#21437

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:masterfromjvasseur:voter-iterator

Apr 4, 2017

Merged

[Security] Use IteratorArgument for voters#21437

fabpot merged 1 commit intosymfony:masterfromjvasseur:voter-iterator

Apr 4, 2017

Conversation

Copy link

Contributor

jvasseur commentedJan 27, 2017

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.

carsonbot added Status: Needs Review Security Feature Deprecation labels

Jan 27, 2017

stof reviewed

Jan 27, 2017

View reviewed changes

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


		/**
		* @returnarray
		* @returniterable

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

jvasseurJan 27, 2017•
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link

Member

Choose a reason for hiding this comment

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

iterable|VoterInterface[] for helping IDE?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link

Member

stof commentedJan 27, 2017

What is the benefit of this change ?

Copy link

ContributorAuthor

jvasseur commentedJan 27, 2017

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 reviewed

Jan 29, 2017

View reviewed changes

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

		*/
		publicfunctionsetVoters(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);

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

jvasseurJan 29, 2017•
edited
Loading

Choose a reason for hiding this comment

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

Changing the type of the$voters argument fromarray toiterable would be a BC break so this would mean having a different argument type for the constructor parameter and this method.

Copy link

Member

nicolas-grekas commentedJan 29, 2017

👍

nicolas-grekas added this to the3.3 milestone

Jan 29, 2017

xabbuh reviewed

Jan 30, 2017

View reviewed changes

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


		$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');

Copy link

Member

Choose a reason for hiding this comment

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

Won't this change break theDebugAccessDecisionManager?

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

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

Copy link

Member

xabbuhJan 30, 2017

Choose a reason for hiding this comment

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

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 SogetVoters() would always return an empty list.

Copy link

ContributorAuthor

jvasseurJan 31, 2017

Choose a reason for hiding this comment

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

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.

Copy link

Member

xabbuhFeb 2, 2017

Choose a reason for hiding this comment

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

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

Copy link

Member

fabpotMar 1, 2017

Choose a reason for hiding this comment

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

What's the conclusion here?

Copy link

Member

xabbuhMar 1, 2017

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

jvasseurMar 1, 2017

Choose a reason for hiding this comment

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

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 theDebugAccessDecisionManager independently, but that would introduce the possibility that the list shown in the profiler is not the same as the one used by theAccessDecisionManager (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 reviewed

Jan 30, 2017

View reviewed changes

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

		* @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

Copy link

Member

fabpotJan 30, 2017

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

jvasseurJan 30, 2017

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekasJan 30, 2017

Choose a reason for hiding this comment

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

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

Copy link

Member

nicolas-grekas commentedJan 31, 2017

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

jvasseur force-pushed thevoter-iterator branch 2 times, most recently from5a25337 to58797f3Compare

January 31, 2017 22:20

nicolas-grekas approved these changes

Feb 14, 2017

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍

jvasseur force-pushed thevoter-iterator branch 5 times, most recently from926a51c toa4b574eCompare

March 1, 2017 15:15

Copy link

Member

nicolas-grekas commentedMar 1, 2017

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

jvasseur force-pushed thevoter-iterator branch froma4b574e toe012064Compare

March 1, 2017 16:41

Copy link

ContributorAuthor

jvasseur commentedMar 1, 2017

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

chalasr approved these changes

Mar 22, 2017

View reviewed changes

Copy link

Member

chalasr left a comment

Choose a reason for hiding this comment

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

changelog/upgrade files should mention this

Copy link

Member

fabpot commentedMar 22, 2017

👍

Copy link

Member

fabpot commentedMar 22, 2017

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

jvasseur force-pushed thevoter-iterator branch frome012064 to794a1b7Compare

March 23, 2017 08:42

Copy link

ContributorAuthor

jvasseur commentedMar 23, 2017

@fabpot rebased on current master.

Remaining test failures are unrelated to this PR.

Copy link

Member

xabbuh commentedMar 23, 2017

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

Copy link

ContributorAuthor

jvasseur commentedMar 23, 2017

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.

Copy link

Member

chalasr commentedMar 23, 2017

CHANGELOG/UPGRADE-* still need an update.

Copy link

Member

nicolas-grekas commentedApr 3, 2017

ping@jvasseur

jvasseur force-pushed thevoter-iterator branch from794a1b7 tobbcd44cCompare

April 4, 2017 12:56

Copy link

ContributorAuthor

jvasseur commentedApr 4, 2017

Note added to CHANGELOG/UPGRADE.

nicolas-grekas reviewed

Apr 4, 2017

View reviewed changes

UPGRADE-3.3.md

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

Copy link

Member

nicolas-grekasApr 4, 2017

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

jvasseurApr 4, 2017

Choose a reason for hiding this comment

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

Fixed

Use IteratorArgument for voters

4ec80b1

jvasseur force-pushed thevoter-iterator branch frombbcd44c to4ec80b1Compare

April 4, 2017 17:58

nicolas-grekas approved these changes

Apr 4, 2017

View reviewed changes

Copy link

Member

nicolas-grekas left a comment

Choose a reason for hiding this comment

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

👍

Copy link

Member

fabpot commentedApr 4, 2017

Thank you@jvasseur.

fabpot merged commit4ec80b1 intosymfony:master

Apr 4, 2017

fabpot added a commit that referenced this pull request

Apr 4, 2017

feature#21437[Security] Use IteratorArgument for voters (jvasseur)

ab624f1

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       | MITUse an IteratorArgument for injecting voters into the AccessDecisionManager.Commits-------4ec80b1 Use IteratorArgument for voters

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