OXDEV-9919: Some fixes and refactorings during review

MarcelOxid · MarcelOxid · commit b1506d2d1445 · 2025-12-02T15:10:32.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Added
 - Extracted reusable Twig code into captcha.html.twig and password.html.twig
+- Facebook login OAuth-provider
 
 ### Changed
 - Show multiple errors on invalid password
diff --git a/services.yaml b/services.yaml
@@ -2,6 +2,7 @@ imports:
   - { resource: src/Captcha/services.yaml }
   - { resource: src/PasswordPolicy/services.yaml }
   - { resource: src/Authentication/services.yaml }
+  - { resource: src/Shared/services.yaml }
 
 services:
   _defaults:
diff --git a/src/Authentication/OAuth2/Infrastructure/Factory/OAuth2UserDTOFactory.php b/src/Authentication/OAuth2/Infrastructure/Factory/OAuth2UserDTOFactory.php
@@ -0,0 +1,26 @@
+<?php
+
+/**
+ * Copyright © OXID eSales AG. All rights reserved.
+ * See LICENSE file for license details.
+ */
+
+declare(strict_types=1);
+
+namespace OxidEsales\SecurityModule\Authentication\OAuth2\Infrastructure\Factory;
+
+use League\OAuth2\Client\Provider\FacebookUser;
+use OxidEsales\SecurityModule\Authentication\OAuth2\DTO\OAuth2UserDTO;
+use OxidEsales\SecurityModule\Authentication\OAuth2\DTO\OAuth2UserDTOInterface;
+
+class OAuth2UserDTOFactory implements OAuth2UserDTOFactoryInterface
+{
+    public function createFromFacebookUser(FacebookUser $facebookUser): OAuth2UserDTOInterface
+    {
+        return new OAuth2UserDTO(
+            $facebookUser->getFirstName(),
+            $facebookUser->getLastName(),
+            $facebookUser->getEmail(),
+        );
+    }
+}
diff --git a/src/Authentication/OAuth2/Infrastructure/Factory/OAuth2UserDTOFactoryInterface.php b/src/Authentication/OAuth2/Infrastructure/Factory/OAuth2UserDTOFactoryInterface.php
@@ -0,0 +1,16 @@
+<?php
+
+/**
+ * Copyright © OXID eSales AG. All rights reserved.
+ * See LICENSE file for license details.
+ */
+
+namespace OxidEsales\SecurityModule\Authentication\OAuth2\Infrastructure\Factory;
+
+use League\OAuth2\Client\Provider\FacebookUser;
+use OxidEsales\SecurityModule\Authentication\OAuth2\DTO\OAuth2UserDTOInterface;
+
+interface OAuth2UserDTOFactoryInterface
+{
+    public function createFromFacebookUser(FacebookUser $facebookUser): OAuth2UserDTOInterface;
+}
diff --git a/src/Authentication/OAuth2/Infrastructure/Factory/services.yaml b/src/Authentication/OAuth2/Infrastructure/Factory/services.yaml
@@ -8,3 +8,6 @@ services:
 
   OxidEsales\SecurityModule\Authentication\OAuth2\Infrastructure\Factory\UserDTOFactoryInterface:
     class: OxidEsales\SecurityModule\Authentication\OAuth2\Infrastructure\Factory\UserDTOFactory
+
+  OxidEsales\SecurityModule\Authentication\OAuth2\Infrastructure\Factory\OAuth2UserDTOFactoryInterface:
+    class: OxidEsales\SecurityModule\Authentication\OAuth2\Infrastructure\Factory\OAuth2UserDTOFactory
diff --git a/src/Authentication/OAuth2/Infrastructure/Provider/Facebook/FacebookAdapter.php b/src/Authentication/OAuth2/Infrastructure/Provider/Facebook/FacebookAdapter.php
@@ -12,8 +12,8 @@
 use League\OAuth2\Client\Provider\FacebookUser;
 use League\OAuth2\Client\Token\AccessToken;
 use League\OAuth2\Client\Token\AccessTokenInterface;
-use OxidEsales\SecurityModule\Authentication\OAuth2\DTO\OAuth2UserDTO;
 use OxidEsales\SecurityModule\Authentication\OAuth2\DTO\OAuth2UserDTOInterface;
