From d0397f9b5354b5b277c0b0e72983ba7cd9b4822e Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Mon, 14 Jan 2019 21:05:52 +0100 Subject: [PATCH 1/2] Generic message on password reset There is no need to inform the user if the account existed or not. Signed-off-by: Roeland Jago Douma --- core/Controller/LostController.php | 16 +++++++++++----- core/js/lostpassword.js | 2 +- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php index 8d1481dfc2..ed802aca58 100644 --- a/core/Controller/LostController.php +++ b/core/Controller/LostController.php @@ -39,6 +39,7 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\Defaults; use OCP\Encryption\IEncryptionModule; use OCP\Encryption\IManager; +use OCP\ILogger; use \OCP\IURLGenerator; use \OCP\IRequest; use \OCP\IL10N; @@ -80,6 +81,8 @@ class LostController extends Controller { protected $timeFactory; /** @var ICrypto */ protected $crypto; + /** @var ILogger */ + private $logger; /** * @param string $appName @@ -108,7 +111,8 @@ class LostController extends Controller { IManager $encryptionManager, IMailer $mailer, ITimeFactory $timeFactory, - ICrypto $crypto) { + ICrypto $crypto, + ILogger $logger) { parent::__construct($appName, $request); $this->urlGenerator = $urlGenerator; $this->userManager = $userManager; @@ -121,6 +125,7 @@ class LostController extends Controller { $this->mailer = $mailer; $this->timeFactory = $timeFactory; $this->crypto = $crypto; + $this->logger = $logger; } /** @@ -236,10 +241,11 @@ class LostController extends Controller { // FIXME: use HTTP error codes try { $this->sendEmail($user); - } catch (\Exception $e){ - $response = new JSONResponse($this->error($e->getMessage())); - $response->throttle(); - return $response; + } catch (\Exception $e) { + // Ignore the error since we do not want to leak this info + $this->logger->logException($e, [ + 'level' => ILogger::WARN + ]); } $response = new JSONResponse($this->success()); diff --git a/core/js/lostpassword.js b/core/js/lostpassword.js index 084a53f412..75e91ffac7 100644 --- a/core/js/lostpassword.js +++ b/core/js/lostpassword.js @@ -2,7 +2,7 @@ OC.Lostpassword = { sendErrorMsg : t('core', 'Couldn\'t send reset email. Please contact your administrator.'), - sendSuccessMsg : t('core', 'The link to reset your password has been sent to your email. If you do not receive it within a reasonable amount of time, check your spam/junk folders.
If it is not there ask your local administrator.'), + sendSuccessMsg : t('core', 'We have send a password reset e-mail to the e-mail address known to us for this account. If you do not receive it within a reasonable amount of time, check your spam/junk folders.
If it is not there ask your local administrator.'), encryptedMsg : t('core', "Your files are encrypted. There will be no way to get your data back after your password is reset.
If you are not sure what to do, please contact your administrator before you continue.
Do you really want to continue?") + ('
') From f42115d6bbfa0c54f1ebb38b5884a6e4356d3738 Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Tue, 15 Jan 2019 15:03:37 +0100 Subject: [PATCH 2/2] Fix tests Signed-off-by: Roeland Jago Douma --- tests/Core/Controller/LostControllerTest.php | 32 ++++++++++++++------ 1 file changed, 23 insertions(+), 9 deletions(-) diff --git a/tests/Core/Controller/LostControllerTest.php b/tests/Core/Controller/LostControllerTest.php index 91b52fc8ef..2aa10cf116 100644 --- a/tests/Core/Controller/LostControllerTest.php +++ b/tests/Core/Controller/LostControllerTest.php @@ -31,6 +31,7 @@ use OCP\Encryption\IEncryptionModule; use OCP\Encryption\IManager; use OCP\IConfig; use OCP\IL10N; +use OCP\ILogger; use OCP\IRequest; use OCP\IURLGenerator; use OCP\IUser; @@ -74,6 +75,8 @@ class LostControllerTest extends \Test\TestCase { private $request; /** @var ICrypto|\PHPUnit_Framework_MockObject_MockObject */ private $crypto; + /** @var ILogger|\PHPUnit_Framework_MockObject_MockObject */ + private $logger; protected function setUp() { parent::setUp(); @@ -124,6 +127,7 @@ class LostControllerTest extends \Test\TestCase { ->method('isEnabled') ->willReturn(true); $this->crypto = $this->createMock(ICrypto::class); + $this->logger = $this->createMock(ILogger::class); $this->lostController = new LostController( 'Core', $this->request, @@ -137,7 +141,8 @@ class LostControllerTest extends \Test\TestCase { $this->encryptionManager, $this->mailer, $this->timeFactory, - $this->crypto + $this->crypto, + $this->logger ); } @@ -265,6 +270,9 @@ class LostControllerTest extends \Test\TestCase { array(false, $nonExistingUser) ))); + $this->logger->expects($this->exactly(2)) + ->method('logException'); + $this->userManager ->method('getByEmail') ->willReturn([]); @@ -272,8 +280,7 @@ class LostControllerTest extends \Test\TestCase { // With a non existing user $response = $this->lostController->email($nonExistingUser); $expectedResponse = new JSONResponse([ - 'status' => 'error', - 'msg' => 'Couldn\'t send reset email. Please make sure your username is correct.' + 'status' => 'success', ]); $expectedResponse->throttle(); $this->assertEquals($expectedResponse, $response); @@ -286,8 +293,7 @@ class LostControllerTest extends \Test\TestCase { ->will($this->returnValue(null)); $response = $this->lostController->email($existingUser); $expectedResponse = new JSONResponse([ - 'status' => 'error', - 'msg' => 'Couldn\'t send reset email. Please make sure your username is correct.' + 'status' => 'success', ]); $expectedResponse->throttle(); $this->assertEquals($expectedResponse, $response); @@ -511,8 +517,11 @@ class LostControllerTest extends \Test\TestCase { $this->equalTo('test@example.comSECRET') )->willReturn('encryptedToken'); + $this->logger->expects($this->exactly(1)) + ->method('logException'); + $response = $this->lostController->email('ExistingUser'); - $expectedResponse = new JSONResponse(['status' => 'error', 'msg' => 'Couldn\'t send reset email. Please contact your administrator.']); + $expectedResponse = new JSONResponse(['status' => 'success']); $expectedResponse->throttle(); $this->assertEquals($expectedResponse, $response); } @@ -708,8 +717,11 @@ class LostControllerTest extends \Test\TestCase { ->with('ExistingUser') ->willReturn($user); + $this->logger->expects($this->exactly(1)) + ->method('logException'); + $response = $this->lostController->email('ExistingUser'); - $expectedResponse = new JSONResponse(['status' => 'error', 'msg' => 'Could not send reset email because there is no email address for this username. Please contact your administrator.']); + $expectedResponse = new JSONResponse(['status' => 'success']); $expectedResponse->throttle(); $this->assertEquals($expectedResponse, $response); } @@ -790,12 +802,14 @@ class LostControllerTest extends \Test\TestCase { ->method('getByEmail') ->willReturn([$user1, $user2]); + $this->logger->expects($this->exactly(1)) + ->method('logException'); + // request password reset for test@example.com $response = $this->lostController->email('test@example.com'); $expectedResponse = new JSONResponse([ - 'status' => 'error', - 'msg' => 'Couldn\'t send reset email. Please make sure your username is correct.' + 'status' => 'success' ]); $expectedResponse->throttle();