From 819a535bfe3bcca8164d530be2f930e8763cdd13 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
Date: Tue, 5 Nov 2024 11:08:11 +0100
Subject: [PATCH 01/13] Create migration to set API keys in name column

---
 .../Core/migrations/Version20241105094747.php | 40 +++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 module/Core/migrations/Version20241105094747.php

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 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkMigrations;
+
+use Doctrine\DBAL\Platforms\MySQLPlatform;
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+/**
+ * In preparation to start hashing API keys, move all plain-text keys to the `name` column for all keys without name,
+ * and append it to the name for all keys which already have a name.
+ */
+final class Version20241105094747 extends AbstractMigration
+{
+    public function up(Schema $schema): void
+    {
+        $keyColumnName = $this->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);
+    }
+}

From a094be2b9ef7f85b3ef8878140c28fcda19cbd11 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
Date: Tue, 5 Nov 2024 11:26:39 +0100
Subject: [PATCH 02/13] Fall back API key names to auto-generated keys

---
 .../src/Command/Api/GenerateKeyCommand.php    | 13 ++-
 .../CLI/src/Command/Api/ListKeysCommand.php   |  3 +-
 .../CLI/test-cli/Command/ListApiKeysTest.php  | 56 ++++++------
 .../Command/Api/GenerateKeyCommandTest.php    |  2 +-
 .../test/Command/Api/ListKeysCommandTest.php  | 90 +++++++++----------
 module/Rest/src/ApiKey/Model/ApiKeyMeta.php   | 18 +++-
 .../ApiKey/Repository/ApiKeyRepository.php    | 23 +++--
 module/Rest/src/Entity/ApiKey.php             | 11 +++
 module/Rest/src/Service/ApiKeyCheckResult.php |  4 +-
 module/Rest/src/Service/ApiKeyService.php     | 18 ++--
 .../Rest/test/Service/ApiKeyServiceTest.php   | 12 ++-
 11 files changed, 139 insertions(+), 111 deletions(-)

diff --git a/module/CLI/src/Command/Api/GenerateKeyCommand.php b/module/CLI/src/Command/Api/GenerateKeyCommand.php
index a7656189..a6b8bad0 100644
--- a/module/CLI/src/Command/Api/GenerateKeyCommand.php
+++ b/module/CLI/src/Command/Api/GenerateKeyCommand.php
@@ -102,19 +102,24 @@ class GenerateKeyCommand extends Command
     {
         $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),
-        ));
+        );
+        $apiKey = $this->apiKeyService->create($apiKeyMeta);
 
         $io = new SymfonyStyle($input, $output);
-        $io->success(sprintf('Generated API key: "%s"', $apiKey->key));
+        $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)]),
+                $apiKey->mapRoles(fn (Role $role, array $meta) => [$role->value, arrayToString($meta, indentSize: 0)]),
                 null,
                 '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/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 = <<<OUT
-        +--------------------+------+---------------------------+--------------------------+
-        | Key                | Name | Expiration date           | Roles                    |
-        +--------------------+------+---------------------------+--------------------------+
-        | valid_api_key      | -    | -                         | Admin                    |
-        +--------------------+------+---------------------------+--------------------------+
-        | expired_api_key    | -    | {$expiredApiKeyDate} | Admin                    |
-        +--------------------+------+---------------------------+--------------------------+
-        | author_api_key     | -    | -                         | Author only              |
-        +--------------------+------+---------------------------+--------------------------+
-        | domain_api_key     | -    | -                         | Domain only: example.com |
-        +--------------------+------+---------------------------+--------------------------+
-        | no_orphans_api_key | -    | -                         | No orphan visits         |
-        +--------------------+------+---------------------------+--------------------------+
+        +--------------------+---------------------------+--------------------------+
+        | Name               | Expiration date           | Roles                    |
+        +--------------------+---------------------------+--------------------------+
+        | valid_api_key      | -                         | Admin                    |
+        +--------------------+---------------------------+--------------------------+
+        | expired_api_key    | {$expiredApiKeyDate} | Admin                    |
+        +--------------------+---------------------------+--------------------------+
+        | author_api_key     | -                         | Author only              |
+        +--------------------+---------------------------+--------------------------+
+        | domain_api_key     | -                         | Domain only: example.com |
+        +--------------------+---------------------------+--------------------------+
+        | no_orphans_api_key | -                         | No orphan visits         |
+        +--------------------+---------------------------+--------------------------+
 
         OUT;
 
         yield 'no flags' => [[], <<<OUT
-            +--------------------+------+------------+---------------------------+--------------------------+
-            | Key                | Name | Is enabled | Expiration date           | Roles                    |
-            +--------------------+------+------------+---------------------------+--------------------------+
-            | valid_api_key      | -    | +++        | -                         | Admin                    |
-            +--------------------+------+------------+---------------------------+--------------------------+
-            | disabled_api_key   | -    | ---        | -                         | Admin                    |
-            +--------------------+------+------------+---------------------------+--------------------------+
-            | expired_api_key    | -    | ---        | {$expiredApiKeyDate} | Admin                    |
-            +--------------------+------+------------+---------------------------+--------------------------+
-            | author_api_key     | -    | +++        | -                         | Author only              |
-            +--------------------+------+------------+---------------------------+--------------------------+
-            | domain_api_key     | -    | +++        | -                         | Domain only: example.com |
-            +--------------------+------+------------+---------------------------+--------------------------+
-            | no_orphans_api_key | -    | +++        | -                         | No orphan visits         |
-            +--------------------+------+------------+---------------------------+--------------------------+
+            +--------------------+------------+---------------------------+--------------------------+
+            | Name               | Is enabled | Expiration date           | Roles                    |
+            +--------------------+------------+---------------------------+--------------------------+
+            | valid_api_key      | +++        | -                         | Admin                    |
+            +--------------------+------------+---------------------------+--------------------------+
+            | disabled_api_key   | ---        | -                         | Admin                    |
+            +--------------------+------------+---------------------------+--------------------------+
+            | expired_api_key    | ---        | {$expiredApiKeyDate} | Admin                    |
+            +--------------------+------------+---------------------------+--------------------------+
+            | author_api_key     | +++        | -                         | Author only              |
+            +--------------------+------------+---------------------------+--------------------------+
+            | domain_api_key     | +++        | -                         | Domain only: example.com |
+            +--------------------+------------+---------------------------+--------------------------+
+            | no_orphans_api_key | +++        | -                         | No orphan visits         |
+            +--------------------+------------+---------------------------+--------------------------+
 
             OUT];
         yield '-e' => [['-e'], $enabledOnlyOutput];
