Merge pull request #974 from shlinkio/develop

Release 2.5.1
2024-12-23 15:40:33 -06:00 · 2021-01-21 20:10:57 +01:00 · 2021-01-21 20:10:57 +01:00 · f57303f8c0
commit f57303f8c0
parent 60cdd8b198 2eff9929d8
14 changed files with 225 additions and 82 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -4,6 +4,24 @@ 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).

+## [2.5.1] - 2021-01-21
+### Added
+* *Nothing*
+
+### Changed
+* *Nothing*
+
+### Deprecated
+* *Nothing*
+
+### Removed
+* *Nothing*
+
+### Fixed
+* [#968](https://github.com/shlinkio/shlink/issues/968) Fixed index error in MariaDB while updating to v2.5.0.
+* [#972](https://github.com/shlinkio/shlink/issues/972) Fixed 500 error when calling single-step short URL creation endpoint.
+
+
 ## [2.5.0] - 2021-01-17
 ### Added
 * [#795](https://github.com/shlinkio/shlink/issues/795) and [#882](https://github.com/shlinkio/shlink/issues/882) Added new roles system to API keys.
--- a/README.md
+++ b/README.md
@ -34,6 +34,8 @@ The idea is that you can just generate a container using the image and provide t
 First, make sure the host where you are going to run shlink fulfills these requirements:

 * PHP 7.4 with JSON, curl, PDO, intl and gd extensions enabled (PHP 8.0 support is coming).
+    * apcu extension is recommended if you don't plan to use swoole.
+    * xml extension is required if you want to generate QR codes in svg format.
 * MySQL, MariaDB, PostgreSQL, Microsoft SQL Server or SQLite.
 * The web server of your choice with PHP integration (Apache or Nginx recommended).

--- a/data/migrations/Version20210102174433.php
+++ b/data/migrations/Version20210102174433.php
@ -25,7 +25,7 @@ final class Version20210102174433 extends AbstractMigration
        $table->setPrimaryKey(['id']);

        $table->addColumn('role_name', Types::STRING, [
-            'length' => 256,
+            'length' => 255,
            'notnull' => true,
        ]);
        $table->addColumn('meta', Types::JSON, [
--- a/data/migrations/Version20210118153932.php
+++ b/data/migrations/Version20210118153932.php
@ -0,0 +1,26 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkMigrations;
+
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+final class Version20210118153932 extends AbstractMigration
+{
+    public function up(Schema $schema): void
+    {
+        // Prev migration used to set the length to 256, which made some set-ups crash
+        // It has been updated to 255, and this migration ensures whoever managed to run the prev one, gets the value
+        // also updated to 255
+
+        $rolesTable = $schema->getTable('api_key_roles');
+        $nameColumn = $rolesTable->getColumn('role_name');
+        $nameColumn->setLength(255);
+    }
+
+    public function down(Schema $schema): void
+    {
+    }
+}
--- a/module/Rest/config/auth.config.php
+++ b/module/Rest/config/auth.config.php
@ -11,9 +11,12 @@ return [
    'auth' => [
        'routes_whitelist' => [
            Action\HealthAction::class,
-            Action\ShortUrl\SingleStepCreateShortUrlAction::class,
            ConfigProvider::UNVERSIONED_HEALTH_ENDPOINT_NAME,
        ],
+
+        'routes_with_query_api_key' => [
+            Action\ShortUrl\SingleStepCreateShortUrlAction::class,
+        ],
    ],

    'dependencies' => [
@ -23,7 +26,11 @@ return [
    ],

    ConfigAbstractFactory::class => [
-        Middleware\AuthenticationMiddleware::class => [Service\ApiKeyService::class, 'config.auth.routes_whitelist'],
+        Middleware\AuthenticationMiddleware::class => [
+            Service\ApiKeyService::class,
+            'config.auth.routes_whitelist',
+            'config.auth.routes_with_query_api_key',
+        ],
    ],

 ];
--- a/module/Rest/config/dependencies.config.php
+++ b/module/Rest/config/dependencies.config.php
@ -57,7 +57,6 @@ return [
        Action\ShortUrl\CreateShortUrlAction::class => [Service\UrlShortener::class, 'config.url_shortener.domain'],
        Action\ShortUrl\SingleStepCreateShortUrlAction::class => [
            Service\UrlShortener::class,
-            ApiKeyService::class,
            'config.url_shortener.domain',
        ],
        Action\ShortUrl\EditShortUrlAction::class => [Service\ShortUrlService::class],
--- a/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKeyRole.php
+++ b/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKeyRole.php
@ -24,7 +24,7 @@ return static function (ClassMetadata $metadata, array $emConfig): void {

    $builder->createField('roleName', Types::STRING)
            ->columnName('role_name')
-            ->length(256)
+            ->length(255)
            ->nullable(false)
            ->build();

--- a/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php
+++ b/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php
@ -8,49 +8,28 @@ use Psr\Http\Message\ServerRequestInterface as Request;
 use Shlinkio\Shlink\Core\Exception\ValidationException;
 use Shlinkio\Shlink\Core\Model\CreateShortUrlData;
 use Shlinkio\Shlink\Core\Model\ShortUrlMeta;
-use Shlinkio\Shlink\Core\Service\UrlShortenerInterface;
 use Shlinkio\Shlink\Core\Validation\ShortUrlMetaInputFilter;
-use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface;
+use Shlinkio\Shlink\Rest\Middleware\AuthenticationMiddleware;

 class SingleStepCreateShortUrlAction extends AbstractCreateShortUrlAction
 {
    protected const ROUTE_PATH = '/short-urls/shorten';
    protected const ROUTE_ALLOWED_METHODS = [self::METHOD_GET];

-    private ApiKeyServiceInterface $apiKeyService;
-
-    public function __construct(
-        UrlShortenerInterface $urlShortener,
-        ApiKeyServiceInterface $apiKeyService,
-        array $domainConfig
-    ) {
-        parent::__construct($urlShortener, $domainConfig);
-        $this->apiKeyService = $apiKeyService;
-    }
-
-    /**
-     * @throws ValidationException
-     */
    protected function buildShortUrlData(Request $request): CreateShortUrlData
    {
        $query = $request->getQueryParams();
        $longUrl = $query['longUrl'] ?? null;

-        $apiKeyResult = $this->apiKeyService->check($query['apiKey'] ?? '');
-        if (! $apiKeyResult->isValid()) {
-            throw ValidationException::fromArray([
-                'apiKey' => 'No API key was provided or it is not valid',
-            ]);
-        }
-
        if ($longUrl === null) {
            throw ValidationException::fromArray([
                'longUrl' => 'A URL was not provided',
            ]);
        }

+        $apiKey = AuthenticationMiddleware::apiKeyFromRequest($request);
        return new CreateShortUrlData($longUrl, [], ShortUrlMeta::fromRawData([
-            ShortUrlMetaInputFilter::API_KEY => $apiKeyResult->apiKey(),
+            ShortUrlMetaInputFilter::API_KEY => $apiKey,
            // This will usually be null, unless this API key enforces one specific domain
            ShortUrlMetaInputFilter::DOMAIN => $request->getAttribute(ShortUrlMetaInputFilter::DOMAIN),
        ]));
--- a/module/Rest/src/Exception/MissingAuthenticationException.php
+++ b/module/Rest/src/Exception/MissingAuthenticationException.php
@ -18,18 +18,36 @@ class MissingAuthenticationException extends RuntimeException implements Problem
    private const TITLE = 'Invalid authorization';
    private const TYPE = 'INVALID_AUTHORIZATION';

-    public static function fromExpectedTypes(array $expectedTypes): self
+    public static function forHeaders(array $expectedHeaders): self
    {
-        $e = new self(sprintf(
+        $e = self::withMessage(sprintf(
            'Expected one of the following authentication headers, ["%s"], but none were provided',
-            implode('", "', $expectedTypes),
+            implode('", "', $expectedHeaders),
        ));
+        $e->additional = [
+            'expectedTypes' => $expectedHeaders, // Deprecated
+            'expectedHeaders' => $expectedHeaders,
+        ];

-        $e->detail = $e->getMessage();
+        return $e;
+    }
+
+    public static function forQueryParam(string $param): self
+    {
+        $e = self::withMessage(sprintf('Expected authentication to be provided in "%s" query param', $param));
+        $e->additional = ['param' => $param];
+
+        return $e;
+    }
+
+    private static function withMessage(string $message): self
+    {
+        $e = new self($message);
+
+        $e->detail = $message;
        $e->title = self::TITLE;
        $e->type = self::TYPE;
        $e->status = StatusCodeInterface::STATUS_UNAUTHORIZED;
-        $e->additional = ['expectedTypes' => $expectedTypes];

        return $e;
    }
--- a/module/Rest/src/Middleware/AuthenticationMiddleware.php
+++ b/module/Rest/src/Middleware/AuthenticationMiddleware.php
@ -8,6 +8,7 @@ use Fig\Http\Message\RequestMethodInterface;
 use Fig\Http\Message\StatusCodeInterface;
 use Mezzio\Router\RouteResult;
 use Psr\Http\Message\ResponseInterface as Response;
+use Psr\Http\Message\ServerRequestInterface;
 use Psr\Http\Message\ServerRequestInterface as Request;
 use Psr\Http\Server\MiddlewareInterface;
 use Psr\Http\Server\RequestHandlerInterface;
@ -24,11 +25,16 @@ class AuthenticationMiddleware implements MiddlewareInterface, StatusCodeInterfa

    private ApiKeyServiceInterface $apiKeyService;
    private array $routesWhitelist;
+    private array $routesWithQueryApiKey;

-    public function __construct(ApiKeyServiceInterface $apiKeyService, array $routesWhitelist)
-    {
+    public function __construct(
+        ApiKeyServiceInterface $apiKeyService,
+        array $routesWhitelist,
+        array $routesWithQueryApiKey
+    ) {
        $this->apiKeyService = $apiKeyService;
        $this->routesWhitelist = $routesWhitelist;
+        $this->routesWithQueryApiKey = $routesWithQueryApiKey;
    }

    public function process(Request $request, RequestHandlerInterface $handler): Response
@ -44,11 +50,7 @@ class AuthenticationMiddleware implements MiddlewareInterface, StatusCodeInterfa
            return $handler->handle($request);
        }

-        $apiKey = $request->getHeaderLine(self::API_KEY_HEADER);
-        if (empty($apiKey)) {
-            throw MissingAuthenticationException::fromExpectedTypes([self::API_KEY_HEADER]);
-        }
-
+        $apiKey = $this->getApiKeyFromRequest($request, $routeResult);
        $result = $this->apiKeyService->check($apiKey);
        if (! $result->isValid()) {
            throw VerifyAuthenticationException::forInvalidApiKey();
@ -61,4 +63,20 @@ class AuthenticationMiddleware implements MiddlewareInterface, StatusCodeInterfa
    {
        return $request->getAttribute(ApiKey::class);
    }
+
+    private function getApiKeyFromRequest(ServerRequestInterface $request, RouteResult $routeResult): string
+    {
+        $routeName = $routeResult->getMatchedRouteName();
+        $query = $request->getQueryParams();
+        $isRouteWithApiKeyInQuery = contains($this->routesWithQueryApiKey, $routeName);
+        $apiKey = $isRouteWithApiKeyInQuery ? ($query['apiKey'] ?? '') : $request->getHeaderLine(self::API_KEY_HEADER);
+
+        if (empty($apiKey)) {
+            throw $isRouteWithApiKeyInQuery
+                ? MissingAuthenticationException::forQueryParam('apiKey')
+                : MissingAuthenticationException::forHeaders([self::API_KEY_HEADER]);
+        }
+
+        return $apiKey;
+    }
 }
--- a/module/Rest/test-api/Action/SingleStepCreateShortUrlTest.php
+++ b/module/Rest/test-api/Action/SingleStepCreateShortUrlTest.php
@ -0,0 +1,56 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioApiTest\Shlink\Rest\Action;
+
+use GuzzleHttp\RequestOptions;
+use Psr\Http\Message\ResponseInterface;
+use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase;
+
+class SingleStepCreateShortUrlTest extends ApiTestCase
+{
+    /**
+     * @test
+     * @dataProvider provideFormats
+     */
+    public function createsNewShortUrlWithExpectedResponse(?string $format, string $expectedContentType): void
+    {
+        $resp = $this->createShortUrl($format, 'valid_api_key');
+
+        self::assertEquals(self::STATUS_OK, $resp->getStatusCode());
+        self::assertEquals($expectedContentType, $resp->getHeaderLine('Content-Type'));
+    }
+
+    public function provideFormats(): iterable
+    {
+        yield 'txt format' => ['txt', 'text/plain'];
+        yield 'json format' => ['json', 'application/json'];
+        yield '<empty> format' => [null, 'application/json'];
+    }
+
+    /** @test */
+    public function authorizationErrorIsReturnedIfNoApiKeyIsSent(): void
+    {
+        $expectedDetail = 'Expected authentication to be provided in "apiKey" query param';
+
+        $resp = $this->createShortUrl();
+        $payload = $this->getJsonResponsePayload($resp);
+
+        self::assertEquals(self::STATUS_UNAUTHORIZED, $resp->getStatusCode());
+        self::assertEquals(self::STATUS_UNAUTHORIZED, $payload['status']);
+        self::assertEquals('INVALID_AUTHORIZATION', $payload['type']);
+        self::assertEquals($expectedDetail, $payload['detail']);
+        self::assertEquals('Invalid authorization', $payload['title']);
+    }
+
+    private function createShortUrl(?string $format = 'json', ?string $apiKey = null): ResponseInterface
+    {
+        $query = [
+            'longUrl' => 'https://app.shlink.io',
+            'apiKey' => $apiKey,
+            'format' => $format,
+        ];
+        return $this->callApi(self::METHOD_GET, '/short-urls/shorten', [RequestOptions::QUERY => $query]);
+    }
+}
--- a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php
+++ b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php
@ -16,8 +16,6 @@ use Shlinkio\Shlink\Core\Model\ShortUrlMeta;
 use Shlinkio\Shlink\Core\Service\UrlShortenerInterface;
 use Shlinkio\Shlink\Rest\Action\ShortUrl\SingleStepCreateShortUrlAction;
 use Shlinkio\Shlink\Rest\Entity\ApiKey;
-use Shlinkio\Shlink\Rest\Service\ApiKeyCheckResult;
-use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface;

 class SingleStepCreateShortUrlActionTest extends TestCase
 {
@ -30,11 +28,9 @@ class SingleStepCreateShortUrlActionTest extends TestCase
    public function setUp(): void
    {
        $this->urlShortener = $this->prophesize(UrlShortenerInterface::class);
-        $this->apiKeyService = $this->prophesize(ApiKeyServiceInterface::class);

        $this->action = new SingleStepCreateShortUrlAction(
            $this->urlShortener->reveal(),
-            $this->apiKeyService->reveal(),
            [
                'schema' => 'http',
                'hostname' => 'foo.com',
@ -42,26 +38,12 @@ class SingleStepCreateShortUrlActionTest extends TestCase
        );
    }

-    /** @test */
-    public function errorResponseIsReturnedIfInvalidApiKeyIsProvided(): void
-    {
-        $request = (new ServerRequest())->withQueryParams(['apiKey' => 'abc123']);
-        $findApiKey = $this->apiKeyService->check('abc123')->willReturn(new ApiKeyCheckResult());
-
-        $this->expectException(ValidationException::class);
-        $findApiKey->shouldBeCalledOnce();
-
-        $this->action->handle($request);
-    }
-
    /** @test */
    public function errorResponseIsReturnedIfNoUrlIsProvided(): void
    {
-        $request = (new ServerRequest())->withQueryParams(['apiKey' => 'abc123']);
-        $findApiKey = $this->apiKeyService->check('abc123')->willReturn(new ApiKeyCheckResult(new ApiKey()));
+        $request = new ServerRequest();

        $this->expectException(ValidationException::class);
-        $findApiKey->shouldBeCalledOnce();

        $this->action->handle($request);
    }
@ -70,13 +52,10 @@ class SingleStepCreateShortUrlActionTest extends TestCase
    public function properDataIsPassedWhenGeneratingShortCode(): void
    {
        $apiKey = new ApiKey();
-        $key = $apiKey->toString();

        $request = (new ServerRequest())->withQueryParams([
-            'apiKey' => $key,
            'longUrl' => 'http://foobar.com',
-        ]);
-        $findApiKey = $this->apiKeyService->check($key)->willReturn(new ApiKeyCheckResult($apiKey));
+        ])->withAttribute(ApiKey::class, $apiKey);
        $generateShortCode = $this->urlShortener->shorten(
            Argument::that(function (string $argument): bool {
                Assert::assertEquals('http://foobar.com', $argument);
@ -89,7 +68,6 @@ class SingleStepCreateShortUrlActionTest extends TestCase
        $resp = $this->action->handle($request);

        self::assertEquals(200, $resp->getStatusCode());
-        $findApiKey->shouldHaveBeenCalled();
        $generateShortCode->shouldHaveBeenCalled();
    }
 }
--- a/module/Rest/test/Exception/MissingAuthenticationExceptionTest.php
+++ b/module/Rest/test/Exception/MissingAuthenticationExceptionTest.php
@ -16,21 +16,22 @@ class MissingAuthenticationExceptionTest extends TestCase
     * @test
     * @dataProvider provideExpectedTypes
     */
-    public function exceptionIsProperlyCreatedFromExpectedTypes(array $expectedTypes): void
+    public function exceptionIsProperlyCreatedFromExpectedHeaders(array $expectedHeaders): void
    {
        $expectedMessage = sprintf(
            'Expected one of the following authentication headers, ["%s"], but none were provided',
-            implode('", "', $expectedTypes),
+            implode('", "', $expectedHeaders),
        );

-        $e = MissingAuthenticationException::fromExpectedTypes($expectedTypes);
+        $e = MissingAuthenticationException::forHeaders($expectedHeaders);

+        $this->assertCommonExceptionShape($e);
        self::assertEquals($expectedMessage, $e->getMessage());
        self::assertEquals($expectedMessage, $e->getDetail());
-        self::assertEquals('Invalid authorization', $e->getTitle());
-        self::assertEquals('INVALID_AUTHORIZATION', $e->getType());
-        self::assertEquals(401, $e->getStatus());
-        self::assertEquals(['expectedTypes' => $expectedTypes], $e->getAdditionalData());
+        self::assertEquals([
+            'expectedTypes' => $expectedHeaders,
+            'expectedHeaders' => $expectedHeaders,
+        ], $e->getAdditionalData());
    }

    public function provideExpectedTypes(): iterable
@ -40,4 +41,34 @@ class MissingAuthenticationExceptionTest extends TestCase
        yield [[]];
        yield [['foo', 'bar', 'baz']];
    }
+
+    /**
+     * @test
+     * @dataProvider provideExpectedParam
+     */
+    public function exceptionIsProperlyCreatedFromExpectedQueryParam(string $param): void
+    {
+        $expectedMessage = sprintf('Expected authentication to be provided in "%s" query param', $param);
+
+        $e = MissingAuthenticationException::forQueryParam($param);
+
+        $this->assertCommonExceptionShape($e);
+        self::assertEquals($expectedMessage, $e->getMessage());
+        self::assertEquals($expectedMessage, $e->getDetail());
+        self::assertEquals(['param' => $param], $e->getAdditionalData());
+    }
+
+    public function provideExpectedParam(): iterable
+    {
+        yield ['foo'];
+        yield ['bar'];
+        yield ['something'];
+    }
+
+    private function assertCommonExceptionShape(MissingAuthenticationException $e): void
+    {
+        self::assertEquals('Invalid authorization', $e->getTitle());
+        self::assertEquals('INVALID_AUTHORIZATION', $e->getType());
+        self::assertEquals(401, $e->getStatus());
+    }
 }
--- a/module/Rest/test/Middleware/AuthenticationMiddlewareTest.php
+++ b/module/Rest/test/Middleware/AuthenticationMiddlewareTest.php
@ -38,7 +38,11 @@ class AuthenticationMiddlewareTest extends TestCase
    public function setUp(): void
    {
        $this->apiKeyService = $this->prophesize(ApiKeyServiceInterface::class);
-        $this->middleware = new AuthenticationMiddleware($this->apiKeyService->reveal(), [HealthAction::class]);
+        $this->middleware = new AuthenticationMiddleware(
+            $this->apiKeyService->reveal(),
+            [HealthAction::class],
+            ['with_query_api_key'],
+        );
        $this->handler = $this->prophesize(RequestHandlerInterface::class);
    }

@ -82,27 +86,34 @@ class AuthenticationMiddlewareTest extends TestCase
     * @test
     * @dataProvider provideRequestsWithoutApiKey
     */
-    public function throwsExceptionWhenNoApiKeyIsProvided(ServerRequestInterface $request): void
-    {
+    public function throwsExceptionWhenNoApiKeyIsProvided(
+        ServerRequestInterface $request,
+        string $expectedMessage
+    ): void {
        $this->apiKeyService->check(Argument::any())->shouldNotBeCalled();
        $this->handler->handle($request)->shouldNotBeCalled();
        $this->expectException(MissingAuthenticationException::class);
-        $this->expectExceptionMessage(
-            'Expected one of the following authentication headers, ["X-Api-Key"], but none were provided',
-        );
+        $this->expectExceptionMessage($expectedMessage);

        $this->middleware->process($request, $this->handler->reveal());
    }

    public function provideRequestsWithoutApiKey(): iterable
    {
-        $baseRequest = ServerRequestFactory::fromGlobals()->withAttribute(
+        $baseRequest = fn (string $routeName) => ServerRequestFactory::fromGlobals()->withAttribute(
            RouteResult::class,
-            RouteResult::fromRoute(new Route('bar', $this->getDummyMiddleware()), []),
+            RouteResult::fromRoute(new Route($routeName, $this->getDummyMiddleware()), []),
        );
+        $apiKeyMessage = 'Expected one of the following authentication headers, ["X-Api-Key"], but none were provided';
+        $queryMessage = 'Expected authentication to be provided in "apiKey" query param';

-        yield 'no api key' => [$baseRequest];
-        yield 'empty api key' => [$baseRequest->withHeader('X-Api-Key', '')];
+        yield 'no api key in header' => [$baseRequest('bar'), $apiKeyMessage];
+        yield 'empty api key in header' => [$baseRequest('bar')->withHeader('X-Api-Key', ''), $apiKeyMessage];
+        yield 'no api key in query' => [$baseRequest('with_query_api_key'), $queryMessage];
+        yield 'empty api key in query' => [
+            $baseRequest('with_query_api_key')->withQueryParams(['apiKey' => '']),
+            $queryMessage,
+        ];
    }

    /** @test */