Merge branch '3.x-dev' into 4.x-dev

7 files changed, 134 insertions, 43 deletions

diff --git a/plugins/Login/Controller.php b/plugins/Login/Controller.php
index aa26ed0406..a340368d41 100644
--- a/plugins/Login/Controller.php
+++ b/plugins/Login/Controller.php
@@ -9,6 +9,8 @@
 namespace Piwik\Plugins\Login;
 
 use Exception;
+use Piwik\Access;
+use Piwik\Auth\Password;
 use Piwik\Common;
 use Piwik\Config;
 use Piwik\Container\StaticContainer;
@@ -18,6 +20,7 @@ use Piwik\Nonce;
 use Piwik\Piwik;
 use Piwik\Plugins\Login\Security\BruteForceDetection;
 use Piwik\Plugins\UsersManager\Model AS UsersModel;
+use Piwik\Plugins\UsersManager\UserUpdater;
 use Piwik\QuickForm2;
 use Piwik\Session;
 use Piwik\Url;
@@ -30,6 +33,8 @@ use Piwik\View;
  */
 class Controller extends \Piwik\Plugin\ControllerAdmin
 {
+    const NONCE_CONFIRMRESETPASSWORD = 'loginConfirmResetPassword';
+
     /**
      * @var PasswordResetter
      */
@@ -424,25 +429,49 @@ class Controller extends \Piwik\Plugin\ControllerAdmin
      */
     public function confirmResetPassword()
     {
+        if (!Url::isValidHost()) {
+            throw new Exception("Cannot confirm reset password with untrusted hostname!");
+        }
+
         $errorMessage = null;
+        $passwordHash = null;
 
-        $login = Common::getRequestVar('login', '');
-        $resetToken = Common::getRequestVar('resetToken', '');
+        $login = Common::getRequestVar('login');
+        $resetToken = Common::getRequestVar('resetToken');
 
         try {
-            $this->passwordResetter->confirmNewPassword($login, $resetToken);
+            $passwordHash = $this->passwordResetter->checkValidConfirmPasswordToken($login, $resetToken);
         } catch (Exception $ex) {
             Log::debug($ex);
 
             $errorMessage = $ex->getMessage();
         }
 
-        if (is_null($errorMessage)) { // if success, show login w/ success message
-            return $this->resetPasswordSuccess();
-        } else {
-            // show login page w/ error. this will keep the token in the URL
+        if (!empty($errorMessage)) {
             return $this->login($errorMessage);
         }
+
+        if (!empty($_POST['nonce'])
+            && !empty($_POST['mtmpasswordconfirm'])
+            && !empty($resetToken)
+            && !empty($login)
+            && !empty($passwordHash)
+            && empty($errorMessage)) {
+            Nonce::checkNonce(self::NONCE_CONFIRMRESETPASSWORD, $_POST['nonce']);
+            if ($this->passwordResetter->doesResetPasswordHashMatchesPassword($_POST['mtmpasswordconfirm'], $passwordHash)) {
+                $this->passwordResetter->setHashedPasswordForLogin($login, $passwordHash);
+                return $this->resetPasswordSuccess();
+            } else {
+                $errorMessage = Piwik::translate('Login_ConfirmPasswordResetWrongPassword');
+            }
+        }
+
+        $nonce = Nonce::getNonce(self::NONCE_CONFIRMRESETPASSWORD);
+
+        return $this->renderTemplateAs('confirmResetPassword', array(
+            'nonce' => $nonce,
+            'errorMessage' => $errorMessage
+        ), 'basic');
     }
 
     /**
@@ -452,6 +481,7 @@ class Controller extends \Piwik\Plugin\ControllerAdmin
      */
     public function resetPasswordSuccess()
     {
+        $_POST = array(); // prevent showing error message username and password is missing
         return $this->login($errorMessage = null, $infoMessage = Piwik::translate('Login_PasswordChanged'));
     }
 
diff --git a/plugins/Login/PasswordResetter.php b/plugins/Login/PasswordResetter.php
index e9ce0d75f6..9c706440f9 100644
--- a/plugins/Login/PasswordResetter.php
+++ b/plugins/Login/PasswordResetter.php
@@ -191,20 +191,7 @@ class PasswordResetter
         }
     }
 
