Skip to content

Commit 00c44bb

Browse files
SiegSieg
committed
OXDEV-9078 Add the flag for user to enable/disable the 2FA
The interface feature is not there yet, should be implemented soon. Signed-off-by: Anton Fedurtsya <anton@fedurtsya.com>
1 parent af164e2 commit 00c44bb

11 files changed

Lines changed: 126 additions & 69 deletions

File tree

‎migration/data/Version20251128093245.php‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,16 +12,11 @@
1212
use Doctrine\DBAL\Schema\Schema;
1313
use Doctrine\Migrations\AbstractMigration;
1414

15-
// todo-critical: remove this
1615
final class Version20251128093245 extends AbstractMigration
1716
{
1817
public function up(Schema $schema): void
1918
{
20-
$this->addSql('ALTER TABLE `oxuser` ADD column `OESMOTPCODE` VARCHAR(128) default NULL COMMENT "OTP code"');
21-
$this->addSql('ALTER TABLE `oxuser` ADD column `OESMOTPEXPTIME` DATETIME default NULL COMMENT "OTP code expiration time"');
22-
$this->addSql('ALTER TABLE `oxuser` ADD column `OESMOTPATTEMPTS` INT NOT NULL default 0 COMMENT "OTP code attempts"');
23-
$this->addSql('ALTER TABLE `oxuser` ADD column `OESMOTPLASTSENT` DATETIME default NULL COMMENT "Last OTP sent timestamp"');
24-
$this->addSql('ALTER TABLE `oxuser` ADD column `OESMEXTERNALAUTH` TINYINT(1) NOT NULL default 0 COMMENT "User registered via external authentication"');
19+
$this->addSql('ALTER TABLE `oxuser` ADD column `OE2FAENABLED` TINYINT(1) NOT NULL default 0 COMMENT "User has 2FA enabled"');
2520
}
2621

2722
public function down(Schema $schema): void

‎src/Authentication/TwoFactorAuth/DTO/User.php‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ class User implements UserInterface
1414
public function __construct(
1515
private string $userId,
1616
private string $email,
17+
private bool $twoFAEnabled,
1718
) {
1819
}
1920

@@ -26,4 +27,9 @@ public function getEmail(): string
2627
{
2728
return $this->email;
2829
}
30+
31+
public function isTwoFAEnabled(): bool
32+
{
33+
return $this->twoFAEnabled;
34+
}
2935
}

‎src/Authentication/TwoFactorAuth/DTO/UserInterface.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,4 +14,6 @@ interface UserInterface
1414
public function getUserId(): string;
1515

1616
public function getEmail(): string;
17+
18+
public function isTwoFAEnabled(): bool;
1719
}

‎src/Authentication/TwoFactorAuth/Infrastructure/Factory/UserFactory.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ public function createFromModel(User $userModel): UserInterface
2020
return new UserDto(
2121
userId: $userModel->getId(),
2222
email: $userModel->getFieldData('oxusername'),
23+
twoFAEnabled: (bool) $userModel->getFieldData('oe2faenabled'),
2324
);
2425
}
2526
}

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
use OxidEsales\Eshop\Core\Utils;
1313
use OxidEsales\EshopCommunity\Internal\Framework\Session\SessionInterface;
1414
use OxidEsales\SecurityModule\Authentication\Service\InternalRedirectServiceInterface;
15+
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Infrastructure\Repository\UserRepositoryInterface;
1516
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Infrastructure\Service\UserLoginAdapterInterface;
1617
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Settings\TwoFASettingsInterface;
1718

@@ -29,6 +30,7 @@ public function __construct(
2930
private SessionInterface $session,
3031
private UserLoginAdapterInterface $loginAdapter,
3132
private InternalRedirectServiceInterface $redirectService,
33+
private UserRepositoryInterface $userRepository,
3234
) {
3335
}
3436

