From 1a9ed7f97ec9b4887bbd77ac4558b442e8f52906 Mon Sep 17 00:00:00 2001 From: adumont-payplug Date: Fri, 17 Jul 2026 12:02:33 +0200 Subject: [PATCH 1/5] PRE-3563: Add SyliusOAuthHttpClient adapter --- src/Auth/SyliusOAuthHttpClient.php | 36 ++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 src/Auth/SyliusOAuthHttpClient.php diff --git a/src/Auth/SyliusOAuthHttpClient.php b/src/Auth/SyliusOAuthHttpClient.php new file mode 100644 index 00000000..12331be3 --- /dev/null +++ b/src/Auth/SyliusOAuthHttpClient.php @@ -0,0 +1,36 @@ + $formParams + * @param array $headers + * + * @return array{status: int, body: string} + */ + public function post(string $url, array $formParams, array $headers = []): array + { + $response = $this->httpClient->request('POST', $url, [ + 'body' => http_build_query($formParams), + 'headers' => $headers, + ]); + + return [ + 'status' => $response->getStatusCode(), + // false = don't throw on non-2xx; OAuth2Client itself checks the status. + 'body' => $response->getContent(false), + ]; + } +} From 3e0f7b4411817b0eeb9020b3286df8471aeeecf4 Mon Sep 17 00:00:00 2001 From: adumont-payplug Date: Fri, 17 Jul 2026 12:06:48 +0200 Subject: [PATCH 2/5] Add SyliusTokenCache adapter for UPC's ITokenCache --- src/Auth/SyliusTokenCache.php | 49 +++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 src/Auth/SyliusTokenCache.php diff --git a/src/Auth/SyliusTokenCache.php b/src/Auth/SyliusTokenCache.php new file mode 100644 index 00000000..e8ca6fe0 --- /dev/null +++ b/src/Auth/SyliusTokenCache.php @@ -0,0 +1,49 @@ +cache->getItem($this->sanitizeKey($key)); + + if (!$item->isHit()) { + return null; + } + + /** @var string $value */ + $value = $item->get(); + + return $value; + } + + public function set(string $key, string $value, int $ttlSeconds): void + { + $item = $this->cache->getItem($this->sanitizeKey($key)); + $item->set($value); + $item->expiresAfter($ttlSeconds); + $this->cache->save($item); + } + + public function delete(string $key): void + { + $this->cache->deleteItem($this->sanitizeKey($key)); + } + + // PSR-6 rejects "{}()/\@:" in cache keys; TokenManager's keys contain ":". + private function sanitizeKey(string $key): string + { + return (string) preg_replace('/[{}()\/\\\\@:]/', '_', $key); + } +} From 25beb1e6c79ce4d046d7a37f555845b4a0ee334c Mon Sep 17 00:00:00 2001 From: adumont-payplug Date: Mon, 20 Jul 2026 15:24:02 +0200 Subject: [PATCH 3/5] PRE-3563: add OAuth2/PKCE throw UPC --- .gitignore | 1 + composer.json | 1 + config/services.yaml | 10 + config/services/client.xml | 19 ++ .../Auth/UnifiedAuthenticationController.php | 90 ++++--- src/ApiClient/PayPlugApiClientFactory.php | 60 +++-- .../UnifiedAuthenticationControllerTest.php | 254 ++++++++++++++++++ .../ApiClient/PayPlugApiClientFactoryTest.php | 175 ++++++++++++ .../Auth/SyliusOAuthHttpClientTest.php | 102 +++++++ tests/PHPUnit/Auth/SyliusTokenCacheTest.php | 115 ++++++++ 10 files changed, 773 insertions(+), 54 deletions(-) create mode 100644 tests/PHPUnit/Action/Admin/Auth/UnifiedAuthenticationControllerTest.php create mode 100644 tests/PHPUnit/ApiClient/PayPlugApiClientFactoryTest.php create mode 100644 tests/PHPUnit/Auth/SyliusOAuthHttpClientTest.php create mode 100644 tests/PHPUnit/Auth/SyliusTokenCacheTest.php diff --git a/.gitignore b/.gitignore index f47650bd..534a23cd 100644 --- a/.gitignore +++ b/.gitignore @@ -25,3 +25,4 @@ CLAUDE.md .claude .review .phpunit.result.cache +docs/ \ No newline at end of file diff --git a/composer.json b/composer.json index 2d93bb4d..d2565406 100755 --- a/composer.json +++ b/composer.json @@ -14,6 +14,7 @@ "ext-json": "*", "giggsey/libphonenumber-for-php": "^8.12", "payplug/payplug-php": "^4.0", + "payplug/unified-plugin-core": "0.0.7", "php-http/message-factory": "^1.1", "sylius/refund-plugin": "^2.0", "sylius/sylius": "^2.0", diff --git a/config/services.yaml b/config/services.yaml index 7db6a5fe..2131eff5 100644 --- a/config/services.yaml +++ b/config/services.yaml @@ -1,3 +1,11 @@ +parameters: + # Overridable via env vars for QA/staging testing; merchants installing the plugin normally + # never need to set these — the defaults below are used automatically. + payplug.oauth_base_url.default: 'https://api.payplug.com' + payplug.oauth_base_url: '%env(default:payplug.oauth_base_url.default:PAYPLUG_OAUTH_BASE_URL)%' + payplug.oauth_audience.default: 'https://www.payplug.com' + payplug.oauth_audience: '%env(default:payplug.oauth_audience.default:PAYPLUG_OAUTH_AUDIENCE)%' + services: _defaults: autowire: true @@ -9,6 +17,8 @@ services: exclude: '../src/{ApiClient,DependencyInjection,Entity,Exception,Model,Repository,PayPlugSyliusPayPlugPlugin.php}' bind: Psr\Log\LoggerInterface: '@monolog.logger.payplug' + $payplugOauthBaseUrl: '%payplug.oauth_base_url%' + $payplugOauthAudience: '%payplug.oauth_audience%' PayPlug\SyliusPayPlugPlugin\Repository\PaymentRepositoryInterface: class: PayPlug\SyliusPayPlugPlugin\Repository\PaymentRepository diff --git a/config/services/client.xml b/config/services/client.xml index 3f2d3391..73148c93 100644 --- a/config/services/client.xml +++ b/config/services/client.xml @@ -10,6 +10,25 @@ + + + + + + + + %payplug.oauth_base_url% + + + %payplug.oauth_audience% + + + + $paymentMethodRepository */ @@ -39,36 +43,46 @@ public function __construct( private EntityManagerInterface $entityManager, private PaymentMethodValidator $paymentMethodValidator, private LoggerInterface $logger, - private CacheInterface $cache, + private IOAuthHttpClient $oauthHttpClient, + private string $payplugOauthBaseUrl, + private string $payplugOauthAudience, ) { } + private function buildOAuth2Client(string $redirectUri): OAuth2Client + { + return new OAuth2Client($this->oauthHttpClient, $this->payplugOauthBaseUrl, $redirectUri, self::PKCE_SCOPE, $this->payplugOauthAudience); + } + #[Route('/setup-redirection', name: 'payplug_sylius_admin_auth_setup_redirection')] public function setupRedirection(Request $request): Response { try { - $clientId = $request->query->get('client_id'); - $companyId = $request->query->get('company_id'); + $clientId = $request->query->getString('client_id'); + $companyId = $request->query->getString('company_id'); $request->getSession()->set('payplug_client_id', $clientId); $request->getSession()->set('payplug_company_id', $companyId); - $challenge = bin2hex(openssl_random_pseudo_bytes(50)); - $request->getSession()->set('payplug_oauth_challenge', $challenge); - $callBackUrl = $this->router->generate('payplug_sylius_admin_auth_oauth_callback', [], RouterInterface::ABSOLUTE_URL); - // This method will redirect the user to PayPlug's oauth page via header('Location')' - Authentication::initiateOAuth($clientId, $callBackUrl, $challenge); - // Fetch the header Location the Sdk put and redirect the user to it - $headers = \headers_list(); - foreach ($headers as $header) { - if (str_starts_with($header, 'Location:')) { - return new RedirectResponse(substr($header, 9)); - } - } - - throw new \LogicException('No location header found'); + // Legacy flow, superseded by PayplugUnifiedCore\Auth\OAuth2Client below. + // $challenge = bin2hex(openssl_random_pseudo_bytes(50)); + // $request->getSession()->set('payplug_oauth_challenge', $challenge); + // Authentication::initiateOAuth($clientId, $callBackUrl, $challenge); + // $headers = \headers_list(); + // foreach ($headers as $header) { + // if (str_starts_with($header, 'Location:')) { + // return new RedirectResponse(substr($header, 9)); + // } + // } + // throw new \LogicException('No location header found'); + + $authorizationRequest = $this->buildOAuth2Client($callBackUrl)->buildAuthorizationUrl($clientId); + $request->getSession()->set('payplug_oauth_state', $authorizationRequest->state); + $request->getSession()->set('payplug_oauth_code_verifier', $authorizationRequest->codeVerifier); + + return new RedirectResponse($authorizationRequest->url); } catch (\Throwable $e) { $this->logger->critical('Error while perform Payplug OAuth Setup redirection', ['message' => $e->getMessage(), 'exception' => $e]); @@ -81,16 +95,31 @@ public function oauthCallback(Request $request): Response { try { $code = $request->query->getString('code'); + $state = $request->query->getString('state'); /** @var string $clientId */ $clientId = $request->getSession()->get('payplug_client_id'); - /** @var string $challenge */ - $challenge = $request->getSession()->get('payplug_oauth_challenge'); - $callback = $this->generateUrl('payplug_sylius_admin_auth_oauth_callback', [], UrlGeneratorInterface::ABSOLUTE_URL); + /** @var string $expectedState */ + $expectedState = $request->getSession()->get('payplug_oauth_state'); + $codeVerifier = $request->getSession()->get('payplug_oauth_code_verifier'); - $jwt = Authentication::generateJWTOneShot($code, $callback, $clientId, $challenge); - if ([] === $jwt || $jwt['httpStatus'] !== 200 || !\is_array($jwt['httpResponse'])) { - throw new BadRequestHttpException('Error while generating JWT'); + if ('' === $state || $state !== $expectedState) { + throw new BadRequestHttpException('OAuth state mismatch'); } + + if (!\is_string($codeVerifier) || '' === $codeVerifier) { + throw new BadRequestHttpException('OAuth code verifier missing from session'); + } + + $callback = $this->generateUrl('payplug_sylius_admin_auth_oauth_callback', [], UrlGeneratorInterface::ABSOLUTE_URL); + + // Legacy flow, superseded by PayplugUnifiedCore\Auth\OAuth2Client below. + // $jwt = Authentication::generateJWTOneShot($code, $callback, $clientId, $challenge); + // if ([] === $jwt || $jwt['httpStatus'] !== 200 || !\is_array($jwt['httpResponse'])) { + // throw new BadRequestHttpException('Error while generating JWT'); + // } + + $token = $this->buildOAuth2Client($callback)->exchangeAuthorizationCode($clientId, $code, $codeVerifier); + $paymentMethodId = $request->getSession()->get('payplug_sylius_oauth_payment_method_id'); if (null === $paymentMethodId) { throw new BadRequestHttpException('No payment method id found in session'); @@ -105,7 +134,7 @@ public function oauthCallback(Request $request): Response } $companyId = $request->getSession()->get('payplug_company_id'); - Payplug::init(['secretKey' => $jwt['httpResponse']['access_token']]); + Payplug::init(['secretKey' => $token->accessToken]); $clientName = 'Sylius - ' . $paymentMethod->getName(); $testClientDataResult = Authentication::createClientIdAndSecret($companyId, $clientName, 'test'); $liveClientDataResult = Authentication::createClientIdAndSecret($companyId, $clientName, 'live'); @@ -119,11 +148,9 @@ public function oauthCallback(Request $request): Response $this->cleanSession($request); $request->getSession()->getFlashBag()->add('success', 'payplug_sylius_payplug_plugin.admin.oauth_callback_success'); - // Clean previous cached client config - $cacheKeyLive = sprintf('payplug_%s_api_key_live', $gatewayConfig->getFactoryName()); - $cacheKeyTest = sprintf('payplug_%s_api_key_test', $gatewayConfig->getFactoryName()); - $this->cache->delete($cacheKeyLive); - $this->cache->delete($cacheKeyTest); + // Token cache invalidation is now handled internally by TokenManager, keyed by + // client_id — createClientIdAndSecret() above always mints a fresh client_id per + // OAuth run, so there is nothing stale to clean up here. // Ensure that the payment method is well configured $this->paymentMethodValidator->process($paymentMethod); @@ -152,7 +179,8 @@ private function cleanSession(Request $request): void $session = $request->getSession(); $session->remove('payplug_client_id'); $session->remove('payplug_company_id'); - $session->remove('payplug_oauth_challenge'); + $session->remove('payplug_oauth_state'); + $session->remove('payplug_oauth_code_verifier'); $session->remove('payplug_sylius_oauth_payment_method_id'); } } diff --git a/src/ApiClient/PayPlugApiClientFactory.php b/src/ApiClient/PayPlugApiClientFactory.php index 7d57cfe9..78da307e 100644 --- a/src/ApiClient/PayPlugApiClientFactory.php +++ b/src/ApiClient/PayPlugApiClientFactory.php @@ -4,19 +4,23 @@ namespace PayPlug\SyliusPayPlugPlugin\ApiClient; -use Payplug\Authentication; +// use Payplug\Authentication; // superseded by PayplugUnifiedCore\Auth\TokenManager below use PayPlug\SyliusPayPlugPlugin\Exception\GatewayConfigurationException; +use PayplugUnifiedCore\Auth\TokenManager; +use PayplugUnifiedCore\Exceptions\ApiException; use Sylius\Component\Payment\Model\GatewayConfigInterface; use Sylius\Component\Payment\Model\PaymentMethodInterface; use Sylius\Component\Resource\Repository\RepositoryInterface; use Symfony\Contracts\Cache\CacheInterface; -use Symfony\Contracts\Cache\ItemInterface; + +// use Symfony\Contracts\Cache\ItemInterface; // superseded, see getTokenForGatewayConfig() final class PayPlugApiClientFactory implements PayPlugApiClientFactoryInterface { public function __construct( private RepositoryInterface $gatewayConfigRepository, private CacheInterface $cache, + private TokenManager $tokenManager, ) { } @@ -54,26 +58,36 @@ private function getTokenForGatewayConfig(GatewayConfigInterface $gatewayConfig) } /** @var array $clientConfig */ $clientConfig = $rawClientConfig; - $cacheKey = sprintf('payplug_%s_api_key_%s', $gatewayConfig->getFactoryName(), $isLive ? 'live' : 'test'); - - return $this->cache->get($cacheKey, function (ItemInterface $item) use ($clientConfig) { - $response = Authentication::generateJWT($clientConfig['client_id'] ?? '', $clientConfig['client_secret'] ?? ''); - if ([] === $response || !is_array($response['httpResponse'])) { - throw new GatewayConfigurationException('Unable to connect to PayPlug API. Please check your credentials in the PayPlug plugin configuration.'); - } - - $accessToken = $response['httpResponse']['access_token']; - if (!is_string($accessToken)) { - throw new GatewayConfigurationException('Unable to connect to PayPlug API. Please check your credentials in the PayPlug plugin configuration.'); - } - $expiresIn = $response['httpResponse']['expires_in']; - if (!is_int($expiresIn)) { - $expiresIn = 200; - } - - $item->expiresAfter($expiresIn); - - return $accessToken; - }); + + // Legacy flow, superseded by PayplugUnifiedCore\Auth\TokenManager below. + // $cacheKey = sprintf('payplug_%s_api_key_%s', $gatewayConfig->getFactoryName(), $isLive ? 'live' : 'test'); + // return $this->cache->get($cacheKey, function (ItemInterface $item) use ($clientConfig) { + // $response = Authentication::generateJWT($clientConfig['client_id'] ?? '', $clientConfig['client_secret'] ?? ''); + // if ([] === $response || !is_array($response['httpResponse'])) { + // throw new GatewayConfigurationException('Unable to connect to PayPlug API. Please check your credentials in the PayPlug plugin configuration.'); + // } + // $accessToken = $response['httpResponse']['access_token']; + // if (!is_string($accessToken)) { + // throw new GatewayConfigurationException('Unable to connect to PayPlug API. Please check your credentials in the PayPlug plugin configuration.'); + // } + // $expiresIn = $response['httpResponse']['expires_in']; + // if (!is_int($expiresIn)) { + // $expiresIn = 200; + // } + // $item->expiresAfter($expiresIn); + // return $accessToken; + // }); + + $clientId = $clientConfig['client_id'] ?? ''; + $clientSecret = $clientConfig['client_secret'] ?? ''; + if ('' === $clientId || '' === $clientSecret) { + throw new GatewayConfigurationException('No client config found for ' . $gatewayConfig->getFactoryName() . '. Please renew your credentials in the PayPlug plugin configuration.'); + } + + try { + return $this->tokenManager->getValidToken($clientId, $clientSecret); + } catch (ApiException $e) { + throw new GatewayConfigurationException('Unable to connect to PayPlug API. Please check your credentials in the PayPlug plugin configuration.', 0, $e); + } } } diff --git a/tests/PHPUnit/Action/Admin/Auth/UnifiedAuthenticationControllerTest.php b/tests/PHPUnit/Action/Admin/Auth/UnifiedAuthenticationControllerTest.php new file mode 100644 index 00000000..98d323e5 --- /dev/null +++ b/tests/PHPUnit/Action/Admin/Auth/UnifiedAuthenticationControllerTest.php @@ -0,0 +1,254 @@ +router = $this->createMock(RouterInterface::class); + $this->paymentMethodRepository = $this->createMock(RepositoryInterface::class); + $this->entityManager = $this->createMock(EntityManagerInterface::class); + // final class — cannot be mocked by PHPUnit. Its process() method is never reached by + // the scenarios covered here (they all stop before that point), so a real instance + // wired with mocked collaborators is built purely to satisfy the constructor type-hint. + $this->paymentMethodValidator = new PaymentMethodValidator( + $this->createMock(RequestStack::class), + $this->createMock(ValidatorInterface::class), + $this->entityManager, + ); + $this->logger = $this->createMock(LoggerInterface::class); + $this->oauthHttpClient = $this->createMock(IOAuthHttpClient::class); + + $this->controller = new UnifiedAuthenticationController( + $this->router, + $this->paymentMethodRepository, + $this->entityManager, + $this->paymentMethodValidator, + $this->logger, + $this->oauthHttpClient, + 'https://api-qa.payplug.com', + 'https://www.payplug.com', + ); + + $this->controller->setContainer(new ServiceLocator([ + 'router' => fn () => $this->router, + ])); + } + + private function buildRequest(array $query = []): Request + { + $request = new Request($query); + $request->setSession(new Session(new MockArraySessionStorage())); + + return $request; + } + + /** + * PHPUnit resolves multiple `method('generate')->with(...)` stubs by registration order, not + * by which constraint actually matches a given call — a single callback branching on the + * route name is the only way to give different routes different return values reliably. + * + * @param array $routeUrls route name => URL to return + * @param array $throwForRoutes route names that should throw instead + */ + private function stubRouterGenerate(array $routeUrls, array $throwForRoutes = []): void + { + $this->router->method('generate')->willReturnCallback( + function (string $route) use ($routeUrls, $throwForRoutes): string { + if (\in_array($route, $throwForRoutes, true)) { + throw new \RuntimeException('router exploded for route ' . $route); + } + + return $routeUrls[$route] ?? '/admin/payment-methods'; + }, + ); + } + + // ------------------------------------------------------------------------- + // setupRedirection() — happy path + // ------------------------------------------------------------------------- + + public function testSetupRedirection_buildsAuthorizationUrlAndStoresPkceStateInSession(): void + { + $this->stubRouterGenerate(['payplug_sylius_admin_auth_oauth_callback' => 'https://shop.example.com/payplug/auth/oauth-callback']); + + $request = $this->buildRequest(['client_id' => 'client_abc', 'company_id' => 'company_xyz']); + + $response = $this->controller->setupRedirection($request); + + self::assertInstanceOf(RedirectResponse::class, $response); + self::assertStringStartsWith('https://api-qa.payplug.com/oauth2/auth?', $response->getTargetUrl()); + self::assertStringContainsString('client_id=client_abc', $response->getTargetUrl()); + self::assertStringContainsString('audience=' . urlencode('https://www.payplug.com'), $response->getTargetUrl()); + + $session = $request->getSession(); + self::assertSame('client_abc', $session->get('payplug_client_id')); + self::assertSame('company_xyz', $session->get('payplug_company_id')); + self::assertNotNull($session->get('payplug_oauth_state')); + self::assertNotNull($session->get('payplug_oauth_code_verifier')); + } + + // ------------------------------------------------------------------------- + // setupRedirection() — failure redirects to payment method index (no id in session yet) + // ------------------------------------------------------------------------- + + public function testSetupRedirection_onFailure_logsAndRedirectsToPaymentMethodIndex(): void + { + $this->stubRouterGenerate( + ['sylius_admin_payment_method_index' => '/admin/payment-methods'], + throwForRoutes: ['payplug_sylius_admin_auth_oauth_callback'], + ); + + $this->logger->expects(self::once())->method('critical') + ->with('Error while perform Payplug OAuth Setup redirection', self::anything()) + ; + + $request = $this->buildRequest(['client_id' => 'client_abc']); + + $response = $this->controller->setupRedirection($request); + + self::assertInstanceOf(RedirectResponse::class, $response); + self::assertSame('payplug_sylius_payplug_plugin.admin.oauth_setup_error', $request->getSession()->getFlashBag()->peek('error')[0] ?? null); + } + + // ------------------------------------------------------------------------- + // oauthCallback() — state mismatch is rejected before any token exchange + // ------------------------------------------------------------------------- + + public function testOauthCallback_withMismatchedState_rejectsBeforeExchangingToken(): void + { + $this->stubRouterGenerate([]); + $this->oauthHttpClient->expects(self::never())->method('post'); + $this->logger->expects(self::once())->method('critical') + ->with('Error while perform Payplug OAuth callback', self::anything()) + ; + + $request = $this->buildRequest(['code' => 'auth_code', 'state' => 'attacker-state']); + $request->getSession()->set('payplug_oauth_state', 'real-state'); + + $response = $this->controller->oauthCallback($request); + + self::assertInstanceOf(RedirectResponse::class, $response); + } + + public function testOauthCallback_withNoStateInSession_rejectsBeforeExchangingToken(): void + { + $this->stubRouterGenerate([]); + // Session never went through setupRedirection() (e.g. expired) — no expected state at all. + $this->oauthHttpClient->expects(self::never())->method('post'); + + $request = $this->buildRequest(['code' => 'auth_code', 'state' => 'some-state']); + + $response = $this->controller->oauthCallback($request); + + self::assertInstanceOf(RedirectResponse::class, $response); + } + + public function testOauthCallback_withEmptyState_rejectsBeforeExchangingToken(): void + { + $this->stubRouterGenerate([]); + $this->oauthHttpClient->expects(self::never())->method('post'); + + $request = $this->buildRequest(['code' => 'auth_code']); // no "state" query param at all + $request->getSession()->set('payplug_oauth_state', ''); + + $response = $this->controller->oauthCallback($request); + + self::assertInstanceOf(RedirectResponse::class, $response); + } + + // ------------------------------------------------------------------------- + // oauthCallback() — valid state, but no code verifier in session + // ------------------------------------------------------------------------- + + /** + * A missing/non-string code_verifier (e.g. session expired, or setupRedirection() was never + * hit) must be rejected before exchangeAuthorizationCode() is called, the same way a state + * mismatch already is — otherwise it falls through to a TypeError, logged as a noisy + * "critical" for what's really just an expired session. + */ + public function testOauthCallback_withMissingCodeVerifier_rejectsBeforeExchangingToken(): void + { + $this->stubRouterGenerate([]); + $this->oauthHttpClient->expects(self::never())->method('post'); + + $request = $this->buildRequest(['code' => 'auth_code', 'state' => 'matching-state']); + $request->getSession()->set('payplug_oauth_state', 'matching-state'); + // Deliberately no 'payplug_oauth_code_verifier' set. + + $response = $this->controller->oauthCallback($request); + + self::assertInstanceOf(RedirectResponse::class, $response); + } + + // ------------------------------------------------------------------------- + // oauthCallback() — valid state, but no payment method id in session + // ------------------------------------------------------------------------- + + public function testOauthCallback_withValidStateButNoPaymentMethodIdInSession_stopsAfterTokenExchange(): void + { + $this->stubRouterGenerate(['payplug_sylius_admin_auth_oauth_callback' => 'https://shop.example.com/payplug/auth/oauth-callback']); + + $this->oauthHttpClient->expects(self::once())->method('post')->willReturn([ + 'status' => 200, + 'body' => json_encode(['access_token' => 'jwt', 'expires_in' => 3600, 'token_type' => 'Bearer']), + ]); + + // Never reached: the "no payment method id" guard throws first. + $this->paymentMethodRepository->expects(self::never())->method('find'); + + $request = $this->buildRequest(['code' => 'auth_code', 'state' => 'matching-state']); + $request->getSession()->set('payplug_client_id', 'client_abc'); + $request->getSession()->set('payplug_oauth_state', 'matching-state'); + $request->getSession()->set('payplug_oauth_code_verifier', 'verifier_123'); + // Deliberately no 'payplug_sylius_oauth_payment_method_id' set. + + $response = $this->controller->oauthCallback($request); + + self::assertInstanceOf(RedirectResponse::class, $response); + self::assertSame('payplug_sylius_payplug_plugin.admin.oauth_setup_error', $request->getSession()->getFlashBag()->peek('error')[0] ?? null); + } +} diff --git a/tests/PHPUnit/ApiClient/PayPlugApiClientFactoryTest.php b/tests/PHPUnit/ApiClient/PayPlugApiClientFactoryTest.php new file mode 100644 index 00000000..a072416c --- /dev/null +++ b/tests/PHPUnit/ApiClient/PayPlugApiClientFactoryTest.php @@ -0,0 +1,175 @@ +gatewayConfigRepository = $this->createMock(RepositoryInterface::class); + $this->cache = $this->createMock(CacheInterface::class); + $this->oauthHttpClient = $this->createMock(IOAuthHttpClient::class); + $this->tokenCache = $this->createMock(ITokenCache::class); + + $oauth2Client = new OAuth2Client($this->oauthHttpClient, 'https://api-qa.payplug.com', '', '', 'https://www.payplug.com'); + $tokenManager = new TokenManager($this->tokenCache, $oauth2Client); + + $this->factory = new PayPlugApiClientFactory($this->gatewayConfigRepository, $this->cache, $tokenManager); + } + + // ------------------------------------------------------------------------- + // create() / createForPaymentMethod() — happy path, token freshly fetched + // ------------------------------------------------------------------------- + + public function testCreateForPaymentMethod_withValidClientCredentials_returnsApiClient(): void + { + $paymentMethod = $this->createMock(PaymentMethodInterface::class); + $paymentMethod->method('getGatewayConfig')->willReturn($this->buildGatewayConfig(isLive: false)); + + $this->tokenCache->method('get')->willReturn(null); // cache miss + $this->oauthHttpClient->method('post')->willReturn([ + 'status' => 200, + 'body' => json_encode(['access_token' => 'fresh-jwt', 'expires_in' => 300, 'token_type' => 'Bearer']), + ]); + + $client = $this->factory->createForPaymentMethod($paymentMethod); + + self::assertInstanceOf(PayPlugApiClientInterface::class, $client); + } + + public function testCreate_withNoGatewayConfigFound_throwsLogicException(): void + { + $this->gatewayConfigRepository->method('findOneBy')->willReturn(null); + + $this->expectException(\LogicException::class); + + $this->factory->create('payplug'); + } + + // ------------------------------------------------------------------------- + // getTokenForGatewayConfig() — missing client config + // ------------------------------------------------------------------------- + + public function testCreateForPaymentMethod_withNoClientConfigForCurrentMode_throwsGatewayConfigurationException(): void + { + $paymentMethod = $this->createMock(PaymentMethodInterface::class); + $gatewayConfig = $this->createMock(GatewayConfigInterface::class); + $gatewayConfig->method('getConfig')->willReturn(['live' => true]); // no 'live_client' key + $gatewayConfig->method('getFactoryName')->willReturn('payplug'); + $paymentMethod->method('getGatewayConfig')->willReturn($gatewayConfig); + + $this->expectException(GatewayConfigurationException::class); + $this->expectExceptionMessage('No client config found for payplug'); + + $this->factory->createForPaymentMethod($paymentMethod); + } + + // ------------------------------------------------------------------------- + // getTokenForGatewayConfig() — client config present but missing client_id/client_secret + // ------------------------------------------------------------------------- + + /** + * A present-but-incomplete client config (e.g. `client_secret` missing) must be rejected + * before any HTTP call is made — otherwise it reaches the token endpoint with an empty + * credential and a genuine misconfiguration gets reported as a connectivity failure instead. + */ + public function testCreateForPaymentMethod_withEmptyClientSecret_throwsGatewayConfigurationExceptionWithoutCallingTokenEndpoint(): void + { + $paymentMethod = $this->createMock(PaymentMethodInterface::class); + $gatewayConfig = $this->createMock(GatewayConfigInterface::class); + $gatewayConfig->method('getConfig')->willReturn([ + 'live' => false, + 'test_client' => ['client_id' => 'client_test'], // no 'client_secret' key + ]); + $gatewayConfig->method('getFactoryName')->willReturn('payplug'); + $paymentMethod->method('getGatewayConfig')->willReturn($gatewayConfig); + + $this->oauthHttpClient->expects(self::never())->method('post'); + + $this->expectException(GatewayConfigurationException::class); + $this->expectExceptionMessage('No client config found for payplug'); + + $this->factory->createForPaymentMethod($paymentMethod); + } + + // ------------------------------------------------------------------------- + // getTokenForGatewayConfig() — token endpoint failure wrapped as GatewayConfigurationException + // ------------------------------------------------------------------------- + + /** + * TokenManager -> OAuth2Client throws ApiException on a non-2xx response; the factory must + * catch it and rethrow as GatewayConfigurationException (never leak the vendor exception type). + */ + public function testCreateForPaymentMethod_whenTokenEndpointRejectsCredentials_wrapsFailureAsGatewayConfigurationException(): void + { + $paymentMethod = $this->createMock(PaymentMethodInterface::class); + $paymentMethod->method('getGatewayConfig')->willReturn($this->buildGatewayConfig(isLive: true)); + + $this->tokenCache->method('get')->willReturn(null); + $this->oauthHttpClient->method('post')->willReturn(['status' => 401, 'body' => '{"error":"invalid_client"}']); + + $this->expectException(GatewayConfigurationException::class); + $this->expectExceptionMessage('Unable to connect to PayPlug API. Please check your credentials in the PayPlug plugin configuration.'); + + $this->factory->createForPaymentMethod($paymentMethod); + } + + // ------------------------------------------------------------------------- + // getTokenForGatewayConfig() — a cached token is reused, no HTTP call made + // ------------------------------------------------------------------------- + + public function testCreateForPaymentMethod_withCachedToken_doesNotCallTheTokenEndpoint(): void + { + $paymentMethod = $this->createMock(PaymentMethodInterface::class); + $paymentMethod->method('getGatewayConfig')->willReturn($this->buildGatewayConfig(isLive: false)); + + $this->tokenCache->method('get')->willReturn('cached-jwt'); + $this->oauthHttpClient->expects(self::never())->method('post'); + + $this->factory->createForPaymentMethod($paymentMethod); + } + + private function buildGatewayConfig(bool $isLive): GatewayConfigInterface&MockObject + { + $gatewayConfig = $this->createMock(GatewayConfigInterface::class); + $gatewayConfig->method('getConfig')->willReturn([ + 'live' => $isLive, + 'live_client' => ['client_id' => 'client_live', 'client_secret' => 'secret_live'], + 'test_client' => ['client_id' => 'client_test', 'client_secret' => 'secret_test'], + ]); + $gatewayConfig->method('getFactoryName')->willReturn('payplug'); + + return $gatewayConfig; + } +} diff --git a/tests/PHPUnit/Auth/SyliusOAuthHttpClientTest.php b/tests/PHPUnit/Auth/SyliusOAuthHttpClientTest.php new file mode 100644 index 00000000..b7fae2fe --- /dev/null +++ b/tests/PHPUnit/Auth/SyliusOAuthHttpClientTest.php @@ -0,0 +1,102 @@ +httpClient = $this->createMock(HttpClientInterface::class); + $this->adapter = new SyliusOAuthHttpClient($this->httpClient); + } + + // ------------------------------------------------------------------------- + // post() — delegates to HttpClientInterface with form-encoded body + // ------------------------------------------------------------------------- + + /** + * Verifies the form params are sent as a URL-encoded body (not a raw array), and the given + * headers are passed through unchanged. + */ + public function testPost_sendsFormEncodedBodyAndHeaders(): void + { + $response = $this->createMock(ResponseInterface::class); + $response->method('getStatusCode')->willReturn(200); + $response->method('getContent')->with(false)->willReturn('{"access_token":"jwt"}'); + + $this->httpClient->expects(self::once()) + ->method('request') + ->with( + 'POST', + 'https://api-qa.payplug.com/oauth2/token', + [ + 'body' => 'grant_type=authorization_code&client_id=client_abc', + 'headers' => ['Content-Type' => 'application/x-www-form-urlencoded'], + ], + ) + ->willReturn($response) + ; + + $result = $this->adapter->post( + 'https://api-qa.payplug.com/oauth2/token', + ['grant_type' => 'authorization_code', 'client_id' => 'client_abc'], + ['Content-Type' => 'application/x-www-form-urlencoded'], + ); + + self::assertSame(['status' => 200, 'body' => '{"access_token":"jwt"}'], $result); + } + + // ------------------------------------------------------------------------- + // post() — non-2xx status does not throw (caller decides how to react) + // ------------------------------------------------------------------------- + + /** + * getContent(false) is used specifically so a 4xx/5xx response body is still returned + * instead of throwing — OAuth2Client itself is responsible for checking the status. + */ + public function testPost_onNon2xxStatus_returnsStatusAndBodyWithoutThrowing(): void + { + $response = $this->createMock(ResponseInterface::class); + $response->method('getStatusCode')->willReturn(401); + $response->method('getContent')->with(false)->willReturn('{"error":"invalid_client"}'); + + $this->httpClient->method('request')->willReturn($response); + + $result = $this->adapter->post('https://api-qa.payplug.com/oauth2/token', ['grant_type' => 'client_credentials']); + + self::assertSame(['status' => 401, 'body' => '{"error":"invalid_client"}'], $result); + } + + // ------------------------------------------------------------------------- + // post() — default empty headers array is accepted + // ------------------------------------------------------------------------- + + public function testPost_withNoHeadersArgument_defaultsToEmptyHeaders(): void + { + $response = $this->createMock(ResponseInterface::class); + $response->method('getStatusCode')->willReturn(200); + $response->method('getContent')->willReturn('{}'); + + $this->httpClient->expects(self::once()) + ->method('request') + ->with(self::anything(), self::anything(), self::callback( + static fn (array $options): bool => [] === $options['headers'], + )) + ->willReturn($response) + ; + + $this->adapter->post('https://api-qa.payplug.com/oauth2/token', ['grant_type' => 'client_credentials']); + } +} diff --git a/tests/PHPUnit/Auth/SyliusTokenCacheTest.php b/tests/PHPUnit/Auth/SyliusTokenCacheTest.php new file mode 100644 index 00000000..71331456 --- /dev/null +++ b/tests/PHPUnit/Auth/SyliusTokenCacheTest.php @@ -0,0 +1,115 @@ +pool = $this->createMock(CacheItemPoolInterface::class); + $this->cache = new SyliusTokenCache($this->pool); + } + + // ------------------------------------------------------------------------- + // get() — cache hit / miss + // ------------------------------------------------------------------------- + + public function testGet_onCacheHit_returnsTheStoredValue(): void + { + $item = $this->createMock(CacheItemInterface::class); + $item->method('isHit')->willReturn(true); + $item->method('get')->willReturn('cached-jwt'); + + $this->pool->method('getItem')->with('upc_oauth_token_client_abc')->willReturn($item); + + self::assertSame('cached-jwt', $this->cache->get('upc_oauth_token:client_abc')); + } + + public function testGet_onCacheMiss_returnsNull(): void + { + $item = $this->createMock(CacheItemInterface::class); + $item->method('isHit')->willReturn(false); + + $this->pool->method('getItem')->willReturn($item); + + self::assertNull($this->cache->get('upc_oauth_token:client_abc')); + } + + // ------------------------------------------------------------------------- + // set() — stores the value with the given TTL + // ------------------------------------------------------------------------- + + public function testSet_storesValueAndTtlThenSavesTheItem(): void + { + $item = $this->createMock(CacheItemInterface::class); + $item->expects(self::once())->method('set')->with('fresh-jwt'); + $item->expects(self::once())->method('expiresAfter')->with(240); + + $this->pool->method('getItem')->with('upc_oauth_token_client_abc')->willReturn($item); + $this->pool->expects(self::once())->method('save')->with($item); + + $this->cache->set('upc_oauth_token:client_abc', 'fresh-jwt', 240); + } + + // ------------------------------------------------------------------------- + // delete() + // ------------------------------------------------------------------------- + + public function testDelete_removesTheSanitizedKeyFromThePool(): void + { + $this->pool->expects(self::once())->method('deleteItem')->with('upc_oauth_token_client_abc'); + + $this->cache->delete('upc_oauth_token:client_abc'); + } + + // ------------------------------------------------------------------------- + // Key sanitization — PSR-6 reserved characters must never reach the pool + // ------------------------------------------------------------------------- + + /** + * Symfony's cache component rejects keys containing any of "{}()/\@:" with an + * InvalidArgumentException. TokenManager's own key format ("upc_oauth_token:{clientId}") + * contains a colon, so this is a real, not hypothetical, input. + * + * @dataProvider reservedCharacterKeys + */ + public function testSanitizeKey_replacesEveryPsr6ReservedCharacter( + string $rawKey, + string $expectedSanitizedKey, + ): void + { + $item = $this->createMock(CacheItemInterface::class); + $item->method('isHit')->willReturn(false); + + $this->pool->expects(self::once())->method('getItem')->with($expectedSanitizedKey)->willReturn($item); + + $this->cache->get($rawKey); + } + + /** + * @return iterable + */ + public static function reservedCharacterKeys(): iterable + { + yield 'colon (TokenManager\'s real format)' => ['upc_oauth_token:client_abc', 'upc_oauth_token_client_abc']; + yield 'curly braces' => ['a{b}c', 'a_b_c']; + yield 'parentheses' => ['a(b)c', 'a_b_c']; + yield 'slash' => ['a/b', 'a_b']; + yield 'backslash' => ['a\\b', 'a_b']; + yield 'at sign' => ['a@b', 'a_b']; + yield 'all reserved characters combined' => ['{}()/\\@:', '________']; + yield 'no reserved characters' => ['plain_key_123', 'plain_key_123']; + } +} From 1d340c1ab99b494bac84d2a96c38d0b9b566cb82 Mon Sep 17 00:00:00 2001 From: adumont-payplug Date: Wed, 22 Jul 2026 12:06:02 +0200 Subject: [PATCH 4/5] PRE-3563: adding coverage check on CI and review fixes --- .github/PULL_REQUEST_TEMPLATE.md | 1 + .github/workflows/ci.yml | 73 ++++++++++++++++++- .gitignore | 3 +- Dockerfile | 38 ++++++++++ Makefile | 14 ++++ README.md | 1 + composer.json | 1 + phpunit.xml.dist | 6 ++ .../Auth/UnifiedAuthenticationController.php | 24 ------ src/ApiClient/PayPlugApiClientFactory.php | 22 ------ 10 files changed, 134 insertions(+), 49 deletions(-) create mode 100644 Dockerfile diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 6fed9975..e0e42858 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -28,6 +28,7 @@ ### Testing - [ ] Unit tests added / updated +- [ ] New/changed code is covered by tests — SonarCloud Quality Gate (coverage on new code) passes on the `sonarcloud` CI job ### Security & Ops - [ ] No sensitive data or secrets introduced diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b1616306..d3874d07 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -19,13 +19,82 @@ jobs: with: symfony-versions: '["7.3"]' + # ----------------------------------------------------------------------- + # COVERAGE — generates a Clover coverage report (PCOV) for SonarCloud to + # ingest. Kept separate from the reusable sylius_phpunit matrix (which + # runs with coverage: none) so coverage generation stays this repo's + # own concern, same as payplug/unified-plugin-core's ci.yml. + # ----------------------------------------------------------------------- + coverage: + name: Coverage + if: github.base_ref == 'develop' + runs-on: ubuntu-latest + env: + APP_ENV: test + services: + mariadb: + image: 'mariadb:10.4.11' + ports: + - '3306:3306' + env: + MYSQL_ALLOW_EMPTY_PASSWORD: true + options: '--health-cmd="mysqladmin ping" --health-interval=5s --health-timeout=2s --health-retries=3' + steps: + - + uses: actions/checkout@v4 + - + name: 'Setup PHP 8.2' + uses: shivammathur/setup-php@v2 + with: + php-version: '8.2' + ini-values: date.timezone=UTC + extensions: intl + tools: symfony + coverage: pcov + - + name: 'Setup Node 20.x' + uses: actions/setup-node@v4 + with: + node-version: '20.x' + - + name: 'Composer - Get Cache Directory' + id: composer-cache + run: 'echo "dir=$(composer config cache-files-dir)" >> $GITHUB_OUTPUT' + - + name: 'Composer - Set cache' + uses: actions/cache@v4 + with: + path: '${{ steps.composer-cache.outputs.dir }}' + key: 'php-8.2-sylius-2.1.0-symfony-7.3-coverage-composer-${{ hashFiles(''**/composer.json'') }}' + restore-keys: 'php-8.2-sylius-2.1.0-symfony-7.3-coverage-composer-' + - + name: 'Composer - Github Auth' + run: 'composer config -g github-oauth.github.com ${{ github.token }}' + - + name: 'Install Sylius-Standard and Plugin' + run: 'make install -e SYLIUS_VERSION=2.1.0 SYMFONY_VERSION=7.3' + id: end-of-setup-sylius + - + name: 'Run tests with coverage' + run: 'composer test-coverage' + if: 'always() && steps.end-of-setup-sylius.outcome == ''success''' + - + name: 'Upload coverage report' + uses: actions/upload-artifact@v4 + with: + name: clover-coverage + path: build/logs/clover.xml + retention-days: 1 + sonarcloud: if: always() && !failure() && !cancelled() && github.base_ref == 'develop' - needs: [sylius-matrix] - uses: payplug/template-ci/.github/workflows/sonarcloud.yml@main + needs: [sylius-matrix, coverage] + uses: payplug/template-ci/.github/workflows/sonarcloud-coverage.yml@main with: project-name: 'github-payplug-payplug-syliuspayplugplugin' src-folder: 'src/' + coverage-report-artifact: 'clover-coverage' + enforce-quality-gate: true secrets: sonar-orga: ${{ secrets.SONAR_ORGA }} sonar-token: ${{ secrets.SONAR_TOKEN }} diff --git a/.gitignore b/.gitignore index 534a23cd..2900961e 100644 --- a/.gitignore +++ b/.gitignore @@ -25,4 +25,5 @@ CLAUDE.md .claude .review .phpunit.result.cache -docs/ \ No newline at end of file +docs/ +build/ \ No newline at end of file diff --git a/Dockerfile b/Dockerfile new file mode 100644 index 00000000..00f23d65 --- /dev/null +++ b/Dockerfile @@ -0,0 +1,38 @@ +FROM composer:2 AS composer + +FROM php:8.2-cli + +RUN apt-get update && apt-get install -y --no-install-recommends \ + git \ + unzip \ + libicu-dev \ + libzip-dev \ + libonig-dev \ + libxml2-dev \ + libpng-dev \ + libjpeg-dev \ + libfreetype6-dev \ + libsodium-dev \ + && docker-php-ext-configure gd --with-freetype --with-jpeg \ + && docker-php-ext-install -j$(nproc) \ + intl \ + gd \ + sodium \ + pdo_mysql \ + mbstring \ + xml \ + dom \ + simplexml \ + xmlwriter \ + zip \ + && pecl install pcov \ + && docker-php-ext-enable pcov \ + && rm -rf /var/lib/apt/lists/* + +COPY --from=composer /usr/bin/composer /usr/local/bin/composer + +RUN useradd --create-home --uid 1000 appuser + +WORKDIR /app + +USER appuser diff --git a/Makefile b/Makefile index b3007cc4..813d5336 100644 --- a/Makefile +++ b/Makefile @@ -8,6 +8,12 @@ SYLIUS_VERSION=2.1.0 SYMFONY_VERSION=6.4 PLUGIN_NAME=payplug/sylius-payplug-plugin +# Coverage runs inside Docker (PHP 8.2 + PCOV) instead of the host PHP, since the host's default +# `php`/`composer` may resolve to an unrelated version with no coverage driver installed. Assumes +# `vendor/` (and the Sylius test-application) is already installed on the host via `make install`. +IMAGE_DEV := sylius-payplug-plugin-dev +DOCKER_RUN := docker run --rm -v $(CURDIR):/app -w /app -u "$$(id -u):$$(id -g)" -e COMPOSER_HOME=/tmp/composer $(IMAGE_DEV) + ### ### DEVELOPMENT ### ¯¯¯¯¯¯¯¯¯¯¯ @@ -23,6 +29,14 @@ phpunit: ## Run PHPUnit tests ./vendor/bin/phpunit .PHONY: phpunit +build-dev: ## Build the Docker image used to run coverage + docker build -t $(IMAGE_DEV) . +.PHONY: build-dev + +coverage: build-dev ## Run PHPUnit tests with a Clover coverage report (build/logs/clover.xml), via Docker + $(DOCKER_RUN) composer test-coverage +.PHONY: coverage + ### ### OTHER ### ¯¯¯¯¯¯ diff --git a/README.md b/README.md index 40b8025e..d71a5990 100644 --- a/README.md +++ b/README.md @@ -2,6 +2,7 @@ [![Quality Gate Status](https://sonarcloud.io/api/project_badges/measure?project=github-payplug-payplug-syliuspayplugplugin&metric=alert_status&token=af29f9f3fbb3a74caff4e4a4d168bddab858f4dc)](https://sonarcloud.io/summary/new_code?id=github-payplug-payplug-syliuspayplugplugin) [![Duplicated Lines (%)](https://sonarcloud.io/api/project_badges/measure?project=github-payplug-payplug-syliuspayplugplugin&metric=duplicated_lines_density&token=af29f9f3fbb3a74caff4e4a4d168bddab858f4dc)](https://sonarcloud.io/summary/new_code?id=github-payplug-payplug-syliuspayplugplugin) [![Code Smells](https://sonarcloud.io/api/project_badges/measure?project=github-payplug-payplug-syliuspayplugplugin&metric=code_smells&token=af29f9f3fbb3a74caff4e4a4d168bddab858f4dc)](https://sonarcloud.io/summary/new_code?id=github-payplug-payplug-syliuspayplugplugin) +[![Coverage](https://sonarcloud.io/api/project_badges/measure?project=github-payplug-payplug-syliuspayplugplugin&metric=coverage&token=af29f9f3fbb3a74caff4e4a4d168bddab858f4dc)](https://sonarcloud.io/summary/new_code?id=github-payplug-payplug-syliuspayplugplugin) [![Version](https://img.shields.io/packagist/v/payplug/sylius-payplug-plugin.svg)](https://packagist.org/packages/payplug/sylius-payplug-plugin) [![Total Downloads](https://poser.pugx.org/payplug/sylius-payplug-plugin/downloads)](https://packagist.org/packages/payplug/sylius-payplug-plugin) diff --git a/composer.json b/composer.json index d2565406..a7e6c659 100755 --- a/composer.json +++ b/composer.json @@ -86,6 +86,7 @@ "phpmd": "phpmd src ansi ruleset/.php_md.xml", "phpstan": "phpstan analyse src -c ruleset/phpstan.neon", "phpunit": "phpunit tests/PHPUnit --colors=always", + "test-coverage": "phpunit tests/PHPUnit --colors=always --coverage-clover=build/logs/clover.xml", "tests": [ "@ecs", "@phpmd", diff --git a/phpunit.xml.dist b/phpunit.xml.dist index b01264ac..89387765 100644 --- a/phpunit.xml.dist +++ b/phpunit.xml.dist @@ -18,4 +18,10 @@ + + + + src + + diff --git a/src/Action/Admin/Auth/UnifiedAuthenticationController.php b/src/Action/Admin/Auth/UnifiedAuthenticationController.php index e1a5f7ab..38d45871 100644 --- a/src/Action/Admin/Auth/UnifiedAuthenticationController.php +++ b/src/Action/Admin/Auth/UnifiedAuthenticationController.php @@ -65,19 +65,6 @@ public function setupRedirection(Request $request): Response $request->getSession()->set('payplug_company_id', $companyId); $callBackUrl = $this->router->generate('payplug_sylius_admin_auth_oauth_callback', [], RouterInterface::ABSOLUTE_URL); - - // Legacy flow, superseded by PayplugUnifiedCore\Auth\OAuth2Client below. - // $challenge = bin2hex(openssl_random_pseudo_bytes(50)); - // $request->getSession()->set('payplug_oauth_challenge', $challenge); - // Authentication::initiateOAuth($clientId, $callBackUrl, $challenge); - // $headers = \headers_list(); - // foreach ($headers as $header) { - // if (str_starts_with($header, 'Location:')) { - // return new RedirectResponse(substr($header, 9)); - // } - // } - // throw new \LogicException('No location header found'); - $authorizationRequest = $this->buildOAuth2Client($callBackUrl)->buildAuthorizationUrl($clientId); $request->getSession()->set('payplug_oauth_state', $authorizationRequest->state); $request->getSession()->set('payplug_oauth_code_verifier', $authorizationRequest->codeVerifier); @@ -111,13 +98,6 @@ public function oauthCallback(Request $request): Response } $callback = $this->generateUrl('payplug_sylius_admin_auth_oauth_callback', [], UrlGeneratorInterface::ABSOLUTE_URL); - - // Legacy flow, superseded by PayplugUnifiedCore\Auth\OAuth2Client below. - // $jwt = Authentication::generateJWTOneShot($code, $callback, $clientId, $challenge); - // if ([] === $jwt || $jwt['httpStatus'] !== 200 || !\is_array($jwt['httpResponse'])) { - // throw new BadRequestHttpException('Error while generating JWT'); - // } - $token = $this->buildOAuth2Client($callback)->exchangeAuthorizationCode($clientId, $code, $codeVerifier); $paymentMethodId = $request->getSession()->get('payplug_sylius_oauth_payment_method_id'); @@ -148,10 +128,6 @@ public function oauthCallback(Request $request): Response $this->cleanSession($request); $request->getSession()->getFlashBag()->add('success', 'payplug_sylius_payplug_plugin.admin.oauth_callback_success'); - // Token cache invalidation is now handled internally by TokenManager, keyed by - // client_id — createClientIdAndSecret() above always mints a fresh client_id per - // OAuth run, so there is nothing stale to clean up here. - // Ensure that the payment method is well configured $this->paymentMethodValidator->process($paymentMethod); diff --git a/src/ApiClient/PayPlugApiClientFactory.php b/src/ApiClient/PayPlugApiClientFactory.php index 78da307e..6b7d8f8a 100644 --- a/src/ApiClient/PayPlugApiClientFactory.php +++ b/src/ApiClient/PayPlugApiClientFactory.php @@ -4,7 +4,6 @@ namespace PayPlug\SyliusPayPlugPlugin\ApiClient; -// use Payplug\Authentication; // superseded by PayplugUnifiedCore\Auth\TokenManager below use PayPlug\SyliusPayPlugPlugin\Exception\GatewayConfigurationException; use PayplugUnifiedCore\Auth\TokenManager; use PayplugUnifiedCore\Exceptions\ApiException; @@ -13,8 +12,6 @@ use Sylius\Component\Resource\Repository\RepositoryInterface; use Symfony\Contracts\Cache\CacheInterface; -// use Symfony\Contracts\Cache\ItemInterface; // superseded, see getTokenForGatewayConfig() - final class PayPlugApiClientFactory implements PayPlugApiClientFactoryInterface { public function __construct( @@ -59,25 +56,6 @@ private function getTokenForGatewayConfig(GatewayConfigInterface $gatewayConfig) /** @var array $clientConfig */ $clientConfig = $rawClientConfig; - // Legacy flow, superseded by PayplugUnifiedCore\Auth\TokenManager below. - // $cacheKey = sprintf('payplug_%s_api_key_%s', $gatewayConfig->getFactoryName(), $isLive ? 'live' : 'test'); - // return $this->cache->get($cacheKey, function (ItemInterface $item) use ($clientConfig) { - // $response = Authentication::generateJWT($clientConfig['client_id'] ?? '', $clientConfig['client_secret'] ?? ''); - // if ([] === $response || !is_array($response['httpResponse'])) { - // throw new GatewayConfigurationException('Unable to connect to PayPlug API. Please check your credentials in the PayPlug plugin configuration.'); - // } - // $accessToken = $response['httpResponse']['access_token']; - // if (!is_string($accessToken)) { - // throw new GatewayConfigurationException('Unable to connect to PayPlug API. Please check your credentials in the PayPlug plugin configuration.'); - // } - // $expiresIn = $response['httpResponse']['expires_in']; - // if (!is_int($expiresIn)) { - // $expiresIn = 200; - // } - // $item->expiresAfter($expiresIn); - // return $accessToken; - // }); - $clientId = $clientConfig['client_id'] ?? ''; $clientSecret = $clientConfig['client_secret'] ?? ''; if ('' === $clientId || '' === $clientSecret) { From b28730d2513598bb20bc495c91336d2293718ca8 Mon Sep 17 00:00:00 2001 From: adumont-payplug Date: Wed, 22 Jul 2026 15:29:39 +0200 Subject: [PATCH 5/5] PRE-3563: review fixes --- src/Auth/SyliusOAuthHttpClient.php | 28 +++++++++++++------ src/Auth/SyliusTokenCache.php | 3 +- .../Auth/SyliusOAuthHttpClientTest.php | 26 +++++++++++++++++ 3 files changed, 46 insertions(+), 11 deletions(-) diff --git a/src/Auth/SyliusOAuthHttpClient.php b/src/Auth/SyliusOAuthHttpClient.php index 12331be3..36611f3d 100644 --- a/src/Auth/SyliusOAuthHttpClient.php +++ b/src/Auth/SyliusOAuthHttpClient.php @@ -5,6 +5,7 @@ namespace PayPlug\SyliusPayPlugPlugin\Auth; use PayplugUnifiedCore\Contracts\IOAuthHttpClient; +use Symfony\Contracts\HttpClient\Exception\TransportExceptionInterface; use Symfony\Contracts\HttpClient\HttpClientInterface; final class SyliusOAuthHttpClient implements IOAuthHttpClient @@ -22,15 +23,24 @@ public function __construct( */ public function post(string $url, array $formParams, array $headers = []): array { - $response = $this->httpClient->request('POST', $url, [ - 'body' => http_build_query($formParams), - 'headers' => $headers, - ]); + try { + $response = $this->httpClient->request('POST', $url, [ + 'body' => http_build_query($formParams), + 'headers' => $headers, + ]); - return [ - 'status' => $response->getStatusCode(), - // false = don't throw on non-2xx; OAuth2Client itself checks the status. - 'body' => $response->getContent(false), - ]; + return [ + 'status' => $response->getStatusCode(), + // false = don't throw on non-2xx; OAuth2Client itself checks the status. + 'body' => $response->getContent(false), + ]; + } catch (TransportExceptionInterface $e) { + // Network-level failure (DNS, timeout, connection reset) — getStatusCode()/getContent() + // throw this regardless of the `false` above, since it only suppresses HTTP status + // exceptions, not transport ones. Status 0 makes OAuth2Client::requestToken() throw its + // own ApiException, which callers (e.g. PayPlugApiClientFactory) already catch and + // translate, the same way a non-2xx response from PayPlug itself would be handled. + return ['status' => 0, 'body' => $e->getMessage()]; + } } } diff --git a/src/Auth/SyliusTokenCache.php b/src/Auth/SyliusTokenCache.php index e8ca6fe0..1be872a8 100644 --- a/src/Auth/SyliusTokenCache.php +++ b/src/Auth/SyliusTokenCache.php @@ -22,10 +22,9 @@ public function get(string $key): ?string return null; } - /** @var string $value */ $value = $item->get(); - return $value; + return \is_string($value) ? $value : null; } public function set(string $key, string $value, int $ttlSeconds): void diff --git a/tests/PHPUnit/Auth/SyliusOAuthHttpClientTest.php b/tests/PHPUnit/Auth/SyliusOAuthHttpClientTest.php index b7fae2fe..eede4155 100644 --- a/tests/PHPUnit/Auth/SyliusOAuthHttpClientTest.php +++ b/tests/PHPUnit/Auth/SyliusOAuthHttpClientTest.php @@ -7,6 +7,7 @@ use PayPlug\SyliusPayPlugPlugin\Auth\SyliusOAuthHttpClient; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; +use Symfony\Component\HttpClient\Exception\TransportException; use Symfony\Contracts\HttpClient\HttpClientInterface; use Symfony\Contracts\HttpClient\ResponseInterface; @@ -99,4 +100,29 @@ public function testPost_withNoHeadersArgument_defaultsToEmptyHeaders(): void $this->adapter->post('https://api-qa.payplug.com/oauth2/token', ['grant_type' => 'client_credentials']); } + + // ------------------------------------------------------------------------- + // post() — transport-level failure (network error) does not throw + // ------------------------------------------------------------------------- + + /** + * getStatusCode()/getContent() throw TransportExceptionInterface on a genuine network error + * (DNS, timeout, connection reset) regardless of the `false` passed to getContent() — that + * flag only suppresses HTTP status exceptions, not transport ones. This must be caught here + * and turned into a status the caller can react to (0, i.e. never a valid HTTP status), + * instead of leaking an uncaught exception into OAuth2Client/TokenManager, which only know + * how to translate a malformed HTTP response into ApiException, not a transport failure. + */ + public function testPost_onTransportFailure_returnsZeroStatusInsteadOfThrowing(): void + { + $response = $this->createMock(ResponseInterface::class); + $response->method('getStatusCode')->willThrowException(new TransportException('Could not resolve host')); + + $this->httpClient->method('request')->willReturn($response); + + $result = $this->adapter->post('https://api-qa.payplug.com/oauth2/token', ['grant_type' => 'client_credentials']); + + self::assertSame(0, $result['status']); + self::assertSame('Could not resolve host', $result['body']); + } }