Skip to content

Commit 55c969c

Browse files
SiegSieg
committed
OXDEV-9078 Improve and cleanup the 2FA part of the UserTest
Signed-off-by: Anton Fedurtsya <anton@fedurtsya.com>
1 parent 32f08a8 commit 55c969c

1 file changed

Lines changed: 146 additions & 100 deletions

File tree

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

Lines changed: 146 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -14,21 +14,20 @@
1414
use OxidEsales\Eshop\Core\Exception\UserException;
1515
use OxidEsales\Eshop\Core\Registry;
1616
use OxidEsales\Eshop\Core\Request;
17-
use OxidEsales\Eshop\Core\Utils;
17+
use Generator;
18+
use PHPUnit\Framework\Attributes\DataProvider;
1819
use OxidEsales\EshopCommunity\Core\Di\ContainerFacade;
19-
use DateTimeImmutable;
2020
// phpcs:ignore Generic.Files.LineLength
21-
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\OTP\Infrastructure\Repository\OtpChallengeStateRepositoryInterface;
22-
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\TwoFAUserService;
21+
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\TwoFAUserServiceInterface;
2322
use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Settings\TwoFASettingsInterface;
2423
use OxidEsales\SecurityModule\Captcha\Service\ModuleSettingsServiceInterface as CaptchaSettingsServiceInterface;
2524
use OxidEsales\SecurityModule\Shared\Model\User as SecurityModuleUser;
2625
use OxidEsales\SecurityModule\Tests\Integration\IntegrationTestCase;
2726

2827
class UserTest extends IntegrationTestCase
2928
{
30-
private const OTP_USER_NAME = 'user@oxid-esales.com';
31-
private const OTP_USER_PASSWORD = 'useruser';
29+
private const TWO_FA_USER_NAME = 'user@oxid-esales.com';
30+
private const TWO_FA_USER_PASSWORD = 'useruser';
3231

3332
protected Request $requestMock;
3433

@@ -61,8 +60,11 @@ public function testCheckValuesWithInvalidCaptcha()
6160
$message = Registry::getLang()->translateString("ERROR_INVALID_CAPTCHA");
6261
$this->expectExceptionMessage($message);
6362

64-
$subject = $this->createUserMock(captchaEnabled: true, twoFaEnabled: false);
65-
$subject->checkValues('', '', '', [], []);
63+
$captchaSettings = $this->createStub(CaptchaSettingsServiceInterface::class);
64+
$captchaSettings->method('isCaptchaEnabled')->willReturn(true);
65+
66+
$sut = $this->getSut([CaptchaSettingsServiceInterface::class => $captchaSettings]);
67+
$sut->checkValues('', '', '', [], []);
6668
}
6769

6870
public function testCheckValuesWithEmptyCaptcha()
@@ -80,8 +82,11 @@ public function testCheckValuesWithEmptyCaptcha()
8082
$message = Registry::getLang()->translateString("ERROR_EMPTY_CAPTCHA");
8183
$this->expectExceptionMessage($message);
8284

83-
$subject = $this->createUserMock(captchaEnabled: true, twoFaEnabled: false);
84-
$subject->checkValues('', '', '', [], []);
85+
$captchaSettings = $this->createStub(CaptchaSettingsServiceInterface::class);
86+
$captchaSettings->method('isCaptchaEnabled')->willReturn(true);
87+
88+
$sut = $this->getSut([CaptchaSettingsServiceInterface::class => $captchaSettings]);
89+
$sut->checkValues('', '', '', [], []);
8590
}
8691

8792
public function testLoginWithInvalidCaptcha()
@@ -98,8 +103,11 @@ public function testLoginWithInvalidCaptcha()
98103
$this->expectException(UserException::class);
99104
$this->expectExceptionMessage("ERROR_INVALID_CAPTCHA");
100105

101-
$subject = $this->createUserMock(captchaEnabled: true, twoFaEnabled: false);
102-
$subject->login('', '');
106+
$captchaSettings = $this->createStub(CaptchaSettingsServiceInterface::class);
107+
$captchaSettings->method('isCaptchaEnabled')->willReturn(true);
108+
109+
$sut = $this->getSut([CaptchaSettingsServiceInterface::class => $captchaSettings]);
110+
$sut->login('', '');
103111
}
104112

105113
public function testLoginWithEmptyCaptcha()
@@ -116,8 +124,11 @@ public function testLoginWithEmptyCaptcha()
116124
$this->expectException(UserException::class);
117125
$this->expectExceptionMessage("ERROR_EMPTY_CAPTCHA");
118126

