Merge pull request #984 from shlinkio/release/2.5.2

Release/2.5.2

CHANGELOG.md

@@ -4,6 +4,25 @@ 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.2] - 2021-01-24
+### Added
+* [#965](https://github.com/shlinkio/shlink/issues/965) Added docs section for Architectural Decision Records, including the one for API key roles.
+
+### Changed
+* *Nothing*
+
+### Deprecated
+* *Nothing*
+
+### Removed
+* *Nothing*
+
+### Fixed
+* [#979](https://github.com/shlinkio/shlink/issues/979) Added missing `itemsPerPage` query param to swagger docs for short RULs list.
+* [#980](https://github.com/shlinkio/shlink/issues/980) Fixed value used for `Access-Control-Allow-Origin`, that could not work as expected when including an IP address.
+* [#947](https://github.com/shlinkio/shlink/issues/947) Fixed incorrect value returned in `Access-Control-Allow-Methods` header, which always contained all methods.
+
+
 ## [2.5.1] - 2021-01-21
 ### Added
 * *Nothing*

CONTRIBUTING.md

@@ -46,27 +46,28 @@ This is a simplified version of the project structure:
 ```
 shlink
 ├── bin
-│   ├── cli
+│   ├── cli
-│   ├── install
+│   ├── install
-│   └── update
+│   └── update
 ├── config
-│   ├── autoload
+│   ├── autoload
-│   ├── params
+│   ├── params
-│   ├── config.php
+│   ├── config.php
-│   └── container.php
+│   └── container.php
 ├── data
-│   ├── cache
+│   ├── cache
-│   ├── locks
+│   ├── locks
-│   ├── log
+│   ├── log
-│   ├── migrations
+│   ├── migrations
-│   └── proxies
+│   └── proxies
 ├── docs
-│   ├── async-api
+│   ├── adr
-│   └── swagger
+│   ├── async-api
+│   └── swagger
 ├── module
-│   ├── CLI
+│   ├── CLI
-│   ├── Core
+│   ├── Core
-│   └── Rest
+│   └── Rest
 ├── public
 ├── composer.json
 └── README.md
@@ -77,7 +78,7 @@ The purposes of every folder are:
 * `bin`: It contains the CLI tools. The `cli` one is the main entry point to run shlink from the command line, while `install` and `update` are helper tools used to install and update shlink when not using the docker image.
 * `config`: Contains application-wide configurations, which are later merged with the ones provided by every module.
 * `data`: Common runtime-generated git-ignored assets, like logs, caches, etc.
-* `docs`: Any project documentation is stored here, like API spec definitions.
+* `docs`: Any project documentation is stored here, like API spec definitions or architectural decision records.
 * `module`: Contains a subfolder for every module in the project. Modules contain the source code, tests and configurations for every context in the project.
 * `public`: Few assets (like `favicon.ico` or `robots.txt`) and the web entry point are stored here. This web entry point is not used when serving the app with swoole.
 
@@ -134,3 +135,9 @@ In order to provide pull requests to this project, you should always start by cr
 The base branch should always be `develop`, and the target branch for the pull request should also be `develop`.
 
 Before your branch can be merged, all the checks described in [Running code checks](#running-code-checks) have to be passing. You can verify that manually by running `./indocker composer ci`, or wait for the build to be run automatically after the pull request is created.
+
+## Architectural Decision Records
+
+The project includes logs for some architectural decisions, using the [adr](https://adr.github.io/) proposal.
+
+If you are curious or want to understand why something has been built in some specific way, [take a look at them](docs/adr).

docs/adr/2021-01-17-support-restrictions-and-permissions-in-api-keys.md

@@ -0,0 +1,50 @@
+# Support restrictions and permissions in API keys
+
+* Status: Accepted
+* Date: 2021-01-17
+
+## Context and problem statement
+
+Historically, every API key generated for Shlink granted you access to all existing resources.
+
+The intention is to be able to apply some form of restriction to API keys, so that only a subset of "resources" can be accessed with it, naming:
+
+* Allowing interactions only with short URLs and related resources, that have been created with the same API key.
+* Allowing interactions only with short URLs and related resources, that have been attached to a specific domain.
+
+The intention is to implement a system that allows adding to API keys as many of these restrictions as wanted.
+
+Supporting more restrictions in the future is also desirable.
+
+## Considered option
+
+* Using an ACL/RBAC library, and checking roles in a middleware.
+* Using a service that, provided an API key, tells if certain resource is reachable while it also allows building queries dynamically.
+* Using some library implementing the specification pattern, to dynamically build queries transparently for outer layers.
+
+## Decision outcome
+
+The main difficulty on implementing this is that the entity conditioning the behavior (the API key) comes in the request in some form, but it can potentially affect database queries performed in the persistence layer.
+
+Because of this, it has to traverse all the application layers from top to bottom, in most of the cases.
+
+This motivated selecting the third option, as we can propagate the API key and delay its handling to the last step, without changing the behavior of the rest of the layers that much (except in some individual use cases).
+
+The domain term used to refer these "restrictions" is finally **roles**.
+
+It can be combined in the future with an ACL/RBAC library, if we want to restrict access to certain resources, but it didn't fulfil the initial requirements.
+
+## Pros and Cons of the Options
+
+### An ACL/RBAC library
+
+* Good, because there are many good libraries out there.
+* Bad, because when you need to filter resources lists this kind of libraries doesn't really work.
+
+### A service with the logic
+
+* Bad, because it would need to be used in many layers of the application, mixing unrelated concerns.
+
+### A library implementing the specification pattern
+
+* Good, because allows centralizing the generation of dynamic specs by the entity itself, that are later translated automatically into database queries.

docs/adr/README.md

@@ -0,0 +1,5 @@
+# Architectural Decision Records
+
+Here listed you will find the different architectural decisions taken in the project, including all the reasoning behind it, options considered, and final outcome.
+
+* [2021-01-17 Support restrictions and permissions in API keys](2021-01-17-support-restrictions-and-permissions-in-api-keys.md)

docs/swagger/paths/v1_short-urls.json

@@ -19,6 +19,15 @@
                     "type": "integer"
                 }
             },
+            {
+                "name": "itemsPerPage",
+                "in": "query",
+                "description": "The amount of items to return on every page. Defaults to 10",
+                "required": false,
+                "schema": {
+                    "type": "number"
+                }
+            },
             {
                 "name": "searchTerm",
                 "in": "query",

module/Rest/src/Middleware/CrossDomainMiddleware.php

@@ -6,7 +6,6 @@ namespace Shlinkio\Shlink\Rest\Middleware;
 
 use Fig\Http\Message\RequestMethodInterface;
 use Laminas\Diactoros\Response\EmptyResponse;
-use Mezzio\Router\RouteResult;
 use Psr\Http\Message\ResponseInterface;
 use Psr\Http\Message\ServerRequestInterface;
 use Psr\Http\Server\MiddlewareInterface;
@@ -32,7 +31,7 @@ class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterfa
         }
 
         // Add Allow-Origin header
-        $response = $response->withHeader('Access-Control-Allow-Origin', $request->getHeader('Origin'));
+        $response = $response->withHeader('Access-Control-Allow-Origin', '*');
         if ($request->getMethod() !== self::METHOD_OPTIONS) {
             return $response;
         }
@@ -42,20 +41,8 @@ class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterfa
 
     private function addOptionsHeaders(ServerRequestInterface $request, ResponseInterface $response): ResponseInterface
     {
-        // TODO This won't work. The route has to be matched from the router as this middleware needs to be executed
-        //      before trying to match the route
-        /** @var RouteResult|null $matchedRoute */
-        $matchedRoute = $request->getAttribute(RouteResult::class);
-        $matchedMethods = $matchedRoute !== null ? $matchedRoute->getAllowedMethods() : [
-            self::METHOD_GET,
-            self::METHOD_POST,
-            self::METHOD_PUT,
-            self::METHOD_PATCH,
-            self::METHOD_DELETE,
-            self::METHOD_OPTIONS,
-        ];
         $corsHeaders = [
-            'Access-Control-Allow-Methods' => implode(',', $matchedMethods),
+            'Access-Control-Allow-Methods' => $this->resolveCorsAllowedMethods($response),
             'Access-Control-Allow-Headers' => $request->getHeaderLine('Access-Control-Request-Headers'),
             'Access-Control-Max-Age' => $this->config['max_age'],
         ];
@@ -63,4 +50,22 @@ class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterfa
         // Options requests should always be empty and have a 204 status code
         return EmptyResponse::withHeaders(array_merge($response->getHeaders(), $corsHeaders));
     }
+
+    private function resolveCorsAllowedMethods(ResponseInterface $response): string
+    {
+        // ImplicitOptionsMiddleware resolves allowed methods using the RouteResult request's attribute and sets them
+        // in the "Allow" header.
+        // If the header is there, we can re-use the value as it is.
+        if ($response->hasHeader('Allow')) {
+            return $response->getHeaderLine('Allow');
+        }
+
+        return implode(',', [
+            self::METHOD_GET,
+            self::METHOD_POST,
+            self::METHOD_PUT,
+            self::METHOD_PATCH,
+            self::METHOD_DELETE,
+        ]);
+    }
 }

module/Rest/test-api/Action/CreateShortUrlTest.php

@@ -12,7 +12,7 @@ use function Functional\map;
 use function range;
 use function sprintf;
 
-class CreateShortUrlActionTest extends ApiTestCase
+class CreateShortUrlTest extends ApiTestCase
 {
     /** @test */
     public function createsNewShortUrlWhenOnlyLongUrlIsProvided(): void

module/Rest/test-api/Action/DeleteShortUrlTest.php

@@ -7,7 +7,7 @@ namespace ShlinkioApiTest\Shlink\Rest\Action;
 use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase;
 use ShlinkioApiTest\Shlink\Rest\Utils\NotFoundUrlHelpersTrait;
 
-class DeleteShortUrlActionTest extends ApiTestCase
+class DeleteShortUrlTest extends ApiTestCase
 {
     use NotFoundUrlHelpersTrait;
 

module/Rest/test-api/Action/EditShortUrlTagsTest.php

@@ -8,7 +8,7 @@ use GuzzleHttp\RequestOptions;
 use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase;
 use ShlinkioApiTest\Shlink\Rest\Utils\NotFoundUrlHelpersTrait;
 
-class EditShortUrlTagsActionTest extends ApiTestCase
+class EditShortUrlTagsTest extends ApiTestCase
 {
     use NotFoundUrlHelpersTrait;
 

module/Rest/test-api/Action/EditShortUrlTest.php

@@ -14,7 +14,7 @@ use ShlinkioApiTest\Shlink\Rest\Utils\NotFoundUrlHelpersTrait;
 use function GuzzleHttp\Psr7\build_query;
 use function sprintf;
 
-class EditShortUrlActionTest extends ApiTestCase
+class EditShortUrlTest extends ApiTestCase
 {
     use ArraySubsetAsserts;
     use NotFoundUrlHelpersTrait;

module/Rest/test-api/Action/GlobalVisitsTest.php

@@ -6,7 +6,7 @@ namespace ShlinkioApiTest\Shlink\Rest\Action;
 
 use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase;
 
-class GlobalVisitsActionTest extends ApiTestCase
+class GlobalVisitsTest extends ApiTestCase
 {
     /**
      * @test

module/Rest/test-api/Action/ListTagsTest.php

@@ -7,7 +7,7 @@ namespace ShlinkioApiTest\Shlink\Rest\Action;
 use GuzzleHttp\RequestOptions;
 use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase;
 
-class ListTagsActionTest extends ApiTestCase
+class ListTagsTest extends ApiTestCase
 {
     /**
      * @test

module/Rest/test-api/Action/ResolveShortUrlTest.php

@@ -11,7 +11,7 @@ use ShlinkioApiTest\Shlink\Rest\Utils\NotFoundUrlHelpersTrait;
 
 use function sprintf;
 
-class ResolveShortUrlActionTest extends ApiTestCase
+class ResolveShortUrlTest extends ApiTestCase
 {
     use NotFoundUrlHelpersTrait;
 

module/Rest/test-api/Action/ShortUrlVisitsTest.php

@@ -11,7 +11,7 @@ use ShlinkioApiTest\Shlink\Rest\Utils\NotFoundUrlHelpersTrait;
 use function GuzzleHttp\Psr7\build_query;
 use function sprintf;
 
-class ShortUrlVisitsActionTest extends ApiTestCase
+class ShortUrlVisitsTest extends ApiTestCase
 {
     use NotFoundUrlHelpersTrait;
 

module/Rest/test-api/Action/TagVisitsTest.php

@@ -8,7 +8,7 @@ use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase;
 
 use function sprintf;
 
-class TagVisitsActionTest extends ApiTestCase
+class TagVisitsTest extends ApiTestCase
 {
     /**
      * @test

module/Rest/test-api/Action/UpdateTagTest.php

@@ -7,7 +7,7 @@ namespace ShlinkioApiTest\Shlink\Rest\Action;
 use GuzzleHttp\RequestOptions;
 use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase;
 
-class UpdateTagActionTest extends ApiTestCase
+class UpdateTagTest extends ApiTestCase
 {
     /**
      * @test

module/Rest/test-api/Middleware/CorsTest.php

@@ -35,7 +35,7 @@ class CorsTest extends ApiTestCase
         ]);
 
         self::assertEquals($expectedStatusCode, $resp->getStatusCode());
-        self::assertEquals($origin, $resp->getHeaderLine('Access-Control-Allow-Origin'));
+        self::assertEquals('*', $resp->getHeaderLine('Access-Control-Allow-Origin'));
         self::assertFalse($resp->hasHeader('Access-Control-Allow-Methods'));
         self::assertFalse($resp->hasHeader('Access-Control-Max-Age'));
         self::assertFalse($resp->hasHeader('Access-Control-Allow-Headers'));
@@ -71,10 +71,9 @@ class CorsTest extends ApiTestCase
 
     public function providePreflightEndpoints(): iterable
     {
-        yield 'invalid route' => ['/foo/bar', 'GET,POST,PUT,PATCH,DELETE,OPTIONS'];
+        yield 'invalid route' => ['/foo/bar', 'GET,POST,PUT,PATCH,DELETE'];
-        yield 'short URLs routes' => ['/short-urls', 'GET,POST,PUT,PATCH,DELETE,OPTIONS'];
+        yield 'short URLs route' => ['/short-urls', 'GET,POST'];
-//        yield 'short URLs routes' => ['/short-urls', 'GET,POST']; // TODO This should be the good one
+        yield 'tags route' => ['/tags', 'GET,POST,PUT,DELETE'];
-        yield 'tags routes' => ['/tags', 'GET,POST,PUT,PATCH,DELETE,OPTIONS'];
+        yield 'health route' => ['/health', 'GET'];
-//        yield 'tags routes' => ['/short-urls', 'GET,POST,PUT,DELETE']; // TODO This should be the good one
     }
 }

module/Rest/test/Middleware/CrossDomainMiddlewareTest.php

@@ -6,8 +6,6 @@ namespace ShlinkioTest\Shlink\Rest\Middleware;
 
 use Laminas\Diactoros\Response;
 use Laminas\Diactoros\ServerRequest;
-use Mezzio\Router\Route;
-use Mezzio\Router\RouteResult;
 use PHPUnit\Framework\TestCase;
 use Prophecy\Argument;
 use Prophecy\PhpUnit\ProphecyTrait;
@@ -15,8 +13,6 @@ use Prophecy\Prophecy\ObjectProphecy;
 use Psr\Http\Server\RequestHandlerInterface;
 use Shlinkio\Shlink\Rest\Middleware\CrossDomainMiddleware;
 
-use function Laminas\Stratigility\middleware;
-
 class CrossDomainMiddlewareTest extends TestCase
 {
     use ProphecyTrait;
@@ -61,7 +57,7 @@ class CrossDomainMiddlewareTest extends TestCase
 
         $headers = $response->getHeaders();
 
-        self::assertEquals('local', $response->getHeaderLine('Access-Control-Allow-Origin'));
+        self::assertEquals('*', $response->getHeaderLine('Access-Control-Allow-Origin'));
         self::assertArrayNotHasKey('Access-Control-Allow-Methods', $headers);
         self::assertArrayNotHasKey('Access-Control-Max-Age', $headers);
         self::assertArrayNotHasKey('Access-Control-Allow-Headers', $headers);
@@ -82,7 +78,7 @@ class CrossDomainMiddlewareTest extends TestCase
 
         $headers = $response->getHeaders();
 
-        self::assertEquals('local', $response->getHeaderLine('Access-Control-Allow-Origin'));
+        self::assertEquals('*', $response->getHeaderLine('Access-Control-Allow-Origin'));
         self::assertArrayHasKey('Access-Control-Allow-Methods', $headers);
         self::assertEquals('1000', $response->getHeaderLine('Access-Control-Max-Age'));
         self::assertEquals('foo, bar, baz', $response->getHeaderLine('Access-Control-Allow-Headers'));
@@ -94,13 +90,15 @@ class CrossDomainMiddlewareTest extends TestCase
      * @dataProvider provideRouteResults
      */
     public function optionsRequestParsesRouteMatchToDetermineAllowedMethods(
-        ?RouteResult $result,
+        ?string $allowHeader,
         string $expectedAllowedMethods
     ): void {
         $originalResponse = new Response();
-        $request = (new ServerRequest())->withAttribute(RouteResult::class, $result)
+        if ($allowHeader !== null) {
-                                        ->withMethod('OPTIONS')
+            $originalResponse = $originalResponse->withHeader('Allow', $allowHeader);
-                                        ->withHeader('Origin', 'local');
+        }
+        $request = (new ServerRequest())->withHeader('Origin', 'local')
+                                        ->withMethod('OPTIONS');
         $this->handler->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce();
 
         $response = $this->middleware->process($request, $this->handler->reveal());
@@ -111,15 +109,9 @@ class CrossDomainMiddlewareTest extends TestCase
 
     public function provideRouteResults(): iterable
     {
-        yield 'with no route result' => [null, 'GET,POST,PUT,PATCH,DELETE,OPTIONS'];
+        yield 'no allow header in response' => [null, 'GET,POST,PUT,PATCH,DELETE'];
-        yield 'with failed route result' => [RouteResult::fromRouteFailure(['POST', 'GET']), 'POST,GET'];
+        yield 'allow header in response' => ['POST,GET', 'POST,GET'];
-        yield 'with success route result' => [
+        yield 'also allow header in response' => ['DELETE,PATCH,PUT', 'DELETE,PATCH,PUT'];
-            RouteResult::fromRoute(
-                new Route('/', middleware(function (): void {
-                }), ['DELETE', 'PATCH', 'PUT']),
-            ),
-            'DELETE,PATCH,PUT',
-        ];
     }
 
     /**