From 6f837b3b91c5a676315b228d226cc9b590d9d405 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 8 Nov 2024 09:03:50 +0100 Subject: [PATCH] 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 {