diff options
author | diosmosis <diosmosis@users.noreply.github.com> | 2020-05-17 22:57:41 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2020-05-17 22:57:41 +0300 |
commit | d89c2b7fb4b295542204cd5d81c35f79b5c93489 (patch) | |
tree | 69cc8c39f3723db0b06af42ffd3d1788509d7b77 | |
parent | 40ba87f59b8134204c537ea9dc40040c8b0d467f (diff) |
Password confirmation fix for custom login plugins. (#15945)
* Password confirmation fix for custom login plugins.
* Throw exception if password confirm is incorrect.
* Update API.php
* apply review feedback
* update submodule
* fix tests
* update screenshot and fix test
* update submodule
m--------- | misc/log-analytics | 0 | ||||
-rw-r--r-- | plugins/Login/PasswordVerifier.php | 2 | ||||
-rw-r--r-- | plugins/TwoFactorAuth/tests/Integration/TwoFactorAuthTest.php | 18 | ||||
-rw-r--r-- | plugins/UsersManager/API.php | 31 | ||||
-rw-r--r-- | plugins/UsersManager/tests/System/ApiTest.php | 23 | ||||
-rw-r--r-- | tests/UI/expected-screenshots/UIIntegrationTest_api_listing.png | 4 |
6 files changed, 34 insertions, 44 deletions
diff --git a/misc/log-analytics b/misc/log-analytics -Subproject 7266244870722c912a00b0ac51b85aa788bd624 +Subproject 099e2bbf61de1c9c5d24e41793259d1127d0477 diff --git a/plugins/Login/PasswordVerifier.php b/plugins/Login/PasswordVerifier.php index d95ba17804..33e267b1ef 100644 --- a/plugins/Login/PasswordVerifier.php +++ b/plugins/Login/PasswordVerifier.php @@ -189,7 +189,7 @@ class PasswordVerifier $sessionNamespace->setExpirationSeconds(self::VERIFY_VALID_FOR_MINUTES * 60 * 5, 'redirectParams'); if ($this->enableRedirect) { - Piwik::redirectToModule('Login', 'confirmPassword'); + Piwik::redirectToModule(Piwik::getLoginPluginName(), 'confirmPassword'); } } } diff --git a/plugins/TwoFactorAuth/tests/Integration/TwoFactorAuthTest.php b/plugins/TwoFactorAuth/tests/Integration/TwoFactorAuthTest.php index 1df67e1e36..78a6c2f055 100644 --- a/plugins/TwoFactorAuth/tests/Integration/TwoFactorAuthTest.php +++ b/plugins/TwoFactorAuth/tests/Integration/TwoFactorAuthTest.php @@ -73,20 +73,22 @@ class TwoFactorAuthTest extends IntegrationTestCase { $token = Request::processRequest('UsersManager.createAppSpecificTokenAuth', array( 'userLogin' => $this->userWithout2Fa, - 'md5Password' => md5($this->userPassword), + 'passwordConfirmation' => $this->userPassword, 'description' => 'twofa test' )); $this->assertEquals(32, strlen($token)); } - public function test_onCreateAppSpecificTokenAuth_returnsRandomTokenWhenNotAuthenticatedEvenWhen2FAenabled() + public function test_onCreateAppSpecificTokenAuth_failsWhenNotAuthenticatedEvenWhen2FAenabled() { - $token = Request::processRequest('UsersManager.createAppSpecificTokenAuth', array( + $this->expectException(\Exception::class); + $this->expectExceptionMessage('UsersManager_CurrentPasswordNotCorrect'); + + Request::processRequest('UsersManager.createAppSpecificTokenAuth', array( 'userLogin' => $this->userWith2Fa, - 'md5Password' => md5('invalidPAssword'), + 'passwordConfirmation' => 'invalidPAssword', 'description' => 'twofa test' )); - $this->assertEquals(32, strlen($token)); } public function test_onCreateAppSpecificTokenAuth_throwsErrorWhenMissingTokenWhenUsing2FaAndAuthenticatedCorrectly() @@ -97,7 +99,7 @@ class TwoFactorAuthTest extends IntegrationTestCase Request::processRequest('UsersManager.createAppSpecificTokenAuth', array( 'userLogin' => $this->userWith2Fa, - 'md5Password' => md5($this->userPassword), + 'passwordConfirmation' => $this->userPassword, 'description' => 'twofa test' )); } @@ -110,7 +112,7 @@ class TwoFactorAuthTest extends IntegrationTestCase $_GET['authCode'] = '111222'; Request::processRequest('UsersManager.createAppSpecificTokenAuth', array( 'userLogin' => $this->userWith2Fa, - 'md5Password' => md5($this->userPassword), + 'passwordConfirmation' => $this->userPassword, 'description' => 'twofa test' )); } @@ -120,7 +122,7 @@ class TwoFactorAuthTest extends IntegrationTestCase $_GET['authCode'] = $this->generateValidAuthCode($this->user2faSecret); $token = Request::processRequest('UsersManager.createAppSpecificTokenAuth', array( 'userLogin' => $this->userWith2Fa, - 'md5Password' => md5($this->userPassword), + 'passwordConfirmation' => $this->userPassword, 'description' => 'twofa test' )); $this->assertEquals(32, strlen($token)); diff --git a/plugins/UsersManager/API.php b/plugins/UsersManager/API.php index 3bbf50b42b..697dfa5566 100644 --- a/plugins/UsersManager/API.php +++ b/plugins/UsersManager/API.php @@ -699,7 +699,8 @@ class API extends \Piwik\Plugin\API * @param string $userLogin the user login. * @param bool|int $hasSuperUserAccess true or '1' to grant Super User access, false or '0' to remove Super User * access. - * @param string $passwordConfirmation the current user's password. + * @param string $passwordConfirmation the current user's password. For security purposes, this value should be + * sent as a POST parameter. * @throws \Exception */ public function setSuperUserAccess($userLogin, $hasSuperUserAccess, $passwordConfirmation = null) @@ -1297,7 +1298,8 @@ class API extends \Piwik\Plugin\API * If the username/password combination is incorrect an invalid token will be returned. * * @param string $userLogin Login or Email address - * @param string $md5Password hashed string of the password (using current hash function; MD5-named for historical reasons) + * @param string $passwordConfirmation the current user's password. For security purposes, this value should be + * sent as a POST parameter. * @param string $description The description for this app specific password, for example your app name. Max 100 characters are allowed * @param string $expireDate Optionally a date when the token should expire * @param string $expireHours Optionally number of hours for how long the token should be valid before it expires. @@ -1305,10 +1307,8 @@ class API extends \Piwik\Plugin\API * If expireDate is set and expireHours, then expireDate will be used. * @return string */ - public function createAppSpecificTokenAuth($userLogin, $md5Password, $description, $expireDate = null, $expireHours = 0) + public function createAppSpecificTokenAuth($userLogin, $passwordConfirmation, $description, $expireDate = null, $expireHours = 0) { - UsersManager::checkPasswordHash($md5Password, Piwik::translate('UsersManager_ExceptionPasswordMD5HashExpected')); - $user = $this->model->getUser($userLogin); if (empty($user) && Piwik::isValidEmailString($userLogin)) { $user = $this->model->getUserByEmail($userLogin); @@ -1317,19 +1317,16 @@ class API extends \Piwik\Plugin\API } } - if (empty($user) || !$this->password->verify($md5Password, $user['password'])) { - /** - * @ignore - * @internal - */ - Piwik::postEvent('Login.authenticate.failed', array($userLogin)); - - return md5($userLogin . microtime(true) . Common::generateUniqId()); - } + if (empty($user) || !$this->passwordVerifier->isPasswordCorrect($userLogin, $passwordConfirmation)) { + if (empty($user)) { + /** + * @ignore + * @internal + */ + Piwik::postEvent('Login.authenticate.failed', array($userLogin)); + } - if ($this->password->needsRehash($user['password'])) { - $userUpdater = new UserUpdater(); - $userUpdater->updateUserWithoutCurrentPassword($userLogin, $this->password->hash($md5Password)); + throw new \Exception(Piwik::translate('UsersManager_CurrentPasswordNotCorrect')); } if (empty($expireDate) && !empty($expireHours) && is_numeric($expireHours)) { diff --git a/plugins/UsersManager/tests/System/ApiTest.php b/plugins/UsersManager/tests/System/ApiTest.php index a4d6bbb74a..e96dbf9016 100644 --- a/plugins/UsersManager/tests/System/ApiTest.php +++ b/plugins/UsersManager/tests/System/ApiTest.php @@ -86,7 +86,7 @@ class ApiTest extends SystemTestCase public function test_createAppSpecificTokenAuth() { $this->model->deleteAllTokensForUser('login1'); - $token = $this->api->createAppSpecificTokenAuth('login1', md5('password'), 'test'); + $token = $this->api->createAppSpecificTokenAuth('login1', 'password', 'test'); $this->assertMd5($token); $user = $this->model->getUserByTokenAuth($token); @@ -96,35 +96,26 @@ class ApiTest extends SystemTestCase public function test_createAppSpecificTokenAuth_canLoginByEmail() { $this->model->deleteAllTokensForUser('login1'); - $token = $this->api->createAppSpecificTokenAuth('login1@example.com', md5('password'), 'test'); + $token = $this->api->createAppSpecificTokenAuth('login1@example.com', 'password', 'test'); $this->assertMd5($token); $user = $this->model->getUserByTokenAuth($token); $this->assertSame('login1', $user['login']); } - public function test_createAppSpecificTokenAuth_notValidPasswordFormat() + public function test_createAppSpecificTokenAuth_failsWhenPasswordNotValid() { $this->expectException(\Exception::class); - $this->expectExceptionMessage('is expecting a MD5-hashed password'); + $this->expectExceptionMessage('The current password you entered is not correct.'); - $this->api->createAppSpecificTokenAuth('login1', 'foobar', 'test'); - } - - public function test_createAppSpecificTokenAuth_generatesRandomTokenWhenPasswordNotValid() - { $this->model->deleteAllTokensForUser('login1'); - $token = $this->api->createAppSpecificTokenAuth('login1', md5('foooooo'), 'test'); - $this->assertMd5($token); - - $user = $this->model->getUserByTokenAuth($token); - $this->assertEmpty($user); + $this->api->createAppSpecificTokenAuth('login1', 'foooooo', 'test'); } public function test_createAppSpecificTokenAuth_withExpireDate() { $this->model->deleteAllTokensForUser('login1'); - $token = $this->api->createAppSpecificTokenAuth('login1', md5('password'), 'test', '2026-01-02 03:04:05'); + $token = $this->api->createAppSpecificTokenAuth('login1', 'password', 'test', '2026-01-02 03:04:05'); $this->assertMd5($token); $tokens = $this->model->getAllNonSystemTokensForLogin('login1'); @@ -138,7 +129,7 @@ class ApiTest extends SystemTestCase { $expireInHours = 48; $this->model->deleteAllTokensForUser('login1'); - $token = $this->api->createAppSpecificTokenAuth('login1', md5('password'), 'test', null, $expireInHours); + $token = $this->api->createAppSpecificTokenAuth('login1', 'password', 'test', null, $expireInHours); $this->assertMd5($token); $tokens = $this->model->getAllNonSystemTokensForLogin('login1'); diff --git a/tests/UI/expected-screenshots/UIIntegrationTest_api_listing.png b/tests/UI/expected-screenshots/UIIntegrationTest_api_listing.png index 1e4f05b359..52b44fffcd 100644 --- a/tests/UI/expected-screenshots/UIIntegrationTest_api_listing.png +++ b/tests/UI/expected-screenshots/UIIntegrationTest_api_listing.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:5ce395b12330146726479d60455d3f5ed943375520c79ef79a739aecb8dd69fc -size 4942045 +oid sha256:229a00c7fa3c345e213fd3a1f46072fa2e40868300934c8686d152863b1c4c67 +size 4941994 |