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/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/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index 17461d12..fea06a1d 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; @@ -31,17 +30,11 @@ class ApiKey extends AbstractEntity ) { } - /** - * @throws Exception - */ public static function create(): ApiKey { return self::fromMeta(ApiKeyMeta::empty()); } - /** - * @throws Exception - */ public static function fromMeta(ApiKeyMeta $meta): self { $apiKey = new self(self::hashKey($meta->key), $meta->name, $meta->expirationDate); diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php index ca43a81f..66cb1b18 100644 --- a/module/Rest/src/Service/ApiKeyService.php +++ b/module/Rest/src/Service/ApiKeyService.php @@ -40,11 +40,25 @@ readonly class ApiKeyService implements ApiKeyServiceInterface } /** - * @throws InvalidArgumentException + * @inheritDoc */ - public function disable(string $key): ApiKey + public function disableByName(string $apiKeyName): ApiKey + { + return $this->disableApiKey($this->em->getRepository(ApiKey::class)->findOneBy([ + 'name' => $apiKeyName, + ])); + } + + /** + * @inheritDoc + */ + public function disableByKey(string $key): ApiKey + { + return $this->disableApiKey($this->getByKey($key)); + } + + private function disableApiKey(ApiKey|null $apiKey): ApiKey { - $apiKey = $this->getByKey($key); if ($apiKey === null) { throw new InvalidArgumentException('Provided API key does not exist and can\'t be disabled'); } diff --git a/module/Rest/src/Service/ApiKeyServiceInterface.php b/module/Rest/src/Service/ApiKeyServiceInterface.php index 167041c5..1fefc5f4 100644 --- a/module/Rest/src/Service/ApiKeyServiceInterface.php +++ b/module/Rest/src/Service/ApiKeyServiceInterface.php @@ -19,7 +19,13 @@ 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[] diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php index b081b99a..ba57fec4 100644 --- a/module/Rest/test/Service/ApiKeyServiceTest.php +++ b/module/Rest/test/Service/ApiKeyServiceTest.php @@ -110,35 +110,37 @@ 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' => ApiKey::hashKey('12345')])->willReturn( - null, - ); + $this->repo->expects($this->once())->method('findOneBy')->with($findOneByArg)->willReturn(null); $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo); $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' => ApiKey::hashKey('12345')])->willReturn( - $key, - ); + $this->repo->expects($this->once())->method('findOneBy')->with($findOneByArg)->willReturn($key); $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo); $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 {