Skip to content

Commit 1daefc7

Browse files
TitaKolevaTitaKoleva
committed
OXDEV-9992 Finalize generate otp code process and add exceptions
1 parent 49c0735 commit 1daefc7

12 files changed

Lines changed: 319 additions & 44 deletions

File tree

‎src/Authentication/TwoFactorAuth/Controller/TwoFactorAuthController.php‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -40,13 +40,4 @@ public function handleOTP(): void
4040
Registry::getUtilsView()->addErrorToDisplay($e->getMessage());
4141
}
4242
}
43-
44-
public function generate(): void
45-
{
46-
//todo: stop execution if not ajax
47-
//todo: prevent spam by rate limiting
48-
//todo: should return json response with success or error message
49-
$authorizeService = $this->getService(AuthorizeServiceInterface::class);
50-
$authorizeService->generate();
51-
}
5243
}

‎src/Authentication/TwoFactorAuth/Exception/UserNotFoundException.php‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
<?php
2+
3+
/**
4+
* Copyright © OXID eSales AG. All rights reserved.
5+
* See LICENSE file for license details.
6+
*/
7+
8+
declare(strict_types=1);
9+
10+
namespace OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Exception;
11+
12+
class UserNotFoundException extends \Exception
13+
{
14+
}

‎src/Authentication/TwoFactorAuth/Infrastructure/Repository/UserRepository.php‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,8 @@
1313
use Doctrine\DBAL\Result;
1414
use OxidEsales\EshopCommunity\Internal\Framework\Database\QueryBuilderFactoryInterface;
1515
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\DTO\User as UserDTO;
16+
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Exception\UserNotFoundException;
1617
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Infrastructure\Factory\UserFactoryInterface;
17-
use RuntimeException;
1818

