From 06306aabd5f4ce7692516a7ff6621897b9bbd3ec Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 22 Dec 2023 13:29:22 +0100 Subject: [PATCH 1/6] Allow QR codes to be generated for disabled short URLs --- config/autoload/qr-codes.global.php | 4 ++++ config/constants.php | 2 ++ config/roadrunner/.rr.dev.yml | 2 +- module/Core/src/Action/QrCodeAction.php | 10 ++++++---- module/Core/src/Config/EnvVars.php | 1 + module/Core/src/Options/QrCodeOptions.php | 14 ++++++++------ .../src/ShortUrl/Model/ShortUrlIdentifier.php | 4 ++-- module/Core/src/ShortUrl/ShortUrlResolver.php | 18 ++++++++++++++---- .../src/ShortUrl/ShortUrlResolverInterface.php | 13 +++++++++++++ 9 files changed, 51 insertions(+), 17 deletions(-) diff --git a/config/autoload/qr-codes.global.php b/config/autoload/qr-codes.global.php index dc4f5f9e..808ff961 100644 --- a/config/autoload/qr-codes.global.php +++ b/config/autoload/qr-codes.global.php @@ -4,6 +4,7 @@ declare(strict_types=1); use Shlinkio\Shlink\Core\Config\EnvVars; +use const Shlinkio\Shlink\DEFAULT_QR_CODE_ENABLED_FOR_DISABLED_SHORT_URLS; use const Shlinkio\Shlink\DEFAULT_QR_CODE_ERROR_CORRECTION; use const Shlinkio\Shlink\DEFAULT_QR_CODE_FORMAT; use const Shlinkio\Shlink\DEFAULT_QR_CODE_MARGIN; @@ -22,6 +23,9 @@ return [ 'round_block_size' => (bool) EnvVars::DEFAULT_QR_CODE_ROUND_BLOCK_SIZE->loadFromEnv( DEFAULT_QR_CODE_ROUND_BLOCK_SIZE, ), + 'enabled_for_disabled_short_urls' => (bool) EnvVars::QR_CODE_FOR_DISABLED_SHORT_URLS->loadFromEnv( + DEFAULT_QR_CODE_ENABLED_FOR_DISABLED_SHORT_URLS, + ), ], ]; diff --git a/config/constants.php b/config/constants.php index 35d4c56e..f08c135c 100644 --- a/config/constants.php +++ b/config/constants.php @@ -19,4 +19,6 @@ const DEFAULT_QR_CODE_MARGIN = 0; const DEFAULT_QR_CODE_FORMAT = 'png'; const DEFAULT_QR_CODE_ERROR_CORRECTION = 'l'; const DEFAULT_QR_CODE_ROUND_BLOCK_SIZE = true; +// Deprecated. Shlink 4.0.0 should change default value to `true` +const DEFAULT_QR_CODE_ENABLED_FOR_DISABLED_SHORT_URLS = false; const MIN_TASK_WORKERS = 4; diff --git a/config/roadrunner/.rr.dev.yml b/config/roadrunner/.rr.dev.yml index cdc1f326..24c3a2fc 100644 --- a/config/roadrunner/.rr.dev.yml +++ b/config/roadrunner/.rr.dev.yml @@ -35,7 +35,7 @@ logs: http: mode: 'off' # Disable logging as Shlink handles it internally server: - level: debug + level: info metrics: level: debug jobs: diff --git a/module/Core/src/Action/QrCodeAction.php b/module/Core/src/Action/QrCodeAction.php index 6c645726..a952243a 100644 --- a/module/Core/src/Action/QrCodeAction.php +++ b/module/Core/src/Action/QrCodeAction.php @@ -18,13 +18,13 @@ use Shlinkio\Shlink\Core\ShortUrl\Helper\ShortUrlStringifierInterface; use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\ShortUrl\ShortUrlResolverInterface; -class QrCodeAction implements MiddlewareInterface +readonly class QrCodeAction implements MiddlewareInterface { public function __construct( private ShortUrlResolverInterface $urlResolver, private ShortUrlStringifierInterface $stringifier, private LoggerInterface $logger, - private QrCodeOptions $defaultOptions, + private QrCodeOptions $options, ) { } @@ -33,13 +33,15 @@ class QrCodeAction implements MiddlewareInterface $identifier = ShortUrlIdentifier::fromRedirectRequest($request); try { - $shortUrl = $this->urlResolver->resolveEnabledShortUrl($identifier); + $shortUrl = $this->options->enabledForDisabledShortUrls + ? $this->urlResolver->resolvePublicShortUrl($identifier) + : $this->urlResolver->resolveEnabledShortUrl($identifier); } catch (ShortUrlNotFoundException $e) { $this->logger->warning('An error occurred while creating QR code. {e}', ['e' => $e]); return $handler->handle($request); } - $params = QrCodeParams::fromRequest($request, $this->defaultOptions); + $params = QrCodeParams::fromRequest($request, $this->options); $qrCodeBuilder = Builder::create() ->data($this->stringifier->stringify($shortUrl)) ->size($params->size) diff --git a/module/Core/src/Config/EnvVars.php b/module/Core/src/Config/EnvVars.php index 790bfe3a..ff64838b 100644 --- a/module/Core/src/Config/EnvVars.php +++ b/module/Core/src/Config/EnvVars.php @@ -43,6 +43,7 @@ enum EnvVars: string case DEFAULT_QR_CODE_FORMAT = 'DEFAULT_QR_CODE_FORMAT'; case DEFAULT_QR_CODE_ERROR_CORRECTION = 'DEFAULT_QR_CODE_ERROR_CORRECTION'; case DEFAULT_QR_CODE_ROUND_BLOCK_SIZE = 'DEFAULT_QR_CODE_ROUND_BLOCK_SIZE'; + case QR_CODE_FOR_DISABLED_SHORT_URLS = 'QR_CODE_FOR_DISABLED_SHORT_URLS'; case DEFAULT_INVALID_SHORT_URL_REDIRECT = 'DEFAULT_INVALID_SHORT_URL_REDIRECT'; case DEFAULT_REGULAR_404_REDIRECT = 'DEFAULT_REGULAR_404_REDIRECT'; case DEFAULT_BASE_URL_REDIRECT = 'DEFAULT_BASE_URL_REDIRECT'; diff --git a/module/Core/src/Options/QrCodeOptions.php b/module/Core/src/Options/QrCodeOptions.php index 1b10c280..fff27858 100644 --- a/module/Core/src/Options/QrCodeOptions.php +++ b/module/Core/src/Options/QrCodeOptions.php @@ -4,20 +4,22 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Options; +use const Shlinkio\Shlink\DEFAULT_QR_CODE_ENABLED_FOR_DISABLED_SHORT_URLS; use const Shlinkio\Shlink\DEFAULT_QR_CODE_ERROR_CORRECTION; use const Shlinkio\Shlink\DEFAULT_QR_CODE_FORMAT; use const Shlinkio\Shlink\DEFAULT_QR_CODE_MARGIN; use const Shlinkio\Shlink\DEFAULT_QR_CODE_ROUND_BLOCK_SIZE; use const Shlinkio\Shlink\DEFAULT_QR_CODE_SIZE; -final class QrCodeOptions +readonly final class QrCodeOptions { public function __construct( - public readonly int $size = DEFAULT_QR_CODE_SIZE, - public readonly int $margin = DEFAULT_QR_CODE_MARGIN, - public readonly string $format = DEFAULT_QR_CODE_FORMAT, - public readonly string $errorCorrection = DEFAULT_QR_CODE_ERROR_CORRECTION, - public readonly bool $roundBlockSize = DEFAULT_QR_CODE_ROUND_BLOCK_SIZE, + public int $size = DEFAULT_QR_CODE_SIZE, + public int $margin = DEFAULT_QR_CODE_MARGIN, + public string $format = DEFAULT_QR_CODE_FORMAT, + public string $errorCorrection = DEFAULT_QR_CODE_ERROR_CORRECTION, + public bool $roundBlockSize = DEFAULT_QR_CODE_ROUND_BLOCK_SIZE, + public bool $enabledForDisabledShortUrls = DEFAULT_QR_CODE_ENABLED_FOR_DISABLED_SHORT_URLS, ) { } } diff --git a/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php b/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php index 78becbed..7ec19df6 100644 --- a/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php +++ b/module/Core/src/ShortUrl/Model/ShortUrlIdentifier.php @@ -10,9 +10,9 @@ use Symfony\Component\Console\Input\InputInterface; use function sprintf; -final class ShortUrlIdentifier +final readonly class ShortUrlIdentifier { - private function __construct(public readonly string $shortCode, public readonly ?string $domain = null) + private function __construct(public string $shortCode, public ?string $domain = null) { } diff --git a/module/Core/src/ShortUrl/ShortUrlResolver.php b/module/Core/src/ShortUrl/ShortUrlResolver.php index 2c4f7bdc..4fd0d015 100644 --- a/module/Core/src/ShortUrl/ShortUrlResolver.php +++ b/module/Core/src/ShortUrl/ShortUrlResolver.php @@ -12,11 +12,11 @@ use Shlinkio\Shlink\Core\ShortUrl\Model\ShortUrlIdentifier; use Shlinkio\Shlink\Core\ShortUrl\Repository\ShortUrlRepository; use Shlinkio\Shlink\Rest\Entity\ApiKey; -class ShortUrlResolver implements ShortUrlResolverInterface +readonly class ShortUrlResolver implements ShortUrlResolverInterface { public function __construct( - private readonly EntityManagerInterface $em, - private readonly UrlShortenerOptions $urlShortenerOptions, + private EntityManagerInterface $em, + private UrlShortenerOptions $urlShortenerOptions, ) { } @@ -39,11 +39,21 @@ class ShortUrlResolver implements ShortUrlResolverInterface * @throws ShortUrlNotFoundException */ public function resolveEnabledShortUrl(ShortUrlIdentifier $identifier): ShortUrl + { + $shortUrl = $this->resolvePublicShortUrl($identifier); + if (! $shortUrl->isEnabled()) { + throw ShortUrlNotFoundException::fromNotFound($identifier); + } + + return $shortUrl; + } + + public function resolvePublicShortUrl(ShortUrlIdentifier $identifier): ShortUrl { /** @var ShortUrlRepository $shortUrlRepo */ $shortUrlRepo = $this->em->getRepository(ShortUrl::class); $shortUrl = $shortUrlRepo->findOneWithDomainFallback($identifier, $this->urlShortenerOptions->mode); - if (! $shortUrl?->isEnabled()) { + if ($shortUrl === null) { throw ShortUrlNotFoundException::fromNotFound($identifier); } diff --git a/module/Core/src/ShortUrl/ShortUrlResolverInterface.php b/module/Core/src/ShortUrl/ShortUrlResolverInterface.php index f92038c3..9dd522c0 100644 --- a/module/Core/src/ShortUrl/ShortUrlResolverInterface.php +++ b/module/Core/src/ShortUrl/ShortUrlResolverInterface.php @@ -17,6 +17,19 @@ interface ShortUrlResolverInterface public function resolveShortUrl(ShortUrlIdentifier $identifier, ?ApiKey $apiKey = null): ShortUrl; /** + * Resolves a public short URL matching provided identifier. + * When trying to match public short URLs, if provided domain is default one, it gets ignored. + * If provided domain is not default, but the short code is found in default domain, we fall back to that short URL. + * + * @throws ShortUrlNotFoundException + */ + public function resolvePublicShortUrl(ShortUrlIdentifier $identifier): ShortUrl; + + /** + * Resolves a public short URL matching provided identifier, only if it's not disabled. + * Disabled short URLs are those which received the max amount of visits, have a `validSince` in the future or have + * a `validUntil` in the past. + * * @throws ShortUrlNotFoundException */ public function resolveEnabledShortUrl(ShortUrlIdentifier $identifier): ShortUrl; From 639329dbe4438f373480156d863997b2ecbc43f1 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 24 Dec 2023 09:48:44 +0100 Subject: [PATCH 2/6] Update installer --- composer.json | 2 +- config/autoload/installer.global.php | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index db0fa4c3..3cb28ddc 100644 --- a/composer.json +++ b/composer.json @@ -49,7 +49,7 @@ "shlinkio/shlink-config": "^2.5", "shlinkio/shlink-event-dispatcher": "^3.1", "shlinkio/shlink-importer": "^5.2.1", - "shlinkio/shlink-installer": "^8.6.1", + "shlinkio/shlink-installer": "dev-develop#62aae8d as 8.7", "shlinkio/shlink-ip-geolocation": "^3.4", "shlinkio/shlink-json": "^1.1", "spiral/roadrunner": "^2023.2", diff --git a/config/autoload/installer.global.php b/config/autoload/installer.global.php index 32f71ea6..45f92153 100644 --- a/config/autoload/installer.global.php +++ b/config/autoload/installer.global.php @@ -62,6 +62,7 @@ return [ Option\QrCode\DefaultFormatConfigOption::class, Option\QrCode\DefaultErrorCorrectionConfigOption::class, Option\QrCode\DefaultRoundBlockSizeConfigOption::class, + Option\QrCode\EnabledForDisabledShortUrlsConfigOption::class, Option\RabbitMq\RabbitMqEnabledConfigOption::class, Option\RabbitMq\RabbitMqHostConfigOption::class, Option\RabbitMq\RabbitMqUseSslConfigOption::class, From c5977389153277ec986cd682d838f5fc40808b8f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 24 Dec 2023 10:13:19 +0100 Subject: [PATCH 3/6] Test how URLs are resolved in QrCodeAction --- module/Core/test/Action/QrCodeActionTest.php | 29 ++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/module/Core/test/Action/QrCodeActionTest.php b/module/Core/test/Action/QrCodeActionTest.php index f6850c1f..9a89ff47 100644 --- a/module/Core/test/Action/QrCodeActionTest.php +++ b/module/Core/test/Action/QrCodeActionTest.php @@ -230,6 +230,35 @@ class QrCodeActionTest extends TestCase ]; } + #[Test, DataProvider('provideEnabled')] + public function qrCodeIsResolvedBasedOnOptions(bool $enabledForDisabledShortUrls): void + { + + if ($enabledForDisabledShortUrls) { + $this->urlResolver->expects($this->once())->method('resolvePublicShortUrl')->willThrowException( + ShortUrlNotFoundException::fromNotFound(ShortUrlIdentifier::fromShortCodeAndDomain('')), + ); + $this->urlResolver->expects($this->never())->method('resolveEnabledShortUrl'); + } else { + $this->urlResolver->expects($this->once())->method('resolveEnabledShortUrl')->willThrowException( + ShortUrlNotFoundException::fromNotFound(ShortUrlIdentifier::fromShortCodeAndDomain('')), + ); + $this->urlResolver->expects($this->never())->method('resolvePublicShortUrl'); + } + + $options = new QrCodeOptions(enabledForDisabledShortUrls: $enabledForDisabledShortUrls); + $this->action($options)->process( + ServerRequestFactory::fromGlobals(), + $this->createMock(RequestHandlerInterface::class), + ); + } + + public static function provideEnabled(): iterable + { + yield 'always enabled' => [true]; + yield 'only enabled short URLs' => [false]; + } + public function action(?QrCodeOptions $options = null): QrCodeAction { return new QrCodeAction( From 8d1776af98082b8a02a89bf6b16e4e23d9a16629 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 24 Dec 2023 10:25:58 +0100 Subject: [PATCH 4/6] Test error when short URLs cannot be resolved --- .../test/ShortUrl/ShortUrlResolverTest.php | 28 +++++++++++++++++-- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/module/Core/test/ShortUrl/ShortUrlResolverTest.php b/module/Core/test/ShortUrl/ShortUrlResolverTest.php index a95426ba..729302c9 100644 --- a/module/Core/test/ShortUrl/ShortUrlResolverTest.php +++ b/module/Core/test/ShortUrl/ShortUrlResolverTest.php @@ -59,7 +59,7 @@ class ShortUrlResolverTest extends TestCase } #[Test, DataProviderExternal(ApiKeyDataProviders::class, 'adminApiKeysProvider')] - public function exceptionIsThrownIfShortcodeIsNotFound(?ApiKey $apiKey): void + public function exceptionIsThrownIfShortCodeIsNotFound(?ApiKey $apiKey): void { $shortCode = 'abc123'; $identifier = ShortUrlIdentifier::fromShortCodeAndDomain($shortCode); @@ -73,7 +73,7 @@ class ShortUrlResolverTest extends TestCase } #[Test] - public function shortCodeToEnabledShortUrlProperlyParsesShortCode(): void + public function resolveEnabledShortUrlProperlyParsesShortCode(): void { $shortUrl = ShortUrl::withLongUrl('https://expected_url'); $shortCode = $shortUrl->getShortCode(); @@ -89,8 +89,30 @@ class ShortUrlResolverTest extends TestCase self::assertSame($shortUrl, $result); } + #[Test, DataProvider('provideResolutionMethods')] + public function resolutionThrowsExceptionIfUrlIsNotEnabled(string $method): void + { + $shortCode = 'abc123'; + + $this->repo->expects($this->once())->method('findOneWithDomainFallback')->with( + ShortUrlIdentifier::fromShortCodeAndDomain($shortCode), + ShortUrlMode::STRICT, + )->willReturn(null); + $this->em->expects($this->once())->method('getRepository')->with(ShortUrl::class)->willReturn($this->repo); + + $this->expectException(ShortUrlNotFoundException::class); + + $this->urlResolver->{$method}(ShortUrlIdentifier::fromShortCodeAndDomain($shortCode)); + } + + public static function provideResolutionMethods(): iterable + { + yield 'resolveEnabledShortUrl' => ['resolveEnabledShortUrl']; + yield 'resolvePublicShortUrl' => ['resolvePublicShortUrl']; + } + #[Test, DataProvider('provideDisabledShortUrls')] - public function shortCodeToEnabledShortUrlThrowsExceptionIfUrlIsNotEnabled(ShortUrl $shortUrl): void + public function resolveEnabledShortUrlThrowsExceptionIfUrlIsNotEnabled(ShortUrl $shortUrl): void { $shortCode = $shortUrl->getShortCode(); From 0f0301ae5cdd3097b2b199506a2628606448298e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 24 Dec 2023 10:27:25 +0100 Subject: [PATCH 5/6] Update changelog --- CHANGELOG.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 61c182dc..4744e4d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,23 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org). +## [Unreleased] +### Added +* *Nothing* + +### Changed +* *Nothing* + +### Deprecated +* *Nothing* + +### Removed +* *Nothing* + +### Fixed +* [#1960](https://github.com/shlinkio/shlink/issues/1960) Allow QR codes to be optionally resolved even when corresponding short URL is not enabled. + + ## [3.7.1] - 2023-12-17 ### Added * *Nothing* From e12bda3f42987ebe28954defdc24c42c0c57ecba Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 24 Dec 2023 10:37:09 +0100 Subject: [PATCH 6/6] Add API test to verify QR codes return a 404 for disabled short URLs --- module/Core/test-api/Action/QrCodeTest.php | 27 ++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 module/Core/test-api/Action/QrCodeTest.php diff --git a/module/Core/test-api/Action/QrCodeTest.php b/module/Core/test-api/Action/QrCodeTest.php new file mode 100644 index 00000000..955e6c7e --- /dev/null +++ b/module/Core/test-api/Action/QrCodeTest.php @@ -0,0 +1,27 @@ +callShortUrl('custom/qr-code'); + self::assertEquals(200, $response->getStatusCode()); + + // This short URL allow max 2 visits + $this->callShortUrl('custom'); + $this->callShortUrl('custom'); + + // After 2 visits, the QR code should return a 404 + $response = $this->callShortUrl('custom/qr-code'); + self::assertEquals(404, $response->getStatusCode()); + } +}