OXDEV-9078 Request class should not check the empty code value, only if its not malformed

Author: Sieg

src/Authentication/TwoFactorAuth/Controller/TwoFactorAuthController.php

@@ -42,9 +42,10 @@ public function __construct(
     public function handleOTP(): ?string
     {
         $userId = $this->twoFAUserService->getPendingUserId();
+        $code = $this->authCodeRequest->getCode();
 
         try {
-            $this->twoFAService->verify($userId, $this->authCodeRequest->getCode());
+            $this->twoFAService->verify($userId, $code);
             $this->twoFAUserService->loginUser($userId);
         } catch (InvalidCodeException $e) {
             $this->utilsView->addErrorToDisplay($e);

src/Authentication/TwoFactorAuth/Service/TwoFAServiceInterface.php

@@ -7,7 +7,7 @@
 
 namespace OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Service;
 
-use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Exception\InvalidCodeException;
+use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Exception\CodeValidationException;
 
 interface TwoFAServiceInterface
 {
@@ -18,7 +18,7 @@ public function triggerChallenge(string $userId): void;
     public function invalidateChallenge(string $userId): void;
 
     /**
-     * @throws InvalidCodeException
+     * @throws CodeValidationException
      */
     public function verify(string $userId, #[\SensitiveParameter] string $code): void;
 

src/Authentication/TwoFactorAuth/Transput/AuthCodeRequest.php

@@ -10,7 +10,7 @@
 namespace OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Transput;
 
 use OxidEsales\EshopCommunity\Internal\Framework\Request\RequestInterface;
-use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Exception\InvalidCodeException;
+use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Exception\MalformedRequestException;
 
 readonly class AuthCodeRequest implements AuthCodeRequestInterface
 {
@@ -21,12 +21,11 @@ public function __construct(
 
     public function getCode(): string
     {
-        $code = $this->request->get('auth_code');
-
-        if (!is_string($code) || $code === '') {
-            throw new InvalidCodeException();
+        $raw = $this->request->get('auth_code');
+        if (!is_string($raw) && !is_int($raw)) {
+            throw new MalformedRequestException();
         }
 
-        return $code;
+        return (string) $raw;
     }
 }

src/Authentication/TwoFactorAuth/Transput/AuthCodeRequestInterface.php

@@ -7,7 +7,10 @@
 
 namespace OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Transput;
 
+use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Exception\MalformedRequestException;
+
 interface AuthCodeRequestInterface
 {
+    /** @throws MalformedRequestException */
     public function getCode(): string;
 }

tests/Unit/Authentication/TwoFactorAuth/Transput/AuthCodeRequestTest.php

@@ -10,79 +10,48 @@
 namespace OxidEsales\SecurityModule\Tests\Unit\Authentication\TwoFactorAuth\Transput;
 
 use OxidEsales\EshopCommunity\Internal\Framework\Request\RequestInterface;
-use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Exception\InvalidCodeException;
+use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Exception\MalformedRequestException;
 use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Transput\AuthCodeRequest;
 use OxidEsales\SecurityModule\Authentication\TwoFactorAuth\Transput\AuthCodeRequestInterface;
+use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\TestCase;
 
 class AuthCodeRequestTest extends TestCase
 {
-    public function testGetCodeReturnsValidCode(): void
+    public static function validCodeDataProvider(): \Generator
     {
         $code = uniqid();
-
-        $requestStub = $this->createStub(RequestInterface::class);
-        $requestStub->method('get')->with('auth_code')->willReturn($code);
-
-        $sut = $this->getSut(
-            request: $requestStub,
-        );
-
-        $this->assertSame($code, $sut->getCode());
-    }
-
-    public function testGetCodeThrowsExceptionWhenCodeIsEmpty(): void
-    {
-        $requestStub = $this->createStub(RequestInterface::class);
-        $requestStub->method('get')->with('auth_code')->willReturn('');
-
-        $sut = $this->getSut(
-            request: $requestStub,
-        );
-
-        $this->expectException(InvalidCodeException::class);
-
-        $sut->getCode();
+        yield 'string code' => ['input' => $code, 'expected' => $code];
+        yield 'empty string' => ['input' => '', 'expected' => ''];
+        yield 'integer code' => ['input' => 123456, 'expected' => '123456'];
     }
 
-    public function testGetCodeThrowsExceptionWhenCodeIsNull(): void
+    #[DataProvider('validCodeDataProvider')]
+    public function testGetCodeReturnsCode(mixed $input, string $expected): void
     {
         $requestStub = $this->createStub(RequestInterface::class);
-        $requestStub->method('get')->with('auth_code')->willReturn(null);
-
-        $sut = $this->getSut(
-            request: $requestStub,
-        );
+        $requestStub->method('get')->with('auth_code')->willReturn($input);
 
-        $this->expectException(InvalidCodeException::class);
+        $sut = $this->getSut(request: $requestStub);
 
-        $sut->getCode();
+        $this->assertSame($expected, $sut->getCode());
     }
 
-    public function testGetCodeThrowsExceptionWhenCodeIsArray(): void
+    public static function invalidCodeDataProvider(): \Generator
     {
-        $requestStub = $this->createStub(RequestInterface::class);
-        $requestStub->method('get')->with('auth_code')->willReturn(['code']);
-
-        $sut = $this->getSut(
-            request: $requestStub,
-        );
-
-        $this->expectException(InvalidCodeException::class);
-
-        $sut->getCode();
+        yield 'null' => [null];
+        yield 'array' => [['code']];
     }
 
-    public function testGetCodeThrowsExceptionWhenCodeIsInteger(): void
+    #[DataProvider('invalidCodeDataProvider')]
+    public function testGetCodeThrowsExceptionForInvalidInput(mixed $value): void
     {
         $requestStub = $this->createStub(RequestInterface::class);
-        $requestStub->method('get')->with('auth_code')->willReturn(123456);
+        $requestStub->method('get')->with('auth_code')->willReturn($value);
 
-        $sut = $this->getSut(
-            request: $requestStub,
-        );
+        $sut = $this->getSut(request: $requestStub);
 
-        $this->expectException(InvalidCodeException::class);
+        $this->expectException(MalformedRequestException::class);
 
         $sut->getCode();
     }