Use proper exception in lostController

There is no need to log the expcetion of most of the stuff here.
We should properly log them but an exception is excessive.

This moves it to a proper exception which we can catch and then log.
The other exceptions will still be fully logged.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php
index 5ac0557e5d6bf5a4118eb8687f46fa77259635bd..7947440785b4d1a84614ef24f6058abd86a55e8a 100644 (file)
--- 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 (file)
index 0000000..cbae1d5
--- /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 d8d9227cb07cfaa4cdbb441d4b18938bab3dbbd9..ead31cd5720cfab8362f73d13efc78cf7709d7ff 100644 (file)
--- 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 043846e04df0fb9b2cb51835af334ec521285d67..e8976f2d3b2e1c63d173b4a93381163f92b080f5 100644 (file)
--- 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 8500819a9cae812c906608fc5bf6802932ed945b..41f3bb03f02bc5500fffa6e689a01545f6054e8d 100644 (file)
--- 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');