From ea76092681cf3d9eccadd1f6a8bdd236c3d921d7 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 13 Oct 2017 12:22:19 +0200 Subject: [PATCH] Ensured a generic template is used to render generic 404 errors, and a more specific one to render 'invalid short url' errors --- module/Common/src/Util/ResponseUtilsTrait.php | 7 ++++--- module/Core/src/Action/PreviewAction.php | 8 +++++--- module/Core/src/Action/QrCodeAction.php | 11 +++++++---- module/Core/src/Action/RedirectAction.php | 7 +++++-- .../Action/Util/ErrorResponseBuilderTrait.php | 18 ++++++++++++++++++ module/Core/src/Response/NotFoundDelegate.php | 11 +++++++---- module/Core/templates/error/404.phtml | 6 +++--- .../Core/templates/invalid-short-code.phtml | 19 +++++++++++++++++++ module/Core/test/Action/PreviewActionTest.php | 9 +++++++-- module/Core/test/Action/QrCodeActionTest.php | 9 +++++++-- .../Core/test/Action/RedirectActionTest.php | 5 ++++- 11 files changed, 86 insertions(+), 24 deletions(-) create mode 100644 module/Core/src/Action/Util/ErrorResponseBuilderTrait.php create mode 100644 module/Core/templates/invalid-short-code.phtml diff --git a/module/Common/src/Util/ResponseUtilsTrait.php b/module/Common/src/Util/ResponseUtilsTrait.php index 7c8fc0b6..90e76c56 100644 --- a/module/Common/src/Util/ResponseUtilsTrait.php +++ b/module/Common/src/Util/ResponseUtilsTrait.php @@ -3,13 +3,14 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Common\Util; +use Psr\Http\Message\ResponseInterface; use Zend\Diactoros\Response; use Zend\Diactoros\Stream; use Zend\Stdlib\ArrayUtils; trait ResponseUtilsTrait { - protected function generateDownloadFileResponse($filePath) + protected function generateDownloadFileResponse(string $filePath): ResponseInterface { return $this->generateBinaryResponse($filePath, [ 'Content-Disposition' => 'attachment; filename=' . basename($filePath), @@ -21,12 +22,12 @@ trait ResponseUtilsTrait ]); } - protected function generateImageResponse($imagePath) + protected function generateImageResponse(string $imagePath): ResponseInterface { return $this->generateBinaryResponse($imagePath); } - protected function generateBinaryResponse($path, $extraHeaders = []) + protected function generateBinaryResponse(string $path, array $extraHeaders = []): ResponseInterface { $body = new Stream($path); return new Response($body, 200, ArrayUtils::merge([ diff --git a/module/Core/src/Action/PreviewAction.php b/module/Core/src/Action/PreviewAction.php index 788c3c2d..0e1043a0 100644 --- a/module/Core/src/Action/PreviewAction.php +++ b/module/Core/src/Action/PreviewAction.php @@ -10,6 +10,7 @@ use Psr\Http\Message\ServerRequestInterface as Request; use Shlinkio\Shlink\Common\Exception\PreviewGenerationException; use Shlinkio\Shlink\Common\Service\PreviewGeneratorInterface; use Shlinkio\Shlink\Common\Util\ResponseUtilsTrait; +use Shlinkio\Shlink\Core\Action\Util\ErrorResponseBuilderTrait; use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; @@ -17,6 +18,7 @@ use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; class PreviewAction implements MiddlewareInterface { use ResponseUtilsTrait; + use ErrorResponseBuilderTrait; /** * @var PreviewGeneratorInterface @@ -51,11 +53,11 @@ class PreviewAction implements MiddlewareInterface $imagePath = $this->previewGenerator->generatePreview($url); return $this->generateImageResponse($imagePath); } catch (InvalidShortCodeException $e) { - return $delegate->process($request); + return $this->buildErrorResponse($request, $delegate); } catch (EntityDoesNotExistException $e) { - return $delegate->process($request); + return $this->buildErrorResponse($request, $delegate); } catch (PreviewGenerationException $e) { - return $delegate->process($request); + return $this->buildErrorResponse($request, $delegate); } } } diff --git a/module/Core/src/Action/QrCodeAction.php b/module/Core/src/Action/QrCodeAction.php index dfd7eaf2..30363619 100644 --- a/module/Core/src/Action/QrCodeAction.php +++ b/module/Core/src/Action/QrCodeAction.php @@ -11,6 +11,7 @@ use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; use Shlinkio\Shlink\Common\Response\QrCodeResponse; +use Shlinkio\Shlink\Core\Action\Util\ErrorResponseBuilderTrait; use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; @@ -18,6 +19,8 @@ use Zend\Expressive\Router\RouterInterface; class QrCodeAction implements MiddlewareInterface { + use ErrorResponseBuilderTrait; + /** * @var RouterInterface */ @@ -55,19 +58,19 @@ class QrCodeAction implements MiddlewareInterface // Make sure the short URL exists for this short code $shortCode = $request->getAttribute('shortCode'); try { - $shortUrl = $this->urlShortener->shortCodeToUrl($shortCode); + $this->urlShortener->shortCodeToUrl($shortCode); } catch (InvalidShortCodeException $e) { $this->logger->warning('Tried to create a QR code with an invalid short code' . PHP_EOL . $e); - return $delegate->process($request); + return $this->buildErrorResponse($request, $delegate); } catch (EntityDoesNotExistException $e) { $this->logger->warning('Tried to create a QR code with a not found short code' . PHP_EOL . $e); - return $delegate->process($request); + return $this->buildErrorResponse($request, $delegate); } $path = $this->router->generateUri('long-url-redirect', ['shortCode' => $shortCode]); $size = $this->getSizeParam($request); - $qrCode = new QrCode($request->getUri()->withPath($path)->withQuery('')); + $qrCode = new QrCode((string) $request->getUri()->withPath($path)->withQuery('')); $qrCode->setSize($size) ->setPadding(0); return new QrCodeResponse($qrCode); diff --git a/module/Core/src/Action/RedirectAction.php b/module/Core/src/Action/RedirectAction.php index 99224934..42560e5f 100644 --- a/module/Core/src/Action/RedirectAction.php +++ b/module/Core/src/Action/RedirectAction.php @@ -9,6 +9,7 @@ use Psr\Http\Message\ResponseInterface as Response; use Psr\Http\Message\ServerRequestInterface as Request; use Psr\Log\LoggerInterface; use Psr\Log\NullLogger; +use Shlinkio\Shlink\Core\Action\Util\ErrorResponseBuilderTrait; use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\UrlShortenerInterface; @@ -17,6 +18,8 @@ use Zend\Diactoros\Response\RedirectResponse; class RedirectAction implements MiddlewareInterface { + use ErrorResponseBuilderTrait; + /** * @var UrlShortenerInterface */ @@ -69,9 +72,9 @@ class RedirectAction implements MiddlewareInterface // Use a temporary redirect to make sure browsers always hit the server for analytics purposes return new RedirectResponse($longUrl); } catch (InvalidShortCodeException $e) { - return $delegate->process($request); + return $this->buildErrorResponse($request, $delegate); } catch (EntityDoesNotExistException $e) { - return $delegate->process($request); + return $this->buildErrorResponse($request, $delegate); } } } diff --git a/module/Core/src/Action/Util/ErrorResponseBuilderTrait.php b/module/Core/src/Action/Util/ErrorResponseBuilderTrait.php new file mode 100644 index 00000000..57380b8b --- /dev/null +++ b/module/Core/src/Action/Util/ErrorResponseBuilderTrait.php @@ -0,0 +1,18 @@ +withAttribute(NotFoundDelegate::NOT_FOUND_TEMPLATE, 'ShlinkCore::invalid-short-code'); + return $delegate->process($request); + } +} diff --git a/module/Core/src/Response/NotFoundDelegate.php b/module/Core/src/Response/NotFoundDelegate.php index da0710bb..abe5f06a 100644 --- a/module/Core/src/Response/NotFoundDelegate.php +++ b/module/Core/src/Response/NotFoundDelegate.php @@ -12,6 +12,8 @@ use Zend\Expressive\Template\TemplateRendererInterface; class NotFoundDelegate implements DelegateInterface { + const NOT_FOUND_TEMPLATE = 'notFoundTemplate'; + /** * @var TemplateRendererInterface */ @@ -19,12 +21,12 @@ class NotFoundDelegate implements DelegateInterface /** * @var string */ - private $template; + private $defaultTemplate; - public function __construct(TemplateRendererInterface $renderer, string $template = 'ShlinkCore::error/404') + public function __construct(TemplateRendererInterface $renderer, string $defaultTemplate = 'ShlinkCore::error/404') { $this->renderer = $renderer; - $this->template = $template; + $this->defaultTemplate = $defaultTemplate; } /** @@ -49,6 +51,7 @@ class NotFoundDelegate implements DelegateInterface ], $status); } - return new Response\HtmlResponse($this->renderer->render($this->template, ['request' => $request]), $status); + $notFoundTemplate = $request->getAttribute(self::NOT_FOUND_TEMPLATE, $this->defaultTemplate); + return new Response\HtmlResponse($this->renderer->render($notFoundTemplate, ['request' => $request]), $status); } } diff --git a/module/Core/templates/error/404.phtml b/module/Core/templates/error/404.phtml index 369a168b..94f42c05 100644 --- a/module/Core/templates/error/404.phtml +++ b/module/Core/templates/error/404.phtml @@ -12,8 +12,8 @@ end() ?> start('main') ?> -

translate('Oops!') ?>

+

404

-

translate('This short URL doesn\'t seem to be valid.') ?>

-

translate('Make sure you included all the characters, with no extra punctuation.') ?>

+

translate('Page not found.') ?>

+

translate('The page you requested could not be found.') ?>

end() ?> diff --git a/module/Core/templates/invalid-short-code.phtml b/module/Core/templates/invalid-short-code.phtml new file mode 100644 index 00000000..369a168b --- /dev/null +++ b/module/Core/templates/invalid-short-code.phtml @@ -0,0 +1,19 @@ +layout('ShlinkCore::layout/default') ?> + +start('title') ?> + translate('URL Not Found') ?> +end() ?> + +start('stylesheets') ?> + +end() ?> + +start('main') ?> +

translate('Oops!') ?>

+
+

translate('This short URL doesn\'t seem to be valid.') ?>

+

translate('Make sure you included all the characters, with no extra punctuation.') ?>

+end() ?> diff --git a/module/Core/test/Action/PreviewActionTest.php b/module/Core/test/Action/PreviewActionTest.php index d791a100..8817eaa4 100644 --- a/module/Core/test/Action/PreviewActionTest.php +++ b/module/Core/test/Action/PreviewActionTest.php @@ -6,6 +6,7 @@ namespace ShlinkioTest\Shlink\Core\Action; use Interop\Http\ServerMiddleware\DelegateInterface; use PHPUnit\Framework\TestCase; use Prophecy\Argument; +use Prophecy\Prophecy\MethodProphecy; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Common\Service\PreviewGenerator; use Shlinkio\Shlink\Core\Action\PreviewAction; @@ -13,6 +14,7 @@ use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\UrlShortener; use ShlinkioTest\Shlink\Common\Util\TestUtils; +use Zend\Diactoros\Response; use Zend\Diactoros\ServerRequestFactory; class PreviewActionTest extends TestCase @@ -46,7 +48,8 @@ class PreviewActionTest extends TestCase $this->urlShortener->shortCodeToUrl($shortCode)->willThrow(EntityDoesNotExistException::class) ->shouldBeCalledTimes(1); $delegate = $this->prophesize(DelegateInterface::class); - $delegate->process(Argument::cetera())->shouldBeCalledTimes(1); + $delegate->process(Argument::cetera())->shouldBeCalledTimes(1) + ->willReturn(new Response()); $this->action->process( ServerRequestFactory::fromGlobals()->withAttribute('shortCode', $shortCode), @@ -83,12 +86,14 @@ class PreviewActionTest extends TestCase $this->urlShortener->shortCodeToUrl($shortCode)->willThrow(InvalidShortCodeException::class) ->shouldBeCalledTimes(1); $delegate = $this->prophesize(DelegateInterface::class); + /** @var MethodProphecy $process */ + $process = $delegate->process(Argument::any())->willReturn(new Response()); $this->action->process( ServerRequestFactory::fromGlobals()->withAttribute('shortCode', $shortCode), $delegate->reveal() ); - $delegate->process(Argument::any())->shouldHaveBeenCalledTimes(1); + $process->shouldHaveBeenCalledTimes(1); } } diff --git a/module/Core/test/Action/QrCodeActionTest.php b/module/Core/test/Action/QrCodeActionTest.php index df452420..c071ef7e 100644 --- a/module/Core/test/Action/QrCodeActionTest.php +++ b/module/Core/test/Action/QrCodeActionTest.php @@ -6,12 +6,14 @@ namespace ShlinkioTest\Shlink\Core\Action; use Interop\Http\ServerMiddleware\DelegateInterface; use PHPUnit\Framework\TestCase; use Prophecy\Argument; +use Prophecy\Prophecy\MethodProphecy; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Common\Response\QrCodeResponse; use Shlinkio\Shlink\Core\Action\QrCodeAction; use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; use Shlinkio\Shlink\Core\Exception\InvalidShortCodeException; use Shlinkio\Shlink\Core\Service\UrlShortener; +use Zend\Diactoros\Response; use Zend\Diactoros\ServerRequestFactory; use Zend\Expressive\Router\RouterInterface; @@ -45,13 +47,14 @@ class QrCodeActionTest extends TestCase $this->urlShortener->shortCodeToUrl($shortCode)->willThrow(EntityDoesNotExistException::class) ->shouldBeCalledTimes(1); $delegate = $this->prophesize(DelegateInterface::class); + $process = $delegate->process(Argument::any())->willReturn(new Response()); $this->action->process( ServerRequestFactory::fromGlobals()->withAttribute('shortCode', $shortCode), $delegate->reveal() ); - $delegate->process(Argument::any())->shouldHaveBeenCalledTimes(1); + $process->shouldHaveBeenCalledTimes(1); } /** @@ -63,13 +66,15 @@ class QrCodeActionTest extends TestCase $this->urlShortener->shortCodeToUrl($shortCode)->willThrow(InvalidShortCodeException::class) ->shouldBeCalledTimes(1); $delegate = $this->prophesize(DelegateInterface::class); + /** @var MethodProphecy $process */ + $process = $delegate->process(Argument::any())->willReturn(new Response()); $this->action->process( ServerRequestFactory::fromGlobals()->withAttribute('shortCode', $shortCode), $delegate->reveal() ); - $delegate->process(Argument::any())->shouldHaveBeenCalledTimes(1); + $process->shouldHaveBeenCalledTimes(1); } /** diff --git a/module/Core/test/Action/RedirectActionTest.php b/module/Core/test/Action/RedirectActionTest.php index 49d3baec..a429549f 100644 --- a/module/Core/test/Action/RedirectActionTest.php +++ b/module/Core/test/Action/RedirectActionTest.php @@ -6,6 +6,7 @@ namespace ShlinkioTest\Shlink\Core\Action; use Interop\Http\ServerMiddleware\DelegateInterface; use PHPUnit\Framework\TestCase; use Prophecy\Argument; +use Prophecy\Prophecy\MethodProphecy; use Prophecy\Prophecy\ObjectProphecy; use Shlinkio\Shlink\Core\Action\RedirectAction; use Shlinkio\Shlink\Core\Exception\EntityDoesNotExistException; @@ -62,10 +63,12 @@ class RedirectActionTest extends TestCase $this->urlShortener->shortCodeToUrl($shortCode)->willThrow(EntityDoesNotExistException::class) ->shouldBeCalledTimes(1); $delegate = $this->prophesize(DelegateInterface::class); + /** @var MethodProphecy $process */ + $process = $delegate->process(Argument::any())->willReturn(new Response()); $request = ServerRequestFactory::fromGlobals()->withAttribute('shortCode', $shortCode); $this->action->process($request, $delegate->reveal()); - $delegate->process($request)->shouldHaveBeenCalledTimes(1); + $process->shouldHaveBeenCalledTimes(1); } }