Merge pull request #16563 from nextcloud/enh/lostcontroller/better_exceptions

Use proper exception in lostController

5 files changed, 53 insertions, 21 deletions

diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php
index 5ac0557e5d6..7947440785b 100644
--- a/core/Controller/LostController.php
+++ b/core/Controller/LostController.php
@@ -32,6 +32,7 @@
 namespace OC\Core\Controller;
 
 use OC\Authentication\TwoFactorAuth\Manager;
+use OC\Core\Exception\ResetPasswordException;
 use OC\HintException;
 use \OCP\AppFramework\Controller;
 use OCP\AppFramework\Http\JSONResponse;
@@ -248,11 +249,11 @@ class LostController extends Controller {
 		// FIXME: use HTTP error codes
 		try {
 			$this->sendEmail($user);
-		} catch (\Exception $e) {
+		} catch (ResetPasswordException $e) {
 			// Ignore the error since we do not want to leak this info
-			$this->logger->logException($e, [
-				'level' => ILogger::WARN
-			]);
+			$this->logger->warning('Could not send password reset email: ' . $e->getMessage());
+		} catch (\Exception $e) {
+			$this->logger->logException($e);
 		}
 
 		$response = new JSONResponse($this->success());
@@ -312,16 +313,15 @@ class LostController extends Controller {
 
 	/**
 	 * @param string $input
-	 * @throws \Exception
+	 * @throws ResetPasswordException
+	 * @throws \OCP\PreConditionNotMetException
 	 */
 	protected function sendEmail($input) {
 		$user = $this->findUserByIdOrMail($input);
 		$email = $user->getEMailAddress();
 
 		if (empty($email)) {
-			throw new \Exception(
-				$this->l10n->t('Could not send reset email because there is no email address for this username. Please contact your administrator.')
-			);
+			throw new ResetPasswordException('Could not send reset e-mail since there is no email for username ' . $input);
 		}
 
 		// Generate the token. It is stored encrypted in the database with the
@@ -367,26 +367,21 @@ class LostController extends Controller {
 			$message->useTemplate($emailTemplate);
 			$this->mailer->send($message);
 		} catch (\Exception $e) {
-			throw new \Exception($this->l10n->t(
-				'Couldn\'t send reset email. Please contact your administrator.'
-			));
+			// Log the exception and continue
+			$this->logger->logException($e);
 		}
 	}
 
 	/**
 	 * @param string $input
 	 * @return IUser
-	 * @throws \InvalidArgumentException
+	 * @throws ResetPasswordException
 	 */
 	protected function findUserByIdOrMail($input) {
-		$userNotFound = new \InvalidArgumentException(
-			$this->l10n->t('Couldn\'t send reset email. Please make sure your username is correct.')
-		);
-
 		$user = $this->userManager->get($input);
 		if ($user instanceof IUser) {
 			if (!$user->isEnabled()) {
-				throw $userNotFound;
+				throw new ResetPasswordException('User is disabled');
 			}
 
 			return $user;
@@ -400,6 +395,6 @@ class LostController extends Controller {
 			return reset($users);
 		}
 
-		throw $userNotFound;
+		throw new ResetPasswordException('Could not find user');
 	}
 }
diff --git a/core/Exception/ResetPasswordException.php b/core/Exception/ResetPasswordException.php
new file mode 100644
index 00000000000..cbae1d59816
--- /dev/null
+++ b/core/Exception/ResetPasswordException.php
@@ -0,0 +1,29 @@
+<?php
+declare(strict_types=1);
+/**
+ * @copyright Copyright (c) 2019, Roeland Jago Douma <roeland@famdouma.nl>
+ *
+ * @author Roeland Jago Douma <roeland@famdouma.nl>
+ *
+ * @license GNU AGPL version 3 or any later version
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+namespace OC\Core\Exception;
+
+class ResetPasswordException extends \Exception {
+
+}
diff --git a/lib/composer/composer/autoload_classmap.php b/lib/composer/composer/autoload_classmap.php
index d8d9227cb07..ead31cd5720 100644
--- a/lib/composer/composer/autoload_classmap.php
+++ b/lib/composer/composer/autoload_classmap.php
@@ -737,6 +737,7 @@ return array(
     'OC\\Core\\Db\\LoginFlowV2' => $baseDir . '/core/Db/LoginFlowV2.php',
     'OC\\Core\\Db\\LoginFlowV2Mapper' => $baseDir . '/core/Db/LoginFlowV2Mapper.php',
     'OC\\Core\\Exception\\LoginFlowV2NotFoundException' => $baseDir . '/core/Exception/LoginFlowV2NotFoundException.php',
+    'OC\\Core\\Exception\\ResetPasswordException' => $baseDir . '/core/Exception/ResetPasswordException.php',
     'OC\\Core\\Middleware\\TwoFactorMiddleware' => $baseDir . '/core/Middleware/TwoFactorMiddleware.php',
     'OC\\Core\\Migrations\\Version13000Date20170705121758' => $baseDir . '/core/Migrations/Version13000Date20170705121758.php',
     'OC\\Core\\Migrations\\Version13000Date20170718121200' => $baseDir . '/core/Migrations/Version13000Date20170718121200.php',
diff --git a/lib/composer/composer/autoload_static.php b/lib/composer/composer/autoload_static.php
index 043846e04df..e8976f2d3b2 100644
--- a/lib/composer/composer/autoload_static.php
+++ b/lib/composer/composer/autoload_static.php
@@ -771,6 +771,7 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
         'OC\\Core\\Db\\LoginFlowV2' => __DIR__ . '/../../..' . '/core/Db/LoginFlowV2.php',
         'OC\\Core\\Db\\LoginFlowV2Mapper' => __DIR__ . '/../../..' . '/core/Db/LoginFlowV2Mapper.php',
         'OC\\Core\\Exception\\LoginFlowV2NotFoundException' => __DIR__ . '/../../..' . '/core/Exception/LoginFlowV2NotFoundException.php',
+        'OC\\Core\\Exception\\ResetPasswordException' => __DIR__ . '/../../..' . '/core/Exception/ResetPasswordException.php',
         'OC\\Core\\Middleware\\TwoFactorMiddleware' => __DIR__ . '/../../..' . '/core/Middleware/TwoFactorMiddleware.php',
         'OC\\Core\\Migrations\\Version13000Date20170705121758' => __DIR__ . '/../../..' . '/core/Migrations/Version13000Date20170705121758.php',
         'OC\\Core\\Migrations\\Version13000Date20170718121200' => __DIR__ . '/../../..' . '/core/Migrations/Version13000Date20170718121200.php',
diff --git a/tests/Core/Controller/LostControllerTest.php b/tests/Core/Controller/LostControllerTest.php
index 8500819a9ca..41f3bb03f02 100644
--- a/tests/Core/Controller/LostControllerTest.php
+++ b/tests/Core/Controller/LostControllerTest.php
@@ -275,8 +275,10 @@ class LostControllerTest extends \Test\TestCase {
 				array(false, $nonExistingUser)
 			)));
 
-		$this->logger->expects($this->exactly(2))
+		$this->logger->expects($this->exactly(0))
 			->method('logException');
+		$this->logger->expects($this->exactly(2))
+			->method('warning');
 
 		$this->userManager
 			->method('getByEmail')
@@ -722,8 +724,10 @@ class LostControllerTest extends \Test\TestCase {
 			->with('ExistingUser')
 			->willReturn($user);
 
-		$this->logger->expects($this->exactly(1))
+		$this->logger->expects($this->exactly(0))
 			->method('logException');
+		$this->logger->expects($this->once())
+			->method('warning');
 
 		$response = $this->lostController->email('ExistingUser');
 		$expectedResponse = new JSONResponse(['status' => 'success']);
@@ -807,8 +811,10 @@ class LostControllerTest extends \Test\TestCase {
 			->method('getByEmail')
 			->willReturn([$user1, $user2]);
 
-		$this->logger->expects($this->exactly(1))
+		$this->logger->expects($this->exactly(0))
 			->method('logException');
+		$this->logger->expects($this->once())
+			->method('warning');
 
 		// request password reset for test@example.com
 		$response = $this->lostController->email('test@example.com');