Feature/pre 3563 unified auth upc - #306
Conversation
a8be416 to
25beb1e
Compare
Wiz Scan Summary
To detect these findings earlier in the dev lifecycle, try using Wiz Code VS Code Extension. |
PR #306 Review — Feature/PRE-3563 Unified Auth UPCOverviewMigrates the plugin's OAuth2 authentication off the legacy Real, meaningful fixes bundled in here:
Code quality & style
Specific issues1. Inconsistent guard against expired-session TypeErrors ( The PR explicitly adds a check for a missing/non-string /** @var string $clientId */
$clientId = $request->getSession()->get('payplug_client_id');
...
$token = $this->buildOAuth2Client($callback)->exchangeAuthorizationCode($clientId, $code, $codeVerifier);If the session expired (or 2. Two unrelated changes bundled in one PR The PR description itself flags this and suggests splitting — I'd second that. The OAuth/PKCE migration is security-sensitive and benefits from an isolated, focused review; the CI coverage-gate change is unrelated in scope and carries its own risk (per the description, current 3. Minor: Test coverageGood first-ever coverage for this flow:
Given finding #1, a test for "missing Security considerations
RecommendationSolid migration with a real security fix, well-tested. Before merging: add the missing |
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.