From a094be2b9ef7f85b3ef8878140c28fcda19cbd11 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 5 Nov 2024 11:26:39 +0100 Subject: [PATCH] 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 = << [[], << [['-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, <<key} | - | --- | - | Admin | - +--------------------------------------+------+------------+---------------------------+-------+ - | {$apiKey2->key} | - | --- | 2020-01-01T00:00:00+00:00 | Admin | - +--------------------------------------+------+------------+---------------------------+-------+ - | {$apiKey3->key} | - | +++ | - | Admin | - +--------------------------------------+------+------------+---------------------------+-------+ + +--------------------------------------+------------+---------------------------+-------+ + | Name | Is enabled | Expiration date | Roles | + +--------------------------------------+------------+---------------------------+-------+ + | {$apiKey1->name} | --- | - | Admin | + +--------------------------------------+------------+---------------------------+-------+ + | {$apiKey2->name} | --- | 2020-01-01T00:00:00+00:00 | Admin | + +--------------------------------------+------------+---------------------------+-------+ + | {$apiKey3->name} | +++ | - | Admin | + +--------------------------------------+------------+---------------------------+-------+ OUTPUT, ]; @@ -68,13 +68,13 @@ class ListKeysCommandTest extends TestCase [$apiKey1 = ApiKey::create()->disable(), $apiKey2 = ApiKey::create()], true, <<key} | - | - | Admin | - +--------------------------------------+------+-----------------+-------+ - | {$apiKey2->key} | - | - | Admin | - +--------------------------------------+------+-----------------+-------+ + +--------------------------------------+-----------------+-------+ + | Name | Expiration date | Roles | + +--------------------------------------+-----------------+-------+ + | {$apiKey1->name} | - | Admin | + +--------------------------------------+-----------------+-------+ + | {$apiKey2->name} | - | Admin | + +--------------------------------------+-----------------+-------+ OUTPUT, ]; @@ -94,45 +94,45 @@ class ListKeysCommandTest extends TestCase ], true, <<key} | - | - | Admin | - +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey2->key} | - | - | Author only | - +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey3->key} | - | - | Domain only: example.com | - +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey4->key} | - | - | Admin | - +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey5->key} | - | - | Author only | - | | | | Domain only: example.com | - +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey6->key} | - | - | Admin | - +--------------------------------------+------+-----------------+--------------------------+ + +--------------------------------------+-----------------+--------------------------+ + | Name | Expiration date | Roles | + +--------------------------------------+-----------------+--------------------------+ + | {$apiKey1->name} | - | Admin | + +--------------------------------------+-----------------+--------------------------+ + | {$apiKey2->name} | - | Author only | + +--------------------------------------+-----------------+--------------------------+ + | {$apiKey3->name} | - | Domain only: example.com | + +--------------------------------------+-----------------+--------------------------+ + | {$apiKey4->name} | - | Admin | + +--------------------------------------+-----------------+--------------------------+ + | {$apiKey5->name} | - | Author only | + | | | Domain only: example.com | + +--------------------------------------+-----------------+--------------------------+ + | {$apiKey6->name} | - | Admin | + +--------------------------------------+-----------------+--------------------------+ OUTPUT, ]; yield 'with names' => [ [ - $apiKey1 = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'Alice')), - $apiKey2 = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'Alice and Bob')), + ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'Alice')), + ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'Alice and Bob')), $apiKey3 = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: '')), $apiKey4 = ApiKey::create(), ], true, <<key} | Alice | - | Admin | - +--------------------------------------+---------------+-----------------+-------+ - | {$apiKey2->key} | Alice and Bob | - | Admin | - +--------------------------------------+---------------+-----------------+-------+ - | {$apiKey3->key} | | - | Admin | - +--------------------------------------+---------------+-----------------+-------+ - | {$apiKey4->key} | - | - | Admin | - +--------------------------------------+---------------+-----------------+-------+ + +--------------------------------------+-----------------+-------+ + | Name | Expiration date | Roles | + +--------------------------------------+-----------------+-------+ + | Alice | - | Admin | + +--------------------------------------+-----------------+-------+ + | Alice and Bob | - | Admin | + +--------------------------------------+-----------------+-------+ + | {$apiKey3->name} | - | Admin | + +--------------------------------------+-----------------+-------+ + | {$apiKey4->name} | - | Admin | + +--------------------------------------+-----------------+-------+ OUTPUT, ]; diff --git a/module/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)); }