From 8ee2cb47d09fbcf7c188d48ab6c840fbffbade95 Mon Sep 17 00:00:00 2001 From: Julius Haertl Date: Thu, 19 May 2016 13:23:12 +0200 Subject: Show error messages if a password reset link is invalid or expired - Moved token validation to method checkPasswordResetToken - Render error with message from exceptions --- core/Controller/LostController.php | 50 ++++++++++++++++++++++++++------------ 1 file changed, 35 insertions(+), 15 deletions(-) diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php index 0e0932b288b..61e29495608 100644 --- a/core/Controller/LostController.php +++ b/core/Controller/LostController.php @@ -121,6 +121,17 @@ class LostController extends Controller { * @return TemplateResponse */ public function resetform($token, $userId) { + try { + $this->checkPasswordResetToken($token, $userId); + } catch (\Exception $e) { + return new TemplateResponse( + 'core', 'error', [ + "errors" => array(array("error" => $e->getMessage())) + ], + 'guest' + ); + } + return new TemplateResponse( 'core', 'lostpassword/resetpassword', @@ -131,6 +142,29 @@ class LostController extends Controller { ); } + /** + * @param string $userId + * @param string $userId + * @throws \Exception + */ + private function checkPasswordResetToken($token, $userId) { + $user = $this->userManager->get($userId); + + $splittedToken = explode(':', $this->config->getUserValue($userId, 'owncloud', 'lostpassword', null)); + if(count($splittedToken) !== 2) { + throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is invalid')); + } + + if ($splittedToken[0] < ($this->timeFactory->getTime() - 60*60*12) || + $user->getLastLogin() > $splittedToken[0]) { + throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is expired')); + } + + if (!StringUtils::equals($splittedToken[1], $token)) { + throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is invalid')); + } + } + /** * @param $message * @param array $additional @@ -178,22 +212,9 @@ class LostController extends Controller { } try { + $this->checkPasswordResetToken($token, $userId); $user = $this->userManager->get($userId); - $splittedToken = explode(':', $this->config->getUserValue($userId, 'owncloud', 'lostpassword', null)); - if(count($splittedToken) !== 2) { - throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is invalid')); - } - - if ($splittedToken[0] < ($this->timeFactory->getTime() - 60*60*12) || - $user->getLastLogin() > $splittedToken[0]) { - throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is expired')); - } - - if (!StringUtils::equals($splittedToken[1], $token)) { - throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is invalid')); - } - if (!$user->setPassword($password)) { throw new \Exception(); } @@ -202,7 +223,6 @@ class LostController extends Controller { $this->config->deleteUserValue($userId, 'owncloud', 'lostpassword'); @\OC_User::unsetMagicInCookie(); - } catch (\Exception $e){ return $this->error($e->getMessage()); } -- cgit v1.2.3 From d06598081471896dde67a15bb02cae49d1548480 Mon Sep 17 00:00:00 2001 From: Julius Haertl Date: Thu, 19 May 2016 22:40:53 +0200 Subject: Add more tests for OC\Core\Controller\LostController - remove testResetFormUnsuccessful as it is now splitted up in different test cases - add testResetFormInvalidToken to check if timestamp and token are present - add testResetFormInvalidTokenMatch to check if the saved token matches the provided - add testResetFormExpiredToken to check if expiration detection works - add testResetFormValidToken to check if detection of valid tokens works --- tests/Core/Controller/LostControllerTest.php | 107 ++++++++++++++++++++++++++- 1 file changed, 104 insertions(+), 3 deletions(-) diff --git a/tests/Core/Controller/LostControllerTest.php b/tests/Core/Controller/LostControllerTest.php index ca63c3404eb..492a04bcfde 100644 --- a/tests/Core/Controller/LostControllerTest.php +++ b/tests/Core/Controller/LostControllerTest.php @@ -114,14 +114,115 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase { ); } - public function testResetFormUnsuccessful() { + public function testResetFormInvalidToken() { $userId = 'admin'; $token = 'MySecretToken'; + $response = $this->lostController->resetform($token, $userId); + $expectedResponse = new TemplateResponse('core', + 'error', + [ + 'errors' => [ + ['error' => 'Couldn\'t reset password because the token is invalid'], + ] + ], + 'guest'); + $this->assertEquals($expectedResponse, $response); + } + + public function testResetFormInvalidTokenMatch() { + $this->config + ->expects($this->once()) + ->method('getUserValue') + ->with('ValidTokenUser', 'owncloud', 'lostpassword', null) + ->will($this->returnValue('12345:TheOnlyAndOnlyOneTokenToResetThePassword')); + $user = $this->getMockBuilder('\OCP\IUser') + ->disableOriginalConstructor()->getMock(); + $user + ->expects($this->once()) + ->method('getLastLogin') + ->will($this->returnValue(12344)); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('ValidTokenUser') + ->will($this->returnValue($user)); + $userId = 'ValidTokenUser'; + $token = '12345:MySecretToken'; + $response = $this->lostController->resetform($token, $userId); + $expectedResponse = new TemplateResponse('core', + 'error', + [ + 'errors' => [ + ['error' => 'Couldn\'t reset password because the token is invalid'], + ] + ], + 'guest'); + $this->assertEquals($expectedResponse, $response); + } + + + public function testResetFormExpiredToken() { + $userId = 'ValidTokenUser'; + $token = '12345:TheOnlyAndOnlyOneTokenToResetThePassword'; + $user = $this->getMockBuilder('\OCP\IUser') + ->disableOriginalConstructor()->getMock(); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('ValidTokenUser') + ->will($this->returnValue($user)); + $this->timeFactory + ->expects($this->once()) + ->method('getTime') + ->will($this->returnValue(12345*60*60*12)); + $userId = 'ValidTokenUser'; + $token = 'TheOnlyAndOnlyOneTokenToResetThePassword'; + $this->config + ->expects($this->once()) + ->method('getUserValue') + ->with('ValidTokenUser', 'owncloud', 'lostpassword', null) + ->will($this->returnValue('12345:TheOnlyAndOnlyOneTokenToResetThePassword')); + $response = $this->lostController->resetform($token, $userId); + $expectedResponse = new TemplateResponse('core', + 'error', + [ + 'errors' => [ + ['error' => 'Couldn\'t reset password because the token is expired'], + ] + ], + 'guest'); + $this->assertEquals($expectedResponse, $response); + } + public function testResetFormValidToken() { + $userId = 'ValidTokenUser'; + $token = '12345:TheOnlyAndOnlyOneTokenToResetThePassword'; + $user = $this->getMockBuilder('\OCP\IUser') + ->disableOriginalConstructor()->getMock(); + $user + ->expects($this->once()) + ->method('getLastLogin') + ->will($this->returnValue(12344)); + $this->userManager + ->expects($this->once()) + ->method('get') + ->with('ValidTokenUser') + ->will($this->returnValue($user)); + $this->timeFactory + ->expects($this->once()) + ->method('getTime') + ->will($this->returnValue(12348)); + $userId = 'ValidTokenUser'; + $token = 'TheOnlyAndOnlyOneTokenToResetThePassword'; + $this->config + ->expects($this->once()) + ->method('getUserValue') + ->with('ValidTokenUser', 'owncloud', 'lostpassword', null) + ->will($this->returnValue('12345:TheOnlyAndOnlyOneTokenToResetThePassword')); $this->urlGenerator ->expects($this->once()) ->method('linkToRouteAbsolute') - ->with('core.lost.setPassword', array('userId' => 'admin', 'token' => 'MySecretToken')) + ->with('core.lost.setPassword', array('userId' => 'ValidTokenUser', 'token' => 'TheOnlyAndOnlyOneTokenToResetThePassword')) ->will($this->returnValue('https://ownCloud.com/index.php/lostpassword/')); $response = $this->lostController->resetform($token, $userId); @@ -329,7 +430,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase { ->with('NewPassword') ->will($this->returnValue(true)); $this->userManager - ->expects($this->once()) + ->expects($this->exactly(2)) ->method('get') ->with('ValidTokenUser') ->will($this->returnValue($user)); -- cgit v1.2.3