diff --git a/module/CLI/test/Command/Api/GenerateKeyCommandTest.php b/module/CLI/test/Command/Api/GenerateKeyCommandTest.php
index a15ad667..9c1d337e 100644
--- a/module/CLI/test/Command/Api/GenerateKeyCommandTest.php
+++ b/module/CLI/test/Command/Api/GenerateKeyCommandTest.php
@@ -36,7 +36,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([]);
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,
             <<<OUTPUT
-            +--------------------------------------+------+------------+---------------------------+-------+
-            | Key                                  | Name | Is enabled | Expiration date           | Roles |
-            +--------------------------------------+------+------------+---------------------------+-------+
-            | {$apiKey1->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,
             <<<OUTPUT
-            +--------------------------------------+------+-----------------+-------+
-            | Key                                  | Name | Expiration date | Roles |
-            +--------------------------------------+------+-----------------+-------+
-            | {$apiKey1->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,
             <<<OUTPUT
-            +--------------------------------------+------+-----------------+--------------------------+
-            | Key                                  | Name | Expiration date | Roles                    |
-            +--------------------------------------+------+-----------------+--------------------------+
-            | {$apiKey1->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,
             <<<OUTPUT
-            +--------------------------------------+---------------+-----------------+-------+
-            | Key                                  | Name          | Expiration date | Roles |
-            +--------------------------------------+---------------+-----------------+-------+
-            | {$apiKey1->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/Rest/src/ApiKey/Model/ApiKeyMeta.php b/module/Rest/src/ApiKey/Model/ApiKeyMeta.php
index 04b37214..21efe16a 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 "censored" 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/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php
index 4f7575c5..8a72b85e 100644
--- a/module/Rest/src/Entity/ApiKey.php
+++ b/module/Rest/src/Entity/ApiKey.php
@@ -15,6 +15,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
 {
     /**
@@ -42,6 +44,7 @@ class ApiKey extends AbstractEntity
      */
     public static function fromMeta(ApiKeyMeta $meta): self
     {
+//        $apiKey = new self(self::hashKey($meta->key), $meta->name, $meta->expirationDate);
         $apiKey = new self($meta->key, $meta->name, $meta->expirationDate);
         foreach ($meta->roleDefinitions as $roleDefinition) {
             $apiKey->registerRole($roleDefinition);
@@ -50,6 +53,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..9b731a55 100644
--- a/module/Rest/src/Service/ApiKeyService.php
+++ b/module/Rest/src/Service/ApiKeyService.php
@@ -10,11 +10,9 @@ 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)
     {
     }
 
@@ -48,11 +46,12 @@ class ApiKeyService implements ApiKeyServiceInterface
     {
         $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 +61,14 @@ 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->em->getRepository(ApiKey::class)->findBy($conditions);
     }
 
     private function getByKey(string $key): ApiKey|null
     {
-        /** @var ApiKey|null $apiKey */
-        $apiKey = $this->em->getRepository(ApiKey::class)->findOneBy([
+        return $this->em->getRepository(ApiKey::class)->findOneBy([
+//            'key' => ApiKey::hashKey($key),
             'key' => $key,
         ]);
-        return $apiKey;
     }
 }
diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php
index a799da27..d6f49fb6 100644
--- a/module/Rest/test/Service/ApiKeyServiceTest.php
+++ b/module/Rest/test/Service/ApiKeyServiceTest.php
@@ -18,6 +18,8 @@ use Shlinkio\Shlink\Rest\ApiKey\Repository\ApiKeyRepository;
 use Shlinkio\Shlink\Rest\Entity\ApiKey;
 use Shlinkio\Shlink\Rest\Service\ApiKeyService;
 
+use function substr;
+
 class ApiKeyServiceTest extends TestCase
 {
     private ApiKeyService $service;
@@ -40,12 +42,14 @@ class ApiKeyServiceTest extends TestCase
         $this->em->expects($this->once())->method('flush');
         $this->em->expects($this->once())->method('persist')->with($this->isInstanceOf(ApiKey::class));
 
-        $key = $this->service->create(
-            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));
         }

From 9f6975119eb0a947e51ef8e8526d723c1b632db5 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
Date: Tue, 5 Nov 2024 22:52:01 +0100
Subject: [PATCH 03/13] Show only API key name in short URLs list

---
 .../Command/ShortUrl/ListShortUrlsCommand.php |  13 +-
 .../ShortUrl/ListShortUrlsCommandTest.php     | 119 +++++++++---------
 module/Core/src/ShortUrl/Entity/ShortUrl.php  |   5 +
 3 files changed, 69 insertions(+), 68 deletions(-)

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/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,
+            <<<OUTPUT
+            +------------+-------+-------------+-----------------+---------------------------+--------------+---------------+
+            | Short Code | Title | Short URL   | Long URL        | Date created              | Visits count | Tags          |
+            +------------+-------+-------------+-----------------+---------------------------+--------------+---------------+
+            | {$shortCode}      |       | http:/{$shortCode} | https://foo.com | {$created} | 0            | foo, bar, baz |
+            +------------+-------+-------------+-------------- Page 1 of 1 ------------------+--------------+---------------+
+            OUTPUT,
+            $shortUrl,
         ];
         yield 'domain only' => [
             ['--show-domain' => true],
-            ['| Domain', '| DEFAULT'],
-            ['| Tags    ', '| foo, bar, baz', '| API Key    ', '| API Key Name |', $key, '| my api key'],
-            $apiKey,
+            <<<OUTPUT
+            +------------+-------+-------------+-----------------+---------------------------+--------------+---------+
+            | Short Code | Title | Short URL   | Long URL        | Date created              | Visits count | Domain  |
+            +------------+-------+-------------+-----------------+---------------------------+--------------+---------+
+            | {$shortCode}      |       | http:/{$shortCode} | https://foo.com | {$created} | 0            | DEFAULT |
+            +------------+-------+-------------+----------- Page 1 of 1 ---------------------+--------------+---------+
+            OUTPUT,
+            $shortUrl,
         ];
         yield 'api key only' => [
             ['--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,
+            <<<OUTPUT
+            +------------+-------+-------------+-----------------+---------------------------+--------------+--------------+
+            | Short Code | Title | Short URL   | Long URL        | Date created              | Visits count | API Key Name |
+            +------------+-------+-------------+-----------------+---------------------------+--------------+--------------+
+            | {$shortCode}      |       | http:/{$shortCode} | https://foo.com | {$created} | 0            | my api key   |
+            +------------+-------+-------------+------------- Page 1 of 1 -------------------+--------------+--------------+
+            OUTPUT,
+            $shortUrl,
         ];
         yield 'tags and api key' => [
             ['--show-tags' => true, '--show-api-key' => true],
-            ['| API Key    ', '| Tags    ', '| foo, bar, baz', $key],
-            ['| API Key Name |', '| my api key'],
-            $apiKey,
+            <<<OUTPUT
+            +------------+-------+-------------+-----------------+---------------------------+--------------+---------------+--------------+
+            | Short Code | Title | Short URL   | Long URL        | Date created              | Visits count | Tags          | API Key Name |
+            +------------+-------+-------------+-----------------+---------------------------+--------------+---------------+--------------+
+            | {$shortCode}      |       | http:/{$shortCode} | https://foo.com | {$created} | 0            | foo, bar, baz | my api key   |
+            +------------+-------+-------------+-----------------+--- Page 1 of 1 -----------+--------------+---------------+--------------+
+            OUTPUT,
+            $shortUrl,
         ];
         yield 'tags and domain' => [
             ['--show-tags' => true, '--show-domain' => true],
-            ['| Tags    ', '| foo, bar, baz', '| Domain', '| DEFAULT'],
-            ['| API Key Name |', '| my api key'],
-            $apiKey,
+            <<<OUTPUT
+            +------------+-------+-------------+-----------------+---------------------------+--------------+---------------+---------+
+            | Short Code | Title | Short URL   | Long URL        | Date created              | Visits count | Tags          | Domain  |
+            +------------+-------+-------------+-----------------+---------------------------+--------------+---------------+---------+
+            | {$shortCode}      |       | http:/{$shortCode} | https://foo.com | {$created} | 0            | foo, bar, baz | DEFAULT |
+            +------------+-------+-------------+-----------------+- Page 1 of 1 -------------+--------------+---------------+---------+
+            OUTPUT,
+            $shortUrl,
         ];
         yield 'all' => [
-            ['--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],
+            <<<OUTPUT
+            +------------+-------+-------------+-----------------+---------------------------+--------------+---------------+---------+--------------+
+            | Short Code | Title | Short URL   | Long URL        | Date created              | Visits count | Tags          | Domain  | API Key Name |
+            +------------+-------+-------------+-----------------+---------------------------+--------------+---------------+---------+--------------+
+            | {$shortCode}      |       | http:/{$shortCode} | https://foo.com | {$created} | 0            | foo, bar, baz | DEFAULT | my api key   |
+            +------------+-------+-------------+-----------------+-------- Page 1 of 1 ------+--------------+---------------+---------+--------------+
+            OUTPUT,
+            $shortUrl,
         ];
+        // phpcs:enable
     }
 
     #[Test, DataProvider('provideArgs')]
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;

From 1b9c8377ae6d9c61fd4a488f3c3d822cf37e47bd Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
Date: Tue, 5 Nov 2024 23:23:06 +0100
Subject: [PATCH 04/13] Hash existing API keys, and do checks against the hash

---
 .../Core/migrations/Version20241105215309.php | 45 +++++++++++++++++++
 module/Rest/src/Entity/ApiKey.php             |  3 +-
 module/Rest/src/Service/ApiKeyService.php     |  3 +-
 .../Repository/ApiKeyRepositoryTest.php       |  4 +-
 .../Rest/test/Service/ApiKeyServiceTest.php   | 16 +++++--
 5 files changed, 61 insertions(+), 10 deletions(-)
 create mode 100644 module/Core/migrations/Version20241105215309.php

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 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkMigrations;
+
+use Doctrine\DBAL\Platforms\MySQLPlatform;
+use Doctrine\DBAL\Schema\Schema;
+use Doctrine\Migrations\AbstractMigration;
+
+use function hash;
+
+/**
+ * Hash API keys as SHA256
+ */
+final class Version20241105215309 extends AbstractMigration
+{
+    public function up(Schema $schema): void
+    {
+        $keyColumnName = $this->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/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php
index 8a72b85e..32f8fff4 100644
--- a/module/Rest/src/Entity/ApiKey.php
+++ b/module/Rest/src/Entity/ApiKey.php
@@ -44,8 +44,7 @@ class ApiKey extends AbstractEntity
      */
     public static function fromMeta(ApiKeyMeta $meta): self
     {
-//        $apiKey = new self(self::hashKey($meta->key), $meta->name, $meta->expirationDate);
-        $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);
         }
diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php
index 9b731a55..ca43a81f 100644
--- a/module/Rest/src/Service/ApiKeyService.php
+++ b/module/Rest/src/Service/ApiKeyService.php
@@ -67,8 +67,7 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
     private function getByKey(string $key): ApiKey|null
     {
         return $this->em->getRepository(ApiKey::class)->findOneBy([
-//            'key' => ApiKey::hashKey($key),
-            'key' => $key,
+            'key' => ApiKey::hashKey($key),
         ]);
     }
 }
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/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php
index d6f49fb6..b081b99a 100644
--- a/module/Rest/test/Service/ApiKeyServiceTest.php
+++ b/module/Rest/test/Service/ApiKeyServiceTest.php
@@ -74,7 +74,9 @@ class ApiKeyServiceTest extends TestCase
     #[Test, DataProvider('provideInvalidApiKeys')]
     public function checkReturnsFalseForInvalidApiKeys(ApiKey|null $invalidKey): void
     {
-        $this->repo->expects($this->once())->method('findOneBy')->with(['key' => '12345'])->willReturn($invalidKey);
+        $this->repo->expects($this->once())->method('findOneBy')->with(['key' => ApiKey::hashKey('12345')])->willReturn(
+            $invalidKey,
+        );
         $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo);
 
         $result = $this->service->check('12345');
@@ -97,7 +99,9 @@ class ApiKeyServiceTest extends TestCase
     {
         $apiKey = ApiKey::create();
 
-        $this->repo->expects($this->once())->method('findOneBy')->with(['key' => '12345'])->willReturn($apiKey);
+        $this->repo->expects($this->once())->method('findOneBy')->with(['key' => ApiKey::hashKey('12345')])->willReturn(
+            $apiKey,
+        );
         $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo);
 
         $result = $this->service->check('12345');
@@ -109,7 +113,9 @@ class ApiKeyServiceTest extends TestCase
     #[Test]
     public function disableThrowsExceptionWhenNoApiKeyIsFound(): void
     {
-        $this->repo->expects($this->once())->method('findOneBy')->with(['key' => '12345'])->willReturn(null);
+        $this->repo->expects($this->once())->method('findOneBy')->with(['key' => ApiKey::hashKey('12345')])->willReturn(
+            null,
+        );
         $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo);
 
         $this->expectException(InvalidArgumentException::class);
@@ -121,7 +127,9 @@ class ApiKeyServiceTest extends TestCase
     public function disableReturnsDisabledApiKeyWhenFound(): void
     {
         $key = ApiKey::create();
-        $this->repo->expects($this->once())->method('findOneBy')->with(['key' => '12345'])->willReturn($key);
+        $this->repo->expects($this->once())->method('findOneBy')->with(['key' => ApiKey::hashKey('12345')])->willReturn(
+            $key,
+        );
         $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo);
         $this->em->expects($this->once())->method('flush');
 

From f6d70c599e6b6be45c0756e0b84692436d81b203 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
Date: Tue, 5 Nov 2024 23:31:10 +0100
Subject: [PATCH 05/13] Make name required in ApiKey entity

---
 module/Rest/src/ApiKey/Model/ApiKeyMeta.php | 2 +-
 module/Rest/src/Entity/ApiKey.php           | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/module/Rest/src/ApiKey/Model/ApiKeyMeta.php b/module/Rest/src/ApiKey/Model/ApiKeyMeta.php
index 21efe16a..66d7d889 100644
--- a/module/Rest/src/ApiKey/Model/ApiKeyMeta.php
+++ b/module/Rest/src/ApiKey/Model/ApiKeyMeta.php
@@ -41,7 +41,7 @@ final readonly class ApiKeyMeta
 
         // If a name was not provided, fall back to the key
         if (empty($name)) {
-            // If the key was auto-generated, fall back to a "censored" version of the UUID, otherwise simply use the
+            // 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))
diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php
index 32f8fff4..17461d12 100644
--- a/module/Rest/src/Entity/ApiKey.php
+++ b/module/Rest/src/Entity/ApiKey.php
@@ -24,7 +24,7 @@ class ApiKey extends AbstractEntity
      */
     private function __construct(
         public readonly string $key,
-        public readonly string|null $name = null,
+        public readonly string $name,
         public readonly Chronos|null $expirationDate = null,
         private bool $enabled = true,
         private Collection $roles = new ArrayCollection(),

From bd73362c94a204eca1aa0d594021ffd7885329be Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
Date: Wed, 6 Nov 2024 20:10:06 +0100
Subject: [PATCH 06/13] Update api-key:disable command to allow passing a name

---
 .../CLI/src/Command/Api/DisableKeyCommand.php | 74 +++++++++++++--
 .../Command/Api/DisableKeyCommandTest.php     | 90 +++++++++++++++++--
 module/Rest/src/Entity/ApiKey.php             |  7 --
 module/Rest/src/Service/ApiKeyService.php     | 20 ++++-
 .../src/Service/ApiKeyServiceInterface.php    |  8 +-
 .../Rest/test/Service/ApiKeyServiceTest.php   | 26 +++---
 6 files changed, 188 insertions(+), 37 deletions(-)

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 = <<<HELP
+        The <info>%command.name%</info> 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.
+
+            <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);
-            $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
     {

From 6f95acc2024bdc07afedd4650a7c83b9d69b3001 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
Date: Thu, 7 Nov 2024 09:34:42 +0100
Subject: [PATCH 07/13] Inject ApiKeyRepository in ApiKeyService

---
 module/Rest/config/dependencies.config.php    |  5 +++-
 module/Rest/src/Service/ApiKeyService.php     | 27 +++++++++----------
 .../Rest/test/Service/ApiKeyServiceTest.php   | 15 +++--------
 3 files changed, 21 insertions(+), 26 deletions(-)

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/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php
index 66cb1b18..e1a2f57a 100644
--- a/module/Rest/src/Service/ApiKeyService.php
+++ b/module/Rest/src/Service/ApiKeyService.php
@@ -12,7 +12,7 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey;
 
 readonly class ApiKeyService implements ApiKeyServiceInterface
 {
-    public function __construct(private EntityManagerInterface $em)
+    public function __construct(private EntityManagerInterface $em, private ApiKeyRepositoryInterface $repo)
     {
     }
 
@@ -28,14 +28,12 @@ readonly 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);
     }
 
@@ -44,9 +42,7 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
      */
     public function disableByName(string $apiKeyName): ApiKey
     {
-        return $this->disableApiKey($this->em->getRepository(ApiKey::class)->findOneBy([
-            'name' => $apiKeyName,
-        ]));
+        return $this->disableApiKey($this->findByName($apiKeyName));
     }
 
     /**
@@ -54,7 +50,7 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
      */
     public function disableByKey(string $key): ApiKey
     {
-        return $this->disableApiKey($this->getByKey($key));
+        return $this->disableApiKey($this->findByKey($key));
     }
 
     private function disableApiKey(ApiKey|null $apiKey): ApiKey
@@ -75,13 +71,16 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
     public function listKeys(bool $enabledOnly = false): array
     {
         $conditions = $enabledOnly ? ['enabled' => true] : [];
-        return $this->em->getRepository(ApiKey::class)->findBy($conditions);
+        return $this->repo->findBy($conditions);
     }
 
-    private function getByKey(string $key): ApiKey|null
+    private function findByKey(string $key): ApiKey|null
     {
-        return $this->em->getRepository(ApiKey::class)->findOneBy([
-            'key' => ApiKey::hashKey($key),
-        ]);
+        return $this->repo->findOneBy(['key' => ApiKey::hashKey($key)]);
+    }
+
+    private function findByName(string $name): ApiKey|null
+    {
+        return $this->repo->findOneBy(['name' => $name]);
     }
 }
diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php
index ba57fec4..d3c62af2 100644
--- a/module/Rest/test/Service/ApiKeyServiceTest.php
+++ b/module/Rest/test/Service/ApiKeyServiceTest.php
@@ -14,7 +14,7 @@ use Shlinkio\Shlink\Common\Exception\InvalidArgumentException;
 use Shlinkio\Shlink\Core\Domain\Entity\Domain;
 use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta;
 use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition;
-use Shlinkio\Shlink\Rest\ApiKey\Repository\ApiKeyRepository;
+use Shlinkio\Shlink\Rest\ApiKey\Repository\ApiKeyRepositoryInterface;
 use Shlinkio\Shlink\Rest\Entity\ApiKey;
 use Shlinkio\Shlink\Rest\Service\ApiKeyService;
 
@@ -24,13 +24,13 @@ 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);
     }
 
     /**
@@ -77,7 +77,6 @@ class ApiKeyServiceTest extends TestCase
         $this->repo->expects($this->once())->method('findOneBy')->with(['key' => ApiKey::hashKey('12345')])->willReturn(
             $invalidKey,
         );
-        $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo);
 
         $result = $this->service->check('12345');
 
@@ -102,7 +101,6 @@ class ApiKeyServiceTest extends TestCase
         $this->repo->expects($this->once())->method('findOneBy')->with(['key' => ApiKey::hashKey('12345')])->willReturn(
             $apiKey,
         );
-        $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo);
 
         $result = $this->service->check('12345');
 
@@ -114,7 +112,6 @@ class ApiKeyServiceTest extends TestCase
     public function disableThrowsExceptionWhenNoApiKeyIsFound(string $disableMethod, array $findOneByArg): void
     {
         $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);
 
@@ -126,7 +123,6 @@ class ApiKeyServiceTest extends TestCase
     {
         $key = ApiKey::create();
         $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());
@@ -147,7 +143,6 @@ class ApiKeyServiceTest extends TestCase
         $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();
 
@@ -160,7 +155,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);
 
@@ -171,7 +165,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');
 

From 4c1ff72438e66fb6a7a746000d526465b12e84c0 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
Date: Thu, 7 Nov 2024 09:55:06 +0100
Subject: [PATCH 08/13] Add method to check if an API exists for a given name

---
 .../Repository/EntityRepositoryInterface.php  | 20 +++++++++++++++++++
 .../Repository/ApiKeyRepositoryInterface.php  |  6 +++---
 module/Rest/src/Service/ApiKeyService.php     |  9 ++++++---
 .../src/Service/ApiKeyServiceInterface.php    |  5 +++++
 .../Rest/test/Service/ApiKeyServiceTest.php   | 12 +++++++++++
 5 files changed, 46 insertions(+), 6 deletions(-)
 create mode 100644 module/Core/src/Repository/EntityRepositoryInterface.php

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 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\Core\Repository;
+
+use Doctrine\Persistence\ObjectRepository;
+
+/**
+ * @template T of object
+ * @extends ObjectRepository<T>
+ */
+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/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<ApiKey>
+ * @extends EntityRepositoryInterface<ApiKey>
  */
-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/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php
index e1a2f57a..4b786c6d 100644
--- a/module/Rest/src/Service/ApiKeyService.php
+++ b/module/Rest/src/Service/ApiKeyService.php
@@ -42,7 +42,7 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
      */
     public function disableByName(string $apiKeyName): ApiKey
     {
-        return $this->disableApiKey($this->findByName($apiKeyName));
+        return $this->disableApiKey($this->repo->findOneBy(['name' => $apiKeyName]));
     }
 
     /**
@@ -79,8 +79,11 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
         return $this->repo->findOneBy(['key' => ApiKey::hashKey($key)]);
     }
 
-    private function findByName(string $name): ApiKey|null
+    /**
+     * @inheritDoc
+     */
+    public function existsWithName(string $apiKeyName): bool
     {
-        return $this->repo->findOneBy(['name' => $name]);
+        return $this->repo->count(['name' => $apiKeyName]) > 0;
     }
 }
diff --git a/module/Rest/src/Service/ApiKeyServiceInterface.php b/module/Rest/src/Service/ApiKeyServiceInterface.php
index 1fefc5f4..73773fba 100644
--- a/module/Rest/src/Service/ApiKeyServiceInterface.php
+++ b/module/Rest/src/Service/ApiKeyServiceInterface.php
@@ -31,4 +31,9 @@ interface ApiKeyServiceInterface
      * @return ApiKey[]
      */
     public function listKeys(bool $enabledOnly = false): array;
+
+    /**
+     * Check if an API key exists for provided name
+     */
+    public function existsWithName(string $apiKeyName): bool;
 }
diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php
index d3c62af2..e304a651 100644
--- a/module/Rest/test/Service/ApiKeyServiceTest.php
+++ b/module/Rest/test/Service/ApiKeyServiceTest.php
@@ -8,6 +8,7 @@ use Cake\Chronos\Chronos;
 use Doctrine\ORM\EntityManager;
 use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\Attributes\Test;
+use PHPUnit\Framework\Attributes\TestWith;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use Shlinkio\Shlink\Common\Exception\InvalidArgumentException;
@@ -176,4 +177,15 @@ class ApiKeyServiceTest extends TestCase
         yield 'first api key' => [ApiKey::create()];
         yield 'existing api keys' => [null];
     }
+
+    #[Test]
+    #[TestWith([0, false])]
+    #[TestWith([1, true])]
+    #[TestWith([27, true])]
+    public function existsWithNameCountsEntriesInRepository(int $count, bool $expected): void
+    {
+        $name = 'the_key';
+        $this->repo->expects($this->once())->method('count')->with(['name' => $name])->willReturn($count);
+        self::assertEquals($this->service->existsWithName($name), $expected);
+    }
 }