+use OxidEsales\SecurityModule\Authentication\OAuth2\Infrastructure\Factory\OAuth2UserDTOFactoryInterface;
 use OxidEsales\SecurityModule\Authentication\OAuth2\Infrastructure\Provider\ProviderAdapterInterface;
 use OxidEsales\SecurityModule\Authentication\OAuth2\Service\ModuleSettingsServiceInterface;
 use League\OAuth2\Client\Provider\Facebook as FacebookProvider;
@@ -25,9 +25,10 @@ class FacebookAdapter implements ProviderAdapterInterface
 
     public function __construct(
         private readonly ModuleSettingsServiceInterface $moduleSettings,
-        private readonly FacebookProviderFactoryInterface $facebookProvider,
+        private readonly FacebookProviderFactoryInterface $facebookProviderFactory,
+        private readonly OAuth2UserDTOFactoryInterface $oAuth2UserDTOFactory,
     ) {
-        $this->provider = $this->facebookProvider->create();
+        $this->provider = $this->facebookProviderFactory->create();
     }
 
     public function isActive(): bool
@@ -59,10 +60,6 @@ public function getUserInfo(AccessTokenInterface $token): OAuth2UserDTOInterface
         /** @var FacebookUser $user */
         $user = $this->provider->getResourceOwner($token);
 
-        return new OAuth2UserDTO(
-            $user->getFirstName(),
-            $user->getLastName(),
-            $user->getEmail(),
-        );
+        return $this->oAuth2UserDTOFactory->createFromFacebookUser($user);
     }
 }
diff --git a/src/Authentication/OAuth2/Infrastructure/Provider/ProviderAdapterInterface.php b/src/Authentication/OAuth2/Infrastructure/Provider/ProviderAdapterInterface.php
@@ -7,6 +7,7 @@
 
 namespace OxidEsales\SecurityModule\Authentication\OAuth2\Infrastructure\Provider;
 
+use Exception;
 use League\OAuth2\Client\Token\AccessTokenInterface;
 use OxidEsales\SecurityModule\Authentication\OAuth2\DTO\OAuth2UserDTOInterface;
 
@@ -35,6 +36,7 @@ public function getAccessToken(string $code): AccessTokenInterface;
     /**
      * Fetch user information (claims) from the provider using the access token.
      * Should return UserInterface
+     * @throws Exception
      */
     public function getUserInfo(AccessTokenInterface $token): OAuth2UserDTOInterface;
 }
diff --git a/src/Authentication/OAuth2/Infrastructure/Repository/UserRepository.php b/src/Authentication/OAuth2/Infrastructure/Repository/UserRepository.php
@@ -14,12 +14,14 @@
 use OxidEsales\SecurityModule\Authentication\OAuth2\Exception\UserNotFoundException;
 use OxidEsales\SecurityModule\Authentication\OAuth2\Infrastructure\Factory\UserDTOFactoryInterface;
 use OxidEsales\SecurityModule\Authentication\OAuth2\Infrastructure\Factory\UserFactoryInterface;
+use OxidEsales\SecurityModule\Shared\Service\PasswordGeneratorServiceInterface;
 
 class UserRepository implements UserRepositoryInterface
 {
     public function __construct(
         private UserFactoryInterface $userFactory,
         private UserDTOFactoryInterface $userDTOFactory,
+        private PasswordGeneratorServiceInterface $passwordGenerator
     ) {
     }
 
@@ -28,10 +30,7 @@ public function getUserByEmail(string $username): UserDTOInterface
         $userModel = $this->userFactory->create();
 
         $userId = $userModel->getIdByUserName($username);
-        if (
-            !$userId ||
-            !$userModel->load($userId)
-        ) {
+        if (!$userId || !$userModel->load($userId)) {
             throw new UserNotFoundException();
         }
 
@@ -45,9 +44,8 @@ public function createUser(OAuth2UserDTOInterface $userDTO): UserDTOInterface
             'OXFNAME'    => $userDTO->getFirstName(),
             'OXLNAME'    => $userDTO->getLastName(),
             'OXUSERNAME' => $userDTO->getEmail(),
-            'OXREGISTER' => date('Y-m-d H:i:s')
         ]);
-        $userModel->setPassword(bin2hex(random_bytes(20)));
+        $userModel->setPassword($this->passwordGenerator->generatePasswordForOAuthUser());
         $userModel->createUser();
 
         return $this->userDTOFactory->createFromModel($userModel);
