From b6dd2ebd3909017b9a5dbe0e145d6c5041a9043c Mon Sep 17 00:00:00 2001 From: Roeland Jago Douma Date: Fri, 26 Jul 2019 15:21:41 +0200 Subject: [PATCH] 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 --- core/Controller/LostController.php | 31 ++++++++------------ core/Exception/ResetPasswordException.php | 29 ++++++++++++++++++ lib/composer/composer/autoload_classmap.php | 1 + lib/composer/composer/autoload_static.php | 1 + tests/Core/Controller/LostControllerTest.php | 12 ++++++-- 5 files changed, 53 insertions(+), 21 deletions(-) create mode 100644 core/Exception/ResetPasswordException.php 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 @@ + + * + * @author Roeland Jago Douma + * + * @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 . + * + */ + +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'); -- 2.39.5