119-
$subject = $this->createUserMock(captchaEnabled: true, twoFaEnabled: false);
120-
$subject->login('', '');
127+
$captchaSettings = $this->createStub(CaptchaSettingsServiceInterface::class);
128+
$captchaSettings->method('isCaptchaEnabled')->willReturn(true);
129+
130+
$sut = $this->getSut([CaptchaSettingsServiceInterface::class => $captchaSettings]);
131+
$sut->login('', '');
121132
}
122133

123134
public function testLoginWithValidCaptchaAndValidCredentials(): void
@@ -131,139 +142,174 @@ public function testLoginWithValidCaptchaAndValidCredentials(): void
131142
return null;
132143
});
133144

134-
$subject = $this->createUserMock(captchaEnabled: true, twoFaEnabled: false);
135-
$result = $subject->login(self::OTP_USER_NAME, self::OTP_USER_PASSWORD);
145+
$captchaSettings = $this->createStub(CaptchaSettingsServiceInterface::class);
146+
$captchaSettings->method('isCaptchaEnabled')->willReturn(true);
147+
148+
$sut = $this->getSut([CaptchaSettingsServiceInterface::class => $captchaSettings]);
149+
$result = $sut->login(self::TWO_FA_USER_NAME, self::TWO_FA_USER_PASSWORD);
136150

137151
$this->assertTrue($result);
138152
}
139153

140-
public function testLoginWithOTPEnabledAndValidCredentialsRedirectsToOTP(): void
154+
public function testLoginWith2FAEnabledAndUnverifiedChallengeTriggersChallenge(): void
141155
{
142-
$utilsMock = $this->createMock(Utils::class);
143-
$utilsMock->expects($this->once())->method('redirect');
144-
Registry::set(Utils::class, $utilsMock);
156+
$userId = $this->getTwoFAUserId();
145157

146-
$subject = $this->createUserMock(captchaEnabled: false, twoFaEnabled: true);
147-
$subject->login(self::OTP_USER_NAME, self::OTP_USER_PASSWORD);
158+
$twoFaSettings = $this->createStub(TwoFASettingsInterface::class);
159+
$twoFaSettings->method('isTwoFactorAuthEnabled')->willReturn(true);
148160

149-
$this->assertNotNull(
150-
Registry::getSession()->getVariable(TwoFAUserService::USER_SESSION_KEY)
151-
);
152-
}
161+
$userServiceSpy = $this->createMock(TwoFAUserServiceInterface::class);
162+
$userServiceSpy->method('isChallengeVerified')
163+
->with($userId)
164+
->willReturn(false);
165+
$userServiceSpy->expects($this->once())
166+
->method('startChallengeForUser')
167+
->with($userId);
153168

154-
public function testLoginWithOTPEnabledAndInvalidCredentialsThrowsException(): void
155-
{
156-
$this->expectException(UserException::class);
157-
$this->expectExceptionMessage('ERROR_MESSAGE_USER_NOVALIDLOGIN');
158-
159-
$subject = $this->createUserMock(captchaEnabled: false, twoFaEnabled: true);
160-
$subject->login(self::OTP_USER_NAME, uniqid());
169+
$sut = $this->getSut([
170+
TwoFASettingsInterface::class => $twoFaSettings,
171+
TwoFAUserServiceInterface::class => $userServiceSpy
172+
]);
173+
$sut->login(self::TWO_FA_USER_NAME, self::TWO_FA_USER_PASSWORD);
161174
}
162175

163-
public function testLoginWithOTPEnabledAndNonExistentUserThrowsException(): void
176+
public function testLoginWith2FAEnabledAndVerifiedChallengeNOTTriggeringChallengeAndLogins(): void
164177
{
165-
$this->expectException(UserException::class);
166-
$this->expectExceptionMessage('ERROR_MESSAGE_USER_NOVALIDLOGIN');
178+
$userId = $this->getTwoFAUserId();
167179

168-
$subject = $this->createUserMock(captchaEnabled: false, twoFaEnabled: true);
169-
$subject->login('nonexistent@test.com', 'anypassword');
170-
}
180+
$twoFaSettings = $this->createStub(TwoFASettingsInterface::class);
181+
$twoFaSettings->method('isTwoFactorAuthEnabled')->willReturn(true);
171182

