Feature/pre 3440 multi shop configuration - #331
Conversation
There was a problem hiding this comment.
Review pass on the full diff
Reviewed in five passes: form/validation (PRE-3628/3629), credential scoping (PRE-3682/3683/3685), auth controllers + extractor + revoker (PRE-3631/3632), tests, config/translations. Full PHPUnit suite run under PHP 8.2: 589 tests, green.
What holds up
Several of the load-bearing claims in the description were checked against vendor source rather than taken on trust, and all of them hold:
- The wither immutability is watertight.
SyliusUpcConfigurationRepositoryusesclone $thisand assigns on the clone; nothing mutates$this. Every consumer insrc/scopes before use — there is no unscopedconfigurationRepository->call left. The cross-tenant leakage failure mode during IPN is closed. - The token-cache purge is correct, which is easy to get wrong:
TOKEN_CACHE_KEY_PREFIXmatches UPC'sTokenManagerexactly, and both the revoker andTokenManagergo through the same sharedSyliusTokenCache, so PSR-6 key sanitization is symmetric on write and delete.GatewayConnectionRevokerTestexercising a real cache over anArrayAdapterinstead of mockingITokenCacheis the right call. - The
@?CSRF argument is sound. Sylius' ownCsrfProtectionEnabledExtension::isCsrfProtectionEnabled()is literally$this->container->has('security.csrf.token_manager')— the template gate and the controller null check are the same condition, so there is no window where the link omits a token the controller then demands. The route also sits behind/admin, so the token is defence-in-depth, not the only authorization. IdTokenEmailExtractoris genuinely total. Every branch checked:json_decodewithoutJSON_THROW_ON_ERRORreturnsnullfor non-UTF8 and for depth > 512; a segment length ≡ 1 mod 4 produces a padbase64_decode(..., true)rejects;filter_varnever throws.- Errors added to
channelsdo not bubble to the root (ChoiceTypesetserror_bubbling => falseon the type itself), andForm::add()insidePOST_SET_DATAdoes re-map data into the replaced child (lockSetDatais only on duringPRE_SET_DATA). Both docblock claims are accurate. GatewayChannelConflictCheckeris the strongest piece here. The asymmetry betweenfindConflicts()(bails on a disabled subject) andfindClaimedChannels()(unconditional, minus the subject's own channels) is non-obvious and correct.SupportedMethodsProvideris a real bug fix on its own — the old??=let the first method's/accountpayload govern every later one in the list.
Two findings with no file in the diff
Three factory-name credential lookups the description does not list as remaining gaps. The breaking-change section says the only open half of PRE-3682 is client.xml's singletons. It isn't:
src/Provider/OneySupportedPaymentChoiceProvider.php:42—findOneByGatewayName(OneyGatewayFactory::FACTORY_NAME)src/Provider/Payment/ApplePayPaymentProvider.php:52and:209— same, for Apple Paysrc/Twig/OneyExtension.php:35—findOneBy(['factoryName' => OneyGatewayFactory::FACTORY_NAME])
findOneByGatewayName() is setMaxResults(1)->getSingleResult(), so with two Oney or two Apple Pay gateways it returns an arbitrary one — the exact bug this PR exists to kill, in shop-facing code (Apple Pay merchant-session/domain validation, Oney simulation display). Not necessarily in scope to fix here, but they should be listed and ticketed. Separately: that method is typed ?PaymentMethodInterface but getSingleResult() throws NoResultException rather than returning null — pre-existing.
No CHANGELOG.md / UPGRADE.md entry. There is an eight-row breaking-change table for anyone extending the plugin — dropped constructor arguments, changed interface signatures, a removed translation key, protected → public hooks. CHANGELOG.md has a live ## [2.0.0] - Unreleased section and neither file was touched. Integrators will not read a PR body.
Assessment
Ready to merge: with fixes. The credential-scoping architecture is sound and the immutability, cache-key and form-mechanics claims all check out. One critical issue (see the IntegratedPaymentController thread) turns a previously dormant assumption into an exploitable one, and the form layer that enforces the new per-channel rule traded its only real-form test for an all-mock one.
On the plan itself: the four pre-justified tradeoffs — GET + query-string CSRF, unsigned id_token parsing, leaving already-held channels selectable, the wither-based scoped repository — all survive scrutiny, and in three cases the reasoning is more careful than the summary lets on. Where the plan under-reaches is that it treats "thread the PaymentMethodInterface through" as sufficient without asking where that PaymentMethodInterface comes from. IntegratedPaymentController is where that omission bites.
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.
5e10bf7 to
2aecfe0
Compare
PRE-3628: scope gateway uniqueness validation per channel PRE-3628: validate base currency against submitted channels PRE-3628: fix CB base-currency gate to read mapped config PRE-3628: address final review polish items - de-dup CB base-currency form errors, not just flashes - flash() no longer throws with no request/session - drop 8 dead gatewayFactoryName property declarations - suppress PHPMD unused-param on shouldValidateBaseCurrency() - assert PaymentMethodTypeExtension::getExtendedTypes() - tighten PaymentMethodRepository docblocks to list<>
2aecfe0 to
d9c957a
Compare
jhoaraupp
left a comment
There was a problem hiding this comment.
Reviewed the full multi-shop scoping chain (credentials/API client, gateway-conflict checker, OAuth connect/disconnect, and the Oney/Apple Pay/simulation providers) via 4 parallel deep-dives. Overall this is careful, well-documented work — the core resolution chain (PayPlugApiClientFactory, ScopedConfigurationRepositoryInterface, IntegratedPaymentController, IPN/webhook trust chain, GatewayChannelConflictChecker, GatewayConnectionRevoker, IdTokenEmailExtractor) is sound, fail-loud where it matters, and the new test suites explicitly assert cross-account/cross-channel isolation rather than just happy paths.
Two of the findings below are HIGH: real regressions of the exact bug class this PR sets out to close (an arbitrary account gets used instead of the channel-scoped one). They're in code paths adjacent to the ones this PR touched but outside this diff, which is presumably why they slipped through.
Summary: 2 HIGH, 4 MEDIUM, 6 LOW/NIT. Three of them sit in files this PR doesn't actually touch, so they can't be left as inline comments here — listed below instead.
[HIGH] RefundUnitsCommandCreatorDecorator::canOneyRefundBeMade() authenticates with an arbitrary Oney account, not the order's own (src/Creator/RefundUnitsCommandCreatorDecorator.php:46,106-111, not touched by this PR)
$this->oneyClient is the ambiguous singleton built via PayPlugApiClientFactory::create('payplug_oney') → findOneBy(['factoryName' => ...]) — exactly the channel-ambiguous lookup this PR eliminates everywhere else. $lastPayment->getMethod() is already resolved a few lines above in fromRequest() and isn't threaded through here.
Failure scenario: merchant has Oney configured on channel A and channel B with two different PayPlug accounts (now legal since PRE-3628). Refunding an order from channel B within the 48h window calls retrieve() against whichever Oney config Doctrine's findOneBy returns first — if that's channel A's config, the refund is wrongly blocked (oney_transaction_less_than_forty_eight_hours) or errors, because it's authenticated against the wrong account.
- private PayPlugApiClientInterface $oneyClient,
+ private PayPlugApiClientFactoryInterface $apiClientFactory,
...
- $data = $this->oneyClient->retrieve($lastPayment->getDetails()['payment_id']);
+ $data = $this->apiClientFactory->createForPaymentMethod($lastPayment->getMethod())->retrieve($lastPayment->getDetails()['payment_id']);[MEDIUM] CardController signs saved-card deletion with an arbitrary CB account, not the card's owning one (src/Controller/CardController.php:30-31,76, not touched by this PR)
Same ambiguous @payplug_sylius_payplug_plugin.api_client.payplug singleton, no channel/payment-method scoping. With two enabled CB configs on different channels/accounts, a card-deletion request gets signed with an arbitrary one of the two. PayPlug card tokens are account-scoped, so today's practical effect is a functional failure (delete silently fails, NotFoundException caught, "deleted_error" flashed) rather than cross-tenant leakage — but it's broken in exactly the multi-shop configuration this PR enables.
[LOW] PostSavePaymentMethodEventListener::onCreate() never runs PaymentMethodValidator::process(), so the new constraint's claimed backstop isn't reached on creation (src/EventListener/PostSavePaymentMethodEventListener.php:29-40, not touched by this PR)
HasNoGatewayChannelConflict's docblock (src/Gateway/Validator/Constraints/HasNoGatewayChannelConflict.php:13-16) says it's "the backstop for every other write path … where no form listener runs", but onCreate() only calls startOAuth() — a raw admin-API POST create bypasses both the admin form's POST_SUBMIT listener (no form involved) and this create path, so a conflicting config created that way isn't caught until the next admin update. Pre-existing gap (the old canBeCreated() check had the identical hole), not a regression — but worth either closing or softening the docblock's claim.
One more MEDIUM with no single anchor line: none of OneySupportedPaymentChoiceProvider, ApplePayPaymentProvider, CachedSimulationDataProvider, OneyExtension, IsOneyEnabledValidator, PayplugPermissionValidator have PHPUnit coverage (pre-existing gap, but this PR adds meaningfully new branching — channel resolution, ChannelNotFoundException handling — precisely in these files with no new/updated tests; exactly this kind of test would have caught the isOneyEnabled() finding below).
Positive notes worth calling out: PayPlugApiClientFactoryInterface dropping create(string $factoryName) entirely so the compiler blocks reintroducing the unsafe lookup is a great API-design call; ScopedConfigurationRepositoryInterface's withGatewayConfig()/forPaymentMethod() returning cloned instances (not mutating the shared singleton) correctly avoids state leaking across requests for IPN/background refresh; and the GatewayChannelConflictChecker/PaymentMethodTypeExtension POST_SUBMIT-ordering reasoning is unusually well commented and checks out against Symfony's actual form-submission algorithm.
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.
|
Thanks — went through all twelve. Three landed in c709aa1 (Apple Pay double-resolution, the never-connected placeholder, the
Test coverage — fair. Deferring it to PRE-3458 alongside the scoping work, since those files are about to be rewritten and tests written against the current shape would be thrown away.
|
jhoaraupp
left a comment
There was a problem hiding this comment.
[MEDIUM] CardController signs saved-card deletion with an arbitrary CB account, not the card's owning one
Not part of this PR's diff (file untouched), so I can't anchor this as an inline diff comment — flagging it here instead since it falls squarely in the blast radius of the feature this PR ships.
src/Controller/CardController.php:30-31,76: $this->payPlugApiClient is autowired as #[Autowire('@payplug_sylius_payplug_plugin.api_client.payplug')], the same channel-ambiguous singleton this PR replaces everywhere else with PayPlugApiClientFactoryInterface::createForPaymentMethod(). $card carries no reference used to resolve the right account before calling deleteCard($cardToken).
Failure scenario: a merchant enables two "payplug" (CB) gateway configs on different channels, each connected to a different PayPlug account — exactly the multi-shop setup this PR enables. A customer's saved-card deletion request gets signed with an arbitrary one of the two accounts. Since PayPlug card tokens are account-scoped, the practical effect is a functional failure rather than cross-tenant leakage (delete silently fails via the caught NotFoundException, "deleted_error" flashed to the customer) — but it's a support-generating regression in the exact scenario the PR is meant to support.
Suggested fix (same pattern as the other call sites fixed in this PR): resolve the card's owning payment method (e.g. store it on the Card entity at creation time, or resolve it from the customer's channel context) and call $this->apiClientFactory->createForPaymentMethod($paymentMethod)->deleteCard($cardToken) instead of the fixed singleton.
c709aa1 to
9c44452
Compare
9c44452 to
880c277
Compare
Description
Multi-shop support: a Sylius installation can now hold several PayPlug gateway configurations of the same type, each connected to its own PayPlug account and scoped to its own set of channels.
Until now the plugin assumed one PayPlug account per installation.
AbstractGatewayConfigurationTyperefused the creation of a second gateway config for a factory name that already existed, and everything downstream — the API client, the/accountpayload, the UPC configuration repository — resolved credentials by factory name alone. That is fine with one account, and silently wrong with several: a name-based lookup returns an arbitrary config, so a request for channel A can be signed with channel B's credentials.This PR replaces the installation-wide uniqueness rule with a per-channel one, then threads the payment method (rather than the factory name) through every place that needs to know which account it is talking to, and gives the admin the two things that become necessary once several accounts coexist: seeing which account a gateway is connected to, and disconnecting one of them without touching the others.
Motivation: merchants running several shops on one Sylius installation need one PayPlug account per channel.
Related issue(s): PRE-3440 — includes PRE-3628, PRE-3629, PRE-3631, PRE-3632, PRE-3682, PRE-3683, PRE-3685.
PRE-3628 — per-channel gateway uniqueness
The rule is now: a channel may be linked to at most one enabled gateway config per factory type. Two CB gateways may coexist and both be enabled as long as their channel sets are disjoint; different factory types never conflict.
Checker/GatewayChannelConflictChecker— matches on channel code rather than object identity, ignores disabled gateways on both sides, and handles the not-yet-persisted subject (no id ⇒ can never match a rival).Gateway/Form/Extension/PaymentMethodTypeExtension— carries both the conflict rule and the base-currency rule on the root payment-method form. That move is the crux: Sylius addschannelsfromCoreBundle's own type extension, i.e. aftergatewayConfig, so a listener insidegatewayConfig.configruns beforeenabledandchannelsare submitted and can only ever see persisted data. RootPOST_SUBMITis the first point where the submitted channel set, the submittedenabledflag and the mapped gateway config all exist.AbstractGatewayConfigurationTypeloses itsPRE_SUBMITlistener, thecanBeCreated()/checkCreationRequirements()pair and two constructor dependencies; the per-gateway currency policy stays where it belongs (one hook per gateway type) and is read back by the extension.PayPlugGatewayFactory::resolveDisplayMode()instead of the unmappedDISPLAY_MODE_FIELDform key, which never reaches the persisted config.PaymentMethodRepository::findEnabledByGatewayName().form.only_one_gateway_allowed→form.gateway_channel_conflict(en/fr/it).PRE-3629 — claimed channels are unselectable in the picker
POST_SET_DATAon the root form replaces thechannelschild with a copy carrying achoice_attrclosure, so a channel already held by another enabled gateway of the same factory rendersdisabledwith atitlenaming the claiming payment method.Channels the edited payment method already holds are deliberately left selectable: browsers do not submit disabled checkboxes, so disabling a checked one would silently drop that channel on save. Pre-existing overlaps are reported by the submit-time rule instead. Both answers come from the same
claims()lookup, which is what keeps the picker and the validator in step.PRE-3682 / PRE-3683 / PRE-3685 — scoping credentials to the payment method
PayPlugApiClientFactoryInterface::create(string $factoryName)is removed from the interface.createForPaymentMethod()is now the only way application code can obtain a client — the compiler, not review, is the guard against reintroducing a channel-ambiguous lookup. The concretecreate()survives as@internalpurely for theclient.xmlservice-factory definitions (the remaining open half of PRE-3682, tracked separately).SupportedMethodsProviderfetches/accountper gateway config, memoized by persisted id (falling back tospl_object_idfor unflushed configs) instead of once per call — previously the first method's account governed every later one in the list. Thepayment_methodssub-key is resolved from the config the payload was fetched for.Upc/ScopedConfigurationRepositoryInterface— UPC'sIConfigurationRepositorytakes no context on any method (it was written for one account per installation). Rather than widen a shared contract that other plugins consume, the scope is carried Sylius-side by a sub-interface withwithGatewayConfig()/forPaymentMethod()withers: the repository is a shared service, and a mutable scope would leak across requests — IPN and background token refresh being exactly where that would go unnoticed.UnifiedApiPaymentCreatorInterface::createPayment(),OperationStatusFetcherInterface::getOperation(), the UHF command handlers,HostedFieldsWebhookNotificationHandler,IpnAction,OneClickAction,IntegratedPaymentController,PaymentStateResolver,CaptureAuthorizedPaymentProcessorand the Oney/permission validators all take or resolve the payment method now.PRE-3631 — the connected account, per gateway
New
Auth/IdTokenEmailExtractorreads theemailclaim out of the OAuthid_tokenat callback time andUnifiedAuthenticationControllerwrites it to the gateway config asaccount_email; a read-onlyconnected_account.html.twigrenders it on the update screen of all seven gateways.Worth knowing:
/accountcarries no email (verified live — the payload isid,company_ref,country,object,is_live,configuration,permissions,payment_methods), and neither does the client-credentials token used for background calls. The interactive authorization-code exchange is the only place the address exists, which is why it is captured at login rather than fetched on demand — same approach as the PrestaShop module. The extractor is total (any malformed input returnsnull) and deliberately does not verify the signature: the token arrives as the direct response body of a server-to-server POST, never via the browser, and the claim is display text, not an authorization decision. A gateway connected before this change shows the "re-authenticate" placeholder until the merchant reconnects.Requires
payplug/unified-plugin-core ^1.1.2, whereTokenOutputgained a nullableidToken— earlier versions dropid_tokenfrom the token response entirely. Constraint bumped accordingly.PRE-3632 — disconnect one gateway
New
UnifiedLogoutController+Auth/GatewayConnectionRevoker— the inverse of the OAuth callback, scoped to a single gateway config. It clearslive_client,test_clientandaccount_email, drops both cached UPC tokens, and disables the payment method.hfIdentifieris cleared only when the config is a CB gateway with Hosted Fields selected; elsewhere it is a merchant-typed value, not account-bound state.live,oneClick,deferredCapture, the display-mode flags andfees_forare untouched.renew_oauthcheckbox, which immediately mints new credentials — logout ends with none.PaymentMethodValidator::process()only ever disables, the merchant must re-tick "Enabled" by hand after reconnecting.GET, notPOST: the button is rendered inside the Sylius payment-method<form>, where a nested<form>would be invalid HTML. The CSRF token travels in the query string, the same shape as Sylius's ownsylius_admin_shipment_resend_confirmation_email.security.csrf.token_manageris injected with@?— it is absent when CSRF protection is off, and a hard reference would break container compilation for such an app.Type of Change
Breaking changes for anyone extending the plugin
PayPlugApiClientFactoryInterface::create(string $factoryName)createForPaymentMethod(PaymentMethodInterface $pm)UnifiedApiPaymentCreatorInterface::createPayment($dto)createPayment($dto, PaymentMethodInterface $method)OperationStatusFetcherInterface::getOperation($id)getOperation($id, PaymentMethodInterface $method)AbstractGatewayConfigurationType::__construct()—$gatewayConfigRepositoryand$requestStackdroppedshouldValidateBaseCurrency()/baseCurrencyViolationMessage()—protected→public, now take the mapped config$gatewayFactoryNameproperty on the 8 configuration typesform.only_one_gateway_allowedform.gateway_channel_conflict(%channel%,%payment_method%)PayplugUnifiedCore\Contracts\IConfigurationRepositoryScopedConfigurationRepositoryInterface, scoped per payment methodChecklist
Code Quality
Testing
New suites:
GatewayChannelConflictCheckerTest,PaymentMethodTypeExtensionTest,IdTokenEmailExtractorTest,GatewayConnectionRevokerTest,UnifiedLogoutControllerTest,IntegratedPaymentControllerTest, plus scoping coverage added to the UPC, API-client-factory andSupportedMethodsProvidertests.Security & Ops
Manual test plan
channelsfield.SupportedMethodsProvideramount limits and allowed countries follow the right account too).