diff --git a/CHANGELOG.md b/CHANGELOG.md index a6741b55..bd80fa59 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,9 +9,19 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#2207](https://github.com/shlinkio/shlink/issues/2207) Add `hasRedirectRules` flag to short URL API model. This flag tells if a specific short URL has any redirect rules attached to it. * [#1520](https://github.com/shlinkio/shlink/issues/1520) Allow short URLs list to be filtered by `domain`. - This change applies both to the `GET /short-urls` endpoint, via the `domain` query parameter, and the `short-url:list` console command, via the `--domain`|`-d` flag. + This change applies both to the `GET /short-urls` endpoint, via the `domain` query parameter, and the `short-url:list` console command, via the `--domain`|`-d` flag. ### Changed +* [#2193](https://github.com/shlinkio/shlink/issues/2193) API keys are now hashed using SHA256, instead of being saved in plain text. + + As a side effect, API key names have now become more important, and are considered unique. + + When people update to this Shlink version, existing API keys will be hashed for everything to continue working. + + In order to avoid data to be lost, plain-text keys will be written in the `name` field, either together with any existing name, or as the name itself. Then users are responsible for renaming them using the new `api-key:rename` command. + + For newly created API keys, it is recommended to provide a name, but if not provided, a name will be generated from a redacted version of the new API key. + * Update to Shlink PHP coding standard 2.4 * Update to `hidehalo/nanoid-php` 2.0 diff --git a/module/CLI/config/cli.config.php b/module/CLI/config/cli.config.php index 8283a7b6..a554db40 100644 --- a/module/CLI/config/cli.config.php +++ b/module/CLI/config/cli.config.php @@ -28,6 +28,7 @@ return [ 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\Api\RenameApiKeyCommand::NAME => Command\Api\RenameApiKeyCommand::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 2f098998..76e7c4f5 100644 --- a/module/CLI/config/dependencies.config.php +++ b/module/CLI/config/dependencies.config.php @@ -59,6 +59,7 @@ return [ Command\Api\DisableKeyCommand::class => ConfigAbstractFactory::class, Command\Api\ListKeysCommand::class => ConfigAbstractFactory::class, Command\Api\InitialApiKeyCommand::class => ConfigAbstractFactory::class, + Command\Api\RenameApiKeyCommand::class => ConfigAbstractFactory::class, Command\Tag\ListTagsCommand::class => ConfigAbstractFactory::class, Command\Tag\RenameTagCommand::class => ConfigAbstractFactory::class, @@ -120,6 +121,7 @@ return [ Command\Api\DisableKeyCommand::class => [ApiKeyService::class], Command\Api\ListKeysCommand::class => [ApiKeyService::class], Command\Api\InitialApiKeyCommand::class => [ApiKeyService::class], + Command\Api\RenameApiKeyCommand::class => [ApiKeyService::class], Command\Tag\ListTagsCommand::class => [TagService::class], Command\Tag\RenameTagCommand::class => [TagService::class], diff --git a/module/CLI/src/Command/Api/DisableKeyCommand.php b/module/CLI/src/Command/Api/DisableKeyCommand.php index 3da85e9e..c2ed4173 100644 --- a/module/CLI/src/Command/Api/DisableKeyCommand.php +++ b/module/CLI/src/Command/Api/DisableKeyCommand.php @@ -6,39 +6,99 @@ namespace Shlinkio\Shlink\CLI\Command\Api; use Shlinkio\Shlink\CLI\Util\ExitCode; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; +use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; +use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; use Symfony\Component\Console\Style\SymfonyStyle; +use function Shlinkio\Shlink\Core\ArrayUtils\map; use function sprintf; class DisableKeyCommand extends Command { public const NAME = 'api-key:disable'; - public function __construct(private ApiKeyServiceInterface $apiKeyService) + public function __construct(private readonly ApiKeyServiceInterface $apiKeyService) { parent::__construct(); } protected function configure(): void { - $this->setName(self::NAME) - ->setDescription('Disables an API key.') - ->addArgument('apiKey', InputArgument::REQUIRED, 'The API key to disable'); + $help = <<%command.name% command allows you to disable an existing API key, via its name or the + plain-text key. + + If no arguments are provided, you will be prompted to select one of the existing non-disabled API keys. + + %command.full_name% + + You can optionally pass the API key name to be disabled. In that case --by-name is also + required, to indicate the first argument is the API key name and not the plain-text key: + + %command.full_name% the_key_name --by-name + + You can pass the plain-text key to be disabled, but that is DEPRECATED. In next major version, + the argument will always be assumed to be the name: + + %command.full_name% d6b6c60e-edcd-4e43-96ad-fa6b7014c143 + + HELP; + + $this + ->setName(self::NAME) + ->setDescription('Disables an API key by name or plain-text key (providing a plain-text key is DEPRECATED)') + ->addArgument( + 'keyOrName', + InputArgument::OPTIONAL, + 'The API key to disable. Pass `--by-name` to indicate this value is the name and not the key.', + ) + ->addOption( + 'by-name', + mode: InputOption::VALUE_NONE, + description: 'Indicates the first argument is the API key name, not the plain-text key.', + ) + ->setHelp($help); + } + + protected function interact(InputInterface $input, OutputInterface $output): void + { + $keyOrName = $input->getArgument('keyOrName'); + + if ($keyOrName === null) { + $apiKeys = $this->apiKeyService->listKeys(enabledOnly: true); + $name = (new SymfonyStyle($input, $output))->choice( + 'What API key do you want to disable?', + map($apiKeys, static fn (ApiKey $apiKey) => $apiKey->name), + ); + + $input->setArgument('keyOrName', $name); + $input->setOption('by-name', true); + } } protected function execute(InputInterface $input, OutputInterface $output): int { - $apiKey = $input->getArgument('apiKey'); + $keyOrName = $input->getArgument('keyOrName'); + $byName = $input->getOption('by-name'); $io = new SymfonyStyle($input, $output); + if (! $keyOrName) { + $io->warning('An API key name was not provided.'); + return ExitCode::EXIT_WARNING; + } + try { - $this->apiKeyService->disable($apiKey); - $io->success(sprintf('API key "%s" properly disabled', $apiKey)); + if ($byName) { + $this->apiKeyService->disableByName($keyOrName); + } else { + $this->apiKeyService->disableByKey($keyOrName); + } + $io->success(sprintf('API key "%s" properly disabled', $keyOrName)); return ExitCode::EXIT_SUCCESS; } catch (InvalidArgumentException $e) { $io->error($e->getMessage()); diff --git a/module/CLI/src/Command/Api/GenerateKeyCommand.php b/module/CLI/src/Command/Api/GenerateKeyCommand.php index a7656189..9fc0bb1d 100644 --- a/module/CLI/src/Command/Api/GenerateKeyCommand.php +++ b/module/CLI/src/Command/Api/GenerateKeyCommand.php @@ -100,23 +100,26 @@ class GenerateKeyCommand extends Command protected function execute(InputInterface $input, OutputInterface $output): int { + $io = new SymfonyStyle($input, $output); $expirationDate = $input->getOption('expiration-date'); - - $apiKey = $this->apiKeyService->create(ApiKeyMeta::fromParams( + $apiKeyMeta = 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->key)); + $apiKey = $this->apiKeyService->create($apiKeyMeta); + $io->success(sprintf('Generated API key: "%s"', $apiKeyMeta->key)); + + if ($input->isInteractive()) { + $io->warning('Save the key in a secure location. You will not be able to get it afterwards.'); + } if (! ApiKey::isAdmin($apiKey)) { ShlinkTable::default($io)->render( ['Role name', 'Role metadata'], - $apiKey->mapRoles(fn (Role $role, array $meta) => [$role->value, arrayToString($meta, 0)]), - null, - 'Roles', + $apiKey->mapRoles(fn (Role $role, array $meta) => [$role->value, arrayToString($meta, indentSize: 0)]), + headerTitle: 'Roles', ); } diff --git a/module/CLI/src/Command/Api/ListKeysCommand.php b/module/CLI/src/Command/Api/ListKeysCommand.php index ab10ebc6..d341389d 100644 --- a/module/CLI/src/Command/Api/ListKeysCommand.php +++ b/module/CLI/src/Command/Api/ListKeysCommand.php @@ -54,7 +54,7 @@ class ListKeysCommand extends Command $messagePattern = $this->determineMessagePattern($apiKey); // Set columns for this row - $rowData = [sprintf($messagePattern, $apiKey->key), sprintf($messagePattern, $apiKey->name ?? '-')]; + $rowData = [sprintf($messagePattern, $apiKey->name ?? '-')]; if (! $enabledOnly) { $rowData[] = sprintf($messagePattern, $this->getEnabledSymbol($apiKey)); } @@ -67,7 +67,6 @@ class ListKeysCommand extends Command }, $this->apiKeyService->listKeys($enabledOnly)); ShlinkTable::withRowSeparators($output)->render(array_filter([ - 'Key', 'Name', ! $enabledOnly ? 'Is enabled' : null, 'Expiration date', diff --git a/module/CLI/src/Command/Api/RenameApiKeyCommand.php b/module/CLI/src/Command/Api/RenameApiKeyCommand.php new file mode 100644 index 00000000..f7e24992 --- /dev/null +++ b/module/CLI/src/Command/Api/RenameApiKeyCommand.php @@ -0,0 +1,77 @@ +setName(self::NAME) + ->setDescription('Renames an API key by name') + ->addArgument('oldName', InputArgument::REQUIRED, 'Current name of the API key to rename') + ->addArgument('newName', InputArgument::REQUIRED, 'New name to set to the API key'); + } + + protected function interact(InputInterface $input, OutputInterface $output): void + { + $io = new SymfonyStyle($input, $output); + $oldName = $input->getArgument('oldName'); + $newName = $input->getArgument('newName'); + + if ($oldName === null) { + $apiKeys = $this->apiKeyService->listKeys(); + $requestedOldName = $io->choice( + 'What API key do you want to rename?', + map($apiKeys, static fn (ApiKey $apiKey) => $apiKey->name), + ); + + $input->setArgument('oldName', $requestedOldName); + } + + if ($newName === null) { + $requestedNewName = $io->ask( + 'What is the new name you want to set?', + validator: static fn (string|null $value): string => $value !== null + ? $value + : throw new InvalidArgumentException('The new name cannot be empty'), + ); + + $input->setArgument('newName', $requestedNewName); + } + } + + protected function execute(InputInterface $input, OutputInterface $output): int + { + $io = new SymfonyStyle($input, $output); + $oldName = $input->getArgument('oldName'); + $newName = $input->getArgument('newName'); + + $this->apiKeyService->renameApiKey(Renaming::fromNames($oldName, $newName)); + $io->success('API key properly renamed'); + + return ExitCode::EXIT_SUCCESS; + } +} diff --git a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php index 900402e1..fadc78e2 100644 --- a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php +++ b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php @@ -118,14 +118,9 @@ class ListShortUrlsCommand extends Command 'show-api-key', 'k', InputOption::VALUE_NONE, - 'Whether to display the API key from which the URL was generated or not.', - ) - ->addOption( - 'show-api-key-name', - 'm', - InputOption::VALUE_NONE, 'Whether to display the API key name from which the URL was generated or not.', ) + ->addOption('show-api-key-name', 'm', InputOption::VALUE_NONE, '[DEPRECATED] Use show-api-key') ->addOption( 'all', 'a', @@ -242,11 +237,7 @@ class ListShortUrlsCommand extends Command $columnsMap['Domain'] = static fn (array $_, ShortUrl $shortUrl): string => $shortUrl->getDomain()?->authority ?? Domain::DEFAULT_AUTHORITY; } - if ($input->getOption('show-api-key')) { - $columnsMap['API Key'] = static fn (array $_, ShortUrl $shortUrl): string => - $shortUrl->authorApiKey?->key ?? ''; - } - if ($input->getOption('show-api-key-name')) { + if ($input->getOption('show-api-key') || $input->getOption('show-api-key-name')) { $columnsMap['API Key Name'] = static fn (array $_, ShortUrl $shortUrl): string|null => $shortUrl->authorApiKey?->name; } diff --git a/module/CLI/src/Command/Tag/RenameTagCommand.php b/module/CLI/src/Command/Tag/RenameTagCommand.php index fdc0f0ce..5830858e 100644 --- a/module/CLI/src/Command/Tag/RenameTagCommand.php +++ b/module/CLI/src/Command/Tag/RenameTagCommand.php @@ -7,7 +7,7 @@ namespace Shlinkio\Shlink\CLI\Command\Tag; use Shlinkio\Shlink\CLI\Util\ExitCode; use Shlinkio\Shlink\Core\Exception\TagConflictException; use Shlinkio\Shlink\Core\Exception\TagNotFoundException; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Core\Tag\TagServiceInterface; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; @@ -40,7 +40,7 @@ class RenameTagCommand extends Command $newName = $input->getArgument('newName'); try { - $this->tagService->renameTag(TagRenaming::fromNames($oldName, $newName)); + $this->tagService->renameTag(Renaming::fromNames($oldName, $newName)); $io->success('Tag properly renamed.'); return ExitCode::EXIT_SUCCESS; } catch (TagNotFoundException | TagConflictException $e) { diff --git a/module/CLI/test-cli/Command/ListApiKeysTest.php b/module/CLI/test-cli/Command/ListApiKeysTest.php index 46e3c135..9e0ce90d 100644 --- a/module/CLI/test-cli/Command/ListApiKeysTest.php +++ b/module/CLI/test-cli/Command/ListApiKeysTest.php @@ -26,38 +26,38 @@ class ListApiKeysTest extends CliTestCase { $expiredApiKeyDate = Chronos::now()->subDays(1)->startOfDay()->toAtomString(); $enabledOnlyOutput = << [[], << [['-e'], $enabledOnlyOutput]; diff --git a/module/CLI/test/Command/Api/DisableKeyCommandTest.php b/module/CLI/test/Command/Api/DisableKeyCommandTest.php index a12cb46f..a617539d 100644 --- a/module/CLI/test/Command/Api/DisableKeyCommandTest.php +++ b/module/CLI/test/Command/Api/DisableKeyCommandTest.php @@ -8,7 +8,10 @@ use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\CLI\Command\Api\DisableKeyCommand; +use Shlinkio\Shlink\CLI\Util\ExitCode; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; +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; use Symfony\Component\Console\Tester\CommandTester; @@ -28,30 +31,103 @@ class DisableKeyCommandTest extends TestCase public function providedApiKeyIsDisabled(): void { $apiKey = 'abcd1234'; - $this->apiKeyService->expects($this->once())->method('disable')->with($apiKey); + $this->apiKeyService->expects($this->once())->method('disableByKey')->with($apiKey); + $this->apiKeyService->expects($this->never())->method('disableByName'); - $this->commandTester->execute([ - 'apiKey' => $apiKey, + $exitCode = $this->commandTester->execute([ + 'keyOrName' => $apiKey, ]); $output = $this->commandTester->getDisplay(); self::assertStringContainsString('API key "abcd1234" properly disabled', $output); + self::assertEquals(ExitCode::EXIT_SUCCESS, $exitCode); } #[Test] - public function errorIsReturnedIfServiceThrowsException(): void + public function providedApiKeyIsDisabledByName(): void + { + $name = 'the key to delete'; + $this->apiKeyService->expects($this->once())->method('disableByName')->with($name); + $this->apiKeyService->expects($this->never())->method('disableByKey'); + + $exitCode = $this->commandTester->execute([ + 'keyOrName' => $name, + '--by-name' => true, + ]); + $output = $this->commandTester->getDisplay(); + + self::assertStringContainsString('API key "the key to delete" properly disabled', $output); + self::assertEquals(ExitCode::EXIT_SUCCESS, $exitCode); + } + + #[Test] + public function errorIsReturnedIfDisableByKeyThrowsException(): void { $apiKey = 'abcd1234'; $expectedMessage = 'API key "abcd1234" does not exist.'; - $this->apiKeyService->expects($this->once())->method('disable')->with($apiKey)->willThrowException( + $this->apiKeyService->expects($this->once())->method('disableByKey')->with($apiKey)->willThrowException( new InvalidArgumentException($expectedMessage), ); + $this->apiKeyService->expects($this->never())->method('disableByName'); - $this->commandTester->execute([ - 'apiKey' => $apiKey, + $exitCode = $this->commandTester->execute([ + 'keyOrName' => $apiKey, ]); $output = $this->commandTester->getDisplay(); self::assertStringContainsString($expectedMessage, $output); + self::assertEquals(ExitCode::EXIT_FAILURE, $exitCode); + } + + #[Test] + public function errorIsReturnedIfDisableByNameThrowsException(): void + { + $name = 'the key to delete'; + $expectedMessage = 'API key "the key to delete" does not exist.'; + $this->apiKeyService->expects($this->once())->method('disableByName')->with($name)->willThrowException( + new InvalidArgumentException($expectedMessage), + ); + $this->apiKeyService->expects($this->never())->method('disableByKey'); + + $exitCode = $this->commandTester->execute([ + 'keyOrName' => $name, + '--by-name' => true, + ]); + $output = $this->commandTester->getDisplay(); + + self::assertStringContainsString($expectedMessage, $output); + self::assertEquals(ExitCode::EXIT_FAILURE, $exitCode); + } + + #[Test] + public function warningIsReturnedIfNoArgumentIsProvidedInNonInteractiveMode(): void + { + $this->apiKeyService->expects($this->never())->method('disableByName'); + $this->apiKeyService->expects($this->never())->method('disableByKey'); + $this->apiKeyService->expects($this->never())->method('listKeys'); + + $exitCode = $this->commandTester->execute([], ['interactive' => false]); + + self::assertEquals(ExitCode::EXIT_WARNING, $exitCode); + } + + #[Test] + public function existingApiKeyNamesAreListedIfNoArgumentIsProvidedInInteractiveMode(): void + { + $name = 'the key to delete'; + $this->apiKeyService->expects($this->once())->method('disableByName')->with($name); + $this->apiKeyService->expects($this->once())->method('listKeys')->willReturn([ + ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'foo')), + ApiKey::fromMeta(ApiKeyMeta::fromParams(name: $name)), + ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'bar')), + ]); + $this->apiKeyService->expects($this->never())->method('disableByKey'); + + $this->commandTester->setInputs([$name]); + $exitCode = $this->commandTester->execute([]); + $output = $this->commandTester->getDisplay(); + + self::assertStringContainsString('API key "the key to delete" properly disabled', $output); + self::assertEquals(ExitCode::EXIT_SUCCESS, $exitCode); } } diff --git a/module/CLI/test/Command/Api/GenerateKeyCommandTest.php b/module/CLI/test/Command/Api/GenerateKeyCommandTest.php index a15ad667..1eb977bf 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\CLI\Util\ExitCode; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface; @@ -36,7 +37,7 @@ class GenerateKeyCommandTest extends TestCase public function noExpirationDateIsDefinedIfNotProvided(): void { $this->apiKeyService->expects($this->once())->method('create')->with( - $this->callback(fn (ApiKeyMeta $meta) => $meta->name === null && $meta->expirationDate === null), + $this->callback(fn (ApiKeyMeta $meta) => $meta->expirationDate === null), )->willReturn(ApiKey::create()); $this->commandTester->execute([]); @@ -64,8 +65,10 @@ class GenerateKeyCommandTest extends TestCase $this->callback(fn (ApiKeyMeta $meta) => $meta->name === 'Alice'), )->willReturn(ApiKey::create()); - $this->commandTester->execute([ + $exitCode = $this->commandTester->execute([ '--name' => 'Alice', ]); + + self::assertEquals(ExitCode::EXIT_SUCCESS, $exitCode); } } diff --git a/module/CLI/test/Command/Api/ListKeysCommandTest.php b/module/CLI/test/Command/Api/ListKeysCommandTest.php index 6f4b816b..54ae4c3e 100644 --- a/module/CLI/test/Command/Api/ListKeysCommandTest.php +++ b/module/CLI/test/Command/Api/ListKeysCommandTest.php @@ -52,15 +52,15 @@ class ListKeysCommandTest extends TestCase ], false, <<key} | - | --- | - | Admin | - +--------------------------------------+------+------------+---------------------------+-------+ - | {$apiKey2->key} | - | --- | 2020-01-01T00:00:00+00:00 | Admin | - +--------------------------------------+------+------------+---------------------------+-------+ - | {$apiKey3->key} | - | +++ | - | Admin | - +--------------------------------------+------+------------+---------------------------+-------+ + +--------------------------------------+------------+---------------------------+-------+ + | Name | Is enabled | Expiration date | Roles | + +--------------------------------------+------------+---------------------------+-------+ + | {$apiKey1->name} | --- | - | Admin | + +--------------------------------------+------------+---------------------------+-------+ + | {$apiKey2->name} | --- | 2020-01-01T00:00:00+00:00 | Admin | + +--------------------------------------+------------+---------------------------+-------+ + | {$apiKey3->name} | +++ | - | Admin | + +--------------------------------------+------------+---------------------------+-------+ OUTPUT, ]; @@ -68,13 +68,13 @@ class ListKeysCommandTest extends TestCase [$apiKey1 = ApiKey::create()->disable(), $apiKey2 = ApiKey::create()], true, <<key} | - | - | Admin | - +--------------------------------------+------+-----------------+-------+ - | {$apiKey2->key} | - | - | Admin | - +--------------------------------------+------+-----------------+-------+ + +--------------------------------------+-----------------+-------+ + | Name | Expiration date | Roles | + +--------------------------------------+-----------------+-------+ + | {$apiKey1->name} | - | Admin | + +--------------------------------------+-----------------+-------+ + | {$apiKey2->name} | - | Admin | + +--------------------------------------+-----------------+-------+ OUTPUT, ]; @@ -94,45 +94,45 @@ class ListKeysCommandTest extends TestCase ], true, <<key} | - | - | Admin | - +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey2->key} | - | - | Author only | - +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey3->key} | - | - | Domain only: example.com | - +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey4->key} | - | - | Admin | - +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey5->key} | - | - | Author only | - | | | | Domain only: example.com | - +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey6->key} | - | - | Admin | - +--------------------------------------+------+-----------------+--------------------------+ + +--------------------------------------+-----------------+--------------------------+ + | Name | Expiration date | Roles | + +--------------------------------------+-----------------+--------------------------+ + | {$apiKey1->name} | - | Admin | + +--------------------------------------+-----------------+--------------------------+ + | {$apiKey2->name} | - | Author only | + +--------------------------------------+-----------------+--------------------------+ + | {$apiKey3->name} | - | Domain only: example.com | + +--------------------------------------+-----------------+--------------------------+ + | {$apiKey4->name} | - | Admin | + +--------------------------------------+-----------------+--------------------------+ + | {$apiKey5->name} | - | Author only | + | | | Domain only: example.com | + +--------------------------------------+-----------------+--------------------------+ + | {$apiKey6->name} | - | Admin | + +--------------------------------------+-----------------+--------------------------+ OUTPUT, ]; yield 'with names' => [ [ - $apiKey1 = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'Alice')), - $apiKey2 = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'Alice and Bob')), + ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'Alice')), + ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'Alice and Bob')), $apiKey3 = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: '')), $apiKey4 = ApiKey::create(), ], true, <<key} | Alice | - | Admin | - +--------------------------------------+---------------+-----------------+-------+ - | {$apiKey2->key} | Alice and Bob | - | Admin | - +--------------------------------------+---------------+-----------------+-------+ - | {$apiKey3->key} | | - | Admin | - +--------------------------------------+---------------+-----------------+-------+ - | {$apiKey4->key} | - | - | Admin | - +--------------------------------------+---------------+-----------------+-------+ + +--------------------------------------+-----------------+-------+ + | Name | Expiration date | Roles | + +--------------------------------------+-----------------+-------+ + | Alice | - | Admin | + +--------------------------------------+-----------------+-------+ + | Alice and Bob | - | Admin | + +--------------------------------------+-----------------+-------+ + | {$apiKey3->name} | - | Admin | + +--------------------------------------+-----------------+-------+ + | {$apiKey4->name} | - | Admin | + +--------------------------------------+-----------------+-------+ OUTPUT, ]; diff --git a/module/CLI/test/Command/Api/RenameApiKeyCommandTest.php b/module/CLI/test/Command/Api/RenameApiKeyCommandTest.php new file mode 100644 index 00000000..41e5689f --- /dev/null +++ b/module/CLI/test/Command/Api/RenameApiKeyCommandTest.php @@ -0,0 +1,83 @@ +apiKeyService = $this->createMock(ApiKeyServiceInterface::class); + $this->commandTester = CliTestUtils::testerForCommand(new RenameApiKeyCommand($this->apiKeyService)); + } + + #[Test] + public function oldNameIsRequestedIfNotProvided(): void + { + $oldName = 'old name'; + $newName = 'new name'; + + $this->apiKeyService->expects($this->once())->method('listKeys')->willReturn([ + ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'foo')), + ApiKey::fromMeta(ApiKeyMeta::fromParams(name: $oldName)), + ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'bar')), + ]); + $this->apiKeyService->expects($this->once())->method('renameApiKey')->with( + Renaming::fromNames($oldName, $newName), + ); + + $this->commandTester->setInputs([$oldName]); + $this->commandTester->execute([ + 'newName' => $newName, + ]); + } + + #[Test] + public function newNameIsRequestedIfNotProvided(): void + { + $oldName = 'old name'; + $newName = 'new name'; + + $this->apiKeyService->expects($this->never())->method('listKeys'); + $this->apiKeyService->expects($this->once())->method('renameApiKey')->with( + Renaming::fromNames($oldName, $newName), + ); + + $this->commandTester->setInputs([$newName]); + $this->commandTester->execute([ + 'oldName' => $oldName, + ]); + } + + #[Test] + public function apiIsRenamedWithProvidedNames(): void + { + $oldName = 'old name'; + $newName = 'new name'; + + $this->apiKeyService->expects($this->never())->method('listKeys'); + $this->apiKeyService->expects($this->once())->method('renameApiKey')->with( + Renaming::fromNames($oldName, $newName), + ); + + $this->commandTester->execute([ + 'oldName' => $oldName, + 'newName' => $newName, + ]); + } +} diff --git a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php index 3b84d175..0a7f9aa0 100644 --- a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php @@ -25,7 +25,6 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey; use ShlinkioTest\Shlink\CLI\Util\CliTestUtils; use Symfony\Component\Console\Tester\CommandTester; -use function count; use function explode; class ListShortUrlsCommandTest extends TestCase @@ -105,94 +104,100 @@ class ListShortUrlsCommandTest extends TestCase #[Test, DataProvider('provideOptionalFlags')] public function provideOptionalFlagsMakesNewColumnsToBeIncluded( array $input, - array $expectedContents, - array $notExpectedContents, - ApiKey $apiKey, + string $expectedOutput, + ShortUrl $shortUrl, ): void { $this->shortUrlService->expects($this->once())->method('listShortUrls')->with( ShortUrlsParams::empty(), )->willReturn(new Paginator(new ArrayAdapter([ - ShortUrlWithDeps::fromShortUrl( - ShortUrl::create(ShortUrlCreation::fromRawData([ - 'longUrl' => 'https://foo.com', - 'tags' => ['foo', 'bar', 'baz'], - 'apiKey' => $apiKey, - ])), - ), + ShortUrlWithDeps::fromShortUrl($shortUrl), ]))); $this->commandTester->setInputs(['y']); $this->commandTester->execute($input); $output = $this->commandTester->getDisplay(); - if (count($expectedContents) === 0 && count($notExpectedContents) === 0) { - self::fail('No expectations were run'); - } - - foreach ($expectedContents as $column) { - self::assertStringContainsString($column, $output); - } - foreach ($notExpectedContents as $column) { - self::assertStringNotContainsString($column, $output); - } + self::assertStringContainsString($expectedOutput, $output); } public static function provideOptionalFlags(): iterable { - $apiKey = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'my api key')); - $key = $apiKey->key; + $shortUrl = ShortUrl::create(ShortUrlCreation::fromRawData([ + 'longUrl' => 'https://foo.com', + 'tags' => ['foo', 'bar', 'baz'], + 'apiKey' => ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'my api key')), + ])); + $shortCode = $shortUrl->getShortCode(); + $created = $shortUrl->dateCreated()->toAtomString(); + // phpcs:disable Generic.Files.LineLength yield 'tags only' => [ ['--show-tags' => true], - ['| Tags ', '| foo, bar, baz'], - ['| API Key ', '| API Key Name |', $key, '| my api key', '| Domain', '| DEFAULT'], - $apiKey, + << [ ['--show-domain' => true], - ['| Domain', '| DEFAULT'], - ['| Tags ', '| foo, bar, baz', '| API Key ', '| API Key Name |', $key, '| my api key'], - $apiKey, + << [ ['--show-api-key' => true], - ['| API Key ', $key], - ['| Tags ', '| foo, bar, baz', '| API Key Name |', '| my api key', '| Domain', '| DEFAULT'], - $apiKey, - ]; - yield 'api key name only' => [ - ['--show-api-key-name' => true], - ['| API Key Name |', '| my api key'], - ['| Tags ', '| foo, bar, baz', '| API Key ', $key], - $apiKey, + << [ ['--show-tags' => true, '--show-api-key' => true], - ['| API Key ', '| Tags ', '| foo, bar, baz', $key], - ['| API Key Name |', '| my api key'], - $apiKey, + << [ ['--show-tags' => true, '--show-domain' => true], - ['| Tags ', '| foo, bar, baz', '| Domain', '| DEFAULT'], - ['| API Key Name |', '| my api key'], - $apiKey, + << [ - ['--show-tags' => true, '--show-domain' => true, '--show-api-key' => true, '--show-api-key-name' => true], - [ - '| API Key ', - '| Tags ', - '| API Key Name |', - '| foo, bar, baz', - $key, - '| my api key', - '| Domain', - '| DEFAULT', - ], - [], - $apiKey, + ['--show-tags' => true, '--show-domain' => true, '--show-api-key' => true], + <<tagService->expects($this->once())->method('renameTag')->with( - TagRenaming::fromNames($oldName, $newName), + Renaming::fromNames($oldName, $newName), )->willThrowException(TagNotFoundException::fromTag('foo')); $this->commandTester->execute([ @@ -50,7 +50,7 @@ class RenameTagCommandTest extends TestCase $oldName = 'foo'; $newName = 'bar'; $this->tagService->expects($this->once())->method('renameTag')->with( - TagRenaming::fromNames($oldName, $newName), + Renaming::fromNames($oldName, $newName), )->willReturn(new Tag($newName)); $this->commandTester->execute([ diff --git a/module/Core/migrations/Version20241105094747.php b/module/Core/migrations/Version20241105094747.php new file mode 100644 index 00000000..4ce5548b --- /dev/null +++ b/module/Core/migrations/Version20241105094747.php @@ -0,0 +1,40 @@ +connection->quoteIdentifier('key'); + + // Append key to the name for all API keys that already have a name + $qb = $this->connection->createQueryBuilder(); + $qb->update('api_keys') + ->set('name', 'CONCAT(name, ' . $this->connection->quote(' - ') . ', ' . $keyColumnName . ')') + ->where($qb->expr()->isNotNull('name')); + $qb->executeStatement(); + + // Set plain key as name for all API keys without a name + $qb = $this->connection->createQueryBuilder(); + $qb->update('api_keys') + ->set('name', $keyColumnName) + ->where($qb->expr()->isNull('name')); + $qb->executeStatement(); + } + + public function isTransactional(): bool + { + return ! ($this->connection->getDatabasePlatform() instanceof MySQLPlatform); + } +} diff --git a/module/Core/migrations/Version20241105215309.php b/module/Core/migrations/Version20241105215309.php new file mode 100644 index 00000000..0e9f7eff --- /dev/null +++ b/module/Core/migrations/Version20241105215309.php @@ -0,0 +1,45 @@ +connection->quoteIdentifier('key'); + + $qb = $this->connection->createQueryBuilder(); + $qb->select($keyColumnName) + ->from('api_keys'); + $result = $qb->executeQuery(); + + $updateQb = $this->connection->createQueryBuilder(); + $updateQb + ->update('api_keys') + ->set($keyColumnName, ':encryptedKey') + ->where($updateQb->expr()->eq($keyColumnName, ':plainTextKey')); + + while ($key = $result->fetchOne()) { + $updateQb->setParameters([ + 'encryptedKey' => hash('sha256', $key), + 'plainTextKey' => $key, + ])->executeStatement(); + } + } + + public function isTransactional(): bool + { + return ! ($this->connection->getDatabasePlatform() instanceof MySQLPlatform); + } +} diff --git a/module/Core/src/Exception/TagConflictException.php b/module/Core/src/Exception/TagConflictException.php index 0fc5c317..e05754c7 100644 --- a/module/Core/src/Exception/TagConflictException.php +++ b/module/Core/src/Exception/TagConflictException.php @@ -7,7 +7,7 @@ namespace Shlinkio\Shlink\Core\Exception; use Fig\Http\Message\StatusCodeInterface; use Mezzio\ProblemDetails\Exception\CommonProblemDetailsExceptionTrait; use Mezzio\ProblemDetails\Exception\ProblemDetailsExceptionInterface; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; +use Shlinkio\Shlink\Core\Model\Renaming; use function Shlinkio\Shlink\Core\toProblemDetailsType; use function sprintf; @@ -19,7 +19,7 @@ class TagConflictException extends RuntimeException implements ProblemDetailsExc private const TITLE = 'Tag conflict'; public const ERROR_CODE = 'tag-conflict'; - public static function forExistingTag(TagRenaming $renaming): self + public static function forExistingTag(Renaming $renaming): self { $e = new self(sprintf('You cannot rename tag %s, because it already exists', $renaming->toString())); diff --git a/module/Core/src/Tag/Model/TagRenaming.php b/module/Core/src/Model/Renaming.php similarity index 86% rename from module/Core/src/Tag/Model/TagRenaming.php rename to module/Core/src/Model/Renaming.php index 9c523b8b..e4cee870 100644 --- a/module/Core/src/Tag/Model/TagRenaming.php +++ b/module/Core/src/Model/Renaming.php @@ -2,15 +2,15 @@ declare(strict_types=1); -namespace Shlinkio\Shlink\Core\Tag\Model; +namespace Shlinkio\Shlink\Core\Model; use Shlinkio\Shlink\Core\Exception\ValidationException; use function sprintf; -final class TagRenaming +final readonly class Renaming { - private function __construct(public readonly string $oldName, public readonly string $newName) + private function __construct(public string $oldName, public string $newName) { } diff --git a/module/Core/src/Repository/EntityRepositoryInterface.php b/module/Core/src/Repository/EntityRepositoryInterface.php new file mode 100644 index 00000000..c6693c44 --- /dev/null +++ b/module/Core/src/Repository/EntityRepositoryInterface.php @@ -0,0 +1,20 @@ + + */ +interface EntityRepositoryInterface extends ObjectRepository +{ + /** + * @todo This should be part of ObjectRepository, so adding here until that interface defines it. + * EntityRepository already implements the method, so classes extending it won't have to add anything. + */ + public function count(array $criteria = []): int; +} diff --git a/module/Core/src/ShortUrl/Entity/ShortUrl.php b/module/Core/src/ShortUrl/Entity/ShortUrl.php index 39bc5ed6..b7fb6c56 100644 --- a/module/Core/src/ShortUrl/Entity/ShortUrl.php +++ b/module/Core/src/ShortUrl/Entity/ShortUrl.php @@ -200,6 +200,11 @@ class ShortUrl extends AbstractEntity return $this->title; } + public function dateCreated(): Chronos + { + return $this->dateCreated; + } + public function reachedVisits(int $visitsAmount): bool { return count($this->visits) >= $visitsAmount; diff --git a/module/Core/src/Tag/TagService.php b/module/Core/src/Tag/TagService.php index e3e5b92f..f91c018f 100644 --- a/module/Core/src/Tag/TagService.php +++ b/module/Core/src/Tag/TagService.php @@ -10,8 +10,8 @@ use Shlinkio\Shlink\Common\Paginator\Paginator; use Shlinkio\Shlink\Core\Exception\ForbiddenTagOperationException; use Shlinkio\Shlink\Core\Exception\TagConflictException; use Shlinkio\Shlink\Core\Exception\TagNotFoundException; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Core\Tag\Entity\Tag; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; use Shlinkio\Shlink\Core\Tag\Model\TagsParams; use Shlinkio\Shlink\Core\Tag\Paginator\Adapter\TagsInfoPaginatorAdapter; use Shlinkio\Shlink\Core\Tag\Paginator\Adapter\TagsPaginatorAdapter; @@ -74,7 +74,7 @@ readonly class TagService implements TagServiceInterface /** * @inheritDoc */ - public function renameTag(TagRenaming $renaming, ApiKey|null $apiKey = null): Tag + public function renameTag(Renaming $renaming, ApiKey|null $apiKey = null): Tag { if (ApiKey::isShortUrlRestricted($apiKey)) { throw ForbiddenTagOperationException::forRenaming(); diff --git a/module/Core/src/Tag/TagServiceInterface.php b/module/Core/src/Tag/TagServiceInterface.php index c09370cf..a22e2ec8 100644 --- a/module/Core/src/Tag/TagServiceInterface.php +++ b/module/Core/src/Tag/TagServiceInterface.php @@ -8,9 +8,9 @@ use Shlinkio\Shlink\Common\Paginator\Paginator; use Shlinkio\Shlink\Core\Exception\ForbiddenTagOperationException; use Shlinkio\Shlink\Core\Exception\TagConflictException; use Shlinkio\Shlink\Core\Exception\TagNotFoundException; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Core\Tag\Entity\Tag; use Shlinkio\Shlink\Core\Tag\Model\TagInfo; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; use Shlinkio\Shlink\Core\Tag\Model\TagsParams; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -37,5 +37,5 @@ interface TagServiceInterface * @throws TagConflictException * @throws ForbiddenTagOperationException */ - public function renameTag(TagRenaming $renaming, ApiKey|null $apiKey = null): Tag; + public function renameTag(Renaming $renaming, ApiKey|null $apiKey = null): Tag; } diff --git a/module/Core/test/Exception/TagConflictExceptionTest.php b/module/Core/test/Exception/TagConflictExceptionTest.php index 2f4bd66a..9126e6f3 100644 --- a/module/Core/test/Exception/TagConflictExceptionTest.php +++ b/module/Core/test/Exception/TagConflictExceptionTest.php @@ -7,7 +7,7 @@ namespace ShlinkioTest\Shlink\Core\Exception; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Exception\TagConflictException; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; +use Shlinkio\Shlink\Core\Model\Renaming; use function sprintf; @@ -19,7 +19,7 @@ class TagConflictExceptionTest extends TestCase $oldName = 'foo'; $newName = 'bar'; $expectedMessage = sprintf('You cannot rename tag %s to %s, because it already exists', $oldName, $newName); - $e = TagConflictException::forExistingTag(TagRenaming::fromNames($oldName, $newName)); + $e = TagConflictException::forExistingTag(Renaming::fromNames($oldName, $newName)); self::assertEquals($expectedMessage, $e->getMessage()); self::assertEquals($expectedMessage, $e->getDetail()); diff --git a/module/Core/test/Tag/TagServiceTest.php b/module/Core/test/Tag/TagServiceTest.php index 7e82eb1c..c1fa8ee7 100644 --- a/module/Core/test/Tag/TagServiceTest.php +++ b/module/Core/test/Tag/TagServiceTest.php @@ -13,9 +13,9 @@ use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Exception\ForbiddenTagOperationException; use Shlinkio\Shlink\Core\Exception\TagConflictException; use Shlinkio\Shlink\Core\Exception\TagNotFoundException; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Core\Tag\Entity\Tag; use Shlinkio\Shlink\Core\Tag\Model\TagInfo; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; use Shlinkio\Shlink\Core\Tag\Model\TagsListFiltering; use Shlinkio\Shlink\Core\Tag\Model\TagsParams; use Shlinkio\Shlink\Core\Tag\Repository\TagRepository; @@ -127,7 +127,7 @@ class TagServiceTest extends TestCase $this->repo->expects($this->once())->method('findOneBy')->willReturn(null); $this->expectException(TagNotFoundException::class); - $this->service->renameTag(TagRenaming::fromNames('foo', 'bar'), $apiKey); + $this->service->renameTag(Renaming::fromNames('foo', 'bar'), $apiKey); } #[Test, DataProvider('provideValidRenames')] @@ -139,7 +139,7 @@ class TagServiceTest extends TestCase $this->repo->expects($this->exactly($count > 0 ? 0 : 1))->method('count')->willReturn($count); $this->em->expects($this->once())->method('flush'); - $tag = $this->service->renameTag(TagRenaming::fromNames($oldName, $newName)); + $tag = $this->service->renameTag(Renaming::fromNames($oldName, $newName)); self::assertSame($expected, $tag); self::assertEquals($newName, (string) $tag); @@ -160,7 +160,7 @@ class TagServiceTest extends TestCase $this->expectException(TagConflictException::class); - $this->service->renameTag(TagRenaming::fromNames('foo', 'bar'), $apiKey); + $this->service->renameTag(Renaming::fromNames('foo', 'bar'), $apiKey); } #[Test] @@ -172,7 +172,7 @@ class TagServiceTest extends TestCase $this->expectExceptionMessage('You are not allowed to rename tags'); $this->service->renameTag( - TagRenaming::fromNames('foo', 'bar'), + Renaming::fromNames('foo', 'bar'), ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls())), ); } diff --git a/module/Rest/config/dependencies.config.php b/module/Rest/config/dependencies.config.php index b69cf36d..df482a46 100644 --- a/module/Rest/config/dependencies.config.php +++ b/module/Rest/config/dependencies.config.php @@ -9,6 +9,7 @@ use Laminas\ServiceManager\Factory\InvokableFactory; use Mezzio\ProblemDetails\ProblemDetailsResponseFactory; use Mezzio\Router\Middleware\ImplicitOptionsMiddleware; use Psr\Log\LoggerInterface; +use Shlinkio\Shlink\Common\Doctrine\EntityRepositoryFactory; use Shlinkio\Shlink\Common\Mercure\LcobucciJwtProvider; use Shlinkio\Shlink\Core\Config; use Shlinkio\Shlink\Core\Domain\DomainService; @@ -17,6 +18,7 @@ use Shlinkio\Shlink\Core\ShortUrl; use Shlinkio\Shlink\Core\ShortUrl\Transformer\ShortUrlDataTransformer; use Shlinkio\Shlink\Core\Tag\TagService; use Shlinkio\Shlink\Core\Visit; +use Shlinkio\Shlink\Rest\ApiKey\Repository\ApiKeyRepository; use Shlinkio\Shlink\Rest\Service\ApiKeyService; return [ @@ -24,6 +26,7 @@ return [ 'dependencies' => [ 'factories' => [ ApiKeyService::class => ConfigAbstractFactory::class, + ApiKeyRepository::class => [EntityRepositoryFactory::class, Entity\ApiKey::class], Action\HealthAction::class => ConfigAbstractFactory::class, Action\MercureInfoAction::class => ConfigAbstractFactory::class, @@ -62,7 +65,7 @@ return [ ], ConfigAbstractFactory::class => [ - ApiKeyService::class => ['em'], + ApiKeyService::class => ['em', ApiKeyRepository::class], Action\HealthAction::class => ['em', Config\Options\AppOptions::class], Action\MercureInfoAction::class => [LcobucciJwtProvider::class, 'config.mercure'], diff --git a/module/Rest/src/Action/Tag/UpdateTagAction.php b/module/Rest/src/Action/Tag/UpdateTagAction.php index 016d008b..e1dc1611 100644 --- a/module/Rest/src/Action/Tag/UpdateTagAction.php +++ b/module/Rest/src/Action/Tag/UpdateTagAction.php @@ -7,7 +7,7 @@ namespace Shlinkio\Shlink\Rest\Action\Tag; use Laminas\Diactoros\Response\EmptyResponse; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Core\Tag\TagServiceInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; use Shlinkio\Shlink\Rest\Middleware\AuthenticationMiddleware; @@ -27,7 +27,7 @@ class UpdateTagAction extends AbstractRestAction $body = $request->getParsedBody(); $apiKey = AuthenticationMiddleware::apiKeyFromRequest($request); - $this->tagService->renameTag(TagRenaming::fromArray($body), $apiKey); + $this->tagService->renameTag(Renaming::fromArray($body), $apiKey); return new EmptyResponse(); } } diff --git a/module/Rest/src/ApiKey/Model/ApiKeyMeta.php b/module/Rest/src/ApiKey/Model/ApiKeyMeta.php index 04b37214..66d7d889 100644 --- a/module/Rest/src/ApiKey/Model/ApiKeyMeta.php +++ b/module/Rest/src/ApiKey/Model/ApiKeyMeta.php @@ -7,6 +7,9 @@ namespace Shlinkio\Shlink\Rest\ApiKey\Model; use Cake\Chronos\Chronos; use Ramsey\Uuid\Uuid; +use function sprintf; +use function substr; + final readonly class ApiKeyMeta { /** @@ -14,7 +17,7 @@ final readonly class ApiKeyMeta */ private function __construct( public string $key, - public string|null $name, + public string $name, public Chronos|null $expirationDate, public iterable $roleDefinitions, ) { @@ -34,8 +37,19 @@ final readonly class ApiKeyMeta Chronos|null $expirationDate = null, iterable $roleDefinitions = [], ): self { + $resolvedKey = $key ?? Uuid::uuid4()->toString(); + + // If a name was not provided, fall back to the key + if (empty($name)) { + // If the key was auto-generated, fall back to a redacted version of the UUID, otherwise simply use the + // plain key as fallback name + $name = $key === null + ? sprintf('%s-****-****-****-************', substr($resolvedKey, offset: 0, length: 8)) + : $key; + } + return new self( - key: $key ?? Uuid::uuid4()->toString(), + key: $resolvedKey, name: $name, expirationDate: $expirationDate, roleDefinitions: $roleDefinitions, diff --git a/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php b/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php index 2d82b23e..b4523371 100644 --- a/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php +++ b/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php @@ -15,31 +15,30 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey; class ApiKeyRepository extends EntitySpecificationRepository implements ApiKeyRepositoryInterface { /** - * Will create provided API key with admin permissions, only if there's no other API keys yet + * Will create provided API key with admin permissions, only if no other API keys exist yet */ public function createInitialApiKey(string $apiKey): ApiKey|null { $em = $this->getEntityManager(); return $em->wrapInTransaction(function () use ($apiKey, $em): ApiKey|null { - // 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') - ->from(ApiKey::class, 'a') - ->setMaxResults(1) - ->getQuery() - ->setLockMode(LockMode::PESSIMISTIC_WRITE) - ->getOneOrNullResult(); + $firstResult = $em->createQueryBuilder() + ->select('a.id') + ->from(ApiKey::class, 'a') + ->setMaxResults(1) + ->getQuery() + ->setLockMode(LockMode::PESSIMISTIC_WRITE) + ->getOneOrNullResult(); // 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); + $initialApiKey = ApiKey::fromMeta(ApiKeyMeta::fromParams(key: $apiKey)); + $em->persist($initialApiKey); $em->flush(); - return $new; + return $initialApiKey; }); } } diff --git a/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php b/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php index 04e55519..0f81dc10 100644 --- a/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php +++ b/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php @@ -4,14 +4,14 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\ApiKey\Repository; -use Doctrine\Persistence\ObjectRepository; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepositoryInterface; +use Shlinkio\Shlink\Core\Repository\EntityRepositoryInterface; use Shlinkio\Shlink\Rest\Entity\ApiKey; /** - * @extends ObjectRepository + * @extends EntityRepositoryInterface */ -interface ApiKeyRepositoryInterface extends ObjectRepository, EntitySpecificationRepositoryInterface +interface ApiKeyRepositoryInterface extends EntityRepositoryInterface, EntitySpecificationRepositoryInterface { /** * Will create provided API key only if there's no API keys yet diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index 4f7575c5..c9cdd3a6 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -7,7 +7,6 @@ namespace Shlinkio\Shlink\Rest\Entity; use Cake\Chronos\Chronos; use Doctrine\Common\Collections\ArrayCollection; use Doctrine\Common\Collections\Collection; -use Exception; use Happyr\DoctrineSpecification\Spec; use Happyr\DoctrineSpecification\Specification\Specification; use Shlinkio\Shlink\Common\Entity\AbstractEntity; @@ -15,6 +14,8 @@ use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; use Shlinkio\Shlink\Rest\ApiKey\Role; +use function hash; + class ApiKey extends AbstractEntity { /** @@ -22,27 +23,22 @@ class ApiKey extends AbstractEntity */ private function __construct( public readonly string $key, - public readonly string|null $name = null, + // TODO Use a property hook to allow public read but private write + public string $name, public readonly Chronos|null $expirationDate = null, private bool $enabled = true, private Collection $roles = new ArrayCollection(), ) { } - /** - * @throws Exception - */ public static function create(): ApiKey { return self::fromMeta(ApiKeyMeta::empty()); } - /** - * @throws Exception - */ public static function fromMeta(ApiKeyMeta $meta): self { - $apiKey = new self($meta->key, $meta->name, $meta->expirationDate); + $apiKey = new self(self::hashKey($meta->key), $meta->name, $meta->expirationDate); foreach ($meta->roleDefinitions as $roleDefinition) { $apiKey->registerRole($roleDefinition); } @@ -50,6 +46,14 @@ class ApiKey extends AbstractEntity return $apiKey; } + /** + * Generates a hash for provided key, in the way Shlink expects API keys to be hashed + */ + public static function hashKey(string $key): string + { + return hash('sha256', $key); + } + public function isExpired(): bool { return $this->expirationDate !== null && $this->expirationDate->lessThan(Chronos::now()); diff --git a/module/Rest/src/Service/ApiKeyCheckResult.php b/module/Rest/src/Service/ApiKeyCheckResult.php index 4a1fc1cf..7c415097 100644 --- a/module/Rest/src/Service/ApiKeyCheckResult.php +++ b/module/Rest/src/Service/ApiKeyCheckResult.php @@ -6,9 +6,9 @@ namespace Shlinkio\Shlink\Rest\Service; use Shlinkio\Shlink\Rest\Entity\ApiKey; -final class ApiKeyCheckResult +final readonly class ApiKeyCheckResult { - public function __construct(public readonly ApiKey|null $apiKey = null) + public function __construct(public ApiKey|null $apiKey = null) { } diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php index 6c825a4a..f517dde5 100644 --- a/module/Rest/src/Service/ApiKeyService.php +++ b/module/Rest/src/Service/ApiKeyService.php @@ -6,21 +6,28 @@ namespace Shlinkio\Shlink\Rest\Service; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Repository\ApiKeyRepositoryInterface; use Shlinkio\Shlink\Rest\Entity\ApiKey; use function sprintf; -class ApiKeyService implements ApiKeyServiceInterface +readonly class ApiKeyService implements ApiKeyServiceInterface { - public function __construct(private readonly EntityManagerInterface $em) + public function __construct(private EntityManagerInterface $em, private ApiKeyRepositoryInterface $repo) { } public function create(ApiKeyMeta $apiKeyMeta): ApiKey { + // TODO If name is auto-generated, do not throw. Instead, re-generate a new key $apiKey = ApiKey::fromMeta($apiKeyMeta); + if ($this->existsWithName($apiKey->name)) { + throw new InvalidArgumentException( + sprintf('Another API key with name "%s" already exists', $apiKeyMeta->name), + ); + } $this->em->persist($apiKey); $this->em->flush(); @@ -30,29 +37,40 @@ class ApiKeyService implements ApiKeyServiceInterface public function createInitial(string $key): ApiKey|null { - /** @var ApiKeyRepositoryInterface $repo */ - $repo = $this->em->getRepository(ApiKey::class); - return $repo->createInitialApiKey($key); + return $this->repo->createInitialApiKey($key); } public function check(string $key): ApiKeyCheckResult { - $apiKey = $this->getByKey($key); + $apiKey = $this->findByKey($key); return new ApiKeyCheckResult($apiKey); } /** - * @throws InvalidArgumentException + * @inheritDoc */ - public function disable(string $key): ApiKey + public function disableByName(string $apiKeyName): ApiKey + { + return $this->disableApiKey($this->repo->findOneBy(['name' => $apiKeyName])); + } + + /** + * @inheritDoc + */ + public function disableByKey(string $key): ApiKey + { + return $this->disableApiKey($this->findByKey($key)); + } + + private function disableApiKey(ApiKey|null $apiKey): ApiKey { - $apiKey = $this->getByKey($key); if ($apiKey === null) { - throw new InvalidArgumentException(sprintf('API key "%s" does not exist and can\'t be disabled', $key)); + throw new InvalidArgumentException('Provided API key does not exist and can\'t be disabled'); } $apiKey->disable(); $this->em->flush(); + return $apiKey; } @@ -62,17 +80,47 @@ class ApiKeyService implements ApiKeyServiceInterface public function listKeys(bool $enabledOnly = false): array { $conditions = $enabledOnly ? ['enabled' => true] : []; - /** @var ApiKey[] $apiKeys */ - $apiKeys = $this->em->getRepository(ApiKey::class)->findBy($conditions); - return $apiKeys; + return $this->repo->findBy($conditions); } - private function getByKey(string $key): ApiKey|null + /** + * @inheritDoc + * @todo This method should be transactional and to a SELECT ... FROM UPDATE when checking if the new name exists, + * to avoid a race condition where the method is called twice in parallel for a new name that doesn't exist, + * causing two API keys to end up with the same name. + */ + public function renameApiKey(Renaming $apiKeyRenaming): ApiKey { - /** @var ApiKey|null $apiKey */ - $apiKey = $this->em->getRepository(ApiKey::class)->findOneBy([ - 'key' => $key, - ]); + $apiKey = $this->repo->findOneBy(['name' => $apiKeyRenaming->oldName]); + if ($apiKey === null) { + throw new InvalidArgumentException( + sprintf('API key with name "%s" could not be found', $apiKeyRenaming->oldName), + ); + } + + if (! $apiKeyRenaming->nameChanged()) { + return $apiKey; + } + + if ($this->existsWithName($apiKeyRenaming->newName)) { + throw new InvalidArgumentException( + sprintf('Another API key with name "%s" already exists', $apiKeyRenaming->newName), + ); + } + + $apiKey->name = $apiKeyRenaming->newName; + $this->em->flush(); + return $apiKey; } + + private function findByKey(string $key): ApiKey|null + { + return $this->repo->findOneBy(['key' => ApiKey::hashKey($key)]); + } + + private function existsWithName(string $apiKeyName): bool + { + return $this->repo->count(['name' => $apiKeyName]) > 0; + } } diff --git a/module/Rest/src/Service/ApiKeyServiceInterface.php b/module/Rest/src/Service/ApiKeyServiceInterface.php index 167041c5..c42505b7 100644 --- a/module/Rest/src/Service/ApiKeyServiceInterface.php +++ b/module/Rest/src/Service/ApiKeyServiceInterface.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Service; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -19,10 +20,21 @@ interface ApiKeyServiceInterface /** * @throws InvalidArgumentException */ - public function disable(string $key): ApiKey; + public function disableByName(string $apiKeyName): ApiKey; + + /** + * @deprecated Use `self::disableByName($name)` instead + * @throws InvalidArgumentException + */ + public function disableByKey(string $key): ApiKey; /** * @return ApiKey[] */ public function listKeys(bool $enabledOnly = false): array; + + /** + * @throws InvalidArgumentException If an API key with oldName does not exist, or newName is in use by another one + */ + public function renameApiKey(Renaming $apiKeyRenaming): ApiKey; } diff --git a/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php b/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php index c19d8512..62d52de6 100644 --- a/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php +++ b/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php @@ -24,9 +24,9 @@ class ApiKeyRepositoryTest extends DatabaseTestCase self::assertCount(0, $this->repo->findAll()); self::assertNotNull($this->repo->createInitialApiKey('initial_value')); self::assertCount(1, $this->repo->findAll()); - self::assertCount(1, $this->repo->findBy(['key' => 'initial_value'])); + self::assertCount(1, $this->repo->findBy(['key' => ApiKey::hashKey('initial_value')])); self::assertNull($this->repo->createInitialApiKey('another_one')); self::assertCount(1, $this->repo->findAll()); - self::assertCount(0, $this->repo->findBy(['key' => 'another_one'])); + self::assertCount(0, $this->repo->findBy(['key' => ApiKey::hashKey('another_one')])); } } diff --git a/module/Rest/test/Action/Tag/UpdateTagActionTest.php b/module/Rest/test/Action/Tag/UpdateTagActionTest.php index 73575baf..f83ee037 100644 --- a/module/Rest/test/Action/Tag/UpdateTagActionTest.php +++ b/module/Rest/test/Action/Tag/UpdateTagActionTest.php @@ -11,8 +11,8 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Core\Exception\ValidationException; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Core\Tag\Entity\Tag; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; use Shlinkio\Shlink\Core\Tag\TagServiceInterface; use Shlinkio\Shlink\Rest\Action\Tag\UpdateTagAction; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -53,7 +53,7 @@ class UpdateTagActionTest extends TestCase 'newName' => 'bar', ]); $this->tagService->expects($this->once())->method('renameTag')->with( - TagRenaming::fromNames('foo', 'bar'), + Renaming::fromNames('foo', 'bar'), $this->isInstanceOf(ApiKey::class), )->willReturn(new Tag('bar')); diff --git a/module/Rest/test/ApiKey/Model/ApiKeyMetaTest.php b/module/Rest/test/ApiKey/Model/ApiKeyMetaTest.php new file mode 100644 index 00000000..dfd5b9f9 --- /dev/null +++ b/module/Rest/test/ApiKey/Model/ApiKeyMetaTest.php @@ -0,0 +1,35 @@ +name); + } + + public static function provideNames(): iterable + { + yield 'name' => [null, 'the name', static fn (ApiKeyMeta $meta) => 'the name']; + yield 'key' => ['the key', null, static fn (ApiKeyMeta $meta) => 'the key']; + yield 'generated key' => [null, null, static fn (ApiKeyMeta $meta) => sprintf( + '%s-****-****-****-************', + substr($meta->key, offset: 0, length: 8), + )]; + } +} diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php index a799da27..adecfbd9 100644 --- a/module/Rest/test/Service/ApiKeyServiceTest.php +++ b/module/Rest/test/Service/ApiKeyServiceTest.php @@ -12,23 +12,26 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; use Shlinkio\Shlink\Core\Domain\Entity\Domain; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; -use Shlinkio\Shlink\Rest\ApiKey\Repository\ApiKeyRepository; +use Shlinkio\Shlink\Rest\ApiKey\Repository\ApiKeyRepositoryInterface; use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\Rest\Service\ApiKeyService; +use function substr; + class ApiKeyServiceTest extends TestCase { private ApiKeyService $service; private MockObject & EntityManager $em; - private MockObject & ApiKeyRepository $repo; + private MockObject & ApiKeyRepositoryInterface $repo; protected function setUp(): void { $this->em = $this->createMock(EntityManager::class); - $this->repo = $this->createMock(ApiKeyRepository::class); - $this->service = new ApiKeyService($this->em); + $this->repo = $this->createMock(ApiKeyRepositoryInterface::class); + $this->service = new ApiKeyService($this->em, $this->repo); } /** @@ -37,15 +40,20 @@ class ApiKeyServiceTest extends TestCase #[Test, DataProvider('provideCreationDate')] public function apiKeyIsProperlyCreated(Chronos|null $date, string|null $name, array $roles): void { + $this->repo->expects($this->once())->method('count')->with( + ! empty($name) ? ['name' => $name] : $this->isType('array'), + )->willReturn(0); $this->em->expects($this->once())->method('flush'); $this->em->expects($this->once())->method('persist')->with($this->isInstanceOf(ApiKey::class)); - $key = $this->service->create( - ApiKeyMeta::fromParams(name: $name, expirationDate: $date, roleDefinitions: $roles), - ); + $meta = ApiKeyMeta::fromParams(name: $name, expirationDate: $date, roleDefinitions: $roles); + $key = $this->service->create($meta); self::assertEquals($date, $key->expirationDate); - self::assertEquals($name, $key->name); + self::assertEquals( + empty($name) ? substr($meta->key, 0, 8) . '-****-****-****-************' : $name, + $key->name, + ); foreach ($roles as $roleDefinition) { self::assertTrue($key->hasRole($roleDefinition->role)); } @@ -67,11 +75,25 @@ class ApiKeyServiceTest extends TestCase yield 'empty name' => [null, '', []]; } + #[Test] + public function exceptionIsThrownWhileCreatingIfNameIsInUse(): void + { + $this->repo->expects($this->once())->method('count')->with(['name' => 'the_name'])->willReturn(1); + $this->em->expects($this->never())->method('flush'); + $this->em->expects($this->never())->method('persist'); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Another API key with name "the_name" already exists'); + + $this->service->create(ApiKeyMeta::fromParams(name: 'the_name')); + } + #[Test, DataProvider('provideInvalidApiKeys')] public function checkReturnsFalseForInvalidApiKeys(ApiKey|null $invalidKey): void { - $this->repo->expects($this->once())->method('findOneBy')->with(['key' => '12345'])->willReturn($invalidKey); - $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo); + $this->repo->expects($this->once())->method('findOneBy')->with(['key' => ApiKey::hashKey('12345')])->willReturn( + $invalidKey, + ); $result = $this->service->check('12345'); @@ -93,8 +115,9 @@ class ApiKeyServiceTest extends TestCase { $apiKey = ApiKey::create(); - $this->repo->expects($this->once())->method('findOneBy')->with(['key' => '12345'])->willReturn($apiKey); - $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo); + $this->repo->expects($this->once())->method('findOneBy')->with(['key' => ApiKey::hashKey('12345')])->willReturn( + $apiKey, + ); $result = $this->service->check('12345'); @@ -102,38 +125,41 @@ class ApiKeyServiceTest extends TestCase self::assertSame($apiKey, $result->apiKey); } - #[Test] - public function disableThrowsExceptionWhenNoApiKeyIsFound(): void + #[Test, DataProvider('provideDisableArgs')] + public function disableThrowsExceptionWhenNoApiKeyIsFound(string $disableMethod, array $findOneByArg): void { - $this->repo->expects($this->once())->method('findOneBy')->with(['key' => '12345'])->willReturn(null); - $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo); + $this->repo->expects($this->once())->method('findOneBy')->with($findOneByArg)->willReturn(null); $this->expectException(InvalidArgumentException::class); - $this->service->disable('12345'); + $this->service->{$disableMethod}('12345'); } - #[Test] - public function disableReturnsDisabledApiKeyWhenFound(): void + #[Test, DataProvider('provideDisableArgs')] + public function disableReturnsDisabledApiKeyWhenFound(string $disableMethod, array $findOneByArg): void { $key = ApiKey::create(); - $this->repo->expects($this->once())->method('findOneBy')->with(['key' => '12345'])->willReturn($key); - $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo); + $this->repo->expects($this->once())->method('findOneBy')->with($findOneByArg)->willReturn($key); $this->em->expects($this->once())->method('flush'); self::assertTrue($key->isEnabled()); - $returnedKey = $this->service->disable('12345'); + $returnedKey = $this->service->{$disableMethod}('12345'); self::assertFalse($key->isEnabled()); self::assertSame($key, $returnedKey); } + public static function provideDisableArgs(): iterable + { + yield 'disableByKey' => ['disableByKey', ['key' => ApiKey::hashKey('12345')]]; + yield 'disableByName' => ['disableByName', ['name' => '12345']]; + } + #[Test] public function listFindsAllApiKeys(): void { $expectedApiKeys = [ApiKey::create(), ApiKey::create(), ApiKey::create()]; $this->repo->expects($this->once())->method('findBy')->with([])->willReturn($expectedApiKeys); - $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo); $result = $this->service->listKeys(); @@ -146,7 +172,6 @@ class ApiKeyServiceTest extends TestCase $expectedApiKeys = [ApiKey::create(), ApiKey::create(), ApiKey::create()]; $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(enabledOnly: true); @@ -157,7 +182,6 @@ class ApiKeyServiceTest extends TestCase public function createInitialDelegatesToRepository(ApiKey|null $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'); @@ -169,4 +193,66 @@ class ApiKeyServiceTest extends TestCase yield 'first api key' => [ApiKey::create()]; yield 'existing api keys' => [null]; } + + #[Test] + public function renameApiKeyThrowsExceptionIfApiKeyIsNotFound(): void + { + $renaming = Renaming::fromNames(oldName: 'old', newName: 'new'); + + $this->repo->expects($this->once())->method('findOneBy')->with(['name' => 'old'])->willReturn(null); + $this->repo->expects($this->never())->method('count'); + $this->em->expects($this->never())->method('flush'); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('API key with name "old" could not be found'); + + $this->service->renameApiKey($renaming); + } + + #[Test] + public function renameApiKeyReturnsApiKeyVerbatimIfBothNamesAreEqual(): void + { + $renaming = Renaming::fromNames(oldName: 'same_value', newName: 'same_value'); + $apiKey = ApiKey::create(); + + $this->repo->expects($this->once())->method('findOneBy')->with(['name' => 'same_value'])->willReturn($apiKey); + $this->repo->expects($this->never())->method('count'); + $this->em->expects($this->never())->method('flush'); + + $result = $this->service->renameApiKey($renaming); + + self::assertSame($apiKey, $result); + } + + #[Test] + public function renameApiKeyThrowsExceptionIfNewNameIsInUse(): void + { + $renaming = Renaming::fromNames(oldName: 'old', newName: 'new'); + $apiKey = ApiKey::create(); + + $this->repo->expects($this->once())->method('findOneBy')->with(['name' => 'old'])->willReturn($apiKey); + $this->repo->expects($this->once())->method('count')->with(['name' => 'new'])->willReturn(1); + $this->em->expects($this->never())->method('flush'); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Another API key with name "new" already exists'); + + $this->service->renameApiKey($renaming); + } + + #[Test] + public function renameApiKeyReturnsApiKeyWithNewName(): void + { + $renaming = Renaming::fromNames(oldName: 'old', newName: 'new'); + $apiKey = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'old')); + + $this->repo->expects($this->once())->method('findOneBy')->with(['name' => 'old'])->willReturn($apiKey); + $this->repo->expects($this->once())->method('count')->with(['name' => 'new'])->willReturn(0); + $this->em->expects($this->once())->method('flush'); + + $result = $this->service->renameApiKey($renaming); + + self::assertSame($apiKey, $result); + self::assertEquals('new', $apiKey->name); + } }