From 95685d958d068f77cb966e47268b1223a8fc1048 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 9 Nov 2024 11:02:10 +0100 Subject: [PATCH 1/3] Update to latest test utils --- composer.json | 2 +- .../ShortUrl/Repository/CrawlableShortCodesQueryTest.php | 3 +-- .../Repository/DeleteExpiredShortUrlsRepositoryTest.php | 3 +-- .../test-db/ShortUrl/Repository/ShortUrlListRepositoryTest.php | 2 +- .../test-db/Visit/Repository/VisitDeleterRepositoryTest.php | 3 +-- .../test-db/Visit/Repository/VisitIterationRepositoryTest.php | 3 +-- 6 files changed, 6 insertions(+), 10 deletions(-) diff --git a/composer.json b/composer.json index 1100f099..84f626b7 100644 --- a/composer.json +++ b/composer.json @@ -73,7 +73,7 @@ "phpunit/phpunit": "^11.4", "roave/security-advisories": "dev-master", "shlinkio/php-coding-standard": "~2.4.0", - "shlinkio/shlink-test-utils": "^4.1.1", + "shlinkio/shlink-test-utils": "^4.2", "symfony/var-dumper": "^7.1", "veewee/composer-run-parallel": "^1.4" }, diff --git a/module/Core/test-db/ShortUrl/Repository/CrawlableShortCodesQueryTest.php b/module/Core/test-db/ShortUrl/Repository/CrawlableShortCodesQueryTest.php index d630520b..60955dd1 100644 --- a/module/Core/test-db/ShortUrl/Repository/CrawlableShortCodesQueryTest.php +++ b/module/Core/test-db/ShortUrl/Repository/CrawlableShortCodesQueryTest.php @@ -16,8 +16,7 @@ class CrawlableShortCodesQueryTest extends DatabaseTestCase protected function setUp(): void { - $em = $this->getEntityManager(); - $this->query = new CrawlableShortCodesQuery($em, $em->getClassMetadata(ShortUrl::class)); + $this->query = $this->createRepository(ShortUrl::class, CrawlableShortCodesQuery::class); } #[Test] diff --git a/module/Core/test-db/ShortUrl/Repository/DeleteExpiredShortUrlsRepositoryTest.php b/module/Core/test-db/ShortUrl/Repository/DeleteExpiredShortUrlsRepositoryTest.php index d90ad256..1751aac9 100644 --- a/module/Core/test-db/ShortUrl/Repository/DeleteExpiredShortUrlsRepositoryTest.php +++ b/module/Core/test-db/ShortUrl/Repository/DeleteExpiredShortUrlsRepositoryTest.php @@ -22,8 +22,7 @@ class DeleteExpiredShortUrlsRepositoryTest extends DatabaseTestCase protected function setUp(): void { - $em = $this->getEntityManager(); - $this->repository = new ExpiredShortUrlsRepository($em, $em->getClassMetadata(ShortUrl::class)); + $this->repository = $this->createRepository(ShortUrl::class, ExpiredShortUrlsRepository::class); } #[Test] diff --git a/module/Core/test-db/ShortUrl/Repository/ShortUrlListRepositoryTest.php b/module/Core/test-db/ShortUrl/Repository/ShortUrlListRepositoryTest.php index 05109365..26d2dff5 100644 --- a/module/Core/test-db/ShortUrl/Repository/ShortUrlListRepositoryTest.php +++ b/module/Core/test-db/ShortUrl/Repository/ShortUrlListRepositoryTest.php @@ -37,7 +37,7 @@ class ShortUrlListRepositoryTest extends DatabaseTestCase protected function setUp(): void { $em = $this->getEntityManager(); - $this->repo = new ShortUrlListRepository($em, $em->getClassMetadata(ShortUrl::class)); + $this->repo = $this->createRepository(ShortUrl::class, ShortUrlListRepository::class); $this->relationResolver = new PersistenceShortUrlRelationResolver($em); } diff --git a/module/Core/test-db/Visit/Repository/VisitDeleterRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitDeleterRepositoryTest.php index 62aa89e7..6529c6a9 100644 --- a/module/Core/test-db/Visit/Repository/VisitDeleterRepositoryTest.php +++ b/module/Core/test-db/Visit/Repository/VisitDeleterRepositoryTest.php @@ -20,8 +20,7 @@ class VisitDeleterRepositoryTest extends DatabaseTestCase protected function setUp(): void { - $em = $this->getEntityManager(); - $this->repo = new VisitDeleterRepository($em, $em->getClassMetadata(Visit::class)); + $this->repo = $this->createRepository(Visit::class, VisitDeleterRepository::class); } #[Test] diff --git a/module/Core/test-db/Visit/Repository/VisitIterationRepositoryTest.php b/module/Core/test-db/Visit/Repository/VisitIterationRepositoryTest.php index 6d3d4b39..ee5843d5 100644 --- a/module/Core/test-db/Visit/Repository/VisitIterationRepositoryTest.php +++ b/module/Core/test-db/Visit/Repository/VisitIterationRepositoryTest.php @@ -25,8 +25,7 @@ class VisitIterationRepositoryTest extends DatabaseTestCase protected function setUp(): void { - $em = $this->getEntityManager(); - $this->repo = new VisitIterationRepository($em, $em->getClassMetadata(Visit::class)); + $this->repo = $this->createRepository(Visit::class, VisitIterationRepository::class); } #[Test, DataProvider('provideBlockSize')] From d228b88e517708b03e60fe1d357f25a8115d980b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 9 Nov 2024 11:09:34 +0100 Subject: [PATCH 2/3] Lock transaction to avoid race conditions when renaming an API key --- .../ApiKey/Repository/ApiKeyRepository.php | 21 +++++++- .../Repository/ApiKeyRepositoryInterface.php | 7 ++- module/Rest/src/Service/ApiKeyService.php | 48 +++++++++---------- .../Repository/ApiKeyRepositoryTest.php | 11 +++++ .../Rest/test/Service/ApiKeyServiceTest.php | 18 +++---- 5 files changed, 69 insertions(+), 36 deletions(-) diff --git a/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php b/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php index b4523371..e1fbf3a6 100644 --- a/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php +++ b/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php @@ -15,7 +15,7 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey; class ApiKeyRepository extends EntitySpecificationRepository implements ApiKeyRepositoryInterface { /** - * Will create provided API key with admin permissions, only if no other API keys exist yet + * @inheritDoc */ public function createInitialApiKey(string $apiKey): ApiKey|null { @@ -41,4 +41,23 @@ class ApiKeyRepository extends EntitySpecificationRepository implements ApiKeyRe return $initialApiKey; }); } + + /** + * @inheritDoc + */ + public function nameExists(string $name): bool + { + $qb = $this->getEntityManager()->createQueryBuilder(); + $qb->select('a.id') + ->from(ApiKey::class, 'a') + ->where($qb->expr()->eq('a.name', ':name')) + ->setParameter('name', $name) + ->setMaxResults(1); + + // Lock for update, to avoid a race condition that inserts a duplicate name after we have checked if one existed + $query = $qb->getQuery(); + $query->setLockMode(LockMode::PESSIMISTIC_WRITE); + + return $query->getOneOrNullResult() !== null; + } } diff --git a/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php b/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php index 0f81dc10..32ada38a 100644 --- a/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php +++ b/module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php @@ -14,7 +14,12 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey; interface ApiKeyRepositoryInterface extends EntityRepositoryInterface, EntitySpecificationRepositoryInterface { /** - * Will create provided API key only if there's no 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; + + /** + * Checks whether an API key with provided name exists or not + */ + public function nameExists(string $name): bool; } diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php index f517dde5..19140534 100644 --- a/module/Rest/src/Service/ApiKeyService.php +++ b/module/Rest/src/Service/ApiKeyService.php @@ -21,18 +21,20 @@ 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), - ); - } + return $this->em->wrapInTransaction(function () use ($apiKeyMeta) { + $apiKey = ApiKey::fromMeta($apiKeyMeta); + // TODO If name is auto-generated, do not throw. Instead, re-generate a new key + if ($this->repo->nameExists($apiKey->name)) { + throw new InvalidArgumentException( + sprintf('Another API key with name "%s" already exists', $apiKeyMeta->name), + ); + } - $this->em->persist($apiKey); - $this->em->flush(); + $this->em->persist($apiKey); + $this->em->flush(); - return $apiKey; + return $apiKey; + }); } public function createInitial(string $key): ApiKey|null @@ -85,9 +87,6 @@ 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 { @@ -102,25 +101,22 @@ readonly class ApiKeyService implements ApiKeyServiceInterface return $apiKey; } - if ($this->existsWithName($apiKeyRenaming->newName)) { - throw new InvalidArgumentException( - sprintf('Another API key with name "%s" already exists', $apiKeyRenaming->newName), - ); - } + return $this->em->wrapInTransaction(function () use ($apiKeyRenaming, $apiKey) { + if ($this->repo->nameExists($apiKeyRenaming->newName)) { + throw new InvalidArgumentException( + sprintf('Another API key with name "%s" already exists', $apiKeyRenaming->newName), + ); + } - $apiKey->name = $apiKeyRenaming->newName; - $this->em->flush(); + $apiKey->name = $apiKeyRenaming->newName; + $this->em->flush(); - return $apiKey; + return $apiKey; + }); } private function findByKey(string $key): ApiKey|null { 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/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php b/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php index 62d52de6..d0f6157d 100644 --- a/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php +++ b/module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace ShlinkioDbTest\Shlink\Rest\ApiKey\Repository; use PHPUnit\Framework\Attributes\Test; +use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Repository\ApiKeyRepository; use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\TestUtils\DbTest\DatabaseTestCase; @@ -29,4 +30,14 @@ class ApiKeyRepositoryTest extends DatabaseTestCase self::assertCount(1, $this->repo->findAll()); self::assertCount(0, $this->repo->findBy(['key' => ApiKey::hashKey('another_one')])); } + + #[Test] + public function nameExistsReturnsExpectedResult(): void + { + $this->getEntityManager()->persist(ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'foo'))); + $this->getEntityManager()->flush(); + + self::assertTrue($this->repo->nameExists('foo')); + self::assertFalse($this->repo->nameExists('bar')); + } } diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php index adecfbd9..2b3d4f96 100644 --- a/module/Rest/test/Service/ApiKeyServiceTest.php +++ b/module/Rest/test/Service/ApiKeyServiceTest.php @@ -30,6 +30,8 @@ class ApiKeyServiceTest extends TestCase protected function setUp(): void { $this->em = $this->createMock(EntityManager::class); + $this->em->method('wrapInTransaction')->willReturnCallback(fn (callable $callback) => $callback()); + $this->repo = $this->createMock(ApiKeyRepositoryInterface::class); $this->service = new ApiKeyService($this->em, $this->repo); } @@ -40,9 +42,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->repo->expects($this->once())->method('nameExists')->with( + ! empty($name) ? $name : $this->isType('string'), + )->willReturn(false); $this->em->expects($this->once())->method('flush'); $this->em->expects($this->once())->method('persist')->with($this->isInstanceOf(ApiKey::class)); @@ -78,7 +80,7 @@ class ApiKeyServiceTest extends TestCase #[Test] public function exceptionIsThrownWhileCreatingIfNameIsInUse(): void { - $this->repo->expects($this->once())->method('count')->with(['name' => 'the_name'])->willReturn(1); + $this->repo->expects($this->once())->method('nameExists')->with('the_name')->willReturn(true); $this->em->expects($this->never())->method('flush'); $this->em->expects($this->never())->method('persist'); @@ -200,7 +202,7 @@ class ApiKeyServiceTest extends TestCase $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->repo->expects($this->never())->method('nameExists'); $this->em->expects($this->never())->method('flush'); $this->expectException(InvalidArgumentException::class); @@ -216,7 +218,7 @@ class ApiKeyServiceTest extends TestCase $apiKey = ApiKey::create(); $this->repo->expects($this->once())->method('findOneBy')->with(['name' => 'same_value'])->willReturn($apiKey); - $this->repo->expects($this->never())->method('count'); + $this->repo->expects($this->never())->method('nameExists'); $this->em->expects($this->never())->method('flush'); $result = $this->service->renameApiKey($renaming); @@ -231,7 +233,7 @@ class ApiKeyServiceTest extends TestCase $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->repo->expects($this->once())->method('nameExists')->with('new')->willReturn(true); $this->em->expects($this->never())->method('flush'); $this->expectException(InvalidArgumentException::class); @@ -247,7 +249,7 @@ class ApiKeyServiceTest extends TestCase $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->repo->expects($this->once())->method('nameExists')->with('new')->willReturn(false); $this->em->expects($this->once())->method('flush'); $result = $this->service->renameApiKey($renaming); From 3c6f12aec614d658409a0c2796786c360e4ef01a Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 9 Nov 2024 12:07:07 +0100 Subject: [PATCH 3/3] Ensure auto-generated name API keys do not throw duplicated name --- module/Rest/src/ApiKey/Model/ApiKeyMeta.php | 7 ++-- module/Rest/src/Entity/ApiKey.php | 2 +- module/Rest/src/Service/ApiKeyService.php | 35 ++++++++++++++----- .../src/Service/ApiKeyServiceInterface.php | 3 ++ .../Rest/test/Service/ApiKeyServiceTest.php | 18 +++++++++- 5 files changed, 53 insertions(+), 12 deletions(-) diff --git a/module/Rest/src/ApiKey/Model/ApiKeyMeta.php b/module/Rest/src/ApiKey/Model/ApiKeyMeta.php index 66d7d889..ae1b189c 100644 --- a/module/Rest/src/ApiKey/Model/ApiKeyMeta.php +++ b/module/Rest/src/ApiKey/Model/ApiKeyMeta.php @@ -18,12 +18,13 @@ final readonly class ApiKeyMeta private function __construct( public string $key, public string $name, + public bool $isNameAutoGenerated, public Chronos|null $expirationDate, public iterable $roleDefinitions, ) { } - public static function empty(): self + public static function create(): self { return self::fromParams(); } @@ -38,9 +39,10 @@ final readonly class ApiKeyMeta iterable $roleDefinitions = [], ): self { $resolvedKey = $key ?? Uuid::uuid4()->toString(); + $isNameAutoGenerated = empty($name); // If a name was not provided, fall back to the key - if (empty($name)) { + if ($isNameAutoGenerated) { // 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 @@ -51,6 +53,7 @@ final readonly class ApiKeyMeta return new self( key: $resolvedKey, name: $name, + isNameAutoGenerated: $isNameAutoGenerated, expirationDate: $expirationDate, roleDefinitions: $roleDefinitions, ); diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index c9cdd3a6..63bb6fc9 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -33,7 +33,7 @@ class ApiKey extends AbstractEntity public static function create(): ApiKey { - return self::fromMeta(ApiKeyMeta::empty()); + return self::fromMeta(ApiKeyMeta::create()); } public static function fromMeta(ApiKeyMeta $meta): self diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php index 19140534..09d1bb76 100644 --- a/module/Rest/src/Service/ApiKeyService.php +++ b/module/Rest/src/Service/ApiKeyService.php @@ -19,17 +19,13 @@ readonly class ApiKeyService implements ApiKeyServiceInterface { } + /** + * @inheritDoc + */ public function create(ApiKeyMeta $apiKeyMeta): ApiKey { return $this->em->wrapInTransaction(function () use ($apiKeyMeta) { - $apiKey = ApiKey::fromMeta($apiKeyMeta); - // TODO If name is auto-generated, do not throw. Instead, re-generate a new key - if ($this->repo->nameExists($apiKey->name)) { - throw new InvalidArgumentException( - sprintf('Another API key with name "%s" already exists', $apiKeyMeta->name), - ); - } - + $apiKey = ApiKey::fromMeta($this->ensureUniqueName($apiKeyMeta)); $this->em->persist($apiKey); $this->em->flush(); @@ -37,6 +33,29 @@ readonly class ApiKeyService implements ApiKeyServiceInterface }); } + /** + * Given an ApiKeyMeta object, it returns another instance ensuring the name is unique. + * - If the name was auto-generated, it continues re-trying until a unique name is resolved. + * - If the name was explicitly provided, it throws in case of name conflict. + */ + private function ensureUniqueName(ApiKeyMeta $apiKeyMeta): ApiKeyMeta + { + if (! $this->repo->nameExists($apiKeyMeta->name)) { + return $apiKeyMeta; + } + + if (! $apiKeyMeta->isNameAutoGenerated) { + throw new InvalidArgumentException( + sprintf('Another API key with name "%s" already exists', $apiKeyMeta->name), + ); + } + + return $this->ensureUniqueName(ApiKeyMeta::fromParams( + expirationDate: $apiKeyMeta->expirationDate, + roleDefinitions: $apiKeyMeta->roleDefinitions, + )); + } + public function createInitial(string $key): ApiKey|null { return $this->repo->createInitialApiKey($key); diff --git a/module/Rest/src/Service/ApiKeyServiceInterface.php b/module/Rest/src/Service/ApiKeyServiceInterface.php index c42505b7..be7b9191 100644 --- a/module/Rest/src/Service/ApiKeyServiceInterface.php +++ b/module/Rest/src/Service/ApiKeyServiceInterface.php @@ -11,6 +11,9 @@ use Shlinkio\Shlink\Rest\Entity\ApiKey; interface ApiKeyServiceInterface { + /** + * @throws InvalidArgumentException + */ public function create(ApiKeyMeta $apiKeyMeta): ApiKey; public function createInitial(string $key): ApiKey|null; diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php index 2b3d4f96..ee33e109 100644 --- a/module/Rest/test/Service/ApiKeyServiceTest.php +++ b/module/Rest/test/Service/ApiKeyServiceTest.php @@ -78,7 +78,23 @@ class ApiKeyServiceTest extends TestCase } #[Test] - public function exceptionIsThrownWhileCreatingIfNameIsInUse(): void + public function autoGeneratedNameIsRegeneratedIfAlreadyExists(): void + { + $callCount = 0; + $this->repo->expects($this->exactly(3))->method('nameExists')->with( + $this->isType('string'), + )->willReturnCallback(function () use (&$callCount): bool { + $callCount++; + return $callCount < 3; + }); + $this->em->expects($this->once())->method('flush'); + $this->em->expects($this->once())->method('persist')->with($this->isInstanceOf(ApiKey::class)); + + $this->service->create(ApiKeyMeta::create()); + } + + #[Test] + public function exceptionIsThrownWhileCreatingIfExplicitlyProvidedNameIsInUse(): void { $this->repo->expects($this->once())->method('nameExists')->with('the_name')->willReturn(true); $this->em->expects($this->never())->method('flush');