From b8887b0f426d1d343c5e403410c163e2bbbf5486 Mon Sep 17 00:00:00 2001 From: Christian Scheb Date: Mon, 28 Dec 2015 17:31:29 +0100 Subject: [PATCH] Revert registry back to the old implementation to fix circular reference exception --- .../Compiler/ProviderCompilerPass.php | 14 ++-- Resources/config/security.xml | 7 +- .../Provider/TwoFactorProviderCollection.php | 35 --------- .../Provider/TwoFactorProviderRegistry.php | 17 ++-- Security/TwoFactor/Voter.php | 33 ++++---- .../Compiler/ProviderCompilerPassTest.php | 57 +++++++++----- .../TwoFactorProviderCollectionTest.php | 34 -------- .../TwoFactorProviderRegistryTest.php | 11 +-- Tests/Security/TwoFactor/VoterTest.php | 77 +++++-------------- 9 files changed, 91 insertions(+), 194 deletions(-) delete mode 100644 Security/TwoFactor/Provider/TwoFactorProviderCollection.php delete mode 100644 Tests/Security/TwoFactor/Provider/TwoFactorProviderCollectionTest.php diff --git a/DependencyInjection/Compiler/ProviderCompilerPass.php b/DependencyInjection/Compiler/ProviderCompilerPass.php index a315d613..47e3b53f 100644 --- a/DependencyInjection/Compiler/ProviderCompilerPass.php +++ b/DependencyInjection/Compiler/ProviderCompilerPass.php @@ -16,22 +16,24 @@ class ProviderCompilerPass implements CompilerPassInterface */ public function process(ContainerBuilder $container) { - if (! $container->hasDefinition("scheb_two_factor.provider_collection")) { + if (! $container->hasDefinition("scheb_two_factor.provider_registry")) { return; } - $definition = $container->getDefinition('scheb_two_factor.provider_collection'); + $registryDefinition = $container->getDefinition('scheb_two_factor.provider_registry'); + $voterDefinition = $container->getDefinition('scheb_two_factor.security_voter'); $taggedServices = $container->findTaggedServiceIds('scheb_two_factor.provider'); $references = array(); + $providerNames = array(); foreach ($taggedServices as $id => $attributes) { if (!isset($attributes[0]['alias'])) { throw new InvalidArgumentException('Tag "scheb_two_factor.provider" requires attribute "alias" to be set.'); } $name = $attributes[0]['alias']; - $definition->addMethodCall( - 'addProvider', - array($name, new Reference($id)) - ); + $references[$name] = new Reference($id); + $providerNames[] = $name; } + $registryDefinition->replaceArgument(1, $references); + $voterDefinition->replaceArgument(1, $providerNames); } } diff --git a/Resources/config/security.xml b/Resources/config/security.xml index 67033d2a..821946c4 100644 --- a/Resources/config/security.xml +++ b/Resources/config/security.xml @@ -6,7 +6,6 @@ Scheb\TwoFactorBundle\Security\TwoFactor\Trusted\TrustedCookieManager Scheb\TwoFactorBundle\Security\TwoFactor\Trusted\TrustedTokenGenerator Scheb\TwoFactorBundle\Security\TwoFactor\Trusted\TrustedFilter - Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderCollection Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderRegistry Scheb\TwoFactorBundle\Security\TwoFactor\Backup\BackupCodeValidator Scheb\TwoFactorBundle\Security\TwoFactor\Voter @@ -32,18 +31,16 @@ %scheb_two_factor.trusted_computer.enabled% %scheb_two_factor.parameter_names.trusted% - - - + - + diff --git a/Security/TwoFactor/Provider/TwoFactorProviderCollection.php b/Security/TwoFactor/Provider/TwoFactorProviderCollection.php deleted file mode 100644 index fc17dbb2..00000000 --- a/Security/TwoFactor/Provider/TwoFactorProviderCollection.php +++ /dev/null @@ -1,35 +0,0 @@ -providers[$name] = $provider; - } - - /** - * getProviders - * @return array - **/ - public function getProviders() - { - return $this->providers; - } -} diff --git a/Security/TwoFactor/Provider/TwoFactorProviderRegistry.php b/Security/TwoFactor/Provider/TwoFactorProviderRegistry.php index be024b7b..070adeb5 100644 --- a/Security/TwoFactor/Provider/TwoFactorProviderRegistry.php +++ b/Security/TwoFactor/Provider/TwoFactorProviderRegistry.php @@ -4,7 +4,6 @@ use Scheb\TwoFactorBundle\Security\TwoFactor\AuthenticationHandlerInterface; use Scheb\TwoFactorBundle\Security\TwoFactor\Session\SessionFlagManager; use Scheb\TwoFactorBundle\Security\TwoFactor\AuthenticationContext; -use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderCollection; use Symfony\Component\HttpFoundation\Response; class TwoFactorProviderRegistry implements AuthenticationHandlerInterface @@ -20,20 +19,20 @@ class TwoFactorProviderRegistry implements AuthenticationHandlerInterface /** * List of two-factor providers * - * @var TwoFactorProviderCollection + * @var array $providers */ - private $providerCollection; + private $providers; /** * Initialize with an array of registered two-factor providers * - * @param \Scheb\TwoFactorBundle\Security\TwoFactor\Session\SessionFlagManager $flagManager - * @param \Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderCollection $providerCollection + * @param \Scheb\TwoFactorBundle\Security\TwoFactor\Session\SessionFlagManager $flagManager + * @param array $providers */ - public function __construct(SessionFlagManager $flagManager, TwoFactorProviderCollection $providerCollection) + public function __construct(SessionFlagManager $flagManager, $providers = array()) { $this->flagManager = $flagManager; - $this->providerCollection = $providerCollection; + $this->providers = $providers; } /** @@ -43,7 +42,7 @@ public function __construct(SessionFlagManager $flagManager, TwoFactorProviderCo */ public function beginAuthentication(AuthenticationContext $context) { - foreach ($this->providerCollection->getProviders() as $providerName => $provider) { + foreach ($this->providers as $providerName => $provider) { if ($provider->beginAuthentication($context)) { $this->flagManager->setBegin($providerName, $context->getToken()); } @@ -62,7 +61,7 @@ public function requestAuthenticationCode(AuthenticationContext $context) $token = $context->getToken(); // Iterate over two-factor providers and ask for completion - foreach ($this->providerCollection->getProviders() as $providerName => $provider) { + foreach ($this->providers as $providerName => $provider) { if ($this->flagManager->isNotAuthenticated($providerName, $token)) { $response = $provider->requestAuthenticationCode($context); diff --git a/Security/TwoFactor/Voter.php b/Security/TwoFactor/Voter.php index 2618520e..f9fb75e6 100644 --- a/Security/TwoFactor/Voter.php +++ b/Security/TwoFactor/Voter.php @@ -1,15 +1,10 @@ sessionFlagManager = $sessionFlagManager; - $this->providerCollection = $providerCollection; + $this->providers = $providers; } /** - * supportsClass * @param string $class + * * @return boolean true **/ public function supportsClass($class) @@ -45,8 +38,8 @@ public function supportsClass($class) } /** - * supportsAttribute * @param string $attribute + * * @return boolean true **/ public function supportsAttribute($attribute) @@ -55,20 +48,20 @@ public function supportsAttribute($attribute) } /** - * vote * @param TokenInterface $token * @param mixed $object * @param array $attributes + * * @return mixed result **/ public function vote(TokenInterface $token, $object, array $attributes) { - foreach ($this->providerCollection->getProviders() as $providerName => $provider) { - $res = $this->sessionFlagManager->isNotAuthenticated($providerName, $token); - if (true === $res) { + foreach ($this->providers as $providerName) { + if ($this->sessionFlagManager->isNotAuthenticated($providerName, $token)) { return VoterInterface::ACCESS_DENIED; } } + return VoterInterface::ACCESS_ABSTAIN; } } diff --git a/Tests/DependencyInjection/Compiler/ProviderCompilerPassTest.php b/Tests/DependencyInjection/Compiler/ProviderCompilerPassTest.php index cf4b029e..cbfd633c 100644 --- a/Tests/DependencyInjection/Compiler/ProviderCompilerPassTest.php +++ b/Tests/DependencyInjection/Compiler/ProviderCompilerPassTest.php @@ -2,7 +2,6 @@ namespace Scheb\TwoFactorBundle\Tests\DependencyInjection\Compiler; use Scheb\TwoFactorBundle\DependencyInjection\Compiler\ProviderCompilerPass; -use Symfony\Component\DependencyInjection\ContainerBuilder; use Symfony\Component\DependencyInjection\Reference; class ProviderCompilerPassTest extends \PHPUnit_Framework_TestCase @@ -21,7 +20,12 @@ class ProviderCompilerPassTest extends \PHPUnit_Framework_TestCase /** * @var \PHPUnit_Framework_MockObject_MockObject */ - private $definition; + private $registryDefinition; + + /** + * @var \PHPUnit_Framework_MockObject_MockObject + */ + private $voterDefinition; public function setUp() { @@ -36,17 +40,22 @@ private function stubContainerService($taggedServices) { $this->createServiceDefinition(); $this->container - ->expects($this->once()) + ->expects($this->at(0)) ->method("hasDefinition") - ->with("scheb_two_factor.provider_collection") + ->with("scheb_two_factor.provider_registry") ->will($this->returnValue(true)); $this->container - ->expects($this->once()) + ->expects($this->at(1)) ->method("getDefinition") - ->with("scheb_two_factor.provider_collection") - ->will($this->returnValue($this->definition)); + ->with("scheb_two_factor.provider_registry") + ->will($this->returnValue($this->registryDefinition)); $this->container - ->expects($this->once()) + ->expects($this->at(2)) + ->method("getDefinition") + ->with("scheb_two_factor.security_voter") + ->will($this->returnValue($this->voterDefinition)); + $this->container + ->expects($this->at(3)) ->method("findTaggedServiceIds") ->with("scheb_two_factor.provider") ->will($this->returnValue($taggedServices)); @@ -54,7 +63,10 @@ private function stubContainerService($taggedServices) private function createServiceDefinition() { - $this->definition = $this->getMockBuilder("Symfony\Component\DependencyInjection\Definition") + $this->registryDefinition = $this->getMockBuilder("Symfony\Component\DependencyInjection\Definition") + ->disableOriginalConstructor() + ->getMock(); + $this->voterDefinition = $this->getMockBuilder("Symfony\Component\DependencyInjection\Definition") ->disableOriginalConstructor() ->getMock(); } @@ -68,7 +80,7 @@ public function process_notHasDefinition_doNothing() $this->container ->expects($this->once()) ->method("hasDefinition") - ->with("scheb_two_factor.provider_collection") + ->with("scheb_two_factor.provider_registry") ->will($this->returnValue(false)); $this->container ->expects($this->never()) @@ -80,16 +92,21 @@ public function process_notHasDefinition_doNothing() /** * @test */ - public function process_noTaggedServices_noProviderAddedToCollection() + public function process_noTaggedServices_replaceArgumentWithEmptyArray() { $this->createServiceDefinition(); $taggedServices = array(); $this->stubContainerService($taggedServices); //Mock the Definition - $this->definition - ->expects($this->never()) - ->method("addMethodCall"); + $this->registryDefinition + ->expects($this->once()) + ->method("replaceArgument") + ->with(1, array()); + $this->voterDefinition + ->expects($this->once()) + ->method("replaceArgument") + ->with(1, array()); $this->compilerPass->process($this->container); } @@ -97,7 +114,7 @@ public function process_noTaggedServices_noProviderAddedToCollection() /** * @test */ - public function process_taggedServices_addProviderToCollection() + public function process_taggedServices_replaceArgumentWithServiceList() { $this->createServiceDefinition(); $taggedServices = array('serviceId' => array( @@ -106,10 +123,14 @@ public function process_taggedServices_addProviderToCollection() $this->stubContainerService($taggedServices); //Mock the Definition - $this->definition + $this->registryDefinition + ->expects($this->once()) + ->method("replaceArgument") + ->with(1, array('providerAlias' => new Reference("serviceId"))); + $this->voterDefinition ->expects($this->once()) - ->method("addMethodCall") - ->with('addProvider', array('providerAlias', new Reference("serviceId"))); + ->method("replaceArgument") + ->with(1, array('providerAlias')); $this->compilerPass->process($this->container); } diff --git a/Tests/Security/TwoFactor/Provider/TwoFactorProviderCollectionTest.php b/Tests/Security/TwoFactor/Provider/TwoFactorProviderCollectionTest.php deleted file mode 100644 index 1b5d0bba..00000000 --- a/Tests/Security/TwoFactor/Provider/TwoFactorProviderCollectionTest.php +++ /dev/null @@ -1,34 +0,0 @@ -getMock("Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderInterface"); - - $collection->addProvider('test', $provider); - - $providers = $collection->getProviders(); - - $this->assertEquals(array('test' => $provider), $providers); - } - - /** - * @test - */ - public function noProvider_getProviders() - { - $collection = new TwoFactorProviderCollection(); - - $providers = $collection->getProviders(); - - $this->assertEquals(array(), $providers); - } -} diff --git a/Tests/Security/TwoFactor/Provider/TwoFactorProviderRegistryTest.php b/Tests/Security/TwoFactor/Provider/TwoFactorProviderRegistryTest.php index 9271008c..548d02d5 100644 --- a/Tests/Security/TwoFactor/Provider/TwoFactorProviderRegistryTest.php +++ b/Tests/Security/TwoFactor/Provider/TwoFactorProviderRegistryTest.php @@ -2,7 +2,6 @@ namespace Scheb\TwoFactorBundle\Tests\Security\TwoFactor\Provider; use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderRegistry; -use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderCollection; use Symfony\Component\HttpFoundation\Response; class TwoFactorProviderRegistryTest extends \PHPUnit_Framework_TestCase @@ -22,11 +21,6 @@ class TwoFactorProviderRegistryTest extends \PHPUnit_Framework_TestCase */ private $registry; - /** - * @var \Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderCollection - **/ - protected $collection; - public function setUp() { $this->flagManager = $this->getMockBuilder("Scheb\TwoFactorBundle\Security\TwoFactor\Session\SessionFlagManager") @@ -35,10 +29,7 @@ public function setUp() $this->provider = $this->getMock("Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderInterface"); - $this->collection = new TwoFactorProviderCollection(); - $this->collection->addProvider('test', $this->provider); - - $this->registry = new TwoFactorProviderRegistry($this->flagManager, $this->collection); + $this->registry = new TwoFactorProviderRegistry($this->flagManager, array('test' => $this->provider)); } private function getToken() diff --git a/Tests/Security/TwoFactor/VoterTest.php b/Tests/Security/TwoFactor/VoterTest.php index a9a58f1e..760d2078 100644 --- a/Tests/Security/TwoFactor/VoterTest.php +++ b/Tests/Security/TwoFactor/VoterTest.php @@ -1,7 +1,6 @@ provider = $this->getMock("Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderInterface"); - - } - - private function getProviderCollection($providers = true) - { - $providerCollection = new TwoFactorProviderCollection(); - if(true === $providers) { - $providerCollection->addProvider('test', $this->provider); - } - - return $providerCollection; } private function getSessionFlagManager() @@ -63,13 +46,11 @@ private function getVoter($providerCollection, $sessionFlagManager) /** * @test - **/ - public function vote_notAuthenticated_withProviders() + */ + public function vote_notAuthenticatedWithProviders_returnAccessDenied() { $token = $this->getToken(); - $sessionFlagManager = $this->getSessionFlagManager(); - $providerCollection = $this->getProviderCollection(); $sessionFlagManager ->expects($this->once()) @@ -77,41 +58,35 @@ public function vote_notAuthenticated_withProviders() ->with('test', $token) ->will($this->returnValue(true)); - $voter = $this->getVoter($providerCollection, $sessionFlagManager); + $voter = $this->getVoter(array("test"), $sessionFlagManager); $this->assertEquals(VoterInterface::ACCESS_DENIED, $voter->vote($token, null, array())); } /** * @test - **/ - public function vote_notAuthenticated_noProviders() + */ + public function vote_notAuthenticatedNoProviders_returnAccessDenied() { $token = $this->getToken(); $sessionFlagManager = $this->getSessionFlagManager(); - $providerCollection = $this->getProviderCollection(false); - $sessionFlagManager ->expects($this->never()) ->method("isNotAuthenticated"); - $voter = $this->getVoter($providerCollection, $sessionFlagManager); + $voter = $this->getVoter(array(), $sessionFlagManager); $this->assertEquals(VoterInterface::ACCESS_ABSTAIN, $voter->vote($token, null, array())); } /** * @test - **/ - public function vote_authenticated_withProviders() + */ + public function vote_authenticatedWithProviders_returnAccessAbstain() { $token = $this->getToken(); - $sessionFlagManager = $this->getSessionFlagManager(); - $sessionFlagManager->setComplete('test', $token); - - $providerCollection = $this->getProviderCollection(); $sessionFlagManager ->expects($this->once()) @@ -119,43 +94,35 @@ public function vote_authenticated_withProviders() ->with('test', $token) ->will($this->returnValue(false)); - $voter = $this->getVoter($providerCollection, $sessionFlagManager); + $voter = $this->getVoter(array("test"), $sessionFlagManager); $this->assertEquals(VoterInterface::ACCESS_ABSTAIN, $voter->vote($token, null, array())); } /** * @test - **/ - public function vote_authenticated_noProviders() + */ + public function vote_authenticatedNoProviders_returnAccessAbstain() { $token = $this->getToken(); - $sessionFlagManager = $this->getSessionFlagManager(); - $sessionFlagManager->setComplete('test', $token); - - $providerCollection = $this->getProviderCollection(false); $sessionFlagManager ->expects($this->never()) ->method("isNotAuthenticated"); - $voter = $this->getVoter($providerCollection, $sessionFlagManager); + $voter = $this->getVoter(array(), $sessionFlagManager); $this->assertEquals(VoterInterface::ACCESS_ABSTAIN, $voter->vote($token, null, array())); } /** * @test - **/ - public function voter_supportsClass() + */ + public function voter_supportsClass_returnTrue() { - $token = $this->getToken(); - $sessionFlagManager = $this->getSessionFlagManager(); - $providerCollection = $this->getProviderCollection(); - - $voter = $this->getVoter($providerCollection, $sessionFlagManager); + $voter = $this->getVoter(array("test"), $sessionFlagManager); $returnValue = $voter->supportsClass('test'); @@ -164,15 +131,11 @@ public function voter_supportsClass() /** * @test - **/ - public function voter_supportsAttribute() + */ + public function voter_supportsAttribute_returnFalse() { - $token = $this->getToken(); - $sessionFlagManager = $this->getSessionFlagManager(); - $providerCollection = $this->getProviderCollection(); - - $voter = $this->getVoter($providerCollection, $sessionFlagManager); + $voter = $this->getVoter(array("test"), $sessionFlagManager); $returnValue = $voter->supportsAttribute('test');