diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f10d1fb..2e5d295c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ### Deprecated -* *Nothing* +* [#406](https://github.com/shlinkio/shlink/issues/406) Deprecated `PUT /short-urls/{shortCode}` REST endpoint in favor of `PATCH /short-urls/{shortCode}`. #### Removed diff --git a/docs/swagger/paths/v1_short-urls_{shortCode}.json b/docs/swagger/paths/v1_short-urls_{shortCode}.json index 778d0fb1..c312db8a 100644 --- a/docs/swagger/paths/v1_short-urls_{shortCode}.json +++ b/docs/swagger/paths/v1_short-urls_{shortCode}.json @@ -81,7 +81,7 @@ } }, - "put": { + "patch": { "operationId": "editShortUrl", "tags": [ "Short URLs" @@ -169,6 +169,95 @@ } }, + "put": { + "deprecated": true, + "operationId": "editShortUrlPut", + "tags": [ + "Short URLs" + ], + "summary": "[DEPRECATED] Edit short URL", + "description": "**[DEPRECATED]** Use [editShortUrl](#/Short_URLs/getShortUrl) instead", + "parameters": [ + { + "name": "shortCode", + "in": "path", + "description": "The short code to edit.", + "required": true, + "schema": { + "type": "string" + } + } + ], + "requestBody": { + "description": "Request body.", + "required": true, + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "validSince": { + "description": "The date (in ISO-8601 format) from which this short code will be valid", + "type": "string" + }, + "validUntil": { + "description": "The date (in ISO-8601 format) until which this short code will be valid", + "type": "string" + }, + "maxVisits": { + "description": "The maximum number of allowed visits for this short code", + "type": "number" + } + } + } + } + } + }, + "security": [ + { + "ApiKey": [] + }, + { + "Bearer": [] + } + ], + "responses": { + "204": { + "description": "The short code has been properly updated." + }, + "400": { + "description": "Provided meta arguments are invalid.", + "content": { + "application/json": { + "schema": { + "$ref": "../definitions/Error.json" + } + } + } + }, + "404": { + "description": "No short URL was found for provided short code.", + "content": { + "application/json": { + "schema": { + "$ref": "../definitions/Error.json" + } + } + } + }, + "500": { + "description": "Unexpected error.", + "content": { + "application/json": { + "schema": { + "$ref": "../definitions/Error.json" + } + } + } + } + } + }, + "delete": { "operationId": "deleteShortUrl", "tags": [ diff --git a/docs/swagger/swagger.json b/docs/swagger/swagger.json index 8f996886..6dd3ff8f 100644 --- a/docs/swagger/swagger.json +++ b/docs/swagger/swagger.json @@ -35,7 +35,7 @@ "name": "X-Api-Key" }, "Bearer": { - "description": "**[Deprecated]** The JWT identifying a previously authenticated API key", + "description": "**[DEPRECATED]** The JWT identifying a previously authenticated API key", "type": "http", "scheme": "bearer", "bearerFormat": "JWT" @@ -66,7 +66,7 @@ }, { "name": "Authentication", - "description": "**[Deprecated]** Authentication-related endpoints" + "description": "**[DEPRECATED]** Authentication-related endpoints" } ], diff --git a/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php b/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php index bd8a8f36..b7bc54ab 100644 --- a/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/EditShortUrlAction.php @@ -19,7 +19,7 @@ use function sprintf; class EditShortUrlAction extends AbstractRestAction { protected const ROUTE_PATH = '/short-urls/{shortCode}'; - protected const ROUTE_ALLOWED_METHODS = [self::METHOD_PUT]; + protected const ROUTE_ALLOWED_METHODS = [self::METHOD_PATCH, self::METHOD_PUT]; /** @var ShortUrlServiceInterface */ private $shortUrlService; diff --git a/module/Rest/src/Middleware/CrossDomainMiddleware.php b/module/Rest/src/Middleware/CrossDomainMiddleware.php index b6cead2e..9ebc84e7 100644 --- a/module/Rest/src/Middleware/CrossDomainMiddleware.php +++ b/module/Rest/src/Middleware/CrossDomainMiddleware.php @@ -4,29 +4,19 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Middleware; use Fig\Http\Message\RequestMethodInterface; -use Psr\Http\Message\ResponseInterface as Response; -use Psr\Http\Message\ServerRequestInterface as Request; +use Psr\Http\Message\ResponseInterface; +use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Rest\Authentication; +use Zend\Expressive\Router\RouteResult; use function implode; class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterface { - /** - * Process an incoming server request and return a response, optionally delegating - * to the next middleware component to create the response. - * - * @param Request $request - * @param RequestHandlerInterface $handler - * - * @return Response - * @throws \InvalidArgumentException - */ - public function process(Request $request, RequestHandlerInterface $handler): Response + public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { - /** @var Response $response */ $response = $handler->handle($request); if (! $request->hasHeader('Origin')) { return $response; @@ -42,13 +32,28 @@ class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterfa return $response; } - // Add OPTIONS-specific headers - foreach ([ - 'Access-Control-Allow-Methods' => 'GET,POST,PUT,DELETE,OPTIONS', // TODO Should be dynamic -// 'Access-Control-Allow-Methods' => $response->getHeaderLine('Allow'), + return $this->addOptionsHeaders($request, $response); + } + + private function addOptionsHeaders(ServerRequestInterface $request, ResponseInterface $response): ResponseInterface + { + /** @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-Max-Age' => '1000', 'Access-Control-Allow-Headers' => $request->getHeaderLine('Access-Control-Request-Headers'), - ] as $key => $value) { + ]; + + foreach ($corsHeaders as $key => $value) { $response = $response->withHeader($key, $value); } diff --git a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php index 17539f86..9e93c500 100644 --- a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php +++ b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php @@ -10,63 +10,108 @@ use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Rest\Middleware\CrossDomainMiddleware; use Zend\Diactoros\Response; use Zend\Diactoros\ServerRequest; +use Zend\Expressive\Router\Route; +use Zend\Expressive\Router\RouteResult; + +use function Zend\Stratigility\middleware; class CrossDomainMiddlewareTest extends TestCase { /** @var CrossDomainMiddleware */ private $middleware; /** @var ObjectProphecy */ - private $delegate; + private $handler; public function setUp(): void { $this->middleware = new CrossDomainMiddleware(); - $this->delegate = $this->prophesize(RequestHandlerInterface::class); + $this->handler = $this->prophesize(RequestHandlerInterface::class); } /** @test */ - public function nonCrossDomainRequestsAreNotAffected() + public function nonCrossDomainRequestsAreNotAffected(): void { $originalResponse = new Response(); - $this->delegate->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce(); + $this->handler->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce(); - $response = $this->middleware->process(new ServerRequest(), $this->delegate->reveal()); + $response = $this->middleware->process(new ServerRequest(), $this->handler->reveal()); $this->assertSame($originalResponse, $response); $headers = $response->getHeaders(); $this->assertArrayNotHasKey('Access-Control-Allow-Origin', $headers); + $this->assertArrayNotHasKey('Access-Control-Expose-Headers', $headers); + $this->assertArrayNotHasKey('Access-Control-Allow-Methods', $headers); + $this->assertArrayNotHasKey('Access-Control-Max-Age', $headers); $this->assertArrayNotHasKey('Access-Control-Allow-Headers', $headers); } /** @test */ - public function anyRequestIncludesTheAllowAccessHeader() + public function anyRequestIncludesTheAllowAccessHeader(): void { $originalResponse = new Response(); - $this->delegate->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce(); + $this->handler->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce(); $response = $this->middleware->process( (new ServerRequest())->withHeader('Origin', 'local'), - $this->delegate->reveal() + $this->handler->reveal() ); $this->assertNotSame($originalResponse, $response); $headers = $response->getHeaders(); $this->assertArrayHasKey('Access-Control-Allow-Origin', $headers); + $this->assertArrayHasKey('Access-Control-Expose-Headers', $headers); + $this->assertArrayNotHasKey('Access-Control-Allow-Methods', $headers); + $this->assertArrayNotHasKey('Access-Control-Max-Age', $headers); $this->assertArrayNotHasKey('Access-Control-Allow-Headers', $headers); } /** @test */ - public function optionsRequestIncludesMoreHeaders() + public function optionsRequestIncludesMoreHeaders(): void { $originalResponse = new Response(); $request = (new ServerRequest())->withMethod('OPTIONS')->withHeader('Origin', 'local'); - $this->delegate->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce(); + $this->handler->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce(); - $response = $this->middleware->process($request, $this->delegate->reveal()); + $response = $this->middleware->process($request, $this->handler->reveal()); $this->assertNotSame($originalResponse, $response); $headers = $response->getHeaders(); $this->assertArrayHasKey('Access-Control-Allow-Origin', $headers); + $this->assertArrayHasKey('Access-Control-Expose-Headers', $headers); + $this->assertArrayHasKey('Access-Control-Allow-Methods', $headers); + $this->assertArrayHasKey('Access-Control-Max-Age', $headers); $this->assertArrayHasKey('Access-Control-Allow-Headers', $headers); } + + /** + * @test + * @dataProvider provideRouteResults + */ + public function optionsRequestParsesRouteMatchToDetermineAllowedMethods( + ?RouteResult $result, + string $expectedAllowedMethods + ): void { + $originalResponse = new Response(); + $request = (new ServerRequest())->withAttribute(RouteResult::class, $result) + ->withMethod('OPTIONS') + ->withHeader('Origin', 'local'); + $this->handler->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce(); + + $response = $this->middleware->process($request, $this->handler->reveal()); + + $this->assertEquals($response->getHeaderLine('Access-Control-Allow-Methods'), $expectedAllowedMethods); + } + + public function provideRouteResults(): iterable + { + yield 'with no route result' => [null, 'GET,POST,PUT,PATCH,DELETE,OPTIONS']; + yield 'with failed route result' => [RouteResult::fromRouteFailure(['POST', 'GET']), 'POST,GET']; + yield 'with success route result' => [ + RouteResult::fromRoute( + new Route('/', middleware(function () { + }), ['DELETE', 'PATCH', 'PUT']) + ), + 'DELETE,PATCH,PUT', + ]; + } }