From 65a0a90a51748b8497f8495ec91d2afd4ff9d848 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Tue, 19 Sep 2023 09:10:17 +0200 Subject: [PATCH] Allow custom API keys to be created --- indocker | 2 +- module/CLI/config/cli.config.php | 1 + module/CLI/src/ApiKey/RoleResolver.php | 15 ++++---- .../CLI/src/ApiKey/RoleResolverInterface.php | 4 +-- .../src/Command/Api/GenerateKeyCommand.php | 30 +++++++++++----- module/CLI/test/ApiKey/RoleResolverTest.php | 4 +-- .../Command/Api/GenerateKeyCommandTest.php | 12 +++---- .../test/Command/Api/ListKeysCommandTest.php | 8 ++--- .../ShortUrl/ListShortUrlsCommandTest.php | 2 +- module/Rest/src/ApiKey/Model/ApiKeyMeta.php | 35 ++++++++++++------- .../ApiKey/Repository/ApiKeyRepository.php | 3 +- module/Rest/src/Entity/ApiKey.php | 20 +++++------ module/Rest/src/Service/ApiKeyService.php | 32 ++++------------- .../src/Service/ApiKeyServiceInterface.php | 9 ++--- .../Rest/test-api/Fixtures/ApiKeyFixture.php | 2 +- .../Rest/test/Service/ApiKeyServiceTest.php | 8 +++-- 16 files changed, 93 insertions(+), 94 deletions(-) diff --git a/indocker b/indocker index 789386ac..7cfbe2c3 100755 --- a/indocker +++ b/indocker @@ -2,7 +2,7 @@ # Run docker containers if they are not up yet if ! [[ $(docker ps | grep shlink_swoole) ]]; then - docker-compose up -d + docker compose up -d fi docker exec -it shlink_swoole /bin/sh -c "$*" diff --git a/module/CLI/config/cli.config.php b/module/CLI/config/cli.config.php index 9feeee7b..2a1bc5e8 100644 --- a/module/CLI/config/cli.config.php +++ b/module/CLI/config/cli.config.php @@ -22,6 +22,7 @@ return [ Command\Visit\GetNonOrphanVisitsCommand::NAME => Command\Visit\GetNonOrphanVisitsCommand::class, Command\Api\GenerateKeyCommand::NAME => Command\Api\GenerateKeyCommand::class, + Command\Api\GenerateKeyCommand::ALIAS => Command\Api\GenerateKeyCommand::class, Command\Api\DisableKeyCommand::NAME => Command\Api\DisableKeyCommand::class, Command\Api\ListKeysCommand::NAME => Command\Api\ListKeysCommand::class, diff --git a/module/CLI/src/ApiKey/RoleResolver.php b/module/CLI/src/ApiKey/RoleResolver.php index c1ae8f05..ad98bde4 100644 --- a/module/CLI/src/ApiKey/RoleResolver.php +++ b/module/CLI/src/ApiKey/RoleResolver.php @@ -14,24 +14,23 @@ use function is_string; class RoleResolver implements RoleResolverInterface { - public function __construct(private DomainServiceInterface $domainService, private string $defaultDomain) - { + public function __construct( + private readonly DomainServiceInterface $domainService, + private readonly string $defaultDomain, + ) { } - public function determineRoles(InputInterface $input): array + public function determineRoles(InputInterface $input): iterable { $domainAuthority = $input->getOption(Role::DOMAIN_SPECIFIC->paramName()); $author = $input->getOption(Role::AUTHORED_SHORT_URLS->paramName()); - $roleDefinitions = []; if ($author) { - $roleDefinitions[] = RoleDefinition::forAuthoredShortUrls(); + yield RoleDefinition::forAuthoredShortUrls(); } if (is_string($domainAuthority)) { - $roleDefinitions[] = $this->resolveRoleForAuthority($domainAuthority); + yield $this->resolveRoleForAuthority($domainAuthority); } - - return $roleDefinitions; } private function resolveRoleForAuthority(string $domainAuthority): RoleDefinition diff --git a/module/CLI/src/ApiKey/RoleResolverInterface.php b/module/CLI/src/ApiKey/RoleResolverInterface.php index 92a04594..e849ad13 100644 --- a/module/CLI/src/ApiKey/RoleResolverInterface.php +++ b/module/CLI/src/ApiKey/RoleResolverInterface.php @@ -10,7 +10,7 @@ use Symfony\Component\Console\Input\InputInterface; interface RoleResolverInterface { /** - * @return RoleDefinition[] + * @return iterable */ - public function determineRoles(InputInterface $input): array; + public function determineRoles(InputInterface $input): iterable; } diff --git a/module/CLI/src/Command/Api/GenerateKeyCommand.php b/module/CLI/src/Command/Api/GenerateKeyCommand.php index c2d6cf10..ec7b5cb2 100644 --- a/module/CLI/src/Command/Api/GenerateKeyCommand.php +++ b/module/CLI/src/Command/Api/GenerateKeyCommand.php @@ -8,10 +8,12 @@ use Cake\Chronos\Chronos; use Shlinkio\Shlink\CLI\ApiKey\RoleResolverInterface; use Shlinkio\Shlink\CLI\Util\ExitCode; use Shlinkio\Shlink\CLI\Util\ShlinkTable; +use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Role; use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface; use Symfony\Component\Console\Command\Command; +use Symfony\Component\Console\Input\InputArgument; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; use Symfony\Component\Console\Output\OutputInterface; @@ -22,11 +24,13 @@ use function sprintf; class GenerateKeyCommand extends Command { - public const NAME = 'api-key:generate'; + public const NAME = 'api-key:create'; + /** @deprecated */ + public const ALIAS = 'api-key:generate'; public function __construct( - private ApiKeyServiceInterface $apiKeyService, - private RoleResolverInterface $roleResolver, + private readonly ApiKeyServiceInterface $apiKeyService, + private readonly RoleResolverInterface $roleResolver, ) { parent::__construct(); } @@ -57,7 +61,13 @@ class GenerateKeyCommand extends Command $this ->setName(self::NAME) - ->setDescription('Generates a new valid API key.') + ->setDescription('Creates a new valid API key.') + ->setAliases([self::ALIAS]) + ->addArgument( + 'key', + InputArgument::OPTIONAL, + 'The API key to create. A random one will be generated if not provided', + ) ->addOption( 'name', 'm', @@ -91,11 +101,13 @@ class GenerateKeyCommand extends Command protected function execute(InputInterface $input, OutputInterface $output): ?int { $expirationDate = $input->getOption('expiration-date'); - $apiKey = $this->apiKeyService->create( - isset($expirationDate) ? Chronos::parse($expirationDate) : null, - $input->getOption('name'), - ...$this->roleResolver->determineRoles($input), - ); + + $apiKey = $this->apiKeyService->create(ApiKeyMeta::fromParams( + key: $input->getArgument('key'), + name: $input->getOption('name'), + expirationDate: isset($expirationDate) ? Chronos::parse($expirationDate) : null, + roleDefinitions: $this->roleResolver->determineRoles($input), + )); $io = new SymfonyStyle($input, $output); $io->success(sprintf('Generated API key: "%s"', $apiKey->toString())); diff --git a/module/CLI/test/ApiKey/RoleResolverTest.php b/module/CLI/test/ApiKey/RoleResolverTest.php index bc151c5e..7aecda6d 100644 --- a/module/CLI/test/ApiKey/RoleResolverTest.php +++ b/module/CLI/test/ApiKey/RoleResolverTest.php @@ -40,7 +40,7 @@ class RoleResolverTest extends TestCase 'example.com', )->willReturn(self::domainWithId(Domain::withAuthority('example.com'))); - $result = $this->resolver->determineRoles($input); + $result = [...$this->resolver->determineRoles($input)]; self::assertEquals($expectedRoles, $result); } @@ -111,7 +111,7 @@ class RoleResolverTest extends TestCase $this->expectException(InvalidRoleConfigException::class); - $this->resolver->determineRoles($input); + [...$this->resolver->determineRoles($input)]; } private static function domainWithId(Domain $domain): Domain diff --git a/module/CLI/test/Command/Api/GenerateKeyCommandTest.php b/module/CLI/test/Command/Api/GenerateKeyCommandTest.php index b5dbe513..0b306357 100644 --- a/module/CLI/test/Command/Api/GenerateKeyCommandTest.php +++ b/module/CLI/test/Command/Api/GenerateKeyCommandTest.php @@ -10,12 +10,15 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\CLI\ApiKey\RoleResolverInterface; use Shlinkio\Shlink\CLI\Command\Api\GenerateKeyCommand; +use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\Entity\ApiKey; use Shlinkio\Shlink\Rest\Service\ApiKeyServiceInterface; use ShlinkioTest\Shlink\CLI\CliTestUtilsTrait; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Tester\CommandTester; +use function is_string; + class GenerateKeyCommandTest extends TestCase { use CliTestUtilsTrait; @@ -37,8 +40,7 @@ class GenerateKeyCommandTest extends TestCase public function noExpirationDateIsDefinedIfNotProvided(): void { $this->apiKeyService->expects($this->once())->method('create')->with( - $this->isNull(), - $this->isNull(), + $this->callback(fn (ApiKeyMeta $meta) => $meta->name === null && $meta->expirationDate === null), )->willReturn(ApiKey::create()); $this->commandTester->execute([]); @@ -51,8 +53,7 @@ class GenerateKeyCommandTest extends TestCase public function expirationDateIsDefinedIfProvided(): void { $this->apiKeyService->expects($this->once())->method('create')->with( - $this->isInstanceOf(Chronos::class), - $this->isNull(), + $this->callback(fn (ApiKeyMeta $meta) => $meta->expirationDate instanceof Chronos), )->willReturn(ApiKey::create()); $this->commandTester->execute([ @@ -64,8 +65,7 @@ class GenerateKeyCommandTest extends TestCase public function nameIsDefinedIfProvided(): void { $this->apiKeyService->expects($this->once())->method('create')->with( - $this->isNull(), - $this->isType('string'), + $this->callback(fn (ApiKeyMeta $meta) => is_string($meta->name)), )->willReturn(ApiKey::create()); $this->commandTester->execute([ diff --git a/module/CLI/test/Command/Api/ListKeysCommandTest.php b/module/CLI/test/Command/Api/ListKeysCommandTest.php index e4cdb438..5e246fc7 100644 --- a/module/CLI/test/Command/Api/ListKeysCommandTest.php +++ b/module/CLI/test/Command/Api/ListKeysCommandTest.php @@ -49,7 +49,7 @@ class ListKeysCommandTest extends TestCase yield 'all keys' => [ [ $apiKey1 = ApiKey::create()->disable(), - $apiKey2 = ApiKey::fromMeta(ApiKeyMeta::withExpirationDate($dateInThePast)), + $apiKey2 = ApiKey::fromMeta(ApiKeyMeta::fromParams(expirationDate: $dateInThePast)), $apiKey3 = ApiKey::create(), ], false, @@ -117,9 +117,9 @@ class ListKeysCommandTest extends TestCase ]; yield 'with names' => [ [ - $apiKey1 = ApiKey::fromMeta(ApiKeyMeta::withName('Alice')), - $apiKey2 = ApiKey::fromMeta(ApiKeyMeta::withName('Alice and Bob')), - $apiKey3 = ApiKey::fromMeta(ApiKeyMeta::withName('')), + $apiKey1 = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'Alice')), + $apiKey2 = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'Alice and Bob')), + $apiKey3 = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: '')), $apiKey4 = ApiKey::create(), ], true, diff --git a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php index d81172ed..aff1ebdb 100644 --- a/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php +++ b/module/CLI/test/Command/ShortUrl/ListShortUrlsCommandTest.php @@ -138,7 +138,7 @@ class ListShortUrlsCommandTest extends TestCase public static function provideOptionalFlags(): iterable { - $apiKey = ApiKey::fromMeta(ApiKeyMeta::withName('my api key')); + $apiKey = ApiKey::fromMeta(ApiKeyMeta::fromParams(name: 'my api key')); $key = $apiKey->toString(); yield 'tags only' => [ diff --git a/module/Rest/src/ApiKey/Model/ApiKeyMeta.php b/module/Rest/src/ApiKey/Model/ApiKeyMeta.php index 430221a2..e28a9ec3 100644 --- a/module/Rest/src/ApiKey/Model/ApiKeyMeta.php +++ b/module/Rest/src/ApiKey/Model/ApiKeyMeta.php @@ -5,36 +5,45 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\ApiKey\Model; use Cake\Chronos\Chronos; +use Ramsey\Uuid\Uuid; final class ApiKeyMeta { /** - * @param RoleDefinition[] $roleDefinitions + * @param iterable $roleDefinitions */ private function __construct( + public readonly string $key, public readonly ?string $name, public readonly ?Chronos $expirationDate, - public readonly array $roleDefinitions, + public readonly iterable $roleDefinitions, ) { } - public static function withName(string $name): self + public static function empty(): self { - return new self($name, null, []); + return self::fromParams(); } - public static function withExpirationDate(Chronos $expirationDate): self - { - return new self(null, $expirationDate, []); - } - - public static function withNameAndExpirationDate(string $name, Chronos $expirationDate): self - { - return new self($name, $expirationDate, []); + /** + * @param iterable $roleDefinitions + */ + public static function fromParams( + ?string $key = null, + ?string $name = null, + ?Chronos $expirationDate = null, + iterable $roleDefinitions = [], + ): self { + return new self( + key: $key ?? Uuid::uuid4()->toString(), + name: $name, + expirationDate: $expirationDate, + roleDefinitions: $roleDefinitions, + ); } public static function withRoles(RoleDefinition ...$roleDefinitions): self { - return new self(null, null, $roleDefinitions); + return self::fromParams(roleDefinitions: $roleDefinitions); } } diff --git a/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php b/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php index ec49145e..eb11fc0d 100644 --- a/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php +++ b/module/Rest/src/ApiKey/Repository/ApiKeyRepository.php @@ -6,6 +6,7 @@ namespace Shlinkio\Shlink\Rest\ApiKey\Repository; use Doctrine\DBAL\LockMode; use Happyr\DoctrineSpecification\Repository\EntitySpecificationRepository; +use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\Entity\ApiKey; class ApiKeyRepository extends EntitySpecificationRepository implements ApiKeyRepositoryInterface @@ -24,7 +25,7 @@ class ApiKeyRepository extends EntitySpecificationRepository implements ApiKeyRe ->getOneOrNullResult(); if ($firstResult === null) { - $em->persist(ApiKey::fromKey($apiKey)); + $em->persist(ApiKey::fromMeta(ApiKeyMeta::fromParams(key: $apiKey))); $em->flush(); } }); diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index 88cfa27e..07d05a03 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -10,7 +10,6 @@ use Doctrine\Common\Collections\Collection; use Exception; use Happyr\DoctrineSpecification\Spec; use Happyr\DoctrineSpecification\Specification\Specification; -use Ramsey\Uuid\Uuid; use Shlinkio\Shlink\Common\Entity\AbstractEntity; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; @@ -28,21 +27,27 @@ class ApiKey extends AbstractEntity /** * @throws Exception */ - private function __construct(?string $key = null) + private function __construct(string $key) { - $this->key = $key ?? Uuid::uuid4()->toString(); + $this->key = $key; $this->enabled = true; $this->roles = new ArrayCollection(); } + /** + * @throws Exception + */ public static function create(): ApiKey { - return new self(); + return self::fromMeta(ApiKeyMeta::empty()); } + /** + * @throws Exception + */ public static function fromMeta(ApiKeyMeta $meta): self { - $apiKey = self::create(); + $apiKey = new self($meta->key); $apiKey->name = $meta->name; $apiKey->expirationDate = $meta->expirationDate; @@ -53,11 +58,6 @@ class ApiKey extends AbstractEntity return $apiKey; } - public static function fromKey(string $key): self - { - return new self($key); - } - public function getExpirationDate(): ?Chronos { return $this->expirationDate; diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php index 7d7e0710..b819c22d 100644 --- a/module/Rest/src/Service/ApiKeyService.php +++ b/module/Rest/src/Service/ApiKeyService.php @@ -4,47 +4,27 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Service; -use Cake\Chronos\Chronos; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; -use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; use Shlinkio\Shlink\Rest\Entity\ApiKey; use function sprintf; class ApiKeyService implements ApiKeyServiceInterface { - public function __construct(private EntityManagerInterface $em) + public function __construct(private readonly EntityManagerInterface $em) { } - public function create( - ?Chronos $expirationDate = null, - ?string $name = null, - RoleDefinition ...$roleDefinitions, - ): ApiKey { - $key = $this->buildApiKeyWithParams($expirationDate, $name); - foreach ($roleDefinitions as $definition) { - $key->registerRole($definition); - } + public function create(ApiKeyMeta $apiKeyMeta): ApiKey + { + $apiKey = ApiKey::fromMeta($apiKeyMeta); - $this->em->persist($key); + $this->em->persist($apiKey); $this->em->flush(); - return $key; - } - - private function buildApiKeyWithParams(?Chronos $expirationDate, ?string $name): ApiKey - { - return match (true) { - $expirationDate !== null && $name !== null => ApiKey::fromMeta( - ApiKeyMeta::withNameAndExpirationDate($name, $expirationDate), - ), - $expirationDate !== null => ApiKey::fromMeta(ApiKeyMeta::withExpirationDate($expirationDate)), - $name !== null => ApiKey::fromMeta(ApiKeyMeta::withName($name)), - default => ApiKey::create(), - }; + return $apiKey; } public function check(string $key): ApiKeyCheckResult diff --git a/module/Rest/src/Service/ApiKeyServiceInterface.php b/module/Rest/src/Service/ApiKeyServiceInterface.php index 85b726df..826931a3 100644 --- a/module/Rest/src/Service/ApiKeyServiceInterface.php +++ b/module/Rest/src/Service/ApiKeyServiceInterface.php @@ -4,18 +4,13 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Service; -use Cake\Chronos\Chronos; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; -use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; +use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\Entity\ApiKey; interface ApiKeyServiceInterface { - public function create( - ?Chronos $expirationDate = null, - ?string $name = null, - RoleDefinition ...$roleDefinitions, - ): ApiKey; + public function create(ApiKeyMeta $apiKeyMeta): ApiKey; public function check(string $key): ApiKeyCheckResult; diff --git a/module/Rest/test-api/Fixtures/ApiKeyFixture.php b/module/Rest/test-api/Fixtures/ApiKeyFixture.php index 5ac886ce..bc33c678 100644 --- a/module/Rest/test-api/Fixtures/ApiKeyFixture.php +++ b/module/Rest/test-api/Fixtures/ApiKeyFixture.php @@ -43,7 +43,7 @@ class ApiKeyFixture extends AbstractFixture implements DependentFixtureInterface private function buildApiKey(string $key, bool $enabled, ?Chronos $expiresAt = null): ApiKey { - $apiKey = $expiresAt !== null ? ApiKey::fromMeta(ApiKeyMeta::withExpirationDate($expiresAt)) : ApiKey::create(); + $apiKey = ApiKey::fromMeta(ApiKeyMeta::fromParams(expirationDate: $expiresAt)); $ref = new ReflectionObject($apiKey); $keyProp = $ref->getProperty('key'); $keyProp->setAccessible(true); diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php index 325713be..952887a5 100644 --- a/module/Rest/test/Service/ApiKeyServiceTest.php +++ b/module/Rest/test/Service/ApiKeyServiceTest.php @@ -40,7 +40,9 @@ class ApiKeyServiceTest extends TestCase $this->em->expects($this->once())->method('flush'); $this->em->expects($this->once())->method('persist')->with($this->isInstanceOf(ApiKey::class)); - $key = $this->service->create($date, $name, ...$roles); + $key = $this->service->create( + ApiKeyMeta::fromParams(name: $name, expirationDate: $date, roleDefinitions: $roles), + ); self::assertEquals($date, $key->getExpirationDate()); self::assertEquals($name, $key->name()); @@ -81,7 +83,7 @@ class ApiKeyServiceTest extends TestCase { yield 'non-existent api key' => [null]; yield 'disabled api key' => [ApiKey::create()->disable()]; - yield 'expired api key' => [ApiKey::fromMeta(ApiKeyMeta::withExpirationDate(Chronos::now()->subDay()))]; + yield 'expired api key' => [ApiKey::fromMeta(ApiKeyMeta::fromParams(expirationDate: Chronos::now()->subDay()))]; } #[Test] @@ -144,7 +146,7 @@ class ApiKeyServiceTest extends TestCase $this->repo->expects($this->once())->method('findBy')->with(['enabled' => true])->willReturn($expectedApiKeys); $this->em->method('getRepository')->with(ApiKey::class)->willReturn($this->repo); - $result = $this->service->listKeys(true); + $result = $this->service->listKeys(enabledOnly: true); self::assertEquals($expectedApiKeys, $result); }