Mirrors/shlink
1
0
Fork 0
mirror of https://github.com/shlinkio/shlink.git synced 2026-03-10 17:23:12 +08:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #2253 from acelaya-forks/feature/api-key-improvements

Browse Source 
Feature/api key improvements
This commit is contained in:
Alejandro Celaya
2024-11-09 12:23:10 +01:00
committed by GitHub
parent 1a278eaf07 3c6f12aec6
commit 12a08cb373
14 changed files with 117 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
composer.json
View File

@@ -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"
},

3
module/Core/test-db/ShortUrl/Repository/CrawlableShortCodesQueryTest.php
View File

@@ -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]

3
module/Core/test-db/ShortUrl/Repository/DeleteExpiredShortUrlsRepositoryTest.php
View File

@@ -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]

2
module/Core/test-db/ShortUrl/Repository/ShortUrlListRepositoryTest.php
View File

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

3
module/Core/test-db/Visit/Repository/VisitDeleterRepositoryTest.php
View File

@@ -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]

3
module/Core/test-db/Visit/Repository/VisitIterationRepositoryTest.php
View File

@@ -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')]

7
module/Rest/src/ApiKey/Model/ApiKeyMeta.php
View File

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

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

2
module/Rest/src/Entity/ApiKey.php
View File

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

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

@@ -19,20 +19,41 @@ readonly class ApiKeyService implements ApiKeyServiceInterface
{
}
/**
* @inheritDoc
*/
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)) {
return $this->em->wrapInTransaction(function () use ($apiKeyMeta) {
$apiKey = ApiKey::fromMeta($this->ensureUniqueName($apiKeyMeta));
$this->em->persist($apiKey);
$this->em->flush();
return $apiKey;
});
}
/**
* 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),
);
}
$this->em->persist($apiKey);
$this->em->flush();
return $apiKey;
return $this->ensureUniqueName(ApiKeyMeta::fromParams(
expirationDate: $apiKeyMeta->expirationDate,
roleDefinitions: $apiKeyMeta->roleDefinitions,
));
}
public function createInitial(string $key): ApiKey|null
@@ -85,9 +106,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 +120,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;
}
}

3
module/Rest/src/Service/ApiKeyServiceInterface.php
View File

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

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

36
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));
@@ -76,9 +78,25 @@ class ApiKeyServiceTest extends TestCase
}
#[Test]
public function exceptionIsThrownWhileCreatingIfNameIsInUse(): void
public function autoGeneratedNameIsRegeneratedIfAlreadyExists(): void
{
$this->repo->expects($this->once())->method('count')->with(['name' => 'the_name'])->willReturn(1);
$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');
$this->em->expects($this->never())->method('persist');
@@ -200,7 +218,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 +234,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 +249,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 +265,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);