Make LostController use IInitialState and LoggerInterface

Signed-off-by: Thomas Citharel <tcit@tcit.fr>

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,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,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', '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');