Make LostController use IInitialState and LoggerInterface

Signed-off-by: Thomas Citharel <[email protected]>

core/Controller/LostController.php

@@ -35,29 +35,30 @@
  */
 namespace OC\Core\Controller;
 
+use Exception;
 use OC\Authentication\TwoFactorAuth\Manager;
 use OC\Core\Events\BeforePasswordResetEvent;
 use OC\Core\Events\PasswordResetEvent;
 use OC\Core\Exception\ResetPasswordException;
 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;
 use OCP\IUserManager;
 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,47 +71,34 @@
  * @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(
- $appName,
+ string $appName,
  IRequest $request,
  IURLGenerator $urlGenerator,
  IUserManager $userManager,
  Defaults $defaults,
  IL10N $l10n,
  IConfig $config,
- $defaultMailAddress,
+ string $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,36 +172,24 @@ 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']);
  }
 
  /**
  * @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,19 +251,18 @@ 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());
  }
 
  return $this->success(['user' => $userId]);
  }
 
  /**
- * @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()) {

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', '[email protected]');
+ $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');