diff --git a/CLAUDE.md b/CLAUDE.md index b6d4d91..798a067 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -116,7 +116,8 @@ inline retry notice (HTTP 502). The controller resolves the api key from the pay config and fetches via `Quickpay/ClientFactory`. `Command/DoctorCommand` (`setono:sylius-quickpay:doctor`) machine-checks the README's Troubleshooting -section per configured gateway: api key ping, private key HMAC self-test (`CallbackValidator`), agreement +section per configured gateway: api key (via the shared `Quickpay/ApiKeyVerifier` ping-then-payments +probe), private key HMAC self-test (`CallbackValidator`), agreement existence (`GET agreements/{id}`, failing open on permission errors), order prefix length + cross-gateway uniqueness, and notify-route registration; `--live` creates a money-less test payment and attempts the link PUT to surface the missing-permission 403. Non-zero exit on any failed check; warnings don't fail. @@ -195,8 +196,11 @@ in that folder) and need `assets:install` in the host app. configs stored under the pre-2.0 option names (`apikey`/`privatekey`/`agreement` → `api_key`/`private_key`/`agreement_id`, the last normalized to int/null for the integer field) and folds a stored `auto_capture` into `use_authorize` (enabled → `false`). The `api_key` carries a `QuickpayCredentials` constraint - (sylius group) whose validator pings Quickpay via `Quickpay/ClientFactory` (symfony/http-client - capped at 5s) — an explicit 401/403 raises a violation, anything else fails open. + (sylius group) whose validator verifies the key through `Quickpay/ApiKeyVerifier` (shared with the + doctor command; backed by `Quickpay/ClientFactory`, symfony/http-client capped at 5s): ping first, + falling back to a one-item `/payments` read because Quickpay answers 401 on `/ping` both for an + invalid key and for a valid key whose api user lacks the `/ping` permission (verified live) — an + explicit 401/403 from the probe raises a violation, anything else fails open. Sylius' admin form theme ignores Symfony's `help_html` option, so the `payment_methods` docs link renders through the plugin's own form theme (`Resources/views/form/theme.html.twig`, scoped to that field's block prefix and registered by diff --git a/README.md b/README.md index a178fc4..025912c 100644 --- a/README.md +++ b/README.md @@ -122,10 +122,11 @@ out the gateway configuration: | Synchronized operations | Run capture, refund and cancel synchronously instead of relying on the Quickpay callback | | Branding id | *(optional)* The payment window branding to use | -When you save the payment method, the plugin verifies the API key against Quickpay's API (a lightweight -ping) and rejects the form if Quickpay rejects the key — a typo'd key is caught immediately instead of by -the first customer whose checkout fails. If Quickpay cannot be reached, the check is skipped so an outage -never blocks saving. +When you save the payment method, the plugin verifies the API key against Quickpay's API and rejects the +form if Quickpay rejects the key — a typo'd key is caught immediately instead of by the first customer +whose checkout fails. A valid key whose API user lacks the `/ping` permission is verified through a +`/payments` read instead, so a locked-down API user never blocks saving; neither does an unreachable +Quickpay — the check fails open on anything but an explicit rejection. ## How it works diff --git a/src/Command/DoctorCommand.php b/src/Command/DoctorCommand.php index f0997aa..2718c75 100644 --- a/src/Command/DoctorCommand.php +++ b/src/Command/DoctorCommand.php @@ -12,8 +12,9 @@ use Setono\Quickpay\Exception\UnauthorizedException; use Setono\Quickpay\Request\Payment\CreateLinkRequest; use Setono\Quickpay\Request\Payment\CreatePaymentRequest; -use Setono\Quickpay\Request\Payment\PaymentsQuery; use Setono\SyliusQuickpayPlugin\Quickpay\ApiKeyResolver; +use Setono\SyliusQuickpayPlugin\Quickpay\ApiKeyVerification; +use Setono\SyliusQuickpayPlugin\Quickpay\ApiKeyVerifierInterface; use Setono\SyliusQuickpayPlugin\Quickpay\ClientFactoryInterface; use Sylius\Bundle\PayumBundle\Model\GatewayConfigInterface; use Sylius\Component\Resource\Repository\RepositoryInterface; @@ -49,6 +50,7 @@ final class DoctorCommand extends Command */ public function __construct( private readonly ClientFactoryInterface $clientFactory, + private readonly ApiKeyVerifierInterface $apiKeyVerifier, private readonly RepositoryInterface $gatewayConfigRepository, private readonly RouterInterface $router, ) { @@ -164,26 +166,8 @@ private function checkApiKey(SymfonyStyle $io, array $config): ?ClientInterface return null; } - $client = $this->clientFactory->create($apiKey); - - try { - $client->ping(); - - $this->ok($io, 'The api key is accepted by Quickpay'); - - return $client; - } catch (UnauthorizedException|ForbiddenException) { - // Quickpay answers 401 both for an invalid key and for a valid key whose api user - // lacks the /ping permission (verified live), so fall through to a request every - // integration needs anyway - } catch (\Throwable $e) { - $this->warn($io, sprintf('Could not verify the api key, Quickpay did not answer: %s', $e->getMessage())); - - return null; - } - try { - $client->payments()->getPage(new PaymentsQuery(pageSize: 1)); + $verification = $this->apiKeyVerifier->verify($apiKey); } catch (UnauthorizedException|ForbiddenException) { $this->fail($io, 'Quickpay rejects the api key — use the API user\'s key (Settings → Users in the Quickpay manager), not a Payment Window agreement\'s'); @@ -194,9 +178,12 @@ private function checkApiKey(SymfonyStyle $io, array $config): ?ClientInterface return null; } - $this->ok($io, 'The api key is accepted by Quickpay (verified via /payments — the api user lacks the /ping permission, which is harmless)'); + $this->ok($io, match ($verification) { + ApiKeyVerification::ViaPing => 'The api key is accepted by Quickpay', + ApiKeyVerification::ViaPayments => 'The api key is accepted by Quickpay (verified via /payments — the api user lacks the /ping permission, which is harmless)', + }); - return $client; + return $this->clientFactory->create($apiKey); } /** diff --git a/src/Quickpay/ApiKeyVerification.php b/src/Quickpay/ApiKeyVerification.php new file mode 100644 index 0000000..14dcb2a --- /dev/null +++ b/src/Quickpay/ApiKeyVerification.php @@ -0,0 +1,16 @@ +clientFactory->create($apiKey); + + try { + $client->ping(); + + return ApiKeyVerification::ViaPing; + } catch (UnauthorizedException|ForbiddenException) { + // Either an invalid key or a valid key without the /ping permission — /payments decides + } + + $client->payments()->getPage(new PaymentsQuery(pageSize: 1)); + + return ApiKeyVerification::ViaPayments; + } +} diff --git a/src/Quickpay/ApiKeyVerifierInterface.php b/src/Quickpay/ApiKeyVerifierInterface.php new file mode 100644 index 0000000..3479e44 --- /dev/null +++ b/src/Quickpay/ApiKeyVerifierInterface.php @@ -0,0 +1,22 @@ + + + + + + + - + @@ -88,6 +95,7 @@ + diff --git a/src/Validator/Constraints/QuickpayCredentialsValidator.php b/src/Validator/Constraints/QuickpayCredentialsValidator.php index 6910335..6ea3603 100644 --- a/src/Validator/Constraints/QuickpayCredentialsValidator.php +++ b/src/Validator/Constraints/QuickpayCredentialsValidator.php @@ -6,14 +6,14 @@ use Setono\Quickpay\Exception\ForbiddenException; use Setono\Quickpay\Exception\UnauthorizedException; -use Setono\SyliusQuickpayPlugin\Quickpay\ClientFactoryInterface; +use Setono\SyliusQuickpayPlugin\Quickpay\ApiKeyVerifierInterface; use Symfony\Component\Validator\Constraint; use Symfony\Component\Validator\ConstraintValidator; use Symfony\Component\Validator\Exception\UnexpectedTypeException; final class QuickpayCredentialsValidator extends ConstraintValidator { - public function __construct(private readonly ClientFactoryInterface $clientFactory) + public function __construct(private readonly ApiKeyVerifierInterface $apiKeyVerifier) { } @@ -32,7 +32,7 @@ public function validate(mixed $value, Constraint $constraint): void } try { - $this->clientFactory->create($value)->ping(); + $this->apiKeyVerifier->verify($value); } catch (UnauthorizedException|ForbiddenException) { $this->context->buildViolation($constraint->message)->addViolation(); } catch (\Throwable) { diff --git a/tests/Command/DoctorCommandTest.php b/tests/Command/DoctorCommandTest.php index 90d5ac3..3a6978f 100644 --- a/tests/Command/DoctorCommandTest.php +++ b/tests/Command/DoctorCommandTest.php @@ -14,6 +14,7 @@ use Setono\Quickpay\Exception\ForbiddenException; use Setono\Quickpay\Exception\NotFoundException; use Setono\SyliusQuickpayPlugin\Command\DoctorCommand; +use Setono\SyliusQuickpayPlugin\Quickpay\ApiKeyVerifier; use Setono\SyliusQuickpayPlugin\Quickpay\ClientFactoryInterface; use Setono\SyliusQuickpayPlugin\Tests\Quickpay\QueuedResponsesHttpClient; use Sylius\Bundle\PayumBundle\Model\GatewayConfigInterface; @@ -532,8 +533,10 @@ private static function routes(bool $withNotifyRoute): RouteCollection private function executeCommand(array $input = []): CommandTester { $application = new Application(); + // A real verifier over the same client factory keeps the probe part of what is tested $application->add(new DoctorCommand( $this->clientFactory->reveal(), + new ApiKeyVerifier($this->clientFactory->reveal()), $this->gatewayConfigRepository->reveal(), $this->router->reveal(), )); diff --git a/tests/Form/Type/GatewayConfigurationTypeTest.php b/tests/Form/Type/GatewayConfigurationTypeTest.php index ca23029..fb9bb54 100644 --- a/tests/Form/Type/GatewayConfigurationTypeTest.php +++ b/tests/Form/Type/GatewayConfigurationTypeTest.php @@ -7,10 +7,10 @@ use Nyholm\Psr7\Response; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; -use Setono\Quickpay\Client\ClientInterface; use Setono\Quickpay\Exception\UnauthorizedException; use Setono\SyliusQuickpayPlugin\Form\Type\GatewayConfigurationType; -use Setono\SyliusQuickpayPlugin\Quickpay\ClientFactoryInterface; +use Setono\SyliusQuickpayPlugin\Quickpay\ApiKeyVerification; +use Setono\SyliusQuickpayPlugin\Quickpay\ApiKeyVerifierInterface; use Setono\SyliusQuickpayPlugin\Validator\Constraints\QuickpayCredentialsValidator; use Symfony\Component\Form\Extension\Validator\ValidatorExtension; use Symfony\Component\Form\FormExtensionInterface; @@ -32,17 +32,11 @@ final class GatewayConfigurationTypeTest extends TypeTestCase */ protected function getExtensions(): array { - $client = $this->prophesize(ClientInterface::class); - $client->ping()->willReturn(true); + $apiKeyVerifier = $this->prophesize(ApiKeyVerifierInterface::class); + $apiKeyVerifier->verify(Argument::type('string'))->willReturn(ApiKeyVerification::ViaPing); + $apiKeyVerifier->verify('rejected-api-key')->willThrow(new UnauthorizedException(new Response(401))); - $rejectingClient = $this->prophesize(ClientInterface::class); - $rejectingClient->ping()->willThrow(new UnauthorizedException(new Response(401))); - - $clientFactory = $this->prophesize(ClientFactoryInterface::class); - $clientFactory->create(Argument::type('string'))->willReturn($client); - $clientFactory->create('rejected-api-key')->willReturn($rejectingClient); - - $credentialsValidator = new QuickpayCredentialsValidator($clientFactory->reveal()); + $credentialsValidator = new QuickpayCredentialsValidator($apiKeyVerifier->reveal()); $validator = Validation::createValidatorBuilder() ->setConstraintValidatorFactory(new class($credentialsValidator) implements ConstraintValidatorFactoryInterface { diff --git a/tests/Quickpay/ApiKeyVerifierTest.php b/tests/Quickpay/ApiKeyVerifierTest.php new file mode 100644 index 0000000..bd869e3 --- /dev/null +++ b/tests/Quickpay/ApiKeyVerifierTest.php @@ -0,0 +1,99 @@ +createVerifier(new Response(200, [], '{}')); + + self::assertSame(ApiKeyVerification::ViaPing, $verifier->verify('the-api-key')); + } + + /** + * Quickpay answers 401 on /ping both for an invalid key and for a valid key whose api user + * lacks the /ping permission (verified live), so a rejected ping is decided by /payments + * + * @test + */ + public function it_verifies_a_key_without_the_ping_permission_through_payments(): void + { + $verifier = $this->createVerifier( + new Response(401, [], '{"message": "Invalid API key"}'), + new Response(200, [], '[]'), + ); + + self::assertSame(ApiKeyVerification::ViaPayments, $verifier->verify('the-api-key')); + } + + /** + * @test + */ + public function it_throws_when_quickpay_rejects_the_key_on_both_endpoints(): void + { + $verifier = $this->createVerifier( + new Response(401, [], '{"message": "Invalid API key"}'), + new Response(401, [], '{"message": "Invalid API key"}'), + ); + + $this->expectException(UnauthorizedException::class); + + $verifier->verify('the-api-key'); + } + + /** + * @test + */ + public function it_propagates_an_unanswered_ping(): void + { + $verifier = $this->createVerifier(new Response(500, [], '{"message": "boom"}')); + + $this->expectException(InternalServerErrorException::class); + + $verifier->verify('the-api-key'); + } + + /** + * @test + */ + public function it_propagates_an_unanswered_payments_fallback(): void + { + $verifier = $this->createVerifier( + new Response(401, [], '{"message": "Invalid API key"}'), + new Response(500, [], '{"message": "boom"}'), + ); + + $this->expectException(InternalServerErrorException::class); + + $verifier->verify('the-api-key'); + } + + private function createVerifier(Response ...$responses): ApiKeyVerifier + { + $clientFactory = $this->prophesize(ClientFactoryInterface::class); + $clientFactory + ->create('the-api-key') + ->willReturn(new Client('the-api-key', new QueuedResponsesHttpClient(...$responses))) + ; + + return new ApiKeyVerifier($clientFactory->reveal()); + } +} diff --git a/tests/Validator/Constraints/QuickpayCredentialsValidatorTest.php b/tests/Validator/Constraints/QuickpayCredentialsValidatorTest.php index f1b957f..04a3349 100644 --- a/tests/Validator/Constraints/QuickpayCredentialsValidatorTest.php +++ b/tests/Validator/Constraints/QuickpayCredentialsValidatorTest.php @@ -8,10 +8,10 @@ use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; use Prophecy\Prophecy\ObjectProphecy; -use Setono\Quickpay\Client\ClientInterface; use Setono\Quickpay\Exception\InternalServerErrorException; use Setono\Quickpay\Exception\UnauthorizedException; -use Setono\SyliusQuickpayPlugin\Quickpay\ClientFactoryInterface; +use Setono\SyliusQuickpayPlugin\Quickpay\ApiKeyVerification; +use Setono\SyliusQuickpayPlugin\Quickpay\ApiKeyVerifierInterface; use Setono\SyliusQuickpayPlugin\Validator\Constraints\QuickpayCredentials; use Setono\SyliusQuickpayPlugin\Validator\Constraints\QuickpayCredentialsValidator; use Symfony\Component\Validator\Constraints\NotBlank; @@ -25,20 +25,14 @@ final class QuickpayCredentialsValidatorTest extends ConstraintValidatorTestCase { use ProphecyTrait; - /** @var ObjectProphecy */ - private ObjectProphecy $clientFactory; - - /** @var ObjectProphecy */ - private ObjectProphecy $client; + /** @var ObjectProphecy */ + private ObjectProphecy $apiKeyVerifier; protected function createValidator(): QuickpayCredentialsValidator { - $this->client = $this->prophesize(ClientInterface::class); - - $this->clientFactory = $this->prophesize(ClientFactoryInterface::class); - $this->clientFactory->create('the-api-key')->willReturn($this->client); + $this->apiKeyVerifier = $this->prophesize(ApiKeyVerifierInterface::class); - return new QuickpayCredentialsValidator($this->clientFactory->reveal()); + return new QuickpayCredentialsValidator($this->apiKeyVerifier->reveal()); } /** @@ -46,7 +40,22 @@ protected function createValidator(): QuickpayCredentialsValidator */ public function it_accepts_a_key_quickpay_accepts(): void { - $this->client->ping()->willReturn(true); + $this->apiKeyVerifier->verify('the-api-key')->willReturn(ApiKeyVerification::ViaPing); + + $this->validator->validate('the-api-key', new QuickpayCredentials()); + + $this->assertNoViolation(); + } + + /** + * A valid key whose api user lacks the /ping permission is verified through /payments and must + * not raise a violation (see the verifier and issue #141) + * + * @test + */ + public function it_accepts_a_key_verified_through_the_payments_fallback(): void + { + $this->apiKeyVerifier->verify('the-api-key')->willReturn(ApiKeyVerification::ViaPayments); $this->validator->validate('the-api-key', new QuickpayCredentials()); @@ -58,7 +67,7 @@ public function it_accepts_a_key_quickpay_accepts(): void */ public function it_raises_a_violation_when_quickpay_rejects_the_key(): void { - $this->client->ping()->willThrow(new UnauthorizedException(new Response(401))); + $this->apiKeyVerifier->verify('the-api-key')->willThrow(new UnauthorizedException(new Response(401))); $constraint = new QuickpayCredentials(); $this->validator->validate('the-api-key', $constraint); @@ -71,7 +80,7 @@ public function it_raises_a_violation_when_quickpay_rejects_the_key(): void */ public function it_fails_open_when_quickpay_cannot_be_reached(): void { - $this->client->ping()->willThrow(new InternalServerErrorException(new Response(500))); + $this->apiKeyVerifier->verify('the-api-key')->willThrow(new InternalServerErrorException(new Response(500))); $this->validator->validate('the-api-key', new QuickpayCredentials()); @@ -83,7 +92,7 @@ public function it_fails_open_when_quickpay_cannot_be_reached(): void */ public function it_ignores_empty_values(): void { - $this->clientFactory->create(Argument::any())->shouldNotBeCalled(); + $this->apiKeyVerifier->verify(Argument::any())->shouldNotBeCalled(); $this->validator->validate(null, new QuickpayCredentials()); $this->validator->validate('', new QuickpayCredentials());