diff --git a/CHANGELOG.md b/CHANGELOG.md index e5a78e82..afbd84d4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,27 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#1819](https://github.com/shlinkio/shlink/issues/1819) Fix incorrect timeout when running DB commands during Shlink start-up. +## [3.6.4] - 2023-09-23 +### Added +* *Nothing* + +### Changed +* [#1866](https://github.com/shlinkio/shlink/issues/1866) The `INITIAL_API_KEY` env var is now only relevant for the official docker image. + + Going forward, new non-docker Shlink installations provisioned with env vars that also wish to provide an initial API key, should do it by using the `vendor/bin/shlink-installer init --initial-api-key=%SOME_KEY%` command, instead of using `INITIAL_API_KEY`. + +### Deprecated +* *Nothing* + +### Removed +* *Nothing* + +### Fixed +* [#1819](https://github.com/shlinkio/shlink/issues/1819) Fix incorrect timeout when running DB commands during Shlink start-up. +* [#1870](https://github.com/shlinkio/shlink/issues/1870) Make sure shared locks include the cache prefix when using Redis. +* [#1866](https://github.com/shlinkio/shlink/issues/1866) Fix error when starting docker image with `INITIAL_API_KEY` env var. + + ## [3.6.3] - 2023-06-14 ### Added * *Nothing* diff --git a/composer.json b/composer.json index 134885d5..769bdbdd 100644 --- a/composer.json +++ b/composer.json @@ -45,11 +45,11 @@ "php-middleware/request-id": "^4.1", "pugx/shortid-php": "^1.1", "ramsey/uuid": "^4.7", - "shlinkio/shlink-common": "dev-main#b38c1ad as 5.6", + "shlinkio/shlink-common": "^5.6", "shlinkio/shlink-config": "dev-main#c0aa01f as 2.5", "shlinkio/shlink-event-dispatcher": "dev-main#bd3a62b as 3.1", "shlinkio/shlink-importer": "^5.1", - "shlinkio/shlink-installer": "dev-develop#b393e6b as 8.5", + "shlinkio/shlink-installer": "^8.5", "shlinkio/shlink-ip-geolocation": "^3.2", "shlinkio/shlink-json": "^1.0", "spiral/roadrunner": "^2023.2", diff --git a/config/autoload/installer.global.php b/config/autoload/installer.global.php index 029a50d6..966e36ee 100644 --- a/config/autoload/installer.global.php +++ b/config/autoload/installer.global.php @@ -86,6 +86,9 @@ return [ InstallationCommand::API_KEY_GENERATE->value => [ 'command' => 'bin/cli ' . Command\Api\GenerateKeyCommand::NAME, ], + InstallationCommand::API_KEY_CREATE->value => [ + 'command' => 'bin/cli ' . Command\Api\InitialApiKeyCommand::NAME, + ], ], ], diff --git a/config/autoload/locks.global.php b/config/autoload/locks.global.php index 5e37e770..28426c6a 100644 --- a/config/autoload/locks.global.php +++ b/config/autoload/locks.global.php @@ -4,6 +4,7 @@ declare(strict_types=1); use Laminas\ServiceManager\AbstractFactory\ConfigAbstractFactory; use Shlinkio\Shlink\Common\Cache\RedisFactory; +use Shlinkio\Shlink\Common\Lock\NamespacedStore; use Shlinkio\Shlink\Common\Logger\LoggerAwareDelegatorFactory; use Shlinkio\Shlink\Core\Config\EnvVars; use Symfony\Component\Lock; @@ -22,11 +23,12 @@ return [ Lock\Store\RedisStore::class => ConfigAbstractFactory::class, Lock\LockFactory::class => ConfigAbstractFactory::class, LOCAL_LOCK_FACTORY => ConfigAbstractFactory::class, + NamespacedStore::class => ConfigAbstractFactory::class, ], 'aliases' => [ 'lock_store' => EnvVars::REDIS_SERVERS->existsInEnv() ? 'redis_lock_store' : 'local_lock_store', - 'redis_lock_store' => Lock\Store\RedisStore::class, + 'redis_lock_store' => NamespacedStore::class, 'local_lock_store' => Lock\Store\FlockStore::class, ], 'delegators' => [ @@ -39,6 +41,8 @@ return [ ConfigAbstractFactory::class => [ Lock\Store\FlockStore::class => ['config.locks.locks_dir'], Lock\Store\RedisStore::class => [RedisFactory::SERVICE_NAME], + NamespacedStore::class => [Lock\Store\RedisStore::class, 'config.cache.namespace'], + Lock\LockFactory::class => ['lock_store'], LOCAL_LOCK_FACTORY => ['local_lock_store'], ], diff --git a/config/config.php b/config/config.php index e0ec6c23..9df29138 100644 --- a/config/config.php +++ b/config/config.php @@ -42,10 +42,9 @@ return (new ConfigAggregator\ConfigAggregator([ Core\ConfigProvider::class, CLI\ConfigProvider::class, Rest\ConfigProvider::class, - new ConfigAggregator\PhpFileProvider('config/autoload/{{,*.}global,{,*.}local}.php'), - $isTestEnv - ? new ConfigAggregator\PhpFileProvider('config/test/*.global.php') - : new ConfigAggregator\ArrayProvider([]), + new ConfigAggregator\PhpFileProvider('config/autoload/{,*.}global.php'), + // Local config should not be loaded during tests, whereas test config should be loaded ONLY during tests + new ConfigAggregator\PhpFileProvider($isTestEnv ? 'config/test/*.global.php' : 'config/autoload/{,*.}local.php'), // Routes have to be loaded last new ConfigAggregator\PhpFileProvider('config/autoload/routes.config.php'), ], 'data/cache/app_config.php', [ diff --git a/docker/docker-entrypoint.sh b/docker/docker-entrypoint.sh index 09bf2ef2..799feb80 100644 --- a/docker/docker-entrypoint.sh +++ b/docker/docker-entrypoint.sh @@ -10,10 +10,15 @@ if [ -z "${GEOLITE_LICENSE_KEY}" ] || [ "${SKIP_INITIAL_GEOLITE_DOWNLOAD}" = "tr flags="${flags} --skip-download-geolite" fi +# If INITIAL_API_KEY was provided, create an initial API key +if [ -n "${INITIAL_API_KEY}" ]; then + flags="${flags} --initial-api-key=${INITIAL_API_KEY}" +fi + php vendor/bin/shlink-installer init ${flags} # Periodically run visit:locate every hour, if ENABLE_PERIODIC_VISIT_LOCATE=true was provided and running as root -# ENABLE_PERIODIC_VISIT_LOCATE is deprecated. Remove cron support in Shlink 4.0.0 +# FIXME: ENABLE_PERIODIC_VISIT_LOCATE is deprecated. Remove cron support in Shlink 4.0.0 if [ "${ENABLE_PERIODIC_VISIT_LOCATE}" = "true" ] && [ "${SHLINK_USER_ID}" = "root" ]; then echo "Configuring periodic visit location..." echo "0 * * * * php /etc/shlink/bin/cli visit:locate -q" > /etc/crontabs/root diff --git a/module/CLI/config/cli.config.php b/module/CLI/config/cli.config.php index 9feeee7b..bcd4fd3c 100644 --- a/module/CLI/config/cli.config.php +++ b/module/CLI/config/cli.config.php @@ -24,6 +24,7 @@ return [ Command\Api\GenerateKeyCommand::NAME => Command\Api\GenerateKeyCommand::class, Command\Api\DisableKeyCommand::NAME => Command\Api\DisableKeyCommand::class, Command\Api\ListKeysCommand::NAME => Command\Api\ListKeysCommand::class, + Command\Api\InitialApiKeyCommand::NAME => Command\Api\InitialApiKeyCommand::class, Command\Tag\ListTagsCommand::NAME => Command\Tag\ListTagsCommand::class, Command\Tag\RenameTagCommand::NAME => Command\Tag\RenameTagCommand::class, diff --git a/module/CLI/config/dependencies.config.php b/module/CLI/config/dependencies.config.php index 6b7fc552..2736a21e 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -53,6 +53,7 @@ return [ Command\Api\GenerateKeyCommand::class => ConfigAbstractFactory::class, Command\Api\DisableKeyCommand::class => ConfigAbstractFactory::class, Command\Api\ListKeysCommand::class => ConfigAbstractFactory::class, + Command\Api\InitialApiKeyCommand::class => ConfigAbstractFactory::class, Command\Tag\ListTagsCommand::class => ConfigAbstractFactory::class, Command\Tag\RenameTagCommand::class => ConfigAbstractFactory::class, @@ -105,6 +106,7 @@ return [ Command\Api\GenerateKeyCommand::class => [ApiKeyService::class, ApiKey\RoleResolver::class], Command\Api\DisableKeyCommand::class => [ApiKeyService::class], Command\Api\ListKeysCommand::class => [ApiKeyService::class], + Command\Api\InitialApiKeyCommand::class => [ApiKeyService::class], Command\Tag\ListTagsCommand::class => [TagService::class], Command\Tag\RenameTagCommand::class => [TagService::class], diff --git a/module/CLI/src/ApiKey/RoleResolver.php b/module/CLI/src/ApiKey/RoleResolver.php index 4df10bf4..76787451 100644 --- a/module/CLI/src/ApiKey/RoleResolver.php +++ b/module/CLI/src/ApiKey/RoleResolver.php @@ -14,28 +14,27 @@ use function is_string; class RoleResolver implements RoleResolverInterface { - public function __construct(private DomainServiceInterface $domainService, private string $defaultDomain) - { + public function __construct( + private readonly DomainServiceInterface $domainService, + private readonly string $defaultDomain, + ) { } - public function determineRoles(InputInterface $input): array + public function determineRoles(InputInterface $input): iterable { $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) { - $roleDefinitions[] = RoleDefinition::forAuthoredShortUrls(); + yield RoleDefinition::forAuthoredShortUrls(); } if (is_string($domainAuthority)) { - $roleDefinitions[] = $this->resolveRoleForAuthority($domainAuthority); + yield $this->resolveRoleForAuthority($domainAuthority); } if ($noOrphanVisits) { - $roleDefinitions[] = RoleDefinition::forNoOrphanVisits(); + yield RoleDefinition::forNoOrphanVisits(); } - - return $roleDefinitions; } private function resolveRoleForAuthority(string $domainAuthority): RoleDefinition diff --git a/module/CLI/src/ApiKey/RoleResolverInterface.php b/module/CLI/src/ApiKey/RoleResolverInterface.php index 92a04594..e849ad13 100644 --- a/module/CLI/src/ApiKey/RoleResolverInterface.php +++ b/module/CLI/src/ApiKey/RoleResolverInterface.php @@ -10,7 +10,7 @@ use Symfony\Component\Console\Input\InputInterface; interface RoleResolverInterface { /** - * @return RoleDefinition[] + * @return iterable */ - public function determineRoles(InputInterface $input): array; + public function determineRoles(InputInterface $input): iterable; } diff --git a/module/CLI/src/Command/Api/GenerateKeyCommand.php b/module/CLI/src/Command/Api/GenerateKeyCommand.php index ab4b5d54..1fe2f996 100644 --- a/module/CLI/src/Command/Api/GenerateKeyCommand.php +++ b/module/CLI/src/Command/Api/GenerateKeyCommand.php @@ -8,6 +8,7 @@ use Cake\Chronos\Chronos; use Shlinkio\Shlink\CLI\ApiKey\RoleResolverInterface; use Shlinkio\Shlink\CLI\Util\ExitCode; use Shlinkio\Shlink\CLI\Util\ShlinkTable; +use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Role; use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface; @@ -60,7 +61,7 @@ class GenerateKeyCommand extends Command $this ->setName(self::NAME) - ->setDescription('Generates a new valid API key.') + ->setDescription('Generate a new valid API key.') ->addOption( 'name', 'm', @@ -100,11 +101,12 @@ class GenerateKeyCommand extends Command protected function execute(InputInterface $input, OutputInterface $output): ?int { $expirationDate = $input->getOption('expiration-date'); - $apiKey = $this->apiKeyService->create( - isset($expirationDate) ? Chronos::parse($expirationDate) : null, - $input->getOption('name'), - ...$this->roleResolver->determineRoles($input), - ); + + $apiKey = $this->apiKeyService->create(ApiKeyMeta::fromParams( + name: $input->getOption('name'), + expirationDate: isset($expirationDate) ? Chronos::parse($expirationDate) : null, + roleDefinitions: $this->roleResolver->determineRoles($input), + )); $io = new SymfonyStyle($input, $output); $io->success(sprintf('Generated API key: "%s"', $apiKey->toString())); diff --git a/module/CLI/src/Command/Api/InitialApiKeyCommand.php b/module/CLI/src/Command/Api/InitialApiKeyCommand.php new file mode 100644 index 00000000..1f5a1794 --- /dev/null +++ b/module/CLI/src/Command/Api/InitialApiKeyCommand.php @@ -0,0 +1,43 @@ +setHidden() + ->setName(self::NAME) + ->setDescription('Tries to create initial API key') + ->addArgument('apiKey', InputArgument::REQUIRED, 'The initial API to create'); + } + + protected function execute(InputInterface $input, OutputInterface $output): ?int + { + $key = $input->getArgument('apiKey'); + $result = $this->apiKeyService->createInitial($key); + + if ($result === null && $output->isVerbose()) { + $output->writeln('Other API keys already exist. Initial API key creation skipped.'); + } + + return ExitCode::EXIT_SUCCESS; + } +} diff --git a/module/CLI/test-cli/Command/InitialApiKeyTest.php b/module/CLI/test-cli/Command/InitialApiKeyTest.php new file mode 100644 index 00000000..4f4f1922 --- /dev/null +++ b/module/CLI/test-cli/Command/InitialApiKeyTest.php @@ -0,0 +1,26 @@ +exec([InitialApiKeyCommand::NAME, 'new_api_key', '-v']); + + self::assertEquals( + <<willReturn(self::domainWithId(Domain::withAuthority('example.com'))); - $result = $this->resolver->determineRoles($input); + $result = [...$this->resolver->determineRoles($input)]; self::assertEquals($expectedRoles, $result); } @@ -111,7 +111,7 @@ class RoleResolverTest extends TestCase $this->expectException(InvalidRoleConfigException::class); - $this->resolver->determineRoles($input); + [...$this->resolver->determineRoles($input)]; } private static function domainWithId(Domain $domain): Domain diff --git a/module/CLI/test/Command/Api/GenerateKeyCommandTest.php b/module/CLI/test/Command/Api/GenerateKeyCommandTest.php index 5935242d..a15ad667 100644 --- a/module/CLI/test/Command/Api/GenerateKeyCommandTest.php +++ b/module/CLI/test/Command/Api/GenerateKeyCommandTest.php @@ -10,6 +10,7 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\CLI\ApiKey\RoleResolverInterface; use Shlinkio\Shlink\CLI\Command\Api\GenerateKeyCommand; +use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface; use ShlinkioTest\Shlink\CLI\Util\CliTestUtils; @@ -35,8 +36,7 @@ class GenerateKeyCommandTest extends TestCase public function noExpirationDateIsDefinedIfNotProvided(): void { $this->apiKeyService->expects($this->once())->method('create')->with( - $this->isNull(), - $this->isNull(), + $this->callback(fn (ApiKeyMeta $meta) => $meta->name === null && $meta->expirationDate === null), )->willReturn(ApiKey::create()); $this->commandTester->execute([]); @@ -49,8 +49,7 @@ class GenerateKeyCommandTest extends TestCase public function expirationDateIsDefinedIfProvided(): void { $this->apiKeyService->expects($this->once())->method('create')->with( - $this->isInstanceOf(Chronos::class), - $this->isNull(), + $this->callback(fn (ApiKeyMeta $meta) => $meta->expirationDate instanceof Chronos), )->willReturn(ApiKey::create()); $this->commandTester->execute([ @@ -62,8 +61,7 @@ class GenerateKeyCommandTest extends TestCase public function nameIsDefinedIfProvided(): void { $this->apiKeyService->expects($this->once())->method('create')->with( - $this->isNull(), - $this->isType('string'), + $this->callback(fn (ApiKeyMeta $meta) => $meta->name === 'Alice'), )->willReturn(ApiKey::create()); $this->commandTester->execute([ diff --git a/module/CLI/test/Command/Api/InitialApiKeyCommandTest.php b/module/CLI/test/Command/Api/InitialApiKeyCommandTest.php new file mode 100644 index 00000000..482bd36f --- /dev/null +++ b/module/CLI/test/Command/Api/InitialApiKeyCommandTest.php @@ -0,0 +1,54 @@ +apiKeyService = $this->createMock(ApiKeyServiceInterface::class); + $this->commandTester = CliTestUtils::testerForCommand(new InitialApiKeyCommand($this->apiKeyService)); + } + + #[Test, DataProvider('provideParams')] + public function initialKeyIsCreatedWithProvidedValue(?ApiKey $result, bool $verbose, string $expectedOutput): void + { + $this->apiKeyService->expects($this->once())->method('createInitial')->with('the_key')->willReturn($result); + + $this->commandTester->execute( + ['apiKey' => 'the_key'], + ['verbosity' => $verbose ? OutputInterface::VERBOSITY_VERBOSE : OutputInterface::VERBOSITY_NORMAL], + ); + $output = $this->commandTester->getDisplay(); + + self::assertEquals($expectedOutput, $output); + } + + public static function provideParams(): iterable + { + yield 'api key created, no verbose' => [ApiKey::create(), false, '']; + yield 'api key created, verbose' => [ApiKey::create(), true, '']; + yield 'no api key created, no verbose' => [null, false, '']; + yield 'no api key created, verbose' => [null, true, << [ [ $apiKey1 = ApiKey::create()->disable(), - $apiKey2 = ApiKey::fromMeta(ApiKeyMeta::withExpirationDate($dateInThePast)), + $apiKey2 = ApiKey::fromMeta(ApiKeyMeta::fromParams(expirationDate: $dateInThePast)), $apiKey3 = ApiKey::create(), ], false, @@ -115,9 +115,9 @@ class ListKeysCommandTest extends TestCase ]; yield 'with names' => [ [ - $apiKey1 = ApiKey::fromMeta(ApiKeyMeta::withName('Alice')), - $apiKey2 = ApiKey::fromMeta(ApiKeyMeta::withName('Alice and Bob')), - $apiKey3 = ApiKey::fromMeta(ApiKeyMeta::withName('')), + $apiKey1 = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'Alice')), + $apiKey2 = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'Alice and Bob')), + $apiKey3 = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: '')), $apiKey4 = ApiKey::create(), ], true, diff --git a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php index 2c6a0979..6859ec13 100644 --- a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php @@ -136,7 +136,7 @@ class ListShortUrlsCommandTest extends TestCase public static function provideOptionalFlags(): iterable { - $apiKey = ApiKey::fromMeta(ApiKeyMeta::withName('my api key')); + $apiKey = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'my api key')); $key = $apiKey->toString(); yield 'tags only' => [ diff --git a/module/Core/src/Config/EnvVars.php b/module/Core/src/Config/EnvVars.php index 44919415..ec7d384c 100644 --- a/module/Core/src/Config/EnvVars.php +++ b/module/Core/src/Config/EnvVars.php @@ -47,7 +47,6 @@ enum EnvVars: string case PORT = 'PORT'; case TASK_WORKER_NUM = 'TASK_WORKER_NUM'; case WEB_WORKER_NUM = 'WEB_WORKER_NUM'; - case INITIAL_API_KEY = 'INITIAL_API_KEY'; case ANONYMIZE_REMOTE_ADDR = 'ANONYMIZE_REMOTE_ADDR'; case TRACK_ORPHAN_VISITS = 'TRACK_ORPHAN_VISITS'; case DISABLE_TRACK_PARAM = 'DISABLE_TRACK_PARAM'; diff --git a/module/Rest/config/initial-api-key.config.php b/module/Rest/config/initial-api-key.config.php deleted file mode 100644 index a44f877f..00000000 --- a/module/Rest/config/initial-api-key.config.php +++ /dev/null @@ -1,26 +0,0 @@ - PHP_SAPI !== 'cli' ? null : EnvVars::INITIAL_API_KEY->loadFromEnv(), - - 'dependencies' => [ - 'delegators' => [ - Application::class => [ - ApiKey\InitialApiKeyDelegator::class, - ], - ], - ], - -]; diff --git a/module/Rest/src/ApiKey/InitialApiKeyDelegator.php b/module/Rest/src/ApiKey/InitialApiKeyDelegator.php deleted file mode 100644 index a5aa9d33..00000000 --- a/module/Rest/src/ApiKey/InitialApiKeyDelegator.php +++ /dev/null @@ -1,31 +0,0 @@ -get('config')['initial_api_key'] ?? null; - if (! empty($initialApiKey)) { - $this->createInitialApiKey($initialApiKey, $container); - } - - return $callback(); - } - - private function createInitialApiKey(string $initialApiKey, ContainerInterface $container): void - { - /** @var ApiKeyRepositoryInterface $repo */ - $repo = $container->get(EntityManager::class)->getRepository(ApiKey::class); - $repo->createInitialApiKey($initialApiKey); - } -} diff --git a/module/Rest/src/ApiKey/Model/ApiKeyMeta.php b/module/Rest/src/ApiKey/Model/ApiKeyMeta.php index 430221a2..e28a9ec3 100644 --- a/module/Rest/src/ApiKey/Model/ApiKeyMeta.php +++ b/module/Rest/src/ApiKey/Model/ApiKeyMeta.php @@ -5,36 +5,45 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\ApiKey\Model; use Cake\Chronos\Chronos; +use Ramsey\Uuid\Uuid; final class ApiKeyMeta { /** - * @param RoleDefinition[] $roleDefinitions + * @param iterable $roleDefinitions */ private function __construct( + public readonly string $key, public readonly ?string $name, public readonly ?Chronos $expirationDate, - public readonly array $roleDefinitions, + public readonly iterable $roleDefinitions, ) { } - public static function withName(string $name): self + public static function empty(): self { - return new self($name, null, []); + return self::fromParams(); } - public static function withExpirationDate(Chronos $expirationDate): self - { - return new self(null, $expirationDate, []); - } - - public static function withNameAndExpirationDate(string $name, Chronos $expirationDate): self - { - return new self($name, $expirationDate, []); + /** + * @param iterable $roleDefinitions + */ + public static function fromParams( + ?string $key = null, + ?string $name = null, + ?Chronos $expirationDate = null, + iterable $roleDefinitions = [], + ): self { + return new self( + key: $key ?? Uuid::uuid4()->toString(), + name: $name, + expirationDate: $expirationDate, + roleDefinitions: $roleDefinitions, + ); } public static function withRoles(RoleDefinition ...$roleDefinitions): self { - return new self(null, null, $roleDefinitions); + return self::fromParams(roleDefinitions: $roleDefinitions); } } diff --git a/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php b/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php index ec49145e..ad09b22d 100644 --- a/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php +++ b/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php @@ -6,14 +6,18 @@ namespace Shlinkio\Shlink\Rest\ApiKey\Repository; use Doctrine\DBAL\LockMode; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; +use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\Entity\ApiKey; class ApiKeyRepository extends EntitySpecificationRepository implements ApiKeyRepositoryInterface { - public function createInitialApiKey(string $apiKey): void + /** + * Will create provided API key with admin permissions, only if there's no other API keys yet + */ + public function createInitialApiKey(string $apiKey): ?ApiKey { $em = $this->getEntityManager(); - $em->wrapInTransaction(function () use ($apiKey, $em): void { + return $em->wrapInTransaction(function () use ($apiKey, $em): ?ApiKey { // Ideally this would be a SELECT COUNT(...), but MsSQL and Postgres do not allow locking on aggregates // Because of that we check if at least one result exists $firstResult = $em->createQueryBuilder()->select('a.id') @@ -23,10 +27,16 @@ class ApiKeyRepository extends EntitySpecificationRepository implements ApiKeyRe ->setLockMode(LockMode::PESSIMISTIC_WRITE) ->getOneOrNullResult(); - if ($firstResult === null) { - $em->persist(ApiKey::fromKey($apiKey)); - $em->flush(); + // Do not create an initial API key if other keys already exist + if ($firstResult !== null) { + return null; } + + $new = ApiKey::fromMeta(ApiKeyMeta::fromParams(key: $apiKey)); + $em->persist($new); + $em->flush(); + + return $new; }); } } diff --git a/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php b/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php index f5beb3e9..a557e603 100644 --- a/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php +++ b/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php @@ -6,11 +6,12 @@ namespace Shlinkio\Shlink\Rest\ApiKey\Repository; use Doctrine\Persistence\ObjectRepository; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepositoryInterface; +use Shlinkio\Shlink\Rest\Entity\ApiKey; interface ApiKeyRepositoryInterface extends ObjectRepository, EntitySpecificationRepositoryInterface { /** * Will create provided API key only if there's no API keys yet */ - public function createInitialApiKey(string $apiKey): void; + public function createInitialApiKey(string $apiKey): ?ApiKey; } diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index bb2372f8..dae30de0 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -10,7 +10,6 @@ use Doctrine\Common\Collections\Collection; use Exception; use Happyr\DoctrineSpecification\Spec; use Happyr\DoctrineSpecification\Specification\Specification; -use Ramsey\Uuid\Uuid; use Shlinkio\Shlink\Common\Entity\AbstractEntity; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; @@ -28,21 +27,27 @@ class ApiKey extends AbstractEntity /** * @throws Exception */ - private function __construct(?string $key = null) + private function __construct(string $key) { - $this->key = $key ?? Uuid::uuid4()->toString(); + $this->key = $key; $this->enabled = true; $this->roles = new ArrayCollection(); } + /** + * @throws Exception + */ public static function create(): ApiKey { - return new self(); + return self::fromMeta(ApiKeyMeta::empty()); } + /** + * @throws Exception + */ public static function fromMeta(ApiKeyMeta $meta): self { - $apiKey = self::create(); + $apiKey = new self($meta->key); $apiKey->name = $meta->name; $apiKey->expirationDate = $meta->expirationDate; @@ -53,11 +58,6 @@ class ApiKey extends AbstractEntity return $apiKey; } - public static function fromKey(string $key): self - { - return new self($key); - } - public function getExpirationDate(): ?Chronos { return $this->expirationDate; diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php index 7d7e0710..21f69f90 100644 --- a/module/Rest/src/Service/ApiKeyService.php +++ b/module/Rest/src/Service/ApiKeyService.php @@ -4,47 +4,35 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Service; -use Cake\Chronos\Chronos; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; -use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; +use Shlinkio\Shlink\Rest\ApiKey\Repository\ApiKeyRepositoryInterface; use Shlinkio\Shlink\Rest\Entity\ApiKey; use function sprintf; class ApiKeyService implements ApiKeyServiceInterface { - public function __construct(private EntityManagerInterface $em) + public function __construct(private readonly EntityManagerInterface $em) { } - public function create( - ?Chronos $expirationDate = null, - ?string $name = null, - RoleDefinition ...$roleDefinitions, - ): ApiKey { - $key = $this->buildApiKeyWithParams($expirationDate, $name); - foreach ($roleDefinitions as $definition) { - $key->registerRole($definition); - } + public function create(ApiKeyMeta $apiKeyMeta): ApiKey + { + $apiKey = ApiKey::fromMeta($apiKeyMeta); - $this->em->persist($key); + $this->em->persist($apiKey); $this->em->flush(); - return $key; + return $apiKey; } - private function buildApiKeyWithParams(?Chronos $expirationDate, ?string $name): ApiKey + public function createInitial(string $key): ?ApiKey { - return match (true) { - $expirationDate !== null && $name !== null => ApiKey::fromMeta( - ApiKeyMeta::withNameAndExpirationDate($name, $expirationDate), - ), - $expirationDate !== null => ApiKey::fromMeta(ApiKeyMeta::withExpirationDate($expirationDate)), - $name !== null => ApiKey::fromMeta(ApiKeyMeta::withName($name)), - default => ApiKey::create(), - }; + /** @var ApiKeyRepositoryInterface $repo */ + $repo = $this->em->getRepository(ApiKey::class); + return $repo->createInitialApiKey($key); } public function check(string $key): ApiKeyCheckResult diff --git a/module/Rest/src/Service/ApiKeyServiceInterface.php b/module/Rest/src/Service/ApiKeyServiceInterface.php index 85b726df..b82d7760 100644 --- a/module/Rest/src/Service/ApiKeyServiceInterface.php +++ b/module/Rest/src/Service/ApiKeyServiceInterface.php @@ -4,18 +4,15 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Service; -use Cake\Chronos\Chronos; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; -use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; +use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\Entity\ApiKey; interface ApiKeyServiceInterface { - public function create( - ?Chronos $expirationDate = null, - ?string $name = null, - RoleDefinition ...$roleDefinitions, - ): ApiKey; + public function create(ApiKeyMeta $apiKeyMeta): ApiKey; + + public function createInitial(string $key): ?ApiKey; public function check(string $key): ApiKeyCheckResult; diff --git a/module/Rest/test-api/Fixtures/ApiKeyFixture.php b/module/Rest/test-api/Fixtures/ApiKeyFixture.php index 86d2cf2a..949a80c3 100644 --- a/module/Rest/test-api/Fixtures/ApiKeyFixture.php +++ b/module/Rest/test-api/Fixtures/ApiKeyFixture.php @@ -51,7 +51,7 @@ class ApiKeyFixture extends AbstractFixture implements DependentFixtureInterface private function buildApiKey(string $key, bool $enabled, ?Chronos $expiresAt = null): ApiKey { - $apiKey = $expiresAt !== null ? ApiKey::fromMeta(ApiKeyMeta::withExpirationDate($expiresAt)) : ApiKey::create(); + $apiKey = ApiKey::fromMeta(ApiKeyMeta::fromParams(expirationDate: $expiresAt)); $ref = new ReflectionObject($apiKey); $keyProp = $ref->getProperty('key'); $keyProp->setAccessible(true); diff --git a/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php b/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php index 86db9176..c19d8512 100644 --- a/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php +++ b/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php @@ -22,10 +22,10 @@ class ApiKeyRepositoryTest extends DatabaseTestCase public function initialApiKeyIsCreatedOnlyOfNoApiKeysExistYet(): void { self::assertCount(0, $this->repo->findAll()); - $this->repo->createInitialApiKey('initial_value'); + self::assertNotNull($this->repo->createInitialApiKey('initial_value')); self::assertCount(1, $this->repo->findAll()); self::assertCount(1, $this->repo->findBy(['key' => 'initial_value'])); - $this->repo->createInitialApiKey('another_one'); + self::assertNull($this->repo->createInitialApiKey('another_one')); self::assertCount(1, $this->repo->findAll()); self::assertCount(0, $this->repo->findBy(['key' => 'another_one'])); } diff --git a/module/Rest/test/ApiKey/InitialApiKeyDelegatorTest.php b/module/Rest/test/ApiKey/InitialApiKeyDelegatorTest.php deleted file mode 100644 index 9b6edff6..00000000 --- a/module/Rest/test/ApiKey/InitialApiKeyDelegatorTest.php +++ /dev/null @@ -1,57 +0,0 @@ -delegator = new InitialApiKeyDelegator(); - $this->container = $this->createMock(ContainerInterface::class); - } - - #[Test, DataProvider('provideConfigs')] - public function apiKeyIsInitializedWhenAppropriate(array $config, int $expectedCalls): void - { - $app = $this->createMock(Application::class); - $apiKeyRepo = $this->createMock(ApiKeyRepositoryInterface::class); - $apiKeyRepo->expects($this->exactly($expectedCalls))->method('createInitialApiKey'); - $em = $this->createMock(EntityManagerInterface::class); - $em->expects($this->exactly($expectedCalls))->method('getRepository')->with(ApiKey::class)->willReturn( - $apiKeyRepo, - ); - $this->container->expects($this->exactly($expectedCalls + 1))->method('get')->willReturnMap([ - ['config', $config], - [EntityManager::class, $em], - ]); - - $result = ($this->delegator)($this->container, '', fn () => $app); - - self::assertSame($result, $app); - } - - public static function provideConfigs(): iterable - { - yield 'no api key' => [[], 0]; - yield 'null api key' => [['initial_api_key' => null], 0]; - yield 'empty api key' => [['initial_api_key' => ''], 0]; - yield 'valid api key' => [['initial_api_key' => 'the_initial_key'], 1]; - } -} diff --git a/module/Rest/test/ConfigProviderTest.php b/module/Rest/test/ConfigProviderTest.php index ee729eb5..72063a72 100644 --- a/module/Rest/test/ConfigProviderTest.php +++ b/module/Rest/test/ConfigProviderTest.php @@ -24,11 +24,10 @@ class ConfigProviderTest extends TestCase { $config = ($this->configProvider)(); - self::assertCount(5, $config); + self::assertCount(4, $config); self::assertArrayHasKey('dependencies', $config); self::assertArrayHasKey('auth', $config); self::assertArrayHasKey('entity_manager', $config); - self::assertArrayHasKey('initial_api_key', $config); self::assertArrayHasKey(ConfigAbstractFactory::class, $config); } diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php index 8fc0ea2c..3740bbfe 100644 --- a/module/Rest/test/Service/ApiKeyServiceTest.php +++ b/module/Rest/test/Service/ApiKeyServiceTest.php @@ -6,7 +6,6 @@ namespace ShlinkioTest\Shlink\Rest\Service; use Cake\Chronos\Chronos; use Doctrine\ORM\EntityManager; -use Doctrine\ORM\EntityRepository; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; @@ -15,6 +14,7 @@ use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; use Shlinkio\Shlink\Core\Domain\Entity\Domain; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; +use Shlinkio\Shlink\Rest\ApiKey\Repository\ApiKeyRepositoryInterface; use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\Rest\Service\ApiKeyService; @@ -22,12 +22,12 @@ class ApiKeyServiceTest extends TestCase { private ApiKeyService $service; private MockObject & EntityManager $em; - private MockObject & EntityRepository $repo; + private MockObject & ApiKeyRepositoryInterface $repo; protected function setUp(): void { $this->em = $this->createMock(EntityManager::class); - $this->repo = $this->createMock(EntityRepository::class); + $this->repo = $this->createMock(ApiKeyRepositoryInterface::class); $this->service = new ApiKeyService($this->em); } @@ -40,7 +40,9 @@ class ApiKeyServiceTest extends TestCase $this->em->expects($this->once())->method('flush'); $this->em->expects($this->once())->method('persist')->with($this->isInstanceOf(ApiKey::class)); - $key = $this->service->create($date, $name, ...$roles); + $key = $this->service->create( + ApiKeyMeta::fromParams(name: $name, expirationDate: $date, roleDefinitions: $roles), + ); self::assertEquals($date, $key->getExpirationDate()); self::assertEquals($name, $key->name()); @@ -81,7 +83,9 @@ class ApiKeyServiceTest extends TestCase { yield 'non-existent api key' => [null]; yield 'disabled api key' => [ApiKey::create()->disable()]; - yield 'expired api key' => [ApiKey::fromMeta(ApiKeyMeta::withExpirationDate(Chronos::now()->subDays(1)))]; + yield 'expired api key' => [ + ApiKey::fromMeta(ApiKeyMeta::fromParams(expirationDate: Chronos::now()->subDays(1))), + ]; } #[Test] @@ -144,8 +148,25 @@ class ApiKeyServiceTest extends TestCase $this->repo->expects($this->once())->method('findBy')->with(['enabled' => true])->willReturn($expectedApiKeys); $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo); - $result = $this->service->listKeys(true); + $result = $this->service->listKeys(enabledOnly: true); self::assertEquals($expectedApiKeys, $result); } + + #[Test, DataProvider('provideInitialApiKeys')] + public function createInitialDelegatesToRepository(?ApiKey $apiKey): void + { + $this->repo->expects($this->once())->method('createInitialApiKey')->with('the_key')->willReturn($apiKey); + $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo); + + $result = $this->service->createInitial('the_key'); + + self::assertSame($result, $apiKey); + } + + public static function provideInitialApiKeys(): iterable + { + yield 'first api key' => [ApiKey::create()]; + yield 'existing api keys' => [null]; + } }