From 9e6f129de6fe066525083ef3e23e1095f54cc3b5 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
Date: Thu, 7 Nov 2024 14:52:06 +0100
Subject: [PATCH 09/13] Make sure a unique name is required by api-key:generate
 command

---
 .../src/Command/Api/GenerateKeyCommand.php    | 15 ++++++++-----
 .../Command/Api/GenerateKeyCommandTest.php    | 22 ++++++++++++++++++-
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/module/CLI/src/Command/Api/GenerateKeyCommand.php b/module/CLI/src/Command/Api/GenerateKeyCommand.php
index a6b8bad0..3a1432ac 100644
--- a/module/CLI/src/Command/Api/GenerateKeyCommand.php
+++ b/module/CLI/src/Command/Api/GenerateKeyCommand.php
@@ -100,16 +100,22 @@ class GenerateKeyCommand extends Command
 
     protected function execute(InputInterface $input, OutputInterface $output): int
     {
+        $io = new SymfonyStyle($input, $output);
         $expirationDate = $input->getOption('expiration-date');
-
         $apiKeyMeta = ApiKeyMeta::fromParams(
             name: $input->getOption('name'),
             expirationDate: isset($expirationDate) ? Chronos::parse($expirationDate) : null,
             roleDefinitions: $this->roleResolver->determineRoles($input),
         );
-        $apiKey = $this->apiKeyService->create($apiKeyMeta);
 
-        $io = new SymfonyStyle($input, $output);
+        if ($this->apiKeyService->existsWithName($apiKeyMeta->name)) {
+            $io->warning(
+                sprintf('An API key with name "%s" already exists. Try with a different ome', $apiKeyMeta->name),
+            );
+            return ExitCode::EXIT_WARNING;
+        }
+
+        $apiKey = $this->apiKeyService->create($apiKeyMeta);
         $io->success(sprintf('Generated API key: "%s"', $apiKeyMeta->key));
 
         if ($input->isInteractive()) {
@@ -120,8 +126,7 @@ class GenerateKeyCommand extends Command
             ShlinkTable::default($io)->render(
                 ['Role name', 'Role metadata'],
                 $apiKey->mapRoles(fn (Role $role, array $meta) => [$role->value, arrayToString($meta, indentSize: 0)]),
-                null,
-                'Roles',
+                headerTitle: 'Roles',
             );
         }
 
diff --git a/module/CLI/test/Command/Api/GenerateKeyCommandTest.php b/module/CLI/test/Command/Api/GenerateKeyCommandTest.php
index 9c1d337e..10633b9a 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;
@@ -64,8 +65,27 @@ 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);
+    }
+
+    #[Test]
+    public function warningIsPrintedIfProvidedNameAlreadyExists(): void
+    {
+        $name = 'The API key';
+
+        $this->apiKeyService->expects($this->never())->method('create');
+        $this->apiKeyService->expects($this->once())->method('existsWithName')->with($name)->willReturn(true);
+
+        $exitCode = $this->commandTester->execute([
+            '--name' => $name,
+        ]);
+        $output = $this->commandTester->getDisplay();
+
+        self::assertEquals(ExitCode::EXIT_WARNING, $exitCode);
+        self::assertStringContainsString('An API key with name "The API key" already exists.', $output);
     }
 }

