Refactored the update of the hashed password and canonical fields

The responsibility of updating these fields is moved to dedicated services instead of being in the UserManager, allowing to inject them in the Doctrine listener and the validator initializer without any issue about circular dependencies.
FriendsOfSymfony · Sep 26, 2014 · 3bc6d57 · 3bc6d57
1 parent 417bc5e
commit 3bc6d57
Show file tree

Hide file tree

Showing 16 changed files with 327 additions and 299 deletions.
diff --git a/Doctrine/UserListener.php b/Doctrine/UserListener.php
@@ -16,6 +16,8 @@
 use Doctrine\Common\Persistence\ObjectManager;
 use Doctrine\ODM\MongoDB\DocumentManager;
 use Doctrine\ORM\EntityManager;
+use FOS\UserBundle\Util\CanonicalFieldsUpdater;
+use FOS\UserBundle\Util\PasswordUpdater;
 use Symfony\Component\DependencyInjection\ContainerInterface;
 use FOS\UserBundle\Model\UserInterface;
 
@@ -27,24 +29,13 @@
  */
 class UserListener implements EventSubscriber
 {
-    /**
-     * @var \FOS\UserBundle\Model\UserManagerInterface
-     */
-    private $userManager;
-
-    /**
-     * @var ContainerInterface
-     */
-    private $container;
+    private $passwordUpdater;
+    private $canonicalFieldsUpdater;
 
-    /**
-     * Constructor
-     *
-     * @param ContainerInterface $container
-     */
-    public function __construct(ContainerInterface $container)
+    public function __construct(PasswordUpdater $passwordUpdater, CanonicalFieldsUpdater $canonicalFieldsUpdater)
     {
-        $this->container = $container;
+        $this->passwordUpdater = $passwordUpdater;
+        $this->canonicalFieldsUpdater = $canonicalFieldsUpdater;
     }
 
     public function getSubscribedEvents()
@@ -90,12 +81,8 @@ public function preUpdate(LifecycleEventArgs $args)
      */
     private function updateUserFields(UserInterface $user)
     {
-        if (null === $this->userManager) {
-            $this->userManager = $this->container->get('fos_user.user_manager');
-        }
-
-        $this->userManager->updateCanonicalFields($user);
-        $this->userManager->updatePassword($user);
+        $this->canonicalFieldsUpdater->updateCanonicalFields($user);
+        $this->passwordUpdater->hashPassword($user);
     }
 
     /**

diff --git a/Doctrine/UserManager.php b/Doctrine/UserManager.php
@@ -14,8 +14,8 @@
 use Doctrine\Common\Persistence\ObjectManager;
 use FOS\UserBundle\Model\UserInterface;
 use FOS\UserBundle\Model\UserManager as BaseUserManager;
-use FOS\UserBundle\Util\CanonicalizerInterface;
-use Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface;
+use FOS\UserBundle\Util\CanonicalFieldsUpdater;
+use FOS\UserBundle\Util\PasswordUpdater;
 
 class UserManager extends BaseUserManager
 {
@@ -26,15 +26,14 @@ class UserManager extends BaseUserManager
     /**
      * Constructor.
      *
-     * @param EncoderFactoryInterface $encoderFactory
-     * @param CanonicalizerInterface  $usernameCanonicalizer
-     * @param CanonicalizerInterface  $emailCanonicalizer
+     * @param PasswordUpdater         $passwordUpdater
+     * @param CanonicalFieldsUpdater  $canonicalFieldsUpdater
      * @param ObjectManager           $om
      * @param string                  $class
      */
-    public function __construct(EncoderFactoryInterface $encoderFactory, CanonicalizerInterface $usernameCanonicalizer, CanonicalizerInterface $emailCanonicalizer, ObjectManager $om, $class)
+    public function __construct(PasswordUpdater $passwordUpdater, CanonicalFieldsUpdater $canonicalFieldsUpdater, ObjectManager $om, $class)
     {
-        parent::__construct($encoderFactory, $usernameCanonicalizer, $emailCanonicalizer);
+        parent::__construct($passwordUpdater, $canonicalFieldsUpdater);
 
         $this->objectManager = $om;
         $this->repository = $om->getRepository($class);

diff --git a/Model/UserManager.php b/Model/UserManager.php
@@ -11,8 +11,9 @@
 
 namespace FOS\UserBundle\Model;
 
+use FOS\UserBundle\Util\CanonicalFieldsUpdater;
 use FOS\UserBundle\Util\CanonicalizerInterface;
-use Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface;
+use FOS\UserBundle\Util\PasswordUpdater;
 use Symfony\Component\Security\Core\Exception\UnsupportedUserException;
 use Symfony\Component\Security\Core\Exception\UsernameNotFoundException;
 use Symfony\Component\Security\Core\User\UserInterface as SecurityUserInterface;
@@ -26,33 +27,13 @@
  */
 abstract class UserManager implements UserManagerInterface, UserProviderInterface
 {
-    /**
-     * @var EncoderFactoryInterface 
-     */
-    protected $encoderFactory;
-
-    /**
-     * @var CanonicalizerInterface 
-     */
-    protected $usernameCanonicalizer;
-
-    /**
-     * @var CanonicalizerInterface
-     */
-    protected $emailCanonicalizer;
+    private $passwordUpdater;
+    private $canonicalFieldsUpdater;
 
-    /**
-     * Constructor.
-     *
-     * @param EncoderFactoryInterface $encoderFactory
-     * @param CanonicalizerInterface  $usernameCanonicalizer
-     * @param CanonicalizerInterface  $emailCanonicalizer
-     */
-    public function __construct(EncoderFactoryInterface $encoderFactory, CanonicalizerInterface $usernameCanonicalizer, CanonicalizerInterface $emailCanonicalizer)
+    public function __construct(PasswordUpdater $passwordUpdater, CanonicalFieldsUpdater $canonicalFieldsUpdater)
     {
-        $this->encoderFactory = $encoderFactory;
-        $this->usernameCanonicalizer = $usernameCanonicalizer;
-        $this->emailCanonicalizer = $emailCanonicalizer;
+        $this->passwordUpdater = $passwordUpdater;
+        $this->canonicalFieldsUpdater = $canonicalFieldsUpdater;
     }
 
     /**
@@ -77,7 +58,7 @@ public function createUser()
      */
     public function findUserByEmail($email)
     {
-        return $this->findUserBy(array('emailCanonical' => $this->canonicalizeEmail($email)));
+        return $this->findUserBy(array('emailCanonical' => $this->canonicalFieldsUpdater->canonicalizeEmail($email)));
     }
 
     /**
@@ -89,7 +70,7 @@ public function findUserByEmail($email)
      */
     public function findUserByUsername($username)
     {
-        return $this->findUserBy(array('usernameCanonical' => $this->canonicalizeUsername($username)));
+        return $this->findUserBy(array('usernameCanonical' => $this->canonicalFieldsUpdater->canonicalizeUsername($username)));
     }
 
     /**
@@ -185,49 +166,15 @@ public function loadUserByUsername($username)
      */
     public function updateCanonicalFields(UserInterface $user)
     {
-        $user->setUsernameCanonical($this->canonicalizeUsername($user->getUsername()));
-        $user->setEmailCanonical($this->canonicalizeEmail($user->getEmail()));
+        $this->canonicalFieldsUpdater->updateCanonicalFields($user);
     }
 
     /**
      * {@inheritDoc}
      */
     public function updatePassword(UserInterface $user)
     {
-        if (0 !== strlen($password = $user->getPlainPassword())) {
-            $encoder = $this->getEncoder($user);
-            $user->setPassword($encoder->encodePassword($password, $user->getSalt()));
-            $user->eraseCredentials();
-        }
-    }
-
-    /**
-     * Canonicalizes an email
-     *
-     * @param string $email
-     *
-     * @return string
-     */
-    protected function canonicalizeEmail($email)
-    {
-        return $this->emailCanonicalizer->canonicalize($email);
-    }
-
-    /**
-     * Canonicalizes a username
-     *
-     * @param string $username
-     *
-     * @return string
-     */
-    protected function canonicalizeUsername($username)
-    {
-        return $this->usernameCanonicalizer->canonicalize($username);
-    }
-
-    protected function getEncoder(UserInterface $user)
-    {
-        return $this->encoderFactory->getEncoder($user);
+        $this->passwordUpdater->hashPassword($user);
     }
 
     /**

diff --git a/Propel/UserManager.php b/Propel/UserManager.php
@@ -13,8 +13,8 @@
 
 use FOS\UserBundle\Model\UserInterface;
 use FOS\UserBundle\Model\UserManager as BaseUserManager;
-use FOS\UserBundle\Util\CanonicalizerInterface;
-use Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface;
+use FOS\UserBundle\Util\CanonicalFieldsUpdater;
+use FOS\UserBundle\Util\PasswordUpdater;
 
 class UserManager extends BaseUserManager
 {
@@ -23,14 +23,13 @@ class UserManager extends BaseUserManager
     /**
      * Constructor.
      *
-     * @param EncoderFactoryInterface $encoderFactory
-     * @param CanonicalizerInterface  $usernameCanonicalizer
-     * @param CanonicalizerInterface  $emailCanonicalizer
+     * @param PasswordUpdater         $passwordUpdater
+     * @param CanonicalFieldsUpdater  $canonicalFieldsUpdater
      * @param string                  $class
      */
-    public function __construct(EncoderFactoryInterface $encoderFactory, CanonicalizerInterface $usernameCanonicalizer, CanonicalizerInterface $emailCanonicalizer, $class)
+    public function __construct(PasswordUpdater $passwordUpdater, CanonicalFieldsUpdater $canonicalFieldsUpdater, $class)
     {
-        parent::__construct($encoderFactory, $usernameCanonicalizer, $emailCanonicalizer);
+        parent::__construct($passwordUpdater, $canonicalFieldsUpdater);
 
         $this->class = $class;
     }

diff --git a/Resources/config/doctrine.xml b/Resources/config/doctrine.xml
@@ -6,9 +6,8 @@
 
     <services>
         <service id="fos_user.user_manager.default" class="FOS\UserBundle\Doctrine\UserManager" public="false">
-            <argument type="service" id="security.encoder_factory" />
-            <argument type="service" id="fos_user.util.username_canonicalizer" />
-            <argument type="service" id="fos_user.util.email_canonicalizer" />
+            <argument type="service" id="fos_user.util.password_updater" />
+            <argument type="service" id="fos_user.util.canonical_fields_updater" />
             <argument type="service" id="fos_user.object_manager" />
             <argument>%fos_user.model.user.class%</argument>
         </service>
@@ -18,7 +17,8 @@
         </service>
 
         <service id="fos_user.user_listener" class="FOS\UserBundle\Doctrine\UserListener" public="false">
-            <argument type="service" id="service_container" />
+            <argument type="service" id="fos_user.util.password_updater" />
+            <argument type="service" id="fos_user.util.canonical_fields_updater" />
         </service>
     </services>
 

diff --git a/Resources/config/propel.xml b/Resources/config/propel.xml
@@ -6,9 +6,8 @@
 
     <services>
         <service id="fos_user.user_manager.default" class="FOS\UserBundle\Propel\UserManager" public="false">
-            <argument type="service" id="security.encoder_factory" />
-            <argument type="service" id="fos_user.util.username_canonicalizer" />
-            <argument type="service" id="fos_user.util.email_canonicalizer" />
+            <argument type="service" id="fos_user.util.password_updater" />
+            <argument type="service" id="fos_user.util.canonical_fields_updater" />
             <argument>%fos_user.model.user.class%</argument>
         </service>
     </services>

diff --git a/Resources/config/util.xml b/Resources/config/util.xml
@@ -16,6 +16,16 @@
             <argument type="service" id="logger" on-invalid="ignore" />
         </service>
 
+        <service id="fos_user.util.password_updater" class="FOS\UserBundle\Util\PasswordUpdater" public="false">
+            <argument type="service" id="security.encoder_factory" />
+        </service>
+
+        <service id="fos_user.util.canonical_fields_updater" class="FOS\UserBundle\Util\CanonicalFieldsUpdater" public="false">
+            <argument type="service" id="fos_user.util.username_canonicalizer" />
+            <argument type="service" id="fos_user.util.email_canonicalizer" />
+        </service>
+
+
     </services>
 
 </container>
diff --git a/Resources/config/validator.xml b/Resources/config/validator.xml
@@ -7,7 +7,7 @@
     <services>
         <service id="fos_user.validator.initializer" class="FOS\UserBundle\Validator\Initializer" public="false">
             <tag name="validator.initializer" />
-            <argument type="service" id="fos_user.user_manager" />
+            <argument type="service" id="fos_user.util.canonical_fields_updater" />
         </service>
     </services>
 

diff --git a/Tests/Doctrine/UserManagerTest.php b/Tests/Doctrine/UserManagerTest.php
@@ -22,8 +22,12 @@ public function setUp()
             $this->markTestSkipped('Doctrine Common has to be installed for this test to run.');
         }
 
-        $c = $this->getMock('FOS\UserBundle\Util\CanonicalizerInterface');
-        $ef = $this->getMock('Symfony\Component\Security\Core\Encoder\EncoderFactoryInterface');
+        $passwordUpdater = $this->getMockBuilder('FOS\UserBundle\Util\PasswordUpdater')
+            ->disableOriginalConstructor()
+            ->getMock();
+        $fieldsUpdater = $this->getMockBuilder('FOS\UserBundle\Util\CanonicalFieldsUpdater')
+            ->disableOriginalConstructor()
+            ->getMock();
         $class = $this->getMock('Doctrine\Common\Persistence\Mapping\ClassMetadata');
         $this->om = $this->getMock('Doctrine\Common\Persistence\ObjectManager');
         $this->repository = $this->getMock('Doctrine\Common\Persistence\ObjectRepository');
@@ -40,7 +44,7 @@ public function setUp()
             ->method('getName')
             ->will($this->returnValue(static::USER_CLASS));
 
-        $this->userManager = $this->createUserManager($ef, $c, $this->om, static::USER_CLASS);
+        $this->userManager = $this->createUserManager($passwordUpdater, $fieldsUpdater, $this->om, static::USER_CLASS);
     }
 
     public function testDeleteUser()
@@ -81,9 +85,9 @@ public function testUpdateUser()
         $this->userManager->updateUser($user);
     }
 
-    protected function createUserManager($encoderFactory, $canonicalizer, $objectManager, $userClass)
+    protected function createUserManager($passwordUpdater, $fieldsUpdater, $objectManager, $userClass)
     {
-        return new UserManager($encoderFactory, $canonicalizer, $canonicalizer, $objectManager, $userClass);
+        return new UserManager($passwordUpdater, $fieldsUpdater, $objectManager, $userClass);
     }
 
     protected function getUser()