diff --git a/core/application.php b/core/application.php index 373965e7fd..12ec6b63fd 100644 --- a/core/application.php +++ b/core/application.php @@ -27,6 +27,7 @@ namespace OC\Core; use OC\AppFramework\Utility\SimpleContainer; +use OC\AppFramework\Utility\TimeFactory; use \OCP\AppFramework\App; use OC\Core\LostPassword\Controller\LostController; use OC\Core\User\UserController; @@ -63,7 +64,8 @@ class Application extends App { $c->query('SecureRandom'), $c->query('DefaultEmailAddress'), $c->query('IsEncryptionEnabled'), - $c->query('Mailer') + $c->query('Mailer'), + $c->query('TimeFactory') ); }); $container->registerService('UserController', function(SimpleContainer $c) { @@ -120,15 +122,15 @@ class Application extends App { $container->registerService('UserFolder', function(SimpleContainer $c) { return $c->query('ServerContainer')->getUserFolder(); }); - - - $container->registerService('Defaults', function() { return new \OC_Defaults; }); $container->registerService('Mailer', function(SimpleContainer $c) { return $c->query('ServerContainer')->getMailer(); }); + $container->registerService('TimeFactory', function(SimpleContainer $c) { + return new TimeFactory(); + }); $container->registerService('DefaultEmailAddress', function() { return Util::getDefaultEmailAddress('lostpassword-noreply'); }); diff --git a/core/lostpassword/controller/lostcontroller.php b/core/lostpassword/controller/lostcontroller.php index 4c5e58c7b6..7d983bd7e3 100644 --- a/core/lostpassword/controller/lostcontroller.php +++ b/core/lostpassword/controller/lostcontroller.php @@ -28,6 +28,7 @@ namespace OC\Core\LostPassword\Controller; use \OCP\AppFramework\Controller; use \OCP\AppFramework\Http\TemplateResponse; +use OCP\AppFramework\Utility\ITimeFactory; use \OCP\IURLGenerator; use \OCP\IRequest; use \OCP\IL10N; @@ -66,6 +67,8 @@ class LostController extends Controller { protected $secureRandom; /** @var IMailer */ protected $mailer; + /** @var ITimeFactory */ + protected $timeFactory; /** * @param string $appName @@ -79,6 +82,7 @@ class LostController extends Controller { * @param string $from * @param string $isDataEncrypted * @param IMailer $mailer + * @param ITimeFactory $timeFactory */ public function __construct($appName, IRequest $request, @@ -90,7 +94,8 @@ class LostController extends Controller { ISecureRandom $secureRandom, $from, $isDataEncrypted, - IMailer $mailer) { + IMailer $mailer, + ITimeFactory $timeFactory) { parent::__construct($appName, $request); $this->urlGenerator = $urlGenerator; $this->userManager = $userManager; @@ -101,6 +106,7 @@ class LostController extends Controller { $this->isDataEncrypted = $isDataEncrypted; $this->config = $config; $this->mailer = $mailer; + $this->timeFactory = $timeFactory; } /** @@ -173,7 +179,17 @@ class LostController extends Controller { try { $user = $this->userManager->get($userId); - if (!StringUtils::equals($this->config->getUserValue($userId, 'owncloud', 'lostpassword', null), $token)) { + $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')); } @@ -216,12 +232,12 @@ class LostController extends Controller { ISecureRandom::CHAR_DIGITS. ISecureRandom::CHAR_LOWER. ISecureRandom::CHAR_UPPER); - $this->config->setUserValue($user, 'owncloud', 'lostpassword', $token); + $this->config->setUserValue($user, 'owncloud', 'lostpassword', $this->timeFactory->getTime() .':'. $token); $link = $this->urlGenerator->linkToRouteAbsolute('core.lost.resetform', array('userId' => $user, 'token' => $token)); $tmpl = new \OC_Template('core/lostpassword', 'email'); - $tmpl->assign('link', $link, false); + $tmpl->assign('link', $link); $msg = $tmpl->fetchPage(); try { diff --git a/tests/core/lostpassword/controller/lostcontrollertest.php b/tests/core/lostpassword/controller/lostcontrollertest.php index f82fc1ba3f..0f8cb4fc5c 100644 --- a/tests/core/lostpassword/controller/lostcontrollertest.php +++ b/tests/core/lostpassword/controller/lostcontrollertest.php @@ -1,9 +1,22 @@ - * This file is licensed under the Affero General Public License version 3 or - * later. - * See the COPYING-README file. + * @author Lukas Reschke + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * */ namespace OC\Core\LostPassword\Controller; @@ -47,6 +60,8 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase { ->disableOriginalConstructor()->getMock(); $this->container['SecureRandom'] = $this->getMockBuilder('\OCP\Security\ISecureRandom') ->disableOriginalConstructor()->getMock(); + $this->container['TimeFactory'] = $this->getMockBuilder('\OCP\AppFramework\Utility\ITimeFactory') + ->disableOriginalConstructor()->getMock(); $this->container['IsEncryptionEnabled'] = true; $this->lostController = $this->container['LostController']; } @@ -116,6 +131,10 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase { ->method('userExists') ->with('ExistingUser') ->will($this->returnValue(true)); + $this->container['TimeFactory'] + ->expects($this->once()) + ->method('getTime') + ->will($this->returnValue(12348)); $this->container['Config'] ->expects($this->once()) ->method('getUserValue') @@ -128,7 +147,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase { $this->container['Config'] ->expects($this->once()) ->method('setUserValue') - ->with('ExistingUser', 'owncloud', 'lostpassword', 'ThisIsMaybeANotSoSecretToken!'); + ->with('ExistingUser', 'owncloud', 'lostpassword', '12348:ThisIsMaybeANotSoSecretToken!'); $this->container['URLGenerator'] ->expects($this->once()) ->method('linkToRouteAbsolute') @@ -190,7 +209,11 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase { $this->container['Config'] ->expects($this->once()) ->method('setUserValue') - ->with('ExistingUser', 'owncloud', 'lostpassword', 'ThisIsMaybeANotSoSecretToken!'); + ->with('ExistingUser', 'owncloud', 'lostpassword', '12348:ThisIsMaybeANotSoSecretToken!'); + $this->container['TimeFactory'] + ->expects($this->once()) + ->method('getTime') + ->will($this->returnValue(12348)); $this->container['URLGenerator'] ->expects($this->once()) ->method('linkToRouteAbsolute') @@ -256,9 +279,13 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase { ->expects($this->once()) ->method('getUserValue') ->with('ValidTokenUser', 'owncloud', 'lostpassword', null) - ->will($this->returnValue('TheOnlyAndOnlyOneTokenToResetThePassword')); + ->will($this->returnValue('12345:TheOnlyAndOnlyOneTokenToResetThePassword')); $user = $this->getMockBuilder('\OCP\IUser') ->disableOriginalConstructor()->getMock(); + $user + ->expects($this->once()) + ->method('getLastLogin') + ->will($this->returnValue(12344)); $user->expects($this->once()) ->method('setPassword') ->with('NewPassword') @@ -272,12 +299,94 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase { ->expects($this->once()) ->method('deleteUserValue') ->with('ValidTokenUser', 'owncloud', 'lostpassword'); + $this->container['TimeFactory'] + ->expects($this->once()) + ->method('getTime') + ->will($this->returnValue(12348)); $response = $this->lostController->setPassword('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser', 'NewPassword', true); $expectedResponse = array('status' => 'success'); $this->assertSame($expectedResponse, $response); } + public function testSetPasswordExpiredToken() { + $this->container['Config'] + ->expects($this->once()) + ->method('getUserValue') + ->with('ValidTokenUser', 'owncloud', 'lostpassword', null) + ->will($this->returnValue('12345:TheOnlyAndOnlyOneTokenToResetThePassword')); + $user = $this->getMockBuilder('\OCP\IUser') + ->disableOriginalConstructor()->getMock(); + $this->container['UserManager'] + ->expects($this->once()) + ->method('get') + ->with('ValidTokenUser') + ->will($this->returnValue($user)); + $this->container['TimeFactory'] + ->expects($this->once()) + ->method('getTime') + ->will($this->returnValue(55546)); + + $response = $this->lostController->setPassword('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser', 'NewPassword', true); + $expectedResponse = [ + 'status' => 'error', + 'msg' => 'Couldn\'t reset password because the token is expired', + ]; + $this->assertSame($expectedResponse, $response); + } + + public function testSetPasswordInvalidDataInDb() { + $this->container['Config'] + ->expects($this->once()) + ->method('getUserValue') + ->with('ValidTokenUser', 'owncloud', 'lostpassword', null) + ->will($this->returnValue('TheOnlyAndOnlyOneTokenToResetThePassword')); + $user = $this->getMockBuilder('\OCP\IUser') + ->disableOriginalConstructor()->getMock(); + $this->container['UserManager'] + ->expects($this->once()) + ->method('get') + ->with('ValidTokenUser') + ->will($this->returnValue($user)); + + $response = $this->lostController->setPassword('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser', 'NewPassword', true); + $expectedResponse = [ + 'status' => 'error', + 'msg' => 'Couldn\'t reset password because the token is invalid', + ]; + $this->assertSame($expectedResponse, $response); + } + + public function testSetPasswordExpiredTokenDueToLogin() { + $this->container['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(12346)); + $this->container['UserManager'] + ->expects($this->once()) + ->method('get') + ->with('ValidTokenUser') + ->will($this->returnValue($user)); + $this->container['TimeFactory'] + ->expects($this->once()) + ->method('getTime') + ->will($this->returnValue(12345)); + + $response = $this->lostController->setPassword('TheOnlyAndOnlyOneTokenToResetThePassword', 'ValidTokenUser', 'NewPassword', true); + $expectedResponse = [ + 'status' => 'error', + 'msg' => 'Couldn\'t reset password because the token is expired', + ]; + $this->assertSame($expectedResponse, $response); + } + public function testIsSetPasswordWithoutTokenFailing() { $this->container['Config'] ->expects($this->once())