New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tests for password resetter and tweak process a bit. #13523
Conversation
will your changes also fix #13519? |
@sgiehl I haven't tested, but I believe so, will add to description. |
plugins/Login/PasswordResetter.php
Outdated
@@ -274,7 +280,7 @@ public function generatePasswordResetToken($user, $expiryTimestamp = null) | |||
|
|||
$expiry = strftime('%Y%m%d%H', $expiryTimestamp); | |||
$token = $this->generateSecureHash( | |||
$expiry . $user['login'] . $user['email'], | |||
$expiry . $user['login'] . $user['email'] . $user['ts_password_modified'] . $resetTime, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to include something random here? like Common::getRandomString()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I can use a random string instead of the reset time.
Verified will fix #13519 |
7fb6380
to
d65159d
Compare
use Piwik\AuthResult; | ||
use Piwik\Container\StaticContainer; | ||
use Piwik\Mail; | ||
use Piwik\Plugins\Cloud\tests\Framework\TestCase\IntegrationTestCase; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's the wrong one
d65159d
to
f9636cd
Compare
Updated. |
Tests are still failing on travis |
…t variables during tests).
plugins/Login/PasswordResetter.php
Outdated
@@ -175,11 +176,12 @@ public function initiatePasswordResetProcess($loginOrEmail, $newPassword) | |||
|
|||
$login = $user['login']; | |||
|
|||
$this->savePasswordResetInfo($login, $newPassword); | |||
$keySuffix = time() . Common::getRandomString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@diosmosis I reckon the only thing I'd change is to use Common::getRandomString($len = 32)
just to have even more randomness. Otherwise looks good to me and it worked 👍
TODO:
Fixes #13519
Fixes #13520