From 9f564b97858c8d145bc2a864146f40713031c6ad Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 8 Nov 2025 09:16:15 +0100 Subject: [PATCH] Do not allow API keys to be disabled by plain-text key --- CHANGELOG.md | 1 + .../CLI/src/Command/Api/DisableKeyCommand.php | 42 ++++---------- .../Command/Api/DisableKeyCommandTest.php | 55 ++----------------- module/Rest/src/Service/ApiKeyService.php | 13 ----- .../src/Service/ApiKeyServiceInterface.php | 6 -- .../Rest/test/Service/ApiKeyServiceTest.php | 22 +++----- 6 files changed, 26 insertions(+), 113 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0098a519..5ec25b8a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#2507](https://github.com/shlinkio/shlink/issues/2507) Drop support for PHP 8.3. * [#2514](https://github.com/shlinkio/shlink/issues/2514) Remove support to generate QR codes. This functionality is now handled by Shlink Web Client and Shlink Dashboard. * [#2517](https://github.com/shlinkio/shlink/issues/2517) Remove `REDIRECT_APPEND_EXTRA_PATH` env var. Use `REDIRECT_EXTRA_PATH_MODE=append` instead. +* [#2519](https://github.com/shlinkio/shlink/issues/2519) Disabling API keys by their plain-text key is no longer supported. When calling `api-key:disable`, the first argument is now always assumed to be the name. ### Fixed * *Nothing* diff --git a/module/CLI/src/Command/Api/DisableKeyCommand.php b/module/CLI/src/Command/Api/DisableKeyCommand.php index 308c432f..46380db8 100644 --- a/module/CLI/src/Command/Api/DisableKeyCommand.php +++ b/module/CLI/src/Command/Api/DisableKeyCommand.php @@ -9,7 +9,6 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface; use Symfony\Component\Console\Attribute\Argument; use Symfony\Component\Console\Attribute\AsCommand; -use Symfony\Component\Console\Attribute\Option; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Output\OutputInterface; @@ -20,24 +19,17 @@ use function sprintf; #[AsCommand( name: DisableKeyCommand::NAME, - description: 'Disables an API key by name or plain-text key (providing a plain-text key is DEPRECATED)', + description: 'Disables an API key by name', help: <<%command.name% command allows you to disable an existing API key, via its name or the - plain-text key. + The %command.name% command allows you to disable an existing API 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: + You can optionally pass the API key name to be disabled: - %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 + %command.full_name% the_key_name HELP, )] @@ -52,41 +44,31 @@ class DisableKeyCommand extends Command protected function interact(InputInterface $input, OutputInterface $output): void { - $keyOrName = $input->getArgument('key-or-name'); + $name = $input->getArgument('name'); - if ($keyOrName === null) { + if ($name === null) { $apiKeys = $this->apiKeyService->listKeys(enabledOnly: true); - $name = (new SymfonyStyle($input, $output))->choice( + $name = new SymfonyStyle($input, $output)->choice( 'What API key do you want to disable?', map($apiKeys, static fn (ApiKey $apiKey) => $apiKey->name), ); - $input->setArgument('key-or-name', $name); - $input->setOption('by-name', true); + $input->setArgument('name', $name); } } public function __invoke( SymfonyStyle $io, - #[Argument( - description: 'The API key to disable. Pass `--by-name` to indicate this value is the name and not the key.', - )] - string|null $keyOrName = null, - #[Option(description: 'Indicates the first argument is the API key name, not the plain-text key.')] - bool $byName = false, + #[Argument('The name of the API key to disable.')] string|null $name = null, ): int { - if ($keyOrName === null) { + if ($name === null) { $io->warning('An API key name was not provided.'); return Command::INVALID; } try { - if ($byName) { - $this->apiKeyService->disableByName($keyOrName); - } else { - $this->apiKeyService->disableByKey($keyOrName); - } - $io->success(sprintf('API key "%s" properly disabled', $keyOrName)); + $this->apiKeyService->disableByName($name); + $io->success(sprintf('API key "%s" properly disabled', $name)); return Command::SUCCESS; } catch (InvalidArgumentException $e) { $io->error($e->getMessage()); diff --git a/module/CLI/test/Command/Api/DisableKeyCommandTest.php b/module/CLI/test/Command/Api/DisableKeyCommandTest.php index 85918305..3a6c93b5 100644 --- a/module/CLI/test/Command/Api/DisableKeyCommandTest.php +++ b/module/CLI/test/Command/Api/DisableKeyCommandTest.php @@ -31,12 +31,9 @@ class DisableKeyCommandTest extends TestCase public function providedApiKeyIsDisabled(): void { $apiKey = 'abcd1234'; - $this->apiKeyService->expects($this->once())->method('disableByKey')->with($apiKey); - $this->apiKeyService->expects($this->never())->method('disableByName'); + $this->apiKeyService->expects($this->once())->method('disableByName')->with($apiKey); - $exitCode = $this->commandTester->execute([ - 'key-or-name' => $apiKey, - ]); + $exitCode = $this->commandTester->execute(['name' => $apiKey]); $output = $this->commandTester->getDisplay(); self::assertStringContainsString('API key "abcd1234" properly disabled', $output); @@ -44,55 +41,15 @@ class DisableKeyCommandTest extends TestCase } #[Test] - 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([ - 'key-or-name' => $name, - '--by-name' => true, - ]); - $output = $this->commandTester->getDisplay(); - - self::assertStringContainsString('API key "the key to delete" properly disabled', $output); - self::assertEquals(Command::SUCCESS, $exitCode); - } - - #[Test] - public function errorIsReturnedIfDisableByKeyThrowsException(): void + public function errorIsReturnedIfDisableByNameThrowsException(): void { $apiKey = 'abcd1234'; $expectedMessage = 'API key "abcd1234" does not exist.'; - $this->apiKeyService->expects($this->once())->method('disableByKey')->with($apiKey)->willThrowException( + $this->apiKeyService->expects($this->once())->method('disableByName')->with($apiKey)->willThrowException( new InvalidArgumentException($expectedMessage), ); - $this->apiKeyService->expects($this->never())->method('disableByName'); - $exitCode = $this->commandTester->execute([ - 'key-or-name' => $apiKey, - ]); - $output = $this->commandTester->getDisplay(); - - self::assertStringContainsString($expectedMessage, $output); - self::assertEquals(Command::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([ - 'key-or-name' => $name, - '--by-name' => true, - ]); + $exitCode = $this->commandTester->execute(['name' => $apiKey]); $output = $this->commandTester->getDisplay(); self::assertStringContainsString($expectedMessage, $output); @@ -103,7 +60,6 @@ class DisableKeyCommandTest extends TestCase 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]); @@ -121,7 +77,6 @@ class DisableKeyCommandTest extends TestCase 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([]); diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php index a5a85b79..d717126d 100644 --- a/module/Rest/src/Service/ApiKeyService.php +++ b/module/Rest/src/Service/ApiKeyService.php @@ -92,19 +92,6 @@ readonly class ApiKeyService implements ApiKeyServiceInterface return $this->disableApiKey($apiKey); } - /** - * @inheritDoc - */ - public function disableByKey(string $key): ApiKey - { - $apiKey = $this->findByKey($key); - if ($apiKey === null) { - throw ApiKeyNotFoundException::forKey($key); - } - - return $this->disableApiKey($apiKey); - } - private function disableApiKey(ApiKey $apiKey): ApiKey { $apiKey->disable(); diff --git a/module/Rest/src/Service/ApiKeyServiceInterface.php b/module/Rest/src/Service/ApiKeyServiceInterface.php index 6eb3df0f..ebd78259 100644 --- a/module/Rest/src/Service/ApiKeyServiceInterface.php +++ b/module/Rest/src/Service/ApiKeyServiceInterface.php @@ -32,12 +32,6 @@ interface ApiKeyServiceInterface */ public function disableByName(string $apiKeyName): ApiKey; - /** - * @deprecated Use `self::disableByName($name)` instead - * @throws ApiKeyNotFoundException - */ - public function disableByKey(string $key): ApiKey; - /** * @return ApiKey[] */ diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php index 146ce0ac..3efe3f43 100644 --- a/module/Rest/test/Service/ApiKeyServiceTest.php +++ b/module/Rest/test/Service/ApiKeyServiceTest.php @@ -143,35 +143,29 @@ class ApiKeyServiceTest extends TestCase self::assertSame($apiKey, $result->apiKey); } - #[Test, DataProvider('provideDisableArgs')] - public function disableThrowsExceptionWhenNoApiKeyIsFound(string $disableMethod, array $findOneByArg): void + #[Test] + public function disableThrowsExceptionWhenNoApiKeyIsFound(): void { - $this->repo->expects($this->once())->method('findOneBy')->with($findOneByArg)->willReturn(null); + $this->repo->expects($this->once())->method('findOneBy')->with(['name' => '12345'])->willReturn(null); $this->expectException(ApiKeyNotFoundException::class); - $this->service->{$disableMethod}('12345'); + $this->service->disableByName('12345'); } - #[Test, DataProvider('provideDisableArgs')] - public function disableReturnsDisabledApiKeyWhenFound(string $disableMethod, array $findOneByArg): void + #[Test] + public function disableReturnsDisabledApiKeyWhenFound(): void { $key = ApiKey::create(); - $this->repo->expects($this->once())->method('findOneBy')->with($findOneByArg)->willReturn($key); + $this->repo->expects($this->once())->method('findOneBy')->with(['name' => '12345'])->willReturn($key); $this->em->expects($this->once())->method('flush'); self::assertTrue($key->isEnabled()); - $returnedKey = $this->service->{$disableMethod}('12345'); + $returnedKey = $this->service->disableByName('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 {