diff --git a/config/autoload/middleware-pipeline.global.php b/config/autoload/middleware-pipeline.global.php index 99f71bce..63d19d4d 100644 --- a/config/autoload/middleware-pipeline.global.php +++ b/config/autoload/middleware-pipeline.global.php @@ -11,6 +11,7 @@ use RKA\Middleware\IpAddress; use Shlinkio\Shlink\Common\Middleware\AccessLogMiddleware; use Shlinkio\Shlink\Common\Middleware\ContentLengthMiddleware; use Shlinkio\Shlink\Common\Middleware\RequestIdMiddleware; +use Shlinkio\Shlink\Core\Geolocation\Middleware\IpGeolocationMiddleware; return [ @@ -67,8 +68,11 @@ return [ ], 'not-found' => [ 'middleware' => [ - // This middleware is in front of tracking actions explicitly. Putting here for orphan visits tracking + // These two middlewares are in front of other tracking actions. + // Putting them here for orphan visits tracking IpAddress::class, + IpGeolocationMiddleware::class, + Core\ErrorHandler\NotFoundTypeResolverMiddleware::class, Core\ShortUrl\Middleware\ExtraPathRedirectMiddleware::class, Core\ErrorHandler\NotFoundTrackerMiddleware::class, diff --git a/config/autoload/routes.config.php b/config/autoload/routes.config.php index da3d1778..1f5425b5 100644 --- a/config/autoload/routes.config.php +++ b/config/autoload/routes.config.php @@ -8,6 +8,7 @@ use Fig\Http\Message\RequestMethodInterface; use RKA\Middleware\IpAddress; use Shlinkio\Shlink\Core\Action as CoreAction; use Shlinkio\Shlink\Core\Config\EnvVars; +use Shlinkio\Shlink\Core\Geolocation\Middleware\IpGeolocationMiddleware; use Shlinkio\Shlink\Core\ShortUrl\Middleware\TrimTrailingSlashMiddleware; use Shlinkio\Shlink\Rest\Action; use Shlinkio\Shlink\Rest\ConfigProvider; @@ -88,6 +89,7 @@ return (static function (): array { 'path' => '/{shortCode}/track', 'middleware' => [ IpAddress::class, + IpGeolocationMiddleware::class, CoreAction\PixelAction::class, ], 'allowed_methods' => [RequestMethodInterface::METHOD_GET], @@ -105,6 +107,7 @@ return (static function (): array { 'path' => sprintf('/{shortCode}%s', $shortUrlRouteSuffix), 'middleware' => [ IpAddress::class, + IpGeolocationMiddleware::class, TrimTrailingSlashMiddleware::class, CoreAction\RedirectAction::class, ], diff --git a/module/CLI/src/GeoLite/GeolocationDbUpdater.php b/module/CLI/src/GeoLite/GeolocationDbUpdater.php index 2abae05b..2a0fda3b 100644 --- a/module/CLI/src/GeoLite/GeolocationDbUpdater.php +++ b/module/CLI/src/GeoLite/GeolocationDbUpdater.php @@ -44,7 +44,7 @@ class GeolocationDbUpdater implements GeolocationDbUpdaterInterface callable|null $beforeDownload = null, callable|null $handleProgress = null, ): GeolocationResult { - if ($this->trackingOptions->disableTracking || $this->trackingOptions->disableIpTracking) { + if (! $this->trackingOptions->isGeolocationRelevant()) { return GeolocationResult::CHECK_SKIPPED; } diff --git a/module/CLI/test/Command/Domain/GetDomainVisitsCommandTest.php b/module/CLI/test/Command/Domain/GetDomainVisitsCommandTest.php index 6563abc0..e174a3b0 100644 --- a/module/CLI/test/Command/Domain/GetDomainVisitsCommandTest.php +++ b/module/CLI/test/Command/Domain/GetDomainVisitsCommandTest.php @@ -40,7 +40,7 @@ class GetDomainVisitsCommandTest extends TestCase public function outputIsProperlyGenerated(): void { $shortUrl = ShortUrl::createFake(); - $visit = Visit::forValidShortUrl($shortUrl, new Visitor('bar', 'foo', '', ''))->locate( + $visit = Visit::forValidShortUrl($shortUrl, Visitor::fromParams('bar', 'foo', ''))->locate( VisitLocation::fromGeolocation(new Location('', 'Spain', '', 'Madrid', 0, 0, '')), ); $domain = 's.test'; diff --git a/module/CLI/test/Command/ShortUrl/GetShortUrlVisitsCommandTest.php b/module/CLI/test/Command/ShortUrl/GetShortUrlVisitsCommandTest.php index ba6735ba..a1905e38 100644 --- a/module/CLI/test/Command/ShortUrl/GetShortUrlVisitsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/GetShortUrlVisitsCommandTest.php @@ -93,7 +93,7 @@ class GetShortUrlVisitsCommandTest extends TestCase #[Test] public function outputIsProperlyGenerated(): void { - $visit = Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('bar', 'foo', '', ''))->locate( + $visit = Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::fromParams('bar', 'foo', ''))->locate( VisitLocation::fromGeolocation(new Location('', 'Spain', '', 'Madrid', 0, 0, '')), ); $shortCode = 'abc123'; diff --git a/module/CLI/test/Command/Tag/GetTagVisitsCommandTest.php b/module/CLI/test/Command/Tag/GetTagVisitsCommandTest.php index 9b79f509..08ca2cd3 100644 --- a/module/CLI/test/Command/Tag/GetTagVisitsCommandTest.php +++ b/module/CLI/test/Command/Tag/GetTagVisitsCommandTest.php @@ -40,7 +40,7 @@ class GetTagVisitsCommandTest extends TestCase public function outputIsProperlyGenerated(): void { $shortUrl = ShortUrl::createFake(); - $visit = Visit::forValidShortUrl($shortUrl, new Visitor('bar', 'foo', '', ''))->locate( + $visit = Visit::forValidShortUrl($shortUrl, Visitor::fromParams('bar', 'foo', ''))->locate( VisitLocation::fromGeolocation(new Location('', 'Spain', '', 'Madrid', 0, 0, '')), ); $tag = 'abc123'; diff --git a/module/CLI/test/Command/Visit/GetNonOrphanVisitsCommandTest.php b/module/CLI/test/Command/Visit/GetNonOrphanVisitsCommandTest.php index 0462c2c0..4ebe780f 100644 --- a/module/CLI/test/Command/Visit/GetNonOrphanVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/GetNonOrphanVisitsCommandTest.php @@ -40,7 +40,7 @@ class GetNonOrphanVisitsCommandTest extends TestCase public function outputIsProperlyGenerated(): void { $shortUrl = ShortUrl::createFake(); - $visit = Visit::forValidShortUrl($shortUrl, new Visitor('bar', 'foo', '', ''))->locate( + $visit = Visit::forValidShortUrl($shortUrl, Visitor::fromParams('bar', 'foo', ''))->locate( VisitLocation::fromGeolocation(new Location('', 'Spain', '', 'Madrid', 0, 0, '')), ); $this->visitsHelper->expects($this->once())->method('nonOrphanVisits')->withAnyParameters()->willReturn( diff --git a/module/CLI/test/Command/Visit/GetOrphanVisitsCommandTest.php b/module/CLI/test/Command/Visit/GetOrphanVisitsCommandTest.php index 29914b61..33a98448 100644 --- a/module/CLI/test/Command/Visit/GetOrphanVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/GetOrphanVisitsCommandTest.php @@ -37,7 +37,7 @@ class GetOrphanVisitsCommandTest extends TestCase #[TestWith([['--type' => OrphanVisitType::BASE_URL->value], true])] public function outputIsProperlyGenerated(array $args, bool $includesType): void { - $visit = Visit::forBasePath(new Visitor('bar', 'foo', '', ''))->locate( + $visit = Visit::forBasePath(Visitor::fromParams('bar', 'foo', ''))->locate( VisitLocation::fromGeolocation(new Location('', 'Spain', '', 'Madrid', 0, 0, '')), ); $this->visitsHelper->expects($this->once())->method('orphanVisits')->with($this->callback( diff --git a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php index b17ca369..0f24a603 100644 --- a/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php +++ b/module/CLI/test/Command/Visit/LocateVisitsCommandTest.php @@ -63,8 +63,8 @@ class LocateVisitsCommandTest extends TestCase bool $expectWarningPrint, array $args, ): void { - $visit = Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')); - $location = VisitLocation::fromGeolocation(Location::emptyInstance()); + $visit = Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::fromParams('', '', '1.2.3.4')); + $location = VisitLocation::fromGeolocation(Location::empty()); $mockMethodBehavior = $this->invokeHelperMethods($visit, $location); $this->lock->method('acquire')->with($this->isFalse())->willReturn(true); @@ -134,7 +134,7 @@ class LocateVisitsCommandTest extends TestCase #[Test] public function errorWhileLocatingIpIsDisplayed(): void { - $visit = Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')); + $visit = Visit::forValidShortUrl(ShortUrl::createFake(), Visitor::fromParams(remoteAddress: '1.2.3.4')); $location = VisitLocation::fromGeolocation(Location::emptyInstance()); $this->lock->method('acquire')->with($this->isFalse())->willReturn(true); diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index ad3452e4..4844e6d5 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -11,6 +11,7 @@ use Shlinkio\Shlink\Common\Doctrine\EntityRepositoryFactory; use Shlinkio\Shlink\Core\Config\Options\NotFoundRedirectOptions; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifier; use Shlinkio\Shlink\Importer\ImportedLinksProcessorInterface; +use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdater; use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; use Symfony\Component\Lock; @@ -102,6 +103,8 @@ return [ EventDispatcher\PublishingUpdatesGenerator::class => ConfigAbstractFactory::class, + Geolocation\Middleware\IpGeolocationMiddleware::class => ConfigAbstractFactory::class, + Importer\ImportedLinksProcessor::class => ConfigAbstractFactory::class, Crawling\CrawlingHelper::class => ConfigAbstractFactory::class, @@ -237,6 +240,13 @@ return [ EventDispatcher\PublishingUpdatesGenerator::class => [ShortUrl\Transformer\ShortUrlDataTransformer::class], + Geolocation\Middleware\IpGeolocationMiddleware::class => [ + IpLocationResolverInterface::class, + DbUpdater::class, + 'Logger_Shlink', + Config\Options\TrackingOptions::class, + ], + Importer\ImportedLinksProcessor::class => [ 'em', ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class, diff --git a/module/Core/config/event_dispatcher.config.php b/module/Core/config/event_dispatcher.config.php index 2491d606..4e130fcf 100644 --- a/module/Core/config/event_dispatcher.config.php +++ b/module/Core/config/event_dispatcher.config.php @@ -15,23 +15,18 @@ use Shlinkio\Shlink\Core\Matomo\MatomoOptions; use Shlinkio\Shlink\Core\Visit\Geolocation\VisitLocator; use Shlinkio\Shlink\Core\Visit\Geolocation\VisitToLocationHelper; use Shlinkio\Shlink\EventDispatcher\Listener\EnabledListenerCheckerInterface; -use Shlinkio\Shlink\IpGeolocation\GeoLite2\DbUpdater; use Shlinkio\Shlink\IpGeolocation\GeoLite2\GeoLite2Options; -use Shlinkio\Shlink\IpGeolocation\Resolver\IpLocationResolverInterface; use function Shlinkio\Shlink\Config\runningInRoadRunner; return (static function (): array { $regularEvents = [ - EventDispatcher\Event\UrlVisited::class => [ - EventDispatcher\LocateVisit::class, - ], EventDispatcher\Event\GeoLiteDbCreated::class => [ EventDispatcher\LocateUnlocatedVisits::class, ], ]; $asyncEvents = [ - EventDispatcher\Event\VisitLocated::class => [ + EventDispatcher\Event\UrlVisited::class => [ EventDispatcher\Mercure\NotifyVisitToMercure::class, EventDispatcher\RabbitMq\NotifyVisitToRabbitMq::class, EventDispatcher\RedisPubSub\NotifyVisitToRedis::class, @@ -46,9 +41,9 @@ return (static function (): array { // Send visits to matomo asynchronously if the runtime allows it if (runningInRoadRunner()) { - $asyncEvents[EventDispatcher\Event\VisitLocated::class][] = EventDispatcher\Matomo\SendVisitToMatomo::class; + $asyncEvents[EventDispatcher\Event\UrlVisited::class][] = EventDispatcher\Matomo\SendVisitToMatomo::class; } else { - $regularEvents[EventDispatcher\Event\VisitLocated::class] = [EventDispatcher\Matomo\SendVisitToMatomo::class]; + $regularEvents[EventDispatcher\Event\UrlVisited::class] = [EventDispatcher\Matomo\SendVisitToMatomo::class]; } return [ @@ -60,7 +55,6 @@ return (static function (): array { 'dependencies' => [ 'factories' => [ - EventDispatcher\LocateVisit::class => ConfigAbstractFactory::class, EventDispatcher\Matomo\SendVisitToMatomo::class => ConfigAbstractFactory::class, EventDispatcher\LocateUnlocatedVisits::class => ConfigAbstractFactory::class, EventDispatcher\Mercure\NotifyVisitToMercure::class => ConfigAbstractFactory::class, @@ -104,13 +98,6 @@ return (static function (): array { ], ConfigAbstractFactory::class => [ - EventDispatcher\LocateVisit::class => [ - IpLocationResolverInterface::class, - 'em', - 'Logger_Shlink', - DbUpdater::class, - EventDispatcherInterface::class, - ], EventDispatcher\LocateUnlocatedVisits::class => [VisitLocator::class, VisitToLocationHelper::class], EventDispatcher\Mercure\NotifyVisitToMercure::class => [ MercureHubPublishingHelper::class, diff --git a/module/Core/functions/functions.php b/module/Core/functions/functions.php index cb8c0a8c..6ccc42e2 100644 --- a/module/Core/functions/functions.php +++ b/module/Core/functions/functions.php @@ -18,6 +18,7 @@ use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Common\Middleware\IpAddressMiddlewareFactory; use Shlinkio\Shlink\Common\Util\DateRange; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlMode; +use Shlinkio\Shlink\IpGeolocation\Model\Location; use function array_keys; use function array_map; @@ -289,3 +290,13 @@ function ipAddressFromRequest(ServerRequestInterface $request): string|null { return $request->getAttribute(IpAddressMiddlewareFactory::REQUEST_ATTR); } + +function geolocationFromRequest(ServerRequestInterface $request): Location|null +{ + $geolocation = $request->getAttribute(Location::class); + if ($geolocation !== null && ! $geolocation instanceof Location) { + // TODO Throw exception + } + + return $geolocation; +} diff --git a/module/Core/src/Action/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php index b7ddb69a..78eebc05 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -15,7 +15,6 @@ use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; -use Shlinkio\Shlink\IpGeolocation\Model\Location; abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMethodInterface { @@ -31,12 +30,9 @@ abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMet try { $shortUrl = $this->urlResolver->resolveEnabledShortUrl($identifier); - $visit = $this->requestTracker->trackIfApplicable($shortUrl, $request); + $this->requestTracker->trackIfApplicable($shortUrl, $request); - return $this->createSuccessResp( - $shortUrl, - $request->withAttribute(Location::class, $visit?->getVisitLocation()), - ); + return $this->createSuccessResp($shortUrl, $request); } catch (ShortUrlNotFoundException) { return $this->createErrorResp($request, $handler); } diff --git a/module/Core/src/Config/Options/TrackingOptions.php b/module/Core/src/Config/Options/TrackingOptions.php index d238bb42..754978f9 100644 --- a/module/Core/src/Config/Options/TrackingOptions.php +++ b/module/Core/src/Config/Options/TrackingOptions.php @@ -59,4 +59,12 @@ final readonly class TrackingOptions { return $this->disableTrackParam !== null && array_key_exists($this->disableTrackParam, $query); } + + /** + * If IP address tracking is disabled, or tracking is disabled all together, then geolocation is not relevant + */ + public function isGeolocationRelevant(): bool + { + return ! $this->disableTracking && ! $this->disableIpTracking; + } } diff --git a/module/Core/src/EventDispatcher/Async/AbstractNotifyVisitListener.php b/module/Core/src/EventDispatcher/Async/AbstractNotifyVisitListener.php index 3ec9417c..e871588f 100644 --- a/module/Core/src/EventDispatcher/Async/AbstractNotifyVisitListener.php +++ b/module/Core/src/EventDispatcher/Async/AbstractNotifyVisitListener.php @@ -8,7 +8,7 @@ use Doctrine\ORM\EntityManagerInterface; use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Common\UpdatePublishing\PublishingHelperInterface; use Shlinkio\Shlink\Common\UpdatePublishing\Update; -use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; +use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\EventDispatcher\PublishingUpdatesGeneratorInterface; use Shlinkio\Shlink\Core\Visit\Entity\Visit; use Throwable; @@ -25,7 +25,7 @@ abstract class AbstractNotifyVisitListener extends AbstractAsyncListener ) { } - public function __invoke(VisitLocated $visitLocated): void + public function __invoke(UrlVisited $visitLocated): void { if (! $this->isEnabled()) { return; diff --git a/module/Core/src/EventDispatcher/Event/AbstractVisitEvent.php b/module/Core/src/EventDispatcher/Event/AbstractVisitEvent.php deleted file mode 100644 index c1fa440a..00000000 --- a/module/Core/src/EventDispatcher/Event/AbstractVisitEvent.php +++ /dev/null @@ -1,27 +0,0 @@ - $this->visitId, 'originalIpAddress' => $this->originalIpAddress]; - } - - public static function fromPayload(array $payload): self - { - return new static($payload['visitId'] ?? '', $payload['originalIpAddress'] ?? null); - } -} diff --git a/module/Core/src/EventDispatcher/Event/ShortUrlCreated.php b/module/Core/src/EventDispatcher/Event/ShortUrlCreated.php index b6ab1a0c..4055935f 100644 --- a/module/Core/src/EventDispatcher/Event/ShortUrlCreated.php +++ b/module/Core/src/EventDispatcher/Event/ShortUrlCreated.php @@ -7,9 +7,9 @@ namespace Shlinkio\Shlink\Core\EventDispatcher\Event; use JsonSerializable; use Shlinkio\Shlink\EventDispatcher\Util\JsonUnserializable; -final class ShortUrlCreated implements JsonSerializable, JsonUnserializable +final readonly class ShortUrlCreated implements JsonSerializable, JsonUnserializable { - public function __construct(public readonly string $shortUrlId) + public function __construct(public string $shortUrlId) { } diff --git a/module/Core/src/EventDispatcher/Event/UrlVisited.php b/module/Core/src/EventDispatcher/Event/UrlVisited.php index d1158a4e..0d25b1a1 100644 --- a/module/Core/src/EventDispatcher/Event/UrlVisited.php +++ b/module/Core/src/EventDispatcher/Event/UrlVisited.php @@ -4,6 +4,24 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\EventDispatcher\Event; -final class UrlVisited extends AbstractVisitEvent +use JsonSerializable; +use Shlinkio\Shlink\EventDispatcher\Util\JsonUnserializable; + +final readonly class UrlVisited implements JsonSerializable, JsonUnserializable { + final public function __construct( + public string $visitId, + public string|null $originalIpAddress = null, + ) { + } + + public function jsonSerialize(): array + { + return ['visitId' => $this->visitId, 'originalIpAddress' => $this->originalIpAddress]; + } + + public static function fromPayload(array $payload): self + { + return new self($payload['visitId'] ?? '', $payload['originalIpAddress'] ?? null); + } } diff --git a/module/Core/src/EventDispatcher/Event/VisitLocated.php b/module/Core/src/EventDispatcher/Event/VisitLocated.php deleted file mode 100644 index 99b7a05e..00000000 --- a/module/Core/src/EventDispatcher/Event/VisitLocated.php +++ /dev/null @@ -1,9 +0,0 @@ -visitId; - - /** @var Visit|null $visit */ - $visit = $this->em->find(Visit::class, $visitId); - if ($visit === null) { - $this->logger->warning('Tried to locate visit with id "{visitId}", but it does not exist.', [ - 'visitId' => $visitId, - ]); - return; - } - - $this->locateVisit($visitId, $shortUrlVisited->originalIpAddress, $visit); - $this->eventDispatcher->dispatch(new VisitLocated($visitId, $shortUrlVisited->originalIpAddress)); - } - - private function locateVisit(string $visitId, string|null $originalIpAddress, Visit $visit): void - { - if (! $this->dbUpdater->databaseFileExists()) { - $this->logger->warning('Tried to locate visit with id "{visitId}", but a GeoLite2 db was not found.', [ - 'visitId' => $visitId, - ]); - return; - } - - $isLocatable = $originalIpAddress !== null || $visit->isLocatable(); - $addr = $originalIpAddress ?? $visit->remoteAddr ?? ''; - - try { - $location = $isLocatable ? $this->ipLocationResolver->resolveIpLocation($addr) : Location::emptyInstance(); - - $visit->locate(VisitLocation::fromGeolocation($location)); - $this->em->flush(); - } catch (WrongIpException $e) { - $this->logger->warning( - 'Tried to locate visit with id "{visitId}", but its address seems to be wrong. {e}', - ['e' => $e, 'visitId' => $visitId], - ); - } catch (Throwable $e) { - $this->logger->error( - 'An unexpected error occurred while trying to locate visit with id "{visitId}". {e}', - ['e' => $e, 'visitId' => $visitId], - ); - } - } -} diff --git a/module/Core/src/EventDispatcher/Matomo/SendVisitToMatomo.php b/module/Core/src/EventDispatcher/Matomo/SendVisitToMatomo.php index 5a85aed4..c7b5bd3c 100644 --- a/module/Core/src/EventDispatcher/Matomo/SendVisitToMatomo.php +++ b/module/Core/src/EventDispatcher/Matomo/SendVisitToMatomo.php @@ -6,7 +6,7 @@ namespace Shlinkio\Shlink\Core\EventDispatcher\Matomo; use Doctrine\ORM\EntityManagerInterface; use Psr\Log\LoggerInterface; -use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; +use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\Matomo\MatomoOptions; use Shlinkio\Shlink\Core\Matomo\MatomoVisitSenderInterface; use Shlinkio\Shlink\Core\Visit\Entity\Visit; @@ -22,7 +22,7 @@ readonly class SendVisitToMatomo ) { } - public function __invoke(VisitLocated $visitLocated): void + public function __invoke(UrlVisited $visitLocated): void { if (! $this->matomoOptions->enabled) { return; diff --git a/module/Core/src/Geolocation/Middleware/IpGeolocationMiddleware.php b/module/Core/src/Geolocation/Middleware/IpGeolocationMiddleware.php new file mode 100644 index 00000000..4e2e533b --- /dev/null +++ b/module/Core/src/Geolocation/Middleware/IpGeolocationMiddleware.php @@ -0,0 +1,58 @@ +trackingOptions->isGeolocationRelevant()) { + return $handler->handle($request); + } + + if (! $this->dbUpdater->databaseFileExists()) { + $this->logger->warning('Tried to geolocate IP address, but a GeoLite2 db was not found.'); + return $handler->handle($request); + } + + $location = $this->geolocateIpAddress(ipAddressFromRequest($request)); + return $handler->handle($request->withAttribute(Location::class, $location)); + } + + private function geolocateIpAddress(string|null $ipAddress): Location + { + try { + return $ipAddress === null ? Location::empty() : $this->ipLocationResolver->resolveIpLocation($ipAddress); + } catch (WrongIpException $e) { + $this->logger->warning('Tried to locate IP address, but it seems to be wrong. {e}', ['e' => $e]); + return Location::empty(); + } catch (Throwable $e) { + $this->logger->error('An unexpected error occurred while trying to locate IP address. {e}', ['e' => $e]); + return Location::empty(); + } + } +} diff --git a/module/Core/src/RedirectRule/Entity/RedirectCondition.php b/module/Core/src/RedirectRule/Entity/RedirectCondition.php index 9468d582..cf1e134b 100644 --- a/module/Core/src/RedirectRule/Entity/RedirectCondition.php +++ b/module/Core/src/RedirectRule/Entity/RedirectCondition.php @@ -9,11 +9,10 @@ use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\RedirectRule\Model\RedirectConditionType; use Shlinkio\Shlink\Core\RedirectRule\Model\Validation\RedirectRulesInputFilter; use Shlinkio\Shlink\Core\Util\IpAddressUtils; -use Shlinkio\Shlink\Core\Visit\Entity\VisitLocation; -use Shlinkio\Shlink\IpGeolocation\Model\Location; use function Shlinkio\Shlink\Core\acceptLanguageToLocales; use function Shlinkio\Shlink\Core\ArrayUtils\some; +use function Shlinkio\Shlink\Core\geolocationFromRequest; use function Shlinkio\Shlink\Core\ipAddressFromRequest; use function Shlinkio\Shlink\Core\normalizeLocale; use function Shlinkio\Shlink\Core\splitLocale; @@ -134,9 +133,8 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable private function matchesGeolocationCountryCode(ServerRequestInterface $request): bool { - $geolocation = $request->getAttribute(Location::class); - // TODO We should eventually rely on `Location` type only - if (! ($geolocation instanceof Location) && ! ($geolocation instanceof VisitLocation)) { + $geolocation = geolocationFromRequest($request); + if ($geolocation === null) { return false; } @@ -145,14 +143,12 @@ class RedirectCondition extends AbstractEntity implements JsonSerializable private function matchesGeolocationCityName(ServerRequestInterface $request): bool { - $geolocation = $request->getAttribute(Location::class); - // TODO We should eventually rely on `Location` type only - if (! ($geolocation instanceof Location) && ! ($geolocation instanceof VisitLocation)) { + $geolocation = geolocationFromRequest($request); + if ($geolocation === null) { return false; } - $cityName = $geolocation instanceof Location ? $geolocation->city : $geolocation->cityName; - return strcasecmp($cityName, $this->matchValue) === 0; + return strcasecmp($geolocation->city, $this->matchValue) === 0; } public function jsonSerialize(): array diff --git a/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php index 2b4ac7cc..4a02f6e9 100644 --- a/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php +++ b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php @@ -17,7 +17,6 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; -use Shlinkio\Shlink\IpGeolocation\Model\Location; use function array_slice; use function count; @@ -74,13 +73,9 @@ readonly class ExtraPathRedirectMiddleware implements MiddlewareInterface try { $shortUrl = $this->resolver->resolveEnabledShortUrl($identifier); - $visit = $this->requestTracker->trackIfApplicable($shortUrl, $request); + $this->requestTracker->trackIfApplicable($shortUrl, $request); - $longUrl = $this->redirectionBuilder->buildShortUrlRedirect( - $shortUrl, - $request->withAttribute(Location::class, $visit?->getVisitLocation()), - $extraPath, - ); + $longUrl = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $request, $extraPath); return $this->redirectResponseHelper->buildRedirectResponse($longUrl); } catch (ShortUrlNotFoundException) { if ($extraPath === null || ! $this->urlShortenerOptions->multiSegmentSlugsEnabled) { diff --git a/module/Core/src/Visit/Entity/Visit.php b/module/Core/src/Visit/Entity/Visit.php index d02d7298..e26d5f80 100644 --- a/module/Core/src/Visit/Entity/Visit.php +++ b/module/Core/src/Visit/Entity/Visit.php @@ -59,14 +59,16 @@ class Visit extends AbstractEntity implements JsonSerializable Visitor $visitor, bool $anonymize, ): self { + $geolocation = $visitor->geolocation; return new self( shortUrl: $shortUrl, type: $type, userAgent: $visitor->userAgent, referer: $visitor->referer, - potentialBot: $visitor->isPotentialBot(), + potentialBot: $visitor->potentialBot, remoteAddr: self::processAddress($visitor->remoteAddress, $anonymize), visitedUrl: $visitor->visitedUrl, + visitLocation: $geolocation !== null ? VisitLocation::fromGeolocation($geolocation) : null, ); } diff --git a/module/Core/src/Visit/Model/Visitor.php b/module/Core/src/Visit/Model/Visitor.php index 493280ef..e13712e1 100644 --- a/module/Core/src/Visit/Model/Visitor.php +++ b/module/Core/src/Visit/Model/Visitor.php @@ -6,78 +6,85 @@ namespace Shlinkio\Shlink\Core\Visit\Model; use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Core\Config\Options\TrackingOptions; +use Shlinkio\Shlink\IpGeolocation\Model\Location; +use function Shlinkio\Shlink\Core\geolocationFromRequest; use function Shlinkio\Shlink\Core\ipAddressFromRequest; use function Shlinkio\Shlink\Core\isCrawler; use function substr; -final class Visitor +final readonly class Visitor { public const USER_AGENT_MAX_LENGTH = 512; public const REFERER_MAX_LENGTH = 1024; public const REMOTE_ADDRESS_MAX_LENGTH = 256; public const VISITED_URL_MAX_LENGTH = 2048; - public readonly string $userAgent; - public readonly string $referer; - public readonly string $visitedUrl; - public readonly string|null $remoteAddress; - private bool $potentialBot; - - public function __construct(string $userAgent, string $referer, string|null $remoteAddress, string $visitedUrl) - { - $this->userAgent = $this->cropToLength($userAgent, self::USER_AGENT_MAX_LENGTH); - $this->referer = $this->cropToLength($referer, self::REFERER_MAX_LENGTH); - $this->visitedUrl = $this->cropToLength($visitedUrl, self::VISITED_URL_MAX_LENGTH); - $this->remoteAddress = $remoteAddress === null ? null : $this->cropToLength( - $remoteAddress, - self::REMOTE_ADDRESS_MAX_LENGTH, - ); - $this->potentialBot = isCrawler($userAgent); + private function __construct( + public string $userAgent, + public string $referer, + public string|null $remoteAddress, + public string $visitedUrl, + public bool $potentialBot, + public Location|null $geolocation, + ) { } - private function cropToLength(string $value, int $length): string + public static function fromParams( + string $userAgent = '', + string $referer = '', + string|null $remoteAddress = null, + string $visitedUrl = '', + Location|null $geolocation = null, + ): self { + return new self( + userAgent: self::cropToLength($userAgent, self::USER_AGENT_MAX_LENGTH), + referer: self::cropToLength($referer, self::REFERER_MAX_LENGTH), + remoteAddress: $remoteAddress === null + ? null + : self::cropToLength($remoteAddress, self::REMOTE_ADDRESS_MAX_LENGTH), + visitedUrl: self::cropToLength($visitedUrl, self::VISITED_URL_MAX_LENGTH), + potentialBot: isCrawler($userAgent), + geolocation: $geolocation, + ); + } + + private static function cropToLength(string $value, int $length): string { return substr($value, 0, $length); } public static function fromRequest(ServerRequestInterface $request): self { - return new self( - $request->getHeaderLine('User-Agent'), - $request->getHeaderLine('Referer'), - ipAddressFromRequest($request), - $request->getUri()->__toString(), + return self::fromParams( + userAgent: $request->getHeaderLine('User-Agent'), + referer: $request->getHeaderLine('Referer'), + remoteAddress: ipAddressFromRequest($request), + visitedUrl: $request->getUri()->__toString(), + geolocation: geolocationFromRequest($request), ); } public static function empty(): self { - return new self('', '', null, ''); + return self::fromParams(); } public static function botInstance(): self { - return new self('cf-facebook', '', null, ''); - } - - public function isPotentialBot(): bool - { - return $this->potentialBot; + return self::fromParams(userAgent: 'cf-facebook'); } public function normalizeForTrackingOptions(TrackingOptions $options): self { - $instance = new self( - $options->disableUaTracking ? '' : $this->userAgent, - $options->disableReferrerTracking ? '' : $this->referer, - $options->disableIpTracking ? null : $this->remoteAddress, - $this->visitedUrl, + return new self( + userAgent: $options->disableUaTracking ? '' : $this->userAgent, + referer: $options->disableReferrerTracking ? '' : $this->referer, + remoteAddress: $options->disableIpTracking ? null : $this->remoteAddress, + visitedUrl: $this->visitedUrl, + // Keep the fact that the visit was a potential bot, even if we no longer save the user agent + potentialBot: $this->potentialBot, + geolocation: $this->geolocation, ); - - // Keep the fact that the visit was a potential bot, even if we no longer save the user agent - $instance->potentialBot = $this->potentialBot; - - return $instance; } } diff --git a/module/Core/test/EventDispatcher/LocateVisitTest.php b/module/Core/test/EventDispatcher/LocateVisitTest.php deleted file mode 100644 index 80e3e318..00000000 --- a/module/Core/test/EventDispatcher/LocateVisitTest.php +++ /dev/null @@ -1,195 +0,0 @@ -ipLocationResolver = $this->createMock(IpLocationResolverInterface::class); - $this->em = $this->createMock(EntityManagerInterface::class); - $this->logger = $this->createMock(LoggerInterface::class); - $this->eventDispatcher = $this->createMock(EventDispatcherInterface::class); - $this->dbUpdater = $this->createMock(DbUpdaterInterface::class); - - $this->locateVisit = new LocateVisit( - $this->ipLocationResolver, - $this->em, - $this->logger, - $this->dbUpdater, - $this->eventDispatcher, - ); - } - - #[Test] - public function invalidVisitLogsWarning(): void - { - $event = new UrlVisited('123'); - $this->em->expects($this->once())->method('find')->with(Visit::class, '123')->willReturn(null); - $this->em->expects($this->never())->method('flush'); - $this->logger->expects($this->once())->method('warning')->with( - 'Tried to locate visit with id "{visitId}", but it does not exist.', - ['visitId' => 123], - ); - $this->eventDispatcher->expects($this->never())->method('dispatch')->with(new VisitLocated('123')); - $this->ipLocationResolver->expects($this->never())->method('resolveIpLocation'); - - ($this->locateVisit)($event); - } - - #[Test] - public function nonExistingGeoLiteDbLogsWarning(): void - { - $event = new UrlVisited('123'); - $this->em->expects($this->once())->method('find')->with(Visit::class, '123')->willReturn( - Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')), - ); - $this->em->expects($this->never())->method('flush'); - $this->dbUpdater->expects($this->once())->method('databaseFileExists')->withAnyParameters()->willReturn(false); - $this->logger->expects($this->once())->method('warning')->with( - 'Tried to locate visit with id "{visitId}", but a GeoLite2 db was not found.', - ['visitId' => 123], - ); - $this->eventDispatcher->expects($this->once())->method('dispatch')->with(new VisitLocated('123')); - $this->ipLocationResolver->expects($this->never())->method('resolveIpLocation'); - - ($this->locateVisit)($event); - } - - #[Test] - public function invalidAddressLogsWarning(): void - { - $event = new UrlVisited('123'); - $this->em->expects($this->once())->method('find')->with(Visit::class, '123')->willReturn( - Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')), - ); - $this->em->expects($this->never())->method('flush'); - $this->dbUpdater->expects($this->once())->method('databaseFileExists')->withAnyParameters()->willReturn(true); - $this->ipLocationResolver->expects( - $this->once(), - )->method('resolveIpLocation')->withAnyParameters()->willThrowException(WrongIpException::fromIpAddress('')); - $this->logger->expects($this->once())->method('warning')->with( - 'Tried to locate visit with id "{visitId}", but its address seems to be wrong. {e}', - $this->isType('array'), - ); - $this->eventDispatcher->expects($this->once())->method('dispatch')->with(new VisitLocated('123')); - - ($this->locateVisit)($event); - } - - #[Test] - public function unhandledExceptionLogsError(): void - { - $event = new UrlVisited('123'); - $this->em->expects($this->once())->method('find')->with(Visit::class, '123')->willReturn( - Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')), - ); - $this->em->expects($this->never())->method('flush'); - $this->dbUpdater->expects($this->once())->method('databaseFileExists')->withAnyParameters()->willReturn(true); - $this->ipLocationResolver->expects( - $this->once(), - )->method('resolveIpLocation')->withAnyParameters()->willThrowException(new OutOfRangeException()); - $this->logger->expects($this->once())->method('error')->with( - 'An unexpected error occurred while trying to locate visit with id "{visitId}". {e}', - $this->isType('array'), - ); - $this->eventDispatcher->expects($this->once())->method('dispatch')->with(new VisitLocated('123')); - - ($this->locateVisit)($event); - } - - #[Test, DataProvider('provideNonLocatableVisits')] - public function nonLocatableVisitsResolveToEmptyLocations(Visit $visit): void - { - $event = new UrlVisited('123'); - $this->em->expects($this->once())->method('find')->with(Visit::class, '123')->willReturn($visit); - $this->em->expects($this->once())->method('flush'); - $this->dbUpdater->expects($this->once())->method('databaseFileExists')->withAnyParameters()->willReturn(true); - $this->ipLocationResolver->expects($this->never())->method('resolveIpLocation'); - - $this->eventDispatcher->expects($this->once())->method('dispatch')->with(new VisitLocated('123')); - $this->logger->expects($this->never())->method('warning'); - - ($this->locateVisit)($event); - - self::assertEquals($visit->getVisitLocation(), VisitLocation::fromGeolocation(Location::emptyInstance())); - } - - public static function provideNonLocatableVisits(): iterable - { - $shortUrl = ShortUrl::createFake(); - - yield 'null IP' => [Visit::forValidShortUrl($shortUrl, new Visitor('', '', null, ''))]; - yield 'empty IP' => [Visit::forValidShortUrl($shortUrl, new Visitor('', '', '', ''))]; - yield 'localhost' => [Visit::forValidShortUrl($shortUrl, new Visitor('', '', IpAddress::LOCALHOST, ''))]; - } - - #[Test, DataProvider('provideIpAddresses')] - public function locatableVisitsResolveToLocation(Visit $visit, string|null $originalIpAddress): void - { - $ipAddr = $originalIpAddress ?? $visit->remoteAddr; - $location = new Location('', '', '', '', 0.0, 0.0, ''); - $event = new UrlVisited('123', $originalIpAddress); - - $this->em->expects($this->once())->method('find')->with(Visit::class, '123')->willReturn($visit); - $this->em->expects($this->once())->method('flush'); - $this->dbUpdater->expects($this->once())->method('databaseFileExists')->withAnyParameters()->willReturn(true); - $this->ipLocationResolver->expects($this->once())->method('resolveIpLocation')->with($ipAddr)->willReturn( - $location, - ); - - $this->eventDispatcher->expects($this->once())->method('dispatch')->with( - new VisitLocated('123', $originalIpAddress), - ); - $this->logger->expects($this->never())->method('warning'); - - ($this->locateVisit)($event); - - self::assertEquals($visit->getVisitLocation(), VisitLocation::fromGeolocation($location)); - } - - public static function provideIpAddresses(): iterable - { - yield 'no original IP address' => [ - Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')), - null, - ]; - yield 'original IP address' => [ - Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor('', '', '1.2.3.4', '')), - '1.2.3.4', - ]; - yield 'base url' => [Visit::forBasePath(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4']; - yield 'invalid short url' => [Visit::forInvalidShortUrl(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4']; - yield 'regular not found' => [Visit::forRegularNotFound(new Visitor('', '', '1.2.3.4', '')), '1.2.3.4']; - } -} diff --git a/module/Core/test/EventDispatcher/Matomo/SendVisitToMatomoTest.php b/module/Core/test/EventDispatcher/Matomo/SendVisitToMatomoTest.php index 725980a1..8a4c1b7d 100644 --- a/module/Core/test/EventDispatcher/Matomo/SendVisitToMatomoTest.php +++ b/module/Core/test/EventDispatcher/Matomo/SendVisitToMatomoTest.php @@ -11,7 +11,7 @@ use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Log\LoggerInterface; -use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; +use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\EventDispatcher\Matomo\SendVisitToMatomo; use Shlinkio\Shlink\Core\Matomo\MatomoOptions; use Shlinkio\Shlink\Core\Matomo\MatomoVisitSenderInterface; @@ -39,7 +39,7 @@ class SendVisitToMatomoTest extends TestCase $this->logger->expects($this->never())->method('error'); $this->logger->expects($this->never())->method('warning'); - ($this->listener(enabled: false))(new VisitLocated('123')); + ($this->listener(enabled: false))(new UrlVisited('123')); } #[Test] @@ -53,7 +53,7 @@ class SendVisitToMatomoTest extends TestCase ['visitId' => '123'], ); - ($this->listener())(new VisitLocated('123')); + ($this->listener())(new UrlVisited('123')); } #[Test, DataProvider('provideOriginalIpAddress')] @@ -67,7 +67,7 @@ class SendVisitToMatomoTest extends TestCase $this->logger->expects($this->never())->method('error'); $this->logger->expects($this->never())->method('warning'); - ($this->listener())(new VisitLocated($visitId, $originalIpAddress)); + ($this->listener())(new UrlVisited($visitId, $originalIpAddress)); } public static function provideOriginalIpAddress(): iterable @@ -92,7 +92,7 @@ class SendVisitToMatomoTest extends TestCase ['e' => $e], ); - ($this->listener())(new VisitLocated($visitId)); + ($this->listener())(new UrlVisited($visitId)); } private function listener(bool $enabled = true): SendVisitToMatomo diff --git a/module/Core/test/EventDispatcher/Mercure/NotifyVisitToMercureTest.php b/module/Core/test/EventDispatcher/Mercure/NotifyVisitToMercureTest.php index 1e3dfb96..91569c9b 100644 --- a/module/Core/test/EventDispatcher/Mercure/NotifyVisitToMercureTest.php +++ b/module/Core/test/EventDispatcher/Mercure/NotifyVisitToMercureTest.php @@ -13,7 +13,7 @@ use Psr\Log\LoggerInterface; use RuntimeException; use Shlinkio\Shlink\Common\UpdatePublishing\PublishingHelperInterface; use Shlinkio\Shlink\Common\UpdatePublishing\Update; -use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; +use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\EventDispatcher\Mercure\NotifyVisitToMercure; use Shlinkio\Shlink\Core\EventDispatcher\PublishingUpdatesGeneratorInterface; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; @@ -54,7 +54,7 @@ class NotifyVisitToMercureTest extends TestCase $this->updatesGenerator->expects($this->never())->method('newVisitUpdate'); $this->helper->expects($this->never())->method('publishUpdate'); - ($this->listener)(new VisitLocated($visitId)); + ($this->listener)(new UrlVisited($visitId)); } #[Test] @@ -74,7 +74,7 @@ class NotifyVisitToMercureTest extends TestCase $this->updatesGenerator->expects($this->once())->method('newVisitUpdate')->with($visit)->willReturn($update); $this->helper->expects($this->exactly(2))->method('publishUpdate')->with($update); - ($this->listener)(new VisitLocated($visitId)); + ($this->listener)(new UrlVisited($visitId)); } #[Test] @@ -98,7 +98,7 @@ class NotifyVisitToMercureTest extends TestCase $this->updatesGenerator->expects($this->once())->method('newVisitUpdate')->with($visit)->willReturn($update); $this->helper->expects($this->once())->method('publishUpdate')->with($update)->willThrowException($e); - ($this->listener)(new VisitLocated($visitId)); + ($this->listener)(new UrlVisited($visitId)); } #[Test, DataProvider('provideOrphanVisits')] @@ -117,7 +117,7 @@ class NotifyVisitToMercureTest extends TestCase $this->updatesGenerator->expects($this->never())->method('newVisitUpdate'); $this->helper->expects($this->once())->method('publishUpdate')->with($update); - ($this->listener)(new VisitLocated($visitId)); + ($this->listener)(new UrlVisited($visitId)); } public static function provideOrphanVisits(): iterable diff --git a/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php b/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php index 26785897..a3cebaa7 100644 --- a/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php +++ b/module/Core/test/EventDispatcher/RabbitMq/NotifyVisitToRabbitMqTest.php @@ -17,7 +17,7 @@ use RuntimeException; use Shlinkio\Shlink\Common\UpdatePublishing\PublishingHelperInterface; use Shlinkio\Shlink\Common\UpdatePublishing\Update; use Shlinkio\Shlink\Core\Config\Options\RabbitMqOptions; -use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; +use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\EventDispatcher\PublishingUpdatesGeneratorInterface; use Shlinkio\Shlink\Core\EventDispatcher\RabbitMq\NotifyVisitToRabbitMq; use Shlinkio\Shlink\Core\ShortUrl\Entity\ShortUrl; @@ -52,7 +52,7 @@ class NotifyVisitToRabbitMqTest extends TestCase $this->logger->expects($this->never())->method('warning'); $this->logger->expects($this->never())->method('debug'); - ($this->listener(new RabbitMqOptions(enabled: false)))(new VisitLocated('123')); + ($this->listener(new RabbitMqOptions(enabled: false)))(new UrlVisited('123')); } #[Test] @@ -67,7 +67,7 @@ class NotifyVisitToRabbitMqTest extends TestCase $this->logger->expects($this->never())->method('debug'); $this->helper->expects($this->never())->method('publishUpdate'); - ($this->listener())(new VisitLocated($visitId)); + ($this->listener())(new UrlVisited($visitId)); } #[Test, DataProvider('provideVisits')] @@ -85,7 +85,7 @@ class NotifyVisitToRabbitMqTest extends TestCase ); $this->logger->expects($this->never())->method('debug'); - ($this->listener())(new VisitLocated($visitId)); + ($this->listener())(new UrlVisited($visitId)); } public static function provideVisits(): iterable @@ -121,7 +121,7 @@ class NotifyVisitToRabbitMqTest extends TestCase ['e' => $e, 'name' => 'RabbitMQ'], ); - ($this->listener())(new VisitLocated($visitId)); + ($this->listener())(new UrlVisited($visitId)); } public static function provideExceptions(): iterable @@ -142,7 +142,7 @@ class NotifyVisitToRabbitMqTest extends TestCase $setup($this->updatesGenerator); $expect($this->helper, $this->updatesGenerator); - ($this->listener())(new VisitLocated($visitId)); + ($this->listener())(new UrlVisited($visitId)); } public static function providePayloads(): iterable diff --git a/module/Core/test/EventDispatcher/RedisPubSub/NotifyVisitToRedisTest.php b/module/Core/test/EventDispatcher/RedisPubSub/NotifyVisitToRedisTest.php index 20cd786a..7cbf68b6 100644 --- a/module/Core/test/EventDispatcher/RedisPubSub/NotifyVisitToRedisTest.php +++ b/module/Core/test/EventDispatcher/RedisPubSub/NotifyVisitToRedisTest.php @@ -15,7 +15,7 @@ use Psr\Log\LoggerInterface; use RuntimeException; use Shlinkio\Shlink\Common\UpdatePublishing\PublishingHelperInterface; use Shlinkio\Shlink\Common\UpdatePublishing\Update; -use Shlinkio\Shlink\Core\EventDispatcher\Event\VisitLocated; +use Shlinkio\Shlink\Core\EventDispatcher\Event\UrlVisited; use Shlinkio\Shlink\Core\EventDispatcher\PublishingUpdatesGeneratorInterface; use Shlinkio\Shlink\Core\EventDispatcher\RedisPubSub\NotifyVisitToRedis; use Shlinkio\Shlink\Core\Visit\Entity\Visit; @@ -45,7 +45,7 @@ class NotifyVisitToRedisTest extends TestCase $this->logger->expects($this->never())->method('warning'); $this->logger->expects($this->never())->method('debug'); - $this->createListener(false)(new VisitLocated('123')); + $this->createListener(false)(new UrlVisited('123')); } #[Test, DataProvider('provideExceptions')] @@ -64,7 +64,7 @@ class NotifyVisitToRedisTest extends TestCase ['e' => $e, 'name' => 'Redis pub/sub'], ); - $this->createListener()(new VisitLocated($visitId)); + $this->createListener()(new UrlVisited($visitId)); } public static function provideExceptions(): iterable diff --git a/module/Core/test/Geolocation/Middleware/IpGeolocationMiddlewareTest.php b/module/Core/test/Geolocation/Middleware/IpGeolocationMiddlewareTest.php new file mode 100644 index 00000000..f203fb85 --- /dev/null +++ b/module/Core/test/Geolocation/Middleware/IpGeolocationMiddlewareTest.php @@ -0,0 +1,172 @@ +ipLocationResolver = $this->createMock(IpLocationResolverInterface::class); + $this->dbUpdater = $this->createMock(DbUpdaterInterface::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->handler = $this->createMock(RequestHandlerInterface::class); + } + + #[Test] + public function geolocationIsSkippedIfTrackingIsDisabled(): void + { + $this->dbUpdater->expects($this->never())->method('databaseFileExists'); + $this->ipLocationResolver->expects($this->never())->method('resolveIpLocation'); + $this->logger->expects($this->never())->method('warning'); + + $request = ServerRequestFactory::fromGlobals(); + $this->handler->expects($this->once())->method('handle')->with($request); + + $this->middleware(disableTracking: true)->process($request, $this->handler); + } + + #[Test] + public function warningIsLoggedIfGeoLiteDbDoesNotExist(): void + { + $this->ipLocationResolver->expects($this->never())->method('resolveIpLocation'); + $this->dbUpdater->expects($this->once())->method('databaseFileExists')->willReturn(false); + $this->logger->expects($this->once())->method('warning')->with( + 'Tried to geolocate IP address, but a GeoLite2 db was not found.', + ); + + $request = ServerRequestFactory::fromGlobals(); + $this->handler->expects($this->once())->method('handle')->with($request); + + $this->middleware()->process($request, $this->handler); + } + + #[Test] + public function emptyLocationIsReturnedIfIpAddressDoesNotExistInRequest(): void + { + $this->dbUpdater->expects($this->once())->method('databaseFileExists')->willReturn(true); + $this->ipLocationResolver->expects($this->never())->method('resolveIpLocation'); + $this->logger->expects($this->never())->method('warning'); + + $request = ServerRequestFactory::fromGlobals(); + $this->handler->expects($this->once())->method('handle')->with($this->callback( + function (ServerRequestInterface $req): bool { + $location = $req->getAttribute(Location::class); + if (! $location instanceof Location) { + return false; + } + + Assert::assertEmpty($location->countryCode); + return true; + }, + )); + + $this->middleware()->process($request, $this->handler); + } + + #[Test] + public function locationIsResolvedFromIpAddress(): void + { + $this->dbUpdater->expects($this->once())->method('databaseFileExists')->willReturn(true); + $this->ipLocationResolver->expects($this->once())->method('resolveIpLocation')->with('1.2.3.4')->willReturn( + new Location(countryCode: 'ES'), + ); + $this->logger->expects($this->never())->method('warning'); + + $request = ServerRequestFactory::fromGlobals()->withAttribute( + IpAddressMiddlewareFactory::REQUEST_ATTR, + '1.2.3.4', + ); + $this->handler->expects($this->once())->method('handle')->with($this->callback( + function (ServerRequestInterface $req): bool { + $location = $req->getAttribute(Location::class); + if (! $location instanceof Location) { + return false; + } + + Assert::assertEquals('ES', $location->countryCode); + return true; + }, + )); + + $this->middleware()->process($request, $this->handler); + } + + #[Test] + #[TestWith([ + new WrongIpException(), + 'warning', + 'Tried to locate IP address, but it seems to be wrong. {e}', + ])] + #[TestWith([ + new RuntimeException('Unknown'), + 'error', + 'An unexpected error occurred while trying to locate IP address. {e}', + ])] + public function warningIsPrintedIfAnErrorOccurs( + Throwable $exception, + string $loggerMethod, + string $expectedLoggedMessage, + ): void { + $this->dbUpdater->expects($this->once())->method('databaseFileExists')->willReturn(true); + $this->ipLocationResolver + ->expects($this->once()) + ->method('resolveIpLocation') + ->with('1.2.3.4') + ->willThrowException($exception); + $this->logger->expects($this->once())->method($loggerMethod)->with($expectedLoggedMessage, ['e' => $exception]); + + $request = ServerRequestFactory::fromGlobals()->withAttribute( + IpAddressMiddlewareFactory::REQUEST_ATTR, + '1.2.3.4', + ); + $this->handler->expects($this->once())->method('handle')->with($this->callback( + function (ServerRequestInterface $req): bool { + $location = $req->getAttribute(Location::class); + if (! $location instanceof Location) { + return false; + } + + Assert::assertEmpty($location->countryCode); + return true; + }, + )); + + $this->middleware()->process($request, $this->handler); + } + + private function middleware(bool $disableTracking = false): IpGeolocationMiddleware + { + return new IpGeolocationMiddleware( + $this->ipLocationResolver, + $this->dbUpdater, + $this->logger, + new TrackingOptions(disableTracking: $disableTracking), + ); + } +} diff --git a/module/Core/test/Matomo/MatomoVisitSenderTest.php b/module/Core/test/Matomo/MatomoVisitSenderTest.php index f78d0f33..0acccd1d 100644 --- a/module/Core/test/Matomo/MatomoVisitSenderTest.php +++ b/module/Core/test/Matomo/MatomoVisitSenderTest.php @@ -92,7 +92,7 @@ class MatomoVisitSenderTest extends TestCase '1.2.3.4', ['setCity', 'setCountry', 'setLatitude', 'setLongitude', 'setIp'], ]; - yield 'fallback IP' => [Visit::forBasePath(new Visitor('', '', '1.2.3.4', '')), null, ['setIp']]; + yield 'fallback IP' => [Visit::forBasePath(Visitor::fromParams(remoteAddress: '1.2.3.4')), null, ['setIp']]; } #[Test, DataProvider('provideUrlsToTrack')] @@ -117,7 +117,7 @@ class MatomoVisitSenderTest extends TestCase { yield 'orphan visit without visited URL' => [Visit::forBasePath(Visitor::empty()), '']; yield 'orphan visit with visited URL' => [ - Visit::forBasePath(new Visitor('', '', null, 'https://s.test/foo')), + Visit::forBasePath(Visitor::fromParams(visitedUrl: 'https://s.test/foo')), 'https://s.test/foo', ]; yield 'non-orphan visit' => [ diff --git a/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php b/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php index d22b632d..2ae5df18 100644 --- a/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php +++ b/module/Core/test/RedirectRule/Entity/RedirectConditionTest.php @@ -10,7 +10,6 @@ use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Common\Middleware\IpAddressMiddlewareFactory; use Shlinkio\Shlink\Core\Model\DeviceType; use Shlinkio\Shlink\Core\RedirectRule\Entity\RedirectCondition; -use Shlinkio\Shlink\Core\Visit\Entity\VisitLocation; use Shlinkio\Shlink\IpGeolocation\Model\Location; use const ShlinkioTest\Shlink\ANDROID_USER_AGENT; @@ -99,7 +98,7 @@ class RedirectConditionTest extends TestCase #[Test, DataProvider('provideVisitsWithCountry')] public function matchesGeolocationCountryCode( - Location|VisitLocation|null $location, + Location|null $location, string $countryCodeToMatch, bool $expected, ): void { @@ -114,21 +113,11 @@ class RedirectConditionTest extends TestCase yield 'non-matching location' => [new Location(countryCode: 'ES'), 'US', false]; yield 'matching location' => [new Location(countryCode: 'US'), 'US', true]; yield 'matching case-insensitive' => [new Location(countryCode: 'US'), 'us', true]; - yield 'matching visit location' => [ - VisitLocation::fromGeolocation(new Location(countryCode: 'US')), - 'US', - true, - ]; - yield 'matching visit case-insensitive' => [ - VisitLocation::fromGeolocation(new Location(countryCode: 'es')), - 'ES', - true, - ]; } #[Test, DataProvider('provideVisitsWithCity')] public function matchesGeolocationCityName( - Location|VisitLocation|null $location, + Location|null $location, string $cityNameToMatch, bool $expected, ): void { @@ -143,15 +132,5 @@ class RedirectConditionTest extends TestCase yield 'non-matching location' => [new Location(city: 'Los Angeles'), 'New York', false]; yield 'matching location' => [new Location(city: 'Madrid'), 'Madrid', true]; yield 'matching case-insensitive' => [new Location(city: 'Los Angeles'), 'los angeles', true]; - yield 'matching visit location' => [ - VisitLocation::fromGeolocation(new Location(city: 'New York')), - 'New York', - true, - ]; - yield 'matching visit case-insensitive' => [ - VisitLocation::fromGeolocation(new Location(city: 'barcelona')), - 'Barcelona', - true, - ]; } } diff --git a/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php b/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php index 2e35c1a4..85168020 100644 --- a/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php +++ b/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php @@ -9,7 +9,6 @@ use Laminas\Diactoros\ServerRequestFactory; use Laminas\Diactoros\Uri; use Mezzio\Router\Route; use Mezzio\Router\RouteResult; -use PHPUnit\Framework\Assert; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; @@ -27,7 +26,6 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; -use Shlinkio\Shlink\IpGeolocation\Model\Location; use function Laminas\Stratigility\middleware; use function str_starts_with; @@ -155,10 +153,7 @@ class ExtraPathRedirectMiddlewareTest extends TestCase ); $this->redirectionBuilder->expects($this->once())->method('buildShortUrlRedirect')->with( $shortUrl, - $this->callback(function (ServerRequestInterface $req) { - Assert::assertArrayHasKey(Location::class, $req->getAttributes()); - return true; - }), + $this->isInstanceOf(ServerRequestInterface::class), $expectedExtraPath, )->willReturn('the_built_long_url'); $this->redirectResponseHelper->expects($this->once())->method('buildRedirectResponse')->with( diff --git a/module/Core/test/Visit/Entity/VisitTest.php b/module/Core/test/Visit/Entity/VisitTest.php index edb47a53..db23af97 100644 --- a/module/Core/test/Visit/Entity/VisitTest.php +++ b/module/Core/test/Visit/Entity/VisitTest.php @@ -22,7 +22,10 @@ class VisitTest extends TestCase #[Test, DataProvider('provideUserAgents')] public function isProperlyJsonSerialized(string $userAgent, bool $expectedToBePotentialBot): void { - $visit = Visit::forValidShortUrl(ShortUrl::createFake(), new Visitor($userAgent, 'some site', '1.2.3.4', '')); + $visit = Visit::forValidShortUrl( + ShortUrl::createFake(), + Visitor::fromParams($userAgent, 'some site', '1.2.3.4'), + ); self::assertEquals([ 'referer' => 'some site', @@ -110,7 +113,7 @@ class VisitTest extends TestCase ): void { $visit = Visit::forValidShortUrl( ShortUrl::createFake(), - new Visitor('Chrome', 'some site', $address, ''), + Visitor::fromParams('Chrome', 'some site', $address), $anonymize, ); diff --git a/module/Core/test/Visit/Geolocation/VisitToLocationHelperTest.php b/module/Core/test/Visit/Geolocation/VisitToLocationHelperTest.php index 1f6b7f09..a9d8f3e5 100644 --- a/module/Core/test/Visit/Geolocation/VisitToLocationHelperTest.php +++ b/module/Core/test/Visit/Geolocation/VisitToLocationHelperTest.php @@ -42,7 +42,7 @@ class VisitToLocationHelperTest extends TestCase { yield [Visit::forBasePath(Visitor::empty()), IpCannotBeLocatedException::forEmptyAddress()]; yield [ - Visit::forBasePath(new Visitor('foo', 'bar', IpAddress::LOCALHOST, '')), + Visit::forBasePath(Visitor::fromParams('foo', 'bar', IpAddress::LOCALHOST)), IpCannotBeLocatedException::forLocalhost(), ]; } @@ -55,6 +55,6 @@ class VisitToLocationHelperTest extends TestCase $this->expectExceptionObject(IpCannotBeLocatedException::forError($e)); $this->ipLocationResolver->expects($this->once())->method('resolveIpLocation')->willThrowException($e); - $this->helper->resolveVisitLocation(Visit::forBasePath(new Visitor('foo', 'bar', '1.2.3.4', ''))); + $this->helper->resolveVisitLocation(Visit::forBasePath(Visitor::fromParams('foo', 'bar', '1.2.3.4'))); } } diff --git a/module/Core/test/Visit/Model/VisitorTest.php b/module/Core/test/Visit/Model/VisitorTest.php index 04e57179..25be7440 100644 --- a/module/Core/test/Visit/Model/VisitorTest.php +++ b/module/Core/test/Visit/Model/VisitorTest.php @@ -20,7 +20,7 @@ class VisitorTest extends TestCase #[Test, DataProvider('provideParams')] public function providedFieldsValuesAreCropped(array $params, array $expected): void { - $visitor = new Visitor(...$params); + $visitor = Visitor::fromParams(...$params); ['userAgent' => $userAgent, 'referer' => $referer, 'remoteAddress' => $remoteAddress] = $expected; self::assertEquals($userAgent, $visitor->userAgent); @@ -75,7 +75,7 @@ class VisitorTest extends TestCase #[Test] public function newNormalizedInstanceIsCreatedFromTrackingOptions(): void { - $visitor = new Visitor( + $visitor = Visitor::fromParams( self::generateRandomString(2000), self::generateRandomString(2000), self::generateRandomString(2000), diff --git a/module/Rest/test-api/Fixtures/VisitsFixture.php b/module/Rest/test-api/Fixtures/VisitsFixture.php index 9972e3a8..e10b6dab 100644 --- a/module/Rest/test-api/Fixtures/VisitsFixture.php +++ b/module/Rest/test-api/Fixtures/VisitsFixture.php @@ -23,43 +23,55 @@ class VisitsFixture extends AbstractFixture implements DependentFixtureInterface { /** @var ShortUrl $abcShortUrl */ $abcShortUrl = $this->getReference('abc123_short_url'); - $manager->persist( - Visit::forValidShortUrl($abcShortUrl, new Visitor('shlink-tests-agent', '', '44.55.66.77', '')), - ); $manager->persist(Visit::forValidShortUrl( $abcShortUrl, - new Visitor('shlink-tests-agent', 'https://google.com', '4.5.6.7', ''), + Visitor::fromParams(userAgent: 'shlink-tests-agent', remoteAddress: '44.55.66.77'), + )); + $manager->persist(Visit::forValidShortUrl( + $abcShortUrl, + Visitor::fromParams('shlink-tests-agent', 'https://google.com', '4.5.6.7'), + )); + $manager->persist(Visit::forValidShortUrl( + $abcShortUrl, + Visitor::fromParams(userAgent: 'shlink-tests-agent', remoteAddress: '1.2.3.4'), )); - $manager->persist(Visit::forValidShortUrl($abcShortUrl, new Visitor('shlink-tests-agent', '', '1.2.3.4', ''))); /** @var ShortUrl $defShortUrl */ $defShortUrl = $this->getReference('def456_short_url'); - $manager->persist( - Visit::forValidShortUrl($defShortUrl, new Visitor('cf-facebook', '', '127.0.0.1', '')), - ); - $manager->persist( - Visit::forValidShortUrl($defShortUrl, new Visitor('shlink-tests-agent', 'https://app.shlink.io', '', '')), - ); + $manager->persist(Visit::forValidShortUrl( + $defShortUrl, + Visitor::fromParams(userAgent: 'cf-facebook', remoteAddress: '127.0.0.1'), + )); + $manager->persist(Visit::forValidShortUrl( + $defShortUrl, + Visitor::fromParams('shlink-tests-agent', 'https://app.shlink.io', ''), + )); /** @var ShortUrl $ghiShortUrl */ $ghiShortUrl = $this->getReference('ghi789_short_url'); - $manager->persist(Visit::forValidShortUrl($ghiShortUrl, new Visitor('shlink-tests-agent', '', '1.2.3.4', ''))); - $manager->persist( - Visit::forValidShortUrl($ghiShortUrl, new Visitor('shlink-tests-agent', 'https://app.shlink.io', '', '')), - ); + $manager->persist(Visit::forValidShortUrl( + $ghiShortUrl, + Visitor::fromParams(userAgent: 'shlink-tests-agent', remoteAddress: '1.2.3.4'), + )); + $manager->persist(Visit::forValidShortUrl( + $ghiShortUrl, + Visitor::fromParams('shlink-tests-agent', 'https://app.shlink.io', ''), + )); $manager->persist($this->setVisitDate( - fn () => Visit::forBasePath(new Visitor('shlink-tests-agent', 'https://s.test', '1.2.3.4', '')), + fn () => Visit::forBasePath(Visitor::fromParams('shlink-tests-agent', 'https://s.test', '1.2.3.4')), '2020-01-01', )); $manager->persist($this->setVisitDate( fn () => Visit::forRegularNotFound( - new Visitor('shlink-tests-agent', 'https://s.test/foo/bar', '1.2.3.4', ''), + Visitor::fromParams('shlink-tests-agent', 'https://s.test/foo/bar', '1.2.3.4'), ), '2020-02-01', )); $manager->persist($this->setVisitDate( - fn () => Visit::forInvalidShortUrl(new Visitor('cf-facebook', 'https://s.test/foo', '1.2.3.4', 'foo.com')), + fn () => Visit::forInvalidShortUrl( + Visitor::fromParams('cf-facebook', 'https://s.test/foo', '1.2.3.4', 'foo.com'), + ), '2020-03-01', ));