Fixed wrong domains getting resolved for an API key roles

This commit is contained in:
Alejandro Celaya 2021-07-27 20:03:39 +02:00 committed by Alejandro Celaya
parent 192308a6a3
commit 4ef5ab7a90
7 changed files with 91 additions and 25 deletions

View File

@ -6,8 +6,13 @@ namespace Shlinkio\Shlink\Core\Domain\Repository;
use Doctrine\ORM\Query\Expr\Join; use Doctrine\ORM\Query\Expr\Join;
use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; 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\Domain;
use Shlinkio\Shlink\Core\Entity\ShortUrl; 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; use Shlinkio\Shlink\Rest\Entity\ApiKey;
class DomainRepository extends EntitySpecificationRepository implements DomainRepositoryInterface 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.regular404Redirect'))
->orHaving($qb->expr()->isNotNull('d.invalidShortUrlRedirect')); ->orHaving($qb->expr()->isNotNull('d.invalidShortUrlRedirect'));
if ($excludedAuthority !== null) { $specs = $this->determineExtraSpecs($excludedAuthority, $apiKey);
$qb->where($qb->expr()->neq('d.authority', ':excludedAuthority')) foreach ($specs as [$alias, $spec]) {
->setParameter('excludedAuthority', $excludedAuthority); $this->applySpecification($qb, $spec, $alias);
}
if ($apiKey !== null) {
$this->applySpecification($qb, $apiKey->spec(), 's');
} }
return $qb->getQuery()->getResult(); 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()],
}) ?? [];
}
} }

View File

@ -0,0 +1,22 @@
<?php
declare(strict_types=1);
namespace Shlinkio\Shlink\Core\Domain\Spec;
use Happyr\DoctrineSpecification\Filter\Filter;
use Happyr\DoctrineSpecification\Spec;
use Happyr\DoctrineSpecification\Specification\BaseSpecification;
class IsDomain extends BaseSpecification
{
public function __construct(private string $domainId, ?string $context = null)
{
parent::__construct($context);
}
protected function getSpec(): Filter
{
return Spec::eq('id', $this->domainId);
}
}

View File

@ -0,0 +1,22 @@
<?php
declare(strict_types=1);
namespace Shlinkio\Shlink\Core\Domain\Spec;
use Happyr\DoctrineSpecification\Filter\Filter;
use Happyr\DoctrineSpecification\Spec;
use Happyr\DoctrineSpecification\Specification\BaseSpecification;
class IsNotAuthority extends BaseSpecification
{
public function __construct(private string $authority, ?string $context = null)
{
parent::__construct($context);
}
protected function getSpec(): Filter
{
return Spec::not(Spec::eq('authority', $this->authority));
}
}

View File

@ -11,13 +11,13 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey;
class BelongsToApiKey extends BaseSpecification 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 protected function getSpec(): Filter
{ {
return Spec::eq('authorApiKey', $this->apiKey, $this->dqlAlias); return Spec::eq('authorApiKey', $this->apiKey);
} }
} }

View File

@ -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 // 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 . '\''); return (string) $qb->expr()->eq('s.domain', '\'' . $this->domainId . '\'');

View File

@ -92,12 +92,12 @@ class DomainRepositoryTest extends DatabaseTestCase
$this->getEntityManager()->persist($bazDomain); $this->getEntityManager()->persist($bazDomain);
$this->getEntityManager()->persist($this->createShortUrl($bazDomain, $authorApiKey)); $this->getEntityManager()->persist($this->createShortUrl($bazDomain, $authorApiKey));
// $detachedDomain = Domain::withAuthority('detached.com'); $detachedDomain = Domain::withAuthority('detached.com');
// $this->getEntityManager()->persist($detachedDomain); $this->getEntityManager()->persist($detachedDomain);
//
// $detachedWithRedirects = Domain::withAuthority('detached-with-redirects.com'); $detachedWithRedirects = Domain::withAuthority('detached-with-redirects.com');
// $detachedWithRedirects->configureNotFoundRedirects(new NotFoundRedirects('foo.com', 'bar.com')); $detachedWithRedirects->configureNotFoundRedirects(new NotFoundRedirects('foo.com', 'bar.com'));
// $this->getEntityManager()->persist($detachedWithRedirects); $this->getEntityManager()->persist($detachedWithRedirects);
$this->getEntityManager()->flush(); $this->getEntityManager()->flush();
@ -109,19 +109,19 @@ class DomainRepositoryTest extends DatabaseTestCase
$barDomainApiKey = ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forDomain($barDomain))); $barDomainApiKey = ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forDomain($barDomain)));
$this->getEntityManager()->persist($barDomainApiKey); $this->getEntityManager()->persist($barDomainApiKey);
// $detachedWithRedirectsApiKey = ApiKey::fromMeta( $detachedWithRedirectsApiKey = ApiKey::fromMeta(
// ApiKeyMeta::withRoles(RoleDefinition::forDomain($detachedWithRedirects)), ApiKeyMeta::withRoles(RoleDefinition::forDomain($detachedWithRedirects)),
// ); );
// $this->getEntityManager()->persist($detachedWithRedirectsApiKey); $this->getEntityManager()->persist($detachedWithRedirectsApiKey);
$this->getEntityManager()->flush(); $this->getEntityManager()->flush();
self::assertEquals([$fooDomain], $this->repo->findDomainsWithout(null, $fooDomainApiKey)); self::assertEquals([$fooDomain], $this->repo->findDomainsWithout(null, $fooDomainApiKey));
self::assertEquals([$barDomain], $this->repo->findDomainsWithout(null, $barDomainApiKey)); self::assertEquals([$barDomain], $this->repo->findDomainsWithout(null, $barDomainApiKey));
// self::assertEquals( self::assertEquals(
// [$detachedWithRedirects], [$detachedWithRedirects],
// $this->repo->findDomainsWithout(null, $detachedWithRedirectsApiKey), $this->repo->findDomainsWithout(null, $detachedWithRedirectsApiKey),
// ); );
self::assertEquals([$bazDomain, $fooDomain], $this->repo->findDomainsWithout(null, $authorApiKey)); self::assertEquals([$bazDomain, $fooDomain], $this->repo->findDomainsWithout(null, $authorApiKey));
self::assertEquals([], $this->repo->findDomainsWithout(null, $authorAndDomainApiKey)); self::assertEquals([], $this->repo->findDomainsWithout(null, $authorAndDomainApiKey));
} }

View File

@ -119,6 +119,11 @@ class ApiKey extends AbstractEntity
return $role?->meta() ?? []; return $role?->meta() ?? [];
} }
/**
* @template T
* @param callable(string $roleName, array $meta): T $fun
* @return T[]
*/
public function mapRoles(callable $fun): array public function mapRoles(callable $fun): array
{ {
return $this->roles->map(fn (ApiKeyRole $role) => $fun($role->name(), $role->meta()))->getValues(); return $this->roles->map(fn (ApiKeyRole $role) => $fun($role->name(), $role->meta()))->getValues();