From 4ef5ab7a90eb1462b2d9059c21471777707b6653 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 27 Jul 2021 20:03:39 +0200 Subject: [PATCH] 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();