diff --git a/src/Authentication/OAuth2/Infrastructure/services.yaml b/src/Authentication/OAuth2/Infrastructure/services.yaml
@@ -2,8 +2,3 @@ imports:
   - { resource: Factory/services.yaml }
   - { resource: Provider/services.yaml }
   - { resource: Repository/services.yaml }
-
-services:
-  _defaults:
-    autowire: true
-    public: false
diff --git a/src/Authentication/OAuth2/Service/UserService.php b/src/Authentication/OAuth2/Service/UserService.php
@@ -30,14 +30,14 @@ public function login(OAuth2UserDTOInterface $auth2UserDTO): void
         }
 
         try {
-            $userModel = $this->userRepository->getUserByEmail($auth2UserDTO->getEmail());
-            if ($userModel->isBlocked()) {
+            $userDTO = $this->userRepository->getUserByEmail($auth2UserDTO->getEmail());
+            if ($userDTO->isBlocked()) {
                 throw new UserBlockedException();
             }
         } catch (UserNotFoundException $e) {
-            $userModel = $this->userRepository->createUser($auth2UserDTO);
+            $userDTO = $this->userRepository->createUser($auth2UserDTO);
         }
 
-        $this->session->set('usr', $userModel->getId());
+        $this->session->set('usr', $userDTO->getId());
     }
 }
diff --git a/src/Shared/Service/PasswordGeneratorService.php b/src/Shared/Service/PasswordGeneratorService.php
@@ -0,0 +1,22 @@
+<?php
+
+/**
+ * Copyright © OXID eSales AG. All rights reserved.
+ * See LICENSE file for license details.
+ */
+
+declare(strict_types=1);
+
+namespace OxidEsales\SecurityModule\Shared\Service;
+
+use OxidEsales\EshopCommunity\Internal\Framework\Module\Facade\ModuleSettingServiceInterface;
+use OxidEsales\SecurityModule\Authentication\OAuth2\Service\ModuleSettingsServiceInterface;
+use OxidEsales\SecurityModule\Core\Module;
+
+class PasswordGeneratorService implements PasswordGeneratorServiceInterface
+{
+    public function generatePasswordForOAuthUser(): string
+    {
+        return bin2hex(random_bytes(20));
+    }
+}
diff --git a/src/Shared/Service/PasswordGeneratorServiceInterface.php b/src/Shared/Service/PasswordGeneratorServiceInterface.php
@@ -0,0 +1,19 @@
+<?php
+
+/**
+ * Copyright © OXID eSales AG. All rights reserved.
+ * See LICENSE file for license details.
+ */
+
+declare(strict_types=1);
+
+namespace OxidEsales\SecurityModule\Shared\Service;
+
+use OxidEsales\EshopCommunity\Internal\Framework\Module\Facade\ModuleSettingServiceInterface;
+use OxidEsales\SecurityModule\Authentication\OAuth2\Service\ModuleSettingsServiceInterface;
+use OxidEsales\SecurityModule\Core\Module;
+
+interface PasswordGeneratorServiceInterface
+{
+    public function generatePasswordForOAuthUser(): string;
+}
diff --git a/src/Shared/Service/services.yaml b/src/Shared/Service/services.yaml
@@ -0,0 +1,7 @@
+services:
+  _defaults:
+    autowire: true
+    public: false
+
+  OxidEsales\SecurityModule\Shared\Service\PasswordGeneratorServiceInterface:
+    class: OxidEsales\SecurityModule\Shared\Service\PasswordGeneratorService
diff --git a/src/Shared/services.yaml b/src/Shared/services.yaml
@@ -0,0 +1,2 @@
+imports:
+  - { resource: Service/services.yaml }
diff --git a/tests/Integration/Authentication/OAuth2/Infrastructure/Factory/OAuth2UserDTOFactoryTest.php b/tests/Integration/Authentication/OAuth2/Infrastructure/Factory/OAuth2UserDTOFactoryTest.php
@@ -0,0 +1,37 @@
+<?php
+
+/**
+ * Copyright © OXID eSales AG. All rights reserved.
+ * See LICENSE file for license details.
+ */
+
+declare(strict_types=1);
+
+namespace OxidEsales\SecurityModule\Tests\Integration\Authentication\OAuth2\Infrastructure\Factory;
+
+use League\OAuth2\Client\Provider\FacebookUser;
+use OxidEsales\SecurityModule\Authentication\OAuth2\Infrastructure\Factory\OAuth2UserDTOFactory;
+use PHPUnit\Framework\TestCase;
+
+class OAuth2UserDTOFactoryTest extends TestCase
+{
+    public function testCreateFromFacebookUser(): void
+    {
+        $expectedFirstName = uniqid();
+        $expectedLastName = uniqid();
+        $expectedEmail = uniqid();
+
+        $facebookUserStub = $this->createStub(FacebookUser::class);
+        $facebookUserStub->method('getFirstName')->willReturn($expectedFirstName);
+        $facebookUserStub->method('getLastName')->willReturn($expectedLastName);
+        $facebookUserStub->method('getEmail')->willReturn($expectedEmail);
+
+        $sut = new OAuth2UserDTOFactory();
+
+        $result = $sut->createFromFacebookUser($facebookUserStub);
+
+        $this->assertSame($expectedFirstName, $result->getFirstName());
+        $this->assertSame($expectedLastName, $result->getLastName());
+        $this->assertSame($expectedEmail, $result->getEmail());
+    }
+}
diff --git a/tests/Integration/Authentication/OAuth2/Infrastructure/Factory/UserDTOFactoryTest.php b/tests/Integration/Authentication/OAuth2/Infrastructure/Factory/UserDTOFactoryTest.php
@@ -7,10 +7,9 @@
 
 declare(strict_types=1);
 