@@ -58,4 +60,14 @@ public function isChallengeVerified(string $userId): bool
5860
{
5961
return $this->twoFAService->isVerified($userId);
6062
}
63+
64+
public function isTwoFARequired(string $userId): bool
65+
{
66+
if (!$this->settings->isTwoFactorAuthEnabled()) {
67+
return false;
68+
}
69+
70+
$user = $this->userRepository->getUserById($userId);
71+
return $user->isTwoFAEnabled();
72+
}
6173
}

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,6 @@ public function getPendingUserId(): string;
1818
public function loginUser(string $userId): void;
1919

2020
public function isChallengeVerified(string $userId): bool;
21+
22+
public function isTwoFARequired(string $userId): bool;
2123
}

‎src/Shared/Model/User.php‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@
1313
use OxidEsales\Eshop\Core\Exception\UserException;
1414
use OxidEsales\Eshop\Core\Registry;
1515
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\TwoFAUserServiceInterface;
16-
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Settings\TwoFASettingsInterface;
1716
use OxidEsales\SecurityModule\Captcha\Captcha\Image\Exception\CaptchaValidateException as ImageCaptchaException;
1817
use OxidEsales\SecurityModule\Captcha\Captcha\HoneyPot\Exception\CaptchaValidateException as HoneyPotCaptchaException;
1918
use OxidEsales\SecurityModule\Captcha\Service\CaptchaServiceInterface;
@@ -90,10 +89,9 @@ protected function onLogin($userName, #[\SensitiveParameter] $password)
9089
parent::onLogin($userName, $password);
9190

9291
$userId = $this->getId();
93-
$settingsService = $this->getService(TwoFASettingsInterface::class);
94-
if ($userId && $settingsService->isTwoFactorAuthEnabled() && !$this->isAdmin()) {
92+
if ($userId && !$this->isAdmin()) {
9593
$twoFAUserService = $this->getService(TwoFAUserServiceInterface::class);
96-
if (!$twoFAUserService->isChallengeVerified($userId)) {
94+
if ($twoFAUserService->isTwoFARequired($userId) && !$twoFAUserService->isChallengeVerified($userId)) {
9795
$twoFAUserService->startChallengeForUser($userId);
9896
}
9997
}

‎tests/Integration/Shared/Model/UserTest.php‎

Lines changed: 40 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@
1919
use OxidEsales\EshopCommunity\Core\Di\ContainerFacade;
2020
// phpcs:ignore Generic.Files.LineLength
2121
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\TwoFAUserServiceInterface;
22-
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Settings\TwoFASettingsInterface;
2322
use OxidEsales\SecurityModule\Captcha\Service\ModuleSettingsServiceInterface as CaptchaSettingsServiceInterface;
2423
use OxidEsales\SecurityModule\Shared\Model\User as SecurityModuleUser;
2524
use OxidEsales\SecurityModule\Tests\Integration\IntegrationTestCase;
@@ -63,7 +62,9 @@ public function testCheckValuesWithInvalidCaptcha()
6362
$captchaSettings = $this->createStub(CaptchaSettingsServiceInterface::class);
6463
$captchaSettings->method('isCaptchaEnabled')->willReturn(true);
6564

66-
$sut = $this->getSut([CaptchaSettingsServiceInterface::class => $captchaSettings]);
65+
$sut = $this->getSut([
66+
CaptchaSettingsServiceInterface::class => $captchaSettings,
67+
]);
6768
$sut->checkValues('', '', '', [], []);
6869
}
6970

@@ -85,7 +86,9 @@ public function testCheckValuesWithEmptyCaptcha()
8586
$captchaSettings = $this->createStub(CaptchaSettingsServiceInterface::class);
8687
$captchaSettings->method('isCaptchaEnabled')->willReturn(true);
8788

88-
$sut = $this->getSut([CaptchaSettingsServiceInterface::class => $captchaSettings]);
89+
$sut = $this->getSut([
90+
CaptchaSettingsServiceInterface::class => $captchaSettings,
91+
]);
8992
$sut->checkValues('', '', '', [], []);
9093
}
9194

@@ -106,7 +109,9 @@ public function testLoginWithInvalidCaptcha()
106109
$captchaSettings = $this->createStub(CaptchaSettingsServiceInterface::class);
107110
$captchaSettings->method('isCaptchaEnabled')->willReturn(true);
108111

109-
$sut = $this->getSut([CaptchaSettingsServiceInterface::class => $captchaSettings]);
112+
$sut = $this->getSut([
113+
CaptchaSettingsServiceInterface::class => $captchaSettings,
114+
]);
110115
$sut->login('', '');
111116
}
112117

