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>tags/v17.0.0beta1
@@ -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'); | |||
} | |||
} |
@@ -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 { | |||
} |
@@ -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', |
@@ -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', |
@@ -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'); |