From c77f20d2f95637d73b65b6c0fd98815377c0ea38 Mon Sep 17 00:00:00 2001 From: Alex P Date: Wed, 8 Mar 2023 17:36:38 +0200 Subject: [PATCH 1/5] Fix name to avoid confusion --- .../src/StatusReportModule.php | 4 +-- modules/ppcp-webhooks/services.php | 4 +-- .../src/IncomingWebhookEndpoint.php | 28 +++++++++---------- ...nfoStorage.php => WebhookEventStorage.php} | 4 +-- .../ppcp-webhooks/src/WebhookRegistrar.php | 24 ++++++++-------- 5 files changed, 32 insertions(+), 32 deletions(-) rename modules/ppcp-webhooks/src/{WebhookInfoStorage.php => WebhookEventStorage.php} (95%) diff --git a/modules/ppcp-status-report/src/StatusReportModule.php b/modules/ppcp-status-report/src/StatusReportModule.php index 3e380b069..abf84b83b 100644 --- a/modules/ppcp-status-report/src/StatusReportModule.php +++ b/modules/ppcp-status-report/src/StatusReportModule.php @@ -20,7 +20,7 @@ use WooCommerce\PayPalCommerce\ApiClient\Helper\DccApplies; use WooCommerce\PayPalCommerce\Button\Helper\MessagesApply; use WooCommerce\PayPalCommerce\Compat\PPEC\PPECHelper; use WooCommerce\PayPalCommerce\Onboarding\State; -use WooCommerce\PayPalCommerce\Webhooks\WebhookInfoStorage; +use WooCommerce\PayPalCommerce\Webhooks\WebhookEventStorage; /** * Class StatusReportModule @@ -62,7 +62,7 @@ class StatusReportModule implements ModuleInterface { $messages_apply = $c->get( 'button.helper.messages-apply' ); $last_webhook_storage = $c->get( 'webhook.last-webhook-storage' ); - assert( $last_webhook_storage instanceof WebhookInfoStorage ); + assert( $last_webhook_storage instanceof WebhookEventStorage ); $billing_agreements_endpoint = $c->get( 'api.endpoint.billing-agreements' ); assert( $billing_agreements_endpoint instanceof BillingAgreementsEndpoint ); diff --git a/modules/ppcp-webhooks/services.php b/modules/ppcp-webhooks/services.php index d7ed8013c..d58065a4a 100644 --- a/modules/ppcp-webhooks/services.php +++ b/modules/ppcp-webhooks/services.php @@ -198,8 +198,8 @@ return array( ); }, - 'webhook.last-webhook-storage' => static function ( ContainerInterface $container ): WebhookInfoStorage { - return new WebhookInfoStorage( $container->get( 'webhook.last-webhook-storage.key' ) ); + 'webhook.last-webhook-storage' => static function ( ContainerInterface $container ): WebhookEventStorage { + return new WebhookEventStorage( $container->get( 'webhook.last-webhook-storage.key' ) ); }, 'webhook.last-webhook-storage.key' => static function ( ContainerInterface $container ): string { return 'ppcp-last-webhook'; diff --git a/modules/ppcp-webhooks/src/IncomingWebhookEndpoint.php b/modules/ppcp-webhooks/src/IncomingWebhookEndpoint.php index 9f8bbcf7a..4a7631572 100644 --- a/modules/ppcp-webhooks/src/IncomingWebhookEndpoint.php +++ b/modules/ppcp-webhooks/src/IncomingWebhookEndpoint.php @@ -77,11 +77,11 @@ class IncomingWebhookEndpoint { private $simulation; /** - * The last webhook info storage. + * The last webhook event storage. * - * @var WebhookInfoStorage + * @var WebhookEventStorage */ - private $last_webhook_storage; + private $last_webhook_event_storage; /** * IncomingWebhookEndpoint constructor. @@ -92,7 +92,7 @@ class IncomingWebhookEndpoint { * @param bool $verify_request Whether requests need to be verified or not. * @param WebhookEventFactory $webhook_event_factory The webhook event factory. * @param WebhookSimulation $simulation The simulation handler. - * @param WebhookInfoStorage $last_webhook_storage The last webhook info storage. + * @param WebhookEventStorage $last_webhook_event_storage The last webhook event storage. * @param RequestHandler ...$handlers The handlers, which process a request in the end. */ public function __construct( @@ -102,18 +102,18 @@ class IncomingWebhookEndpoint { bool $verify_request, WebhookEventFactory $webhook_event_factory, WebhookSimulation $simulation, - WebhookInfoStorage $last_webhook_storage, + WebhookEventStorage $last_webhook_event_storage, RequestHandler ...$handlers ) { - $this->webhook_endpoint = $webhook_endpoint; - $this->webhook = $webhook; - $this->handlers = $handlers; - $this->logger = $logger; - $this->verify_request = $verify_request; - $this->webhook_event_factory = $webhook_event_factory; - $this->last_webhook_storage = $last_webhook_storage; - $this->simulation = $simulation; + $this->webhook_endpoint = $webhook_endpoint; + $this->webhook = $webhook; + $this->handlers = $handlers; + $this->logger = $logger; + $this->verify_request = $verify_request; + $this->webhook_event_factory = $webhook_event_factory; + $this->last_webhook_event_storage = $last_webhook_event_storage; + $this->simulation = $simulation; } /** @@ -186,7 +186,7 @@ class IncomingWebhookEndpoint { public function handle_request( \WP_REST_Request $request ): \WP_REST_Response { $event = $this->event_from_request( $request ); - $this->last_webhook_storage->save( $event ); + $this->last_webhook_event_storage->save( $event ); if ( $this->simulation->is_simulation_event( $event ) ) { $this->logger->info( 'Received simulated webhook.' ); diff --git a/modules/ppcp-webhooks/src/WebhookInfoStorage.php b/modules/ppcp-webhooks/src/WebhookEventStorage.php similarity index 95% rename from modules/ppcp-webhooks/src/WebhookInfoStorage.php rename to modules/ppcp-webhooks/src/WebhookEventStorage.php index 9fb9865f6..2c2370c0e 100644 --- a/modules/ppcp-webhooks/src/WebhookInfoStorage.php +++ b/modules/ppcp-webhooks/src/WebhookEventStorage.php @@ -12,9 +12,9 @@ namespace WooCommerce\PayPalCommerce\Webhooks; use WooCommerce\PayPalCommerce\ApiClient\Entity\WebhookEvent; /** - * Class WebhookInfoStorage + * Class WebhookEventStorage */ -class WebhookInfoStorage { +class WebhookEventStorage { /** * The WP option key. diff --git a/modules/ppcp-webhooks/src/WebhookRegistrar.php b/modules/ppcp-webhooks/src/WebhookRegistrar.php index c16aa9acd..03769aa61 100644 --- a/modules/ppcp-webhooks/src/WebhookRegistrar.php +++ b/modules/ppcp-webhooks/src/WebhookRegistrar.php @@ -45,11 +45,11 @@ class WebhookRegistrar { private $rest_endpoint; /** - * The last webhook info storage. + * The last webhook event storage. * - * @var WebhookInfoStorage + * @var WebhookEventStorage */ - private $last_webhook_storage; + private $last_webhook_event_storage; /** * The logger. @@ -64,22 +64,22 @@ class WebhookRegistrar { * @param WebhookFactory $webhook_factory The Webhook factory. * @param WebhookEndpoint $endpoint The Webhook endpoint. * @param IncomingWebhookEndpoint $rest_endpoint The WordPress Rest API endpoint. - * @param WebhookInfoStorage $last_webhook_storage The last webhook info storage. + * @param WebhookEventStorage $last_webhook_event_storage The last webhook event storage. * @param LoggerInterface $logger The logger. */ public function __construct( WebhookFactory $webhook_factory, WebhookEndpoint $endpoint, IncomingWebhookEndpoint $rest_endpoint, - WebhookInfoStorage $last_webhook_storage, + WebhookEventStorage $last_webhook_event_storage, LoggerInterface $logger ) { - $this->webhook_factory = $webhook_factory; - $this->endpoint = $endpoint; - $this->rest_endpoint = $rest_endpoint; - $this->last_webhook_storage = $last_webhook_storage; - $this->logger = $logger; + $this->webhook_factory = $webhook_factory; + $this->endpoint = $endpoint; + $this->rest_endpoint = $rest_endpoint; + $this->last_webhook_event_storage = $last_webhook_event_storage; + $this->logger = $logger; } /** @@ -102,7 +102,7 @@ class WebhookRegistrar { self::KEY, $created->to_array() ); - $this->last_webhook_storage->clear(); + $this->last_webhook_event_storage->clear(); $this->logger->info( 'Webhooks subscribed.' ); return true; } catch ( RuntimeException $error ) { @@ -131,7 +131,7 @@ class WebhookRegistrar { if ( $success ) { delete_option( self::KEY ); - $this->last_webhook_storage->clear(); + $this->last_webhook_event_storage->clear(); $this->logger->info( 'Webhooks deleted.' ); } return $success; From 3d3fe3718587834de36c3f2e7a1fdf6a059ca4c2 Mon Sep 17 00:00:00 2001 From: Alex P Date: Thu, 9 Mar 2023 10:46:25 +0200 Subject: [PATCH 2/5] Improve webhook deletion error output --- .../src/Endpoint/WebhookEndpoint.php | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/modules/ppcp-api-client/src/Endpoint/WebhookEndpoint.php b/modules/ppcp-api-client/src/Endpoint/WebhookEndpoint.php index 252c1a294..d8cc06dca 100644 --- a/modules/ppcp-api-client/src/Endpoint/WebhookEndpoint.php +++ b/modules/ppcp-api-client/src/Endpoint/WebhookEndpoint.php @@ -175,12 +175,12 @@ class WebhookEndpoint { * * @param Webhook $hook The webhook to delete. * - * @return bool * @throws RuntimeException If the request fails. + * @throws PayPalApiException If the request fails. */ - public function delete( Webhook $hook ): bool { + public function delete( Webhook $hook ): void { if ( ! $hook->id() ) { - return false; + return; } $bearer = $this->bearer->bearer(); @@ -198,7 +198,18 @@ class WebhookEndpoint { __( 'Not able to delete the webhook.', 'woocommerce-paypal-payments' ) ); } - return wp_remote_retrieve_response_code( $response ) === 204; + + $status_code = (int) wp_remote_retrieve_response_code( $response ); + if ( 204 !== $status_code ) { + $json = null; + if ( is_array( $response ) ) { + $json = json_decode( $response['body'] ); + } + throw new PayPalApiException( + $json, + $status_code + ); + } } /** From 4c593372471884267176c245d6f0c0a349afab3d Mon Sep 17 00:00:00 2001 From: Alex P Date: Thu, 9 Mar 2023 10:47:29 +0200 Subject: [PATCH 3/5] Delete all webhooks before adding --- .../src/OnboardingRESTController.php | 3 +- .../src/Endpoint/ResubscribeEndpoint.php | 2 -- modules/ppcp-webhooks/src/WebhookModule.php | 1 - .../ppcp-webhooks/src/WebhookRegistrar.php | 30 +++++++++---------- 4 files changed, 16 insertions(+), 20 deletions(-) diff --git a/modules/ppcp-onboarding/src/OnboardingRESTController.php b/modules/ppcp-onboarding/src/OnboardingRESTController.php index 4eac5ba72..283cf64be 100644 --- a/modules/ppcp-onboarding/src/OnboardingRESTController.php +++ b/modules/ppcp-onboarding/src/OnboardingRESTController.php @@ -12,6 +12,7 @@ namespace WooCommerce\PayPalCommerce\Onboarding; use WooCommerce\PayPalCommerce\Vendor\Psr\Container\ContainerInterface; use WooCommerce\PayPalCommerce\WcGateway\Gateway\CreditCardGateway; use WooCommerce\PayPalCommerce\WcGateway\Gateway\PayPalGateway; +use WooCommerce\PayPalCommerce\Webhooks\WebhookRegistrar; /** * Exposes and handles REST routes related to onboarding. @@ -249,7 +250,7 @@ class OnboardingRESTController { } $webhook_registrar = $this->container->get( 'webhook.registrar' ); - $webhook_registrar->unregister(); + assert( $webhook_registrar instanceof WebhookRegistrar ); $webhook_registrar->register(); return array(); diff --git a/modules/ppcp-webhooks/src/Endpoint/ResubscribeEndpoint.php b/modules/ppcp-webhooks/src/Endpoint/ResubscribeEndpoint.php index b504cc38a..9fe20a1ed 100644 --- a/modules/ppcp-webhooks/src/Endpoint/ResubscribeEndpoint.php +++ b/modules/ppcp-webhooks/src/Endpoint/ResubscribeEndpoint.php @@ -62,8 +62,6 @@ class ResubscribeEndpoint { // Validate nonce. $this->request_data->read_request( $this->nonce() ); - $this->registrar->unregister(); - if ( ! $this->registrar->register() ) { wp_send_json_error( 'Webhook subscription failed.', 500 ); return false; diff --git a/modules/ppcp-webhooks/src/WebhookModule.php b/modules/ppcp-webhooks/src/WebhookModule.php index 6f8669a8b..e9cc66909 100644 --- a/modules/ppcp-webhooks/src/WebhookModule.php +++ b/modules/ppcp-webhooks/src/WebhookModule.php @@ -152,7 +152,6 @@ class WebhookModule implements ModuleInterface { add_action( 'init', function () use ( $registrar ) { - $registrar->unregister(); $registrar->register(); } ); diff --git a/modules/ppcp-webhooks/src/WebhookRegistrar.php b/modules/ppcp-webhooks/src/WebhookRegistrar.php index 03769aa61..8f8919ec2 100644 --- a/modules/ppcp-webhooks/src/WebhookRegistrar.php +++ b/modules/ppcp-webhooks/src/WebhookRegistrar.php @@ -88,6 +88,8 @@ class WebhookRegistrar { * @return bool */ public function register(): bool { + $this->unregister(); + $webhook = $this->webhook_factory->for_url_and_events( $this->rest_endpoint->url(), $this->rest_endpoint->handled_event_types() @@ -113,27 +115,23 @@ class WebhookRegistrar { /** * Unregister webhooks with PayPal. - * - * @return bool */ - public function unregister(): bool { - $data = (array) get_option( self::KEY, array() ); - if ( ! $data ) { - return false; - } + public function unregister(): void { try { - $webhook = $this->webhook_factory->from_array( $data ); - $success = $this->endpoint->delete( $webhook ); + $webhooks = $this->endpoint->list(); + foreach ( $webhooks as $webhook ) { + try { + $this->endpoint->delete( $webhook ); + } catch ( RuntimeException $deletion_error ) { + $this->logger->error( "Failed to delete webhook {$webhook->id()}: {$deletion_error->getMessage()}" ); + } + } } catch ( RuntimeException $error ) { $this->logger->error( 'Failed to delete webhooks: ' . $error->getMessage() ); - return false; } - if ( $success ) { - delete_option( self::KEY ); - $this->last_webhook_event_storage->clear(); - $this->logger->info( 'Webhooks deleted.' ); - } - return $success; + delete_option( self::KEY ); + $this->last_webhook_event_storage->clear(); + $this->logger->info( 'Webhooks deleted.' ); } } From 8188faeec4c1e96d686500fdfe2aa3814509f276 Mon Sep 17 00:00:00 2001 From: Alex P Date: Fri, 10 Mar 2023 10:34:28 +0200 Subject: [PATCH 4/5] Redirect on any credentials change (including sandbox checkbox) otherwise we may use wrong state/host e.g. for webhook subscription --- .../src/Settings/SettingsListener.php | 12 +----------- .../WcGateway/Settings/SettingsListenerTest.php | 2 ++ 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/modules/ppcp-wc-gateway/src/Settings/SettingsListener.php b/modules/ppcp-wc-gateway/src/Settings/SettingsListener.php index 980b0db80..829e005b3 100644 --- a/modules/ppcp-wc-gateway/src/Settings/SettingsListener.php +++ b/modules/ppcp-wc-gateway/src/Settings/SettingsListener.php @@ -322,16 +322,6 @@ class SettingsListener { } $this->settings->persist(); - if ( $credentials_change_status ) { - if ( in_array( - $credentials_change_status, - array( self::CREDENTIALS_ADDED, self::CREDENTIALS_CHANGED ), - true - ) ) { - $this->webhook_registrar->register(); - } - } - if ( $this->cache->has( PayPalBearer::CACHE_KEY ) ) { $this->cache->delete( PayPalBearer::CACHE_KEY ); } @@ -345,7 +335,7 @@ class SettingsListener { } $redirect_url = false; - if ( self::CREDENTIALS_ADDED === $credentials_change_status ) { + if ( self::CREDENTIALS_UNCHANGED !== $credentials_change_status ) { $redirect_url = $this->get_onboarding_redirect_url(); } diff --git a/tests/PHPUnit/WcGateway/Settings/SettingsListenerTest.php b/tests/PHPUnit/WcGateway/Settings/SettingsListenerTest.php index bad2dda33..403217a1b 100644 --- a/tests/PHPUnit/WcGateway/Settings/SettingsListenerTest.php +++ b/tests/PHPUnit/WcGateway/Settings/SettingsListenerTest.php @@ -85,6 +85,8 @@ class SettingsListenerTest extends ModularTestCase $dcc_status_cache->shouldReceive('has') ->andReturn(false); + expect('wp_safe_redirect')->once(); + $testee->listen(); } } From 9adcfe4b7d76d578fcd4b565f15176b9d5c5f2a3 Mon Sep 17 00:00:00 2001 From: Alex P Date: Fri, 10 Mar 2023 10:39:13 +0200 Subject: [PATCH 5/5] Hide webhook elements when sandbox state is not matching the server --- .../ppcp-webhooks/resources/js/status-page.js | 22 +++++++++++++++++++ modules/ppcp-webhooks/services.php | 3 ++- .../Assets/WebhooksStatusPageAssets.php | 22 ++++++++++++++----- 3 files changed, 41 insertions(+), 6 deletions(-) diff --git a/modules/ppcp-webhooks/resources/js/status-page.js b/modules/ppcp-webhooks/resources/js/status-page.js index f477ec403..8f5334f51 100644 --- a/modules/ppcp-webhooks/resources/js/status-page.js +++ b/modules/ppcp-webhooks/resources/js/status-page.js @@ -1,3 +1,5 @@ +import {setVisibleByClass} from "../../../ppcp-button/resources/js/modules/Helper/Hiding" + document.addEventListener( 'DOMContentLoaded', () => { @@ -147,5 +149,25 @@ document.addEventListener( simulateBtn.prop('disabled', false); } }); + + const sandboxCheckbox = document.querySelector('#ppcp-sandbox_on'); + if (sandboxCheckbox) { + const setWebhooksVisibility = (show) => { + [ + '#field-webhook_status_heading', + '#field-webhooks_list', + '#field-webhooks_resubscribe', + '#field-webhooks_simulate', + ].forEach(selector => { + setVisibleByClass(selector, show, 'hide'); + }); + }; + + const serverSandboxState = PayPalCommerceGatewayWebhooksStatus.environment === 'sandbox'; + setWebhooksVisibility(serverSandboxState === sandboxCheckbox.checked); + sandboxCheckbox.addEventListener('click', () => { + setWebhooksVisibility(serverSandboxState === sandboxCheckbox.checked); + }); + } } ); diff --git a/modules/ppcp-webhooks/services.php b/modules/ppcp-webhooks/services.php index d58065a4a..9e330e812 100644 --- a/modules/ppcp-webhooks/services.php +++ b/modules/ppcp-webhooks/services.php @@ -167,7 +167,8 @@ return array( 'webhook.status.assets' => function( ContainerInterface $container ) : WebhooksStatusPageAssets { return new WebhooksStatusPageAssets( $container->get( 'webhook.module-url' ), - $container->get( 'ppcp.asset-version' ) + $container->get( 'ppcp.asset-version' ), + $container->get( 'onboarding.environment' ) ); }, diff --git a/modules/ppcp-webhooks/src/Status/Assets/WebhooksStatusPageAssets.php b/modules/ppcp-webhooks/src/Status/Assets/WebhooksStatusPageAssets.php index 28a9ddd84..475c2a85e 100644 --- a/modules/ppcp-webhooks/src/Status/Assets/WebhooksStatusPageAssets.php +++ b/modules/ppcp-webhooks/src/Status/Assets/WebhooksStatusPageAssets.php @@ -9,6 +9,7 @@ declare(strict_types=1); namespace WooCommerce\PayPalCommerce\Webhooks\Status\Assets; +use WooCommerce\PayPalCommerce\Onboarding\Environment; use WooCommerce\PayPalCommerce\Webhooks\Endpoint\ResubscribeEndpoint; use WooCommerce\PayPalCommerce\Webhooks\Endpoint\SimulateEndpoint; use WooCommerce\PayPalCommerce\Webhooks\Endpoint\SimulationStateEndpoint; @@ -33,18 +34,28 @@ class WebhooksStatusPageAssets { */ private $version; + /** + * The environment object. + * + * @var Environment + */ + private $environment; + /** * WebhooksStatusPageAssets constructor. * - * @param string $module_url The URL to the module. - * @param string $version The assets version. + * @param string $module_url The URL to the module. + * @param string $version The assets version. + * @param Environment $environment The environment object. */ public function __construct( string $module_url, - string $version + string $version, + Environment $environment ) { - $this->module_url = untrailingslashit( $module_url ); - $this->version = $version; + $this->module_url = untrailingslashit( $module_url ); + $this->version = $version; + $this->environment = $environment; } /** @@ -103,6 +114,7 @@ class WebhooksStatusPageAssets { 'tooLongDelayMessage' => __( 'Looks like the webhook cannot be received. Check that your website is accessible from the internet.', 'woocommerce-paypal-payments' ), ), ), + 'environment' => $this->environment->current_environment(), ); }