Merge pull request #24735 from juliushaertl/passwordreset-invalid

Show error messages if a password reset link is invalid or expired

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

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