From 139fb8de9471e83155c141640ce91c66d42d7b28 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 23 Aug 2016 12:54:45 +0200 Subject: [PATCH 1/2] Remove "password reset token" after successful login --- core/Controller/LoginController.php | 3 +++ tests/Core/Controller/LoginControllerTest.php | 20 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index dbc1f3157f..56b6315593 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(), 'owncloud', 'lostpassword'); + if ($this->twoFactorManager->isTwoFactorAuthenticated($loginResult)) { $this->twoFactorManager->prepareTwoFactorLogin($loginResult); if (!is_null($redirect_url)) { diff --git a/tests/Core/Controller/LoginControllerTest.php b/tests/Core/Controller/LoginControllerTest.php index 8eaa7c9843..7fcc8222bc 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', 'owncloud', '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', 'owncloud', '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', 'owncloud', '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)); From 736e884e9a53fb485073800dca2583a31c12d24d Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Tue, 23 Aug 2016 15:01:38 +0200 Subject: [PATCH 2/2] Move the reset token to core app --- core/Controller/LoginController.php | 2 +- core/Controller/LostController.php | 6 ++-- tests/Core/Controller/LoginControllerTest.php | 6 ++-- tests/Core/Controller/LostControllerTest.php | 36 +++++++++---------- 4 files changed, 25 insertions(+), 25 deletions(-) diff --git a/core/Controller/LoginController.php b/core/Controller/LoginController.php index 56b6315593..67e1e21528 100644 --- a/core/Controller/LoginController.php +++ b/core/Controller/LoginController.php @@ -238,7 +238,7 @@ class LoginController extends Controller { $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(), 'owncloud', 'lostpassword'); + $this->config->deleteUserValue($loginResult->getUID(), 'core', 'lostpassword'); if ($this->twoFactorManager->isTwoFactorAuthenticated($loginResult)) { $this->twoFactorManager->prepareTwoFactorLogin($loginResult); diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php index c3716e9a8e..fe6be1e685 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 7fcc8222bc..417a60a9e5 100644 --- a/tests/Core/Controller/LoginControllerTest.php +++ b/tests/Core/Controller/LoginControllerTest.php @@ -370,7 +370,7 @@ class LoginControllerTest extends TestCase { ->will($this->returnValue(false)); $this->config->expects($this->once()) ->method('deleteUserValue') - ->with('uid', 'owncloud', 'lostpassword'); + ->with('uid', 'core', 'lostpassword'); $expected = new \OCP\AppFramework\Http\RedirectResponse($indexPageUrl); $this->assertEquals($expected, $this->loginController->tryLogin($user, $password, null)); @@ -499,7 +499,7 @@ class LoginControllerTest extends TestCase { ->will($this->returnValue($redirectUrl)); $this->config->expects($this->once()) ->method('deleteUserValue') - ->with('jane', 'owncloud', 'lostpassword'); + ->with('jane', 'core', 'lostpassword'); $expected = new \OCP\AppFramework\Http\RedirectResponse(urldecode($redirectUrl)); $this->assertEquals($expected, $this->loginController->tryLogin('Jane', $password, $originalUrl)); @@ -553,7 +553,7 @@ class LoginControllerTest extends TestCase { ->will($this->returnValue($challengeUrl)); $this->config->expects($this->once()) ->method('deleteUserValue') - ->with('john', 'owncloud', 'lostpassword'); + ->with('john', 'core', 'lostpassword'); $expected = new RedirectResponse($challengeUrl); $this->assertEquals($expected, $this->loginController->tryLogin('john@doe.com', $password, null)); diff --git a/tests/Core/Controller/LostControllerTest.php b/tests/Core/Controller/LostControllerTest.php index 492a04bcfd..2e7d6721d5 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);