Add tests for password resetter and tweak process a bit. (#13523)

* Add tests for password resetter and tweak process a bit.

* Add random string to reset key suffix.

* Tweak email message.

* Fixing tests (includes change to always use latest testing environment variables during tests).

* Tweak to randomstring

3 files changed, 197 insertions, 26 deletions

diff --git a/plugins/Login/PasswordResetter.php b/plugins/Login/PasswordResetter.php
index 730004b634..e3f6bcde57 100644
--- a/plugins/Login/PasswordResetter.php
+++ b/plugins/Login/PasswordResetter.php
@@ -16,6 +16,7 @@ use Piwik\IP;
 use Piwik\Mail;
 use Piwik\Option;
 use Piwik\Piwik;
+use Piwik\Plugins\UsersManager\Model;
 use Piwik\Plugins\UsersManager\UsersManager;
 use Piwik\Plugins\UsersManager\API as UsersManagerAPI;
 use Piwik\SettingsPiwik;
@@ -175,11 +176,12 @@ class PasswordResetter
 
         $login = $user['login'];
 
-        $this->savePasswordResetInfo($login, $newPassword);
+        $keySuffix = time() . Common::getRandomString($length = 32);
+        $this->savePasswordResetInfo($login, $newPassword, $keySuffix);
 
         // ... send email with confirmation link
         try {
-            $this->sendEmailConfirmationLink($user);
+            $this->sendEmailConfirmationLink($user, $keySuffix);
         } catch (Exception $ex) {
             // remove password reset info
             $this->removePasswordResetInfo($login);
@@ -210,14 +212,17 @@ class PasswordResetter
         }
 
         // check that the reset token is valid
-        $resetPassword = $this->getPasswordToResetTo($login);
-        if ($resetPassword === false
-            || !$this->isTokenValid($resetToken, $user)
+        $resetInfo = $this->getPasswordToResetTo($login);
+        if ($resetInfo === false
+            || empty($resetInfo['hash'])
+            || empty($resetInfo['keySuffix'])
+            || !$this->isTokenValid($resetToken, $user, $resetInfo['keySuffix'])
         ) {
             throw new Exception(Piwik::translate('Login_InvalidOrExpiredToken'));
         }
 
         // check that the stored password hash is valid (sanity check)
+        $resetPassword = $resetInfo['hash'];
         $this->checkPasswordHash($resetPassword);
 
         // reset password of user
@@ -234,15 +239,16 @@ class PasswordResetter
      *
      * @param string $token The reset token to check.
      * @param array $user The user information returned by the UsersManager API.
+     * @param string $keySuffix The suffix used in generating a token.
      * @return bool true if valid, false otherwise.
      */
-    public function isTokenValid($token, $user)
+    public function isTokenValid($token, $user, $keySuffix)
     {
         $now = time();
 
         // token valid for 24 hrs (give or take, due to the coarse granularity in our strftime format string)
         for ($i = 0; $i <= 24; $i++) {
-            $generatedToken = $this->generatePasswordResetToken($user, $now + $i * 60 * 60);
+            $generatedToken = $this->generatePasswordResetToken($user, $keySuffix, $now + $i * 60 * 60);
             if ($generatedToken === $token) {
                 return true;
             }
@@ -258,11 +264,12 @@ class PasswordResetter
      * The reset token is generated using a user's email, login and the time when the token expires.
      *
      * @param array $user The user information.
+     * @param string $keySuffix The suffix used in generating a token.
      * @param int|null $expiryTimestamp The expiration timestamp to use or null to generate one from
      *                                  the current timestamp.
      * @return string The generated token.
      */
-    public function generatePasswordResetToken($user, $expiryTimestamp = null)
+    public function generatePasswordResetToken($user, $keySuffix, $expiryTimestamp = null)
     {
         /*
          * Piwik does not store the generated password reset token.
@@ -274,7 +281,7 @@ class PasswordResetter
 
         $expiry = strftime('%Y%m%d%H', $expiryTimestamp);
         $token = $this->generateSecureHash(
-            $expiry . $user['login'] . $user['email'],
+            $expiry . $user['login'] . $user['email'] . $user['ts_password_modified'] . $keySuffix,
             $user['password']
         );
         return $token;
@@ -371,16 +378,15 @@ class PasswordResetter
      */
     protected function getUserInformation($loginOrMail)
     {
-        $usersManager = $this->usersManagerApi;
-        return Access::doAsSuperUser(function () use ($loginOrMail, $usersManager) {
-            $user = null;
-            if ($usersManager->userExists($loginOrMail)) {
-                $user = $usersManager->getUser($loginOrMail);
-            } else if ($usersManager->userEmailExists($loginOrMail)) {
-                $user = $usersManager->getUserByEmail($loginOrMail);
-            }
-            return $user;
-        });
+        $userModel = new Model();
+
+        $user = null;
+        if ($userModel->userExists($loginOrMail)) {
+            $user = $userModel->getUser($loginOrMail);
+        } else if ($userModel->userEmailExists($loginOrMail)) {
+            $user = $userModel->getUserByEmail($loginOrMail);
+        }
+        return $user;
     }
 
     /**
@@ -406,14 +412,15 @@ class PasswordResetter
      * Sends email confirmation link for a password reset request.
      *
      * @param array $user User info for the requested password reset.
+     * @param string $keySuffix The suffix used in generating a token.
      */
-    private function sendEmailConfirmationLink($user)
+    private function sendEmailConfirmationLink($user, $keySuffix)
     {
         $login = $user['login'];
         $email = $user['email'];
 
         // construct a password reset token from user information
-        $resetToken = $this->generatePasswordResetToken($user);
+        $resetToken = $this->generatePasswordResetToken($user, $keySuffix);
 
         $confirmPasswordModule = $this->confirmPasswordModule;
         $confirmPasswordAction = $this->confirmPasswordAction;
@@ -430,7 +437,7 @@ class PasswordResetter
         $bodyText = str_replace(
                 '\n',
                 "\n",
-                sprintf(Piwik::translate('Login_MailPasswordChangeBody'), $login, $ip, $url)
+                Piwik::translate('Login_MailPasswordChangeBody2', [$login, $ip, $url])
             ) . "\n";
         $mail->setBodyText($bodyText);
 
@@ -448,11 +455,16 @@ class PasswordResetter
      *
      * @param string $login The user login for whom a password change was requested.
      * @param string $newPassword The new password to set.
+     * @param string $keySuffix The suffix used in generating a token.
      */
-    private function savePasswordResetInfo($login, $newPassword)
+    private function savePasswordResetInfo($login, $newPassword, $keySuffix)
     {
         $optionName = $this->getPasswordResetInfoOptionName($login);
-        $optionData = $this->passwordHelper->hash(UsersManager::getPasswordHash($newPassword));
+        $optionData = [
+            'hash' => $this->passwordHelper->hash(UsersManager::getPasswordHash($newPassword)),
+            'keySuffix' => $keySuffix,
+        ];
+        $optionData = json_encode($optionData);
 
         Option::set($optionName, $optionData);
     }
@@ -466,7 +478,9 @@ class PasswordResetter
     private function getPasswordToResetTo($login)
     {
         $optionName = self::getPasswordResetInfoOptionName($login);
-        return Option::get($optionName);
+        $optionValue = Option::get($optionName);
+        $optionValue = json_decode($optionValue, $isAssoc = true);
+        return $optionValue;
     }
 
     /**
diff --git a/plugins/Login/lang/en.json b/plugins/Login/lang/en.json
index cfa860cee3..48f06f20b0 100644
--- a/plugins/Login/lang/en.json
+++ b/plugins/Login/lang/en.json
@@ -13,7 +13,7 @@
         "LoginPasswordNotCorrect": "Wrong Username and password combination.",
         "LostYourPassword": "Lost your password?",
         "ChangeYourPassword": "Change your password",
-        "MailPasswordChangeBody": "Hi %1$s,\n\nA password reset request was received from %2$s. To confirm this password change so you can login with your new credentials, visit the following link:\n\n%3$s\n\nAttention: Changing the password will also change your token_auth. You can look up your new token_auth on your settings page.\n\nIf you are using your API token_auth in any external applications or for archiving, make sure to update the token_auth as requests to the API will fail otherwise.\n\nNote: this link will expire in 24 hours.\n\nAnd thank you for using Matomo!",
+        "MailPasswordChangeBody2": "Hi %1$s,\n\nA password reset request was received from %2$s. To confirm this password change so you can login with your new credentials, please copy and paste the following link in your browser:\n\n%3$s\n\nNote: this link will expire in 24 hours.\n\nAnd thank you for using Matomo!",
         "MailTopicPasswordChange": "Confirm Password Change",
         "NewPassword": "New password",
         "NewPasswordRepeat": "New password (repeat)",
diff --git a/plugins/Login/tests/Integration/PasswordResetterTest.php b/plugins/Login/tests/Integration/PasswordResetterTest.php
new file mode 100644
index 0000000000..27196b3ae4
--- /dev/null
+++ b/plugins/Login/tests/Integration/PasswordResetterTest.php
@@ -0,0 +1,157 @@
+<?php
+/**
+ * Piwik - free/libre analytics platform
+ *
+ * @link http://piwik.org
+ * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ */
+
+namespace Piwik\Plugins\Login\tests\Integration;
+
+
+use Piwik\Access;
+use Piwik\Auth;
+use Piwik\Container\StaticContainer;
+use Piwik\Mail;
+use Piwik\Plugin\Manager;
+use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
+use Piwik\Plugins\Login\PasswordResetter;
+use Piwik\Plugins\UsersManager\Model;
+use Piwik\Tests\Framework\Fixture;
+
+class PasswordResetterTest extends IntegrationTestCase
+{
+    const NEWPASSWORD = 'newpassword';
+
+    /**
+     * @var Model
+     */
+    private $userModel;
+
+    /**
+     * @var string
+     */
+    private $capturedToken;
+
+    /**
+     * @var PasswordResetter
+     */
+    private $passwordResetter;
+
+    public function setUp()
+    {
+        parent::setUp();
+        $this->passwordResetter = new PasswordResetter();
+        $this->userModel = new Model();
+        $this->capturedToken = null;
+
+        Manager::getInstance()->loadPluginTranslations();
+    }
+
+    public function test_passwordReset_processWorksAsExpected()
+    {
+        $user = $this->userModel->getUser('superUserLogin');
+        $password = $user['password'];
+        $passwordModified = $user['ts_password_modified'];
+
+        $this->passwordResetter->initiatePasswordResetProcess('superUserLogin', self::NEWPASSWORD);
+
+        $this->assertNotEmpty($this->capturedToken);
+
+        $user = $this->userModel->getUser('superUserLogin');
+        $this->assertEquals($password, $user['password']);
+        $this->assertEquals($passwordModified, $user['ts_password_modified']);
+
+        $this->passwordResetter->confirmNewPassword('superUserLogin', $this->capturedToken);
+
+        $this->checkPasswordIs(self::NEWPASSWORD);
+    }
+
+    /**
+     * @expectedException \Exception
+     * @expectedExceptionMessage Token is invalid or has expired
+     */
+    public function test_passwordReset_shouldNotAllowTokenToBeUsedMoreThanOnce()
+    {
+        $this->passwordResetter->initiatePasswordResetProcess('superUserLogin', self::NEWPASSWORD);
+        $this->assertNotEmpty($this->capturedToken);
+
+        $this->passwordResetter->confirmNewPassword('superUserLogin', $this->capturedToken);
+        $this->checkPasswordIs(self::NEWPASSWORD);
+
+        sleep(1);
+
+        $oldCapturedToken = $this->capturedToken;
+        $this->passwordResetter->initiatePasswordResetProcess('superUserLogin', 'anotherpassword');
+        $this->assertNotEquals($oldCapturedToken, $this->capturedToken);
+
+        $this->passwordResetter->confirmNewPassword('superUserLogin', $oldCapturedToken);
+    }
+
+    public function test_passwordReset_shouldNeverGenerateTheSameToken()
+    {
+        $this->passwordResetter->initiatePasswordResetProcess('superUserLogin', self::NEWPASSWORD);
+        $this->assertNotEmpty($this->capturedToken);
+
+        sleep(1);
+
+        $oldCapturedToken = $this->capturedToken;
+        $this->passwordResetter->initiatePasswordResetProcess('superUserLogin', self::NEWPASSWORD);
+        $this->assertNotEquals($oldCapturedToken, $this->capturedToken);
+    }
+
+    /**
+     * @expectedException \Exception
+     * @expectedExceptionMessage Token is invalid or has expired
+     */
+    public function test_passwordReset_shouldNotAllowOldTokenToBeUsedAfterAnotherResetRequest()
+    {
+        $this->passwordResetter->initiatePasswordResetProcess('superUserLogin', self::NEWPASSWORD);
+        $this->assertNotEmpty($this->capturedToken);
+
+        sleep(1);
+
+        $oldCapturedToken = $this->capturedToken;
+        $this->passwordResetter->initiatePasswordResetProcess('superUserLogin', self::NEWPASSWORD);
+        $this->assertNotEquals($oldCapturedToken, $this->capturedToken);
+
+        $this->passwordResetter->confirmNewPassword('superUserLogin', $oldCapturedToken);
+    }
+
+    /**
+     * @param Fixture $fixture
+     */
+    protected static function configureFixture($fixture)
+    {
+        parent::configureFixture($fixture);
+        $fixture->createSuperUser = true;
+        $fixture->extraTestEnvVars['loadRealTranslations'] = true;
+    }
+
+    private function checkPasswordIs($pwd)
+    {
+        $auth = StaticContainer::get(Auth::class);
+        $auth->setLogin('superUserLogin');
+        $auth->setPassword($pwd);
+        $auth->setTokenAuth(null);
+        $auth->setPasswordHash(null);
+
+        $result = Access::getInstance()->reloadAccess($auth);
+        $this->assertTrue($result);
+    }
+
+    public function provideContainerConfig()
+    {
+        return [
+            'observers.global' => [
+                ['Mail.send', function (Mail $mail) {
+                    $body = $mail->getBodyText(true);
+                    preg_match('/resetToken=3D([a-zA-Z0-9=\s]+?)=0A/', $body, $matches);
+                    $capturedToken = $matches[1];
+                    $capturedToken = preg_replace('/=\s*/', '', $capturedToken);
+                    $this->capturedToken = $capturedToken;
+                }],
+            ],
+        ];
+    }
+}
\ No newline at end of file