OXDEV-9078 Reimplement the login flow service and use new code in the flow process

Author: Sieg

src/Authentication/TwoFactorAuth/Controller/TwoFactorAuthController.php

@@ -11,9 +11,10 @@
 
 use OxidEsales\Eshop\Application\Controller\FrontendController;
 use OxidEsales\Eshop\Core\UtilsView;
-use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Exception\OTPValidationException;
+use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Exception\InvalidCodeException;
 use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\AuthorizeServiceInterface;
-use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\UserServiceInterface;
+use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\TwoFAServiceInterface;
+use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\TwoFAUserServiceInterface;
 use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Transput\AuthCodeRequestInterface;
 use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Transput\JsonResponseInterface;
 
@@ -28,8 +29,9 @@ class TwoFactorAuthController extends FrontendController
     protected $_sThisTemplate = '@oe_security_module/templates/two_factor_auth';
 
     public function __construct(
+        private readonly TwoFAServiceInterface $twoFAService,
+        private readonly TwoFAUserServiceInterface $twoFAUserService,
         private readonly AuthorizeServiceInterface $authService,
-        private readonly UserServiceInterface $userService,
         private readonly AuthCodeRequestInterface $authCodeRequest,
         private readonly UtilsView $utilsView,
         private readonly JsonResponseInterface $jsonResponse,
@@ -39,13 +41,12 @@ public function __construct(
 
     public function handleOTP(): ?string
     {
-        try {
-            $this->authService->validate(
-                $this->authCodeRequest->getCode()
-            );
+        $userId = $this->twoFAUserService->getPendingUserId();
 
-            $this->userService->finalizeLogin();
-        } catch (OTPValidationException $e) {
+        try {
+            $this->twoFAService->verify($userId, $this->authCodeRequest->getCode());
+            $this->twoFAUserService->loginUser($userId);
+        } catch (InvalidCodeException $e) {
             $this->utilsView->addErrorToDisplay($e);
         }
 

src/Authentication/TwoFactorAuth/OTP/Service/OtpCodeValidatorServiceInterface.php

@@ -9,8 +9,13 @@
 
 namespace OxidEsales\SecurityModule\Authentication\TwoFactorAuth\OTP\Service;
 
+use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Exception\InvalidCodeException;
+
 interface OtpCodeValidatorServiceInterface
 {
+    /**
+     * @throws InvalidCodeException
+     */
     public function validateCode(
         string $userId,
         #[\SensitiveParameter] string $inputCode

src/Authentication/TwoFactorAuth/Service/TwoFAServiceInterface.php

@@ -7,6 +7,8 @@
 
 namespace OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service;
 
+use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Exception\InvalidCodeException;
+
 interface TwoFAServiceInterface
 {
     public function isVerified(string $userId): bool;
@@ -15,6 +17,9 @@ public function triggerChallenge(string $userId): void;
 
     public function invalidateChallenge(string $userId): void;
 
+    /**
+     * @throws InvalidCodeException
+     */
     public function verify(string $userId, #[\SensitiveParameter] string $code): void;
 
     public function resend(string $userId): void;

src/Authentication/TwoFactorAuth/Service/TwoFAUserService.php

@@ -0,0 +1,61 @@
+<?php
+
+/**
+ * Copyright © OXID eSales AG. All rights reserved.
+ * See LICENSE file for license details.
+ */
+
+declare(strict_types=1);
+
+namespace OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service;
+
+use OxidEsales\Eshop\Core\Utils;
+use OxidEsales\EshopCommunity\Internal\Framework\Session\SessionInterface;
+use OxidEsales\SecurityModule\Authentication\Service\InternalRedirectServiceInterface;
+use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Infrastructure\Factory\UserModelFactoryInterface;
+use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Settings\TwoFASettingsInterface;
+
+class TwoFAUserService implements TwoFAUserServiceInterface
+{
+    public const USER_SESSION_KEY = 'pending_authorized_user';
+
+    public function __construct(
+        private TwoFAServiceInterface $twoFAService,
+        private TwoFASettingsInterface $settings,
+        private Utils $utils,
+        private SessionInterface $session,
+        private UserModelFactoryInterface $userFactory,
+        private InternalRedirectServiceInterface $redirectService,
+    ) {
+    }
+
+    public function startChallengeForUser(string $userId): void
+    {
+        $this->session->set(self::USER_SESSION_KEY, $userId);
+        $this->twoFAService->triggerChallenge($userId);
+        $this->utils->redirect($this->settings->getVerificationUrl());
+    }
+
+    public function getPendingUserId(): string
+    {
+        return $this->session->get(self::USER_SESSION_KEY);
+    }
+
+    public function loginUser(string $userId): void
+    {
+        $this->session->remove(self::USER_SESSION_KEY);
+
+        $user = $this->userFactory->create();
+        $user->load($userId);
+
+        /** @phpstan-ignore argument.type (password is null because user already authenticated to come here) */
+        $user->login($user->getFieldData('oxusername'), null, false);
+
+        $this->utils->redirect($this->redirectService->getRedirectUrl(), false);
+    }
+
+    public function isChallengeVerified(string $userId): bool
+    {
+        return $this->twoFAService->isVerified($userId);
+    }
+}

src/Authentication/TwoFactorAuth/Service/TwoFAUserServiceInterface.php

@@ -0,0 +1,21 @@
+<?php
+
+/**
+ * Copyright © OXID eSales AG. All rights reserved.
+ * See LICENSE file for license details.
+ */
+
+declare(strict_types=1);
+
+namespace OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service;
+
+interface TwoFAUserServiceInterface
+{
+    public function startChallengeForUser(string $userId): void;
+
+    public function getPendingUserId(): string;
+
+    public function loginUser(string $userId): void;
+
+    public function isChallengeVerified(string $userId): bool;
+}

src/Authentication/TwoFactorAuth/Service/services.yaml

@@ -35,3 +35,7 @@ services:
   OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\ResendOTPServiceInterface:
     class: OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\ResendOTPService
     public: true
+
+  OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\TwoFAUserServiceInterface:
+    class: OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\TwoFAUserService
+    public: true

src/Shared/Model/User.php

@@ -12,8 +12,7 @@
 use OxidEsales\Eshop\Core\Exception\InputException;
 use OxidEsales\Eshop\Core\Exception\UserException;
 use OxidEsales\Eshop\Core\Registry;
-// use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\TwoFAServiceInterface;
-use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\UserServiceInterface;
+use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\TwoFAUserServiceInterface;
 use OxidEsales\SecurityModule\Captcha\Captcha\Image\Exception\CaptchaValidateException as ImageCaptchaException;
 use OxidEsales\SecurityModule\Captcha\Captcha\HoneyPot\Exception\CaptchaValidateException as HoneyPotCaptchaException;
 use OxidEsales\SecurityModule\Captcha\Service\CaptchaServiceInterface;
@@ -91,31 +90,14 @@ protected function onLogin($userName, #[\SensitiveParameter] $password)
     {
         parent::onLogin($userName, $password);
 
-        if (!$this->isOTPEnabled() || $this->isAdmin()) {
-            return;
-        }
-
         $userId = $this->getId();
-        if (!$userId) {
-            return;
-        }
-
-        $userService = $this->getService(UserServiceInterface::class);
-        $userSessionKey = Registry::getSession()->getVariable('OTP_PASS');
-        if ($userSessionKey && $userSessionKey === $userId) {
-            $userService->clearOTPSessionVariables();
-            return;
+        $settingsService = $this->getService(TwoFASettingsServiceInterface::class);
+        if ($userId && $settingsService->isTwoFactorAuthEnabled() && !$this->isAdmin()) {
+            $twoFAUserService = $this->getService(TwoFAUserServiceInterface::class);
+            if (!$twoFAUserService->isChallengeVerified($userId)) {
+                $twoFAUserService->startChallengeForUser($userId);
+            }
         }
-
-        $userService->handleLogin($userId);
-
-//        $settingsService = $this->getService(TwoFASettingsServiceInterface::class);
-//        if ($settingsService->isTwoFactorAuthEnabled() && !$this->isAdmin()) {
-//            $authentication = $this->getService(TwoFAServiceInterface::class);
-//            if (!$authentication->isVerified($userId)) {
-//                $authentication->triggerChallenge($userId);
-//            }
-//        }
     }
 
     private function isCaptchaEnabled(): bool
@@ -124,12 +106,6 @@ private function isCaptchaEnabled(): bool
         return $settingsService->isCaptchaEnabled() || $settingsService->isHoneyPotCaptchaEnabled();
     }
 
-    private function isOTPEnabled(): bool
-    {
-        $settingsService = $this->getService(TwoFASettingsServiceInterface::class);
-        return $settingsService->isTwoFactorAuthEnabled();
-    }
-
     protected function shouldValidateCaptcha(): bool
     {
         return !$this->getUser();

tests/Integration/Shared/Model/UserTest.php

@@ -17,12 +17,15 @@
 use OxidEsales\Eshop\Core\Utils;
 use OxidEsales\EshopCommunity\Core\Di\ContainerFacade;
 use OxidEsales\EshopCommunity\Internal\Framework\Module\Facade\ModuleSettingServiceInterface;
+use DateTimeImmutable;
+use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\OTP\Infrastructure\Repository\OtpChallengeStateRepositoryInterface;
 use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\AuthorizeService;
 use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service\ModuleSettingsService;
 use OxidEsales\SecurityModule\Captcha\Service\ModuleSettingsServiceInterface as CaptchaSettingsServiceInterface;
 use OxidEsales\SecurityModule\Core\Module;
 use OxidEsales\SecurityModule\Tests\Integration\IntegrationTestCase;
 
+// todo-critical: rework the 2FA part of the test. it changes the real configs on the fly and causing side effects
 class UserTest extends IntegrationTestCase
 {
     private const OTP_USER_NAME = 'user@oxid-esales.com';
@@ -207,31 +210,24 @@ public function testLoginWithOTPEnabledStoresUserIdInSession(): void
         $this->assertEquals($subject->getId(), $sessionUserId);
     }
 
-    public function testLoginWithOTPPassSessionVariableSkipsOTPRedirect(): void
+    public function testLoginWithVerifiedChallengeStateSkipsOTPRedirect(): void
     {
         $this->disableCaptcha();
         $this->enableTwoFactorAuth();
 
-        $subject = oxNew(User::class);
-        $subject->load($this->getOTPUserId());
+        $userId = $this->getOTPUserId();
 
-        Registry::getSession()->setVariable('OTP_PASS', $subject->getId());
+        $stateRepo = $this->get(OtpChallengeStateRepositoryInterface::class);
+        $stateRepo->createChallengeState($userId, 'hash', new DateTimeImmutable('+5 minutes'));
+        $stateRepo->markVerified($userId);
 
         $utilsMock = $this->createMock(Utils::class);
         $utilsMock->expects($this->never())->method('redirect');
         Registry::set(Utils::class, $utilsMock);
 
-        $result = $subject->login(self::OTP_USER_NAME, self::OTP_USER_PASSWORD);
+        $result = oxNew(User::class)->login(self::OTP_USER_NAME, self::OTP_USER_PASSWORD);
 
         $this->assertTrue($result);
-        $this->assertNull(
-            Registry::getSession()->getVariable('OTP_PASS'),
-            'OTP_PASS session variable should be cleared after successful login'
-        );
-        $this->assertNull(
-            Registry::getSession()->getVariable(AuthorizeService::USER_SESSION_KEY),
-            'USER_SESSION_KEY should not be set when OTP was already validated'
-        );
     }
 
     public function testLoginWithOTPPassSessionVariableMismatchTriggersOTPFlow(): void