-    /**
-     * Confirms a password reset. This should be called after {@link initiatePasswordResetProcess()}
-     * is called.
-     *
-     * This method will get the new password associated with a reset token and set it
-     * as the specified user's password.
-     *
-     * @param string $login The login of the user whose password is being reset.
-     * @param string $resetToken The generated string token contained in the reset password
-     *                           email.
-     * @throws Exception If there is no user with login '$login', if $resetToken is not a
-     *                   valid token or if the token has expired.
-     */
-    public function confirmNewPassword($login, $resetToken)
+    public function checkValidConfirmPasswordToken($login, $resetToken)
     {
         // get password reset info & user info
         $user = self::getUserInformation($login);
@@ -224,15 +211,32 @@ class PasswordResetter
 
         // check that the stored password hash is valid (sanity check)
         $resetPassword = $resetInfo['hash'];
+
         $this->checkPasswordHash($resetPassword);
 
-        // reset password of user
-        $usersManager = $this->usersManagerApi;
-        Access::doAsSuperUser(function () use ($usersManager, $user, $resetPassword) {
+        return $resetPassword;
+    }
+
+    /**
+     * Confirms a password reset. This should be called after {@link initiatePasswordResetProcess()}
+     * is called.
+     *
+     * This method will get the new password associated with a reset token and set it
+     * as the specified user's password.
+     *
+     * @param string $login The login of the user whose password is being reset.
+     * @param string $passwordHash The generated string token contained in the reset password
+     *                           email.
+     * @throws Exception If there is no user with login '$login', if $resetToken is not a
+     *                   valid token or if the token has expired.
+     */
+    public function setHashedPasswordForLogin($login, $passwordHash)
+    {
+        Access::doAsSuperUser(function () use ($login, $passwordHash) {
             $userUpdater = new UserUpdater();
             $userUpdater->updateUserWithoutCurrentPassword(
-                $user['login'],
-                $resetPassword,
+                $login,
+                $passwordHash,
                 $email = false,
                 $isPasswordHashed = true
             );
@@ -293,6 +297,12 @@ class PasswordResetter
         return $token;
     }
 
+    public function doesResetPasswordHashMatchesPassword($passwordPlain, $passwordHash)
+    {
+        $passwordPlain = UsersManager::getPasswordHash($passwordPlain);
+        return $this->passwordHelper->verify($passwordPlain, $passwordHash);
+    }
+
     /**
      * Generates a hash using a hash "identifier" and some data to hash. The hash identifier is
      * a string that differentiates the hash in some way.
diff --git a/plugins/Login/lang/en.json b/plugins/Login/lang/en.json
index d13dffbca8..8b72632048 100644
--- a/plugins/Login/lang/en.json
+++ b/plugins/Login/lang/en.json
@@ -20,6 +20,9 @@
         "SettingBruteForceMaxFailedLoginsHelp": "If more than this number of failed logins are recorded within the time range configured below, block the IP.",
         "SettingBruteForceTimeRange": "Count login retries within this time range in minutes",
         "SettingBruteForceTimeRangeHelp": "Enter a number in minutes.",
+        "ConfirmPasswordReset": "Confirm password reset",
+        "ConfirmPasswordResetIntro": "To confirm it is really you who requested this password change, please enter your new password again.",
+        "ConfirmPasswordResetWrongPassword": "The entered password does not match your new password. If you don't remember your newly chosen password you can reset your password again. If you didn't request the password change, simply do nothing and your password won't be changed.",
         "LoginNotAllowedBecauseBlocked": "You are currently not allowed to log in because you had too many failed logins, try again later.",
         "CurrentlyBlockedIPs": "Currently blocked IPs",
         "IPsAlwaysBlocked": "These IPs are always blocked",
diff --git a/plugins/Login/templates/confirmResetPassword.twig b/plugins/Login/templates/confirmResetPassword.twig
new file mode 100644
index 0000000000..4abb1b82dd
--- /dev/null
+++ b/plugins/Login/templates/confirmResetPassword.twig
@@ -0,0 +1,45 @@
+{% extends '@Login/loginLayout.twig' %}
+
+{% set title %}{{ 'Login_ConfirmPasswordToContinue'|translate }}{% endset %}
+
+{% block loginContent %}
+    <div class="contentForm loginForm confirmPasswordForm">
+        {% embed 'contentBlock.twig' with {'title': ('Login_ConfirmPasswordReset'|translate)} %}
+        {% block content %}
+            <p>{{ 'Login_ConfirmPasswordResetIntro'|translate }}</p>
+
+            <div class="message_container">
+                {% if errorMessage is not empty %}
+                    <div piwik-notification
+                         noclear="true"
+                         context="error">
+                        <strong>{{ 'General_Error'|translate }}</strong>: {{ errorMessage }}<br/>
+                    </div>
+                {% endif %}
+            </div>
+            <br>
+
+            <form action="{{ linkTo({'module': 'Login', 'action': 'confirmResetPassword'}) }}" ng-non-bindable method="post">
+                <div class="row">
+                    <div class="col s12 input-field">
+                        <input type="hidden" name="nonce" value="{{ nonce }}"/>
+                        <input type="password" placeholder="" name="mtmpasswordconfirm" id="mtmpasswordconfirm" class="input" value="" size="20"
+                               autocorrect="off" autocapitalize="none"
+                               tabindex="20" />
+                        <label for="mtmpasswordconfirm"><i class="icon-locked icon"></i> {{ 'Login_NewPassword'|translate }}</label>
+                    </div>
+                </div>
+
+                <div class="row actions">
+                    <div class="col s12">
+                        <input class="submit btn" id='login_reset_confirm' type="submit" value="{{ 'General_Confirm'|translate }}"
+                               tabindex="100"/>
+                    </div>
+                </div>
+
+            </form>
+        {% endblock %}
+        {% endembed %}
+    </div>
+
+{% endblock %}
\ No newline at end of file
diff --git a/plugins/Login/tests/Integration/PasswordResetterTest.php b/plugins/Login/tests/Integration/PasswordResetterTest.php
index fe6f3f0b31..b600d031c4 100644
--- a/plugins/Login/tests/Integration/PasswordResetterTest.php
+++ b/plugins/Login/tests/Integration/PasswordResetterTest.php
@@ -54,19 +54,7 @@ class PasswordResetterTest extends IntegrationTestCase
 
     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->passwordResetter->setHashedPasswordForLogin('superUserLogin', $this->capturedToken);
 
         $this->checkPasswordIs(self::NEWPASSWORD);
     }
@@ -118,6 +106,9 @@ class PasswordResetterTest extends IntegrationTestCase
 
         Option::set($optionName, json_encode($data));
 
+        $this->assertTrue($this->passwordResetter->doesResetPasswordHashMatchesPassword(self::NEWPASSWORD, $data['hash']));
+        $this->assertFalse($this->passwordResetter->doesResetPasswordHashMatchesPassword('foobar', $data['hash']));
+
         $this->passwordResetter->initiatePasswordResetProcess('superUserLogin', self::NEWPASSWORD);
 
         $optionName = $this->passwordResetter->getPasswordResetInfoOptionName('superUserLogin');
@@ -134,7 +125,7 @@ class PasswordResetterTest extends IntegrationTestCase
         $this->passwordResetter->initiatePasswordResetProcess('superUserLogin', self::NEWPASSWORD);
         $this->assertNotEmpty($this->capturedToken);
 
-        $this->passwordResetter->confirmNewPassword('superUserLogin', $this->capturedToken);
+        $this->passwordResetter->checkValidConfirmPasswordToken('superUserLogin', $this->capturedToken);
         $this->checkPasswordIs(self::NEWPASSWORD);
 
         sleep(1);
@@ -143,7 +134,7 @@ class PasswordResetterTest extends IntegrationTestCase
         $this->passwordResetter->initiatePasswordResetProcess('superUserLogin', 'anotherpassword');
         $this->assertNotEquals($oldCapturedToken, $this->capturedToken);
 
-        $this->passwordResetter->confirmNewPassword('superUserLogin', $oldCapturedToken);
+        $this->passwordResetter->checkValidConfirmPasswordToken('superUserLogin', $oldCapturedToken);
     }
 
     public function test_passwordReset_shouldNeverGenerateTheSameToken()
@@ -172,7 +163,7 @@ class PasswordResetterTest extends IntegrationTestCase
         $this->passwordResetter->initiatePasswordResetProcess('superUserLogin', self::NEWPASSWORD);
         $this->assertNotEquals($oldCapturedToken, $this->capturedToken);
 
-        $this->passwordResetter->confirmNewPassword('superUserLogin', $oldCapturedToken);
+        $this->passwordResetter->checkValidConfirmPasswordToken('superUserLogin', $oldCapturedToken);
     }
 
     /**
diff --git a/plugins/Login/tests/UI/Login_spec.js b/plugins/Login/tests/UI/Login_spec.js
index 28f4020b20..89fc353559 100644
--- a/plugins/Login/tests/UI/Login_spec.js
+++ b/plugins/Login/tests/UI/Login_spec.js
@@ -149,7 +149,7 @@ describe("Login", function () {
         expect(await page.screenshot({ fullPage: true })).to.matchImage('password_reset');
     });
 
-    it("should reset password when password reset link is clicked", async function() {
+    it("should show reset password confirmation page when password reset link is clicked", async function() {
         var expectedMailOutputFile = PIWIK_INCLUDE_PATH + '/tmp/Login.resetPassword.mail.json',
             fileContents = require("fs").readFileSync(expectedMailOutputFile),
             mailSent = JSON.parse(fileContents),
@@ -163,6 +163,15 @@ describe("Login", function () {
         await page.goto(resetUrl);
         await page.waitForNetworkIdle();
 
+        expect(await page.screenshot({ fullPage: true })).to.matchImage('password_reset_confirm');
+    });
+
+    it("should reset password when password reset link is clicked", async function() {
+
+        await page.type("#mtmpasswordconfirm", "superUserPass2");
+        await page.click("#login_reset_confirm");
+        await page.waitForNetworkIdle();
+
         expect(await page.screenshot({ fullPage: true })).to.matchImage('password_reset_complete');
     });
 
diff --git a/plugins/Login/tests/UI/expected-screenshots/Login_password_reset_confirm.png b/plugins/Login/tests/UI/expected-screenshots/Login_password_reset_confirm.png
new file mode 100644
index 0000000000..5cbabdf2f9
--- /dev/null
+++ b/plugins/Login/tests/UI/expected-screenshots/Login_password_reset_confirm.png
@@ -0,0 +1,3 @@
+version https://git-lfs.github.com/spec/v1
+oid sha256:ec46909c1a4f613f345285313d8b3b41fc446d123bddc096a2d45759f2c14c0b
+size 34444