@@ -127,7 +132,9 @@ public function testLoginWithEmptyCaptcha()
127132
$captchaSettings = $this->createStub(CaptchaSettingsServiceInterface::class);
128133
$captchaSettings->method('isCaptchaEnabled')->willReturn(true);
129134

130-
$sut = $this->getSut([CaptchaSettingsServiceInterface::class => $captchaSettings]);
135+
$sut = $this->getSut([
136+
CaptchaSettingsServiceInterface::class => $captchaSettings,
137+
]);
131138
$sut->login('', '');
132139
}
133140

@@ -145,7 +152,9 @@ public function testLoginWithValidCaptchaAndValidCredentials(): void
145152
$captchaSettings = $this->createStub(CaptchaSettingsServiceInterface::class);
146153
$captchaSettings->method('isCaptchaEnabled')->willReturn(true);
147154

148-
$sut = $this->getSut([CaptchaSettingsServiceInterface::class => $captchaSettings]);
155+
$sut = $this->getSut([
156+
CaptchaSettingsServiceInterface::class => $captchaSettings,
157+
]);
149158
$result = $sut->login(self::TWO_FA_USER_NAME, self::TWO_FA_USER_PASSWORD);
150159

151160
$this->assertTrue($result);
@@ -155,20 +164,15 @@ public function testLoginWith2FAEnabledAndUnverifiedChallengeTriggersChallenge()
155164
{
156165
$userId = $this->getTwoFAUserId();
157166

158-
$twoFaSettings = $this->createStub(TwoFASettingsInterface::class);
159-
$twoFaSettings->method('isTwoFactorAuthEnabled')->willReturn(true);
160-
161167
$userServiceSpy = $this->createMock(TwoFAUserServiceInterface::class);
162-
$userServiceSpy->method('isChallengeVerified')
163-
->with($userId)
164-
->willReturn(false);
168+
$userServiceSpy->method('isTwoFARequired')->with($userId)->willReturn(true);
169+
$userServiceSpy->method('isChallengeVerified')->with($userId)->willReturn(false);
165170
$userServiceSpy->expects($this->once())
166171
->method('startChallengeForUser')
167172
->with($userId);
168173

169174
$sut = $this->getSut([
170-
TwoFASettingsInterface::class => $twoFaSettings,
171-
TwoFAUserServiceInterface::class => $userServiceSpy
175+
TwoFAUserServiceInterface::class => $userServiceSpy,
172176
]);
173177
$sut->login(self::TWO_FA_USER_NAME, self::TWO_FA_USER_PASSWORD);
174178
}
@@ -177,19 +181,13 @@ public function testLoginWith2FAEnabledAndVerifiedChallengeNOTTriggeringChalleng
177181
{
178182
$userId = $this->getTwoFAUserId();
179183

180-
$twoFaSettings = $this->createStub(TwoFASettingsInterface::class);
181-
$twoFaSettings->method('isTwoFactorAuthEnabled')->willReturn(true);
182-
183184
$userServiceSpy = $this->createMock(TwoFAUserServiceInterface::class);
184-
$userServiceSpy->method('isChallengeVerified')
185-
->with($userId)
186-
->willReturn(true);
187-
$userServiceSpy->expects($this->never())
188-
->method('startChallengeForUser');
185+
$userServiceSpy->method('isTwoFARequired')->with($userId)->willReturn(true);
186+
$userServiceSpy->method('isChallengeVerified')->with($userId)->willReturn(true);
187+
$userServiceSpy->expects($this->never())->method('startChallengeForUser');
189188

190189
$sut = $this->getSut([
191-
TwoFASettingsInterface::class => $twoFaSettings,
192-
TwoFAUserServiceInterface::class => $userServiceSpy
190+
TwoFAUserServiceInterface::class => $userServiceSpy,
193191
]);
194192

195193
$result = $sut->login(self::TWO_FA_USER_NAME, self::TWO_FA_USER_PASSWORD);
@@ -200,14 +198,11 @@ public function testLoginWithoutPasswordOnLoadedUserWith2FAEnabledAndChallengeVe
200198
{
201199
$userId = $this->getTwoFAUserId();
202200

203-
$twoFaSettings = $this->createStub(TwoFASettingsInterface::class);
204-
$twoFaSettings->method('isTwoFactorAuthEnabled')->willReturn(true);
205-
206201
$userServiceMock = $this->createMock(TwoFAUserServiceInterface::class);
202+
$userServiceMock->method('isTwoFARequired')->with($userId)->willReturn(true);
207203
$userServiceMock->method('isChallengeVerified')->with($userId)->willReturn(true);
208204

209205
$sut = $this->getSut([
210-
TwoFASettingsInterface::class => $twoFaSettings,
211206
TwoFAUserServiceInterface::class => $userServiceMock,
212207
]);
213208
$sut->load($userId);
@@ -216,21 +211,18 @@ public function testLoginWithoutPasswordOnLoadedUserWith2FAEnabledAndChallengeVe
216211
$this->assertTrue($result);
217212
}
218213

219-
public function testLoginWithoutPasswordOnLoadedUserWith2FAEnabledAndChallengeNotVerifiedNOTLogsUserIn(): void
214+
public function testLoginWithoutPasswordOnLoadedUserWith2FAEnabledAndChallengeNotVerifiedTriggersChallenge(): void
220215
{
221216
$userId = $this->getTwoFAUserId();
222217

223-
$twoFaSettings = $this->createStub(TwoFASettingsInterface::class);
224-
$twoFaSettings->method('isTwoFactorAuthEnabled')->willReturn(true);
225-
226218
$userServiceSpy = $this->createMock(TwoFAUserServiceInterface::class);
219+
$userServiceSpy->method('isTwoFARequired')->with($userId)->willReturn(true);
227220
$userServiceSpy->method('isChallengeVerified')->with($userId)->willReturn(false);
228221
$userServiceSpy->expects($this->once())
229222
->method('startChallengeForUser')
230223
->with($userId);
231224

232225
$sut = $this->getSut([
233-
TwoFASettingsInterface::class => $twoFaSettings,
234226
TwoFAUserServiceInterface::class => $userServiceSpy,
235227
]);
236228
$sut->load($userId);
@@ -246,10 +238,7 @@ public function testLoginWith2FAEnabledAndBadCredentialsThrowsException(
246238
$this->expectException(UserException::class);
247239
$this->expectExceptionMessage('ERROR_MESSAGE_USER_NOVALIDLOGIN');
248240

249-
$twoFaSettings = $this->createStub(TwoFASettingsInterface::class);
250-
$twoFaSettings->method('isTwoFactorAuthEnabled')->willReturn(true);
251-
252-
$sut = $this->getSut([TwoFASettingsInterface::class => $twoFaSettings]);
241+
$sut = $this->getSut();
253242
$sut->login($username, $password);
254243
}
255244

@@ -259,20 +248,17 @@ public static function invalidLoginDataProvider(): Generator
259248
yield 'nonexistent user' => ['nonexistent@test.com', 'anypassword'];
260249
}
261250

262-
public function testLoginWith2FADisabledDoesntTouch2FA(): void
251+
public function testLogin2FANotRequiredDoesntTouchChallengeAndJustLogins(): void
263252
{
264-
$twoFaSettings = $this->createStub(TwoFASettingsInterface::class);
265-
$twoFaSettings->method('isTwoFactorAuthEnabled')->willReturn(false);
253+
$userId = $this->getTwoFAUserId();
266254

267255
$userServiceSpy = $this->createMock(TwoFAUserServiceInterface::class);
268-
$userServiceSpy->expects($this->never())
269-
->method('isChallengeVerified');
270-
$userServiceSpy->expects($this->never())
271-
->method('startChallengeForUser');
256+
$userServiceSpy->method('isTwoFARequired')->with($userId)->willReturn(false);
257+
$userServiceSpy->expects($this->never())->method('isChallengeVerified');
258+
$userServiceSpy->expects($this->never())->method('startChallengeForUser');
272259

273260
$sut = $this->getSut([
274-
TwoFASettingsInterface::class => $twoFaSettings,
275-
TwoFAUserServiceInterface::class => $userServiceSpy
261+
TwoFAUserServiceInterface::class => $userServiceSpy,
276262
]);
277263

278264
$result = $sut->login(self::TWO_FA_USER_NAME, self::TWO_FA_USER_PASSWORD);
@@ -281,16 +267,16 @@ public function testLoginWith2FADisabledDoesntTouch2FA(): void
281267

282268
private function getSut(array $serviceOverrides = []): SecurityModuleUser
283269
{
284-
$captchaDefault = $this->createStub(CaptchaSettingsServiceInterface::class);
285-
$captchaDefault->method('isCaptchaEnabled')->willReturn(false);
286-
287-
$twoFaDefault = $this->createStub(TwoFASettingsInterface::class);
288-
$twoFaDefault->method('isTwoFactorAuthEnabled')->willReturn(false);
289-
290270
$services = array_merge(
291271
[
292-
CaptchaSettingsServiceInterface::class => $captchaDefault,
293-
TwoFASettingsInterface::class => $twoFaDefault,
272+
CaptchaSettingsServiceInterface::class => $this->createConfiguredStub(
273+
CaptchaSettingsServiceInterface::class,
274+
['isCaptchaEnabled' => false]
275+
),
276+
TwoFAUserServiceInterface::class => $this->createConfiguredStub(
277+
TwoFAUserServiceInterface::class,
278+
['isTwoFARequired' => false]
279+
),
294280
],
295281
$serviceOverrides
296282
);

‎tests/Unit/Authentication/TwoFactorAuth/DTO/UserTest.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,12 @@ public function initializeAndReadProperties(): void
2222
$sut = new User(
2323
userId: $userId = uniqid(),
2424
email: $email = uniqid(),
25+
twoFAEnabled: $twoFAEnabled = (bool) random_int(0, 1),
2526
);
2627

2728
$this->assertInstanceOf(UserInterface::class, $sut);
2829
$this->assertSame($userId, $sut->getUserId());
2930
$this->assertSame($email, $sut->getEmail());
31+
$this->assertSame($twoFAEnabled, $sut->isTwoFAEnabled());
3032
}
3133
}

‎tests/Unit/Authentication/TwoFactorAuth/Infrastructure/Factory/UserFactoryTest.php‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,18 +18,23 @@
1818
class UserFactoryTest extends TestCase
1919
{
2020
#[Test]
21-
public function createFromModelMapsIdAndEmail(): void
21+
public function createFromModelMapsAllProperties(): void
2222
{
23+
$twoFAEnabledRaw = random_int(0, 1);
24+
2325
$userModelStub = $this->createStub(User::class);
2426
$userModelStub->method('getId')->willReturn($userId = uniqid());
25-
$userModelStub->method('getFieldData')->with('oxusername')->willReturn($email = uniqid());
26-
27-
$sut = $this->getSut();
27+
$userModelStub->method('getFieldData')
28+
->willReturnMap([
29+
['oxusername', $email = uniqid()],
30+
['oe2faenabled', $twoFAEnabledRaw],
31+
]);
2832

29-
$result = $sut->createFromModel(userModel: $userModelStub);
33+
$result = $this->getSut()->createFromModel(userModel: $userModelStub);
3034

3135
$this->assertSame($userId, $result->getUserId());
3236
$this->assertSame($email, $result->getEmail());
37+
$this->assertSame((bool) $twoFAEnabledRaw, $result->isTwoFAEnabled());
3338
}
3439

3540
#[Test]

0 commit comments

Comments
 (0)