Revert registry back to the old implementation to fix circular refere…

…nce exception
scheb · Dec 28, 2015 · b8887b0 · b8887b0
1 parent b0da3a8
commit b8887b0
Show file tree

Hide file tree

Showing 9 changed files with 91 additions and 194 deletions.
diff --git 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
@@ -6,7 +6,6 @@
 		<parameter key="scheb_two_factor.trusted_cookie_manager.class">Scheb\TwoFactorBundle\Security\TwoFactor\Trusted\TrustedCookieManager</parameter>
 		<parameter key="scheb_two_factor.trusted_token_generator.class">Scheb\TwoFactorBundle\Security\TwoFactor\Trusted\TrustedTokenGenerator</parameter>
 		<parameter key="scheb_two_factor.trusted_filter.class">Scheb\TwoFactorBundle\Security\TwoFactor\Trusted\TrustedFilter</parameter>
-		<parameter key="scheb_two_factor.provider_collection.class">Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderCollection</parameter>
 		<parameter key="scheb_two_factor.provider_registry.class">Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderRegistry</parameter>
 		<parameter key="scheb_two_factor.backup_code_validator.class">Scheb\TwoFactorBundle\Security\TwoFactor\Backup\BackupCodeValidator</parameter>
 		<parameter key="scheb_two_factor.security_voter.class">Scheb\TwoFactorBundle\Security\TwoFactor\Voter</parameter>
@@ -32,18 +31,16 @@
 			<argument>%scheb_two_factor.trusted_computer.enabled%</argument>
 			<argument>%scheb_two_factor.parameter_names.trusted%</argument>
 		</service>
-		<service id="scheb_two_factor.provider_collection" class="%scheb_two_factor.provider_collection.class%">
-		</service>
 		<service id="scheb_two_factor.provider_registry" class="%scheb_two_factor.provider_registry.class%">
 			<argument type="service" id="scheb_two_factor.session_flag_manager" />
-			<argument type="service" id="scheb_two_factor.provider_collection" />
+			<argument></argument> <!-- Two-factor providers -->
 		</service>
 		<service id="scheb_two_factor.backup_code_validator" class="%scheb_two_factor.backup_code_validator.class%">
 			<argument type="service" id="scheb_two_factor.persister.doctrine" />
 		</service>
 		<service id="scheb_two_factor.security_voter" class="%scheb_two_factor.security_voter.class%">
 			<argument type="service" id="scheb_two_factor.session_flag_manager" />
-			<argument type="service" id="scheb_two_factor.provider_collection" />
+			<argument></argument> <!-- Two-factor provider ids -->
 			<tag name="security.voter" />
 		</service>
 	</services>

diff --git a/Security/TwoFactor/Provider/TwoFactorProviderCollection.php b/Security/TwoFactor/Provider/TwoFactorProviderCollection.php
diff --git 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
@@ -1,15 +1,10 @@
 <?php
-
 namespace Scheb\TwoFactorBundle\Security\TwoFactor;
 
 use Symfony\Component\Security\Core\Authorization\Voter\VoterInterface;
 use Symfony\Component\Security\Core\Authentication\Token\TokenInterface;
 use Scheb\TwoFactorBundle\Security\TwoFactor\Session\SessionFlagManager;
-use Scheb\TwoFactorBundle\Security\TwoFactor\Provider\TwoFactorProviderCollection;
 
-/**
- * Class Voter
- */
 class Voter implements VoterInterface
 {
     /**
@@ -18,25 +13,23 @@ class Voter implements VoterInterface
     protected $sessionFlagManager;
 
     /**
-     * @var TwoFactorProviderCollection
+     * @var string[]
      **/
-    protected $providerCollection;
+    protected $providers;
 
     /**
-     * __construct
-     * @param SessionFlagManager          $sessionFlagManager
-     * @param TwoFactorProviderCollection $providers
-     * @return void
-     **/
-    public function __construct(SessionFlagManager $sessionFlagManager, TwoFactorProviderCollection $providerCollection)
+     * @param SessionFlagManager $sessionFlagManager
+     * @param string[] $providers
+     */
+    public function __construct(SessionFlagManager $sessionFlagManager, array $providers)
     {
         $this->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
@@ -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,25 +40,33 @@ 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));
     }
 
     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,24 +92,29 @@ 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);
     }
 
     /**
      * @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