1919
class UserRepository implements UserRepositoryInterface
2020
{
@@ -26,7 +26,6 @@ public function __construct(
2626

2727
public function getUserOTPData(string $userName): UserDTO
2828
{
29-
//todo: exception if not found
3029
$builder = $this->queryBuilderFactory->create();
3130
$builder->select([
3231
'OXID',
@@ -42,8 +41,7 @@ public function getUserOTPData(string $userName): UserDTO
4241
$queryResult = $builder->execute();
4342
$userData = $queryResult->fetchAssociative();
4443
if (!$userData) {
45-
//todo: throw correct exception
46-
throw new RuntimeException('User not found');
44+
throw new UserNotFoundException();
4745
}
4846

4947
return new UserDTO(
@@ -90,7 +88,7 @@ public function resetCodeFields(string $userId): void
9088
$userModel->save();
9189
}
9290

93-
public function getUserPasswordHash(string $userName): string
91+
public function getUserPasswordHash(string $userName): ?string
9492
{
9593
$builder = $this->queryBuilderFactory->create();
9694
$builder->select('OXPASSWORD')
@@ -100,6 +98,8 @@ public function getUserPasswordHash(string $userName): string
10098

10199
/** @var Result $queryResult */
102100
$queryResult = $builder->execute();
103-
return $queryResult->fetchOne();
101+
$userPass = $queryResult->fetchOne();
102+
103+
return $userPass ?: null;
104104
}
105105
}

‎src/Authentication/TwoFactorAuth/Infrastructure/Repository/UserRepositoryInterface.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,5 +20,5 @@ public function resetCodeFields(string $userId): void;
2020

2121
public function addOTPtoUser(string $userId, string $otp, DateTime $expiresAt): bool;
2222

23-
public function getUserPasswordHash(string $userId): string;
23+
public function getUserPasswordHash(string $userId): ?string;
2424
}

‎src/Authentication/TwoFactorAuth/Service/UserService.php‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,8 @@
99

1010
namespace OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service;
1111

12-
use OxidEsales\Eshop\Core\Registry;
12+
use OxidEsales\Eshop\Core\Request;
13+
use OxidEsales\Eshop\Core\Utils;
1314
use OxidEsales\EshopCommunity\Internal\Domain\Authentication\Bridge\PasswordServiceBridgeInterface;
1415
use OxidEsales\EshopCommunity\Internal\Framework\Session\SessionInterface;
1516
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Infrastructure\Repository\UserRepositoryInterface;
@@ -20,7 +21,9 @@ public function __construct(
2021
private AuthorizeServiceInterface $authorizeService,
2122
private UserRepositoryInterface $userRepository,
2223
private PasswordServiceBridgeInterface $pwdServiceBridge,
23-
private SessionInterface $session
24+
private SessionInterface $session,
25+
private Request $request,
26+
private Utils $utils,
2427
) {
2528
}
2629

@@ -29,21 +32,27 @@ public function handleLogin(string $userName): void
2932
$this->session->set(AuthorizeService::USER_SESSION_KEY, $userName);
3033
$this->session->set(
3134
AuthorizeService::OTP_TARGET_URL,
32-
//todo: bind registry
33-
Registry::getRequest()->getRequestUrl()
35+
$this->request->getRequestUrl()
3436
);
3537

38+
//todo: prevent spam by rate limiting
3639
$this->authorizeService->generate();
3740

38-
//todo: return full url
3941
$redirectUrl = $this->authorizeService->getVerificationUrl();
40-
Registry::getUtils()->redirect(Registry::getConfig()->getShopHomeUrl() . 'cl=' . $redirectUrl);
42+
$this->utils->redirect($redirectUrl);
4143
}
4244

4345
public function checkPassword(string $userName, string $password): bool
4446
{
45-
//todo: got exception if user not found
46-
$userPasswordHash = $this->userRepository->getUserPasswordHash($userName);
47+
try {
48+
$userPasswordHash = $this->userRepository->getUserPasswordHash($userName);
49+
} catch (\Throwable $e) {
50+
return false;
51+
}
52+
53+
if ($userPasswordHash === null) {
54+
return false;
55+
}
4756

4857
return $this->pwdServiceBridge
4958
->verifyPassword($password, $userPasswordHash);

‎src/Authentication/TwoFactorAuth/Service/Verificator/OTP/OTPVerificator.php‎

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99

1010
namespace OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\Verificator\OTP;
1111

12-
use OxidEsales\Eshop\Core\Registry;
12+
use OxidEsales\Eshop\Core\Config;
1313
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Exception\InvalidCodeException;
1414
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Infrastructure\Repository\UserRepositoryInterface;
1515
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\Verificator\OTP\Generator\OTPGeneratorInterface;
@@ -22,6 +22,7 @@ public function __construct(
2222
private OTPGeneratorInterface $otpGenerator,
2323
private OTPValidatorInterface $otpValidator,
2424
private UserRepositoryInterface $userRepository,
25+
private Config $config,
2526
) {
2627
}
2728

@@ -50,10 +51,7 @@ public function validateCode(string $userName, string $inputCode): void
5051

5152
public function generate(string $userName): string
5253
{
53-
//todo: userId from parameter or DTO
5454
$otpData = $this->userRepository->getUserOTPData($userName);
55-
56-
//todo: stop if user not found?
5755
//todo: wait time between generations, in case of abuse like
5856
//spamming the generate button or
5957
//user hit limit and try to generate new code to bypass it
@@ -63,6 +61,6 @@ public function generate(string $userName): string
6361

6462
public function getVerificationUrl(): string
6563
{
66-
return 'twofactorauth';
64+
return $this->config->getShopHomeUrl() . 'cl=twofactorauth';
6765
}
6866
}

‎src/Authentication/TwoFactorAuth/Service/Verificator/services.yaml‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ services:
55
_defaults:
66
autowire: true
77
public: false
8+
bind:
9+
OxidEsales\Eshop\Core\Config: '@=service("OxidEsales\\SecurityModule\\Core\\Registry").getConfig()'
810

911
OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\Verificator\OTP\OTPVerificator:
1012
tags: [ 'security.twofa.tag.verificator' ]

‎src/Authentication/TwoFactorAuth/Service/services.yaml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@ services:
55
_defaults:
66
autowire: true
77
public: false
8+
bind:
9+
OxidEsales\Eshop\Core\Request: '@=service("OxidEsales\\SecurityModule\\Core\\Registry").getRequest()'
10+
OxidEsales\Eshop\Core\Utils: '@=service("OxidEsales\\SecurityModule\\Core\\Registry").getUtils()'
811

912
OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\ModuleSettingsServiceInterface:
1013
class: OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\ModuleSettingsService

‎src/Shared/Model/User.php‎

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -61,11 +61,11 @@ public function checkValues($sLogin, $sPassword, $sPassword2, $aInvAddress, $aDe
6161
*/
6262
public function login($userName, $password, $setSessionCookie = false): bool
6363
{
64-
//todo: login should be reworded, disabled captcha is killing OTP login flow
65-
if (!$this->isCaptchaEnabled()) {
66-
// return parent::login($userName, $password, $setSessionCookie);
64+
if ($this->isAdmin()) {
65+
return parent::login($userName, $password, $setSessionCookie);
6766
}
68-
if (!$this->isAdmin()) {
67+
68+
if ($this->isCaptchaEnabled()) {
6969
$captchaService = $this->getService(CaptchaServiceInterface::class);
7070

7171
try {
@@ -77,19 +77,22 @@ public function login($userName, $password, $setSessionCookie = false): bool
7777
}
7878
}
7979

80-
if (!$this->isOTPEnabled() || $this->isAdmin()) {
81-
return parent::login($userName, $password, $setSessionCookie);
82-
}
80+
if ($this->isOTPEnabled()) {
81+
$userService = $this->getService(UserServiceInterface::class);
82+
if (!$userService->checkPassword($userName, $password)) {
83+
throw oxNew(UserException::class, 'ERROR_MESSAGE_USER_NOVALIDLOGIN');
84+
}
8385

84-
$userService = $this->getService(UserServiceInterface::class);
85-
if (!$userService->checkPassword($userName, $password)) {
86-
//todo: log invalid login attempt and throw exception
87-
return false; // invalid login
88-
}
86+
try {
87+
$userService->handleLogin($userName);
88+
} catch (\Exception $e) {
89+
throw oxNew(UserException::class, 'ERROR_MESSAGE_USER_NOVALIDLOGIN');
90+
}
8991

90-
$userService->handleLogin($userName);
92+
return false;
93+
}
9194

92-
return false;
95+
return parent::login($userName, $password, $setSessionCookie);
9396
}
9497

9598
private function isCaptchaEnabled(): bool

‎tests/Unit/Authentication/TwoFactorAuth/Infrastructure/Repository/UserRepositoryTest.php‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,75 @@
1+
<?php
2+
3+
/**
4+
* Copyright © OXID eSales AG. All rights reserved.
5+
* See LICENSE file for license details.
6+
*/
7+
8+
declare(strict_types=1);
9+
10+
namespace OxidEsales\SecurityModule\Tests\Unit\Authentication\TwoFactorAuth\Infrastructure\Repository;
11+
12+
use DateTime;
13+
use Doctrine\DBAL\Query\QueryBuilder;
14+
use Doctrine\DBAL\Result;
15+
use OxidEsales\EshopCommunity\Internal\Framework\Database\QueryBuilderFactoryInterface;
16+
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Infrastructure\Factory\UserFactoryInterface;
17+
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Infrastructure\Repository\UserRepository;
18+
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Infrastructure\Repository\UserRepositoryInterface;
19+
use PHPUnit\Framework\TestCase;
20+
21+
class UserRepositoryTest extends TestCase
22+
{
23+
public function getSut(
24+
UserFactoryInterface $userFactory = null,
25+
QueryBuilderFactoryInterface $qbFactory = null,
26+
): UserRepositoryInterface {
27+
return new UserRepository(
28+
userFactory: $userFactory ?? $this->createMock(UserFactoryInterface::class),
29+
queryBuilderFactory: $qbFactory ?? $this->createMock(QueryBuilderFactoryInterface::class),
30+
);
31+
}
32+
33+
public function testGetUserOtpDataReturnsDto(): void
34+
{
35+
$data = [
36+
'OXID' => uniqid(),
37+
'OESMOTPCODE' => uniqid(),
38+
'OESMOTPATTEMPTS' => random_int(1, 10),
39+
'OESMOTPEXPTIME' => '2026-01-01T10:00:00',
40+
];
41+
42+
$qb = $this->createMock(QueryBuilder::class);
43+
$qb->method('select')->willReturnSelf();
44+
$qb->method('from')->willReturnSelf();
45+
$qb->method('where')->willReturnSelf();
46+
$qb->method('setParameter')->willReturnSelf();
47+
48+
$qbFactory = $this->createMock(QueryBuilderFactoryInterface::class);
49+
$qbFactory->expects($this->once())
50+
->method('create')
51+
->willReturn($qb);
52+
53+
$result = $this->createMock(Result::class);
54+
$qb->expects($this->once())
55+
->method('execute')
56+
->willReturn($result);
57+
58+
$result->expects($this->once())
59+
->method('fetchAssociative')
60+
->willReturn($data);
61+
62+
$repository = $this->getSut(
63+
qbFactory: $qbFactory,
64+
);
65+
$dto = $repository->getUserOTPData('user');
66+
67+
$this->assertSame($data['OXID'], $dto->getId());
68+
$this->assertSame($data['OESMOTPATTEMPTS'], $dto->getAttempts());
69+
$this->assertSame($data['OESMOTPCODE'], $dto->getCode());
70+
$this->assertEquals(
71+
new DateTime($data['OESMOTPEXPTIME']),
72+
$dto->getExpiresAt()
73+
);
74+
}
75+
}

0 commit comments

Comments
 (0)