-namespace OxidEsales\SecurityModule\Tests\Unit\Authentication\OAuth2\Infrastructure\Factory;
+namespace OxidEsales\SecurityModule\Tests\Integration\Authentication\OAuth2\Infrastructure\Factory;
 
 use OxidEsales\Eshop\Application\Model\User as UserModel;
-use OxidEsales\SecurityModule\Authentication\OAuth2\DTO\UserDTO;
 use OxidEsales\SecurityModule\Authentication\OAuth2\Infrastructure\Factory\UserDTOFactory;
 use PHPUnit\Framework\TestCase;
 
@@ -19,17 +18,16 @@ class UserDTOFactoryTest extends TestCase
     public function testCreateFromModel(): void
     {
         $userId = uniqid();
-        $isBlocked = true;
+        $isBlocked = (bool)rand(0, 1);
 
         $userModelMock = $this->createMock(UserModel::class);
         $userModelMock->method('getId')->willReturn($userId);
         $userModelMock->method('inGroup')->with('oxidblocked')->willReturn($isBlocked);
 
-        $factory = new UserDTOFactory();
+        $sut = new UserDTOFactory();
 
-        $userDTO = $factory->createFromModel($userModelMock);
+        $userDTO = $sut->createFromModel($userModelMock);
 
-        $this->assertInstanceOf(UserDTO::class, $userDTO);
         $this->assertSame($userId, $userDTO->getId());
         $this->assertSame($isBlocked, $userDTO->isBlocked());
     }
diff --git a/tests/Integration/Authentication/OAuth2/Infrastructure/Provider/Facebook/FacebookProviderFactoryTest.php b/tests/Integration/Authentication/OAuth2/Infrastructure/Provider/Facebook/FacebookProviderFactoryTest.php
@@ -7,28 +7,28 @@
 
 declare(strict_types=1);
 
-namespace Authentication\OAuth2\Infrastructure\Provider\Facebook;
+namespace OxidEsales\SecurityModule\Tests\Integration\Authentication\OAuth2\Infrastructure\Provider\Facebook;
 
 use OxidEsales\SecurityModule\Authentication\OAuth2\Infrastructure\Provider\Facebook\FacebookProviderFactory;
 use OxidEsales\SecurityModule\Authentication\OAuth2\Service\ModuleSettingsServiceInterface;
 use PHPUnit\Framework\TestCase;
 use League\OAuth2\Client\Provider\Facebook as FacebookProvider;
 
