diff --git a/module/Core/config/dependencies.config.php b/module/Core/config/dependencies.config.php index 9809c5dd..82bec10e 100644 --- a/module/Core/config/dependencies.config.php +++ b/module/Core/config/dependencies.config.php @@ -51,7 +51,7 @@ return [ Options\NotFoundRedirectOptions::class => ['config.not_found_redirects'], Options\UrlShortenerOptions::class => ['config.url_shortener'], - Service\UrlShortener::class => [Util\UrlValidator::class, 'em', Options\UrlShortenerOptions::class], + Service\UrlShortener::class => [Util\UrlValidator::class, 'em'], Service\VisitsTracker::class => ['em', EventDispatcherInterface::class], Service\ShortUrlService::class => ['em', Service\ShortUrl\ShortUrlResolver::class], Service\VisitService::class => ['em'], @@ -63,7 +63,7 @@ return [ ], Service\ShortUrl\ShortUrlResolver::class => ['em'], - Util\UrlValidator::class => ['httpClient'], + Util\UrlValidator::class => ['httpClient', Options\UrlShortenerOptions::class], Action\RedirectAction::class => [ Service\ShortUrl\ShortUrlResolver::class, diff --git a/module/Core/src/Service/UrlShortener.php b/module/Core/src/Service/UrlShortener.php index ab45143a..821c8e90 100644 --- a/module/Core/src/Service/UrlShortener.php +++ b/module/Core/src/Service/UrlShortener.php @@ -11,7 +11,6 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; -use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Util\TagManagerTrait; use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; @@ -24,17 +23,14 @@ class UrlShortener implements UrlShortenerInterface use TagManagerTrait; private EntityManagerInterface $em; - private UrlShortenerOptions $options; private UrlValidatorInterface $urlValidator; public function __construct( UrlValidatorInterface $urlValidator, - EntityManagerInterface $em, - UrlShortenerOptions $options + EntityManagerInterface $em ) { $this->urlValidator = $urlValidator; $this->em = $em; - $this->options = $options; } /** @@ -53,11 +49,7 @@ class UrlShortener implements UrlShortenerInterface return $existingShortUrl; } - // If the URL validation is enabled, check that the URL actually exists - if ($this->options->isUrlValidationEnabled()) { - $this->urlValidator->validateUrl($url); - } - + $this->urlValidator->validateUrl($url); $this->em->beginTransaction(); $shortUrl = new ShortUrl($url, $meta, new PersistenceDomainResolver($this->em)); $shortUrl->setTags($this->tagNamesToEntities($this->em, $tags)); diff --git a/module/Core/src/Util/UrlValidator.php b/module/Core/src/Util/UrlValidator.php index dca037cd..8d8cd072 100644 --- a/module/Core/src/Util/UrlValidator.php +++ b/module/Core/src/Util/UrlValidator.php @@ -9,16 +9,19 @@ use GuzzleHttp\ClientInterface; use GuzzleHttp\Exception\GuzzleException; use GuzzleHttp\RequestOptions; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; +use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; class UrlValidator implements UrlValidatorInterface, RequestMethodInterface { private const MAX_REDIRECTS = 15; private ClientInterface $httpClient; + private UrlShortenerOptions $options; - public function __construct(ClientInterface $httpClient) + public function __construct(ClientInterface $httpClient, UrlShortenerOptions $options) { $this->httpClient = $httpClient; + $this->options = $options; } /** @@ -26,6 +29,11 @@ class UrlValidator implements UrlValidatorInterface, RequestMethodInterface */ public function validateUrl(string $url): void { + // If the URL validation is not enabled, skip check + if (! $this->options->isUrlValidationEnabled()) { + return; + } + try { $this->httpClient->request(self::METHOD_GET, $url, [ RequestOptions::ALLOW_REDIRECTS => ['max' => self::MAX_REDIRECTS], diff --git a/module/Core/test/Service/UrlShortenerTest.php b/module/Core/test/Service/UrlShortenerTest.php index a0392489..0e855d90 100644 --- a/module/Core/test/Service/UrlShortenerTest.php +++ b/module/Core/test/Service/UrlShortenerTest.php @@ -17,7 +17,6 @@ use Shlinkio\Shlink\Core\Entity\ShortUrl; use Shlinkio\Shlink\Core\Entity\Tag; use Shlinkio\Shlink\Core\Exception\NonUniqueSlugException; use Shlinkio\Shlink\Core\Model\ShortUrlMeta; -use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\Repository\ShortUrlRepository; use Shlinkio\Shlink\Core\Service\UrlShortener; use Shlinkio\Shlink\Core\Util\UrlValidatorInterface; @@ -33,6 +32,10 @@ class UrlShortenerTest extends TestCase public function setUp(): void { $this->urlValidator = $this->prophesize(UrlValidatorInterface::class); + $this->urlValidator->validateUrl('http://foobar.com/12345/hello?foo=bar')->will( + function (): void { + }, + ); $this->em = $this->prophesize(EntityManagerInterface::class); $conn = $this->prophesize(Connection::class); @@ -50,16 +53,7 @@ class UrlShortenerTest extends TestCase $repo->shortCodeIsInUse(Argument::cetera())->willReturn(false); $this->em->getRepository(ShortUrl::class)->willReturn($repo->reveal()); - $this->setUrlShortener(false); - } - - private function setUrlShortener(bool $urlValidationEnabled): void - { - $this->urlShortener = new UrlShortener( - $this->urlValidator->reveal(), - $this->em->reveal(), - new UrlShortenerOptions(['validate_url' => $urlValidationEnabled]), - ); + $this->urlShortener = new UrlShortener($this->urlValidator->reveal(), $this->em->reveal()); } /** @test */ @@ -119,24 +113,6 @@ class UrlShortenerTest extends TestCase ); } - /** @test */ - public function validatorIsCalledWhenUrlValidationIsEnabled(): void - { - $this->setUrlShortener(true); - $validateUrl = $this->urlValidator->validateUrl('http://foobar.com/12345/hello?foo=bar')->will( - function (): void { - }, - ); - - $this->urlShortener->urlToShortCode( - new Uri('http://foobar.com/12345/hello?foo=bar'), - [], - ShortUrlMeta::createEmpty(), - ); - - $validateUrl->shouldHaveBeenCalledOnce(); - } - /** @test */ public function exceptionIsThrownWhenNonUniqueSlugIsProvided(): void { @@ -175,6 +151,7 @@ class UrlShortenerTest extends TestCase $findExisting->shouldHaveBeenCalledOnce(); $getRepo->shouldHaveBeenCalledOnce(); $this->em->persist(Argument::cetera())->shouldNotHaveBeenCalled(); + $this->urlValidator->validateUrl(Argument::cetera())->shouldNotHaveBeenCalled(); $this->assertSame($expected, $result); } diff --git a/module/Core/test/Util/UrlValidatorTest.php b/module/Core/test/Util/UrlValidatorTest.php index 5f018a05..50b70961 100644 --- a/module/Core/test/Util/UrlValidatorTest.php +++ b/module/Core/test/Util/UrlValidatorTest.php @@ -13,17 +13,20 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Exception\InvalidUrlException; +use Shlinkio\Shlink\Core\Options\UrlShortenerOptions; use Shlinkio\Shlink\Core\Util\UrlValidator; class UrlValidatorTest extends TestCase { private UrlValidator $urlValidator; private ObjectProphecy $httpClient; + private UrlShortenerOptions $options; public function setUp(): void { $this->httpClient = $this->prophesize(ClientInterface::class); - $this->urlValidator = new UrlValidator($this->httpClient->reveal()); + $this->options = new UrlShortenerOptions(['validate_url' => true]); + $this->urlValidator = new UrlValidator($this->httpClient->reveal(), $this->options); } /** @test */ @@ -52,4 +55,15 @@ class UrlValidatorTest extends TestCase $request->shouldHaveBeenCalledOnce(); } + + /** @test */ + public function noCheckIsPerformedWhenUrlValidationIsDisabled(): void + { + $request = $this->httpClient->request(Argument::cetera())->willReturn(new Response()); + $this->options->validateUrl = false; + + $this->urlValidator->validateUrl(''); + + $request->shouldNotHaveBeenCalled(); + } }