From a661d051000d2087154f2875098290923e90787e Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
Date: Fri, 8 Nov 2024 08:25:07 +0100
Subject: [PATCH 10/13] Allow API keys to be renamed

---
 .../CLI/src/Command/Tag/RenameTagCommand.php  |  4 +-
 .../test/Command/Tag/RenameTagCommandTest.php |  6 +-
 .../src/Exception/TagConflictException.php    |  4 +-
 .../TagRenaming.php => Model/Renaming.php}    |  6 +-
 module/Core/src/Tag/TagService.php            |  4 +-
 module/Core/src/Tag/TagServiceInterface.php   |  4 +-
 .../Exception/TagConflictExceptionTest.php    |  4 +-
 module/Core/test/Tag/TagServiceTest.php       | 10 +--
 .../Rest/src/Action/Tag/UpdateTagAction.php   |  4 +-
 module/Rest/src/Entity/ApiKey.php             |  3 +-
 module/Rest/src/Service/ApiKeyService.php     | 41 ++++++++++--
 .../src/Service/ApiKeyServiceInterface.php    |  6 ++
 .../test/Action/Tag/UpdateTagActionTest.php   |  4 +-
 .../Rest/test/Service/ApiKeyServiceTest.php   | 63 +++++++++++++++++++
 14 files changed, 132 insertions(+), 31 deletions(-)
 rename module/Core/src/{Tag/Model/TagRenaming.php => Model/Renaming.php} (86%)

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/Command/Tag/RenameTagCommandTest.php b/module/CLI/test/Command/Tag/RenameTagCommandTest.php
index 296926b8..e7fb630d 100644
--- a/module/CLI/test/Command/Tag/RenameTagCommandTest.php
+++ b/module/CLI/test/Command/Tag/RenameTagCommandTest.php
@@ -9,8 +9,8 @@ use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use Shlinkio\Shlink\CLI\Command\Tag\RenameTagCommand;
 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\TagServiceInterface;
 use ShlinkioTest\Shlink\CLI\Util\CliTestUtils;
 use Symfony\Component\Console\Tester\CommandTester;