172-
public function testLoginWithOTPDisabledCallsParentLogin(): void
173-
{
174-
$subject = $this->createUserMock(captchaEnabled: false, twoFaEnabled: false);
175-
$result = $subject->login(self::OTP_USER_NAME, self::OTP_USER_PASSWORD);
183+
$userServiceSpy = $this->createMock(TwoFAUserServiceInterface::class);
184+
$userServiceSpy->method('isChallengeVerified')
185+
->with($userId)
186+
->willReturn(true);
187+
$userServiceSpy->expects($this->never())
188+
->method('startChallengeForUser');
189+
190+
$sut = $this->getSut([
191+
TwoFASettingsInterface::class => $twoFaSettings,
192+
TwoFAUserServiceInterface::class => $userServiceSpy
193+
]);
176194

195+
$result = $sut->login(self::TWO_FA_USER_NAME, self::TWO_FA_USER_PASSWORD);
177196
$this->assertTrue($result);
178-
$this->assertNull(
179-
Registry::getSession()->getVariable(TwoFAUserService::USER_SESSION_KEY)
180-
);
181197
}
182198

183-
public function testLoginWithOTPEnabledStoresUserIdInSession(): void
199+
public function testLoginWithoutPasswordOnLoadedUserWith2FAEnabledAndChallengeVerifiedLogsUserIn(): void
184200
{
185-
$utilsMock = $this->createMock(Utils::class);
186-
$utilsMock->method('redirect');
187-
Registry::set(Utils::class, $utilsMock);
201+
$userId = $this->getTwoFAUserId();
188202

189-
$subject = $this->createUserMock(captchaEnabled: false, twoFaEnabled: true);
190-
$subject->login(self::OTP_USER_NAME, self::OTP_USER_PASSWORD);
203+
$twoFaSettings = $this->createStub(TwoFASettingsInterface::class);
204+
$twoFaSettings->method('isTwoFactorAuthEnabled')->willReturn(true);
191205

192-
$sessionUserId = Registry::getSession()->getVariable(TwoFAUserService::USER_SESSION_KEY);
193-
$this->assertNotNull($sessionUserId);
194-
$this->assertEquals($subject->getId(), $sessionUserId);
206+
$userServiceMock = $this->createMock(TwoFAUserServiceInterface::class);
207+
$userServiceMock->method('isChallengeVerified')->with($userId)->willReturn(true);
208+
209+
$sut = $this->getSut([
210+
TwoFASettingsInterface::class => $twoFaSettings,
211+
TwoFAUserServiceInterface::class => $userServiceMock,
212+
]);
213+
$sut->load($userId);
214+
215+
$result = $sut->login(self::TWO_FA_USER_NAME, null);
216+
$this->assertTrue($result);
195217
}
196218

197-
public function testLoginWithVerifiedChallengeStateSkipsOTPRedirect(): void
219+
public function testLoginWithoutPasswordOnLoadedUserWith2FAEnabledAndChallengeNotVerifiedNOTLogsUserIn(): void
198220
{
199-
$userId = $this->getOTPUserId();
221+
$userId = $this->getTwoFAUserId();
200222

201-
$stateRepo = $this->get(OtpChallengeStateRepositoryInterface::class);
202-
$stateRepo->createChallengeState($userId, 'hash', new DateTimeImmutable('+5 minutes'));
203-
$stateRepo->markVerified($userId);
223+
$twoFaSettings = $this->createStub(TwoFASettingsInterface::class);
224+
$twoFaSettings->method('isTwoFactorAuthEnabled')->willReturn(true);
204225

205-
$utilsMock = $this->createMock(Utils::class);
206-
$utilsMock->expects($this->never())->method('redirect');
207-
Registry::set(Utils::class, $utilsMock);
226+
$userServiceSpy = $this->createMock(TwoFAUserServiceInterface::class);
227+
$userServiceSpy->method('isChallengeVerified')->with($userId)->willReturn(false);
228+
$userServiceSpy->expects($this->once())
229+
->method('startChallengeForUser')
230+
->with($userId);
208231

209-
$subject = $this->createUserMock(captchaEnabled: false, twoFaEnabled: true);
210-
$result = $subject->login(self::OTP_USER_NAME, self::OTP_USER_PASSWORD);
232+
$sut = $this->getSut([
233+
TwoFASettingsInterface::class => $twoFaSettings,
234+
TwoFAUserServiceInterface::class => $userServiceSpy,
235+
]);
236+
$sut->load($userId);
211237

212-
$this->assertTrue($result);
238+
$sut->login(self::TWO_FA_USER_NAME, null);
213239
}
214240

