From d228b88e517708b03e60fe1d357f25a8115d980b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sat, 9 Nov 2024 11:09:34 +0100 Subject: [PATCH] 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);