Update api-key:disable command to allow passing a name

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)
+        $help = <<<HELP
-             ->setDescription('Disables an API key.')
+        The <info>%command.name%</info> command allows you to disable an existing API key, via its name or the
-             ->addArgument('apiKey', InputArgument::REQUIRED, 'The API key to disable');
+        plain-text key.
+
+        If no arguments are provided, you will be prompted to select one of the existing non-disabled API keys.
+
+            <info>%command.full_name%</info>
+
+        You can optionally pass the API key name to be disabled. In that case <comment>--by-name</comment> is also
+        required, to indicate the first argument is the API key name and not the plain-text key:
+
+            <info>%command.full_name% the_key_name --by-name</info>
+
+        You can pass the plain-text key to be disabled, but that is <options=bold>DEPRECATED</>. In next major version,
+        the argument will always be assumed to be the name:
+
+            <info>%command.full_name% d6b6c60e-edcd-4e43-96ad-fa6b7014c143</info>
+
+        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);
+            if ($byName) {
-            $io->success(sprintf('API key "%s" properly disabled', $apiKey));
+                $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());

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([
+        $exitCode = $this->commandTester->execute([
-            'apiKey' => $apiKey,
+            '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([
+        $exitCode = $this->commandTester->execute([
-            'apiKey' => $apiKey,
+            '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);
     }
 }

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);

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');
         }

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[]

module/Rest/test/Service/ApiKeyServiceTest.php

@@ -110,35 +110,37 @@ class ApiKeyServiceTest extends TestCase
         self::assertSame($apiKey, $result->apiKey);
     }
 
-    #[Test]
+    #[Test, DataProvider('provideDisableArgs')]
-    public function disableThrowsExceptionWhenNoApiKeyIsFound(): void
+    public function disableThrowsExceptionWhenNoApiKeyIsFound(string $disableMethod, array $findOneByArg): void
     {
-        $this->repo->expects($this->once())->method('findOneBy')->with(['key' => ApiKey::hashKey('12345')])->willReturn(
+        $this->repo->expects($this->once())->method('findOneBy')->with($findOneByArg)->willReturn(null);
-            null,
-        );
         $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo);
 
         $this->expectException(InvalidArgumentException::class);
 
-        $this->service->disable('12345');
+        $this->service->{$disableMethod}('12345');
     }
 
-    #[Test]
+    #[Test, DataProvider('provideDisableArgs')]
-    public function disableReturnsDisabledApiKeyWhenFound(): void
+    public function disableReturnsDisabledApiKeyWhenFound(string $disableMethod, array $findOneByArg): void
     {
         $key = ApiKey::create();
-        $this->repo->expects($this->once())->method('findOneBy')->with(['key' => ApiKey::hashKey('12345')])->willReturn(
+        $this->repo->expects($this->once())->method('findOneBy')->with($findOneByArg)->willReturn($key);
-            $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
     {