@@ -32,7 +32,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),
         )->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/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/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/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/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php
index fea06a1d..c9cdd3a6 100644
--- a/module/Rest/src/Entity/ApiKey.php
+++ b/module/Rest/src/Entity/ApiKey.php
@@ -23,7 +23,8 @@ class ApiKey extends AbstractEntity
      */
     private function __construct(
         public readonly string $key,
-        public readonly string $name,
+        // 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(),
diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php
index 4b786c6d..38ed004d 100644
--- a/module/Rest/src/Service/ApiKeyService.php
+++ b/module/Rest/src/Service/ApiKeyService.php
@@ -6,10 +6,13 @@ 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;
+
 readonly class ApiKeyService implements ApiKeyServiceInterface
 {
     public function __construct(private EntityManagerInterface $em, private ApiKeyRepositoryInterface $repo)
@@ -74,11 +77,6 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
         return $this->repo->findBy($conditions);
     }
 
-    private function findByKey(string $key): ApiKey|null
-    {
-        return $this->repo->findOneBy(['key' => ApiKey::hashKey($key)]);
-    }
-
     /**
      * @inheritDoc
      */
@@ -86,4 +84,37 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
     {
         return $this->repo->count(['name' => $apiKeyName]) > 0;
     }
+
+    /**
+     * @inheritDoc
+     */
+    public function renameApiKey(Renaming $apiKeyRenaming): ApiKey
+    {
+        $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)]);
+    }
 }
