Merge pull request #13595 from nextcloud/enh/do_not_leak_user_existence

Generic message on password reset
nextcloud · Jan 15, 2019 · a32577d · a32577d
2 parents 53c077a + f42115d
commit a32577d
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 15 deletions.
diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php
@@ -39,6 +39,7 @@
 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 @@ public function __construct($appName,
 								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 @@ public function __construct($appName,
 		$this->mailer = $mailer;
 		$this->timeFactory = $timeFactory;
 		$this->crypto = $crypto;
+		$this->logger = $logger;
 	}
 
 	/**
@@ -236,10 +241,11 @@ public function email($user){
 		// 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
@@ -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.<br>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.<br>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.<br />If you are not sure what to do, please contact your administrator before you continue. <br />Do you really want to continue?")
 			+ ('<br /><input type="checkbox" id="encrypted-continue" class="checkbox checkbox--white" value="Yes" />')

diff --git a/tests/Core/Controller/LostControllerTest.php b/tests/Core/Controller/LostControllerTest.php
@@ -31,6 +31,7 @@
 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 @@ protected function setUp() {
 			->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 @@ protected function setUp() {
 			$this->encryptionManager,
 			$this->mailer,
 			$this->timeFactory,
-			$this->crypto
+			$this->crypto,
+			$this->logger
 		);
 	}
 
@@ -265,15 +270,17 @@ public function testEmailUnsuccessful() {
 				array(false, $nonExistingUser)
 			)));
 
+		$this->logger->expects($this->exactly(2))
+			->method('logException');
+
 		$this->userManager
 			->method('getByEmail')
 			->willReturn([]);
 
 		// 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 @@ public function testEmailUnsuccessful() {
 			->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 @@ public function testEmailCantSendException() {
 				$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 @@ public function testSendEmailNoEmail() {
 			->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 @@ public function testTwoUsersWithSameEmail() {
 			->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();