Feature/pre 3563 unified auth upc#306
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates PayPlug authentication/token acquisition toward payplug/unified-plugin-core by introducing Sylius adapters (HTTP + cache), switching PayPlugApiClientFactory to TokenManager, and updating the admin OAuth flow to PKCE/state-based OAuth2.
Changes:
- Add Sylius implementations of
IOAuthHttpClientandITokenCache, with PHPUnit coverage. - Switch
PayPlugApiClientFactoryfrom legacyAuthentication::generateJWT()toTokenManager::getValidToken(), and wire new services. - Update
UnifiedAuthenticationControllerto usePayplugUnifiedCore\Auth\OAuth2Clientfor PKCE/state and code exchange.
1. What's Good
SyliusOAuthHttpClientcorrectly usesgetContent(false)to avoid throwing on non-2xx and leaves status handling to the caller.SyliusTokenCacheexplicitly sanitizes PSR-6 reserved characters and has a dedicated data-provider test for it.config/services/client.xmlsetsautowire="true", which makes the new Unified Core services wiring straightforward.
2. Summary table
| Dimension | Rating |
|---|---|
| Security | company_id) |
| Correctness | |
| Performance | ✅ Fine |
| Maintainability | |
| Operational Implications | ❌ High (path repository + dev branch dependency likely breaks installs) |
3. Closing one-liner
Address the installability problem in composer.json and the few guard/test issues before merge.
4. Individual findings (one section per issue)
Heading: Operational Implications ❌ High
Subtitle (bold): Non-portable Composer dependency setup (composer.json:120)
+ "require": {
+ ...
+ "payplug/unified-plugin-core": "dev-feature/PRE-3563_oauth2_client",
+ ...
+ },
+ "repositories": [
+ {
+ "type": "path",
+ "url": "../unified-plugin-core",
+ "canonical": true,
+ "options": { "symlink": true }
+ }
+ ]Including a path repository pointing to a sibling directory will fail for typical users installing the plugin via Packagist/VCS (there is no ../unified-plugin-core on their filesystem). Pinning to a hard dev-* branch also makes releases brittle.
Fix: Publish/consume Unified Core via VCS/tagged release, and remove the path repo from the distributed composer.json (exact constraint/repo depends on your release workflow).
Heading: Correctness ⚠️ Medium
Subtitle (bold): Missing client_id / client_secret validation before TokenManager call (src/ApiClient/PayPlugApiClientFactory.php:81)
+ try {
+ return $this->tokenManager->getValidToken($clientConfig['client_id'] ?? '', $clientConfig['client_secret'] ?? '');
+ } catch (ApiException $e) {
+ throw new GatewayConfigurationException('Unable to connect to PayPlug API. Please check your credentials in the PayPlug plugin configuration.', 0, $e);
+ }If config keys are missing, this currently calls the token flow with empty strings and then wraps the error as an API connectivity/credentials issue. That makes a configuration problem harder to diagnose and can cause unnecessary HTTP calls.
Fix: Extract and validate both values first; throw GatewayConfigurationException for missing config before calling TokenManager.
Heading: Security ⚠️ Low
Subtitle (bold): company_id query param should be normalized to string (src/Action/Admin/Auth/UnifiedAuthenticationController.php:62)
- $companyId = $request->query->get('company_id');
+ $companyId = $request->query->getString('company_id');Request::query->get() can return arrays (e.g. ?company_id[]=x), and that value is later used in Authentication::createClientIdAndSecret(). Normalizing at the boundary avoids type confusion.
Fix: Use getString('company_id') (same approach as client_id).
Heading: Correctness ⚠️ Medium
Subtitle (bold): Missing guard for PKCE code verifier before token exchange (src/Action/Admin/Auth/UnifiedAuthenticationController.php:101)
+ /** @var string $codeVerifier */
+ $codeVerifier = $request->getSession()->get('payplug_oauth_code_verifier');
...
+ $token = $this->buildOAuth2Client($callback)->exchangeAuthorizationCode($clientId, $code, $codeVerifier);If the session expired or setup wasn’t completed, payplug_oauth_code_verifier may be null/non-string. Failing fast with a clear 400 avoids a vendor exception/TypeError and prevents an unnecessary token call.
Fix: Validate is_string($codeVerifier) && $codeVerifier !== '' before calling exchangeAuthorizationCode().
Heading: Correctness ⚠️ Medium
Subtitle (bold): Test expects an exception message that no longer matches (tests/PHPUnit/ApiClient/PayPlugApiClientFactoryTest.php:93)
- $this->expectExceptionMessage('No client config found for payplug');
+ // factory now appends guidance: "Please renew your credentials..."expectExceptionMessage() checks the full message, so the current assertion will fail.
Fix: Update the expected message to match the thrown message (or switch to expectExceptionMessageMatches() for a prefix/regex).
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/PHPUnit/Auth/SyliusTokenCacheTest.php | Adds coverage for cache hit/miss, TTL, deletion, and reserved-character sanitization. |
| tests/PHPUnit/Auth/SyliusOAuthHttpClientTest.php | Adds coverage for form-encoded POST behavior and non-2xx handling. |
| tests/PHPUnit/ApiClient/PayPlugApiClientFactoryTest.php | Adds tests for TokenManager-based token retrieval and error wrapping (one assertion needs update). |
| tests/PHPUnit/Action/Admin/Auth/UnifiedAuthenticationControllerTest.php | Adds unit coverage for PKCE setup and callback failure guards. |
| src/Auth/SyliusTokenCache.php | Implements Unified Core ITokenCache with PSR-6 key sanitization. |
| src/Auth/SyliusOAuthHttpClient.php | Implements Unified Core IOAuthHttpClient via Symfony HttpClient. |
| src/ApiClient/PayPlugApiClientFactory.php | Switches JWT generation to Unified Core TokenManager. |
| src/Action/Admin/Auth/UnifiedAuthenticationController.php | Replaces legacy OAuth initiation with Unified Core OAuth2Client PKCE/state flow. |
| config/services/client.xml | Registers Unified Core services and aliases to Sylius adapters. |
| config/services.yaml | Adds env-overridable OAuth base URL/audience parameters and binds for DI. |
| composer.json | Adds Unified Core dependency + local path repository (currently non-portable). |
| .gitignore | Ignores docs/ directory. |
a8be416 to
25beb1e
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
Description
Migrates this plugin's OAuth2 connection flow — the interactive authorization-code+PKCE login (
UnifiedAuthenticationController) and the background client-credentials token refresh (PayPlugApiClientFactory) — off the legacypayplug/payplug-phpAuthenticationclass and ontopayplug/unified-plugin-core's newAuth/OAuth2Client+Auth/TokenManager.Two new adapters bridge UPC's contracts to this plugin's existing infrastructure:
SyliusOAuthHttpClientimplements UPC'sIOAuthHttpClientvia Symfony'sHttpClientInterface. It also catchesTransportExceptionInterface(network-level failures — DNS, timeout, connection reset) rather than letting them escape uncaught, so a transient network blip is reported the same way a non-2xx response from PayPlug is: as anApiExceptionfromOAuth2Client, caught and translated byPayPlugApiClientFactoryintoGatewayConfigurationException— matching the resilience the legacyAuthentication::generateJWT()had (it caught all exceptions internally).SyliusTokenCacheimplements UPC'sITokenCachevia the PSR-6CacheItemPoolInterface, sanitizing cache keys since PSR-6 rejects{}()/\@:andTokenManager's own key format contains a colon.createClientIdAndSecret()(portal client provisioning, still called fromoauthCallback()after the token exchange) is untouched — out of scope, still going through the legacy SDK.This PR also adds a SonarCloud code coverage check to CI, mirroring the setup already in place on
payplug/unified-plugin-core. Previously this plugin'ssonarcloudjob ran static analysis only, with no coverage data for SonarCloud to report or gate on. A newcoveragejob installs the Sylius test application (PHP 8.2, PCOV) and runs PHPUnit with--coverage-clover, uploading the report as a workflow artifact;sonarcloudnow consumes it viasonarcloud-coverage.ymland enforces the Quality Gate. ADockerfile+make coveragetarget were added so coverage can also be generated locally (PHP 8.2 + PCOV in a container), since host PHP setups commonly lack a coverage driver for the required PHP version.Motivation:
The legacy flow had two real defects this closes, not just an internal refactor:
Authentication::initiateOAuth()did a rawheader('Location: ...')call, forcingsetupRedirection()to scrapeheaders_list()for aLocation:line to recover the URL.OAuth2Client::buildAuthorizationUrl()returns the URL directly instead.stateinternally but never returned it to the caller —oauthCallback()had no CSRF protection at all on the OAuth callback. This PR storesstate(andcode_verifier) in session frombuildAuthorizationUrl()'s return value and rejects the callback outright on a mismatch, before any token exchange happens.It also adds the first automated test coverage this connection flow has ever had —
UnifiedAuthenticationControllerTest,PayPlugApiClientFactoryTest, plus tests for both new adapters, including a case covering the transport-failure handling inSyliusOAuthHttpClient. (Full happy-path coverage ofoauthCallback()still stops at thecreateClientIdAndSecret()boundary — those are static calls on the legacy SDK and unmockable; see the docblock onUnifiedAuthenticationControllerTest.)Separately, the coverage-check addition to CI is meant to catch coverage regressions on new code going forward — worth noting current overall
src/coverage is only ~22% (693/3,111 statements), soenforce-quality-gate: trueon thesonarcloudjob may start blocking PRs sooner than expected until that baseline improves.Related issue(s): PRE-3563
Type of Change
[ ] 💥 Breaking change (fix or feature that causes existing functionality to change and that could impact other libs)
Checklist
Code Quality
Testing
sonarcloudCI jobSecurity & Ops
Files changed
OAuth2/PKCE migration:
src/Action/Admin/Auth/UnifiedAuthenticationController.php— built onOAuth2Client, state/PKCE stored in sessionsrc/ApiClient/PayPlugApiClientFactory.php— background token retrieval viaTokenManager::getValidToken()src/Auth/SyliusOAuthHttpClient.php— newIOAuthHttpClientadapter; catchesTransportExceptionInterfaceso network failures translate intoApiException/GatewayConfigurationExceptioninstead of an uncaught exception reaching payment processing (capture/refund/IPN/status)src/Auth/SyliusTokenCache.php— newITokenCacheadapterCoverage check in CI:
.github/workflows/ci.yml— newcoveragejob;sonarcloudjob switched tosonarcloud-coverage.yml@mainwithenforce-quality-gate: truecomposer.json— newtest-coveragescriptphpunit.xml.dist—<coverage><include>restricted tosrc/Dockerfile+Makefile—make coverageruns PHPUnit w/ PCOV in a container, for local parity with CIREADME.md— added SonarCloud Coverage badge.github/PULL_REQUEST_TEMPLATE.md— added coverage/Quality Gate checklist itemBefore merging, please double-check:
enforce-quality-gate: truewill fail this and most future PRs outright.