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(