Expire token after 12h and if user logged-in again

As an hardening measure we should expire password reset tokens after 12h and if the user has logged-in again successfully after the token was requested.
2015-08-22 20:42:45 +02:00 · 2015-08-22 20:42:45 +02:00 · db4cb1dd4d
parent 510010e774
commit db4cb1dd4d
3 changed files with 142 additions and 15 deletions
--- 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');
 		});
--- 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 {
--- a/tests/core/lostpassword/controller/lostcontrollertest.php
+++ b/tests/core/lostpassword/controller/lostcontrollertest.php
@ -1,9 +1,22 @@
 <?php
 /**
- * Copyright (c) 2014-2015 Lukas Reschke <lukas@owncloud.com>
- * This file is licensed under the Affero General Public License version 3 or
- * later.
- * See the COPYING-README file.
+ * @author Lukas Reschke <lukas@owncloud.com>
+ *
+ * @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 <http://www.gnu.org/licenses/>
+ *
 */

 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())