From 4b67d413622c649c5de39280faeb729509e89a12 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 4 Jan 2021 20:15:42 +0100 Subject: [PATCH] Applied API role specs to short URL creation --- module/Core/src/Domain/DomainService.php | 12 ++ .../src/Domain/DomainServiceInterface.php | 3 + .../src/Exception/DomainNotFoundException.php | 32 ++++ .../src/ShortUrl/Spec/BelongsToDomain.php | 4 +- .../ShortUrl/Spec/BelongsToDomainInlined.php | 4 +- module/Core/test/Domain/DomainServiceTest.php | 24 +++ .../Exception/DomainNotFoundExceptionTest.php | 28 ++++ .../Exception/TagNotFoundExceptionTest.php | 2 +- module/Rest/config/dependencies.config.php | 2 + module/Rest/config/routes.config.php | 7 +- .../SingleStepCreateShortUrlAction.php | 2 + module/Rest/src/ApiKey/Role.php | 7 +- module/Rest/src/Entity/ApiKey.php | 7 + .../ShortUrl/OverrideDomainMiddleware.php | 46 ++++++ .../ShortUrl/OverrideDomainMiddlewareTest.php | 141 ++++++++++++++++++ 15 files changed, 314 insertions(+), 7 deletions(-) create mode 100644 module/Core/src/Exception/DomainNotFoundException.php create mode 100644 module/Core/test/Exception/DomainNotFoundExceptionTest.php create mode 100644 module/Rest/src/Middleware/ShortUrl/OverrideDomainMiddleware.php create mode 100644 module/Rest/test/Middleware/ShortUrl/OverrideDomainMiddlewareTest.php diff --git a/module/Core/src/Domain/DomainService.php b/module/Core/src/Domain/DomainService.php index 836ca1da..e80f36b7 100644 --- a/module/Core/src/Domain/DomainService.php +++ b/module/Core/src/Domain/DomainService.php @@ -8,6 +8,7 @@ use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Core\Domain\Model\DomainItem; use Shlinkio\Shlink\Core\Domain\Repository\DomainRepositoryInterface; use Shlinkio\Shlink\Core\Entity\Domain; +use Shlinkio\Shlink\Core\Exception\DomainNotFoundException; use Shlinkio\Shlink\Rest\ApiKey\Role; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -43,4 +44,15 @@ class DomainService implements DomainServiceInterface ...$mappedDomains, ]; } + + public function getDomain(string $domainId): Domain + { + /** @var Domain|null $domain */ + $domain = $this->em->find(Domain::class, $domainId); + if ($domain === null) { + throw DomainNotFoundException::fromId($domainId); + } + + return $domain; + } } diff --git a/module/Core/src/Domain/DomainServiceInterface.php b/module/Core/src/Domain/DomainServiceInterface.php index 681000c5..0a2ef914 100644 --- a/module/Core/src/Domain/DomainServiceInterface.php +++ b/module/Core/src/Domain/DomainServiceInterface.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Domain; use Shlinkio\Shlink\Core\Domain\Model\DomainItem; +use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Rest\Entity\ApiKey; interface DomainServiceInterface @@ -13,4 +14,6 @@ interface DomainServiceInterface * @return DomainItem[] */ public function listDomains(?ApiKey $apiKey = null): array; + + public function getDomain(string $domainId): Domain; } diff --git a/module/Core/src/Exception/DomainNotFoundException.php b/module/Core/src/Exception/DomainNotFoundException.php new file mode 100644 index 00000000..b1b97c91 --- /dev/null +++ b/module/Core/src/Exception/DomainNotFoundException.php @@ -0,0 +1,32 @@ +detail = $e->getMessage(); + $e->title = self::TITLE; + $e->type = self::TYPE; + $e->status = StatusCodeInterface::STATUS_NOT_FOUND; + $e->additional = ['id' => $id]; + + return $e; + } +} diff --git a/module/Core/src/ShortUrl/Spec/BelongsToDomain.php b/module/Core/src/ShortUrl/Spec/BelongsToDomain.php index 27f93665..81b4388a 100644 --- a/module/Core/src/ShortUrl/Spec/BelongsToDomain.php +++ b/module/Core/src/ShortUrl/Spec/BelongsToDomain.php @@ -10,10 +10,10 @@ use Happyr\DoctrineSpecification\Spec; class BelongsToDomain extends BaseSpecification { - private int $domainId; + private string $domainId; private string $dqlAlias; - public function __construct(int $domainId, ?string $dqlAlias = null) + public function __construct(string $domainId, ?string $dqlAlias = null) { $this->domainId = $domainId; $this->dqlAlias = $dqlAlias ?? 's'; diff --git a/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php b/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php index edadf760..a8ef527e 100644 --- a/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php +++ b/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php @@ -9,9 +9,9 @@ use Happyr\DoctrineSpecification\Specification\Specification; class BelongsToDomainInlined implements Specification { - private int $domainId; + private string $domainId; - public function __construct(int $domainId) + public function __construct(string $domainId) { $this->domainId = $domainId; } diff --git a/module/Core/test/Domain/DomainServiceTest.php b/module/Core/test/Domain/DomainServiceTest.php index 55094b53..0201d7d9 100644 --- a/module/Core/test/Domain/DomainServiceTest.php +++ b/module/Core/test/Domain/DomainServiceTest.php @@ -12,6 +12,7 @@ use Shlinkio\Shlink\Core\Domain\DomainService; use Shlinkio\Shlink\Core\Domain\Model\DomainItem; use Shlinkio\Shlink\Core\Domain\Repository\DomainRepositoryInterface; use Shlinkio\Shlink\Core\Entity\Domain; +use Shlinkio\Shlink\Core\Exception\DomainNotFoundException; class DomainServiceTest extends TestCase { @@ -54,4 +55,27 @@ class DomainServiceTest extends TestCase [$default, new DomainItem('foo.com', false), new DomainItem('bar.com', false)], ]; } + + /** @test */ + public function getDomainThrowsExceptionWhenDomainIsNotFound(): void + { + $find = $this->em->find(Domain::class, '123')->willReturn(null); + + $this->expectException(DomainNotFoundException::class); + $find->shouldBeCalledOnce(); + + $this->domainService->getDomain('123'); + } + + /** @test */ + public function getDomainReturnsEntityWhenFound(): void + { + $domain = new Domain(''); + $find = $this->em->find(Domain::class, '123')->willReturn($domain); + + $result = $this->domainService->getDomain('123'); + + self::assertSame($domain, $result); + $find->shouldHaveBeenCalledOnce(); + } } diff --git a/module/Core/test/Exception/DomainNotFoundExceptionTest.php b/module/Core/test/Exception/DomainNotFoundExceptionTest.php new file mode 100644 index 00000000..6ac26efd --- /dev/null +++ b/module/Core/test/Exception/DomainNotFoundExceptionTest.php @@ -0,0 +1,28 @@ +getMessage()); + self::assertEquals($expectedMessage, $e->getDetail()); + self::assertEquals('Domain not found', $e->getTitle()); + self::assertEquals('DOMAIN_NOT_FOUND', $e->getType()); + self::assertEquals(['id' => $id], $e->getAdditionalData()); + self::assertEquals(404, $e->getStatus()); + } +} diff --git a/module/Core/test/Exception/TagNotFoundExceptionTest.php b/module/Core/test/Exception/TagNotFoundExceptionTest.php index c6e8bf1d..ccd63788 100644 --- a/module/Core/test/Exception/TagNotFoundExceptionTest.php +++ b/module/Core/test/Exception/TagNotFoundExceptionTest.php @@ -2,7 +2,7 @@ declare(strict_types=1); -namespace ShlinkioTest\Shlink\Rest\Exception; +namespace ShlinkioTest\Shlink\Core\Exception; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Exception\TagNotFoundException; diff --git a/module/Rest/config/dependencies.config.php b/module/Rest/config/dependencies.config.php index 6f94556d..c2181f70 100644 --- a/module/Rest/config/dependencies.config.php +++ b/module/Rest/config/dependencies.config.php @@ -45,6 +45,7 @@ return [ Middleware\ShortUrl\CreateShortUrlContentNegotiationMiddleware::class => InvokableFactory::class, Middleware\ShortUrl\DropDefaultDomainFromRequestMiddleware::class => ConfigAbstractFactory::class, Middleware\ShortUrl\DefaultShortCodesLengthMiddleware::class => ConfigAbstractFactory::class, + Middleware\ShortUrl\OverrideDomainMiddleware::class => ConfigAbstractFactory::class, ], ], @@ -81,6 +82,7 @@ return [ Middleware\ShortUrl\DefaultShortCodesLengthMiddleware::class => [ 'config.url_shortener.default_short_codes_length', ], + Middleware\ShortUrl\OverrideDomainMiddleware::class => [DomainService::class], ], ]; diff --git a/module/Rest/config/routes.config.php b/module/Rest/config/routes.config.php index 64333254..a5382c38 100644 --- a/module/Rest/config/routes.config.php +++ b/module/Rest/config/routes.config.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Rest; $contentNegotiationMiddleware = Middleware\ShortUrl\CreateShortUrlContentNegotiationMiddleware::class; $dropDomainMiddleware = Middleware\ShortUrl\DropDefaultDomainFromRequestMiddleware::class; +$overrideDomainMiddleware = Middleware\ShortUrl\OverrideDomainMiddleware::class; return [ @@ -16,9 +17,13 @@ return [ Action\ShortUrl\CreateShortUrlAction::getRouteDef([ $contentNegotiationMiddleware, $dropDomainMiddleware, + $overrideDomainMiddleware, Middleware\ShortUrl\DefaultShortCodesLengthMiddleware::class, ]), - Action\ShortUrl\SingleStepCreateShortUrlAction::getRouteDef([$contentNegotiationMiddleware]), + Action\ShortUrl\SingleStepCreateShortUrlAction::getRouteDef([ + $contentNegotiationMiddleware, + $overrideDomainMiddleware, + ]), Action\ShortUrl\EditShortUrlAction::getRouteDef([$dropDomainMiddleware]), Action\ShortUrl\DeleteShortUrlAction::getRouteDef([$dropDomainMiddleware]), Action\ShortUrl\ResolveShortUrlAction::getRouteDef([$dropDomainMiddleware]), diff --git a/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php b/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php index cbb06386..e9edee41 100644 --- a/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php +++ b/module/Rest/src/Action/ShortUrl/SingleStepCreateShortUrlAction.php @@ -51,6 +51,8 @@ class SingleStepCreateShortUrlAction extends AbstractCreateShortUrlAction return new CreateShortUrlData($longUrl, [], ShortUrlMeta::fromRawData([ ShortUrlMetaInputFilter::API_KEY => $apiKeyResult->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/ApiKey/Role.php b/module/Rest/src/ApiKey/Role.php index 83a78087..87bad5fc 100644 --- a/module/Rest/src/ApiKey/Role.php +++ b/module/Rest/src/ApiKey/Role.php @@ -24,10 +24,15 @@ class Role } if ($role->name() === self::DOMAIN_SPECIFIC) { - $domainId = $role->meta()['domain_id'] ?? -1; + $domainId = self::domainIdFromMeta($role->meta()); return $inlined ? new BelongsToDomainInlined($domainId) : new BelongsToDomain($domainId); } return Spec::andX(); } + + public static function domainIdFromMeta(array $meta): string + { + return $meta['domain_id'] ?? '-1'; + } } diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index 28b236fc..3d600ce7 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -83,4 +83,11 @@ class ApiKey extends AbstractEntity { return $this->roles->exists(fn ($key, ApiKeyRole $role) => $role->name() === $roleName); } + + public function getRoleMeta(string $roleName): array + { + /** @var ApiKeyRole|false $role */ + $role = $this->roles->filter(fn (ApiKeyRole $role) => $role->name() === $roleName)->first(); + return ! $role ? [] : $role->meta(); + } } diff --git a/module/Rest/src/Middleware/ShortUrl/OverrideDomainMiddleware.php b/module/Rest/src/Middleware/ShortUrl/OverrideDomainMiddleware.php new file mode 100644 index 00000000..817570a8 --- /dev/null +++ b/module/Rest/src/Middleware/ShortUrl/OverrideDomainMiddleware.php @@ -0,0 +1,46 @@ +domainService = $domainService; + } + + public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface + { + $apiKey = AuthenticationMiddleware::apiKeyFromRequest($request); + if (! $apiKey->hasRole(Role::DOMAIN_SPECIFIC)) { + return $handler->handle($request); + } + + $requestMethod = $request->getMethod(); + $domainId = Role::domainIdFromMeta($apiKey->getRoleMeta(Role::DOMAIN_SPECIFIC)); + $domain = $this->domainService->getDomain($domainId); + + if ($requestMethod === RequestMethodInterface::METHOD_POST) { + $payload = $request->getParsedBody(); + $payload[ShortUrlMetaInputFilter::DOMAIN] = $domain->getAuthority(); + + return $handler->handle($request->withParsedBody($payload)); + } + + return $handler->handle($request->withAttribute(ShortUrlMetaInputFilter::DOMAIN, $domain->getAuthority())); + } +} diff --git a/module/Rest/test/Middleware/ShortUrl/OverrideDomainMiddlewareTest.php b/module/Rest/test/Middleware/ShortUrl/OverrideDomainMiddlewareTest.php new file mode 100644 index 00000000..dcf4d7ce --- /dev/null +++ b/module/Rest/test/Middleware/ShortUrl/OverrideDomainMiddlewareTest.php @@ -0,0 +1,141 @@ +apiKey = $this->prophesize(ApiKey::class); + $this->handler = $this->prophesize(RequestHandlerInterface::class); + + $this->domainService = $this->prophesize(DomainServiceInterface::class); + $this->middleware = new OverrideDomainMiddleware($this->domainService->reveal()); + } + + /** @test */ + public function nextMiddlewareIsCalledWhenApiKeyDoesNotHaveProperRole(): void + { + $request = $this->requestWithApiKey(); + $response = new Response(); + $hasRole = $this->apiKey->hasRole(Role::DOMAIN_SPECIFIC)->willReturn(false); + $handle = $this->handler->handle($request)->willReturn($response); + $getDomain = $this->domainService->getDomain(Argument::cetera()); + + $result = $this->middleware->process($request, $this->handler->reveal()); + + self::assertSame($response, $result); + $hasRole->shouldHaveBeenCalledOnce(); + $handle->shouldHaveBeenCalledOnce(); + $getDomain->shouldNotHaveBeenCalled(); + } + + /** + * @test + * @dataProvider provideBodies + */ + public function overwritesRequestBodyWhenMethodIsPost(Domain $domain, array $body, array $expectedBody): void + { + $request = $this->requestWithApiKey()->withMethod('POST')->withParsedBody($body); + $hasRole = $this->apiKey->hasRole(Role::DOMAIN_SPECIFIC)->willReturn(true); + $getRoleMeta = $this->apiKey->getRoleMeta(Role::DOMAIN_SPECIFIC)->willReturn(['domain_id' => '123']); + $getDomain = $this->domainService->getDomain('123')->willReturn($domain); + $handle = $this->handler->handle(Argument::that( + function (ServerRequestInterface $req) use ($expectedBody): bool { + Assert::assertEquals($req->getParsedBody(), $expectedBody); + return true; + }, + ))->willReturn(new Response()); + + $this->middleware->process($request, $this->handler->reveal()); + + $hasRole->shouldHaveBeenCalledOnce(); + $getRoleMeta->shouldHaveBeenCalledOnce(); + $getDomain->shouldHaveBeenCalledOnce(); + $handle->shouldHaveBeenCalledOnce(); + } + + public function provideBodies(): iterable + { + yield 'no domain provided' => [new Domain('foo.com'), [], [ShortUrlMetaInputFilter::DOMAIN => 'foo.com']]; + yield 'other domain provided' => [ + new Domain('bar.com'), + [ShortUrlMetaInputFilter::DOMAIN => 'foo.com'], + [ShortUrlMetaInputFilter::DOMAIN => 'bar.com'], + ]; + yield 'same domain provided' => [ + new Domain('baz.com'), + [ShortUrlMetaInputFilter::DOMAIN => 'baz.com'], + [ShortUrlMetaInputFilter::DOMAIN => 'baz.com'], + ]; + yield 'more body params' => [ + new Domain('doma.in'), + [ShortUrlMetaInputFilter::DOMAIN => 'baz.com', 'something' => 'else', 'foo' => 123], + [ShortUrlMetaInputFilter::DOMAIN => 'doma.in', 'something' => 'else', 'foo' => 123], + ]; + } + + /** + * @test + * @dataProvider provideMethods + */ + public function setsRequestAttributeWhenMethodIsNotPost(string $method): void + { + $domain = new Domain('something.com'); + $request = $this->requestWithApiKey()->withMethod($method); + $hasRole = $this->apiKey->hasRole(Role::DOMAIN_SPECIFIC)->willReturn(true); + $getRoleMeta = $this->apiKey->getRoleMeta(Role::DOMAIN_SPECIFIC)->willReturn(['domain_id' => '123']); + $getDomain = $this->domainService->getDomain('123')->willReturn($domain); + $handle = $this->handler->handle(Argument::that( + function (ServerRequestInterface $req): bool { + Assert::assertEquals($req->getAttribute(ShortUrlMetaInputFilter::DOMAIN), 'something.com'); + return true; + }, + ))->willReturn(new Response()); + + $this->middleware->process($request, $this->handler->reveal()); + + $hasRole->shouldHaveBeenCalledOnce(); + $getRoleMeta->shouldHaveBeenCalledOnce(); + $getDomain->shouldHaveBeenCalledOnce(); + $handle->shouldHaveBeenCalledOnce(); + } + + public function provideMethods(): iterable + { + yield 'GET' => ['GET']; + yield 'PUT' => ['PUT']; + yield 'PATCH' => ['PATCH']; + yield 'DELETE' => ['DELETE']; + } + + private function requestWithApiKey(): ServerRequestInterface + { + return ServerRequestFactory::fromGlobals()->withAttribute(ApiKey::class, $this->apiKey->reveal()); + } +}