Internal: Improve code for automated password encryption change - refs GH#4063

main/auth/profile.php

@@ -426,10 +426,11 @@ if ($form->validate()) {
         api_get_setting('profile', 'email') == 'true')
     ) {
         $passwordWasChecked = true;
-        $validPassword = UserManager::isPasswordValid(
+        $validPassword = UserManager::checkPassword(
             $user->getPassword(),
             $user_data['password0'],
-            $user->getSalt()
+            $user->getSalt(),
+            $user->getId()
         );
 
         if ($validPassword) {

main/inc/lib/usermanager.lib.php

@@ -127,14 +127,13 @@ class UserManager
     }
 
     /**
-     * Validates the password.
-     *
-     * @param $encoded
-     * @param $salt
+     * Detects and returns the type of encryption of the given encrypted
+     * password.
      *
-     * @return bool
+     * @param string $encoded The encrypted password
+     * @param string $salt The user salt, if any
      */
-    public static function detectPasswordEncryption($encoded, $salt)
+    public static function detectPasswordEncryption(string $encoded, string $salt): bool
     {
 
         $encryption = false;
@@ -164,30 +163,41 @@ class UserManager
         return $encryption;
     }
 
-    public static function checkPassword($encoded, $raw, $salt, $userId)
+    /**
+     * Checks if the password is correct for this user.
+     * If the password_conversion setting is true, also update the password
+     * in the database to a new encryption method.
+     *
+     * @param string $encoded  Encrypted password
+     * @param string $raw      Clear password given through login form
+     * @param string $salt     User salt, if any
+     * @param int $userId   The user's internal ID
+     */
+    public static function checkPassword(string $encoded, string $raw, string $salt, int $userId): bool
     {
         $result = false;
 
-        $detectedEncryption = self::detectPasswordEncryption($encoded, $salt);
-        if (api_get_configuration_value('password_conversion') && self::getPasswordEncryption() != $detectedEncryption) {
-            $encoder = new \Chamilo\UserBundle\Security\Encoder($detectedEncryption);
-            $result = $encoder->isPasswordValid($encoded, $raw, $salt);
-            if ($result) {
-                self::updatePassword($userId, $raw);
+        if (true === api_get_configuration_value('password_conversion')) {
+            $detectedEncryption = self::detectPasswordEncryption($encoded, $salt);
+            if (self::getPasswordEncryption() != $detectedEncryption) {
+                $encoder = new \Chamilo\UserBundle\Security\Encoder($detectedEncryption);
+                $result = $encoder->isPasswordValid($encoded, $raw, $salt);
+                if ($result) {
+                    self::updatePassword($userId, $raw);
+                }
             }
-        }
-        else {
+        } else {
             return self::isPasswordValid($encoded, $raw, $salt);
         }
 
         return $result;
     }
     /**
-     * @param string $raw
+     * Encrypt the password using the current encoder
      *
-     * @return string
+     * @param string $raw The clear password
      */
-    public static function encryptPassword($raw, User $user)
+    public static function encryptPassword(string $raw, User $user): string
     {
         $encoder = self::getEncoder($user);
 
@@ -198,10 +208,11 @@ class UserManager
     }
 
     /**
-     * @param int    $userId
-     * @param string $password
+     * Update the password of the given user to the given (in-clear) password
+     * @param int    $userId    Internal user ID
+     * @param string $password  Password in clear
      */
-    public static function updatePassword($userId, $password)
+    public static function updatePassword(int $userId, string $password): void
     {
         $repository = self::getRepository();
         /** @var User $user */

main/inc/local.inc.php

@@ -511,18 +511,11 @@ if (!empty($_SESSION['_user']['user_id']) && !($login || $logout)) {
             if ($uData['auth_source'] == PLATFORM_AUTH_SOURCE ||
                 $uData['auth_source'] == CAS_AUTH_SOURCE
             ) {
-                /*
-                $validPassword = isset($password) && UserManager::isPasswordValid(
-                    $uData['password'],
-                    $password,
-                    $uData['salt']
-                );
-                */
                 $validPassword = isset($password) && UserManager::checkPassword(
                         $uData['password'],
                         $password,
                         $uData['salt'],
-                        $uData['user_id']
+                        $uData['id']
                     );
 
                 $checkUserFromExternalWebservice = false;

main/install/configuration.dist.php

@@ -163,8 +163,9 @@ $_configuration['cdn'] = [
 $_configuration['security_key'] = '{SECURITY_KEY}';
 // Hash function method
 $_configuration['password_encryption'] = '{ENCRYPT_PASSWORD}';
-// allow to convert passwords after login if password_encryption has changed since last login
-$_configuration['password_conversion'] = false;
+// Set to true to allow automated password conversion after login if
+// password_encryption has changed since last login. See GH#4063 for details.
+//$_configuration['password_conversion'] = false;
 // You may have to restart your web server if you change this
 $_configuration['session_stored_in_db'] = false;
 // Session lifetime

plugin/whispeakauth/ajax/authentify_password.php

@@ -42,7 +42,7 @@ $lpItemInfo = ChamiloSession::read(WhispeakAuthPlugin::SESSION_LP_ITEM, []);
 /** @var array $quizQuestionInfo */
 $quizQuestionInfo = ChamiloSession::read(WhispeakAuthPlugin::SESSION_QUIZ_QUESTION, []);
 
-$isValidPassword = UserManager::isPasswordValid($user->getPassword(), $password, $user->getSalt());
+$isValidPassword = UserManager::checkPassword($user->getPassword(), $password, $user->getSalt(), $user->getId());
 $isActive = $user->isActive();
 $isExpired = empty($user->getExpirationDate()) || $user->getExpirationDate() > api_get_utc_datetime(null, false, true);