-class FacebookFactoryTest extends TestCase
+class FacebookProviderFactoryTest extends TestCase
 {
     public function testFactory(): void
     {
         $clientId = uniqid();
         $redirectUrl = uniqid();
 
-        $moduleSettingsMock = $this->createStub(ModuleSettingsServiceInterface::class);
-        $moduleSettingsMock->method('getFacebookClientId')->willReturn($clientId);
-        $moduleSettingsMock->method('getFacebookRedirectUrl')->willReturn($redirectUrl);
+        $moduleSettingsStub = $this->createStub(ModuleSettingsServiceInterface::class);
+        $moduleSettingsStub->method('getFacebookClientId')->willReturn($clientId);
+        $moduleSettingsStub->method('getFacebookRedirectUrl')->willReturn($redirectUrl);
 
-        $fbProviderFactory = new FacebookProviderFactory(
-            $moduleSettingsMock
+        $sut = new FacebookProviderFactory(
+            $moduleSettingsStub
         );
-        $facebookProvider = $fbProviderFactory->create();
+        $facebookProvider = $sut->create();
 
         $authUrl = $facebookProvider->getAuthorizationUrl();
 
diff --git a/tests/Unit/Authentication/OAuth2/Exception/ProviderNotFoundTest.php b/tests/Unit/Authentication/OAuth2/Exception/ProviderNotFoundTest.php
@@ -18,6 +18,6 @@ public function testException(): void
     {
         $exception = new ProviderNotFoundException();
 
-        $this->assertInstanceOf(ProviderNotFoundException::class, $exception);
+        $this->assertInstanceOf(\Exception::class, $exception);
     }
 }
diff --git a/tests/Unit/Authentication/OAuth2/Exception/UserBlockedExceptionTest.php b/tests/Unit/Authentication/OAuth2/Exception/UserBlockedExceptionTest.php
@@ -18,6 +18,6 @@ public function testException(): void
     {
         $exception = new UserBlockedException();
 
-        $this->assertInstanceOf(UserBlockedException::class, $exception);
+        $this->assertInstanceOf(\Exception::class, $exception);
     }
 }
diff --git a/tests/Unit/Authentication/OAuth2/Exception/UserNotFoundExceptionTest.php b/tests/Unit/Authentication/OAuth2/Exception/UserNotFoundExceptionTest.php
@@ -18,6 +18,6 @@ public function testException(): void
     {
         $exception = new UserNotFoundException();
 
-        $this->assertInstanceOf(UserNotFoundException::class, $exception);
+        $this->assertInstanceOf(\Exception::class, $exception);
     }
 }
diff --git a/tests/Unit/Authentication/OAuth2/Infrastructure/Provider/Facebook/FacebookAdapterTest.php b/tests/Unit/Authentication/OAuth2/Infrastructure/Provider/Facebook/FacebookAdapterTest.php
diff --git a/tests/Unit/Authentication/OAuth2/Infrastructure/Repository/UserRepositoryTest.php b/tests/Unit/Authentication/OAuth2/Infrastructure/Repository/UserRepositoryTest.php
diff --git a/tests/Unit/Authentication/OAuth2/Service/ProviderCollectorTest.php b/tests/Unit/Authentication/OAuth2/Service/ProviderCollectorTest.php
diff --git a/tests/Unit/Authentication/OAuth2/Service/UserServiceTest.php b/tests/Unit/Authentication/OAuth2/Service/UserServiceTest.php
diff --git a/tests/Unit/Captcha/Infrastructure/LanguageWrapperTest.php b/tests/Unit/Captcha/Infrastructure/LanguageWrapperTest.php
diff --git a/tests/Unit/Shared/Service/PasswordGeneratorServiceTest.php b/tests/Unit/Shared/Service/PasswordGeneratorServiceTest.php
diff --git a/tests/phpunit.xml b/tests/phpunit.xml

Original file line number	Diff line number	Diff line change
`@@ -30,14 +30,14 @@ public function login(OAuth2UserDTOInterface $auth2UserDTO): void`
`30`	`30`	`}`
`31`	`31`
`32`	`32`	`try {`
`33`		`- $userModel = $this->userRepository->getUserByEmail($auth2UserDTO->getEmail());`
`34`		`- if ($userModel->isBlocked()) {`
	`33`	`+ $userDTO = $this->userRepository->getUserByEmail($auth2UserDTO->getEmail());`
	`34`	`+ if ($userDTO->isBlocked()) {`
`35`	`35`	`throw new UserBlockedException();`
`36`	`36`	`}`
`37`	`37`	`} catch (UserNotFoundException $e) {`
`38`		`- $userModel = $this->userRepository->createUser($auth2UserDTO);`
	`38`	`+ $userDTO = $this->userRepository->createUser($auth2UserDTO);`
`39`	`39`	`}`
`40`	`40`
`41`		`- $this->session->set('usr', $userModel->getId());`
	`41`	`+ $this->session->set('usr', $userDTO->getId());`
`42`	`42`	`}`
`43`	`43`	`}`