From d4cad337fc7293d8050d0cfe3f3c0bec2ce9b481 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 14 Jul 2021 16:36:03 +0200 Subject: [PATCH 1/8] Created component wrapping the logic to determine what's the URL to redirect to for a ShortUrl --- module/Core/config/dependencies.config.php | 4 ++ .../src/Action/AbstractTrackingAction.php | 26 +++------- module/Core/src/Action/RedirectAction.php | 4 +- .../Helper/ShortUrlRedirectionBuilder.php | 50 +++++++++++++++++++ .../ShortUrlRedirectionBuilderInterface.php | 12 +++++ module/Core/test/Action/PixelActionTest.php | 5 ++ .../Core/test/Action/RedirectActionTest.php | 37 ++++++++------ 7 files changed, 103 insertions(+), 35 deletions(-) create mode 100644 module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php create mode 100644 module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilderInterface.php diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 7dfd5df2..7a53f343 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -53,6 +53,7 @@ return [ ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class => ConfigAbstractFactory::class, ShortUrl\Helper\ShortUrlStringifier::class => ConfigAbstractFactory::class, ShortUrl\Helper\ShortUrlTitleResolutionHelper::class => ConfigAbstractFactory::class, + ShortUrl\Helper\ShortUrlRedirectionBuilder::class => ConfigAbstractFactory::class, ShortUrl\Transformer\ShortUrlDataTransformer::class => ConfigAbstractFactory::class, Mercure\MercureUpdatesGenerator::class => ConfigAbstractFactory::class, @@ -117,6 +118,7 @@ return [ Action\RedirectAction::class => [ Service\ShortUrl\ShortUrlResolver::class, Visit\VisitsTracker::class, + ShortUrl\Helper\ShortUrlRedirectionBuilder::class, Options\TrackingOptions::class, Util\RedirectResponseHelper::class, 'Logger_Shlink', @@ -124,6 +126,7 @@ return [ Action\PixelAction::class => [ Service\ShortUrl\ShortUrlResolver::class, Visit\VisitsTracker::class, + ShortUrl\Helper\ShortUrlRedirectionBuilder::class, Options\TrackingOptions::class, 'Logger_Shlink', ], @@ -137,6 +140,7 @@ return [ ShortUrl\Resolver\PersistenceShortUrlRelationResolver::class => ['em'], ShortUrl\Helper\ShortUrlStringifier::class => ['config.url_shortener.domain', 'config.router.base_path'], ShortUrl\Helper\ShortUrlTitleResolutionHelper::class => [Util\UrlValidator::class], + ShortUrl\Helper\ShortUrlRedirectionBuilder::class => [Options\TrackingOptions::class], ShortUrl\Transformer\ShortUrlDataTransformer::class => [ShortUrl\Helper\ShortUrlStringifier::class], Mercure\MercureUpdatesGenerator::class => [ diff --git a/module/Core/src/Action/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php index 7de21fa8..c6b2279d 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -5,8 +5,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Action; use Fig\Http\Message\RequestMethodInterface; -use GuzzleHttp\Psr7\Query; -use League\Uri\Uri; use Mezzio\Router\Middleware\ImplicitHeadMiddleware; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; @@ -14,16 +12,15 @@ use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; -use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Options\TrackingOptions; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; +use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlRedirectionBuilderInterface; use Shlinkio\Shlink\Core\Visit\VisitsTrackerInterface; use function array_key_exists; -use function array_merge; abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMethodInterface { @@ -32,6 +29,7 @@ abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMet public function __construct( private ShortUrlResolverInterface $urlResolver, private VisitsTrackerInterface $visitTracker, + private ShortUrlRedirectionBuilderInterface $redirectionBuilder, private TrackingOptions $trackingOptions, ?LoggerInterface $logger = null ) { @@ -42,36 +40,24 @@ abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMet { $identifier = ShortUrlIdentifier::fromRedirectRequest($request); $query = $request->getQueryParams(); - $disableTrackParam = $this->trackingOptions->getDisableTrackParam(); try { $shortUrl = $this->urlResolver->resolveEnabledShortUrl($identifier); - if ($this->shouldTrackRequest($request, $query, $disableTrackParam)) { + if ($this->shouldTrackRequest($request, $query)) { $this->visitTracker->track($shortUrl, Visitor::fromRequest($request)); } - return $this->createSuccessResp($this->buildUrlToRedirectTo($shortUrl, $query, $disableTrackParam)); + return $this->createSuccessResp($this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $query)); } catch (ShortUrlNotFoundException $e) { $this->logger->warning('An error occurred while tracking short code. {e}', ['e' => $e]); return $this->createErrorResp($request, $handler); } } - private function buildUrlToRedirectTo(ShortUrl $shortUrl, array $currentQuery, ?string $disableTrackParam): string - { - $uri = Uri::createFromString($shortUrl->getLongUrl()); - $hardcodedQuery = Query::parse($uri->getQuery() ?? ''); - if ($disableTrackParam !== null) { - unset($currentQuery[$disableTrackParam]); - } - $mergedQuery = array_merge($hardcodedQuery, $currentQuery); - - return (string) (empty($mergedQuery) ? $uri : $uri->withQuery(Query::build($mergedQuery))); - } - - private function shouldTrackRequest(ServerRequestInterface $request, array $query, ?string $disableTrackParam): bool + private function shouldTrackRequest(ServerRequestInterface $request, array $query): bool { + $disableTrackParam = $this->trackingOptions->getDisableTrackParam(); $forwardedMethod = $request->getAttribute(ImplicitHeadMiddleware::FORWARDED_HTTP_METHOD_ATTRIBUTE); if ($forwardedMethod === self::METHOD_HEAD) { return false; diff --git a/module/Core/src/Action/RedirectAction.php b/module/Core/src/Action/RedirectAction.php index e1c6757c..7c313c53 100644 --- a/module/Core/src/Action/RedirectAction.php +++ b/module/Core/src/Action/RedirectAction.php @@ -11,6 +11,7 @@ use Psr\Http\Server\RequestHandlerInterface; use Psr\Log\LoggerInterface; use Shlinkio\Shlink\Core\Options; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; +use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlRedirectionBuilderInterface; use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; use Shlinkio\Shlink\Core\Visit\VisitsTrackerInterface; @@ -19,11 +20,12 @@ class RedirectAction extends AbstractTrackingAction implements StatusCodeInterfa public function __construct( ShortUrlResolverInterface $urlResolver, VisitsTrackerInterface $visitTracker, + ShortUrlRedirectionBuilderInterface $redirectionBuilder, Options\TrackingOptions $trackingOptions, private RedirectResponseHelperInterface $redirectResponseHelper, ?LoggerInterface $logger = null ) { - parent::__construct($urlResolver, $visitTracker, $trackingOptions, $logger); + parent::__construct($urlResolver, $visitTracker, $redirectionBuilder, $trackingOptions, $logger); } protected function createSuccessResp(string $longUrl): Response diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php new file mode 100644 index 00000000..1c45698f --- /dev/null +++ b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php @@ -0,0 +1,50 @@ +getLongUrl()); + + return $uri + ->withQuery($this->resolveQuery($uri, $currentQuery)) + ->withPath($this->resolvePath($uri, $extraPath)) + ->__toString(); + } + + private function resolveQuery(Uri $uri, array $currentQuery): string + { + $hardcodedQuery = Query::parse($uri->getQuery() ?? ''); + + $disableTrackParam = $this->trackingOptions->getDisableTrackParam(); + if ($disableTrackParam !== null) { + unset($currentQuery[$disableTrackParam]); + } + + $mergedQuery = array_merge($hardcodedQuery, $currentQuery); + + return empty($mergedQuery) ? '' : Query::build($mergedQuery); + } + + private function resolvePath(Uri $uri, ?string $extraPath): string + { + $hardcodedPath = $uri->getPath(); + return $extraPath === null ? $hardcodedPath : sprintf('%s%s', $hardcodedPath, $extraPath); + } +} diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilderInterface.php b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilderInterface.php new file mode 100644 index 00000000..d957ad14 --- /dev/null +++ b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilderInterface.php @@ -0,0 +1,12 @@ +urlResolver = $this->prophesize(ShortUrlResolverInterface::class); $this->visitTracker = $this->prophesize(VisitsTracker::class); + $redirectBuilder = $this->prophesize(ShortUrlRedirectionBuilderInterface::class); + $redirectBuilder->buildShortUrlRedirect(Argument::cetera())->willReturn('http://domain.com/foo/bar'); + $this->action = new PixelAction( $this->urlResolver->reveal(), $this->visitTracker->reveal(), + $redirectBuilder->reveal(), new TrackingOptions(), ); } diff --git a/module/Core/test/Action/RedirectActionTest.php b/module/Core/test/Action/RedirectActionTest.php index dde9144c..72408302 100644 --- a/module/Core/test/Action/RedirectActionTest.php +++ b/module/Core/test/Action/RedirectActionTest.php @@ -19,6 +19,7 @@ use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Options; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; +use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlRedirectionBuilderInterface; use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; use Shlinkio\Shlink\Core\Visit\VisitsTrackerInterface; @@ -28,6 +29,8 @@ class RedirectActionTest extends TestCase { use ProphecyTrait; + private const LONG_URL = 'https://domain.com/foo/bar?some=thing'; + private RedirectAction $action; private ObjectProphecy $urlResolver; private ObjectProphecy $visitTracker; @@ -39,9 +42,13 @@ class RedirectActionTest extends TestCase $this->visitTracker = $this->prophesize(VisitsTrackerInterface::class); $this->redirectRespHelper = $this->prophesize(RedirectResponseHelperInterface::class); + $redirectBuilder = $this->prophesize(ShortUrlRedirectionBuilderInterface::class); + $redirectBuilder->buildShortUrlRedirect(Argument::cetera())->willReturn(self::LONG_URL); + $this->action = new RedirectAction( $this->urlResolver->reveal(), $this->visitTracker->reveal(), + $redirectBuilder->reveal(), new Options\TrackingOptions(['disableTrackParam' => 'foobar']), $this->redirectRespHelper->reveal(), ); @@ -51,17 +58,17 @@ class RedirectActionTest extends TestCase * @test * @dataProvider provideQueries */ - public function redirectionIsPerformedToLongUrl(string $expectedUrl, array $query): void + public function redirectionIsPerformedToLongUrl(array $query): void { $shortCode = 'abc123'; - $shortUrl = ShortUrl::withLongUrl('http://domain.com/foo/bar?some=thing'); + $shortUrl = ShortUrl::withLongUrl(self::LONG_URL); $shortCodeToUrl = $this->urlResolver->resolveEnabledShortUrl( new ShortUrlIdentifier($shortCode, ''), )->willReturn($shortUrl); $track = $this->visitTracker->track(Argument::cetera())->will(function (): void { }); - $expectedResp = new Response\RedirectResponse($expectedUrl); - $buildResp = $this->redirectRespHelper->buildRedirectResponse($expectedUrl)->willReturn($expectedResp); + $expectedResp = new Response\RedirectResponse(self::LONG_URL); + $buildResp = $this->redirectRespHelper->buildRedirectResponse(self::LONG_URL)->willReturn($expectedResp); $request = (new ServerRequest())->withAttribute('shortCode', $shortCode)->withQueryParams($query); $response = $this->action->process($request, $this->prophesize(RequestHandlerInterface::class)->reveal()); @@ -74,12 +81,14 @@ class RedirectActionTest extends TestCase public function provideQueries(): iterable { - yield ['http://domain.com/foo/bar?some=thing', []]; - yield ['http://domain.com/foo/bar?some=thing', ['foobar' => 'notrack']]; - yield ['http://domain.com/foo/bar?some=thing&else', ['else' => null]]; - yield ['http://domain.com/foo/bar?some=thing&foo=bar', ['foo' => 'bar']]; - yield ['http://domain.com/foo/bar?some=overwritten&foo=bar', ['foo' => 'bar', 'some' => 'overwritten']]; - yield ['http://domain.com/foo/bar?some=overwritten', ['foobar' => 'notrack', 'some' => 'overwritten']]; + yield [[]]; + yield [['foobar' => 'notrack']]; + yield [['foobar' => 'barfoo']]; + yield [['foobar' => null]]; +// yield ['http://domain.com/foo/bar?some=thing&else', ['else' => null]]; +// yield ['http://domain.com/foo/bar?some=thing&foo=bar', ['foo' => 'bar']]; +// yield ['http://domain.com/foo/bar?some=overwritten&foo=bar', ['foo' => 'bar', 'some' => 'overwritten']]; +// yield ['http://domain.com/foo/bar?some=overwritten', ['foobar' => 'notrack', 'some' => 'overwritten']]; } /** @test */ @@ -104,13 +113,13 @@ class RedirectActionTest extends TestCase public function trackingIsDisabledWhenRequestIsForwardedFromHead(): void { $shortCode = 'abc123'; - $shortUrl = ShortUrl::withLongUrl('http://domain.com/foo/bar?some=thing'); + $shortUrl = ShortUrl::withLongUrl(self::LONG_URL); $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode, ''))->willReturn($shortUrl); $track = $this->visitTracker->track(Argument::cetera())->will(function (): void { }); - $buildResp = $this->redirectRespHelper->buildRedirectResponse( - 'http://domain.com/foo/bar?some=thing', - )->willReturn(new Response\RedirectResponse('')); + $buildResp = $this->redirectRespHelper->buildRedirectResponse(self::LONG_URL)->willReturn( + new Response\RedirectResponse(''), + ); $request = (new ServerRequest())->withAttribute('shortCode', $shortCode) ->withAttribute( From fe5460e0c59b4e7497d2c41dce79d39968a7a755 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Wed, 14 Jul 2021 16:44:21 +0200 Subject: [PATCH 2/8] Created ShortUrlRedirectBuilder test --- .../Core/test/Action/RedirectActionTest.php | 4 -- .../Helper/ShortUrlRedirectionBuilderTest.php | 49 +++++++++++++++++++ 2 files changed, 49 insertions(+), 4 deletions(-) create mode 100644 module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php diff --git a/module/Core/test/Action/RedirectActionTest.php b/module/Core/test/Action/RedirectActionTest.php index 72408302..e3c4694a 100644 --- a/module/Core/test/Action/RedirectActionTest.php +++ b/module/Core/test/Action/RedirectActionTest.php @@ -85,10 +85,6 @@ class RedirectActionTest extends TestCase yield [['foobar' => 'notrack']]; yield [['foobar' => 'barfoo']]; yield [['foobar' => null]]; -// yield ['http://domain.com/foo/bar?some=thing&else', ['else' => null]]; -// yield ['http://domain.com/foo/bar?some=thing&foo=bar', ['foo' => 'bar']]; -// yield ['http://domain.com/foo/bar?some=overwritten&foo=bar', ['foo' => 'bar', 'some' => 'overwritten']]; -// yield ['http://domain.com/foo/bar?some=overwritten', ['foobar' => 'notrack', 'some' => 'overwritten']]; } /** @test */ diff --git a/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php new file mode 100644 index 00000000..c64147c1 --- /dev/null +++ b/module/Core/test/ShortUrl/Helper/ShortUrlRedirectionBuilderTest.php @@ -0,0 +1,49 @@ +trackingOptions = new TrackingOptions(['disable_track_param' => 'foobar']); + $this->redirectionBuilder = new ShortUrlRedirectionBuilder($this->trackingOptions); + } + + /** + * @test + * @dataProvider provideData + */ + public function buildShortUrlRedirectBuildsExpectedUrl(string $expectedUrl, array $query, ?string $extraPath): void + { + $shortUrl = ShortUrl::withLongUrl('https://domain.com/foo/bar?some=thing'); + $result = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $query, $extraPath); + + self::assertEquals($expectedUrl, $result); + } + + public function provideData(): iterable + { + yield ['https://domain.com/foo/bar?some=thing', [], null]; + yield ['https://domain.com/foo/bar?some=thing&else', ['else' => null], null]; + yield ['https://domain.com/foo/bar?some=thing&foo=bar', ['foo' => 'bar'], null]; + yield ['https://domain.com/foo/bar?some=overwritten&foo=bar', ['foo' => 'bar', 'some' => 'overwritten'], null]; + yield ['https://domain.com/foo/bar?some=overwritten', ['foobar' => 'notrack', 'some' => 'overwritten'], null]; + yield ['https://domain.com/foo/bar/something/else-baz?some=thing', [], '/something/else-baz']; + yield [ + 'https://domain.com/foo/bar/something/else-baz?some=thing&hello=world', + ['hello' => 'world'], + '/something/else-baz', + ]; + } +} From 265e8cdeaf4dc27e4fa15c5262b158de35a2bccf Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 15 Jul 2021 13:28:31 +0200 Subject: [PATCH 3/8] Refactored tracking actions --- config/autoload/url-shortener.global.php | 5 +++- module/Core/config/dependencies.config.php | 5 +--- .../src/Action/AbstractTrackingAction.php | 24 ++++++++----------- module/Core/src/Action/PixelAction.php | 3 ++- module/Core/src/Action/RedirectAction.php | 16 ++++--------- .../Core/src/Options/UrlShortenerOptions.php | 11 +++++++++ module/Core/test/Action/PixelActionTest.php | 5 ---- .../Core/test/Action/RedirectActionTest.php | 2 +- 8 files changed, 34 insertions(+), 37 deletions(-) diff --git a/config/autoload/url-shortener.global.php b/config/autoload/url-shortener.global.php index d7cd8b02..4a6afbc2 100644 --- a/config/autoload/url-shortener.global.php +++ b/config/autoload/url-shortener.global.php @@ -16,9 +16,12 @@ return [ 'validate_url' => false, // Deprecated 'visits_webhooks' => [], 'default_short_codes_length' => DEFAULT_SHORT_CODES_LENGTH, + 'auto_resolve_titles' => false, + 'append_extra_path' => false, + + // TODO Move these two options to their own config namespace. Maybe "redirects". 'redirect_status_code' => DEFAULT_REDIRECT_STATUS_CODE, 'redirect_cache_lifetime' => DEFAULT_REDIRECT_CACHE_LIFETIME, - 'auto_resolve_titles' => false, ], ]; diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 7a53f343..34de226d 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -118,17 +118,14 @@ return [ Action\RedirectAction::class => [ Service\ShortUrl\ShortUrlResolver::class, Visit\VisitsTracker::class, - ShortUrl\Helper\ShortUrlRedirectionBuilder::class, Options\TrackingOptions::class, + ShortUrl\Helper\ShortUrlRedirectionBuilder::class, Util\RedirectResponseHelper::class, - 'Logger_Shlink', ], Action\PixelAction::class => [ Service\ShortUrl\ShortUrlResolver::class, Visit\VisitsTracker::class, - ShortUrl\Helper\ShortUrlRedirectionBuilder::class, Options\TrackingOptions::class, - 'Logger_Shlink', ], Action\QrCodeAction::class => [ Service\ShortUrl\ShortUrlResolver::class, diff --git a/module/Core/src/Action/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php index c6b2279d..b0f3d6ee 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -7,33 +7,27 @@ namespace Shlinkio\Shlink\Core\Action; use Fig\Http\Message\RequestMethodInterface; use Mezzio\Router\Middleware\ImplicitHeadMiddleware; use Psr\Http\Message\ResponseInterface; +use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; -use Psr\Log\LoggerInterface; -use Psr\Log\NullLogger; +use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Model\Visitor; use Shlinkio\Shlink\Core\Options\TrackingOptions; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; -use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlRedirectionBuilderInterface; use Shlinkio\Shlink\Core\Visit\VisitsTrackerInterface; use function array_key_exists; abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMethodInterface { - private LoggerInterface $logger; - public function __construct( private ShortUrlResolverInterface $urlResolver, private VisitsTrackerInterface $visitTracker, - private ShortUrlRedirectionBuilderInterface $redirectionBuilder, private TrackingOptions $trackingOptions, - ?LoggerInterface $logger = null ) { - $this->logger = $logger ?? new NullLogger(); } public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface @@ -48,9 +42,8 @@ abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMet $this->visitTracker->track($shortUrl, Visitor::fromRequest($request)); } - return $this->createSuccessResp($this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $query)); + return $this->createSuccessResp($shortUrl, $request); } catch (ShortUrlNotFoundException $e) { - $this->logger->warning('An error occurred while tracking short code. {e}', ['e' => $e]); return $this->createErrorResp($request, $handler); } } @@ -66,10 +59,13 @@ abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMet return $disableTrackParam === null || ! array_key_exists($disableTrackParam, $query); } - abstract protected function createSuccessResp(string $longUrl): ResponseInterface; - - abstract protected function createErrorResp( + abstract protected function createSuccessResp( + ShortUrl $shortUrl, ServerRequestInterface $request, - RequestHandlerInterface $handler, ): ResponseInterface; + + protected function createErrorResp(ServerRequestInterface $request, RequestHandlerInterface $handler): Response + { + return $handler->handle($request); + } } diff --git a/module/Core/src/Action/PixelAction.php b/module/Core/src/Action/PixelAction.php index 3f67bdec..0cf2a801 100644 --- a/module/Core/src/Action/PixelAction.php +++ b/module/Core/src/Action/PixelAction.php @@ -8,10 +8,11 @@ use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Common\Response\PixelResponse; +use Shlinkio\Shlink\Core\Entity\ShortUrl; class PixelAction extends AbstractTrackingAction { - protected function createSuccessResp(string $longUrl): ResponseInterface + protected function createSuccessResp(ShortUrl $shortUrl, ServerRequestInterface $request): ResponseInterface { return new PixelResponse(); } diff --git a/module/Core/src/Action/RedirectAction.php b/module/Core/src/Action/RedirectAction.php index 7c313c53..2711f5bc 100644 --- a/module/Core/src/Action/RedirectAction.php +++ b/module/Core/src/Action/RedirectAction.php @@ -7,8 +7,7 @@ namespace Shlinkio\Shlink\Core\Action; use Fig\Http\Message\StatusCodeInterface; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface; -use Psr\Http\Server\RequestHandlerInterface; -use Psr\Log\LoggerInterface; +use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Options; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlRedirectionBuilderInterface; @@ -20,21 +19,16 @@ class RedirectAction extends AbstractTrackingAction implements StatusCodeInterfa public function __construct( ShortUrlResolverInterface $urlResolver, VisitsTrackerInterface $visitTracker, - ShortUrlRedirectionBuilderInterface $redirectionBuilder, Options\TrackingOptions $trackingOptions, + private ShortUrlRedirectionBuilderInterface $redirectionBuilder, private RedirectResponseHelperInterface $redirectResponseHelper, - ?LoggerInterface $logger = null ) { - parent::__construct($urlResolver, $visitTracker, $redirectionBuilder, $trackingOptions, $logger); + parent::__construct($urlResolver, $visitTracker, $trackingOptions); } - protected function createSuccessResp(string $longUrl): Response + protected function createSuccessResp(ShortUrl $shortUrl, ServerRequestInterface $request): Response { + $longUrl = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $request->getQueryParams()); return $this->redirectResponseHelper->buildRedirectResponse($longUrl); } - - protected function createErrorResp(ServerRequestInterface $request, RequestHandlerInterface $handler): Response - { - return $handler->handle($request); - } } diff --git a/module/Core/src/Options/UrlShortenerOptions.php b/module/Core/src/Options/UrlShortenerOptions.php index a0005da2..31ecc137 100644 --- a/module/Core/src/Options/UrlShortenerOptions.php +++ b/module/Core/src/Options/UrlShortenerOptions.php @@ -19,6 +19,7 @@ class UrlShortenerOptions extends AbstractOptions private int $redirectStatusCode = DEFAULT_REDIRECT_STATUS_CODE; private int $redirectCacheLifetime = DEFAULT_REDIRECT_CACHE_LIFETIME; private bool $autoResolveTitles = false; + private bool $appendExtraPath = false; public function isUrlValidationEnabled(): bool { @@ -67,6 +68,16 @@ class UrlShortenerOptions extends AbstractOptions $this->autoResolveTitles = $autoResolveTitles; } + public function appendExtraPath(): bool + { + return $this->appendExtraPath; + } + + protected function setAppendExtraPath(bool $appendExtraPath): void + { + $this->appendExtraPath = $appendExtraPath; + } + /** @deprecated */ protected function setAnonymizeRemoteAddr(bool $anonymizeRemoteAddr): void { diff --git a/module/Core/test/Action/PixelActionTest.php b/module/Core/test/Action/PixelActionTest.php index 2436ac27..6df2498a 100644 --- a/module/Core/test/Action/PixelActionTest.php +++ b/module/Core/test/Action/PixelActionTest.php @@ -16,7 +16,6 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\Options\TrackingOptions; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; -use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlRedirectionBuilderInterface; use Shlinkio\Shlink\Core\Visit\VisitsTracker; class PixelActionTest extends TestCase @@ -32,13 +31,9 @@ class PixelActionTest extends TestCase $this->urlResolver = $this->prophesize(ShortUrlResolverInterface::class); $this->visitTracker = $this->prophesize(VisitsTracker::class); - $redirectBuilder = $this->prophesize(ShortUrlRedirectionBuilderInterface::class); - $redirectBuilder->buildShortUrlRedirect(Argument::cetera())->willReturn('http://domain.com/foo/bar'); - $this->action = new PixelAction( $this->urlResolver->reveal(), $this->visitTracker->reveal(), - $redirectBuilder->reveal(), new TrackingOptions(), ); } diff --git a/module/Core/test/Action/RedirectActionTest.php b/module/Core/test/Action/RedirectActionTest.php index e3c4694a..cc5690d0 100644 --- a/module/Core/test/Action/RedirectActionTest.php +++ b/module/Core/test/Action/RedirectActionTest.php @@ -48,8 +48,8 @@ class RedirectActionTest extends TestCase $this->action = new RedirectAction( $this->urlResolver->reveal(), $this->visitTracker->reveal(), - $redirectBuilder->reveal(), new Options\TrackingOptions(['disableTrackParam' => 'foobar']), + $redirectBuilder->reveal(), $this->redirectRespHelper->reveal(), ); } From 32f7b4fbf6e979806dd0fc5d903c19010369178f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 15 Jul 2021 16:54:54 +0200 Subject: [PATCH 4/8] Created new middleware that redirects to short URLs with an extra path --- .../autoload/middleware-pipeline.global.php | 1 + module/Core/config/dependencies.config.php | 8 ++ .../src/Action/AbstractTrackingAction.php | 2 +- .../Helper/ShortUrlRedirectionBuilder.php | 4 +- .../ExtraPathRedirectMiddleware.php | 74 +++++++++++++++++++ 5 files changed, 86 insertions(+), 3 deletions(-) create mode 100644 module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php diff --git a/config/autoload/middleware-pipeline.global.php b/config/autoload/middleware-pipeline.global.php index c60e1ba7..0466ebc5 100644 --- a/config/autoload/middleware-pipeline.global.php +++ b/config/autoload/middleware-pipeline.global.php @@ -68,6 +68,7 @@ return [ // This middleware is in front of tracking actions explicitly. Putting here for orphan visits tracking IpAddress::class, Core\ErrorHandler\NotFoundTypeResolverMiddleware::class, + Core\ShortUrl\Middleware\ExtraPathRedirectMiddleware::class, Core\ErrorHandler\NotFoundTrackerMiddleware::class, Core\ErrorHandler\NotFoundRedirectHandler::class, Core\ErrorHandler\NotFoundTemplateHandler::class, diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 34de226d..baecce9f 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -55,6 +55,7 @@ return [ ShortUrl\Helper\ShortUrlTitleResolutionHelper::class => ConfigAbstractFactory::class, ShortUrl\Helper\ShortUrlRedirectionBuilder::class => ConfigAbstractFactory::class, ShortUrl\Transformer\ShortUrlDataTransformer::class => ConfigAbstractFactory::class, + ShortUrl\Middleware\ExtraPathRedirectMiddleware::class => ConfigAbstractFactory::class, Mercure\MercureUpdatesGenerator::class => ConfigAbstractFactory::class, @@ -139,6 +140,13 @@ return [ ShortUrl\Helper\ShortUrlTitleResolutionHelper::class => [Util\UrlValidator::class], ShortUrl\Helper\ShortUrlRedirectionBuilder::class => [Options\TrackingOptions::class], ShortUrl\Transformer\ShortUrlDataTransformer::class => [ShortUrl\Helper\ShortUrlStringifier::class], + ShortUrl\Middleware\ExtraPathRedirectMiddleware::class => [ + Service\ShortUrl\ShortUrlResolver::class, + Visit\VisitsTracker::class, + ShortUrl\Helper\ShortUrlRedirectionBuilder::class, + Util\RedirectResponseHelper::class, + Options\UrlShortenerOptions::class, + ], Mercure\MercureUpdatesGenerator::class => [ ShortUrl\Transformer\ShortUrlDataTransformer::class, diff --git a/module/Core/src/Action/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php index b0f3d6ee..554c1844 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -43,7 +43,7 @@ abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMet } return $this->createSuccessResp($shortUrl, $request); - } catch (ShortUrlNotFoundException $e) { + } catch (ShortUrlNotFoundException) { return $this->createErrorResp($request, $handler); } } diff --git a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php index 1c45698f..43ea4993 100644 --- a/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php +++ b/module/Core/src/ShortUrl/Helper/ShortUrlRedirectionBuilder.php @@ -28,7 +28,7 @@ class ShortUrlRedirectionBuilder implements ShortUrlRedirectionBuilderInterface ->__toString(); } - private function resolveQuery(Uri $uri, array $currentQuery): string + private function resolveQuery(Uri $uri, array $currentQuery): ?string { $hardcodedQuery = Query::parse($uri->getQuery() ?? ''); @@ -39,7 +39,7 @@ class ShortUrlRedirectionBuilder implements ShortUrlRedirectionBuilderInterface $mergedQuery = array_merge($hardcodedQuery, $currentQuery); - return empty($mergedQuery) ? '' : Query::build($mergedQuery); + return empty($mergedQuery) ? null : Query::build($mergedQuery); } private function resolvePath(Uri $uri, ?string $extraPath): string diff --git a/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php new file mode 100644 index 00000000..401c7a0a --- /dev/null +++ b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php @@ -0,0 +1,74 @@ +getAttribute(NotFoundType::class); + + // We'll apply this logic only if actively opted in and current URL is potentially /{shortCode}/[...] + if (! $notFoundType?->isRegularNotFound() || ! $this->urlShortenerOptions->appendExtraPath()) { + return $handler->handle($request); + } + + $uri = $request->getUri(); + $query = $request->getQueryParams(); + [$potentialShortCode, $extraPath] = $this->resolvePotentialShortCodeAndExtraPath($uri); + $identifier = ShortUrlIdentifier::fromShortCodeAndDomain($potentialShortCode, $uri->getAuthority()); + + try { + $shortUrl = $this->resolver->resolveEnabledShortUrl($identifier); + + // TODO Track visit + + $longUrl = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $query, $extraPath); + return $this->redirectResponseHelper->buildRedirectResponse($longUrl); + } catch (ShortUrlNotFoundException) { + return $handler->handle($request); + } + } + + /** + * @return array{0: string, 1: string|null} + */ + private function resolvePotentialShortCodeAndExtraPath(UriInterface $uri): array + { + $pathParts = explode('/', trim($uri->getPath(), '/'), 2); + [$potentialShortCode, $extraPath] = array_pad($pathParts, 2, null); + + return [$potentialShortCode, $extraPath === null ? null : sprintf('/%s', $extraPath)]; + } +} From 050f83e3bbfd8579a12461faa9753b65ae473ed2 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 15 Jul 2021 17:23:09 +0200 Subject: [PATCH 5/8] Wrapped logic to track requests to a new RequestTracker service --- module/Core/config/dependencies.config.php | 15 +-- .../src/Action/AbstractTrackingAction.php | 27 +---- module/Core/src/Action/RedirectAction.php | 8 +- .../NotFoundTrackerMiddleware.php | 19 +--- .../ExtraPathRedirectMiddleware.php | 7 +- module/Core/src/Visit/RequestTracker.php | 60 ++++++++++ .../src/Visit/RequestTrackerInterface.php | 15 +++ module/Core/test/Action/PixelActionTest.php | 15 +-- .../Core/test/Action/RedirectActionTest.php | 85 +++++++------- .../NotFoundTrackerMiddlewareTest.php | 106 ++++++++++-------- 10 files changed, 194 insertions(+), 163 deletions(-) create mode 100644 module/Core/src/Visit/RequestTracker.php create mode 100644 module/Core/src/Visit/RequestTrackerInterface.php diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index baecce9f..03147bcb 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -37,6 +37,7 @@ return [ Domain\DomainService::class => ConfigAbstractFactory::class, Visit\VisitsTracker::class => ConfigAbstractFactory::class, + Visit\RequestTracker::class => ConfigAbstractFactory::class, Visit\VisitLocator::class => ConfigAbstractFactory::class, Visit\VisitsStatsHelper::class => ConfigAbstractFactory::class, Visit\Transformer\OrphanVisitDataTransformer::class => InvokableFactory::class, @@ -71,7 +72,7 @@ return [ ConfigAbstractFactory::class => [ ErrorHandler\NotFoundTypeResolverMiddleware::class => ['config.router.base_path'], - ErrorHandler\NotFoundTrackerMiddleware::class => [Visit\VisitsTracker::class], + ErrorHandler\NotFoundTrackerMiddleware::class => [Visit\RequestTracker::class], ErrorHandler\NotFoundRedirectHandler::class => [ NotFoundRedirectOptions::class, Util\RedirectResponseHelper::class, @@ -94,6 +95,7 @@ return [ EventDispatcherInterface::class, Options\TrackingOptions::class, ], + Visit\RequestTracker::class => [Visit\VisitsTracker::class, Options\TrackingOptions::class], Service\ShortUrlService::class => [ 'em', Service\ShortUrl\ShortUrlResolver::class, @@ -118,16 +120,11 @@ return [ Action\RedirectAction::class => [ Service\ShortUrl\ShortUrlResolver::class, - Visit\VisitsTracker::class, - Options\TrackingOptions::class, + Visit\RequestTracker::class, ShortUrl\Helper\ShortUrlRedirectionBuilder::class, Util\RedirectResponseHelper::class, ], - Action\PixelAction::class => [ - Service\ShortUrl\ShortUrlResolver::class, - Visit\VisitsTracker::class, - Options\TrackingOptions::class, - ], + Action\PixelAction::class => [Service\ShortUrl\ShortUrlResolver::class, Visit\RequestTracker::class], Action\QrCodeAction::class => [ Service\ShortUrl\ShortUrlResolver::class, ShortUrl\Helper\ShortUrlStringifier::class, @@ -142,7 +139,7 @@ return [ ShortUrl\Transformer\ShortUrlDataTransformer::class => [ShortUrl\Helper\ShortUrlStringifier::class], ShortUrl\Middleware\ExtraPathRedirectMiddleware::class => [ Service\ShortUrl\ShortUrlResolver::class, - Visit\VisitsTracker::class, + Visit\RequestTracker::class, ShortUrl\Helper\ShortUrlRedirectionBuilder::class, Util\RedirectResponseHelper::class, Options\UrlShortenerOptions::class, diff --git a/module/Core/src/Action/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php index 554c1844..8e9aaa09 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -5,7 +5,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Action; use Fig\Http\Message\RequestMethodInterface; -use Mezzio\Router\Middleware\ImplicitHeadMiddleware; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface; @@ -14,33 +13,24 @@ use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; -use Shlinkio\Shlink\Core\Model\Visitor; -use Shlinkio\Shlink\Core\Options\TrackingOptions; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; -use Shlinkio\Shlink\Core\Visit\VisitsTrackerInterface; - -use function array_key_exists; +use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMethodInterface { public function __construct( private ShortUrlResolverInterface $urlResolver, - private VisitsTrackerInterface $visitTracker, - private TrackingOptions $trackingOptions, + private RequestTrackerInterface $requestTracker, ) { } public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { $identifier = ShortUrlIdentifier::fromRedirectRequest($request); - $query = $request->getQueryParams(); try { $shortUrl = $this->urlResolver->resolveEnabledShortUrl($identifier); - - if ($this->shouldTrackRequest($request, $query)) { - $this->visitTracker->track($shortUrl, Visitor::fromRequest($request)); - } + $this->requestTracker->trackIfApplicable($shortUrl, $request); return $this->createSuccessResp($shortUrl, $request); } catch (ShortUrlNotFoundException) { @@ -48,17 +38,6 @@ abstract class AbstractTrackingAction implements MiddlewareInterface, RequestMet } } - private function shouldTrackRequest(ServerRequestInterface $request, array $query): bool - { - $disableTrackParam = $this->trackingOptions->getDisableTrackParam(); - $forwardedMethod = $request->getAttribute(ImplicitHeadMiddleware::FORWARDED_HTTP_METHOD_ATTRIBUTE); - if ($forwardedMethod === self::METHOD_HEAD) { - return false; - } - - return $disableTrackParam === null || ! array_key_exists($disableTrackParam, $query); - } - abstract protected function createSuccessResp( ShortUrl $shortUrl, ServerRequestInterface $request, diff --git a/module/Core/src/Action/RedirectAction.php b/module/Core/src/Action/RedirectAction.php index 2711f5bc..8126a85a 100644 --- a/module/Core/src/Action/RedirectAction.php +++ b/module/Core/src/Action/RedirectAction.php @@ -8,22 +8,20 @@ use Fig\Http\Message\StatusCodeInterface; use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Core\Entity\ShortUrl; -use Shlinkio\Shlink\Core\Options; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlRedirectionBuilderInterface; use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; -use Shlinkio\Shlink\Core\Visit\VisitsTrackerInterface; +use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; class RedirectAction extends AbstractTrackingAction implements StatusCodeInterface { public function __construct( ShortUrlResolverInterface $urlResolver, - VisitsTrackerInterface $visitTracker, - Options\TrackingOptions $trackingOptions, + RequestTrackerInterface $requestTracker, private ShortUrlRedirectionBuilderInterface $redirectionBuilder, private RedirectResponseHelperInterface $redirectResponseHelper, ) { - parent::__construct($urlResolver, $visitTracker, $trackingOptions); + parent::__construct($urlResolver, $requestTracker); } protected function createSuccessResp(ShortUrl $shortUrl, ServerRequestInterface $request): Response diff --git a/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php b/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php index 473a0b60..f3342c5a 100644 --- a/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php +++ b/module/Core/src/ErrorHandler/NotFoundTrackerMiddleware.php @@ -8,30 +8,17 @@ use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; -use Shlinkio\Shlink\Core\ErrorHandler\Model\NotFoundType; -use Shlinkio\Shlink\Core\Model\Visitor; -use Shlinkio\Shlink\Core\Visit\VisitsTrackerInterface; +use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; class NotFoundTrackerMiddleware implements MiddlewareInterface { - public function __construct(private VisitsTrackerInterface $visitsTracker) + public function __construct(private RequestTrackerInterface $requestTracker) { } public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { - /** @var NotFoundType $notFoundType */ - $notFoundType = $request->getAttribute(NotFoundType::class); - $visitor = Visitor::fromRequest($request); - - if ($notFoundType->isBaseUrl()) { - $this->visitsTracker->trackBaseUrlVisit($visitor); - } elseif ($notFoundType->isRegularNotFound()) { - $this->visitsTracker->trackRegularNotFoundVisit($visitor); - } elseif ($notFoundType->isInvalidShortUrl()) { - $this->visitsTracker->trackInvalidShortUrlVisit($visitor); - } - + $this->requestTracker->trackNotFoundIfApplicable($request); return $handler->handle($request); } } diff --git a/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php index 401c7a0a..9d92067c 100644 --- a/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php +++ b/module/Core/src/ShortUrl/Middleware/ExtraPathRedirectMiddleware.php @@ -16,7 +16,7 @@ use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlRedirectionBuilderInterface; use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; -use Shlinkio\Shlink\Core\Visit\VisitsTrackerInterface; +use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; use function array_pad; use function explode; @@ -27,7 +27,7 @@ class ExtraPathRedirectMiddleware implements MiddlewareInterface { public function __construct( private ShortUrlResolverInterface $resolver, - private VisitsTrackerInterface $visitTracker, + private RequestTrackerInterface $requestTracker, private ShortUrlRedirectionBuilderInterface $redirectionBuilder, private RedirectResponseHelperInterface $redirectResponseHelper, private UrlShortenerOptions $urlShortenerOptions, @@ -51,8 +51,7 @@ class ExtraPathRedirectMiddleware implements MiddlewareInterface try { $shortUrl = $this->resolver->resolveEnabledShortUrl($identifier); - - // TODO Track visit + $this->requestTracker->trackIfApplicable($shortUrl, $request); $longUrl = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, $query, $extraPath); return $this->redirectResponseHelper->buildRedirectResponse($longUrl); diff --git a/module/Core/src/Visit/RequestTracker.php b/module/Core/src/Visit/RequestTracker.php new file mode 100644 index 00000000..3e5bfb51 --- /dev/null +++ b/module/Core/src/Visit/RequestTracker.php @@ -0,0 +1,60 @@ +shouldTrackRequest($request)) { + $this->visitsTracker->track($shortUrl, Visitor::fromRequest($request)); + } + } + + public function trackNotFoundIfApplicable(ServerRequestInterface $request): void + { + if (! $this->shouldTrackRequest($request)) { + return; + } + + /** @var NotFoundType|null $notFoundType */ + $notFoundType = $request->getAttribute(NotFoundType::class); + $visitor = Visitor::fromRequest($request); + + if ($notFoundType?->isBaseUrl()) { + $this->visitsTracker->trackBaseUrlVisit($visitor); + } elseif ($notFoundType?->isRegularNotFound()) { + $this->visitsTracker->trackRegularNotFoundVisit($visitor); + } elseif ($notFoundType?->isInvalidShortUrl()) { + $this->visitsTracker->trackInvalidShortUrlVisit($visitor); + } + } + + private function shouldTrackRequest(ServerRequestInterface $request): bool + { + $query = $request->getQueryParams(); + $disableTrackParam = $this->trackingOptions->getDisableTrackParam(); + $forwardedMethod = $request->getAttribute(ImplicitHeadMiddleware::FORWARDED_HTTP_METHOD_ATTRIBUTE); + if ($forwardedMethod === self::METHOD_HEAD) { + return false; + } + + return $disableTrackParam === null || ! array_key_exists($disableTrackParam, $query); + } +} diff --git a/module/Core/src/Visit/RequestTrackerInterface.php b/module/Core/src/Visit/RequestTrackerInterface.php new file mode 100644 index 00000000..ec2c4cb1 --- /dev/null +++ b/module/Core/src/Visit/RequestTrackerInterface.php @@ -0,0 +1,15 @@ +urlResolver = $this->prophesize(ShortUrlResolverInterface::class); - $this->visitTracker = $this->prophesize(VisitsTracker::class); + $this->requestTracker = $this->prophesize(RequestTrackerInterface::class); - $this->action = new PixelAction( - $this->urlResolver->reveal(), - $this->visitTracker->reveal(), - new TrackingOptions(), - ); + $this->action = new PixelAction($this->urlResolver->reveal(), $this->requestTracker->reveal()); } /** @test */ @@ -45,7 +40,7 @@ class PixelActionTest extends TestCase $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode, ''))->willReturn( ShortUrl::withLongUrl('http://domain.com/foo/bar'), )->shouldBeCalledOnce(); - $this->visitTracker->track(Argument::cetera())->shouldBeCalledOnce(); + $this->requestTracker->trackIfApplicable(Argument::cetera())->shouldBeCalledOnce(); $request = (new ServerRequest())->withAttribute('shortCode', $shortCode); $response = $this->action->process($request, $this->prophesize(RequestHandlerInterface::class)->reveal()); diff --git a/module/Core/test/Action/RedirectActionTest.php b/module/Core/test/Action/RedirectActionTest.php index cc5690d0..3932810e 100644 --- a/module/Core/test/Action/RedirectActionTest.php +++ b/module/Core/test/Action/RedirectActionTest.php @@ -17,13 +17,10 @@ use Shlinkio\Shlink\Core\Action\RedirectAction; use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\ShortUrlNotFoundException; use Shlinkio\Shlink\Core\Model\ShortUrlIdentifier; -use Shlinkio\Shlink\Core\Options; use Shlinkio\Shlink\Core\Service\ShortUrl\ShortUrlResolverInterface; use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlRedirectionBuilderInterface; use Shlinkio\Shlink\Core\Util\RedirectResponseHelperInterface; -use Shlinkio\Shlink\Core\Visit\VisitsTrackerInterface; - -use function array_key_exists; +use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; class RedirectActionTest extends TestCase { @@ -33,13 +30,13 @@ class RedirectActionTest extends TestCase private RedirectAction $action; private ObjectProphecy $urlResolver; - private ObjectProphecy $visitTracker; + private ObjectProphecy $requestTracker; private ObjectProphecy $redirectRespHelper; public function setUp(): void { $this->urlResolver = $this->prophesize(ShortUrlResolverInterface::class); - $this->visitTracker = $this->prophesize(VisitsTrackerInterface::class); + $this->requestTracker = $this->prophesize(RequestTrackerInterface::class); $this->redirectRespHelper = $this->prophesize(RedirectResponseHelperInterface::class); $redirectBuilder = $this->prophesize(ShortUrlRedirectionBuilderInterface::class); @@ -47,45 +44,41 @@ class RedirectActionTest extends TestCase $this->action = new RedirectAction( $this->urlResolver->reveal(), - $this->visitTracker->reveal(), - new Options\TrackingOptions(['disableTrackParam' => 'foobar']), + $this->requestTracker->reveal(), $redirectBuilder->reveal(), $this->redirectRespHelper->reveal(), ); } - /** - * @test - * @dataProvider provideQueries - */ - public function redirectionIsPerformedToLongUrl(array $query): void + /** @test */ + public function redirectionIsPerformedToLongUrl(): void { $shortCode = 'abc123'; $shortUrl = ShortUrl::withLongUrl(self::LONG_URL); $shortCodeToUrl = $this->urlResolver->resolveEnabledShortUrl( new ShortUrlIdentifier($shortCode, ''), )->willReturn($shortUrl); - $track = $this->visitTracker->track(Argument::cetera())->will(function (): void { + $track = $this->requestTracker->trackIfApplicable(Argument::cetera())->will(function (): void { }); $expectedResp = new Response\RedirectResponse(self::LONG_URL); $buildResp = $this->redirectRespHelper->buildRedirectResponse(self::LONG_URL)->willReturn($expectedResp); - $request = (new ServerRequest())->withAttribute('shortCode', $shortCode)->withQueryParams($query); + $request = (new ServerRequest())->withAttribute('shortCode', $shortCode); $response = $this->action->process($request, $this->prophesize(RequestHandlerInterface::class)->reveal()); self::assertSame($expectedResp, $response); $buildResp->shouldHaveBeenCalledOnce(); $shortCodeToUrl->shouldHaveBeenCalledOnce(); - $track->shouldHaveBeenCalledTimes(array_key_exists('foobar', $query) ? 0 : 1); + $track->shouldHaveBeenCalledOnce(); } - public function provideQueries(): iterable - { - yield [[]]; - yield [['foobar' => 'notrack']]; - yield [['foobar' => 'barfoo']]; - yield [['foobar' => null]]; - } +// public function provideQueries(): iterable +// { +// yield [[]]; +// yield [['foobar' => 'notrack']]; +// yield [['foobar' => 'barfoo']]; +// yield [['foobar' => null]]; +// } /** @test */ public function nextMiddlewareIsInvokedIfLongUrlIsNotFound(): void @@ -94,7 +87,7 @@ class RedirectActionTest extends TestCase $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode, '')) ->willThrow(ShortUrlNotFoundException::class) ->shouldBeCalledOnce(); - $this->visitTracker->track(Argument::cetera())->shouldNotBeCalled(); + $this->requestTracker->trackIfApplicable(Argument::cetera())->shouldNotBeCalled(); $handler = $this->prophesize(RequestHandlerInterface::class); $handle = $handler->handle(Argument::any())->willReturn(new Response()); @@ -105,26 +98,26 @@ class RedirectActionTest extends TestCase $handle->shouldHaveBeenCalledOnce(); } - /** @test */ - public function trackingIsDisabledWhenRequestIsForwardedFromHead(): void - { - $shortCode = 'abc123'; - $shortUrl = ShortUrl::withLongUrl(self::LONG_URL); - $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode, ''))->willReturn($shortUrl); - $track = $this->visitTracker->track(Argument::cetera())->will(function (): void { - }); - $buildResp = $this->redirectRespHelper->buildRedirectResponse(self::LONG_URL)->willReturn( - new Response\RedirectResponse(''), - ); - - $request = (new ServerRequest())->withAttribute('shortCode', $shortCode) - ->withAttribute( - ImplicitHeadMiddleware::FORWARDED_HTTP_METHOD_ATTRIBUTE, - RequestMethodInterface::METHOD_HEAD, - ); - $this->action->process($request, $this->prophesize(RequestHandlerInterface::class)->reveal()); - - $buildResp->shouldHaveBeenCalled(); - $track->shouldNotHaveBeenCalled(); - } +// /** @test */ +// public function trackingIsDisabledWhenRequestIsForwardedFromHead(): void +// { +// $shortCode = 'abc123'; +// $shortUrl = ShortUrl::withLongUrl(self::LONG_URL); +// $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode, ''))->willReturn($shortUrl); +// $track = $this->requestTracker->trackIfApplicable(Argument::cetera())->will(function (): void { +// }); +// $buildResp = $this->redirectRespHelper->buildRedirectResponse(self::LONG_URL)->willReturn( +// new Response\RedirectResponse(''), +// ); +// +// $request = (new ServerRequest())->withAttribute('shortCode', $shortCode) +// ->withAttribute( +// ImplicitHeadMiddleware::FORWARDED_HTTP_METHOD_ATTRIBUTE, +// RequestMethodInterface::METHOD_HEAD, +// ); +// $this->action->process($request, $this->prophesize(RequestHandlerInterface::class)->reveal()); +// +// $buildResp->shouldHaveBeenCalled(); +// $track->shouldNotHaveBeenCalled(); +// } } diff --git a/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php b/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php index 560a2468..1a29a4b5 100644 --- a/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php +++ b/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php @@ -14,8 +14,7 @@ use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Core\ErrorHandler\Model\NotFoundType; use Shlinkio\Shlink\Core\ErrorHandler\NotFoundTrackerMiddleware; -use Shlinkio\Shlink\Core\Model\Visitor; -use Shlinkio\Shlink\Core\Visit\VisitsTrackerInterface; +use Shlinkio\Shlink\Core\Visit\RequestTrackerInterface; class NotFoundTrackerMiddlewareTest extends TestCase { @@ -23,7 +22,7 @@ class NotFoundTrackerMiddlewareTest extends TestCase private NotFoundTrackerMiddleware $middleware; private ServerRequestInterface $request; - private ObjectProphecy $visitsTracker; + private ObjectProphecy $requestTracker; private ObjectProphecy $notFoundType; private ObjectProphecy $handler; @@ -33,8 +32,8 @@ class NotFoundTrackerMiddlewareTest extends TestCase $this->handler = $this->prophesize(RequestHandlerInterface::class); $this->handler->handle(Argument::cetera())->willReturn(new Response()); - $this->visitsTracker = $this->prophesize(VisitsTrackerInterface::class); - $this->middleware = new NotFoundTrackerMiddleware($this->visitsTracker->reveal()); + $this->requestTracker = $this->prophesize(RequestTrackerInterface::class); + $this->middleware = new NotFoundTrackerMiddleware($this->requestTracker->reveal()); $this->request = ServerRequestFactory::fromGlobals()->withAttribute( NotFoundType::class, @@ -43,53 +42,62 @@ class NotFoundTrackerMiddlewareTest extends TestCase } /** @test */ - public function baseUrlErrorIsTracked(): void + public function delegatesIntoRequestTracker(): void { - $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(true); - $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(false); - $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(false); - $this->middleware->process($this->request, $this->handler->reveal()); - $isBaseUrl->shouldHaveBeenCalledOnce(); - $isRegularNotFound->shouldNotHaveBeenCalled(); - $isInvalidShortUrl->shouldNotHaveBeenCalled(); - $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); - $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); - $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); + $this->requestTracker->trackNotFoundIfApplicable($this->request)->shouldHaveBeenCalledOnce(); + $this->handler->handle($this->request)->shouldHaveBeenCalledOnce(); } - /** @test */ - public function regularNotFoundErrorIsTracked(): void - { - $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(false); - $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(true); - $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(false); - - $this->middleware->process($this->request, $this->handler->reveal()); - - $isBaseUrl->shouldHaveBeenCalledOnce(); - $isRegularNotFound->shouldHaveBeenCalledOnce(); - $isInvalidShortUrl->shouldNotHaveBeenCalled(); - $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); - $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); - $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); - } - - /** @test */ - public function invalidShortUrlErrorIsTracked(): void - { - $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(false); - $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(false); - $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(true); - - $this->middleware->process($this->request, $this->handler->reveal()); - - $isBaseUrl->shouldHaveBeenCalledOnce(); - $isRegularNotFound->shouldHaveBeenCalledOnce(); - $isInvalidShortUrl->shouldHaveBeenCalledOnce(); - $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); - $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); - $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); - } +// /** @test */ +// public function baseUrlErrorIsTracked(): void +// { +// $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(true); +// $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(false); +// $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(false); +// +// $this->middleware->process($this->request, $this->handler->reveal()); +// +// $isBaseUrl->shouldHaveBeenCalledOnce(); +// $isRegularNotFound->shouldNotHaveBeenCalled(); +// $isInvalidShortUrl->shouldNotHaveBeenCalled(); +// $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); +// $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); +// $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); +// } +// +// /** @test */ +// public function regularNotFoundErrorIsTracked(): void +// { +// $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(false); +// $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(true); +// $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(false); +// +// $this->middleware->process($this->request, $this->handler->reveal()); +// +// $isBaseUrl->shouldHaveBeenCalledOnce(); +// $isRegularNotFound->shouldHaveBeenCalledOnce(); +// $isInvalidShortUrl->shouldNotHaveBeenCalled(); +// $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); +// $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); +// $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); +// } +// +// /** @test */ +// public function invalidShortUrlErrorIsTracked(): void +// { +// $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(false); +// $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(false); +// $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(true); +// +// $this->middleware->process($this->request, $this->handler->reveal()); +// +// $isBaseUrl->shouldHaveBeenCalledOnce(); +// $isRegularNotFound->shouldHaveBeenCalledOnce(); +// $isInvalidShortUrl->shouldHaveBeenCalledOnce(); +// $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); +// $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); +// $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); +// } } From 0096a778ac1a8d118392f6b875cbb83480dffb52 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 15 Jul 2021 17:43:29 +0200 Subject: [PATCH 6/8] Created RequestTracker test --- .../Core/test/Action/RedirectActionTest.php | 33 ---- .../NotFoundTrackerMiddlewareTest.php | 51 ------ module/Core/test/Visit/RequestTrackerTest.php | 147 ++++++++++++++++++ 3 files changed, 147 insertions(+), 84 deletions(-) create mode 100644 module/Core/test/Visit/RequestTrackerTest.php diff --git a/module/Core/test/Action/RedirectActionTest.php b/module/Core/test/Action/RedirectActionTest.php index 3932810e..b3017fad 100644 --- a/module/Core/test/Action/RedirectActionTest.php +++ b/module/Core/test/Action/RedirectActionTest.php @@ -4,10 +4,8 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Core\Action; -use Fig\Http\Message\RequestMethodInterface; use Laminas\Diactoros\Response; use Laminas\Diactoros\ServerRequest; -use Mezzio\Router\Middleware\ImplicitHeadMiddleware; use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; @@ -72,14 +70,6 @@ class RedirectActionTest extends TestCase $track->shouldHaveBeenCalledOnce(); } -// public function provideQueries(): iterable -// { -// yield [[]]; -// yield [['foobar' => 'notrack']]; -// yield [['foobar' => 'barfoo']]; -// yield [['foobar' => null]]; -// } - /** @test */ public function nextMiddlewareIsInvokedIfLongUrlIsNotFound(): void { @@ -97,27 +87,4 @@ class RedirectActionTest extends TestCase $handle->shouldHaveBeenCalledOnce(); } - -// /** @test */ -// public function trackingIsDisabledWhenRequestIsForwardedFromHead(): void -// { -// $shortCode = 'abc123'; -// $shortUrl = ShortUrl::withLongUrl(self::LONG_URL); -// $this->urlResolver->resolveEnabledShortUrl(new ShortUrlIdentifier($shortCode, ''))->willReturn($shortUrl); -// $track = $this->requestTracker->trackIfApplicable(Argument::cetera())->will(function (): void { -// }); -// $buildResp = $this->redirectRespHelper->buildRedirectResponse(self::LONG_URL)->willReturn( -// new Response\RedirectResponse(''), -// ); -// -// $request = (new ServerRequest())->withAttribute('shortCode', $shortCode) -// ->withAttribute( -// ImplicitHeadMiddleware::FORWARDED_HTTP_METHOD_ATTRIBUTE, -// RequestMethodInterface::METHOD_HEAD, -// ); -// $this->action->process($request, $this->prophesize(RequestHandlerInterface::class)->reveal()); -// -// $buildResp->shouldHaveBeenCalled(); -// $track->shouldNotHaveBeenCalled(); -// } } diff --git a/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php b/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php index 1a29a4b5..81fef1a6 100644 --- a/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php +++ b/module/Core/test/ErrorHandler/NotFoundTrackerMiddlewareTest.php @@ -49,55 +49,4 @@ class NotFoundTrackerMiddlewareTest extends TestCase $this->requestTracker->trackNotFoundIfApplicable($this->request)->shouldHaveBeenCalledOnce(); $this->handler->handle($this->request)->shouldHaveBeenCalledOnce(); } - -// /** @test */ -// public function baseUrlErrorIsTracked(): void -// { -// $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(true); -// $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(false); -// $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(false); -// -// $this->middleware->process($this->request, $this->handler->reveal()); -// -// $isBaseUrl->shouldHaveBeenCalledOnce(); -// $isRegularNotFound->shouldNotHaveBeenCalled(); -// $isInvalidShortUrl->shouldNotHaveBeenCalled(); -// $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); -// $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); -// $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); -// } -// -// /** @test */ -// public function regularNotFoundErrorIsTracked(): void -// { -// $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(false); -// $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(true); -// $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(false); -// -// $this->middleware->process($this->request, $this->handler->reveal()); -// -// $isBaseUrl->shouldHaveBeenCalledOnce(); -// $isRegularNotFound->shouldHaveBeenCalledOnce(); -// $isInvalidShortUrl->shouldNotHaveBeenCalled(); -// $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); -// $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); -// $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); -// } -// -// /** @test */ -// public function invalidShortUrlErrorIsTracked(): void -// { -// $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(false); -// $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(false); -// $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(true); -// -// $this->middleware->process($this->request, $this->handler->reveal()); -// -// $isBaseUrl->shouldHaveBeenCalledOnce(); -// $isRegularNotFound->shouldHaveBeenCalledOnce(); -// $isInvalidShortUrl->shouldHaveBeenCalledOnce(); -// $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); -// $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); -// $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); -// } } diff --git a/module/Core/test/Visit/RequestTrackerTest.php b/module/Core/test/Visit/RequestTrackerTest.php new file mode 100644 index 00000000..46faf9fd --- /dev/null +++ b/module/Core/test/Visit/RequestTrackerTest.php @@ -0,0 +1,147 @@ +notFoundType = $this->prophesize(NotFoundType::class); + $this->visitsTracker = $this->prophesize(VisitsTrackerInterface::class); + + $this->requestTracker = new RequestTracker( + $this->visitsTracker->reveal(), + new TrackingOptions(['disable_track_param' => 'foobar']), + ); + + $this->request = ServerRequestFactory::fromGlobals()->withAttribute( + NotFoundType::class, + $this->notFoundType->reveal(), + ); + } + + /** + * @test + * @dataProvider provideNonTrackingRequests + */ + public function trackingIsDisabledWhenRequestDoesNotMeetConditions(ServerRequestInterface $request): void + { + $shortUrl = ShortUrl::withLongUrl(self::LONG_URL); + + $this->requestTracker->trackIfApplicable($shortUrl, $request); + + $this->visitsTracker->track(Argument::cetera())->shouldNotHaveBeenCalled(); + } + + public function provideNonTrackingRequests(): iterable + { + yield 'forwarded from head' => [ServerRequestFactory::fromGlobals()->withAttribute( + ImplicitHeadMiddleware::FORWARDED_HTTP_METHOD_ATTRIBUTE, + RequestMethodInterface::METHOD_HEAD, + )]; + yield 'disable track param' => [ServerRequestFactory::fromGlobals()->withQueryParams(['foobar' => 'foo'])]; + yield 'disable track param as null' => [ + ServerRequestFactory::fromGlobals()->withQueryParams(['foobar' => null]), + ]; + } + + /** @test */ + public function trackingHappensOverShortUrlsWhenRequestMeetsConditions(): void + { + $shortUrl = ShortUrl::withLongUrl(self::LONG_URL); + + $this->requestTracker->trackIfApplicable($shortUrl, $this->request); + + $this->visitsTracker->track($shortUrl, Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); + } + + /** @test */ + public function baseUrlErrorIsTracked(): void + { + $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(true); + $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(false); + $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(false); + + $this->requestTracker->trackNotFoundIfApplicable($this->request); + + $isBaseUrl->shouldHaveBeenCalledOnce(); + $isRegularNotFound->shouldNotHaveBeenCalled(); + $isInvalidShortUrl->shouldNotHaveBeenCalled(); + $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); + $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); + $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); + } + + /** @test */ + public function regularNotFoundErrorIsTracked(): void + { + $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(false); + $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(true); + $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(false); + + $this->requestTracker->trackNotFoundIfApplicable($this->request); + + $isBaseUrl->shouldHaveBeenCalledOnce(); + $isRegularNotFound->shouldHaveBeenCalledOnce(); + $isInvalidShortUrl->shouldNotHaveBeenCalled(); + $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); + $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); + $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); + } + + /** @test */ + public function invalidShortUrlErrorIsTracked(): void + { + $isBaseUrl = $this->notFoundType->isBaseUrl()->willReturn(false); + $isRegularNotFound = $this->notFoundType->isRegularNotFound()->willReturn(false); + $isInvalidShortUrl = $this->notFoundType->isInvalidShortUrl()->willReturn(true); + + $this->requestTracker->trackNotFoundIfApplicable($this->request); + + $isBaseUrl->shouldHaveBeenCalledOnce(); + $isRegularNotFound->shouldHaveBeenCalledOnce(); + $isInvalidShortUrl->shouldHaveBeenCalledOnce(); + $this->visitsTracker->trackBaseUrlVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); + $this->visitsTracker->trackRegularNotFoundVisit(Argument::type(Visitor::class))->shouldNotHaveBeenCalled(); + $this->visitsTracker->trackInvalidShortUrlVisit(Argument::type(Visitor::class))->shouldHaveBeenCalledOnce(); + } + + /** + * @test + * @dataProvider provideNonTrackingRequests + */ + public function notFoundIsNotTrackedIfRequestDoesNotMeetConditions(ServerRequestInterface $request): void + { + $this->requestTracker->trackNotFoundIfApplicable($request); + + $this->visitsTracker->trackBaseUrlVisit(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->visitsTracker->trackRegularNotFoundVisit(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->visitsTracker->trackInvalidShortUrlVisit(Argument::cetera())->shouldNotHaveBeenCalled(); + } +} From 20575a2b0f232fa0d7d53f01299db1cba64fc639 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 15 Jul 2021 18:57:32 +0200 Subject: [PATCH 7/8] Added support to provide append_extra_path config from installer or env vars for docker --- composer.json | 2 +- config/autoload/installer.global.php | 1 + docker/config/shlink_in_docker.local.php | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 4571e63e..7d637a60 100644 --- a/composer.json +++ b/composer.json @@ -51,7 +51,7 @@ "shlinkio/shlink-config": "^1.0", "shlinkio/shlink-event-dispatcher": "^2.1", "shlinkio/shlink-importer": "^2.3", - "shlinkio/shlink-installer": "^6.0", + "shlinkio/shlink-installer": "dev-develop#fa6a4ca as 6.1", "shlinkio/shlink-ip-geolocation": "^2.0", "symfony/console": "^5.1", "symfony/filesystem": "^5.1", diff --git a/config/autoload/installer.global.php b/config/autoload/installer.global.php index 0a72c6fa..0a3374e1 100644 --- a/config/autoload/installer.global.php +++ b/config/autoload/installer.global.php @@ -42,6 +42,7 @@ return [ Option\UrlShortener\RedirectStatusCodeConfigOption::class, Option\UrlShortener\RedirectCacheLifeTimeConfigOption::class, Option\UrlShortener\AutoResolveTitlesConfigOption::class, + Option\UrlShortener\AppendExtraPathConfigOption::class, Option\Tracking\IpAnonymizationConfigOption::class, Option\Tracking\OrphanVisitsTrackingConfigOption::class, Option\Tracking\DisableTrackParamConfigOption::class, diff --git a/docker/config/shlink_in_docker.local.php b/docker/config/shlink_in_docker.local.php index 2f1c9499..d4526f50 100644 --- a/docker/config/shlink_in_docker.local.php +++ b/docker/config/shlink_in_docker.local.php @@ -111,9 +111,10 @@ return [ 'validate_url' => (bool) env('VALIDATE_URLS', false), 'visits_webhooks' => $helper->getVisitsWebhooks(), 'default_short_codes_length' => $helper->getDefaultShortCodesLength(), + 'auto_resolve_titles' => (bool) env('AUTO_RESOLVE_TITLES', false), 'redirect_status_code' => (int) env('REDIRECT_STATUS_CODE', DEFAULT_REDIRECT_STATUS_CODE), 'redirect_cache_lifetime' => (int) env('REDIRECT_CACHE_LIFETIME', DEFAULT_REDIRECT_CACHE_LIFETIME), - 'auto_resolve_titles' => (bool) env('AUTO_RESOLVE_TITLES', false), + 'append_extra_path' => (bool) env('REDIRECT_APPEND_EXTRA_PATH', false), ], 'tracking' => [ From eabaa94e06fbb82081c54f918d0340f667bb482f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 15 Jul 2021 19:37:09 +0200 Subject: [PATCH 8/8] Created ExtraPathRedirectMiddleware test --- CHANGELOG.md | 6 + .../ExtraPathRedirectMiddlewareTest.php | 147 ++++++++++++++++++ 2 files changed, 153 insertions(+) create mode 100644 module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 76e14a26..63d9c6fe 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this Now, when calling the `GET /{shorCode}/qr-code` URL, you can pass the `errorCorrection` query param with values `L` for Low, `M` for Medium, `Q` for Quartile or `H` for High. +* [#1080](https://github.com/shlinkio/shlink/issues/1080) Added support to redirect to URLs as soon as the path starts with a valid short code, appending the rest of the path to the redirected long URL. + + With this, if you have the `https://example.com/abc123` short URL redirecting to `https://www.twitter.com`, a visit to `https://example.com/abc123/shlinkio` will take you to `https://www.twitter.com/shlinkio`. + + This behavior needs to be actively opted in, via installer config options or env vars. + ### Changed * *Nothing* diff --git a/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php b/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php new file mode 100644 index 00000000..24917366 --- /dev/null +++ b/module/Core/test/ShortUrl/Middleware/ExtraPathRedirectMiddlewareTest.php @@ -0,0 +1,147 @@ +resolver = $this->prophesize(ShortUrlResolverInterface::class); + $this->requestTracker = $this->prophesize(RequestTrackerInterface::class); + $this->redirectionBuilder = $this->prophesize(ShortUrlRedirectionBuilderInterface::class); + $this->redirectResponseHelper = $this->prophesize(RedirectResponseHelperInterface::class); + $this->options = new UrlShortenerOptions(['append_extra_path' => true]); + + $this->middleware = new ExtraPathRedirectMiddleware( + $this->resolver->reveal(), + $this->requestTracker->reveal(), + $this->redirectionBuilder->reveal(), + $this->redirectResponseHelper->reveal(), + $this->options, + ); + + $this->handler = $this->prophesize(RequestHandlerInterface::class); + $this->handler->handle(Argument::cetera())->willReturn(new RedirectResponse('')); + } + + /** + * @test + * @dataProvider provideNonRedirectingRequests + */ + public function handlerIsCalledWhenConfigPreventsRedirectWithExtraPath( + bool $appendExtraPath, + ServerRequestInterface $request + ): void { + $this->options->appendExtraPath = $appendExtraPath; + + $this->middleware->process($request, $this->handler->reveal()); + + $this->resolver->resolveEnabledShortUrl(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->requestTracker->trackIfApplicable(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->redirectionBuilder->buildShortUrlRedirect(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->redirectResponseHelper->buildRedirectResponse(Argument::cetera())->shouldNotHaveBeenCalled(); + } + + public function provideNonRedirectingRequests(): iterable + { + $baseReq = ServerRequestFactory::fromGlobals(); + $buildReq = static fn (?NotFoundType $type): ServerRequestInterface => + $baseReq->withAttribute(NotFoundType::class, $type); + + yield 'disabled option' => [false, $buildReq(NotFoundType::fromRequest($baseReq, '/foo/bar'))]; + yield 'base_url error' => [true, $buildReq(NotFoundType::fromRequest($baseReq, ''))]; + yield 'invalid_short_url error' => [ + true, + $buildReq(NotFoundType::fromRequest($baseReq, ''))->withAttribute( + RouteResult::class, + RouteResult::fromRoute(new Route( + '', + $this->prophesize(MiddlewareInterface::class)->reveal(), + ['GET'], + )), + ), + ]; + yield 'no error type' => [true, $buildReq(null)]; + } + + /** @test */ + public function handlerIsCalledWhenNoShortUrlIsFound(): void + { + $type = $this->prophesize(NotFoundType::class); + $type->isRegularNotFound()->willReturn(true); + $request = ServerRequestFactory::fromGlobals()->withAttribute(NotFoundType::class, $type->reveal()) + ->withUri(new Uri('/shortCode/bar/baz')); + + $resolve = $this->resolver->resolveEnabledShortUrl(Argument::cetera())->willThrow( + ShortUrlNotFoundException::class, + ); + + $this->middleware->process($request, $this->handler->reveal()); + + $resolve->shouldHaveBeenCalledOnce(); + $this->requestTracker->trackIfApplicable(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->redirectionBuilder->buildShortUrlRedirect(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->redirectResponseHelper->buildRedirectResponse(Argument::cetera())->shouldNotHaveBeenCalled(); + } + + /** @test */ + public function visitIsTrackedAndRedirectIsReturnedWhenShortUrlIsFound(): void + { + $type = $this->prophesize(NotFoundType::class); + $type->isRegularNotFound()->willReturn(true); + $request = ServerRequestFactory::fromGlobals()->withAttribute(NotFoundType::class, $type->reveal()) + ->withUri(new Uri('https://doma.in/shortCode/bar/baz')); + $shortUrl = ShortUrl::withLongUrl(''); + $identifier = ShortUrlIdentifier::fromShortCodeAndDomain('shortCode', 'doma.in'); + + $resolve = $this->resolver->resolveEnabledShortUrl($identifier)->willReturn($shortUrl); + $buildLongUrl = $this->redirectionBuilder->buildShortUrlRedirect($shortUrl, [], '/bar/baz')->willReturn( + 'the_built_long_url', + ); + $buildResp = $this->redirectResponseHelper->buildRedirectResponse('the_built_long_url')->willReturn( + new RedirectResponse(''), + ); + + $this->middleware->process($request, $this->handler->reveal()); + + $resolve->shouldHaveBeenCalledOnce(); + $buildLongUrl->shouldHaveBeenCalledOnce(); + $buildResp->shouldHaveBeenCalledOnce(); + $this->requestTracker->trackIfApplicable($shortUrl, $request)->shouldHaveBeenCalledOnce(); + } +}