From a661d051000d2087154f2875098290923e90787e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Fri, 8 Nov 2024 08:25:07 +0100 Subject: [PATCH] Allow API keys to be renamed --- .../CLI/src/Command/Tag/RenameTagCommand.php | 4 +- .../test/Command/Tag/RenameTagCommandTest.php | 6 +- .../src/Exception/TagConflictException.php | 4 +- .../TagRenaming.php => Model/Renaming.php} | 6 +- module/Core/src/Tag/TagService.php | 4 +- module/Core/src/Tag/TagServiceInterface.php | 4 +- .../Exception/TagConflictExceptionTest.php | 4 +- module/Core/test/Tag/TagServiceTest.php | 10 +-- .../Rest/src/Action/Tag/UpdateTagAction.php | 4 +- module/Rest/src/Entity/ApiKey.php | 3 +- module/Rest/src/Service/ApiKeyService.php | 41 ++++++++++-- .../src/Service/ApiKeyServiceInterface.php | 6 ++ .../test/Action/Tag/UpdateTagActionTest.php | 4 +- .../Rest/test/Service/ApiKeyServiceTest.php | 63 +++++++++++++++++++ 14 files changed, 132 insertions(+), 31 deletions(-) rename module/Core/src/{Tag/Model/TagRenaming.php => Model/Renaming.php} (86%) diff --git a/module/CLI/src/Command/Tag/RenameTagCommand.php b/module/CLI/src/Command/Tag/RenameTagCommand.php index fdc0f0ce..5830858e 100644 --- a/module/CLI/src/Command/Tag/RenameTagCommand.php +++ b/module/CLI/src/Command/Tag/RenameTagCommand.php @@ -7,7 +7,7 @@ namespace Shlinkio\Shlink\CLI\Command\Tag; use Shlinkio\Shlink\CLI\Util\ExitCode; use Shlinkio\Shlink\Core\Exception\TagConflictException; use Shlinkio\Shlink\Core\Exception\TagNotFoundException; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Core\Tag\TagServiceInterface; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputArgument; @@ -40,7 +40,7 @@ class RenameTagCommand extends Command $newName = $input->getArgument('newName'); try { - $this->tagService->renameTag(TagRenaming::fromNames($oldName, $newName)); + $this->tagService->renameTag(Renaming::fromNames($oldName, $newName)); $io->success('Tag properly renamed.'); return ExitCode::EXIT_SUCCESS; } catch (TagNotFoundException | TagConflictException $e) { diff --git a/module/CLI/test/Command/Tag/RenameTagCommandTest.php b/module/CLI/test/Command/Tag/RenameTagCommandTest.php index 296926b8..e7fb630d 100644 --- a/module/CLI/test/Command/Tag/RenameTagCommandTest.php +++ b/module/CLI/test/Command/Tag/RenameTagCommandTest.php @@ -9,8 +9,8 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\CLI\Command\Tag\RenameTagCommand; use Shlinkio\Shlink\Core\Exception\TagNotFoundException; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Core\Tag\Entity\Tag; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; use Shlinkio\Shlink\Core\Tag\TagServiceInterface; use ShlinkioTest\Shlink\CLI\Util\CliTestUtils; use Symfony\Component\Console\Tester\CommandTester; @@ -32,7 +32,7 @@ class RenameTagCommandTest extends TestCase $oldName = 'foo'; $newName = 'bar'; $this->tagService->expects($this->once())->method('renameTag')->with( - TagRenaming::fromNames($oldName, $newName), + Renaming::fromNames($oldName, $newName), )->willThrowException(TagNotFoundException::fromTag('foo')); $this->commandTester->execute([ @@ -50,7 +50,7 @@ class RenameTagCommandTest extends TestCase $oldName = 'foo'; $newName = 'bar'; $this->tagService->expects($this->once())->method('renameTag')->with( - TagRenaming::fromNames($oldName, $newName), + Renaming::fromNames($oldName, $newName), )->willReturn(new Tag($newName)); $this->commandTester->execute([ diff --git a/module/Core/src/Exception/TagConflictException.php b/module/Core/src/Exception/TagConflictException.php index 0fc5c317..e05754c7 100644 --- a/module/Core/src/Exception/TagConflictException.php +++ b/module/Core/src/Exception/TagConflictException.php @@ -7,7 +7,7 @@ namespace Shlinkio\Shlink\Core\Exception; use Fig\Http\Message\StatusCodeInterface; use Mezzio\ProblemDetails\Exception\CommonProblemDetailsExceptionTrait; use Mezzio\ProblemDetails\Exception\ProblemDetailsExceptionInterface; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; +use Shlinkio\Shlink\Core\Model\Renaming; use function Shlinkio\Shlink\Core\toProblemDetailsType; use function sprintf; @@ -19,7 +19,7 @@ class TagConflictException extends RuntimeException implements ProblemDetailsExc private const TITLE = 'Tag conflict'; public const ERROR_CODE = 'tag-conflict'; - public static function forExistingTag(TagRenaming $renaming): self + public static function forExistingTag(Renaming $renaming): self { $e = new self(sprintf('You cannot rename tag %s, because it already exists', $renaming->toString())); diff --git a/module/Core/src/Tag/Model/TagRenaming.php b/module/Core/src/Model/Renaming.php similarity index 86% rename from module/Core/src/Tag/Model/TagRenaming.php rename to module/Core/src/Model/Renaming.php index 9c523b8b..e4cee870 100644 --- a/module/Core/src/Tag/Model/TagRenaming.php +++ b/module/Core/src/Model/Renaming.php @@ -2,15 +2,15 @@ declare(strict_types=1); -namespace Shlinkio\Shlink\Core\Tag\Model; +namespace Shlinkio\Shlink\Core\Model; use Shlinkio\Shlink\Core\Exception\ValidationException; use function sprintf; -final class TagRenaming +final readonly class Renaming { - private function __construct(public readonly string $oldName, public readonly string $newName) + private function __construct(public string $oldName, public string $newName) { } diff --git a/module/Core/src/Tag/TagService.php b/module/Core/src/Tag/TagService.php index e3e5b92f..f91c018f 100644 --- a/module/Core/src/Tag/TagService.php +++ b/module/Core/src/Tag/TagService.php @@ -10,8 +10,8 @@ use Shlinkio\Shlink\Common\Paginator\Paginator; use Shlinkio\Shlink\Core\Exception\ForbiddenTagOperationException; use Shlinkio\Shlink\Core\Exception\TagConflictException; use Shlinkio\Shlink\Core\Exception\TagNotFoundException; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Core\Tag\Entity\Tag; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; use Shlinkio\Shlink\Core\Tag\Model\TagsParams; use Shlinkio\Shlink\Core\Tag\Paginator\Adapter\TagsInfoPaginatorAdapter; use Shlinkio\Shlink\Core\Tag\Paginator\Adapter\TagsPaginatorAdapter; @@ -74,7 +74,7 @@ readonly class TagService implements TagServiceInterface /** * @inheritDoc */ - public function renameTag(TagRenaming $renaming, ApiKey|null $apiKey = null): Tag + public function renameTag(Renaming $renaming, ApiKey|null $apiKey = null): Tag { if (ApiKey::isShortUrlRestricted($apiKey)) { throw ForbiddenTagOperationException::forRenaming(); diff --git a/module/Core/src/Tag/TagServiceInterface.php b/module/Core/src/Tag/TagServiceInterface.php index c09370cf..a22e2ec8 100644 --- a/module/Core/src/Tag/TagServiceInterface.php +++ b/module/Core/src/Tag/TagServiceInterface.php @@ -8,9 +8,9 @@ use Shlinkio\Shlink\Common\Paginator\Paginator; use Shlinkio\Shlink\Core\Exception\ForbiddenTagOperationException; use Shlinkio\Shlink\Core\Exception\TagConflictException; use Shlinkio\Shlink\Core\Exception\TagNotFoundException; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Core\Tag\Entity\Tag; use Shlinkio\Shlink\Core\Tag\Model\TagInfo; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; use Shlinkio\Shlink\Core\Tag\Model\TagsParams; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -37,5 +37,5 @@ interface TagServiceInterface * @throws TagConflictException * @throws ForbiddenTagOperationException */ - public function renameTag(TagRenaming $renaming, ApiKey|null $apiKey = null): Tag; + public function renameTag(Renaming $renaming, ApiKey|null $apiKey = null): Tag; } diff --git a/module/Core/test/Exception/TagConflictExceptionTest.php b/module/Core/test/Exception/TagConflictExceptionTest.php index 2f4bd66a..9126e6f3 100644 --- a/module/Core/test/Exception/TagConflictExceptionTest.php +++ b/module/Core/test/Exception/TagConflictExceptionTest.php @@ -7,7 +7,7 @@ namespace ShlinkioTest\Shlink\Core\Exception; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Exception\TagConflictException; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; +use Shlinkio\Shlink\Core\Model\Renaming; use function sprintf; @@ -19,7 +19,7 @@ class TagConflictExceptionTest extends TestCase $oldName = 'foo'; $newName = 'bar'; $expectedMessage = sprintf('You cannot rename tag %s to %s, because it already exists', $oldName, $newName); - $e = TagConflictException::forExistingTag(TagRenaming::fromNames($oldName, $newName)); + $e = TagConflictException::forExistingTag(Renaming::fromNames($oldName, $newName)); self::assertEquals($expectedMessage, $e->getMessage()); self::assertEquals($expectedMessage, $e->getDetail()); diff --git a/module/Core/test/Tag/TagServiceTest.php b/module/Core/test/Tag/TagServiceTest.php index 7e82eb1c..c1fa8ee7 100644 --- a/module/Core/test/Tag/TagServiceTest.php +++ b/module/Core/test/Tag/TagServiceTest.php @@ -13,9 +13,9 @@ use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Core\Exception\ForbiddenTagOperationException; use Shlinkio\Shlink\Core\Exception\TagConflictException; use Shlinkio\Shlink\Core\Exception\TagNotFoundException; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Core\Tag\Entity\Tag; use Shlinkio\Shlink\Core\Tag\Model\TagInfo; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; use Shlinkio\Shlink\Core\Tag\Model\TagsListFiltering; use Shlinkio\Shlink\Core\Tag\Model\TagsParams; use Shlinkio\Shlink\Core\Tag\Repository\TagRepository; @@ -127,7 +127,7 @@ class TagServiceTest extends TestCase $this->repo->expects($this->once())->method('findOneBy')->willReturn(null); $this->expectException(TagNotFoundException::class); - $this->service->renameTag(TagRenaming::fromNames('foo', 'bar'), $apiKey); + $this->service->renameTag(Renaming::fromNames('foo', 'bar'), $apiKey); } #[Test, DataProvider('provideValidRenames')] @@ -139,7 +139,7 @@ class TagServiceTest extends TestCase $this->repo->expects($this->exactly($count > 0 ? 0 : 1))->method('count')->willReturn($count); $this->em->expects($this->once())->method('flush'); - $tag = $this->service->renameTag(TagRenaming::fromNames($oldName, $newName)); + $tag = $this->service->renameTag(Renaming::fromNames($oldName, $newName)); self::assertSame($expected, $tag); self::assertEquals($newName, (string) $tag); @@ -160,7 +160,7 @@ class TagServiceTest extends TestCase $this->expectException(TagConflictException::class); - $this->service->renameTag(TagRenaming::fromNames('foo', 'bar'), $apiKey); + $this->service->renameTag(Renaming::fromNames('foo', 'bar'), $apiKey); } #[Test] @@ -172,7 +172,7 @@ class TagServiceTest extends TestCase $this->expectExceptionMessage('You are not allowed to rename tags'); $this->service->renameTag( - TagRenaming::fromNames('foo', 'bar'), + Renaming::fromNames('foo', 'bar'), ApiKey::fromMeta(ApiKeyMeta::withRoles(RoleDefinition::forAuthoredShortUrls())), ); } diff --git a/module/Rest/src/Action/Tag/UpdateTagAction.php b/module/Rest/src/Action/Tag/UpdateTagAction.php index 016d008b..e1dc1611 100644 --- a/module/Rest/src/Action/Tag/UpdateTagAction.php +++ b/module/Rest/src/Action/Tag/UpdateTagAction.php @@ -7,7 +7,7 @@ namespace Shlinkio\Shlink\Rest\Action\Tag; use Laminas\Diactoros\Response\EmptyResponse; use Psr\Http\Message\ResponseInterface; use Psr\Http\Message\ServerRequestInterface; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Core\Tag\TagServiceInterface; use Shlinkio\Shlink\Rest\Action\AbstractRestAction; use Shlinkio\Shlink\Rest\Middleware\AuthenticationMiddleware; @@ -27,7 +27,7 @@ class UpdateTagAction extends AbstractRestAction $body = $request->getParsedBody(); $apiKey = AuthenticationMiddleware::apiKeyFromRequest($request); - $this->tagService->renameTag(TagRenaming::fromArray($body), $apiKey); + $this->tagService->renameTag(Renaming::fromArray($body), $apiKey); return new EmptyResponse(); } } diff --git a/module/Rest/src/Entity/ApiKey.php b/module/Rest/src/Entity/ApiKey.php index fea06a1d..c9cdd3a6 100644 --- a/module/Rest/src/Entity/ApiKey.php +++ b/module/Rest/src/Entity/ApiKey.php @@ -23,7 +23,8 @@ class ApiKey extends AbstractEntity */ private function __construct( public readonly string $key, - public readonly string $name, + // TODO Use a property hook to allow public read but private write + public string $name, public readonly Chronos|null $expirationDate = null, private bool $enabled = true, private Collection $roles = new ArrayCollection(), diff --git a/module/Rest/src/Service/ApiKeyService.php b/module/Rest/src/Service/ApiKeyService.php index 4b786c6d..38ed004d 100644 --- a/module/Rest/src/Service/ApiKeyService.php +++ b/module/Rest/src/Service/ApiKeyService.php @@ -6,10 +6,13 @@ namespace Shlinkio\Shlink\Rest\Service; use Doctrine\ORM\EntityManagerInterface; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Repository\ApiKeyRepositoryInterface; use Shlinkio\Shlink\Rest\Entity\ApiKey; +use function sprintf; + readonly class ApiKeyService implements ApiKeyServiceInterface { public function __construct(private EntityManagerInterface $em, private ApiKeyRepositoryInterface $repo) @@ -74,11 +77,6 @@ readonly class ApiKeyService implements ApiKeyServiceInterface return $this->repo->findBy($conditions); } - private function findByKey(string $key): ApiKey|null - { - return $this->repo->findOneBy(['key' => ApiKey::hashKey($key)]); - } - /** * @inheritDoc */ @@ -86,4 +84,37 @@ readonly class ApiKeyService implements ApiKeyServiceInterface { return $this->repo->count(['name' => $apiKeyName]) > 0; } + + /** + * @inheritDoc + */ + public function renameApiKey(Renaming $apiKeyRenaming): ApiKey + { + $apiKey = $this->repo->findOneBy(['name' => $apiKeyRenaming->oldName]); + if ($apiKey === null) { + throw new InvalidArgumentException( + sprintf('API key with name "%s" could not be found', $apiKeyRenaming->oldName), + ); + } + + if (! $apiKeyRenaming->nameChanged()) { + return $apiKey; + } + + if ($this->existsWithName($apiKeyRenaming->newName)) { + throw new InvalidArgumentException( + sprintf('Another API key with name "%s" already exists', $apiKeyRenaming->newName), + ); + } + + $apiKey->name = $apiKeyRenaming->newName; + $this->em->flush(); + + return $apiKey; + } + + private function findByKey(string $key): ApiKey|null + { + return $this->repo->findOneBy(['key' => ApiKey::hashKey($key)]); + } } diff --git a/module/Rest/src/Service/ApiKeyServiceInterface.php b/module/Rest/src/Service/ApiKeyServiceInterface.php index 73773fba..4197b3fd 100644 --- a/module/Rest/src/Service/ApiKeyServiceInterface.php +++ b/module/Rest/src/Service/ApiKeyServiceInterface.php @@ -5,6 +5,7 @@ declare(strict_types=1); namespace Shlinkio\Shlink\Rest\Service; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -36,4 +37,9 @@ interface ApiKeyServiceInterface * Check if an API key exists for provided name */ public function existsWithName(string $apiKeyName): bool; + + /** + * @throws InvalidArgumentException If an API key with oldName does not exist, or newName is in use by another one + */ + public function renameApiKey(Renaming $apiKeyRenaming): ApiKey; } diff --git a/module/Rest/test/Action/Tag/UpdateTagActionTest.php b/module/Rest/test/Action/Tag/UpdateTagActionTest.php index 73575baf..f83ee037 100644 --- a/module/Rest/test/Action/Tag/UpdateTagActionTest.php +++ b/module/Rest/test/Action/Tag/UpdateTagActionTest.php @@ -11,8 +11,8 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Psr\Http\Message\ServerRequestInterface; use Shlinkio\Shlink\Core\Exception\ValidationException; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Core\Tag\Entity\Tag; -use Shlinkio\Shlink\Core\Tag\Model\TagRenaming; use Shlinkio\Shlink\Core\Tag\TagServiceInterface; use Shlinkio\Shlink\Rest\Action\Tag\UpdateTagAction; use Shlinkio\Shlink\Rest\Entity\ApiKey; @@ -53,7 +53,7 @@ class UpdateTagActionTest extends TestCase 'newName' => 'bar', ]); $this->tagService->expects($this->once())->method('renameTag')->with( - TagRenaming::fromNames('foo', 'bar'), + Renaming::fromNames('foo', 'bar'), $this->isInstanceOf(ApiKey::class), )->willReturn(new Tag('bar')); diff --git a/module/Rest/test/Service/ApiKeyServiceTest.php b/module/Rest/test/Service/ApiKeyServiceTest.php index e304a651..f439e5f9 100644 --- a/module/Rest/test/Service/ApiKeyServiceTest.php +++ b/module/Rest/test/Service/ApiKeyServiceTest.php @@ -13,6 +13,7 @@ use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; use Shlinkio\Shlink\Common\Exception\InvalidArgumentException; use Shlinkio\Shlink\Core\Domain\Entity\Domain; +use Shlinkio\Shlink\Core\Model\Renaming; use Shlinkio\Shlink\Rest\ApiKey\Model\ApiKeyMeta; use Shlinkio\Shlink\Rest\ApiKey\Model\RoleDefinition; use Shlinkio\Shlink\Rest\ApiKey\Repository\ApiKeyRepositoryInterface; @@ -188,4 +189,66 @@ class ApiKeyServiceTest extends TestCase $this->repo->expects($this->once())->method('count')->with(['name' => $name])->willReturn($count); self::assertEquals($this->service->existsWithName($name), $expected); } + + #[Test] + public function renameApiKeyThrowsExceptionIfApiKeyIsNotFound(): void + { + $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->em->expects($this->never())->method('flush'); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('API key with name "old" could not be found'); + + $this->service->renameApiKey($renaming); + } + + #[Test] + public function renameApiKeyReturnsApiKeyVerbatimIfBothNamesAreEqual(): void + { + $renaming = Renaming::fromNames(oldName: 'same_value', newName: 'same_value'); + $apiKey = ApiKey::create(); + + $this->repo->expects($this->once())->method('findOneBy')->with(['name' => 'same_value'])->willReturn($apiKey); + $this->repo->expects($this->never())->method('count'); + $this->em->expects($this->never())->method('flush'); + + $result = $this->service->renameApiKey($renaming); + + self::assertSame($apiKey, $result); + } + + #[Test] + public function renameApiKeyThrowsExceptionIfNewNameIsInUse(): void + { + $renaming = Renaming::fromNames(oldName: 'old', newName: 'new'); + $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->em->expects($this->never())->method('flush'); + + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage('Another API key with name "new" already exists'); + + $this->service->renameApiKey($renaming); + } + + #[Test] + public function renameApiKeyReturnsApiKeyWithNewName(): void + { + $renaming = Renaming::fromNames(oldName: 'old', newName: 'new'); + $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->em->expects($this->once())->method('flush'); + + $result = $this->service->renameApiKey($renaming); + + self::assertSame($apiKey, $result); + self::assertEquals('new', $apiKey->name); + } }