215-
public function testLoginWithOTPPassSessionVariableMismatchTriggersOTPFlow(): void
216-
{
217-
$mismatchedUserId = 'different-user-id';
218-
Registry::getSession()->setVariable('OTP_PASS', $mismatchedUserId);
241+
#[DataProvider('invalidLoginDataProvider')]
242+
public function testLoginWith2FAEnabledAndBadCredentialsThrowsException(
243+
string $username,
244+
string $password
245+
): void {
246+
$this->expectException(UserException::class);
247+
$this->expectExceptionMessage('ERROR_MESSAGE_USER_NOVALIDLOGIN');
219248

220-
$utilsMock = $this->createMock(Utils::class);
221-
$utilsMock->method('redirect');
222-
Registry::set(Utils::class, $utilsMock);
249+
$twoFaSettings = $this->createStub(TwoFASettingsInterface::class);
250+
$twoFaSettings->method('isTwoFactorAuthEnabled')->willReturn(true);
223251

224-
$subject = $this->createUserMock(captchaEnabled: false, twoFaEnabled: true);
225-
$subject->login(self::OTP_USER_NAME, self::OTP_USER_PASSWORD);
252+
$sut = $this->getSut([TwoFASettingsInterface::class => $twoFaSettings]);
253+
$sut->login($username, $password);
254+
}
226255

227-
$this->assertNotNull(
228-
Registry::getSession()->getVariable(TwoFAUserService::USER_SESSION_KEY),
229-
'USER_SESSION_KEY should be set when OTP flow is triggered'
230-
);
231-
$this->assertSame(
232-
$mismatchedUserId,
233-
Registry::getSession()->getVariable('OTP_PASS'),
234-
'OTP_PASS should NOT be cleared when user ID does not match'
235-
);
256+
public static function invalidLoginDataProvider(): Generator
257+
{
258+
yield 'invalid password' => [self::TWO_FA_USER_NAME, uniqid()];
259+
yield 'nonexistent user' => ['nonexistent@test.com', 'anypassword'];
236260
}
237261

238-
private function createUserMock(bool $captchaEnabled, bool $twoFaEnabled): SecurityModuleUser
262+
public function testLoginWith2FADisabledDoesntTouch2FA(): void
239263
{
240-
$captchaSettings = $this->createMock(CaptchaSettingsServiceInterface::class);
241-
$captchaSettings->method('isCaptchaEnabled')->willReturn($captchaEnabled);
242-
$captchaSettings->method('isHoneyPotCaptchaEnabled')->willReturn(false);
264+
$twoFaSettings = $this->createStub(TwoFASettingsInterface::class);
265+
$twoFaSettings->method('isTwoFactorAuthEnabled')->willReturn(false);
243266

244-
$twoFaSettings = $this->createMock(TwoFASettingsInterface::class);
245-
$twoFaSettings->method('isTwoFactorAuthEnabled')->willReturn($twoFaEnabled);
267+
$userServiceSpy = $this->createMock(TwoFAUserServiceInterface::class);
268+
$userServiceSpy->expects($this->never())
269+
->method('isChallengeVerified');
270+
$userServiceSpy->expects($this->never())
271+
->method('startChallengeForUser');
246272

247-
$serviceMocks = [
248-
CaptchaSettingsServiceInterface::class => $captchaSettings,
273+
$sut = $this->getSut([
249274
TwoFASettingsInterface::class => $twoFaSettings,
250-
];
275+
TwoFAUserServiceInterface::class => $userServiceSpy
276+
]);
277+
278+
$result = $sut->login(self::TWO_FA_USER_NAME, self::TWO_FA_USER_PASSWORD);
279+
$this->assertTrue($result);
280+
}
281+
282+
private function getSut(array $serviceOverrides = []): SecurityModuleUser
283+
{
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+
290+
$services = array_merge(
291+
[
292+
CaptchaSettingsServiceInterface::class => $captchaDefault,
293+
TwoFASettingsInterface::class => $twoFaDefault,
294+
],
295+
$serviceOverrides
296+
);
251297

252-
/** @var SecurityModuleUser $userMock */
253-
$userMock = $this->getMockBuilder(SecurityModuleUser::class)
298+
/** @var SecurityModuleUser $sut */
299+
$sut = $this->getMockBuilder(SecurityModuleUser::class)
254300
->onlyMethods(['getService'])
255301
->getMock();
256-
$userMock->method('getService')->willReturnCallback(
257-
fn(string $id) => $serviceMocks[$id] ?? ContainerFacade::get($id)
302+
$sut->method('getService')->willReturnCallback(
303+
fn(string $id) => $services[$id] ?? ContainerFacade::get($id)
258304
);
259305

260-
return $userMock;
306+
return $sut;
261307
}
262308

263-
private function getOTPUserId(): string
309+
private function getTwoFAUserId(): string
264310
{
265311
$user = oxNew(User::class);
266-
$user->load($user->getIdByUserName(self::OTP_USER_NAME));
312+
$user->load($user->getIdByUserName(self::TWO_FA_USER_NAME));
267313
return $user->getId();
268314
}
269315
}

0 commit comments

Comments
 (0)