diff --git a/module/Rest/src/Service/ApiKeyServiceInterface.php b/module/Rest/src/Service/ApiKeyServiceInterface.php
index 73773fba..4197b3fd 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;
 
@@ -36,4 +37,9 @@ interface ApiKeyServiceInterface
      * Check if an API key exists for provided name
      */
     public function existsWithName(string $apiKeyName): bool;
+
+    /**
+     * @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/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/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php
index e304a651..f439e5f9 100644
--- a/module/Rest/test/Service/ApiKeyServiceTest.php
+++ b/module/Rest/test/Service/ApiKeyServiceTest.php
@@ -13,6 +13,7 @@ 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\ApiKeyRepositoryInterface;
@@ -188,4 +189,66 @@ class ApiKeyServiceTest extends TestCase
         $this->repo->expects($this->once())->method('count')->with(['name' => $name])->willReturn($count);
         self::assertEquals($this->service->existsWithName($name), $expected);
     }
+
+    #[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);
+    }
 }

From b08c498b13ef247913ee024c32dd0256b54024ba Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
Date: Fri, 8 Nov 2024 08:47:49 +0100
Subject: [PATCH 11/13] Create command to rename API keys

---
 module/CLI/config/cli.config.php              |  1 +
 module/CLI/config/dependencies.config.php     |  2 +
 .../src/Command/Api/RenameApiKeyCommand.php   | 77 +++++++++++++++++++
 module/Rest/src/Service/ApiKeyService.php     |  3 +
 4 files changed, 83 insertions(+)
 create mode 100644 module/CLI/src/Command/Api/RenameApiKeyCommand.php

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/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 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Shlinkio\Shlink\CLI\Command\Api;
+
+use Shlinkio\Shlink\CLI\Util\ExitCode;
+use Shlinkio\Shlink\Core\Exception\InvalidArgumentException;
+use Shlinkio\Shlink\Core\Model\Renaming;
+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\Output\OutputInterface;
+use Symfony\Component\Console\Style\SymfonyStyle;
+
+use function Shlinkio\Shlink\Core\ArrayUtils\map;
+
+class RenameApiKeyCommand extends Command
+{
+    public const NAME = 'api-key:rename';
+
+    public function __construct(private readonly ApiKeyServiceInterface $apiKeyService)
+    {
+        parent::__construct();
+    }
+
+    protected function configure(): void
+    {
+        $this
+            ->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/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php
index 38ed004d..66876204 100644
--- a/module/Rest/src/Service/ApiKeyService.php
+++ b/module/Rest/src/Service/ApiKeyService.php
@@ -87,6 +87,9 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
 
     /**
      * @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
     {

From 6f837b3b91c5a676315b228d226cc9b590d9d405 Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
Date: Fri, 8 Nov 2024 09:03:50 +0100
Subject: [PATCH 12/13] Move logic to determine if a new key has a duplicated
 name to the APiKeyService

---
 CHANGELOG.md                                  | 12 +++++++-
 .../src/Command/Api/GenerateKeyCommand.php    |  7 -----
 .../Command/Api/GenerateKeyCommandTest.php    | 17 -----------
 module/Rest/src/Service/ApiKeyService.php     | 19 +++++++------
 .../src/Service/ApiKeyServiceInterface.php    |  5 ----
 .../Rest/test/Service/ApiKeyServiceTest.php   | 28 +++++++++++--------
 6 files changed, 38 insertions(+), 50 deletions(-)

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/src/Command/Api/GenerateKeyCommand.php b/module/CLI/src/Command/Api/GenerateKeyCommand.php
index 3a1432ac..9fc0bb1d 100644
--- a/module/CLI/src/Command/Api/GenerateKeyCommand.php
+++ b/module/CLI/src/Command/Api/GenerateKeyCommand.php
@@ -108,13 +108,6 @@ class GenerateKeyCommand extends Command
             roleDefinitions: $this->roleResolver->determineRoles($input),
         );
 
-        if ($this->apiKeyService->existsWithName($apiKeyMeta->name)) {
-            $io->warning(
-                sprintf('An API key with name "%s" already exists. Try with a different ome', $apiKeyMeta->name),
-            );
-            return ExitCode::EXIT_WARNING;
-        }
-
         $apiKey = $this->apiKeyService->create($apiKeyMeta);
         $io->success(sprintf('Generated API key: "%s"', $apiKeyMeta->key));
 
diff --git a/module/CLI/test/Command/Api/GenerateKeyCommandTest.php b/module/CLI/test/Command/Api/GenerateKeyCommandTest.php
index 10633b9a..1eb977bf 100644
--- a/module/CLI/test/Command/Api/GenerateKeyCommandTest.php
+++ b/module/CLI/test/Command/Api/GenerateKeyCommandTest.php
@@ -71,21 +71,4 @@ class GenerateKeyCommandTest extends TestCase
 
         self::assertEquals(ExitCode::EXIT_SUCCESS, $exitCode);
     }
-
-    #[Test]
-    public function warningIsPrintedIfProvidedNameAlreadyExists(): void
-    {
-        $name = 'The API key';
-
-        $this->apiKeyService->expects($this->never())->method('create');
-        $this->apiKeyService->expects($this->once())->method('existsWithName')->with($name)->willReturn(true);
-
-        $exitCode = $this->commandTester->execute([
-            '--name' => $name,
-        ]);
-        $output = $this->commandTester->getDisplay();
-
-        self::assertEquals(ExitCode::EXIT_WARNING, $exitCode);
-        self::assertStringContainsString('An API key with name "The API key" already exists.', $output);
-    }
 }
diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php
index 66876204..f517dde5 100644
--- a/module/Rest/src/Service/ApiKeyService.php
+++ b/module/Rest/src/Service/ApiKeyService.php
@@ -21,7 +21,13 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
 
     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();
@@ -77,14 +83,6 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
         return $this->repo->findBy($conditions);
     }
 
-    /**
-     * @inheritDoc
-     */
-    public function existsWithName(string $apiKeyName): bool
-    {
-        return $this->repo->count(['name' => $apiKeyName]) > 0;
-    }
-
     /**
      * @inheritDoc
      * @todo This method should be transactional and to a SELECT ... FROM UPDATE when checking if the new name exists,
@@ -120,4 +118,9 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
     {
         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 4197b3fd..c42505b7 100644
--- a/module/Rest/src/Service/ApiKeyServiceInterface.php
+++ b/module/Rest/src/Service/ApiKeyServiceInterface.php
@@ -33,11 +33,6 @@ interface ApiKeyServiceInterface
      */
     public function listKeys(bool $enabledOnly = false): array;
 
