diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c967cdf..204486f1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this ## [Unreleased] ### Added -* *Nothing* +* [#1780](https://github.com/shlinkio/shlink/issues/1780) Add new `NO_ORPHAN_VISITS` API key role. + + Keys with this role will always get `0` when fetching orphan visits. + + When trying to delete orphan visits the result will also be `0` and no visits will actually get deleted. ### Changed * *Nothing* diff --git a/module/CLI/src/ApiKey/RoleResolver.php b/module/CLI/src/ApiKey/RoleResolver.php index c1ae8f05..4df10bf4 100644 --- a/module/CLI/src/ApiKey/RoleResolver.php +++ b/module/CLI/src/ApiKey/RoleResolver.php @@ -22,6 +22,7 @@ class RoleResolver implements RoleResolverInterface { $domainAuthority = $input->getOption(Role::DOMAIN_SPECIFIC->paramName()); $author = $input->getOption(Role::AUTHORED_SHORT_URLS->paramName()); + $noOrphanVisits = $input->getOption(Role::NO_ORPHAN_VISITS->paramName()); $roleDefinitions = []; if ($author) { @@ -30,6 +31,9 @@ class RoleResolver implements RoleResolverInterface if (is_string($domainAuthority)) { $roleDefinitions[] = $this->resolveRoleForAuthority($domainAuthority); } + if ($noOrphanVisits) { + $roleDefinitions[] = RoleDefinition::forNoOrphanVisits(); + } return $roleDefinitions; } diff --git a/module/CLI/src/Command/Api/GenerateKeyCommand.php b/module/CLI/src/Command/Api/GenerateKeyCommand.php index c2d6cf10..ab4b5d54 100644 --- a/module/CLI/src/Command/Api/GenerateKeyCommand.php +++ b/module/CLI/src/Command/Api/GenerateKeyCommand.php @@ -25,8 +25,8 @@ class GenerateKeyCommand extends Command public const NAME = 'api-key:generate'; public function __construct( - private ApiKeyServiceInterface $apiKeyService, - private RoleResolverInterface $roleResolver, + private readonly ApiKeyServiceInterface $apiKeyService, + private readonly RoleResolverInterface $roleResolver, ) { parent::__construct(); } @@ -35,6 +35,8 @@ class GenerateKeyCommand extends Command { $authorOnly = Role::AUTHORED_SHORT_URLS->paramName(); $domainOnly = Role::DOMAIN_SPECIFIC->paramName(); + $noOrphanVisits = Role::NO_ORPHAN_VISITS->paramName(); + $help = <<%command.name% generates a new valid API key. @@ -52,7 +54,8 @@ class GenerateKeyCommand extends Command * Can interact with short URLs created with this API key: %command.full_name% --{$authorOnly} * Can interact with short URLs for one domain: %command.full_name% --{$domainOnly}=example.com - * Both: %command.full_name% --{$authorOnly} --{$domainOnly}=example.com + * Cannot see orphan visits: %command.full_name% --{$noOrphanVisits} + * All: %command.full_name% --{$authorOnly} --{$domainOnly}=example.com --{$noOrphanVisits} HELP; $this @@ -85,6 +88,12 @@ class GenerateKeyCommand extends Command Role::DOMAIN_SPECIFIC->value, ), ) + ->addOption( + $noOrphanVisits, + 'o', + InputOption::VALUE_NONE, + sprintf('Adds the "%s" role to the new API key.', Role::NO_ORPHAN_VISITS->value), + ) ->setHelp($help); } diff --git a/module/CLI/src/Command/Api/ListKeysCommand.php b/module/CLI/src/Command/Api/ListKeysCommand.php index 87b239b7..4fd4b005 100644 --- a/module/CLI/src/Command/Api/ListKeysCommand.php +++ b/module/CLI/src/Command/Api/ListKeysCommand.php @@ -27,7 +27,7 @@ class ListKeysCommand extends Command public const NAME = 'api-key:list'; - public function __construct(private ApiKeyServiceInterface $apiKeyService) + public function __construct(private readonly ApiKeyServiceInterface $apiKeyService) { parent::__construct(); } @@ -60,10 +60,7 @@ class ListKeysCommand extends Command } $rowData[] = $expiration?->toAtomString() ?? '-'; $rowData[] = ApiKey::isAdmin($apiKey) ? 'Admin' : implode("\n", $apiKey->mapRoles( - fn (Role $role, array $meta) => - empty($meta) - ? $role->toFriendlyName() - : sprintf('%s: %s', $role->toFriendlyName(), Role::domainAuthorityFromMeta($meta)), + fn (Role $role, array $meta) => $role->toFriendlyName($meta), )); return $rowData; diff --git a/module/CLI/test-cli/Command/ListApiKeysTest.php b/module/CLI/test-cli/Command/ListApiKeysTest.php index f8781d54..633cf819 100644 --- a/module/CLI/test-cli/Command/ListApiKeysTest.php +++ b/module/CLI/test-cli/Command/ListApiKeysTest.php @@ -26,34 +26,38 @@ class ListApiKeysTest extends CliTestCase { $expiredApiKeyDate = Chronos::now()->subDay()->startOfDay()->toAtomString(); $enabledOnlyOutput = << [[], << [['-e'], $enabledOnlyOutput]; diff --git a/module/Core/src/Domain/Repository/DomainRepository.php b/module/Core/src/Domain/Repository/DomainRepository.php index dcbc3d9e..aae44aed 100644 --- a/module/Core/src/Domain/Repository/DomainRepository.php +++ b/module/Core/src/Domain/Repository/DomainRepository.php @@ -79,6 +79,7 @@ class DomainRepository extends EntitySpecificationRepository implements DomainRe yield from $apiKey?->mapRoles(fn (Role $role, array $meta) => match ($role) { Role::DOMAIN_SPECIFIC => ['d', new IsDomain(Role::domainIdFromMeta($meta))], Role::AUTHORED_SHORT_URLS => ['s', new BelongsToApiKey($apiKey)], + default => null, }) ?? []; } } diff --git a/module/Core/src/Tag/Repository/TagRepository.php b/module/Core/src/Tag/Repository/TagRepository.php index 68e5df4b..278dbe8b 100644 --- a/module/Core/src/Tag/Repository/TagRepository.php +++ b/module/Core/src/Tag/Repository/TagRepository.php @@ -56,10 +56,11 @@ class TagRepository extends EntitySpecificationRepository implements TagReposito Role::AUTHORED_SHORT_URLS => $qb->andWhere( $qb->expr()->eq('s.author_api_key_id', $conn->quote($apiKey->getId())), ), + default => $qb, }); - // For admins and when no API key is present, we'll return tags which are not linked to any short URL - $joiningMethod = ApiKey::isAdmin($apiKey) ? 'leftJoin' : 'join'; + // For non-restricted API keys, we'll return tags which are not linked to any short URL + $joiningMethod = ! ApiKey::isShortUrlRestricted($apiKey) ? 'leftJoin' : 'join'; $tagsSubQb = $conn->createQueryBuilder(); $tagsSubQb ->select('t.id AS tag_id', 't.name AS tag', 'COUNT(DISTINCT s.id) AS short_urls_count') diff --git a/module/Core/src/Tag/TagService.php b/module/Core/src/Tag/TagService.php index ea9a4e8b..36fd0514 100644 --- a/module/Core/src/Tag/TagService.php +++ b/module/Core/src/Tag/TagService.php @@ -59,7 +59,7 @@ class TagService implements TagServiceInterface */ public function deleteTags(array $tagNames, ?ApiKey $apiKey = null): void { - if (! ApiKey::isAdmin($apiKey)) { + if (ApiKey::isShortUrlRestricted($apiKey)) { throw ForbiddenTagOperationException::forDeletion(); } @@ -75,7 +75,7 @@ class TagService implements TagServiceInterface */ public function renameTag(TagRenaming $renaming, ?ApiKey $apiKey = null): Tag { - if (! ApiKey::isAdmin($apiKey)) { + if (ApiKey::isShortUrlRestricted($apiKey)) { throw ForbiddenTagOperationException::forRenaming(); } diff --git a/module/Core/src/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapter.php b/module/Core/src/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapter.php index 4e6e4daf..e871d125 100644 --- a/module/Core/src/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapter.php +++ b/module/Core/src/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapter.php @@ -9,11 +9,15 @@ use Shlinkio\Shlink\Core\Visit\Model\VisitsParams; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering; use Shlinkio\Shlink\Core\Visit\Repository\VisitRepositoryInterface; +use Shlinkio\Shlink\Rest\Entity\ApiKey; class OrphanVisitsPaginatorAdapter extends AbstractCacheableCountPaginatorAdapter { - public function __construct(private readonly VisitRepositoryInterface $repo, private readonly VisitsParams $params) - { + public function __construct( + private readonly VisitRepositoryInterface $repo, + private readonly VisitsParams $params, + private readonly ?ApiKey $apiKey, + ) { } protected function doCount(): int @@ -21,6 +25,7 @@ class OrphanVisitsPaginatorAdapter extends AbstractCacheableCountPaginatorAdapte return $this->repo->countOrphanVisits(new VisitsCountFiltering( dateRange: $this->params->dateRange, excludeBots: $this->params->excludeBots, + apiKey: $this->apiKey, )); } @@ -29,6 +34,7 @@ class OrphanVisitsPaginatorAdapter extends AbstractCacheableCountPaginatorAdapte return $this->repo->findOrphanVisits(new VisitsListFiltering( dateRange: $this->params->dateRange, excludeBots: $this->params->excludeBots, + apiKey: $this->apiKey, limit: $length, offset: $offset, )); diff --git a/module/Core/src/Visit/Repository/VisitRepository.php b/module/Core/src/Visit/Repository/VisitRepository.php index 7021e70b..dc54057c 100644 --- a/module/Core/src/Visit/Repository/VisitRepository.php +++ b/module/Core/src/Visit/Repository/VisitRepository.php @@ -17,6 +17,7 @@ use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering; use Shlinkio\Shlink\Core\Visit\Spec\CountOfNonOrphanVisits; use Shlinkio\Shlink\Core\Visit\Spec\CountOfOrphanVisits; +use Shlinkio\Shlink\Rest\ApiKey\Role; use const PHP_INT_MAX; @@ -139,6 +140,10 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo public function findOrphanVisits(VisitsListFiltering $filtering): array { + if ($filtering->apiKey?->hasRole(Role::NO_ORPHAN_VISITS)) { + return []; + } + $qb = $this->createAllVisitsQueryBuilder($filtering); $qb->andWhere($qb->expr()->isNull('v.shortUrl')); return $this->resolveVisitsWithNativeQuery($qb, $filtering->limit, $filtering->offset); @@ -146,6 +151,10 @@ class VisitRepository extends EntitySpecificationRepository implements VisitRepo public function countOrphanVisits(VisitsCountFiltering $filtering): int { + if ($filtering->apiKey?->hasRole(Role::NO_ORPHAN_VISITS)) { + return 0; + } + return (int) $this->matchSingleScalarResult(new CountOfOrphanVisits($filtering)); } diff --git a/module/Core/src/Visit/VisitsDeleter.php b/module/Core/src/Visit/VisitsDeleter.php index 5a8adca5..2b925e17 100644 --- a/module/Core/src/Visit/VisitsDeleter.php +++ b/module/Core/src/Visit/VisitsDeleter.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Core\Visit; use Shlinkio\Shlink\Core\Model\BulkDeleteResult; use Shlinkio\Shlink\Core\Visit\Repository\VisitDeleterRepositoryInterface; +use Shlinkio\Shlink\Rest\ApiKey\Role; use Shlinkio\Shlink\Rest\Entity\ApiKey; class VisitsDeleter implements VisitsDeleterInterface @@ -16,7 +17,7 @@ class VisitsDeleter implements VisitsDeleterInterface public function deleteOrphanVisits(?ApiKey $apiKey = null): BulkDeleteResult { - // TODO Check API key has permissions for orphan visits - return new BulkDeleteResult($this->repository->deleteOrphanVisits()); + $affectedItems = $apiKey?->hasRole(Role::NO_ORPHAN_VISITS) ? 0 : $this->repository->deleteOrphanVisits(); + return new BulkDeleteResult($affectedItems); } } diff --git a/module/Core/src/Visit/VisitsStatsHelper.php b/module/Core/src/Visit/VisitsStatsHelper.php index 25f44921..bdd2fd3b 100644 --- a/module/Core/src/Visit/VisitsStatsHelper.php +++ b/module/Core/src/Visit/VisitsStatsHelper.php @@ -43,11 +43,13 @@ class VisitsStatsHelper implements VisitsStatsHelperInterface return new VisitsStats( nonOrphanVisitsTotal: $visitsRepo->countNonOrphanVisits(VisitsCountFiltering::withApiKey($apiKey)), - orphanVisitsTotal: $visitsRepo->countOrphanVisits(new VisitsCountFiltering()), + orphanVisitsTotal: $visitsRepo->countOrphanVisits(VisitsCountFiltering::withApiKey($apiKey)), nonOrphanVisitsNonBots: $visitsRepo->countNonOrphanVisits( new VisitsCountFiltering(excludeBots: true, apiKey: $apiKey), ), - orphanVisitsNonBots: $visitsRepo->countOrphanVisits(new VisitsCountFiltering(excludeBots: true)), + orphanVisitsNonBots: $visitsRepo->countOrphanVisits( + new VisitsCountFiltering(excludeBots: true, apiKey: $apiKey), + ), ); } @@ -114,12 +116,12 @@ class VisitsStatsHelper implements VisitsStatsHelperInterface /** * @return Visit[]|Paginator */ - public function orphanVisits(VisitsParams $params): Paginator + public function orphanVisits(VisitsParams $params, ?ApiKey $apiKey = null): Paginator { /** @var VisitRepositoryInterface $repo */ $repo = $this->em->getRepository(Visit::class); - return $this->createPaginator(new OrphanVisitsPaginatorAdapter($repo, $params), $params); + return $this->createPaginator(new OrphanVisitsPaginatorAdapter($repo, $params, $apiKey), $params); } public function nonOrphanVisits(VisitsParams $params, ?ApiKey $apiKey = null): Paginator diff --git a/module/Core/src/Visit/VisitsStatsHelperInterface.php b/module/Core/src/Visit/VisitsStatsHelperInterface.php index af6cb77c..71173553 100644 --- a/module/Core/src/Visit/VisitsStatsHelperInterface.php +++ b/module/Core/src/Visit/VisitsStatsHelperInterface.php @@ -43,7 +43,7 @@ interface VisitsStatsHelperInterface /** * @return Visit[]|Paginator */ - public function orphanVisits(VisitsParams $params): Paginator; + public function orphanVisits(VisitsParams $params, ?ApiKey $apiKey = null): Paginator; /** * @return Visit[]|Paginator diff --git a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php index 6eb2fe4e..cca71a14 100644 --- a/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php +++ b/module/Core/test-db/Visit/Repository/VisitRepositoryTest.php @@ -262,6 +262,9 @@ class VisitRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); + $noOrphanVisitsApiKey = ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forNoOrphanVisits())); + $this->getEntityManager()->persist($noOrphanVisitsApiKey); + $apiKey1 = ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls())); $this->getEntityManager()->persist($apiKey1); $shortUrl = ShortUrl::create( @@ -305,6 +308,7 @@ class VisitRepositoryTest extends DatabaseTestCase self::assertEquals(4, $this->repo->countNonOrphanVisits(VisitsCountFiltering::withApiKey($apiKey1))); self::assertEquals(5 + 7, $this->repo->countNonOrphanVisits(VisitsCountFiltering::withApiKey($apiKey2))); self::assertEquals(4 + 7, $this->repo->countNonOrphanVisits(VisitsCountFiltering::withApiKey($domainApiKey))); + self::assertEquals(0, $this->repo->countOrphanVisits(VisitsCountFiltering::withApiKey($noOrphanVisitsApiKey))); self::assertEquals(4, $this->repo->countNonOrphanVisits(new VisitsCountFiltering(DateRange::since( Chronos::parse('2016-01-05')->startOfDay(), )))); @@ -326,6 +330,9 @@ class VisitRepositoryTest extends DatabaseTestCase $this->getEntityManager()->persist($shortUrl); $this->createVisitsForShortUrl($shortUrl, 7); + $noOrphanVisitsApiKey = ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forNoOrphanVisits())); + $this->getEntityManager()->persist($noOrphanVisitsApiKey); + $botsCount = 3; for ($i = 0; $i < 6; $i++) { $this->getEntityManager()->persist($this->setDateOnVisit( @@ -346,6 +353,7 @@ class VisitRepositoryTest extends DatabaseTestCase $this->getEntityManager()->flush(); + self::assertCount(0, $this->repo->findOrphanVisits(new VisitsListFiltering(apiKey: $noOrphanVisitsApiKey))); self::assertCount(18, $this->repo->findOrphanVisits(new VisitsListFiltering())); self::assertCount(15, $this->repo->findOrphanVisits(new VisitsListFiltering(null, true))); self::assertCount(5, $this->repo->findOrphanVisits(new VisitsListFiltering(null, false, null, 5))); diff --git a/module/Core/test/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapterTest.php b/module/Core/test/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapterTest.php index 6a367ed7..04e3f84c 100644 --- a/module/Core/test/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapterTest.php +++ b/module/Core/test/Visit/Paginator/Adapter/OrphanVisitsPaginatorAdapterTest.php @@ -15,18 +15,22 @@ use Shlinkio\Shlink\Core\Visit\Paginator\Adapter\OrphanVisitsPaginatorAdapter; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsCountFiltering; use Shlinkio\Shlink\Core\Visit\Persistence\VisitsListFiltering; use Shlinkio\Shlink\Core\Visit\Repository\VisitRepositoryInterface; +use Shlinkio\Shlink\Rest\Entity\ApiKey; class OrphanVisitsPaginatorAdapterTest extends TestCase { private OrphanVisitsPaginatorAdapter $adapter; private MockObject & VisitRepositoryInterface $repo; private VisitsParams $params; + private ApiKey $apiKey; protected function setUp(): void { $this->repo = $this->createMock(VisitRepositoryInterface::class); $this->params = VisitsParams::fromRawData([]); - $this->adapter = new OrphanVisitsPaginatorAdapter($this->repo, $this->params); + $this->apiKey = ApiKey::create(); + + $this->adapter = new OrphanVisitsPaginatorAdapter($this->repo, $this->params, $this->apiKey); } #[Test] @@ -34,7 +38,7 @@ class OrphanVisitsPaginatorAdapterTest extends TestCase { $expectedCount = 5; $this->repo->expects($this->once())->method('countOrphanVisits')->with( - new VisitsCountFiltering($this->params->dateRange), + new VisitsCountFiltering($this->params->dateRange, apiKey: $this->apiKey), )->willReturn($expectedCount); $result = $this->adapter->getNbResults(); @@ -51,9 +55,13 @@ class OrphanVisitsPaginatorAdapterTest extends TestCase { $visitor = Visitor::emptyInstance(); $list = [Visit::forRegularNotFound($visitor), Visit::forInvalidShortUrl($visitor)]; - $this->repo->expects($this->once())->method('findOrphanVisits')->with( - new VisitsListFiltering($this->params->dateRange, $this->params->excludeBots, null, $limit, $offset), - )->willReturn($list); + $this->repo->expects($this->once())->method('findOrphanVisits')->with(new VisitsListFiltering( + $this->params->dateRange, + $this->params->excludeBots, + $this->apiKey, + $limit, + $offset, + ))->willReturn($list); $result = $this->adapter->getSlice($offset, $limit); diff --git a/module/Core/test/Visit/VisitsDeleterTest.php b/module/Core/test/Visit/VisitsDeleterTest.php index 155d0725..e47706a9 100644 --- a/module/Core/test/Visit/VisitsDeleterTest.php +++ b/module/Core/test/Visit/VisitsDeleterTest.php @@ -10,6 +10,9 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Visit\Repository\VisitDeleterRepositoryInterface; use Shlinkio\Shlink\Core\Visit\VisitsDeleter; +use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; +use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; +use Shlinkio\Shlink\Rest\Entity\ApiKey; class VisitsDeleterTest extends TestCase { @@ -38,4 +41,16 @@ class VisitsDeleterTest extends TestCase yield '5000' => [5000]; yield '0' => [0]; } + + #[Test] + public function returnsNoDeletedVisitsForApiKeyWithNoPermission(): void + { + $this->repo->expects($this->never())->method('deleteOrphanVisits'); + + $result = $this->visitsDeleter->deleteOrphanVisits( + ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forNoOrphanVisits())), + ); + + self::assertEquals(0, $result->affectedItems); + } } diff --git a/module/Core/test/Visit/VisitsStatsHelperTest.php b/module/Core/test/Visit/VisitsStatsHelperTest.php index 18954a22..3fc024df 100644 --- a/module/Core/test/Visit/VisitsStatsHelperTest.php +++ b/module/Core/test/Visit/VisitsStatsHelperTest.php @@ -50,13 +50,14 @@ class VisitsStatsHelperTest extends TestCase } #[Test, DataProvider('provideCounts')] - public function returnsExpectedVisitsStats(int $expectedCount): void + public function returnsExpectedVisitsStats(int $expectedCount, ?ApiKey $apiKey): void { $repo = $this->createMock(VisitRepository::class); $callCount = 0; $repo->expects($this->exactly(2))->method('countNonOrphanVisits')->willReturnCallback( - function (VisitsCountFiltering $options) use ($expectedCount, &$callCount) { + function (VisitsCountFiltering $options) use ($expectedCount, $apiKey, &$callCount) { Assert::assertEquals($callCount !== 0, $options->excludeBots); + Assert::assertEquals($apiKey, $options->apiKey); $callCount++; return $expectedCount * 3; @@ -67,14 +68,17 @@ class VisitsStatsHelperTest extends TestCase )->willReturn($expectedCount); $this->em->expects($this->once())->method('getRepository')->with(Visit::class)->willReturn($repo); - $stats = $this->helper->getVisitsStats(); + $stats = $this->helper->getVisitsStats($apiKey); self::assertEquals(new VisitsStats($expectedCount * 3, $expectedCount), $stats); } public static function provideCounts(): iterable { - return map(range(0, 50, 5), fn (int $value) => [$value]); + return [ + ...map(range(0, 50, 5), fn (int $value) => [$value, null]), + ...map(range(0, 18, 3), fn (int $value) => [$value, ApiKey::create()]), + ]; } #[Test, DataProvider('provideAdminApiKeys')] diff --git a/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKey.php b/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKey.php index 1e0b041b..7d0f3583 100644 --- a/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKey.php +++ b/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKey.php @@ -44,7 +44,7 @@ return static function (ClassMetadata $metadata, array $emConfig): void { $builder->createOneToMany('roles', ApiKeyRole::class) ->mappedBy('apiKey') - ->setIndexBy('roleName') + ->setIndexBy('role') ->cascadePersist() ->orphanRemoval() ->build(); diff --git a/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKeyRole.php b/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKeyRole.php index 8df324a4..04d1cf9d 100644 --- a/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKeyRole.php +++ b/module/Rest/config/entities-mappings/Shlinkio.Shlink.Rest.Entity.ApiKeyRole.php @@ -25,7 +25,7 @@ return static function (ClassMetadata $metadata, array $emConfig): void { ->build(); (new FieldBuilder($builder, [ - 'fieldName' => 'roleName', + 'fieldName' => 'role', 'type' => Types::STRING, 'enumType' => Role::class, ]))->columnName('role_name') diff --git a/module/Rest/src/Action/Visit/OrphanVisitsAction.php b/module/Rest/src/Action/Visit/OrphanVisitsAction.php index af5292a2..c7adf3a1 100644 --- a/module/Rest/src/Action/Visit/OrphanVisitsAction.php +++ b/module/Rest/src/Action/Visit/OrphanVisitsAction.php @@ -12,6 +12,7 @@ use Shlinkio\Shlink\Common\Rest\DataTransformerInterface; use Shlinkio\Shlink\Core\Visit\Model\VisitsParams; use Shlinkio\Shlink\Core\Visit\VisitsStatsHelperInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; +use Shlinkio\Shlink\Rest\Middleware\AuthenticationMiddleware; class OrphanVisitsAction extends AbstractRestAction { @@ -29,7 +30,8 @@ class OrphanVisitsAction extends AbstractRestAction public function handle(ServerRequestInterface $request): ResponseInterface { $params = VisitsParams::fromRawData($request->getQueryParams()); - $visits = $this->visitsHelper->orphanVisits($params); + $apiKey = AuthenticationMiddleware::apiKeyFromRequest($request); + $visits = $this->visitsHelper->orphanVisits($params, $apiKey); return new JsonResponse([ 'visits' => $this->serializePaginator($visits, $this->orphanVisitTransformer), diff --git a/module/Rest/src/ApiKey/Model/RoleDefinition.php b/module/Rest/src/ApiKey/Model/RoleDefinition.php index 403e6214..a35e904c 100644 --- a/module/Rest/src/ApiKey/Model/RoleDefinition.php +++ b/module/Rest/src/ApiKey/Model/RoleDefinition.php @@ -25,4 +25,9 @@ final class RoleDefinition ['domain_id' => $domain->getId(), 'authority' => $domain->authority], ); } + + public static function forNoOrphanVisits(): self + { + return new self(Role::NO_ORPHAN_VISITS, []); + } } diff --git a/module/Rest/src/ApiKey/Role.php b/module/Rest/src/ApiKey/Role.php index 5a4edb81..dd2d8ae7 100644 --- a/module/Rest/src/ApiKey/Role.php +++ b/module/Rest/src/ApiKey/Role.php @@ -12,16 +12,20 @@ use Shlinkio\Shlink\Core\ShortUrl\Spec\BelongsToDomain; use Shlinkio\Shlink\Core\ShortUrl\Spec\BelongsToDomainInlined; use Shlinkio\Shlink\Rest\Entity\ApiKeyRole; +use function sprintf; + enum Role: string { case AUTHORED_SHORT_URLS = 'AUTHORED_SHORT_URLS'; case DOMAIN_SPECIFIC = 'DOMAIN_SPECIFIC'; + case NO_ORPHAN_VISITS = 'NO_ORPHAN_VISITS'; - public function toFriendlyName(): string + public function toFriendlyName(array $meta): string { return match ($this) { self::AUTHORED_SHORT_URLS => 'Author only', - self::DOMAIN_SPECIFIC => 'Domain only', + self::DOMAIN_SPECIFIC => sprintf('Domain only: %s', Role::domainAuthorityFromMeta($meta)), + self::NO_ORPHAN_VISITS => 'No orphan visits', }; } @@ -30,6 +34,7 @@ enum Role: string return match ($this) { self::AUTHORED_SHORT_URLS => 'author-only', self::DOMAIN_SPECIFIC => 'domain-only', + self::NO_ORPHAN_VISITS => 'no-orphan-visits', }; } @@ -38,6 +43,7 @@ enum Role: string return match ($role->role()) { self::AUTHORED_SHORT_URLS => new BelongsToApiKey($role->apiKey(), $context), self::DOMAIN_SPECIFIC => new BelongsToDomain(self::domainIdFromMeta($role->meta()), $context), + default => Spec::andX(), }; } @@ -46,6 +52,7 @@ enum Role: string return match ($role->role()) { self::AUTHORED_SHORT_URLS => Spec::andX(new BelongsToApiKeyInlined($role->apiKey())), self::DOMAIN_SPECIFIC => Spec::andX(new BelongsToDomainInlined(self::domainIdFromMeta($role->meta()))), + default => Spec::andX(), }; } diff --git a/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php b/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php index 9a8f8056..122829ed 100644 --- a/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php +++ b/module/Rest/src/ApiKey/Spec/WithApiKeySpecsEnsuringJoin.php @@ -18,7 +18,7 @@ class WithApiKeySpecsEnsuringJoin extends BaseSpecification protected function getSpec(): Specification { - return $this->apiKey === null || ApiKey::isAdmin($this->apiKey) ? Spec::andX() : Spec::andX( + return $this->apiKey === null || ! ApiKey::isShortUrlRestricted($this->apiKey) ? Spec::andX() : Spec::andX( Spec::join($this->fieldToJoin, 's'), $this->apiKey->spec($this->fieldToJoin), ); diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index 88cfa27e..72977c86 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -122,6 +122,21 @@ class ApiKey extends AbstractEntity return $apiKey === null || $apiKey->roles->isEmpty(); } + /** + * Tells if provided API key has any of the roles restricting at the short URL level + */ + public static function isShortUrlRestricted(?ApiKey $apiKey): bool + { + if ($apiKey === null) { + return false; + } + + return ( + $apiKey->roles->containsKey(Role::AUTHORED_SHORT_URLS->value) + || $apiKey->roles->containsKey(Role::DOMAIN_SPECIFIC->value) + ); + } + public function hasRole(Role $role): bool { return $this->roles->containsKey($role->value); diff --git a/module/Rest/src/Entity/ApiKeyRole.php b/module/Rest/src/Entity/ApiKeyRole.php index 8491cfce..6fadb839 100644 --- a/module/Rest/src/Entity/ApiKeyRole.php +++ b/module/Rest/src/Entity/ApiKeyRole.php @@ -9,13 +9,24 @@ use Shlinkio\Shlink\Rest\ApiKey\Role; class ApiKeyRole extends AbstractEntity { - public function __construct(private Role $roleName, private array $meta, private ApiKey $apiKey) + public function __construct(public readonly Role $role, private array $meta, public readonly ApiKey $apiKey) { } + /** + * @deprecated Use property access directly + */ public function role(): Role { - return $this->roleName; + return $this->role; + } + + /** + * @deprecated Use property access directly + */ + public function apiKey(): ApiKey + { + return $this->apiKey; } public function meta(): array @@ -27,9 +38,4 @@ class ApiKeyRole extends AbstractEntity { $this->meta = $newMeta; } - - public function apiKey(): ApiKey - { - return $this->apiKey; - } } diff --git a/module/Rest/test-api/Action/DeleteOrphanVisitsTest.php b/module/Rest/test-api/Action/DeleteOrphanVisitsTest.php index b7cf59b9..63a2f165 100644 --- a/module/Rest/test-api/Action/DeleteOrphanVisitsTest.php +++ b/module/Rest/test-api/Action/DeleteOrphanVisitsTest.php @@ -10,7 +10,7 @@ use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase; class DeleteOrphanVisitsTest extends ApiTestCase { #[Test] - public function deletesVisitsForShortUrlWithoutAffectingTheRest(): void + public function deletesOrphanVisitsWithoutAffectingTheRest(): void { self::assertEquals(7, $this->getTotalVisits()); self::assertEquals(3, $this->getOrphanVisits()); @@ -24,6 +24,21 @@ class DeleteOrphanVisitsTest extends ApiTestCase self::assertEquals(0, $this->getOrphanVisits()); } + #[Test] + public function doesNotDeleteOrphanVisitsForRestrictedApiKey(): void + { + self::assertEquals(7, $this->getTotalVisits()); + self::assertEquals(3, $this->getOrphanVisits()); + + $resp = $this->callApiWithKey(self::METHOD_DELETE, '/visits/orphan', apiKey: 'no_orphans_api_key'); + $payload = $this->getJsonResponsePayload($resp); + + self::assertEquals(200, $resp->getStatusCode()); + self::assertEquals(0, $payload['deletedVisits']); + self::assertEquals(7, $this->getTotalVisits()); // This verifies that regular visits have not been affected + self::assertEquals(3, $this->getOrphanVisits()); // This verifies that all orphan visits still exist + } + private function getTotalVisits(): int { $resp = $this->callApiWithKey(self::METHOD_GET, '/visits/non-orphan'); diff --git a/module/Rest/test-api/Action/GlobalVisitsTest.php b/module/Rest/test-api/Action/GlobalVisitsTest.php index 50591a14..657f16a6 100644 --- a/module/Rest/test-api/Action/GlobalVisitsTest.php +++ b/module/Rest/test-api/Action/GlobalVisitsTest.php @@ -11,7 +11,7 @@ use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase; class GlobalVisitsTest extends ApiTestCase { #[Test, DataProvider('provideApiKeys')] - public function returnsExpectedVisitsStats(string $apiKey, int $expectedVisits): void + public function returnsExpectedVisitsStats(string $apiKey, int $expectedVisits, int $expectedOrphanVisits): void { $resp = $this->callApiWithKey(self::METHOD_GET, '/visits', [], $apiKey); $payload = $this->getJsonResponsePayload($resp); @@ -20,13 +20,14 @@ class GlobalVisitsTest extends ApiTestCase self::assertArrayHasKey('visitsCount', $payload['visits']); self::assertArrayHasKey('orphanVisitsCount', $payload['visits']); self::assertEquals($expectedVisits, $payload['visits']['visitsCount']); - self::assertEquals(3, $payload['visits']['orphanVisitsCount']); + self::assertEquals($expectedOrphanVisits, $payload['visits']['orphanVisitsCount']); } public static function provideApiKeys(): iterable { - yield 'admin API key' => ['valid_api_key', 7]; - yield 'domain API key' => ['domain_api_key', 0]; - yield 'author API key' => ['author_api_key', 5]; + yield 'admin API key' => ['valid_api_key', 7, 3]; + yield 'domain API key' => ['domain_api_key', 0, 3]; + yield 'author API key' => ['author_api_key', 5, 3]; + yield 'no orphans API key' => ['no_orphans_api_key', 7, 0]; } } diff --git a/module/Rest/test-api/Action/OrphanVisitsTest.php b/module/Rest/test-api/Action/OrphanVisitsTest.php index 2049d80d..2c8b2479 100644 --- a/module/Rest/test-api/Action/OrphanVisitsTest.php +++ b/module/Rest/test-api/Action/OrphanVisitsTest.php @@ -69,4 +69,16 @@ class OrphanVisitsTest extends ApiTestCase [self::REGULAR_NOT_FOUND], ]; } + + #[Test] + public function noVisitsAreReturnedForRestrictedApiKey(): void + { + $resp = $this->callApiWithKey(self::METHOD_GET, '/visits/orphan', apiKey: 'no_orphans_api_key'); + $payload = $this->getJsonResponsePayload($resp); + $visits = $payload['visits']['data'] ?? null; + + self::assertIsArray($visits); + self::assertEmpty($visits); + self::assertEquals(0, $payload['visits']['pagination']['totalItems'] ?? Paginator::ALL_ITEMS); + } } diff --git a/module/Rest/test-api/Fixtures/ApiKeyFixture.php b/module/Rest/test-api/Fixtures/ApiKeyFixture.php index 5ac886ce..ef971d63 100644 --- a/module/Rest/test-api/Fixtures/ApiKeyFixture.php +++ b/module/Rest/test-api/Fixtures/ApiKeyFixture.php @@ -23,21 +23,29 @@ class ApiKeyFixture extends AbstractFixture implements DependentFixtureInterface public function load(ObjectManager $manager): void { - $manager->persist($this->buildApiKey('valid_api_key', true)); - $manager->persist($this->buildApiKey('disabled_api_key', false)); - $manager->persist($this->buildApiKey('expired_api_key', true, Chronos::now()->subDay()->startOfDay())); + $manager->persist($this->buildApiKey('valid_api_key', enabled: true)); + $manager->persist($this->buildApiKey('disabled_api_key', enabled: false)); + $manager->persist($this->buildApiKey( + 'expired_api_key', + enabled: true, + expiresAt: Chronos::now()->subDay()->startOfDay(), + )); - $authorApiKey = $this->buildApiKey('author_api_key', true); + $authorApiKey = $this->buildApiKey('author_api_key', enabled: true); $authorApiKey->registerRole(RoleDefinition::forAuthoredShortUrls()); $manager->persist($authorApiKey); $this->addReference('author_api_key', $authorApiKey); /** @var Domain $exampleDomain */ $exampleDomain = $this->getReference('example_domain'); - $domainApiKey = $this->buildApiKey('domain_api_key', true); + $domainApiKey = $this->buildApiKey('domain_api_key', enabled: true); $domainApiKey->registerRole(RoleDefinition::forDomain($exampleDomain)); $manager->persist($domainApiKey); + $authorApiKey = $this->buildApiKey('no_orphans_api_key', enabled: true); + $authorApiKey->registerRole(RoleDefinition::forNoOrphanVisits()); + $manager->persist($authorApiKey); + $manager->flush(); } diff --git a/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php b/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php index f4a22caa..da660d0e 100644 --- a/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php +++ b/module/Rest/test/Action/Visit/OrphanVisitsActionTest.php @@ -17,6 +17,7 @@ use Shlinkio\Shlink\Core\Visit\Model\Visitor; use Shlinkio\Shlink\Core\Visit\Model\VisitsParams; use Shlinkio\Shlink\Core\Visit\VisitsStatsHelperInterface; use Shlinkio\Shlink\Rest\Action\Visit\OrphanVisitsAction; +use Shlinkio\Shlink\Rest\Entity\ApiKey; use function count; @@ -48,7 +49,9 @@ class OrphanVisitsActionTest extends TestCase )->willReturn([]); /** @var JsonResponse $response */ - $response = $this->action->handle(ServerRequestFactory::fromGlobals()); + $response = $this->action->handle( + ServerRequestFactory::fromGlobals()->withAttribute(ApiKey::class, ApiKey::create()), + ); $payload = $response->getPayload(); self::assertCount($visitsAmount, $payload['visits']['data']); diff --git a/module/Rest/test/ApiKey/RoleTest.php b/module/Rest/test/ApiKey/RoleTest.php index b572630b..bf02318a 100644 --- a/module/Rest/test/ApiKey/RoleTest.php +++ b/module/Rest/test/ApiKey/RoleTest.php @@ -86,14 +86,15 @@ class RoleTest extends TestCase } #[Test, DataProvider('provideRoleNames')] - public function getsExpectedRoleFriendlyName(Role $role, string $expectedFriendlyName): void + public function getsExpectedRoleFriendlyName(Role $role, array $meta, string $expectedFriendlyName): void { - self::assertEquals($expectedFriendlyName, $role->toFriendlyName()); + self::assertEquals($expectedFriendlyName, $role->toFriendlyName($meta)); } public static function provideRoleNames(): iterable { - yield Role::AUTHORED_SHORT_URLS->value => [Role::AUTHORED_SHORT_URLS, 'Author only']; - yield Role::DOMAIN_SPECIFIC->value => [Role::DOMAIN_SPECIFIC, 'Domain only']; + yield Role::AUTHORED_SHORT_URLS->value => [Role::AUTHORED_SHORT_URLS, [], 'Author only']; + yield Role::DOMAIN_SPECIFIC->value => [Role::DOMAIN_SPECIFIC, ['authority' => 's.test'], 'Domain only: s.test']; + yield Role::NO_ORPHAN_VISITS->value => [Role::NO_ORPHAN_VISITS, [], 'No orphan visits']; } }