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

Merged
merged 5 commits into from Oct 12, 2018

Conversation

Projects
None yet
3 participants
@diosmosis
Member

diosmosis commented Oct 3, 2018

TODO:

  • manual test

Fixes #13519
Fixes #13520

@diosmosis diosmosis added this to the 3.6.1 milestone Oct 3, 2018

@sgiehl

This comment has been minimized.

Member

sgiehl commented Oct 3, 2018

will your changes also fix #13519?

@diosmosis

This comment has been minimized.

Member

diosmosis commented Oct 3, 2018

@sgiehl I haven't tested, but I believe so, will add to description.

@@ -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,

This comment has been minimized.

@tsteur

tsteur Oct 3, 2018

Member

Is it possible to include something random here? like Common::getRandomString()?

This comment has been minimized.

@diosmosis

diosmosis Oct 4, 2018

Member

👍 I can use a random string instead of the reset time.

@diosmosis

This comment has been minimized.

Member

diosmosis commented Oct 4, 2018

Verified will fix #13519

@diosmosis diosmosis force-pushed the reset-password-changes branch from 7fb6380 to d65159d Oct 4, 2018

use Piwik\AuthResult;
use Piwik\Container\StaticContainer;
use Piwik\Mail;
use Piwik\Plugins\Cloud\tests\Framework\TestCase\IntegrationTestCase;

This comment has been minimized.

@sgiehl

sgiehl Oct 9, 2018

Member

that's the wrong one

@diosmosis diosmosis force-pushed the reset-password-changes branch from d65159d to f9636cd Oct 9, 2018

@diosmosis

This comment has been minimized.

Member

diosmosis commented Oct 9, 2018

Updated.

@sgiehl

This comment has been minimized.

Member

sgiehl commented Oct 11, 2018

Tests are still failing on travis

@@ -175,11 +176,12 @@ public function initiatePasswordResetProcess($loginOrEmail, $newPassword)
$login = $user['login'];
$this->savePasswordResetInfo($login, $newPassword);
$keySuffix = time() . Common::getRandomString();

This comment has been minimized.

@tsteur

tsteur Oct 12, 2018

Member

@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 👍

@diosmosis diosmosis merged commit 8ba828c into 3.x-dev Oct 12, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@diosmosis diosmosis deleted the reset-password-changes branch Oct 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment