Skip to content

Commit 3e3114b

Browse files
tcitworldtcitworld
committed
Make LostController use IInitialState and LoggerInterface
Signed-off-by: Thomas Citharel <tcit@tcit.fr>
1 parent 6ec8b11 commit 3e3114b

File tree

2 files changed

+63
-86
lines changed

2 files changed

+63
-86
lines changed

‎core/Controller/LostController.php‎

Lines changed: 39 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -35,29 +35,30 @@
3535
*/
3636
namespace OC\Core\Controller;
3737

38+
use Exception;
3839
use OC\Authentication\TwoFactorAuth\Manager;
3940
use OC\Core\Events\BeforePasswordResetEvent;
4041
use OC\Core\Events\PasswordResetEvent;
4142
use OC\Core\Exception\ResetPasswordException;
4243
use OCP\AppFramework\Controller;
4344
use OCP\AppFramework\Http\JSONResponse;
4445
use OCP\AppFramework\Http\TemplateResponse;
46+
use OCP\AppFramework\Services\IInitialState;
4547
use OCP\Defaults;
4648
use OCP\Encryption\IEncryptionModule;
4749
use OCP\Encryption\IManager;
4850
use OCP\EventDispatcher\IEventDispatcher;
4951
use OCP\HintException;
5052
use OCP\IConfig;
51-
use OCP\IInitialStateService;
5253
use OCP\IL10N;
53-
use OCP\ILogger;
5454
use OCP\IRequest;
5555
use OCP\IURLGenerator;
5656
use OCP\IUser;
5757
use OCP\IUserManager;
5858
use OCP\Mail\IMailer;
5959
use OCP\Security\VerificationToken\InvalidTokenException;
6060
use OCP\Security\VerificationToken\IVerificationToken;
61+
use Psr\Log\LoggerInterface;
6162
use function array_filter;
6263
use function count;
6364
use function reset;
@@ -70,47 +71,34 @@
7071
* @package OC\Core\Controller
7172
*/
7273
class LostController extends Controller {
73-
/** @var IURLGenerator */
74-
protected $urlGenerator;
75-
/** @var IUserManager */
76-
protected $userManager;
77-
/** @var Defaults */
78-
protected $defaults;
79-
/** @var IL10N */
80-
protected $l10n;
81-
/** @var string */
82-
protected $from;
83-
/** @var IManager */
84-
protected $encryptionManager;
85-
/** @var IConfig */
86-
protected $config;
87-
/** @var IMailer */
88-
protected $mailer;
89-
/** @var ILogger */
90-
private $logger;
91-
/** @var Manager */
92-
private $twoFactorManager;
93-
/** @var IInitialStateService */
94-
private $initialStateService;
95-
/** @var IVerificationToken */
96-
private $verificationToken;
97-
74+
protected IURLGenerator $urlGenerator;
75+
protected IUserManager $userManager;
76+
protected Defaults $defaults;
77+
protected IL10N $l10n;
78+
protected string $from;
79+
protected IManager $encryptionManager;
80+
protected IConfig $config;
81+
protected IMailer $mailer;
82+
private LoggerInterface $logger;
83+
private Manager $twoFactorManager;
84+
private IInitialState $initialState;
85+
private IVerificationToken $verificationToken;
9886
private IEventDispatcher $eventDispatcher;
9987

10088
public function __construct(
101-
$appName,
89+
string $appName,
10290
IRequest $request,
10391
IURLGenerator $urlGenerator,
10492
IUserManager $userManager,
10593
Defaults $defaults,
10694
IL10N $l10n,
10795
IConfig $config,
108-
$defaultMailAddress,
96+
string $defaultMailAddress,
10997
IManager $encryptionManager,
11098
IMailer $mailer,
111-
ILogger $logger,
99+
LoggerInterface $logger,
112100
Manager $twoFactorManager,
113-
IInitialStateService $initialStateService,
101+
IInitialState $initialState,
114102
IVerificationToken $verificationToken,
115103
IEventDispatcher $eventDispatcher
116104
) {
@@ -125,7 +113,7 @@ public function __construct(
125113
$this->mailer = $mailer;
126114
$this->logger = $logger;
127115
$this->twoFactorManager = $twoFactorManager;
128-
$this->initialStateService = $initialStateService;
116+
$this->initialState = $initialState;
129117
$this->verificationToken = $verificationToken;
130118
$this->eventDispatcher = $eventDispatcher;
131119
}
@@ -136,14 +124,11 @@ public function __construct(
136124
* @PublicPage
137125
* @NoCSRFRequired
138126
*
139-
* @param string $token
140-
* @param string $userId
141-
* @return TemplateResponse
142127
*/
143-
public function resetform($token, $userId) {
128+
public function resetform(string $token, string $userId): TemplateResponse {
144129
try {
145130
$this->checkPasswordResetToken($token, $userId);
146-
} catch (\Exception $e) {
131+
} catch (Exception $e) {
147132
if ($this->config->getSystemValue('lost_password_link', '') !== 'disabled'
148133
|| ($e instanceof InvalidTokenException
149134
&& !in_array($e->getCode(), [InvalidTokenException::TOKEN_NOT_FOUND, InvalidTokenException::USER_UNKNOWN]))
@@ -161,8 +146,8 @@ public function resetform($token, $userId) {
161146
TemplateResponse::RENDER_AS_GUEST
162147
);
163148
}
164-
$this->initialStateService->provideInitialState('core', 'resetPasswordUser', $userId);
165-
$this->initialStateService->provideInitialState('core', 'resetPasswordTarget',
149+
$this->initialState->provideInitialState('resetPasswordUser', $userId);
150+
$this->initialState->provideInitialState('resetPasswordTarget',
166151
$this->urlGenerator->linkToRouteAbsolute('core.lost.setPassword', ['userId' => $userId, 'token' => $token])
167152
);
168153

@@ -177,7 +162,7 @@ public function resetform($token, $userId) {
177162
/**
178163
* @param string $token
179164
* @param string $userId
180-
* @throws \Exception
165+
* @throws Exception
181166
*/
182167
protected function checkPasswordResetToken(string $token, string $userId): void {
183168
try {
@@ -187,36 +172,24 @@ protected function checkPasswordResetToken(string $token, string $userId): void
187172
$error = $e->getCode() === InvalidTokenException::TOKEN_EXPIRED
188173
? $this->l10n->t('Could not reset password because the token is expired')
189174
: $this->l10n->t('Could not reset password because the token is invalid');
190-
throw new \Exception($error, (int)$e->getCode(), $e);
175+
throw new Exception($error, (int)$e->getCode(), $e);
191176
}
192177
}
193178

194-
/**
195-
* @param $message
196-
* @param array $additional
197-
* @return array
198-
*/
199-
private function error($message, array $additional = []) {
179+
private function error(string $message, array $additional = []): array {
200180
return array_merge(['status' => 'error', 'msg' => $message], $additional);
201181
}
202182

203-
/**
204-
* @param array $data
205-
* @return array
206-
*/
207-
private function success($data = []) {
183+
private function success(array $data = []): array {
208184
return array_merge($data, ['status' => 'success']);
209185
}
210186

211187
/**
212188
* @PublicPage
213189
* @BruteForceProtection(action=passwordResetEmail)
214190
* @AnonRateThrottle(limit=10, period=300)
215-
*
216-
* @param string $user
217-
* @return JSONResponse
218191
*/
219-
public function email($user) {
192+
public function email(string $user): JSONResponse {
220193
if ($this->config->getSystemValue('lost_password_link', '') !== '') {
221194
return new JSONResponse($this->error($this->l10n->t('Password reset is disabled')));
222195
}
@@ -232,9 +205,9 @@ public function email($user) {
232205
$this->sendEmail($user);
233206
} catch (ResetPasswordException $e) {
234207
// Ignore the error since we do not want to leak this info
235-
$this->logger->warning('Could not send password reset email: ' . $e->getMessage());
236-
} catch (\Exception $e) {
237-
$this->logger->logException($e);
208+
$this->logger->warning('Could not send password reset email: ' . $e->getMessage(), ['exception' => $e]);
209+
} catch (Exception $e) {
210+
$this->logger->error('Could not send password reset email: ' . $e->getMessage(), ['exception' => $e]);
238211
}
239212

240213
$response = new JSONResponse($this->success());
@@ -244,13 +217,8 @@ public function email($user) {
244217

245218
/**
246219
* @PublicPage
247-
* @param string $token
248-
* @param string $userId
249-
* @param string $password
250-
* @param boolean $proceed
251-
* @return array
252220
*/
253-
public function setPassword($token, $userId, $password, $proceed) {
221+
public function setPassword(string $token, string $userId, string $password, bool $proceed): array {
254222
if ($this->encryptionManager->isEnabled() && !$proceed) {
255223
$encryptionModules = $this->encryptionManager->getEncryptionModules();
256224
foreach ($encryptionModules as $module) {
@@ -271,7 +239,7 @@ public function setPassword($token, $userId, $password, $proceed) {
271239
\OC_Hook::emit('\OC\Core\LostPassword\Controller\LostController', 'pre_passwordReset', ['uid' => $userId, 'password' => $password]);
272240

273241
if (!$user->setPassword($password)) {
274-
throw new \Exception();
242+
throw new Exception();
275243
}
276244

277245
$this->eventDispatcher->dispatchTyped(new PasswordResetEvent($user, $password));
@@ -283,19 +251,18 @@ public function setPassword($token, $userId, $password, $proceed) {
283251
@\OC::$server->getUserSession()->unsetMagicInCookie();
284252
} catch (HintException $e) {
285253
return $this->error($e->getHint());
286-
} catch (\Exception $e) {
254+
} catch (Exception $e) {
287255
return $this->error($e->getMessage());
288256
}
289257

290258
return $this->success(['user' => $userId]);
291259
}
292260

293261
/**
294-
* @param string $input
295262
* @throws ResetPasswordException
296263
* @throws \OCP\PreConditionNotMetException
297264
*/
298-
protected function sendEmail($input) {
265+
protected function sendEmail(string $input) {
299266
$user = $this->findUserByIdOrMail($input);
300267
$email = $user->getEMailAddress();
301268

@@ -337,18 +304,16 @@ protected function sendEmail($input) {
337304
$message->setFrom([$this->from => $this->defaults->getName()]);
338305
$message->useTemplate($emailTemplate);
339306
$this->mailer->send($message);
340-
} catch (\Exception $e) {
307+
} catch (Exception $e) {
341308
// Log the exception and continue
342-
$this->logger->logException($e);
309+
$this->logger->error("Failed to send password reset e-mail", ['exception' => $e]);
343310
}
344311
}
345312

346313
/**
347-
* @param string $input
348-
* @return IUser
349314
* @throws ResetPasswordException
350315
*/
351-
protected function findUserByIdOrMail($input) {
316+
protected function findUserByIdOrMail(string $input): IUser {
352317
$user = $this->userManager->get($input);
353318
if ($user instanceof IUser) {
354319
if (!$user->isEnabled()) {

‎tests/Core/Controller/LostControllerTest.php‎

Lines changed: 24 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,13 @@
2828
use OC\Mail\Message;
2929
use OCP\AppFramework\Http\JSONResponse;
3030
use OCP\AppFramework\Http\TemplateResponse;
31+
use OCP\AppFramework\Services\IInitialState;
3132
use OCP\Defaults;
3233
use OCP\Encryption\IEncryptionModule;
3334
use OCP\Encryption\IManager;
3435
use OCP\EventDispatcher\IEventDispatcher;
3536
use OCP\IConfig;
36-
use OCP\IInitialStateService;
3737
use OCP\IL10N;
38-
use OCP\ILogger;
3938
use OCP\IRequest;
4039
use OCP\IURLGenerator;
4140
use OCP\IUser;
@@ -45,6 +44,7 @@
4544
use OCP\Security\VerificationToken\InvalidTokenException;
4645
use OCP\Security\VerificationToken\IVerificationToken;
4746
use PHPUnit\Framework\MockObject\MockObject;
47+
use Psr\Log\LoggerInterface;
4848
use Test\TestCase;
4949

5050
/**
@@ -72,12 +72,12 @@ class LostControllerTest extends TestCase {
7272
private $encryptionManager;
7373
/** @var IRequest|MockObject */
7474
private $request;
75-
/** @var ILogger|MockObject */
75+
/** @var LoggerInterface|MockObject */
7676
private $logger;
7777
/** @var Manager|MockObject */
7878
private $twofactorManager;
79-
/** @var IInitialStateService|MockObject */
80-
private $initialStateService;
79+
/** @var IInitialState|MockObject */
80+
private $initialState;
8181
/** @var IVerificationToken|MockObject */
8282
private $verificationToken;
8383
/** @var IEventDispatcher|MockObject */
@@ -124,9 +124,9 @@ protected function setUp(): void {
124124
$this->encryptionManager->expects($this->any())
125125
->method('isEnabled')
126126
->willReturn(true);
127-
$this->logger = $this->createMock(ILogger::class);
127+
$this->logger = $this->createMock(LoggerInterface::class);
128128
$this->twofactorManager = $this->createMock(Manager::class);
129-
$this->initialStateService = $this->createMock(IInitialStateService::class);
129+
$this->initialState = $this->createMock(IInitialState::class);
130130
$this->verificationToken = $this->createMock(IVerificationToken::class);
131131
$this->eventDispatcher = $this->createMock(IEventDispatcher::class);
132132
$this->lostController = new LostController(
@@ -142,7 +142,7 @@ protected function setUp(): void {
142142
$this->mailer,
143143
$this->logger,
144144
$this->twofactorManager,
145-
$this->initialStateService,
145+
$this->initialState,
146146
$this->verificationToken,
147147
$this->eventDispatcher
148148
);
@@ -176,6 +176,18 @@ public function testResetFormValidToken() {
176176
$this->verificationToken->expects($this->once())
177177
->method('check')
178178
->with('MySecretToken', $this->existingUser, 'lostpassword', 'test@example.com');
179+
$this->urlGenerator
180+
->expects($this->once())
181+
->method('linkToRouteAbsolute')
182+
->with('core.lost.setPassword', ['userId' => 'ValidTokenUser', 'token' => 'MySecretToken'])
183+
->willReturn('https://example.tld/index.php/lostpassword/set/sometoken/someuser');
184+
$this->initialState
185+
->expects($this->exactly(2))
186+
->method('provideInitialState')
187+
->withConsecutive(
188+
['resetPasswordUser', 'ValidTokenUser'],
189+
['resetPasswordTarget', 'https://example.tld/index.php/lostpassword/set/sometoken/someuser']
190+
);
179191

180192
$response = $this->lostController->resetform('MySecretToken', 'ValidTokenUser');
181193
$expectedResponse = new TemplateResponse('core',
@@ -197,7 +209,7 @@ public function testEmailUnsuccessful() {
197209
]);
198210

199211
$this->logger->expects($this->exactly(0))
200-
->method('logException');
212+
->method('error');
201213
$this->logger->expects($this->exactly(2))
202214
->method('warning');
203215

@@ -398,7 +410,7 @@ public function testEmailCantSendException() {
398410
->will($this->throwException(new \Exception()));
399411

400412
$this->logger->expects($this->exactly(1))
401-
->method('logException');
413+
->method('error');
402414

403415
$response = $this->lostController->email('ExistingUser');
404416
$expectedResponse = new JSONResponse(['status' => 'success']);
@@ -561,7 +573,7 @@ public function testSendEmailNoEmail() {
561573
->willReturn($user);
562574

563575
$this->logger->expects($this->exactly(0))
564-
->method('logException');
576+
->method('error');
565577
$this->logger->expects($this->once())
566578
->method('warning');
567579

@@ -644,7 +656,7 @@ public function testTwoUsersWithSameEmail() {
644656
->willReturn([$user1, $user2]);
645657

646658
$this->logger->expects($this->exactly(0))
647-
->method('logException');
659+
->method('error');
648660
$this->logger->expects($this->once())
649661
->method('warning');
650662

0 commit comments

Comments
 (0)