shlinkio/shlink
1
0
Fork 0
mirror of https://github.com/shlinkio/shlink.git synced 2025-12-10 11:05:50 -06:00
Code Issues Packages Projects Releases 146 Wiki Activity

Lock transaction to avoid race conditions when renaming an API key

Browse Source
This commit is contained in:
Alejandro Celaya 2024-11-09 11:09:34 +01:00
parent 95685d958d
commit d228b88e51
5 changed files with 69 additions and 36 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

21
module/Rest/src/ApiKey/Repository/ApiKeyRepository.php
View File

@ -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;
}
}

7
module/Rest/src/ApiKey/Repository/ApiKeyRepositoryInterface.php
View File

@ -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;
}

48
module/Rest/src/Service/ApiKeyService.php
View File

@ -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;
}
}

11
module/Rest/test-db/ApiKey/Repository/ApiKeyRepositoryTest.php
View File

@ -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'));
}
}

18
module/Rest/test/Service/ApiKeyServiceTest.php
View File

@ -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);