From c718b94937c0acc372e20ec9b7a5934236ec88f6 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 9 Oct 2021 10:35:10 +0200 Subject: [PATCH 1/4] Fixed crash when notifying orphan visits to a webhook --- .../EventDispatcher/NotifyVisitToWebHooks.php | 21 +++++------- .../NotifyVisitToWebHooksTest.php | 34 +++++++++++++------ 2 files changed, 33 insertions(+), 22 deletions(-) diff --git a/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php b/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php index dcd69b21..ad8381be 100644 --- a/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php +++ b/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php @@ -4,7 +4,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\EventDispatcher; -use Closure; use Doctrine\ORM\EntityManagerInterface; use Fig\Http\Message\RequestMethodInterface; use GuzzleHttp\ClientInterface; @@ -20,7 +19,6 @@ use Shlinkio\Shlink\Core\Options\AppOptions; use Throwable; use function Functional\map; -use function Functional\partial_left; class NotifyVisitToWebHooks { @@ -61,15 +59,16 @@ class NotifyVisitToWebHooks private function buildRequestOptions(Visit $visit): array { + $payload = ['visit' => $visit->jsonSerialize()]; + $shortUrl = $visit->getShortUrl(); + if ($shortUrl !== null) { + $payload['shortUrl'] = $this->transformer->transform($shortUrl); + } + return [ RequestOptions::TIMEOUT => 10, - RequestOptions::HEADERS => [ - 'User-Agent' => (string) $this->appOptions, - ], - RequestOptions::JSON => [ - 'shortUrl' => $this->transformer->transform($visit->getShortUrl()), - 'visit' => $visit->jsonSerialize(), - ], + RequestOptions::JSON => $payload, + RequestOptions::HEADERS => ['User-Agent' => $this->appOptions->__toString()], ]; } @@ -78,13 +77,11 @@ class NotifyVisitToWebHooks */ private function performRequests(array $requestOptions, string $visitId): array { - $logWebhookFailure = Closure::fromCallable([$this, 'logWebhookFailure']); - return map( $this->webhooks, fn (string $webhook): PromiseInterface => $this->httpClient ->requestAsync(RequestMethodInterface::METHOD_POST, $webhook, $requestOptions) - ->otherwise(partial_left($logWebhookFailure, $webhook, $visitId)), + ->otherwise(fn (Throwable $e) => $this->logWebhookFailure($webhook, $visitId, $e)), ); } diff --git a/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php index fcd97d2d..9e884753 100644 --- a/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php +++ b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php @@ -75,34 +75,39 @@ class NotifyVisitToWebHooksTest extends TestCase $requestAsync->shouldNotHaveBeenCalled(); } - /** @test */ - public function expectedRequestsArePerformedToWebhooks(): void + /** + * @test + * @dataProvider provideVisits + */ + public function expectedRequestsArePerformedToWebhooks(Visit $visit, array $expectedResponseKeys): void { $webhooks = ['foo', 'invalid', 'bar', 'baz']; $invalidWebhooks = ['invalid', 'baz']; - $find = $this->em->find(Visit::class, '1')->willReturn( - Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance()), - ); + $find = $this->em->find(Visit::class, '1')->willReturn($visit); $requestAsync = $this->httpClient->requestAsync( RequestMethodInterface::METHOD_POST, Argument::type('string'), - Argument::that(function (array $requestOptions) { + Argument::that(function (array $requestOptions) use ($expectedResponseKeys) { Assert::assertArrayHasKey(RequestOptions::HEADERS, $requestOptions); Assert::assertArrayHasKey(RequestOptions::JSON, $requestOptions); Assert::assertArrayHasKey(RequestOptions::TIMEOUT, $requestOptions); Assert::assertEquals($requestOptions[RequestOptions::TIMEOUT], 10); Assert::assertEquals($requestOptions[RequestOptions::HEADERS], ['User-Agent' => 'Shlink:v1.2.3']); - Assert::assertArrayHasKey('shortUrl', $requestOptions[RequestOptions::JSON]); - Assert::assertArrayHasKey('visit', $requestOptions[RequestOptions::JSON]); + + $json = $requestOptions[RequestOptions::JSON]; + Assert::assertCount(count($expectedResponseKeys), $json); + foreach ($expectedResponseKeys as $key) { + Assert::assertArrayHasKey($key, $json); + } return $requestOptions; }), )->will(function (array $args) use ($invalidWebhooks) { [, $webhook] = $args; - $e = new Exception(''); + $shouldReject = contains($invalidWebhooks, $webhook); - return contains($invalidWebhooks, $webhook) ? new RejectedPromise($e) : new FulfilledPromise(''); + return $shouldReject ? new RejectedPromise(new Exception('')) : new FulfilledPromise(''); }); $logWarning = $this->logger->warning( 'Failed to notify visit with id "{visitId}" to webhook "{webhook}". {e}', @@ -122,6 +127,15 @@ class NotifyVisitToWebHooksTest extends TestCase $logWarning->shouldHaveBeenCalledTimes(count($invalidWebhooks)); } + public function provideVisits(): iterable + { + yield 'regular visit' => [ + Visit::forValidShortUrl(ShortUrl::createEmpty(), Visitor::emptyInstance()), + ['shortUrl', 'visit'], + ]; + yield 'orphan visit' => [Visit::forBasePath(Visitor::emptyInstance()), ['visit'],]; + } + private function createListener(array $webhooks): NotifyVisitToWebHooks { return new NotifyVisitToWebHooks( From d16fda3f1698f952cad60b83fedaecc357c3eaeb Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 9 Oct 2021 10:53:21 +0200 Subject: [PATCH 2/4] Added option to send orphan visits to webhooks --- config/autoload/redirects.global.php | 9 +++++ config/autoload/url-shortener.global.php | 8 ---- config/autoload/webhooks.global.php | 19 +++++++++ module/Core/config/dependencies.config.php | 2 + .../Core/config/event_dispatcher.config.php | 2 +- .../EventDispatcher/NotifyVisitToWebHooks.php | 12 ++++-- module/Core/src/Options/WebhookOptions.php | 40 +++++++++++++++++++ .../NotifyVisitToWebHooksTest.php | 25 +++++++++++- 8 files changed, 102 insertions(+), 15 deletions(-) create mode 100644 config/autoload/webhooks.global.php create mode 100644 module/Core/src/Options/WebhookOptions.php diff --git a/config/autoload/redirects.global.php b/config/autoload/redirects.global.php index 0707f1e4..339ca27d 100644 --- a/config/autoload/redirects.global.php +++ b/config/autoload/redirects.global.php @@ -4,6 +4,9 @@ declare(strict_types=1); use function Shlinkio\Shlink\Common\env; +use const Shlinkio\Shlink\DEFAULT_REDIRECT_CACHE_LIFETIME; +use const Shlinkio\Shlink\DEFAULT_REDIRECT_STATUS_CODE; + return [ 'not_found_redirects' => [ @@ -12,4 +15,10 @@ return [ 'base_url' => env('BASE_URL_REDIRECT_TO'), ], + 'url_shortener' => [ + // TODO Move these options to their own config namespace. Maybe "redirects". + 'redirect_status_code' => (int) env('REDIRECT_STATUS_CODE', DEFAULT_REDIRECT_STATUS_CODE), + 'redirect_cache_lifetime' => (int) env('REDIRECT_CACHE_LIFETIME', DEFAULT_REDIRECT_CACHE_LIFETIME), + ], + ]; diff --git a/config/autoload/url-shortener.global.php b/config/autoload/url-shortener.global.php index 432c63e1..ae6bfe84 100644 --- a/config/autoload/url-shortener.global.php +++ b/config/autoload/url-shortener.global.php @@ -4,13 +4,10 @@ declare(strict_types=1); use function Shlinkio\Shlink\Common\env; -use const Shlinkio\Shlink\DEFAULT_REDIRECT_CACHE_LIFETIME; -use const Shlinkio\Shlink\DEFAULT_REDIRECT_STATUS_CODE; use const Shlinkio\Shlink\DEFAULT_SHORT_CODES_LENGTH; use const Shlinkio\Shlink\MIN_SHORT_CODES_LENGTH; return (static function (): array { - $webhooks = env('VISITS_WEBHOOKS'); $shortCodesLength = (int) env('DEFAULT_SHORT_CODES_LENGTH', DEFAULT_SHORT_CODES_LENGTH); $shortCodesLength = $shortCodesLength < MIN_SHORT_CODES_LENGTH ? MIN_SHORT_CODES_LENGTH : $shortCodesLength; $useHttps = env('USE_HTTPS'); // Deprecated. For v3, set this to true by default, instead of null @@ -24,14 +21,9 @@ return (static function (): array { 'hostname' => env('DEFAULT_DOMAIN', env('SHORT_DOMAIN_HOST', '')), ], 'validate_url' => (bool) env('VALIDATE_URLS', false), // Deprecated - 'visits_webhooks' => $webhooks === null ? [] : explode(',', $webhooks), 'default_short_codes_length' => $shortCodesLength, 'auto_resolve_titles' => (bool) env('AUTO_RESOLVE_TITLES', false), 'append_extra_path' => (bool) env('REDIRECT_APPEND_EXTRA_PATH', false), - - // TODO Move these two options to their own config namespace. Maybe "redirects". - 'redirect_status_code' => (int) env('REDIRECT_STATUS_CODE', DEFAULT_REDIRECT_STATUS_CODE), - 'redirect_cache_lifetime' => (int) env('REDIRECT_CACHE_LIFETIME', DEFAULT_REDIRECT_CACHE_LIFETIME), ], ]; diff --git a/config/autoload/webhooks.global.php b/config/autoload/webhooks.global.php new file mode 100644 index 00000000..585d3eb2 --- /dev/null +++ b/config/autoload/webhooks.global.php @@ -0,0 +1,19 @@ + [ + // TODO Move these options to their own config namespace + 'visits_webhooks' => $webhooks === null ? [] : explode(',', $webhooks), + 'notify_orphan_visits_to_webhooks' => (bool) env('NOTIFY_ORPHAN_VISITS_TO_WEBHOOKS', false), + ], + + ]; +})(); diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 7c3d7468..16b84819 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -26,6 +26,7 @@ return [ Options\UrlShortenerOptions::class => ConfigAbstractFactory::class, Options\TrackingOptions::class => ConfigAbstractFactory::class, Options\QrCodeOptions::class => ConfigAbstractFactory::class, + Options\WebhookOptions::class => ConfigAbstractFactory::class, Service\UrlShortener::class => ConfigAbstractFactory::class, Service\ShortUrlService::class => ConfigAbstractFactory::class, @@ -88,6 +89,7 @@ return [ Options\UrlShortenerOptions::class => ['config.url_shortener'], Options\TrackingOptions::class => ['config.tracking'], Options\QrCodeOptions::class => ['config.qr_codes'], + Options\WebhookOptions::class => ['config.url_shortener'], // TODO This config is currently under url_shortener Service\UrlShortener::class => [ ShortUrl\Helper\ShortUrlTitleResolutionHelper::class, diff --git a/module/Core/config/event_dispatcher.config.php b/module/Core/config/event_dispatcher.config.php index bddd59f5..5256bc92 100644 --- a/module/Core/config/event_dispatcher.config.php +++ b/module/Core/config/event_dispatcher.config.php @@ -58,7 +58,7 @@ return [ 'httpClient', 'em', 'Logger_Shlink', - 'config.url_shortener.visits_webhooks', + Options\WebhookOptions::class, ShortUrl\Transformer\ShortUrlDataTransformer::class, Options\AppOptions::class, ], diff --git a/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php b/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php index ad8381be..b5c2e501 100644 --- a/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php +++ b/module/Core/src/EventDispatcher/NotifyVisitToWebHooks.php @@ -16,6 +16,7 @@ use Shlinkio\Shlink\Common\Rest\DataTransformerInterface; use Shlinkio\Shlink\Core\Entity\Visit; use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; use Shlinkio\Shlink\Core\Options\AppOptions; +use Shlinkio\Shlink\Core\Options\WebhookOptions; use Throwable; use function Functional\map; @@ -26,8 +27,7 @@ class NotifyVisitToWebHooks private ClientInterface $httpClient, private EntityManagerInterface $em, private LoggerInterface $logger, - /** @var string[] */ - private array $webhooks, + private WebhookOptions $webhookOptions, private DataTransformerInterface $transformer, private AppOptions $appOptions, ) { @@ -35,7 +35,7 @@ class NotifyVisitToWebHooks public function __invoke(VisitLocated $shortUrlLocated): void { - if (empty($this->webhooks)) { + if (! $this->webhookOptions->hasWebhooks()) { return; } @@ -50,6 +50,10 @@ class NotifyVisitToWebHooks return; } + if ($visit->isOrphan() && ! $this->webhookOptions->notifyOrphanVisits()) { + return; + } + $requestOptions = $this->buildRequestOptions($visit); $requestPromises = $this->performRequests($requestOptions, $visitId); @@ -78,7 +82,7 @@ class NotifyVisitToWebHooks private function performRequests(array $requestOptions, string $visitId): array { return map( - $this->webhooks, + $this->webhookOptions->webhooks(), fn (string $webhook): PromiseInterface => $this->httpClient ->requestAsync(RequestMethodInterface::METHOD_POST, $webhook, $requestOptions) ->otherwise(fn (Throwable $e) => $this->logWebhookFailure($webhook, $visitId, $e)), diff --git a/module/Core/src/Options/WebhookOptions.php b/module/Core/src/Options/WebhookOptions.php new file mode 100644 index 00000000..c86789b2 --- /dev/null +++ b/module/Core/src/Options/WebhookOptions.php @@ -0,0 +1,40 @@ +visitsWebhooks; + } + + public function hasWebhooks(): bool + { + return ! empty($this->visitsWebhooks); + } + + protected function setVisitsWebhooks(array $visitsWebhooks): void + { + $this->visitsWebhooks = $visitsWebhooks; + } + + public function notifyOrphanVisits(): bool + { + return $this->notifyOrphanVisitsToWebhooks; + } + + protected function setNotifyOrphanVisitsToWebhooks(bool $notifyOrphanVisitsToWebhooks): void + { + $this->notifyOrphanVisitsToWebhooks = $notifyOrphanVisitsToWebhooks; + } +} diff --git a/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php index 9e884753..99609bb4 100644 --- a/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php +++ b/module/Core/test/EventDispatcher/NotifyVisitToWebHooksTest.php @@ -23,6 +23,7 @@ use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; use Shlinkio\Shlink\Core\EventDispatcher\NotifyVisitToWebHooks; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Options\AppOptions; +use Shlinkio\Shlink\Core\Options\WebhookOptions; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifier; use Shlinkio\Shlink\Core\ShortUrl\Transformer\ShortUrlDataTransformer; @@ -75,6 +76,24 @@ class NotifyVisitToWebHooksTest extends TestCase $requestAsync->shouldNotHaveBeenCalled(); } + /** @test */ + public function orphanVisitDoesNotPerformAnyRequestWhenDisabled(): void + { + $find = $this->em->find(Visit::class, '1')->willReturn(Visit::forBasePath(Visitor::emptyInstance())); + $requestAsync = $this->httpClient->requestAsync( + RequestMethodInterface::METHOD_POST, + Argument::type('string'), + Argument::type('array'), + )->willReturn(new FulfilledPromise('')); + $logWarning = $this->logger->warning(Argument::cetera()); + + $this->createListener(['foo', 'bar'], false)(new VisitLocated('1')); + + $find->shouldHaveBeenCalledOnce(); + $logWarning->shouldNotHaveBeenCalled(); + $requestAsync->shouldNotHaveBeenCalled(); + } + /** * @test * @dataProvider provideVisits @@ -136,13 +155,15 @@ class NotifyVisitToWebHooksTest extends TestCase yield 'orphan visit' => [Visit::forBasePath(Visitor::emptyInstance()), ['visit'],]; } - private function createListener(array $webhooks): NotifyVisitToWebHooks + private function createListener(array $webhooks, bool $notifyOrphanVisits = true): NotifyVisitToWebHooks { return new NotifyVisitToWebHooks( $this->httpClient->reveal(), $this->em->reveal(), $this->logger->reveal(), - $webhooks, + new WebhookOptions( + ['visits_webhooks' => $webhooks, 'notify_orphan_visits_to_webhooks' => $notifyOrphanVisits], + ), new ShortUrlDataTransformer(new ShortUrlStringifier([])), new AppOptions(['name' => 'Shlink', 'version' => '1.2.3']), ); From 483bdddb18583d5a0a93905817579f327ada3104 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 9 Oct 2021 12:35:45 +0200 Subject: [PATCH 3/4] Updated to installer version with support for orphan visits webhooks --- CHANGELOG.md | 3 +++ composer.json | 2 +- config/autoload/installer.global.php | 1 + 3 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eef5ab39..d17a5b8f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this When they are used in the query, the values are URL encoded. * [#1119](https://github.com/shlinkio/shlink/issues/1119) Added support to provide redis sentinel when using redis cache. +* [#1016](https://github.com/shlinkio/shlink/issues/1016) Added new option to send orphan visits to webhooks, via env var or installer tool. + + The option is disabled by default, as the payload is backwards incompatible. You will need to adapt your webhooks to treat the `shortUrl` property as optional before enabling this option. ### Changed * [#1142](https://github.com/shlinkio/shlink/issues/1142) Replaced `doctrine/cache` package with `symfony/cache`. diff --git a/composer.json b/composer.json index b00900e7..694cb399 100644 --- a/composer.json +++ b/composer.json @@ -50,7 +50,7 @@ "shlinkio/shlink-config": "^1.2", "shlinkio/shlink-event-dispatcher": "^2.1", "shlinkio/shlink-importer": "^2.3.1", - "shlinkio/shlink-installer": "dev-develop#2f87995 as 6.2", + "shlinkio/shlink-installer": "dev-develop#b45a340 as 6.2", "shlinkio/shlink-ip-geolocation": "^2.0", "symfony/console": "^5.3", "symfony/filesystem": "^5.3", diff --git a/config/autoload/installer.global.php b/config/autoload/installer.global.php index fe9f3f5d..e89b382d 100644 --- a/config/autoload/installer.global.php +++ b/config/autoload/installer.global.php @@ -24,6 +24,7 @@ return [ Option\UrlShortener\ShortDomainSchemaConfigOption::class, Option\UrlShortener\ValidateUrlConfigOption::class, Option\Visit\VisitsWebhooksConfigOption::class, + Option\Visit\OrphanVisitsWebhooksConfigOption::class, Option\Redirect\BaseUrlRedirectConfigOption::class, Option\Redirect\InvalidShortUrlRedirectConfigOption::class, Option\Redirect\Regular404RedirectConfigOption::class, From 14ba11e1ab4e4d73e655ec980f511d8fda606a94 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 9 Oct 2021 12:36:37 +0200 Subject: [PATCH 4/4] Enhanced changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d17a5b8f..c30406e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this When they are used in the query, the values are URL encoded. * [#1119](https://github.com/shlinkio/shlink/issues/1119) Added support to provide redis sentinel when using redis cache. -* [#1016](https://github.com/shlinkio/shlink/issues/1016) Added new option to send orphan visits to webhooks, via env var or installer tool. +* [#1016](https://github.com/shlinkio/shlink/issues/1016) Added new option to send orphan visits to webhooks, via `NOTIFY_ORPHAN_VISITS_TO_WEBHOOKS` env var or installer tool. The option is disabled by default, as the payload is backwards incompatible. You will need to adapt your webhooks to treat the `shortUrl` property as optional before enabling this option.