From 192308a6a3cb6997caac319257d5c02f605be2ab Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 25 Jul 2021 18:46:48 +0200 Subject: [PATCH 01/17] Added swagger docs for endpoint do edit domain redirects --- .../definitions/NotFoundRedirects.json | 20 +++++ docs/swagger/paths/v2_domains_redirects.json | 81 +++++++++++++++++++ docs/swagger/swagger.json | 5 +- 3 files changed, 105 insertions(+), 1 deletion(-) create mode 100644 docs/swagger/definitions/NotFoundRedirects.json create mode 100644 docs/swagger/paths/v2_domains_redirects.json diff --git a/docs/swagger/definitions/NotFoundRedirects.json b/docs/swagger/definitions/NotFoundRedirects.json new file mode 100644 index 00000000..6887ed0c --- /dev/null +++ b/docs/swagger/definitions/NotFoundRedirects.json @@ -0,0 +1,20 @@ +{ + "type": "object", + "properties": { + "baseUrlRedirect": { + "type": "string", + "nullable": true, + "description": "URL to redirect to when a user hits the domain's base URL" + }, + "regular404Redirect": { + "type": "string", + "nullable": true, + "description": "URL to redirect to when a user hits a not found URL other than an invalid short URL" + }, + "invalidShortUrlRedirect": { + "type": "string", + "nullable": true, + "description": "URL to redirect to when a user hits an invalid short URL" + } + } +} diff --git a/docs/swagger/paths/v2_domains_redirects.json b/docs/swagger/paths/v2_domains_redirects.json new file mode 100644 index 00000000..0868504e --- /dev/null +++ b/docs/swagger/paths/v2_domains_redirects.json @@ -0,0 +1,81 @@ +{ + "patch": { + "operationId": "setDomainRedirects", + "tags": [ + "Domains" + ], + "summary": "Sets domain \"not found\" redirects", + "description": "Sets the URLs that you want a visitor to get redirected to for \not found\" URLs for a specific domain", + "security": [ + { + "ApiKey": [] + } + ], + "parameters": [ + { + "$ref": "../parameters/version.json" + } + ], + "requestBody": { + "description": "Request body.", + "required": true, + "content": { + "application/json": { + "schema": { + "type": "object", + "allOf": [ + { + "required": ["domain"], + "properties": { + "domain": { + "description": "The domain's authority for which you want to set redirects", + "type": "string" + } + } + }, + { + "$ref": "../definitions/NotFoundRedirects.json" + } + ] + } + } + } + }, + "responses": { + "200": { + "description": "The domain's redirects after the update, when existing redirects have been merged with provided ones.", + "content": { + "application/json": { + "schema": { + "allOf": [ + { + "required": ["baseUrlRedirect", "regular404Redirect", "invalidShortUrlRedirect"] + }, + { + "$ref": "../definitions/NotFoundRedirects.json" + } + ] + } + } + }, + "examples": { + "application/json": { + "baseUrlRedirect": "https://example.com/my-landing-page", + "regular404Redirect": null, + "invalidShortUrlRedirect": "https://example.com/invalid-url" + } + } + }, + "500": { + "description": "Unexpected error.", + "content": { + "application/problem+json": { + "schema": { + "$ref": "../definitions/Error.json" + } + } + } + } + } + } +} diff --git a/docs/swagger/swagger.json b/docs/swagger/swagger.json index 21547f90..705069cc 100644 --- a/docs/swagger/swagger.json +++ b/docs/swagger/swagger.json @@ -1,5 +1,5 @@ { - "openapi": "3.0.0", + "openapi": "3.0.3", "info": { "title": "Shlink", "description": "Shlink, the self-hosted URL shortener", @@ -102,6 +102,9 @@ "/rest/v{version}/domains": { "$ref": "paths/v2_domains.json" }, + "/rest/v{version}/domains/redirects": { + "$ref": "paths/v2_domains_redirects.json" + }, "/rest/v{version}/mercure-info": { "$ref": "paths/v2_mercure-info.json" From 4ef5ab7a90eb1462b2d9059c21471777707b6653 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 27 Jul 2021 20:03:39 +0200 Subject: [PATCH 02/17] Fixed wrong domains getting resolved for an API key roles --- .../Domain/Repository/DomainRepository.php | 31 ++++++++++++++----- module/Core/src/Domain/Spec/IsDomain.php | 22 +++++++++++++ .../Core/src/Domain/Spec/IsNotAuthority.php | 22 +++++++++++++ .../src/ShortUrl/Spec/BelongsToApiKey.php | 6 ++-- .../ShortUrl/Spec/BelongsToDomainInlined.php | 2 +- .../Repository/DomainRepositoryTest.php | 28 ++++++++--------- module/Rest/src/Entity/ApiKey.php | 5 +++ 7 files changed, 91 insertions(+), 25 deletions(-) create mode 100644 module/Core/src/Domain/Spec/IsDomain.php create mode 100644 module/Core/src/Domain/Spec/IsNotAuthority.php diff --git a/module/Core/src/Domain/Repository/DomainRepository.php b/module/Core/src/Domain/Repository/DomainRepository.php index e0862558..6d246924 100644 --- a/module/Core/src/Domain/Repository/DomainRepository.php +++ b/module/Core/src/Domain/Repository/DomainRepository.php @@ -6,8 +6,13 @@ namespace Shlinkio\Shlink\Core\Domain\Repository; use Doctrine\ORM\Query\Expr\Join; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; +use Happyr\DoctrineSpecification\Spec; +use Shlinkio\Shlink\Core\Domain\Spec\IsDomain; +use Shlinkio\Shlink\Core\Domain\Spec\IsNotAuthority; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Entity\ShortUrl; +use Shlinkio\Shlink\Core\ShortUrl\Spec\BelongsToApiKey; +use Shlinkio\Shlink\Rest\ApiKey\Role; use Shlinkio\Shlink\Rest\Entity\ApiKey; class DomainRepository extends EntitySpecificationRepository implements DomainRepositoryInterface @@ -26,15 +31,27 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe ->orHaving($qb->expr()->isNotNull('d.regular404Redirect')) ->orHaving($qb->expr()->isNotNull('d.invalidShortUrlRedirect')); - if ($excludedAuthority !== null) { - $qb->where($qb->expr()->neq('d.authority', ':excludedAuthority')) - ->setParameter('excludedAuthority', $excludedAuthority); - } - - if ($apiKey !== null) { - $this->applySpecification($qb, $apiKey->spec(), 's'); + $specs = $this->determineExtraSpecs($excludedAuthority, $apiKey); + foreach ($specs as [$alias, $spec]) { + $this->applySpecification($qb, $spec, $alias); } return $qb->getQuery()->getResult(); } + + private function determineExtraSpecs(?string $excludedAuthority, ?ApiKey $apiKey): iterable + { + if ($excludedAuthority !== null) { + yield ['d', new IsNotAuthority($excludedAuthority)]; + } + + // FIXME The $apiKey->spec() method cannot be used here, as it returns a single spec which assumes the + // ShortUrl is the root entity. Here, the Domain is the root entity. + // Think on a way to centralize the conditional behavior and make $apiKey->spec() more flexible. + yield from $apiKey?->mapRoles(fn (string $roleName, array $meta) => match ($roleName) { + Role::DOMAIN_SPECIFIC => ['d', new IsDomain(Role::domainIdFromMeta($meta))], + Role::AUTHORED_SHORT_URLS => ['s', new BelongsToApiKey($apiKey)], + default => [null, Spec::andX()], + }) ?? []; + } } diff --git a/module/Core/src/Domain/Spec/IsDomain.php b/module/Core/src/Domain/Spec/IsDomain.php new file mode 100644 index 00000000..cf7463cc --- /dev/null +++ b/module/Core/src/Domain/Spec/IsDomain.php @@ -0,0 +1,22 @@ +domainId); + } +} diff --git a/module/Core/src/Domain/Spec/IsNotAuthority.php b/module/Core/src/Domain/Spec/IsNotAuthority.php new file mode 100644 index 00000000..0f0f0653 --- /dev/null +++ b/module/Core/src/Domain/Spec/IsNotAuthority.php @@ -0,0 +1,22 @@ +authority)); + } +} diff --git a/module/Core/src/ShortUrl/Spec/BelongsToApiKey.php b/module/Core/src/ShortUrl/Spec/BelongsToApiKey.php index 84852275..3c95593c 100644 --- a/module/Core/src/ShortUrl/Spec/BelongsToApiKey.php +++ b/module/Core/src/ShortUrl/Spec/BelongsToApiKey.php @@ -11,13 +11,13 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey; class BelongsToApiKey extends BaseSpecification { - public function __construct(private ApiKey $apiKey, private ?string $dqlAlias = null) + public function __construct(private ApiKey $apiKey, ?string $context = null) { - parent::__construct(); + parent::__construct($context); } protected function getSpec(): Filter { - return Spec::eq('authorApiKey', $this->apiKey, $this->dqlAlias); + return Spec::eq('authorApiKey', $this->apiKey); } } diff --git a/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php b/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php index 414b3f74..4ce130b7 100644 --- a/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php +++ b/module/Core/src/ShortUrl/Spec/BelongsToDomainInlined.php @@ -13,7 +13,7 @@ class BelongsToDomainInlined implements Filter { } - public function getFilter(QueryBuilder $qb, string $dqlAlias): string + public function getFilter(QueryBuilder $qb, string $context): string { // Parameters in this query need to be inlined, not bound, as we need to use it as sub-query later return (string) $qb->expr()->eq('s.domain', '\'' . $this->domainId . '\''); diff --git a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php index 9b0270a6..9e3fe9be 100644 --- a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php +++ b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php @@ -92,12 +92,12 @@ class DomainRepositoryTest extends DatabaseTestCase $this->getEntityManager()->persist($bazDomain); $this->getEntityManager()->persist($this->createShortUrl($bazDomain, $authorApiKey)); -// $detachedDomain = Domain::withAuthority('detached.com'); -// $this->getEntityManager()->persist($detachedDomain); -// -// $detachedWithRedirects = Domain::withAuthority('detached-with-redirects.com'); -// $detachedWithRedirects->configureNotFoundRedirects(new NotFoundRedirects('foo.com', 'bar.com')); -// $this->getEntityManager()->persist($detachedWithRedirects); + $detachedDomain = Domain::withAuthority('detached.com'); + $this->getEntityManager()->persist($detachedDomain); + + $detachedWithRedirects = Domain::withAuthority('detached-with-redirects.com'); + $detachedWithRedirects->configureNotFoundRedirects(new NotFoundRedirects('foo.com', 'bar.com')); + $this->getEntityManager()->persist($detachedWithRedirects); $this->getEntityManager()->flush(); @@ -109,19 +109,19 @@ class DomainRepositoryTest extends DatabaseTestCase $barDomainApiKey = ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forDomain($barDomain))); $this->getEntityManager()->persist($barDomainApiKey); -// $detachedWithRedirectsApiKey = ApiKey::fromMeta( -// ApiKeyMeta::withRoles(RoleDefinition::forDomain($detachedWithRedirects)), -// ); -// $this->getEntityManager()->persist($detachedWithRedirectsApiKey); + $detachedWithRedirectsApiKey = ApiKey::fromMeta( + ApiKeyMeta::withRoles(RoleDefinition::forDomain($detachedWithRedirects)), + ); + $this->getEntityManager()->persist($detachedWithRedirectsApiKey); $this->getEntityManager()->flush(); self::assertEquals([$fooDomain], $this->repo->findDomainsWithout(null, $fooDomainApiKey)); self::assertEquals([$barDomain], $this->repo->findDomainsWithout(null, $barDomainApiKey)); -// self::assertEquals( -// [$detachedWithRedirects], -// $this->repo->findDomainsWithout(null, $detachedWithRedirectsApiKey), -// ); + self::assertEquals( + [$detachedWithRedirects], + $this->repo->findDomainsWithout(null, $detachedWithRedirectsApiKey), + ); self::assertEquals([$bazDomain, $fooDomain], $this->repo->findDomainsWithout(null, $authorApiKey)); self::assertEquals([], $this->repo->findDomainsWithout(null, $authorAndDomainApiKey)); } diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index 6c63c67b..121bea18 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -119,6 +119,11 @@ class ApiKey extends AbstractEntity return $role?->meta() ?? []; } + /** + * @template T + * @param callable(string $roleName, array $meta): T $fun + * @return T[] + */ public function mapRoles(callable $fun): array { return $this->roles->map(fn (ApiKeyRole $role) => $fun($role->name(), $role->meta()))->getValues(); From 2ac7be4363f7e86ad2061da2dca557afa3c51bc5 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 29 Jul 2021 18:23:41 +0200 Subject: [PATCH 03/17] Extended DomainNotFoundException to allow creating from an authority --- .../src/Exception/DomainNotFoundException.php | 27 +++++++++++++------ .../Exception/DomainNotFoundExceptionTest.php | 17 +++++++++++- 2 files changed, 35 insertions(+), 9 deletions(-) diff --git a/module/Core/src/Exception/DomainNotFoundException.php b/module/Core/src/Exception/DomainNotFoundException.php index b1b97c91..cb19608a 100644 --- a/module/Core/src/Exception/DomainNotFoundException.php +++ b/module/Core/src/Exception/DomainNotFoundException.php @@ -17,16 +17,27 @@ class DomainNotFoundException extends DomainException implements ProblemDetailsE private const TITLE = 'Domain not found'; private const TYPE = 'DOMAIN_NOT_FOUND'; + private function __construct(string $message, array $additional) + { + parent::__construct($message); + + $this->detail = $message; + $this->title = self::TITLE; + $this->type = self::TYPE; + $this->status = StatusCodeInterface::STATUS_NOT_FOUND; + $this->additional = $additional; + } + public static function fromId(string $id): self { - $e = new self(sprintf('Domain with id "%s" could not be found', $id)); + return new self(sprintf('Domain with id "%s" could not be found', $id), ['id' => $id]); + } - $e->detail = $e->getMessage(); - $e->title = self::TITLE; - $e->type = self::TYPE; - $e->status = StatusCodeInterface::STATUS_NOT_FOUND; - $e->additional = ['id' => $id]; - - return $e; + public static function fromAuthority(string $authority): self + { + return new self( + sprintf('Domain with authority "%s" could not be found', $authority), + ['authority' => $authority], + ); } } diff --git a/module/Core/test/Exception/DomainNotFoundExceptionTest.php b/module/Core/test/Exception/DomainNotFoundExceptionTest.php index 6ac26efd..5f2b9889 100644 --- a/module/Core/test/Exception/DomainNotFoundExceptionTest.php +++ b/module/Core/test/Exception/DomainNotFoundExceptionTest.php @@ -12,7 +12,7 @@ use function sprintf; class DomainNotFoundExceptionTest extends TestCase { /** @test */ - public function properlyCreatesExceptionFromNotFoundTag(): void + public function properlyCreatesExceptionFromId(): void { $id = '123'; $expectedMessage = sprintf('Domain with id "%s" could not be found', $id); @@ -25,4 +25,19 @@ class DomainNotFoundExceptionTest extends TestCase self::assertEquals(['id' => $id], $e->getAdditionalData()); self::assertEquals(404, $e->getStatus()); } + + /** @test */ + public function properlyCreatesExceptionFromAuthority(): void + { + $authority = 'example.com'; + $expectedMessage = sprintf('Domain with authority "%s" could not be found', $authority); + $e = DomainNotFoundException::fromAuthority($authority); + + self::assertEquals($expectedMessage, $e->getMessage()); + self::assertEquals($expectedMessage, $e->getDetail()); + self::assertEquals('Domain not found', $e->getTitle()); + self::assertEquals('DOMAIN_NOT_FOUND', $e->getType()); + self::assertEquals(['authority' => $authority], $e->getAdditionalData()); + self::assertEquals(404, $e->getStatus()); + } } From 5a1a4f5594c6135351cf410719524c43d6bff403 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 29 Jul 2021 19:08:29 +0200 Subject: [PATCH 04/17] Added support to configure domain redirects but taking into consideration the permissions on an API key --- module/Core/src/Domain/DomainService.php | 23 ++++++---- .../src/Domain/DomainServiceInterface.php | 16 +++++-- .../Domain/Repository/DomainRepository.php | 39 +++++++++++++---- .../Repository/DomainRepositoryInterface.php | 2 + .../Repository/DomainRepositoryTest.php | 18 +++++++- module/Core/test/Domain/DomainServiceTest.php | 42 +++++++++++++++---- 6 files changed, 112 insertions(+), 28 deletions(-) diff --git a/module/Core/src/Domain/DomainService.php b/module/Core/src/Domain/DomainService.php index 99bade7c..708984b5 100644 --- a/module/Core/src/Domain/DomainService.php +++ b/module/Core/src/Domain/DomainService.php @@ -59,15 +59,21 @@ class DomainService implements DomainServiceInterface return $domain; } - public function findByAuthority(string $authority): ?Domain + public function findByAuthority(string $authority, ?ApiKey $apiKey = null): ?Domain { $repo = $this->em->getRepository(Domain::class); - return $repo->findOneBy(['authority' => $authority]); + return $repo->findOneByAuthority($authority, $apiKey); } - public function getOrCreate(string $authority): Domain + public function getOrCreate(string $authority, ?ApiKey $apiKey = null): Domain { - $domain = $this->findByAuthority($authority) ?? Domain::withAuthority($authority); + $domain = $this->findByAuthority($authority, $apiKey); + if ($domain === null && $apiKey?->hasRole(Role::DOMAIN_SPECIFIC)) { + // This API key is restricted to one domain and a different one was tried to be fetched + throw DomainNotFoundException::fromAuthority($authority); + } + + $domain = $domain ?? Domain::withAuthority($authority); $this->em->persist($domain); $this->em->flush(); @@ -75,9 +81,12 @@ class DomainService implements DomainServiceInterface return $domain; } - public function configureNotFoundRedirects(string $authority, NotFoundRedirects $notFoundRedirects): Domain - { - $domain = $this->getOrCreate($authority); + public function configureNotFoundRedirects( + string $authority, + NotFoundRedirects $notFoundRedirects, + ?ApiKey $apiKey = null + ): Domain { + $domain = $this->getOrCreate($authority, $apiKey); $domain->configureNotFoundRedirects($notFoundRedirects); $this->em->flush(); diff --git a/module/Core/src/Domain/DomainServiceInterface.php b/module/Core/src/Domain/DomainServiceInterface.php index 802ecc7a..9ac48e69 100644 --- a/module/Core/src/Domain/DomainServiceInterface.php +++ b/module/Core/src/Domain/DomainServiceInterface.php @@ -22,9 +22,19 @@ interface DomainServiceInterface */ public function getDomain(string $domainId): Domain; - public function getOrCreate(string $authority): Domain; + /** + * @throws DomainNotFoundException If the API key is restricted to one domain and a different one is provided + */ + public function getOrCreate(string $authority, ?ApiKey $apiKey = null): Domain; - public function findByAuthority(string $authority): ?Domain; + public function findByAuthority(string $authority, ?ApiKey $apiKey = null): ?Domain; - public function configureNotFoundRedirects(string $authority, NotFoundRedirects $notFoundRedirects): Domain; + /** + * @throws DomainNotFoundException If the API key is restricted to one domain and a different one is provided + */ + public function configureNotFoundRedirects( + string $authority, + NotFoundRedirects $notFoundRedirects, + ?ApiKey $apiKey = null, + ): Domain; } diff --git a/module/Core/src/Domain/Repository/DomainRepository.php b/module/Core/src/Domain/Repository/DomainRepository.php index 6d246924..50033604 100644 --- a/module/Core/src/Domain/Repository/DomainRepository.php +++ b/module/Core/src/Domain/Repository/DomainRepository.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Domain\Repository; use Doctrine\ORM\Query\Expr\Join; +use Doctrine\ORM\QueryBuilder; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Happyr\DoctrineSpecification\Spec; use Shlinkio\Shlink\Core\Domain\Spec\IsDomain; @@ -22,14 +23,8 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe */ public function findDomainsWithout(?string $excludedAuthority, ?ApiKey $apiKey = null): array { - $qb = $this->createQueryBuilder('d'); - $qb->leftJoin(ShortUrl::class, 's', Join::WITH, 's.domain = d') - ->orderBy('d.authority', 'ASC') - ->groupBy('d') - ->having($qb->expr()->gt('COUNT(s.id)', '0')) - ->orHaving($qb->expr()->isNotNull('d.baseUrlRedirect')) - ->orHaving($qb->expr()->isNotNull('d.regular404Redirect')) - ->orHaving($qb->expr()->isNotNull('d.invalidShortUrlRedirect')); + $qb = $this->createPublicDomainsQueryBuilder(); + $qb->orderBy('d.authority', 'ASC'); $specs = $this->determineExtraSpecs($excludedAuthority, $apiKey); foreach ($specs as [$alias, $spec]) { @@ -39,6 +34,34 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe return $qb->getQuery()->getResult(); } + public function findOneByAuthority(string $authority, ?ApiKey $apiKey = null): ?Domain + { + $qb = $this->createPublicDomainsQueryBuilder(); + $qb->where($qb->expr()->eq('d.authority', ':authority')) + ->setParameter('authority', $authority) + ->setMaxResults(1); + + $specs = $this->determineExtraSpecs(null, $apiKey); + foreach ($specs as [$alias, $spec]) { + $this->applySpecification($qb, $spec, $alias); + } + + return $qb->getQuery()->getOneOrNullResult(); + } + + private function createPublicDomainsQueryBuilder(): QueryBuilder + { + $qb = $this->createQueryBuilder('d'); + $qb->leftJoin(ShortUrl::class, 's', Join::WITH, 's.domain = d') + ->groupBy('d') + ->having($qb->expr()->gt('COUNT(s.id)', '0')) + ->orHaving($qb->expr()->isNotNull('d.baseUrlRedirect')) + ->orHaving($qb->expr()->isNotNull('d.regular404Redirect')) + ->orHaving($qb->expr()->isNotNull('d.invalidShortUrlRedirect')); + + return $qb; + } + private function determineExtraSpecs(?string $excludedAuthority, ?ApiKey $apiKey): iterable { if ($excludedAuthority !== null) { diff --git a/module/Core/src/Domain/Repository/DomainRepositoryInterface.php b/module/Core/src/Domain/Repository/DomainRepositoryInterface.php index 1d201520..123e349d 100644 --- a/module/Core/src/Domain/Repository/DomainRepositoryInterface.php +++ b/module/Core/src/Domain/Repository/DomainRepositoryInterface.php @@ -15,4 +15,6 @@ interface DomainRepositoryInterface extends ObjectRepository, EntitySpecificatio * @return Domain[] */ public function findDomainsWithout(?string $excludedAuthority, ?ApiKey $apiKey = null): array; + + public function findOneByAuthority(string $authority, ?ApiKey $apiKey = null): ?Domain; } diff --git a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php index 9e3fe9be..f52ba54a 100644 --- a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php +++ b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php @@ -27,7 +27,7 @@ class DomainRepositoryTest extends DatabaseTestCase } /** @test */ - public function findDomainsReturnsExpectedResult(): void + public function expectedDomainsAreFoundWhenNoApiKeyIsInvolved(): void { $fooDomain = Domain::withAuthority('foo.com'); $this->getEntityManager()->persist($fooDomain); @@ -70,10 +70,15 @@ class DomainRepositoryTest extends DatabaseTestCase [$barDomain, $bazDomain, $fooDomain], $this->repo->findDomainsWithout('detached-with-redirects.com'), ); + + self::assertEquals($barDomain, $this->repo->findOneByAuthority('bar.com')); + self::assertEquals($detachedWithRedirects, $this->repo->findOneByAuthority('detached-with-redirects.com')); + self::assertNull($this->repo->findOneByAuthority('does-not-exist.com')); + self::assertNull($this->repo->findOneByAuthority('detached.com')); } /** @test */ - public function findDomainsReturnsJustThoseMatchingProvidedApiKey(): void + public function expectedDomainsAreFoundWhenApiKeyIsProvided(): void { $authorApiKey = ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls())); $this->getEntityManager()->persist($authorApiKey); @@ -124,6 +129,15 @@ class DomainRepositoryTest extends DatabaseTestCase ); self::assertEquals([$bazDomain, $fooDomain], $this->repo->findDomainsWithout(null, $authorApiKey)); self::assertEquals([], $this->repo->findDomainsWithout(null, $authorAndDomainApiKey)); + + self::assertEquals($fooDomain, $this->repo->findOneByAuthority('foo.com', $authorApiKey)); + self::assertNull($this->repo->findOneByAuthority('bar.com', $authorApiKey)); + self::assertEquals($barDomain, $this->repo->findOneByAuthority('bar.com', $barDomainApiKey)); + self::assertEquals( + $detachedWithRedirects, + $this->repo->findOneByAuthority('detached-with-redirects.com', $detachedWithRedirectsApiKey), + ); + self::assertNull($this->repo->findOneByAuthority('foo.com', $detachedWithRedirectsApiKey)); } private function createShortUrl(Domain $domain, ?ApiKey $apiKey = null): ShortUrl diff --git a/module/Core/test/Domain/DomainServiceTest.php b/module/Core/test/Domain/DomainServiceTest.php index b53b4a26..6e91b425 100644 --- a/module/Core/test/Domain/DomainServiceTest.php +++ b/module/Core/test/Domain/DomainServiceTest.php @@ -133,16 +133,16 @@ class DomainServiceTest extends TestCase * @test * @dataProvider provideFoundDomains */ - public function getOrCreateAlwaysPersistsDomain(?Domain $foundDomain): void + public function getOrCreateAlwaysPersistsDomain(?Domain $foundDomain, ?ApiKey $apiKey): void { $authority = 'example.com'; $repo = $this->prophesize(DomainRepositoryInterface::class); - $repo->findOneBy(['authority' => $authority])->willReturn($foundDomain); + $repo->findOneByAuthority($authority, $apiKey)->willReturn($foundDomain); $getRepo = $this->em->getRepository(Domain::class)->willReturn($repo->reveal()); $persist = $this->em->persist($foundDomain ?? Argument::type(Domain::class)); $flush = $this->em->flush(); - $result = $this->domainService->getOrCreate($authority); + $result = $this->domainService->getOrCreate($authority, $apiKey); if ($foundDomain !== null) { self::assertSame($result, $foundDomain); @@ -152,15 +152,33 @@ class DomainServiceTest extends TestCase $flush->shouldHaveBeenCalledOnce(); } + /** @test */ + public function getOrCreateThrowsExceptionForApiKeysWithDomainRole(): void + { + $authority = 'example.com'; + $domain = Domain::withAuthority($authority)->setId('1'); + $apiKey = ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forDomain($domain))); + $repo = $this->prophesize(DomainRepositoryInterface::class); + $repo->findOneByAuthority($authority, $apiKey)->willReturn(null); + $getRepo = $this->em->getRepository(Domain::class)->willReturn($repo->reveal()); + + $this->expectException(DomainNotFoundException::class); + $getRepo->shouldBeCalledOnce(); + $this->em->persist(Argument::cetera())->shouldNotBeCalled(); + $this->em->flush()->shouldNotBeCalled(); + + $this->domainService->getOrCreate($authority, $apiKey); + } + /** * @test * @dataProvider provideFoundDomains */ - public function configureNotFoundRedirectsConfiguresFetchedDomain(?Domain $foundDomain): void + public function configureNotFoundRedirectsConfiguresFetchedDomain(?Domain $foundDomain, ?ApiKey $apiKey): void { $authority = 'example.com'; $repo = $this->prophesize(DomainRepositoryInterface::class); - $repo->findOneBy(['authority' => $authority])->willReturn($foundDomain); + $repo->findOneByAuthority($authority, $apiKey)->willReturn($foundDomain); $getRepo = $this->em->getRepository(Domain::class)->willReturn($repo->reveal()); $persist = $this->em->persist($foundDomain ?? Argument::type(Domain::class)); $flush = $this->em->flush(); @@ -169,7 +187,7 @@ class DomainServiceTest extends TestCase 'foo.com', 'bar.com', 'baz.com', - )); + ), $apiKey); if ($foundDomain !== null) { self::assertSame($result, $foundDomain); @@ -184,7 +202,15 @@ class DomainServiceTest extends TestCase public function provideFoundDomains(): iterable { - yield 'domain not found' => [null]; - yield 'domain found' => [Domain::withAuthority('')]; + $domain = Domain::withAuthority(''); + $adminApiKey = ApiKey::create(); + $authorApiKey = ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls())); + + yield 'domain not found and no API key' => [null, null]; + yield 'domain found and no API key' => [$domain, null]; + yield 'domain not found and admin API key' => [null, $adminApiKey]; + yield 'domain found and admin API key' => [$domain, $adminApiKey]; + yield 'domain not found and author API key' => [null, $authorApiKey]; + yield 'domain found and author API key' => [$domain, $authorApiKey]; } } From 6a40bbdcb54cf603eb96a6a5cd923a84aaada269 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 30 Jul 2021 18:53:57 +0200 Subject: [PATCH 05/17] Created new action to set redirects for a domain --- docs/swagger/paths/v2_domains_redirects.json | 33 ++++++++++ module/Core/src/Config/NotFoundRedirects.php | 13 +++- module/Rest/config/dependencies.config.php | 2 + module/Rest/config/routes.config.php | 1 + .../Action/Domain/DomainRedirectsAction.php | 41 +++++++++++++ .../src/Action/Domain/ListDomainsAction.php | 2 + .../Domain/Request/DomainRedirectsRequest.php | 60 +++++++++++++++++++ 7 files changed, 151 insertions(+), 1 deletion(-) create mode 100644 module/Rest/src/Action/Domain/DomainRedirectsAction.php create mode 100644 module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php diff --git a/docs/swagger/paths/v2_domains_redirects.json b/docs/swagger/paths/v2_domains_redirects.json index 0868504e..bba6fbb7 100644 --- a/docs/swagger/paths/v2_domains_redirects.json +++ b/docs/swagger/paths/v2_domains_redirects.json @@ -66,6 +66,39 @@ } } }, + "400": { + "description": "Provided data is invalid.", + "content": { + "application/problem+json": { + "schema": { + "type": "object", + "allOf": [ + { + "$ref": "../definitions/Error.json" + }, + { + "type": "object", + "required": ["invalidElements"], + "properties": { + "invalidElements": { + "type": "array", + "items": { + "type": "string", + "enum": [ + "domain", + "baseUrlRedirect", + "regular404Redirect", + "invalidShortUrlRedirect" + ] + } + } + } + } + ] + } + } + } + }, "500": { "description": "Unexpected error.", "content": { diff --git a/module/Core/src/Config/NotFoundRedirects.php b/module/Core/src/Config/NotFoundRedirects.php index 2a1e68d4..bb8c578c 100644 --- a/module/Core/src/Config/NotFoundRedirects.php +++ b/module/Core/src/Config/NotFoundRedirects.php @@ -4,7 +4,9 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Config; -final class NotFoundRedirects +use JsonSerializable; + +final class NotFoundRedirects implements JsonSerializable { public function __construct( private ?string $baseUrlRedirect = null, @@ -27,4 +29,13 @@ final class NotFoundRedirects { return $this->invalidShortUrlRedirect; } + + public function jsonSerialize(): array + { + return [ + 'baseUrlRedirect' => $this->baseUrlRedirect, + 'regular404Redirect' => $this->regular404Redirect, + 'invalidShortUrlRedirect' => $this->invalidShortUrlRedirect, + ]; + } } diff --git a/module/Rest/config/dependencies.config.php b/module/Rest/config/dependencies.config.php index e1a869df..5e0267d6 100644 --- a/module/Rest/config/dependencies.config.php +++ b/module/Rest/config/dependencies.config.php @@ -40,6 +40,7 @@ return [ Action\Tag\CreateTagsAction::class => ConfigAbstractFactory::class, Action\Tag\UpdateTagAction::class => ConfigAbstractFactory::class, Action\Domain\ListDomainsAction::class => ConfigAbstractFactory::class, + Action\Domain\DomainRedirectsAction::class => ConfigAbstractFactory::class, ImplicitOptionsMiddleware::class => Middleware\EmptyResponseImplicitOptionsMiddlewareFactory::class, Middleware\BodyParserMiddleware::class => InvokableFactory::class, @@ -81,6 +82,7 @@ return [ Action\Tag\CreateTagsAction::class => [TagService::class], Action\Tag\UpdateTagAction::class => [TagService::class], Action\Domain\ListDomainsAction::class => [DomainService::class], + Action\Domain\DomainRedirectsAction::class => [DomainService::class], Middleware\CrossDomainMiddleware::class => ['config.cors'], Middleware\ShortUrl\DropDefaultDomainFromRequestMiddleware::class => ['config.url_shortener.domain.hostname'], diff --git a/module/Rest/config/routes.config.php b/module/Rest/config/routes.config.php index 9b09a266..991f4bb3 100644 --- a/module/Rest/config/routes.config.php +++ b/module/Rest/config/routes.config.php @@ -44,6 +44,7 @@ return [ // Domains Action\Domain\ListDomainsAction::getRouteDef(), + Action\Domain\DomainRedirectsAction::getRouteDef(), Action\MercureInfoAction::getRouteDef(), ], diff --git a/module/Rest/src/Action/Domain/DomainRedirectsAction.php b/module/Rest/src/Action/Domain/DomainRedirectsAction.php new file mode 100644 index 00000000..d9259582 --- /dev/null +++ b/module/Rest/src/Action/Domain/DomainRedirectsAction.php @@ -0,0 +1,41 @@ +getParsedBody(); + $requestData = DomainRedirectsRequest::fromRawData($body); + $apiKey = AuthenticationMiddleware::apiKeyFromRequest($request); + + $authority = $requestData->authority(); + $domain = $this->domainService->getOrCreate($authority); + $notFoundRedirects = $requestData->toNotFoundRedirects($domain); + + $this->domainService->configureNotFoundRedirects($authority, $notFoundRedirects, $apiKey); + + return new JsonResponse($notFoundRedirects); + } +} diff --git a/module/Rest/src/Action/Domain/ListDomainsAction.php b/module/Rest/src/Action/Domain/ListDomainsAction.php index c8f9a475..3d9205f8 100644 --- a/module/Rest/src/Action/Domain/ListDomainsAction.php +++ b/module/Rest/src/Action/Domain/ListDomainsAction.php @@ -25,6 +25,8 @@ class ListDomainsAction extends AbstractRestAction $apiKey = AuthenticationMiddleware::apiKeyFromRequest($request); $domainItems = $this->domainService->listDomains($apiKey); + // TODO Support including not found redirects if requested via query param + return new JsonResponse([ 'domains' => [ 'data' => $domainItems, diff --git a/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php b/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php new file mode 100644 index 00000000..d43ca223 --- /dev/null +++ b/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php @@ -0,0 +1,60 @@ +validateAndInit($payload); + return $instance; + } + + private function validateAndInit(array $payload): void + { + // TODO Validate data + $this->baseUrlRedirectWasProvided = array_key_exists('baseUrlRedirect', $payload); + $this->regular404RedirectWasProvided = array_key_exists('regular404Redirect', $payload); + $this->invalidShortUrlRedirectWasProvided = array_key_exists('invalidShortUrlRedirect', $payload); + + $this->authority = $payload['domain']; + $this->baseUrlRedirect = $payload['baseUrlRedirect'] ?? null; + $this->regular404Redirect = $payload['regular404Redirect'] ?? null; + $this->invalidShortUrlRedirect = $payload['invalidShortUrlRedirect'] ?? null; + } + + public function authority(): string + { + return $this->authority; + } + + public function toNotFoundRedirects(?NotFoundRedirectConfigInterface $defaults = null): NotFoundRedirects + { + return new NotFoundRedirects( + $this->baseUrlRedirectWasProvided ? $this->baseUrlRedirect : $defaults?->baseUrlRedirect(), + $this->regular404RedirectWasProvided ? $this->regular404Redirect : $defaults?->regular404Redirect(), + $this->invalidShortUrlRedirectWasProvided + ? $this->invalidShortUrlRedirect + : $defaults?->invalidShortUrlRedirect(), + ); + } +} From b78660c685a94db536d5b7c913fb088e5ad23447 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 2 Aug 2021 18:16:13 +0200 Subject: [PATCH 06/17] Updated installer --- composer.json | 2 +- .../Rest/src/Action/Domain/Request/DomainRedirectsRequest.php | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/composer.json b/composer.json index e7338665..0b0e1ac4 100644 --- a/composer.json +++ b/composer.json @@ -51,7 +51,7 @@ "shlinkio/shlink-config": "^1.0", "shlinkio/shlink-event-dispatcher": "^2.1", "shlinkio/shlink-importer": "^2.3.1", - "shlinkio/shlink-installer": "dev-develop#fa6a4ca as 6.1", + "shlinkio/shlink-installer": "dev-develop#0ddcc3d as 6.1", "shlinkio/shlink-ip-geolocation": "^2.0", "symfony/console": "^5.1", "symfony/filesystem": "^5.1", diff --git a/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php b/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php index d43ca223..8b9f69e5 100644 --- a/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php +++ b/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Rest\Action\Domain\Request; use Shlinkio\Shlink\Core\Config\NotFoundRedirectConfigInterface; use Shlinkio\Shlink\Core\Config\NotFoundRedirects; + use function array_key_exists; class DomainRedirectsRequest From 6860855c717474e3522df634428c43f3a8bccc5c Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 3 Aug 2021 09:55:21 +0200 Subject: [PATCH 07/17] Prevent double flush when editing domain redirects --- module/Core/src/Domain/DomainService.php | 45 ++++++++++++------- module/Core/test/Domain/DomainServiceTest.php | 2 +- 2 files changed, 31 insertions(+), 16 deletions(-) diff --git a/module/Core/src/Domain/DomainService.php b/module/Core/src/Domain/DomainService.php index 708984b5..807c6dce 100644 --- a/module/Core/src/Domain/DomainService.php +++ b/module/Core/src/Domain/DomainService.php @@ -65,7 +65,37 @@ class DomainService implements DomainServiceInterface return $repo->findOneByAuthority($authority, $apiKey); } + /** + * @throws DomainNotFoundException + */ public function getOrCreate(string $authority, ?ApiKey $apiKey = null): Domain + { + $domain = $this->getPersistedDomain($authority, $apiKey); + $this->em->flush(); + + return $domain; + } + + /** + * @throws DomainNotFoundException + */ + public function configureNotFoundRedirects( + string $authority, + NotFoundRedirects $notFoundRedirects, + ?ApiKey $apiKey = null + ): Domain { + $domain = $this->getPersistedDomain($authority, $apiKey); + $domain->configureNotFoundRedirects($notFoundRedirects); + + $this->em->flush(); + + return $domain; + } + + /** + * @throws DomainNotFoundException + */ + private function getPersistedDomain(string $authority, ?ApiKey $apiKey): Domain { $domain = $this->findByAuthority($authority, $apiKey); if ($domain === null && $apiKey?->hasRole(Role::DOMAIN_SPECIFIC)) { @@ -74,22 +104,7 @@ class DomainService implements DomainServiceInterface } $domain = $domain ?? Domain::withAuthority($authority); - $this->em->persist($domain); - $this->em->flush(); - - return $domain; - } - - public function configureNotFoundRedirects( - string $authority, - NotFoundRedirects $notFoundRedirects, - ?ApiKey $apiKey = null - ): Domain { - $domain = $this->getOrCreate($authority, $apiKey); - $domain->configureNotFoundRedirects($notFoundRedirects); - - $this->em->flush(); return $domain; } diff --git a/module/Core/test/Domain/DomainServiceTest.php b/module/Core/test/Domain/DomainServiceTest.php index 6e91b425..060b24ab 100644 --- a/module/Core/test/Domain/DomainServiceTest.php +++ b/module/Core/test/Domain/DomainServiceTest.php @@ -197,7 +197,7 @@ class DomainServiceTest extends TestCase self::assertEquals('baz.com', $result->invalidShortUrlRedirect()); $getRepo->shouldHaveBeenCalledOnce(); $persist->shouldHaveBeenCalledOnce(); - $flush->shouldHaveBeenCalledTimes(2); + $flush->shouldHaveBeenCalledOnce(); } public function provideFoundDomains(): iterable From 8fbf05acd4c6eeb03fc584924271c68ec2308753 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 3 Aug 2021 10:02:44 +0200 Subject: [PATCH 08/17] Added deprecated keyword to ensure something is changed for v3.0.0 --- module/Core/src/Exception/DeleteShortUrlException.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/Core/src/Exception/DeleteShortUrlException.php b/module/Core/src/Exception/DeleteShortUrlException.php index f98b7e14..600fca57 100644 --- a/module/Core/src/Exception/DeleteShortUrlException.php +++ b/module/Core/src/Exception/DeleteShortUrlException.php @@ -15,7 +15,7 @@ class DeleteShortUrlException extends DomainException implements ProblemDetailsE use CommonProblemDetailsExceptionTrait; private const TITLE = 'Cannot delete short URL'; - private const TYPE = 'INVALID_SHORTCODE_DELETION'; // FIXME Should be INVALID_SHORT_URL_DELETION + private const TYPE = 'INVALID_SHORTCODE_DELETION'; // FIXME Deprecated: Should be INVALID_SHORT_URL_DELETION public static function fromVisitsThreshold(int $threshold, string $shortCode): self { From 20f70b8b07066cc92f6d065401122211663a2873 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 3 Aug 2021 10:21:42 +0200 Subject: [PATCH 09/17] Created new table with row separators for CLI, to use with multi-line rows --- .../src/Command/Api/GenerateKeyCommand.php | 2 +- .../CLI/src/Command/Api/ListKeysCommand.php | 2 +- .../src/Command/Domain/ListDomainsCommand.php | 3 ++- .../src/Command/ShortUrl/GetVisitsCommand.php | 2 +- .../Command/ShortUrl/ListShortUrlsCommand.php | 2 +- .../CLI/src/Command/Tag/ListTagsCommand.php | 2 +- module/CLI/src/Util/ShlinkTable.php | 22 +++++++++++++++---- .../test/Command/Api/ListKeysCommandTest.php | 11 ++++++++++ .../Command/Domain/ListDomainsCommandTest.php | 2 ++ module/CLI/test/Util/ShlinkTableTest.php | 4 ++-- 10 files changed, 40 insertions(+), 12 deletions(-) diff --git a/module/CLI/src/Command/Api/GenerateKeyCommand.php b/module/CLI/src/Command/Api/GenerateKeyCommand.php index c8f607a4..d39c05fa 100644 --- a/module/CLI/src/Command/Api/GenerateKeyCommand.php +++ b/module/CLI/src/Command/Api/GenerateKeyCommand.php @@ -97,7 +97,7 @@ class GenerateKeyCommand extends BaseCommand $io->success(sprintf('Generated API key: "%s"', $apiKey->toString())); if (! $apiKey->isAdmin()) { - ShlinkTable::fromOutput($io)->render( + ShlinkTable::default($io)->render( ['Role name', 'Role metadata'], $apiKey->mapRoles(fn (string $name, array $meta) => [$name, arrayToString($meta, 0)]), null, diff --git a/module/CLI/src/Command/Api/ListKeysCommand.php b/module/CLI/src/Command/Api/ListKeysCommand.php index f435f1ea..23258993 100644 --- a/module/CLI/src/Command/Api/ListKeysCommand.php +++ b/module/CLI/src/Command/Api/ListKeysCommand.php @@ -69,7 +69,7 @@ class ListKeysCommand extends BaseCommand return $rowData; }); - ShlinkTable::fromOutput($output)->render(array_filter([ + ShlinkTable::withRowSeparators($output)->render(array_filter([ 'Key', 'Name', ! $enabledOnly ? 'Is enabled' : null, diff --git a/module/CLI/src/Command/Domain/ListDomainsCommand.php b/module/CLI/src/Command/Domain/ListDomainsCommand.php index 5e368170..447bf92f 100644 --- a/module/CLI/src/Command/Domain/ListDomainsCommand.php +++ b/module/CLI/src/Command/Domain/ListDomainsCommand.php @@ -43,8 +43,9 @@ class ListDomainsCommand extends Command $domains = $this->domainService->listDomains(); $showRedirects = $input->getOption('show-redirects'); $commonFields = ['Domain', 'Is default']; + $table = $showRedirects ? ShlinkTable::withRowSeparators($output) : ShlinkTable::default($output); - ShlinkTable::fromOutput($output)->render( + $table->render( $showRedirects ? [...$commonFields, '"Not found" redirects'] : $commonFields, map($domains, function (DomainItem $domain) use ($showRedirects) { $commonValues = [$domain->toString(), $domain->isDefault() ? 'Yes' : 'No']; diff --git a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php index aac45aff..5113debc 100644 --- a/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php +++ b/module/CLI/src/Command/ShortUrl/GetVisitsCommand.php @@ -81,7 +81,7 @@ class GetVisitsCommand extends AbstractWithDateRangeCommand $rowData['country'] = ($visit->getVisitLocation() ?? new UnknownVisitLocation())->getCountryName(); return select_keys($rowData, ['referer', 'date', 'userAgent', 'country']); }); - ShlinkTable::fromOutput($output)->render(['Referer', 'Date', 'User agent', 'Country'], $rows); + ShlinkTable::default($output)->render(['Referer', 'Date', 'User agent', 'Country'], $rows); return ExitCodes::EXIT_SUCCESS; } diff --git a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php index b5d242dc..ff01030a 100644 --- a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php +++ b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php @@ -164,7 +164,7 @@ class ListShortUrlsCommand extends AbstractWithDateRangeCommand return map($columnsMap, fn (callable $call) => $call($rawShortUrl, $shortUrl)); }); - ShlinkTable::fromOutput($output)->render( + ShlinkTable::default($output)->render( array_keys($columnsMap), $rows, $all ? null : $this->formatCurrentPageMessage($shortUrls, 'Page %s of %s'), diff --git a/module/CLI/src/Command/Tag/ListTagsCommand.php b/module/CLI/src/Command/Tag/ListTagsCommand.php index 99889fa3..61d4e6e0 100644 --- a/module/CLI/src/Command/Tag/ListTagsCommand.php +++ b/module/CLI/src/Command/Tag/ListTagsCommand.php @@ -32,7 +32,7 @@ class ListTagsCommand extends Command protected function execute(InputInterface $input, OutputInterface $output): ?int { - ShlinkTable::fromOutput($output)->render(['Name', 'URLs amount', 'Visits amount'], $this->getTagsRows()); + ShlinkTable::default($output)->render(['Name', 'URLs amount', 'Visits amount'], $this->getTagsRows()); return ExitCodes::EXIT_SUCCESS; } diff --git a/module/CLI/src/Util/ShlinkTable.php b/module/CLI/src/Util/ShlinkTable.php index 5788ce12..1d4143c1 100644 --- a/module/CLI/src/Util/ShlinkTable.php +++ b/module/CLI/src/Util/ShlinkTable.php @@ -5,20 +5,33 @@ declare(strict_types=1); namespace Shlinkio\Shlink\CLI\Util; use Symfony\Component\Console\Helper\Table; +use Symfony\Component\Console\Helper\TableSeparator; use Symfony\Component\Console\Output\OutputInterface; +use function Functional\intersperse; + final class ShlinkTable { private const DEFAULT_STYLE_NAME = 'default'; private const TABLE_TITLE_STYLE = ' %s '; - public function __construct(private Table $baseTable) + private function __construct(private Table $baseTable, private bool $withRowSeparators) { } - public static function fromOutput(OutputInterface $output): self + public static function default(OutputInterface $output): self { - return new self(new Table($output)); + return new self(new Table($output), false); + } + + public static function withRowSeparators(OutputInterface $output): self + { + return new self(new Table($output), true); + } + + public static function fromBaseTable(Table $baseTable): self + { + return new self($baseTable, false); } public function render(array $headers, array $rows, ?string $footerTitle = null, ?string $headerTitle = null): void @@ -26,11 +39,12 @@ final class ShlinkTable $style = Table::getStyleDefinition(self::DEFAULT_STYLE_NAME); $style->setFooterTitleFormat(self::TABLE_TITLE_STYLE) ->setHeaderTitleFormat(self::TABLE_TITLE_STYLE); + $tableRows = $this->withRowSeparators ? intersperse($rows, new TableSeparator()) : $rows; $table = clone $this->baseTable; $table->setStyle($style) ->setHeaders($headers) - ->setRows($rows) + ->setRows($tableRows) ->setFooterTitle($footerTitle) ->setHeaderTitle($headerTitle) ->render(); diff --git a/module/CLI/test/Command/Api/ListKeysCommandTest.php b/module/CLI/test/Command/Api/ListKeysCommandTest.php index 389c6bbd..a124993f 100644 --- a/module/CLI/test/Command/Api/ListKeysCommandTest.php +++ b/module/CLI/test/Command/Api/ListKeysCommandTest.php @@ -53,7 +53,9 @@ class ListKeysCommandTest extends TestCase | Key | Name | Is enabled | Expiration date | Roles | +--------------------------------------+------+------------+-----------------+-------+ | {$apiKey1} | - | +++ | - | Admin | + +--------------------------------------+------+------------+-----------------+-------+ | {$apiKey2} | - | +++ | - | Admin | + +--------------------------------------+------+------------+-----------------+-------+ | {$apiKey3} | - | +++ | - | Admin | +--------------------------------------+------+------------+-----------------+-------+ @@ -67,6 +69,7 @@ class ListKeysCommandTest extends TestCase | Key | Name | Expiration date | Roles | +--------------------------------------+------+-----------------+-------+ | {$apiKey1} | - | - | Admin | + +--------------------------------------+------+-----------------+-------+ | {$apiKey2} | - | - | Admin | +--------------------------------------+------+-----------------+-------+ @@ -92,11 +95,16 @@ class ListKeysCommandTest extends TestCase | Key | Name | Expiration date | Roles | +--------------------------------------+------+-----------------+--------------------------+ | {$apiKey1} | - | - | Admin | + +--------------------------------------+------+-----------------+--------------------------+ | {$apiKey2} | - | - | Author only | + +--------------------------------------+------+-----------------+--------------------------+ | {$apiKey3} | - | - | Domain only: example.com | + +--------------------------------------+------+-----------------+--------------------------+ | {$apiKey4} | - | - | Admin | + +--------------------------------------+------+-----------------+--------------------------+ | {$apiKey5} | - | - | Author only | | | | | Domain only: example.com | + +--------------------------------------+------+-----------------+--------------------------+ | {$apiKey6} | - | - | Admin | +--------------------------------------+------+-----------------+--------------------------+ @@ -115,8 +123,11 @@ class ListKeysCommandTest extends TestCase | Key | Name | Expiration date | Roles | +--------------------------------------+---------------+-----------------+-------+ | {$apiKey1} | Alice | - | Admin | + +--------------------------------------+---------------+-----------------+-------+ | {$apiKey2} | Alice and Bob | - | Admin | + +--------------------------------------+---------------+-----------------+-------+ | {$apiKey3} | | - | Admin | + +--------------------------------------+---------------+-----------------+-------+ | {$apiKey4} | - | - | Admin | +--------------------------------------+---------------+-----------------+-------+ diff --git a/module/CLI/test/Command/Domain/ListDomainsCommandTest.php b/module/CLI/test/Command/Domain/ListDomainsCommandTest.php index 9f4be920..3f31f7e0 100644 --- a/module/CLI/test/Command/Domain/ListDomainsCommandTest.php +++ b/module/CLI/test/Command/Domain/ListDomainsCommandTest.php @@ -77,9 +77,11 @@ class ListDomainsCommandTest extends TestCase | foo.com | Yes | * Base URL: https://foo.com/default/base | | | | * Regular 404: N/A | | | | * Invalid short URL: https://foo.com/default/invalid | + +---------+------------+---------------------------------------------------------+ | bar.com | No | * Base URL: N/A | | | | * Regular 404: N/A | | | | * Invalid short URL: N/A | + +---------+------------+---------------------------------------------------------+ | baz.com | No | * Base URL: N/A | | | | * Regular 404: https://foo.com/baz-domain/regular | | | | * Invalid short URL: https://foo.com/baz-domain/invalid | diff --git a/module/CLI/test/Util/ShlinkTableTest.php b/module/CLI/test/Util/ShlinkTableTest.php index 71bff82b..1ca612d4 100644 --- a/module/CLI/test/Util/ShlinkTableTest.php +++ b/module/CLI/test/Util/ShlinkTableTest.php @@ -24,7 +24,7 @@ class ShlinkTableTest extends TestCase public function setUp(): void { $this->baseTable = $this->prophesize(Table::class); - $this->shlinkTable = new ShlinkTable($this->baseTable->reveal()); + $this->shlinkTable = ShlinkTable::fromBaseTable($this->baseTable->reveal()); } /** @test */ @@ -57,7 +57,7 @@ class ShlinkTableTest extends TestCase /** @test */ public function newTableIsCreatedForFactoryMethod(): void { - $instance = ShlinkTable::fromOutput($this->prophesize(OutputInterface::class)->reveal()); + $instance = ShlinkTable::default($this->prophesize(OutputInterface::class)->reveal()); $ref = new ReflectionObject($instance); $baseTable = $ref->getProperty('baseTable'); From 9f25979b4c67812a2be3f2ee44751c1d2c44c14b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 3 Aug 2021 14:08:36 +0200 Subject: [PATCH 10/17] Added validation to not found redirects for domain --- .../Command/Domain/DomainRedirectsCommand.php | 2 +- .../Domain/DomainRedirectsCommandTest.php | 12 ++--- .../Command/Domain/ListDomainsCommandTest.php | 2 +- module/Core/src/Config/NotFoundRedirects.php | 21 ++++++-- .../Validation/DomainRedirectsInputFilter.php | 50 +++++++++++++++++++ .../Repository/DomainRepositoryTest.php | 4 +- module/Core/test/Domain/DomainServiceTest.php | 2 +- .../Domain/Request/DomainRedirectsRequest.php | 36 +++++++++---- .../Rest/test-api/Fixtures/DomainFixture.php | 2 +- 9 files changed, 106 insertions(+), 25 deletions(-) create mode 100644 module/Core/src/Domain/Validation/DomainRedirectsInputFilter.php diff --git a/module/CLI/src/Command/Domain/DomainRedirectsCommand.php b/module/CLI/src/Command/Domain/DomainRedirectsCommand.php index 9a97e5fd..90cfd1f7 100644 --- a/module/CLI/src/Command/Domain/DomainRedirectsCommand.php +++ b/module/CLI/src/Command/Domain/DomainRedirectsCommand.php @@ -92,7 +92,7 @@ class DomainRedirectsCommand extends Command }; }; - $this->domainService->configureNotFoundRedirects($domainAuthority, new NotFoundRedirects( + $this->domainService->configureNotFoundRedirects($domainAuthority, NotFoundRedirects::withRedirects( $ask( 'URL to redirect to when a user hits this domain\'s base URL', $domain?->baseUrlRedirect(), diff --git a/module/CLI/test/Command/Domain/DomainRedirectsCommandTest.php b/module/CLI/test/Command/Domain/DomainRedirectsCommandTest.php index 1f8b93ab..9801930e 100644 --- a/module/CLI/test/Command/Domain/DomainRedirectsCommandTest.php +++ b/module/CLI/test/Command/Domain/DomainRedirectsCommandTest.php @@ -40,7 +40,7 @@ class DomainRedirectsCommandTest extends TestCase $findDomain = $this->domainService->findByAuthority($domainAuthority)->willReturn($domain); $configureRedirects = $this->domainService->configureNotFoundRedirects( $domainAuthority, - new NotFoundRedirects('foo.com', null, 'baz.com'), + NotFoundRedirects::withRedirects('foo.com', null, 'baz.com'), )->willReturn(Domain::withAuthority('')); $this->commandTester->setInputs(['foo.com', '', 'baz.com']); @@ -71,12 +71,12 @@ class DomainRedirectsCommandTest extends TestCase { $domainAuthority = 'example.com'; $domain = Domain::withAuthority($domainAuthority); - $domain->configureNotFoundRedirects(new NotFoundRedirects('foo.com', 'bar.com', 'baz.com')); + $domain->configureNotFoundRedirects(NotFoundRedirects::withRedirects('foo.com', 'bar.com', 'baz.com')); $findDomain = $this->domainService->findByAuthority($domainAuthority)->willReturn($domain); $configureRedirects = $this->domainService->configureNotFoundRedirects( $domainAuthority, - new NotFoundRedirects(null, 'edited.com', 'baz.com'), + NotFoundRedirects::withRedirects(null, 'edited.com', 'baz.com'), )->willReturn($domain); $this->commandTester->setInputs(['2', '1', 'edited.com', '0']); @@ -105,7 +105,7 @@ class DomainRedirectsCommandTest extends TestCase $findDomain = $this->domainService->findByAuthority($domainAuthority)->willReturn($domain); $configureRedirects = $this->domainService->configureNotFoundRedirects( $domainAuthority, - new NotFoundRedirects(), + NotFoundRedirects::withoutRedirects(), )->willReturn($domain); $this->commandTester->setInputs([$domainAuthority, '', '', '']); @@ -132,7 +132,7 @@ class DomainRedirectsCommandTest extends TestCase $findDomain = $this->domainService->findByAuthority($domainAuthority)->willReturn($domain); $configureRedirects = $this->domainService->configureNotFoundRedirects( $domainAuthority, - new NotFoundRedirects(), + NotFoundRedirects::withoutRedirects(), )->willReturn($domain); $this->commandTester->setInputs(['1', '', '', '']); @@ -162,7 +162,7 @@ class DomainRedirectsCommandTest extends TestCase $findDomain = $this->domainService->findByAuthority($domainAuthority)->willReturn($domain); $configureRedirects = $this->domainService->configureNotFoundRedirects( $domainAuthority, - new NotFoundRedirects(), + NotFoundRedirects::withoutRedirects(), )->willReturn($domain); $this->commandTester->setInputs(['2', $domainAuthority, '', '', '']); diff --git a/module/CLI/test/Command/Domain/ListDomainsCommandTest.php b/module/CLI/test/Command/Domain/ListDomainsCommandTest.php index 3f31f7e0..13e6d062 100644 --- a/module/CLI/test/Command/Domain/ListDomainsCommandTest.php +++ b/module/CLI/test/Command/Domain/ListDomainsCommandTest.php @@ -36,7 +36,7 @@ class ListDomainsCommandTest extends TestCase public function allDomainsAreProperlyPrinted(array $input, string $expectedOutput): void { $bazDomain = Domain::withAuthority('baz.com'); - $bazDomain->configureNotFoundRedirects(new NotFoundRedirects( + $bazDomain->configureNotFoundRedirects(NotFoundRedirects::withRedirects( null, 'https://foo.com/baz-domain/regular', 'https://foo.com/baz-domain/invalid', diff --git a/module/Core/src/Config/NotFoundRedirects.php b/module/Core/src/Config/NotFoundRedirects.php index bb8c578c..c00d35d0 100644 --- a/module/Core/src/Config/NotFoundRedirects.php +++ b/module/Core/src/Config/NotFoundRedirects.php @@ -8,13 +8,26 @@ use JsonSerializable; final class NotFoundRedirects implements JsonSerializable { - public function __construct( - private ?string $baseUrlRedirect = null, - private ?string $regular404Redirect = null, - private ?string $invalidShortUrlRedirect = null, + private function __construct( + private ?string $baseUrlRedirect, + private ?string $regular404Redirect, + private ?string $invalidShortUrlRedirect, ) { } + public static function withRedirects( + ?string $baseUrlRedirect = null, + ?string $regular404Redirect = null, + ?string $invalidShortUrlRedirect = null, + ): self { + return new self($baseUrlRedirect, $regular404Redirect, $invalidShortUrlRedirect); + } + + public static function withoutRedirects(): self + { + return new self(null, null, null); + } + public function baseUrlRedirect(): ?string { return $this->baseUrlRedirect; diff --git a/module/Core/src/Domain/Validation/DomainRedirectsInputFilter.php b/module/Core/src/Domain/Validation/DomainRedirectsInputFilter.php new file mode 100644 index 00000000..94c15217 --- /dev/null +++ b/module/Core/src/Domain/Validation/DomainRedirectsInputFilter.php @@ -0,0 +1,50 @@ +initializeInputs(); + $instance->setData($data); + + return $instance; + } + + private function initializeInputs(): void + { + $domain = $this->createInput(self::DOMAIN); + $domain->getValidatorChain()->attach(new Validator\NotEmpty([ + Validator\NotEmpty::OBJECT, + Validator\NotEmpty::SPACE, + Validator\NotEmpty::NULL, + Validator\NotEmpty::EMPTY_ARRAY, + Validator\NotEmpty::BOOLEAN, + ])); + $this->add($domain); + + $this->add($this->createInput(self::BASE_URL_REDIRECT, false)); + $this->add($this->createInput(self::REGULAR_404_REDIRECT, false)); + $this->add($this->createInput(self::INVALID_SHORT_URL_REDIRECT, false)); + } +} diff --git a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php index f52ba54a..c58edfe6 100644 --- a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php +++ b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php @@ -45,7 +45,7 @@ class DomainRepositoryTest extends DatabaseTestCase $this->getEntityManager()->persist($detachedDomain); $detachedWithRedirects = Domain::withAuthority('detached-with-redirects.com'); - $detachedWithRedirects->configureNotFoundRedirects(new NotFoundRedirects('foo.com', 'bar.com')); + $detachedWithRedirects->configureNotFoundRedirects(NotFoundRedirects::withRedirects('foo.com', 'bar.com')); $this->getEntityManager()->persist($detachedWithRedirects); $this->getEntityManager()->flush(); @@ -101,7 +101,7 @@ class DomainRepositoryTest extends DatabaseTestCase $this->getEntityManager()->persist($detachedDomain); $detachedWithRedirects = Domain::withAuthority('detached-with-redirects.com'); - $detachedWithRedirects->configureNotFoundRedirects(new NotFoundRedirects('foo.com', 'bar.com')); + $detachedWithRedirects->configureNotFoundRedirects(NotFoundRedirects::withRedirects('foo.com', 'bar.com')); $this->getEntityManager()->persist($detachedWithRedirects); $this->getEntityManager()->flush(); diff --git a/module/Core/test/Domain/DomainServiceTest.php b/module/Core/test/Domain/DomainServiceTest.php index 060b24ab..812210dd 100644 --- a/module/Core/test/Domain/DomainServiceTest.php +++ b/module/Core/test/Domain/DomainServiceTest.php @@ -183,7 +183,7 @@ class DomainServiceTest extends TestCase $persist = $this->em->persist($foundDomain ?? Argument::type(Domain::class)); $flush = $this->em->flush(); - $result = $this->domainService->configureNotFoundRedirects($authority, new NotFoundRedirects( + $result = $this->domainService->configureNotFoundRedirects($authority, NotFoundRedirects::withRedirects( 'foo.com', 'bar.com', 'baz.com', diff --git a/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php b/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php index 8b9f69e5..e2b27e23 100644 --- a/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php +++ b/module/Rest/src/Action/Domain/Request/DomainRedirectsRequest.php @@ -6,6 +6,8 @@ namespace Shlinkio\Shlink\Rest\Action\Domain\Request; use Shlinkio\Shlink\Core\Config\NotFoundRedirectConfigInterface; use Shlinkio\Shlink\Core\Config\NotFoundRedirects; +use Shlinkio\Shlink\Core\Domain\Validation\DomainRedirectsInputFilter; +use Shlinkio\Shlink\Core\Exception\ValidationException; use function array_key_exists; @@ -30,17 +32,33 @@ class DomainRedirectsRequest return $instance; } + /** + * @throws ValidationException + */ private function validateAndInit(array $payload): void { - // TODO Validate data - $this->baseUrlRedirectWasProvided = array_key_exists('baseUrlRedirect', $payload); - $this->regular404RedirectWasProvided = array_key_exists('regular404Redirect', $payload); - $this->invalidShortUrlRedirectWasProvided = array_key_exists('invalidShortUrlRedirect', $payload); + $inputFilter = DomainRedirectsInputFilter::withData($payload); + if (! $inputFilter->isValid()) { + throw ValidationException::fromInputFilter($inputFilter); + } - $this->authority = $payload['domain']; - $this->baseUrlRedirect = $payload['baseUrlRedirect'] ?? null; - $this->regular404Redirect = $payload['regular404Redirect'] ?? null; - $this->invalidShortUrlRedirect = $payload['invalidShortUrlRedirect'] ?? null; + $this->baseUrlRedirectWasProvided = array_key_exists( + DomainRedirectsInputFilter::BASE_URL_REDIRECT, + $payload, + ); + $this->regular404RedirectWasProvided = array_key_exists( + DomainRedirectsInputFilter::REGULAR_404_REDIRECT, + $payload, + ); + $this->invalidShortUrlRedirectWasProvided = array_key_exists( + DomainRedirectsInputFilter::INVALID_SHORT_URL_REDIRECT, + $payload, + ); + + $this->authority = $inputFilter->getValue(DomainRedirectsInputFilter::DOMAIN); + $this->baseUrlRedirect = $inputFilter->getValue(DomainRedirectsInputFilter::BASE_URL_REDIRECT); + $this->regular404Redirect = $inputFilter->getValue(DomainRedirectsInputFilter::REGULAR_404_REDIRECT); + $this->invalidShortUrlRedirect = $inputFilter->getValue(DomainRedirectsInputFilter::INVALID_SHORT_URL_REDIRECT); } public function authority(): string @@ -50,7 +68,7 @@ class DomainRedirectsRequest public function toNotFoundRedirects(?NotFoundRedirectConfigInterface $defaults = null): NotFoundRedirects { - return new NotFoundRedirects( + return NotFoundRedirects::withRedirects( $this->baseUrlRedirectWasProvided ? $this->baseUrlRedirect : $defaults?->baseUrlRedirect(), $this->regular404RedirectWasProvided ? $this->regular404Redirect : $defaults?->regular404Redirect(), $this->invalidShortUrlRedirectWasProvided diff --git a/module/Rest/test-api/Fixtures/DomainFixture.php b/module/Rest/test-api/Fixtures/DomainFixture.php index 31e68f21..619dfdc4 100644 --- a/module/Rest/test-api/Fixtures/DomainFixture.php +++ b/module/Rest/test-api/Fixtures/DomainFixture.php @@ -20,7 +20,7 @@ class DomainFixture extends AbstractFixture $manager->persist(Domain::withAuthority('this_domain_is_detached.com')); $detachedWithRedirects = Domain::withAuthority('detached-with-redirects.com'); - $detachedWithRedirects->configureNotFoundRedirects(new NotFoundRedirects('foo.com', 'bar.com')); + $detachedWithRedirects->configureNotFoundRedirects(NotFoundRedirects::withRedirects('foo.com', 'bar.com')); $manager->persist($detachedWithRedirects); $manager->flush(); From 7b43403b1cef4ac9845accc037413e80571eb1f9 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 3 Aug 2021 16:48:17 +0200 Subject: [PATCH 11/17] Fixed error when editing domain redirects for a new domain --- .../Domain/Repository/DomainRepository.php | 28 ++++++++----------- .../Repository/DomainRepositoryTest.php | 2 +- .../Action/Domain/DomainRedirectsAction.php | 2 +- 3 files changed, 13 insertions(+), 19 deletions(-) diff --git a/module/Core/src/Domain/Repository/DomainRepository.php b/module/Core/src/Domain/Repository/DomainRepository.php index 50033604..33538011 100644 --- a/module/Core/src/Domain/Repository/DomainRepository.php +++ b/module/Core/src/Domain/Repository/DomainRepository.php @@ -23,8 +23,14 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe */ public function findDomainsWithout(?string $excludedAuthority, ?ApiKey $apiKey = null): array { - $qb = $this->createPublicDomainsQueryBuilder(); - $qb->orderBy('d.authority', 'ASC'); + $qb = $this->createQueryBuilder('d'); + $qb->leftJoin(ShortUrl::class, 's', Join::WITH, 's.domain = d') + ->groupBy('d') + ->orderBy('d.authority', 'ASC') + ->having($qb->expr()->gt('COUNT(s.id)', '0')) + ->orHaving($qb->expr()->isNotNull('d.baseUrlRedirect')) + ->orHaving($qb->expr()->isNotNull('d.regular404Redirect')) + ->orHaving($qb->expr()->isNotNull('d.invalidShortUrlRedirect')); $specs = $this->determineExtraSpecs($excludedAuthority, $apiKey); foreach ($specs as [$alias, $spec]) { @@ -36,8 +42,9 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe public function findOneByAuthority(string $authority, ?ApiKey $apiKey = null): ?Domain { - $qb = $this->createPublicDomainsQueryBuilder(); - $qb->where($qb->expr()->eq('d.authority', ':authority')) + $qb = $this->createQueryBuilder('d'); + $qb->leftJoin(ShortUrl::class, 's', Join::WITH, 's.domain = d') + ->where($qb->expr()->eq('d.authority', ':authority')) ->setParameter('authority', $authority) ->setMaxResults(1); @@ -49,19 +56,6 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe return $qb->getQuery()->getOneOrNullResult(); } - private function createPublicDomainsQueryBuilder(): QueryBuilder - { - $qb = $this->createQueryBuilder('d'); - $qb->leftJoin(ShortUrl::class, 's', Join::WITH, 's.domain = d') - ->groupBy('d') - ->having($qb->expr()->gt('COUNT(s.id)', '0')) - ->orHaving($qb->expr()->isNotNull('d.baseUrlRedirect')) - ->orHaving($qb->expr()->isNotNull('d.regular404Redirect')) - ->orHaving($qb->expr()->isNotNull('d.invalidShortUrlRedirect')); - - return $qb; - } - private function determineExtraSpecs(?string $excludedAuthority, ?ApiKey $apiKey): iterable { if ($excludedAuthority !== null) { diff --git a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php index c58edfe6..1eaf6ea9 100644 --- a/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php +++ b/module/Core/test-db/Domain/Repository/DomainRepositoryTest.php @@ -74,7 +74,7 @@ class DomainRepositoryTest extends DatabaseTestCase self::assertEquals($barDomain, $this->repo->findOneByAuthority('bar.com')); self::assertEquals($detachedWithRedirects, $this->repo->findOneByAuthority('detached-with-redirects.com')); self::assertNull($this->repo->findOneByAuthority('does-not-exist.com')); - self::assertNull($this->repo->findOneByAuthority('detached.com')); + self::assertEquals($detachedDomain, $this->repo->findOneByAuthority('detached.com')); } /** @test */ diff --git a/module/Rest/src/Action/Domain/DomainRedirectsAction.php b/module/Rest/src/Action/Domain/DomainRedirectsAction.php index d9259582..ca4346f8 100644 --- a/module/Rest/src/Action/Domain/DomainRedirectsAction.php +++ b/module/Rest/src/Action/Domain/DomainRedirectsAction.php @@ -23,7 +23,7 @@ class DomainRedirectsAction extends AbstractRestAction public function handle(ServerRequestInterface $request): ResponseInterface { - // TODO Do not allow to set redirects for default domain + // TODO Do not allow to set redirects for default domain. Or do allow. Check if there could be any issue /** @var array $body */ $body = $request->getParsedBody(); From 565fe4c348bb50cf89a253dd56fa5e95fefcbce0 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 3 Aug 2021 17:00:26 +0200 Subject: [PATCH 12/17] Added redirects to the list of domains --- docs/swagger/paths/v2_domains.json | 28 +++++++++++++---- module/Core/src/Config/NotFoundRedirects.php | 5 ++++ module/Core/src/Domain/Model/DomainItem.php | 2 ++ .../Domain/Repository/DomainRepository.php | 1 - .../src/Action/Domain/ListDomainsAction.php | 2 -- .../Rest/test-api/Action/ListDomainsTest.php | 30 +++++++++++++++++++ 6 files changed, 60 insertions(+), 8 deletions(-) diff --git a/docs/swagger/paths/v2_domains.json b/docs/swagger/paths/v2_domains.json index d92ae995..ef63ee4e 100644 --- a/docs/swagger/paths/v2_domains.json +++ b/docs/swagger/paths/v2_domains.json @@ -18,7 +18,7 @@ ], "responses": { "200": { - "description": "The list of tags", + "description": "The list of domains", "content": { "application/json": { "schema": { @@ -33,13 +33,16 @@ "type": "array", "items": { "type": "object", - "required": ["domain", "isDefault"], + "required": ["domain", "isDefault", "redirects"], "properties": { "domain": { "type": "string" }, "isDefault": { "type": "boolean" + }, + "redirects": { + "$ref": "../definitions/NotFoundRedirects.json" } } } @@ -56,15 +59,30 @@ "data": [ { "domain": "example.com", - "isDefault": true + "isDefault": true, + "redirects": { + "baseUrlRedirect": "https://example.com/my-landing-page", + "regular404Redirect": null, + "invalidShortUrlRedirect": "https://example.com/invalid-url" + } }, { "domain": "aaa.com", - "isDefault": false + "isDefault": false, + "redirects": { + "baseUrlRedirect": null, + "regular404Redirect": null, + "invalidShortUrlRedirect": null + } }, { "domain": "bbb.com", - "isDefault": false + "isDefault": false, + "redirects": { + "baseUrlRedirect": null, + "regular404Redirect": null, + "invalidShortUrlRedirect": "https://example.com/invalid-url" + } } ] } diff --git a/module/Core/src/Config/NotFoundRedirects.php b/module/Core/src/Config/NotFoundRedirects.php index c00d35d0..9f277be5 100644 --- a/module/Core/src/Config/NotFoundRedirects.php +++ b/module/Core/src/Config/NotFoundRedirects.php @@ -28,6 +28,11 @@ final class NotFoundRedirects implements JsonSerializable return new self(null, null, null); } + public static function fromConfig(NotFoundRedirectConfigInterface $config): self + { + return new self($config->baseUrlRedirect(), $config->regular404Redirect(), $config->invalidShortUrlRedirect()); + } + public function baseUrlRedirect(): ?string { return $this->baseUrlRedirect; diff --git a/module/Core/src/Domain/Model/DomainItem.php b/module/Core/src/Domain/Model/DomainItem.php index bad7d5cf..cfd09d90 100644 --- a/module/Core/src/Domain/Model/DomainItem.php +++ b/module/Core/src/Domain/Model/DomainItem.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Core\Domain\Model; use JsonSerializable; use Shlinkio\Shlink\Core\Config\NotFoundRedirectConfigInterface; +use Shlinkio\Shlink\Core\Config\NotFoundRedirects; use Shlinkio\Shlink\Core\Entity\Domain; final class DomainItem implements JsonSerializable @@ -32,6 +33,7 @@ final class DomainItem implements JsonSerializable return [ 'domain' => $this->authority, 'isDefault' => $this->isDefault, + 'redirects' => NotFoundRedirects::fromConfig($this->notFoundRedirectConfig), ]; } diff --git a/module/Core/src/Domain/Repository/DomainRepository.php b/module/Core/src/Domain/Repository/DomainRepository.php index 33538011..1741cea7 100644 --- a/module/Core/src/Domain/Repository/DomainRepository.php +++ b/module/Core/src/Domain/Repository/DomainRepository.php @@ -5,7 +5,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Domain\Repository; use Doctrine\ORM\Query\Expr\Join; -use Doctrine\ORM\QueryBuilder; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; use Happyr\DoctrineSpecification\Spec; use Shlinkio\Shlink\Core\Domain\Spec\IsDomain; diff --git a/module/Rest/src/Action/Domain/ListDomainsAction.php b/module/Rest/src/Action/Domain/ListDomainsAction.php index 3d9205f8..c8f9a475 100644 --- a/module/Rest/src/Action/Domain/ListDomainsAction.php +++ b/module/Rest/src/Action/Domain/ListDomainsAction.php @@ -25,8 +25,6 @@ class ListDomainsAction extends AbstractRestAction $apiKey = AuthenticationMiddleware::apiKeyFromRequest($request); $domainItems = $this->domainService->listDomains($apiKey); - // TODO Support including not found redirects if requested via query param - return new JsonResponse([ 'domains' => [ 'data' => $domainItems, diff --git a/module/Rest/test-api/Action/ListDomainsTest.php b/module/Rest/test-api/Action/ListDomainsTest.php index 075b6d09..5f33c20b 100644 --- a/module/Rest/test-api/Action/ListDomainsTest.php +++ b/module/Rest/test-api/Action/ListDomainsTest.php @@ -31,30 +31,60 @@ class ListDomainsTest extends ApiTestCase [ 'domain' => 'doma.in', 'isDefault' => true, + 'redirects' => [ + 'baseUrlRedirect' => null, + 'regular404Redirect' => null, + 'invalidShortUrlRedirect' => null, + ], ], [ 'domain' => 'detached-with-redirects.com', 'isDefault' => false, + 'redirects' => [ + 'baseUrlRedirect' => 'foo.com', + 'regular404Redirect' => 'bar.com', + 'invalidShortUrlRedirect' => null, + ], ], [ 'domain' => 'example.com', 'isDefault' => false, + 'redirects' => [ + 'baseUrlRedirect' => null, + 'regular404Redirect' => null, + 'invalidShortUrlRedirect' => null, + ], ], [ 'domain' => 'some-domain.com', 'isDefault' => false, + 'redirects' => [ + 'baseUrlRedirect' => null, + 'regular404Redirect' => null, + 'invalidShortUrlRedirect' => null, + ], ], ]]; yield 'author API key' => ['author_api_key', [ [ 'domain' => 'doma.in', 'isDefault' => true, + 'redirects' => [ + 'baseUrlRedirect' => null, + 'regular404Redirect' => null, + 'invalidShortUrlRedirect' => null, + ], ], ]]; yield 'domain API key' => ['domain_api_key', [ [ 'domain' => 'example.com', 'isDefault' => false, + 'redirects' => [ + 'baseUrlRedirect' => null, + 'regular404Redirect' => null, + 'invalidShortUrlRedirect' => null, + ], ], ]]; } From 9abf611d6331fc9958631a4c3e11c44849dd3594 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 3 Aug 2021 18:09:39 +0200 Subject: [PATCH 13/17] Created DomainResirectsAction unit test --- module/Core/src/Config/NotFoundRedirects.php | 2 +- .../Validation/DomainRedirectsInputFilter.php | 9 +- .../src/Validation/ShortUrlInputFilter.php | 2 +- .../Domain/DomainRedirectsActionTest.php | 160 ++++++++++++++++++ 4 files changed, 163 insertions(+), 10 deletions(-) create mode 100644 module/Rest/test/Action/Domain/DomainRedirectsActionTest.php diff --git a/module/Core/src/Config/NotFoundRedirects.php b/module/Core/src/Config/NotFoundRedirects.php index 9f277be5..492a00bc 100644 --- a/module/Core/src/Config/NotFoundRedirects.php +++ b/module/Core/src/Config/NotFoundRedirects.php @@ -16,7 +16,7 @@ final class NotFoundRedirects implements JsonSerializable } public static function withRedirects( - ?string $baseUrlRedirect = null, + ?string $baseUrlRedirect, ?string $regular404Redirect = null, ?string $invalidShortUrlRedirect = null, ): self { diff --git a/module/Core/src/Domain/Validation/DomainRedirectsInputFilter.php b/module/Core/src/Domain/Validation/DomainRedirectsInputFilter.php index 94c15217..de627c1c 100644 --- a/module/Core/src/Domain/Validation/DomainRedirectsInputFilter.php +++ b/module/Core/src/Domain/Validation/DomainRedirectsInputFilter.php @@ -5,7 +5,6 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Core\Domain\Validation; use Laminas\InputFilter\InputFilter; -use Laminas\Validator; use Shlinkio\Shlink\Common\Validation; class DomainRedirectsInputFilter extends InputFilter @@ -34,13 +33,7 @@ class DomainRedirectsInputFilter extends InputFilter private function initializeInputs(): void { $domain = $this->createInput(self::DOMAIN); - $domain->getValidatorChain()->attach(new Validator\NotEmpty([ - Validator\NotEmpty::OBJECT, - Validator\NotEmpty::SPACE, - Validator\NotEmpty::NULL, - Validator\NotEmpty::EMPTY_ARRAY, - Validator\NotEmpty::BOOLEAN, - ])); + $domain->getValidatorChain()->attach(new Validation\HostAndPortValidator()); $this->add($domain); $this->add($this->createInput(self::BASE_URL_REDIRECT, false)); diff --git a/module/Core/src/Validation/ShortUrlInputFilter.php b/module/Core/src/Validation/ShortUrlInputFilter.php index c7cdaa43..b969d95e 100644 --- a/module/Core/src/Validation/ShortUrlInputFilter.php +++ b/module/Core/src/Validation/ShortUrlInputFilter.php @@ -75,7 +75,7 @@ class ShortUrlInputFilter extends InputFilter $customSlug = $this->createInput(self::CUSTOM_SLUG, false)->setContinueIfEmpty(true); $customSlug->getFilterChain()->attach(new Validation\SluggerFilter(new CocurSymfonySluggerBridge(new Slugify([ 'regexp' => CUSTOM_SLUGS_REGEXP, - 'lowercase' => false, // We want to keep it case sensitive + 'lowercase' => false, // We want to keep it case-sensitive 'rulesets' => ['default'], ])))); $customSlug->getValidatorChain()->attach(new Validator\NotEmpty([ diff --git a/module/Rest/test/Action/Domain/DomainRedirectsActionTest.php b/module/Rest/test/Action/Domain/DomainRedirectsActionTest.php new file mode 100644 index 00000000..5d09f3f7 --- /dev/null +++ b/module/Rest/test/Action/Domain/DomainRedirectsActionTest.php @@ -0,0 +1,160 @@ +domainService = $this->prophesize(DomainServiceInterface::class); + $this->action = new DomainRedirectsAction($this->domainService->reveal()); + } + + /** + * @test + * @dataProvider provideInvalidBodies + */ + public function invalidDataThrowsException(array $body): void + { + $request = ServerRequestFactory::fromGlobals()->withParsedBody($body); + + $this->expectException(ValidationException::class); + $this->domainService->getOrCreate(Argument::cetera())->shouldNotBeCalled(); + $this->domainService->configureNotFoundRedirects(Argument::cetera())->shouldNotBeCalled(); + + $this->action->handle($request); + } + + public function provideInvalidBodies(): iterable + { + yield 'no domain' => [[]]; + yield 'empty domain' => [['domain' => '']]; + yield 'invalid domain' => [['domain' => '192.168.1.20']]; + } + + /** + * @test + * @dataProvider provideDomainsAndRedirects + */ + public function domainIsFetchedAndUsedToGetItConfigured( + Domain $domain, + array $redirects, + array $expectedResult, + ): void { + $authority = 'doma.in'; + $redirects['domain'] = $authority; + $apiKey = ApiKey::create(); + $request = ServerRequestFactory::fromGlobals()->withParsedBody($redirects) + ->withAttribute(ApiKey::class, $apiKey); + + $getOrCreate = $this->domainService->getOrCreate($authority)->willReturn($domain); + $configureNotFoundRedirects = $this->domainService->configureNotFoundRedirects( + $authority, + NotFoundRedirects::withRedirects( + array_key_exists(DomainRedirectsInputFilter::BASE_URL_REDIRECT, $redirects) + ? $redirects[DomainRedirectsInputFilter::BASE_URL_REDIRECT] + : $domain?->baseUrlRedirect(), + array_key_exists(DomainRedirectsInputFilter::REGULAR_404_REDIRECT, $redirects) + ? $redirects[DomainRedirectsInputFilter::REGULAR_404_REDIRECT] + : $domain?->regular404Redirect(), + array_key_exists(DomainRedirectsInputFilter::INVALID_SHORT_URL_REDIRECT, $redirects) + ? $redirects[DomainRedirectsInputFilter::INVALID_SHORT_URL_REDIRECT] + : $domain?->invalidShortUrlRedirect(), + ), + $apiKey, + ); + + /** @var JsonResponse $response */ + $response = $this->action->handle($request); + /** @var NotFoundRedirects $payload */ + $payload = $response->getPayload(); + + self::assertEquals($expectedResult, $payload->jsonSerialize()); + $getOrCreate->shouldHaveBeenCalledOnce(); + $configureNotFoundRedirects->shouldHaveBeenCalledOnce(); + } + + public function provideDomainsAndRedirects(): iterable + { + yield 'full overwrite' => [Domain::withAuthority(''), [ + DomainRedirectsInputFilter::BASE_URL_REDIRECT => 'foo', + DomainRedirectsInputFilter::REGULAR_404_REDIRECT => 'bar', + DomainRedirectsInputFilter::INVALID_SHORT_URL_REDIRECT => 'baz', + ], [ + DomainRedirectsInputFilter::BASE_URL_REDIRECT => 'foo', + DomainRedirectsInputFilter::REGULAR_404_REDIRECT => 'bar', + DomainRedirectsInputFilter::INVALID_SHORT_URL_REDIRECT => 'baz', + ]]; + yield 'partial overwrite' => [Domain::withAuthority(''), [ + DomainRedirectsInputFilter::BASE_URL_REDIRECT => 'foo', + DomainRedirectsInputFilter::INVALID_SHORT_URL_REDIRECT => 'baz', + ], [ + DomainRedirectsInputFilter::BASE_URL_REDIRECT => 'foo', + DomainRedirectsInputFilter::REGULAR_404_REDIRECT => null, + DomainRedirectsInputFilter::INVALID_SHORT_URL_REDIRECT => 'baz', + ]]; + yield 'no override' => [ + (static function (): Domain { + $domain = Domain::withAuthority(''); + $domain->configureNotFoundRedirects(NotFoundRedirects::withRedirects( + 'baz', + 'bar', + 'foo', + )); + + return $domain; + })(), + [], + [ + DomainRedirectsInputFilter::BASE_URL_REDIRECT => 'baz', + DomainRedirectsInputFilter::REGULAR_404_REDIRECT => 'bar', + DomainRedirectsInputFilter::INVALID_SHORT_URL_REDIRECT => 'foo', + ], + ]; + yield 'reset' => [ + (static function (): Domain { + $domain = Domain::withAuthority(''); + $domain->configureNotFoundRedirects(NotFoundRedirects::withRedirects( + 'foo', + 'bar', + 'baz', + )); + + return $domain; + })(), + [ + DomainRedirectsInputFilter::BASE_URL_REDIRECT => null, + DomainRedirectsInputFilter::REGULAR_404_REDIRECT => null, + DomainRedirectsInputFilter::INVALID_SHORT_URL_REDIRECT => null, + ], + [ + DomainRedirectsInputFilter::BASE_URL_REDIRECT => null, + DomainRedirectsInputFilter::REGULAR_404_REDIRECT => null, + DomainRedirectsInputFilter::INVALID_SHORT_URL_REDIRECT => null, + ], + ]; + } +} From 7c06633a678f76e3a2ec17aeb8865f8350a7b2f3 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 3 Aug 2021 18:28:04 +0200 Subject: [PATCH 14/17] Ensured default domain redirects cannot be edited through regular approach --- module/Core/src/Domain/DomainService.php | 6 ++++ .../src/Domain/DomainServiceInterface.php | 2 ++ .../src/Exception/InvalidDomainException.php | 33 +++++++++++++++++++ module/Core/test/Domain/DomainServiceTest.php | 12 +++++++ .../Exception/InvalidDomainExceptionTest.php | 24 ++++++++++++++ .../Action/Domain/DomainRedirectsAction.php | 2 -- 6 files changed, 77 insertions(+), 2 deletions(-) create mode 100644 module/Core/src/Exception/InvalidDomainException.php create mode 100644 module/Core/test/Exception/InvalidDomainExceptionTest.php diff --git a/module/Core/src/Domain/DomainService.php b/module/Core/src/Domain/DomainService.php index 807c6dce..6051c254 100644 --- a/module/Core/src/Domain/DomainService.php +++ b/module/Core/src/Domain/DomainService.php @@ -10,6 +10,7 @@ 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\Core\Exception\InvalidDomainException; use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; use Shlinkio\Shlink\Rest\ApiKey\Role; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -78,12 +79,17 @@ class DomainService implements DomainServiceInterface /** * @throws DomainNotFoundException + * @throws InvalidDomainException */ public function configureNotFoundRedirects( string $authority, NotFoundRedirects $notFoundRedirects, ?ApiKey $apiKey = null ): Domain { + if ($authority === $this->defaultDomain) { + throw InvalidDomainException::forDefaultDomainRedirects(); + } + $domain = $this->getPersistedDomain($authority, $apiKey); $domain->configureNotFoundRedirects($notFoundRedirects); diff --git a/module/Core/src/Domain/DomainServiceInterface.php b/module/Core/src/Domain/DomainServiceInterface.php index 9ac48e69..7748284d 100644 --- a/module/Core/src/Domain/DomainServiceInterface.php +++ b/module/Core/src/Domain/DomainServiceInterface.php @@ -8,6 +8,7 @@ use Shlinkio\Shlink\Core\Config\NotFoundRedirects; use Shlinkio\Shlink\Core\Domain\Model\DomainItem; use Shlinkio\Shlink\Core\Entity\Domain; use Shlinkio\Shlink\Core\Exception\DomainNotFoundException; +use Shlinkio\Shlink\Core\Exception\InvalidDomainException; use Shlinkio\Shlink\Rest\Entity\ApiKey; interface DomainServiceInterface @@ -31,6 +32,7 @@ interface DomainServiceInterface /** * @throws DomainNotFoundException If the API key is restricted to one domain and a different one is provided + * @throws InvalidDomainException If default domain is provided */ public function configureNotFoundRedirects( string $authority, diff --git a/module/Core/src/Exception/InvalidDomainException.php b/module/Core/src/Exception/InvalidDomainException.php new file mode 100644 index 00000000..6e71c831 --- /dev/null +++ b/module/Core/src/Exception/InvalidDomainException.php @@ -0,0 +1,33 @@ +detail = $e->getMessage(); + $e->title = self::TITLE; + $e->type = self::TYPE; + $e->status = StatusCodeInterface::STATUS_BAD_REQUEST; + + return $e; + } +} diff --git a/module/Core/test/Domain/DomainServiceTest.php b/module/Core/test/Domain/DomainServiceTest.php index 812210dd..159fb6ca 100644 --- a/module/Core/test/Domain/DomainServiceTest.php +++ b/module/Core/test/Domain/DomainServiceTest.php @@ -15,6 +15,7 @@ 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\Core\Exception\InvalidDomainException; use Shlinkio\Shlink\Core\Options\NotFoundRedirectOptions; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; @@ -213,4 +214,15 @@ class DomainServiceTest extends TestCase yield 'domain not found and author API key' => [null, $authorApiKey]; yield 'domain found and author API key' => [$domain, $authorApiKey]; } + + /** @test */ + public function anExceptionIsThrowsWhenTryingToEditRedirectsForDefaultDomain(): void + { + $this->expectException(InvalidDomainException::class); + $this->expectExceptionMessage( + 'You cannot configure default domain\'s redirects this way. Use the configuration or env vars.', + ); + + $this->domainService->configureNotFoundRedirects('default.com', NotFoundRedirects::withoutRedirects()); + } } diff --git a/module/Core/test/Exception/InvalidDomainExceptionTest.php b/module/Core/test/Exception/InvalidDomainExceptionTest.php new file mode 100644 index 00000000..e4592cd8 --- /dev/null +++ b/module/Core/test/Exception/InvalidDomainExceptionTest.php @@ -0,0 +1,24 @@ +getMessage()); + self::assertEquals($expected, $e->getDetail()); + self::assertEquals('Invalid domain', $e->getTitle()); + self::assertEquals('INVALID_DOMAIN', $e->getType()); + self::assertEquals(400, $e->getStatus()); + } +} diff --git a/module/Rest/src/Action/Domain/DomainRedirectsAction.php b/module/Rest/src/Action/Domain/DomainRedirectsAction.php index ca4346f8..e98aa339 100644 --- a/module/Rest/src/Action/Domain/DomainRedirectsAction.php +++ b/module/Rest/src/Action/Domain/DomainRedirectsAction.php @@ -23,8 +23,6 @@ class DomainRedirectsAction extends AbstractRestAction public function handle(ServerRequestInterface $request): ResponseInterface { - // TODO Do not allow to set redirects for default domain. Or do allow. Check if there could be any issue - /** @var array $body */ $body = $request->getParsedBody(); $requestData = DomainRedirectsRequest::fromRawData($body); From 40a7d5a112143e7e305c092997d7eac0258b3933 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 3 Aug 2021 18:33:50 +0200 Subject: [PATCH 15/17] Documented error when trying to edit default domain redirects through endpoint --- docs/swagger/paths/v2_domains_redirects.json | 10 ++++++++++ module/Core/src/Exception/InvalidDomainException.php | 2 +- .../Core/test/Exception/InvalidDomainExceptionTest.php | 2 +- 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/docs/swagger/paths/v2_domains_redirects.json b/docs/swagger/paths/v2_domains_redirects.json index bba6fbb7..9bf16841 100644 --- a/docs/swagger/paths/v2_domains_redirects.json +++ b/docs/swagger/paths/v2_domains_redirects.json @@ -99,6 +99,16 @@ } } }, + "403": { + "description": "Default domain was provided, and it cannot be edited this way.", + "content": { + "application/problem+json": { + "schema": { + "$ref": "../definitions/Error.json" + } + } + } + }, "500": { "description": "Unexpected error.", "content": { diff --git a/module/Core/src/Exception/InvalidDomainException.php b/module/Core/src/Exception/InvalidDomainException.php index 6e71c831..d41e71ac 100644 --- a/module/Core/src/Exception/InvalidDomainException.php +++ b/module/Core/src/Exception/InvalidDomainException.php @@ -26,7 +26,7 @@ class InvalidDomainException extends DomainException implements ProblemDetailsEx $e->detail = $e->getMessage(); $e->title = self::TITLE; $e->type = self::TYPE; - $e->status = StatusCodeInterface::STATUS_BAD_REQUEST; + $e->status = StatusCodeInterface::STATUS_FORBIDDEN; return $e; } diff --git a/module/Core/test/Exception/InvalidDomainExceptionTest.php b/module/Core/test/Exception/InvalidDomainExceptionTest.php index e4592cd8..06b78ff2 100644 --- a/module/Core/test/Exception/InvalidDomainExceptionTest.php +++ b/module/Core/test/Exception/InvalidDomainExceptionTest.php @@ -19,6 +19,6 @@ class InvalidDomainExceptionTest extends TestCase self::assertEquals($expected, $e->getDetail()); self::assertEquals('Invalid domain', $e->getTitle()); self::assertEquals('INVALID_DOMAIN', $e->getType()); - self::assertEquals(400, $e->getStatus()); + self::assertEquals(403, $e->getStatus()); } } From de81e81ecb7c99c87f01f21833271ab2fb889eef Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 3 Aug 2021 19:43:30 +0200 Subject: [PATCH 16/17] Created API test for Domain redirects --- .../test-api/Action/DomainRedirectsTest.php | 100 ++++++++++++++++++ 1 file changed, 100 insertions(+) create mode 100644 module/Rest/test-api/Action/DomainRedirectsTest.php diff --git a/module/Rest/test-api/Action/DomainRedirectsTest.php b/module/Rest/test-api/Action/DomainRedirectsTest.php new file mode 100644 index 00000000..987c09d6 --- /dev/null +++ b/module/Rest/test-api/Action/DomainRedirectsTest.php @@ -0,0 +1,100 @@ +callApiWithKey(self::METHOD_PATCH, '/domains/redirects', [ + RequestOptions::JSON => ['domain' => 'doma.in'], + ]); + $payload = $this->getJsonResponsePayload($resp); + + self::assertEquals(self::STATUS_FORBIDDEN, $resp->getStatusCode()); + self::assertEquals(self::STATUS_FORBIDDEN, $payload['status']); + self::assertEquals('INVALID_DOMAIN', $payload['type']); + self::assertEquals( + 'You cannot configure default domain\'s redirects this way. Use the configuration or env vars.', + $payload['detail'], + ); + self::assertEquals('Invalid domain', $payload['title']); + } + + /** + * @test + * @dataProvider provideInvalidDomains + */ + public function anErrorIsReturnedWhenTryingToEditAnInvalidDomain(array $request): void + { + $resp = $this->callApiWithKey(self::METHOD_PATCH, '/domains/redirects', [ + RequestOptions::JSON => $request, + ]); + $payload = $this->getJsonResponsePayload($resp); + + self::assertEquals(self::STATUS_BAD_REQUEST, $resp->getStatusCode()); + self::assertEquals(self::STATUS_BAD_REQUEST, $payload['status']); + self::assertEquals('INVALID_ARGUMENT', $payload['type']); + self::assertEquals('Provided data is not valid', $payload['detail']); + self::assertEquals('Invalid data', $payload['title']); + } + + public function provideInvalidDomains(): iterable + { + yield 'no domain' => [[]]; + yield 'empty domain' => [['domain' => '']]; + yield 'null domain' => [['domain' => null]]; + yield 'invalid domain' => [['domain' => '192.168.1.1']]; + } + + /** + * @test + * @dataProvider provideRequests + */ + public function allowsToEditDomainRedirects(array $request, array $expectedResponse): void + { + $resp = $this->callApiWithKey(self::METHOD_PATCH, '/domains/redirects', [ + RequestOptions::JSON => $request, + ]); + $payload = $this->getJsonResponsePayload($resp); + + self::assertEquals(self::STATUS_OK, $resp->getStatusCode()); + self::assertEquals($expectedResponse, $payload); + } + + public function provideRequests(): iterable + { + yield 'new domain' => [[ + 'domain' => 'my-new-domain.com', + 'regular404Redirect' => 'foo.com', + ], [ + 'baseUrlRedirect' => null, + 'regular404Redirect' => 'foo.com', + 'invalidShortUrlRedirect' => null, + ]]; + yield 'existing domain with redirects' => [[ + 'domain' => 'detached-with-redirects.com', + 'baseUrlRedirect' => null, + 'invalidShortUrlRedirect' => 'foo.com', + ], [ + 'baseUrlRedirect' => null, + 'regular404Redirect' => 'bar.com', + 'invalidShortUrlRedirect' => 'foo.com', + ]]; + yield 'existing domain with no redirects' => [[ + 'domain' => 'example.com', + 'baseUrlRedirect' => null, + 'invalidShortUrlRedirect' => 'foo.com', + ], [ + 'baseUrlRedirect' => null, + 'regular404Redirect' => null, + 'invalidShortUrlRedirect' => 'foo.com', + ]]; + } +} From 0c97c8f04f3a624ecb22e0a452057cac66ecf529 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 3 Aug 2021 19:47:44 +0200 Subject: [PATCH 17/17] Updated changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 97d960a9..038c68ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,7 +19,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#943](https://github.com/shlinkio/shlink/issues/943) Added support to define different "not-found" redirects for every domain handled by Shlink. - Shlink will continue to allow defining the default values via env vars or config, but afterwards, you can use the `domain:redirects` command to define specific values for every single domain. + Shlink will continue to allow defining the default values via env vars or config, but afterwards, you can use the `domain:redirects` command or the `PATCH /domains/redirects` REST endpoint to define specific values for every single domain. ### Changed * [#1118](https://github.com/shlinkio/shlink/issues/1118) Increased phpstan required level to 8.