Merge pull request #1024 from nextcloud/remove-password-reset-link-after-successful-login

Remove "password reset token" after successful login

4 files changed, 44 insertions, 21 deletions

diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php
index dbc1f3157fd..67e1e215289 100644
--- a/core/Controller/LoginController.php
+++ b/core/Controller/LoginController.php
@@ -237,6 +237,9 @@ class LoginController extends Controller {
 		$this->userSession->login($user, $password);
 		$this->userSession->createSessionToken($this->request, $loginResult->getUID(), $user, $password);
 
+		// User has successfully logged in, now remove the password reset link, when it is available
+		$this->config->deleteUserValue($loginResult->getUID(), 'core', 'lostpassword');
+
 		if ($this->twoFactorManager->isTwoFactorAuthenticated($loginResult)) {
 			$this->twoFactorManager->prepareTwoFactorLogin($loginResult);
 			if (!is_null($redirect_url)) {
diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php
index c3716e9a8e7..fe6be1e6852 100644
--- a/core/Controller/LostController.php
+++ b/core/Controller/LostController.php
@@ -151,7 +151,7 @@ class LostController extends Controller {
 	private function checkPasswordResetToken($token, $userId) {
 		$user = $this->userManager->get($userId);
 
-		$splittedToken = explode(':', $this->config->getUserValue($userId, 'owncloud', 'lostpassword', null));
+		$splittedToken = explode(':', $this->config->getUserValue($userId, 'core', 'lostpassword', null));
 		if(count($splittedToken) !== 2) {
 			throw new \Exception($this->l10n->t('Couldn\'t reset password because the token is invalid'));
 		}
@@ -222,7 +222,7 @@ class LostController extends Controller {
 
 			\OC_Hook::emit('\OC\Core\LostPassword\Controller\LostController', 'post_passwordReset', array('uid' => $userId, 'password' => $password));
 
-			$this->config->deleteUserValue($userId, 'owncloud', 'lostpassword');
+			$this->config->deleteUserValue($userId, 'core', 'lostpassword');
 			@\OC_User::unsetMagicInCookie();
 		} catch (\Exception $e){
 			return $this->error($e->getMessage());
@@ -253,7 +253,7 @@ class LostController extends Controller {
 			ISecureRandom::CHAR_DIGITS.
 			ISecureRandom::CHAR_LOWER.
 			ISecureRandom::CHAR_UPPER);
-		$this->config->setUserValue($user, 'owncloud', 'lostpassword', $this->timeFactory->getTime() .':'. $token);
+		$this->config->setUserValue($user, 'core', 'lostpassword', $this->timeFactory->getTime() .':'. $token);
 
 		$link = $this->urlGenerator->linkToRouteAbsolute('core.lost.resetform', array('userId' => $user, 'token' => $token));
 
diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php
index 8eaa7c9843b..417a60a9e5f 100644
--- a/tests/Core/Controller/LoginControllerTest.php
+++ b/tests/Core/Controller/LoginControllerTest.php
@@ -322,6 +322,8 @@ class LoginControllerTest extends TestCase {
 
 		$this->userSession->expects($this->never())
 			->method('createSessionToken');
+		$this->config->expects($this->never())
+			->method('deleteUserValue');
 
 		$expected = new \OCP\AppFramework\Http\RedirectResponse($loginPageUrl);
 		$this->assertEquals($expected, $this->loginController->tryLogin($user, $password, ''));
@@ -330,6 +332,9 @@ class LoginControllerTest extends TestCase {
 	public function testLoginWithValidCredentials() {
 		/** @var IUser | \PHPUnit_Framework_MockObject_MockObject $user */
 		$user = $this->getMockBuilder('\OCP\IUser')->getMock();
+		$user->expects($this->any())
+			->method('getUID')
+			->will($this->returnValue('uid'));
 		$password = 'secret';
 		$indexPageUrl = \OC_Util::getDefaultPageUrl();
 
@@ -363,6 +368,9 @@ class LoginControllerTest extends TestCase {
 			->method('isTwoFactorAuthenticated')
 			->with($user)
 			->will($this->returnValue(false));
+		$this->config->expects($this->once())
+			->method('deleteUserValue')
+			->with('uid', 'core', 'lostpassword');
 
 		$expected = new \OCP\AppFramework\Http\RedirectResponse($indexPageUrl);
 		$this->assertEquals($expected, $this->loginController->tryLogin($user, $password, null));
@@ -398,6 +406,8 @@ class LoginControllerTest extends TestCase {
 			->method('isLoggedIn')
 			->with()
 			->will($this->returnValue(false));
+		$this->config->expects($this->never())
+			->method('deleteUserValue');
 
 		$expected = new \OCP\AppFramework\Http\RedirectResponse(\OC_Util::getDefaultPageUrl());
 		$this->assertEquals($expected, $this->loginController->tryLogin('Jane', $password, $originalUrl));
@@ -438,6 +448,8 @@ class LoginControllerTest extends TestCase {
 			->method('getAbsoluteURL')
 			->with(urldecode($originalUrl))
 			->will($this->returnValue($redirectUrl));
+		$this->config->expects($this->never())
+			->method('deleteUserValue');
 
 		$expected = new \OCP\AppFramework\Http\RedirectResponse($redirectUrl);
 		$this->assertEquals($expected, $this->loginController->tryLogin('Jane', $password, $originalUrl));
@@ -485,6 +497,9 @@ class LoginControllerTest extends TestCase {
 			->method('getAbsoluteURL')
 			->with(urldecode($originalUrl))
 			->will($this->returnValue($redirectUrl));
+		$this->config->expects($this->once())
+			->method('deleteUserValue')
+			->with('jane', 'core', 'lostpassword');
 
 		$expected = new \OCP\AppFramework\Http\RedirectResponse(urldecode($redirectUrl));
 		$this->assertEquals($expected, $this->loginController->tryLogin('Jane', $password, $originalUrl));
@@ -536,6 +551,9 @@ class LoginControllerTest extends TestCase {
 			->method('linkToRoute')
 			->with('core.TwoFactorChallenge.selectChallenge')
 			->will($this->returnValue($challengeUrl));
+		$this->config->expects($this->once())
+			->method('deleteUserValue')
+			->with('john', 'core', 'lostpassword');
 
 		$expected = new RedirectResponse($challengeUrl);
 		$this->assertEquals($expected, $this->loginController->tryLogin('john@doe.com', $password, null));
@@ -586,6 +604,8 @@ class LoginControllerTest extends TestCase {
 			->expects($this->once())
 			->method('registerAttempt')
 			->with('login', '192.168.0.1', ['user' => 'john@doe.com']);
+		$this->config->expects($this->never())
+			->method('deleteUserValue');
 
 		$expected = new RedirectResponse('');
 		$this->assertEquals($expected, $this->loginController->tryLogin('john@doe.com', 'just wrong', null));
diff --git a/tests/Core/Controller/LostControllerTest.php b/tests/Core/Controller/LostControllerTest.php
index 492a04bcfde..2e7d6721d56 100644
--- a/tests/Core/Controller/LostControllerTest.php
+++ b/tests/Core/Controller/LostControllerTest.php
@@ -133,7 +133,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 		$this->config
 			->expects($this->once())
 			->method('getUserValue')
-			->with('ValidTokenUser', 'owncloud', 'lostpassword', null)
+			->with('ValidTokenUser', 'core', 'lostpassword', null)
 			->will($this->returnValue('12345:TheOnlyAndOnlyOneTokenToResetThePassword'));
 		$user = $this->getMockBuilder('\OCP\IUser')
 			->disableOriginalConstructor()->getMock();
@@ -180,7 +180,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 		$this->config
 			->expects($this->once())
 			->method('getUserValue')
-			->with('ValidTokenUser', 'owncloud', 'lostpassword', null)
+			->with('ValidTokenUser', 'core', 'lostpassword', null)
 			->will($this->returnValue('12345:TheOnlyAndOnlyOneTokenToResetThePassword'));
 		$response = $this->lostController->resetform($token, $userId);
 		$expectedResponse = new TemplateResponse('core',
@@ -217,19 +217,19 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 		$this->config
 			->expects($this->once())
 			->method('getUserValue')
-			->with('ValidTokenUser', 'owncloud', 'lostpassword', null)
+			->with('ValidTokenUser', 'core', 'lostpassword', null)
 			->will($this->returnValue('12345:TheOnlyAndOnlyOneTokenToResetThePassword'));
 		$this->urlGenerator
 			->expects($this->once())
 			->method('linkToRouteAbsolute')
 			->with('core.lost.setPassword', array('userId' => 'ValidTokenUser', 'token' => 'TheOnlyAndOnlyOneTokenToResetThePassword'))
-			->will($this->returnValue('https://ownCloud.com/index.php/lostpassword/'));
+			->will($this->returnValue('https://example.tld/index.php/lostpassword/'));
 
 		$response = $this->lostController->resetform($token, $userId);
 		$expectedResponse = new TemplateResponse('core',
 			'lostpassword/resetpassword',
 			array(
-				'link' => 'https://ownCloud.com/index.php/lostpassword/',
+				'link' => 'https://example.tld/index.php/lostpassword/',
 			),
 			'guest');
 		$this->assertEquals($expectedResponse, $response);
@@ -291,12 +291,12 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 		$this->config
 			->expects($this->once())
 			->method('setUserValue')
-			->with('ExistingUser', 'owncloud', 'lostpassword', '12348:ThisIsMaybeANotSoSecretToken!');
+			->with('ExistingUser', 'core', 'lostpassword', '12348:ThisIsMaybeANotSoSecretToken!');
 		$this->urlGenerator
 			->expects($this->once())
 			->method('linkToRouteAbsolute')
 			->with('core.lost.resetform', array('userId' => 'ExistingUser', 'token' => 'ThisIsMaybeANotSoSecretToken!'))
-			->will($this->returnValue('https://ownCloud.com/index.php/lostpassword/'));
+			->will($this->returnValue('https://example.tld/index.php/lostpassword/'));
 		$message = $this->getMockBuilder('\OC\Mail\Message')
 			->disableOriginalConstructor()->getMock();
 		$message
@@ -310,7 +310,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 		$message
 			->expects($this->at(2))
 			->method('setPlainBody')
-			->with('Use the following link to reset your password: https://ownCloud.com/index.php/lostpassword/');
+			->with('Use the following link to reset your password: https://example.tld/index.php/lostpassword/');
 		$message
 			->expects($this->at(3))
 			->method('setFrom')
@@ -348,7 +348,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 		$this->config
 			->expects($this->once())
 			->method('setUserValue')
-			->with('ExistingUser', 'owncloud', 'lostpassword', '12348:ThisIsMaybeANotSoSecretToken!');
+			->with('ExistingUser', 'core', 'lostpassword', '12348:ThisIsMaybeANotSoSecretToken!');
 		$this->timeFactory
 			->expects($this->once())
 			->method('getTime')
@@ -357,7 +357,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 			->expects($this->once())
 			->method('linkToRouteAbsolute')
 			->with('core.lost.resetform', array('userId' => 'ExistingUser', 'token' => 'ThisIsMaybeANotSoSecretToken!'))
-			->will($this->returnValue('https://ownCloud.com/index.php/lostpassword/'));
+			->will($this->returnValue('https://example.tld/index.php/lostpassword/'));
 		$message = $this->getMockBuilder('\OC\Mail\Message')
 			->disableOriginalConstructor()->getMock();
 		$message
@@ -371,7 +371,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 		$message
 			->expects($this->at(2))
 			->method('setPlainBody')
-			->with('Use the following link to reset your password: https://ownCloud.com/index.php/lostpassword/');
+			->with('Use the following link to reset your password: https://example.tld/index.php/lostpassword/');
 		$message
 			->expects($this->at(3))
 			->method('setFrom')
@@ -395,7 +395,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 		$this->config
 			->expects($this->once())
 			->method('getUserValue')
-			->with('InvalidTokenUser', 'owncloud', 'lostpassword', null)
+			->with('InvalidTokenUser', 'core', 'lostpassword', null)
 			->will($this->returnValue('TheOnlyAndOnlyOneTokenToResetThePassword'));
 
 		// With an invalid token
@@ -417,7 +417,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 		$this->config
 			->expects($this->once())
 			->method('getUserValue')
-			->with('ValidTokenUser', 'owncloud', 'lostpassword', null)
+			->with('ValidTokenUser', 'core', 'lostpassword', null)
 			->will($this->returnValue('12345:TheOnlyAndOnlyOneTokenToResetThePassword'));
 		$user = $this->getMockBuilder('\OCP\IUser')
 			->disableOriginalConstructor()->getMock();
@@ -437,7 +437,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 		$this->config
 			->expects($this->once())
 			->method('deleteUserValue')
-			->with('ValidTokenUser', 'owncloud', 'lostpassword');
+			->with('ValidTokenUser', 'core', 'lostpassword');
 		$this->timeFactory
 			->expects($this->once())
 			->method('getTime')
@@ -452,7 +452,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 		$this->config
 			->expects($this->once())
 			->method('getUserValue')
-			->with('ValidTokenUser', 'owncloud', 'lostpassword', null)
+			->with('ValidTokenUser', 'core', 'lostpassword', null)
 			->will($this->returnValue('12345:TheOnlyAndOnlyOneTokenToResetThePassword'));
 		$user = $this->getMockBuilder('\OCP\IUser')
 			->disableOriginalConstructor()->getMock();
@@ -478,7 +478,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 		$this->config
 			->expects($this->once())
 			->method('getUserValue')
-			->with('ValidTokenUser', 'owncloud', 'lostpassword', null)
+			->with('ValidTokenUser', 'core', 'lostpassword', null)
 			->will($this->returnValue('TheOnlyAndOnlyOneTokenToResetThePassword'));
 		$user = $this->getMockBuilder('\OCP\IUser')
 			->disableOriginalConstructor()->getMock();
@@ -500,7 +500,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 		$this->config
 			->expects($this->once())
 			->method('getUserValue')
-			->with('ValidTokenUser', 'owncloud', 'lostpassword', null)
+			->with('ValidTokenUser', 'core', 'lostpassword', null)
 			->will($this->returnValue('12345:TheOnlyAndOnlyOneTokenToResetThePassword'));
 		$user = $this->getMockBuilder('\OCP\IUser')
 			->disableOriginalConstructor()->getMock();
@@ -530,7 +530,7 @@ class LostControllerTest extends \PHPUnit_Framework_TestCase {
 		$this->config
 			->expects($this->once())
 			->method('getUserValue')
-			->with('ValidTokenUser', 'owncloud', 'lostpassword', null)
+			->with('ValidTokenUser', 'core', 'lostpassword', null)
 			->will($this->returnValue(null));
 
 		$response = $this->lostController->setPassword('', 'ValidTokenUser', 'NewPassword', true);