diff --git a/core/Controller/LostController.php b/core/Controller/LostController.php index cc402fe1e615a..9a23e197369cb 100644 --- a/core/Controller/LostController.php +++ b/core/Controller/LostController.php @@ -35,6 +35,7 @@ */ namespace OC\Core\Controller; +use Exception; use OC\Authentication\TwoFactorAuth\Manager; use OC\Core\Events\BeforePasswordResetEvent; use OC\Core\Events\PasswordResetEvent; @@ -42,15 +43,14 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\TemplateResponse; +use OCP\AppFramework\Services\IInitialState; use OCP\Defaults; use OCP\Encryption\IEncryptionModule; use OCP\Encryption\IManager; use OCP\EventDispatcher\IEventDispatcher; use OCP\HintException; use OCP\IConfig; -use OCP\IInitialStateService; use OCP\IL10N; -use OCP\ILogger; use OCP\IRequest; use OCP\IURLGenerator; use OCP\IUser; @@ -58,6 +58,7 @@ use OCP\Mail\IMailer; use OCP\Security\VerificationToken\InvalidTokenException; use OCP\Security\VerificationToken\IVerificationToken; +use Psr\Log\LoggerInterface; use function array_filter; use function count; use function reset; @@ -70,31 +71,18 @@ * @package OC\Core\Controller */ class LostController extends Controller { - /** @var IURLGenerator */ - protected $urlGenerator; - /** @var IUserManager */ - protected $userManager; - /** @var Defaults */ - protected $defaults; - /** @var IL10N */ - protected $l10n; - /** @var string */ - protected $from; - /** @var IManager */ - protected $encryptionManager; - /** @var IConfig */ - protected $config; - /** @var IMailer */ - protected $mailer; - /** @var ILogger */ - private $logger; - /** @var Manager */ - private $twoFactorManager; - /** @var IInitialStateService */ - private $initialStateService; - /** @var IVerificationToken */ - private $verificationToken; - + protected IURLGenerator $urlGenerator; + protected IUserManager $userManager; + protected Defaults $defaults; + protected IL10N $l10n; + protected string $from; + protected IManager $encryptionManager; + protected IConfig $config; + protected IMailer $mailer; + private LoggerInterface $logger; + private Manager $twoFactorManager; + private IInitialState $initialState; + private IVerificationToken $verificationToken; private IEventDispatcher $eventDispatcher; public function __construct( @@ -108,9 +96,9 @@ public function __construct( $defaultMailAddress, IManager $encryptionManager, IMailer $mailer, - ILogger $logger, + LoggerInterface $logger, Manager $twoFactorManager, - IInitialStateService $initialStateService, + IInitialState $initialState, IVerificationToken $verificationToken, IEventDispatcher $eventDispatcher ) { @@ -125,7 +113,7 @@ public function __construct( $this->mailer = $mailer; $this->logger = $logger; $this->twoFactorManager = $twoFactorManager; - $this->initialStateService = $initialStateService; + $this->initialState = $initialState; $this->verificationToken = $verificationToken; $this->eventDispatcher = $eventDispatcher; } @@ -136,14 +124,11 @@ public function __construct( * @PublicPage * @NoCSRFRequired * - * @param string $token - * @param string $userId - * @return TemplateResponse */ - public function resetform($token, $userId) { + public function resetform(string $token, string $userId): TemplateResponse { try { $this->checkPasswordResetToken($token, $userId); - } catch (\Exception $e) { + } catch (Exception $e) { if ($this->config->getSystemValue('lost_password_link', '') !== 'disabled' || ($e instanceof InvalidTokenException && !in_array($e->getCode(), [InvalidTokenException::TOKEN_NOT_FOUND, InvalidTokenException::USER_UNKNOWN])) @@ -161,8 +146,8 @@ public function resetform($token, $userId) { TemplateResponse::RENDER_AS_GUEST ); } - $this->initialStateService->provideInitialState('core', 'resetPasswordUser', $userId); - $this->initialStateService->provideInitialState('core', 'resetPasswordTarget', + $this->initialState->provideInitialState('resetPasswordUser', $userId); + $this->initialState->provideInitialState('resetPasswordTarget', $this->urlGenerator->linkToRouteAbsolute('core.lost.setPassword', ['userId' => $userId, 'token' => $token]) ); @@ -177,7 +162,7 @@ public function resetform($token, $userId) { /** * @param string $token * @param string $userId - * @throws \Exception + * @throws Exception */ protected function checkPasswordResetToken(string $token, string $userId): void { try { @@ -187,24 +172,15 @@ protected function checkPasswordResetToken(string $token, string $userId): void $error = $e->getCode() === InvalidTokenException::TOKEN_EXPIRED ? $this->l10n->t('Could not reset password because the token is expired') : $this->l10n->t('Could not reset password because the token is invalid'); - throw new \Exception($error, (int)$e->getCode(), $e); + throw new Exception($error, (int)$e->getCode(), $e); } } - /** - * @param $message - * @param array $additional - * @return array - */ - private function error($message, array $additional = []) { + private function error(string $message, array $additional = []): array { return array_merge(['status' => 'error', 'msg' => $message], $additional); } - /** - * @param array $data - * @return array - */ - private function success($data = []) { + private function success(array $data = []): array { return array_merge($data, ['status' => 'success']); } @@ -212,11 +188,8 @@ private function success($data = []) { * @PublicPage * @BruteForceProtection(action=passwordResetEmail) * @AnonRateThrottle(limit=10, period=300) - * - * @param string $user - * @return JSONResponse */ - public function email($user) { + public function email(string $user): JSONResponse { if ($this->config->getSystemValue('lost_password_link', '') !== '') { return new JSONResponse($this->error($this->l10n->t('Password reset is disabled'))); } @@ -232,9 +205,9 @@ public function email($user) { $this->sendEmail($user); } catch (ResetPasswordException $e) { // Ignore the error since we do not want to leak this info - $this->logger->warning('Could not send password reset email: ' . $e->getMessage()); - } catch (\Exception $e) { - $this->logger->logException($e); + $this->logger->warning('Could not send password reset email: ' . $e->getMessage(), ['exception' => $e]); + } catch (Exception $e) { + $this->logger->error('Could not send password reset email: ' . $e->getMessage(), ['exception' => $e]); } $response = new JSONResponse($this->success()); @@ -244,13 +217,8 @@ public function email($user) { /** * @PublicPage - * @param string $token - * @param string $userId - * @param string $password - * @param boolean $proceed - * @return array */ - public function setPassword($token, $userId, $password, $proceed) { + public function setPassword(string $token, string $userId, string $password, bool $proceed): array { if ($this->encryptionManager->isEnabled() && !$proceed) { $encryptionModules = $this->encryptionManager->getEncryptionModules(); foreach ($encryptionModules as $module) { @@ -271,7 +239,7 @@ public function setPassword($token, $userId, $password, $proceed) { \OC_Hook::emit('\OC\Core\LostPassword\Controller\LostController', 'pre_passwordReset', ['uid' => $userId, 'password' => $password]); if (!$user->setPassword($password)) { - throw new \Exception(); + throw new Exception(); } $this->eventDispatcher->dispatchTyped(new PasswordResetEvent($user, $password)); @@ -283,7 +251,7 @@ public function setPassword($token, $userId, $password, $proceed) { @\OC::$server->getUserSession()->unsetMagicInCookie(); } catch (HintException $e) { return $this->error($e->getHint()); - } catch (\Exception $e) { + } catch (Exception $e) { return $this->error($e->getMessage()); } @@ -291,11 +259,10 @@ public function setPassword($token, $userId, $password, $proceed) { } /** - * @param string $input * @throws ResetPasswordException * @throws \OCP\PreConditionNotMetException */ - protected function sendEmail($input) { + protected function sendEmail(string $input) { $user = $this->findUserByIdOrMail($input); $email = $user->getEMailAddress(); @@ -337,18 +304,16 @@ protected function sendEmail($input) { $message->setFrom([$this->from => $this->defaults->getName()]); $message->useTemplate($emailTemplate); $this->mailer->send($message); - } catch (\Exception $e) { + } catch (Exception $e) { // Log the exception and continue - $this->logger->logException($e); + $this->logger->error("Failed to send password reset e-mail", ['exception' => $e]); } } /** - * @param string $input - * @return IUser * @throws ResetPasswordException */ - protected function findUserByIdOrMail($input) { + protected function findUserByIdOrMail(string $input): IUser { $user = $this->userManager->get($input); if ($user instanceof IUser) { if (!$user->isEnabled()) { diff --git a/tests/Core/Controller/LostControllerTest.php b/tests/Core/Controller/LostControllerTest.php index 0e3d92b854add..29d85162840cf 100644 --- a/tests/Core/Controller/LostControllerTest.php +++ b/tests/Core/Controller/LostControllerTest.php @@ -28,14 +28,13 @@ use OC\Mail\Message; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\TemplateResponse; +use OCP\AppFramework\Services\IInitialState; use OCP\Defaults; use OCP\Encryption\IEncryptionModule; use OCP\Encryption\IManager; use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; -use OCP\IInitialStateService; use OCP\IL10N; -use OCP\ILogger; use OCP\IRequest; use OCP\IURLGenerator; use OCP\IUser; @@ -45,6 +44,7 @@ use OCP\Security\VerificationToken\InvalidTokenException; use OCP\Security\VerificationToken\IVerificationToken; use PHPUnit\Framework\MockObject\MockObject; +use Psr\Log\LoggerInterface; use Test\TestCase; /** @@ -72,12 +72,12 @@ class LostControllerTest extends TestCase { private $encryptionManager; /** @var IRequest|MockObject */ private $request; - /** @var ILogger|MockObject */ + /** @var LoggerInterface|MockObject */ private $logger; /** @var Manager|MockObject */ private $twofactorManager; - /** @var IInitialStateService|MockObject */ - private $initialStateService; + /** @var IInitialState|MockObject */ + private $initialState; /** @var IVerificationToken|MockObject */ private $verificationToken; /** @var IEventDispatcher|MockObject */ @@ -124,9 +124,9 @@ protected function setUp(): void { $this->encryptionManager->expects($this->any()) ->method('isEnabled') ->willReturn(true); - $this->logger = $this->createMock(ILogger::class); + $this->logger = $this->createMock(LoggerInterface::class); $this->twofactorManager = $this->createMock(Manager::class); - $this->initialStateService = $this->createMock(IInitialStateService::class); + $this->initialState = $this->createMock(IInitialState::class); $this->verificationToken = $this->createMock(IVerificationToken::class); $this->eventDispatcher = $this->createMock(IEventDispatcher::class); $this->lostController = new LostController( @@ -142,7 +142,7 @@ protected function setUp(): void { $this->mailer, $this->logger, $this->twofactorManager, - $this->initialStateService, + $this->initialState, $this->verificationToken, $this->eventDispatcher ); @@ -176,6 +176,18 @@ public function testResetFormValidToken() { $this->verificationToken->expects($this->once()) ->method('check') ->with('MySecretToken', $this->existingUser, 'lostpassword', 'test@example.com'); + $this->urlGenerator + ->expects($this->once()) + ->method('linkToRouteAbsolute') + ->with('core.lost.setPassword', ['userId' => 'ValidTokenUser', 'token' => 'MySecretToken']) + ->willReturn('https://example.tld/index.php/lostpassword/set/sometoken/someuser'); + $this->initialState + ->expects($this->exactly(2)) + ->method('provideInitialState') + ->withConsecutive( + ['resetPasswordUser', 'ValidTokenUser'], + ['resetPasswordTarget', 'https://example.tld/index.php/lostpassword/set/sometoken/someuser'] + ); $response = $this->lostController->resetform('MySecretToken', 'ValidTokenUser'); $expectedResponse = new TemplateResponse('core', @@ -197,7 +209,7 @@ public function testEmailUnsuccessful() { ]); $this->logger->expects($this->exactly(0)) - ->method('logException'); + ->method('error'); $this->logger->expects($this->exactly(2)) ->method('warning'); @@ -398,7 +410,7 @@ public function testEmailCantSendException() { ->will($this->throwException(new \Exception())); $this->logger->expects($this->exactly(1)) - ->method('logException'); + ->method('error'); $response = $this->lostController->email('ExistingUser'); $expectedResponse = new JSONResponse(['status' => 'success']); @@ -561,7 +573,7 @@ public function testSendEmailNoEmail() { ->willReturn($user); $this->logger->expects($this->exactly(0)) - ->method('logException'); + ->method('error'); $this->logger->expects($this->once()) ->method('warning'); @@ -644,7 +656,7 @@ public function testTwoUsersWithSameEmail() { ->willReturn([$user1, $user2]); $this->logger->expects($this->exactly(0)) - ->method('logException'); + ->method('error'); $this->logger->expects($this->once()) ->method('warning');