diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f034cd0..7d940e7d 100644 --- 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. diff --git a/README.md b/README.md index 3a7373b2..873f83ec 100644 --- 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). diff --git a/data/migrations/Version20210102174433.php b/data/migrations/Version20210102174433.php index 95ee62fe..835fcbda 100644 --- 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, [ diff --git a/data/migrations/Version20210118153932.php b/data/migrations/Version20210118153932.php new file mode 100644 index 00000000..e17ff533 --- /dev/null +++ b/data/migrations/Version20210118153932.php @@ -0,0 +1,26 @@ +getTable('api_key_roles'); + $nameColumn = $rolesTable->getColumn('role_name'); + $nameColumn->setLength(255); + } + + public function down(Schema $schema): void + { + } +} diff --git a/module/Rest/config/auth.config.php b/module/Rest/config/auth.config.php index 0779502f..8f406071 100644 --- 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', + ], ], ]; diff --git a/module/Rest/config/dependencies.config.php b/module/Rest/config/dependencies.config.php index c2181f70..dc960fb4 100644 --- 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], diff --git a/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKeyRole.php b/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKeyRole.php index 9c6355e3..b7787b1a 100644 --- 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(); diff --git a/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php b/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php index e9edee41..b8bd86aa 100644 --- 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), ])); diff --git a/module/Rest/src/Exception/MissingAuthenticationException.php b/module/Rest/src/Exception/MissingAuthenticationException.php index 9cef9a2b..4e1057bc 100644 --- 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; } diff --git a/module/Rest/src/Middleware/AuthenticationMiddleware.php b/module/Rest/src/Middleware/AuthenticationMiddleware.php index 1eff50d2..9d75bfc4 100644 --- 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; + } } diff --git a/module/Rest/test-api/Action/SingleStepCreateShortUrlTest.php b/module/Rest/test-api/Action/SingleStepCreateShortUrlTest.php new file mode 100644 index 00000000..0c5ab1b4 --- /dev/null +++ b/module/Rest/test-api/Action/SingleStepCreateShortUrlTest.php @@ -0,0 +1,56 @@ +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 ' 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]); + } +} diff --git a/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php b/module/Rest/test/Action/ShortUrl/SingleStepCreateShortUrlActionTest.php index b42b95fb..0973a198 100644 --- 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(); } } diff --git a/module/Rest/test/Exception/MissingAuthenticationExceptionTest.php b/module/Rest/test/Exception/MissingAuthenticationExceptionTest.php index afe2a54e..1b7730b5 100644 --- 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()); + } } diff --git a/module/Rest/test/Middleware/AuthenticationMiddlewareTest.php b/module/Rest/test/Middleware/AuthenticationMiddlewareTest.php index 39559f67..015e38e8 100644 --- 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 */