From 79c5418ac2935880cf1a481d75ee5d5b5b4fc920 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Mon, 4 Nov 2024 14:22:39 +0100 Subject: [PATCH] Simplify ApiKey entity by exposing key as a readonly prop --- .../src/Command/Api/GenerateKeyCommand.php | 2 +- .../CLI/src/Command/Api/ListKeysCommand.php | 2 +- .../Command/ShortUrl/ListShortUrlsCommand.php | 2 +- .../test/Command/Api/ListKeysCommandTest.php | 30 +++++++++---------- .../ShortUrl/ListShortUrlsCommandTest.php | 2 +- module/Rest/src/ApiKey/Model/ApiKeyMeta.php | 10 +++---- module/Rest/src/Entity/ApiKey.php | 13 +------- .../Rest/test-api/Fixtures/ApiKeyFixture.php | 8 +---- .../AuthenticationMiddlewareTest.php | 5 ++-- 9 files changed, 28 insertions(+), 46 deletions(-) diff --git a/module/CLI/src/Command/Api/GenerateKeyCommand.php b/module/CLI/src/Command/Api/GenerateKeyCommand.php index 0a35bef7..a7656189 100644 --- a/module/CLI/src/Command/Api/GenerateKeyCommand.php +++ b/module/CLI/src/Command/Api/GenerateKeyCommand.php @@ -109,7 +109,7 @@ class GenerateKeyCommand extends Command )); $io = new SymfonyStyle($input, $output); - $io->success(sprintf('Generated API key: "%s"', $apiKey->toString())); + $io->success(sprintf('Generated API key: "%s"', $apiKey->key)); if (! ApiKey::isAdmin($apiKey)) { ShlinkTable::default($io)->render( diff --git a/module/CLI/src/Command/Api/ListKeysCommand.php b/module/CLI/src/Command/Api/ListKeysCommand.php index 40ae8eef..ab10ebc6 100644 --- a/module/CLI/src/Command/Api/ListKeysCommand.php +++ b/module/CLI/src/Command/Api/ListKeysCommand.php @@ -54,7 +54,7 @@ class ListKeysCommand extends Command $messagePattern = $this->determineMessagePattern($apiKey); // Set columns for this row - $rowData = [sprintf($messagePattern, $apiKey), sprintf($messagePattern, $apiKey->name ?? '-')]; + $rowData = [sprintf($messagePattern, $apiKey->key), sprintf($messagePattern, $apiKey->name ?? '-')]; if (! $enabledOnly) { $rowData[] = sprintf($messagePattern, $this->getEnabledSymbol($apiKey)); } diff --git a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php index 72a92fe8..900402e1 100644 --- a/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php +++ b/module/CLI/src/Command/ShortUrl/ListShortUrlsCommand.php @@ -244,7 +244,7 @@ class ListShortUrlsCommand extends Command } if ($input->getOption('show-api-key')) { $columnsMap['API Key'] = static fn (array $_, ShortUrl $shortUrl): string => - $shortUrl->authorApiKey?->__toString() ?? ''; + $shortUrl->authorApiKey?->key ?? ''; } if ($input->getOption('show-api-key-name')) { $columnsMap['API Key Name'] = static fn (array $_, ShortUrl $shortUrl): string|null => diff --git a/module/CLI/test/Command/Api/ListKeysCommandTest.php b/module/CLI/test/Command/Api/ListKeysCommandTest.php index 478dbaa5..6f4b816b 100644 --- a/module/CLI/test/Command/Api/ListKeysCommandTest.php +++ b/module/CLI/test/Command/Api/ListKeysCommandTest.php @@ -55,11 +55,11 @@ class ListKeysCommandTest extends TestCase +--------------------------------------+------+------------+---------------------------+-------+ | Key | Name | Is enabled | Expiration date | Roles | +--------------------------------------+------+------------+---------------------------+-------+ - | {$apiKey1} | - | --- | - | Admin | + | {$apiKey1->key} | - | --- | - | Admin | +--------------------------------------+------+------------+---------------------------+-------+ - | {$apiKey2} | - | --- | 2020-01-01T00:00:00+00:00 | Admin | + | {$apiKey2->key} | - | --- | 2020-01-01T00:00:00+00:00 | Admin | +--------------------------------------+------+------------+---------------------------+-------+ - | {$apiKey3} | - | +++ | - | Admin | + | {$apiKey3->key} | - | +++ | - | Admin | +--------------------------------------+------+------------+---------------------------+-------+ OUTPUT, @@ -71,9 +71,9 @@ class ListKeysCommandTest extends TestCase +--------------------------------------+------+-----------------+-------+ | Key | Name | Expiration date | Roles | +--------------------------------------+------+-----------------+-------+ - | {$apiKey1} | - | - | Admin | + | {$apiKey1->key} | - | - | Admin | +--------------------------------------+------+-----------------+-------+ - | {$apiKey2} | - | - | Admin | + | {$apiKey2->key} | - | - | Admin | +--------------------------------------+------+-----------------+-------+ OUTPUT, @@ -97,18 +97,18 @@ class ListKeysCommandTest extends TestCase +--------------------------------------+------+-----------------+--------------------------+ | Key | Name | Expiration date | Roles | +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey1} | - | - | Admin | + | {$apiKey1->key} | - | - | Admin | +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey2} | - | - | Author only | + | {$apiKey2->key} | - | - | Author only | +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey3} | - | - | Domain only: example.com | + | {$apiKey3->key} | - | - | Domain only: example.com | +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey4} | - | - | Admin | + | {$apiKey4->key} | - | - | Admin | +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey5} | - | - | Author only | + | {$apiKey5->key} | - | - | Author only | | | | | Domain only: example.com | +--------------------------------------+------+-----------------+--------------------------+ - | {$apiKey6} | - | - | Admin | + | {$apiKey6->key} | - | - | Admin | +--------------------------------------+------+-----------------+--------------------------+ OUTPUT, @@ -125,13 +125,13 @@ class ListKeysCommandTest extends TestCase +--------------------------------------+---------------+-----------------+-------+ | Key | Name | Expiration date | Roles | +--------------------------------------+---------------+-----------------+-------+ - | {$apiKey1} | Alice | - | Admin | + | {$apiKey1->key} | Alice | - | Admin | +--------------------------------------+---------------+-----------------+-------+ - | {$apiKey2} | Alice and Bob | - | Admin | + | {$apiKey2->key} | Alice and Bob | - | Admin | +--------------------------------------+---------------+-----------------+-------+ - | {$apiKey3} | | - | Admin | + | {$apiKey3->key} | | - | Admin | +--------------------------------------+---------------+-----------------+-------+ - | {$apiKey4} | - | - | Admin | + | {$apiKey4->key} | - | - | Admin | +--------------------------------------+---------------+-----------------+-------+ OUTPUT, diff --git a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php index 41b3fe88..3b84d175 100644 --- a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php @@ -140,7 +140,7 @@ class ListShortUrlsCommandTest extends TestCase public static function provideOptionalFlags(): iterable { $apiKey = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'my api key')); - $key = $apiKey->toString(); + $key = $apiKey->key; yield 'tags only' => [ ['--show-tags' => true], diff --git a/module/Rest/src/ApiKey/Model/ApiKeyMeta.php b/module/Rest/src/ApiKey/Model/ApiKeyMeta.php index 020c1f27..04b37214 100644 --- a/module/Rest/src/ApiKey/Model/ApiKeyMeta.php +++ b/module/Rest/src/ApiKey/Model/ApiKeyMeta.php @@ -7,16 +7,16 @@ namespace Shlinkio\Shlink\Rest\ApiKey\Model; use Cake\Chronos\Chronos; use Ramsey\Uuid\Uuid; -final class ApiKeyMeta +final readonly class ApiKeyMeta { /** * @param iterable $roleDefinitions */ private function __construct( - public readonly string $key, - public readonly string|null $name, - public readonly Chronos|null $expirationDate, - public readonly iterable $roleDefinitions, + public string $key, + public string|null $name, + public Chronos|null $expirationDate, + public iterable $roleDefinitions, ) { } diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index 1cca4f3a..4f7575c5 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -19,10 +19,9 @@ class ApiKey extends AbstractEntity { /** * @param Collection $roles - * @throws Exception */ private function __construct( - private string $key, + public readonly string $key, public readonly string|null $name = null, public readonly Chronos|null $expirationDate = null, private bool $enabled = true, @@ -75,16 +74,6 @@ class ApiKey extends AbstractEntity return $this->isEnabled() && ! $this->isExpired(); } - public function __toString(): string - { - return $this->key; - } - - public function toString(): string - { - return $this->key; - } - public function spec(string|null $context = null): Specification { $specs = $this->roles->map(fn (ApiKeyRole $role) => Role::toSpec($role, $context))->getValues(); diff --git a/module/Rest/test-api/Fixtures/ApiKeyFixture.php b/module/Rest/test-api/Fixtures/ApiKeyFixture.php index 1b4f64ea..c734e342 100644 --- a/module/Rest/test-api/Fixtures/ApiKeyFixture.php +++ b/module/Rest/test-api/Fixtures/ApiKeyFixture.php @@ -8,7 +8,6 @@ use Cake\Chronos\Chronos; use Doctrine\Common\DataFixtures\AbstractFixture; use Doctrine\Common\DataFixtures\DependentFixtureInterface; use Doctrine\Persistence\ObjectManager; -use ReflectionObject; use Shlinkio\Shlink\Core\Domain\Entity\Domain; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; @@ -51,12 +50,7 @@ class ApiKeyFixture extends AbstractFixture implements DependentFixtureInterface private function buildApiKey(string $key, bool $enabled, Chronos|null $expiresAt = null): ApiKey { - $apiKey = ApiKey::fromMeta(ApiKeyMeta::fromParams(expirationDate: $expiresAt)); - $ref = new ReflectionObject($apiKey); - $keyProp = $ref->getProperty('key'); - $keyProp->setAccessible(true); - $keyProp->setValue($apiKey, $key); - + $apiKey = ApiKey::fromMeta(ApiKeyMeta::fromParams(key: $key, expirationDate: $expiresAt)); if (! $enabled) { $apiKey->disable(); } diff --git a/module/Rest/test/Middleware/AuthenticationMiddlewareTest.php b/module/Rest/test/Middleware/AuthenticationMiddlewareTest.php index 99a8b3e6..5c530480 100644 --- a/module/Rest/test/Middleware/AuthenticationMiddlewareTest.php +++ b/module/Rest/test/Middleware/AuthenticationMiddlewareTest.php @@ -130,18 +130,17 @@ class AuthenticationMiddlewareTest extends TestCase public function validApiKeyFallsBackToNextMiddleware(): void { $apiKey = ApiKey::create(); - $key = $apiKey->toString(); $request = ServerRequestFactory::fromGlobals() ->withAttribute( RouteResult::class, RouteResult::fromRoute(new Route('bar', self::getDummyMiddleware()), []), ) - ->withHeader('X-Api-Key', $key); + ->withHeader('X-Api-Key', $apiKey->key); $this->handler->expects($this->once())->method('handle')->with( $request->withAttribute(ApiKey::class, $apiKey), )->willReturn(new Response()); - $this->apiKeyService->expects($this->once())->method('check')->with($key)->willReturn( + $this->apiKeyService->expects($this->once())->method('check')->with($apiKey->key)->willReturn( new ApiKeyCheckResult($apiKey), );