-    /**
-     * Check if an API key exists for provided name
-     */
-    public function existsWithName(string $apiKeyName): bool;
-
     /**
      * @throws InvalidArgumentException If an API key with oldName does not exist, or newName is in use by another one
      */
diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php
index f439e5f9..adecfbd9 100644
--- a/module/Rest/test/Service/ApiKeyServiceTest.php
+++ b/module/Rest/test/Service/ApiKeyServiceTest.php
@@ -8,7 +8,6 @@ use Cake\Chronos\Chronos;
 use Doctrine\ORM\EntityManager;
 use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\Attributes\Test;
-use PHPUnit\Framework\Attributes\TestWith;
 use PHPUnit\Framework\MockObject\MockObject;
 use PHPUnit\Framework\TestCase;
 use Shlinkio\Shlink\Common\Exception\InvalidArgumentException;
@@ -41,6 +40,9 @@ 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));
 
@@ -73,6 +75,19 @@ 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
     {
@@ -179,17 +194,6 @@ class ApiKeyServiceTest extends TestCase
         yield 'existing api keys' => [null];
     }
 
-    #[Test]
-    #[TestWith([0, false])]
-    #[TestWith([1, true])]
-    #[TestWith([27, true])]
-    public function existsWithNameCountsEntriesInRepository(int $count, bool $expected): void
-    {
-        $name = 'the_key';
-        $this->repo->expects($this->once())->method('count')->with(['name' => $name])->willReturn($count);
-        self::assertEquals($this->service->existsWithName($name), $expected);
-    }
-
     #[Test]
     public function renameApiKeyThrowsExceptionIfApiKeyIsNotFound(): void
     {

From 7e573bdb9b24bf06d7ad41afbd823cff13733b9c Mon Sep 17 00:00:00 2001
From: Alejandro Celaya <alejandrocelaya@gmail.com>
Date: Fri, 8 Nov 2024 09:58:02 +0100
Subject: [PATCH 13/13] Add tests for RenameApiKeyCOmmand and ApiKeyMeta

---
 .../Command/Api/RenameApiKeyCommandTest.php   | 83 +++++++++++++++++++
 .../Rest/test/ApiKey/Model/ApiKeyMetaTest.php | 35 ++++++++
 2 files changed, 118 insertions(+)
 create mode 100644 module/CLI/test/Command/Api/RenameApiKeyCommandTest.php
 create mode 100644 module/Rest/test/ApiKey/Model/ApiKeyMetaTest.php

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 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\CLI\Command\Api;
+
+use PHPUnit\Framework\Attributes\Test;
+use PHPUnit\Framework\MockObject\MockObject;
+use PHPUnit\Framework\TestCase;
+use Shlinkio\Shlink\CLI\Command\Api\RenameApiKeyCommand;
+use Shlinkio\Shlink\Core\Model\Renaming;
+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;
+
+class RenameApiKeyCommandTest extends TestCase
+{
+    private CommandTester $commandTester;
+    private MockObject & ApiKeyServiceInterface $apiKeyService;
+
+    protected function setUp(): void
+    {
+        $this->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/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 @@
+<?php
+
+declare(strict_types=1);
+
+namespace ShlinkioTest\Shlink\Rest\ApiKey\Model;
+
+use PHPUnit\Framework\Attributes\DataProvider;
+use PHPUnit\Framework\Attributes\Test;
+use PHPUnit\Framework\TestCase;
+use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta;
+
+use function sprintf;
+use function substr;
+
+class ApiKeyMetaTest extends TestCase
+{
+    #[Test, DataProvider('provideNames')]
+    public function nameIsInferredWhenNotProvided(string|null $key, string|null $name, callable $getExpectedName): void
+    {
+        $meta = ApiKeyMeta::fromParams(key: $key, name: $name);
+        $expectedName = $getExpectedName($meta);
+
+        self::assertEquals($expectedName, $meta->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),
+        )];
+    }
+}