diff options
author | diosmosis <diosmosis@users.noreply.github.com> | 2018-10-12 05:32:00 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2018-10-12 05:32:00 +0300 |
commit | 8ba828c91e9b6fb337989935648d710128bb645a (patch) | |
tree | dae1a22b07d3b44a21ffdc92d9efe8bb2ae54100 /plugins/Login | |
parent | 66a7797be7229e4a9ccba042d4028442e994310e (diff) |
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
Diffstat (limited to 'plugins/Login')
-rw-r--r-- | plugins/Login/PasswordResetter.php | 64 | ||||
-rw-r--r-- | plugins/Login/lang/en.json | 2 | ||||
-rw-r--r-- | plugins/Login/tests/Integration/PasswordResetterTest.php | 157 |
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 |